From 36a8f163cff4d10d91cf6cab775c8d4cc652a41b Mon Sep 17 00:00:00 2001 From: webzwo0i Date: Thu, 24 Dec 2020 14:15:49 +0100 Subject: [PATCH] add test for duplicate query parameters; return 400 instead of 500 on some errors --- src/ep.json | 12 +++---- src/node/hooks/express/errorhandling.js | 37 ++++++++++----------- src/node/utils/caching_middleware.js | 14 ++++++-- tests/backend/specs/caching_middleware.js | 40 ++++++++++++++++++----- 4 files changed, 66 insertions(+), 37 deletions(-) diff --git a/src/ep.json b/src/ep.json index 7e73b69c5..e469bd0c1 100644 --- a/src/ep.json +++ b/src/ep.json @@ -20,6 +20,12 @@ "shutdown": "ep_etherpad-lite/node/hooks/express" } }, + { + "name": "errorhandling", + "hooks": { + "expressCreateServer": "ep_etherpad-lite/node/hooks/express/errorhandling" + } + }, { "name": "static", "hooks": { @@ -74,12 +80,6 @@ "expressCreateServer": "ep_etherpad-lite/node/hooks/express/importexport" } }, - { - "name": "errorhandling", - "hooks": { - "expressCreateServer": "ep_etherpad-lite/node/hooks/express/errorhandling" - } - }, { "name": "socketio", "hooks": { diff --git a/src/node/hooks/express/errorhandling.js b/src/node/hooks/express/errorhandling.js index 993c8e60b..7c7d1b4c0 100644 --- a/src/node/hooks/express/errorhandling.js +++ b/src/node/hooks/express/errorhandling.js @@ -1,26 +1,23 @@ +'use strict'; + const stats = require('ep_etherpad-lite/node/stats'); -exports.expressCreateServer = function (hook_name, args, cb) { - exports.app = args.app; - - // Handle errors +/** + * Express already comes with an error handler that is attached + * as the last middleware. Within all routes it's possible to call + * `next(err)` where `err` is an Error object. You can specify + * `statusCode` and `statusMessage`. + * For more details see "The default error handler" section on + * https://expressjs.com/en/guide/error-handling.html + * + * This method is only used for metrics + * + */ +exports.expressCreateServer = (hookName, args, cb) => { args.app.use((err, req, res, next) => { - // These are errors from caching_middleware, handle them with a 400 - if (err.toString() === 'cm1') { - res.status(400).send({error: 'query parameter callback is not require.define'}); - } else if (err.toString() === 'cm2') { - res.status(400).send({error: 'query parameter v contains the wrong version string'}); - } else if (err.toString() === 'cm3') { - res.status(400).send({error: 'an unknown query parameter is present'}); - } else { - // if an error occurs Connect will pass it down - // through these "error-handling" middleware - // allowing you to respond however you like - res.status(500).send({error: 'Sorry, something bad happened!'}); - console.error(err.stack ? err.stack : err.toString()); - stats.meter('http500').mark(); - } - + const status = err.statusCode || err.status; + stats.meter(`http${status}`).mark(); + next(err); }); return cb(); diff --git a/src/node/utils/caching_middleware.js b/src/node/utils/caching_middleware.js index 08777e9e9..ecf4114ea 100644 --- a/src/node/utils/caching_middleware.js +++ b/src/node/utils/caching_middleware.js @@ -97,19 +97,27 @@ CachingMiddleware.prototype = new function () { const query = queryString.parse(URL.query); // callback must be `require.define` + // implicitly also checks for the parameter to not be present more than once if (query.callback !== 'require.define') { - return next('cm1', req, res); + const error = new Error('query parameter callback is not require.define'); + error.status = 400; + return next(error); } // in case the v parameter is given, it must contain the current version string + // implicitly also checks for the parameter to not be present more than once if (query.v && query.v !== settings.randomVersionString) { - return next('cm2', req, res); + const error = new Error('query parameter v contains the wrong version string'); + error.status = 400; + return next(error); } // does it contain more than the two allowed parameter `callback` and `v`? Object.keys(query).forEach((param) => { if (param !== 'callback' && param !== 'v') { - return next('cm3', req, res); + const error = new Error('an unknown query parameter is present'); + error.status = 400; + return next(error); } }); diff --git a/tests/backend/specs/caching_middleware.js b/tests/backend/specs/caching_middleware.js index d65c5e00b..13f755d84 100644 --- a/tests/backend/specs/caching_middleware.js +++ b/tests/backend/specs/caching_middleware.js @@ -144,11 +144,11 @@ describe(__filename, function () { const missingCallbackKnownFile = '/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js'; await agent.get(missingCallbackUnknownFile) .then((res) => { - assert.equal(res.statusCode, 500); + assert.equal(res.statusCode, 400); }); await agent.get(missingCallbackKnownFile) .then((res) => { - assert.equal(res.statusCode, 500); + assert.equal(res.statusCode, 400); }); }); @@ -161,7 +161,7 @@ describe(__filename, function () { }); await agent.get(vQueryWrong) .then((res) => { - assert.equal(res.statusCode, 500); + assert.equal(res.statusCode, 400); }); }); @@ -172,7 +172,19 @@ describe(__filename, function () { await Promise.all(notAllowed.map(async (resource) => await agent.get(resource) .then((res) => { - assert.equal(res.statusCode, 500) + assert.equal(res.statusCode, 400) + }) + )); + }); + + it('a parameter is not allowed to appear more than once', async function() { + const notAllowed = [ `/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js?callback=require.define&v=${settings.randomVersionString}&v=${settings.randomVersionString}`, + `/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js?callback=require.define&v=${settings.randomVersionString}&callback=require.define`, + ] + await Promise.all(notAllowed.map(async (resource) => + await agent.get(resource) + .then((res) => { + assert.equal(res.statusCode, 400) }) )); }); @@ -301,11 +313,11 @@ describe(__filename, function () { const missingCallbackKnownFile = '/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js'; await agent.get(missingCallbackUnknownFile) .then((res) => { - assert.equal(res.statusCode, 500); + assert.equal(res.statusCode, 400); }); await agent.get(missingCallbackKnownFile) .then((res) => { - assert.equal(res.statusCode, 500); + assert.equal(res.statusCode, 400); }); }); @@ -318,7 +330,7 @@ describe(__filename, function () { }); await agent.get(vQueryWrong) .then((res) => { - assert.equal(res.statusCode, 500); + assert.equal(res.statusCode, 400); }); }); @@ -328,10 +340,22 @@ describe(__filename, function () { ] await Promise.all(notAllowed.map(async (resource) => await agent.get(resource) .then((res) => { - assert.equal(res.statusCode, 500) + assert.equal(res.statusCode, 400) }))); }); + it('a parameter is not allowed to appear more than once', async function() { + const notAllowed = [ `/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js?callback=require.define&v=${settings.randomVersionString}&v=${settings.randomVersionString}`, + `/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js?callback=require.define&v=${settings.randomVersionString}&callback=require.define`, + ] + await Promise.all(notAllowed.map(async (resource) => + await agent.get(resource) + .then((res) => { + assert.equal(res.statusCode, 400) + }) + )); + }); + context('expiration', function(){ it('has date, last-modified and expires header', async function() { await Promise.all(packages.map(async (resource) => await agent.get(resource)