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);
         }
     });
-    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
+    // site map - this should probably be refactored to be an internal app
     sitemapHandler(blogApp);
+    debug('Internal apps done');
 
-    // 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) {