feat(client): Stop passing 400 page error messages via query parameters.

Use a session cookie to pass the message instead.

fixes #3649

app/scripts/lib/error-utils.js

@@ -14,7 +14,6 @@ define(function (require, exports, module) {
   var Logger = require('lib/logger');
   var OAuthErrors = require('lib/oauth-errors');
   var p = require('lib/promise');
-  var Url = require('lib/url');
 
   var ERROR_REDIRECT_TIMEOUT_MS = Constants.ERROR_REDIRECT_TIMEOUT_MS;
 
@@ -25,30 +24,35 @@ define(function (require, exports, module) {
      * Get the URL of the error page to which an error should redirect.
      *
      * @param {Error} error - error for which to get error page URL
-     * @param {Object} translator - translator to translate error
      * @returns {String}
      */
-    getErrorPageUrl: function (error, translator) {
+    getErrorPageUrl: function (error) {
       if (AuthErrors.is(error, 'INVALID_PARAMETER') ||
           AuthErrors.is(error, 'MISSING_PARAMETER') ||
           OAuthErrors.is(error, 'INVALID_PARAMETER') ||
           OAuthErrors.is(error, 'MISSING_PARAMETER') ||
           OAuthErrors.is(error, 'UNKNOWN_CLIENT')) {
-        var queryString = Url.objToSearchString({
-          client_id: error.client_id, //eslint-disable-line camelcase
-          context: error.context,
-          errno: error.errno,
-          message: error.errorModule.toInterpolatedMessage(error, translator),
-          namespace: error.namespace,
-          param: error.param
-        });
-
-        return Constants.BAD_REQUEST_PAGE + queryString;
+        return Constants.BAD_REQUEST_PAGE;
       }
 
       return Constants.INTERNAL_ERROR_PAGE;
     },
 
+    /**
+     * Get an interpolated and translated error message.
+     *
+     * @param {Error} error - error to convert
+     * @param {Object} translator - translator to translate error
+     * @returns {String}
+     */
+    getDisplayedErrorMessage: function (error, translator) {
+      if (error.errorModule) {
+        return error.errorModule.toInterpolatedMessage(error, translator);
+      } else {
+        return error.message;
+      }
+    },
+
     /**
      * Report an error to metrics. No metrics report is sent.
      *
@@ -104,7 +108,14 @@ define(function (require, exports, module) {
         // otherwise Safari Mobile redirects too quickly.
         .delay(self.ERROR_REDIRECT_TIMEOUT_MS)
         .then(function () {
-          var errorPageUrl = self.getErrorPageUrl(error, translator);
+          var errorMessage = self.getDisplayedErrorMessage(error, translator);
+
+          // Pass the error message in a session cookie to prevent messages
+          // from being tampered with. The cookie is immediately cleared
+          // by the server.
+          win.document.cookie = '__400_message=' + errorMessage + '; path=/400.html;';
+
+          var errorPageUrl = self.getErrorPageUrl(error);
           win.location.href = errorPageUrl;
         });
     }

app/tests/spec/lib/error-utils.js

@@ -65,6 +65,22 @@ define(function (require, exports, module) {
       });
     });
 
+    describe('getDisplayedErrorMessage', function () {
+      describe('with an internal error', function () {
+        it('returns the interpolated message', function () {
+          var err = AuthErrors.toInvalidParameterError('email');
+          assert.equal(ErrorUtils.getDisplayedErrorMessage(err), 'Invalid parameter: email');
+        });
+      });
+
+      describe('with a normal exception', function () {
+        it('returns the exceptions `message`', function () {
+          var err = new Error('boom');
+          assert.equal(ErrorUtils.getDisplayedErrorMessage(err), 'boom');
+        });
+      });
+    });
+
     describe('captureError', function () {
       beforeEach(function () {
         err = AuthErrors.toError('UNEXPECTED_ERROR');
@@ -127,6 +143,11 @@ define(function (require, exports, module) {
         assert.isTrue(metrics.flush.called);
       });
 
+      it('sets `cookie.__400_message` for display on the error page', function () {
+        assert.equal(
+          windowMock.document.cookie, '__400_message=Unexpected error; path=/400.html;');
+      });
+
       it('redirects the user to the error page', function () {
         assert.include(windowMock.location.href, '500.html');
       });

server/lib/routes.js

@@ -22,15 +22,16 @@ module.exports = function (config, i18n) {
   var STATIC_RESOURCE_URL = config.get('static_resource_url');
 
   var routes = [
-    require('./routes/get-terms-privacy')(i18n),
-    require('./routes/get-index')(config),
-    require('./routes/get-ver.json'),
+    require('./routes/get-400.js')(config),
     require('./routes/get-client.json')(i18n),
     require('./routes/get-config')(i18n),
+    require('./routes/get-index')(config),
     require('./routes/get-metrics-errors')(),
     require('./routes/get-openid-authenticate')(config),
     require('./routes/get-openid-configuration')(config),
     require('./routes/get-openid-login')(config),
+    require('./routes/get-terms-privacy')(i18n),
+    require('./routes/get-ver.json'),
     require('./routes/get-version.json'),
     require('./routes/post-metrics')(),
     require('./routes/post-metrics-errors')()
@@ -185,35 +186,10 @@ module.exports = function (config, i18n) {
       });
     }
 
-    // we always want to handle these so we can do some logging.
-    app.get('/400.html', function (req, res) {
-      res.removeHeader('x-frame-options');
-      logger.error('400.html', {
-        client_id: req.query.client_id, //eslint-disable-line camelcase
-        context: req.query.context,
-        errno: req.query.errno,
-        message: req.query.message,
-        namespace: req.query.namespace,
-        param: req.query.param
-      });
-      return res.render('400', {
-        message: req.query.message,
-        staticResourceUrl: STATIC_RESOURCE_URL
-      });
-    });
-
     app.get('/500.html', function (req, res) {
       res.removeHeader('x-frame-options');
-      logger.error('500.html', {
-        context: req.query.context,
-        errno: req.query.errno,
-        message: req.query.message,
-        namespace: req.query.namespace
-      });
       return res.render('500', { staticResourceUrl: STATIC_RESOURCE_URL });
     });
-
   };
-
 };
 

server/lib/routes/get-400.js

@@ -0,0 +1,30 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+module.exports = function (config) {
+  var STATIC_RESOURCE_URL = config.get('static_resource_url');
+
+  return {
+    method: 'get',
+    path: '/400.html',
+    process: function (req, res) {
+      res.removeHeader('x-frame-options');
+
+      // The front end will set a __400_message session cookie. If
+      // no cookie exists, default to `Unexpected error`.
+      // Clear the cookie afterwards so the error message does
+      // not re-appear in error.
+      var message = req.cookies['__400_message'] ||
+                    req.gettext('Unexpected error');
+
+      res.clearCookie('__400_message', { path: '/400.html' });
+
+      return res.render('400', {
+        message: message,
+        staticResourceUrl: STATIC_RESOURCE_URL
+      });
+    }
+  };
+};
+

tests/intern_server.js

@@ -24,6 +24,7 @@ define([
     'tests/server/configuration',
     'tests/server/statsd-collector',
     'tests/server/activity-event',
+    'tests/server/routes/get-400',
     'tests/server/routes/get-index',
     'tests/server/routes/post-csp',
     'tests/server/routes/post-metrics'

tests/server/routes/get-400.js

@@ -0,0 +1,118 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+define([
+  'intern!object',
+  'intern/chai!assert',
+  'intern/dojo/node!bluebird',
+  'intern/dojo/node!path',
+  'intern/dojo/node!sinon',
+  'intern/dojo/node!../../../server/lib/routes/get-400',
+], function (registerSuite, assert, Promise, path, sinon, route) {
+  var config, instance, request, response;
+
+  registerSuite({
+    name: 'routes/get-400',
+
+    'route interface is correct': function () {
+      assert.isFunction(route);
+      assert.lengthOf(route, 1);
+    },
+
+    'initialise route': {
+      setup: function () {
+        config = {
+          get: sinon.spy(function () {
+            return 'foo';
+          })
+        };
+        instance = route(config);
+      },
+
+      'instance interface is correct': function () {
+        assert.isObject(instance);
+        assert.lengthOf(Object.keys(instance), 3);
+        assert.equal(instance.method, 'get');
+        assert.equal(instance.path, '/400.html');
+        assert.isFunction(instance.process);
+        assert.lengthOf(instance.process, 2);
+      },
+
+      'route.process with __400_message cookie set': {
+        setup: function () {
+          request = {
+            cookies: {
+              '__400_message': 'Invalid parameter: email'
+            },
+            gettext: function (msg) {
+              return msg;
+            }
+          };
+          response = {
+            clearCookie: sinon.spy(),
+            removeHeader: sinon.spy(),
+            render: sinon.spy()
+          };
+          instance.process(request, response);
+        },
+
+        'x-frame-options headers are removed': function () {
+          assert.isTrue(response.removeHeader.calledOnce);
+          assert.isTrue(response.removeHeader.calledWith('x-frame-options'));
+        },
+
+        'the __400_message cookie is cleared': function () {
+          assert.isTrue(response.clearCookie.calledOnce);
+          assert.isTrue(response.clearCookie.calledWith('__400_message', { path: '/400.html' }));
+        },
+
+        'response.render was called correctly': function () {
+          assert.equal(response.render.callCount, 1);
+
+          var args = response.render.args[0];
+          assert.lengthOf(args, 2);
+
+          assert.equal(args[0], '400');
+
+          assert.isObject(args[1]);
+          assert.lengthOf(Object.keys(args[1]), 2);
+          assert.equal(args[1].message, 'Invalid parameter: email');
+          assert.equal(args[1].staticResourceUrl, 'foo');
+        }
+      },
+
+      'route.process without __400_message cookie set': {
+        setup: function () {
+          request = {
+            cookies: {
+            },
+            gettext: function (msg) {
+              return msg;
+            }
+          };
+          response = {
+            clearCookie: sinon.spy(),
+            removeHeader: sinon.spy(),
+            render: sinon.spy()
+          };
+          instance.process(request, response);
+        },
+
+        'response.render was called correctly': function () {
+          assert.equal(response.render.callCount, 1);
+
+          var args = response.render.args[0];
+          assert.lengthOf(args, 2);
+
+          assert.equal(args[0], '400');
+
+          assert.isObject(args[1]);
+          assert.lengthOf(Object.keys(args[1]), 2);
+          assert.equal(args[1].message, 'Unexpected error');
+          assert.equal(args[1].staticResourceUrl, 'foo');
+        }
+      }
+    }
+  });
+});