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

[JohnMcLear]

Fixes GHSA-73h9-c5xp-gfg4 (triage, high — runtime-proven admin takeover). Verified still present on develop: webaccess.ts sets req.session.user with no session.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_connect plugin, 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

webaccess calls req.session.regenerate() at the authentication boundary — after the authenticate hook (or the HTTP-Basic path) establishes req.session.user — carrying the session data onto a freshly-minted id. It:

• fires only on a fresh login — already-authenticated requests short-circuit in Step 2, so reaching the authenticate step with no prior user means a genuine privilege upgrade;
• no-ops when the session store doesn't expose regenerate();
• lives in core webaccess, so it protects every SSO/auth plugin (the root fix the advisory recommends over patching each plugin).

Tests

• New sessionFixation.ts: a server-issued pre-auth session that then authenticates gets a different express_sid, and the rotated session stays authenticated (user preserved).
• Full webaccess.ts suite (53 tests) still passes; tsc --noEmit clean.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

security(auth): Regenerate session ID on login to prevent session fixation

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Rotate express-session ID after successful authentication to prevent session fixation.
• Preserve session data while keeping the fresh cookie created by regenerate().
• Add backend regression tests asserting ID rotation and authenticated session continuity.

Diagram

graph TD
  A["Client request"] --> B["webaccess.checkAccess"] --> C{"Step 2: authorized?"}
  C -->|"yes"| G["Next middleware"]
  C -->|"no"| D["Step 3: authenticate (hook/basic)"] --> E["regenerate session id"] --> F{"Step 4: authorized?"}
  F -->|"yes"| G
  F -->|"no"| H["401/403 response"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Destroy old session and create a new authenticated session

• ➕ Simple mental model: no data copying between session objects
• ➕ Guarantees pre-auth session state cannot leak into the post-auth session
• ➖ Breaks plugins/features that legitimately persist state pre-auth (e.g., OAuth state)
• ➖ Requires careful rehydration of needed fields; higher compatibility risk

2. Patch individual SSO/auth plugins to regenerate on login

• ➕ Limits change surface area in core
• ➕ Plugin can tailor what session state to preserve
• ➖ Does not protect all auth paths (missed plugins/custom auth schemes)
• ➖ Hard to enforce; easy to regress and violates advisory guidance to fix at the boundary

3. Rotate only the session ID without copying full session data

• ➕ Reduces risk of carrying over unintended pre-auth state
• ➖ Likely breaks flows that depend on pre-auth session data surviving login (common in OAuth/OIDC RPs)
• ➖ More complicated to decide which keys are safe/required across boundary

Recommendation: Keep the current core-level boundary fix: regenerate the session ID only on fresh logins and preserve existing session data (excluding the cookie) across rotation. This provides defense-in-depth for all authentication mechanisms (hooks + HTTP Basic) with lower ecosystem breakage than destroying sessions or patching plugins individually.

Files changed (2) +133 / -0

Bug fix (1) +36 / -0

webaccess.tsRegenerate session ID on fresh authentication while preserving session data +36/-0

Regenerate session ID on fresh authentication while preserving session data

• Adds a helper to rotate the express-session identifier while copying existing session data onto the new session (excluding the cookie). Tracks whether the request was already authenticated and triggers regeneration only when an anonymous session becomes authenticated, returning 500 on regeneration/save failures.

src/node/hooks/express/webaccess.ts

Tests (1) +97 / -0

sessionFixation.tsAdd regression tests for session fixation ID rotation and user preservation +97/-0

Add regression tests for session fixation ID rotation and user preservation

• Introduces a backend spec that simulates an auth plugin creating a pre-auth session, then authenticating on a subsequent request. Asserts that the Set-Cookie session value changes across the auth boundary and that the rotated session remains authorized on follow-up requests.

src/tests/backend/specs/sessionFixation.ts

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. No rotation on privilege-upgrade ✓ Resolved 🐞 Bug ⛨ Security

Description

checkAccess only regenerates when the request started with no req.session.user, but Step 3
explicitly supports re-authentication with different credentials, so an already-authenticated
session that upgrades identity/privileges (e.g. non-admin → admin for /admin-auth) will keep the
same session id. This leaves a session fixation window across an authentication/privilege boundary
that the PR intends to protect.

Code

src/node/hooks/express/webaccess.ts[R177-184]

+  // 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 ');

Relevance

⭐⭐⭐ High

Security fix aligned with PR intent; rotation should cover privilege/identity upgrade boundary, not
only anonymous→auth.

PR-#7792
PR-#7667

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Step 3 is designed to handle re-authentication with different credentials, and Step 2 can force an
already-authenticated user into Step 3 for admin-only access; but the new wasAlreadyAuthenticated
guard prevents rotation in that case, so the session id is not rotated across a privilege upgrade.

src/node/hooks/express/webaccess.ts[167-173]
src/node/hooks/express/webaccess.ts[144-151]
src/node/hooks/express/webaccess.ts[175-244]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`checkAccess()` skips session id rotation whenever `req.session.user` was already set at the start of Step 3. However, Step 3 is documented/implemented to allow re-authentication with different credentials, which can change the authenticated principal or upgrade privileges (for example, non-admin→admin on `/admin-auth`). In those cases, keeping the old session id defeats the session fixation protection across the privilege boundary.

## Issue Context
- Step 2 can fail for an already-authenticated non-admin user attempting to access an admin-only resource.
- Step 3 then authenticates again and can replace/upgrade `req.session.user`.
- Current logic only rotates when the pre-Step-3 session had no user.

## Fix approach
- Capture a minimal "pre-auth" identity snapshot before running the authenticate hook (e.g., `prevUser = req.session?.user` and fields like `username`, `is_admin`, `readOnly`).
- After authentication, compare `prevUser` vs `req.session.user`.
- Regenerate the session if:
 - `prevUser` was null/undefined (fresh login), OR
 - the authenticated principal changed (e.g., username changed), OR
 - privileges increased (e.g., `prevUser.is_admin !== true` and `newUser.is_admin === true`).
- Consider adding a regression test for the non-admin→admin transition (or any identity change) to ensure the session id rotates on privilege upgrades.

## Fix Focus Areas
- src/node/hooks/express/webaccess.ts[175-244]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Test ignores cookie prefix ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The new sessionFixation.ts hard-codes the session cookie name as 'express_sid', but Etherpad
configures express-session to use ${settings.cookie.prefix}express_sid. The test will fail under
valid configurations that set a non-empty cookie prefix.

Code

src/tests/backend/specs/sessionFixation.ts[R69-76]

+    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')

Relevance

⭐⭐⭐ High

Deterministic test hardening; repo often accepts making tests respect runtime settings/config
variations.

PR-#7647
PR-#7796

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The server-side express-session cookie name is configured to include settings.cookie.prefix, but
the test uses a fixed name and therefore does not match the runtime cookie name when a prefix is
configured.

src/node/hooks/express.ts[208-217]
src/tests/backend/specs/sessionFixation.ts[67-96]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new session fixation regression test assumes the session cookie name is always `express_sid`. In production, the cookie name is derived from configuration (`settings.cookie.prefix`), so the test is not robust to non-default cookie prefix settings.

## Issue Context
`src/node/hooks/express.ts` configures express-session cookie name as `${settings.cookie.prefix}express_sid`, so tests should compute the cookie name the same way.

## Fix approach
- In `sessionFixation.ts`, compute `const sidCookieName = `${settings.cookie.prefix || ''}express_sid`;`.
- Use `sidCookieName` in `getSetCookie()` calls and when setting the `Cookie` request header.

## Fix Focus Areas
- src/tests/backend/specs/sessionFixation.ts[26-96]
- src/node/hooks/express.ts[208-217]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📜 Skill insights (0)

1. Reauth skips session rotation 🐞 Bug ⛨ Security ⭐ New

Description

checkAccess() only regenerates the session when wasAlreadyAuthenticated is false, but Step 3
explicitly supports reauthentication with different credentials, so an authenticated session can
change identity/privilege without rotating the session id. This can leave a session-fixation window
across a privilege upgrade (for example, non-admin session upgrading to admin via /admin-auth/).

Code

src/node/hooks/express/webaccess.ts[R230-244]

+  // 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');
+    }
+  }

