diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 302557db..2cb573f9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -371,6 +371,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..d7902f06 --- /dev/null +++ b/scripts/quality/username_access_key_baseline.txt @@ -0,0 +1,63 @@ +# 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 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 +# 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" + )