From 9f500723446dfafc9ed7da8292e72b0ae478c271 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Mon, 24 Nov 2025 15:09:48 +0100 Subject: [PATCH 1/2] Use 302 instead of 301 when redirecting for an invalid lang We are enabling/disabling locales depending on whether their completion percentage so a permanent (301) redirect could be problematic. The caching duration could still be an issue, but at least the non-permanent nature of the redirect should make things easier, and it's more consistent with addons-server which was already using a 302 (with that same caching duration). --- src/amo/middleware/prefixMiddleware.js | 7 ++++++- tests/unit/amo/middleware/test_prefixMiddleware.js | 8 ++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/amo/middleware/prefixMiddleware.js b/src/amo/middleware/prefixMiddleware.js index 3fdfcfd0fd8..e19be36828c 100644 --- a/src/amo/middleware/prefixMiddleware.js +++ b/src/amo/middleware/prefixMiddleware.js @@ -28,6 +28,7 @@ export function prefixMiddleware(req, res, next, { _config = config } = {}) { lang: URLPathParts[0], acceptLanguage, }); + let hasReplacedLang = false; const hasValidLang = isValidLang(URLPathParts[0]); const hasValidClientAppInPartOne = isValidClientApp(URLPathParts[0], { @@ -58,6 +59,7 @@ export function prefixMiddleware(req, res, next, { _config = config } = {}) { hasValidClientAppUrlExceptionInPartTwo ) { log.debug(`Replacing lang in URL: ${URLPathParts[0]} with ${lang}`); + hasReplacedLang = true; URLPathParts.splice(0, 1, lang); } else { log.debug(`Prepending lang and clientApp to URL: ${lang}/${userAgentApp}`); @@ -104,7 +106,10 @@ export function prefixMiddleware(req, res, next, { _config = config } = {}) { res.vary('user-agent'); } res.set('Cache-Control', [`max-age=${ONE_YEAR_IN_SECONDS}`]); - return res.redirect(301, newURL); + // If there was a locale in the URL but we considered it invalid, + // return a 302 temporary redirect, in case this was a locale we have + // disabled and might re-enable later, otherwise 301 permanent. + return res.redirect(hasReplacedLang ? 302 : 301, newURL); } // Add the data to res.locals to be utilised later. diff --git a/tests/unit/amo/middleware/test_prefixMiddleware.js b/tests/unit/amo/middleware/test_prefixMiddleware.js index 33d6f890d5b..2c0b2a05239 100644 --- a/tests/unit/amo/middleware/test_prefixMiddleware.js +++ b/tests/unit/amo/middleware/test_prefixMiddleware.js @@ -32,7 +32,7 @@ describe(__filename, () => { headers: {}, }; prefixMiddleware(fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); - sinon.assert.calledWith(fakeRes.redirect, 301, '/en-US/firefox'); + sinon.assert.calledWith(fakeRes.redirect, 302, '/en-US/firefox'); sinon.assert.calledWith(fakeRes.set, 'Cache-Control', ['max-age=31536000']); sinon.assert.notCalled(fakeNext); }); @@ -43,7 +43,7 @@ describe(__filename, () => { headers: {}, }; prefixMiddleware(fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); - sinon.assert.calledWith(fakeRes.redirect, 301, '/en-US/firefox'); + sinon.assert.calledWith(fakeRes.redirect, 302, '/en-US/firefox'); sinon.assert.calledWith(fakeRes.set, 'Cache-Control', ['max-age=31536000']); sinon.assert.notCalled(fakeNext); }); @@ -90,7 +90,7 @@ describe(__filename, () => { headers: {}, }; prefixMiddleware(fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); - sinon.assert.calledWith(fakeRes.redirect, 301, '/en-US/developers/'); + sinon.assert.calledWith(fakeRes.redirect, 302, '/en-US/developers/'); sinon.assert.calledWith(fakeRes.set, 'Cache-Control', ['max-age=31536000']); sinon.assert.calledWith(fakeRes.vary, 'accept-language'); }); @@ -114,7 +114,7 @@ describe(__filename, () => { headers: {}, }; prefixMiddleware(fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); - sinon.assert.calledWith(fakeRes.redirect, 301, '/pt-PT/firefox/whatever'); + sinon.assert.calledWith(fakeRes.redirect, 302, '/pt-PT/firefox/whatever'); sinon.assert.calledWith(fakeRes.set, 'Cache-Control', ['max-age=31536000']); }); From caacbd8261cb75def48b00084fce89a331b20f1b Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Tue, 25 Nov 2025 14:15:21 +0100 Subject: [PATCH 2/2] Also do a 302 if the first part is present but not recognized --- src/amo/middleware/prefixMiddleware.js | 13 +++++++------ tests/unit/amo/middleware/test_prefixMiddleware.js | 6 +++--- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/amo/middleware/prefixMiddleware.js b/src/amo/middleware/prefixMiddleware.js index e19be36828c..911343c2416 100644 --- a/src/amo/middleware/prefixMiddleware.js +++ b/src/amo/middleware/prefixMiddleware.js @@ -28,7 +28,7 @@ export function prefixMiddleware(req, res, next, { _config = config } = {}) { lang: URLPathParts[0], acceptLanguage, }); - let hasReplacedLang = false; + let hasUnknownPartOne = false; const hasValidLang = isValidLang(URLPathParts[0]); const hasValidClientAppInPartOne = isValidClientApp(URLPathParts[0], { @@ -59,10 +59,11 @@ export function prefixMiddleware(req, res, next, { _config = config } = {}) { hasValidClientAppUrlExceptionInPartTwo ) { log.debug(`Replacing lang in URL: ${URLPathParts[0]} with ${lang}`); - hasReplacedLang = true; + hasUnknownPartOne = true; URLPathParts.splice(0, 1, lang); } else { log.debug(`Prepending lang and clientApp to URL: ${lang}/${userAgentApp}`); + hasUnknownPartOne = !!URLPathParts[0]; URLPathParts.splice(0, 0, lang, userAgentApp); isApplicationFromHeader = true; } @@ -106,10 +107,10 @@ export function prefixMiddleware(req, res, next, { _config = config } = {}) { res.vary('user-agent'); } res.set('Cache-Control', [`max-age=${ONE_YEAR_IN_SECONDS}`]); - // If there was a locale in the URL but we considered it invalid, - // return a 302 temporary redirect, in case this was a locale we have - // disabled and might re-enable later, otherwise 301 permanent. - return res.redirect(hasReplacedLang ? 302 : 301, newURL); + // If there was something at the beginning of the URL we didn't recognize, + // it could be an old locale we have since disabled and might re-enable + // later, so make the redirect temporary (302), otherwise permanent (301). + return res.redirect(hasUnknownPartOne ? 302 : 301, newURL); } // Add the data to res.locals to be utilised later. diff --git a/tests/unit/amo/middleware/test_prefixMiddleware.js b/tests/unit/amo/middleware/test_prefixMiddleware.js index 2c0b2a05239..8efab3e5b0c 100644 --- a/tests/unit/amo/middleware/test_prefixMiddleware.js +++ b/tests/unit/amo/middleware/test_prefixMiddleware.js @@ -77,7 +77,7 @@ describe(__filename, () => { prefixMiddleware(fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); sinon.assert.calledWith( fakeRes.redirect, - 301, + 302, '/en-US/validprefix/whatever', ); sinon.assert.calledWith(fakeRes.set, 'Cache-Control', ['max-age=31536000']); @@ -126,7 +126,7 @@ describe(__filename, () => { }, }; prefixMiddleware(fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); - sinon.assert.calledWith(fakeRes.redirect, 301, '/pt-BR/firefox/whatever'); + sinon.assert.calledWith(fakeRes.redirect, 302, '/pt-BR/firefox/whatever'); sinon.assert.calledWith(fakeRes.vary, 'accept-language'); sinon.assert.calledWith(fakeRes.vary, 'user-agent'); sinon.assert.calledWith(fakeRes.set, 'Cache-Control', ['max-age=31536000']); @@ -218,7 +218,7 @@ describe(__filename, () => { prefixMiddleware(fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); sinon.assert.calledWith( fakeRes.redirect, - 301, + 302, '/en-US/firefox/foo/bar?test=1&bar=2', ); sinon.assert.calledWith(fakeRes.set, 'Cache-Control', ['max-age=31536000']);