From 0b1cd30f84fdb0a75a5fafaa4a473a6e185077ee Mon Sep 17 00:00:00 2001 From: Stuart Colville Date: Tue, 4 Jun 2019 17:17:32 +0100 Subject: [PATCH] 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 --- src/core/middleware/prefixMiddleware.js | 43 +++++++++++-------- .../core/middleware/test_prefixMiddleware.js | 30 +++++++++++++ 2 files changed, 56 insertions(+), 17 deletions(-) diff --git a/src/core/middleware/prefixMiddleware.js b/src/core/middleware/prefixMiddleware.js index 9e174573fd0..f8f57a1b0c9 100644 --- a/src/core/middleware/prefixMiddleware.js +++ b/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. diff --git a/tests/unit/core/middleware/test_prefixMiddleware.js b/tests/unit/core/middleware/test_prefixMiddleware.js index 0b84016948d..c5cbbaa06b1 100644 --- a/tests/unit/core/middleware/test_prefixMiddleware.js +++ b/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',