From 07dba4da9e1298114b3110f0bc725f43c19c8893 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 12 Jul 2026 03:07:13 +0200 Subject: [PATCH] fix(auth): address SA4 review findings (WS epoch check, commit ordering, verify guard) Adversarial re-review of the session-epoch work surfaced: - WS auth (messages_ws) skipped the epoch check, so a revoked-but-unexpired cookie could still open the live push channel after a reset/logout-others. Now mirrors current_user: loads the user once, rejects a stale-epoch cookie before connecting. - set_password + logout_others re-stamped the cookie BEFORE db.commit(); a failed commit would strand the current session at a newer epoch than the DB and wrongly 401 it. Commit first, then re-stamp. - Welcome verify effect could double-POST the single-use token (StrictMode/remount) and flip the banner to a false 'invalid'. Fire-once useRef guard. Left as-is (low value, documented): the Plex image proxy authenticates without a DB load / epoch check (poster/art fetches only); adding one would cost a DB hit per image. --- backend/app/auth.py | 9 ++++++--- backend/app/routes/messages.py | 8 +++++++- frontend/src/components/Welcome.tsx | 10 +++++++--- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/backend/app/auth.py b/backend/app/auth.py index 59d616a..edbb458 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -809,9 +809,11 @@ def logout_others( in somewhere" / a suspected session compromise. The demo account (shared) is excluded.""" if user.is_demo: raise HTTPException(status_code=403, detail="Not available in the demo account.") + # Commit the bump BEFORE re-stamping this cookie, so a failed commit can't strand the current + # session at a newer epoch than the DB (see set_password). bump_session_epoch(user) - _remember_epoch(request, user) db.commit() + _remember_epoch(request, user) log.info("Logout-others: uid=%s (epoch=%s)", user.id, user.session_epoch) return {"ok": True} @@ -892,10 +894,11 @@ def set_password( raise HTTPException(status_code=403, detail="Current password is incorrect.") user.password_hash = hash_password(new_password) # SA4: a password change logs out every OTHER session; re-stamp this cookie at the new epoch so - # the person making the change stays signed in here. + # the person making the change stays signed in here. Commit BEFORE re-stamping so a failed commit + # can't leave the cookie at a newer epoch than the DB (which would wrongly 401 THIS session). bump_session_epoch(user) - _remember_epoch(request, user) db.commit() + _remember_epoch(request, user) log.info("Password set/changed: uid=%s", user.id) return {"ok": True} diff --git a/backend/app/routes/messages.py b/backend/app/routes/messages.py index 53a68e8..420bd58 100644 --- a/backend/app/routes/messages.py +++ b/backend/app/routes/messages.py @@ -381,7 +381,13 @@ async def messages_ws(ws: WebSocket) -> None: return db = SessionLocal() try: - ok = is_messageable_user(db.get(User, uid)) + user = db.get(User, uid) + # SA4: honour server-side session revocation on the live channel too — reject a cookie whose + # recorded epoch is behind the account's current one (mirrors current_user). Without this a + # copied cookie could keep receiving pushes after a password reset / "log out everywhere". + epochs = sess.get("epochs") or {} + epoch_ok = user is not None and int(epochs.get(str(uid), 0)) == (user.session_epoch or 0) + ok = epoch_ok and is_messageable_user(user) finally: db.close() if not ok: diff --git a/frontend/src/components/Welcome.tsx b/frontend/src/components/Welcome.tsx index a071097..446630a 100644 --- a/frontend/src/components/Welcome.tsx +++ b/frontend/src/components/Welcome.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from "react"; +import { useEffect, useRef, useState } from "react"; import { useTranslation } from "react-i18next"; import { ArrowRight, @@ -282,9 +282,13 @@ function AuthCard() { } }, []); - // Confirm a fragment verification token by POSTing it (new SB3 flow). + // Confirm a fragment verification token by POSTing it (new SB3 flow). Fire ONCE: the token is + // single-use, so a double POST (StrictMode remount / any re-render) would 400 on the second call + // and wrongly flip the banner to "invalid". + const verifyFired = useRef(false); useEffect(() => { - if (!verifyToken) return; + if (!verifyToken || verifyFired.current) return; + verifyFired.current = true; api .verifyEmail(verifyToken) .then(() => setVerify("ok"))