diff --git a/config/default-amo.js b/config/default-amo.js index 538f68a49a1..ddd0abd9e39 100644 --- a/config/default-amo.js +++ b/config/default-amo.js @@ -78,6 +78,26 @@ module.exports = { 'user-media', '__version__', ], + // These URLs are exceptions to our trailing slash URL redirects; if we + // find a URL that matches this pattern we won't redirect to the same url + // with an appended `/`. This is usually because if we redirect, it will + // cause a redirect loop with addons-server; see: + // https://github.com/mozilla/addons-frontend/issues/2037 + // + // We use $lang and $clientApp as placeholders so we can have URLs in this + // list that don't include those URL pieces, if needed. + validTrailingSlashUrlExceptions: [ + // User URLs, found in: + // https://github.com/mozilla/addons-server/blob/master/src/olympia/users/urls.py#L20 + '/$lang/$clientApp/user/abuse', + '/$lang/$clientApp/user/rmlocale', + '/$lang/$clientApp/users/ajax', + '/$lang/$clientApp/users/delete', + '/$lang/$clientApp/users/edit', + '/$lang/$clientApp/users/login', + '/$lang/$clientApp/users/logout', + '/$lang/$clientApp/users/register', + ], trackingEnabled: true, trackingId: 'UA-36116321-7', diff --git a/config/default.js b/config/default.js index f08a7d23b30..31e7dbaca78 100644 --- a/config/default.js +++ b/config/default.js @@ -88,6 +88,7 @@ module.exports = { 'validClientApplications', 'validLocaleUrlExceptions', 'validClientAppUrlExceptions', + 'validTrailingSlashUrlExceptions', ], // Content Security Policy. @@ -220,6 +221,7 @@ module.exports = { validLocaleUrlExceptions: [], validClientAppUrlExceptions: [], + validTrailingSlashUrlExceptions: [], // The default app used in the URL. defaultClientApp: 'firefox', diff --git a/src/amo/routes.js b/src/amo/routes.js index fff800d107d..956176478d8 100644 --- a/src/amo/routes.js +++ b/src/amo/routes.js @@ -29,7 +29,12 @@ export default ( - {/* Hack to make the proxy serve this URL from addons-server */} + {/* These user routes are to make the proxy serve each URL from */} + {/* addons-server until we can fix the :visibleAddonType route below. */} + {/* https://github.com/mozilla/addons-frontend/issues/2029 */} + {/* We are mimicing these URLs: https://github.com/mozilla/addons-server/blob/master/src/olympia/users/urls.py#L20 */} + + {/* https://github.com/mozilla/addons-frontend/issues/1975 */} diff --git a/src/core/middleware.js b/src/core/middleware.js index e7f66481048..06db700aa68 100644 --- a/src/core/middleware.js +++ b/src/core/middleware.js @@ -3,8 +3,9 @@ import config from 'config'; import { getClientApp, isValidClientApp, - isValidLocaleUrlException, isValidClientAppUrlException, + isValidLocaleUrlException, + isValidTrailingSlashUrlException, } from 'core/utils'; import { getLanguage, isValidLang } from 'core/i18n/utils'; import log from 'core/logger'; @@ -19,15 +20,18 @@ export function prefixMiddleWare(req, res, next, { _config = config } = {}) { // or missing. const [langFromURL, appFromURL] = URLParts; - // Get language from URL or fall-back to detecting it from accept-language header. + // Get language from URL or fall-back to detecting it from accept-language + // header. const acceptLanguage = req.headers['accept-language']; - const { lang, isLangFromHeader } = getLanguage({ lang: langFromURL, acceptLanguage }); + const { lang, isLangFromHeader } = getLanguage({ + lang: langFromURL, acceptLanguage }); // Get the application from the UA if one wasn't specified in the URL (or // if it turns out to be invalid). const application = getClientApp(req.headers['user-agent']); const hasValidLang = isValidLang(langFromURL); - const hasValidLocaleException = isValidLocaleUrlException(appFromURL, { _config }); + const hasValidLocaleException = isValidLocaleUrlException( + appFromURL, { _config }); const hasValidClientApp = isValidClientApp(appFromURL, { _config }); let hasValidClientAppUrlException = isValidClientAppUrlException( appFromURL, { _config }); @@ -129,13 +133,39 @@ export function prefixMiddleWare(req, res, next, { _config = config } = {}) { return next(); } -export function trailingSlashesMiddleware(req, res, next) { - const URLParts = req.originalUrl.split('?'); +export function trailingSlashesMiddleware( + req, res, next, { _config = config } = {} +) { + const UrlParts = req.originalUrl.split('?'); + const UrlSlashSeparated = UrlParts[0].replace(/^\//, '').split('/'); - // If the URL doesn't end with a trailing slash, we'll add one. - if (URLParts[0].substr(-1) !== '/') { - URLParts[0] = `${URLParts[0]}/`; - return res.redirect(301, URLParts.join('?')); + // If part of this URL should include a lang or clientApp, check for one + // and make sure they're valid. + if (isValidLang(UrlSlashSeparated[0], { _config })) { + UrlSlashSeparated[0] = '$lang'; + } + if (isValidClientApp(UrlSlashSeparated[1], { _config })) { + UrlSlashSeparated[1] = '$clientApp'; + } else if (isValidClientApp(UrlSlashSeparated[0], { _config })) { + // It's possible there is a clientApp in the first part of the URL if this + // URL is in validLocaleUrlExceptions. + UrlSlashSeparated[0] = '$clientApp'; + } + + const urlToCheck = `/${UrlSlashSeparated.join('/')}`; + + // If the URL doesn't end with a trailing slash, and it isn't an exception, + // we'll add a trailing slash. + if ( + !isValidTrailingSlashUrlException(urlToCheck, { _config }) && + UrlParts[0].substr(-1) !== '/' + ) { + UrlParts[0] = `${UrlParts[0]}/`; + return res.redirect(301, UrlParts.join('?')); + } else if (isValidTrailingSlashUrlException(urlToCheck, { _config })) { + log.info( + 'Not adding a trailing slash; validTrailingSlashUrlException found', + urlToCheck); } return next(); diff --git a/src/core/utils.js b/src/core/utils.js index 08167e8c408..938047fe322 100644 --- a/src/core/utils.js +++ b/src/core/utils.js @@ -216,6 +216,12 @@ export function isValidClientAppUrlException(value, { _config = config } = {}) { return _config.get('validClientAppUrlExceptions').includes(value); } +export function isValidTrailingSlashUrlException( + value, { _config = config } = {} +) { + return _config.get('validTrailingSlashUrlExceptions').includes(value); +} + /* * Make sure a callback returns a rejected promise instead of throwing an error. * diff --git a/tests/client/core/test_middleware.js b/tests/client/core/test_middleware.js index ba5eb32bff1..e92ba7327f5 100644 --- a/tests/client/core/test_middleware.js +++ b/tests/client/core/test_middleware.js @@ -225,6 +225,13 @@ describe('Trailing Slashes Middleware', () => { }; fakeConfig = new Map(); fakeConfig.set('enableTrailingSlashesMiddleware', true); + fakeConfig.set('validClientApplications', ['firefox']); + fakeConfig.set('validTrailingSlashUrlExceptions', [ + '/$lang/lack/trailing', + '/$clientApp/none/trailing', + '/$lang/$clientApp/slash/trailing', + '/no/trailing', + ]); }); it('should call next and do nothing with a valid, trailing slash URL', () => { @@ -248,6 +255,121 @@ describe('Trailing Slashes Middleware', () => { assert.notOk(fakeNext.called); }); + it('should not add trailing slashes if the URL has an exception', () => { + const fakeReq = { + originalUrl: '/no/trailing', + headers: {}, + }; + trailingSlashesMiddleware( + fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); + assert.notOk(fakeRes.redirect.called); + assert.ok(fakeNext.called); + }); + + it('should handle trailing slash exceptions with $lang', () => { + const fakeReq = { + originalUrl: '/en-US/lack/trailing', + headers: {}, + }; + trailingSlashesMiddleware( + fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); + assert.notOk(fakeRes.redirect.called); + assert.ok(fakeNext.called); + }); + + it('should handle trailing slash exceptions with $clientApp', () => { + const fakeReq = { + originalUrl: '/firefox/none/trailing', + headers: {}, + }; + trailingSlashesMiddleware( + fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); + assert.notOk(fakeRes.redirect.called); + assert.ok(fakeNext.called); + }); + + it('should handle trailing slash exceptions with $lang/$clientApp', () => { + const fakeReq = { + originalUrl: '/fr/firefox/slash/trailing', + headers: {}, + }; + trailingSlashesMiddleware( + fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); + assert.notOk(fakeRes.redirect.called); + assert.ok(fakeNext.called); + }); + + it('should not be an exception without $lang/$clientApp', () => { + const fakeReq = { + originalUrl: '/slash/trailing', + headers: {}, + }; + trailingSlashesMiddleware( + fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); + assert.deepEqual(fakeRes.redirect.firstCall.args, + [301, '/slash/trailing/']); + assert.notOk(fakeNext.called); + }); + + it('should not be an exception without both $lang/$clientApp', () => { + const fakeReq = { + originalUrl: '/en-US/slash/trailing', + headers: {}, + }; + trailingSlashesMiddleware( + fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); + assert.deepEqual(fakeRes.redirect.firstCall.args, + [301, '/en-US/slash/trailing/']); + assert.notOk(fakeNext.called); + }); + + it('redirects a URL with $lang and $clientApp without an exception', () => { + const fakeReq = { + originalUrl: '/en-US/firefox/trailing', + headers: {}, + }; + trailingSlashesMiddleware( + fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); + assert.deepEqual(fakeRes.redirect.firstCall.args, + [301, '/en-US/firefox/trailing/']); + assert.notOk(fakeNext.called); + }); + + it('detects an exception that has a query string', () => { + const fakeReq = { + originalUrl: '/no/trailing?query=test', + headers: {}, + }; + trailingSlashesMiddleware( + fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); + assert.notOk(fakeRes.redirect.called); + assert.ok(fakeNext.called); + }); + + it('does not remove query string', () => { + const fakeReq = { + originalUrl: '/hello?query=test', + headers: {}, + }; + trailingSlashesMiddleware( + fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); + assert.deepEqual(fakeRes.redirect.firstCall.args, + [301, '/hello/?query=test']); + assert.notOk(fakeNext.called); + }); + + it('should not be an exception without both $lang/$clientApp', () => { + const fakeReq = { + originalUrl: '/firefox/slash/trailing', + headers: {}, + }; + trailingSlashesMiddleware( + fakeReq, fakeRes, fakeNext, { _config: fakeConfig }); + assert.deepEqual(fakeRes.redirect.firstCall.args, + [301, '/firefox/slash/trailing/']); + assert.notOk(fakeNext.called); + }); + it('should include query params in the redirect', () => { const fakeReq = { originalUrl: '/foo/search?q=foo&category=bar', diff --git a/tests/server/amo/TestViews.js b/tests/server/amo/TestViews.js index ce545f95802..869ba2638e6 100644 --- a/tests/server/amo/TestViews.js +++ b/tests/server/amo/TestViews.js @@ -66,4 +66,54 @@ describe('AMO GET Requests', () => { it('should respond with a 404 to user pages', () => request(app) .get('/en-US/firefox/user/some-user/') .expect(404)); + + it('should respond with a 404 to the user ajax page', () => request(app) + .get('/en-US/firefox/users/ajax') + .expect(404)); + + it('should respond with a 404 to the user delete page', () => request(app) + .get('/en-US/firefox/users/delete') + .expect(404)); + + it('should respond with a 404 to the user edit page', () => request(app) + .get('/en-US/firefox/users/edit') + .expect(404)); + + it('should respond with a 404 to the user login page', () => request(app) + .get('/en-US/firefox/users/login') + .expect(404)); + + it('should respond with a 404 to the user logout page', () => request(app) + .get('/en-US/firefox/users/logout') + .expect(404)); + + it('should respond with a 404 to the user register page', () => request(app) + .get('/en-US/firefox/users/register') + .expect(404)); + + // These will cause addons-server to redirect back to the URL without a + // trailing slash, but that will work as we 404 on that too. + it('should 404 the user edit page with slashes', () => request(app) + .get('/en-US/firefox/users/edit/') + .expect(404)); + + it('should 404 the user login page with slashes', () => request(app) + .get('/en-US/firefox/users/login/') + .expect(404)); + + it('should respond with a 404 to a specific user edit', () => request(app) + .get('/en-US/firefox/users/edit/1234/') + .expect(404)); + + it('should respond with a 404 to user unsubscribe actions', () => request(app) + .get('/en-US/firefox/users/unsubscribe/token/signature/permission/') + .expect(404)); + + it('should respond with a 404 to deleting a user photo', () => request(app) + .get('/en-US/firefox/users/delete_photo/1234/') + .expect(404)); + + it('should respond with a 404 to any user action', () => request(app) + .get('/en-US/firefox/users/login/') + .expect(404)); });