Mirrors/etherpad-lite
1
0
Fork 0
mirror of https://github.com/ether/etherpad-lite.git synced 2026-07-30 05:23:24 +00:00
Code Issues Projects Releases Wiki Activity

security(auth): regenerate session id on authentication (session fixation) (#8074)

Browse source 
* security(auth): regenerate session id on authentication (session fixation)

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) <noreply@anthropic.com>

* security(auth): rotate session on identity/privilege change, not just anon->auth

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) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
John McLear 2026-07-27 19:18:40 +01:00 • committed by GitHub
parent 4d95a12845
commit fa6df9a9ff
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 174 additions and 0 deletions
Download patch file Download diff file Expand all files Collapse all files

41
src/node/hooks/express/webaccess.ts
View file

@ -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<void>((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};
// 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 ');
@ -206,6 +227,26 @@ 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): 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) {
httpLogger.error(`failed to regenerate session on authentication: ${err}`);
return res.status(500).send('Internal Server Error');
}
}
const {username = '<no username>'} = req.session.user;
httpLogger.info(
`Successful authentication from IP ${anonymizeIp(req.ip, settings.ipLogging)} ` +