From 8a4c498c933bc76f6d132b0cf362ae8a7e429532 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 23 Aug 2026 14:28:16 -0500 Subject: [PATCH 1/2] feat(quality): the username-as-access-key screen now runs, and can fail on a new site (BACKLOG #1226) The screen shipped with 281 lines and five test arms and NOTHING INVOKED IT. Measured with a control before touching anything: git grep -l username_access_key origin/main -> BACKLOG.md, the screen, 2 test files git grep -l control_char_check origin/main -> ci.yml AND .pre-commit-config.yaml A wired screen appears in those two files. This one appeared in neither, so it read as coverage and produced none -- which is the same false-green family as the four checks.py sites. WIRING IT AS-IS WOULD HAVE INSTALLED A STEP THAT CANNOT FAIL. main() returns 0 whatever it finds, deliberately: several candidates are correct code, and the item forbids turning the screen into a verdict-emitter. So the design the item's own re-score asks for is a shape that fails on a NEW key without judging any of them. --baseline is that shape. It is silent on judged sites and red on a key nobody has read yet. `Candidate.key` was already written stable -- `callee::slot`, no line number, documented as "stable across rebases" -- and had ZERO consumers. This completes what its author started rather than inventing a mechanism. THE BASELINE RECORDS THAT A HUMAN LOOKED, NEVER THAT A SITE IS CORRECT. One of its eight entries is a confirmed defect, which is exactly why it must not be read as an approval list. JUDGED THE CANDIDATE THE ROW NAMES, and it is real. `GET /me/security-events` reaches `store.security_events_for_user`, whose SQL is: SELECT ts, action, detail FROM audit_log WHERE actor = ? AND action LIKE 'auth.%' The username is not labelling a row there -- it IS the WHERE clause deciding which rows the caller sees, which is this item's definition verbatim. The tell the row predicted holds: the sibling route two lines up scopes on user_id. NOT FIXED HERE, and the reason is measured rather than assumed: audit_log has NO immutable actor id. Its columns are id/ts/actor/action/channel_id/detail/client/row_hash, and a grep for user_id against the audit store returns ZERO against a positive control of 3 for actor. The fix is a schema change to a table carrying a row_hash tamper-evidence chain. Named as a subject, not a number, because none is allocated. CONDITIONAL SEVERITY (section 0 -- zero deployments): on a first deployment, an operator who deleted a user and recreated the same username would hand the new holder the previous holder's sign-ins, lockouts and password changes. 10 passed. The wiring-parity arm uses control_char_check as its positive control, so a broken reading fails the control first. --- .github/workflows/ci.yml | 12 ++ .pre-commit-config.yaml | 10 ++ .../quality/username_access_key_baseline.txt | 58 ++++++++++ scripts/quality/username_access_key_screen.py | 53 ++++++++- tests/test_username_access_key_screen.py | 103 ++++++++++++++++++ 5 files changed, 235 insertions(+), 1 deletion(-) create mode 100644 scripts/quality/username_access_key_baseline.txt diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ec7aed61..a456d201 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -370,6 +370,18 @@ jobs: if: runner.os == 'Linux' run: python scripts/quality/control_char_check.py + # BACKLOG #1226. The screen shipped and NOTHING RAN IT, so it read as coverage and produced + # none -- a username reaching a WHERE clause was found three times by accident and zero times + # on purpose. --baseline is what makes this step able to FAIL without turning the screen into + # a verdict-emitter: it is silent on the judged sites and red on a key nobody has read yet. + # Several judged entries are deliberately CORRECT code and one is a confirmed defect, so the + # baseline records that a human looked, never that a site is fine. stdlib only. + - name: Username-as-access-key screen (new sites only) + if: runner.os == 'Linux' + run: >- + python scripts/quality/username_access_key_screen.py + --baseline scripts/quality/username_access_key_baseline.txt + # Two mypy passes on Linux so BOTH platforms' typed branches are checked without paying for a # Windows mypy: the default linux-platform run, plus an explicit --platform win32 run that types # the sys.platform=='win32' branches (ctypes.windll / DPAPI / service_control) the linux run diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index d5118ceb..69e9217e 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -229,6 +229,16 @@ repos: pass_filenames: true files: \.(py|ps1|sh|ts|js|go|md|toml|ya?ml|json|cfg|ini|txt)$ + # BACKLOG #1226. always_run with pass_filenames FALSE: the screen walks its own DEFAULT_SCOPE, + # and handing it a changed-file list would silently narrow what it looked at while still + # printing a confident total -- the shape this screen exists to catch, in its own wiring. + - id: username-access-key + name: username-as-access-key screen (new sites only) + entry: python scripts/quality/username_access_key_screen.py --baseline scripts/quality/username_access_key_baseline.txt + language: system + pass_filenames: false + always_run: true + # Secret scanning — blocks a commit that would introduce a credential/token/key. # (rev is verified by `pre-commit autoupdate` on first install; a wrong tag fails loudly there, # never silently in CI. Run it TARGETED -- `pre-commit autoupdate --repo diff --git a/scripts/quality/username_access_key_baseline.txt b/scripts/quality/username_access_key_baseline.txt new file mode 100644 index 00000000..566e0665 --- /dev/null +++ b/scripts/quality/username_access_key_baseline.txt @@ -0,0 +1,58 @@ +# Judged username-as-access-key sites (BACKLOG #1226). +# +# A KEY IN THIS FILE RECORDS THAT A HUMAN HAS LOOKED AT THE SITE. It does NOT record that the site is +# correct: one entry below is a CONFIRMED DEFECT that cannot be fixed inside this item. Treating the +# baseline as an approval list would turn a screen the item explicitly designed to emit CANDIDATES +# into the verdict-emitter it forbids. +# +# NO LINE NUMBERS, BY CONSTRUCTION. The key is `file::callee::slot`, because the same site is +# `create_search_preset` or `upsert_search_preset` depending where you stand, at different lines +# depending on your base. `Candidate.key` was written stable for this and went unused until now. +# +# A NEW KEY FAILS THE RUN. That is the whole point of wiring: read the site, decide, then add it here +# with a reason. Adding a key without reading one is the only way to defeat this, and it is visible in +# review because every line carries its reasoning. + +# ---- CORRECT BY DESIGN: the username is a display label, not a key ------------------------------- + +# The item names this one itself: the username is deliberately the label while `uploader_id` carries +# the access key. A screen that auto-classified would be wrong on its own author's code. +app.py::save::uploader + +# Response-model fields. These SHOW a name to the caller; nothing is scoped by them. +auth_routes.py::CurrentUser::username +auth_routes.py::UserSummary::username +auth_routes.py::UserPermissions::username + +# ---- CORRECT BY DESIGN: the username IS the subject of the operation ------------------------------ + +# Looking a user up BY NAME is the function's entire purpose; there is no other key to use yet. +auth_routes.py::get_user_by_username::arg0 + +# Policy checks against the PROPOSED name, before any account exists to have an id. +auth_routes.py::password_violations::username +auth_routes.py::create_local_user::username + +# ---- CONFIRMED INSTANCE, ROUTED ONWARD, NOT FIXED HERE ------------------------------------------- + +# `GET /me/security-events` -> AuthService.security_events_for(identity.username) -> +# store.security_events_for_user(username), whose SQL is: +# SELECT ts, action, detail FROM audit_log WHERE actor = ? AND action LIKE 'auth.%' +# +# The username is not labelling a row here -- it IS the WHERE clause that decides which rows the +# caller may see, which is this item's definition of the defect verbatim. The tell the item predicted +# holds: two lines above, the sibling route scopes on `identity.user_id`. +# +# CONDITIONAL SEVERITY (CLAUDE.md section 0 -- zero deployments, nothing is exposed today): on a first +# deployment, an operator who deleted a user and recreated the same username would hand the new +# account holder the previous holder's sign-ins, lockouts and password changes. +# +# WHY IT IS NOT FIXED IN THIS ITEM, measured rather than assumed: `audit_log` has NO immutable key to +# re-key onto. Its columns are id/ts/actor/action/channel_id/detail/client/row_hash -- `actor TEXT` +# is the only identity, and a repo-wide grep for `user_id` against the audit store returns ZERO +# against a positive control of 3 for `actor`. So the fix is a schema change to a table carrying a +# `row_hash` TAMPER-EVIDENCE CHAIN, which is a different piece of work with different risks. +# +# Named as a subject, not a number, because no number is allocated for it: "the self-service +# security-events view scopes on a reassignable username and audit_log has no immutable actor id". +auth_routes.py::security_events_for::arg0 diff --git a/scripts/quality/username_access_key_screen.py b/scripts/quality/username_access_key_screen.py index f8769770..46f63a8a 100644 --- a/scripts/quality/username_access_key_screen.py +++ b/scripts/quality/username_access_key_screen.py @@ -242,7 +242,34 @@ def screen_source(path: str, source: str) -> tuple[list[Candidate], list[Candida return candidates, labels +#: Judged sites, one ``file::callee::slot`` per line. A key records that a HUMAN HAS LOOKED, never +#: that the site is correct -- several judged entries are deliberately correct code, and one is a +#: known defect awaiting its own item. Keys carry NO LINE NUMBER on purpose: the same site is +#: `create_search_preset` or `upsert_search_preset` depending where you stand, at different lines +#: depending on your base, which is why ``Candidate.key`` was built stable and unused until now. +DEFAULT_BASELINE = _ROOT / "scripts" / "quality" / "username_access_key_baseline.txt" + + +def load_baseline(path: Path) -> set[str]: + out: set[str] = set() + for raw in path.read_text(encoding="utf-8").splitlines(): + line = raw.split("#", 1)[0].strip() + if line: + out.add(line) + return out + + def main(argv: list[str]) -> int: + # --baseline turns the screen from an advisory report into a step that can FAIL, WITHOUT making + # it a verdict-emitter. It fails only on a key nobody has judged yet. Wiring it without this + # would install a CI step that cannot fail for the reason it exists -- decoration that reads as + # coverage, which is the class this screen was written to catch. + baseline_path: Path | None = None + if "--baseline" in argv: + i = argv.index("--baseline") + baseline_path = Path(argv[i + 1]) if i + 1 < len(argv) else DEFAULT_BASELINE + argv = argv[:i] + argv[i + 2 :] if i + 1 < len(argv) else argv[:i] + paths = argv or [str(_ROOT / p) for p in DEFAULT_SCOPE] all_candidates: list[Candidate] = [] label_total = 0 @@ -272,7 +299,31 @@ def main(argv: list[str]) -> int: " These are CANDIDATES, not defects. A username is correct as a label on an audit row " "and wrong as a key that scopes a resource; only a reader can tell which this is." ) - return 0 + + if baseline_path is None: + return 0 + + judged = load_baseline(baseline_path) + seen = {f"{c.path}::{c.key}" for c in all_candidates} + unjudged = sorted(seen - judged) + resolved = sorted(judged - seen) + + # STATED EVEN WHEN EMPTY. A baseline entry that no longer matches is either a fixed site or a + # screen that stopped seeing it, and those are opposite facts; printing the count is what lets a + # reader notice the second one. + print(f" baseline: {len(judged)} judged, {len(seen)} seen, {len(resolved)} no longer reported") + for key in resolved: + print(f" NO LONGER REPORTED (fixed, or the screen stopped seeing it): {key}") + if not unjudged: + return 0 + print("") + print( + "username-access-key: NEW UNJUDGED SITE(S) -- read each, then add its key to the baseline:" + ) + for key in unjudged: + print(f" {key}") + print(f" baseline file: {baseline_path}") + return 1 if __name__ == "__main__": diff --git a/tests/test_username_access_key_screen.py b/tests/test_username_access_key_screen.py index d1bfa163..9d3d246f 100644 --- a/tests/test_username_access_key_screen.py +++ b/tests/test_username_access_key_screen.py @@ -23,6 +23,12 @@ class this screen exists to catch. ``save(uploader=identity.username)`` where the username is deliberately the display label and a sibling ``uploader_id`` carries the key. The screen therefore exits 0 whatever it finds, and a test asserting "zero hits on correct code" would be unsatisfiable by design. + +**AMENDED (#1226 wiring): that remains exactly true of the DEFAULT invocation, and ``--baseline`` +does not weaken it.** Baseline mode fails only on a key nobody has JUDGED yet -- never on a +candidate being present -- so the screen still emits candidates rather than verdicts. The baseline +records that a human read the site; one of its entries is a confirmed defect, which is precisely +why it must not be read as an approval list. """ from __future__ import annotations @@ -146,3 +152,100 @@ def test_the_live_api_scope_still_surfaces_the_unjudged_candidate() -> None: assert proc.returncode == 0, proc.stderr assert "security_events_for(arg0=...)" in proc.stdout, proc.stdout assert "audit-label site(s) excluded as correct" in proc.stdout + + +# --------------------------------------------------------------------------------------------- +# BACKLOG #1226, the wiring limb. The screen shipped and NOTHING INVOKED IT, so it read as +# coverage and produced none. Measured before the fix, with a control: +# git grep -l username_access_key origin/main -> BACKLOG.md, the screen, 2 test files +# git grep -l control_char_check origin/main -> ci.yml AND .pre-commit-config.yaml +# A wired screen shows up in those two files. This one did not. +# --------------------------------------------------------------------------------------------- + +BASELINE = ROOT / "scripts" / "quality" / "username_access_key_baseline.txt" +CI = ROOT / ".github" / "workflows" / "ci.yml" +PRECOMMIT = ROOT / ".pre-commit-config.yaml" + + +def _run(*args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + [sys.executable, str(SCREEN), *args], + capture_output=True, + text=True, + cwd=str(ROOT), + check=False, + ) + + +def test_the_live_baseline_covers_every_site_the_screen_reports() -> None: + """MUST NOT FIRE, and it is the arm that keeps the wiring honest. + + If this reds, a new username-as-access-key site has appeared and nobody has read it yet. That + is the entire purpose of the step; the fix is to read the site and record a judgement, never to + delete the arm.""" + proc = _run("--baseline", str(BASELINE)) + assert proc.returncode == 0, f"unjudged site(s) present:\n{proc.stdout}" + + +def test_an_unjudged_site_makes_the_step_FAIL(tmp_path: Path) -> None: + """MUST FIRE. Without this the wiring would install a step that cannot fail for the reason it + exists -- decoration that reads as coverage, which is the class this screen was written to + catch, reproduced in its own gate.""" + thinned = tmp_path / "baseline.txt" + kept = [ + ln + for ln in BASELINE.read_text(encoding="utf-8").splitlines() + if not ln.strip().startswith("auth_routes.py::security_events_for") + ] + assert len(kept) < len(BASELINE.read_text(encoding="utf-8").splitlines()), ( + "the line this arm removes is absent -- the mutation would be a no-op reporting success" + ) + thinned.write_text("\n".join(kept), encoding="utf-8") + + proc = _run("--baseline", str(thinned)) + assert proc.returncode == 1, f"an unjudged site did not fail the step:\n{proc.stdout}" + assert "NEW UNJUDGED SITE" in proc.stdout + assert "security_events_for" in proc.stdout + + +def test_without_a_baseline_it_stays_ADVISORY() -> None: + """MUST NOT FIRE -- the property the module docstring pins, preserved. + + Eight candidates are reported on the default scope and the exit code is still 0. Baseline mode + is opt-in, so nothing that invoked this screen before behaves differently now.""" + proc = _run() + assert proc.returncode == 0 + assert "candidate(s) FOR JUDGEMENT" in proc.stdout + + +def test_no_baseline_key_carries_a_line_number() -> None: + """The item states this as a hard rule: cite the greppable name, never a line. + + The same site is ``create_search_preset`` or ``upsert_search_preset`` depending where you + stand, at different lines depending on your base. A baseline keyed on lines would go stale on + the next rebase and re-fire on sites already judged -- a false alarm that teaches the reader to + delete the step.""" + for raw in BASELINE.read_text(encoding="utf-8").splitlines(): + key = raw.split("#", 1)[0].strip() + if not key: + continue + assert not any(part.isdigit() for part in key.split("::")), f"line-keyed entry: {key}" + + +def test_the_screen_is_wired_in_BOTH_places_like_its_sibling() -> None: + """THE ANTI-VACUITY ARM, and the one that decays. A screen wired in one place and not the other + is half a gate, and the half that is missing is invisible in a green run. + + ``control_char_check`` is the POSITIVE CONTROL: it is wired in both, so if this test's own + reading were broken the control would fail first and say so.""" + ci = CI.read_text(encoding="utf-8") + pre = PRECOMMIT.read_text(encoding="utf-8") + + assert "control_char_check" in ci and "control_char_check" in pre, ( + "the positive control is missing from one of the two files -- this test cannot be trusted" + ) + assert "username_access_key_screen" in ci, "not wired into CI" + assert "username_access_key_screen" in pre, "not wired into pre-commit" + assert "--baseline" in ci and "--baseline" in pre, ( + "wired WITHOUT --baseline, which installs a step that can never fail" + ) From 5be1e508a3a027950830bd171b511924579061f4 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 23 Aug 2026 14:34:24 -0500 Subject: [PATCH 2/2] fix(quality): the #1226 baseline asserted a contrast it had not measured The CONFIRMED INSTANCE block said the sibling route scopes on `identity.user_id` "two lines above". Two things were wrong with that sentence and neither changes the finding. IT IS THIRTEEN LINES, NOT TWO, AND IT IS THE PRECEDING ROUTE HANDLER rather than an adjacent line. The author inherited "two lines" from a dispatch brief and wrote it as confirmed. In a security claim, in a public repository. AND THE CONTRAST IS NOT EVIDENCE OF AN OVERSIGHT, which is how "the tell the item predicted holds" reads. Measured on main: 9 `FROM audit_log` sites in store.py, ZERO keyed on `user_id`, against a positive control of 1 keyed on `actor = ?`. The sessions table has a user id; audit_log does not. The author of the second site had no immutable key available, so the two routes differ because the schemas differ. That is a better account than the original and it makes the finding cleaner. The defect stands on the SQL and the schema alone; the contrast was corroboration and it was overstated. Two deviations from the replacement text as supplied, both to fit the file's own rules rather than to change its meaning: NO LINE NUMBER. The file's header says the key carries no line numbers BY CONSTRUCTION, because the same site sits at different lines depending on your base. Prose citing auth_routes.py:464 would drift for exactly that reason, so the route is named instead. THE POSITIVE CONTROL IS LABELLED WITH WHAT IT COUNTS. The block already carries a positive control of 3 for `actor` over a different sweep, so an unqualified second "positive control 1 for actor" would have read as a contradiction. Co-Authored-By: Claude Opus 5 --- scripts/quality/username_access_key_baseline.txt | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/scripts/quality/username_access_key_baseline.txt b/scripts/quality/username_access_key_baseline.txt index 566e0665..d7902f06 100644 --- a/scripts/quality/username_access_key_baseline.txt +++ b/scripts/quality/username_access_key_baseline.txt @@ -40,8 +40,13 @@ auth_routes.py::create_local_user::username # SELECT ts, action, detail FROM audit_log WHERE actor = ? AND action LIKE 'auth.%' # # The username is not labelling a row here -- it IS the WHERE clause that decides which rows the -# caller may see, which is this item's definition of the defect verbatim. The tell the item predicted -# holds: two lines above, the sibling route scopes on `identity.user_id`. +# caller may see, which is this item's definition of the defect verbatim. +# +# The PRECEDING route handler in the same file, `my_sessions`, scopes on an immutable id -- +# `list_sessions(identity.user_id)`. That contrast is real but it is NOT evidence of an oversight: +# the sessions table HAS a user id and `audit_log` does not. Measured on main: 9 `FROM audit_log` +# sites in store.py, ZERO keyed on `user_id`, against a positive control of 1 keyed on `actor = ?`. +# THE AUTHOR HAD NO IMMUTABLE KEY AVAILABLE AT THIS SITE. # # CONDITIONAL SEVERITY (CLAUDE.md section 0 -- zero deployments, nothing is exposed today): on a first # deployment, an operator who deleted a user and recreated the same username would hand the new