diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 2369e95d..d5118ceb 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -151,6 +151,36 @@ repos: language: system pass_filenames: false always_run: true + # BACKLOG #1259: the ledger must PARSE at commit time, not only in CI. + # + # `parse_items` refuses a source carrying conflict markers, and that refusal landed -- but it + # protected only programmatic readers and the CI leg. NOTHING called it on the commit path. + # MEASURED at dd655da2 against a docs/BACKLOG.md carrying a realistic prose-level conflict: + # every wired hook PASSED, overall rc 0, so the conflicted ledger commits cleanly, while the + # checker itself exits 1 on the same file. The capability existed and was simply not wired. + # + # NOT `pre-commit/pre-commit-hooks`' check-merge-conflict, and the reason is not dependency + # squeamishness. That repo is absent from this config, so it would be a NEW third-party source + # with its own pinned rev -- but the deciding argument is SINGLE SOURCE: `parse_items` *defines* + # what a readable ledger is, and CLAUDE.md section 11 requires this file be read through it and + # never through a hand-rolled scan. A generic textual matcher is exactly that second, silently + # different definition, and it would drift from the one every other reader uses. + # + # SCOPED TO THE LEDGER FILES, not always_run: the checker reads the whole namespace itself, so + # running it on unrelated commits buys nothing and spends the credibility a gate needs on the + # day it fires. `pass_filenames: false` because it resolves its own sources -- feeding it a + # staged path list would narrow the corpus that `--min-items` exists to protect. + # + # A CONFLICT THAT ADDS A HEADING IS ALREADY CAUGHT, INCIDENTALLY, BY THE GATE ABOVE -- both + # sides' `## N.` headings read as unallocated numbers. That is why the measurement above uses a + # PROSE conflict: it is the case nothing covers, and testing the heading case instead would + # have shown a green gate and concluded there was no defect. + - id: backlog-parses + name: backlog ledger parses (conflict markers) + entry: python scripts/docs/backlog_status_check.py --quiet + language: system + pass_filenames: false + files: ^docs/(BACKLOG\.md|archive/backlog/.*\.md)$ # Leak guard (always on): keep customer/PHI-adjacent strings -- partner names, site data, # routable host IPs, internal worktree slugs and absolute home paths -- out of the TRACKED tree # so they can never reach this public repo. Mirrors the CI forbidden-content gate. diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index e365f6dd..341483d9 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -12213,7 +12213,14 @@ _FHIR_ID_RE.fullmatch("abc\n") -> False the fix **Source:** routed 2026-08-14 by the role-playbooks seat alongside #1266, with an explicit request that the two be kept apart. ## 1268. SQL Server `users.username` lacks the binary collation its sibling identifier columns carry, and the bootstrap-retirement gate compares case-sensitively against it -> ๐Ÿ”ข **Re-scored 2026-08-20 -> P1.** Value **8/10** ยท Difficulty **4/10** ยท _quick win_. Both limbs survive at HEAD: sqlserver.py:1349 is the one identifier column in that file without a COLLATE clause against six BIN2 siblings, the bootstrap guard at auth/service.py:724 is a case-sensitive Python compare against BOOTSTRAP_USERNAME (:71), postgres.py:513 and store.py:1638 are both plain TEXT, and no username case normalisation exists on either path. Value 8 -- on a SQL Server store the bootstrap expiry and supersession control would be walked past by a different capitalisation while the login itself succeeds, and nothing reports that the control did not run. Difficulty 4: the COLLATE clause matches the file's own convention and the comparison normalisation is small, with the cost being the design ruling on case-sensitivity plus the three-backend test that must be shown failing on SQL Server only. _(previously unscored.)_ +> โœ… **SHIPPED 2026-08-20 -- BOTH LIMBS FIXED, and the second does not depend on the first.** [ADR 0169](adr/0169-username-identity-is-case-sensitive-and-must-not-depend-on-store-collation.md). Limb 1: `users.username` now pins `COLLATE Latin1_General_100_BIN2`, the collation its five sibling identifier columns in the same file already carried. Limb 2: the bootstrap-retirement gate compares the value **the store returned**, never the caller's input, then re-reads by id since retirement may have disabled the row. **Measured before the fix, with 6.4.1 disarmed so it could not mask the result: `login("admin")` refused and retired; `login("Admin")` returned `ok=True` with a session issued.** Conditional per `CLAUDE.md` section 0 -- **zero deployments**, so that is what a first deployment against a SQL Server store would have hit, not a live exposure. +> **RESIDUAL, stated because a closing banner that lists only what it fixed is half a record:** the `users` DDL is **creation-guarded**, so an EXISTING SQL Server database keeps its original column collation -- no re-type is attempted. Nothing to migrate (zero deployments), and **limb 2 is correct on such a database anyway**, which is why limb 2 was built to not depend on limb 1. The schema-hash bump is that guard, not a column alteration. +> **Fix and ADR authored by the builder lane; this banner authored by the lander** under [ADR 0165](adr/0165-a-builder-pr-satisfies-the-ledger-gate-with-a-paired-commit-authored-by-the-dispatcher-or-lander.md). Original filing follows. +> **Filed 2026-08-14 - not started. THE USERNAME COLUMN IS THE ONE IDENTIFIER COLUMN IN THIS FILE WITHOUT AN EXPLICIT COLLATION, AND IT IS THE ONE AUTHENTICATION DEPENDS ON.** Filed as ONE item with two limbs, not two items: they are the same root cause -- *what counts as "the same username" is defined in two places that do not agree* -- and fixing either limb alone leaves the other a live trap. + +> **LIMB 1: THE COLUMN.** [`store/sqlserver.py:1348`](messagefoundry/store/sqlserver.py:1348) declares `username NVARCHAR(256) NOT NULL UNIQUE` **with no `COLLATE` clause**, so it inherits the database default -- on a stock SQL Server install a **case-INsensitive** collation (`SQL_Latin1_General_CP1_CI_AS`). The same file already pins **`COLLATE Latin1_General_100_BIN2`** on its other identifier columns at `:1305`, `:1306`, `:1307`, `:1313`, `:1314` and `:1338`. **The pattern is established in this very file and the auth column is the omission.** The other backends disagree with SQL Server and with each other's defaults: Postgres declares `username TEXT NOT NULL UNIQUE` ([`store/postgres.py:512`](messagefoundry/store/postgres.py:512)), which is case-SENSITIVE, as is SQLite's default `BINARY`. So `Admin` and `admin` are **two accounts on Postgres/SQLite and one account on SQL Server** -- a store-dependent identity model under a `UNIQUE` constraint that reads as if it settled the question. + +> **LIMB 2: THE GATE, AND THIS IS THE SHARP END.** [`auth/service.py:650`](messagefoundry/auth/service.py:650) runs the bootstrap expiry/supersession enforcement behind a **Python** comparison: > > **Filed 2026-08-14 - not started. THE USERNAME COLUMN IS THE ONE IDENTIFIER COLUMN IN THIS FILE WITHOUT AN EXPLICIT COLLATION, AND IT IS THE ONE AUTHENTICATION DEPENDS ON.** Filed as ONE item with two limbs, not two items: they are the same root cause -- *what counts as "the same username" is defined in two places that do not agree* -- and fixing either limb alone leaves the other a live trap. > **LIMB 1: THE COLUMN.** [`store/sqlserver.py:1348`](messagefoundry/store/sqlserver.py:1348) declares `username NVARCHAR(256) NOT NULL UNIQUE` **with no `COLLATE` clause**, so it inherits the database default -- on a stock SQL Server install a **case-INsensitive** collation (`SQL_Latin1_General_CP1_CI_AS`). The same file already pins **`COLLATE Latin1_General_100_BIN2`** on its other identifier columns at `:1305`, `:1306`, `:1307`, `:1313`, `:1314` and `:1338`. **The pattern is established in this very file and the auth column is the omission.** The other backends disagree with SQL Server and with each other's defaults: Postgres declares `username TEXT NOT NULL UNIQUE` ([`store/postgres.py:512`](messagefoundry/store/postgres.py:512)), which is case-SENSITIVE, as is SQLite's default `BINARY`. So `Admin` and `admin` are **two accounts on Postgres/SQLite and one account on SQL Server** -- a store-dependent identity model under a `UNIQUE` constraint that reads as if it settled the question. @@ -13517,3 +13524,32 @@ blind window is about a day rather than open-ended. **Source:** the two-act structure was named by the ASVS Tracker seat; this row is the Dispatcher's, whose handoff rule assumed a map that does not exist. +## 1305. the worktree gate matches git by SPELLING, so a case variant of the program name bypasses every rule + +> ๐Ÿ”ข **Filed 2026-08-21 -- not started. `Git -C reset --hard` IS ALLOWED WHERE `git -C reset --hard` IS DENIED, AND THE ONLY DIFFERENCE IS THE CAPITAL LETTER.** Measured against `origin/main` driving the real hook as a subprocess, with the lowercase spelling as a discriminating control and a benign command as a negative control. The same holds for `GIT`, and it is not confined to one rule -- `Git ... checkout` allows too. No quoting, no wrapper, no escape sequence: the bypass is typing the program name differently. + +> **WHY IT WORKS.** Windows resolves `Git`, `GIT` and `git` to the same `git.exe`, so all three RUN. The gate's token comparison is case-SENSITIVE, so only one of them MATCHES. The gate is therefore matching a **spelling** and calling it a program, while the operating system matches an **executable**. Everything downstream of that comparison inherits the gap, which is why it is not a single-rule defect. + +> **THE CLASS, and it is the transferable half.** This is a LEXICAL check standing in for a SEMANTIC one -- the same shape this repository has now hit repeatedly on unrelated surfaces: an absence claim keyed on a symbol NAME, a tier predicate keyed on a body SPLIT that discarded a table, a ledger row detector keyed on a fixed digit WIDTH. Each was a string test where the real question was about a referent. **The fix direction is to compare the RESOLVED EXECUTABLE, not a spelling** -- otherwise the next bypass is a trailing dot, a short path, a quoted absolute path, or an alias, and each one closes as its own item forever. + +> **NOT INTRODUCED BY ANY UNLANDED WORK, and that is measured rather than assumed.** It reproduces identically on `origin/main` and on the unlanded lane branch carrying that work. [#1229](#1229)'s residual work neither opened nor closed it. A case-INSENSITIVE emit was briefly built as part of that work and reverted -- it addressed only the quoted-program-path corner of this gap and cost 12 measured false denies plus two new fail-opens, so it is **not** the remedy here and should not be re-derived. See the gate's own comment at the emit site, which records that retraction. + +> **BOUND, stated because a clean-looking finding invites over-reading.** What is measured is: bare `Git`/`GIT` for `reset --hard` and `checkout`, on the Bash tool, with the cwd inside the governed primary. Rule 3b, the linked-worktree path, is unsampled entirely, as are the target-path attack shapes. The exhaustive spelling-by-rule matrix is kept out of this file deliberately; it is in the builder-1 episode note under the gate sections. + +**Cluster:** Worktree gate / developer guardrail. **Priority:** P2. **Verdict:** build. +**Severity:** no deployment axis (sec. 0) -- this guard is coordination tooling and is not shipped in the wheel. The cost is that a guard believed to be governing every session is bypassed by an ordinary typo-shaped variation, which is worse than a guard known to be absent. + +## 1301. a ledger banner citing a commit sha must cite a commit whose subject names the item it sits under + +> ๐Ÿ”ข **Filed 2026-08-21 -- not started. ONE EDIT CORRUPTED TWO ITEMS IN OPPOSITE DIRECTIONS AND NO GATE COULD SEE IT, BUT A SHA-TO-ITEM AGREEMENT CHECK WOULD HAVE.** A retirement banner intended for one item was written onto another. The Markdown stayed valid, the item count did not move, the status glyph was untouched, and the misplaced paragraph carried no glyph of its own -- so `parse_items` had no second banner to object to and every ledger gate passed. + +> **THE SIGNAL THAT WAS THERE ALL ALONG.** The overwritten banner cited two commit shas whose subjects both ended in that item's own number. The paragraph that replaced it cited a sha whose subject named a DIFFERENT item. So the rule is mechanical and needs no judgement: **a banner citing a commit sha must cite a commit whose subject names the item the banner sits under.** + +> **WHY THIS IS STRONGER THAN THE OBVIOUS RULE.** The natural response to a transposed edit is "key the edit on the item NUMBER, not on a same-shaped sentence". That is correct and it only helps an author who remembers to follow it. A sha-to-item check catches the edit that was **already keyed wrong**, which is the case that actually happened -- twice, silently, in the same file. + +> **BOUND, and it makes this a PARTIAL control that must be described as one.** It fires only on banners that cite shas. Retirement and SHIPPED banners nearly always do; a `Filed -- not started` banner carries none and is outside the check entirely. It reduces the blast radius of a class of ledger corruption; it does not eliminate it. + +> **A KNOWN WAY TO MIS-VERIFY THIS, recorded so the next reader does not repeat it.** A first attempt at the "has anyone already fixed this" screen used a needle that omitted the backticks around the symbol and returned False everywhere, reading as **already fixed**. The re-run normalised and carried a positive control per item. Any implementation of this check needs the same discipline: a screen that finds nothing is indistinguishable from a clean corpus until a positive control says otherwise. + +**Cluster:** Ledger integrity / commit gates. **Priority:** P3. **Verdict:** build. +**Severity:** no deployment axis (sec. 0) -- ledger hygiene. The cost is that a wrongly-transposed banner reads as a working cross-reference forever, and the two items it corrupts fail in opposite directions: one over-reports its status and one under-reports it. diff --git a/docs/adr/0169-username-identity-is-case-sensitive-and-must-not-depend-on-store-collation.md b/docs/adr/0169-username-identity-is-case-sensitive-and-must-not-depend-on-store-collation.md new file mode 100644 index 00000000..89f5dd45 --- /dev/null +++ b/docs/adr/0169-username-identity-is-case-sensitive-and-must-not-depend-on-store-collation.md @@ -0,0 +1,152 @@ + + + +# ADR 0169 โ€” Username identity is case-sensitive, and no identity decision may depend on store collation + +- **Status:** Proposed (2026-08-20) +- **Date:** 2026-08-20 +- **Related:** [BACKLOG #1268](../BACKLOG.md) ยท [ADR 0164](0164-record-bootstrap-claimed-ness-never-infer-a-monotonic-lifecycle-fact-from-mutable-credential-state.md) (the other half of the WP-3 bootstrap lifecycle) ยท [SECURITY.md](../SECURITY.md) ยง"Auto-retirement (WP-3)" ยท [CLAUDE.md](../../CLAUDE.md) ยง0 (not deployed), ยง11 (SDS-3.7) + +--- + +## Context + +### Two places answered "is this the same username?", and they did not agree + +`users.username` was the **one identifier column in the SQL Server schema carrying no `COLLATE` +clause**, so it inherited the *database* default โ€” case-**IN**sensitive on a stock install +(`SQL_Latin1_General_CP1_CI_AS`). Every sibling identifier column in the same file already pinned +`COLLATE Latin1_General_100_BIN2`. SQLite (`BINARY`) and Postgres (`TEXT`) are both +case-**SENSITIVE**. + +So `Admin` and `admin` were **two accounts on two backends and one account on the third**, under a +`UNIQUE` constraint that reads as if it had settled the question. + +That divergence is a portability defect on its own. It became a **security** defect because a second +site answered the same question with a different rule. `_login_local` decided whether to run the +WP-3 bootstrap expiry/supersession enforcement with a **Python** comparison against the caller's +input: + +``` +auth/service.py:724 if username == BOOTSTRAP_USERNAME: # case-SENSITIVE +auth/service.py:725 await self._retire_superseded_bootstrap() +auth/service.py:726 user = await self._store.get_user_by_username(username) # collation-resolved +``` + +On a case-insensitive store the two disagree **in exactly one direction**: a login as `Admin` +**fails** the Python guard, so retirement never runs, and then **succeeds** at the lookup, handing +back the very row the skipped call would have disabled. + +### Measured, before the fix + +With an unclaimed bootstrap whose WP-3 window had lapsed, and the ASVS 6.4.1 credential expiry +disarmed so it could not mask the result: + +| login as | outcome | account after | +|---|---|---| +| `admin` | refused | `disabled = True` โ€” the control fired | +| `Admin` | **`ok=True`, session issued** | `disabled = False` โ€” the control never ran | + +A lapsed, unclaimed first-run credential logging in successfully because one letter was capitalised. +This is **SDS-3.7** exactly โ€” a compensating control resting on a false premise, the premise being +that *the username the gate compared is the username the store matched*. + +**No live exposure: there are zero deployments ([CLAUDE.md](../../CLAUDE.md) ยง0).** This is what a +first deployment against a SQL Server store would hit. SQLite and Postgres deployments would not be +affected, which is what made it easy to look past โ€” the control is genuinely sound on the default +development store, so a green suite there certified nothing. + +--- + +## Decision + +**1. Usernames are case-sensitive.** `Admin` and `admin` are different accounts, on every backend. +The SQL Server column now pins `COLLATE Latin1_General_100_BIN2`, matching the convention its own +file already applied to every other identifier column. + +**2. No identity decision may be delegated to store collation.** Where the engine must decide +whether some string names a particular account, it compares against **the value the store returned**, +never against the caller's input. The bootstrap gate now reads: + +```python +user = await self._store.get_user_by_username(username) +if user is not None and user.username == BOOTSTRAP_USERNAME: + await self._retire_superseded_bootstrap() + user = await self._store.get_user(user.id) # re-read: retirement may have disabled it +``` + +**Rule 2 is the load-bearing half, and it does not depend on rule 1.** The stored value is canonical +by construction (`_ensure_bootstrap_admin` writes it), so the `==` is exact on purpose. This stays +correct under a collation the engine does **not** control โ€” an operator-supplied database, a restored +dump, a column altered downstream. Rule 1 alone would only make SQL Server behave like the others; it +would leave the gate one `ALTER COLUMN` away from being wrong again, with nothing reporting it. + +### Cost + +One extra lookup **on the bootstrap path only**. An ordinary login does the single lookup it always +did, plus a string compare โ€” so the original guard's stated intent ("keep normal logins free of the +extra lookups") is preserved, not traded away. + +--- + +## Alternatives rejected + +**Normalise usernames case-insensitively (casefold on write and on compare).** Rejected on three +grounds, none of which is "it is more work": + +- It requires choosing a canonicalisation and being right about it forever. Unicode case folding is + not locale-neutral (the Turkish dotless `i` is the standard example), and a normalisation that is + wrong for one script silently merges two distinct accounts โ€” a strictly worse failure than the one + being fixed, and one that a `UNIQUE` constraint would enforce rather than catch. +- It would have to be applied consistently across three backends **and** the audit trail, where + `_audit(..., actor=username)` currently records what was typed. Every one of those is a new place + for the two rules to diverge again, which is the defect this ADR exists to close. +- Two of the three backends are already case-sensitive, so it is the larger change measured against + the shipped behaviour, not the smaller one. + +**Fix only the column (limb 1) and leave the guard.** Rejected: it makes the gate *accidentally* +correct, contingent on a schema the engine stops controlling the moment an operator restores a dump +or alters the column. The premise would still be false; it would merely happen to hold. + +**Fix only the guard (limb 2) and leave the column.** Rejected as incomplete rather than wrong. It +closes the security defect, but leaves account identity store-dependent โ€” `Admin` and `admin` still +one account on SQL Server and two elsewhere โ€” which is a live portability trap for anything else that +compares usernames. + +--- + +## Consequences + +- **New SQL Server databases** get the binary collation. The DDL is guarded + (`IF OBJECT_ID('users','U') IS NULL`), so **an existing database keeps its original column + collation** โ€” a re-type of a populated column is not attempted here. Per ยง0 there are zero + deployments and therefore nothing to migrate; this is recorded so a later reader does not mistake + the schema-hash bump for a column alteration. Rule 2 is what makes that acceptable: the gate is + correct on such a database anyway. +- The schema-content hash changes, forcing one full DDL batch run on the next open (ADR 0064). No + test pins a literal hash value, so nothing else moves. +- **Flagged, not decided here:** two accounts differing only in case are themselves a confusability + risk, and case-sensitivity preserves rather than removes it. Refusing to *create* a username that + differs from an existing one only by case is a separate, additive control that does not require + reopening this decision โ€” it is a registration-time check, not an identity rule. Not built, and not + filed as an item by this lane. + +## Verification + +`tests/test_username_identity_collation.py`, four assertions, and the ordering matters: + +- The column test carries its own **positive control** โ€” a sibling column in the same statement is + asserted to already carry the collation, so an unreadable `_SCHEMA` or a renamed table fails loudly + instead of passing over nothing. +- The gate test is paired with an **exact-case control running against the same proxy**, so when the + differently-cased login behaves differently the case is the only variable. +- Both gate tests **passed against the unfixed code** in their first form, which is what caught the + design: they used the *supersession* arm, and `create_local_user` retires the bootstrap eagerly at + `service.py:2685`, so the account was already disabled before the login ran. Supersession can never + exercise this defect. The *expiry* arm can โ€” nothing evaluates that window except the call sitting + behind the guard under test. That retraction is kept in the test file, because a reader who sees + only the corrected version cannot tell it was ever wrong. +- Limb 2 is pinned on a **simulated** case-insensitive store rather than gated on + `MEFOR_TEST_SQLSERVER`. The mechanism is the disagreement between two comparisons, not anything SQL + Server does uniquely, and gating it would mean the assertion that pins the fix does not run in + normal CI โ€” which is precisely the condition that hid the defect. diff --git a/docs/adr/README.md b/docs/adr/README.md index a87a204d..099dc4ca 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -191,5 +191,6 @@ what is withheld and what you can request. | [0164](0164-record-bootstrap-claimed-ness-never-infer-a-monotonic-lifecycle-fact-from-mutable-credential-state.md) | **Record bootstrap claimed-ness; never infer a monotonic lifecycle fact from mutable credential state** (BACKLOG #1245) -- bootstrap auto-retirement (WP-3) gates on `must_change_password` and reads it as *"this account was never claimed"* (`auth/service.py:584`, with the comment at `:585` naming the proxy outright). That reading is sound only while the flag has ONE writer; it has five, and `admin_reset_password` re-raises it at `:2733`. So an administrator resetting the password of the account named `admin` makes it look UNCLAIMED again and the next trigger disables it -- the victim's own next login (`:651`, which fires BEFORE the row is fetched at `:652` and before the credential is verified at `:669`), an engine restart through `initialize()` (`:518`), or any `create_local_user` (`:2551`). **The defect is structural, not a missing guard: "never claimed" is MONOTONIC while `must_change_password` is not** (self-rotation clears it at `:1911`, a reset re-sets it), and no non-monotonic bit can encode a monotonic predicate across a re-set. It is also on the READER, not the writer -- all five writers assert the same true proposition ("the credential now on this account is issuer-issued, not holder-chosen"); only the retirement gate over-reads that into a lifecycle claim. The same file already contains a second reader that gets it right by pairing the flag with a timestamp the reset refreshes (`:697-700` against `password_changed_at`); the retirement reader pairs it with `created_at`, which no reset touches -- a fresh flag held against a stale clock. Decision: a new `users.password_claimed_at` on all three backends, recording only *the holder set their own credential via authenticated self-service rotation*; ONE structurally-constrained writer (a `set_password` whose `must_change_password` is False, reachable only from self-service rotation, so **you cannot record a claim without being the authenticated holder** -- a conventional single-writer rule is precisely what this item documents failing); monotonicity enforced in SQL via `COALESCE(password_claimed_at, ?)` with no statement assigning NULL after creation; ONE named predicate called from both gates, because `_retire_superseded_bootstrap` (`:584`) and `bootstrap_expiry_warning` (`:618`) carried the identical open-coded test and two copies of one lifecycle question is how the warning path inherited the blind spot; and a one-time backfill INSIDE the column-creation guard, whose placement is load-bearing because hoisted out it becomes a permanent second writer of the field whose single-writer property is the entire point. **`admin_reset_password` stays byte-identical -- the fix the defect's own wording most naturally suggests is the wrong one**, since suppressing the flag there would mean an administrative reset no longer forces rotation, breaking the ASVS 6.4.6 property that path exists to provide. Rejected on measurement, both re-proposable and both wrong non-obviously: `last_login_at` (genuinely unforgeable by the reset, but written at `:716` AFTER the credential check, so a bootstrap that logs in once with the printed one-time password and never rotates becomes permanently non-retirable -- **silently deleting the ASVS 6.4.5 time-expiry arm for exactly the case it exists to cover**, and unrepairable within its own inputs because a reset re-arms BOTH of them), and `password_changed_at != created_at` (the reset refreshes it, `store.py:7725`). Records that the defeated property was already asserted in prose twice -- the docstring at `:579-581` and `SECURITY.md:58-59`/`:1164`, both corrected here -- which is the argument for an ADR: **prose is exactly what failed**, and bootstrap retirement had no ADR, living only in `SECURITY.md`, which is how the guarantee drifted from the code with nothing reporting it. Explicitly does NOT fix the second, stacked proxy at `:583` (username as bootstrap identity, `BOOTSTRAP_USERNAME = "admin"` at `:71`, no marker column and no role check, so ANY local account named `admin` is subject to this and delete-and-recreate mints a second silently-dead one) -- narrowed, not removed, and filed separately. Not a whole-system lockout: `USERS_MANAGE` is unmintable into a custom role and self-reset is refused (`auth_routes.py:762`), so the resetter is necessarily a second enabled administrator | Proposed (2026-08-13) -- written in the conditional throughout; **zero deployments** (CLAUDE.md section 0), so this is what a first deployment would hit, not a live exposure | | [0165](0165-a-builder-pr-satisfies-the-ledger-gate-with-a-paired-commit-authored-by-the-dispatcher-or-lander.md) | **A builder PR satisfies the ledger gate with a paired commit authored by the Dispatcher or Lander** โ€” **two correct rules met and produced an unsatisfiable state**, which is why it needed a decision and not a fix. The required check *"a PR that implements BACKLOG #N must update BACKLOG.md"* demands a ledger edit in the PR's own diff; the owner's 2026-08-13 authoring ruling forbids a **builder** to author ledger content, on the property that **a mechanical union cannot invent a disposition but authoring a banner can, and a seat that can author its own item's banner can turn its own PR green**. Measured live: **PR #379 went red for OBEYING the ruling.** Decision: the Dispatcher or Lander **authors** the disposition and the commit rides **on the PR branch**. **The expected answer inverted on reading the gate rather than reasoning about it** โ€” `backlog-hygiene.yml:64-98` computes `git diff --name-only BASE...HEAD` and passes if the changed set touches `docs/BACKLOG.md` or `docs/archive/backlog/`; it **never inspects authorship**, so a Dispatcher-authored commit cherry-picked onto the head is indistinguishable from a builder's. Evaluated against the real cherry-picked head: `touches_code` 1, ledger 1, **PASS** โ€” so **no gate change was required and none is pending**. The ledger gate permits the cherry-pick for a non-obvious reason: it iterates **headings added relative to base**, and a banner flip or amendment on an item already on `main` adds no `## N.` heading, so ownership is never consulted and the committing seat is irrelevant (confirmed โ€” pre-commit hooks ran clean on the cherry-pick); **this holds only for landed items, a PR that FILES an item is a different shape**. **(a2)** โ€” land the ledger commit separately and correlate it โ€” **rejected because it would undo a deliberate control**: the gate uses three-dot on purpose and its own comment says two-dot *"would pass while enforcing nothing"*. **(b)** โ€” a builder carve-out to flip only its own item's banner โ€” rejected, reopens the self-approval hazard (property identified by the Builder 2 seat before any ruling existed). **(c)** โ€” the same pattern as an interim โ€” **dissolved rather than rejected: (c) and the decision are one mechanism, so there is no transition.** Carries a recorded near-miss: the ruling was briefly written as *"(c) is fine until (a) lands"*, **an expiry whose trigger had ALREADY FIRED** โ€” it looks like the safe construction and behaves like the unsafe one, and would have become permanent by default while appearing bounded. Consequence stated rather than softened: **one manual step per builder PR indefinitely**, a Dispatcher/Lander serialisation point, and the builder **MUST declare the withheld banner in its PR body** because a missing flip is visually identical to the BACKLOG #1237 defect โ€” a fix on `main` with its item still reading *not started*, same shape, opposite cause. No engine behaviour changes | **Accepted (2026-08-13)** โ€” **already in force; no code change was required or is pending.** Executed on PR #379 before the ADR was written. Provenance split three ways because each half is only checkable if attributed: the collision found by the **Lander** on #379's red check, the self-approval property by **Builder 2**, the gate measurement and the no-build finding by the **Dispatcher**, the ruling by the **owner** | | [0167](0167-phi-security-notification-readiness-gates-on-a-deliverable-address-checked-early-in-the-asgi-lifespan.md) | **PHI security-notification readiness gates on a deliverable address, checked early in the ASGI lifespan** (BACKLOG #1020, owner-ruled 2026-08-13 option (b)) -- the PHI startup gate computes `security_channel_ready` from the SMTP transport alone (`notify_security_events` + `email_smtp_host` + `email_from`), which asks *"is a transport configured"* and never *"can the account that matters actually receive"* (SDS-3.8). The two come apart on exactly the instance the gate protects: `_ensure_bootstrap_admin` creates the account holding `frozenset(Permission)` with **no** `email=`, and `SecurityEventNotifier.notify` opens `if not event.email: return`, so all ten notice types no-op for the most privileged account while the gate reports healthy. Decision: gate on `has_notifiable_admin()` -- at least one **enabled administrator with an address** -- scoped to the ROLE, not the bootstrap account, because `email` is optional for any Administrator so a hand-created privileged account has the identical hole. **Placement is the decision this ADR exists for, and it was settled by measurement, not preference.** LIFESPAN-after-`engine.start()` is OUT: BACKLOG #1257 records that an exception there unwinds nothing and **hangs** -- strictly worse than the defect, since an operator can see a wrong readiness answer but not a process that never finishes starting. PREFLIGHT was recommended **and withdrawn by its own author on measurement**: `_serve` (1042-2833) runs entirely before the lifespan and opens a store **zero** times, and `list_users()` is async, so it is not one cheap read but the first store open in a preflight that has never had one, from sync code. EARLY-LIFESPAN wins -- the 191-line window between `api/app.py:5540` (`open_store`) and `:5731` (`engine.start()`), where the store is open and no engine tasks exist, so the check is a plain `await`. **The exit code changes and the divergence is FORCED:** measured, raising there exits **3** in 0.49s, and `sys.exit(2)` there **also** exits 3 -- uvicorn catches `SystemExit` and treats it as a startup failure -- against a positive control that reached a RUNNING server and self-stopped with a distinct 99, which is what makes the exits mean anything. Cost stated no larger than it is: `_serve` returns 2 at 32 sites and `DEPLOYMENT.md` says "(exit 2)" twice, but **both citations are scoped to specific refusals and no line generalises it**, so this is an inconsistency with two documented refusals, not a contradiction of a universal claim -- an earlier draft called it a "documented-contract divergence" and that is corrected here rather than dropped. **It must not be claimed that exit 2 gives a clean stop today:** `install-service.ps1:463` sets NSSM `AppExit Default Restart`, so the shipped wrapper restarts on any code and the operational delta is approximately nil. Accepted with the cheap honest fix -- a `DEPLOYMENT.md` line recording that a startup-stage refusal exits 3. **Rejected: a sentinel** catching `SystemExit` at the `uvicorn.run()` call site to re-exit 2 -- **untested** (the "if" was never measured) and a cross-layer mechanism bought to remove an inconsistency the NSSM finding makes nearly free; recorded rather than omitted so it is not re-derived. Records that there are now **three** independent copies of "who is an enabled administrator" in `auth/service.py`, agreeing by convention with nothing binding them | **Proposed (2026-08-15)** -- the predicate is built (`29a026e2`, 3 asymmetric arms, mutation-proven: removing the role test reds the non-administrator arm ALONE); the gate that consumes it is not yet written. โš ๏ธ **Does NOT discharge #1020's rider:** the termination evidence is a MINIMAL REPRO, not the real gate, and a rider that exists because someone inferred is not satisfied by an inference | +| [0169](0169-username-identity-is-case-sensitive-and-must-not-depend-on-store-collation.md) | **Username identity is case-sensitive, and no identity decision may depend on store collation** (BACKLOG #1268) -- `users.username` was the one identifier column in the SQL Server schema with no `COLLATE` clause, so it inherited the DATABASE default (case-INsensitive on a stock install) while every sibling identifier column in the same file pinned `Latin1_General_100_BIN2` and both other backends were case-SENSITIVE -- `Admin` and `admin` two accounts on two backends and one on the third, under a `UNIQUE` constraint that reads as if it had settled the question. That portability defect became a SECURITY defect because a second site answered the same question by a different rule: `_login_local` gated WP-3 bootstrap expiry/supersession enforcement on a PYTHON `username == BOOTSTRAP_USERNAME` against the caller's input, while the lookup one line below was resolved by the COLUMN'S collation. The two disagree in exactly one direction -- `Admin` FAILS the Python guard so retirement never runs, then SUCCEEDS at the lookup and returns the very row the skipped call would have disabled. MEASURED on a lapsed unclaimed bootstrap with 6.4.1 disarmed so it could not mask the result: `login("admin")` refused and retired, `login("Admin")` returned ok=True with a session issued and `disabled` unset -- SDS-3.7 exactly, a compensating control resting on the false premise that the username the gate compared is the username the store matched. Decision, two rules: usernames ARE case-sensitive (the column now pins the collation its own file's convention already required), and **no identity decision may be delegated to store collation** -- the gate compares the value THE STORE RETURNED, never the caller's input, then re-reads by id since retirement may have disabled the row. The second rule is load-bearing and does NOT depend on the first: it stays correct under a collation the engine does not control (operator-supplied database, restored dump, column altered downstream), where limb 1 alone leaves the gate one `ALTER COLUMN` from being wrong again with nothing reporting it. Cost is one extra lookup ON THE BOOTSTRAP PATH ONLY, so the original guard's stated intent (normal logins free of extra lookups) is preserved rather than traded. Rejected: case-INsensitive normalisation (requires a canonicalisation that is not locale-neutral -- the Turkish dotless `i` -- so a wrong fold silently MERGES two accounts under a UNIQUE constraint that enforces rather than catches it; and it would have to hold across three backends plus the audit trail, every one a fresh place for the two rules to diverge again); column-only (makes the gate accidentally correct, contingent on a schema the engine stops controlling); gate-only (closes the security defect, leaves identity store-dependent). Existing SQL Server databases keep their original collation -- the DDL is creation-guarded and no re-type is attempted; zero deployments (CLAUDE.md section 0) so there is nothing to migrate, recorded so the schema-hash bump is not misread as a column alteration. Flagged undecided: two accounts differing only in case are themselves a confusability risk, closeable additively by a registration-time refusal without reopening this decision. Verification carries its own retraction -- the first version of both gate tests PASSED against unfixed code because they used the supersession arm, which `create_local_user` retires eagerly at `service.py:2685`, so the account was already disabled before the login ran; only the EXPIRY arm reaches the login path with retirement still pending | Proposed (2026-08-20) -- written in the conditional; **zero deployments**, so this is what a first deployment against a SQL Server store would hit, not a live exposure | | [0170](0170-constant-work-recovery-code-verification-pad-to-the-configured-slot-count-rather-than-short-circuit.md) | **Constant-work recovery-code verification: pad to the configured slot count rather than short-circuit** (BACKLOG #1167, ASVS 11.2.4) -- `_verify_second_factor` walked the argon2id recovery-code hashes and `return`ed on the first match, so the NUMBER of ~64 MiB verifications was a function of which code was presented. **Two leaks and only one matters:** the matched INDEX is worthless (the attacker holds the code and the response answers them anyway), but on the FAILURE path the cost is one verify per REMAINING code -- so anyone holding the password can time a wrong-code refusal and learn how many recovery codes an account has left, without authenticating to the second factor. **The item rated this difficulty 7 on a premise that does not survive measurement:** the re-score says a constant loop 'converts a timing leak into a memory and CPU amplification target', which is the right objection to raise -- and the failure path ALREADY verifies every remaining hash, so making the walk unconditional introduces no new cost, it makes today's WORST CASE the only case. Decision: always run exactly `mfa_recovery_code_count` verifies, padding with the same fixed `_DUMMY_PASSWORD_HASH` the local login leg uses, and select the winner AFTER the loop. Ceiling unmoved (default 10, validator-capped 50); `_argon2`'s semaphore means the concurrent-argon2 footprint cannot widen either; and the path sits behind primary authentication, so it is not an unauthenticated flood surface. **Claims constant WORK, not constant TIME** -- the store round trip on a match is not equalized, the TOTP branch returns earlier, and argon2's own constant-timeness is INHERITED from `argon2-cffi` and has never been measured in this tree, a gap #1167 names and this does not close. No timing measurement was run by the item or by this change. Rejected: leaving the short-circuit as accepted (the fix cost nothing against the existing ceiling, so 'accepted' would have been a judgement made before the amplification premise was checked); and a non-secret lookup index so only ONE verify ever runs -- strictly better on both axes, rejected as OUT OF SCOPE rather than wrong, needing a schema change across three backends and a migration, and recorded so it is not re-derived if the constant walk's cost ever bites | **Accepted (2026-08-22)** -- built with the change. Three parametrized tests pin the count for a first-slot match, a last-slot match and a non-match; proven red-first, removing the padding reds ALL THREE and the file restores byte-identical by SHA-256. Severity conditional per CLAUDE.md section 0 -- **zero deployments**, so this is what a first deployment would have inherited | | [0171](0171-offline-administrator-unlock-a-host-gated-cli-recovery-path-for-a-sole-administrator-lockout.md) | **Offline administrator unlock: a host-gated CLI recovery path for a sole-administrator lockout** (BACKLOG #1236) -- a deployment with ONE administrator had no recovery from account lockout, and every exit is individually deliberate: the bootstrap account is literally `admin`, it is created with no email so the ACCOUNT_LOCKED notice never leaves the process, self-reset is refused, an admin reset needs ANOTHER admin, re-bootstrap fires only on an EMPTY users table, and none of 38 CLI subcommands managed users. **The defect is that they close SIMULTANEOUSLY for that deployment** and nothing notices the conjunction. **The filed acceptance criterion could not discriminate and was amended 2026-08-21:** "recover without hand-editing the database and without a second admin" PASSES ON THE SHIPPED SYSTEM BY WAITING, since the lock self-expires after `lockout_minutes`; a test both a fixed and a broken system pass is not a test. Decision: `messagefoundry admin-unlock --username `. **The gate is HOST ACCESS and it is a real gate rather than an absent one** -- reaching it needs the config, the store path and on an encrypted store the key material, so anyone holding all three already has the database and does not need an unlock to reach an account; that is why it ships unauthenticated, and it is the load-bearing claim. **Clears the lockout and does NOT reset the password** -- deliberately narrower, since a reset would hand the runner a working account. **Reuses `record_login_failure(failed_attempts=0, locked_until=None)` rather than adding a protocol method**, decided by a MEASURED cross-lane fact rather than taste: a named `clear_lockout` would touch base/store/postgres/sqlserver, and all four were uncommitted in a peer lane at the time, so reuse avoided a four-file collision. Exit codes follow the `--json` convention (`_emit_error`, 1) not the M-31 lineage (stderr, 2), verified against `audit-verify` which has no `--json` flag. Carries M-31 forward: a typo'd `--db` is refused rather than creating an empty SQLite store and reporting a false "no such account" | **Accepted (2026-08-22)** -- built with the change. Four tests; **exactly ONE is the control** and the other three are deliberately insensitive -- neutering the clearing call reds only the acceptance test, and the audit-row test still passes under that plant, so it evidences the flow RAN and never that it WORKED. Does NOT address #1236's repetition limb: lock cycles remain unbounded and an attacker can re-lock. Severity conditional per CLAUDE.md section 0 -- **zero deployments** | diff --git a/messagefoundry/api/tls.py b/messagefoundry/api/tls.py index 471d8230..fa4548ef 100644 --- a/messagefoundry/api/tls.py +++ b/messagefoundry/api/tls.py @@ -16,6 +16,7 @@ from messagefoundry.config.settings import ApiSettings from messagefoundry.config.tls_policy import ( harden_cipher_suites, + harden_crl_check, harden_kex_groups, harden_verify_flags, ) @@ -60,4 +61,10 @@ def build_api_ssl_context(api: ApiSettings, *, enforcing: bool = True) -> ssl.SS enforce_anchor(client_ca, enforcing=enforcing) # #285: pin + owner-only-DACL preflight ctx.load_verify_locations(cafile=api.tls_client_ca_file) ctx.verify_mode = ssl.CERT_REQUIRED + # Opt-in revocation (#1005). NOTE THE POSITION: harden_verify_flags runs ABOVE, before the + # CA is loaded, and this must NOT sit beside it. The CRL goes into the trust store, so + # loading it before the CA yields a context with the check flag set and zero CRLs -- which + # refuses EVERY client rather than skipping the check. + if api.tls_client_crl_file: + harden_crl_check(ctx, api.tls_client_crl_file) return ctx diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 68dc2606..fb50c3d8 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -744,11 +744,28 @@ async def _login_local( ) -> LoginOutcome: # Enforce bootstrap expiry/supersession before the credential check: an unclaimed bootstrap # that lapsed (or was superseded) is disabled here, so the disabled-account path below refuses - # it like any other invalid login (WP-3). Scoped to the bootstrap username to keep normal - # logins free of the extra lookups. - if username == BOOTSTRAP_USERNAME: - await self._retire_superseded_bootstrap() + # it like any other invalid login (WP-3). + # + # BACKLOG #1268: THE GATE ASKS THE ROW THE STORE RESOLVED, NEVER THE CALLER'S SPELLING. It + # used to read `if username == BOOTSTRAP_USERNAME` -- a PYTHON comparison against the input, + # while the lookup below is resolved by the COLUMN'S COLLATION. On a case-insensitive store + # those disagree in exactly one direction: `Admin` FAILS the Python guard, so retirement never + # runs, and then SUCCEEDS at the lookup, handing back the very row the skipped call would have + # disabled. MEASURED before this fix: a lapsed, unclaimed bootstrap credential logged in + # successfully as `Admin` while the identical attempt as `admin` was refused and retired. + # That is SDS-3.7 exactly -- a compensating control resting on a false premise, the premise + # being that the username the gate compared is the username the store matched. + # + # Comparing the STORED value keeps this correct under any collation, including one the engine + # does not control (an operator-supplied database, a restored dump, a column altered + # downstream), so it does not depend on the sibling fix in the SQL Server DDL. The `==` is + # exact on purpose: `_ensure_bootstrap_admin` writes the canonical spelling, so the stored + # value is canonical by construction. The cost is one extra lookup ON THE BOOTSTRAP PATH ONLY + # -- an ordinary login still does the single lookup it always did, plus a string compare. user = await self._store.get_user_by_username(username) + if user is not None and user.username == BOOTSTRAP_USERNAME: + await self._retire_superseded_bootstrap() + user = await self._store.get_user(user.id) # re-read: retirement may have disabled it if user is None or user.auth_provider != AuthProvider.LOCAL.value or user.disabled: # Equalize timing with the real-password path so a missing/disabled/AD account is not # distinguishable from a wrong password (defeats username enumeration via latency). diff --git a/messagefoundry/config/models.py b/messagefoundry/config/models.py index 31a9e671..00e05747 100644 --- a/messagefoundry/config/models.py +++ b/messagefoundry/config/models.py @@ -252,6 +252,21 @@ class Source(BaseModel): # when it suppresses a would-be production refusal. `tls_hop_attested_reason` records why (audit). tls_hop_attested: bool = False tls_hop_attested_reason: str | None = None + # BACKLOG #1005: the INBOUND mirror of Destination.tls_revocation_attested. Per-connection + # attestation that a revocation-checking PKI backs a VERIFYING mTLS listener -- the operator + # taking responsibility for revocation, exactly as the outbound flag does. + # + # Needed because the three server builders load a CA and set CERT_REQUIRED but check no + # revocation, so a revoked-but-chain-valid client authenticates until its notAfter. #1005 adds + # an opt-in `tls_crl_file`; this flag is what lets a site whose PKI covers revocation OUT of the + # engine decline it without being refused. Every sibling refusal here pairs with an attestation + # (`tls_hop_attested` for a cleartext/verify-off hop, #200), and a revocation refusal with no + # escape would be the only control without one. + # + # Default False -> keyed purely on posture, so every existing inbound is byte-identical. + # DISTINCT from tls_hop_attested: that one attests a hop is secure DESPITE no/weak TLS; this one + # attests that a VERIFYING hop's certificates are checked for revocation elsewhere. + tls_revocation_attested: bool = False @model_validator(mode="after") def _validate_hop_attestation(self) -> Source: diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index b9cfdfb1..8ec12640 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -768,6 +768,15 @@ class ApiSettings(_Section): tls_ciphers: str | None = None # Optional CA bundle to verify CLIENT certs (mTLS for the console; opt-in, future). tls_client_ca_file: str | None = None + #: Opt-in CRL for the mTLS client certificates `tls_client_ca_file` verifies (BACKLOG #1005). + #: A PEM carrying the CA **and** its CRL. Absent, client certificates are verified for chain + #: and RFC 5280 conformance but NOT for revocation -- measured, a revoked-but-chain-valid + #: client is ACCEPTED. Set it and a revoked partner certificate is refused at the handshake. + #: + #: **An expired CRL refuses EVERY client, not only revoked ones**, so this is read at startup + #: and refused loudly there rather than at the first partner handshake. See + #: :func:`~messagefoundry.config.tls_policy.harden_crl_check`. + tls_client_crl_file: str | None = None # WP #285 (ASVS 6.7.1): optional SHA-256 pin over the mTLS client-CA trust anchor above. Set to the # lowercase-hex SHA-256 of the PEM file's bytes; the loaded anchor's fingerprint is checked against # it at construction AND at reload and a mismatch REFUSES to start โ€” always, independent of diff --git a/messagefoundry/config/tls_policy.py b/messagefoundry/config/tls_policy.py index 62351530..4daa8ed2 100644 --- a/messagefoundry/config/tls_policy.py +++ b/messagefoundry/config/tls_policy.py @@ -34,6 +34,7 @@ import logging import os import ssl +import time import urllib.request from collections.abc import Callable, Iterator, Mapping from contextlib import contextmanager @@ -211,6 +212,70 @@ def harden_verify_flags(ctx: ssl.SSLContext) -> None: ctx.verify_flags |= strict +def harden_crl_check(ctx: ssl.SSLContext, crl_file: str) -> None: + """Load a CRL onto a *verifying* ``ctx`` and turn on leaf revocation checking (BACKLOG #1005). + + The opt-in revocation half of :func:`harden_verify_flags`, which does strict RFC 5280 path + validation and explicitly NOT revocation. Call it only on a context that already verifies the + peer, after the CA is loaded. + + **Three refusals, and each one is a measured failure mode rather than defensive habit.** + Re-measured on this worktree, CPython 3.14.6 / OpenSSL 3.5.7, TLS 1.2 pinned so client auth is + in-handshake: + + * ``cadata=`` loads **zero** CRLs from the same PEM bytes that ``cafile=`` loads one from, while + still setting the check flag -- and the observable is not a skipped check, it is EVERY client + refused with ``unable to get certificate CRL``. No error and no warning at load time. So this + loads through ``cafile=`` only, and asserts ``cert_store_stats()["crl"] >= 1`` afterwards: + that count is the only thing that distinguishes "loaded" from "silently ignored". + * A CRL past ``nextUpdate`` refuses every client, not just revoked ones (verify error 12). + Checked here at construction so an unrefreshed CRL fails loudly at startup instead of at the + first partner handshake, where the operator's only symptom is every partner dropping at once. + * A missing file must not degrade to "no revocation checking". A configured control that + silently does nothing is worse than an absent one. + + **``capath=`` IS MEASURED AND IT WORKS -- and the guard above would REFUSE it.** OpenSSL's + hashed directory (``c_rehash`` producing ``.0`` + ``.r0``) is the natural shape for + a refreshable CRL drop, and measured on this worktree it enforces revocation identically: + revoked client REFUSED ``certificate revoked``, good client ACCEPTED, against a ``cafile=`` + positive control and a CA-only baseline that accepts the revoked client. + + **But ``cert_store_stats()["crl"]`` reports ZERO for it**, because a hashed directory is read + LAZILY during verification rather than at load time. So the ``>= 1`` assertion above -- which is + exactly right for ``cafile=`` -- is not a valid liveness check for ``capath=`` and would reject + a working configuration. Anyone adding ``capath=`` support needs a different proof that the + directory is real, not this one. That is why this loader stays ``cafile=``-only for now.""" + from pathlib import Path + + path = Path(crl_file) + if not path.is_file(): + raise ValueError( + f"[tls] crl file {crl_file!r} does not exist; refusing to build a context that would " + "advertise revocation checking and perform none" + ) + + # Freshness BEFORE loading: an expired CRL refuses every client, so say so at startup. + from messagefoundry.pki import read_crl_facts + + facts = read_crl_facts(path.read_bytes(), now=time.time()) + if facts.expired: + raise ValueError( + f"[tls] crl file {crl_file!r} expired at {facts.next_update_iso} " + f"({-facts.days_remaining} day(s) ago); an expired CRL refuses EVERY client, not only " + "revoked ones, so this would take the listener down at the first partner handshake" + ) + + ctx.load_verify_locations(cafile=str(path)) # cafile= ONLY -- cadata= loads zero CRLs + loaded = ctx.cert_store_stats().get("crl", 0) + if loaded < 1: + raise ValueError( + f"[tls] crl file {crl_file!r} loaded no CRL into the trust store " + f"(cert_store_stats crl={loaded}); the check flag would be set with nothing to check " + "against, which refuses every client rather than skipping the check" + ) + ctx.verify_flags |= ssl.VERIFY_CRL_CHECK_LEAF + + #: OpenSSL ``X509_V_FLAG_NO_CHECK_TIME`` (``openssl/x509_vfy.h``) โ€” a **stable public constant** #: (``0x200000``, unchanged since OpenSSL 1.0.2 through 3.x). It disables ONLY the certificate #: validity-period check (both ``notBefore`` AND ``notAfter``) during chain verification; the chain diff --git a/messagefoundry/config/wiring.py b/messagefoundry/config/wiring.py index ec7573bd..0790fee4 100644 --- a/messagefoundry/config/wiring.py +++ b/messagefoundry/config/wiring.py @@ -1068,6 +1068,8 @@ def MLLP( | None = None, # passphrase for an ENCRYPTED tls_key_file (put the secret in env()) tls_ca_file: str | None = None, # trust anchor โ€” inbound: verify client certs (mTLS); outbound: verify server + tls_crl_file: str + | None = None, # INBOUND: opt-in CRL for mTLS client certs (#1005) โ€” CA bundle + CRL, PEM tls_verify: bool = True, # OUTBOUND: verify the server cert (false is MITM-able โ†’ needs MEFOR_ALLOW_INSECURE_TLS) tls_check_hostname: bool = True, # OUTBOUND: require the server cert to match `host` tls_allow_expired: bool = False, # OUTBOUND: honour an EXPIRED server cert (chain+hostname still verified; #129) @@ -1183,6 +1185,7 @@ def MLLP( "tls_key_file": tls_key_file, "tls_key_password": tls_key_password, "tls_ca_file": tls_ca_file, + "tls_crl_file": tls_crl_file, "tls_verify": tls_verify, "tls_check_hostname": tls_check_hostname, "tls_allow_expired": tls_allow_expired, @@ -1350,6 +1353,8 @@ def Http( | EnvRef | None = None, # passphrase for an ENCRYPTED tls_key_file (put the secret in env()) tls_ca_file: str | None = None, # trust anchor โ€” opt-in mTLS (require + verify a client cert) + tls_crl_file: str + | None = None, # opt-in CRL for mTLS client certs (#1005) โ€” CA bundle + CRL, PEM # --- Intake authentication (ADR 0154 D6) โ€” a PEER control on this connector, not admin RBAC --- intake_auth: Literal[ "none", "api_key", "bearer", "mtls_subject" @@ -1460,6 +1465,7 @@ def Http( "tls_key_file": tls_key_file, "tls_key_password": tls_key_password, "tls_ca_file": tls_ca_file, + "tls_crl_file": tls_crl_file, "intake_auth": intake_auth, "intake_api_key": intake_api_key, "intake_api_key_next": intake_api_key_next, @@ -2191,6 +2197,9 @@ def DICOM( tls_ca_file: str | EnvRef | None = None, # opt-in mTLS: require + verify a calling peer's client cert + tls_crl_file: str + | EnvRef + | None = None, # opt-in CRL for mTLS client certs (#1005) โ€” CA bundle + CRL, PEM tls_allow_expired: bool = False, # OUTBOUND SCU: honour an EXPIRED PACS cert (chain+hostname still verified; #129) max_object_bytes: int | None = 128 * 1024 * 1024, # per-C-STORE-object cap; over-cap โ†’ DIMSE # failure BEFORE the durable commit (the X12 max_interchange_bytes analog; OOM/DoS guard, ยง9) @@ -2234,6 +2243,7 @@ def DICOM( "tls_key_file": tls_key_file, "tls_key_password": tls_key_password, "tls_ca_file": tls_ca_file, + "tls_crl_file": tls_crl_file, "tls_allow_expired": tls_allow_expired, "max_object_bytes": max_object_bytes, "max_associations": max_associations, diff --git a/messagefoundry/pipeline/alert_sinks.py b/messagefoundry/pipeline/alert_sinks.py index 1f667f66..a060c116 100644 --- a/messagefoundry/pipeline/alert_sinks.py +++ b/messagefoundry/pipeline/alert_sinks.py @@ -830,6 +830,15 @@ def cert_expiry(self, name: str, *, path: str, not_after: str, days_remaining: i } ) + def crl_expiry(self, name: str, *, path: str, not_after: str, days_remaining: int) -> None: + """BACKLOG #1005. Routed exactly like :meth:`cert_expiry` -- same fan-out, same redaction -- + because a CRL path is config metadata and carries no PHI. Kept a SEPARATE method because an + expired CRL refuses every client rather than degrading one identity, so an operator filtering + on it is asking a different question.""" + self.cert_expiry( + f"{name} (CRL)", path=path, not_after=not_after, days_remaining=days_remaining + ) + def secret_rotation_due( self, name: str, diff --git a/messagefoundry/pipeline/alerts.py b/messagefoundry/pipeline/alerts.py index 48e13168..b636a8d7 100644 --- a/messagefoundry/pipeline/alerts.py +++ b/messagefoundry/pipeline/alerts.py @@ -98,6 +98,19 @@ def cert_expiry(self, name: str, *, path: str, not_after: str, days_remaining: i Emitted by the :class:`~messagefoundry.pipeline.cert_expiry.CertExpiryRunner`.""" ... + def crl_expiry(self, name: str, *, path: str, not_after: str, days_remaining: int) -> None: + """A configured CRL is expired or within the warn window (BACKLOG #1005). ``name`` labels the + connection; ``path`` is the PEM; ``not_after`` is the ISO ``nextUpdate``; ``days_remaining`` + is negative once expired. + + **SEPARATE FROM :meth:`cert_expiry` BECAUSE THE REMEDY AND THE BLAST RADIUS DIFFER.** An + expiring server certificate degrades one identity and is fixed by reissuing it. An expired + CRL makes OpenSSL refuse EVERY client presenting a certificate under that issuer -- not + merely revoked ones -- so an unrefreshed CRL is a total interface outage, and the fix is a + PKI refresh rather than a reissue. Emitting both down one method would give an operator one + string for two causes with opposite remedies.""" + ... + def secret_rotation_due( self, name: str, @@ -301,6 +314,28 @@ def cert_expiry(self, name: str, *, path: str, not_after: str, days_remaining: i not_after, ) + def crl_expiry(self, name: str, *, path: str, not_after: str, days_remaining: int) -> None: + # An EXPIRED crl refuses every client, so it is an ERROR rather than a warning: the listener + # is effectively down, not merely approaching a deadline (BACKLOG #1005). + if days_remaining < 0: + log.error( + "crl_expiry: %r CRL EXPIRED at %s (%d day(s) ago) โ€” this listener refuses EVERY " + "client until it is refreshed: %s", + name, + not_after, + -days_remaining, + path, + ) + else: + log.warning( + "crl_expiry: %r CRL expires at %s (%d day(s) left); refresh it before then or the " + "listener will refuse every client: %s", + name, + not_after, + days_remaining, + path, + ) + def secret_rotation_due( self, name: str, diff --git a/messagefoundry/pipeline/cert_expiry.py b/messagefoundry/pipeline/cert_expiry.py index 8f37cabc..409ccd2e 100644 --- a/messagefoundry/pipeline/cert_expiry.py +++ b/messagefoundry/pipeline/cert_expiry.py @@ -33,7 +33,7 @@ from messagefoundry.config.settings import CertMonitorSettings from messagefoundry.pipeline.alerts import AlertSink, LoggingAlertSink -from messagefoundry.pki import read_cert_facts +from messagefoundry.pki import read_cert_facts, read_crl_facts if TYPE_CHECKING: from messagefoundry.config.wiring import Registry @@ -61,6 +61,11 @@ class MonitoredCert: label: str path: str + #: ``"cert"`` or ``"crl"`` (BACKLOG #1005). Defaults so every existing construction is unchanged. + #: A CRL is watched by the same monitor because the operator question is identical -- "is a file + #: I depend on about to expire" -- but it alerts down a SEPARATE sink method, because an expired + #: CRL refuses every client rather than degrading one identity. + kind: str = "cert" @dataclass(frozen=True) @@ -72,6 +77,7 @@ class CertCheck: path: str not_after_iso: str days_remaining: int + kind: str = "cert" @property def expired(self) -> bool: @@ -123,6 +129,13 @@ def certs_from_registry( ib_path = ib.spec.settings.get("tls_cert_file") if isinstance(ib_path, str) and ib_path: certs.append(MonitoredCert(ib.name, ib_path)) + # BACKLOG #1005: a configured CRL expires like a certificate, and unrefreshed it takes + # the listener DOWN -- past nextUpdate OpenSSL refuses every client, not just revoked + # ones. Inbound only: a CRL verifies the peers we REQUIRE certificates from, and only an + # inbound listener does that. + ib_crl = ib.spec.settings.get("tls_crl_file") + if isinstance(ib_crl, str) and ib_crl: + certs.append(MonitoredCert(ib.name, ib_crl, kind="crl")) for ob in registry.outbound.values(): ob_path = ob.spec.settings.get("tls_cert_file") if isinstance(ob_path, str) and ob_path: @@ -245,6 +258,19 @@ def run_once(self, now: float | None = None) -> list[CertCheck]: continue checks.append(check) if check.days_remaining <= self._settings.warn_days: + if check.kind == "crl": + try: + self._alert_sink.crl_expiry( + check.label, + path=check.path, + not_after=check.not_after_iso, + days_remaining=check.days_remaining, + ) + except Exception: + log.warning( + "crl_expiry alert sink failed for %r", check.label, exc_info=True + ) + continue # The sink never raises (contract), but be defensive โ€” one bad sink call must not # abort the scan of the remaining certs. try: @@ -264,6 +290,15 @@ def _inspect(self, cert: MonitoredCert, now: float) -> CertCheck | None: try: with open(cert.path, "rb") as fh: pem = fh.read() + if cert.kind == "crl": + crl_facts = read_crl_facts(pem, now=now) + return CertCheck( + label=cert.label, + path=cert.path, + not_after_iso=crl_facts.next_update_iso, + days_remaining=crl_facts.days_remaining, + kind="crl", + ) facts = read_cert_facts(pem, now=now) except FileNotFoundError: log.warning("cert_expiry: certificate for %r not found: %s", cert.label, cert.path) diff --git a/messagefoundry/pipeline/wiring_runner.py b/messagefoundry/pipeline/wiring_runner.py index 3b776e6b..cf9af681 100644 --- a/messagefoundry/pipeline/wiring_runner.py +++ b/messagefoundry/pipeline/wiring_runner.py @@ -2199,6 +2199,10 @@ async def _start_inbound_unsafe(self, name: str) -> None: allow_insecure_bind=self._allow_insecure_bind, posture=self._hop_posture, ) + # #1005: the revocation sibling of the four gates above. Separate because it fires on the + # opposite condition -- those refuse a listener with NO TLS, this one refuses a listener + # whose TLS is correct but whose client certificates are never checked for revocation. + check_inbound_revocation(source_cfg, ic.name, posture=self._hop_posture) # ADR 0154 D7, immediately after its confidentiality sibling and deliberately separate from it: # check_http_tls_exposure returns early whenever tls is truthy, which is exactly the case an # authentication requirement most needs to cover. No allow_insecure_bind is passed โ€” a @@ -6747,6 +6751,81 @@ def _inbound_insecure_bind_permitted( return not (posture.enforcing and posture.is_phi) +def _inbound_revocation_gap_permitted(*, attested: bool, posture: HopPosture | None) -> bool: + """Whether a VERIFYING inbound mTLS listener that checks NO revocation may bind (warn-and-cross) + rather than being REFUSED (BACKLOG #1005). The revocation sibling of + :func:`_inbound_insecure_bind_permitted`, and deliberately the same three rungs in the same order. + + A per-connection ``tls_revocation_attested`` permits it -- the operator declaring that a + revocation-checking PKI covers these certificates outside the engine, exactly as the outbound + ``Destination.tls_revocation_attested`` does for a verified outbound hop. + + An **unstamped** posture (``None``) permits it, for the same reason the sibling does: the check + ran outside the ENFORCED gate, so this is a direct / embedding call and must never acquire a new + refusal there. Otherwise it is refused only on an instance that is BOTH enforcing AND PHI -- + every other instance warns and crosses. + + There is deliberately NO blunt process-wide escape here. ``MEFOR_ALLOW_INSECURE_TLS`` governs + weakened TLS, and a listener that verifies its peers correctly but does not check revocation is + not a weakened-TLS hop; reusing that escape would let one env var silence a control it was never + scoped to.""" + if attested: + return True + if posture is None: + return True # un-postured (direct/embedding) call: never a new refusal (see above) + return not (posture.enforcing and posture.is_phi) + + +def check_inbound_revocation( + source: Source, name: str, *, posture: HopPosture | None = None +) -> None: + """Exposed-gate sibling (BACKLOG #1005, ASVS 12.1.4 band B1): refuse an mTLS listener that + verifies client certificates but checks NO revocation, on an enforcing production-PHI instance. + + **Measured on this tree**: the three server builders load a CA, set ``CERT_REQUIRED`` and finish + with ``harden_verify_flags`` -- strict RFC 5280 path validation, NOT revocation -- so a client + certificate revoked this morning keeps authenticating until its ``notAfter``. Set + ``tls_crl_file`` on the connection (a PEM carrying the CA and its CRL), or declare + ``tls_revocation_attested=true`` if your PKI checks revocation outside the engine. + + **Why this refusal cannot be delegated away for two of the three listeners.** + ``harden_verify_flags``' own docstring delegates live revocation to the deploying org -- OCSP + must-staple at a proxy plus the OS trust store. That is credible for the API/UI surface. **An + HTTP proxy can terminate neither MLLP framing nor DIMSE**, so for those two the named delegation + does not reach and no workaround remains. + + Applies only where an mTLS listener exists: MLLP (which also serves the inbound HTTP listener), + HTTP and DIMSE. Raw TCP/X12 have no TLS option at all, so they cannot have a client certificate + to revoke.""" + if source.type not in (ConnectorType.MLLP, ConnectorType.HTTP, ConnectorType.DIMSE): + return + settings = source.settings + # No mTLS means no client certificate is requested, so there is nothing whose revocation could + # matter -- the same composition rule the outbound verify-off arm states. + if not settings.get("tls") or not settings.get("tls_ca_file"): + return + if settings.get("tls_crl_file"): + return + if _inbound_revocation_gap_permitted(attested=source.tls_revocation_attested, posture=posture): + log.warning( + "inbound %r requires and verifies a client certificate (mTLS) but checks NO revocation: " + "a revoked partner certificate would keep authenticating until its notAfter. Set " + "tls_crl_file on the connection, or tls_revocation_attested=true if your PKI checks " + "revocation outside the engine.", + name, + ) + return + raise WiringError( + f"inbound connection {name!r} requires and verifies a client certificate (mTLS) but checks " + "no revocation, on an enforcing production-PHI instance; a partner certificate revoked " + "today would keep authenticating to this interface until its notAfter. Set tls_crl_file " + "(a PEM carrying the CA and its CRL) on the connection, or set " + "tls_revocation_attested=true if a revocation-checking PKI covers these certificates " + "outside the engine. An HTTP proxy can terminate neither MLLP nor DIMSE, so for those " + "listeners the documented out-of-engine delegation does not reach." + ) + + def check_mllp_tls_exposure( source: Source, name: str, *, allow_insecure_bind: bool, posture: HopPosture | None = None ) -> None: diff --git a/messagefoundry/pki.py b/messagefoundry/pki.py index 2c500b45..b56fffed 100644 --- a/messagefoundry/pki.py +++ b/messagefoundry/pki.py @@ -92,6 +92,61 @@ def key_to_pem(key: PrivateKeyTypes) -> bytes: ) +@dataclass(frozen=True) +class CrlFacts: + """Read-only public facts about one certificate revocation list (BACKLOG #1005). + + The revocation sibling of :class:`CertFacts`, and it keeps that class's conventions exactly: + ``days_remaining`` is negative once the CRL is past ``nextUpdate``, and ``expired`` is precisely + ``days_remaining < 0``. Same ``86_400`` s/day arithmetic as the expiry monitor. + + **Why a CRL's expiry is an AVAILABILITY fact and not a security one.** Past ``nextUpdate`` + OpenSSL refuses EVERY client presenting a certificate under that issuer, not merely revoked + ones -- measured on CPython 3.14.6 / OpenSSL 3.5.7, verify error 12 ``CRL has expired``. So a + CRL nobody refreshed converts a PKI housekeeping lapse into a total interface outage whose + first symptom is every partner dropping at once. That is why this is read at construction and + alarmed on before expiry, rather than discovered at a partner handshake.""" + + issuer: str + next_update_iso: str + days_remaining: int + + @property + def expired(self) -> bool: + return self.days_remaining < 0 + + +def read_crl_facts(pem: bytes, *, now: float) -> CrlFacts: + """Parse a PEM CRL into its public inventory facts, evaluated at ``now`` (epoch seconds). + + Accepts a bundle: a file may concatenate the issuing CA and its CRL, which is exactly the shape + ``harden_crl_check`` loads through ``cafile=``. The FIRST ``X509 CRL`` block is read and any + certificate blocks are skipped, so the same path serves a bare ``.crl`` and a CA+CRL bundle. + + Raises ``ValueError`` when the bytes carry no CRL at all -- a configured-but-CRL-less file must + never degrade to "revocation checking silently off".""" + marker = b"-----BEGIN X509 CRL-----" + end = b"-----END X509 CRL-----" + start = pem.find(marker) + if start < 0: + raise ValueError("no CRL found in the supplied PEM (expected an 'X509 CRL' block)") + stop = pem.find(end, start) + if stop < 0: + raise ValueError("truncated CRL: an 'X509 CRL' block opened but never closed") + crl = x509.load_pem_x509_crl(pem[start : stop + len(end)]) + nxt = crl.next_update_utc + if nxt is None: + # RFC 5280 makes nextUpdate optional, but OpenSSL treats a CRL without one as never + # expiring, which would silence the freshness control entirely. Refuse rather than + # inherit an unbounded lifetime. + raise ValueError("the CRL carries no nextUpdate, so its freshness cannot be checked") + return CrlFacts( + issuer=crl.issuer.rfc4514_string(), + next_update_iso=nxt.isoformat(), + days_remaining=int((nxt.timestamp() - now) // 86_400), + ) + + def read_cert_facts(pem: bytes, *, now: float) -> CertFacts: """Parse a PEM certificate into its public inventory facts, evaluated at ``now`` (epoch seconds). diff --git a/messagefoundry/store/sqlserver.py b/messagefoundry/store/sqlserver.py index 8e5a061c..968ba65e 100644 --- a/messagefoundry/store/sqlserver.py +++ b/messagefoundry/store/sqlserver.py @@ -1358,8 +1358,16 @@ def __init__(self, conn: Any, cur: Any) -> None: approver NVARCHAR(256) NULL, decided_at FLOAT NULL, expires_at FLOAT NULL)""", """IF INDEXPROPERTY(OBJECT_ID('pending_approvals'),'ix_pending_approvals_status','IndexID') IS NULL CREATE INDEX ix_pending_approvals_status ON pending_approvals(status, requested_at)""", + # BACKLOG #1268: `username` carries the same binary collation as every other identifier column in + # this schema, and it is the one the engine authenticates against. Without it the column inherits + # the DATABASE default -- case-INsensitive on a stock install (SQL_Latin1_General_CP1_CI_AS) -- + # while SQLite (BINARY) and Postgres (TEXT) are both case-SENSITIVE. That made `Admin` and `admin` + # two accounts on two backends and one account on the third, under a UNIQUE constraint that reads + # as if it had settled the question. Pinning it here makes account identity a property of the + # ENGINE rather than of whichever collation an operator's database happened to be created with. """IF OBJECT_ID('users','U') IS NULL CREATE TABLE users ( - id NVARCHAR(64) NOT NULL PRIMARY KEY, username NVARCHAR(256) NOT NULL UNIQUE, + id NVARCHAR(64) NOT NULL PRIMARY KEY, + username NVARCHAR(256) COLLATE Latin1_General_100_BIN2 NOT NULL UNIQUE, auth_provider NVARCHAR(16) NOT NULL, display_name NVARCHAR(256) NULL, email NVARCHAR(256) NULL, disabled BIT NOT NULL DEFAULT 0, created_at FLOAT NOT NULL, updated_at FLOAT NOT NULL, last_login_at FLOAT NULL, password_hash NVARCHAR(512) NULL, diff --git a/messagefoundry/transports/dicom.py b/messagefoundry/transports/dicom.py index 7409f189..0fdaac86 100644 --- a/messagefoundry/transports/dicom.py +++ b/messagefoundry/transports/dicom.py @@ -61,6 +61,7 @@ TrustAnchorPolicy, build_verifying_client_context, harden_cipher_suites, + harden_crl_check, harden_kex_groups, harden_verify_flags, relax_verify_expiry, @@ -142,6 +143,11 @@ def _server_ssl_context(s: dict[str, Any]) -> ssl.SSLContext | None: if ca: # opt-in mTLS: require + verify a calling peer's client cert against this trust anchor ctx.load_verify_locations(cafile=str(ca)) ctx.verify_mode = ssl.CERT_REQUIRED + # Opt-in revocation (#1005), after the CA load and inside the mTLS branch -- see mllp.py. + # An HTTP proxy can terminate neither DIMSE nor MLLP, so for this listener the documented + # out-of-engine delegation does not reach and there is no workaround. + if crl := s.get("tls_crl_file"): + harden_crl_check(ctx, str(crl)) harden_kex_groups(ctx) # pin approved ECDHE groups where supported (ASVS 11.6.2) harden_cipher_suites(ctx, connector="DICOM listener") # assert forward secrecy (ASVS 12.1.2) harden_verify_flags(ctx) # strict RFC 5280 validation of any mTLS client cert (ASVS 12.1.4) diff --git a/messagefoundry/transports/direct.py b/messagefoundry/transports/direct.py index 290b3f64..6dfc1151 100644 --- a/messagefoundry/transports/direct.py +++ b/messagefoundry/transports/direct.py @@ -231,6 +231,21 @@ def __init__(self, config: Destination) -> None: "Direct destination sends SMTP AUTH credentials over an UNVERIFIED TLS session " "(tls_verify=false); refused โ€” credentials require a verified TLS session" ) + else: + # BACKLOG #1314: the THIRD weakening axis. The chain IS verified here, but with + # `tls_check_hostname=false` the peer NAME is not, so any certificate chaining to the + # configured anchor is accepted whatever it was issued to -- the AUTH exchange then + # hands the credential to a peer whose identity was never established. + # + # ABSOLUTE, like the two arms above, and deliberately keyed on no escape: in both of + # them the escape governs the BODY posture and never the CREDENTIAL. A third arm keeps + # that split, so a hop cannot attest its way to a credentialed unverified-name session. + if not self.tls_check_hostname and self.username is not None: + raise ValueError( + "Direct destination sends SMTP AUTH credentials over a TLS session whose peer " + "NAME is unverified (tls_check_hostname=false); refused โ€” credentials " + "require a session bound to the host, not merely to the trust anchor" + ) # Built once at construction (fail-fast), reused by every send. None when TLS is off entirely. # DIRECT does not take a RevocationHopGuard even though the hop now verifies: adding it would # make the enumerated count eight and force four "seven verifying hops" docs to change, and the diff --git a/messagefoundry/transports/email.py b/messagefoundry/transports/email.py index b590d1ef..38a4dfd5 100644 --- a/messagefoundry/transports/email.py +++ b/messagefoundry/transports/email.py @@ -210,6 +210,20 @@ def __init__(self, config: Destination) -> None: # whose revocation status could matter, and the composition rule forbids two gates deciding # the same hop (docs/DEPLOYMENT.md ยง"composition"). The refusal above is that hop's gate. else: + # BACKLOG #1314: the THIRD weakening axis. The chain IS verified here, but with + # `tls_check_hostname=false` the peer NAME is not, so any certificate chaining to the + # configured anchor is accepted whatever it was issued to -- the AUTH exchange then + # hands the credential to a peer whose identity was never established. + # + # ABSOLUTE, like the two arms above, and deliberately keyed on no escape: in both of + # them the escape governs the BODY posture and never the CREDENTIAL. A third arm keeps + # that split, so a hop cannot attest its way to a credentialed unverified-name session. + if not self.tls_check_hostname and self.username is not None: + raise ValueError( + "Email destination sends SMTP AUTH credentials over a TLS session whose peer NAME is " + "unverified (tls_check_hostname=false); refused โ€” credentials require a " + "session bound to the host, not merely to the trust anchor" + ) # #201 (ADR 0078 amendment): the hop now genuinely verifies the server cert (#323 โ€” it did # not before; smtplib's default context is ssl._create_unverified_context), but stdlib ssl # does NO OCSP/CRL revocation, so a revoked-but-unexpired SMTP-server cert is still diff --git a/messagefoundry/transports/mllp.py b/messagefoundry/transports/mllp.py index e589a504..23323121 100644 --- a/messagefoundry/transports/mllp.py +++ b/messagefoundry/transports/mllp.py @@ -49,6 +49,7 @@ current_hop_posture, enforce_insecure_hop, harden_cipher_suites, + harden_crl_check, harden_kex_groups, harden_verify_flags, insecure_hop_disposition, @@ -549,6 +550,12 @@ def _mllp_ssl_context( if ca: # opt-in mTLS: require + verify a client cert against this trust anchor ctx.load_verify_locations(cafile=ca) ctx.verify_mode = ssl.CERT_REQUIRED + # Opt-in revocation (#1005). AFTER the CA load, because the CRL goes into the same + # trust store. Only meaningful under mTLS -- with no client cert required there is + # nothing to revoke. Covers the inbound HTTP listener too: it calls this builder + # (http_listener.py), so one wiring serves two listeners. + if crl := s.get("tls_crl_file"): + harden_crl_check(ctx, str(crl)) harden_kex_groups(ctx) # pin approved ECDHE groups where supported (ASVS 11.6.2) harden_cipher_suites(ctx, connector="MLLP listener") # assert forward secrecy (ASVS 12.1.2) harden_verify_flags(ctx) # strict RFC 5280 validation of any mTLS client cert (ASVS 12.1.4) diff --git a/scripts/docs/backlog_status_check.py b/scripts/docs/backlog_status_check.py index 4331946d..022d86ed 100644 --- a/scripts/docs/backlog_status_check.py +++ b/scripts/docs/backlog_status_check.py @@ -283,6 +283,25 @@ def main(argv: list[str] | None = None) -> int: print(f"ERROR: --backlog {p} does not exist", file=sys.stderr) return 1 + # BACKLOG #1259: surface the conflict refusal as a REPORT, not a traceback, and NAME THE FILE. + # + # `parse_items` already refuses a source carrying conflict markers, and that refusal is what makes + # this file safe to read. But it raises from inside `scan`, so every caller -- the CI leg and now + # the pre-commit hook -- rendered it as an uncaught ValueError. Two costs, and the second is the + # one that matters: a traceback reads as *the checker is broken* rather than *your ledger is + # conflicted*, which sends an author to the wrong file; and the exception carries a LINE number + # but no PATH, so with several sources scanned it does not say which one to open. + # + # Parsing each source here also removes a real double-parse -- the count and the scanned-list + # below each called `parse_items` again on every source. + parsed: list[tuple[str, int]] = [] + for label, text in sources: + try: + parsed.append((label, len(parse_items(text)))) + except ValueError as exc: + print(f"ERROR: {label}: {exc}", file=sys.stderr) + return 1 + changelog = args.changelog.read_text(encoding="utf-8") if args.changelog else None errors, warnings = scan(sources, changelog) @@ -291,8 +310,8 @@ def main(argv: list[str] | None = None) -> int: for e in errors: print(f"ERROR: {e}", file=sys.stderr) - n = sum(len(parse_items(text)) for _, text in sources) - scanned = ", ".join(f"{label} ({len(parse_items(text))})" for label, text in sources) + n = sum(count for _, count in parsed) + scanned = ", ".join(f"{label} ({count})" for label, count in parsed) if args.min_items is not None and n < args.min_items: # Printed to stderr *with the file list*, because "which files did you actually read" is the diff --git a/scripts/docs/dangling_citation_check.py b/scripts/docs/dangling_citation_check.py index 39a7248f..26d9a4cd 100644 --- a/scripts/docs/dangling_citation_check.py +++ b/scripts/docs/dangling_citation_check.py @@ -10,6 +10,20 @@ exists in neither is invisible there and is this one's. Neither subsumes the other, and the near- identical names are the reason this paragraph is the first thing in the file. +AND THE NAMES DIVERGE ON WIRING TOO, WHICH IS THE HALF THAT MISLEADS. That sibling IS run in CI +(`.github/workflows/backlog-hygiene.yml`, and its test is named in `ci.yml`'s docs-lane list). THIS +SCRIPT IS RUN BY NO WORKFLOW AND NO PRE-COMMIT HOOK -- it reaches CI only through +`tests/test_dangling_citation_check.py`, which is manifest-classified and therefore runs on the +tooling leg. Measured, with a positive control in the same pass: `dangling_citation_check` returns +ZERO hits across `.github/` and `.pre-commit-config.yaml`, while `backlog_status_check`, `ledger_check` +and `scan_forbidden` return 3, 2 and 3 -- so the probe finds wiring where wiring exists. + +WHY THAT IS WORTH A PARAGRAPH RATHER THAN LEFT TO BE LOOKED UP: a grep for `citation_check` HITS A +WORKFLOW, and the hit belongs to the sibling. The answer is confident, well-formed, and wrong in the +direction of believing a change here is CI-covered when it is not. Anyone reasoning about whether a +behaviour change in this file can red a build must grep the FULL name, and should treat a bare +`citation_check` match as evidence about the other script until they have checked which one it named. + WHAT THE DEFECT IS. While a number names nothing, a citation to it resolves to NOTHING -- honest and harmless, and it advertises its own brokenness. If that number is later issued, the citation starts resolving to unrelated work and NOTHING anywhere reports a problem. The ledger's own erratum names @@ -115,6 +129,16 @@ def _load_backlog_module() -> object: return module +#: The scan's coverage bound, stated ONCE and printed on EVERY exit path (BACKLOG #1235). +#: It used to be a literal inside the hits branch, so a CLEAN run never showed it -- the bound was +#: absent at exactly the moment a reader concludes "clean", which is the population it qualifies. +#: Naming it is not tidiness: two copies of a caveat drift, and the copy that goes stale is the one +#: nobody reads because it only prints on the path they are not on. +_COVERAGE_BOUND = ( + "Not scanned: the private companion repository, where a citation is invisible to this repo." +) + + def allocation_floor(sources: list[Path] | None = None) -> int: """The highest item number the ledgers know about. Nothing at or below it can ever be issued. @@ -241,11 +265,49 @@ def main(argv: list[str] | None = None) -> int: paths = args.paths or sorted(Path("docs").rglob("*.md")) allocated = allocated_numbers() + + # BACKLOG #1235: REFUSE AN EMPTY POPULATION INSTEAD OF REPORTING IT CLEAN. + # + # The default path list is `Path("docs").rglob(...)` -- relative to the CURRENT WORKING DIRECTORY, + # not the repo root. MEASURED from a directory with no `docs/`: this printed + # No unresolved backlog citation in 0 file(s). + # Resolved against 0 allocated item numbers (open and closed). + # and exited 0. **Both counts were zero and nothing objected.** A green that is a statement about + # the ENVIRONMENT rather than the SUBJECT, which is worse than an untested gate because it closes + # the question instead of inviting it. + # + # THIS IS THE TRAP UNDER THE ITEM'S FIRST DISJUNCT, not a hypothetical: the >200-file population + # floor lives ONLY in `test_the_docs_scan_actually_covers_something`, so it guards the PYTEST arm + # and nothing else. Anyone who satisfies the item by wiring this CLI into `.github/` or + # `.pre-commit-config.yaml` -- the documented way to close it -- inherits a gate that reports clean + # when it scanned nothing, and inherits it precisely BECAUSE they closed the item. + # + # Refusing on an empty ALLOCATION as well as an empty PATH list is deliberate: an unreadable ledger + # resolves every citation to "no filed item", which fails the other way and would bury a real run + # in false positives. Both zeros mean the same thing -- the tool is not looking at this repository. + if not paths or not allocated: + print( + f"ERROR: refusing to report on an empty population -- {len(paths)} file(s) scanned, " + f"{len(allocated)} allocated item number(s) resolved.\n" + f" scanned from: {Path.cwd()}\n" + " The default path list is CWD-relative, so this is what running outside the repo " + "root looks like.\n" + " A clean result over nothing is not a clean result. Pass paths explicitly, or run " + "from the repo root.", + file=sys.stderr, + ) + return 1 + hits = unresolved_citations(list(paths), allocated) if not hits: print(f"No unresolved backlog citation in {len(paths)} file(s).") print(f"Resolved against {len(allocated)} allocated item numbers (open and closed).") + # BACKLOG #1235: THE COVERAGE BOUND PRINTS ON A CLEAN RUN TOO. It used to sit only after the + # hits loop, so this early return skipped it -- absent at exactly the moment a reader concludes + # "clean", which is the whole population the bound exists to qualify. It is in the docstring, + # and a docstring is not what a CI log shows. + print(_COVERAGE_BOUND) return 0 floor = allocation_floor() @@ -258,13 +320,35 @@ def main(argv: list[str] | None = None) -> int: ) print(f"{hit.path}:{hit.lineno}: #{hit.number} resolves to no filed item{note}") print(f" {hit.line}") - state = ( - f"BELOW THE FLOOR ({floor}) -- the allocator only ever issues above its high-water " - "mark, so this number can NEVER be issued and the citation is permanently harmless." - if hit.number <= floor - else f"ABOVE THE FLOOR ({floor}) -- this number CAN still be issued to unrelated work. " - "This is the live shape." - ) + # BACKLOG #1235: THE ANNOTATION ASKS `is_live_shape`, THE SAME PREDICATE THE EXIT CODE ASKS. + # + # It used to branch on `hit.number <= floor` ALONE and never consult `pr_shaped` -- a THIRD + # definition of the live-shape rule, beside the exit list and the test's copy, and it + # DISAGREED IN PRODUCTION rather than in principle. MEASURED at 4c28badd: six hits printed + # BOTH `[PR/issue/foreign-repo shaped -- very likely NOT a backlog citation]` AND `This is + # the live shape.` -- two contradictory annotations on the SAME hit, two lines apart -- while + # the process exited 0. A reader saw six live-shape citations above a green gate. + # + # It survived the single-definition refactor that is this item's own subject, which is the + # part worth keeping: `is_live_shape` was extracted and the exit list and the test were both + # repointed at it, while the branch that PRINTS the verdict to a human was left behind. The + # definitions a tool COMPUTES with get unified; the one it NARRATES with is easy to miss + # precisely because no assertion reads it. + if hit.number <= floor: + state = ( + f"BELOW THE FLOOR ({floor}) -- the allocator only ever issues above its high-water " + "mark, so this number can NEVER be issued and the citation is permanently harmless." + ) + elif not is_live_shape(hit, floor): + state = ( + f"ABOVE THE FLOOR ({floor}), but NOT the live shape -- see the annotation above. " + "The exit code passes over this hit deliberately." + ) + else: + state = ( + f"ABOVE THE FLOOR ({floor}) -- this number CAN still be issued to unrelated work. " + "This is the live shape." + ) print(f" -> {state}") distinct = sorted({hit.number for hit in hits}) @@ -283,9 +367,7 @@ def main(argv: list[str] | None = None) -> int: "This is an UPPER BOUND ON CITATIONS, NOT A COUNT OF DEFECTS -- some hits are very likely" ) print("not backlog references at all. Each is printed above with its source line; judge them.") - print( - "Not scanned: the private companion repository, where a citation is invisible to this repo." - ) + print(_COVERAGE_BOUND) # FAIL CLOSED, ON THE LIVE SHAPE ONLY (BACKLOG #1235). The first version of this took `--fail` # as opt-in and nothing passed it, so a planted dangling citation was reported correctly AND the # process still exited 0 -- a checker that cannot fail is not a check, which is the defect this diff --git a/scripts/hooks/worktree_gate.ps1 b/scripts/hooks/worktree_gate.ps1 index e7133498..27d0cd01 100644 --- a/scripts/hooks/worktree_gate.ps1 +++ b/scripts/hooks/worktree_gate.ps1 @@ -344,7 +344,31 @@ function Get-GitTargetCandidatesRaw([string]$Line, [string]$Prefix, [string]$Cwd # # Three false positives came from scanning the raw string: a two-line command whose second line read # `echo about to merge stuff` denied with verb=merge; `echo "git checkout main"` denied; and -function Remove-QuotedSpans([string]$s) { +function Remove-QuotedSpans([string]$s, [bool]$PosixEscapes = $false) { + <# + ``$PosixEscapes`` -- DOES THIS HOST TREAT A BACKSLASH AS AN ESCAPE? (BACKLOG #1229 residual, second + round.) `sh` does; **PowerShell does NOT** -- its escape is the BACKTICK, so `"C:\Temp\"` is a + COMPLETE string there and whatever follows it RUNS. + + THIS PARAMETER EXISTS BECAUSE ITS ABSENCE RE-CREATED #1229's OWN DEFECT ON THE OTHER HOST. The + first version of this fix honoured the escape unconditionally, which is correct POSIX -- but line + 999 scans BOTH tool names through ONE matcher, so on a PowerShell payload the scan held a span open + that PowerShell had already closed, straddled the live command between it and a later quote, and + blanked it. MEASURED on the shipped fix, both tool names: + + Write-Output "C:\Temp\" ; git -C reset --hard ; Write-Output "x" ALLOW + ... same line with ONE FEWER backslash (control) DENY + ... same line with TWO backslashes (even count) DENY + + An ODD count before the closer was the trigger. Verified the middle statement really executes with + an inert payload that COMPUTES rather than echoes, so an echo-back could not be mistaken for a run. + + DEFAULT IS $false, AND THE DIRECTION IS THE WHOLE POINT. Honouring the escape makes spans LONGER, + so it BLANKS MORE and can hide a command -- fail OPEN. Refusing it makes spans shorter, leaving more + text visible to the rules -- fail CLOSED. An unknown or unrecognised host therefore gets the + conservative reading, and only a host known to use backslash escapes opts in. + #> + <# Blank every quoted span in ONE LEFT-TO-RIGHT PASS, so the quote that OPENS FIRST owns the span and the other quote character is an ordinary literal inside it -- which is what a POSIX shell does. @@ -375,12 +399,67 @@ function Remove-QuotedSpans([string]$s) { for ($i = 0; $i -lt $s.Length; $i++) { $ch = $s[$i] if ($quote -eq [char]0) { - if ($ch -eq '"' -or $ch -eq "'") { $quote = $ch; $openAt = $i } + # A BACKSLASH ESCAPE OUTSIDE A SPAN IS A LITERAL AND OPENS NOTHING (BACKLOG #1229 + # residual). `\"` is an ordinary character to the shell, so the command around it RUNS -- + # but this scan treated it as an opener, paired it with the next escaped quote, and blanked + # the live command between them. Same straddle as the two-regex defect above, one character + # class over, and RULE-AGNOSTIC: it disarms whatever rule sits behind it, so it hid + # `reset --hard` and `worktree add` and not only `checkout`. + if ($PosixEscapes -and $ch -eq '\' -and $i + 1 -lt $s.Length) { + [void]$out.Append($ch); [void]$out.Append($s[$i + 1]); $i++ + } + elseif ($ch -eq '"' -or $ch -eq "'") { $quote = $ch; $openAt = $i } else { [void]$out.Append($ch) } } + elseif ($PosixEscapes -and $quote -eq '"' -and $ch -eq '\' -and $i + 1 -lt $s.Length) { + # Inside a DOUBLE-quoted span a backslash escapes the next character, so `\"` does not + # close it. DELIBERATELY NOT APPLIED INSIDE A SINGLE-QUOTED SPAN: sh gives the backslash no + # special meaning there, so `'a\'` really does close at that quote. Treating the two alike + # would swallow the rest of the line from a trailing backslash -- fail-open, which is the + # direction this whole function exists to avoid. + $i++ + } elseif ($ch -eq $quote) { - # Emit the blanked pair only on a CLOSED span, matching what the regexes produced. - [void]$out.Append($quote); [void]$out.Append($quote) + # A QUOTED PROGRAM PATH KEEPS ITS GIT TOKEN, DECIDED HERE RATHER THAN IN A PRE-PASS + # (BACKLOG #1229 residual). This used to be two regexes run BEFORE this scan, double quotes + # first -- which is the same ordered-pair shape the scan replaced, so it straddled the same + # way: `"` ... `/git"` paired ACROSS a live command and collapsed it to a bare `git`, + # stripping the verb and its arguments so no rule matched. Deciding it on a span this scan + # already OWNS means it cannot pair across anything. + # + # CASE-SENSITIVE, AND THE CASE-INSENSITIVE VERSION IS A RETRACTION RATHER THAN AN + # OVERSIGHT (owner ruling 2026-08-21). This site briefly used `-match` plus + # `.ToLowerInvariant()`, on the reasoning that `GIT.EXE` is a real Windows spelling the + # case-SENSITIVE rules downstream would otherwise skip, so canonicalising was the + # fail-CLOSED direction. That reasoning is sound in isolation and was withdrawn on + # measurement, because this emit does not only ever see programs: + # + # "<...>\Git\bin\GIT.EXE" -C reset --hard a PROGRAM. Should deny. + # cp -r "/c/backups/Git" restore a PATH. Must not deny. + # + # A case-insensitive match cannot tell those apart -- both end in separator-then-`Git` -- + # so canonicalising minted a `git` token for the second and the next ordinary word became + # its verb. MEASURED: 12 shapes DENY here and ALLOW on `origin/main` (`cp`, `mv`, `ls`, + # `rsync`, `find -exec`, `7z`, `echo`, `python --src`, `Copy-Item`, `Move-Item`), against + # ZERO fail-opens gained. Twelve daily false denies on the guard itself is what buys a + # gate disabled wholesale, which is the failure this file's own preamble names. + # + # WHAT THIS DELIBERATELY DOES NOT FIX, stated because a one-sided note reads as a clean + # win: the quoted `GIT.EXE`-as-PROGRAM spelling stays ALLOW. That is NOT a regression -- + # `origin/main` allows it today, measured -- it is a pre-existing hole this change + # declines to close, because the only remedy tried costs the twelve above. Closing it + # needs a POSITION test (is this span a program or an argument). That was built, and + # measured to open `cmd /c ""` and PowerShell dot-source as NEW fail-opens that + # main denies, so it was reverted. Do NOT re-add the lowercase emit without that + # discriminator, and do not add the discriminator without re-measuring those two. + $span = $s.Substring($openAt + 1, $i - $openAt - 1) + if ($span -cmatch '[\\/](git(?:\.exe)?)$') { + [void]$out.Append($Matches[1]) + } + else { + # Emit the blanked pair only on a CLOSED span, matching what the regexes produced. + [void]$out.Append($quote); [void]$out.Append($quote) + } $quote = [char]0; $openAt = -1 } } @@ -400,7 +479,82 @@ function Remove-QuotedSpans([string]$s) { # Each entry carries BOTH forms. Scan is for deciding whether a git verb is present; Raw is for parsing # PATHS out of the same line, since the blanking that stops a commit message supplying a verb would also # erase the path. -function Get-ScannableSegments([string]$Cmd) { +function Get-FlagOwner([string]$Left) { + <# + WHICH PROGRAM OWNS THE FLAG THAT WAS JUST MATCHED, and does it EXECUTE its argument? + (BACKLOG #1229 residual, fourth round.) Returns 'posix', 'win' or 'none'. + + THE FLAG SHAPE IS NOT THE QUESTION, AND IT IS BARELY CORRELATED WITH THE ANSWER. `$shFlag` is + `-[a-z]*c` under (?i), which matches `-C`, `-ic`, `-rc`, `-static`, `-sync`, `-exec` -- and + `$cmdExeFlag` walks an ordinary POSIX path one component at a time. Enumerated over a hand-built + axis of 33 non-interpreter invocations, 28 matched; over 36 real interpreter invocations, 18 did + not. Two consequences, and this function is the answer to the first: + + 1. A NON-INTERPRETER'S ARGUMENT WAS SCANNED AS CODE. `grep -c 'git reset --hard' history.log` + -- an ordinary search of a log -- DENIED. So did `rg -c`, `ag -c`, `curl -c`, `sort -c`, + `uniq -c`, `wc -c`, `cut -c`, `head -c`, `tail -c`, `ls -c`, `tar -c`, `gzip -c`, `md5sum -c`, + `cmp -c`, `diff -c`, `rsync -c`, `gcc -static` and `make -C`. None of them executes its + argument: driven on the real binaries with a payload that COMPUTES (`expr 111 \* 3` -> 333, + so an echo-back cannot be mistaken for a run), every one left no marker, while `bash -c`, + `sh -c` and `python -c` all printed 333. + 2. THE SPELLINGS IT MISSES STAY MISSED. `perl -e`, `node -e`, `ruby -e`, `awk`, `eval` and + `ssh host CMD` never match the flag pattern at all, so their payload is blanked as inert data. + Pre-existing, unchanged here, and NOT closed by this function -- it is asked only about flags + the matcher already found. + + WHY AN ALLOWLIST, WHEN THIS FILE'S OWN DOCTRINE PREFERS A GENERATING RULE. There is no syntactic + property separating `cp` from `sudo`, or `ls /usr/src/c` from `cmd /usr/src/c`; program identity is + the only discriminator, and identity cannot be generated. Read both sets as "AT LEAST these" + (CLAUDE.md section 11), and see the disclosed cost in the caller. + + THE SCAN IS BOUNDED AND IT CONTINUES LEFT. Bounding it at the last command separator is what stops + a program named before a `;` or a pipe from voting on this flag, and continuing left past bare + words is what keeps `su someone -c`, `docker run --rm img sh -c` and `cmd /d /Q/C` classified -- + each has a non-interpreter word between the interpreter and its flag. + + THAT IS WHY THE $cmdExeFlag NOTE ABOVE IS CORRECTED RATHER THAN CITED. It listed five shapes that + defeated five earlier program-token candidates: `echo hi;cmd /k`, `(cmd /mnt/c`, `cmd /d /Q/C`, an + ALIAS, and a RENAMED copy of cmd.exe. The first three break ADJACENCY -- every one of those + candidates asked whether the token IMMEDIATELY LEFT of the switch run was a cmd spelling -- and all + three were measured to DENY under this scan, which is a different instrument. The last two break + IDENTITY, and this function does not close them: an unknown name gets no recursion, which is the + disclosed cost recorded at the caller. Four of five, not five of five. + #> + # Both hosts take their code under `/c` as well as `-Command`, so a cmd-family match is theirs. + $winSet = @('pwsh', 'powershell', 'cmd', 'wsl') + # Anything that runs the string it is handed. `find` is NOT optional: `-exec` ends in `c`, so the + # matcher reaches it, and `find . -name x -exec '' \;` really executes -- dropping find from + # this list was measured to regress it from DENY to ALLOW. + $posixSet = @( + 'sh', 'bash', 'dash', 'zsh', 'ksh', 'ash', 'mksh', 'busybox', 'fish', 'csh', 'tcsh', + 'env', 'nohup', 'timeout', 'xargs', 'nice', 'ionice', 'setsid', 'stdbuf', 'script', + 'flock', 'watch', 'parallel', 'su', 'runuser', 'chroot', 'ssh', 'find', 'command', 'eval', + 'python', 'python3', 'py', 'perl', 'ruby', 'node', 'nodejs', 'php', 'lua', 'tclsh', + 'awk', 'gawk', 'mawk', 'deno', 'bun', 'osascript', 'rscript', 'julia' + ) + $seg = $Left -replace '(?s).*[;&|()`]', '' + # WRAPPED IN @() AND THAT IS LOAD-BEARING. On a single-token left context [regex]::Split returns a + # SCALAR string, so indexing it yields a [char], .StartsWith throws, every owner comes back 'none' + # and the caller then refuses ALL recursion -- including `bash -c`. That is a silent, total + # fail-open, and a prototype hit it. Quote characters are delimiters for the same class of reason: + # without them the inner `-c` of `bash -c 'bash -c ""'` reads its program as `'bash`. + $toks = @([regex]::Split($seg, '[\s"'']+') | Where-Object { $_ }) + for ($k = $toks.Count - 1; $k -ge 0; $k--) { + $t = $toks[$k] + if ($t.StartsWith('-')) { continue } # an option, not a program + # A SINGLE-COMPONENT slash token is a cmd switch (`/d`, `/Q`) and is skipped. A MULTI-component + # one is a POSIX path and IS a program -- `/usr/bin/bash -c` must still classify, so this + # cannot be a blanket "starts with a slash" skip. + if ($t -match '^/[^/]*$') { continue } + if ($t -match '^[A-Za-z_][A-Za-z0-9_]*=') { continue } # FOO=1, an assignment prefix + $name = ($t -split '[\\/]')[-1] -replace '(?i)\.exe$', '' + if ($winSet -contains $name.ToLowerInvariant()) { return 'win' } + if ($posixSet -contains $name.ToLowerInvariant()) { return 'posix' } + } + 'none' +} + +function Get-ScannableSegments([string]$Cmd, [bool]$PosixEscapes = $false) { # Fold line continuations FIRST, or the per-line split below separates `git \` from its verb and the # rule stops seeing the command at all. Prose does not end a line with a continuation character, so # this does not resurrect the `echo about to merge stuff` false positive. @@ -473,10 +627,15 @@ function Get-ScannableSegments([string]$Cmd) { # # COST, measured rather than assumed: recursion only ADDS a scan line, and a line still needs a git # token AND a gated verb to deny, so a path argument behind a family flag (`git -C ""`, - # `tar -C ""`) changes no verdict. The one class that widens is a search whose PATTERN spells a - # git command -- `grep -vc "git checkout main"` now denies where it did not. That class already - # existed for `-c` (`grep -c "git checkout main"` has always denied), so this adds members to it - # rather than creating it. + # `tar -C ""`) changes no verdict. + # + # THIS PARAGRAPH USED TO NAME A CLASS THAT NO LONGER EXISTS, and the correction matters more than + # the deletion would. It said the one widening was "a search whose PATTERN spells a git command -- + # `grep -vc "git checkout main"` now denies where it did not", and called that acceptable because + # `grep -c` had always denied. Both halves were true and the conclusion was wrong: the flag shape + # is not evidence of interpreter-ness at all, and 28 of 33 non-interpreter invocations matched it. + # A flag match is now a QUESTION, answered by Get-FlagOwner below, and a program that does not + # execute its argument gets no recursion. `grep -c 'git reset --hard' history.log` allows. # # --------------------------------------------------------------------------------------------- # @@ -589,12 +748,12 @@ function Get-ScannableSegments([string]$Cmd) { # the attached form it does, so the across-the-board relaxation reddens it and the per-host split does # not. Recorded because the useless probe LOOKED like the bound: it named the right risk, asserted the # right verdict, and could not fail. - # A KNOWN FALSE DENY THIS RULE COSTS, recorded HERE because this script is what gets installed to - # %USERPROFILE%\.claude\hooks and it travels without the tests -- a note that lives only in a test - # file is invisible to whoever is reading the installed copy: + # A FALSE DENY THIS RULE USED TO COST, kept here rather than deleted because the reasoning under it + # is what a later reader needs, and because this script is what gets installed to + # %USERPROFILE%\.claude\hooks and travels without the tests: # - # ls /usr/src/c "git checkout main" DENIES, and should not - # ls /usr/src/lib "git checkout main" ALLOWs (control: the `/c` ending is the trigger) + # ls /usr/src/c "git checkout main" DENIED, and should not have. Now ALLOWs. + # ls /usr/src/lib "git checkout main" ALLOWs (control: the `/c` ending was the trigger) # # THE CAUSE IS THE `(?:/[^/\s]+)*` CLUSTER PREFIX IN $cmdExeFlag, not the `\s*` separator beside # it. The prefix exists so cmd's CONCATENATED switch runs (`/Q/C`, `/V:ON/C`) are recognised, and @@ -619,17 +778,26 @@ function Get-ScannableSegments([string]$Cmd) { # `/usr` binds as `/U`, `/src` as `/S`, `/zzz` is ignored. So `(?:/[^/\s]+)*/[ck]` is very nearly # EXACTLY the family cmd accepts, not an over-match. # - # WHICH MAKES THIS A PROGRAM-IDENTITY PROBLEM, and that is why it is not fixed here. The only - # thing separating `ls /usr/src/c "..."` from `cmd /usr/src/c "..."` is the program token -- but - # every program-token spelling tried was defeated by something that EXECUTES: `echo hi;cmd /k` - # and `(cmd /mnt/c` (the `;` and `(` are not whitespace, and the outer `(?:^|\s)` anchor sits - # before this whole alternation), an alias, a renamed copy of cmd.exe, and `cmd /d /Q/C` where the - # program is not adjacent to the switch run. Five candidates were built and driven as real gate - # mutants; each traded this one disclosed false deny for four or more measured DENY-to-ALLOW - # regressions. At this rule's threat model the two directions are not symmetric -- a false DENY - # stops legitimate work loudly and has a workaround; a false ALLOW lets a reset land in the shared - # primary silently -- so the false deny is KEPT and disclosed. - # tests/test_worktree_gate_interpreter_sigils.py pins the deny so the cost cannot be lost. + # WHICH MAKES THIS A PROGRAM-IDENTITY PROBLEM -- and that half was right. The sentence that + # followed it, "and that is why it is not fixed here", was WRONG, and it is corrected rather than + # deleted because it is the sentence a reader would have acted on. It rested on five candidates + # that were each defeated by something that EXECUTES: `echo hi;cmd /k`, `(cmd /mnt/c`, an alias, a + # renamed copy of cmd.exe, and `cmd /d /Q/C` where the program is not adjacent to the switch run. + # + # FOUR OF THOSE FIVE SHARE ONE PROPERTY: they break ADJACENCY, not identity. Each candidate asked + # "is the token immediately left of the switch run a cmd spelling", and each counterexample simply + # put something between. A LEFTWARD SCAN BOUNDED BY THE LAST COMMAND SEPARATOR is a different + # instrument, and all four were measured to survive it: `echo hi;cmd /k`, `(cmd /mnt/c`, + # `cmd /d /Q/C` and `cmd /usr/src/c` all still DENY, because the scan skips options and switches + # and keeps going left until it reaches `cmd` or a separator. See Get-FlagOwner below. + # + # THE FIFTH IS NOT CLOSED AND IS NOT CLAIMED: a renamed copy of cmd.exe, or an alias, is an + # unknown program name and gets no recursion. That is the same disclosed weakening the caller + # records for `myrunner -c ''`, and it is the price of an allowlist. + # + # SO THE FALSE DENY IS CLOSED, not kept: `ls /usr/src/c "git checkout main"` ALLOWs. The direction + # asymmetry the old note ended on still holds and still governs the rest of this file; what + # changed is that this row no longer costs anything to fix. $flagThenSep = "(?:(?:$psFlag|$shFlag)\s+|$cmdExeFlag\s*)" # The payload is a NAMED group. It was Groups[1], which still resolves correctly (.NET numbers @@ -637,23 +805,90 @@ function Get-ScannableSegments([string]$Cmd) { # now that $sigil contributes a named group of its own. $inner = @() foreach ($ln in $lines) { + # THE EXTRACTION MUST AGREE WITH THE BLANKING ABOUT WHERE THE ARGUMENT ENDS + # (BACKLOG #1229 residual, third round). `[^"]*` is escape-BLIND: it stops at the first + # quote, INCLUDING an escaped one. Once Remove-QuotedSpans became escape-AWARE, the two + # disagreed -- and the inner code was never re-scanned: + # + # bash -c "bash -c \\"git -C reset --hard\\"" + # extraction got: `bash -c \\` -- truncated at the escaped quote, no verb + # blanking removed: the whole span -- so nothing reached any rule -> ALLOW + # + # MEASURED: main DENY x3, the escape-aware fix ALLOW x3, and the control (same nesting, + # NO escape) DENY on both -- so the trigger is the ESCAPE, not the nesting. The inner + # command really runs: `bash -c "bash -c \\"expr 111 \\* 3\\""` prints 333. + # + # ON MAIN THE TWO AGREED BY ACCIDENT, both being escape-blind, which left the verb visible + # OUTSIDE the span. Making one side escape-aware removed the accident without replacing it. + # This is why a host flag alone cannot close it: the failing host is BASH, where the escape + # is real and honouring it is correct. + # + # The SINGLE-quoted arm stays escape-blind on purpose: sh gives the backslash no special + # meaning inside a single-quoted word, which is the same asymmetry Remove-QuotedSpans keeps. + $dqCode = if ($PosixEscapes) { "(?(?:\\.|[^`"\\])*)" } else { "(?[^`"]*)" } foreach ($pat in @( - "(?i)(?:^|\s)$flagThenSep`"(?[^`"]*)`"" + "(?i)(?:^|\s)$flagThenSep`"$dqCode`"" "(?i)(?:^|\s)$flagThenSep'(?[^']*)'" )) { - foreach ($m in [regex]::Matches($ln, $pat)) { $inner += $m.Groups['code'].Value } + foreach ($m in [regex]::Matches($ln, $pat)) { + # WHO OWNS THIS FLAG DECIDES BOTH QUESTIONS -- whether to recurse at all, and under + # WHICH ESCAPE CONVENTION (BACKLOG #1229 residual, fourth round). `(?:^|\s)` consumes + # the separator, so $m.Index lands on the whitespace before the flag and the text left + # of it is the command segment that owns it. + $owner = Get-FlagOwner $ln.Substring(0, $m.Index) + # A PROGRAM THAT DOES NOT EXECUTE ITS ARGUMENT GETS NO RECURSION. The span then falls + # through to the blanking below as the ordinary quoted data it is. + # + # THE DISCLOSED COST, in the same shape as this file's other owner-ruled weakenings: + # `myrunner -c ''` -- an unknown program with a -c flag -- goes DENY to ALLOW, + # and if such a program IS an interpreter that is a fail-open. It is the same class as + # the 18 interpreter spellings the flag pattern already misses, and the old catch was + # accidental rather than designed, but it is a deliberate move against origin/main. + if ($owner -eq 'none') { continue } + # THE CONVENTION MUST COME FROM THE INTERPRETER, NOT THE OUTER TOOL NAME, and that was + # a live fail-open. `$PosixEscapes` is decided once from the tool name at each call + # site, so a Bash tool call invoking pwsh applied POSIX backslash rules to a PowerShell + # payload; the span straddled `C:\Temp\` and swallowed the gated command between it and + # a later quote. MEASURED to really run, with a payload that COMPUTES (marker 333): + # pwsh -Command '$d = "C:\Temp\" ; git -C reset --hard ; ...' ALLOW + # and the same for `pwsh -c` and `powershell -Command`. The IDENTICAL payload text + # under `bash -c` is INERT on this host (bash reports an unterminated quote), so the + # ALLOW there is CORRECT -- the same characters have opposite right answers depending + # on which interpreter receives them, which is why one flag for the whole line cannot + # express it. `win` maps to $false, which is also the direction the parameter's own + # docstring names conservative: shorter spans, more text left visible, fail CLOSED. + # + # The EXTRACTION regex above keeps the OUTER convention on purpose: it is parsing the + # OUTER command line's quoting, and that line really is the outer host's. + $inner += [pscustomobject]@{ Text = $m.Groups['code'].Value; Posix = ($owner -eq 'posix') } + } } } - foreach ($line in @($lines + $inner)) { + # RAW LINES FIRST, then payloads -- the order is not cosmetic. Rule 3 records the FIRST + # verb-bearing segment it sees, so putting extracted payloads last keeps a recursed line from + # outranking a gated command written plainly on a raw line. + foreach ($line in $lines) { # A quoted PROGRAM path must keep its git token -- `"C:\Program Files\Git\bin\git.exe" checkout - # main` is a real spelling and blanking it wholesale would be a false NEGATIVE. Collapse that form - # to a bare token first, then blank every remaining quoted span. - $s = $line -replace '"[^"]*[\\/](git(?:\.exe)?)"', '$1' - $s = $s -replace "'[^']*[\\/](git(?:\.exe)?)'", '$1' - $s = Remove-QuotedSpans $s + # main` is a real spelling and blanking it wholesale would be a false NEGATIVE. That collapse now + # happens INSIDE Remove-QuotedSpans, on a span the scan already owns. + # + # IT USED TO BE TWO ORDERED REGEXES RIGHT HERE, DOUBLE QUOTES FIRST, AND THAT WAS A SECOND LIVE + # FAIL-OPEN OF THE EXACT SHAPE THE SCAN BELOW EXISTS TO CLOSE. Running before the scan, they + # could pair a quote with a distant `/git"` ACROSS a gated command and replace the whole middle + # with a bare token -- verb and arguments gone, nothing left for any rule to match. Ownership + # cannot be decided by a regex that has no idea which quote opened first, which is the same + # sentence this file already wrote about the blanking order. + $s = Remove-QuotedSpans $line $PosixEscapes [pscustomobject]@{ Raw = $line; Scan = $s } } + + # Each extracted payload carries ITS OWN convention, taken from the interpreter that was matched + # rather than from the tool name at the call site. See the note at the extraction above. + foreach ($item in $inner) { + $s = Remove-QuotedSpans $item.Text $item.Posix + [pscustomobject]@{ Raw = $item.Text; Scan = $s } + } } try { $hook = [Console]::In.ReadToEnd() | ConvertFrom-Json } catch { exit 0 } @@ -973,7 +1208,7 @@ if ($tool -in @("Bash", "PowerShell")) { # sibling worktrees and the primary alike. Any git failure falls through to ALLOW. # ----------------------------------------------------------------------------------------------- $dangerKeys = 'core\.hookspath|core\.worktree|alias\.[\w.-]+|include\.path|includeif\.' - foreach ($seg in (Get-ScannableSegments $cmd)) { + foreach ($seg in (Get-ScannableSegments $cmd ($tool -eq "Bash"))) { if ($seg.Scan -cnotmatch '(^|[\s;&|(''"\\/])git(\.exe)?["'']?(\s|$)') { continue } if ($seg.Scan -notmatch "(?:\bconfig\b[^|;&]*?\s|-c\s+)(?$dangerKeys)") { continue } $badKey = $Matches['key'] @@ -1101,7 +1336,7 @@ What to do instead: # entirely. Ask git whether the path is a registered worktree of a governed repo instead. Any git # failure -- a path that is not a worktree, or does not exist -- falls through to ALLOW. # ----------------------------------------------------------------------------------------------- - foreach ($seg in (Get-ScannableSegments $cmd)) { + foreach ($seg in (Get-ScannableSegments $cmd ($tool -eq "Bash"))) { if ($seg.Scan -cnotmatch '(^|[\s;&|(''"\\/])git(\.exe)?["'']?(\s|$)') { continue } if ($seg.Scan -cnotmatch '\bworktree\s+(?remove|move)(?=\s|$)') { continue } $wtVerb = $Matches['wtverb'] @@ -1364,7 +1599,7 @@ $cleanupBullet # denying it because the primary's path appears in the `cd` is a false positive. $anyInferredTarget = $false $gitToken = '(^|[\s;&|(''"\\/])git(\.exe)?["'']?(\s|$)' - foreach ($seg in (Get-ScannableSegments $cmd)) { + foreach ($seg in (Get-ScannableSegments $cmd ($tool -eq "Bash"))) { # Match a git invocation however it is spelled: git, git.exe, or an absolute path to either. if ($seg.Scan -cnotmatch $gitToken) { continue } if ($seg.Scan -cnotmatch "\bgit(\.exe)?\b[^|;&]*?\s(?$verbs)(?=\s|$)") { continue } diff --git a/tests/test_backlog_status_check.py b/tests/test_backlog_status_check.py index 252c3213..ad20857f 100644 --- a/tests/test_backlog_status_check.py +++ b/tests/test_backlog_status_check.py @@ -336,3 +336,69 @@ def test_a_setext_heading_underline_is_NOT_read_as_a_conflict_marker() -> None: text = "A Heading\n=======\n\n## 1. An item\n\nprose\n" items = bsc.parse_items(text) assert [it.num for it in items] == [1] + + +# --- BACKLOG #1259: the refusal must reach the COMMIT path, and reach it as a REPORT ------------- + + +def test_a_conflicted_source_is_reported_with_its_PATH_not_raised( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """``main`` must NAME THE FILE and exit 1, rather than let the ValueError escape. + + Two costs, and the second is the one that matters. A traceback reads as *the checker is broken* + rather than *your ledger is conflicted*, which sends an author to the wrong file. And the + exception carries a LINE number but no PATH, so with several sources scanned it does not say + which one to open -- and this checker scans the published ledger plus every archive file by + default. + """ + ledger = tmp_path / "BACKLOG.md" + ledger.write_text( + "## 1. An item\n\n<<<<<<< HEAD\nmine\n=======\ntheirs\n>>>>>>> other\n", encoding="utf-8" + ) + assert bsc.main(["--backlog", str(ledger)]) == 1 + err = capsys.readouterr().err + assert "BACKLOG.md" in err, "the report must name which source is conflicted" + assert "conflict marker" in err + assert "Traceback" not in err + + +def test_a_clean_source_still_passes(tmp_path: Path) -> None: + """The negative control. A gate that fires on the healthy case is one everybody learns to skip.""" + ledger = tmp_path / "BACKLOG.md" + ledger.write_text("## 1. An item\n\n> \U0001f6a7 open\n\nprose\n", encoding="utf-8") + assert bsc.main(["--backlog", str(ledger), "--quiet"]) == 0 + + +def test_the_checker_is_WIRED_to_the_commit_path() -> None: + """The whole of what #1259 had left, and it is a wiring fact rather than a code one. + + The refusal shipped and protected programmatic readers and CI, while NOTHING called it before a + commit. Measured at dd655da2 against a docs/BACKLOG.md carrying a realistic PROSE-level conflict: + every wired hook passed, overall rc 0. Asserted on the config rather than by running pre-commit, + because the executable resolves from the PRIMARY checkout's venv and is absent from a lane venv -- + a test that shelled out would skip silently in exactly the trees this repo is developed in. + + A conflict that ADDS A HEADING is already caught incidentally by the ledger gate (both sides' + numbers read as unallocated), which is why the case that needed covering is the prose one. + """ + raw = (Path(__file__).resolve().parents[1] / ".pre-commit-config.yaml").read_text( + encoding="utf-8" + ) + # Read the DIRECTIVES, never the raw text. Both assertions below name strings this file's own + # explanatory comments also contain, and the first draft of this test FAILED on its own prose -- + # which is the same no-inline-code-stripping shape that makes a certain CI grep read a quoted + # token as a claim. A substring check over a commented config cannot tell a wiring from a note + # explaining why that wiring was NOT chosen. + directives = [ + line.split("#", 1)[0] for line in raw.splitlines() if line.split("#", 1)[0].strip() + ] + body = "\n".join(directives) + assert "backlog_status_check.py" in body, ( + "the ledger parse check is not wired into .pre-commit-config.yaml, so a conflicted " + "docs/BACKLOG.md commits with every gate green (BACKLOG #1259)" + ) + assert "check-merge-conflict" not in body, ( + "a generic textual matcher is a SECOND definition of what a readable ledger is; " + "CLAUDE.md section 11 requires this file be read through parse_items and nothing else" + ) diff --git a/tests/test_cert_expiry.py b/tests/test_cert_expiry.py index 7b18617a..04eb13f8 100644 --- a/tests/test_cert_expiry.py +++ b/tests/test_cert_expiry.py @@ -15,6 +15,7 @@ from cryptography.x509.oid import NameOID from messagefoundry.config.settings import CertMonitorSettings +from messagefoundry.config.wiring import MLLP, InboundConnection, Registry from messagefoundry.pipeline.alert_sinks import NotifierAlertSink from messagefoundry.pipeline.alerts import LoggingAlertSink from messagefoundry.pipeline.cert_expiry import ( @@ -53,6 +54,7 @@ class _RecordingSink: def __init__(self) -> None: self.cert_calls: list[tuple[str, str, str, int]] = [] + self.crl_calls: list[tuple[str, str, str, int]] = [] def connection_stopped(self, name: str, *, detail: str) -> None: pass @@ -66,6 +68,9 @@ def storage_threshold(self, path: str, *, size_bytes: int, limit_bytes: int) -> def cert_expiry(self, name: str, *, path: str, not_after: str, days_remaining: int) -> None: self.cert_calls.append((name, path, not_after, days_remaining)) + def crl_expiry(self, name: str, *, path: str, not_after: str, days_remaining: int) -> None: + self.crl_calls.append((name, path, not_after, days_remaining)) + def secret_rotation_due( self, name: str, *, secret: str, last_rotated: str, days_overdue: int ) -> None: @@ -380,3 +385,116 @@ async def _go() -> None: ) asyncio.run(_go()) + + +# --- BACKLOG #1005 scope 4: the CRL pre-expiry alarm -------------------------------------------- +# +# A CRL is watched by the same monitor because the operator question is identical -- "is a file I +# depend on about to expire" -- but it alerts down a SEPARATE sink method, and that separation is +# the property under test. An expiring CERTIFICATE degrades one identity and is fixed by reissuing +# it. An EXPIRED CRL makes OpenSSL refuse EVERY client presenting a certificate under that issuer, +# not merely revoked ones, so it is a total interface outage fixed by a PKI refresh. One method for +# both would hand an operator one string for two causes with opposite remedies. + + +def _write_crl(path: Path, *, next_update: datetime.datetime) -> None: + """A CA bundled with its own CRL, the shape harden_crl_check loads and the monitor reads.""" + key = ec.generate_private_key(ec.SECP256R1()) + name = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, "mefor-test-ca")]) + ca = ( + x509.CertificateBuilder() + .subject_name(name) + .issuer_name(name) + .public_key(key.public_key()) + .serial_number(x509.random_serial_number()) + .not_valid_before(next_update - datetime.timedelta(days=400)) + .not_valid_after(next_update + datetime.timedelta(days=400)) + .add_extension(x509.BasicConstraints(ca=True, path_length=None), critical=True) + .sign(key, hashes.SHA256()) + ) + crl = ( + x509.CertificateRevocationListBuilder() + .issuer_name(ca.subject) + .last_update(next_update - datetime.timedelta(days=30)) + .next_update(next_update) + .sign(key, hashes.SHA256()) + ) + path.write_bytes( + ca.public_bytes(serialization.Encoding.PEM) + crl.public_bytes(serialization.Encoding.PEM) + ) + + +def test_a_crl_inside_the_warn_window_alerts_crl_expiry_not_cert_expiry(tmp_path: Path) -> None: + # THE SEPARATION IS THE POINT. Asserting crl_calls alone would pass if the runner fired BOTH. + crl = tmp_path / "ca_and_crl.pem" + _write_crl(crl, next_update=_REF + datetime.timedelta(days=5)) + sink = _RecordingSink() + _runner([MonitoredCert("IB_PARTNER", str(crl), kind="crl")], sink).run_once() + assert len(sink.crl_calls) == 1 + assert sink.cert_calls == [] + name, path, _, days = sink.crl_calls[0] + assert (name, path) == ("IB_PARTNER", str(crl)) + assert days == 5 + + +def test_an_expired_crl_reports_negative_days(tmp_path: Path) -> None: + # Negative days_remaining is what tells the sink to escalate: past nextUpdate the listener is + # already refusing every client, so this is not an approaching deadline. + crl = tmp_path / "stale.pem" + _write_crl(crl, next_update=_REF - datetime.timedelta(days=3)) + sink = _RecordingSink() + _runner([MonitoredCert("IB_PARTNER", str(crl), kind="crl")], sink).run_once() + assert len(sink.crl_calls) == 1 + assert sink.crl_calls[0][3] == -3 + + +def test_a_fresh_crl_alerts_nothing(tmp_path: Path) -> None: + # POSITIVE CONTROL: the alarm can stay SILENT. Without it the two tests above would pass just as + # well against a monitor that alerts on every CRL it is handed. + crl = tmp_path / "fresh.pem" + _write_crl(crl, next_update=_REF + datetime.timedelta(days=365)) + sink = _RecordingSink() + _runner([MonitoredCert("IB_PARTNER", str(crl), kind="crl")], sink).run_once() + assert sink.crl_calls == [] + assert sink.cert_calls == [] + + +def test_a_cert_and_a_crl_on_one_connection_route_to_their_own_methods(tmp_path: Path) -> None: + # The realistic configuration: one listener presenting a server identity AND checking a CRL. + # Both expire, and an operator must be able to tell which one from the alert alone. + cert = tmp_path / "server.pem" + crl = tmp_path / "ca_and_crl.pem" + _write_cert(cert, not_after=_REF + datetime.timedelta(days=7)) + _write_crl(crl, next_update=_REF + datetime.timedelta(days=2)) + sink = _RecordingSink() + _runner( + [ + MonitoredCert("IB_PARTNER", str(cert)), + MonitoredCert("IB_PARTNER", str(crl), kind="crl"), + ], + sink, + ).run_once() + assert [c[3] for c in sink.cert_calls] == [7] + assert [c[3] for c in sink.crl_calls] == [2] + + +def test_an_inbound_tls_crl_file_is_collected_from_the_registry() -> None: + # Scope 1 wired the setting into the three listeners; this is what makes the monitor SEE it. + # Outbound is deliberately not collected: a CRL verifies peers we REQUIRE certificates from, + # and only an inbound listener does that. + registry = Registry() + registry.inbound["IB_PARTNER"] = InboundConnection( + name="IB_PARTNER", + spec=MLLP( + port=2575, + tls=True, + tls_cert_file="c.pem", + tls_ca_file="ca.pem", + tls_crl_file="ca_and_crl.pem", + ), + router="r", + ) + collected = certs_from_registry(registry, None) + kinds = {(mc.kind, mc.path) for mc in collected} + assert ("crl", "ca_and_crl.pem") in kinds + assert ("cert", "c.pem") in kinds diff --git a/tests/test_dangling_citation_check.py b/tests/test_dangling_citation_check.py index 36cd3811..7f2600dc 100644 --- a/tests/test_dangling_citation_check.py +++ b/tests/test_dangling_citation_check.py @@ -306,3 +306,117 @@ def test_a_PR_SHAPED_reference_is_reported_but_does_not_fail(tmp_path: pathlib.P even when its number is above the floor.""" doc = _doc(tmp_path, f"shipped in PR #{_unissued_above_floor()}\n") assert cc.main([doc]) == 0 + + +# --- BACKLOG #1235: the ANNOTATION must ask the same predicate the EXIT CODE asks ---------------- + + +def test_a_pr_shaped_hit_is_not_narrated_as_the_live_shape( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """The third definition of the live-shape rule, and the only one that talks to a human. + + MEASURED at 4c28badd before this fix: six hits printed BOTH + ``[PR/issue/foreign-repo shaped -- very likely NOT a backlog citation]`` AND + ``This is the live shape.`` -- two contradictory annotations on the SAME hit, two lines apart -- + while the process exited 0. + + Asserted on the CONTRADICTION rather than on either sentence alone, because either one in + isolation is correct: the hit IS above the floor, and it IS pr-shaped. Only their co-occurrence + on one hit is the defect, so only that co-occurrence can pin it. + """ + doc = tmp_path / "d.md" + floor = cc.allocation_floor() + doc.write_text(f"see upstream `someproject#{floor + 500}` for context\n", encoding="utf-8") + assert cc.main([str(doc)]) == 0, "a pr-shaped hit must not fail the gate" + out = capsys.readouterr().out + assert "PR/issue/foreign-repo shaped" in out, "control failed: the hit was not annotated at all" + assert "This is the live shape." not in out, ( + "the annotation called a pr-shaped hit the live shape while the exit code passed over it " + "-- a third definition of the rule, disagreeing with the other two (BACKLOG #1235)" + ) + + +def test_a_genuinely_live_hit_is_still_narrated_as_the_live_shape( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """The negative control. Suppressing the sentence everywhere would pass the test above and + destroy the tool's only human-readable verdict -- a fix that trades a wrong answer for none.""" + doc = tmp_path / "d.md" + floor = cc.allocation_floor() + doc.write_text(f"resolves to BACKLOG #{floor + 500} which was never filed\n", encoding="utf-8") + assert cc.main([str(doc)]) == 1, "a genuinely live citation must fail the gate" + assert "This is the live shape." in capsys.readouterr().out + + +# --- BACKLOG #1235: a clean result over an EMPTY population is not a clean result ----------------- + + +def test_an_empty_population_is_REFUSED_not_reported_clean( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """The trap under the item's first disjunct, and it arms by CLOSING the item. + + The default path list is ``Path("docs").rglob(...)`` -- CWD-relative, not repo-relative. + MEASURED before this fix, from a directory with no ``docs/``:: + + No unresolved backlog citation in 0 file(s). + Resolved against 0 allocated item numbers (open and closed). + exit 0 + + Both counts zero, nothing objecting. The >200-file population floor lives only in + ``test_the_docs_scan_actually_covers_something``, so it guards the PYTEST arm and nothing else -- + anyone who satisfies the item by wiring this CLI into CI, which is the documented way to close it, + inherits a gate that reports clean when it scanned nothing. + """ + monkeypatch.chdir(tmp_path) + assert cc.main([]) == 1 + err = capsys.readouterr().err + assert "empty population" in err + assert "0 file(s) scanned" in err, "the refusal must print WHAT it scanned, not just refuse" + assert str(tmp_path) in err, "and WHERE from -- the defect is a CWD-relative default" + + +def test_a_real_population_is_still_reported_normally( + capsys: pytest.CaptureFixture[str], +) -> None: + """The negative control, and without it the fix above could be 'always refuse'. + + Run against the real docs/ from the repo root: a populated scan must still reach a verdict rather + than trip the emptiness guard. This is the arm that makes the refusal a guard instead of a wall. + """ + assert cc.main([]) in (0, 1) # a verdict, whichever way -- NOT the emptiness refusal + assert "empty population" not in capsys.readouterr().err + + +def test_the_coverage_bound_prints_on_a_CLEAN_run_too( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """It used to print only after the hits loop, so the early clean return skipped it. + + The bound was therefore absent at exactly the moment a reader concludes "clean" -- which is the + whole population it exists to qualify. It is in the module docstring, and a docstring is not what + a CI log shows. + """ + doc = tmp_path / "clean.md" + doc.write_text("no citations here at all\n", encoding="utf-8") + assert cc.main([str(doc)]) == 0 + out = capsys.readouterr().out + assert "No unresolved backlog citation" in out, "control: this must be the CLEAN path" + assert cc._COVERAGE_BOUND in out, ( + "the coverage bound is absent from the clean run, which is the one a reader acts on" + ) + + +def test_the_coverage_bound_has_ONE_definition() -> None: + """Both exit paths print the same text because there is only one string to print. + + Two copies of a caveat drift, and the copy that goes stale is the one nobody reads -- it only + prints on the path they are not on. This is the same single-definition rule the item is about, + applied to prose rather than to a predicate. + """ + # Read the module's OWN file, via the loaded object, so this cannot drift from what was + # imported. Naming the path again here would be a second definition inside a test whose + # subject is second definitions. + source = Path(cc.__file__).read_text(encoding="utf-8") + assert source.count("Not scanned: the private companion repository") == 1 diff --git a/tests/test_direct_transport.py b/tests/test_direct_transport.py index 570271f1..649a4e12 100644 --- a/tests/test_direct_transport.py +++ b/tests/test_direct_transport.py @@ -470,3 +470,26 @@ def test_direct_factory_exported(pki: dict[str, Any]) -> None: assert spec.settings["host"] == "hisp.partner.example" assert spec.settings["port"] == 587 assert "Direct" in mf.__all__ + + +def test_check_hostname_false_refuses_credentials(pki: dict[str, Any]) -> None: + # BACKLOG #1314, the THIRD weakening axis, mirroring the EMAIL connector. The chain IS + # verified but the peer NAME is not, so any certificate chaining to the trust anchor is + # accepted whatever it was issued to, and the AUTH exchange goes to an unidentified peer. + # + # This connector had NO else arm before #1314 -- the branch is new, so these three tests are + # the only thing standing between it and a silent no-op. + with pytest.raises(ValueError, match="tls_check_hostname=false"): + DirectDestination(_dest(pki, tls_check_hostname=False, username="svc", password="pw")) + + +def test_check_hostname_false_without_credentials_still_constructs(pki: dict[str, Any]) -> None: + # NEGATIVE CONTROL: the gate is keyed on the CREDENTIAL, not on the posture. Widening it to a + # name-unchecked hop carrying no username is the "must not widen the existing arms" failure. + DirectDestination(_dest(pki, tls_check_hostname=False)) + + +def test_credentials_over_a_fully_verified_hop_still_construct(pki: dict[str, Any]) -> None: + # POSITIVE CONTROL: proves the new gate can be PASSED, so the refusal test above is not green + # merely because DirectDestination rejects every credentialed construction. + DirectDestination(_dest(pki, username="svc", password="pw")) diff --git a/tests/test_email_destination.py b/tests/test_email_destination.py index 89abd883..2344df51 100644 --- a/tests/test_email_destination.py +++ b/tests/test_email_destination.py @@ -551,6 +551,31 @@ def test_tls_verify_false_refuses_credentials_even_with_escape( EmailDestination(_dest(tls_verify=False, username="svc", password="pw")) +def test_check_hostname_false_refuses_credentials() -> None: + # BACKLOG #1314, the THIRD weakening axis. TLS is on and the chain IS verified, but the peer + # NAME is not checked, so any certificate chaining to the configured anchor is accepted + # regardless of who it was issued to. An AUTH exchange on that hop hands the credential to a + # peer whose identity was never established -- the same loss the two arms above refuse. + # + # ABSOLUTE, and deliberately not keyed on any escape: in both existing arms the escape governs + # the BODY posture and never the credential. A third arm keeps that split. + with pytest.raises(ValueError, match="tls_check_hostname=false"): + EmailDestination(_dest(tls_check_hostname=False, username="svc", password="pw")) + + +def test_check_hostname_false_without_credentials_still_constructs() -> None: + # NEGATIVE CONTROL. The refusal must be keyed on the CREDENTIAL, not on the posture. A + # name-unchecked hop carrying no username is this item's out of scope -- widening to it would + # be the "must not widen the existing arms" failure the item names. + EmailDestination(_dest(tls_check_hostname=False)) + + +def test_credentials_over_a_fully_verified_hop_still_construct() -> None: + # POSITIVE CONTROL. Proves the new gate can be PASSED, so the test above is not green merely + # because EmailDestination refuses every credentialed construction. + EmailDestination(_dest(username="svc", password="pw")) + + async def test_tls_verify_false_with_escape_builds_an_unverified_context( monkeypatch: pytest.MonkeyPatch, ) -> None: diff --git a/tests/test_hop_refusal_wiring.py b/tests/test_hop_refusal_wiring.py index 09c54c84..5ee71437 100644 --- a/tests/test_hop_refusal_wiring.py +++ b/tests/test_hop_refusal_wiring.py @@ -22,6 +22,7 @@ Registry, build_outbound_connection, ) +from messagefoundry.pipeline.wiring_runner import WiringError, check_inbound_revocation # --- decision 2: the escape clamp (MEFOR_ALLOW_INSECURE_TLS downgrades REFUSE->WARN, non-enforcing) --- @@ -437,3 +438,86 @@ def test_logging_forward_hop_attestation_also_requires_a_reason() -> None: forward_hop_attested=True, forward_hop_attested_reason="management segment is isolated" ) assert ok.forward_hop_attested is True + + +# --- BACKLOG #1005 scope 3: the revocation-gap refusal ------------------------------------------ +# +# The revocation sibling of the cleartext exposed-gate above, and it fires on the OPPOSITE +# condition: those refuse a listener with NO TLS, this refuses one whose TLS is correct but whose +# client certificates are never checked for revocation. Measured on this tree, a revoked-but- +# chain-valid client is ACCEPTED, so a partner certificate revoked this morning would keep +# authenticating until its notAfter. + + +def _mtls(**overrides: object) -> Source: + """An MLLP listener with mTLS ON. Paths are never opened by the gate -- it reads settings only.""" + settings: dict[str, object] = {"tls": True, "tls_cert_file": "c.pem", "tls_ca_file": "ca.pem"} + settings.update({k: v for k, v in overrides.items() if k != "attested"}) + return Source( + type=ConnectorType.MLLP, + settings=settings, + tls_revocation_attested=bool(overrides.get("attested", False)), + ) + + +_PHI_ENFORCING = HopPosture(is_phi=True, enforcing=True) + + +def test_mtls_without_a_crl_is_refused_on_an_enforcing_phi_instance() -> None: + # THE CONTROL ITSELF. Everything below is a rung of its escape ladder, so this must fail first + # or none of them mean anything. + with pytest.raises(WiringError, match="no revocation"): + check_inbound_revocation(_mtls(), "IB_PARTNER", posture=_PHI_ENFORCING) + + +def test_a_configured_crl_passes() -> None: + # POSITIVE CONTROL: the refusal can be SATISFIED, not merely avoided. Without this the test + # above would pass equally well against a gate that refuses every mTLS listener. + check_inbound_revocation(_mtls(tls_crl_file="ca_and_crl.pem"), "IB", posture=_PHI_ENFORCING) + + +def test_the_per_connection_attestation_passes() -> None: + # The surgical opt-in: a site whose PKI checks revocation OUTSIDE the engine declines the + # in-engine control without being refused. Mirrors Destination.tls_revocation_attested, and + # every sibling refusal here pairs with an attestation. + check_inbound_revocation(_mtls(attested=True), "IB", posture=_PHI_ENFORCING) + + +def test_a_non_phi_instance_warns_rather_than_refusing() -> None: + check_inbound_revocation(_mtls(), "IB", posture=HopPosture(is_phi=False, enforcing=True)) + + +def test_a_non_enforcing_instance_warns_rather_than_refusing() -> None: + check_inbound_revocation(_mtls(), "IB", posture=HopPosture(is_phi=True, enforcing=False)) + + +def test_an_unstamped_posture_never_acquires_a_new_refusal() -> None: + # posture=None means the check ran OUTSIDE the enforced gate -- a direct or embedding call. + # The sibling makes the same promise, and it is what keeps this an ADD at the enforced surface + # rather than a new refusal for callers who never opted into the gate. + check_inbound_revocation(_mtls(), "IB", posture=None) + + +def test_tls_without_mtls_is_not_a_revocation_gap() -> None: + # PLACEMENT GUARD: no tls_ca_file means no client certificate is requested, so there is nothing + # whose revocation could matter. Refusing here would demand a CRL from every TLS listener. + src = Source(type=ConnectorType.MLLP, settings={"tls": True, "tls_cert_file": "c.pem"}) + check_inbound_revocation(src, "IB", posture=_PHI_ENFORCING) + + +def test_a_connector_with_no_mtls_surface_is_ignored() -> None: + # Raw TCP/X12 have no TLS option at all, so they cannot hold a client certificate to revoke. + src = Source(type=ConnectorType.TCP, settings={"tls": True, "tls_ca_file": "ca.pem"}) + check_inbound_revocation(src, "IB", posture=_PHI_ENFORCING) + + +def test_the_dimse_and_http_listeners_are_covered_too() -> None: + # The item's whole point is that an HTTP proxy can terminate neither MLLP framing nor DIMSE, so + # for those the documented out-of-engine delegation does not reach. Both must be gated. + for connector in (ConnectorType.DIMSE, ConnectorType.HTTP): + src = Source( + type=connector, + settings={"tls": True, "tls_cert_file": "c.pem", "tls_ca_file": "ca.pem"}, + ) + with pytest.raises(WiringError, match="no revocation"): + check_inbound_revocation(src, f"IB_{connector.name}", posture=_PHI_ENFORCING) diff --git a/tests/test_mllp_tls.py b/tests/test_mllp_tls.py index 17ab2a05..aa27e835 100644 --- a/tests/test_mllp_tls.py +++ b/tests/test_mllp_tls.py @@ -326,3 +326,101 @@ def test_factory_carries_tls_key_password_and_redacts_it() -> None: assert spec.settings["tls_key_password"] == "pw" # Defence in depth: an inline passphrase is scrubbed from the /metadata view (it should be an env() ref). assert redacted_settings(spec.settings)["tls_key_password"] == "***" + + +def _ca_and_crl(tmp_path: Path, *, revoked_serial: int = 4000) -> str: + """A CA bundled with its own fresh CRL, written as one PEM -- the shape harden_crl_check loads. + + Separate from :func:`_cert` because that one is self-signed-and-CA:TRUE for convenience, while a + CRL has to be signed by the key whose certificate is the trust anchor. + """ + key = ec.generate_private_key(ec.SECP256R1()) + name = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, "mllp-crl-ca")]) + now = datetime.datetime.now(datetime.UTC) + ca = ( + x509.CertificateBuilder() + .subject_name(name) + .issuer_name(name) + .public_key(key.public_key()) + .serial_number(x509.random_serial_number()) + .not_valid_before(now - datetime.timedelta(days=1)) + .not_valid_after(now + datetime.timedelta(days=3650)) + .add_extension(x509.BasicConstraints(ca=True, path_length=None), critical=True) + .sign(key, hashes.SHA256()) + ) + crl = ( + x509.CertificateRevocationListBuilder() + .issuer_name(ca.subject) + .last_update(now - datetime.timedelta(days=1)) + .next_update(now + datetime.timedelta(days=30)) + .add_revoked_certificate( + x509.RevokedCertificateBuilder() + .serial_number(revoked_serial) + .revocation_date(now - datetime.timedelta(hours=1)) + .build() + ) + .sign(key, hashes.SHA256()) + ) + bundle = tmp_path / "ca_and_crl.pem" + bundle.write_bytes( + ca.public_bytes(serialization.Encoding.PEM) + crl.public_bytes(serialization.Encoding.PEM) + ) + return str(bundle) + + +def test_server_mtls_loads_the_crl_when_configured(tmp_path: Path) -> None: + # BACKLOG #1005. The setting must reach the trust store, not merely be accepted by the factory. + # cert_store_stats()["crl"] is the only thing separating "loaded" from "silently ignored" -- + # a context can carry the check flag with zero CRLs and then refuse EVERY client. + cert, key = _cert(tmp_path) + bundle = _ca_and_crl(tmp_path) + ctx = _mllp_ssl_context( + { + "tls": True, + "tls_cert_file": cert, + "tls_key_file": key, + "tls_ca_file": bundle, + "tls_crl_file": bundle, + }, + server=True, + ) + assert ctx is not None + assert ctx.verify_mode == ssl.CERT_REQUIRED + assert ctx.cert_store_stats()["crl"] >= 1 + assert ctx.verify_flags & ssl.VERIFY_CRL_CHECK_LEAF + + +def test_server_mtls_without_a_crl_does_no_revocation_checking(tmp_path: Path) -> None: + # NEGATIVE CONTROL, and it pins the SHIPPED GAP this item is filed against: mTLS on, chain + # verified, RFC 5280 strictness applied -- and no revocation whatsoever, so a partner + # certificate revoked this morning keeps authenticating until its notAfter. + # + # If this ever starts failing, revocation arrived by some other route and the item's premise + # needs re-deriving. Do not relax it to make it pass. + cert, key = _cert(tmp_path) + ctx = _mllp_ssl_context( + {"tls": True, "tls_cert_file": cert, "tls_key_file": key, "tls_ca_file": cert}, + server=True, + ) + assert ctx is not None + assert ctx.verify_mode == ssl.CERT_REQUIRED + assert not (ctx.verify_flags & ssl.VERIFY_CRL_CHECK_LEAF) + + +def test_a_crl_without_mtls_is_not_loaded(tmp_path: Path) -> None: + # PLACEMENT GUARD. The CRL call lives INSIDE the mTLS branch: with no client certificate + # required there is nothing to revoke, and loading one anyway would set a check flag on a + # context that never asks for a peer certificate. + cert, key = _cert(tmp_path) + ctx = _mllp_ssl_context( + { + "tls": True, + "tls_cert_file": cert, + "tls_key_file": key, + "tls_crl_file": _ca_and_crl(tmp_path), + }, + server=True, + ) + assert ctx is not None + assert ctx.verify_mode == ssl.CERT_NONE + assert not (ctx.verify_flags & ssl.VERIFY_CRL_CHECK_LEAF) diff --git a/tests/test_security_posture_defaults.py b/tests/test_security_posture_defaults.py index c8e5ad4f..c243be33 100644 --- a/tests/test_security_posture_defaults.py +++ b/tests/test_security_posture_defaults.py @@ -258,6 +258,13 @@ def test_every_security_bool_at_its_insecure_value_is_reported() -> None: "tls_key_file": "material/path, not a posture switch", "tls_key_password": "material/path, not a posture switch", "tls_ca_file": "material/path, not a posture switch", + # BACKLOG #1005 added this one. It is exempt for BOTH of the reasons already used above, and + # stating only the first would be the weaker half: it is a material PATH like tls_ca_file + # beside it, AND its ABSENCE is GATED rather than reported -- check_inbound_revocation refuses + # an mTLS listener with no CRL on an enforcing PHI instance, the same way the ADR 0092 hop cell + # gates tls/tls_verify below. A reader that merely reported "no CRL configured" would be strictly + # weaker than the refusal that already exists. + "tls_crl_file": "material/path; its absence is gated by #1005's posture-keyed revocation refusal", # Not TLS at all โ€” the regex matches the word 'verify' in an HL7 ACK correlation check. "verify_ack_control_id": "HL7 ACK control-id correlation, unrelated to transport TLS", # Verify-off and TLS-off are GATED rather than reported: the ADR 0092 posture-keyed cell refuses diff --git a/tests/test_tls_policy.py b/tests/test_tls_policy.py index 33e437fd..590f5a66 100644 --- a/tests/test_tls_policy.py +++ b/tests/test_tls_policy.py @@ -30,6 +30,7 @@ enforce_insecure_hop, fips_attestation, harden_cipher_suites, + harden_crl_check, harden_kex_groups, harden_verify_flags, in_process_tls_revocation_refused, @@ -960,3 +961,239 @@ def test_harden_cipher_suites_raises_on_a_null_cipher_context() -> None: ctx.set_ciphers(_NULL_CIPHER) with pytest.raises(ValueError, match="NULL-cipher"): harden_cipher_suites(ctx, connector="test-null") + + +# --- BACKLOG #1005: opt-in CRL checking on the verifying server contexts ----------------------------- +# +# Both traps from the item are ASSERTIONS here, not comments. Re-measured on this worktree +# (CPython 3.14.6 / OpenSSL 3.5.7) before any of this was written: +# +# arm crls good client revoked client +# CA only, no CRL flag (shipped) 0 ACCEPTED ACCEPTED <- the gap +# cafile= CA + FRESH crl, flag ON 1 ACCEPTED REFUSED: revoked <- the control works +# cadata= CA + FRESH crl, flag ON 0 REFUSED REFUSED <- TRAP 1 +# cafile= CA + STALE crl, flag ON 1 REFUSED REFUSED <- TRAP 2 +# +# TRAP 1 is why the helper asserts cert_store_stats()["crl"] >= 1: a loader that silently loads +# ZERO CRLs still sets the flag, and then refuses EVERY client with "unable to get certificate +# CRL". Nothing at load time says so. The count is the only thing that distinguishes "loaded" +# from "silently ignored". +# +# TRAP 2 is why an expired CRL is refused at BUILD time: past nextUpdate, OpenSSL refuses every +# client rather than just revoked ones, so an unrefreshed CRL is an outage whose first symptom is +# every partner dropping at once. Failing loudly at startup beats failing at a partner handshake. + + +@pytest.fixture(scope="module") +def _crl_material(tmp_path_factory: pytest.TempPathFactory) -> dict[str, str]: + """A throwaway CA plus a fresh and an expired CRL. Synthetic, no PHI, never leaves tmp.""" + import datetime + + from cryptography import x509 + from cryptography.hazmat.primitives import hashes, serialization + from cryptography.hazmat.primitives.asymmetric import rsa + from cryptography.x509.oid import NameOID + + tmp = tmp_path_factory.mktemp("crl1005") + now = datetime.datetime.now(datetime.UTC) + day = datetime.timedelta(days=1) + + key = rsa.generate_private_key(public_exponent=65537, key_size=2048) + name = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, "crl-probe-ca")]) + ca = ( + x509.CertificateBuilder() + .subject_name(name) + .issuer_name(name) + .public_key(key.public_key()) + .serial_number(x509.random_serial_number()) + .not_valid_before(now - day) + .not_valid_after(now + 365 * day) + .add_extension(x509.BasicConstraints(ca=True, path_length=None), critical=True) + .sign(key, hashes.SHA256()) + ) + + def crl(next_update: datetime.datetime) -> bytes: + builder = ( + x509.CertificateRevocationListBuilder() + .issuer_name(ca.subject) + .last_update(now - 2 * day) + .next_update(next_update) + .add_revoked_certificate( + x509.RevokedCertificateBuilder() + .serial_number(4000) + .revocation_date(now - day) + .build() + ) + ) + return builder.sign(key, hashes.SHA256()).public_bytes(serialization.Encoding.PEM) + + def leaf(cn: str, serial: int, *, server: bool) -> tuple[bytes, bytes]: + """A CA-issued leaf. `serial` 4000 is the one the CRL above revokes.""" + lk = rsa.generate_private_key(public_exponent=65537, key_size=2048) + oid = ( + x509.oid.ExtendedKeyUsageOID.SERVER_AUTH + if server + else x509.oid.ExtendedKeyUsageOID.CLIENT_AUTH + ) + builder = ( + x509.CertificateBuilder() + .subject_name(x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, cn)])) + .issuer_name(ca.subject) + .public_key(lk.public_key()) + .serial_number(serial) + .not_valid_before(now - day) + .not_valid_after(now + 90 * day) + .add_extension(x509.BasicConstraints(ca=False, path_length=None), critical=True) + .add_extension(x509.ExtendedKeyUsage([oid]), critical=False) + ) + if server: + builder = builder.add_extension( + x509.SubjectAlternativeName([x509.DNSName("localhost")]), critical=False + ) + cert = builder.sign(key, hashes.SHA256()) + return ( + cert.public_bytes(serialization.Encoding.PEM), + lk.private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + serialization.NoEncryption(), + ), + ) + + ca_pem = ca.public_bytes(serialization.Encoding.PEM) + out: dict[str, str] = {} + + def put(name: str, data: bytes) -> None: + p = tmp / name + p.write_bytes(data) + out[name.split(".")[0]] = str(p) + + put("ca_only.pem", ca_pem) + put("ca_and_fresh.pem", ca_pem + crl(now + 30 * day)) + put("ca_and_expired.pem", ca_pem + crl(now - day)) + for cn, serial, is_server, stem in ( + ("localhost", 2000, True, "server"), + ("good-client", 3000, False, "good"), + ("revoked-client", 4000, False, "revoked"), + ): + cert_pem, key_pem = leaf(cn, serial, server=is_server) + put(f"{stem}.pem", cert_pem) + put(f"{stem}_key.pem", key_pem) + return out + + +def _verifying_ctx() -> ssl.SSLContext: + ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) + ctx.verify_mode = ssl.CERT_REQUIRED + return ctx + + +def test_harden_crl_check_loads_the_crl_and_sets_the_flag(_crl_material: dict[str, str]) -> None: + # POSITIVE CONTROL for the two refusals below: the helper CAN succeed, so those tests are not + # green merely because it rejects everything handed to it. + ctx = _verifying_ctx() + harden_crl_check(ctx, _crl_material["ca_and_fresh"]) + assert ctx.cert_store_stats()["crl"] >= 1 + assert ctx.verify_flags & ssl.VERIFY_CRL_CHECK_LEAF + + +def test_harden_crl_check_refuses_a_file_carrying_no_crl(_crl_material: dict[str, str]) -> None: + # TRAP 1. Without this assertion the context comes back with the flag set and nothing to check + # against, and every client -- good or revoked -- is refused "unable to get certificate CRL". + # The failure is a total outage that reads, at the call site, like a working control. + with pytest.raises(ValueError, match="no CRL"): + harden_crl_check(_verifying_ctx(), _crl_material["ca_only"]) + + +def test_harden_crl_check_refuses_an_already_expired_crl(_crl_material: dict[str, str]) -> None: + # TRAP 2 preflight. Past nextUpdate OpenSSL refuses EVERY client, not just revoked ones, so an + # unrefreshed CRL takes a live interface down. Refuse it loudly at construction instead of at + # the first partner handshake, where the operator sees only "every partner dropped at once". + with pytest.raises(ValueError, match="expired"): + harden_crl_check(_verifying_ctx(), _crl_material["ca_and_expired"]) + + +def test_harden_crl_check_refuses_a_missing_file(tmp_path: Path) -> None: + # A configured-but-absent CRL must not degrade to "no revocation checking". Fail-closed by + # construction is the whole reason this item is sized 5 rather than 3. + with pytest.raises(ValueError, match="does not exist"): + harden_crl_check(_verifying_ctx(), str(tmp_path / "nope.pem")) + + +def _crl_handshake(crl_bundle: str | None, client_stem: str, mat: dict[str, str]) -> str: + """Complete one real mTLS handshake. Returns "ACCEPTED" or the OpenSSL refusal reason. + + TLS 1.2 is pinned so client authentication happens IN the handshake and the server-side + outcome is unambiguous -- under 1.3 the client cert arrives after the server has finished and + the failure surfaces on a later read instead. + """ + import socket + import threading + + srv_ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) + srv_ctx.minimum_version = ssl.TLSVersion.TLSv1_2 + srv_ctx.maximum_version = ssl.TLSVersion.TLSv1_2 + srv_ctx.load_cert_chain(mat["server"], mat["server_key"]) + srv_ctx.verify_mode = ssl.CERT_REQUIRED + srv_ctx.load_verify_locations(cafile=mat["ca_only"]) + if crl_bundle is not None: + harden_crl_check(srv_ctx, crl_bundle) + + cli_ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT) + cli_ctx.minimum_version = ssl.TLSVersion.TLSv1_2 + cli_ctx.maximum_version = ssl.TLSVersion.TLSv1_2 + cli_ctx.load_verify_locations(cafile=mat["ca_only"]) + cli_ctx.load_cert_chain(mat[client_stem], mat[f"{client_stem}_key"]) + + listener = socket.socket() + listener.bind(("127.0.0.1", 0)) + listener.listen(1) + port = listener.getsockname()[1] + box: dict[str, str] = {} + + def accept() -> None: + try: + conn, _ = listener.accept() + with srv_ctx.wrap_socket(conn, server_side=True): + box["result"] = "ACCEPTED" + except ssl.SSLError as exc: + box["result"] = str(exc) + except OSError as exc: # pragma: no cover - transport teardown race + box["result"] = f"OSError: {exc}" + + thread = threading.Thread(target=accept) + thread.start() + with ( + contextlib.suppress(OSError, ssl.SSLError), + socket.create_connection(("127.0.0.1", port), timeout=10) as sock, + cli_ctx.wrap_socket(sock, server_hostname="localhost"), + ): + pass + thread.join(timeout=10) + listener.close() + return box.get("result", "NO SERVER RESULT") + + +def test_a_revoked_client_is_refused_by_a_crl_checked_context( + _crl_material: dict[str, str], +) -> None: + # THE CLAIM THAT MATTERS. Every other test in this block asserts that a flag is set or that a + # bad input is refused; none of them establishes that revocation actually happens. This drives + # a real mTLS handshake with a certificate the CRL names. + result = _crl_handshake(_crl_material["ca_and_fresh"], "revoked", _crl_material) + assert "revoked" in result.lower(), result + + +def test_a_good_client_is_accepted_by_the_same_context(_crl_material: dict[str, str]) -> None: + # POSITIVE CONTROL, and it is what separates a working revocation check from a context that + # refuses everyone -- which is exactly what trap 1 produces and what a flag assertion cannot + # tell apart. + assert _crl_handshake(_crl_material["ca_and_fresh"], "good", _crl_material) == "ACCEPTED" + + +def test_without_the_crl_the_revoked_client_gets_in(_crl_material: dict[str, str]) -> None: + # THE GAP ITSELF, pinned as a NEGATIVE CONTROL. This is the shipped posture the item is filed + # against: CA loaded, CERT_REQUIRED set, no CRL -- and a certificate revoked this morning + # authenticates until its notAfter. If this ever starts failing, revocation arrived by some + # other route and the item's premise needs re-deriving rather than the test relaxing. + assert _crl_handshake(None, "revoked", _crl_material) == "ACCEPTED" diff --git a/tests/test_username_identity_collation.py b/tests/test_username_identity_collation.py new file mode 100644 index 00000000..5b8e688a --- /dev/null +++ b/tests/test_username_identity_collation.py @@ -0,0 +1,219 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""BACKLOG #1268: what counts as "the same username" must be decided in ONE place. + +Two limbs of one root cause, and fixing either alone leaves the other a live trap: + +**Limb 1 -- the column.** ``users.username`` was the one identifier column in the SQL Server schema +without an explicit ``COLLATE``, so it inherited the database default -- case-INsensitive on a stock +install (``SQL_Latin1_General_CP1_CI_AS``). SQLite (``BINARY``) and Postgres (``TEXT``) are both +case-SENSITIVE, so ``Admin`` and ``admin`` were two accounts on two backends and one account on the +third, under a ``UNIQUE`` constraint that reads as if it had settled the question. + +**Limb 2 -- the gate.** ``_login_local`` decided whether to run the WP-3 bootstrap +expiry/supersession enforcement with a **Python** ``==`` against the caller's input, while the row +underneath was resolved by the **column's** collation. On a case-insensitive store those disagree in +exactly one direction: a login as ``Admin`` FAILS the Python guard (so retirement never runs) and +then SUCCEEDS at the lookup (returning the still-enabled bootstrap row). The ASVS 6.4.5 control that +disables a lapsed or superseded unclaimed bootstrap was reachable only through the guard it had just +walked past -- a compensating control resting on a false premise (SDS-3.7), the premise being that +the username the gate compared is the username the store matched. + +**Why limb 2 is tested against a SIMULATED case-insensitive store rather than a real SQL Server.** +The defect's mechanism is the disagreement between the two comparisons, not anything SQL Server does +uniquely. Gating this test on ``MEFOR_TEST_SQLSERVER`` would mean the assertion that actually pins +the fix does not run in normal CI -- and a green suite on the default store was exactly what made +this invisible in the first place. The proxy below reproduces the disagreement on SQLite, so the +guard is pinned everywhere, and limb 1 keeps the real column honest. +""" + +from __future__ import annotations + +import re +import time +from typing import Any + +import pytest + +from messagefoundry.auth.service import AuthService +from messagefoundry.config.settings import AuthSettings +from messagefoundry.store.store import MessageStore + +_BIN2 = "COLLATE Latin1_General_100_BIN2" + +#: The users table's declaration in any of the three dialects. Deliberately tolerant of the +#: ``IF NOT EXISTS`` all three actually use, and anchored on a word boundary so it cannot be +#: satisfied by a table merely named ``users_something``. +_USERS_TABLE = re.compile(r"CREATE TABLE(?:\s+IF NOT EXISTS)?\s+users\b", re.IGNORECASE) + + +# --- Limb 1: the column ------------------------------------------------------------------------- + + +def _sqlserver_users_ddl() -> str: + sqlserver = pytest.importorskip( + "messagefoundry.store.sqlserver", reason="requires the sqlserver extra (aioodbc)" + ) + users = [s for s in sqlserver._SCHEMA if _USERS_TABLE.search(s) is not None] + assert len(users) == 1, f"expected exactly one users DDL statement, got {len(users)}" + return users[0] + + +def test_the_username_column_pins_a_binary_collation() -> None: + """The auth column must not inherit the database default. + + Carries its own POSITIVE CONTROL: a sibling identifier column in the same statement is asserted + to already carry the collation. Without it, a test that only looked for ``username ... BIN2`` + would pass identically if ``_SCHEMA`` stopped being readable, if the users statement were + renamed, or if the collation string itself changed -- the null and the pass are the same output. + """ + ddl = _sqlserver_users_ddl() + assert _BIN2 in ddl, "control failed: no binary collation anywhere in the users DDL" + username = next( + (seg for seg in ddl.split(",") if seg.strip().startswith("username")), + None, + ) + assert username is not None, "control failed: no username column found in the users DDL" + assert _BIN2 in username, ( + "users.username inherits the database default collation. On a stock SQL Server install that " + "is case-INsensitive, which makes account identity store-dependent and lets a differently " + "cased spelling resolve to another account's row (BACKLOG #1268 limb 1)." + ) + + +def test_no_backend_declares_the_username_case_insensitively() -> None: + """All three stores must agree that usernames are case-SENSITIVE. + + Stated as a refusal of the case-insensitive spellings rather than a positive match, because the + three backends express the same decision three different ways (an explicit binary collation on + SQL Server; the absence of ``COLLATE NOCASE`` on SQLite; the absence of ``CITEXT`` or a + ``lower()`` functional index on Postgres). A positive match would have to enumerate three + dialects and would go quiet the moment a fourth backend arrived. + """ + from messagefoundry.store import store as sqlite_store + + postgres = pytest.importorskip( + "messagefoundry.store.postgres", reason="requires the postgres extra (asyncpg)" + ) + + # Matched on the table name alone, never on the full "CREATE TABLE users" phrase: every backend + # here spells it "CREATE TABLE IF NOT EXISTS users", so the literal phrase matches NOTHING and a + # test written around it reports a clean pass over an empty string. The control below is what + # turned that into a failure instead of a false green. + sqlite_ddl = next( + (s for s in sqlite_store._SCHEMA.split(";") if _USERS_TABLE.search(s) is not None), + None, + ) + assert sqlite_ddl is not None, "control failed: no users DDL found in the SQLite schema" + assert "username" in sqlite_ddl.lower(), "control failed: no username column in the SQLite DDL" + assert "nocase" not in sqlite_ddl.lower(), ( + "SQLite users.username must not be COLLATE NOCASE (#1268)" + ) + + pg_ddl = next((s for s in postgres._SCHEMA if _USERS_TABLE.search(s) is not None), None) + assert pg_ddl is not None, "control failed: no users DDL found in the Postgres schema" + assert "username" in pg_ddl.lower(), "control failed: no username column in the Postgres DDL" + assert "citext" not in pg_ddl.lower(), "Postgres users.username must not be CITEXT (#1268)" + + +# --- Limb 2: the gate --------------------------------------------------------------------------- + + +class _CaseInsensitiveLookupStore: + """A store whose ``get_user_by_username`` matches case-INsensitively, as a SQL Server column + under a stock ``CI`` collation does. Everything else delegates to the real store. + + This is the whole mechanism of #1268 limb 2 in one object: the ROW the engine gets back is + resolved by the store's rules, while the engine's own guard compared with Python's. + """ + + def __init__(self, inner: MessageStore) -> None: + self._inner = inner + + def __getattr__(self, name: str) -> Any: + return getattr(self._inner, name) + + async def get_user_by_username(self, username: str) -> Any: + exact = await self._inner.get_user_by_username(username) + if exact is not None: + return exact + for candidate in await self._inner.list_users(): + if candidate.username.casefold() == username.casefold(): + return await self._inner.get_user(candidate.id) + return None + + +async def _lapsed_bootstrap(inner: MessageStore, store: Any) -> tuple[AuthService, str]: + """An UNCLAIMED bootstrap admin whose WP-3 window has LAPSED, with every other refusal disarmed, + so the login gate is the only thing that can still refuse it. Returns service and password. + + **The EXPIRY arm, deliberately, and the SUPERSESSION arm is the trap.** The first version of + these tests used supersession -- create a second administrator, then log in. Both tests PASSED + against the unfixed code, which is what caught it: ``create_local_user`` retires the bootstrap + eagerly at ``service.py:2685``, so the account was **already disabled before the login ran** and + both tests were asserting a refusal that had nothing to do with the gate. Supersession can never + exercise this defect, because it never reaches the login path with retirement still pending. + Expiry can: nothing evaluates the window except ``_retire_superseded_bootstrap``, and on the + login path that call sits behind the guard under test. + + ``initial_password_expiry_hours=0`` disarms the ASVS 6.4.1 credential expiry, which would + otherwise refuse this login on its own and mask the result -- 6.4.1 is checked AFTER the password + verifies and is not routed through the bootstrap guard, so leaving it on produces a refusal that + looks like the control working while the control is being walked past. + """ + service = AuthService( + store, AuthSettings(bootstrap_expiry_hours=72, initial_password_expiry_hours=0) + ) + boot = await service.initialize() + assert boot is not None + admin = await inner.get_user_by_username("admin") + assert admin is not None and not admin.disabled + await inner._db.execute( + "UPDATE users SET created_at=? WHERE id=?", (time.time() - 73 * 3600, admin.id) + ) + await inner._db.commit() + return service, boot.password + + +async def test_exact_case_login_retires_the_lapsed_bootstrap() -> None: + """POSITIVE CONTROL for the test below, and it must run against the SAME proxy. + + Its job is to prove the proxy has not broken the ordinary path, so that when the differently + cased login behaves differently the CASE is the only variable. Run against a plain store it + would prove nothing about the proxied one. + """ + inner = await MessageStore.open(":memory:") + try: + service, password = await _lapsed_bootstrap(inner, _CaseInsensitiveLookupStore(inner)) + assert not (await service.login("admin", password)).ok + retired = await inner.get_user_by_username("admin") + assert retired is not None and retired.disabled + finally: + await inner.close() + + +async def test_a_differently_cased_login_cannot_walk_past_bootstrap_retirement() -> None: + """The sharp end of #1268. + + MEASURED against the unfixed code, with the control above passing in the same run: + ``login("admin")`` was refused and the account retired, while ``login("Admin")`` returned + ``ok=True`` and left ``disabled`` unset -- a lapsed, unclaimed bootstrap credential logging in + successfully because one letter was capitalised. ``"Admin" == "admin"`` is False, so + ``_retire_superseded_bootstrap`` never ran; the lookup underneath then resolved + case-insensitively and handed back the very row the skipped call would have disabled. + """ + inner = await MessageStore.open(":memory:") + try: + service, password = await _lapsed_bootstrap(inner, _CaseInsensitiveLookupStore(inner)) + outcome = await service.login("Admin", password) + assert not outcome.ok, ( + "a differently cased spelling of the bootstrap username walked past WP-3 retirement " + "and logged in with a LAPSED credential (BACKLOG #1268 limb 2)" + ) + retired = await inner.get_user_by_username("admin") + assert retired is not None and retired.disabled, ( + "retirement never ran for the differently cased login, so the lapsed bootstrap account " + "is still enabled" + ) + finally: + await inner.close() diff --git a/tests/test_worktree_gate_escaped_quote.py b/tests/test_worktree_gate_escaped_quote.py new file mode 100644 index 00000000..0c4ccd74 --- /dev/null +++ b/tests/test_worktree_gate_escaped_quote.py @@ -0,0 +1,675 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Escape-blind span scanning let a gated git command hide from every rule (BACKLOG #1229 residual). + +Two fail-opens, one root cause, and it is the same sentence this gate has already written twice: +**whichever quote opens first owns the span, and nothing that cannot decide ownership may blank text.** + +**ONE -- A BACKSLASH-ESCAPED QUOTE IS A SHELL LITERAL, AND THE SCAN READ IT AS AN OPENER.** In sh, +``\\"`` is an ordinary character; the command around it RUNS. ``Remove-QuotedSpans`` resolved quote +OWNERSHIP correctly and ignored ESCAPING entirely, so it paired two escaped quotes across a live +command and deleted the middle. No rule ever saw it. + +**IT IS RULE-AGNOSTIC, WHICH IS WHY THE COVERAGE HERE IS WIDER THAN THE ORIGINAL STRADDLE'S.** This is +not a checkout bug -- it is a SCANNER bug that disarms whatever rule sits behind it. Measured on the +shipped hook before the fix: the same shape hid ``checkout`` (rule 3) and ``reset --hard`` (rule 3, DESTRUCTIVE) alike. A suite that +pinned only the checkout case would go green over the destructive one. + +**TWO -- THE QUOTED-PROGRAM-PATH COLLAPSE WAS TWO ORDERED REGEXES RUN BEFORE THE SCAN**, double quotes +first, which is the *exact* shape the scan replaced. It could pair a quote with a distant ``/git"`` +ACROSS a gated command and rewrite the whole middle to a bare token -- verb and arguments gone. Both +are now decided inside the single left-to-right pass, on spans it already owns. + +**PRE-EXISTING, NOT A REGRESSION.** Both shapes ALLOW on the pre-fix blob too. This suite is the first +thing in the repository that can see either: measured across all 13 ``test_worktree_gate*`` modules +before it, ZERO carried a backslash-escaped-quote case (positive control: the token ``escap`` appears +in four of them, every hit unrelated). + +**WHY THE PAYLOADS ARE HERE IN A PUBLIC FILE, since that was a real question and not an oversight.** +Owner-ruled 2026-08-20: pin it publicly. A gate that refuses a class needs a real offender to prove it +refuses it -- an anti-vacuity control cannot be written from a description -- and this repo already +ships that shape deliberately (``tests/test_cp1252_console_safety.py`` builds a synthetic offender; +``scan_forbidden.py`` commits one it recognises without it being usable). The non-reusable variant was +designed first and does not reach here: the scanner is a PowerShell function with no importable +surface, and all 13 sibling suites drive the hook end-to-end through this same subprocess harness. +Inventing a second, unprecedented test path for a security gate to avoid a construct the owner already +authorised would be the worse trade. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from tests.test_worktree_gate import assert_denied, run_gate # reuse the subprocess harness + +# Built by concatenation, matching the sibling straddle suite: a test about quote handling must not +# depend on how this file's own literals nest. The escape is spelled once, here, for the same reason. +DQ = '"' +SQ = "'" +ESC_DQ = "\\" + DQ # a BACKSLASH then a quote -- one shell literal, not a span opener +ESC_SQ = "\\" + SQ + + +@pytest.fixture +def primary(tmp_path: Path) -> Path: + return tmp_path / "Repo" + + +@pytest.fixture +def repos_file(tmp_path: Path, primary: Path) -> Path: + f = tmp_path / "repos.txt" + f.write_text(f"{primary}\n", encoding="utf-8") + return f + + +def shell(command: str, cwd: Path) -> dict[str, object]: + """A Bash tool payload, matching the sibling suites' harness.""" + return {"tool_name": "Bash", "tool_input": {"command": command}, "cwd": str(cwd)} + + +# THE VERB SET IS THE POINT OF THIS PARAMETRISATION, not thoroughness for its own sake. The defect is +# in the SCANNER, so it disarms every rule equally; pinning one verb would leave the suite green while +# the destructive arm stayed open. +# +# ***BUT THIS SUITE DOES NOT ACTUALLY COVER MORE THAN ONE RULE, AND AN EARLIER VERSION OF THIS COMMENT +# CLAIMED IT DID. MEASURED, WHICH IS THE ONLY REASON I KNOW:*** both verbs are denied by the SAME rule +# -- each returns "would change the working tree of the SHARED PRIMARY checkout". Two verbs, one rule. +# The defect is rule-AGNOSTIC, so the coverage that would actually pin that property has to exercise +# DIFFERENT RULES, and this file exercises one. +# +# **AND "DENIED" IS A WEAKER CLAIM THAN "DENIED BY THE RULE WE THINK."** A suite that asserts only that +# a deny happened cannot tell a rule-agnostic scanner fix from a lucky overlap in one rule's matching. +# The stronger form -- reading back the rule ids the gate RECORDS and asserting they are distinct -- +# was built independently on another lane and is the right home for that coverage; this file should +# not grow a second, thinner version of it. Cross-checked there: 10 of its 11 rows pass against this +# gate unchanged. +# +# Left as-is deliberately rather than widened here, and the over-claim corrected in place rather than +# deleted, because the gap is real and a reader who saw only the fixed comment could not tell this +# suite had ever asserted coverage it does not have. +# +# `worktree add` WAS IN THIS LIST AND WAS REMOVED, WHICH IS WORTH RECORDING RATHER THAN TIDYING AWAY. +# A report reached me claiming the escape hid it, so it went in as a third case and FAILED. The +# discriminating probe is the one that settles it -- run the same command WITH and WITHOUT the escape: +# git -C worktree add ../x main -> ALLOW +# echo \" ; git -C worktree add ../x main ; echo \" -> ALLOW +# It allows either way, so the escape hides nothing there and the case proved nothing about this fix. +# The cause is stated in this file's own rule-3b resolver comment: 3b handles checkout/switch only. +# KEEPING THE FAILING CASE WOULD HAVE PRESSURED THE NEXT READER TO WIDEN A SECURITY GATE UNTIL A TEST +# BUILT ON A FALSE PREMISE WENT GREEN. Whether `worktree add` SHOULD be governed is a real question and +# a separate one; it is not evidence about escape handling and must not be smuggled in as such. +@pytest.mark.parametrize( + "verb", + [ + "checkout main", # rule 3 -- the shape the original straddle used + "reset --hard", # rule 3, DESTRUCTIVE + ], +) +def test_an_escaped_quote_does_not_hide_a_gated_command( + primary: Path, repos_file: Path, verb: str +) -> None: + """The gated command sits between two ESCAPED quotes and must still be seen. + + Before the fix both ALLOWed. The escapes are literals to the shell, so the middle command + really does run -- this is not a theoretical parse difference. + """ + command = f"echo {ESC_DQ} ; git -C {primary} {verb} ; echo {ESC_DQ}" + assert_denied(run_gate(shell(command, primary.parent), repos_file)) + + +def test_the_mirrored_escaped_shape_also_denies(primary: Path, repos_file: Path) -> None: + """The apostrophe arm, so neither quote character is special-cased. + + The ORIGINAL straddle was asymmetric -- the mirrored shape denied by accident of regex ordering, + which is what hid it from one side. A fix that restored an asymmetry would pass the test above. + """ + command = f"echo {ESC_SQ} ; git -C {primary} reset --hard ; echo {ESC_SQ}" + assert_denied(run_gate(shell(command, primary.parent), repos_file)) + + +def test_an_escape_inside_a_real_quoted_span_is_still_blanked( + primary: Path, repos_file: Path +) -> None: + """THE OTHER ARM, and without it the fix could be 'never blank anything'. + + Here the backslash sits INSIDE a genuine single-quoted span, where sh gives it no special meaning. + The span is real, so it must still be blanked and must not supply a verb -- a commit message + cannot become a command. This is the case that makes the test above evidence rather than an + assertion that the gate denies everything. + """ + command = f"git -C {primary} commit -m {SQ}a\\{DQ}b checkout main{SQ}" + assert run_gate(shell(command, primary.parent), repos_file) is None + + +def test_a_quoted_program_path_still_keeps_its_git_token(primary: Path, repos_file: Path) -> None: + """The false-NEGATIVE guard the collapse exists to provide, now decided inside the scan. + + `"C:/Program Files/Git/bin/git.exe" -C reset --hard` is a real spelling. Blanking the + span wholesale would drop the verb and ALLOW, so the token has to survive -- and it has to survive + without a pre-pass that can pair across a live command. + """ + command = f"{DQ}C:/Program Files/Git/bin/git.exe{DQ} -C {primary} reset --hard" + assert_denied(run_gate(shell(command, primary.parent), repos_file)) + + +@pytest.mark.parametrize("spelling", ["git", "git.exe"]) +def test_the_program_path_token_survives_the_backslash_separated_spelling( + primary: Path, repos_file: Path, spelling: str +) -> None: + """The backslash-separated program path, which the naive form of this fix misses. + + ``GIT.EXE`` AND ``Git`` WERE ROWS HERE AND ARE NOT ANY MORE, on a measurement rather than a + tidy-up. The emit is deliberately case-SENSITIVE (``-cmatch``), so those spellings blank wholesale + and ALLOW -- and that is NOT a regression, because ``origin/main`` ALLOWs them too. Main's collapse + regex is case-INSENSITIVE but substitutes ``$1``, preserving the original case, and every rule + downstream compares case-sensitively; main therefore mints a token nothing recognises and lands on + the same verdict by a different route. Measured on both blobs:: + + "<...>\\Git\\bin\\git.exe" -C reset --hard main=DENY this build=DENY + "<...>\\Git\\bin\\GIT.EXE" -C reset --hard main=ALLOW this build=ALLOW + "<...>\\Git\\bin\\Git" -C reset --hard main=ALLOW this build=ALLOW + + The uppercase hole is disclosed as a tripwire further down this file rather than dropped silently: + see ``test_the_UPPERCASE_quoted_PROGRAM_spelling_is_a_known_open_residual``. + """ + command = f"{DQ}C:\\Program Files\\Git\\bin\\{spelling}{DQ} -C {primary} reset --hard" + assert_denied(run_gate(shell(command, primary.parent), repos_file)) + + +def test_an_ordinary_command_is_still_allowed(primary: Path, repos_file: Path) -> None: + """The global negative control. A gate that denies everything passes every test above.""" + assert run_gate(shell("echo hello ; ls -la", primary.parent), repos_file) is None + + +# VERIFIED STATE OF THIS FAMILY, recorded because TWO OF MY OWN COMMIT MESSAGES SAY OTHERWISE AND +# A COMMIT MESSAGE CANNOT BE AMENDED ONCE PUSHED (BACKLOG #1229 residual). +# +# `4d46a2a0` and `c308cc34` both state that rules 3c and 3d are UNADDRESSED and that I could not +# reproduce them. **THEY ARE CLOSED.** Verified COLD by the seat that found them, on a rig that +# predates my fix and which I did not write: all four PowerShell shapes now DENY where the previous +# fix ALLOWed, including 3c (shared core.hooksPath) and 3d (another session's worktree). +# +# MY NON-REPRODUCTION WAS MY HARNESS, NOT THE GATE. My control allowed on origin/main too, which +# means my shapes never exercised rule 3c at all -- evidence about my construction. Gating the +# escape AT THE SCANNER closed all three rules at once, which is what rule-agnostic cuts both ways +# means: one character disarmed three rules, and one flag re-armed them. +# +# AND THE EXTRACTION FIX CLOSED AN INHERITED DEFECT TOO, which I did not set out to fix and would +# not have claimed without measuring both gates: +# +# one-level escaped interpreter arg main ALLOW -> DENY inherited, now closed +# +# bash -c "echo \\"x\\"; git -C reset --hard" +# +# NAMED HERE DELIBERATELY, under the owner's ruling that a construct is published only ALONGSIDE +# THE FIX THAT MAKES IT INERT. This one is inert in this commit and travels in the same change, +# so it cannot be lifted from here and used against this tree. It is named because an unnamed +# improvement claim is not checkable: 'main ALLOW -> DENY' asks a reader to take the author's +# word for what was measured, and the whole point of the left column is that it can be re-run. +# two-level escaped interpreter arg main DENY -> DENY my regression, repaired +# +# THE LEFT COLUMN IS THE POINT. A fix is not only judged against the branch it repairs; measuring +# against main is what distinguishes 'restored' from 'improved', and I would have reported the +# weaker claim. +# +# LEFT OPEN AND NOT CLAIMED: twelve further shapes the same lens reports as INHERITED from main -- +# command substitution, backticks, ANSI-C quoting, concatenated quoting, heredocs, bare program +# names, and the per-line split. None is introduced here and none is fixed here. They are with the +# Dispatcher as a disclosure question. + + +# COVERAGE NOTE, deliberately a comment rather than an assertion, because what it names is a CLASS and +# a class cannot be enumerated by a test (BACKLOG #1229 residual). +# +# What let both fail-opens live for so long was not a missing case -- it was that NOTHING in 13 suites +# could see the class at all, so 13 green suites were evidence about the classes they covered and +# silent about this one. Both defects were pre-existing, and both survived a dedicated pass over this +# exact function. +# +# The scan's contract is: text a rule must judge is never blanked, and text inside a span the SHELL +# would quote always is. Anything that decides span boundaries WITHOUT that ownership rule -- a regex +# pair, a pre-pass, a lookahead -- has been wrong here three times now. If you add one, it belongs +# inside Remove-QuotedSpans' single pass, and it needs a case in this file. + + +# --- BACKLOG #1229 residual, SECOND ROUND: the escape rule is HOST-SPECIFIC ---------------------- +# +# THE FIRST VERSION OF THIS FIX RE-CREATED #1229'S OWN DEFECT ON THE OTHER HOST, and it did so through +# the decision this file argued FOR. Honouring a backslash escape inside a double-quoted span is +# correct POSIX -- but `scripts/hooks/worktree_gate.ps1:999` scans BOTH tool names through ONE matcher +# (`$tool -in @("Bash", "PowerShell")`), and **PowerShell has no backslash escape**; its escape is the +# backtick. So on a PowerShell payload the scan held a span open that PowerShell had already CLOSED, +# straddled the live command, and blanked it. +# +# Found by the seat whose own design row asserted exactly this property -- a row I argued should be +# dropped, and which they dropped agreeing with me. Their commit called it "a deny no shell requires". +# That sentence is true of sh and FALSE OF POWERSHELL, and neither of us checked the second half. +# +# EVERY ROW BELOW IS PINNED TO WHETHER THE COMMAND ACTUALLY EXECUTES ON THAT HOST, measured with an +# inert payload that COMPUTES (`111*3` -> 333) rather than echoes, so an echo-back cannot be mistaken +# for a run. That is the only ground truth here: "should this deny" is a question about the shell, not +# about the gate. +_ODD_BS = 'Write-Output "C:\\Temp\\" ; {gated} ; Write-Output "x"' +# NOT MADE RAW, WHICH IS THE ESCAPE-SEQUENCE GATE'S OWN PRESCRIBED REMEDY (BACKLOG #1229 residual). +# `r'...'` would preserve the `\\\` as TWO backslashes, turning this line's ODD trailing count +# into an EVEN one -- and per the measured table ODD is the ALLOW case under test while EVEN DENIES. +# The remedy would have yielded a GREEN test of the non-regressing case: the regression row neutered, +# with nothing reporting it. Escaping only the INVALID sequence preserves the value exactly (verified +# by comparing ast.literal_eval before and after, not by reading the line). +# Generally: that remedy is unsafe for any literal that ALSO contains a valid backslash escape. +_STRADDLE = 'echo \\" ; {gated} ; echo \\"' + + +@pytest.mark.parametrize( + "tool,template,expect_deny,measured", + [ + # pwsh -NoProfile -Command '...(111*3)...' printed 333 -> the middle statement RAN. + ("PowerShell", _ODD_BS, True, "333 printed: middle RAN"), + # bash -c '...' -> "unexpected EOF while looking for matching \"" -> nothing parses, nothing runs. + ("Bash", _ODD_BS, False, "syntax error: nothing executes"), + # pwsh printed the line as a literal string, no 333 -> the middle did NOT run. + ("PowerShell", _STRADDLE, False, "no 333: middle did NOT run"), + # bash printed 333 -> the middle RAN. + ("Bash", _STRADDLE, True, "333 printed: middle RAN"), + ], +) +def test_the_verdict_matches_whether_the_command_RUNS_on_that_host( + primary: Path, repos_file: Path, tool: str, template: str, expect_deny: bool, measured: str +) -> None: + """The gate must model each shell's real quoting, not one shell's. + + ``measured`` is not decoration -- it records the observation each row is pinned to, so a future + reader can tell an assertion grounded in shell behaviour from one grounded in a previous verdict. + A row that only asserted "deny" would be satisfied by a gate that denies everything. + """ + command = template.format(gated=f"git -C {primary} reset --hard") + result = run_gate( + {"tool_name": tool, "tool_input": {"command": command}, "cwd": str(primary.parent)}, + repos_file, + ) + if expect_deny: + assert_denied(result), f"{tool}: {measured} -- the gate must see it" + else: + assert result is None, f"{tool}: {measured} -- denying it would be a FALSE DENY" + + +def test_an_unknown_host_gets_the_CONSERVATIVE_reading() -> None: + """The default is fail-CLOSED, and the direction is the whole reason it is a default. + + Honouring the escape makes spans LONGER, so it blanks MORE and can hide a command -- fail OPEN. + Refusing it leaves more text visible to the rules -- fail CLOSED. So `$PosixEscapes` defaults to + false and only a host known to use backslash escapes opts in. + + Asserted on the SOURCE because the parameter default is the guarantee; a behavioural probe would + need a third tool name the gate does not currently accept. + """ + gate = ( + Path(__file__).resolve().parents[1] / "scripts" / "hooks" / "worktree_gate.ps1" + ).read_text(encoding="utf-8") + assert "[bool]$PosixEscapes = $false" in gate, ( + "the escape rule must default to OFF: an unrecognised host has to get the reading that blanks " + "less, or a future tool name silently inherits sh semantics (BACKLOG #1229 residual)" + ) + assert '($tool -eq "Bash")' in gate, ( + "the opt-in must be keyed on the host, not left unconditional" + ) + + +def test_an_escaped_quote_in_an_interpreter_ARGUMENT_still_reaches_the_inner_code() -> None: + """BACKLOG #1229 residual, THIRD round -- and the host flag alone could not close it. + + THE EXTRACTION AND THE BLANKING MUST AGREE ABOUT WHERE THE ARGUMENT ENDS. The interpreter-argument + regex was ``[^"]*``, which is escape-BLIND and stops at the first quote INCLUDING an escaped one. + Once the span blanking became escape-AWARE the two disagreed:: + + bash -c "bash -c \\"git -C reset --hard\\"" + extraction got `bash -c \\` -- truncated at the escaped quote, no verb in it + blanking removed the whole span + so nothing reached any rule -> ALLOW + + MEASURED: origin/main DENY x3, the escape-aware fix ALLOW x3, and the control below DENY on both. + The inner command really runs -- ``bash -c "bash -c \\"expr 111 \\* 3\\""`` prints 333. + + ON MAIN THE TWO AGREED BY ACCIDENT, both being escape-blind, which left the verb visible OUTSIDE + the span. Making one side escape-aware removed the accident without replacing it. **A host flag + cannot fix this**: the failing host is Bash, where the escape is real and honouring it is correct. + """ + import tempfile + + d = Path(tempfile.mkdtemp()) + primary = d / "Repo" + rf = d / "repos.txt" + rf.write_text(f"{primary}\n", encoding="utf-8") + gated = f"git -C {primary} reset --hard" + + escaped = f"bash -c {DQ}bash -c {ESC_DQ}{gated}{ESC_DQ}{DQ}" + assert_denied(run_gate(shell(escaped, d), rf)) + + # THE DISCRIMINATING CONTROL: identical nesting, no escape. It denies on main and on the fix, so + # the trigger is the ESCAPE and not the nesting -- without this row the test above would pass + # against a gate that simply denied anything containing `bash -c`. + plain = f"bash -c {SQ}bash -c {DQ}{gated}{DQ}{SQ}" + assert_denied(run_gate(shell(plain, d), rf)) + + +# --- BACKLOG #1229 residual, FOURTH ROUND: the emit is CASE-SENSITIVE, and the PROGRAM-POSITION +# --- experiment that briefly stood here was REVERTED (owner ruling, 2026-08-21) ------------------- +# +# Keeping the git token of a closed span is the false-NEGATIVE guard the three tests above pin. Two +# further changes were layered on top of it and BOTH ARE GONE. The tests that pinned them went with +# them; this block records the dead end so the next reader does not walk back into it. +# +# WHAT WAS TRIED. +# (a) The emit was made case-INSENSITIVE and canonicalised to lowercase, so that `GIT.EXE` -- a real +# Windows spelling the case-SENSITIVE rules downstream otherwise skip -- would still present a +# verb for those rules to judge. Fail-closed in direction, and sound in isolation. +# (b) (a) then read `cp -r "/c/backups/Git" restore` as a git command, so a `Test-GitProgramPosition` +# predicate was added to keep the token only where the span is dispatched as a PROGRAM -- a +# command boundary reachable leftward across an allowlist of wrapper words. +# +# WHY IT WAS REVERTED, and it is not the false denies. The predicate bought those back at the price of +# two fail-OPENS on shapes `origin/main` DENIES: +# +# cmd /c "<...>\Git\bin\git.exe" -C reset --hard main=DENY experiment=ALLOW +# . "<...>\Git\bin\git.exe" -C reset --hard main=DENY experiment=ALLOW +# (the second is a PowerShell dot-source, on a PowerShell tool call) +# +# Spending a security gate's DENY to buy a tidier false-deny profile is the wrong direction, so (a) and +# (b) were both withdrawn. RE-MEASURED after the revert, on the same rig and the real hook: both rows +# are DENY again, matching main. +# +# WHAT IS TRUE NOW. Every row below measured over the real hook against `origin/main` and against this +# tree, and IDENTICAL on the two blobs -- so none of it is introduced here and none of it is repaired +# here: +# +# "<...>\Git\bin\git.exe" -C reset --hard DENY the guard the emit exists for +# "<...>\Git\bin\GIT.EXE" -C reset --hard ALLOW a residual, pinned below +# cp -r "/c/backups/Git" restore (cwd = the governed repo) ALLOW the leaf case is what saves it +# cp -r "/c/backups/git" restore (cwd = the governed repo) DENY a false deny, pinned below +# +# So the case-sensitivity is not a tie-break between two right answers. It is the ONLY thing standing +# between the argument-position family and a daily false deny, and it leaks the uppercase PROGRAM +# spelling in exchange. BOTH ends are pinned as tripwires below rather than left to a comment, because +# a residual that lives only in prose is one nobody notices closing. + + +# The must-ALLOW half: the case-sensitivity is what keeps this family out of the deny path. Every row +# is the same span in ARGUMENT position, and each carries its own discriminating control that varies +# THE LEAF CASE ALONE -- so no row can pass against a gate that has simply stopped keeping the token, +# and none can pass against one that denies everything. +@pytest.mark.parametrize( + "argument_shape", + [ + "cp -r {q} restore", + "rsync -a {q} restore", + "mv {q} clean", + "ls {q} clean", + "echo {q} merge", + "7z a out.7z {q} am", + "cp -r {q} switch", + "du -sh {q} restore", + "head -n 5 {q} clean", + "docker run --rm -v {q} restore", + # A REDIRECT TARGET, a command substitution and a brace expansion left of the span, and an + # earlier span this same scan already blanked. All four were shapes the reverted predicate had + # to reason about explicitly; under a case-sensitive emit they need no special handling at all, + # which is most of the argument for the simpler rule. + "echo hi > {q} clean", + "cp -r $(pwd) {q} clean", + "cp -r ${BACKUP} {q} clean", + 'cp -r "/a/Git" {q} clean', + ], +) +def test_a_TITLE_CASED_quoted_git_leaf_cannot_supply_a_verb( + primary: Path, repos_file: Path, argument_shape: str +) -> None: + """A quoted PATH whose leaf is `Git` is not a git command, and the CWD here is load-bearing. + + THE EARLIER VERSION OF THIS TEST RAN FROM ``primary.parent`` AND COULD NOT FAIL. From there a bare + ``git restore`` names no governed target, so the row ALLOWs whatever the scanner emitted -- it was + measuring the target resolver, not the emit. Moving the cwd INTO the governed repo makes the emit + the only variable, and the leaf case then separates cleanly. Measured, all 14 rows, both blobs:: + + cp -r "/c/backups/Git" restore ALLOW + cp -r "/c/backups/git" restore DENY + """ + quoted = f"{DQ}/c/backups/Git{DQ}" + # `.replace` rather than `.format`: one row carries a literal `${BACKUP}` and str.format would + # read those braces as a field name and raise -- turning a probe into a collection error. + shaped = argument_shape.replace("{q}", quoted) + assert run_gate(shell(shaped, primary), repos_file) is None, ( + "a quoted PATH whose leaf is `Git` is not a git command, and denying it stops legitimate " + "work over a directory name (BACKLOG #1229 residual, fourth round)" + ) + # THE DISCRIMINATING CONTROL, and it varies THE LEAF CASE alone: the identical line with a + # lowercase leaf really does emit a token and really does deny. This is a FALSE DENY and it is + # pinned as such below -- it is used here because it is the sharpest available control, not + # because it is the right answer. + assert_denied( + run_gate( + shell(shaped.replace(f"{DQ}/c/backups/Git{DQ}", f"{DQ}/c/backups/git{DQ}"), primary), + repos_file, + ) + ) + + +def test_the_UPPERCASE_quoted_PROGRAM_spelling_is_a_known_open_residual( + primary: Path, repos_file: Path +) -> None: + """A TRIPWIRE OVER THE COST OF THE CASE-SENSITIVE EMIT. It asserts ALLOW and that is NOT an + endorsement -- read this before acting on it. + + ``GIT.EXE`` is a real spelling on Windows and the emit is case-SENSITIVE, so the span blanks + wholesale and no verb reaches any rule:: + + "<...>\\Git\\bin\\GIT.EXE" -C reset --hard ALLOW (origin/main: ALLOW) + "<...>\\Git\\bin\\Git" -C reset --hard ALLOW (origin/main: ALLOW) + + PRE-EXISTING, NOT INTRODUCED HERE, and the mechanism on main is worth stating because it looks + like it should differ: main's collapse regex IS case-insensitive, but it substitutes ``$1``, which + preserves the original case, and every rule downstream compares case-sensitively. Main therefore + mints a token nothing recognises and lands on the same verdict by a different route. + + THE ONLY REMEDY TRIED COST MORE THAN IT BOUGHT. Making the emit case-insensitive closes these two + and re-opens the whole argument-position family above; adding a program-position discriminator to + hold both ends opened `cmd /c` and PowerShell dot-source as fail-opens that main denies. Do NOT + re-add the lowercase emit without a discriminator, and do not add a discriminator without + re-measuring those two. + + WHEN THIS TEST REDS, that is the success signal: somebody closed the hole. Delete the row; do not + restore the ALLOW. + """ + for leaf in ("GIT.EXE", "Git", "Git.exe"): + shape = f"{DQ}C:\\Program Files\\Git\\bin\\{leaf}{DQ} -C {primary} reset --hard" + assert run_gate(shell(shape, primary.parent), repos_file) is None, ( + f"{shape} now DENIES. If you closed the uppercase program spelling deliberately, that is " + "the intended outcome -- delete this test. Do NOT restore the ALLOW, and check that " + "`cmd /c` and PowerShell dot-source still DENY." + ) + # THE CONTROL that keeps the tripwire attached to the CASE and not to the whole emit: the + # lowercase spelling in the identical slot must still deny. + assert_denied( + run_gate( + shell( + f"{DQ}C:\\Program Files\\Git\\bin\\git.exe{DQ} -C {primary} reset --hard", + primary.parent, + ), + repos_file, + ) + ) + + +def test_a_LOWERCASE_quoted_git_LEAF_in_argument_position_is_a_known_open_FALSE_DENY( + primary: Path, repos_file: Path +) -> None: + """THE OTHER END OF THE SAME TRADE, and it asserts the WRONG answer on purpose. + + The emit is unconditional in position, so an ordinary backup directory whose leaf really is + lowercase ``git`` reads as a git command and its next word becomes a verb:: + + cp -r "/c/backups/git" restore DENY (origin/main: DENY) + cp -r "./git" restore DENY (origin/main: DENY) + + Both measured from INSIDE the governed repo, on both blobs. This is a FALSE DENY: nothing here is + a git invocation. It is pinned rather than described because the previous occupant of this slot -- + a test asserting these ALLOW -- passed only because it ran from ``primary.parent``, where no + governed target resolves and the verdict is decided somewhere else entirely. + + NOT A WEAKENING AND NOT A REGRESSION EITHER WAY: main denies these too, and the reverted predicate + is what briefly allowed them. + + WHEN THIS TEST REDS, somebody taught the emit to tell a program from an argument without the two + fail-opens the last attempt cost. Delete the row; do not restore the DENY. + """ + for shape in (f"cp -r {DQ}/c/backups/git{DQ} restore", f"cp -r {DQ}./git{DQ} restore"): + # If this reds, the shape now ALLOWs. Delete the row; do not restore the DENY, and re-check + # that `cmd /c` and PowerShell dot-source of a quoted `git.exe` still DENY. + assert_denied(run_gate(shell(shape, primary), repos_file)) + # THE CONTROL: a leaf that is not a git token at all never emitted one, so it never denied. It + # separates "the emit fired" from "this gate denies any `cp`". + assert run_gate(shell(f"cp -r {DQ}/c/backups/GitHub{DQ} restore", primary), repos_file) is None + + +@pytest.mark.parametrize( + "tool,template,why", + [ + # cmd.exe /c runs the quoted program; the default Git install path contains a space and MUST + # be quoted, so this is the ordinary spelling and not an exotic one. Executed here: + # cmd.exe /c '"C:\Program Files\Git\bin\git.exe" --version' printed a git version. + ("Bash", "cmd /c {prog} -C {gated} reset --hard", "cmd /c dispatches the quoted program"), + # PowerShell dot-source. Verified on pwsh 7.6.4 that `.` runs the executable, same as `&`. + ("PowerShell", ". {prog} -C {gated} reset --hard", "dot-source dispatches it too"), + ], +) +def test_the_two_fail_OPENS_the_reverted_PREDICATE_INTRODUCED_are_DENIED_again( + primary: Path, repos_file: Path, tool: str, template: str, why: str +) -> None: + """THE REVERT'S OWN JUSTIFICATION, AS AN ASSERTION RATHER THAN A COMMENT. + + The `Test-GitProgramPosition` predicate was withdrawn because it moved exactly these two shapes + from DENY to ALLOW while `origin/main` denies both. That reason lived only in prose -- in the + narrative block above and inside two failure strings -- so nothing would have reported it if the + predicate came back. Every test that DOES red on the predicate reds on a FALSE-DENY row whose own + text says to delete it, which means the suite's stated remedy, followed literally, lands both + fail-opens green. These two rows are the missing half. + + Measured over the real hook, cwd = the governed repo, on `origin/main` and on this tree:: + + cmd /c "<...>\\Git\\bin\\git.exe" -C reset --hard DENY / DENY + . "<...>\\Git\\bin\\git.exe" -C reset --hard DENY / DENY + + and on the withdrawn predicate blob (`fb93c9ca`), ALLOW for both. + + WHEN THIS TEST REDS, a change has re-opened a hole `origin/main` closes. Do NOT delete the row. + """ + prog = f"{DQ}C:\\Program Files\\Git\\bin\\git.exe{DQ}" + gated = template.format(prog=prog, gated=primary) + ( + assert_denied( + run_gate( + {"tool_name": tool, "tool_input": {"command": gated}, "cwd": str(primary)}, + repos_file, + ) + ), + f"{tool}: {why} -- this is the fail-open the revert exists to keep shut", + ) + # THE CONTROL, and it is what stops the row degenerating into "the gate denies any `cmd /c`": + # the identical line aimed at a path no repos file governs must ALLOW. Measured ALLOW on both + # blobs, so a gate that denied unconditionally would fail here. + ungoverned = template.format(prog=prog, gated=primary.parent / "NotGoverned") + assert ( + run_gate( + { + "tool_name": tool, + "tool_input": {"command": ungoverned}, + "cwd": str(primary.parent), + }, + repos_file, + ) + is None + ), f"{tool}: an UNGOVERNED target must allow, or the deny above proves nothing" + + +# --- BACKLOG #1229 residual, FOURTH ROUND: the convention belongs to the INTERPRETER -------------- +# +# The parametrised test above pins the OUTER host: a Bash tool call gets POSIX escape rules and a +# PowerShell tool call does not. That is right about the line the tool typed, and it was applied to +# something else as well -- the payload EXTRACTED from an interpreter flag on that line. A Bash tool +# call invoking pwsh therefore read a PowerShell payload under POSIX backslash rules, held open a span +# PowerShell had already closed, and blanked the gated command between it and a later quote. +# +# THE PROOF THAT ONE FLAG PER LINE CANNOT EXPRESS THIS is that the SAME CHARACTERS have opposite +# correct answers. Measured on this box with a payload that COMPUTES rather than echoes: +# +# pwsh -NoProfile -Command '$d = "C:\Temp\" ; 111*3 ; Write-Output "x"' -> printed 333: it RAN +# bash -c '$d = "C:\Temp\" ; expr 111 \* 3 ; echo "x"' -> unexpected EOF: INERT +# sh -c (same) -> unexpected EOF: INERT +# +# So the pwsh row must deny and the bash row must not, from one line, under one tool name. The +# convention now comes from Get-FlagOwner -- the program that owns the matched flag -- and the outer +# line keeps the outer host's rules, because the outer line really is the outer host's. + +_INNER_STRADDLE = '$d = "C:\\Temp\\" ; {gated} ; Write-Output "x"' + + +@pytest.mark.parametrize( + "tool,invocation,expect_deny,measured", + [ + # The fail-open. All three read a PowerShell payload under the OUTER host's POSIX rules. + ("Bash", "pwsh -Command", True, "333 printed: the middle statement RAN"), + ("Bash", "pwsh -c", True, "333 printed: the middle statement RAN"), + ("Bash", "powershell -Command", True, "333 printed: the middle statement RAN"), + # The same characters where POSIX rules are CORRECT. Denying these would be a false deny, and + # they are what stops the rows above from being satisfied by "always fail closed". + ("Bash", "bash -c", False, "unexpected EOF: nothing parses, nothing runs"), + ("Bash", "sh -c", False, "unexpected EOF: nothing parses, nothing runs"), + # THE CONVERSE DIRECTION, and it is the half that proves the convention is not simply pinned + # to the payload's own tool name either: a PowerShell tool call invoking bash used to apply + # PowerShell rules to a POSIX payload, which denied a shape that cannot run. + ("PowerShell", "bash -c", False, "unexpected EOF: nothing parses, nothing runs"), + ("PowerShell", "sh -c", False, "unexpected EOF: nothing parses, nothing runs"), + ("PowerShell", "pwsh -Command", True, "333 printed: the middle statement RAN"), + ], +) +def test_the_escape_convention_of_an_EXTRACTED_payload_comes_from_its_interpreter( + primary: Path, repos_file: Path, tool: str, invocation: str, expect_deny: bool, measured: str +) -> None: + """Every row is pinned to whether the payload RUNS on the interpreter that receives it. + + ``measured`` records the observation rather than a previous verdict, for the reason the sibling + matrix above gives: a row that asserted only "deny" would be satisfied by a gate that denies + everything, and a row that asserted only "allow" by one that recurses into nothing. + """ + payload = _INNER_STRADDLE.format(gated=f"git -C {primary} reset --hard") + command = f"{invocation} {SQ}{payload}{SQ}" + result = run_gate( + {"tool_name": tool, "tool_input": {"command": command}, "cwd": str(primary.parent)}, + repos_file, + ) + if expect_deny: + assert_denied(result), f"{tool}/{invocation}: {measured} -- the gate must see it" + else: + assert result is None, ( + f"{tool}/{invocation}: {measured} -- denying it would be a FALSE DENY" + ) + + +def test_the_escape_count_and_not_the_nesting_is_what_triggers_the_straddle( + primary: Path, repos_file: Path +) -> None: + """CONTROL for the matrix above: the same nesting with an EVEN backslash run, and with none. + + Both denied before this change and both deny after it, on either reading of the escape -- so a + row from the matrix that moved has moved because of the ODD trailing run, not because the gate + started or stopped objecting to ``pwsh -Command`` in general. + """ + gated = f"git -C {primary} reset --hard" + for payload in ( + '$d = "C:\\Temp\\\\" ; ' + gated + ' ; Write-Output "x"', # EVEN run + '$d = "C:/Temp" ; ' + gated + ' ; Write-Output "x"', # no backslash at all + ): + assert_denied( + run_gate(shell(f"pwsh -Command {SQ}{payload}{SQ}", primary.parent), repos_file) + ) diff --git a/tests/test_worktree_gate_hijack.py b/tests/test_worktree_gate_hijack.py index 5f23e497..008cd71a 100644 --- a/tests/test_worktree_gate_hijack.py +++ b/tests/test_worktree_gate_hijack.py @@ -459,3 +459,101 @@ def test_a_worktree_of_an_UNGOVERNED_repo_is_untouched(tmp_path: Path) -> None: repos = tmp_path / "repos.txt" repos.write_text(f"{tmp_path / 'SomethingGoverned'}\n", encoding="utf-8") # NOT the alien repo assert run_gate(shell("git checkout some-branch", cwd=wt), repos) is None + + +# ------------------------------------------- a quoted path must not SHADOW the verb rule 3b judges +# +# BACKLOG #1229 residual, fourth round. Rule 3 records the FIRST verb-bearing segment it sees and +# revises that record only when a later segment resolves a GOVERNED target. Inside a linked worktree no +# segment ever does, so whatever line one carries is what rule 3b is handed -- so any line that LOOKS +# like a git command carrying a gated verb eats the hijack behind it. +# +# WHAT THIS GUARDS IS THE CASE-SENSITIVITY OF THE EMIT IN Remove-QuotedSpans, and that is why it lives +# here rather than beside the false-deny rows in tests/test_worktree_gate_escaped_quote.py. A +# case-INSENSITIVE emit -- tried, and reverted on 2026-08-21 -- gave `cp -r "/c/backups/Git" switch` a +# git token, and a real `git switch ` in somebody else's worktree then came back ALLOW. +# So the case rule is not only a false-deny question: relaxing it opens a fail-OPEN on rule 3b. + + +@pytest.mark.parametrize( + "leaf", + [ + "Git", # the Title-cased spelling a case-insensitive emit would have kept + "Git.exe", # and the `.exe` spelling, which a fix that special-cased `.exe` would still leak + ], +) +@pytest.mark.parametrize( + "shadow_verb", + [ + "switch", # the same verb as the hijack below + "clean", # any gated verb captures the record -- it need not be a hijack verb + ], +) +def test_a_quoted_git_path_on_an_earlier_line_does_not_shadow_a_hijack( + repo: SimpleNamespace, leaf: str, shadow_verb: str +) -> None: + """MEASURED main=DENY, case-insensitive-emit=ALLOW, this build=DENY, over the real hook. + + The two parametrised axes are not thoroughness. ``leaf`` separates a fix that governs every + spelling from one that special-cases ``.exe`` -- the latter closes the first row and leaves the + second ALLOWing, which is exactly the shape a reader would call fixed. ``shadow_verb`` proves the + capture is of the RECORD and not of a matching verb pair. + """ + command = f'cp -r "/c/backups/{leaf}" {shadow_verb}\ngit switch {repo.other}' + reason = assert_denied(run_gate(shell(command, cwd=repo.wt), repo.repos)) + assert "LINKED WORKTREE" in reason, ( + "the deny must be rule 3b judging the hijack on line two, not some other rule objecting to " + f"line one -- otherwise this test would stay green over the shadow. Reason was: {reason}" + ) + + +def test_the_shadow_probe_is_discriminating(repo: SimpleNamespace) -> None: + """CONTROLS for the test above, and it needs three of them. + + Without these the shadow rows would pass against a gate that denied any two-line command, any + command mentioning a directory called Git, or the hijack line on its own regardless of context. + """ + # ONE: the hijack alone denies, so the ALLOW the shadow produced came from the ADDED line. + assert_denied(run_gate(shell(f"git switch {repo.other}", cwd=repo.wt), repo.repos)) + # TWO: a leaf that does not end in a git token never emitted one, so it never shadowed -- and this + # row denies under a case-INSENSITIVE emit too. It separates "the emit" from "an extra line". + assert_denied( + run_gate( + shell(f'cp -r "/c/backups/GitHub" switch\ngit switch {repo.other}', cwd=repo.wt), + repo.repos, + ) + ) + # THREE: the line that used to shadow must not now deny ON ITS OWN. If it did, the rows above + # would be green for the wrong reason -- a false deny standing in for a repaired fail-open. + assert run_gate(shell('cp -r "/c/backups/Git" switch', cwd=repo.wt), repo.repos) is None + + +def test_a_SAME_LINE_semicolon_compound_still_shadows_and_is_NOT_fixed_here( + repo: SimpleNamespace, +) -> None: + """A KNOWN OPEN RESIDUAL, asserted as ALLOW, and NOT an endorsement -- read before acting on it. + + ``Get-ScannableSegments`` splits on NEWLINES only, so a ``;`` compound is ONE segment, and + ``Test-WorktreeHijack`` strips a segment up to its FIRST gated verb. So the shadow survives on one + line, with or without any quoted path:: + + cp -r "/c/backups/Git" switch ; git switch ALLOW + git -C clean -fd NEWLINE git switch ALLOW (no quoting at all) + + Both ALLOW on origin/main as well, so neither is introduced by anything in the emit and neither is + closed by it. This row exists because a shadow test written on ONE LINE would pass vacuously -- + it would be measuring the segment splitter, not the emit -- and the next person to write one needs + to see that stated rather than rediscover it. + + WHEN THIS TEST REDS, somebody fixed rule 3b's first-verb capture. Delete the row; do not restore + the ALLOW. + """ + same_line = f'cp -r "/c/backups/Git" switch ; git switch {repo.other}' + assert run_gate(shell(same_line, cwd=repo.wt), repo.repos) is None, ( + "the same-line shadow now DENIES. If you fixed rule 3b's first-verb capture deliberately, " + "that is the intended outcome -- delete this test. Do NOT restore the ALLOW." + ) + # The quoting-free twin, which proves the residual is the SEGMENT SPLIT and the first-verb capture + # rather than anything Remove-QuotedSpans does. + unquoted = f"git -C {repo.primary.parent} clean -fd\ngit switch {repo.other}" + assert run_gate(shell(unquoted, cwd=repo.wt), repo.repos) is None diff --git a/tests/test_worktree_gate_interpreter_flags.py b/tests/test_worktree_gate_interpreter_flags.py index 6bdc99b9..0914cb6f 100644 --- a/tests/test_worktree_gate_interpreter_flags.py +++ b/tests/test_worktree_gate_interpreter_flags.py @@ -313,3 +313,132 @@ def test_a_multi_line_interpreter_argument_still_denies( "fixed" against a green nobody could have seen fail.""" command = f'pwsh -NoProfile {flag} "\n{PAYLOAD}\n"' assert_denied(run_gate(shell(command, cwd=primary), repos_file)) + + +# ------------------------------------- the flag shape is a QUESTION, not the answer (BACKLOG #1229) +# +# Everything above establishes which flag SPELLINGS an interpreter accepts. It does not establish that +# the program in front of the flag is an interpreter, and ``$shFlag`` is ``-[a-z]*c`` under (?i) -- so +# it matches ``-C``, ``-ic``, ``-rc``, ``-static``, ``-sync`` and ``-exec``, while ``$cmdExeFlag`` +# walks an ordinary POSIX path one component at a time. Enumerated over a hand-built axis: 28 of 33 +# NON-interpreter invocations matched, and 18 of 36 real interpreter invocations did not. The flag +# shape is close to uncorrelated with executing an argument. +# +# The consequence was that an ordinary search of a log file was scanned as code and DENIED. Whether +# each program below actually executes its argument was settled by driving the real binary with a +# payload that COMPUTES (``expr 111 \* 3`` -> 333, so an echo-back cannot be mistaken for a run): +# every row here left no marker, while ``bash -c``, ``sh -c`` and ``python -c`` all printed 333. +# +# WHAT THIS DOES NOT CLOSE, stated so no stronger claim is inferred: the 18 interpreter spellings the +# matcher never reaches -- ``perl -e``, ``node -e``, ``ruby -e``, ``awk``, ``eval``, ``ssh host CMD`` +# -- are unaffected. They were ALLOW before and are ALLOW now. ``Get-FlagOwner`` is asked only about +# flags the matcher already found. + + +@pytest.mark.parametrize( + "command", + [ + # THE ROW THAT MOTIVATES THIS: an ordinary search of a shell history for a dangerous command. + "grep -c 'git reset --hard' /c/logs/history.log", + "grep -ic '{payload}' notes.txt", + "grep -rc '{payload}' .", + "rg -c '{payload}' .", + "ag -c '{payload}' .", + # -c on programs where it means count, check, stdout, bytes, characters, create, compare... + "sort -c '{payload}'", + "uniq -c '{payload}'", + "wc -c '{payload}'", + "cut -c '{payload}'", + "head -c '{payload}'", + "tail -c '{payload}'", + "ls -c '{payload}'", + "tar -c '{payload}'", + "gzip -c '{payload}'", + "md5sum -c '{payload}'", + "cmp -c '{payload}'", + "diff -c '{payload}'", + "rsync -c '{payload}'", + # A long option that merely ENDS in c, which `-[a-z]*c` also reaches. + "gcc -static '{payload}'", + # curl -c takes a cookie-jar FILENAME. + "curl -c '{payload}' https://example.invalid", + # The family-flag path arguments, already pinned above as must-allow, now for a payload that + # DOES carry a git token and a gated verb -- which is what makes these rows non-vacuous. + "make -C '{payload}'", + "git -C '{payload}'", + ], +) +def test_a_program_that_does_not_EXECUTE_its_argument_is_not_recursed_into( + primary: Path, repos_file: Path, command: str +) -> None: + """The false-deny half, with the discriminating control inline on every row.""" + payload = f"git -C {primary} reset --hard" + assert run_gate(shell(command.format(payload=payload), cwd=primary), repos_file) is None, ( + "this program does not run its argument, so its argument is data and denying it stops " + "legitimate work (BACKLOG #1229 residual, fourth round)" + ) + # THE DISCRIMINATING CONTROL, varying the PROGRAM alone: an interpreter handed the identical + # string must still deny. Without it a gate that had stopped recursing entirely -- which is a + # total bypass of rules 3, 3b, 3c and 3d through `bash -c` -- would pass every row above. + assert_denied(run_gate(shell(f"bash -c '{payload}'", cwd=primary), repos_file)) + + +@pytest.mark.parametrize( + "invocation", + [ + # A NON-INTERPRETER WORD SITS BETWEEN the interpreter and its flag in every row here, which is + # exactly what an adjacency test gets wrong. The scan goes leftward, skipping options and + # switch components, bounded by the last command separator. + "su someone -c", + "flock /tmp/l -c", + "script -c", + "timeout 5 bash -c", + "env FOO=1 bash -c", + "docker run --rm img sh -c", + "/usr/bin/bash -c", + "bash.exe -c", + # `find -exec` really executes, and `-exec` ends in `c`, so it was caught by accident before. + # Dropping `find` from the recursing set was measured to regress this from DENY to ALLOW. + "find . -name x -exec", + ], +) +def test_an_interpreter_behind_a_wrapper_word_is_still_recursed_into( + primary: Path, repos_file: Path, invocation: str +) -> None: + """The fail-open half. Each row really runs the quoted string.""" + payload = f"git -C {primary} reset --hard" + suffix = " \\;" if invocation.endswith("-exec") else "" + assert_denied(run_gate(shell(f"{invocation} '{payload}'{suffix}", cwd=primary), repos_file)) + # THE CONTROL: the same shape with a program that does NOT execute its argument must allow, or + # this test would be green against a gate that recursed into everything -- the state it replaces. + assert run_gate(shell(f"grep -c '{payload}' f.txt", cwd=primary), repos_file) is None + + +def test_an_UNKNOWN_program_with_an_interpreter_flag_is_a_known_open_residual( + primary: Path, repos_file: Path +) -> None: + """A TRIPWIRE OVER THE COST OF THIS CHANGE. It asserts ALLOW and that is NOT an endorsement. + + ``Get-FlagOwner`` decides by an ALLOWLIST of program names, because there is no syntactic property + separating ``cp`` from ``sudo`` or ``ls /usr/src/c`` from ``cmd /usr/src/c``. A program it does not + know gets no recursion:: + + myrunner -c '' ALLOW (origin/main: DENY) + + If ``myrunner`` really is an interpreter that is a fail-open. It is the same class as the 18 + interpreter spellings the flag matcher already misses -- ``perl -e``, ``node -e``, ``eval`` -- and + the previous catch was an accident of the flag shape rather than a decision, but it is still a + deliberate DENY-to-ALLOW move against origin/main and it is recorded as one. It also covers the + fifth counterexample the gate's ``$cmdExeFlag`` note lists: a renamed or aliased copy of cmd.exe. + + WHEN THIS TEST REDS, that is the success signal: somebody added the name, or replaced the + allowlist. Delete the row; do not restore the ALLOW. + """ + payload = f"git -C {primary} reset --hard" + assert run_gate(shell(f"myrunner -c '{payload}'", cwd=primary), repos_file) is None, ( + "myrunner -c now DENIES. If you widened the interpreter vocabulary deliberately, that is the " + "intended outcome -- delete this test and the residual note in Get-ScannableSegments. Do NOT " + "restore the ALLOW to make this pass." + ) + # The control that keeps this tripwire attached to the VOCABULARY and not to the whole mechanism. + assert_denied(run_gate(shell(f"sh -c '{payload}'", cwd=primary), repos_file)) diff --git a/tests/test_worktree_gate_interpreter_sigils.py b/tests/test_worktree_gate_interpreter_sigils.py index 81a09eb7..6025d695 100644 --- a/tests/test_worktree_gate_interpreter_sigils.py +++ b/tests/test_worktree_gate_interpreter_sigils.py @@ -493,95 +493,69 @@ def test_the_double_slash_sigil_is_a_known_open_residual( ) -def test_the_cmd_switch_cluster_prefix_costs_a_posix_path_false_deny( +def test_the_cmd_switch_cluster_prefix_no_longer_costs_a_posix_path_false_deny( primary: Path, repos_file: Path ) -> None: - """A DISCLOSED COST OF THIS CHANGE, pinned so it cannot be lost, and NOT an endorsement. - - THE CAUSE IS THE CLUSTER PREFIX, NOT THE SEPARATOR. This test was first written as - ``test_the_relaxed_cmd_separator_costs_...`` and that name, and the four places that repeated it, - were WRONG -- caught by an adversarial re-read of the commit that introduced them. ``$cmdExeFlag`` - is ``(?:/[^/\\s]+)*/[ck]``. The leading ``(?:/[^/\\s]+)*`` was added so cmd's CONCATENATED switch - runs (``/Q/C``, ``/V:ON/C``) are recognised, and it is that prefix which lets an ordinary POSIX - path walk into the matcher: ``/usr`` then ``/src`` then ``/c``. Rebuilt the pattern and measured - the row directly:: - - cluster prefix + \\s* -> MATCH, captures `git checkout main` - cluster prefix + \\s+ -> MATCH, captures `git checkout main` <-- separator irrelevant - no cluster + \\s* -> no match - no cluster + \\s+ -> no match - - The line has a literal space before the quote, so the ``\\s*`` relaxation was never involved. It - was blamed because it was the newest thing nearby -- the same mistake as the separator claim this - lane corrected one commit earlier, made while correcting it. - - A POSIX path whose last component is ``c`` matches that shape, so a quoted span after it is - recursed into as if it were cmd's command argument. Measured against both gates:: - - ls /usr/src/c "some file.txt" main ALLOW this build ALLOW (no git token: vacuous) - ls /usr/src/c "git checkout main" main ALLOW this build DENY <-- the new false deny - ls /usr/src/lib "git checkout main" main ALLOW this build ALLOW (control: the `/c` ending - is the trigger, not the path) - - THIS TEST ASSERTS THE DENY, which is a deliberate and uncomfortable choice, so read why. The row it - replaced asserted ALLOW on a payload with no git token -- green under any matcher, including one - widened to match everything. A guard that cannot fail is not a guard, and this file's own - neighbouring docstring congratulates itself for removing exactly that defect elsewhere. - - So the choice was between deleting the row (losing the record) and pinning the real behaviour. It is - pinned. A false DENY is a cost, not a hole: it stops legitimate work rather than admitting illegitimate - work, and the rule-3 direction that matters -- DENY becoming ALLOW -- is unaffected. If someone later - narrows ``$cmdExeFlag`` so this ALLOWs again, this test reds, and that is the intended signal to come - and read this docstring rather than a regression to paper over. - - DECIDED, AND THE DECISION REVERSED THE PREMISE. This docstring previously said the narrowing that - works is "require a cmd-like PROGRAM token before the switch run". That was MEASURED FALSE and is - corrected here, because it is the sentence a future reader would act on. - - THE CLUSTER PREFIX IS NOT THE SLOPPY PART. cmd.exe itself accepts arbitrary ``/junk`` components - in a switch run and then executes the quoted payload. Driven against the real binary with a - payload that COMPUTES its answer (``set /a 111*3`` -> 333, so an echo-back cannot be mistaken for - a run), with controls in the same batch -- ``cmd /c`` must run, ``cmdd /c`` must not:: - - cmd /usr/src/c "" RUNS cmd /zzz/c "" RUNS (z is no switch) + """THIS TEST USED TO ASSERT THE DENY. It now asserts the ALLOW, and that inversion is the point. + + It was named ``test_the_cmd_switch_cluster_prefix_COSTS_a_posix_path_false_deny`` and its docstring + said: "If someone later narrows ``$cmdExeFlag`` so this ALLOWs again, this test reds, and that is + the intended signal to come and read this docstring rather than a regression to paper over." The + tripwire fired. This is what it was pointing at. + + WHAT THE OLD DOCSTRING GOT RIGHT, and it is most of it. ``$cmdExeFlag`` is + ``(?:/[^/\\s]+)*/[ck]``, and the leading cluster prefix is NOT an over-match -- cmd.exe really does + accept arbitrary ``/junk`` components in a switch run and then execute the quoted payload:: + + cmd /usr/src/c "" RUNS cmd /zzz/c "" RUNS cmd /mnt/c "" RUNS cmd /usr/lib/k "" RUNS - cmd /a:/c "" RUNS cmd /d /usr/src/c "

