Improve API error handling

close #2757, refs #5286 - moves error formatting from api/index into errors lib - moves error handling from api/index into its own middleware - adds extra middleware for method not allowed which captures all unsupported routes
TryGhost · Jun 15, 2015 · 254e0f0 · 254e0f0
1 parent b15f1da
commit 254e0f0
Show file tree

Hide file tree

Showing 10 changed files with 141 additions and 69 deletions.
diff --git a/core/server/api/index.js b/core/server/api/index.js
@@ -21,10 +21,8 @@ var _              = require('lodash'),
     authentication = require('./authentication'),
     uploads        = require('./upload'),
     dataExport     = require('../data/export'),
-    errors         = require('../errors'),
 
     http,
-    formatHttpErrors,
     addHeaders,
     cacheInvalidationHeader,
     locationHeader,
@@ -144,37 +142,6 @@ contentDispositionHeader = function contentDispositionHeader() {
     });
 };
 
-/**
- * ### Format HTTP Errors
- * Converts the error response from the API into a format which can be returned over HTTP
- *
- * @private
- * @param {Array} error
- * @return {{errors: Array, statusCode: number}}
- */
-formatHttpErrors = function formatHttpErrors(error) {
-    var statusCode = 500,
-        errors = [];
-
-    if (!_.isArray(error)) {
-        error = [].concat(error);
-    }
-
-    _.each(error, function each(errorItem) {
-        var errorContent = {};
-
-        // TODO: add logic to set the correct status code
-        statusCode = errorItem.code || 500;
-
-        errorContent.message = _.isString(errorItem) ? errorItem :
-            (_.isObject(errorItem) ? errorItem.message : 'Unknown API Error');
-        errorContent.errorType = errorItem.errorType || 'InternalServerError';
-        errors.push(errorContent);
-    });
-
-    return {errors: errors, statusCode: statusCode};
-};
-
 addHeaders = function addHeaders(apiMethod, req, res, result) {
     var cacheInvalidation,
         location,
@@ -221,7 +188,7 @@ addHeaders = function addHeaders(apiMethod, req, res, result) {
  * @return {Function} middleware format function to be called by the route when a matching request is made
  */
 http = function http(apiMethod) {
-    return function apiHandler(req, res) {
+    return function apiHandler(req, res, next) {
         // We define 2 properties for using as arguments in API calls:
         var object = req.body,
             options = _.extend({}, req.files, req.query, req.params, {
@@ -243,11 +210,9 @@ http = function http(apiMethod) {
         }).then(function then(response) {
             // Send a properly formatting HTTP response containing the data with correct headers
             res.json(response || {});
-        }).catch(function onError(error) {
-            errors.logError(error);
-            var httpErrors = formatHttpErrors(error);
-            // Send a properly formatted HTTP response containing the errors
-            res.status(httpErrors.statusCode).json({errors: httpErrors.errors});
+        }).catch(function onAPIError(error) {
+            // To be handled by the API middleware
+            next(error);
         });
     };
 };

diff --git a/core/server/errors/index.js b/core/server/errors/index.js
@@ -175,6 +175,37 @@ errors = {
         };
     },
 
+    /**
+     * ### Format HTTP Errors
+     * Converts the error response from the API into a format which can be returned over HTTP
+     *
+     * @private
+     * @param {Array} error
+     * @return {{errors: Array, statusCode: number}}
+     */
+    formatHttpErrors: function formatHttpErrors(error) {
+        var statusCode = 500,
+            errors = [];
+
+        if (!_.isArray(error)) {
+            error = [].concat(error);
+        }
+
+        _.each(error, function each(errorItem) {
+            var errorContent = {};
+
+            // TODO: add logic to set the correct status code
+            statusCode = errorItem.code || 500;
+
+            errorContent.message = _.isString(errorItem) ? errorItem :
+                (_.isObject(errorItem) ? errorItem.message : 'Unknown API Error');
+            errorContent.errorType = errorItem.errorType || 'InternalServerError';
+            errors.push(errorContent);
+        });
+
+        return {errors: errors, statusCode: statusCode};
+    },
+
     handleAPIError: function (error, permsMessage) {
         if (!error) {
             return this.rejectError(
@@ -342,6 +373,7 @@ _.each([
     'logErrorAndExit',
     'logErrorWithRedirect',
     'handleAPIError',
+    'formatHttpErrors',
     'renderErrorPage',
     'error404',
     'error500'

diff --git a/core/server/errors/method-not-allowed-error.js b/core/server/errors/method-not-allowed-error.js
@@ -5,7 +5,7 @@ function MethodNotAllowedError(message) {
     this.message = message;
     this.stack = new Error().stack;
     this.code = 405;
-    this.type = this.name;
+    this.errorType = this.name;
 }
 
 MethodNotAllowedError.prototype = Object.create(Error.prototype);

diff --git a/core/server/middleware/api-error-handlers.js b/core/server/middleware/api-error-handlers.js
@@ -0,0 +1,13 @@
+var errors = require('../errors');
+
+module.exports.methodNotAllowed = function methodNotAllowed(req, res, next) {
+    next(new errors.MethodNotAllowedError('Unknown method: ' + req.path));
+};
+
+module.exports.errorHandler = function errorHandler(err, req, res, next) {
+    /*jshint unused:false */
+    var httpErrors = errors.formatHttpErrors(err);
+    errors.logError(err);
+    // Send a properly formatted HTTP response containing the errors
+    res.status(httpErrors.statusCode).json({errors: httpErrors.errors});
+};
diff --git a/core/server/middleware/index.js b/core/server/middleware/index.js
@@ -217,16 +217,15 @@ function serveSharedFile(file, type, maxAge) {
 setupMiddleware = function setupMiddleware(blogAppInstance, adminApp) {
     var logging = config.logging,
         corePath = config.paths.corePath,
-        oauthServer = oauth2orize.createServer(),
-        apiRouter;
+        oauthServer = oauth2orize.createServer();
 
     // silence JSHint without disabling unused check for the whole file
     authStrategies = authStrategies;
 
     // Cache express server instance
     blogApp = blogAppInstance;
     middleware.cacheBlogApp(blogApp);
-    middleware.cacheOauthServer(oauthServer);
+    middleware.api.cacheOauthServer(oauthServer);
     oauth.init(oauthServer, middleware.spamPrevention.resetCounter);
 
     // Make sure 'req.secure' is valid for proxied requests
@@ -311,19 +310,7 @@ setupMiddleware = function setupMiddleware(blogAppInstance, adminApp) {
 
     // ### Routing
     // Set up API routes
-    apiRouter = routes.api(middleware);
-    blogApp.use(routes.apiBaseUri, apiRouter);
-    // ### Invalid method call on valid route
-    apiRouter.use(function (req, res, next) {
-        apiRouter.stack.forEach(function (item) {
-            if (item.regexp.test(req.path) && item.route !== undefined) {
-                return next(new errors.MethodNotAllowedError('Method not allowed'));
-            }
-        });
-
-        // Didn't match any path -> 404
-        res.status(404).json({errors: {type: 'NotFoundError', message: 'Unknown API endpoint.'}});
-    });
+    blogApp.use(routes.apiBaseUri, routes.api(middleware));
 
     // Mount admin express app to /ghost and set up routes
     adminApp.use(middleware.redirectToSetup);

diff --git a/core/server/middleware/middleware.js b/core/server/middleware/middleware.js
@@ -18,8 +18,9 @@ var _           = require('lodash'),
 
     busboy       = require('./ghost-busboy'),
     cacheControl = require('./cache-control'),
-    spamPrevention  = require('./spam-prevention'),
-    clientAuth  = require('./client-auth'),
+    spamPrevention   = require('./spam-prevention'),
+    clientAuth       = require('./client-auth'),
+    apiErrorHandlers = require('./api-error-handlers'),
 
     middleware,
     blogApp;
@@ -306,10 +307,14 @@ middleware = {
 module.exports = middleware;
 module.exports.cacheBlogApp = cacheBlogApp;
 
-module.exports.addClientSecret = clientAuth.addClientSecret;
-module.exports.cacheOauthServer = clientAuth.cacheOauthServer;
-module.exports.authenticateClient = clientAuth.authenticateClient;
-module.exports.generateAccessToken = clientAuth.generateAccessToken;
+module.exports.api = {
+    addClientSecret: clientAuth.addClientSecret,
+    cacheOauthServer: clientAuth.cacheOauthServer,
+    authenticateClient: clientAuth.authenticateClient,
+    generateAccessToken: clientAuth.generateAccessToken,
+    methodNotAllowed: apiErrorHandlers.methodNotAllowed,
+    errorHandler: apiErrorHandlers.errorHandler
+};
 
 // SSL helper functions are exported primarily for unity testing.
 module.exports.isSSLrequired = isSSLrequired;

diff --git a/core/server/routes/api.js b/core/server/routes/api.js
@@ -3,7 +3,7 @@ var express     = require('express'),
     api         = require('../api'),
     apiRoutes;
 
-apiRoutes = function (middleware) {
+apiRoutes = function apiRoutes(middleware) {
     var router = express.Router();
     // alias delete with del
     router.del = router.delete;
@@ -79,15 +79,20 @@ apiRoutes = function (middleware) {
     router.get('/authentication/setup', api.http(api.authentication.isSetup));
     router.post('/authentication/token',
         middleware.spamPrevention.signin,
-        middleware.addClientSecret,
-        middleware.authenticateClient,
-        middleware.generateAccessToken
+        middleware.api.addClientSecret,
+        middleware.api.authenticateClient,
+        middleware.api.generateAccessToken
     );
     router.post('/authentication/revoke', api.http(api.authentication.revoke));
 
     // ## Uploads
     router.post('/uploads', middleware.busboy, api.http(api.uploads.add));
 
+    // API Router middleware
+    router.use(middleware.api.methodNotAllowed);
+
+    router.use(middleware.api.errorHandler);
+
     return router;
 };
 

diff --git a/core/test/unit/middleware/api-error-handlers_spec.js b/core/test/unit/middleware/api-error-handlers_spec.js
@@ -0,0 +1,61 @@
+/*globals describe, beforeEach, afterEach, it*/
+/*jshint expr:true*/
+var should          = require('should'),
+    sinon           = require('sinon'),
+
+    middleware      = require('../../../server/middleware').middleware,
+    errors          = require('../../../server/errors');
+
+// To stop jshint complaining
+should.equal(true, true);
+
+describe('Middleware: API Error Handlers', function () {
+    var sandbox, req, res, next;
+
+    beforeEach(function () {
+        sandbox = sinon.sandbox.create();
+        req = {};
+        res = {};
+        res.json = sandbox.spy();
+        res.status = sandbox.stub().returns(res);
+        next = sandbox.spy();
+    });
+
+    afterEach(function () {
+        sandbox.restore();
+    });
+
+    describe('methodNotAllowed', function () {
+        it('calls next with an error', function () {
+            req.path = 'test';
+
+            middleware.api.methodNotAllowed(req, res, next);
+
+            next.calledOnce.should.be.true;
+            next.firstCall.args[0].code.should.equal(405);
+            next.firstCall.args[0].errorType.should.equal('MethodNotAllowedError');
+            next.firstCall.args[0].message.should.match(/test$/);
+        });
+    });
+
+    describe('errorHandler', function () {
+        it('sends a JSON error response', function () {
+            errors.logError = sandbox.spy(errors, 'logError');
+            errors.formatHttpErrors = sandbox.spy(errors, 'formatHttpErrors');
+
+            var msg = 'Something got lost',
+                err = new errors.NotFoundError(msg);
+
+            middleware.api.errorHandler(err, req, res, next);
+
+            next.called.should.be.false;
+            errors.logError.calledOnce.should.be.true;
+            errors.formatHttpErrors.calledOnce.should.be.true;
+
+            res.status.calledWith(404).should.be.true;
+            res.json.calledOnce.should.be.true;
+            res.json.firstCall.args[0].errors[0].message.should.eql(msg);
+            res.json.firstCall.args[0].errors[0].errorType.should.eql('NotFoundError');
+        });
+    });
+});
diff --git a/core/test/unit/middleware/clientAuth_spec.js → .../test/unit/middleware/client-auth_spec.js b/core/test/unit/middleware/clientAuth_spec.js → .../test/unit/middleware/client-auth_spec.js
@@ -20,7 +20,7 @@ describe('Middleware: Client Auth', function () {
 
             req.body = requestBody;
 
-            middleware.addClientSecret(req, res, next);
+            middleware.api.addClientSecret(req, res, next);
 
             next.called.should.be.true;
             should(req.body).have.property('client_secret');
@@ -34,7 +34,7 @@ describe('Middleware: Client Auth', function () {
 
             req.body = requestBody;
 
-            middleware.addClientSecret(req, res, next);
+            middleware.api.addClientSecret(req, res, next);
 
             next.called.should.be.true;
             should(req.body).have.property('client_secret');

diff --git a/core/test/unit/middleware/decide-is-admin_spec.js b/core/test/unit/middleware/decide-is-admin_spec.js
@@ -1,8 +1,12 @@
 /*globals describe, beforeEach, afterEach, it*/
 /*jshint expr:true*/
-var sinon           = require('sinon'),
+var should          = require('should'),
+    sinon           = require('sinon'),
     decideIsAdmin   = require('../../../server/middleware/decide-is-admin');
 
+// To stop jshint complaining
+should.equal(true, true);
+
 describe('Middleware: decideIsAdmin', function () {
     var sandbox,
         res,