diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 4e90e4dc..d59b3d45 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -4204,6 +4204,7 @@ Retiring the tree costs the engine nothing operationally: **`tests/test_ech_egre ## 1020. The first-run bootstrap Administrator is created with no email address, and the PHI notification gate cannot see it +> **AMENDED 2026-08-20 -- THE HOLD CONDITION THIS ITEM NAMES HAS BEEN SATISFIED, AND THE ITEM STILL DOES NOT CLOSE. THE REASON CHANGED; THE VERDICT DID NOT.** The rulings below say *"do not close this before `#1257` lands"*. **`#1257` HAS LANDED** -- `tests/test_lifespan_startup_unwinds.py` is present on `origin/main` and `api/app.py` carries the unwind around `engine.start()`. Content-tested against `origin/main`, not read from `#1257`'s banner, which still reads open. **So anyone watching that banner flip would close this item for a reason that was never the real one.** **THE LIVE REASON, measured on `origin/main` the same day:** `__main__.py:2350-2354` still computes `security_channel_ready` as `notify_security_events AND email_smtp_host AND email_from` -- **SMTP wiring alone.** It asks *"is a transport configured"*, never *"can the account holding `frozenset(Permission)` receive"*, which is this item's second sentence verbatim. The landing branch adds the deliverable-address predicate in `auth/service.py` and **never wires it in**: `__main__.py` is not in its changed-file list and the whole file hashes identically on both refs. **A reason that can expire silently is a worse reason even while it is still true** -- this one is checkable in one command. Recorded by the lander under ADR 0165. > ๐Ÿ”ข **Filed 2026-08-04 โ€” not started.** Value **5/10** ยท Difficulty **3/10** ยท _fill-in_. `_ensure_bootstrap_admin` calls `create_user` with no `email=`, so the account holding `frozenset(Permission)` has a NULL email and `SecurityEventNotifier.notify`'s `if not event.email: return` makes all ten notice types no-op for it. The PHI startup gate that refuses to serve without a notification channel computes readiness from the SMTP transport alone, so it would report a healthy channel while no notice about the all-permission account could be delivered. > โš ๏ธ **AMENDED 2026-08-11 โ€” the bootstrap half shipped on a branch; THE PART WITH TEETH IS UNTOUCHED, so this stays OPEN.** `w3-l2-auth-policy` (`a46f7a83`, pushed, **unlanded**) retires the emailless bootstrap admin. But the item's second sentence โ€” the PHI startup gate reporting a healthy channel while no notice about the all-permission account could be delivered โ€” **is not addressed**. Re-read on `main` rather than relayed: `messagefoundry/__main__.py:2292-2296` still computes `security_channel_ready` as `notify_security_events and email_smtp_host and email_from`, which is **SMTP wiring alone**. It asks *"is a transport configured"*, never *"can the account that matters actually receive"* โ€” SDS-3.8, the instrument answering the adjacent question. A deploying site would get a green readiness signal for a channel that cannot reach the one account holding `frozenset(Permission)`. @@ -5995,7 +5996,8 @@ rules out the family `#1014` already fixed and points at the one it did not. ## 1105. `harden_kex_groups`' docstring undercounts its own call sites, in the paragraph written to warn about exactly that -> ๐Ÿ”ข **Filed 2026-08-08 - not started. Measured on `main` at 166634c9, not hypothesised.** Value **3/10** ยท Difficulty **1/10**. `messagefoundry/config/tls_policy.py:125` says `APPROVED_KEX_GROUPS` reaches "zero of this function's **six** call sites"; `:136` repeats "a call at **six** sites with zero effect". Scanning `messagefoundry/`, `tests/`, `harness/`, `packaging/` and `ide/` for `harden_kex_groups(` finds **seven** sites that build and harden a real TLS context, plus an eighth reference added by `#338`. Nothing checks the number - the tests derive their site list instead - so the docstring is the only place it is asserted, and it is wrong. +> โœ… **SHIPPED 2026-08-20 -- the count was DELETED, not corrected, and that is the fix.** The docstring no longer states a number: `APPROVED_KEX_GROUPS` reaches **none** of the call sites, and the paragraph now says why no count is stated -- the site list is *derived* by `tests/test_tls_policy.py`, so a number written into prose is a second definition that nothing checks. **A cited number carrying no conclusion is a liability; delete it rather than correct it.** Correcting "six" to "seven" would have produced a true sentence measuring the wrong thing, and it would have read as freshly checked forever. **No behaviour change of any kind** -- the separately-recorded fact that `harden_kex_groups` pins nothing on any interpreter this project runs on (`SSLContext.set_groups` is a 3.15 API) is untouched. Fix authored by the builder lane; this banner by the lander under ADR 0165. Original filing follows. +> **Filed 2026-08-08 - not started. Measured on `main` at 166634c9, not hypothesised.** Value **3/10** ยท Difficulty **1/10**. `messagefoundry/config/tls_policy.py:125` says `APPROVED_KEX_GROUPS` reaches "zero of this function's **six** call sites"; `:136` repeats "a call at **six** sites with zero effect". Scanning `messagefoundry/`, `tests/`, `harness/`, `packaging/` and `ide/` for `harden_kex_groups(` finds **seven** sites that build and harden a real TLS context, plus an eighth reference added by `#338`. Nothing checks the number - the tests derive their site list instead - so the docstring is the only place it is asserted, and it is wrong. **Cluster:** Documentation accuracy / security-comment drift. **Priority:** P3. **Verdict:** build. **Severity:** no product effect and no PHI effect, and **no behaviour change of any kind** - the separate, @@ -10126,6 +10128,7 @@ _FHIR_ID_RE.fullmatch("abc\n") -> False the fix **Severity:** no deployment axis (ยง0) -- dev-box test tooling; **CI (Linux) is unaffected and reports nothing**. The cost is that every seat running a full suite on this box may or may not hit a 160-failure wall depending on its own `PATH`, and while it is hit the module cannot detect the defect it exists to catch. ## 1273. the module that exists to write the C0/DEL test once writes it twice, and logging_setup re-derives it a third time +> **PARTIAL 2026-08-20 -- ONE OF THE TWO SPELLINGS INSIDE THE MODULE IS GONE; THE THIRD, IN ANOTHER MODULE, IS NOT. THIS ITEM STAYS OPEN.** `controlchars.py` now states the set once, in `_is_control_char` at `:67`, and both `has_control_char` and `strip_control_chars` read it -- so the module's own interior no longer contradicts its docstring. **But `logging_setup.py:66-70` still re-derives the same set independently** (`for _i in range(0x20)` plus `[0x7F]`), and the landing branch does not touch that file at all. The item names **three** spellings and asks for **one definition of the SET**; two of three now share one. **The leverage the module exists to provide is still absent across the module boundary:** widen `_is_control_char` and it reaches neither `_CTRL_TRANSLATION` nor anything reading it, and nothing reports the omission. Measured on the landing branch, not inferred. Recorded by the lander under ADR 0165, because a closing banner that lists what a change fixed and not what it left is half a record. > ๐Ÿ”ข **Filed 2026-08-15 - not started. THE CONSOLIDATION DOES NOT CONSOLIDATE ITS OWN TWO FUNCTIONS.** [`controlchars.py`](../messagefoundry/controlchars.py) was created by [#1253](BACKLOG.md) to write the C0/DEL test **once**; its docstring is titled *"The C0/DEL control-character test, written once"* and ends *"THE POINT IS THE COPYING PRACTICE, not the seven known lines. If you need this test, import it."* **It then spells the predicate out twice inside itself**, and a third statement of the same set lives in `logging_setup`. > **MEASURED, three independent spellings of one set:** diff --git a/docs/adr/0167-phi-security-notification-readiness-gates-on-a-deliverable-address-checked-early-in-the-asgi-lifespan.md b/docs/adr/0167-phi-security-notification-readiness-gates-on-a-deliverable-address-checked-early-in-the-asgi-lifespan.md new file mode 100644 index 00000000..af21f40e --- /dev/null +++ b/docs/adr/0167-phi-security-notification-readiness-gates-on-a-deliverable-address-checked-early-in-the-asgi-lifespan.md @@ -0,0 +1,220 @@ + + + +# ADR 0167 โ€” PHI security-notification readiness gates on a deliverable address, checked early in the ASGI lifespan + +- **Status:** Proposed (2026-08-15) โ€” the predicate is built (`29a026e2`); the gate that consumes it is not yet written +- **Date:** 2026-08-15 +- **Related:** [BACKLOG #1020](../BACKLOG.md) (the item, owner-ruled 2026-08-13) ยท [BACKLOG #1257](../BACKLOG.md) (a startup refusal after `engine.start()` hangs) ยท [`__main__.py`](../../messagefoundry/__main__.py) (the existing SMTP-only gate) ยท [`api/app.py`](../../messagefoundry/api/app.py) (the lifespan) ยท [`auth/service.py`](../../messagefoundry/auth/service.py) (`has_notifiable_admin`) ยท [DEPLOYMENT.md](../DEPLOYMENT.md) (exit codes) ยท [CLAUDE.md](../../CLAUDE.md) ยง0 (not deployed), ยง11 (SDS-3.8) + +--- + +## Context + +### The gate answers the adjacent question + +On a PHI instance under `enforcement=enforce`, `serve` refuses to start without a security-notification +channel. It computes readiness as: + +```python +security_channel_ready = bool( + settings.auth.notify_security_events + and settings.alerts.email_smtp_host + and settings.alerts.email_from +) +``` + +That is **SMTP wiring alone**. It asks *"is a transport configured"* and never *"can the account that +matters actually receive"* โ€” **SDS-3.8**, the instrument answering a neighbouring question. + +The two come apart on exactly the instance the gate exists to protect. `_ensure_bootstrap_admin` calls +`create_user` with no `email=`, so on a first run the only account โ€” the one holding +`frozenset(Permission)` โ€” has a NULL address, and `SecurityEventNotifier.notify` opens +`if not event.email: return`. **All ten notice types no-op for the most privileged account on the +instance while the gate reports a healthy channel.** + +Per **CLAUDE.md ยง0** this is written in the conditional: there are zero deployments, so nothing is +exposed today. It is wrong in the shipped code, and it is wrong in the direction a first deployment +would not notice. + +### Owner ruling + +**2026-08-13: option (b) โ€” gate startup on a deliverable channel.** The design question was settled +before this ADR. What was not settled, and what this ADR is for, is **where the check runs**. + +--- + +## Decision + +**1. Gate on a deliverable address, scoped to the ROLE.** `AuthService.has_notifiable_admin()` is true +iff at least one **enabled** administrator has an email on file. Not the bootstrap account: `email` is +optional in `UserCreateRequest` and is not required for the Administrator role, so a hand-created +privileged account has the identical hole. Keying on the bootstrap user would close the instance this +was found on and leave the class open. + +**2. Run the check AFTER the bootstrap admin is created โ€” which is AFTER `engine.start()`.** + +> ## โš ๏ธ OVERTURNED 2026-08-15 03:20Z, BY WRITING THE CODE. THE TITLE OF THIS ADR IS NOW WRONG. +> +> **This ADR chose EARLY-LIFESPAN โ€” after `open_store` at `:5540`, before `engine.start()` at +> `:5731` โ€” and that placement is IMPOSSIBLE for this check.** Measured on `api/app.py`, one +> lifespan, in order: +> +> ``` +> 5540 store = await open_store(...) <- the window I chose starts +> 5731 await engine.start() <- the window I chose ends +> 5837 auth = AuthService(...) <- the service does not EXIST until here +> 5852 bootstrap = await auth.initialize() <- CREATES the bootstrap admin +> 5923 yield +> ``` +> +> **Two independent blockers, either one fatal.** In the chosen window there is **no `AuthService`** +> to call `has_notifiable_admin()` on; and on a first run there is **no administrator at all**, +> because `initialize()` is what creates it (`auth/service.py:517`, `_ensure_bootstrap_admin`). A +> check there would **refuse every first run, before the account it is about exists** โ€” turning a +> gate that reports a wrong answer into a gate that prevents startup outright. +> +> **SO THE `#1257` DEPENDENCY IS REAL AND RETURNS.** The check must sit after `:5852`, which is +> inside the post-`engine.start()` window that `#1257` records as hanging. **The Dispatcher's +> original ruling โ€” do not close `#1020` before `#1257` โ€” was right on its own terms all along, and +> my narrowing of it was wrong.** +> +> **THIS VINDICATES THE RIDER'S AUTHOR.** They assumed the lifespan placement and were right, for a +> reason neither the Dispatcher nor I identified while arguing about it: not merely *"the store is +> there"*, but **the data the check needs does not exist until after the engine has started.** +> +> **What does NOT change:** the decision to gate on a deliverable address; the predicate +> (`has_notifiable_admin`, built at `29a026e2`); the scoping to the ROLE; and the exit-code findings, +> which were always about the post-start path. +> +> **This was found by writing the code, not by reading it.** Three seats reasoned about this +> placement across two hours and none of us asked where the bootstrap admin is created โ€” the one +> question the check's own subject makes load-bearing. + +### Placements considered + +| name | placement | has the data? | terminates? | exit code | +|---|---|---|---|---| +| **LIFESPAN (post-bootstrap, after `:5852`)** | after `engine.start()` | **YES โ€” the only placement that does** | hangs on `origin/main`; **fixed on PR #394** | n/a | +| **PREFLIGHT** | `_serve`, before the ASGI app | **no** โ€” no store, no `AuthService` | yes | **2** | +| **EARLY-LIFESPAN** | `:5540`โ€“`:5731` | **NO โ€” no `AuthService`, and on a first run no admin exists yet** | yes, measured | 3 | + +**The "has the data?" column is the one that decides it, and it is the column this ADR originally +did not have.** The first two versions compared placements on store access, termination and exit +code โ€” three real properties, none of which is the binding constraint. **The binding constraint is +that the check's subject does not exist until `:5852`.** + +**LIFESPAN is out on a measurement, not a preference โ€” AND THE MEASUREMENT IS SCOPED TO A REF.** +#1257 records that an exception after `engine.start()` unwinds nothing, so the refusal hangs the +process instead of exiting โ€” **strictly worse than the defect this ADR fixes**, because an operator +can see a wrong readiness answer but cannot see a process that never finishes starting. + +> โš ๏ธ **THAT DISQUALIFICATION IS TRUE OF `origin/main`, NOT OF THE CODEBASE.** **#1257's fix is +> already built on PR #394** โ€” verified by reading the artifact rather than the claim: +> `tests/test_lifespan_startup_unwinds.py` exists at `refs/pull/394/head`, docstring *"BACKLOG +> #1257: a startup failure after `engine.start()` must let the PROCESS exit."* **Once #394 lands, +> LIFESPAN stops being disqualified.** +> +> **This ADR still chooses EARLY-LIFESPAN, and the choice does not depend on the hang:** it avoids +> the post-`engine.start()` window entirely, so it is right whether or not #1257 has landed. What +> changes is only *why the alternative was rejected*. +> +> **Recorded this way deliberately.** A structural-sounding argument about a defect that no longer +> exists is worse than no argument: the next reader finds no hang, concludes the ADR is wrong, and +> distrusts the parts that are still right. **This one was only ever right about a ref.** + +**PREFLIGHT was recommended and withdrawn by its author on measurement.** `_serve` (lines 1042-2833) +runs entirely before the lifespan and never opens a store โ€” zero `open_store` calls across the whole +function. `list_users()` is async. So PREFLIGHT is not "one cheap read": it is **the first store open +in a preflight that has never opened one, driven from sync code**, ahead of whatever `open_store` does +on first touch. That machinery would be bought to preserve an exit code (see below) that the shipped +service wrapper ignores. + +**EARLY-LIFESPAN gets the store as a plain `await`** โ€” no extra open, no sync/async bridge โ€” and its +refusal exits. + +--- + +## The exit code changes, and the divergence is forced + +**Measured** (uvicorn, CPython 3.14.6, minimal ASGI app, probe kept out of tree): + +| arm | result | +|---|---| +| raise early in the lifespan | **exited 0.49s, code 3** โ€” `ERROR: Application startup failed. Exiting.` | +| `sys.exit(2)` early in the lifespan | **exited 0.50s, code 3** โ€” stderr shows `SystemExit: 2`, then the same uvicorn line | +| **positive control**, no raise | reached a **RUNNING** server, self-stopped with a distinct code **99** | + +**The control is what makes the exits mean anything.** A non-exit was observable in the same rig, so +"it exited" was not the only outcome the harness could produce. + +**uvicorn catches `SystemExit`, treats it as a startup failure like any other exception, and exits 3 +regardless of the code requested.** From inside the lifespan there is **no spelling of a refusal that +keeps exit 2**. The divergence is forced, not chosen. + +### What that costs, stated no larger than it is + +`_serve` returns **2** at 32 sites, and `DEPLOYMENT.md` states "(exit 2)" twice โ€” at `:210` for the +PHI/enforce TLS preconditions and at `:564` for the off-loopback bind refusal. **Both citations are +scoped to specific refusals. No line generalises exit 2 to all refusals.** + +So a new refusal exiting 3 is **an inconsistency with two documented specific refusals, not a +contradiction of a published universal claim.** An earlier draft of this reasoning called it a +"documented-contract divergence"; that was stronger than the text supports and is corrected here +rather than quietly dropped. + +**And it must not be claimed that exit 2 gives operators a clean stop today:** +`scripts/service/install-service.ps1:463` sets NSSM `AppExit Default Restart`, which restarts on **any** +exit code. Under the shipped service wrapper the operational delta between 2 and 3 is approximately +nil. **The cost is a reader's surprise, not a broken script.** + +**Accepted, with the cheap honest fix:** `DEPLOYMENT.md` gains a line recording that a **startup-stage** +refusal exits 3, beside the existing exit-2 statements. That converts an undocumented inconsistency +into a documented one for the cost of a sentence, and it is the only part of this decision an operator +will ever see. + +--- + +## Rejected: a sentinel that re-exits 2 + +`uvicorn.run()` does not return on the startup-failure path โ€” it exits the process itself. **If** that +exit is a catchable `SystemExit` at the call site, a caller could catch it and re-exit 2 off a flag set +by the refusal, preserving the exit code with the lifespan placement. + +**Considered and declined. It is untested โ€” that "if" was never measured.** It is a **cross-layer +mechanism** (lifespan sets state, caller intercepts, re-exits) bought to remove an inconsistency that +the NSSM finding above makes nearly free. **The sentinel costs more than the thing it fixes.** + +Recorded rather than omitted, because an unmentioned alternative gets re-derived by the next reader and +a declined one with a reason does not. + +--- + +## Consequences + +- A PHI instance under `enforce` with a configured SMTP transport and **no notifiable administrator** + will refuse to start, where today it starts and reports a healthy channel. +- **That refusal exits 3, not 2**, and is the first refusal in this codebase to do so. +- The check runs on **every** serve of a PHI instance, adding one `list_users()` plus a role lookup per + enabled user at startup only. +- **There are now three independent copies of "who is an enabled administrator"** in `auth/service.py` + (`has_notifiable_admin`, `is_last_enabled_admin`, `_other_enabled_admin_exists`). Their agreement is a + convention with nothing binding it. Extracting a shared enumeration is worth its own item and is + deliberately not done here. + +## What is NOT demonstrated + +**The termination evidence is a minimal repro, not the real gate.** It proves uvicorn's +lifespan-startup-failure path exits; it does **not** prove the real refusal exits with a real store open +and everything `api/app.py` has constructed by `:5540`. + +**BACKLOG #1020's rider asks for the refusal to be demonstrated to terminate under `uvicorn`, and a +rider that exists because someone inferred is not satisfied by an inference.** **This ADR must not be +cited as discharging it.** The real gate carries that obligation when it lands. + +**AND ONE OPEN CHECK AGAINST THE POST-#394 TREE.** The exit-code results above were taken on a +minimal repro, which #394 does not touch โ€” so they stand as measurements of *uvicorn's* behaviour. +**But the REAL gate's behaviour inside a lifespan that now unwinds properly has not been measured by +anyone.** It is plausible that a correctly-unwinding lifespan changes nothing about the exit code, +and plausible is not measured. **Re-run the arms against the post-#394 tree before treating exit 3 as +settled for the shipped gate.** diff --git a/docs/adr/README.md b/docs/adr/README.md index 39fd0d88..ea565d77 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -190,3 +190,4 @@ what is withheld and what you can request. | [0161](0161-async-session-mail-for-unreachable-peers.md) | **Async session mail for unreachable peers** (BACKLOG #1028) โ€” the realtime session channel cannot address two peer classes, **structurally rather than by a filter**: a session launched by the VS Code extension is never entered into the Desktop app's in-memory map of sessions it spawned, and a session under a different login sits behind an independent config root (measured 2026-08-05: one repo, one moment, Desktop sessions on one root and VS Code sessions on a second, neither side visible to the other). The client's compiled peer-to-peer protocol is inert โ€” the registry field carrying a peer's socket address is written by no code path and the lookup **fails silently green**, an empty peer list rather than an error. Decision: an **async file-drop lane** under `/mefor-coord/mail/`, keyed by the recipient's normalised **worktree path** (not session id, which `/clear` re-mints; not worktree name, which is a creation-time label โ€” one worktree was observed on four branches under four sessions in a day), with the key function held in **one definition** dot-sourced by both ends because a drifted second copy fails silently on both sides. Location is the leak control: nothing under `.git` can enter a commit, and `mefor-coord` is not a ref namespace, so `push --mirror` cannot carry it โ€” which is why worktree paths are plain text in the queue deliberately. Explicitly **does not replace** the realtime channel for desktop-to-desktop. **The claim primitive is the load-bearing part, and it was forced by measurement:** `[System.IO.File]::Move` **returns success without moving** for losers under contention โ€” 16 racers x 500 rounds on .NET 10.0.9 / Windows 10.0.26200, instrumented in C# because PowerShell scriptblock closures do not capture loop variables reliably; **every** round had more than one racer return with no exception, in **375 of 500** rounds **all sixteen** did, and the obvious fix โ€” verify `File.Exists(dst) && !File.Exists(src)` โ€” was **true for all 16 racers in all 500 rounds**, because the winner's move makes it true for everybody. A destination unique per claimer yielded exactly one winner in **500 of 500**. Controls confirm the no-op is contention-only (a `Move` of a nonexistent or already-moved source raises). `scripts/coord/claim.ps1` is **not** affected โ€” exclusive `CreateNew` plus a per-PID-unique target โ€” do not "fix" it. Also decided: the **on-disk filename is authoritative and the JSON `id` is discarded** (sanitising it would be a weaker control that looks identical), the drain **never emits a runnable command**, one body sanitiser at the single injection point so a body cannot forge the frame, receiver-side caps (a send-time cap is bypassed by the write that *is* the transport), and a content rule with the force of the secrets rule. Bad consequences are stated rather than softened: the **write-side trust boundary cannot be enforced** by this design (any process running as the user can drop into any inbox, so every `from.*` field is a self-assertion), **delivery duplicates the body into a transcript no prune reaches**, and the urgent `asyncRewake` tier is **one-shot** because the rewake belongs to the process Claude Code spawned and a self-respawned grandchild's exit code is heard by nobody | **Proposed (2026-08-05)** โ€” the code is a **PROTOTYPE and is deliberately NOT WIRED**; the installer rows exist but no config root has been installed from them. Wiring is a separate owner-approved step gated on BACKLOG #1028, and the ten EARS criteria link to functions that exist in `tests/test_session_mail.py`. **Arm B of the claim measurement was later corrected:** it was taken with 16 threads in ONE process, and re-measuring with 16 separate processes showed `File.Exists(own destination)` reporting a win to more than one racer in 46 of 800 rounds, so the shipped verdict is an exclusive open, not an existence check | | [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 | diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 2a0823b3..a1ed3865 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -2908,6 +2908,37 @@ async def is_last_enabled_admin(self, user_id: str) -> bool: admins.add(user.id) return admins == {user_id} + async def has_notifiable_admin(self) -> bool: + """True iff at least one ENABLED administrator has an email address on file. + + BACKLOG #1020. The PHI startup gate computes notification readiness from the SMTP transport + alone (``notify_security_events`` + ``email_smtp_host`` + ``email_from``), which answers + *"is a transport configured"* and never *"can the account that matters actually receive"*. + Those come apart on a first run: ``_ensure_bootstrap_admin`` creates the account holding + ``frozenset(Permission)`` with no ``email=``, and ``SecurityEventNotifier.notify`` starts + ``if not event.email: return`` -- so every notice about the most privileged account on the + instance no-ops while the gate reports a healthy channel. + + Deliberately scoped to the ROLE, not to the bootstrap account: ``email`` is optional in + ``UserCreateRequest`` and is not required for the Administrator role, so a hand-created + privileged account has the identical hole. Keying on the bootstrap user alone would close + the instance this was found on and leave the class open. + + Enumerates as :meth:`is_last_enabled_admin` and :meth:`_other_enabled_admin_exists` do -- + same store calls, same disabled-skip, same role test. **That agreement is a convention, not + a mechanism, and this docstring must not claim otherwise:** these are now THREE independent + copies of "who is an enabled administrator", and nothing binds them. If one gains a + condition -- a lockout check, an auth_provider filter -- the others keep the old answer + silently. Extracting a shared enumeration is worth its own item; it is deliberately not done + here, because it would rewrite two guards this change has no business touching. + """ + for user in await self._store.list_users(): + if user.disabled or not user.email: + continue + if Role.ADMINISTRATOR.value in await self._store.get_user_role_ids(user.id): + return True + return False + async def set_ad_group_map(self, entries: Sequence[tuple[str, str]], *, actor: str) -> None: await self._store.set_ad_group_role_map(entries) await self._audit( diff --git a/messagefoundry/config/tls_policy.py b/messagefoundry/config/tls_policy.py index c15fb6ba..e8aabefb 100644 --- a/messagefoundry/config/tls_policy.py +++ b/messagefoundry/config/tls_policy.py @@ -122,8 +122,10 @@ def harden_kex_groups(ctx: ssl.SSLContext) -> str | None: *assertable*. This docstring previously said "Python 3.13+"; that was wrong, and it was repeated into `docs/PHI.md`, `docs/ASVS-L2-PHASE0-CHANGES.md`, ADR 0092 ยง4(b) and the ASVS scorecard. Measured on this tree: Python 3.14.6 / OpenSSL 3.5.7, ``hasattr(ctx, "set_groups")`` is ``False``, - so :data:`APPROVED_KEX_GROUPS` reaches **zero** of this function's six call sites and every built - context falls back to OpenSSL's default group list. + so :data:`APPROVED_KEX_GROUPS` reaches **none** of this function's call sites and every built + context falls back to OpenSSL's default group list. **No count is stated here on purpose** โ€” the + site list is *derived* by ``tests/test_tls_policy.py``, so a number written out here is a second + definition that nothing checks. The "six" this sentence used to carry was wrong (BACKLOG #1105). That default *is* forward-secret โ€” which is the property the TLS 1.2+ floor guarantees and the one ASVS 11.6.2's first clause is about โ€” but it is **wider than the approved list**. Measured against @@ -133,8 +135,8 @@ def harden_kex_groups(ctx: ssl.SSLContext) -> str | None: the engine's key-exchange groups as "pinned" anywhere โ€” they are **inherited**. The **return value is the point.** A security control that cannot report whether it did anything - reports success forever; that is how a call at six sites with zero effect survived three - assessments. ``tests/test_tls_policy.py`` asserts the ``None`` *unconditionally*, so the first + reports success forever; that is how a call at every one of those sites with zero effect + survived three assessments. ``tests/test_tls_policy.py`` asserts the ``None`` *unconditionally*, so the first interpreter that grows the API turns that test red โ€” which is the signal to re-derive this docstring, `docs/PHI.md` ยง4 and the 11.6.2 row, and to switch the test to ``get_groups()``. diff --git a/messagefoundry/controlchars.py b/messagefoundry/controlchars.py index f4308797..bc9c64ac 100644 --- a/messagefoundry/controlchars.py +++ b/messagefoundry/controlchars.py @@ -42,9 +42,34 @@ #: screens values destined for byte-oriented sinks -- a request line, a header, a path -- where C0 #: and DEL are the injection alphabet. Widening it is a behaviour change at seven call sites at #: once, which is exactly the leverage this module exists to provide; make it deliberately. +def _is_control_char(ch: str) -> bool: + """THE ONE DEFINITION of the alphabet this module screens for (BACKLOG #1273). + + It was previously written out TWICE -- once in each public arm -- inside the module whose whole + purpose is to state it once. The module docstring above records that this replaced the same + expression written seven times across six files; it then kept two copies of its own. + + That is not a cosmetic duplication, and the risk is ASYMMETRIC -- measured on the two-copy + structure before this change, not predicted: + + * widening the **predicate** arm alone was **CAUGHT** -- 4 tests red, because + ``test_c1_and_unicode_separators_are_deliberately_NOT_caught`` pins the alphabet directly; + * widening the **strip** arm alone was **NOT CAUGHT** -- 47 passed, exit 0. + + So the copies were partly bound and partly not, and the unguarded direction is the one that + matters: **widening the alphabet is the stated reason this module exists** ("a behaviour change + at seven call sites at once ... make it deliberately"), and a deliberate widening applied to the + neutraliser would silently strip more than the screen refuses -- a screen and its neutraliser + disagreeing about their own alphabet, with every test green. + + One definition closes both directions by construction rather than by a test noticing. + """ + return ord(ch) < 0x20 or ord(ch) == 0x7F + + def has_control_char(text: str) -> bool: """True if ``text`` contains any C0 control character or DEL.""" - return any(ord(ch) < 0x20 or ord(ch) == 0x7F for ch in text) + return any(_is_control_char(ch) for ch in text) def strip_control_chars(text: str) -> str: @@ -53,4 +78,4 @@ def strip_control_chars(text: str) -> str: The strip arm, used where a value must be neutralised rather than refused. See the module docstring: this is NOT the general remedy and must not be substituted for a rejection. """ - return "".join(ch for ch in text if not (ord(ch) < 0x20 or ord(ch) == 0x7F)) + return "".join(ch for ch in text if not _is_control_char(ch)) diff --git a/scripts/docs/dangling_citation_check.py b/scripts/docs/dangling_citation_check.py index 1aee0144..39a7248f 100644 --- a/scripts/docs/dangling_citation_check.py +++ b/scripts/docs/dangling_citation_check.py @@ -198,6 +198,27 @@ def unresolved_citations(paths: list[Path], allocated: set[int]) -> list[Hit]: return hits +def is_live_shape(hit: Hit, floor: int) -> bool: + """True iff this unresolved citation can EVER arm -- the shape the exit code keys on. + + THE ONE DEFINITION (BACKLOG #1235 residual 2). This predicate previously existed twice: once + inline in :func:`main` and once re-derived inside the gate test, agreeing by convention with + nothing binding them. Two definitions of the rule a gate fires on is the defect this whole tool + exists to catch in other people's gates, reproduced inside it. + + Both conditions exclude a citation that is REPORTED but is not a defect: + + * ``number > floor`` -- at or below the allocator's high-water mark a number can never be + issued (``alloc.ps1`` starts at ``$observed + 1`` and only ever scans upward), so the + citation resolves to nothing permanently rather than by luck. + * ``not pr_shaped`` -- a PR, issue or foreign-repo reference is not a backlog citation at all. + + Keeping them here rather than at the call site is what makes a single mutation red BOTH the + exit-code arms and the real-tree gate. Before this, mutating one left the other green. + """ + return hit.number > floor and not hit.pr_shaped + + def main(argv: list[str] | None = None) -> int: parser = argparse.ArgumentParser( description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter @@ -275,7 +296,7 @@ def main(argv: list[str] | None = None) -> int: # never be issued, and a PR/issue/foreign-repo reference is not a backlog citation at all; both # are reported for a human to read and neither is a defect. Failing on them would red the tree # today for hits that are correct, and a gate that cries wolf gets switched off. - live = [h for h in hits if h.number > floor and not h.pr_shaped] + live = [h for h in hits if is_live_shape(h, floor)] if live: print() print(f"LIVE SHAPE: {len(live)} citation(s) name a number that can still be issued.") diff --git a/tests/test_dangling_citation_check.py b/tests/test_dangling_citation_check.py index 42d00b26..36cd3811 100644 --- a/tests/test_dangling_citation_check.py +++ b/tests/test_dangling_citation_check.py @@ -188,25 +188,26 @@ def _repo_root() -> Path: def _live_shape_citations() -> list[tuple[str, int, int]]: - """Citations naming a number ABOVE the floor that are not PR/foreign-repo shaped. + """Citations naming a number that can still be issued -- the shape that can arm. - Below the floor is unreachable forever, and a foreign reference is not a backlog citation at - all; both are reported by the tool for a human to read and neither is a defect. + CALLS THE SHIPPED PREDICATES RATHER THAN RE-DERIVING THEM (BACKLOG #1235 residual 2). This + helper previously re-implemented both halves inline -- `number in filed` duplicating + `unresolved_citations`, and `number <= floor or pr_shaped` duplicating what `main` filters on. + Two of the three agreed with the script by convention, with nothing binding them: a rule with + two definitions is the exact defect this tool exists to catch in other people's gates. + + The consequence is checkable and is the point: a single mutation to `cc.is_live_shape` now reds + BOTH this real-tree gate and the exit-code arms below. Before, mutating one left the other + green, which is what "agreeing by convention" buys you. """ root = _repo_root() floor = cc.allocation_floor() - filed = cc.allocated_numbers() - out: list[tuple[str, int, int]] = [] - for path in sorted((root / "docs").rglob("*.md")): - try: - text = path.read_text(encoding="utf-8") - except (OSError, UnicodeDecodeError): - continue - for lineno, number, _line, pr_shaped in cc.citations_in(text): - if number in filed or number <= floor or pr_shaped: - continue - out.append((str(path.relative_to(root)), lineno, number)) - return out + hits = cc.unresolved_citations(sorted((root / "docs").rglob("*.md")), cc.allocated_numbers()) + return [ + (str(hit.path.relative_to(root)), hit.lineno, hit.number) + for hit in hits + if cc.is_live_shape(hit, floor) + ] def test_the_docs_scan_actually_covers_something() -> None: @@ -224,3 +225,84 @@ def test_no_docs_citation_names_a_number_that_can_still_be_issued() -> None: assert not live, "citations naming a still-issuable number:\n " + "\n ".join( f"{p}:{n} #{num}" for p, n, num in live ) + + +# --- main()'s EXIT CODE, which nothing above asserts (BACKLOG #1235, residual) --------------------- +# +# HOW THE GAP WAS FOUND, because the method transfers: a SET DIFFERENCE over the module's public +# surface, not a grep for something missing. The module defines six top-level names -- +# _load_backlog_module, allocation_floor, allocated_numbers, citations_in, unresolved_citations, +# main -- and the suite above exercises four. `main` was the SOLE untouched public entry point. +# That is a positive enumeration on both sides, so it does not depend on anyone's choice of pattern. +# +# WHY IT MATTERS HERE SPECIFICALLY: main()'s last line IS the fail-closed contract -- +# return 1 if (live and not args.advisory) else 0 +# -- and it had ZERO coverage. Invert the `not`, or return 0 unconditionally, and every test above +# still passes. This file's own header calls a detector that cannot fail "not a gate"; its exit code +# was in exactly that state. +# +# THE LAST TWO ARMS ARE THE POINT. Both produce HITS and both must exit 0, because the contract keys +# on the LIVE SHAPE rather than on the hit count. Without them, a mutation to `return 1 if hits +# else 0` passes everything -- and that mutation reds the tree today on 26 permanently-harmless +# citations, which is how a gate gets switched off. + + +def _unissued_above_floor() -> int: + """A number the allocator CAN still issue -- the only shape that can ever arm.""" + # int() is load-bearing for mypy, not decoration: `cc` is loaded via importlib at runtime, so + # every attribute on it is Any and the arithmetic silently widens the return type. + number = int(cc.allocation_floor()) + 100 + assert number < 9000, "citations_in only scans [1000,9000); pick differently" + return number + + +def _unissued_below_floor() -> int: + """A permanent hole: at or below the high-water mark, so never issuable. + + Derived rather than hardcoded. A literal would silently become a RESOLVING number the day it is + filed, at which point this stops testing the below-floor branch and nothing would say so. + """ + allocated = cc.allocated_numbers() + floor = cc.allocation_floor() + for number in range(1000, floor + 1): + if number not in allocated: + return number + raise AssertionError("no hole below the floor; this arm needs a different construction") + + +def _doc(tmp_path: pathlib.Path, body: str) -> str: + path = tmp_path / "doc.md" + path.write_text(body, encoding="utf-8") + return str(path) + + +def test_a_live_shape_citation_makes_main_exit_1(tmp_path: pathlib.Path) -> None: + """FAIL CLOSED BY DEFAULT. The whole point of the flip from opt-in `--fail`.""" + doc = _doc(tmp_path, f"see #{_unissued_above_floor()} for the rationale\n") + assert cc.main([doc]) == 1 + + +def test_advisory_reports_the_same_hit_and_exits_0(tmp_path: pathlib.Path) -> None: + """The documented escape. Untested, an opt-out is indistinguishable from a broken gate.""" + doc = _doc(tmp_path, f"see #{_unissued_above_floor()} for the rationale\n") + assert cc.main([doc]) == 1 # same corpus, so the arms differ ONLY by the flag + assert cc.main([doc, "--advisory"]) == 0 + + +def test_a_file_with_no_citation_exits_0(tmp_path: pathlib.Path) -> None: + assert cc.main([_doc(tmp_path, "no citation here at all\n")]) == 0 + + +def test_a_citation_BELOW_the_floor_is_reported_but_does_not_fail(tmp_path: pathlib.Path) -> None: + """ASYMMETRIC ARM 1: a hit that must NOT fail. Below the high-water mark the allocator can never + issue that number, so the citation is permanently harmless -- reported for a human, not a defect. + """ + doc = _doc(tmp_path, f"see #{_unissued_below_floor()} for the rationale\n") + assert cc.main([doc]) == 0 + + +def test_a_PR_SHAPED_reference_is_reported_but_does_not_fail(tmp_path: pathlib.Path) -> None: + """ASYMMETRIC ARM 2, on the other axis: a foreign reference is not a backlog citation at all, + 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 diff --git a/tests/test_notifiable_admin.py b/tests/test_notifiable_admin.py new file mode 100644 index 00000000..39e240c6 --- /dev/null +++ b/tests/test_notifiable_admin.py @@ -0,0 +1,126 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""BACKLOG #1020: an ENABLED administrator with a deliverable address, as a predicate. + +The PHI startup gate computes notification readiness from the SMTP transport alone +(``notify_security_events`` + ``email_smtp_host`` + ``email_from``). That answers *"is a transport +configured"* and never *"can the account that matters actually receive"* -- and the two come apart on +exactly the instance the gate is meant to protect: ``_ensure_bootstrap_admin`` creates the account +holding ``frozenset(Permission)`` with no ``email=``, so every notice about it no-ops while the gate +reports a healthy channel. + +``has_notifiable_admin`` is the missing half of that question. These arms are deliberately +ASYMMETRIC -- a control that failed on everything would not distinguish *"the predicate keys on a +deliverable admin"* from *"the predicate is just hard to satisfy"*: + +* the REAL bootstrap path yields False (the defect, reproduced rather than described); +* an administrator WITH an address yields True; +* a non-administrator with an address still yields False -- so the predicate keys on the ROLE, not + on "some mailbox exists somewhere", which is the scope the item asks for (``email`` is optional + for any Administrator, so a hand-created privileged account has the identical hole). +""" + +from __future__ import annotations + +from collections.abc import AsyncIterator +from pathlib import Path + +import httpx +import pytest + +from messagefoundry.api import create_app +from messagefoundry.auth.service import AuthService +from messagefoundry.config.settings import AuthSettings +from messagefoundry.pipeline import Engine + +PW = "a-strong-test-passphrase" # >=15, no app/vendor terms -- satisfies the ASVS policy (WP-3) + + +@pytest.fixture +async def engine(tmp_path: Path) -> AsyncIterator[Engine]: + eng = await Engine.create(tmp_path / "notifiable_admin.db", poll_interval=0.02) + yield eng + await eng.stop() + + +def _client(engine: Engine, service: AuthService) -> httpx.AsyncClient: + transport = httpx.ASGITransport(app=create_app(engine, auth=service)) + return httpx.AsyncClient(transport=transport, base_url="http://t") + + +async def _admin_session(c: httpx.AsyncClient, service: AuthService) -> dict[str, str]: + """Bootstrap the first admin exactly as a first run does, and clear its must-change flag.""" + boot = await service.initialize() + assert boot is not None + tok = ( + await c.post( + "/auth/login", + json={"username": "admin", "password": boot.password, "provider": "local"}, + ) + ).json()["token"] + h = {"Authorization": f"Bearer {tok}"} + await c.post( + "/me/password", + headers=h, + json={"current_password": boot.password, "new_password": "a-rotated-passphrase-99"}, + ) + tok = ( + await c.post( + "/auth/login", + json={"username": "admin", "password": "a-rotated-passphrase-99", "provider": "local"}, + ) + ).json()["token"] + return {"Authorization": f"Bearer {tok}"} + + +async def test_the_bootstrap_admin_alone_is_not_notifiable(engine: Engine) -> None: + """The defect, on the REAL first-run path rather than a hand-built fixture. + + This is the state a deploying site is in at the moment the SMTP-only gate passes: one account, + holding every permission, with no address any notice could reach. + """ + service = AuthService(engine.store, AuthSettings(require_mfa=False)) + async with _client(engine, service) as c: + await _admin_session(c, service) + assert await service.has_notifiable_admin() is False + + +async def test_an_administrator_with_an_address_is_notifiable(engine: Engine) -> None: + service = AuthService(engine.store, AuthSettings(require_mfa=False)) + async with _client(engine, service) as c: + h = await _admin_session(c, service) + r = await c.post( + "/users", + headers=h, + json={ + "username": "root2", + "password": PW, + "roles": ["administrator"], + "email": "ops@example.org", + }, + ) + assert r.status_code == 201, r.text + assert await service.has_notifiable_admin() is True + + +async def test_a_non_administrator_with_an_address_is_not_enough(engine: Engine) -> None: + """The asymmetric arm: an address on a NON-privileged account must not satisfy the predicate. + + Without this, a predicate that merely asked *"does any user have an email"* would pass every + other arm here -- and would report a healthy channel on exactly the instance #1020 describes. + """ + service = AuthService(engine.store, AuthSettings(require_mfa=False)) + async with _client(engine, service) as c: + h = await _admin_session(c, service) + r = await c.post( + "/users", + headers=h, + json={ + "username": "viewer1", + "password": PW, + "roles": ["viewer"], + "email": "viewer@example.org", + }, + ) + assert r.status_code == 201, r.text + assert await service.has_notifiable_admin() is False diff --git a/tests/test_tls_floor_probe.py b/tests/test_tls_floor_probe.py index bc874fde..e1346812 100644 --- a/tests/test_tls_floor_probe.py +++ b/tests/test_tls_floor_probe.py @@ -203,7 +203,7 @@ def test_a_missing_tlsversion_enum_raises_rather_than_skipping( """The `harden_kex_groups` failure mode, refused on purpose. That helper pins nothing on this interpreter because `SSLContext.set_groups` does not exist and it - returns silently โ€” six call sites, zero effect, green tests. A probe that skipped when a + returns silently โ€” every call site, zero effect, green tests. A probe that skipped when a deprecated `TLSVersion` disappeared would become a gate that cannot fail, and would report success forever afterwards. diff --git a/tests/test_tls_policy.py b/tests/test_tls_policy.py index ab5ef3f7..5b4c6a24 100644 --- a/tests/test_tls_policy.py +++ b/tests/test_tls_policy.py @@ -121,7 +121,7 @@ def test_the_group_pin_is_inert_on_this_runtime_and_says_so() -> None: """A liveness receipt for ASVS 11.6.2 โ€” written to FAIL on the interpreter upgrade. ``SSLContext.set_groups`` is a **Python 3.15** addition, so ``harden_kex_groups`` pins nothing on - any interpreter this project runs on and ``APPROVED_KEX_GROUPS`` reaches zero of its six call + any interpreter this project runs on and ``APPROVED_KEX_GROUPS`` reaches none of its call sites. That was already true; what was missing was any way to NOTICE. The tests that stood here asserted (a) that the call does not raise, (b) that it no-ops on an object without the API, and (c) the contents of a string constant โ€” all three pass identically whether or not a single group is