add test for duplicate query parameters; return 400 instead of 500 on some errors

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": {

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

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

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)