fix(api,ui): the demo refusal says why at every site, and the contract guard cannot fail open

Round 6 converted three of four `is_demo` 403s to a structured detail and wrote a docstring saying it
had converted all the sites there are. The account-delete route in `routes/me.py` still answered a
hand-written English sentence, which `notifyActionError` discards in favour of the caller's
translated fallback — so the demo user gets no hint that the demo account is the reason. It is
UI-gated today, so the string is unreachable; what was reachable was the false completeness claim,
which is precisely what stops the next reader from looking. The site is converted, and the list is
now DERIVED from the source: a new test walks `app/` for every `is_demo`-guarded 403, asserts each
raises a structured detail, and fails when the count moves — so this file cannot fall behind the code
the way it just did.

The same round's frontend contract guard was failing open. Its pattern required a bare identifier
before `.detail` and excluded `.detail` used as a last call argument, so `setErr(e.detail)`,
`(e as HttpError).detail`, `String(e.detail)` and four more idiomatic forms were invisible — and
because the file set is compared with `toEqual`, a file the pattern cannot see is simply never listed
and the gate stays green over a fresh copy of the contract. Widened to cover cast receivers, optional
chains and call-argument reads; the pinned set is unchanged, so the narrowing had removed no false
positive. A destructured `detail` stays out of reach (a pattern loose enough for it fires on
react-query's own `const { data: detail }`), and that hole is written into the test rather than
papered over with two innocent files.

Its anti-vacuity guard also did not guard anything: the comment says the glob must have matched
something, but the assertion under it read a sibling file by its own URL, which resolves whether or
not the glob enumerated anything. That only mattered while the pinned list was non-empty — the moment
the deferral sweep lands and it becomes [], the whole file would pass over a glob matching zero
files. The scan size is asserted now.
This commit is contained in:
2026-09-12 16:22:27 +02:00
parent 63bf1bca78
commit 8c51062e5b
3 changed files with 103 additions and 11 deletions
+6 -1
View File
@@ -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,
+47 -4
View File
@@ -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):
+50 -6
View File
@@ -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);
});
});