From bc9e34d0ae913ea5fdb5baf84fd9cfbbf1a02c33 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 5 Sep 2026 01:35:09 +0200 Subject: [PATCH] fix(auth): close the three leftovers E9.5 logged but kept out of its diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DX-17, three one-liners the epic's canonical sweep did not reach: `routes/setup.py::setup_admin` still resolved the wizard's admin by literal `User.email ==`. The wizard can be re-run, and E9.5's premise is that two spellings of one Gmail mailbox are one person — so a second run under the other spelling would have created exactly the duplicate admin row the epic exists to prevent. `routes/admin.py::_decide` hand-rolled `db.get` + 404 where the file's newer endpoints use `get_or_404`. There were TWO such sites, not the one the review named; both are converted, per the house rule that a principle gets fixed everywhere in one sweep. `useVerifyEntry`'s `hashchange` listener recognised `#verify=` but not `#reset=`, so pasting a reset link into a tab already running the app was silently ignored — the same silence that made the verify half a bug. It now notices, and says out loud that reset links need a signed-out window; the fragment is still deliberately left in the address bar (review round 4), so signing out from that tab recovers it. DX-16: `.catch(() => setOutcome("invalid"))` could not tell a spent token from a 502 mid-deploy or a dropped connection, and by then the fragment is cleared — so a network blip was reported as "that link is no longer valid", pushing the user to request a fresh one on the strength of a false statement. Only a 400 carrying {code:"verify"} is the server's verdict now; everything else gets its own outcome, with no resend CTA, because the token is untouched and retrying it works. The rule was already written one file away for the reset-link probe: "a network blip must never be cached as 'this link is dead'". Two existing tests asserted the old conflation by rejecting with a bare Error("400"); they now reject with the real HttpError. The errors are built inside the rejection, not hoisted: mountAt calls vi.resetModules(), so an HttpError imported earlier is a different class identity and `instanceof` would quietly miss. Gate: vitest 671 -> 676, pytest 924 -> 925. --- backend/app/routes/admin.py | 8 +-- backend/app/routes/setup.py | 9 ++- backend/tests/test_email_canonical.py | 25 +++++++ frontend/src/components/VerifyBanner.tsx | 26 +++++-- frontend/src/components/Welcome.tsx | 6 ++ .../components/authCardStates.dom.test.tsx | 7 +- .../welcomeMode.render.dom.test.tsx | 17 ++++- frontend/src/i18n/locales/en/header.json | 2 + frontend/src/i18n/locales/en/welcome.json | 1 + frontend/src/i18n/locales/hu/header.json | 2 + frontend/src/i18n/locales/hu/welcome.json | 1 + frontend/src/lib/useVerifyEntry.ts | 45 ++++++++++-- frontend/src/lib/verifyEntry.dom.test.tsx | 71 +++++++++++++++++-- 13 files changed, 198 insertions(+), 22 deletions(-) diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index c1343dd0..cc651976 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -168,9 +168,7 @@ def _activate_user_by_email(db: Session, email: str) -> None: def _decide(db: Session, invite_id: int, admin: User, approved: bool) -> Invite: - inv = db.get(Invite, invite_id) - if inv is None: - raise HTTPException(status_code=404, detail="No such invite") + inv = get_or_404(db, Invite, invite_id, detail="No such invite") # Idempotency guard: only a still-pending invite can be decided. Without this a repeat/stale # approve or deny re-runs its side effects — re-activating a user, or (now that deny emails) # sending a duplicate decision mail, even flipping an already-approved invite to "denied" and @@ -361,9 +359,7 @@ def delete_invite( falls back to the env ALLOWED_EMAILS / ADMIN_EMAILS bootstrap lists once no invite exists, so for an address on those the deletion leaves access intact (and an ADMIN_EMAILS address is still promoted to admin on its next Google sign-in). The admin has to hear that, not guess it.""" - inv = db.get(Invite, invite_id) - if inv is None: - raise HTTPException(status_code=404, detail="No such invite") + inv = get_or_404(db, Invite, invite_id, detail="No such invite") if _accounts_for(db, [inv.email]): raise HTTPException( status_code=409, diff --git a/backend/app/routes/setup.py b/backend/app/routes/setup.py index 1c1b5b58..a57e0d8a 100644 --- a/backend/app/routes/setup.py +++ b/backend/app/routes/setup.py @@ -16,6 +16,7 @@ from sqlalchemy.orm import Session from app import email as email_mod from app import mailtext from app import state, sysconfig +from app.utils import canonical_email from app.auth import PASSWORD_MIN_LEN from app.db import get_db from app.models import User @@ -70,7 +71,13 @@ def setup_admin(body: SetupAdminIn, db: Session = Depends(require_setup)) -> dic raise HTTPException( status_code=400, detail=f"Password must be at least {PASSWORD_MIN_LEN} characters." ) - user = db.execute(select(User).where(User.email == email)).scalar_one_or_none() + # Resolve canonically, not by the literal string (DX-17). The wizard can be re-run, and E9.5's + # whole point is that two spellings of one Gmail mailbox are one person — matching literally here + # would let a second run under the other spelling create the DUPLICATE admin row the epic exists + # to prevent. Every other lookup went canonical in S3; this one was outside that diff. + user = db.execute( + select(User).where(User.email_canonical == canonical_email(email)) + ).scalar_one_or_none() if user is None: user = User(email=email, role="admin") db.add(user) diff --git a/backend/tests/test_email_canonical.py b/backend/tests/test_email_canonical.py index c5eca415..1fd45233 100644 --- a/backend/tests/test_email_canonical.py +++ b/backend/tests/test_email_canonical.py @@ -345,3 +345,28 @@ def test_me_reports_the_google_address_only_when_it_differs(client, db, make_use == 200 ) assert client.get("/api/me").json()["google_email"] == "me@gmail.com" + + +# --- DX-17 · the setup wizard resolves its admin canonically --------------------------------- + + +def test_rerunning_the_wizard_with_another_gmail_spelling_updates_the_same_admin(db): + """The wizard can be re-run, and E9.5's premise is that two spellings of one Gmail mailbox are one + person. Resolving by the literal string here would create a SECOND admin row — exactly the + duplicate the epic exists to prevent. This site was outside S3's sweep (DX-17). + + Calls the handler directly: `require_setup` is an instance-state gate, not the behaviour under + test, and the test fixtures present an already-configured instance.""" + from app.routes.setup import SetupAdminIn, setup_admin + + setup_admin(SetupAdminIn(email="a.b.c@gmail.com", password=_UAT_PW), db) + setup_admin(SetupAdminIn(email="abc@gmail.com", password="another good passphrase"), db) + + admins = db.execute(select(User).where(User.role == "admin")).scalars().all() + assert len(admins) == 1, [u.email for u in admins] + # The address stays AS TYPED on the row it matched; only identity is canonical. + assert admins[0].email == "a.b.c@gmail.com" + # And the second run's password is the live one — it updated, it did not fork. + from app.security import verify_password + + assert verify_password("another good passphrase", admins[0].password_hash) diff --git a/frontend/src/components/VerifyBanner.tsx b/frontend/src/components/VerifyBanner.tsx index 0d99bab2..f34d6a88 100644 --- a/frontend/src/components/VerifyBanner.tsx +++ b/frontend/src/components/VerifyBanner.tsx @@ -14,15 +14,33 @@ import type { VerifyEntry } from "../lib/useVerifyEntry"; // wrong-account case is why the address is named. export default function VerifyBanner({ entry }: { entry: VerifyEntry }) { const { t } = useTranslation(); + // A reset link opened here can only be ACTED on signed out, so say that rather than nothing — + // silence is precisely what made the verify half a bug. The token stays in the address bar, so + // signing out from this same tab picks it up. + if (!entry.outcome && entry.resetPending) { + return ( + + {t("header.verify.resetSignedOut")} + + ); + } if (!entry.outcome) return null; - const ok = entry.outcome !== "invalid"; + const ok = entry.outcome === "ok" || entry.outcome === "ok_reset"; const text = entry.outcome === "invalid" ? t("header.verify.invalid") - : entry.email - ? t("header.verify.confirmedNamed", { email: entry.email }) - : t("header.verify.confirmed"); + : entry.outcome === "unreachable" + ? t("header.verify.unreachable") + : entry.email + ? t("header.verify.confirmedNamed", { email: entry.email }) + : t("header.verify.confirmed"); return ( )} + {/* No resend CTA here, unlike the invalid banner below: the token was NOT spent — we simply + could not reach the server — so pushing the user to request a fresh link would act on a + statement we know to be false (DX-16). Retrying the same link is the correct next move. */} + {verifyOutcome === "unreachable" && ( + {t("welcome.auth.verifyUnreachable")} + )} {verifyOutcome === "invalid" && ( {t("welcome.auth.verifyInvalid")}{" "} diff --git a/frontend/src/components/authCardStates.dom.test.tsx b/frontend/src/components/authCardStates.dom.test.tsx index 9c03bdc4..59c37ade 100644 --- a/frontend/src/components/authCardStates.dom.test.tsx +++ b/frontend/src/components/authCardStates.dom.test.tsx @@ -11,7 +11,12 @@ import { describe, it, expect, afterEach, vi } from "vitest"; import { render, screen, fireEvent, cleanup, act } from "@testing-library/react"; import type { VerifyEntry } from "../lib/useVerifyEntry"; -const noVerify: VerifyEntry = { outcome: null, email: null, dismiss: () => {} }; +const noVerify: VerifyEntry = { + outcome: null, + email: null, + resetPending: false, + dismiss: () => {}, +}; afterEach(() => { cleanup(); diff --git a/frontend/src/components/welcomeMode.render.dom.test.tsx b/frontend/src/components/welcomeMode.render.dom.test.tsx index 6b9b796e..2705bf23 100644 --- a/frontend/src/components/welcomeMode.render.dom.test.tsx +++ b/frontend/src/components/welcomeMode.render.dom.test.tsx @@ -32,7 +32,12 @@ import type { VerifyEntry } from "../lib/useVerifyEntry"; // App confirms the emailed token and hands the outcome down; these tests are about the card's // sub-views, so there is nothing to report. -const noVerify: VerifyEntry = { outcome: null, email: null, dismiss: () => {} }; +const noVerify: VerifyEntry = { + outcome: null, + email: null, + resetPending: false, + dismiss: () => {}, +}; const back = () => act(() => { @@ -91,6 +96,7 @@ describe("the verification outcome handed down from App", () => { const withOutcome = (outcome: VerifyEntry["outcome"]): VerifyEntry => ({ outcome, email: "someone@example.com", + resetPending: false, dismiss: () => {}, }); @@ -112,6 +118,15 @@ describe("the verification outcome handed down from App", () => { expect(screen.getByText("welcome.auth.resendCta")).toBeTruthy(); }); + // DX-16: a transport failure is not a verdict on the token, so it must NOT offer a fresh link — + // that would act on a statement we know to be false. Retrying the same link is the way out. + it("does not offer a new link when the server was merely unreachable", async () => { + render(); + expect(await screen.findByText("welcome.auth.verifyUnreachable")).toBeTruthy(); + expect(screen.queryByText("welcome.auth.resendCta")).toBeNull(); + expect(screen.queryByText("welcome.auth.verifyInvalid")).toBeNull(); + }); + it("shows no banner when there was no token to confirm", async () => { render(); await screen.findByText("welcome.auth.signinTitle"); diff --git a/frontend/src/i18n/locales/en/header.json b/frontend/src/i18n/locales/en/header.json index b5f7eb63..256a8888 100644 --- a/frontend/src/i18n/locales/en/header.json +++ b/frontend/src/i18n/locales/en/header.json @@ -61,6 +61,8 @@ "confirmed": "Email address confirmed.", "confirmedNamed": "Email address confirmed — {{email}}.", "invalid": "That verification link is no longer valid — it has expired or was already used.", + "unreachable": "Couldn't reach the server to confirm that link, so it hasn't been used up. Try opening it again in a moment.", + "resetSignedOut": "Password-reset links open in a signed-out window. Sign out here, or open the link in a private window, and it will pick up where it left off.", "reset": "The password this account registered with was cleared for security; set a new one before signing in." } } diff --git a/frontend/src/i18n/locales/en/welcome.json b/frontend/src/i18n/locales/en/welcome.json index 0dfecd5b..4bd4a9a2 100644 --- a/frontend/src/i18n/locales/en/welcome.json +++ b/frontend/src/i18n/locales/en/welcome.json @@ -95,6 +95,7 @@ "verifyOkReset": "Email confirmed. The password this account registered with had been cleared for security — choose a new one to sign in.", "verifyOkResetCta": "Set a new password", "verifyInvalid": "That verification link is invalid or has expired.", + "verifyUnreachable": "Couldn't reach the server to confirm that link, so it hasn't been used up. Try opening it again in a moment.", "resendCta": "Send a new link", "resendDone": "If that email has an unconfirmed account, we've sent a new confirmation link. Check your inbox, and your spam folder if it hasn't arrived in a minute.", "oauthCancelled": "Google sign-in was cancelled. You can try again or use email and password.", diff --git a/frontend/src/i18n/locales/hu/header.json b/frontend/src/i18n/locales/hu/header.json index cb3b9333..005a09c2 100644 --- a/frontend/src/i18n/locales/hu/header.json +++ b/frontend/src/i18n/locales/hu/header.json @@ -61,6 +61,8 @@ "confirmed": "Megerősítettük az e-mail-címedet.", "confirmedNamed": "Megerősítettük az e-mail-címedet — {{email}}.", "invalid": "Ez a megerősítő link már nem érvényes — lejárt, vagy már felhasználták.", + "unreachable": "Nem sikerült elérni a szervert a link megerősítéséhez, így az nem is használódott el. Próbáld megnyitni újra egy pillanat múlva.", + "resetSignedOut": "A jelszó-visszaállító linkek kijelentkezett ablakban nyílnak meg. Jelentkezz ki itt, vagy nyisd meg a linket privát ablakban, és onnan folytatódik.", "reset": "A fiók regisztrációkori jelszavát biztonsági okból töröltük; belépés előtt állíts be újat." } } diff --git a/frontend/src/i18n/locales/hu/welcome.json b/frontend/src/i18n/locales/hu/welcome.json index 2bb1f998..6b8242c7 100644 --- a/frontend/src/i18n/locales/hu/welcome.json +++ b/frontend/src/i18n/locales/hu/welcome.json @@ -95,6 +95,7 @@ "verifyOkReset": "E-mail megerősítve. A fiók regisztrációkori jelszavát biztonsági okból korábban töröltük — 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.", + "verifyUnreachable": "Nem sikerült elérni a szervert a link megerősítéséhez, így az nem is használódott el. Próbáld megnyitni újra egy pillanat múlva.", "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, és a Spam mappát is, ha egy percen belül nem érkezik meg.", "oauthCancelled": "A Google-belépés megszakadt. Próbáld újra, vagy lépj be e-maillel és jelszóval.", diff --git a/frontend/src/lib/useVerifyEntry.ts b/frontend/src/lib/useVerifyEntry.ts index 11c06648..a71f83aa 100644 --- a/frontend/src/lib/useVerifyEntry.ts +++ b/frontend/src/lib/useVerifyEntry.ts @@ -1,16 +1,26 @@ import { useEffect, useRef, useState } from "react"; import { api } from "./api"; +import { HttpError } from "./api/core"; import { clearEntryUrl, entryHash } from "./entryUrl"; /** What confirming the emailed token produced. `ok_reset` means the address is confirmed but this * account's password had been revoked (SEC-10), so signing in would only meet the uniform credentials - * error — the user has to set a new one. */ -type VerifyOutcome = "ok" | "ok_reset" | "invalid"; + * error — the user has to set a new one. `unreachable` is a TRANSPORT failure, kept apart from + * `invalid` on purpose (DX-16): the fragment is already cleared by the time we know, so telling + * someone their link is spent when the server was merely restarting sends them to request a fresh one + * on the strength of a false statement. The same rule is already written one file away, for the + * reset-link probe: "a network blip must never be cached as 'this link is dead'". */ +type VerifyOutcome = "ok" | "ok_reset" | "invalid" | "unreachable"; export type VerifyEntry = { outcome: VerifyOutcome | null; /** The address that was confirmed, when the server named it. */ email: string | null; + /** A `#reset=` link opened in a tab that already holds a session. Only the signed-OUT auth card + * consumes a reset token, so nothing here can act on it — but silence is what made the verify half + * a bug, so this says out loud that the link needs a signed-out window. The fragment is deliberately + * left in the address bar (E9.5 review round 4), so signing out still recovers it. */ + resetPending: boolean; /** Dismiss the outcome — the banner is a one-time entry notice, not persistent state. */ dismiss: () => void; }; @@ -38,12 +48,18 @@ export function useVerifyEntry(): VerifyEntry { const [token, setToken] = useState(() => entryHash.get("verify")); const [outcome, setOutcome] = useState(null); const [email, setEmail] = useState(null); + const [resetPending, setResetPending] = useState(() => !!entryHash.get("reset")); const consumed = useRef(new Set()); useEffect(() => { const onHashChange = () => { - const next = new URLSearchParams(window.location.hash.replace(/^#/, "")).get("verify"); + const hash = new URLSearchParams(window.location.hash.replace(/^#/, "")); + const next = hash.get("verify"); if (next) setToken(next); + // Both halves of the mailed pair arrive the same way — pasted into a running tab, which is a + // same-document navigation the import-time snapshot never sees. The verify half got this + // listener in E9.5 S2; the reset half was left out, so it stayed silent (DX-17). + if (hash.get("reset")) setResetPending(true); }; // replaceState (how clearEntryUrl strips the fragment) does NOT fire hashchange, so this cannot // feed itself. @@ -67,8 +83,27 @@ export function useVerifyEntry(): VerifyEntry { setOutcome(r.password_reset_required ? "ok_reset" : "ok"); setEmail(r.email ?? null); }) - .catch(() => setOutcome("invalid")); + // Only the server's own verdict on the token means "invalid". `/auth/verify` refuses a spent + // or expired token with a 400 carrying {code:"verify"}; a 502 during a deploy, or a dropped + // connection (no HttpError at all), says nothing about the token and must not be reported as + // if it did. The token itself is unspent in that case — but the fragment has already been + // cleared, so the honest message is "we could not reach the server", not "request a new link". + .catch((e: unknown) => { + const spent = + e instanceof HttpError && + e.status === 400 && + (e.detailData as { code?: string } | undefined)?.code === "verify"; + setOutcome(spent ? "invalid" : "unreachable"); + }); }, [token]); - return { outcome, email, dismiss: () => setOutcome(null) }; + return { + outcome, + email, + resetPending, + dismiss: () => { + setOutcome(null); + setResetPending(false); + }, + }; } diff --git a/frontend/src/lib/verifyEntry.dom.test.tsx b/frontend/src/lib/verifyEntry.dom.test.tsx index 00d04db3..e2533ecf 100644 --- a/frontend/src/lib/verifyEntry.dom.test.tsx +++ b/frontend/src/lib/verifyEntry.dom.test.tsx @@ -30,7 +30,12 @@ async function mountAt(url: string, verifyEmail: ReturnType) { function Probe() { const entry = useVerifyEntry(); - return
{`${entry.outcome ?? "none"}:${entry.email ?? "-"}`}
; + return ( + <> +
{`${entry.outcome ?? "none"}:${entry.email ?? "-"}`}
+
{entry.resetPending ? "reset-pending" : "-"}
+ + ); } await act(async () => { render(); @@ -67,14 +72,51 @@ describe("confirming an emailed verification link", () => { expect(outcome()).toBe("ok_reset:revoked@example.com"); }); - it("reports invalid when the link is expired or already used", async () => { - const verifyEmail = vi.fn().mockRejectedValue(new Error("400")); + // Errors are built INSIDE the rejection, not hoisted above the test: mountAt calls + // vi.resetModules(), so an HttpError imported earlier would be a different class identity than the + // one the hook imports afterwards, and `instanceof` would quietly miss. + const rejectWith = (make: (E: typeof import("./api/core").HttpError) => unknown) => + vi.fn().mockImplementation(async () => { + const { HttpError } = await import("./api/core"); + throw make(HttpError); + }); + + it("reports invalid when the SERVER says the link is expired or already used", async () => { + const verifyEmail = rejectWith((E) => new E(400, "verify", { code: "verify" })); const outcome = await mountAt("/#verify=tok-dead", verifyEmail); expect(outcome()).toBe("invalid:-"); }); + // DX-16: the fragment is already cleared by the time this resolves, so reporting a transport + // failure as "that link is no longer valid" pushes the user into requesting a fresh link on the + // strength of a statement that is false — the token is untouched and retrying it works. + it("does not call the link dead on a gateway failure mid-deploy", async () => { + const verifyEmail = rejectWith((E) => new E(502, "Bad Gateway")); + + const outcome = await mountAt("/#verify=tok-live", verifyEmail); + + expect(outcome()).toBe("unreachable:-"); + }); + + it("does not call the link dead when the connection drops with no HTTP status at all", async () => { + const verifyEmail = vi.fn().mockRejectedValue(new TypeError("Failed to fetch")); + + const outcome = await mountAt("/#verify=tok-offline", verifyEmail); + + expect(outcome()).toBe("unreachable:-"); + }); + + // A 400 that is not the verify refusal is still not a verdict on the token. + it("does not call the link dead on a 400 that is not the verify refusal", async () => { + const verifyEmail = rejectWith((E) => new E(400, "nope", { code: "other" })); + + const outcome = await mountAt("/#verify=tok-other", verifyEmail); + + expect(outcome()).toBe("unreachable:-"); + }); + it("clears the one-time query flags it owns even with nothing to confirm", async () => { const verifyEmail = vi.fn(); @@ -97,6 +139,22 @@ describe("confirming an emailed verification link", () => { expect(window.location.hash).toBe("#reset=unspent"); expect(window.location.search).toBe(""); // the flag it does own still goes + expect(screen.getByTestId("reset").textContent).toBe("reset-pending"); + }); + + // DX-17: the verify half got a hashchange listener in E9.5 S2, the reset half did not — so pasting + // a reset link into a tab already running the app was silently ignored. Our mails were plain text + // until 0.78.8, which makes pasting the normal way to use one. + it("notices a reset link pasted into a tab that is already running", async () => { + await mountAt("/", vi.fn()); + expect(screen.getByTestId("reset").textContent).toBe("-"); + + await act(async () => { + window.history.replaceState(window.history.state, "", "/#reset=pasted"); + window.dispatchEvent(new HashChangeEvent("hashchange")); + }); + + expect(screen.getByTestId("reset").textContent).toBe("reset-pending"); }); // This hook runs on the FIRST commit of App, which is several commits before the setup wizard, the @@ -145,7 +203,12 @@ describe("a verification link that arrives as a fragment-only navigation", () => const verifyEmail = vi .fn() .mockResolvedValueOnce({ ok: true, email: "first@example.com" }) - .mockRejectedValueOnce(new Error("400")); + // The server's own refusal, not a bare Error: only a 400 carrying {code:"verify"} means the + // token is spent, and this case is about the SECOND outcome replacing the first. + .mockImplementationOnce(async () => { + const { HttpError } = await import("./api/core"); + throw new HttpError(400, "verify", { code: "verify" }); + }); const outcome = await mountAt("/#verify=first-token", verifyEmail); expect(outcome()).toBe("ok:first@example.com");