" RUNS - - ``/usr`` binds as ``/U``, ``/src`` as ``/S``, ``/zzz`` is ignored. So ``(?:/[^/\\s]+)*/[ck]`` is - very nearly EXACTLY the family cmd accepts, and dropping it would lose real coverage rather than - trim an over-match. - - SO THIS IS A PROGRAM-IDENTITY PROBLEM, and it is not solvable at this layer. The only thing - separating ``ls /usr/src/c "..."`` from ``cmd /usr/src/c "..."`` is the program token -- but every - program-token spelling tried was defeated by something that EXECUTES: ``echo hi;cmd /k`` and - ``(cmd /mnt/c`` (neither ``;`` nor ``(`` is whitespace, and the outer ``(?:^|\\s)`` anchor sits - before the whole alternation), an alias, a renamed copy of cmd.exe, and ``cmd /d /Q/C`` where the - program is not adjacent to the switch run. Five candidates were built and driven as real gate - mutants; each traded this one disclosed false deny for four or more measured DENY-to-ALLOW - regressions on shapes that run. - - The false deny is therefore KEPT. At this rule's threat model the two directions are not - symmetric: a false DENY stops legitimate work loudly and has a workaround, while a false ALLOW - lets a reset land in the shared primary silently. - - AND ONE NON-REMEDY, RECORDED SO NOBODY REACHES FOR IT: dropping the ``\\s*`` relaxation does NOT - remove this false deny (see the table above -- it matches under ``\\s+`` too) and it REGRESSES - ``cmd /c"git checkout main"`` from DENY to ALLOW, because the attached form is exactly what - ``\\s*`` exists to catch. That advice was printed here in the first version of this docstring. It - would have cost a real DENY-to-ALLOW and bought nothing.""" - reason = assert_denied( - run_gate(shell('ls /usr/src/c "git checkout main"', cwd=primary), repos_file) - ) - # ATTRIBUTE the deny, do not merely count it. Asserting "something denied" would keep this - # tripwire green if some future unrelated rule denied the same string, and it would then hold - # green straight through a $cmdExeFlag narrowing -- the tripwire silently measuring nothing. - # Today the deny is fully attributable: removing the cluster prefix ALLOWs this outright. - assert "checkout" in reason, ( - "the POSIX-path false deny is still a deny, but no longer for the gated verb -- so this " - f"tripwire is no longer measuring the cluster-prefix path. Reason was: {reason}" + + So the flag pattern was never the thing to narrow, and dropping the cluster prefix would have lost + real coverage. That analysis stands; the rows above are still asserted below. + + WHAT IT GOT WRONG WAS ONE SENTENCE: "SO THIS IS A PROGRAM-IDENTITY PROBLEM, and it is not solvable + at this layer." The first clause is right. The second was inferred from five candidate mutants, + each defeated by something that EXECUTES -- ``echo hi;cmd /k``, ``(cmd /mnt/c``, an alias, a + renamed copy of cmd.exe, and ``cmd /d /Q/C`` where the program is not adjacent to the switch run. + + FOUR OF THOSE FIVE BREAK ADJACENCY, NOT IDENTITY. Every one of those candidates asked "is the token + IMMEDIATELY LEFT of the switch run a cmd spelling", and every counterexample simply put something + in between. ``Get-FlagOwner`` scans LEFTWARD, skipping options and switch components, bounded by + the last command separator -- a different instrument -- and all four survive it, measured on the + real hook and asserted below. The fifth, a renamed or aliased cmd.exe, is NOT closed and is not + claimed: an unknown program name gets no recursion, which is the same disclosed weakening the gate + records for ``myrunner -c ''``. + + THE DIRECTION ASYMMETRY THE OLD DOCSTRING ENDED ON STILL HOLDS -- a false DENY stops legitimate + work loudly and has a workaround, a false ALLOW lets a reset land in the shared primary silently. + It is not what changed. What changed is that this row stopped costing anything to fix. + """ + # THE CLOSURE. `ls` is not an interpreter, so its quoted argument is no longer scanned as code. + assert run_gate(shell('ls /usr/src/c "git checkout main"', cwd=primary), repos_file) is None, ( + "the POSIX-path false deny is back. If you widened the recursion deliberately, that is the " + "intended outcome -- rewrite this docstring. Do NOT restore the DENY to make this pass." ) - # The control: the same payload behind a path NOT ending in `c` must still ALLOW, or the cause is - # something wider than the cmd branch and this test is measuring the wrong thing. + # THE OTHER ARM, and without it the assertion above would be satisfied by a gate that had simply + # stopped recursing at all. Every row here was measured to RUN against the real cmd.exe with a + # payload that COMPUTES its answer (`set /a 111*3` -> 333, so an echo-back cannot be mistaken for + # a run). The last three are the adjacency counterexamples that defeated the five earlier + # candidates; they are the reason the scan is leftward and bounded rather than adjacent. + gated = f"git -C {primary} checkout main" + for still_denies in ( + f"cmd /c '{gated}'", + f"cmd /usr/src/c '{gated}'", + f"cmd /zzz/c '{gated}'", + f"cmd /mnt/c '{gated}'", + f"cmd /usr/lib/k '{gated}'", + f"cmd /d /Q/C '{gated}'", + f"echo hi;cmd /k '{gated}'", + f"(cmd /mnt/c '{gated}'", + ): + reason = assert_denied(run_gate(shell(still_denies, cwd=primary), repos_file)) + # ATTRIBUTE the deny, do not merely count it: a deny for some other reason would keep this + # arm green straight through a recursion that had stopped working. + assert "checkout" in reason, f"{still_denies} denied, but not for the gated verb: {reason}" + # The original control, unchanged: a path NOT ending in `c` was never the trigger. assert run_gate(shell('ls /usr/src/lib "git checkout main"', cwd=primary), repos_file) is None diff --git a/tests/test_worktree_gate_rule_agnostic_coverage.py b/tests/test_worktree_gate_rule_agnostic_coverage.py new file mode 100644 index 00000000..b46dcca6 --- /dev/null +++ b/tests/test_worktree_gate_rule_agnostic_coverage.py @@ -0,0 +1,261 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The escape-blind span scan is RULE-AGNOSTIC, so its coverage must be too. + +WHAT THE DEFECT WAS is stated once, in `Remove-QuotedSpans` in the hook itself, and pinned by +`tests/test_worktree_gate_escaped_quote.py`. This suite does not restate it. It exists for a +property that suite cannot show: the scan blanks text BEFORE any rule is dispatched, so it disarms +whichever rule WOULD have judged that text. The sibling suite parametrises two verbs, and both reach +the SAME rule -- the one guarding the primary working tree. + +WHY A SINGLE-RULE SUITE OVER A RULE-AGNOSTIC DEFECT IS THE SHAPE TO DISTRUST. The class survived in +this codebase behind fourteen green gate suites, none of which could see it. A green suite is +evidence only about the classes it can SEE. Pinning one rule against a defect that disarms all of +them rebuilds that exact condition one layer up: a later change that repairs one rule's own matching +while leaving the scan blind would keep every row green. + +SO THIS ASSERTS THE RULE IDS THE GATE ITSELF RECORDED, not merely that a deny happened. "The command +was DENIED" and "the command was denied BY THE RULE WE EXPECTED" are different claims, and only the +second catches a fix that denies for an accidental reason -- a suite checking outcomes alone passes +when the right answer arrives by the wrong route. The ids come from the gate's own receipt log beside +the allowlist rather than from the deny prose, which is a message to a human and is rewritten +whenever a remediation changes. + +FOUR RULES, MEASURED RATHER THAN ASSUMED: against the pre-fix hook on a real governed repository one +identical escape wrapper turned DENY into ALLOW on the primary working tree, the linked-worktree +hijack, the shared git configuration and worktree removal. + +DELIBERATELY NOT DUPLICATED HERE, so a reader does not mistake the omissions for gaps: the +ordinary-quoted-commit-message and unterminated-quote boundaries live in +`tests/test_worktree_gate_quote_straddle.py`, and the escape-inside-a-real-span control and the +program-path spellings live in `tests/test_worktree_gate_escaped_quote.py`. Third copies would add no +information. + +A NOTE ON WHAT IS **NOT** AN ARM HERE, because it was reported as one and is not. `git worktree add` +allows with AND without the escape -- two seats measured that independently, on separate trees -- so +it says nothing about escape handling. Whether it should be governed at all is a real and separate +question. A case built on that false premise would have pressured its next reader to widen a security +gate until a test went green. +""" + +from __future__ import annotations + +import re +import shutil +import subprocess +from pathlib import Path +from types import SimpleNamespace +from typing import Any + +import pytest + +from tests.test_worktree_gate import assert_denied, run_gate # reuse the subprocess harness + +pytestmark = pytest.mark.skipif( + shutil.which("pwsh") is None or shutil.which("git") is None, + reason="needs pwsh (PowerShell 7) and git on PATH", +) + +# Built from character codes rather than written inline, matching the sibling suites: a test about +# escaping must not depend on how this file's own literals escape. This is not hypothetical -- one +# draft of the FIX was written through a shell heredoc and arrived with the backslash silently +# removed, leaving a line that still parsed and asserted nothing. +BS = chr(92) +DQ = '"' + + +def git(*args: str, cwd: Path | None = None) -> None: + subprocess.run( + ["git", *args], cwd=str(cwd) if cwd else None, check=True, capture_output=True, text=True + ) + + +@pytest.fixture +def repo(tmp_path: Path) -> SimpleNamespace: + """A real governed primary plus a linked worktree. + + The sibling suites string-match paths and need no real repo. Three of the four rules here ask git + itself what it is looking at, so the multi-rule coverage this suite exists to assert is reachable + only against a real one -- which is also why a synthetic fixture reported one of these arms as a + fail-open when it was simply unreachable. + """ + primary = tmp_path / "Primary" + git("init", "-b", "main", str(primary)) + git("config", "user.email", "t@example.com", cwd=primary) + git("config", "user.name", "t", cwd=primary) + (primary / "seed.txt").write_text("seed\n", encoding="utf-8") + git("add", "-A", cwd=primary) + git("commit", "-m", "seed", cwd=primary) + # A branch that exists and is checked out NOWHERE -- the grabbable one the hijack rule guards. + git("branch", "claude/other-branch", cwd=primary) + wt = tmp_path / "Primary-wt" + git("worktree", "add", "-b", "wt-branch", str(wt), cwd=primary) + repos = tmp_path / "repos.txt" + repos.write_text(f"{primary}\n", encoding="utf-8") + return SimpleNamespace(primary=primary, wt=wt, repos=repos, log=tmp_path / "worktree-gate.log") + + +def shell(command: str, cwd: Path | str) -> dict[str, Any]: + """A Bash tool payload, matching the sibling suites' harness.""" + return { + "session_id": "s-1", + "cwd": str(cwd), + "hook_event_name": "PreToolUse", + "tool_name": "Bash", + "tool_input": {"command": command}, + } + + +def straddle(inner: str) -> str: + """Wrap a command in two backslash-escaped quotes: two shell literals, live command between.""" + return f"echo {BS}{DQ} ; {inner} ; echo {BS}{DQ}" + + +def rules_logged(repo: SimpleNamespace) -> list[str]: + """The rule ids the gate recorded, read from its own receipt log beside the allowlist.""" + if not repo.log.exists(): + return [] + text = repo.log.read_text(encoding="utf-8", errors="replace") + return [m.group(1) for m in re.finditer(r"\trule=(\S+)\t", text)] + + +# Each arm names the rule it must reach, so the coverage assertion has something to compare against +# that does not drift when a parametrisation is edited. +ARMS: list[tuple[str, str]] = [ + ("3", "primary working tree"), + ("3b", "linked worktree hijack"), + ("3c", "shared git configuration"), + ("3d", "worktree removal"), +] + + +def arm_command(rule: str, repo: SimpleNamespace) -> tuple[str, Path]: + """The gated command for an arm, plus the cwd it must be issued from. + + The cwd is part of the arm and not incidental: standing in the linked worktree is what separates + the hijack rule from the primary-tree rule for the very same verb. + """ + if rule == "3": + return f"git -C {repo.primary} reset --hard", repo.primary + if rule == "3b": + return "git checkout claude/other-branch", repo.wt + if rule == "3c": + return f"git -C {repo.primary} config core.hooksPath /dev/null", repo.primary + if rule == "3d": + return f"git -C {repo.primary} worktree remove {repo.wt}", repo.primary + raise AssertionError(f"unknown arm {rule!r}") + + +@pytest.mark.parametrize("rule,what", ARMS, ids=[r for r, _ in ARMS]) +def test_an_escaped_quote_does_not_hide_a_gated_command( + repo: SimpleNamespace, rule: str, what: str +) -> None: + """One offender arm per rule, each with its own positive control run first. + + The control asserts the bare command is gated in this fixture AT ALL. Without it a row could go + green because the command never reached a rule, and an arm that cannot fail is not evidence -- + which is precisely how a synthetic fixture once made an ungoverned verb look like a fail-open. + """ + command, cwd = arm_command(rule, repo) + control = run_gate(shell(command, cwd=cwd), repo.repos) + assert control is not None, ( + f"positive control failed: the bare {what} command is not gated in this fixture, so the " + "escaped arm below would prove nothing" + ) + assert_denied(control) + assert_denied(run_gate(shell(straddle(command), cwd=cwd), repo.repos)) + + +def test_the_escape_arms_cover_more_than_one_rule(repo: SimpleNamespace) -> None: + """THE POINT OF THIS FILE. The defect is rule-agnostic, so one-rule coverage is not evidence. + + An assertion rather than a comment, because the failure it guards against is SILENT: an edit that + narrows the parametrisation, or a fixture change that quietly stops one arm reaching its rule, + leaves every other row in this file green and says nothing at all. + """ + for rule, _ in ARMS: + command, cwd = arm_command(rule, repo) + assert_denied(run_gate(shell(straddle(command), cwd=cwd), repo.repos)) + seen = set(rules_logged(repo)) + assert seen >= {r for r, _ in ARMS}, ( + "the escaped straddle must be shown to disarm EVERY listed rule, not just the first. " + f"expected at least {sorted(r for r, _ in ARMS)}, the gate recorded {sorted(seen)}" + ) + + +def counterpart_command(rule: str, repo: SimpleNamespace) -> tuple[str, Path]: + """An ORDINARY use of the same rule's subject, which must be allowed. Returns (command, cwd). + + One per rule, deliberately close to that rule's offender: the same verb family or the same + subject, differing only in being legitimate. + """ + if rule == "3": + return f'git -C {repo.primary} commit -m "chore: clean up dead code"', repo.primary + if rule == "3b": + # Creating a NEW branch is not a hijack -- rule 3b denies only a switch onto an EXISTING one. + return "git checkout -b claude/brand-new-branch", repo.wt + if rule == "3c": + return f"git -C {repo.primary} config user.name someone", repo.primary + if rule == "3d": + return f"git -C {repo.primary} worktree list", repo.primary + raise AssertionError(f"unknown arm {rule!r}") + + +@pytest.mark.parametrize("rule,what", ARMS, ids=[r for r, _ in ARMS]) +def test_the_counterpart_for_each_rule_must_not_fire( + repo: SimpleNamespace, rule: str, what: str +) -> None: + """THE HARDENED COUNTERPART, one per rule. Without it this file cannot fail in one direction. + + MEASURED, AGAINST THIS FILE, WHICH IS WHY IT IS HERE AND NOT DELEGATED. An earlier draft dropped + its must-ALLOW rows on the grounds that equivalents live in the sibling suites, reasoning that a + load-bearing fact should be stated once. That is right for PROSE and wrong for a TEST: a suite + only discriminates through the assertions it actually runs. Mutation-tested standalone, the + draft stayed 7-of-7 GREEN against a gate mutated to deny every verb against a governed tree -- + it could see a gate that had stopped denying and not one that had started denying everything. + + The offender rows above and these rows together are what make a verdict here mean something. A + fix that widened the gate until the offenders passed would redden these instead. + """ + command, cwd = counterpart_command(rule, repo) + result = run_gate(shell(command, cwd=cwd), repo.repos) + assert result is None, ( + f"an ORDINARY {what} command was denied. The gate has been widened rather than corrected, " + f"which passes the offender rows above for the wrong reason.\nCommand: {command}\n" + f"Deny object:\n{result}" + ) + + +def test_an_ungoverned_repo_is_untouched(tmp_path: Path) -> None: + """Anti-vacuity on the other axis: the deny must come from GOVERNANCE, not from the backslash. + + The identical escaped shape against a repo that is not on the allowlist must still ALLOW. Without + it, a scan that simply refused anything containing an escaped quote would pass every row above + while denying ordinary work in every session on the box -- the fail-closed direction is safe for + the tree and still wrong. + """ + other = tmp_path / "Ungoverned" + git("init", "-b", "main", str(other)) + repos = tmp_path / "repos-elsewhere.txt" + repos.write_text(f"{tmp_path / 'NoSuchGovernedCheckout'}\n", encoding="utf-8") + command = straddle(f"git -C {other} reset --hard") + assert run_gate(shell(command, cwd=other), repos) is None, ( + "an ungoverned repo was denied -- the scan is now firing on the escape rather than on the " + "target, which is a false-deny surface across every session on the box" + ) + + +def test_the_receipt_reader_can_observe_a_rule_id(repo: SimpleNamespace) -> None: + """The positive control for `rules_logged`, which the coverage assertion rests on. + + A reader that returns nothing makes that assertion fail loudly. A reader whose PATTERN has + drifted returns a stale or partial set and fails it QUIETLY in the other direction -- or, worse, + satisfies it by accident. Assert the instrument sees a known-good deny before anything trusts + what it does not see. + """ + assert not rules_logged(repo), "the receipt log must start empty, or the reading below is stale" + assert_denied(run_gate(shell(f"git -C {repo.primary} reset --hard", repo.primary), repo.repos)) + assert rules_logged(repo) == ["3"], ( + "the receipt log did not record the rule for a deny the gate definitely made -- the reader " + f"is broken, not the gate. Saw: {rules_logged(repo)!r}" + ) diff --git a/tests/tooling_manifest.txt b/tests/tooling_manifest.txt index 301d3afb..20d14b1f 100644 --- a/tests/tooling_manifest.txt +++ b/tests/tooling_manifest.txt @@ -107,6 +107,7 @@ tests/test_worktree_gate_command_parsing.py tests/test_worktree_gate_control_plane.py tests/test_worktree_gate_default_reposfile.py tests/test_worktree_gate_emitter.py +tests/test_worktree_gate_escaped_quote.py tests/test_worktree_gate_git.py tests/test_worktree_gate_hijack.py tests/test_worktree_gate_interpreter_flags.py @@ -114,6 +115,7 @@ tests/test_worktree_gate_interpreter_sigils.py tests/test_worktree_gate_quote_straddle.py tests/test_worktree_gate_receipts.py tests/test_worktree_gate_remedy_families.py +tests/test_worktree_gate_rule_agnostic_coverage.py tests/test_worktree_gate_shell_semantics.py tests/test_worktree_gate.py tests/test_worktree_new_cleanup_advice.py