express: Move static handlers to expressPreSession

This avoids the need to exempt the paths from authentication checks,
and it eliminates unnecessary express-session state.

CHANGELOG.md

@@ -8,6 +8,9 @@
   * `padOptions.showChat`
   * `padOptions.userColor`
   * `padOptions.userName`
+* Requests for static content (e.g., `/robots.txt`) and special pages (e.g., the
+  HTTP API, `/stats`) no longer cause the server to generate database records
+  intended to manage browser sessions (`sessionstorage:*`).
 * Fixed the return value of the `getText` HTTP API when called with a specific
   revision.
 * Fixed a potential attribute pool corruption bug with `copyPadWithoutHistory`.

src/ep.json

@@ -23,7 +23,7 @@
     {
       "name": "static",
       "hooks": {
-        "expressCreateServer": "ep_etherpad-lite/node/hooks/express/static"
+        "expressPreSession": "ep_etherpad-lite/node/hooks/express/static"
       }
     },
     {
@@ -35,13 +35,14 @@
     {
       "name": "i18n",
       "hooks": {
-        "expressCreateServer": "ep_etherpad-lite/node/hooks/i18n"
+        "expressPreSession": "ep_etherpad-lite/node/hooks/i18n"
       }
     },
     {
       "name": "specialpages",
       "hooks": {
-        "expressCreateServer": "ep_etherpad-lite/node/hooks/express/specialpages"
+        "expressCreateServer": "ep_etherpad-lite/node/hooks/express/specialpages",
+        "expressPreSession": "ep_etherpad-lite/node/hooks/express/specialpages"
       }
     },
     {
@@ -53,7 +54,7 @@
     {
       "name": "apicalls",
       "hooks": {
-        "expressCreateServer": "ep_etherpad-lite/node/hooks/express/apicalls"
+        "expressPreSession": "ep_etherpad-lite/node/hooks/express/apicalls"
       }
     },
     {
@@ -79,7 +80,7 @@
     {
       "name": "tests",
       "hooks": {
-        "expressCreateServer": "ep_etherpad-lite/node/hooks/express/tests"
+        "expressPreSession": "ep_etherpad-lite/node/hooks/express/tests"
       }
     },
     {
@@ -105,7 +106,7 @@
     {
       "name": "openapi",
       "hooks": {
-        "expressCreateServer": "ep_etherpad-lite/node/hooks/express/openapi"
+        "expressPreSession": "ep_etherpad-lite/node/hooks/express/openapi"
       }
     }
   ]

src/node/hooks/express/apicalls.js

@@ -6,9 +6,9 @@ const formidable = require('formidable');
 const apiHandler = require('../../handler/APIHandler');
 const util = require('util');
 
-exports.expressCreateServer = (hookName, args, cb) => {
+exports.expressPreSession = async (hookName, {app}) => {
   // The Etherpad client side sends information about how a disconnect happened
-  args.app.post('/ep/pad/connection-diagnostic-info', (req, res) => {
+  app.post('/ep/pad/connection-diagnostic-info', (req, res) => {
     new formidable.IncomingForm().parse(req, (err, fields, files) => {
       clientLogger.info(`DIAGNOSTIC-INFO: ${fields.diagnosticInfo}`);
       res.end('OK');
@@ -23,7 +23,7 @@ exports.expressCreateServer = (hookName, args, cb) => {
   });
 
   // The Etherpad client side sends information about client side javscript errors
-  args.app.post('/jserror', (req, res, next) => {
+  app.post('/jserror', (req, res, next) => {
     (async () => {
       const data = JSON.parse(await parseJserrorForm(req));
       clientLogger.warn(`${data.msg} --`, {
@@ -38,9 +38,7 @@ exports.expressCreateServer = (hookName, args, cb) => {
   });
 
   // Provide a possibility to query the latest available API version
-  args.app.get('/api', (req, res) => {
+  app.get('/api', (req, res) => {
     res.json({currentVersion: apiHandler.latestApiVersion});
   });
-
-  return cb();
 };

src/node/hooks/express/openapi.js

@@ -540,9 +540,7 @@ const generateDefinitionForVersion = (version, style = APIPathStyle.FLAT) => {
   return definition;
 };
 
-exports.expressCreateServer = (hookName, args, cb) => {
-  const {app} = args;
-
+exports.expressPreSession = async (hookName, {app}) => {
   // create openapi-backend handlers for each api version under /api/{version}/*
   for (const version of Object.keys(apiHandler.version)) {
     // we support two different styles of api: flat + rest
@@ -690,7 +688,6 @@ exports.expressCreateServer = (hookName, args, cb) => {
       });
     }
   }
-  return cb();
 };
 
 // helper to get api root

src/node/hooks/express/specialpages.js

@@ -10,25 +10,16 @@ const settings = require('../../utils/Settings');
 const util = require('util');
 const webaccess = require('./webaccess');
 
-exports.expressCreateServer = (hookName, args, cb) => {
-  // expose current stats
-  args.app.get('/stats', (req, res) => {
+exports.expressPreSession = async (hookName, {app}) => {
+  app.get('/stats', (req, res) => {
     res.json(require('../../stats').toJSON());
   });
 
-  // serve index.html under /
-  args.app.get('/', (req, res) => {
-    res.send(eejs.require('ep_etherpad-lite/templates/index.html', {req}));
-  });
-
-  // serve javascript.html
-  args.app.get('/javascript', (req, res) => {
+  app.get('/javascript', (req, res) => {
     res.send(eejs.require('ep_etherpad-lite/templates/javascript.html', {req}));
   });
 
-
-  // serve robots.txt
-  args.app.get('/robots.txt', (req, res) => {
+  app.get('/robots.txt', (req, res) => {
     let filePath = path.join(
         settings.root,
         'src',
@@ -46,6 +37,34 @@ exports.expressCreateServer = (hookName, args, cb) => {
     });
   });
 
+  app.get('/favicon.ico', (req, res, next) => {
+    (async () => {
+      const fns = [
+        ...(settings.favicon ? [path.resolve(settings.root, settings.favicon)] : []),
+        path.join(settings.root, 'src', 'static', 'skins', settings.skinName, 'favicon.ico'),
+        path.join(settings.root, 'src', 'static', 'favicon.ico'),
+      ];
+      for (const fn of fns) {
+        try {
+          await fsp.access(fn, fs.constants.R_OK);
+        } catch (err) {
+          continue;
+        }
+        res.setHeader('Cache-Control', `public, max-age=${settings.maxAge}`);
+        await util.promisify(res.sendFile.bind(res))(fn);
+        return;
+      }
+      next();
+    })().catch((err) => next(err || new Error(err)));
+  });
+};
+
+exports.expressCreateServer = (hookName, args, cb) => {
+  // serve index.html under /
+  args.app.get('/', (req, res) => {
+    res.send(eejs.require('ep_etherpad-lite/templates/index.html', {req}));
+  });
+
   // serve pad.html under /p
   args.app.get('/p/:pad', (req, res, next) => {
     // The below might break for pads being rewritten
@@ -77,26 +96,5 @@ exports.expressCreateServer = (hookName, args, cb) => {
     }));
   });
 
-  args.app.get('/favicon.ico', (req, res, next) => {
-    (async () => {
-      const fns = [
-        ...(settings.favicon ? [path.resolve(settings.root, settings.favicon)] : []),
-        path.join(settings.root, 'src', 'static', 'skins', settings.skinName, 'favicon.ico'),
-        path.join(settings.root, 'src', 'static', 'favicon.ico'),
-      ];
-      for (const fn of fns) {
-        try {
-          await fsp.access(fn, fs.constants.R_OK);
-        } catch (err) {
-          continue;
-        }
-        res.setHeader('Cache-Control', `public, max-age=${settings.maxAge}`);
-        await util.promisify(res.sendFile.bind(res))(fn);
-        return;
-      }
-      next();
-    })().catch((err) => next(err || new Error(err)));
-  });
-
   return cb();
 };

src/node/hooks/express/static.js

@@ -28,14 +28,14 @@ const getTar = async () => {
   return tar;
 };
 
-exports.expressCreateServer = async (hookName, args) => {
+exports.expressPreSession = async (hookName, {app}) => {
   // Cache both minified and static.
   const assetCache = new CachingMiddleware();
-  args.app.all(/\/javascripts\/(.*)/, assetCache.handle.bind(assetCache));
+  app.all(/\/javascripts\/(.*)/, assetCache.handle.bind(assetCache));
 
   // Minify will serve static files compressed (minify enabled). It also has
   // file-specific hacks for ace/require-kernel/etc.
-  args.app.all('/static/:filename(*)', minify.minify);
+  app.all('/static/:filename(*)', minify.minify);
 
   // Setup middleware that will package JavaScript files served by minify for
   // CommonJS loader on the client-side.
@@ -53,12 +53,12 @@ exports.expressCreateServer = async (hookName, args) => {
   const associator = new StaticAssociator(associations);
   jsServer.setAssociator(associator);
 
-  args.app.use(jsServer.handle.bind(jsServer));
+  app.use(jsServer.handle.bind(jsServer));
 
   // serve plugin definitions
   // not very static, but served here so that client can do
   // require("pluginfw/static/js/plugin-definitions.js");
-  args.app.get('/pluginfw/plugin-definitions.json', (req, res, next) => {
+  app.get('/pluginfw/plugin-definitions.json', (req, res, next) => {
     const clientParts = plugins.parts.filter((part) => part.client_hooks != null);
     const clientPlugins = {};
     for (const name of new Set(clientParts.map((part) => part.plugin))) {

src/node/hooks/express/tests.js

@@ -29,8 +29,8 @@ const findSpecs = async (specDir) => {
   return specs;
 };
 
-exports.expressCreateServer = (hookName, args, cb) => {
-  args.app.get('/tests/frontend/frontendTestSpecs.json', (req, res, next) => {
+exports.expressPreSession = async (hookName, {app}) => {
+  app.get('/tests/frontend/frontendTestSpecs.json', (req, res, next) => {
     (async () => {
       const modules = [];
       await Promise.all(Object.entries(plugins.plugins).map(async ([plugin, def]) => {
@@ -59,14 +59,14 @@ exports.expressCreateServer = (hookName, args, cb) => {
 
   const rootTestFolder = path.join(settings.root, 'src/tests/frontend/');
 
-  args.app.get('/tests/frontend/index.html', (req, res) => {
+  app.get('/tests/frontend/index.html', (req, res) => {
     res.redirect(['./', ...req.url.split('?').slice(1)].join('?'));
   });
 
   // The regexp /[\d\D]{0,}/ is equivalent to the regexp /.*/. The Express route path used here
   // uses the more verbose /[\d\D]{0,}/ pattern instead of /.*/ because path-to-regexp v0.1.7 (the
   // version used with Express v4.x) interprets '.' and '*' differently than regexp.
-  args.app.get('/tests/frontend/:file([\\d\\D]{0,})', (req, res, next) => {
+  app.get('/tests/frontend/:file([\\d\\D]{0,})', (req, res, next) => {
     (async () => {
       let file = sanitizePathname(req.params.file);
       if (['', '.', './'].includes(file)) file = 'index.html';
@@ -74,9 +74,7 @@ exports.expressCreateServer = (hookName, args, cb) => {
     })().catch((err) => next(err || new Error(err)));
   });
 
-  args.app.get('/tests/frontend', (req, res) => {
+  app.get('/tests/frontend', (req, res) => {
     res.redirect(['./frontend/', ...req.url.split('?').slice(1)].join('?'));
   });
-
-  return cb();
 };

src/node/hooks/express/webaccess.js

@@ -9,23 +9,6 @@ const readOnlyManager = require('../../db/ReadOnlyManager');
 
 hooks.deprecationNotices.authFailure = 'use the authnFailure and authzFailure hooks instead';
 
-const staticPathsRE = new RegExp(`^/(?:${[
-  'api(?:/.*)?',
-  'favicon\\.ico',
-  'ep/pad/connection-diagnostic-info',
-  'javascript',
-  'javascripts/.*',
-  'jserror/?',
-  'locales\\.json',
-  'locales/.*',
-  'rest/.*',
-  'pluginfw/.*',
-  'robots.txt',
-  'static/.*',
-  'stats/?',
-  'tests/frontend(?:/.*)?',
-].join('|')})$`);
-
 // Promisified wrapper around hooks.aCallFirst.
 const aCallFirst = (hookName, context, pred = null) => new Promise((resolve, reject) => {
   hooks.aCallFirst(hookName, context, (err, r) => err != null ? reject(err) : resolve(r), pred);
@@ -90,7 +73,6 @@ const preAuthorize = async (req, res, next) => {
     return;
   }
   if (locals.skip) return;
-  if (staticPathsRE.test(req.path)) results.push(true);
   if (requireAdmin) {
     // Filter out all 'true' entries to prevent plugin authors from accidentally granting admin
     // privileges to the general public.

src/node/hooks/i18n.js

@@ -100,13 +100,13 @@ const generateLocaleIndex = (locales) => {
 };
 
 
-exports.expressCreateServer = (n, args, cb) => {
+exports.expressPreSession = async (hookName, {app}) => {
   // regenerate locales on server restart
   const locales = getAllLocales();
   const localeIndex = generateLocaleIndex(locales);
   exports.availableLangs = getAvailableLangs(locales);
 
-  args.app.get('/locales/:locale', (req, res) => {
+  app.get('/locales/:locale', (req, res) => {
     // works with /locale/en and /locale/en.json requests
     const locale = req.params.locale.split('.')[0];
     if (Object.prototype.hasOwnProperty.call(exports.availableLangs, locale)) {
@@ -118,11 +118,9 @@ exports.expressCreateServer = (n, args, cb) => {
     }
   });
 
-  args.app.get('/locales.json', (req, res) => {
+  app.get('/locales.json', (req, res) => {
     res.setHeader('Cache-Control', `public, max-age=${settings.maxAge}`);
     res.setHeader('Content-Type', 'application/json; charset=utf-8');
     res.send(localeIndex);
   });
-
-  return cb();
 };

src/tests/backend/specs/webaccess.js

@@ -77,6 +77,11 @@ describe(__filename, function () {
       settings.requireAuthorization = false;
       await agent.get('/admin/').auth('admin', 'admin-password').expect(200);
     });
+    it('authn authz anonymous /robots.txt -> 200', async function () {
+      settings.requireAuthentication = true;
+      settings.requireAuthorization = true;
+      await agent.get('/robots.txt').expect(200);
+    });
     it('authn authz user / -> 403', async function () {
       settings.requireAuthentication = true;
       settings.requireAuthorization = true;