Fix redirect loop

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

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/amo/routes.js

@@ -29,7 +29,12 @@ export default (
     <IndexRoute component={Home} />
     <Route path="addon/:slug/" component={DetailPage} />
     <Route path="addon/:addonSlug/reviews/" component={AddonReviewList} />
-    {/* 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 */}
+    <Route path="users/:userAction" component={NotFound} />
+    <Route path="users/:userAction/" component={NotFound} />
     {/* https://github.com/mozilla/addons-frontend/issues/1975 */}
     <Route path="user/:user/" component={NotFound} />
     <Route path=":visibleAddonType/categories/" component={CategoryList} />

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();

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

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));
 });