fix(auth): close the three leftovers E9.5 logged but kept out of its diff

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.
2026-09-05 01:35:09 +02:00
parent 0cc450d1c9
commit bc9e34d0ae
13 changed files with 198 additions and 22 deletions
@@ -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,
@@ -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)
@@ -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)
@@ -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 (
+      <Banner
+        tone="warning"
+        icon={AlertTriangle}
+        role="status"
+        onDismiss={entry.dismiss}
+        dismissTitle={t("header.banner.dismiss")}
+      >
+        {t("header.verify.resetSignedOut")}
+      </Banner>
+    );
+  }
  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 (
    <Banner
@@ -555,6 +555,12 @@ function AuthCard({ verify }: { verify: VerifyEntry }) {
          </button>
        </Banner>
      )}
+      {/* 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" && (
+        <Banner tone="warn">{t("welcome.auth.verifyUnreachable")}</Banner>
+      )}
      {verifyOutcome === "invalid" && (
        <Banner tone="warn">
          {t("welcome.auth.verifyInvalid")}{" "}
@@ -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();
@@ -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(<Welcome verify={withOutcome("unreachable")} />);
+    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(<Welcome verify={noVerify} />);
    await screen.findByText("welcome.auth.signinTitle");
@@ -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."
  }
 }
@@ -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.",
@@ -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."
  }
 }
@@ -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.",
@@ -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<string | null>(() => entryHash.get("verify"));
  const [outcome, setOutcome] = useState<VerifyOutcome | null>(null);
  const [email, setEmail] = useState<string | null>(null);
+  const [resetPending, setResetPending] = useState<boolean>(() => !!entryHash.get("reset"));
  const consumed = useRef(new Set<string>());

  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);
+    },
+  };
 }
@@ -30,7 +30,12 @@ async function mountAt(url: string, verifyEmail: ReturnType<typeof vi.fn>) {

  function Probe() {
    const entry = useVerifyEntry();
-    return <div data-testid="outcome">{`${entry.outcome ?? "none"}:${entry.email ?? "-"}`}</div>;
+    return (
+      <>
+        <div data-testid="outcome">{`${entry.outcome ?? "none"}:${entry.email ?? "-"}`}</div>
+        <div data-testid="reset">{entry.resetPending ? "reset-pending" : "-"}</div>
+      </>
+    );
  }
  await act(async () => {
    render(<Probe />);
@@ -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");