From 4e78ad37051bec24f4e9efde18e477f4d2a632ad Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 4 Aug 2026 23:47:20 -0500 Subject: [PATCH] docs(security): the DELETE /me/mfa route table claimed a refusal the code does not make MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docs/SECURITY.md:329 asserted that DELETE /me/mfa is "refused when it would remove the last factor while MFA is required". It is not. That route resolves to disable_my_mfa (api/auth_routes.py:429), whose own docstring says "turn off the caller's TOTP MFA", and disable_mfa (auth/service.py:2083-2100) goes straight from get_user to disable_totp with no guard -- verified by reading both, not inferred. The row now states the absence and points at BACKLOG #1022. That is a security document asserting a control that does not exist, which is the compensating-control-on-a-false-premise defect CLAUDE.md section 11 forbids. It stands independently of whether #1022 is ever built: while the guard is absent, the doc must say so. ⚠️ THIS ALSO CORRECTS #1022'S OWN BODY, filed hours ago in PR #197. That body cited docs/SECURITY.md:752 as stating the refusal "as an unconditional property of the system". The citation was WRONG. Line 752 sits inside a WebAuthn paragraph, describes PASSKEY removal, and quotes the passkey guard's own error string ("enroll another factor first") -- the path where the guard genuinely does exist, so the line is defensible as written. The false claim was the route-table row at :329, which nobody had looked at. The discriminating fact: there is NO WebAuthn credential DELETE route in api/ at all (`grep '\.delete("'` over auth_routes.py returns /me/mfa, sessions, roles and users only), so passkey removal is console-only and cannot be what a route table row for DELETE /me/mfa describes. So #1022's remaining doc obligation is ADR 0068 line 140, not a SECURITY.md line, and its body and ranked-table row now say that. The wrong citation is recorded as corrected rather than quietly swapped, because a body that mis-cites its own evidence sends the next reader to a line that reads fine. Verified: backlog_status_check.py OK (105 open, 194 archived, 299 total, each declaring exactly one status), and 60 tests pass across tests/test_security_doc_drift.py and tests/test_backlog_status_check.py. --- docs/BACKLOG.md | 6 ++++-- docs/SECURITY.md | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 9ead4fe4..3f31b0fe 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -227,7 +227,7 @@ Ordered by value descending, then difficulty ascending (cheapest first at equal | 48 | **#1020** | The first-run bootstrap Administrator is created with no email address, and the PHI notification gate cannot see it | 5 | 3 | _fill-in_ | P2 | `_ensure_bootstrap_admin` calls `create_user` with no `email=` (`auth/service.py:527-533`), so the account holding `frozenset(Permission)` has a NULL email and `SecurityEventNotifier.notify` returns before enqueueing (`pipeline/security_notify.py:130`), making all ten notice types no-op for it — including LOGIN_AFTER_FAILURES, the compromise signal. The part with teeth is gate blindness: the PHI startup gate refuses to serve without a notification channel but computes readiness from `notify_security_events` plus `email_smtp_host` plus `email_from` alone (`__main__.py:2260`), so it would report a healthy channel while no notice about the all-permission account could be delivered. The allocated title's second half is REFUTED and the body says so: lockout is time-bounded (15 minutes by default, `settings.py:1769`), an admin reset clears it (`store/store.py:7693`), the documented break-glass is a sealed file (`api/app.py:5104`), and no email-driven recovery flow exists anywhere in the code. Value 5 because every event is still an audit row surfaced by `GET /me/security-events`. Difficulty 3 with no schema change; the only judgment is which of three candidate fixes the owner wants. | | 49 | **#1019** | install-selfheal.ps1 has no installed-vs-source payload-parity instrument, and it wires the most privileged hook in the estate | 5 | 3 | _fill-in_ | P3 | The installer copies `worktree-selfheal.ps1` to `~/.claude-hooks/` and wires it as a user-scope SessionStart hook with no divergence detection: no `-Status` (the surface is `-ConfigDir` plus `-HookPath`), no version stamp, a bare `Copy-Item -Force` (`:57`), and no test that reads the installed copy — every selfheal test binds `ROOT` under a synthetic home, and the repo's only installed-vs-source parity test names the gate copy alone (`tests/test_gate_installed_parity.py:44`). The title's "no parity instrument at all" is narrowed in the body: source-level guard parity DOES exist (`test_both_installers_carry_the_same_refusal`), as do the CLAUDECODE refusal, the backup-validate-rollback, and an unconditional payload refresh — what is absent is payload parity. The claimed comparator is also wrong, and PR #191 has since sharpened this: it landed payload parity on `install-git-hooks.ps1` (SHA256, IN SYNC/STALE, plus a pytest-side assertion), so `install-selfheal.ps1` is now the ONLY installer in the estate without one, with two worked examples to copy. Privilege is substantiated: user scope in every config dir, and a hook that runs `git checkout` on the shared primary unattended (`worktree-selfheal.ps1:106`). Measured 2026-08-04, installed and source agree (`c41c70ecf885`), so detection is absent rather than divergence present. Difficulty 3 with four constraints, chiefly folding CRLF exactly as the existing instruments do. | | 50 | **#236** | Test-this-step and test-up-to-step with pinned upstream values | 5 | 4 | _fill-in_ | P2 | Real debug breadth — whole-handler traced values already fold onto rows (`mergeLiveValues`, ide/src/stepsModel.ts:544) so partial runs are a convenience, but pinning an expensive `db_lookup`/`fhir_lookup` has no equivalent at all; largely a stop condition plus state dump on ADR 0072's shipped trace, with the lookup mock and keeping `buildLensTraceArgs` (:674) incapable of emitting `--show-phi` the real work. | -| 51 | **#1022** | disable_mfa has no last-factor guard where delete_webauthn_credential does, so the two removal paths can be ordered to reach zero factors | 5 | 4 | _fill-in_ | P2 | `delete_webauthn_credential` computes `last_second_factor` and refuses when MFA is required (`auth/service.py:2426-2432`); `disable_mfa` does nothing between `get_user` and `disable_totp` (`:2086-2087`), so a user holding TOTP plus one passkey can delete the passkey (permitted while `totp_enabled` is True) and then disable TOTP, arriving at zero enrolled factors — the state ADR 0068 AC-10 says the system shall refuse and `docs/SECURITY.md:752` asserts unconditionally. The consequence is overstated in the obvious reading and the body corrects it: login enforcement is NOT missing, since `mfa_verified=not mfa_required` (`:718`) plus the ASVS 6.3.3 access gate in `require()` (`api/security.py:224-234`) make the outcome a forced re-enrollment rather than single-factor access. Value 5: genuine, demonstrable, defeats a numbered acceptance criterion by ordering and makes a shipped doc guarantee untrue, but no bypass and no PHI consequence. Difficulty 4 because the raise needs mapping at two call sites that would otherwise 500, one existing test breaks by construction (`tests/test_mfa.py:189`), and two docs move with it. Filed because ADR 0068 line 140 promised this parity follow-up and no item carries it. | +| 51 | **#1022** | disable_mfa has no last-factor guard where delete_webauthn_credential does, so the two removal paths can be ordered to reach zero factors | 5 | 4 | _fill-in_ | P2 | `delete_webauthn_credential` computes `last_second_factor` and refuses when MFA is required (`auth/service.py:2426-2432`); `disable_mfa` does nothing between `get_user` and `disable_totp` (`:2086-2087`), so a user holding TOTP plus one passkey can delete the passkey (permitted while `totp_enabled` is True) and then disable TOTP, arriving at zero enrolled factors — the state ADR 0068 AC-10 says the system shall refuse. (Corrected: the false doc claim was the `DELETE /me/mfa` **route-table row** at `docs/SECURITY.md:329`, since fixed to state the absence; `:752` is passkey-scoped and defensible, so the remaining doc obligation is ADR 0068 line 140.) The consequence is overstated in the obvious reading and the body corrects it: login enforcement is NOT missing, since `mfa_verified=not mfa_required` (`:718`) plus the ASVS 6.3.3 access gate in `require()` (`api/security.py:224-234`) make the outcome a forced re-enrollment rather than single-factor access. Value 5: genuine, demonstrable, defeats a numbered acceptance criterion by ordering and makes a shipped doc guarantee untrue, but no bypass and no PHI consequence. Difficulty 4 because the raise needs mapping at two call sites that would otherwise 500, one existing test breaks by construction (`tests/test_mfa.py:189`), and two docs move with it. Filed because ADR 0068 line 140 promised this parity follow-up and no item carries it. | | 52 | **#165** | DB schema browser + ad-hoc query runner | 5 | 5 | _fill-in_ | DEMAND-GATE | Corepoint-parity authoring aid whose external-SQL-client workaround is fully clean — the only DB reach today is the `SELECT 1` reachability probe (`messagefoundry/transports/database.py:484-501`) and dry-run refuses `db_lookup` (`messagefoundry/pipeline/dryrun.py:570`); the build is a net-new API surface plus per-dialect introspection, read-only statement gating, a permission, audit and a console pane. | | 53 | **#232** | Steps view for routers | 5 | 5 | _fill-in_ | P2 | Real Steps-view breadth gap exactly where destination selection is decided, with a workaround — read a five-line guard-and-return — clean enough to hold it off the top; a `route` row kind widens the ADR 0076 §3 grammar, so an amendment lands first, then `return []` disambiguation in a lens that skips routers outright today (messagefoundry/lens.py:306, :344-347), a router palette, and byte-stable rewrite parity. | | 54 | **#78** | Custom message-definition data model + conformance validator; NCPDP codec | 5 | 6 | _money pit_ | DEMAND-GATE | Corepoint-parity persisted-definition model plus a report-only validator and an additive NCPDP codec, all cleanly worked around today by a code-first Handler, so useful breadth rather than a blocker; the whole scope is still remainder — NCPDP appears nowhere in `messagefoundry/` and `profile` is merely "reserved for a conformance-profile" (`messagefoundry/parsing/validate.py:56`) — spanning a new stored model the code reads, a validator, and a new codec class. | @@ -4841,7 +4841,9 @@ Retiring the tree costs the engine nothing operationally: **`tests/test_ech_egre ⚠️ **Lead with the correction: MFA enforcement at login is NOT missing and must not be touched.** The obvious reading of the title sends a fixer at enforcement that already exists. Disabling TOTP under `require_mfa` would not leave the account reachable with a single factor: `login` resolves `mfa_required` and issues the session with `mfa_verified=not mfa_required` (`auth/service.py:715-718`), and `require()` applies the second factor as an ASVS 6.3.3 **access** gate, 403ing every request whose (method, path) is not one of the six exempt pairs (`api/security.py:189-234`, `:79-88`). So the post-disable state is a **forced re-enrollment** — recoverable, because the enroll routes ride `require_reauth_only*` with `mfa_gate=False` (`api/security.py:586-589`) — not a factor downgrade and not a lockout. -**The defect that survives.** The invariant is defeated by ordering, and it makes a shipped guarantee untrue: `docs/SECURITY.md:752` states the refusal as an unconditional property of the system, which the code does not deliver on this path. `disable_mfa` (`auth/service.py:2083-2099`) does nothing between `get_user` at `:2086` and `disable_totp` at `:2087` — it consults neither `has_webauthn_credentials` nor `_mfa_required_for`. It is the single enforcement point: both `api/auth_routes.py:438` and `messagefoundry_webconsole/routes/account.py:281` reach it unguarded. +**The defect that survives.** The invariant is defeated by ordering. `disable_mfa` (`auth/service.py:2083-2099`) does nothing between `get_user` at `:2086` and `disable_totp` at `:2087` — it consults neither `has_webauthn_credentials` nor `_mfa_required_for`. It is the single enforcement point: both `api/auth_routes.py:438` and `messagefoundry_webconsole/routes/account.py:281` reach it unguarded. + +⚠️ **CORRECTED 2026-08-04, and the correction narrows the doc half of this item.** This body originally cited `docs/SECURITY.md:752` as stating the refusal "as an unconditional property of the system". **That citation was wrong.** Line 752 sits inside a WebAuthn paragraph, describes *passkey* removal, and quotes the passkey guard's own error string ("enroll another factor first") — the path where the guard genuinely **does** exist. It is defensible as written. The false claim was one line in a **route table**: `docs/SECURITY.md:329`, the row for `DELETE /me/mfa`, which asserted "refused when it would remove the last factor while MFA is required". That route resolves to `disable_my_mfa` (`api/auth_routes.py:429`) whose own docstring says "turn off the caller's **TOTP** MFA", and there is **no WebAuthn credential DELETE route in `api/` at all** — passkey removal is console-only. `:329` has been corrected to state the absence and point here; the remaining doc obligation for this item is **ADR 0068 line 140**, not a `SECURITY.md` line. **This is a promised follow-up that was never filed, which is the main reason to file it rather than close it.** ADR 0068 line 140 records "TOTP-disable keeps its existing behavior this lane (parity follow-up recorded)" — and no backlog item carries that follow-up. Searches for `disable_mfa`, "TOTP-disable", "parity follow-up" and "last factor" across `docs/BACKLOG.md` and the closed archive return zero hits. So the asymmetry is a recorded decision, not an oversight. diff --git a/docs/SECURITY.md b/docs/SECURITY.md index c4ea2e95..11bfcf37 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -326,7 +326,7 @@ tuple: they act only on the caller's own account. | `GET` | `/me/mfa` | `require` | | | `POST` | `/me/mfa/enroll` | `require_reauth_only_action` (action `mfa_enroll`) | password-only step-up — the MFA gate is skipped so a required-but-unenrolled user cannot deadlock | | `POST` | `/me/mfa/confirm` | `require_reauth_only_action` (action `mfa_confirm`) | per-actor ceremony limiter; password-only step-up | -| `DELETE` | `/me/mfa` | `require_step_up_action` (action `mfa_disable`) | refused when it would remove the last factor while MFA is required | +| `DELETE` | `/me/mfa` | `require_step_up_action` (action `mfa_disable`) | step-up bound to the disable action (current factor + a fresh password). ⚠️ **No last-factor guard** — this is the TOTP path (`disable_mfa`), and it does **not** refuse when it would leave the account with zero enrolled factors. The passkey removal path does refuse; see BACKLOG #1022 for the asymmetry | | `GET` | `/me/sessions` | `require` | | | `GET` | `/me/security-events` | `require` | | | `DELETE` | `/me/sessions/{session_id}` | `require_reauth_only` | password-only step-up |