security(auth): regenerate session id on authentication (session fixation)#8074
Open
JohnMcLear wants to merge 2 commits into
Open
security(auth): regenerate session id on authentication (session fixation)#8074JohnMcLear wants to merge 2 commits into
JohnMcLear wants to merge 2 commits into
Conversation
…tion) 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>
PR Summary by Qodosecurity(auth): Regenerate session ID on login to prevent session fixation
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Code Review by Qodo
1. Reauth skips session rotation
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR mitigates a session fixation vulnerability (CWE-384) by rotating the Express session ID at the moment an anonymous session becomes authenticated in the core webaccess middleware, ensuring pre-auth session cookies cannot be reused to hijack authenticated (including admin) sessions.
Changes:
- Regenerates the Express session ID after successful authentication while preserving session data (
req.session.user, etc.). - Adds a backend regression test that verifies the session cookie changes on authentication and that the authenticated session remains valid after rotation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/node/hooks/express/webaccess.ts |
Regenerates the session ID at the authentication boundary to prevent session fixation while preserving session data. |
src/tests/backend/specs/sessionFixation.ts |
Adds regression coverage to ensure the session cookie rotates on login and the rotated session stays authenticated. |
Comments suppressed due to low confidence (1)
src/tests/backend/specs/sessionFixation.ts:71
- This test hard-codes the cookie name and manually constructs the Cookie header; it should use the
sidCookieName()helper so it matches the server's configured cookie prefix.
const r1 = await agent.get('/').expect(401);
const preSid = getSetCookie(r1, 'express_sid');
assert.ok(preSid, 'server issued a pre-auth session cookie');
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+14
to
+19
| 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) => ({ |
Comment on lines
+86
to
+96
| 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); | ||
| }); |
Comment on lines
+240
to
+243
| } catch (err) { | ||
| httpLogger.error(`failed to regenerate session on authentication: ${err}`); | ||
| return res.status(500).send('Internal Server Error'); | ||
| } |
… 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>
Member
Author
|
Thanks @qodo-free-for-open-source-projects — both valid. (1) The first cut only rotated on anon→auth; it now rotates whenever authentication changes the principal (anonymous→user, or username / |
Thanks for the update. The revised implementation now covers both anonymous-to-authenticated and principal/privilege changes, while avoiding unnecessary rotation for unchanged identities. The deterministic privilege-upgrade test and prefix-derived cookie name also address the testing concerns. No further issues from me. |
| // 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; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes GHSA-73h9-c5xp-gfg4 (triage, high — runtime-proven admin takeover). Verified still present on
develop:webaccess.tssetsreq.session.userwith nosession.regenerate()(0 calls tree-wide).The bug (CWE-384)
Etherpad never rotates the express-session id when an anonymous session becomes authenticated. Any auth scheme that establishes a pre-authentication session — every OIDC/OAuth RP, including the official
ep_openid_connectplugin, which persists OAuth state (callbackChecks) before redirecting to the IdP — is exposed: an attacker who plants or captures the pre-auth cookie owns the victim's authenticated (possibly admin) session after they log in.Fix
webaccesscallsreq.session.regenerate()at the authentication boundary — after theauthenticatehook (or the HTTP-Basic path) establishesreq.session.user— carrying the session data onto a freshly-minted id. It:regenerate();webaccess, so it protects every SSO/auth plugin (the root fix the advisory recommends over patching each plugin).Tests
sessionFixation.ts: a server-issued pre-auth session that then authenticates gets a differentexpress_sid, and the rotated session stays authenticated (user preserved).webaccess.tssuite (53 tests) still passes;tsc --noEmitclean.🤖 Generated with Claude Code