fix: Don't add trailing slashes for certain URLs

fix #2037

config/default-amo.js

@@ -78,6 +78,17 @@ 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: [
+    '/$lang/$clientApp/user/edit',
+  ],
 
   trackingEnabled: true,
   trackingId: 'UA-36116321-7',

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',

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';
@@ -132,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 = req.originalUrl.replace(/^\//, '').split('/');
+
+  // 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, we'll add one.
-  if (URLParts[0].substr(-1) !== '/') {
-    URLParts[0] = `${URLParts[0]}/`;
-    return res.redirect(301, URLParts.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();

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.
  *

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/no/trailing',
+      '/$clientApp/no/trailing',
+      '/$lang/$clientApp/no/trailing',
+      '/no/trailing',
+    ]);
   });
 
   it('should call next and do nothing with a valid, trailing slash URL', () => {
@@ -248,6 +255,50 @@ 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/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 $clientApp', () => {
+    const fakeReq = {
+      originalUrl: '/firefox/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/$clientApp', () => {
+    const fakeReq = {
+      originalUrl: '/fr/firefox/no/trailing',
+      headers: {},
+    };
+    trailingSlashesMiddleware(
+      fakeReq, fakeRes, fakeNext, { _config: fakeConfig });
+    assert.notOk(fakeRes.redirect.called);
+    assert.ok(fakeNext.called);
+  });
+
   it('should include query params in the redirect', () => {
     const fakeReq = {
       originalUrl: '/foo/search?q=foo&category=bar',