Logout does not invalidate the session; add server-side revocation #61
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Severity: HIGH
The bug
clear_session_cookie(backend/app/auth/session.py:37-38) callsresponse.delete_cookie(...),which only asks the browser to drop the cookie. The signed token stays cryptographically valid
for its full 30-day
max_age. Anyone holding a copy — from a proxy log, a shared machine, acaptured plaintext request, or an XSS exfiltration — keeps authenticating after logout.
There is no server-side session store, no token id, and no revocation list. The only way to
terminate a session is
is_active=False(no endpoint exposes it) or rotatingCIRCA_SECRET_KEY, which logs out everyone.Role changes are picked up correctly (the role is read fresh per request), but a compromised
or unwanted session cannot be ended.
Contradicts the spec
docs/circa-spec.md§4.1: "Session invalidation on logout is immediate and server-enforced"and "short-lived signed JWTs; refresh tokens stored server-side with rotation." The
implementation is a 30-day bearer token with no revocation — the opposite of both.
Why it matters here
This is a collaboration tool. When a contributor should no longer have access, there must be a
way to end their access that takes effect immediately.
Fix
Add a session table (
session_id,user_id,created_at,expires_at,revoked_at), put thesession_idin the cookie instead of the user id, and check revocation per request. Reducesession_max_age_secondsfrom 30 days to something like 8-24 hours with sliding renewal. Add anadmin action to revoke a user's sessions, and revoke automatically on role change or
deactivation.
Done when
References
backend/app/auth/session.py:22,37-38backend/app/api/routes/auth.py:76-79docs/circa-spec.md§4.1Done in
f745175.The cookie now carries the id of a
user_sessionrow (table from migration003) instead of the user id, and every request resolves that row and checksrevoked_at/expires_at. The signature is kept but no longer authorises anything — it means a forged cookie is refused by an HMAC check rather than a database lookup.Design decisions worth recording:
Two deadlines, not one. A 12-hour idle window that slides forward on use, under a hard 7-day ceiling measured from login. The ceiling is the part that matters and the issue didn't ask for it: an idle timeout alone can be held open indefinitely by an attacker who simply keeps using the stolen cookie, which defeats the point of shortening the lifetime. Renewal is throttled to the second half of the window so an authenticated
GETis not a database write, and it commits insideget_current_userbecause read-only routes never commit and would otherwise discard the extension. Both values are configurable (CIRCA_SESSION_MAX_AGE_SECONDS,CIRCA_SESSION_ABSOLUTE_MAX_AGE_SECONDS); 12h/7d is my choice, not yours — say if you'd rather trade security for fewer re-logins, since this is a family archive people use occasionally and the friction lands on them.Revocation on access change is asymmetric on purpose. Demotion and deactivation revoke; promotion does not. The role is read fresh per request, so a promoted user's new rights already apply to the session in hand — logging them out is friction with nothing behind it. Deactivation revokes even though
get_current_useralready refuses an inactive user, because without it the rows stay live and reactivating an account would silently hand back every session it had before. That case has its own test.The 401 is identical for revoked, expired, and unknown sessions. Distinguishing them tells the holder of a stale cookie whether it was deliberately killed.
Admin revoke returns a count, not session ids — an admin needs to know the action took effect, and a session id is the credential.
Logins are also swept for long-dead rows (30 days past expiry, configurable) so the table stays bounded; recently-ended sessions survive, since those are what you'd want after an incident.
Migration
004adds thesession_revokedaudit type and recreates the #67 append-only triggers, whichbatch_alter_tabledrops — see my note on #67; I confirmed the failure mode rather than assuming it.36 tests in
backend/tests/test_session_revocation.py, all against genuinely minted cookies viaanon_client— the*_clientfixtures overrideget_current_userand would have bypassed the very mechanism under test. The headline one replays a captured cookie from a second client after the legitimate user logs out.All four "done when" items are covered: replay-after-logout rejected, admin revocation, lifetime materially shorter than 30 days, tests for both.
Note for you: everyone is logged out on deploy — existing cookies carry a user id, which is not a valid session id. That is the correct outcome, just worth expecting.