Evidence

Step 3 is documented to support reauthentication with different credentials, and Step 2 can route an
already-authenticated but non-admin session into Step 3 for /admin-auth/. The new regeneration
guard skips rotation whenever a session user existed pre-authentication, so a
privilege/identity-changing reauthentication can keep the same session id.

src/node/hooks/express/webaccess.ts[144-151]
src/node/hooks/express/webaccess.ts[167-173]
src/node/hooks/express/webaccess.ts[177-181]
src/node/hooks/express/webaccess.ts[230-244]
src/node/hooks/express/openapi-admin.ts[30-38]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`webaccess.checkAccess()` currently skips `req.session.regenerate()` for any request that already has `req.session.user` before authentication runs. This fails to rotate the session id when a request reauthenticates into a *different* user or elevated privileges (e.g., non-admin -> admin), which is still an authentication boundary that should invalidate any attacker-known/planted session id.

### Issue Context
Step 3 explicitly supports reauthentication with different credentials. `/admin-auth/` is documented as a route that can establish an admin session cookie.

### Fix Focus Areas
- src/node/hooks/express/webaccess.ts[177-244]

### Implementation notes
- Capture a snapshot of the pre-auth identity/privilege before running the authenticate logic (e.g., `preUsername`, `preIsAdmin`).
- After authentication, compare with the post-auth identity/privilege (`postUsername`, `postIsAdmin`).
- Regenerate when:
 - there was no authenticated user before (`preUsername == null`), **or**
 - the authenticated identity changed (`preUsername !== postUsername`), **or**
 - privilege materially increased (at minimum `preIsAdmin === false && postIsAdmin === true`).
