From 19dde146c133729ee9b6cae504e9802fbe257762 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Wed, 22 Apr 2020 19:28:38 +0100 Subject: [PATCH] Renamed redirect functions for clarity - we had urlRedirects, urlRedirects.adminRedirect and adminRedirects - all do kinda similar things, but for different contexts so for now I've done a minimal renaming for clarity - and updated some comments!! - also removed totally unnecessary if res.isAdmin clause, as we don't use that, and it was never true --- core/server/web/admin/app.js | 2 +- .../server/web/api/canary/admin/middleware.js | 6 +++--- .../web/api/canary/content/middleware.js | 2 +- core/server/web/api/v2/admin/middleware.js | 6 +++--- core/server/web/api/v2/content/middleware.js | 2 +- .../web/shared/middlewares/url-redirects.js | 12 ++++-------- core/server/web/site/app.js | 6 +++--- core/server/web/site/middleware/index.js | 2 +- ...edirects.js => redirect-ghost-to-admin.js} | 4 +--- .../unit/web/middleware/url-redirects_spec.js | 19 +++++-------------- 10 files changed, 23 insertions(+), 38 deletions(-) rename core/server/web/site/middleware/{admin-redirects.js => redirect-ghost-to-admin.js} (84%) diff --git a/core/server/web/admin/app.js b/core/server/web/admin/app.js index c5b0ce18e8..a69724aca0 100644 --- a/core/server/web/admin/app.js +++ b/core/server/web/admin/app.js @@ -37,7 +37,7 @@ module.exports = function setupAdminApp() { // Force SSL if required // must happen AFTER asset loading and BEFORE routing - adminApp.use(shared.middlewares.urlRedirects.adminRedirect); + adminApp.use(shared.middlewares.urlRedirects.adminSSLAndHostRedirect); // Add in all trailing slashes & remove uppercase // must happen AFTER asset loading and BEFORE routing diff --git a/core/server/web/api/canary/admin/middleware.js b/core/server/web/api/canary/admin/middleware.js index 0532bab187..94ad557172 100644 --- a/core/server/web/api/canary/admin/middleware.js +++ b/core/server/web/api/canary/admin/middleware.js @@ -55,7 +55,7 @@ module.exports.authAdminApi = [ auth.authorize.authorizeAdminApi, apiMw.updateUserLastSeen, apiMw.cors, - shared.middlewares.urlRedirects.adminRedirect, + shared.middlewares.urlRedirects.adminSSLAndHostRedirect, shared.middlewares.prettyUrls, notImplemented ]; @@ -69,7 +69,7 @@ module.exports.authAdminApiWithUrl = [ auth.authorize.authorizeAdminApi, apiMw.updateUserLastSeen, apiMw.cors, - shared.middlewares.urlRedirects.adminRedirect, + shared.middlewares.urlRedirects.adminSSLAndHostRedirect, shared.middlewares.prettyUrls, notImplemented ]; @@ -79,7 +79,7 @@ module.exports.authAdminApiWithUrl = [ */ module.exports.publicAdminApi = [ apiMw.cors, - shared.middlewares.urlRedirects.adminRedirect, + shared.middlewares.urlRedirects.adminSSLAndHostRedirect, shared.middlewares.prettyUrls, notImplemented ]; diff --git a/core/server/web/api/canary/content/middleware.js b/core/server/web/api/canary/content/middleware.js index 434fa1aa86..e3f837b6bd 100644 --- a/core/server/web/api/canary/content/middleware.js +++ b/core/server/web/api/canary/content/middleware.js @@ -18,6 +18,6 @@ module.exports.authenticatePublic = [ auth.authenticate.authenticateContentApi, auth.authorize.authorizeContentApi, cors(), - shared.middlewares.urlRedirects.adminRedirect, + shared.middlewares.urlRedirects.adminSSLAndHostRedirect, shared.middlewares.prettyUrls ]; diff --git a/core/server/web/api/v2/admin/middleware.js b/core/server/web/api/v2/admin/middleware.js index 1e84d0f972..a9ebb7294f 100644 --- a/core/server/web/api/v2/admin/middleware.js +++ b/core/server/web/api/v2/admin/middleware.js @@ -52,7 +52,7 @@ module.exports.authAdminApi = [ auth.authorize.authorizeAdminApi, apiMw.updateUserLastSeen, apiMw.cors, - shared.middlewares.urlRedirects.adminRedirect, + shared.middlewares.urlRedirects.adminSSLAndHostRedirect, shared.middlewares.prettyUrls, notImplemented ]; @@ -66,7 +66,7 @@ module.exports.authAdminApiWithUrl = [ auth.authorize.authorizeAdminApi, apiMw.updateUserLastSeen, apiMw.cors, - shared.middlewares.urlRedirects.adminRedirect, + shared.middlewares.urlRedirects.adminSSLAndHostRedirect, shared.middlewares.prettyUrls, notImplemented ]; @@ -76,7 +76,7 @@ module.exports.authAdminApiWithUrl = [ */ module.exports.publicAdminApi = [ apiMw.cors, - shared.middlewares.urlRedirects.adminRedirect, + shared.middlewares.urlRedirects.adminSSLAndHostRedirect, shared.middlewares.prettyUrls, notImplemented ]; diff --git a/core/server/web/api/v2/content/middleware.js b/core/server/web/api/v2/content/middleware.js index 434fa1aa86..e3f837b6bd 100644 --- a/core/server/web/api/v2/content/middleware.js +++ b/core/server/web/api/v2/content/middleware.js @@ -18,6 +18,6 @@ module.exports.authenticatePublic = [ auth.authenticate.authenticateContentApi, auth.authorize.authorizeContentApi, cors(), - shared.middlewares.urlRedirects.adminRedirect, + shared.middlewares.urlRedirects.adminSSLAndHostRedirect, shared.middlewares.prettyUrls ]; diff --git a/core/server/web/shared/middlewares/url-redirects.js b/core/server/web/shared/middlewares/url-redirects.js index 770ef03d30..8f5fcb06b6 100644 --- a/core/server/web/shared/middlewares/url-redirects.js +++ b/core/server/web/shared/middlewares/url-redirects.js @@ -111,17 +111,13 @@ _private.redirect = (req, res, next, redirectFn) => { next(); }; -/* - * @deprecated: in favor of adminRedirect (extract public getBlogRedirectUrl method when needed) - */ -const urlRedirects = (req, res, next) => { - const redirectFn = res.isAdmin ? _private.getAdminRedirectUrl : _private.getBlogRedirectUrl; - _private.redirect(req, res, next, redirectFn); +const frontendRedirect = (req, res, next) => { + _private.redirect(req, res, next, _private.getBlogRedirectUrl); }; const adminRedirect = (req, res, next) => { _private.redirect(req, res, next, _private.getAdminRedirectUrl); }; -module.exports = urlRedirects; -module.exports.adminRedirect = adminRedirect; +module.exports.frontendSSLRedirect = frontendRedirect; +module.exports.adminSSLAndHostRedirect = adminRedirect; diff --git a/core/server/web/site/app.js b/core/server/web/site/app.js index 50135b7a96..c5123ec7b2 100644 --- a/core/server/web/site/app.js +++ b/core/server/web/site/app.js @@ -96,12 +96,12 @@ module.exports = function setupSiteApp(options = {}) { // see https://github.com/TryGhost/Ghost/issues/7707 shared.middlewares.customRedirects.use(siteApp); - // More redirects - siteApp.use(mw.adminRedirects()); + // (Optionally) redirect any requests to /ghost to the admin panel + siteApp.use(mw.redirectGhostToAdmin()); // force SSL if blog url is set to https. The redirects handling must happen before asset and page routing, // otherwise we serve assets/pages with http. This can cause mixed content warnings in the admin client. - siteApp.use(shared.middlewares.urlRedirects); + siteApp.use(shared.middlewares.urlRedirects.frontendSSLRedirect); // Static content/assets // @TODO make sure all of these have a local 404 error handler diff --git a/core/server/web/site/middleware/index.js b/core/server/web/site/middleware/index.js index 6d6f87b35c..2ed0e79ba1 100644 --- a/core/server/web/site/middleware/index.js +++ b/core/server/web/site/middleware/index.js @@ -1,6 +1,6 @@ module.exports = { - adminRedirects: require('./admin-redirects'), handleImageSizes: require('./handle-image-sizes'), + redirectGhostToAdmin: require('./redirect-ghost-to-admin'), serveFavicon: require('./serve-favicon'), servePublicFile: require('./serve-public-file'), staticTheme: require('./static-theme') diff --git a/core/server/web/site/middleware/admin-redirects.js b/core/server/web/site/middleware/redirect-ghost-to-admin.js similarity index 84% rename from core/server/web/site/middleware/admin-redirects.js rename to core/server/web/site/middleware/redirect-ghost-to-admin.js index e9ee8e9711..b030004851 100644 --- a/core/server/web/site/middleware/admin-redirects.js +++ b/core/server/web/site/middleware/redirect-ghost-to-admin.js @@ -9,11 +9,9 @@ const adminRedirect = (path) => { }; // redirect to /ghost to the admin -module.exports = function adminRedirects() { +module.exports = function redirectGhostToAdmin() { const router = express.Router(); - console.log('loading admin redirects'); - if (config.get('admin:redirects')) { router.get(/^\/ghost\/?$/, adminRedirect('/')); } diff --git a/test/unit/web/middleware/url-redirects_spec.js b/test/unit/web/middleware/url-redirects_spec.js index 5f25bff059..b47b2bc7ce 100644 --- a/test/unit/web/middleware/url-redirects_spec.js +++ b/test/unit/web/middleware/url-redirects_spec.js @@ -3,7 +3,7 @@ var should = require('should'), rewire = require('rewire'), urlUtils = require('../../../utils/urlUtils'), urlRedirects = rewire('../../../../core/server/web/shared/middlewares/url-redirects'), - {adminRedirect} = urlRedirects, + {frontendSSLRedirect, adminSSLAndHostRedirect} = urlRedirects, getAdminRedirectUrl = urlRedirects.__get__('_private.getAdminRedirectUrl'), getBlogRedirectUrl = urlRedirects.__get__('_private.getBlogRedirectUrl'), redirect = urlRedirects.__get__('_private.redirect'); @@ -38,27 +38,18 @@ describe('UNIT: url redirects', function () { urlRedirects.__set__('_private.redirect', redirectSpy); }); - it('urlRedirects passes getAdminRedirectUrl method when iAdmin flag is not set', function () { + it('frontendSSLRedirect passes getBlogRedirectUrl', function () { urlRedirects.__set__('urlUtils', urlUtils.getInstance({url: 'https://default.com:2368/'})); - urlRedirects(req, res, next); + frontendSSLRedirect(req, res, next); redirectSpy.calledWith(req, res, next, getBlogRedirectUrl).should.eql(true); }); - it('urlRedirects passes getAdminRedirectUrl method when iAdmin flag present', function () { - res.isAdmin = true; + it('adminSSLAndHostRedirect passes getAdminRedirectUrl', function () { urlRedirects.__set__('urlUtils', urlUtils.getInstance({url: 'https://default.com:2368/'})); - urlRedirects(req, res, next); - - redirectSpy.calledWith(req, res, next, getAdminRedirectUrl).should.eql(true); - }); - - it('adminRedirect passes getAdminRedirectUrl', function () { - urlRedirects.__set__('urlUtils', urlUtils.getInstance({url: 'https://default.com:2368/'})); - - adminRedirect(req, res, next); + adminSSLAndHostRedirect(req, res, next); redirectSpy.calledWith(req, res, next, getAdminRedirectUrl).should.eql(true); });