Changing the password neither regenerates the current session nor invalidates any other one #278

New Issue

clawbot · 2026-08-24T02:48:41+02:00

clawbot commented

2026-08-24 02:48:41 +02:00

Two halves of the same gap, found independently by two lanes of the 2026-08-24 audit.

The current session is not regenerated. Found during the review of #276: the password-change flow writes no session cookie at all (internal/handlers/profile.go only reads). Login DOES regenerate — session.Regenerate is called there, which is what closed #38 — so the mechanism exists and this path simply does not use it.

No other session is invalidated. Found by the security lane, verified by execution: with two live sessions, changing the password through the UI succeeded (new password logs in 303, old password 401 with no cookie issued) and the OTHER session's cookie still returned 200 afterwards. The store is gorilla/sessions CookieStore, which is stateless, so nothing server-side tracks or revokes a session.

Why it matters together: changing a password is what an operator does when they believe a credential is exposed. Today that action does not end the exposure. A captured cookie stays valid to the 24 h idle / 7-day absolute bound, and the only remediation available is deleting session_key from settings and restarting — which is not documented anywhere.

Not milestoned: single-operator, self-hosted, and the attacker needs a stolen cookie to begin with. But it is the wrong behaviour for the one action whose entire purpose is revoking access.

Definition of done

Changing the password regenerates the acting session, so the credential in the operator's own browser is replaced. This is the cheap half — the mechanism already exists at login.
Changing the password invalidates OTHER outstanding sessions. This is the real design question: CookieStore is stateless, so it needs either server-side session tracking or a value mixed into the cookie's signing/validation that changes on password change (a per-user token bumped on change is the usual shape and requires no session table). Choose deliberately, state the choice and its cost in the PR body, and note that rotating the global session_key is NOT acceptable — it would log out every user and break sessions on unrelated changes.
Whatever is chosen must not log the operator out of the session they are currently using to make the change; regenerating it is the point, ending it is not.
If the second half is judged too large for one unit, implement the regeneration half and file the revocation half separately with the design question stated — do not silently ship half and close the issue.
Document the remediation path for a suspected session leak, whatever it ends up being. Right now an operator has no documented recourse at all.

Verification

make check green.
A test asserting the session cookie value changes across a password change, and that the acting session still works afterwards.
A test asserting a second, separately established session stops working after the password changes.

Two halves of the same gap, found independently by two lanes of the 2026-08-24 audit. **The current session is not regenerated.** Found during the review of https://git.eeqj.de/sneak/webhooker/pulls/276: the password-change flow writes no session cookie at all (`internal/handlers/profile.go` only reads). Login DOES regenerate — `session.Regenerate` is called there, which is what closed https://git.eeqj.de/sneak/webhooker/issues/38 — so the mechanism exists and this path simply does not use it. **No other session is invalidated.** Found by the security lane, verified by execution: with two live sessions, changing the password through the UI succeeded (new password logs in `303`, old password `401` with no cookie issued) and the OTHER session's cookie still returned `200` afterwards. The store is `gorilla/sessions` `CookieStore`, which is stateless, so nothing server-side tracks or revokes a session. Why it matters together: changing a password is what an operator does when they believe a credential is exposed. Today that action does not end the exposure. A captured cookie stays valid to the 24 h idle / 7-day absolute bound, and the only remediation available is deleting `session_key` from `settings` and restarting — which is not documented anywhere. Not milestoned: single-operator, self-hosted, and the attacker needs a stolen cookie to begin with. But it is the wrong behaviour for the one action whose entire purpose is revoking access. ## Definition of done - Changing the password regenerates the acting session, so the credential in the operator's own browser is replaced. This is the cheap half — the mechanism already exists at login. - Changing the password invalidates OTHER outstanding sessions. This is the real design question: `CookieStore` is stateless, so it needs either server-side session tracking or a value mixed into the cookie's signing/validation that changes on password change (a per-user token bumped on change is the usual shape and requires no session table). Choose deliberately, state the choice and its cost in the PR body, and note that rotating the global `session_key` is NOT acceptable — it would log out every user and break sessions on unrelated changes. - Whatever is chosen must not log the operator out of the session they are currently using to make the change; regenerating it is the point, ending it is not. - If the second half is judged too large for one unit, implement the regeneration half and file the revocation half separately with the design question stated — do not silently ship half and close the issue. - Document the remediation path for a suspected session leak, whatever it ends up being. Right now an operator has no documented recourse at all. ## Verification - `make check` green. - A test asserting the session cookie value changes across a password change, and that the acting session still works afterwards. - A test asserting a second, separately established session stops working after the password changes.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#278