From 8d6aa05bc801b5d64fcff994c99c400ab0d84eee Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 14 Nov 2026 19:46:35 +0100 Subject: [PATCH] fix(auth): a cookie from before session ids signs in once more; a session's age is its renewal A sign-out sent with a cookie that had no session id revoked nothing, so a request of its still in flight signed the browser back in; its id, drawn from the raw cookie, also changed whenever the cookie was re-signed. Such a cookie is now refused, like one with no epoch: every one would have run out within 14 days anyway. And any route that writes the session re-signs the cookie (the anonymous Google sign-in start too), so a refused one could be kept fresh past the row revoking it: a session is now as old as its renewed stamp, which only a sign-in and a renewal write. --- backend/app/auth.py | 48 +++++++++++++++------------- backend/tests/test_auth_session.py | 50 +++++++++++++++++++----------- 2 files changed, 58 insertions(+), 40 deletions(-) diff --git a/backend/app/auth.py b/backend/app/auth.py index b5f3b3e4..b9df7e51 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -1,4 +1,3 @@ -import hashlib import secrets from datetime import datetime, timedelta, timezone @@ -282,14 +281,24 @@ def stamp_session(request: Request, user: User) -> None: # (`revoke_session`). Landing after "log out everywhere" or a password change re-stamped this # browser's cookie, it carries the old epoch and signs this browser out: that race fails closed (sign # in again; nothing is reached), and is left so. +# +# The session's age is its `renewed` stamp, not the cookie's signature: any route that writes the +# session re-signs the cookie (an anonymous one too — the Google sign-in start stores its state +# there), so a refused cookie could be kept fresh forever. `renewed` is written only by a sign-in and +# by a renewal, and a refused session gets neither, so it dies `SESSION_MAX_AGE` after its last one. SESSION_MAX_AGE = 14 * 86400 # the cookie's life; `main` hands it to SessionMiddleware _RENEW_AFTER_SECONDS = 86400 +def _session_expired(request: Request) -> bool: + renewed = request.session.get("renewed") + now = int(datetime.now(timezone.utc).timestamp()) + return not isinstance(renewed, int) or now - renewed > SESSION_MAX_AGE + + def _renew_session(request: Request) -> None: now = int(datetime.now(timezone.utc).timestamp()) - renewed = request.session.get("renewed") - if not isinstance(renewed, int) or now - renewed >= _RENEW_AFTER_SECONDS: + if now - request.session["renewed"] >= _RENEW_AFTER_SECONDS: request.session["renewed"] = now @@ -297,23 +306,11 @@ def new_session_id() -> str: return secrets.token_urlsafe(16) -def _session_id(request: Request) -> str: - """This session's id. A cookie from before session ids gets one drawn from the cookie itself, so - parallel requests carrying it agree on the id a sign-out revokes, and two browsers do not.""" - sid = request.session.get("sid") - if isinstance(sid, str): - return sid - raw = request.cookies.get("session", "") - sid = hashlib.sha256(raw.encode()).hexdigest()[:22] - request.session["sid"] = sid - return sid - - def revoke_session(db: Session, request: Request) -> None: """Revoke this browser's session id, so a cookie carrying it that lands later is refused. Not committed — the caller commits. A row is dropped once no cookie with its id can still be valid: - a refused cookie is never renewed, so it dies `SESSION_MAX_AGE` after it was signed, at most a - request's time after the revoke (a day of margin).""" + a refused session is never renewed, so it expires `SESSION_MAX_AGE` after its `renewed` stamp, + which is no later than the revoke (a day of margin).""" sid = request.session.get("sid") if not isinstance(sid, str): return @@ -368,11 +365,18 @@ def _session_user(request: Request, db: Session, user_id: int | None) -> User: if epoch is None or int(epoch) != (user.session_epoch or 0): request.session.clear() raise refused(401, "not_authenticated") - # A session signed out (or replaced by a sign-in as someone else) whose cookie landed late. Not - # cleared: this request may have left before a sign-in as someone else answered, and a cookie - # deletion landing after that sign-in's cookie would sign the browser out of it. A revoked id is - # refused on every request and never renewed, so leaving its cookie costs nothing. - if db.get(RevokedSession, _session_id(request)) is not None: + # A session signed out (or replaced by a sign-in as someone else) whose cookie landed late; one + # with no id, which only a build before session ids wrote (its holder signs in once more); or one + # not renewed for its whole life. Not cleared: this request may have left before a sign-in as + # someone else answered, and a cookie deletion landing after that sign-in's cookie would sign the + # browser out of it. Such a session is refused on every request and never renewed, so leaving + # its cookie costs nothing. + sid = request.session.get("sid") + if ( + not isinstance(sid, str) + or _session_expired(request) + or db.get(RevokedSession, sid) is not None + ): raise refused(401, "not_authenticated") _renew_session(request) # Attribute this request's log lines to the account. Here, not in the request middleware: diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index 971149b5..a4657432 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -172,16 +172,26 @@ def test_a_session_renewed_today_is_not_re_signed_on_every_request(client, make_ assert "set-cookie" not in r.headers -def test_a_session_with_no_renewal_stamp_is_renewed(client, make_user, monkeypatch): - # A cookie signed before the session slid carries no `renewed`: its first request stamps one. +def test_a_session_with_no_renewal_stamp_signs_in_once_more(client, make_user): + # The session's age is its `renewed` stamp; a cookie without one cannot say how old it is. make_user(email="slide3@example.com", password=_PW) assert _login(client, "slide3@example.com", _PW).status_code == 200 - payload = {k: v for k, v in _session(client).items() if k != "renewed"} - now = int(TimestampSigner(str(settings.secret_key)).get_timestamp()) - _hold(client, _signed_at(monkeypatch, now - 13 * _DAY, payload)) + _hold(client, _cookie({k: v for k, v in _session(client).items() if k != "renewed"})) r = client.get(_PROTECTED) - assert r.status_code == 200 - assert isinstance(_decode(r.cookies.get("session")).get("renewed"), int) + assert r.status_code == 401 + assert "set-cookie" not in r.headers + + +def test_a_fresh_signature_does_not_make_an_old_session_young(client, make_user): + # Any route that writes the session re-signs the cookie — an anonymous one too (the Google + # sign-in start stores its state there) — so a refused cookie could be kept freshly signed for + # ever, and outlive the row that revoked it. Its age is its `renewed` stamp instead. + make_user(email="slide4@example.com", password=_PW) + assert _login(client, "slide4@example.com", _PW).status_code == 200 + now = int(TimestampSigner(str(settings.secret_key)).get_timestamp()) + stale = {**_session(client), "renewed": now - 15 * _DAY} + _hold(client, _cookie(stale)) # signed just now + assert client.get(_PROTECTED).status_code == 401 # A request still in flight when the browser signs out (or signs in as someone else) answers with the @@ -213,19 +223,16 @@ def test_a_cookie_from_before_a_sign_in_as_another_account_is_refused(client, ma assert client.get("/api/me").json()["email"] == "late2b@example.com" -def test_a_session_from_before_session_ids_gets_one_and_its_sign_out_holds(client, make_user): +def test_a_cookie_from_before_session_ids_signs_in_once_more(client, make_user): + # Its sign-out could revoke nothing, so a request of its still in flight signed the browser back + # in. Every such cookie would have run out within 14 days anyway: its holder signs in once more. make_user(email="late3@example.com", password=_PW) assert _login(client, "late3@example.com", _PW).status_code == 200 - old = _cookie({k: v for k, v in _session(client).items() if k != "sid"}) - _hold(client, old) + _hold(client, _cookie({k: v for k, v in _session(client).items() if k != "sid"})) r = client.get(_PROTECTED) - assert r.status_code == 200 - in_flight = r.cookies.get("session") - assert _decode(in_flight).get("sid") - _hold(client, in_flight) - assert client.post("/auth/logout").status_code == 200 - _hold(client, in_flight) - assert client.get(_PROTECTED).status_code == 401 + assert r.status_code == 401 + assert "set-cookie" not in r.headers + assert client.get("/api/me").json() == {"authenticated": False} def test_a_revoke_drops_only_the_revocations_no_cookie_can_outlive(db): @@ -272,7 +279,14 @@ def test_a_google_link_lands_only_on_the_account_still_signed_in(db, make_user, from app.routes.auth import _complete_link starter, other = make_user(), make_user() - session = {"user_id": starter.id if signed_in_as == "starter" else other.id, "epoch": 0} + import time + + session = { + "user_id": starter.id if signed_in_as == "starter" else other.id, + "epoch": 0, + "sid": "a-session", + "renewed": int(time.time()), + } request = Request({"type": "http", "session": session, "headers": []}) resp = _complete_link(request, db, starter.id, True, "sub-123", {"email": ""}, {"scope": ""}) db.refresh(starter)