From 7f3d0e71f7a8de79f1f9d5ddab78ac49cda055fe Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sat, 18 Dec 2021 01:05:31 -0500
Subject: [PATCH] express: Check access before `expressConfigure` middleware

There are no guarantees about the order of execution of hook
functions, which means that a plugin's `expressConfigure` hook
function could theoretically register a handler/middleware before the
access check middleware is registered. If that happens, the plugin's
handler would run before the access check, which would be bad. Avoid
the problem by explicitly installing the `webaccess.checkAccess`
middleware before running the `expressConfigure` hook.
---
 src/ep.json                         | 6 ------
 src/node/hooks/express.js           | 2 ++
 src/node/hooks/express/webaccess.js | 9 ++++++---
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/src/ep.json b/src/ep.json
index b917aa1f3..63942ac17 100644
--- a/src/ep.json
+++ b/src/ep.json
@@ -50,12 +50,6 @@
         "expressCreateServer": "ep_etherpad-lite/node/hooks/express/padurlsanitize"
       }
     },
-    {
-      "name": "webaccess",
-      "hooks": {
-        "expressConfigure": "ep_etherpad-lite/node/hooks/express/webaccess"
-      }
-    },
     {
       "name": "apicalls",
       "hooks": {
diff --git a/src/node/hooks/express.js b/src/node/hooks/express.js
index 351ab5bf2..94d914009 100644
--- a/src/node/hooks/express.js
+++ b/src/node/hooks/express.js
@@ -12,6 +12,7 @@ const SessionStore = require('../db/SessionStore');
 const settings = require('../utils/Settings');
 const stats = require('../stats');
 const util = require('util');
+const webaccess = require('./express/webaccess');
 
 const logger = log4js.getLogger('http');
 let serverName;
@@ -203,6 +204,7 @@ exports.restartServer = async () => {
   app.use(exports.sessionMiddleware);
 
   app.use(cookieParser(settings.sessionKey, {}));
+  app.use(webaccess.checkAccess);
 
   await Promise.all([
     hooks.aCallAll('expressConfigure', {app}),
diff --git a/src/node/hooks/express/webaccess.js b/src/node/hooks/express/webaccess.js
index 3d47b0aeb..9ab338498 100644
--- a/src/node/hooks/express/webaccess.js
+++ b/src/node/hooks/express/webaccess.js
@@ -203,7 +203,10 @@ const checkAccess = async (req, res, next) => {
   res.status(403).send('Forbidden');
 };
 
-exports.expressConfigure = (hookName, args, cb) => {
-  args.app.use((req, res, next) => { checkAccess(req, res, next).catch(next); });
-  return cb();
+/**
+ * Express middleware to authenticate the user and check authorization. Must be installed after the
+ * express-session middleware.
+ */
+exports.checkAccess = (req, res, next) => {
+  checkAccess(req, res, next).catch((err) => next(err || new Error(err)));
 };