Handle query params correctly for client app urls with no trailing slash (#8120)

* Handle query params correctly for client app urls with no trailing slash

* Add test for locale only starting url

Author: muffinresearch

src/core/middleware/prefixMiddleware.js

@@ -11,12 +11,14 @@ import { getLanguage, isValidLang } from 'core/i18n/utils';
 import log from 'core/logger';
 
 export function prefixMiddleware(req, res, next, { _config = config } = {}) {
+  const URLParts = req.originalUrl.split('?');
+
   // Split on slashes after removing the leading slash.
-  const URLParts = req.originalUrl.replace(/^\//, '').split('/');
+  const URLPathParts = URLParts[0].replace(/^\//, '').split('/');
 
   // Get lang and app parts from the URL. At this stage they may be incorrect
   // or missing.
-  const [langFromURL, appFromURL] = URLParts;
+  const [langFromURL, appFromURL] = URLPathParts;
 
   // Get language from URL or fall-back to detecting it from accept-language
   // header.
@@ -45,14 +47,16 @@ export function prefixMiddleware(req, res, next, { _config = config } = {}) {
   let prependedOrMovedApplication = false;
 
   if (hasValidLocaleException) {
-    log.info(oneLine`Second part of URL is a locale exception (${URLParts[1]});
+    log.info(oneLine`Second part of URL is a locale exception (${
+      URLPathParts[1]
+    });
       make sure the clientApp is valid`);
 
     // Normally we look for a clientApp in the second part of a URL, but URLs
     // that match a locale exception don't have a locale so we look for the
     // clientApp in the first part of the URL.
     if (!isValidClientApp(langFromURL, { _config })) {
-      URLParts[0] = application;
+      URLPathParts[0] = application;
       isApplicationFromHeader = true;
       prependedOrMovedApplication = true;
     }
@@ -65,25 +69,28 @@ export function prefixMiddleware(req, res, next, { _config = config } = {}) {
     // * It's valid and we've mapped it e.g: pt -> pt-PT.
     // * The lang is invalid but we have a valid application
     //   e.g. /bogus/firefox/.
-    log.info(`Replacing lang in URL ${URLParts[0]} -> ${lang}`);
-    URLParts[0] = lang;
-  } else if (isValidLocaleUrlException(URLParts[0], { _config })) {
+    log.info(`Replacing lang in URL ${URLPathParts[0]} -> ${lang}`);
+    URLPathParts[0] = lang;
+  } else if (isValidLocaleUrlException(URLPathParts[0], { _config })) {
     log.info(`Prepending clientApp to URL: ${application}`);
-    URLParts.splice(0, 0, application);
+    URLPathParts.splice(0, 0, application);
     isApplicationFromHeader = true;
     prependedOrMovedApplication = true;
   } else if (!hasValidLang) {
     // If lang wasn't valid or was missing prepend one.
     log.info(`Prepending lang to URL: ${lang}`);
-    URLParts.splice(0, 0, lang);
+    URLPathParts.splice(0, 0, lang);
     // If we've prepended the lang to the URL we need to re-check our
     // URL exception and make sure it's valid.
-    hasValidClientAppUrlException = isValidClientAppUrlException(URLParts[1], {
-      _config,
-    });
+    hasValidClientAppUrlException = isValidClientAppUrlException(
+      URLPathParts[1],
+      {
+        _config,
+      },
+    );
   }
 
-  if (!hasValidClientApp && isValidClientApp(URLParts[1], { _config })) {
+  if (!hasValidClientApp && isValidClientApp(URLPathParts[1], { _config })) {
     // We skip prepending an app if we'd previously prepended a lang and the
     // 2nd part of the URL is now a valid app.
     log.info('Application in URL is valid following prepending a lang.');
@@ -93,7 +100,7 @@ export function prefixMiddleware(req, res, next, { _config = config } = {}) {
     );
   } else if (hasValidLocaleException || hasValidClientAppUrlException) {
     if (
-      clientAppRoutes.includes(URLParts[1]) === false &&
+      clientAppRoutes.includes(URLPathParts[1]) === false &&
       (hasValidLang || hasValidLocaleException)
     ) {
       log.info('Exception in URL found; we fallback to addons-server.');
@@ -107,14 +114,16 @@ export function prefixMiddleware(req, res, next, { _config = config } = {}) {
   } else if (!hasValidClientApp) {
     // If the app supplied is not valid we need to prepend one.
     log.info(`Prepending application to URL: ${application}`);
-    URLParts.splice(1, 0, application);
+    URLPathParts.splice(1, 0, application);
     isApplicationFromHeader = true;
   }
 
   // Redirect to the new URL.
   // For safety we'll deny a redirect to a URL starting with '//' since
   // that will be treated as a protocol-free URL.
-  const newURL = `/${URLParts.join('/')}`;
+  URLParts[0] = `/${URLPathParts.join('/')}`;
+  const newURL = URLParts.join('?');
+
   if (newURL !== req.originalUrl && !newURL.startsWith('//')) {
     // Collect vary headers to apply to the redirect
     // so we can make it cacheable.
@@ -130,7 +139,7 @@ export function prefixMiddleware(req, res, next, { _config = config } = {}) {
 
   // Add the data to res.locals to be utilised later.
   /* eslint-disable no-param-reassign */
-  const [newLang, newApp] = URLParts;
+  const [newLang, newApp] = URLPathParts;
   res.locals.lang = newLang;
   // The newApp part of the URL might not be a client application
   // so it's important to re-check that here before assuming it's good.

tests/unit/core/middleware/test_prefixMiddleware.js

@@ -229,6 +229,36 @@ describe(__filename, () => {
     sinon.assert.called(fakeRes.redirect);
   });
 
+  it('should not redirect for locale + app urls missing a trailing slash with query params', () => {
+    const fakeReq = {
+      originalUrl: '/en-US/android?foo=1',
+      headers: {},
+    };
+    prefixMiddleware(fakeReq, fakeRes, fakeNext, { _config: fakeConfig });
+    expect(fakeRes.locals.lang).toEqual('en-US');
+    expect(fakeRes.locals.clientApp).toEqual('android');
+    sinon.assert.notCalled(fakeRes.redirect);
+    sinon.assert.called(fakeNext);
+  });
+
+  it('should redirect for app url missing a trailing slash with query params', () => {
+    const fakeReq = {
+      originalUrl: '/android?foo=2',
+      headers: {},
+    };
+    prefixMiddleware(fakeReq, fakeRes, fakeNext, { _config: fakeConfig });
+    sinon.assert.calledWith(fakeRes.redirect, 301, '/en-US/android?foo=2');
+  });
+
+  it('should redirect for locale url missing a trailing slash with query params', () => {
+    const fakeReq = {
+      originalUrl: '/en-US?foo=3',
+      headers: {},
+    };
+    prefixMiddleware(fakeReq, fakeRes, fakeNext, { _config: fakeConfig });
+    sinon.assert.calledWith(fakeRes.redirect, 301, '/en-US/firefox?foo=3');
+  });
+
   it('should not mangle a query string for a redirect', () => {
     const fakeReq = {
       originalUrl: '/foo/bar?test=1&bar=2',