diff --git a/core/server/web/shared/middlewares/custom-redirects.js b/core/server/web/shared/middlewares/custom-redirects.js index 58b2412b57..ea13d48ee7 100644 --- a/core/server/web/shared/middlewares/custom-redirects.js +++ b/core/server/web/shared/middlewares/custom-redirects.js @@ -17,77 +17,90 @@ const _private = {}; let customRedirectsRouter; -_private.registerRoutes = () => { +/** + * + * @param {Object} redirects valid redirects JSON + * @returns {Object} instance of express.Router express router handling redirects based on config + */ +_private.registerRoutes = (redirects) => { debug('redirects loading'); - customRedirectsRouter = express.Router('redirects'); + const redirectsRouter = express.Router('redirects'); - try { - const redirects = redirectsService.loadRedirectsFile(); + if (labsService.isSet('offers')) { + redirects.unshift({ + from: '/zimo50', + to: '/#/portal/offers/abcdefuckoff' + }); + } - if (labsService.isSet('offers')) { - redirects.unshift({ - from: '/zimo50', - to: '/#/portal/offers/abcdefuckoff' - }); + redirects.forEach((redirect) => { + /** + * Detect case insensitive modifier when regex is enclosed by + * / ... /i + */ + let options = ''; + if (redirect.from.match(/^\/.*\/i$/)) { + redirect.from = redirect.from.slice(1, -2); + options = 'i'; } + /** + * always delete trailing slashes, doesn't matter if regex or not + * Example: + * - you define /my-blog-post-1/ as from property + * - /my-blog-post-1 or /my-blog-post-1/ should work + */ + + if (redirect.from.match(/\/$/)) { + redirect.from = redirect.from.slice(0, -1); + } + + if (redirect.from[redirect.from.length - 1] !== '$') { + redirect.from += '/?$'; + } + + debug('register', redirect.from); + redirectsRouter.get(new RegExp(redirect.from, options), function (req, res) { + const maxAge = redirect.permanent ? config.get('caching:customRedirects:maxAge') : 0; + const toURL = url.parse(redirect.to); + const toURLParams = querystring.parse(toURL.query); + const currentURL = url.parse(req.url); + const currentURLParams = querystring.parse(currentURL.query); + const params = Object.assign({}, currentURLParams, toURLParams); + const search = querystring.stringify(params); + + toURL.pathname = currentURL.pathname.replace(new RegExp(redirect.from, options), toURL.pathname); + toURL.search = search !== '' ? `?${search}` : null; + + /** + * Only if the URL is internal should we prepend the Ghost subdirectory + * @see https://github.com/TryGhost/Ghost/issues/10776 + */ + if (!toURL.hostname) { + toURL.pathname = urlUtils.urlJoin(urlUtils.getSubdir(), toURL.pathname); + } + + res.set({ + 'Cache-Control': `public, max-age=${maxAge}` + }); + + res.redirect(redirect.permanent ? 301 : 302, url.format(toURL)); + }); + }); + + debug('redirects loaded'); + + return redirectsRouter; +}; + +const loadRoutes = () => { + try { + const redirects = redirectsService.loadRedirectsFile(); redirectsService.validate(redirects); - redirects.forEach((redirect) => { - /** - * Detect case insensitive modifier when regex is enclosed by - * / ... /i - */ - let options = ''; - if (redirect.from.match(/^\/.*\/i$/)) { - redirect.from = redirect.from.slice(1, -2); - options = 'i'; - } - - /** - * always delete trailing slashes, doesn't matter if regex or not - * Example: - * - you define /my-blog-post-1/ as from property - * - /my-blog-post-1 or /my-blog-post-1/ should work - */ - - if (redirect.from.match(/\/$/)) { - redirect.from = redirect.from.slice(0, -1); - } - - if (redirect.from[redirect.from.length - 1] !== '$') { - redirect.from += '/?$'; - } - - debug('register', redirect.from); - customRedirectsRouter.get(new RegExp(redirect.from, options), function (req, res) { - const maxAge = redirect.permanent ? config.get('caching:customRedirects:maxAge') : 0; - const toURL = url.parse(redirect.to); - const toURLParams = querystring.parse(toURL.query); - const currentURL = url.parse(req.url); - const currentURLParams = querystring.parse(currentURL.query); - const params = Object.assign({}, currentURLParams, toURLParams); - const search = querystring.stringify(params); - - toURL.pathname = currentURL.pathname.replace(new RegExp(redirect.from, options), toURL.pathname); - toURL.search = search !== '' ? `?${search}` : null; - - /** - * Only if the URL is internal should we prepend the Ghost subdirectory - * @see https://github.com/TryGhost/Ghost/issues/10776 - */ - if (!toURL.hostname) { - toURL.pathname = urlUtils.urlJoin(urlUtils.getSubdir(), toURL.pathname); - } - - res.set({ - 'Cache-Control': `public, max-age=${maxAge}` - }); - - res.redirect(redirect.permanent ? 301 : 302, url.format(toURL)); - }); - }); + const redirectsRouter = _private.registerRoutes(redirects); + customRedirectsRouter = redirectsRouter; } catch (err) { if (errors.utils.isIgnitionError(err)) { logging.error(err); @@ -100,8 +113,6 @@ _private.registerRoutes = () => { })); } } - - debug('redirects loaded'); }; /** @@ -110,7 +121,7 @@ _private.registerRoutes = () => { * - file loads synchronously, because we need to register the routes before anything else */ exports.use = function use(siteApp) { - _private.registerRoutes(); + loadRoutes(); // Recommended approach by express, see https://github.com/expressjs/express/issues/2596#issuecomment-81353034. // As soon as the express router get's re-instantiated, the old router instance is not used anymore. @@ -120,5 +131,5 @@ exports.use = function use(siteApp) { }; exports.reload = function reload() { - _private.registerRoutes(); + loadRoutes(); }; diff --git a/test/regression/api/canary/admin/redirects.test.js b/test/regression/api/canary/admin/redirects.test.js index b750d211f7..6040cc1172 100644 --- a/test/regression/api/canary/admin/redirects.test.js +++ b/test/regression/api/canary/admin/redirects.test.js @@ -383,21 +383,6 @@ describe('Redirects API', function () { }); }); - // https://github.com/TryGhost/Ghost/issues/10898 - describe('Merge querystring', function () { - it('toURL param takes precedence, other params pass through', function () { - return startGhost({forceStart: true, redirectsFileExt: '.json'}) - .then(function () { - return request - .get('/test-params/?q=123&lang=js') - .expect(301) - .then(function (res) { - res.headers.location.should.eql('/result?q=abc&lang=js'); - }); - }); - }); - }); - // TODO: For backward compatibility, we only check if download, upload endpoints work here. // when updating to v4, the old endpoints should be updated to the new ones. // And the tests below should be removed. diff --git a/test/regression/api/v3/admin/redirects.test.js b/test/regression/api/v3/admin/redirects.test.js index 3e8b302fd7..a975dc8838 100644 --- a/test/regression/api/v3/admin/redirects.test.js +++ b/test/regression/api/v3/admin/redirects.test.js @@ -383,21 +383,6 @@ describe('Redirects API', function () { }); }); - // https://github.com/TryGhost/Ghost/issues/10898 - describe('Merge querystring', function () { - it('toURL param takes precedence, other params pass through', function () { - return startGhost({forceStart: true, redirectsFileExt: '.json'}) - .then(function () { - return request - .get('/test-params/?q=123&lang=js') - .expect(301) - .then(function (res) { - res.headers.location.should.eql('/result?q=abc&lang=js'); - }); - }); - }); - }); - // TODO: For backward compatibility, we only check if download, upload endpoints work here. // when updating to v4, the old endpoints should be updated to the new ones. // And the tests below should be removed. diff --git a/test/unit/web/shared/middleware/custom-redirects.test.js b/test/unit/web/shared/middleware/custom-redirects.test.js new file mode 100644 index 0000000000..6ecf840d78 --- /dev/null +++ b/test/unit/web/shared/middleware/custom-redirects.test.js @@ -0,0 +1,48 @@ +const should = require('should'); +const sinon = require('sinon'); +const rewire = require('rewire'); + +const customRedirects = rewire('../../../../../core/server/web/shared/middlewares/custom-redirects'); +const registerRoutes = customRedirects.__get__('_private.registerRoutes'); + +describe('UNIT: custom redirects', function () { + let res; + let req; + let next; + + beforeEach(function () { + req = { + method: 'GET' + }; + res = { + redirect: sinon.spy(), + set: sinon.spy() + }; + + next = sinon.spy(); + }); + + afterEach(function () { + sinon.restore(); + }); + + // Related to: https://github.com/TryGhost/Ghost/issues/10898 + it('toURL param takes precedence, other params pass through', function () { + const redirectsConfig = [{ + permanent: true, + from: '/test-params', + to: '/result?q=abc' + }]; + const redirect = registerRoutes(redirectsConfig); + + req.url = '/test-params/?q=123&lang=js'; + redirect(req, res, next); + + next.called.should.be.false(); + res.redirect.called.should.be.true(); + res.redirect.calledWith(301, '/result?q=abc&lang=js').should.be.true(); + res.set.calledWith({ + 'Cache-Control': `public, max-age=0` + }); + }); +});