diff --git a/backend/app/routes/me.py b/backend/app/routes/me.py index 6de8d72d..ec625c5f 100644 --- a/backend/app/routes/me.py +++ b/backend/app/routes/me.py @@ -159,8 +159,13 @@ def delete_my_account( which all cascade on the users row. Quota-audit events are kept but anonymised (FK SET NULL). The shared demo account can't be deleted, and the last remaining admin can't delete itself (that would lock everyone out of the admin surfaces).""" + # Structured, like the other three demo refusals: `req` leaves 403 to the caller and + # `notifyActionError` discards a plain-string detail in favour of the caller's translated + # fallback, so a hand-written English sentence here reaches a Hungarian UI as a generic + # "couldn't do that" — or, on this affordance, as nothing at all. `reason` distinguishes it + # from the other sites' bare `{code: "demo"}` so the UI can say what specifically is refused. if user.is_demo: - raise HTTPException(status_code=403, detail="The shared demo account can't be deleted.") + raise HTTPException(status_code=403, detail={"code": "demo", "reason": "delete_account"}) if user.role == "admin" and count_admins(db) <= 1: raise HTTPException( status_code=400, diff --git a/backend/tests/test_demo_refusal_detail.py b/backend/tests/test_demo_refusal_detail.py index 6596c862..f629f9cf 100644 --- a/backend/tests/test_demo_refusal_detail.py +++ b/backend/tests/test_demo_refusal_detail.py @@ -7,10 +7,19 @@ of the caller's already-translated fallback, so from that commit on the demo use this gate is UI-gated on `is_demo` either — the comfort shelf is not, so the pin button is live. A structured `{code: "demo"}` is what `localizeDetail` can turn into `errors.demo` in both locales. -Pinned at all three sites: the shared dependency and the two auth routes that hand-roll the same -check, because an asymmetric fix is the smell this repo bans. Needs the DB. +Pinned at all FOUR sites: the shared dependency, the two auth routes that hand-roll the same check, +and the account-delete route in `routes/me.py` — which round 6 missed while its docstring claimed +all the sites were covered, so the next reader had no reason to look (round 7). An asymmetric fix is +the smell this repo bans, and a docstring asserting symmetry it does not have is worse than none. + +The list is derived from the source, not typed out: `test_every_is_demo_refusal_is_on_the_list` +below fails when a fifth `is_demo` 403 appears anywhere in `app/`, so this file cannot fall behind +the code the way round 6's did. Needs the DB. """ +import re +from pathlib import Path + from app.models import User @@ -20,13 +29,47 @@ def test_every_demo_refusal_carries_the_same_structured_reason(db, make_user, as for method, url, body in [ # `require_human` itself, through an affordance the UI does NOT gate on is_demo. ("post", "/api/discover/pins", {"kind": "video", "ref": "v1"}), - # The two routes that repeat the check inline rather than depending on the gate. + # The three routes that repeat the check inline rather than depending on the gate. ("post", "/auth/logout-others", None), ("post", "/auth/set-password", {"password": "a long enough password"}), + ("delete", "/api/me/account", None), ]: r = getattr(client, method)(url, **({"json": body} if body is not None else {})) assert r.status_code == 403, (url, r.text) - assert r.json()["detail"] == {"code": "demo"}, url + detail = r.json()["detail"] + # Every one of them is a dict whose `code` the frontend can localize; the delete route adds + # a `reason` so its refusal can be worded specifically. + assert isinstance(detail, dict), (url, detail) + assert detail["code"] == "demo", (url, detail) + + +def test_every_is_demo_refusal_is_on_the_list(): + """A fifth site must not be able to appear unnoticed. + + Round 6 converted three of four `is_demo` 403s and wrote a docstring saying it had converted + them all; the fourth kept answering a bare English string for another round because nothing + counted them. So count them here: every `is_demo`-guarded 403 in `app/` has to raise a + structured detail, and the number of them has to match what this file exercises.""" + app_dir = Path(__file__).resolve().parents[1] / "app" + sites = [] + for path in sorted(app_dir.rglob("*.py")): + lines = path.read_text(encoding="utf-8").splitlines() + for i, line in enumerate(lines): + # `if user.is_demo:` / `if u.is_demo:` — the hand-rolled gate, not the ORM filters + # (`User.is_demo.is_(False)`) and not the column definition. + if not re.search(r"^\s*if\s+\w+\.is_demo\s*:", line): + continue + block = "\n".join(lines[i : i + 6]) + if "403" not in block: + continue + sites.append(f"{path.relative_to(app_dir)}:{i + 1}") + assert 'detail={"code": "demo"' in block, ( + f"{path.relative_to(app_dir)}:{i + 1} refuses the demo account with an " + f"unstructured detail — the frontend cannot localize it:\n{block}" + ) + # auth.py's shared gate + routes/auth.py's two + routes/me.py's delete. A new one is welcome, + # but it has to be added to the loop above in the same commit. + assert len(sites) == 4, sites def test_a_real_account_is_not_refused_by_the_gate(db, make_user, as_user): diff --git a/frontend/src/lib/actionErrors.contract.test.ts b/frontend/src/lib/actionErrors.contract.test.ts index f15de4fe..9b11f047 100644 --- a/frontend/src/lib/actionErrors.contract.test.ts +++ b/frontend/src/lib/actionErrors.contract.test.ts @@ -15,8 +15,25 @@ import { describe, expect, it } from "vitest"; // glob stops matching (E10 review round 4 learned that the hard way). // Reads a raw `HttpError.detail` and uses it as the message a user sees. Matches `e.detail ||`, -// `err.detail ? err.detail :` and `(e instanceof HttpError ? e.detail : undefined) ??`. -const RAW_DETAIL = /\b[A-Za-z_$][\w$]*\.detail\b(?!\s*(?:=[^=]|\)))/; +// `err.detail ? err.detail :`, `(e instanceof HttpError ? e.detail : undefined) ??`, a cast +// receiver (`(e as HttpError).detail`), an optional chain (`e?.detail`) and a `.detail` passed +// straight as a call argument (`setErr(e.detail)`). +// +// The receiver alternation and the absence of a `)` lookahead are both load-bearing. The first +// version required a bare identifier before `.detail` and excluded `.detail)` — so `setErr(e.detail)`, +// `String(e.detail)` and `(e as HttpError).detail` were all invisible, and because the file SET is +// compared with `toEqual` a file the pattern cannot see is simply never listed: the guard stayed +// green over a fresh copy of the contract written in any of those shapes (round 7). The `)` +// exclusion bought nothing — the only `.detail)` in the frontend is inside `lib/api/core.ts`, which +// `IMPLEMENTATION` skips wholesale — and re-scanning with the wider pattern returns the same five +// files, so it removes no false positive. +const RAW_DETAIL = /(?:\)|[A-Za-z_$][\w$]*)\s*\??\.detail\b(?!\s*=[^=])/; +// KNOWN LIMIT: a `detail` pulled out by destructuring (`const { detail } = err`) is out of reach +// of a line-level scan — by then there is no receiver left to match, and a pattern loose enough +// to catch it fires on every unrelated `detail` field too (measured: `const { data: detail } = +// useQuery(...)` in VideoInfoCard and an options destructure in plexPlayerHooks, neither of which +// touches an HttpError). Naming the hole beats pinning two innocent files into the list below as +// though they were copies of the contract. // Every place that still prefers the server's raw string over a translated fallback. These are // INLINE surfaces — a form field's error line, a setup step's banner — not toasts, and each shows a @@ -34,7 +51,7 @@ const KNOWN_RAW_DETAIL_CONSUMERS = [ // Where the rule itself lives, plus the request layer that produces and localizes the value. const IMPLEMENTATION = ["lib/actionErrors.ts", "lib/api/core.ts"]; -function consumers(): string[] { +function consumers(): { hits: string[]; scanned: number } { const files = import.meta.glob("../**/*.{ts,tsx}", { query: "?raw", import: "default", @@ -51,15 +68,42 @@ function consumers(): string[] { .filter((l) => !/^\s*(\/\/|\*|\/\*)/.test(l)); // a comment about the rule is not a copy of it if (lines.some((l) => RAW_DETAIL.test(l))) hits.push(path); } - return hits.sort(); + return { hits: hits.sort(), scanned: Object.keys(files).length }; } describe("the refused-write contract", () => { it("has no copies beyond the ones on the deferral list", () => { - // The glob must actually match something, or this whole file is a green no-op. + const { hits, scanned } = consumers(); + // The GLOB must have matched something, or this whole file is a green no-op — and the old + // version of this guard did not check that: it read `actionErrors.ts` by its own URL, which + // resolves whether or not `import.meta.glob` enumerated a single file. That only mattered + // because the pinned set was non-empty; the moment the deferral sweep below lands and the list + // becomes `[]`, `expect([]).toEqual([])` would pass over a glob that matched nothing (round 7). + expect(scanned).toBeGreaterThan(100); expect(readFileSync(new URL("./actionErrors.ts", import.meta.url), "utf8")).toContain( "detailData", ); - expect(consumers()).toEqual(KNOWN_RAW_DETAIL_CONSUMERS); + expect(hits).toEqual(KNOWN_RAW_DETAIL_CONSUMERS); + }); + + it("sees a raw detail written in any of the shapes this codebase uses", () => { + // The pattern is the whole guard, so pin the pattern itself: each of these is a real form + // found in this frontend or in its sibling PRs, and every one of the first five was invisible + // to the original regex. + for (const line of [ + " setErr(e.detail);", + " reportError(err.detail);", + " notify({ message: (e as HttpError).detail });", + " toast(String(e.detail));", + " notify({ message: (err as HttpError).detail ?? fallback });", + ' setError(e.detail ?? t("x"));', + ' const msg = err.detail || t("x");', + " setErr(state.err?.detail);", + ]) + expect(RAW_DETAIL.test(line), line).toBe(true); + // And it must not fire on the places that PRODUCE or localize the value, which is why + // `IMPLEMENTATION` exists rather than a cleverer pattern: an assignment is not a read. + for (const line of [" detail = await r.json();", " this.detail = detail;"]) + expect(RAW_DETAIL.test(line), line).toBe(false); }); });