- Keep the existing error handling (500 on regeneration failure) consistent with your security posture.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. No rotation on privilege-upgrade 🐞 Bug ⛨ Security

Description

checkAccess only regenerates when the request started with no req.session.user, but Step 3
explicitly supports re-authentication with different credentials, so an already-authenticated
session that upgrades identity/privileges (e.g. non-admin → admin for /admin-auth) will keep the
same session id. This leaves a session fixation window across an authentication/privilege boundary
that the PR intends to protect.

Code

src/node/hooks/express/webaccess.ts[R177-184]

+  // 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 ');

Relevance

⭐⭐⭐ High

Security fix aligned with PR intent; rotation should cover privilege/identity upgrade boundary, not
only anonymous→auth.

PR-#7792
PR-#7667

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Step 3 is designed to handle re-authentication with different credentials, and Step 2 can force an
already-authenticated user into Step 3 for admin-only access; but the new wasAlreadyAuthenticated
guard prevents rotation in that case, so the session id is not rotated across a privilege upgrade.

src/node/hooks/express/webaccess.ts[167-173]
src/node/hooks/express/webaccess.ts[144-151]
src/node/hooks/express/webaccess.ts[175-244]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`checkAccess()` skips session id rotation whenever `req.session.user` was already set at the start of Step 3. However, Step 3 is documented/implemented to allow re-authentication with different credentials, which can change the authenticated principal or upgrade privileges (for example, non-admin→admin on `/admin-auth`). In those cases, keeping the old session id defeats the session fixation protection across the privilege boundary.
## Issue Context
- Step 2 can fail for an already-authenticated non-admin user attempting to access an admin-only resource.
- Step 3 then authenticates again and can replace/upgrade `req.session.user`.
- Current logic only rotates when the pre-Step-3 session had no user.
## Fix approach
- Capture a minimal "pre-auth" identity snapshot before running the authenticate hook (e.g., `prevUser = req.session?.user` and fields like `username`, `is_admin`, `readOnly`).
- After authentication, compare `prevUser` vs `req.session.user`.
- Regenerate the session if:
- `prevUser` was null/undefined (fresh login), OR
- the authenticated principal changed (e.g., username changed), OR
- privileges increased (e.g., `prevUser.is_admin !== true` and `newUser.is_admin === true`).
- Consider adding a regression test for the non-admin→admin transition (or any identity change) to ensure the session id rotates on privilege upgrades.
## Fix Focus Areas
- src/node/hooks/express/webaccess.ts[175-244]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Test ignores cookie prefix 🐞 Bug ⚙ Maintainability ⭐ New

Description

sessionFixation.ts hard-codes the session cookie name as express_sid, but the express-session
middleware sets the cookie name to ${settings.cookie.prefix}express_sid. This makes the new
regression test fail or become misleading under non-empty cookie prefixes.

Code

src/tests/backend/specs/sessionFixation.ts[R67-92]

+  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');

Evidence

The application configures the session cookie name with a configurable prefix, but the new test only
looks for and sends an unprefixed express_sid cookie. Other existing tests already account for
settings.cookie.prefix.

src/node/hooks/express.ts[208-217]
src/tests/backend/specs/sessionFixation.ts[67-96]
src/tests/backend/specs/sessionIdCookie.ts[26-27]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new `sessionFixation` spec assumes the session cookie name is always `express_sid`, but production config prefixes it with `settings.cookie.prefix`. The test should derive the cookie name from settings to remain valid when prefixing is enabled.

### Issue Context
Core express-session config uses `name: `${settings.cookie.prefix}express_sid``.

### Fix Focus Areas
- src/tests/backend/specs/sessionFixation.ts[67-96]

### Implementation notes
- Add a helper similar to other tests:
 - `const cookiePrefix = () => settings.cookie?.prefix || '';`
 - `const sidCookieName = `${cookiePrefix()}express_sid`;`
- Replace all uses of `'express_sid'` and `Cookie: express_sid=...` with `sidCookieName`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[Copilot]

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.

[JohnMcLear]

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 / is_admin change), so a non-admin→admin re-auth reaching Step 3 also rotates — with a deterministic test. A no-op re-auth of the same principal is left alone (no churn). (2) The test now derives the cookie name from settings.cookie.prefix instead of hardcoding express_sid.

[qodo-free-for-open-source-projects]

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 / is_admin change), so a non-admin→admin re-auth reaching Step 3 also rotates — with a deterministic test. A no-op re-auth of the same principal is left alone (no churn). (2) The test now derives the cookie name from settings.cookie.prefix instead of hardcoding express_sid.

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.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.