diff --git a/src/node/hooks/express/webaccess.js b/src/node/hooks/express/webaccess.js index 8a183681c..3d47b0aeb 100644 --- a/src/node/hooks/express/webaccess.js +++ b/src/node/hooks/express/webaccess.js @@ -106,18 +106,22 @@ const checkAccess = async (req, res, next) => { // /////////////////////////////////////////////////////////////////////////////////////////////// let results; + let skip = false; + const preAuthorizeNext = (...args) => { skip = true; next(...args); }; try { - results = await aCallFirst('preAuthorize', {req, res, next}, + results = await aCallFirst('preAuthorize', {req, res, next: preAuthorizeNext}, // This predicate will cause aCallFirst to call the hook functions one at a time until one // of them returns a non-empty list, with an exception: If the request is for an /admin // page, truthy entries are filtered out before checking to see whether the list is empty. // This prevents plugin authors from accidentally granting admin privileges to the general // public. - (r) => (r != null && r.filter((x) => (!requireAdmin || !x)).length > 0)); + (r) => (skip || (r != null && r.filter((x) => (!requireAdmin || !x)).length > 0))); } catch (err) { httpLogger.error(`Error in preAuthorize hook: ${err.stack || err.toString()}`); - return res.status(500).send('Internal Server Error'); + if (!skip) res.status(500).send('Internal Server Error'); + return; } + if (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 diff --git a/src/tests/backend/specs/webaccess.js b/src/tests/backend/specs/webaccess.js index 7594b57e3..70a220aef 100644 --- a/src/tests/backend/specs/webaccess.js +++ b/src/tests/backend/specs/webaccess.js @@ -135,7 +135,7 @@ describe(__filename, function () { assert(!this.called); this.called = true; callOrder.push(this.id); - return cb(this.innerHandle(context.req)); + return cb(this.innerHandle(context)); } }; const handlers = {}; @@ -179,6 +179,13 @@ describe(__filename, function () { await agent.get('/').expect(403); assert.deepEqual(callOrder, ['preAuthorize_0']); }); + it('bypasses authenticate and authorize hooks when next is called', async function () { + settings.requireAuthentication = true; + settings.requireAuthorization = true; + handlers.preAuthorize[0].innerHandle = ({next}) => next(); + await agent.get('/').expect(200); + assert.deepEqual(callOrder, ['preAuthorize_0']); + }); it('bypasses authenticate and authorize hooks for static content, defers', async function () { settings.requireAuthentication = true; settings.requireAuthorization = true; @@ -251,13 +258,13 @@ describe(__filename, function () { 'authenticate_1']); }); it('does not defer if return [true], 200', async function () { - handlers.authenticate[0].innerHandle = (req) => { req.session.user = {}; return [true]; }; + handlers.authenticate[0].innerHandle = ({req}) => { req.session.user = {}; return [true]; }; await agent.get('/').expect(200); // Note: authenticate_1 was not called because authenticate_0 handled it. assert.deepEqual(callOrder, ['preAuthorize_0', 'preAuthorize_1', 'authenticate_0']); }); it('does not defer if return [false], 401', async function () { - handlers.authenticate[0].innerHandle = (req) => [false]; + handlers.authenticate[0].innerHandle = () => [false]; await agent.get('/').expect(401); // Note: authenticate_1 was not called because authenticate_0 handled it. assert.deepEqual(callOrder, ['preAuthorize_0', 'preAuthorize_1', 'authenticate_0']); @@ -355,7 +362,7 @@ describe(__filename, function () { 'authorize_0']); }); it('does not defer if return [false], 403', async function () { - handlers.authorize[0].innerHandle = (req) => [false]; + handlers.authorize[0].innerHandle = () => [false]; await agent.get('/').auth('user', 'user-password').expect(403); // Note: authorize_1 was not called because authorize_0 handled it. assert.deepEqual(callOrder, ['preAuthorize_0',