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

ensure parameter v contains the random version string
forbid any parameter except v and callback
on error, call `next` with an error instead of sending an error response
directly

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,16 +1,23 @@
+'use strict';
+
 const stats = require('ep_etherpad-lite/node/stats');
 
-exports.expressCreateServer = function (hook_name, args, cb) {
+/**
-  exports.app = args.app;
+ * Express already comes with an error handler that is attached
-
+ * as the last middleware. Within all routes it's possible to call
-  // Handle errors
+ * `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) => {
-    // if an error occurs Connect will pass it down
+    const status = err.statusCode || err.status;
-    // through these "error-handling" middleware
+    stats.meter(`http${status}`).mark();
-    // allowing you to respond however you like
+    next(err);
-    res.status(500).send({error: 'Sorry, something bad happened!'});
-    console.error(err.stack ? err.stack : err.toString());
-    stats.meter('http500').mark();
   });
 
   return cb();

src/node/utils/caching_middleware.js

@@ -21,6 +21,8 @@ const path = require('path');
 const zlib = require('zlib');
 const settings = require('./Settings');
 const existsSync = require('./path_exists');
+const queryString = require('querystring');
+const url = require('url');
 
 /*
  * The crypto module can be absent on reduced node installations.
@@ -89,9 +91,37 @@ CachingMiddleware.prototype = new function () {
     const old_res = {};
 
     const supportsGzip =
-        (req.get('Accept-Encoding') || '').indexOf('gzip') != -1;
+        (req.get('Accept-Encoding') || '').indexOf('gzip') !== -1;
 
-    const path = require('url').parse(req.url).path;
+    const URL = url.parse(req.url);
+    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') {
+      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) {
+      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') {
+        const error = new Error('an unknown query parameter is present');
+        error.status = 400;
+        return next(error);
+      }
+    });
+
+    const path = URL.path;
     const cacheKey = generateCacheKey(path);
 
     fs.stat(`${CACHE_DIR}minified_${cacheKey}`, (error, stats) => {

tests/backend/specs/caching_middleware.js

@@ -42,6 +42,14 @@ function disableAutoDeflate(request) {
   };
 }
 
+/**
+ * Adds some seconds to a date object
+ */
+function addSecondsToDate(date, seconds) {
+  date.setSeconds(date.getSeconds() + seconds);
+  return date;
+}
+
 describe(__filename, function () {
   const backups = {};
   const fantasyEncoding = 'brainwaves'; // non-working encoding until https://github.com/visionmedia/superagent/pull/1560 is resolved
@@ -51,6 +59,7 @@ describe(__filename, function () {
     '/javascripts/lib/ep_etherpad-lite/static/js/pad.js?callback=require.define',
     '/javascripts/lib/ep_etherpad-lite/static/js/timeslider.js?callback=require.define',
   ];
+  const unsupportedMethods = ['post', 'put', 'delete', 'options', 'trace', 'patch'];
 
   before(async function () {
     agent = await common.init();
@@ -68,26 +77,34 @@ describe(__filename, function () {
       settings.minify = false;
     });
     it('gets packages uncompressed without Accept-Encoding gzip', async function () {
-      await Promise.all(packages.map(async (resource) => agent.get(resource)
+      await Promise.all(packages.map(async (resource) => await agent.get(resource)
           .set('Accept-Encoding', fantasyEncoding)
           .use(disableAutoDeflate)
           .then((res) => {
             assert.match(res.header['content-type'], /application\/javascript/);
             assert.equal(res.header['content-encoding'], undefined);
             assert.equal(isPlaintextResponse(res.text, resource), true);
-            return;
           })));
     });
     
+    // need to use head here - cant unset Accept-Encoding in GET requests 
+    it('head request without Accept-Encoding header does not set Content-Encoding', async function () {
+      await agent
+          .head(packages[0]) 
+          .then((res) => {
+            assert.match(res.header['content-type'], /application\/javascript/);
+            assert.equal(res.header['content-encoding'], undefined);
+          });
+    });
+
     it('gets packages compressed with Accept-Encoding gzip', async function () {
-      await Promise.all(packages.map(async (resource) => agent.get(resource)
+      await Promise.all(packages.map(async (resource) => await agent.get(resource)
           .set('Accept-Encoding', 'gzip')
           .use(disableAutoDeflate)
           .then((res) => {
             assert.match(res.header['content-type'], /application\/javascript/);
             assert.equal(res.header['content-encoding'], 'gzip');
             assert.equal(isPlaintextResponse(res.text, resource), false);
-            return;
           })));
     });
 
@@ -102,6 +119,126 @@ describe(__filename, function () {
           .set('Accept-Encoding', fantasyEncoding)
           .then((res) => assert.equal(res.header['content-encoding'], undefined));
     });
+
+    it('only HEAD and GET are supported', async function() {
+      await Promise.all(unsupportedMethods.map(async (method) => {
+        await agent[method](packages[0])
+          .then((res) => {
+            assert.equal(res.statusCode, 405)
+          })
+      }));
+    });
+
+    it('should 200 for unknown resources with jsonp callback', async function() {
+      const missingResource = '/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner2.js?callback=require.define';
+      const expectedResource = "require.define({\n  \"ep_etherpad-lite/static/js/ace2_inner2.js\": null\n});\n";
+      await agent.get(missingResource)
+          .then((res) => {
+            assert.equal(expectedResource, res.text); 
+            assert.equal(res.statusCode, 200);
+          });
+    });
+
+    it('should return 400 for resources without jsonp callback', async function() {
+      const missingCallbackUnknownFile = '/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner2.js';
+      const missingCallbackKnownFile = '/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js';
+      await agent.get(missingCallbackUnknownFile)
+          .then((res) => {
+            assert.equal(res.statusCode, 400);
+          });
+      await agent.get(missingCallbackKnownFile)
+          .then((res) => {
+            assert.equal(res.statusCode, 400);
+          });
+    });
+
+    it('if a query parameter v is given, it must equal the versionString', async function() {
+      const vQueryWrong = '/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js?callback=require.define&v=123';
+      const vQueryRight = `/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js?callback=require.define&v=${settings.randomVersionString}`;
+      await agent.get(vQueryRight)
+          .then((res) => {
+            assert.equal(res.statusCode, 200);
+          });
+      await agent.get(vQueryWrong)
+          .then((res) => {
+            assert.equal(res.statusCode, 400);
+          });
+    });
+
+    it('any parameter except v and callback is forbidden', async function() {
+      const notAllowed = [ `/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js?callback=require.define&v=${settings.randomVersionString}&anotherParam`,
+                           `/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js?callback=require.define&v=${settings.randomVersionString}&anotherParam=123`,
+      ]
+      await Promise.all(notAllowed.map(async (resource) =>
+        await agent.get(resource)
+            .then((res) => {
+              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)
+            .then((res) => {
+              const date = res.header['date'] && new Date(res.header['date']);
+              const lastModified = res.header['last-modified'] && new Date(res.header['last-modified']);
+              const expires = res.header['expires'] && new Date(res.header['expires']);
+              assert.notEqual(date, 'Invalid Date');
+              assert.notEqual(lastModified, 'Invalid Date');
+              assert.notEqual(expires, 'Invalid Date');
+            })));
+      });
+
+      it('maxAge is set and limits the expires value', async function() {
+        await Promise.all(packages.map(async (resource) => await agent.get(resource)
+            .then((res) => {
+              const date = res.header['date'] && new Date(res.header['date']);
+              const expires = res.header['expires'] && new Date(res.header['expires']);
+              const maxAge = res.header['cache-control'];
+              assert.equal(maxAge, `max-age=${settings.maxAge}`);
+              const expirationDate = addSecondsToDate(date, settings.maxAge);
+              assert.ok(Math.abs(expirationDate - expires) <= 1);
+            })));
+        });
+
+      it('returns 304 with correct if-modified-since header', async function(){
+        await Promise.all(packages.map(async (resource) => {
+          await agent.get(resource)
+            .then(async (res) => {
+              const origResult = res.text;
+              const lastModified = res.header['last-modified'] && new Date(res.header['last-modified']);
+              const futureDate = addSecondsToDate(lastModified, +1000);
+
+              await agent.get(resource)
+                  .set('If-Modified-Since', futureDate)
+                  .then((res) => {
+                    assert.equal(res.statusCode, 304);
+                  })
+
+              const pastDate = addSecondsToDate(lastModified, -1100);
+              await agent.get(resource)
+                  .set('If-Modified-Since', pastDate)
+                  .then((res) => {
+                    assert.equal(res.statusCode, 200);
+                    assert.equal(origResult, res.text);
+                  })
+            });
+        }));
+      });
+    });
   });
 
   context('when minify is true', function () {
@@ -109,26 +246,34 @@ describe(__filename, function () {
       settings.minify = true;
     });
     it('gets packages uncompressed without Accept-Encoding gzip', async function () {
-      await Promise.all(packages.map(async (resource) => agent.get(resource)
+      await Promise.all(packages.map(async (resource) => await agent.get(resource)
           .set('Accept-Encoding', fantasyEncoding)
           .use(disableAutoDeflate)
           .then((res) => {
             assert.match(res.header['content-type'], /application\/javascript/);
             assert.equal(res.header['content-encoding'], undefined);
             assert.equal(isPlaintextResponse(res.text, resource), true);
-            return;
           })));
     });
 
+    // need to use head here - cant unset Accept-Encoding in GET requests 
+    it('head request without Accept-Encoding header does not set Content-Encoding', async function () {
+      await agent
+          .head(packages[0]) 
+          .then((res) => {
+            assert.match(res.header['content-type'], /application\/javascript/);
+            assert.equal(res.header['content-encoding'], undefined);
+          });
+    });
+
     it('gets packages compressed with Accept-Encoding gzip', async function () {
-      await Promise.all(packages.map(async (resource) => agent.get(resource)
+      await Promise.all(packages.map(async (resource) => await agent.get(resource)
           .set('Accept-Encoding', 'gzip')
           .use(disableAutoDeflate)
           .then((res) => {
             assert.match(res.header['content-type'], /application\/javascript/);
             assert.equal(res.header['content-encoding'], 'gzip');
             assert.equal(isPlaintextResponse(res.text, resource), false);
-            return;
           })));
     });
 
@@ -143,5 +288,123 @@ describe(__filename, function () {
           .set('Accept-Encoding', fantasyEncoding)
           .then((res) => assert.equal(res.header['content-encoding'], undefined));
     });
+
+    it('only HEAD and GET are supported', async function() {
+      await Promise.all(unsupportedMethods.map(async (method) => {
+        await agent[method](packages[0])
+          .then((res) => {
+            assert.equal(res.statusCode, 405)
+          })
+      }));
+    });
+
+    it('should 200 for unknown resources with jsonp callback', async function() {
+      const missingResource = '/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner2.js?callback=require.define';
+      const expectedResource = "require.define({\n  \"ep_etherpad-lite/static/js/ace2_inner2.js\": null\n});\n";
+      await agent.get(missingResource)
+          .then((res) => {
+            assert.equal(expectedResource, res.text); 
+            assert.equal(res.statusCode, 200);
+          });
+    });
+
+    it('should return 400 for resources without jsonp callback', async function() {
+      const missingCallbackUnknownFile = '/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner2.js';
+      const missingCallbackKnownFile = '/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js';
+      await agent.get(missingCallbackUnknownFile)
+          .then((res) => {
+            assert.equal(res.statusCode, 400);
+          });
+      await agent.get(missingCallbackKnownFile)
+          .then((res) => {
+            assert.equal(res.statusCode, 400);
+          });
+    });
+
+    it('if a query parameter v is given, it must equal the versionString', async function() {
+      const vQueryWrong = '/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js?callback=require.define&v=123';
+      const vQueryRight = `/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js?callback=require.define&v=${settings.randomVersionString}`;
+      await agent.get(vQueryRight)
+          .then((res) => {
+            assert.equal(res.statusCode, 200);
+          });
+      await agent.get(vQueryWrong)
+          .then((res) => {
+            assert.equal(res.statusCode, 400);
+          });
+    });
+
+    it('any parameter except v and callback is forbidden', async function() {
+      const notAllowed = [ `/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js?callback=require.define&v=${settings.randomVersionString}&anotherParam`,
+                           `/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js?callback=require.define&v=${settings.randomVersionString}&anotherParam=123`,
+      ]
+      await Promise.all(notAllowed.map(async (resource) => await agent.get(resource)
+          .then((res) => {
+            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)
+            .then((res) => {
+              const date = res.header['date'] && new Date(res.header['date']);
+              const lastModified = res.header['last-modified'] && new Date(res.header['last-modified']);
+              const expires = res.header['expires'] && new Date(res.header['expires']);
+              assert.notEqual(date, 'Invalid Date');
+              assert.notEqual(lastModified, 'Invalid Date');
+              assert.notEqual(expires, 'Invalid Date');
+            })));
+        });
+
+      it('maxAge is set and limits the expires value', async function() {
+        await Promise.all(packages.map(async (resource) => await agent.get(resource)
+            .then((res) => {
+              const date = res.header['date'] && new Date(res.header['date']);
+              const expires = res.header['expires'] && new Date(res.header['expires']);
+              const maxAge = res.header['cache-control'];
+              assert.equal(maxAge, `max-age=${settings.maxAge}`);
+              const expirationDate = addSecondsToDate(date, settings.maxAge);
+              assert.ok(Math.abs(expirationDate - expires) <= 1);
+            })));
+        });
+
+      it('returns 304 with correct if-modified-since header', async function(){
+        await Promise.all(packages.map(async (resource) => {
+          await agent.get(resource)
+            .then(async (res) => {
+              const origResult = res.text;
+              const lastModified = res.header['last-modified'] && new Date(res.header['last-modified']);
+              const futureDate = addSecondsToDate(lastModified, +1000);
+
+              await agent.get(resource)
+                  .set('If-Modified-Since', futureDate)
+                  .then((res) => {
+                    assert.equal(res.statusCode, 304);
+                  })
+
+              const pastDate = addSecondsToDate(lastModified, -1100);
+              await agent.get(resource)
+                  .set('If-Modified-Since', pastDate)
+                  .then((res) => {
+                    assert.equal(res.statusCode, 200);
+                    assert.equal(origResult, res.text);
+                  })
+            });
+        }));
+      });
+    });
   });
 });

tests/frontend/travis/runner.sh

@@ -34,12 +34,22 @@ done
 log "Successfully connected to Etherpad on http://localhost:9001"
 
 # start the remote runner
-try cd "${MY_DIR}"
+echo "Now starting the remote runner"
-log "Starting the remote runner..."
+failed=0
-node remote_runner.js
+node remote_runner.js || failed=1
-exit_code=$?
 
-kill "$(cat /tmp/sauce.pid)"
+kill $(cat /tmp/sauce.pid)
-kill "$ep_pid" && wait "$ep_pid"
+kill $ep_pid
-log "Done."
+
-exit "$exit_code"
+cd "${MY_DIR}/../../../"
+# print the start of every minified file for debugging
+find var/ -type f -name "minified_*" -not -name "*.gz" |xargs head -n2
+
+# is any package minified more than once?
+find var/ -type f -name "minified_*" |xargs md5sum|cut -d" " -f1|sort|uniq -c|egrep "^\W+1\W" -v
+if [ $? -eq 0 ]; then
+  echo "FAILED: a resource is packaged multiple times"
+  failed=2
+fi
+
+exit $failed