Merge branch 'api-v3' into v3_make_linter_happy

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,

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();
 

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();
   }
 

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);
 }

website/src/models/user.js

@@ -688,7 +688,6 @@ mongoose.model('User')
   .then((foundMods) => {
     // Using push to maintain the reference to mods
     mods.push(...foundMods);
-  }, (err) => {
-    // @TODO convert this to a catch
+  }, (err) => { // TODO replace with .catch which for some reason was throwing an error
     throw err; // TODO ?
   });