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.
This commit is contained in:
@@ -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")
|
||||
+47
-2
@@ -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
|
||||
|
||||
@@ -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)
|
||||
)
|
||||
|
||||
@@ -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)."""
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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())
|
||||
|
||||
@@ -75,6 +75,7 @@ CORE_MODELS = {
|
||||
"User",
|
||||
"OAuthToken",
|
||||
"AuthToken",
|
||||
"RevokedSession",
|
||||
"Invite",
|
||||
"ApiQuotaUsage",
|
||||
"QuotaEvent",
|
||||
|
||||
Reference in New Issue
Block a user