Misc Middleware cleanup (#7526)

* 💄 Combine slashes & uncapitalise middleware

- these bits of middleware belong together
- ideally they should be optimised

* 🎨 Move ghostLocals out of themeHandler

GhostLocals sets several important values which are needed for every part of the application,
admin, api and theme. Therefore, it doesn't make sense for it to be bundled in the themeHandler.

* 🐛 Fix the uncapitalise middleware

- Updated to make correct use of req.baseUrl, req.path, req.url & req.originalUrl
- Updated the tests to actually cover our weird cases

* 🎨 Move ghostVersion logic out of config

* 💄 Group static / asset-related middleware together

* 🔥 Remove /shared/ asset handling

- The 5 files which are located in `/shared/` are all handled by individual calls to `serveSharedFile`
- Therefore this code is redundant

core/server/api/configuration.js

@@ -2,6 +2,7 @@
 // RESTful API for browsing the configuration
 var _                  = require('lodash'),
     config             = require('../config'),
+    ghostVersion       = require('../utils/ghost-version'),
     Promise            = require('bluebird'),
 
     configuration;
@@ -20,7 +21,7 @@ function fetchAvailableTimezones() {
 
 function getAboutConfig() {
     return {
-        version: config.get('ghostVersion'),
+        version: ghostVersion.full,
         environment: process.env.NODE_ENV,
         database: config.get('database').client,
         mail: _.isObject(config.get('mail')) ? config.get('mail').transport : ''

core/server/config/index.js

@@ -2,7 +2,6 @@ var Nconf = require('nconf'),
     nconf = new Nconf.Provider(),
     path = require('path'),
     localUtils = require('./utils'),
-    packageInfo = require('../../../package.json'),
     env = process.env.NODE_ENV || 'development';
 
 /**
@@ -36,9 +35,7 @@ localUtils.makePathsAbsolute.bind(nconf)();
 
 /**
  * values we have to set manual
- * @TODO: ghost-cli?
  */
-nconf.set('ghostVersion', packageInfo.version);
 nconf.set('env', env);
 
 module.exports = nconf;

core/server/data/schema/versioning.js

@@ -2,8 +2,8 @@ var path = require('path'),
     Promise = require('bluebird'),
     db = require('../db'),
     errors = require('../../errors'),
-    config = require('../../config'),
-    i18n = require('../../i18n');
+    i18n = require('../../i18n'),
+    ghostVersion = require('../../utils/ghost-version');
 
 /**
  * Database version has always two digits
@@ -52,7 +52,7 @@ function getDatabaseVersion() {
 }
 
 function getNewestDatabaseVersion() {
-    return config.get('ghostVersion').slice(0, 3);
+    return ghostVersion.safe;
 }
 
 /**

core/server/middleware/ghost-locals.js

@@ -0,0 +1,16 @@
+var ghostVersion = require('../utils/ghost-version');
+
+// ### GhostLocals Middleware
+// Expose the standard locals that every request will need to have available
+module.exports = function ghostLocals(req, res, next) {
+    // Make sure we have a locals value.
+    res.locals = res.locals || {};
+    // The current Ghost version
+    res.locals.version = ghostVersion.full;
+    // The current Ghost version, but only major.minor
+    res.locals.safeVersion = ghostVersion.safe;
+    // relative path from the URL
+    res.locals.relativeUrl = req.path;
+
+    next();
+};

core/server/middleware/index.js

@@ -8,7 +8,6 @@ var debug           = require('debug')('ghost:middleware'),
     multer          = require('multer'),
     tmpdir          = require('os').tmpdir,
     serveStatic     = require('express').static,
-    slashes         = require('connect-slashes'),
     routes          = require('../routes'),
     config          = require('../config'),
     storage         = require('../storage'),
@@ -21,11 +20,12 @@ var debug           = require('debug')('ghost:middleware'),
     checkSSL         = require('./check-ssl'),
     decideIsAdmin    = require('./decide-is-admin'),
     redirectToSetup  = require('./redirect-to-setup'),
+    ghostLocals     = require('./ghost-locals'),
+    prettyURLs      = require('./pretty-urls'),
     serveSharedFile  = require('./serve-shared-file'),
     spamPrevention   = require('./spam-prevention'),
     staticTheme      = require('./static-theme'),
     themeHandler     = require('./theme-handler'),
-    uncapitalise     = require('./uncapitalise'),
     maintenance      = require('./maintenance'),
     errorHandler     = require('./error-handler'),
     versionMatch     = require('./api/version-match'),
@@ -53,8 +53,7 @@ middleware = {
 setupMiddleware = function setupMiddleware(blogApp) {
     debug('Middleware start');
 
-    var corePath = config.get('paths').corePath,
-        adminApp = express(),
+    var adminApp = express(),
         adminHbs = hbs.create();
 
     // ##Configuration
@@ -112,45 +111,49 @@ setupMiddleware = function setupMiddleware(blogApp) {
         }));
     }
 
+    // This sets global res.locals which are needed everywhere
+    blogApp.use(ghostLocals);
+
+    // First determine whether we're serving admin or theme content
+    // @TODO refactor this horror away!
+    blogApp.use(decideIsAdmin);
+
+    // Theme middleware
+    // rightly or wrongly currently comes before theme static assets
+    // @TODO revisit where and when these are needed
+    blogApp.use(themeHandler.updateActiveTheme);
+    blogApp.use(themeHandler.configHbsForContext);
+    debug('Themes done');
+
+    // Static content/assets
     // Favicon
     blogApp.use(serveSharedFile('favicon.ico', 'image/x-icon', utils.ONE_DAY_S));
-
     // Ghost-Url
     blogApp.use(serveSharedFile('shared/ghost-url.js', 'application/javascript', utils.ONE_HOUR_S));
     blogApp.use(serveSharedFile('shared/ghost-url.min.js', 'application/javascript', utils.ONE_HOUR_S));
-
-    // Static assets
-    blogApp.use('/shared', serveStatic(
-        path.join(corePath, '/shared'),
-        {maxAge: utils.ONE_HOUR_MS, fallthrough: false}
-    ));
-
+    // Serve sitemap.xsl file
+    blogApp.use(serveSharedFile('sitemap.xsl', 'text/xsl', utils.ONE_DAY_S));
+    // Serve robots.txt if not found in theme
+    blogApp.use(serveSharedFile('robots.txt', 'text/plain', utils.ONE_HOUR_S));
+    // Serve blog images using the storage adapter
     blogApp.use('/content/images', storage.getStorage().serve());
 
-    debug('Static content done');
-
-    // First determine whether we're serving admin or theme content
-    blogApp.use(decideIsAdmin);
-    blogApp.use(themeHandler.updateActiveTheme);
-    blogApp.use(themeHandler.configHbsForContext);
-
+    // Admin assets
     // Admin only config
     blogApp.use('/ghost/assets', serveStatic(
         config.get('paths').clientAssets,
         {maxAge: utils.ONE_YEAR_MS}
     ));
 
+    // Theme static assets/files
+    blogApp.use(staticTheme());
+    debug('Static content done');
+
     // Force SSL
-    // NOTE: Importantly this is _after_ the check above for admin-theme static resources,
-    //       which do not need HTTPS. In fact, if HTTPS is forced on them, then 404 page might
-    //       not display properly when HTTPS is not available!
+    // must happen AFTER asset loading and BEFORE routing
     blogApp.use(checkSSL);
     adminApp.set('views', config.get('paths').adminViews);
 
-    // Theme only config
-    blogApp.use(staticTheme());
-    debug('Themes done');
-
     // setup middleware for internal apps
     // @TODO: refactor this to be a proper app middleware hook for internal & external apps
     config.get('internalApps').forEach(function (appName) {
@@ -159,24 +162,14 @@ setupMiddleware = function setupMiddleware(blogApp) {
             app.setupMiddleware(blogApp);
         }
     });
+
+    // site map - this should probably be refactored to be an internal app
+    sitemapHandler(blogApp);
     debug('Internal apps done');
 
-    // Serve sitemap.xsl file
-    blogApp.use(serveSharedFile('sitemap.xsl', 'text/xsl', utils.ONE_DAY_S));
-
-    // Serve robots.txt if not found in theme
-    blogApp.use(serveSharedFile('robots.txt', 'text/plain', utils.ONE_HOUR_S));
-
-    // site map
-    sitemapHandler(blogApp);
-
-    // Add in all trailing slashes
-    blogApp.use(slashes(true, {
-        headers: {
-            'Cache-Control': 'public, max-age=' + utils.ONE_YEAR_S
-        }
-    }));
-    blogApp.use(uncapitalise);
+    // Add in all trailing slashes & remove uppercase
+    // must happen AFTER asset loading and BEFORE routing
+    blogApp.use(prettyURLs);
 
     // Body parsing
     blogApp.use(bodyParser.json({limit: '1mb'}));
@@ -190,9 +183,6 @@ setupMiddleware = function setupMiddleware(blogApp) {
     // API shouldn't be cached
     blogApp.use(routes.apiBaseUri, cacheControl('private'));
 
-    // local data
-    blogApp.use(themeHandler.ghostLocals);
-
     debug('General middleware done');
 
     // ### Routing

core/server/middleware/pretty-urls.js

@@ -0,0 +1,19 @@
+// Pretty URL redirects
+//
+// These are two pieces of middleware that handle ensuring that
+// URLs get formatted correctly.
+// Slashes ensures that we get trailing slashes
+// Uncapitalise changes case to lowercase
+// @TODO optimise this to reduce the number of redirects required to get to a pretty URL
+// @TODO move this to being used by routers?
+var slashes = require('connect-slashes'),
+    utils = require('../utils');
+
+module.exports = [
+    slashes(true, {
+        headers: {
+            'Cache-Control': 'public, max-age=' + utils.ONE_YEAR_S
+        }
+    }),
+    require('./uncapitalise')
+];

core/server/middleware/theme-handler.js

@@ -10,20 +10,6 @@ var _      = require('lodash'),
     themeHandler;
 
 themeHandler = {
-    // ### GhostLocals Middleware
-    // Expose the standard locals that every external page should have available,
-    // separating between the theme and the admin
-    ghostLocals: function ghostLocals(req, res, next) {
-        // Make sure we have a locals value.
-        res.locals = res.locals || {};
-        res.locals.version = config.get('ghostVersion');
-        res.locals.safeVersion = config.get('ghostVersion').match(/^(\d+\.)?(\d+)/)[0];
-        // relative path from the URL
-        res.locals.relativeUrl = req.path;
-
-        next();
-    },
-
     // ### configHbsForContext Middleware
     // Setup handlebars for the current context (admin or theme)
     configHbsForContext: function configHbsForContext(req, res, next) {

core/server/middleware/uncapitalise.js

@@ -5,15 +5,20 @@
 // App: Admin|Blog|API
 //
 // Detect upper case in req.path.
+//
+//  Example req:
+//  req.originalUrl = /blog/ghost/signin/?asdAD=asdAS
+//  req.url = /ghost/signin/?asdAD=asdAS
+//  req.baseUrl = /blog
+//  req.path =  /ghost/signin/
 
 var utils = require('../utils'),
     uncapitalise;
 
 uncapitalise = function uncapitalise(req, res, next) {
-    /*jslint unparam:true*/
-    var pathToTest = req.path,
-        isSignupOrReset = req.path.match(/(\/ghost\/(signup|reset)\/)/i),
-        isAPI = req.path.match(/(\/ghost\/api\/v[\d\.]+\/.*?\/)/i),
+    var pathToTest = (req.baseUrl ? req.baseUrl : '') + req.path,
+        isSignupOrReset = pathToTest.match(/^(.*\/ghost\/(signup|reset)\/)/i),
+        isAPI = pathToTest.match(/^(.*\/ghost\/api\/v[\d\.]+\/.*?\/)/i),
         redirectPath;
 
     if (isSignupOrReset) {
@@ -30,10 +35,8 @@ uncapitalise = function uncapitalise(req, res, next) {
      * That encoding isn't useful here, as it triggers an extra uncapitalise redirect, so we decode the path first
      */
     if (/[A-Z]/.test(decodeURIComponent(pathToTest))) {
-        // Adding baseUrl ensures subdirectories are kept
         redirectPath = (
-          (req.baseUrl ? req.baseUrl : '') +
-          utils.removeOpenRedirectFromUrl(req.url.replace(pathToTest, pathToTest.toLowerCase()))
+          utils.removeOpenRedirectFromUrl((req.originalUrl || req.url).replace(pathToTest, pathToTest.toLowerCase()))
         );
 
         res.set('Cache-Control', 'public, max-age=' + utils.ONE_YEAR_S);

core/server/update-check.js

@@ -30,13 +30,13 @@ var crypto   = require('crypto'),
     url      = require('url'),
     api      = require('./api'),
     config   = require('./config'),
-    logging   = require('./logging'),
+    logging  = require('./logging'),
     errors   = require('./errors'),
     i18n     = require('./i18n'),
+    currentVersion = require('./utils/ghost-version').full,
     internal = {context: {internal: true}},
     allowedCheckEnvironments = ['development', 'production'],
-    checkEndpoint = 'updates.ghost.org',
-    currentVersion = config.get('ghostVersion');
+    checkEndpoint = 'updates.ghost.org';
 
 function updateCheckError(err) {
     api.settings.edit(

core/server/utils/ghost-version.js

@@ -0,0 +1,8 @@
+var packageInfo = require('../../../package.json'),
+    version = packageInfo.version;
+
+module.exports = {
+    full: version,
+    safe: version.match(/^(\d+\.)?(\d+)/)[0]
+};
+

core/test/unit/middleware/ghost-locals_spec.js

@@ -0,0 +1,27 @@
+var sinon        = require('sinon'),
+    should       = require('should'),
+    ghostLocals  = require('../../../server/middleware/ghost-locals');
+
+describe('Theme Handler', function () {
+    var req, res, next;
+
+    beforeEach(function () {
+        req = sinon.spy();
+        res = sinon.spy();
+        next = sinon.spy();
+    });
+
+    describe('ghostLocals', function () {
+        it('sets all locals', function () {
+            req.path = '/awesome-post';
+
+            ghostLocals(req, res, next);
+
+            res.locals.should.be.an.Object();
+            should.exist(res.locals.version);
+            should.exist(res.locals.safeVersion);
+            res.locals.relativeUrl.should.equal(req.path);
+            next.called.should.be.true();
+        });
+    });
+});

core/test/unit/middleware/theme-handler_spec.js

@@ -26,20 +26,6 @@ describe('Theme Handler', function () {
         configUtils.restore();
     });
 
-    describe('ghostLocals', function () {
-        it('sets all locals', function () {
-            req.path = '/awesome-post';
-
-            themeHandler.ghostLocals(req, res, next);
-
-            res.locals.should.be.an.Object();
-            should.exist(res.locals.version);
-            should.exist(res.locals.safeVersion);
-            res.locals.relativeUrl.should.equal(req.path);
-            next.called.should.be.true();
-        });
-    });
-
     describe('activateTheme', function () {
         it('should activate new theme with partials', function () {
             var fsStub = sandbox.stub(fs, 'stat', function (path, cb) {

core/test/unit/middleware/uncapitalise_spec.js

@@ -1,81 +1,199 @@
 var sinon        = require('sinon'),
     should       = require('should'),
-    uncapitalise = require('../../../server/middleware/uncapitalise');
+    uncapitalise = require('../../../server/middleware/uncapitalise'),
+
+    sandbox = sinon.sandbox.create();
 
 should.equal(true, true);
 
+// NOTE: all urls will have had trailing slashes added before uncapitalise is called
+
 describe('Middleware: uncapitalise', function () {
-    var sandbox,
-        res,
-        req,
-        next;
+    var res, req, next;
 
     beforeEach(function () {
-        sandbox = sinon.sandbox.create();
-
-        res = sinon.spy();
-        req = sinon.spy();
-        next = sinon.spy();
+        res = {
+            redirect: sandbox.spy(),
+            set: sandbox.spy()
+        };
+        req = {};
+        next = sandbox.spy();
     });
 
     afterEach(function () {
         sandbox.restore();
     });
 
-    describe('A signup or reset request', function () {
-        it('does nothing if there are no capital letters', function (done) {
-            req.path = '/ghost/signup';
+    describe('Signup or reset request', function () {
+        it('[signup] does nothing if there are no capitals in req.path', function (done) {
+            req.path = '/ghost/signup/';
             uncapitalise(req, res, next);
 
             next.calledOnce.should.be.true();
             done();
         });
 
-        it('redirects to the lower case slug if there are capital letters', function (done) {
-            req.path = '/ghost/SignUP';
-            req.url = 'http://localhost' + req.path;
-            res = {
-                redirect: sinon.spy(),
-                set: sinon.spy()
-            };
+        it('[signup] does nothing if there are no capitals in the baseUrl', function (done) {
+            req.baseUrl = '/ghost/signup/';
+            req.path = '';
+            uncapitalise(req, res, next);
+
+            next.calledOnce.should.be.true();
+            done();
+        });
+
+        it('[signup] does nothing if there are no capitals except in a token', function (done) {
+            req.baseUrl = '/blog';
+            req.path = '/ghost/signup/XEB123';
+
+            uncapitalise(req, res, next);
+
+            next.calledOnce.should.be.true();
+            done();
+        });
+
+        it('[reset] does nothing if there are no capitals except in a token', function (done) {
+            req.baseUrl = '/blog';
+            req.path = '/ghost/reset/NCR3NjY4NzI1ODI1OHzlcmlzZHNAZ51haWwuY29tfEpWeGxRWHUzZ3Y0cEpQRkNYYzQvbUZyc2xFSVozU3lIZHZWeFJLRml6cY54';
+            uncapitalise(req, res, next);
+
+            next.calledOnce.should.be.true();
+            done();
+        });
+
+        it('[signup] redirects if there are capitals in req.path', function (done) {
+            req.path = '/ghost/SignUP/';
+            req.url = req.path;
 
             uncapitalise(req, res, next);
 
             next.called.should.be.false();
             res.redirect.calledOnce.should.be.true();
-            res.redirect.calledWith(301, 'http://localhost/ghost/signup').should.be.true();
+            res.redirect.calledWith(301, '/ghost/signup/').should.be.true();
+            done();
+        });
+
+        it('[signup] redirects if there are capitals in req.baseUrl', function (done) {
+            req.baseUrl = '/ghost/SignUP/';
+            req.path = '';
+            req.url = req.path;
+            req.originalUrl = req.baseUrl + req.path;
+
+            uncapitalise(req, res, next);
+
+            next.called.should.be.false();
+            res.redirect.calledOnce.should.be.true();
+            res.redirect.calledWith(301, '/ghost/signup/').should.be.true();
+            done();
+        });
+
+        it('[signup] redirects correctly if there are capitals in req.path and req.baseUrl', function (done) {
+            req.baseUrl = '/Blog';
+            req.path = '/ghosT/signUp/';
+            req.url = req.path;
+            req.originalUrl = req.baseUrl + req.path;
+
+            uncapitalise(req, res, next);
+
+            next.called.should.be.false();
+            res.redirect.calledOnce.should.be.true();
+            res.redirect.calledWith(301, '/blog/ghost/signup/').should.be.true();
+            done();
+        });
+
+        it('[signup] redirects correctly with capitals in req.path if there is a token', function (done) {
+            req.path = '/ghosT/sigNup/XEB123';
+            req.url = req.path;
+
+            uncapitalise(req, res, next);
+
+            next.called.should.be.false();
+            res.redirect.calledOnce.should.be.true();
+            res.redirect.calledWith(301, '/ghost/signup/XEB123').should.be.true();
+            done();
+        });
+
+        it('[reset] redirects correctly with capitals in req.path & req.baseUrl if there is a token', function (done) {
+            req.baseUrl = '/Blog';
+            req.path = '/Ghost/Reset/NCR3NjY4NzI1ODI1OHzlcmlzZHNAZ51haWwuY29tfEpWeGxRWHUzZ3Y0cEpQRkNYYzQvbUZyc2xFSVozU3lIZHZWeFJLRml6cY54';
+            req.url = req.path;
+            req.originalUrl = req.baseUrl + req.path;
+
+            uncapitalise(req, res, next);
+
+            next.called.should.be.false();
+            res.redirect.calledOnce.should.be.true();
+            res.redirect.calledWith(301, '/blog/ghost/reset/NCR3NjY4NzI1ODI1OHzlcmlzZHNAZ51haWwuY29tfEpWeGxRWHUzZ3Y0cEpQRkNYYzQvbUZyc2xFSVozU3lIZHZWeFJLRml6cY54').should.be.true();
             done();
         });
     });
 
     describe('An API request', function () {
-        it('does nothing if there are no capital letters', function (done) {
-            req.path = '/ghost/api/v0.1';
+        it('does nothing if there are no capitals', function (done) {
+            req.path = '/ghost/api/v0.1/endpoint/';
             uncapitalise(req, res, next);
 
             next.calledOnce.should.be.true();
             done();
         });
 
-        it('redirects to the lower case slug if there are capital letters', function (done) {
-            req.path = '/ghost/api/v0.1/ASDfJ';
-            req.url = 'http://localhost' + req.path;
-            res = {
-                redirect: sinon.spy(),
-                set: sinon.spy()
-            };
+        it('redirects to the lower case slug if there are capitals', function (done) {
+            req.path = '/ghost/api/v0.1/ASDfJ/';
+            req.url = req.path;
 
             uncapitalise(req, res, next);
 
             next.called.should.be.false();
             res.redirect.calledOnce.should.be.true();
-            res.redirect.calledWith(301, 'http://localhost/ghost/api/v0.1/asdfj').should.be.true();
+            res.redirect.calledWith(301, '/ghost/api/v0.1/asdfj/').should.be.true();
+            done();
+        });
+
+        it('redirects to the lower case slug if there are capitals in req.baseUrl', function (done) {
+            req.baseUrl = '/Blog';
+            req.path = '/ghost/api/v0.1/ASDfJ/';
+            req.url = req.path;
+            req.originalUrl = req.baseUrl + req.path;
+
+            uncapitalise(req, res, next);
+
+            next.called.should.be.false();
+            res.redirect.calledOnce.should.be.true();
+            res.redirect.calledWith(301, '/blog/ghost/api/v0.1/asdfj/').should.be.true();
+            done();
+        });
+
+        it('does not convert any capitals after the endpoint', function (done) {
+            var query = '?filter=mAgic';
+            req.path = '/Ghost/API/v0.1/settings/isPrivate/';
+            req.url = req.path + query;
+
+            uncapitalise(req, res, next);
+
+            next.called.should.be.false();
+            res.redirect.calledOnce.should.be.true();
+            res.redirect.calledWith(301, '/ghost/api/v0.1/settings/isPrivate/?filter=mAgic').should.be.true();
+            done();
+        });
+
+        it('does not convert any capitals after the endpoint with baseUrl', function (done) {
+            var query = '?filter=mAgic';
+            req.baseUrl = '/Blog';
+            req.path = '/ghost/api/v0.1/mail/test@example.COM/';
+            req.url = req.path + query;
+            req.originalUrl = req.baseUrl + req.path + query;
+
+            uncapitalise(req, res, next);
+
+            next.called.should.be.false();
+            res.redirect.calledOnce.should.be.true();
+            res.redirect.calledWith(301, '/blog/ghost/api/v0.1/mail/test@example.COM/?filter=mAgic').should.be.true();
             done();
         });
     });
 
     describe('Any other request', function () {
-        it('does nothing if there are no capital letters', function (done) {
+        it('does nothing if there are no capitals', function (done) {
             req.path = '/this-is-my-blog-post';
             uncapitalise(req, res, next);
 
@@ -83,19 +201,15 @@ describe('Middleware: uncapitalise', function () {
             done();
         });
 
-        it('redirects to the lower case slug if there are capital letters', function (done) {
+        it('redirects to the lower case slug if there are capitals', function (done) {
             req.path = '/THis-iS-my-BLOg-poSt';
-            req.url = 'http://localhost' + req.path;
-            res = {
-                redirect: sinon.spy(),
-                set: sinon.spy()
-            };
+            req.url = req.path;
 
             uncapitalise(req, res, next);
 
             next.called.should.be.false();
             res.redirect.calledOnce.should.be.true();
-            res.redirect.calledWith(301, 'http://localhost/this-is-my-blog-post').should.be.true();
+            res.redirect.calledWith(301, '/this-is-my-blog-post').should.be.true();
             done();
         });
     });