From 7528ec8c3b44226896dff1edb7cd03dfd77c4e0d Mon Sep 17 00:00:00 2001 From: Kukhyeon Heo Date: Tue, 5 Jan 2021 10:11:06 +0900 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20redirects=20"to"=20query?= =?UTF-8?q?=20params=20forwarding=20=20(#12333)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref #10898 - The redirects configuration's `to` & `from` URL parameters used to ignore it's query string parameters, which resulted in unexpected behavior - Current changeset only partially fixes the issue. Now `to` URL's query parameters always take precedence over incoming query parameters and the rest of query parameters are passed through. --- .../shared/middlewares/custom-redirects.js | 7 ++- .../api/canary/admin/redirects_spec.js | 45 ++++++++++--------- test/utils/fixtures/data/redirects.json | 5 +++ test/utils/fixtures/data/redirects.yaml | 1 + 4 files changed, 37 insertions(+), 21 deletions(-) diff --git a/core/server/web/shared/middlewares/custom-redirects.js b/core/server/web/shared/middlewares/custom-redirects.js index 1bb95964fd..e79f50755e 100644 --- a/core/server/web/shared/middlewares/custom-redirects.js +++ b/core/server/web/shared/middlewares/custom-redirects.js @@ -1,5 +1,6 @@ const express = require('../../../../shared/express'); const url = require('url'); +const querystring = require('querystring'); const debug = require('ghost-ignition').debug('web:shared:mw:custom-redirects'); const config = require('../../../../shared/config'); const urlUtils = require('../../../../shared/url-utils'); @@ -52,10 +53,14 @@ _private.registerRoutes = () => { 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 = currentURL.search; + toURL.search = search !== '' ? `?${search}` : null; /** * Only if the URL is internal should we prepend the Ghost subdirectory diff --git a/test/regression/api/canary/admin/redirects_spec.js b/test/regression/api/canary/admin/redirects_spec.js index 79649c31a7..3e8b302fd7 100644 --- a/test/regression/api/canary/admin/redirects_spec.js +++ b/test/regression/api/canary/admin/redirects_spec.js @@ -27,6 +27,16 @@ describe('Redirects API', function () { }); }); + const startGhost = (options) => { + return ghost(options) + .then(() => { + request = supertest.agent(config.get('url')); + }) + .then(() => { + return localUtils.doAuth(request); + }); + }; + describe('Download', function () { afterEach(function () { configUtils.config.set('paths:contentPath', originalContentPath); @@ -133,16 +143,6 @@ describe('Redirects API', function () { }); describe('Ensure re-registering redirects works', function () { - const startGhost = (options) => { - return ghost(options) - .then(() => { - request = supertest.agent(config.get('url')); - }) - .then(() => { - return localUtils.doAuth(request); - }); - }; - it('no redirects file exists', function () { return startGhost({redirectsFile: false, forceStart: true}) .then(() => { @@ -269,16 +269,6 @@ describe('Redirects API', function () { }); describe('Ensure re-registering redirects works', function () { - const startGhost = (options) => { - return ghost(options) - .then(() => { - request = supertest.agent(config.get('url')); - }) - .then(() => { - return localUtils.doAuth(request); - }); - }; - it('no redirects file exists', function () { return startGhost({redirectsFile: false, forceStart: true}) .then(() => { @@ -393,6 +383,21 @@ 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/utils/fixtures/data/redirects.json b/test/utils/fixtures/data/redirects.json index 85f0b3c5ea..1066edc689 100644 --- a/test/utils/fixtures/data/redirects.json +++ b/test/utils/fixtures/data/redirects.json @@ -8,6 +8,11 @@ "from": "/my-old-blog-post/", "to": "/revamped-url/" }, + { + "permanent": true, + "from": "/test-params", + "to": "/result?q=abc" + }, { "from": "^\\/what(\\/?)$", "to": "/what-does-god-say" diff --git a/test/utils/fixtures/data/redirects.yaml b/test/utils/fixtures/data/redirects.yaml index 10fe707f3a..96dc3e819c 100644 --- a/test/utils/fixtures/data/redirects.yaml +++ b/test/utils/fixtures/data/redirects.yaml @@ -1,5 +1,6 @@ 301: /my-old-blog-post/: /revamped-url/ + /test-params/: /result?q=abc 302: ^/post/[0-9]+/([a-z0-9\-]+): /$1