From 9eacddb70d93b7d92ca32914a976ccc4ff928c5b Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 25 Jul 2026 18:41:37 +0100 Subject: [PATCH 1/2] security(auth): regenerate session id on authentication (session fixation) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GHSA-73h9-c5xp-gfg4. Etherpad never rotated the express-session id when an anonymous session was upgraded to an authenticated one. Any auth scheme that establishes a pre-authentication session — every OIDC/OAuth RP, including the official ep_openid_connect plugin, which persists OAuth state before redirecting to the IdP — was exposed to CWE-384: an attacker who planted or captured the pre-auth cookie ends up owning the victim's authenticated (possibly admin) session after they log in. webaccess now calls req.session.regenerate() at the authentication boundary (after the authenticate hook / HTTP-Basic path establishes req.session.user), preserving the session data onto the new id. It fires only on a *fresh* login (already-authenticated requests short-circuit in Step 2) and no-ops when the store doesn't expose regenerate(). This lives in core, so it protects every SSO/auth plugin. Adds a regression test proving the session id changes across the auth boundary and that the rotated session stays authenticated. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/node/hooks/express/webaccess.ts | 36 ++++++++ src/tests/backend/specs/sessionFixation.ts | 97 ++++++++++++++++++++++ 2 files changed, 133 insertions(+) create mode 100644 src/tests/backend/specs/sessionFixation.ts diff --git a/src/node/hooks/express/webaccess.ts b/src/node/hooks/express/webaccess.ts index 890a6f37a3d..eb7f64110c3 100644 --- a/src/node/hooks/express/webaccess.ts +++ b/src/node/hooks/express/webaccess.ts @@ -22,6 +22,22 @@ const aCallFirst0 = // @ts-ignore async (hookName: string, context:any, pred = null) => (await aCallFirst(hookName, context, pred))[0]; +// Rotate the express-session id while preserving the session's data. Used at the +// authentication boundary to prevent session fixation (GHSA-73h9-c5xp-gfg4). +// The freshly minted cookie for the new id is kept; all other session data +// (notably req.session.user) is carried across onto the new session. +const regenerateSessionPreservingData = (req: any) => new Promise((resolve, reject) => { + // Session prototype methods (regenerate/save/...) are non-enumerable, so the + // spread captures only data properties. Drop `cookie` so the new session keeps + // the fresh cookie regenerate() creates. + const {cookie, ...data} = req.session; + req.session.regenerate((err: any) => { + if (err) return reject(err); + Object.assign(req.session, data); + req.session.save((saveErr: any) => saveErr != null ? reject(saveErr) : resolve()); + }); +}); + exports.normalizeAuthzLevel = (level: string|boolean) => { if (!level) return false; switch (level) { @@ -158,6 +174,11 @@ const checkAccess = async (req:any, res:any, next: Function) => { if (settings.users == null) settings.users = {}; const ctx:WebAccessTypes = {req, res, users: settings.users, next}; + // Whether this request already carried an authenticated session BEFORE the + // authenticate step runs. Already-authenticated requests are short-circuited + // in Step 2, so reaching here with no user means a *fresh* login is about to + // happen — the point at which the session id must be rotated (see below). + const wasAlreadyAuthenticated = req.session != null && req.session.user != null; // If the HTTP basic auth header is present, extract the username and password so it can be given // to authn plugins. const httpBasicAuth = req.headers.authorization && req.headers.authorization.startsWith('Basic '); @@ -206,6 +227,21 @@ const checkAccess = async (req:any, res:any, next: Function) => { httpLogger.error('authenticate hook failed to add user settings to session'); return res.status(500).send('Internal Server Error'); } + // Session fixation defense (GHSA-73h9-c5xp-gfg4): a fresh authentication just + // upgraded an anonymous session to an authenticated one. Rotate the session id + // so a pre-auth id (e.g. one an SSO plugin persisted before redirecting to the + // IdP, which an attacker may have planted or captured) can never own the + // resulting authenticated session. Skip when the session was already + // authenticated (re-authorization of an existing login) and when the session + // store doesn't expose regenerate(). + if (!wasAlreadyAuthenticated && typeof req.session.regenerate === 'function') { + try { + await regenerateSessionPreservingData(req); + } catch (err) { + httpLogger.error(`failed to regenerate session on authentication: ${err}`); + return res.status(500).send('Internal Server Error'); + } + } const {username = ''} = req.session.user; httpLogger.info( `Successful authentication from IP ${anonymizeIp(req.ip, settings.ipLogging)} ` + diff --git a/src/tests/backend/specs/sessionFixation.ts b/src/tests/backend/specs/sessionFixation.ts new file mode 100644 index 00000000000..2470b4328fa --- /dev/null +++ b/src/tests/backend/specs/sessionFixation.ts @@ -0,0 +1,97 @@ +'use strict'; + +/** + * Regression for GHSA-73h9-c5xp-gfg4 (SSO session fixation). Etherpad + * establishes a pre-authentication express-session (e.g. an SSO plugin persists + * OAuth state before redirecting to the IdP). If the session id is NOT rotated + * when the session becomes authenticated, an attacker who planted / captured the + * pre-auth cookie ends up owning the victim's authenticated session. + * + * webaccess must call req.session.regenerate() at the authentication boundary so + * the id issued after login differs from the one held before it. + */ + +const assert = require('assert').strict; +const common = require('../common'); +const plugins = require('../../../static/js/pluginfw/plugin_defs'); +import settings from '../../../node/utils/Settings'; + +const makeHook = (hookName: string, hookFn: Function) => ({ + hook_fn: hookFn, + hook_fn_name: `sessfix/${hookName}`, + hook_name: hookName, + part: {plugin: 'sessfix'}, +}); + +// Pull a raw Set-Cookie value (e.g. the signed `s%3A.` blob) by name. +const getSetCookie = (res: any, name: string): string | null => { + const arr: string[] = res.headers['set-cookie'] || []; + for (const c of arr) { + if (c.startsWith(`${name}=`)) return c.slice(name.length + 1).split(';')[0]; + } + return null; +}; + +describe(__filename, function () { + this.timeout(30000); + let agent: any; + const backup: any = {}; + + before(async function () { agent = await common.init(); }); + + beforeEach(function () { + backup.authenticate = plugins.hooks.authenticate; + backup.requireAuthentication = settings.requireAuthentication; + backup.requireAuthorization = settings.requireAuthorization; + settings.requireAuthentication = true; + settings.requireAuthorization = false; + // An authn plugin that: (a) always touches the session so express-session + // persists a cookie even pre-auth, and (b) only establishes req.session.user + // when the login header is present (simulating a completed SSO callback). + plugins.hooks.authenticate = [makeHook('authenticate', (hookName: string, ctx: any, cb: Function) => { + ctx.req.session.sessfixTouched = true; + if (ctx.req.headers['x-do-login'] === '1') { + ctx.req.session.user = {username: 'victim'}; + return cb([true]); + } + return cb([false]); + })]; + }); + + afterEach(function () { + plugins.hooks.authenticate = backup.authenticate; + settings.requireAuthentication = backup.requireAuthentication; + settings.requireAuthorization = backup.requireAuthorization; + }); + + it('rotates the session id when a pre-auth session authenticates', async function () { + // Step 1 — anonymous request establishes a server-issued pre-auth session. + const r1 = await agent.get('/').expect(401); + const preSid = getSetCookie(r1, 'express_sid'); + assert.ok(preSid, 'server issued a pre-auth session cookie'); + + // Step 2 — same session cookie, now authenticate. + const r2 = await agent.get('/') + .set('Cookie', `express_sid=${preSid}`) + .set('x-do-login', '1') + .expect(200); + const postSid = getSetCookie(r2, 'express_sid'); + assert.ok(postSid, + 'the authentication response must rotate the session cookie (Set-Cookie present)'); + assert.notEqual(postSid, preSid, + 'session id must change at the authentication boundary (session fixation)'); + }); + + it('preserves the authenticated user across the rotation', async function () { + const r1 = await agent.get('/').expect(401); + const preSid = getSetCookie(r1, 'express_sid'); + const r2 = await agent.get('/') + .set('Cookie', `express_sid=${preSid}`) + .set('x-do-login', '1') + .expect(200); + const postSid = getSetCookie(r2, 'express_sid'); + // The rotated session must still be authenticated: a follow-up request with + // the NEW cookie (and no login header) is authorized, not bounced to 401. + await agent.get('/').set('Cookie', `express_sid=${postSid}`).expect(200); + }); +}); From f4f211810b4b81bc9f60ba5991a9178e9a0191cd Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 25 Jul 2026 19:02:41 +0100 Subject: [PATCH 2/2] security(auth): rotate session on identity/privilege change, not just anon->auth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Qodo review on #8074. The first cut only regenerated when the request started with no session.user, so a privilege upgrade reaching the authenticate step for an already-authenticated session (e.g. non-admin -> admin re-authentication) would NOT rotate the id, leaving a fixation window on the privilege change. Rotate whenever authentication changes the principal — anonymous -> user, or a username / is_admin change — while still leaving a no-op re-auth of the same principal alone (no per-request churn). Adds a deterministic test for the non-admin -> admin rotation. Also derives the session cookie name from settings.cookie.prefix in the test instead of hardcoding 'express_sid'. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/node/hooks/express/webaccess.ts | 31 ++++++++------ src/tests/backend/specs/sessionFixation.ts | 50 +++++++++++++++++++--- 2 files changed, 61 insertions(+), 20 deletions(-) diff --git a/src/node/hooks/express/webaccess.ts b/src/node/hooks/express/webaccess.ts index eb7f64110c3..440cb352553 100644 --- a/src/node/hooks/express/webaccess.ts +++ b/src/node/hooks/express/webaccess.ts @@ -174,11 +174,11 @@ const checkAccess = async (req:any, res:any, next: Function) => { if (settings.users == null) settings.users = {}; const ctx:WebAccessTypes = {req, res, users: settings.users, next}; - // Whether this request already carried an authenticated session BEFORE the - // authenticate step runs. Already-authenticated requests are short-circuited - // in Step 2, so reaching here with no user means a *fresh* login is about to - // happen — the point at which the session id must be rotated (see below). - const wasAlreadyAuthenticated = req.session != null && req.session.user != null; + // Identity carried by the session BEFORE the authenticate step runs. Used + // below to decide whether authentication changed the principal (anonymous -> + // user, or a privilege/identity change such as non-admin -> admin), which is + // the point at which the session id must be rotated (see below). + const prevUser = req.session != null ? req.session.user : null; // If the HTTP basic auth header is present, extract the username and password so it can be given // to authn plugins. const httpBasicAuth = req.headers.authorization && req.headers.authorization.startsWith('Basic '); @@ -227,14 +227,19 @@ const checkAccess = async (req:any, res:any, next: Function) => { httpLogger.error('authenticate hook failed to add user settings to session'); return res.status(500).send('Internal Server Error'); } - // Session fixation defense (GHSA-73h9-c5xp-gfg4): a fresh authentication just - // upgraded an anonymous session to an authenticated one. Rotate the session id - // so a pre-auth id (e.g. one an SSO plugin persisted before redirecting to the - // IdP, which an attacker may have planted or captured) can never own the - // resulting authenticated session. Skip when the session was already - // authenticated (re-authorization of an existing login) and when the session - // store doesn't expose regenerate(). - if (!wasAlreadyAuthenticated && typeof req.session.regenerate === 'function') { + // Session fixation defense (GHSA-73h9-c5xp-gfg4): rotate the session id + // whenever authentication changed the principal — an anonymous session + // becoming authenticated, OR an authenticated session changing identity or + // privilege level (e.g. non-admin -> admin re-authentication). This prevents a + // pre-auth / lower-privilege id (which an attacker may have planted or + // captured — e.g. one an SSO plugin persisted before redirecting to the IdP) + // from owning the resulting session. A no-op re-authentication of the same + // principal is left alone (no churn), and the rotation is skipped when the + // session store doesn't expose regenerate(). + const identityChanged = prevUser == null || + prevUser.username !== req.session.user.username || + !!prevUser.is_admin !== !!req.session.user.is_admin; + if (identityChanged && typeof req.session.regenerate === 'function') { try { await regenerateSessionPreservingData(req); } catch (err) { diff --git a/src/tests/backend/specs/sessionFixation.ts b/src/tests/backend/specs/sessionFixation.ts index 2470b4328fa..26a0c88266d 100644 --- a/src/tests/backend/specs/sessionFixation.ts +++ b/src/tests/backend/specs/sessionFixation.ts @@ -23,6 +23,9 @@ const makeHook = (hookName: string, hookFn: Function) => ({ part: {plugin: 'sessfix'}, }); +// Etherpad names the session cookie `${cookie.prefix}express_sid`. +const sessionCookieName = () => `${settings.cookie.prefix || ''}express_sid`; + // Pull a raw Set-Cookie value (e.g. the signed `s%3A.` blob) by name. const getSetCookie = (res: any, name: string): string | null => { const arr: string[] = res.headers['set-cookie'] || []; @@ -67,31 +70,64 @@ describe(__filename, function () { it('rotates the session id when a pre-auth session authenticates', async function () { // Step 1 — anonymous request establishes a server-issued pre-auth session. const r1 = await agent.get('/').expect(401); - const preSid = getSetCookie(r1, 'express_sid'); + const preSid = getSetCookie(r1, sessionCookieName()); assert.ok(preSid, 'server issued a pre-auth session cookie'); // Step 2 — same session cookie, now authenticate. const r2 = await agent.get('/') - .set('Cookie', `express_sid=${preSid}`) + .set('Cookie', `${sessionCookieName()}=${preSid}`) .set('x-do-login', '1') .expect(200); - const postSid = getSetCookie(r2, 'express_sid'); + const postSid = getSetCookie(r2, sessionCookieName()); assert.ok(postSid, 'the authentication response must rotate the session cookie (Set-Cookie present)'); assert.notEqual(postSid, preSid, 'session id must change at the authentication boundary (session fixation)'); }); + it('rotates the session id on a privilege upgrade (non-admin -> admin)', async function () { + // Reach Step 3 (authenticate) for an ALREADY-authenticated session by making + // authorization deny unless an `x-authz` header is present. + settings.requireAuthorization = true; + const authzBackup = plugins.hooks.authorize; + plugins.hooks.authorize = [makeHook('authorize', (hookName: string, ctx: any, cb: Function) => + cb([ctx.req.headers['x-authz'] === '1']))]; + plugins.hooks.authenticate = [makeHook('authenticate', (hookName: string, ctx: any, cb: Function) => { + const login = ctx.req.headers['x-login']; + if (login === 'user') { ctx.req.session.user = {username: 'u', is_admin: false}; return cb([true]); } + if (login === 'admin') { ctx.req.session.user = {username: 'u', is_admin: true}; return cb([true]); } + return cb([false]); + })]; + try { + // Log in as a non-admin (authorization granted via x-authz). + const r1 = await agent.get('/').set('x-login', 'user').set('x-authz', '1').expect(200); + const sid1 = getSetCookie(r1, sessionCookieName()); + assert.ok(sid1, 'non-admin login issued a session cookie'); + + // Re-authenticate as admin carrying sid1 and NO x-authz, so Step 2 + // authorization denies and Step 3 runs the privilege upgrade. + const r2 = await agent.get('/') + .set('Cookie', `${sessionCookieName()}=${sid1}`) + .set('x-login', 'admin') + .expect(200); + const sid2 = getSetCookie(r2, sessionCookieName()); + assert.ok(sid2, 'the privilege upgrade rotated the session cookie'); + assert.notEqual(sid2, sid1, 'session id must change on a privilege upgrade'); + } finally { + plugins.hooks.authorize = authzBackup; + } + }); + it('preserves the authenticated user across the rotation', async function () { const r1 = await agent.get('/').expect(401); - const preSid = getSetCookie(r1, 'express_sid'); + const preSid = getSetCookie(r1, sessionCookieName()); const r2 = await agent.get('/') - .set('Cookie', `express_sid=${preSid}`) + .set('Cookie', `${sessionCookieName()}=${preSid}`) .set('x-do-login', '1') .expect(200); - const postSid = getSetCookie(r2, 'express_sid'); + const postSid = getSetCookie(r2, sessionCookieName()); // The rotated session must still be authenticated: a follow-up request with // the NEW cookie (and no login header) is authorized, not bounced to 401. - await agent.get('/').set('Cookie', `express_sid=${postSid}`).expect(200); + await agent.get('/').set('Cookie', `${sessionCookieName()}=${postSid}`).expect(200); }); });