From 7dd6eb76c96750c258cc38ae26fbd2e10e628014 Mon Sep 17 00:00:00 2001
From: Matteo Pagliazzi <matteopagliazzi@gmail.com>
Date: Sun, 8 Nov 2015 22:48:31 +0100
Subject: [PATCH] add express-validator, add body parser middleware, support
 some more errors in error handler middleware

---
 package.json                                  |  3 +-
 .../v3/unit/middlewares/errorHandler.test.js  | 34 ++++++++++++++++++-
 .../src/middlewares/api-v3/errorHandler.js    | 11 +++++-
 website/src/middlewares/api-v3/index.js       |  9 +++++
 4 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/package.json b/package.json
index 53b5620478..92d3e6bc39 100644
--- a/package.json
+++ b/package.json
@@ -24,6 +24,7 @@
     "estraverse": "^4.1.1",
     "express": "~4.13.3",
     "express-csv": "~0.6.0",
+    "express-validator": "^2.18.0",
     "firebase": "^2.2.9",
     "firebase-token-generator": "^2.0.0",
     "glob": "^4.3.5",
@@ -82,7 +83,7 @@
     "superagent": "~1.4.0",
     "swagger-node-express": "lefnire/swagger-node-express#habitrpg",
     "universal-analytics": "~0.3.2",
-    "validator": "~3.19.0",
+    "validator": "~4.2.1",
     "winston": "~2.0.1"
   },
   "private": true,
diff --git a/test/api/v3/unit/middlewares/errorHandler.test.js b/test/api/v3/unit/middlewares/errorHandler.test.js
index 7ce86127e5..7df6ed8713 100644
--- a/test/api/v3/unit/middlewares/errorHandler.test.js
+++ b/test/api/v3/unit/middlewares/errorHandler.test.js
@@ -20,7 +20,7 @@ describe('errorHandler', () => {
     sandbox.stub(logger, 'error');
   });
 
-  it('sends internal server error if error is not a CustomError', () => {
+  it('sends internal server error if error is not a CustomError and is not identified', () => {
     let error = new Error();
 
     errorHandler(error, req, res, next);
@@ -35,6 +35,38 @@ describe('errorHandler', () => {
     });
   });
 
+  it('identifies errors with statusCode property and format them correctly', () => {
+    let error = new Error('Error message');
+    error.statusCode = 400;
+
+    errorHandler(error, req, res, next);
+
+    expect(res.status).to.be.calledOnce;
+    expect(res.json).to.be.calledOnce;
+
+    expect(res.status).to.be.calledWith(400);
+    expect(res.json).to.be.calledWith({
+      error: 'Error',
+      message: 'Error message',
+    });
+  });
+
+  it('doesn\'t leak info about 500 errors', () => {
+    let error = new Error('Some secret error message');
+    error.statusCode = 500;
+
+    errorHandler(error, req, res, next);
+
+    expect(res.status).to.be.calledOnce;
+    expect(res.json).to.be.calledOnce;
+
+    expect(res.status).to.be.calledWith(500);
+    expect(res.json).to.be.calledWith({
+      error: 'InternalServerError',
+      message: 'Internal server error.',
+    });
+  });
+
   it('sends CustomError', () => {
     let error = new BadRequest();
 
diff --git a/website/src/middlewares/api-v3/errorHandler.js b/website/src/middlewares/api-v3/errorHandler.js
index 97e324aee3..20c7dae30b 100644
--- a/website/src/middlewares/api-v3/errorHandler.js
+++ b/website/src/middlewares/api-v3/errorHandler.js
@@ -23,11 +23,20 @@ export default function errorHandler (err, req, res, next) {
   // If we can't identify it, respond with a generic 500 error
   let responseErr = err instanceof CustomError ? err : null;
 
-  if (!responseErr) {
+  // Handle errors created with 'http-errors' or similar that have a status/statusCode property
+  if (err.statusCode && typeof err.statusCode === 'number') {
+    responseErr = new CustomError();
+    responseErr.httpCode = err.statusCode;
+    responseErr.error = err.name;
+    responseErr.message = err.message;
+  }
+
+  if (!responseErr || responseErr.httpCode >= 500) {
     // Try to identify the error...
     // ...
     // Otherwise create an InternalServerError and use it
     // we don't want to leak anything, just a generic error message
+    // Use it also in case of identified errors but with httpCode === 500
     responseErr = new InternalServerError();
   }
 
diff --git a/website/src/middlewares/api-v3/index.js b/website/src/middlewares/api-v3/index.js
index 460e4c53e4..6ce30edc58 100644
--- a/website/src/middlewares/api-v3/index.js
+++ b/website/src/middlewares/api-v3/index.js
@@ -1,8 +1,17 @@
 // This module is only used to attach middlewares to the express app
 
 import errorHandler from './errorHandler';
+import bodyParser from 'body-parser';
 
 export default function attachMiddlewares (app) {
+
+  // Parse query parameters and json bodies
+  // TODO handle errors
+  app.use(bodyParser.urlencoded(
+    extended: true, // Uses 'qs' library as old connect middleware
+  }));
+  app.use(bodyParser.json());
+
   // Error handler middleware, define as the last one
   app.use(errorHandler);
 }