🐛 Fixed redirects "to" query params forwarding (#12333)

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.

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

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.

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"

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