From b9d4e5b5f0010a91f07f68d9eb06e538125d92b7 Mon Sep 17 00:00:00 2001 From: Shane Tomlinson Date: Fri, 6 May 2016 10:53:51 +0100 Subject: [PATCH] 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 | 39 ++++++---- app/tests/spec/lib/error-utils.js | 21 ++++++ server/lib/routes.js | 32 +------- server/lib/routes/get-400.js | 30 ++++++++ tests/intern_server.js | 1 + tests/server/routes/get-400.js | 118 ++++++++++++++++++++++++++++++ 6 files changed, 199 insertions(+), 42 deletions(-) create mode 100644 server/lib/routes/get-400.js create mode 100644 tests/server/routes/get-400.js diff --git a/app/scripts/lib/error-utils.js b/app/scripts/lib/error-utils.js index aa894b8a51..35291dd0e7 100644 --- a/app/scripts/lib/error-utils.js +++ b/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; }); } diff --git a/app/tests/spec/lib/error-utils.js b/app/tests/spec/lib/error-utils.js index 05b3981395..036557f33b 100644 --- a/app/tests/spec/lib/error-utils.js +++ b/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'); }); diff --git a/server/lib/routes.js b/server/lib/routes.js index dc3d1ea77e..f261ae3af3 100644 --- a/server/lib/routes.js +++ b/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 }); }); - }; - }; diff --git a/server/lib/routes/get-400.js b/server/lib/routes/get-400.js new file mode 100644 index 0000000000..866e712ae4 --- /dev/null +++ b/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 + }); + } + }; +}; + diff --git a/tests/intern_server.js b/tests/intern_server.js index f3932637c9..aaecf91d30 100644 --- a/tests/intern_server.js +++ b/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' diff --git a/tests/server/routes/get-400.js b/tests/server/routes/get-400.js new file mode 100644 index 0000000000..bf7285b42b --- /dev/null +++ b/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'); + } + } + } + }); +});