From 649fbdccf52cf3d1fd72f4a375dbdefd2d2b5b6a Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 17 Dec 2021 17:01:55 -0500 Subject: [PATCH] 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 | 3 ++ src/ep.json | 13 ++--- src/node/hooks/express/apicalls.js | 10 ++-- src/node/hooks/express/openapi.js | 5 +- src/node/hooks/express/specialpages.js | 66 +++++++++++++------------- src/node/hooks/express/static.js | 10 ++-- src/node/hooks/express/tests.js | 12 ++--- src/node/hooks/express/webaccess.js | 18 ------- src/node/hooks/i18n.js | 8 ++-- src/tests/backend/specs/webaccess.js | 5 ++ 10 files changed, 65 insertions(+), 85 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39ff70f3..4c0b9465 100644 --- a/CHANGELOG.md +++ b/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`. diff --git a/src/ep.json b/src/ep.json index 63942ac1..ec09696c 100644 --- a/src/ep.json +++ b/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" } } ] diff --git a/src/node/hooks/express/apicalls.js b/src/node/hooks/express/apicalls.js index a0fbbc63..834cb4a4 100644 --- a/src/node/hooks/express/apicalls.js +++ b/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(); }; diff --git a/src/node/hooks/express/openapi.js b/src/node/hooks/express/openapi.js index c4c1ccf5..c687b538 100644 --- a/src/node/hooks/express/openapi.js +++ b/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 diff --git a/src/node/hooks/express/specialpages.js b/src/node/hooks/express/specialpages.js index 66ee0221..8aab5ce4 100644 --- a/src/node/hooks/express/specialpages.js +++ b/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(); }; diff --git a/src/node/hooks/express/static.js b/src/node/hooks/express/static.js index 2b01f84c..26c18995 100644 --- a/src/node/hooks/express/static.js +++ b/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))) { diff --git a/src/node/hooks/express/tests.js b/src/node/hooks/express/tests.js index 1b1fe8f5..66b47d2a 100644 --- a/src/node/hooks/express/tests.js +++ b/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(); }; diff --git a/src/node/hooks/express/webaccess.js b/src/node/hooks/express/webaccess.js index 16d3bb49..42586b75 100644 --- a/src/node/hooks/express/webaccess.js +++ b/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. diff --git a/src/node/hooks/i18n.js b/src/node/hooks/i18n.js index 1cd663c4..76487fc5 100644 --- a/src/node/hooks/i18n.js +++ b/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(); }; diff --git a/src/tests/backend/specs/webaccess.js b/src/tests/backend/specs/webaccess.js index 70a220ae..c55c98ab 100644 --- a/src/tests/backend/specs/webaccess.js +++ b/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;