From 02714e4986ad5eb2819a51b51da02c486f1a8fa6 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 14 Nov 2026 17:56:36 +0100 Subject: [PATCH] fix(auth): a cookie that lands after sign-out no longer signs the browser back in A renewal answers with the session its request started with. Each session now carries an id; signing out, or signing in as someone else, revokes it (revoked_sessions, migration 0120), and a cookie carrying a revoked id is refused. A cookie from before session ids gets one drawn from the cookie itself. The race after log-out-everywhere fails closed and stays. --- .../alembic/versions/0120_revoked_sessions.py | 33 +++++++++++ backend/app/auth.py | 49 ++++++++++++++- backend/app/main.py | 2 + backend/app/models.py | 12 ++++ backend/app/routes/auth.py | 16 +++-- backend/tests/test_auth_session.py | 59 ++++++++++++++++++- backend/tests/test_unit_fences.py | 1 + 7 files changed, 164 insertions(+), 8 deletions(-) create mode 100644 backend/alembic/versions/0120_revoked_sessions.py diff --git a/backend/alembic/versions/0120_revoked_sessions.py b/backend/alembic/versions/0120_revoked_sessions.py new file mode 100644 index 00000000..e077e2f8 --- /dev/null +++ b/backend/alembic/versions/0120_revoked_sessions.py @@ -0,0 +1,33 @@ +"""revoked_sessions: a signed-out browser session's id, so a cookie that lands after the sign-out is refused + +Revision ID: 0120_revoked_sessions +Revises: 0119_plex_transcode_gpu +Create Date: 2026-11-14 + +The session slides: a request renews its cookie once a day. A request still in flight when the browser +signs out answers with the cookie it started with, and that cookie, landing after the sign-out cleared +it, signed the browser back in. Each session now carries an id (`sid`); signing out, or signing in as +someone else, stores the old id here, and `auth._session_user` refuses a cookie that carries one. +""" + +import sqlalchemy as sa +from alembic import op + +revision: str = "0120_revoked_sessions" +down_revision: str | None = "0119_plex_transcode_gpu" +branch_labels: str | None = None +depends_on: str | None = None + + +def upgrade() -> None: + op.create_table( + "revoked_sessions", + sa.Column("sid", sa.String(32), primary_key=True), + sa.Column("revoked_at", sa.DateTime(timezone=True), nullable=False), + ) + op.create_index("ix_revoked_sessions_revoked_at", "revoked_sessions", ["revoked_at"]) + + +def downgrade() -> None: + op.drop_index("ix_revoked_sessions_revoked_at", table_name="revoked_sessions") + op.drop_table("revoked_sessions") diff --git a/backend/app/auth.py b/backend/app/auth.py index 4ebebad6..baf01969 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -1,8 +1,11 @@ -from datetime import datetime, timezone +import hashlib +import secrets +from datetime import datetime, timedelta, timezone import httpx from fastapi import BackgroundTasks, Depends, HTTPException, Request from sqlalchemy import delete, func, select +from sqlalchemy.dialects.postgresql import insert as pg_insert from sqlalchemy.orm import Session from starlette.requests import HTTPConnection @@ -14,7 +17,7 @@ from app import mailtext from app import sysconfig from app.config import settings from app.db import get_db -from app.models import Invite, LogEvent, User +from app.models import Invite, LogEvent, RevokedSession, User from app.security import decrypt_optional from app.utils import canonical_email from app.refusal import refused @@ -263,6 +266,7 @@ def stamp_session(request: Request, user: User) -> None: """Record the account's current `session_epoch` in the signed cookie, so `current_user` can later reject the cookie once the epoch moves (password reset/change, or "log out everywhere").""" request.session["epoch"] = user.session_epoch or 0 + request.session.setdefault("sid", new_session_id()) request.session["renewed"] = int(datetime.now(timezone.utc).timestamp()) @@ -272,6 +276,13 @@ def stamp_session(request: Request, user: User) -> None: # after the last renewal writes a fresh stamp, which re-signs the cookie for another 14 days. Only 14 # idle days end it; "log out everywhere" (the epoch) stays the way to revoke it. Once a day, not on # every request: a Set-Cookie on each response would race between parallel requests for nothing. +# +# A renewal answers with the session its request STARTED with. Landing after a sign-out, that cookie +# would sign the browser back in, so each session carries an id (`sid`) and a sign-out revokes it +# (`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. +SESSION_MAX_AGE = 14 * 86400 # the cookie's life; `main` hands it to SessionMiddleware _RENEW_AFTER_SECONDS = 86400 @@ -282,6 +293,36 @@ def _renew_session(request: Request) -> None: request.session["renewed"] = now +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).""" + sid = request.session.get("sid") + if not isinstance(sid, str): + return + now = datetime.now(timezone.utc) + cutoff = now - timedelta(seconds=SESSION_MAX_AGE + 86400) + db.execute(delete(RevokedSession).where(RevokedSession.revoked_at < cutoff)) + db.execute(pg_insert(RevokedSession).values(sid=sid, revoked_at=now).on_conflict_do_nothing()) + + # The account a tab was built for, sent with every request it makes (`identityHeaders`, the # frontend). The cookie alone says who is asking, and one browser holds one account: a sign-in as # someone else in another tab replaces it under every open tab. A tab still showing the previous @@ -327,6 +368,10 @@ 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. + if db.get(RevokedSession, _session_id(request)) is not None: + request.session.clear() + raise refused(401, "not_authenticated") _renew_session(request) # Attribute this request's log lines to the account. Here, not in the request middleware: # this is the one place a session becomes an account, and the middleware runs outside both the diff --git a/backend/app/main.py b/backend/app/main.py index a84431fd..18115276 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -26,6 +26,7 @@ configure_logging("api") log = logs.get_logger(__name__) from app import bodylimit, collation, logsink, state, sysconfig # noqa: E402 — logging is configured first +from app import auth as auth_core # noqa: E402 — logging is configured first from app.config import settings # noqa: E402 — logging is configured first from app.db import SessionLocal, engine # noqa: E402 — logging is configured first import app.tables # noqa: E402 — every table, before the first query @@ -221,6 +222,7 @@ async def _nul_in_text(request: Request, exc: DataError) -> JSONResponse: app.add_middleware( SessionMiddleware, secret_key=settings.secret_key, + max_age=auth_core.SESSION_MAX_AGE, # the revoked-session rows outlive it (`auth.revoke_session`) same_site="lax", # required so the cookie rides the OAuth redirect back from Google https_only=settings.session_https_only, # Secure flag when served over HTTPS (prod) ) diff --git a/backend/app/models.py b/backend/app/models.py index 2eeee5ba..56d5f1be 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -190,6 +190,18 @@ class AuthToken(Base, TimestampMixin): used_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True)) +class RevokedSession(Base): + """The id of a browser session that signed out, or that a sign-in as someone else replaced + (`auth.revoke_session`). A request still in flight then can answer with that session's cookie, + re-signed by its renewal, after the sign-out cleared it; a cookie whose id is here is refused. A + row outlives every cookie that could carry its id, then `revoke_session` drops it.""" + + __tablename__ = "revoked_sessions" + + sid: Mapped[str] = mapped_column(String(32), primary_key=True) + revoked_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), index=True) + + class Invite(Base, CanonicalEmailMixin): """Access-request / whitelist row. Source of truth for who may sign in; the env ALLOWED_EMAILS/ADMIN_EMAILS act only as a bootstrap fallback (see auth.is_allowed).""" diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index b90791dd..b440af61 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -40,6 +40,7 @@ from app.auth import ( log, operator_contact, optional_current_user, + revoke_session, session_user_id, stamp_session, ) @@ -531,7 +532,7 @@ async def callback( db.flush() _store_token(db, user, token) - _establish_session(request, user) + _establish_session(db, request, user) db.commit() log.info( "signed in", @@ -641,7 +642,7 @@ def _complete_link( @router.post("/logout") -async def logout(request: Request): +def logout(request: Request, db: Session = Depends(get_db)): """Sign this browser out: the session holds one account, and it ends — unless the tab asking was built for another account than the one signed in now (a sign-in as someone else in another tab): its Sign out would end THAT person's session (see `EXPECTED_ACCOUNT_HEADER`).""" @@ -650,14 +651,19 @@ async def logout(request: Request): signed_in, request.headers.get(EXPECTED_ACCOUNT_HEADER) ): raise refused(401, "account_changed") + revoke_session(db, request) # a request still in flight must not sign it back in + db.commit() request.session.clear() return JSONResponse({"ok": True}) -def _establish_session(request: Request, user: User) -> None: +def _establish_session(db: Session, request: Request, user: User) -> None: """Sign `user` in for this browser session — the ONE sign-in path, shared by the Google callback and password login, so both stamp `last_login_at` (the caller commits). One account per session: - the sign-in replaces whoever was signed in here before.""" + the sign-in replaces whoever was signed in here before, and revokes that session's id, so a request + of theirs still in flight cannot land it back.""" + revoke_session(db, request) + request.session.pop("sid", None) request.session["user_id"] = user.id stamp_session(request, user) user.last_login_at = datetime.now(timezone.utc) @@ -1040,7 +1046,7 @@ def password_login( raise _login_refused(403, "email_unverified", user) if not user.is_active: raise _login_refused(403, "pending_approval", user) - _establish_session(request, user) + _establish_session(db, request, user) db.commit() log.info( "signed in", diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index 4dc63654..b33e5f8f 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -172,9 +172,66 @@ 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. + 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)) + r = client.get(_PROTECTED) + assert r.status_code == 200 + assert isinstance(_decode(r.cookies.get("session")).get("renewed"), int) + + +# A request still in flight when the browser signs out (or signs in as someone else) answers with the +# session it started with — re-signed, if it renewed it — and that late cookie lands after the one the +# sign-out cleared. Holding it must not sign the browser back in. +def test_a_cookie_that_lands_after_sign_out_is_refused(client, make_user): + make_user(email="late1@example.com", password=_PW) + assert _login(client, "late1@example.com", _PW).status_code == 200 + in_flight = client.cookies.get("session") + assert client.post("/auth/logout").status_code == 200 + _hold(client, in_flight) + assert client.get(_PROTECTED).status_code == 401 + assert client.get("/api/me").json() == {"authenticated": False} + + +def test_a_cookie_from_before_a_sign_in_as_another_account_is_refused(client, make_user): + make_user(email="late2a@example.com", password=_PW) + make_user(email="late2b@example.com", password=_PW) + assert _login(client, "late2a@example.com", _PW).status_code == 200 + in_flight = client.cookies.get("session") + assert _login(client, "late2b@example.com", _PW).status_code == 200 + bob = client.cookies.get("session") + _hold(client, in_flight) + assert client.get(_PROTECTED).status_code == 401 + _hold(client, bob) # Bob's own session is untouched + 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): + 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) + 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 + + def _session(client) -> dict: """The decoded payload of the client's session cookie.""" - raw = client.cookies.get("session") + return _decode(client.cookies.get("session")) + + +def _decode(raw: str | None) -> dict: + """The payload of a session cookie's value.""" if not raw: return {} data = TimestampSigner(str(settings.secret_key)).unsign(raw.encode()) diff --git a/backend/tests/test_unit_fences.py b/backend/tests/test_unit_fences.py index 985df0d2..bbe57f02 100644 --- a/backend/tests/test_unit_fences.py +++ b/backend/tests/test_unit_fences.py @@ -75,6 +75,7 @@ CORE_MODELS = { "User", "OAuthToken", "AuthToken", + "RevokedSession", "Invite", "ApiQuotaUsage", "QuotaEvent",