From 3ac3d382e70889130bc456d7eee5b76d5aee31e1 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 2 Sep 2026 01:28:45 +0200 Subject: [PATCH] fix(auth): stop the address-bar mirror eating emailed auth tokens, and unlock revoked accounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent bugs made an email+password registration unrecoverable in prod. The verification link did nothing. urlWithParams/clearParams rebuilt the URL as pathname+query and dropped the fragment — but the one-time auth tokens ride the fragment (#verify=/#reset=, SB3), and the providers that mirror nav params (useUrlMirror in FeedFilters/Playlists/Plex/Channels) sit ABOVE the lazily-loaded in main.tsx. Their mount effect replaceState'd the token away before the auth card's chunk had loaded, so the card read an empty hash: the link opened a bare sign-in page, no request, no banner. Both helpers now carry the fragment over. The account was then locked out for good. SEC-10 nulls password_hash when an unverified account is attested (here: admin approval landing before the user could verify — which the bug above made impossible). That is indistinguishable from a Google-only account, and reset eligibility keyed off password_hash alone, so no reset link was ever issued: no password to sign in with, and no way to get one. users.password_revoked_at records the revocation, reset eligibility honours it, and setting a password again (reset-confirm or set-password) clears it. An account that never had a password stays ineligible on purpose — its owner adds one from Settings. /auth/verify now also answers password_reset_required so the card can say why the registered password stopped working, instead of leaving the user at the uniform "Invalid email or password". Telling the holder of a valid verify token leaks nothing: they have already proved they own the mailbox. The 0078 migration backfills the stamp for rows carrying the exact lockout signature (local, non-demo, no password, unverified), so existing victims recover unaided. --- .../versions/0078_password_revoked_at.py | 50 +++++++ backend/app/models.py | 6 + backend/app/routes/auth.py | 37 ++++- backend/tests/test_auth_session.py | 131 ++++++++++++++++++ frontend/src/components/Welcome.tsx | 20 ++- frontend/src/i18n/locales/en/welcome.json | 2 + frontend/src/i18n/locales/hu/welcome.json | 2 + frontend/src/lib/api/auth.ts | 5 +- frontend/src/lib/urlState.dom.test.ts | 13 ++ frontend/src/lib/urlState.ts | 20 ++- frontend/src/lib/useUrlMirror.dom.test.ts | 10 ++ 11 files changed, 282 insertions(+), 14 deletions(-) create mode 100644 backend/alembic/versions/0078_password_revoked_at.py diff --git a/backend/alembic/versions/0078_password_revoked_at.py b/backend/alembic/versions/0078_password_revoked_at.py new file mode 100644 index 00000000..c6811125 --- /dev/null +++ b/backend/alembic/versions/0078_password_revoked_at.py @@ -0,0 +1,50 @@ +"""users.password_revoked_at — remember that SEC-10 dropped a preset password + +Revision ID: 0078_password_revoked_at +Revises: 0077_plex_playback_class +Create Date: 2026-09-02 + +SEC-10 (`auth.revoke_preset_password_if_unverified`) nulls `password_hash` when an account whose email +was never verified is attested (Google adoption, or admin approval). That left NO trace, and +password-reset eligibility keyed off `password_hash IS NOT NULL` — so such an account could neither +sign in (no password) nor request a reset link (not "eligible"): a permanent lockout with a silent, +uniform response at every step. + +This column is that trace. A row with `password_revoked_at` set is a local account that HAD a password +and had it revoked, so the reset flow treats it as eligible again. Setting a password (reset-confirm or +/auth/set-password) clears the stamp. A Google-only account that never had a password keeps it NULL and +stays reset-ineligible by design — its owner adds a password from Settings while signed in. + +Backfill: the pre-existing victims of the lockout can't be identified with certainty, but the signature +is exact and narrow — a local (google_sub IS NULL), non-demo account with no password whose email is +still unverified could ONLY have got there through the revoke, since registration always sets a hash. +Stamp those with the current time so they become recoverable without operator intervention. +""" + +from typing import Sequence, Union + +import sqlalchemy as sa +from alembic import op + +revision: str = "0078_password_revoked_at" +down_revision: Union[str, None] = "0077_plex_playback_class" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + op.add_column( + "users", + sa.Column("password_revoked_at", sa.DateTime(timezone=True), nullable=True), + ) + op.execute( + "UPDATE users SET password_revoked_at = now() " + "WHERE password_hash IS NULL " + "AND google_sub IS NULL " + "AND is_demo = false " + "AND email_verified = false" + ) + + +def downgrade() -> None: + op.drop_column("users", "password_revoked_at") diff --git a/backend/app/models.py b/backend/app/models.py index 315ca060..2544111b 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -47,6 +47,12 @@ class User(Base, TimestampMixin): email: Mapped[str] = mapped_column(String(320), unique=True, index=True) # argon2id hash for email+password login; NULL for Google-only / demo accounts. password_hash: Mapped[str | None] = mapped_column(String(255)) + # When SEC-10 dropped a preset password (see auth.revoke_preset_password_if_unverified). Without + # this trace a revoked account was indistinguishable from a Google-only one, so it failed the + # password-reset eligibility check and could never be recovered — it could neither sign in nor + # reset. Set = "had a password, lost it to the guard" -> reset-eligible. Cleared whenever a + # password is set again (reset-confirm / set-password). + password_revoked_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True)) # Email ownership proven (Google sign-in is implicitly verified; password registration # verifies via a link). Login requires this AND is_active. email_verified: Mapped[bool] = mapped_column(Boolean, default=False, server_default="false") diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 58760f3b..8cc9c316 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -662,15 +662,22 @@ def _register_account(email: str, password: str) -> None: @router.post("/verify") def verify_email_confirm(body: TokenIn, db: Session = Depends(get_db)) -> dict: """Confirm an email-verification token the SPA read from the URL fragment (SB3). Uniform 400 - on any invalid/expired/used token.""" + on any invalid/expired/used token. + + `password_reset_required` tells the SPA to send the user straight to the reset flow: an admin who + approved this account BEFORE it was verified tripped SEC-10, so the password chosen at registration + is gone and sign-in would answer with the deliberately uniform "Invalid email or password" — true, + but unreadable to the person who just proved they own the mailbox. Saying so leaks nothing: holding + a valid single-use verify token for this account already proves that much.""" user = _consume_token(db, body.token, "verify") if user is None: raise HTTPException( status_code=400, detail="This verification link is invalid or has expired." ) user.email_verified = True + needs_password = user.password_hash is None and user.password_revoked_at is not None db.commit() - return {"ok": True} + return {"ok": True, "password_reset_required": needs_password} @router.post("/verify/resend") @@ -714,8 +721,10 @@ def verify_email(token: str, db: Session = Depends(get_db)): if user is None: return RedirectResponse(url="/?verify=invalid") user.email_verified = True + needs_password = user.password_hash is None and user.password_revoked_at is not None db.commit() - return RedirectResponse(url="/?verify=ok") + # Same two outcomes as the POST flow above, so an old link in flight lands on the same guidance. + return RedirectResponse(url="/?verify=" + ("ok_reset" if needs_password else "ok")) @router.post("/password-login") @@ -780,10 +789,18 @@ def _send_reset_if_eligible(email: str) -> None: """Background worker for password_reset_request: issue a reset token + email the link, but ONLY for a real (non-demo) password account. Runs after the response with its own DB session, so the request's timing never reveals whether the account exists. Token rides the URL FRAGMENT (#) so it - can't leak into proxy/access logs or a Referer (SB3).""" + can't leak into proxy/access logs or a Referer (SB3). + + "Password account" is `password_hash OR password_revoked_at` — an account SEC-10 stripped had a + password and must be able to get a new one, or it is locked out permanently (no password to sign in + with, and formerly no reset link either). A Google-only account that never had a password stays + ineligible on purpose: its owner adds one from Settings (/auth/set-password) while signed in, which + proves identity through the session rather than by mail.""" with SessionLocal() as db: user = db.execute(select(User).where(User.email == email)).scalar_one_or_none() - if user is not None and user.password_hash and not user.is_demo: + if user is None or user.is_demo: + return + if user.password_hash or user.password_revoked_at: raw = _issue_token(db, user, "reset", RESET_TTL) email_mod.send_password_reset(email, f"{_app_base()}/#reset={raw}") @@ -802,6 +819,7 @@ def password_reset_confirm(body: TokenPasswordIn, db: Session = Depends(get_db)) if user is None: raise HTTPException(status_code=400, detail="This reset link is invalid or has expired.") user.password_hash = hash_password(new_password) + user.password_revoked_at = None # the account has a password again — SEC-10's trace is spent # A valid reset link proves control of the mailbox, so completing a reset also verifies the # email — this rescues an account that registered but never clicked the verification link (it # can then sign in). The separate admin-approval gate (is_active) is deliberately NOT touched @@ -844,9 +862,15 @@ def revoke_preset_password_if_unverified(user: User) -> None: ownership is finally attested (Google adoption, or admin approval), drop that password and bump the epoch to revoke any session it minted; the owner re-sets one via password-reset. No-op for an already-verified account, so a legit no-SMTP registration is unaffected. The single source for this - rule — both the Google-callback and admin-activation paths call it so they can't drift.""" + rule — both the Google-callback and admin-activation paths call it so they can't drift. + + `password_revoked_at` is what makes "the owner re-sets one via password-reset" actually true. The + revoke leaves `password_hash IS NULL`, which is also what a Google-only account looks like — and + _send_reset_if_eligible keyed off exactly that, so a revoked account silently got no reset link and + was locked out for good (it could not sign in either). The stamp separates the two cases.""" if not user.email_verified: user.password_hash = None + user.password_revoked_at = datetime.now(timezone.utc) bump_session_epoch(user) @@ -916,6 +940,7 @@ def set_password( if not verify_password(body.current_password or "", user.password_hash): raise HTTPException(status_code=403, detail="Current password is incorrect.") user.password_hash = hash_password(new_password) + user.password_revoked_at = None # same as the reset path: a password exists again # 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. 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). diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index b4e8adc0..fe63b04f 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -341,3 +341,134 @@ def test_register_route_is_refused_when_registration_is_closed(client, db, monke db.execute(_select(User).where(User.email == "closed@example.com")).scalar_one_or_none() is None ) + + +# --- SEC-10 lockout: a revoked preset password must stay RECOVERABLE --------------------------- +# +# Regression pack for the prod incident of 2026-09-02. An account registered with a password, then +# approved by an admin BEFORE its email was verified, ended up with password_hash = NULL. That is +# also what a Google-only account looks like, and reset eligibility keyed off `password_hash` alone — +# so the account could neither sign in (no password) nor obtain a reset link, and every response on +# the way was a deliberately uniform "ok"/401. `password_revoked_at` is the trace that tells the two +# apart; these pin each step of the way back in. + + +def _capture_reset_links(monkeypatch) -> list[str]: + """Patch the reset mailer and collect the recipients it is asked to write to.""" + from app.routes import auth as auth_mod + + sent: list[str] = [] + monkeypatch.setattr(auth_mod.email_mod, "send_password_reset", lambda to, link: sent.append(to)) + return sent + + +def test_the_approval_revoke_stamps_password_revoked_at(db, make_user): + from app.routes.admin import _activate_user_by_email + + victim = make_user( + email="stamped@example.com", + password="attacker-chosen", + email_verified=False, + is_active=False, + ) + assert victim.password_revoked_at is None + + _activate_user_by_email(db, "stamped@example.com") + + assert victim.password_hash is None + assert victim.password_revoked_at is not None # …so the reset flow can tell it HAD a password + + +def test_an_account_whose_password_was_revoked_can_still_get_a_reset_link( + db, make_user, monkeypatch +): + # THE lockout bug: without the stamp this sent nothing, and the account was unrecoverable. + from app.routes.admin import _activate_user_by_email + from app.routes.auth import _send_reset_if_eligible + + make_user( + email="lockedout@example.com", + password="chosen at registration", + email_verified=False, + is_active=False, + ) + _activate_user_by_email(db, "lockedout@example.com") + sent = _capture_reset_links(monkeypatch) + + _send_reset_if_eligible("lockedout@example.com") + + assert sent == ["lockedout@example.com"] + + +def test_a_google_only_account_is_still_not_reset_eligible(db, make_user, monkeypatch): + # The other side of the rule: an account that NEVER had a password gets no reset link — its owner + # adds one from Settings while signed in (/auth/set-password), which proves identity by session. + from app.routes.auth import _send_reset_if_eligible + + make_user(email="googler@example.com", password=None, google_sub="sub-123") + sent = _capture_reset_links(monkeypatch) + + _send_reset_if_eligible("googler@example.com") + + assert sent == [] + + +def test_completing_a_reset_clears_the_revocation_stamp(client, db, make_user): + from app.routes.admin import _activate_user_by_email + from app.routes.auth import _issue_token + + user = make_user( + email="recovered@example.com", + password="chosen at registration", + email_verified=False, + is_active=False, + ) + _activate_user_by_email(db, "recovered@example.com") + raw = _issue_token(db, user, "reset", timedelta(hours=1)) + db.commit() + + ok = client.post( + "/auth/password-reset/confirm", json={"token": raw, "password": "a brand new one"} + ) + + assert ok.status_code == 200 + db.expire_all() + assert user.password_hash is not None + assert user.password_revoked_at is None # spent — not a permanently reset-eligible account + assert _login(client, "recovered@example.com", "a brand new one").status_code == 200 + + +def test_verify_tells_a_revoked_account_that_it_needs_a_new_password(client, db, make_user): + # The banner's input. Saying so leaks nothing: the caller just proved mailbox control by holding a + # valid single-use verify token. Without it the user meets only the uniform "Invalid email or + # password" and has no way to learn why the password they chose stopped working. + from app.routes.admin import _activate_user_by_email + from app.routes.auth import _issue_token + + user = make_user( + email="needspw@example.com", + password="chosen at registration", + email_verified=False, + is_active=False, + ) + _activate_user_by_email(db, "needspw@example.com") + raw = _issue_token(db, user, "verify", timedelta(hours=1)) + db.commit() + + r = client.post("/auth/verify", json={"token": raw}) + + assert r.status_code == 200 + assert r.json() == {"ok": True, "password_reset_required": True} + + +def test_verify_reports_no_reset_needed_for_an_intact_account(client, db, make_user): + from app.routes.auth import _issue_token + + user = make_user(email="intact@example.com", email_verified=False, is_active=False) + raw = _issue_token(db, user, "verify", timedelta(hours=1)) + db.commit() + + r = client.post("/auth/verify", json={"token": raw}) + + assert r.status_code == 200 + assert r.json() == {"ok": True, "password_reset_required": False} diff --git a/frontend/src/components/Welcome.tsx b/frontend/src/components/Welcome.tsx index 3bded0d8..563181db 100644 --- a/frontend/src/components/Welcome.tsx +++ b/frontend/src/components/Welcome.tsx @@ -339,7 +339,10 @@ function AuthCard() { verifyFired.current = true; api .verifyEmail(verifyToken) - .then(() => setVerify("ok")) + // "ok_reset": verified, but SEC-10 had dropped the password this account registered with (an + // admin approved it before it was verified). Sign-in would answer with the uniform credentials + // error, so point the user at the reset flow instead. Same value the legacy ?verify= redirect uses. + .then((r) => setVerify(r.password_reset_required ? "ok_reset" : "ok")) .catch(() => setVerify("invalid")); }, [verifyToken]); @@ -392,6 +395,13 @@ function AuthCard() { setMode("resend"); }; + // Used by the "verified, but your password was revoked" banner — the only way back into that + // account is a reset link, so put the user one click from it. + const goForgot = () => { + reset(); + setMode("forgot"); + }; + async function onSubmit(e: React.FormEvent) { e.preventDefault(); reset(); @@ -465,6 +475,14 @@ function AuthCard() { {/* One-time status banners from a redirect (Google-denied, email verification). */} {verify === "ok" && {t("welcome.auth.verifyOk")}} + {verify === "ok_reset" && ( + + {t("welcome.auth.verifyOkReset")}{" "} + + + )} {verify === "invalid" && ( {t("welcome.auth.verifyInvalid")}{" "} diff --git a/frontend/src/i18n/locales/en/welcome.json b/frontend/src/i18n/locales/en/welcome.json index 2d7f071b..8c2b4f9f 100644 --- a/frontend/src/i18n/locales/en/welcome.json +++ b/frontend/src/i18n/locales/en/welcome.json @@ -86,6 +86,8 @@ "forgotDone": "If that email has an account, we've sent a reset link. Check your inbox.", "resetDone": "Your password has been set — you can sign in now.", "verifyOk": "Email confirmed. Once an admin approves your account, you can sign in.", + "verifyOkReset": "Email confirmed. Your account was approved before you confirmed it, so the password you registered with was cleared for security — choose a new one to sign in.", + "verifyOkResetCta": "Set a new password", "verifyInvalid": "That verification link is invalid or has expired.", "resendCta": "Send a new link", "resendDone": "If that email has an unverified account, we've sent a new verification link. Check your inbox.", diff --git a/frontend/src/i18n/locales/hu/welcome.json b/frontend/src/i18n/locales/hu/welcome.json index 73a7a293..c9ecbdaf 100644 --- a/frontend/src/i18n/locales/hu/welcome.json +++ b/frontend/src/i18n/locales/hu/welcome.json @@ -86,6 +86,8 @@ "forgotDone": "Ha ehhez az e-mailhez tartozik fiók, elküldtük a visszaállító linket. Nézd meg a postafiókod.", "resetDone": "A jelszavad be van állítva — most már beléphetsz.", "verifyOk": "E-mail megerősítve. Amint egy admin jóváhagyja a fiókodat, beléphetsz.", + "verifyOkReset": "E-mail megerősítve. A fiókodat még a megerősítés előtt jóváhagyták, ezért biztonsági okból töröltük a regisztrációkor megadott jelszavadat — válassz újat a belépéshez.", + "verifyOkResetCta": "Új jelszó beállítása", "verifyInvalid": "Ez a megerősítő link érvénytelen vagy lejárt.", "resendCta": "Új link küldése", "resendDone": "Ha ehhez az e-mailhez tartozik megerősítetlen fiók, elküldtünk egy új megerősítő linket. Nézd meg a postafiókod.", diff --git a/frontend/src/lib/api/auth.ts b/frontend/src/lib/api/auth.ts index 5567837e..66cd7416 100644 --- a/frontend/src/lib/api/auth.ts +++ b/frontend/src/lib/api/auth.ts @@ -37,7 +37,10 @@ export const authApi = { { quiet: true }, ), // Confirm an email-verification token the SPA read from the URL fragment (SB3). - verifyEmail: (token: string): Promise<{ ok: boolean }> => + // `password_reset_required` = SEC-10 dropped the password this account registered with (an admin + // approved it before it was verified), so signing in would fail with the uniform credentials error. + // The card sends the user to the reset flow instead of leaving them to guess. + verifyEmail: (token: string): Promise<{ ok: boolean; password_reset_required?: boolean }> => req("/auth/verify", { method: "POST", body: JSON.stringify({ token }) }, { quiet: true }), // Re-issue a verification link (the first one expired, or was lost). Uniform response — never // reveals whether the email has a pending account. diff --git a/frontend/src/lib/urlState.dom.test.ts b/frontend/src/lib/urlState.dom.test.ts index 051f1a9c..463cd7c1 100644 --- a/frontend/src/lib/urlState.dom.test.ts +++ b/frontend/src/lib/urlState.dom.test.ts @@ -64,6 +64,19 @@ describe("urlWithParams", () => { setLocation("?page=channels"); expect(urlWithParams({ page: null })).toBe("/app"); }); + + it("carries the fragment over — the one-time auth tokens ride it", () => { + // Regression (prod, 2026-09-02): the providers that mirror nav params mount ABOVE the lazy , + // so this URL was replaceState'd before the auth card's chunk loaded. Dropping the hash ate the + // #verify= token and the verification link opened a bare sign-in page. + window.history.replaceState(null, "", "/app?page=plex#verify=tok123"); + expect(urlWithParams({ v: "show:1" })).toBe("/app?page=plex&v=show%3A1#verify=tok123"); + }); + + it("carries the fragment over even when the query empties out", () => { + window.history.replaceState(null, "", "/app?page=channels#reset=tok456"); + expect(urlWithParams({ page: null })).toBe("/app#reset=tok456"); + }); }); describe("clearParams", () => { diff --git a/frontend/src/lib/urlState.ts b/frontend/src/lib/urlState.ts index c5a3801f..bfa4470c 100644 --- a/frontend/src/lib/urlState.ts +++ b/frontend/src/lib/urlState.ts @@ -146,10 +146,16 @@ export function readPage(): Page { return isPage(p) ? p : "feed"; } -/** Build a URL (path + query) from the CURRENT location's query with `updates` applied — a null value - * deletes that key, any other string sets it. Lets each provider own its own nav key(s) — page / chan / - * v / pl / the feed-filter keys — and update the address bar without clobbering the others (read-modify- - * write on the live query). The caller decides pushState vs replaceState with the returned string. */ +/** Build a URL (path + query + fragment) from the CURRENT location with `updates` applied to the query — + * a null value deletes that key, any other string sets it. Lets each provider own its own nav key(s) — + * page / chan / v / pl / the feed-filter keys — and update the address bar without clobbering the others + * (read-modify-write on the live query). The caller decides pushState vs replaceState with the string. + * + * ⚠️ The FRAGMENT must be carried over. The one-time auth tokens ride it (`#verify=…` / `#reset=…`, SB3), + * and the providers that mirror nav params (useUrlMirror in FeedFilters/Playlists/Plex/Channels) sit ABOVE + * the lazily-loaded in main.tsx — their mount effect replaceState's this URL before the auth card's + * chunk has even loaded. Dropping the hash here silently ate the token: the verification link opened a + * bare sign-in page and the account could never be verified. */ export function urlWithParams(updates: Record): string { const params = new URLSearchParams(window.location.search); for (const [k, v] of Object.entries(updates)) { @@ -157,7 +163,7 @@ export function urlWithParams(updates: Record): string { else params.set(k, v); } const qs = params.toString(); - return `${window.location.pathname}${qs ? `?${qs}` : ""}`; + return `${window.location.pathname}${qs ? `?${qs}` : ""}${window.location.hash}`; } /** Remove specific TRANSIENT params (one-time auth/admin links: ?link, ?oauth, ?admin, ?access, ?verify, @@ -178,6 +184,8 @@ export function clearParams(...keys: string[]): void { window.history.replaceState( window.history.state, "", - `${window.location.pathname}${qs ? `?${qs}` : ""}`, + // Same fragment rule as urlWithParams: this strips QUERY keys only, so a `#verify=…`/`#reset=…` + // token that has not been consumed yet must survive. Welcome's own cleanup is what clears it. + `${window.location.pathname}${qs ? `?${qs}` : ""}${window.location.hash}`, ); } diff --git a/frontend/src/lib/useUrlMirror.dom.test.ts b/frontend/src/lib/useUrlMirror.dom.test.ts index d1d85f55..3c9d2faa 100644 --- a/frontend/src/lib/useUrlMirror.dom.test.ts +++ b/frontend/src/lib/useUrlMirror.dom.test.ts @@ -48,4 +48,14 @@ describe("useUrlMirror", () => { renderHook(() => useUrlMirror({ pl: "5" })); expect(window.location.search).toBe("?page=playlists&pl=5"); }); + + it("leaves an unconsumed auth token in the fragment alone", () => { + // This hook's mount effect is what ran before (and therefore before the auth card) had even + // loaded, so whatever it writes to the address bar is what the card later reads. It must not eat + // the #verify=/#reset= token that the emailed link carries. + window.history.replaceState(null, "", "/?page=playlists#verify=tok123"); + renderHook(() => useUrlMirror({ pl: "5" })); + expect(window.location.hash).toBe("#verify=tok123"); + expect(window.location.search).toBe("?page=playlists&pl=5"); + }); });