docs(security): the DELETE /me/mfa route table claimed a refusal the code does not make - #200
Merged
Merged
Conversation
…code does not make 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.
wshallwshall
enabled auto-merge (squash)
August 5, 2026 04:47
wshallwshall
added a commit
that referenced
this pull request
Aug 5, 2026
…floor that had 23 items of slack (#202) TEN doc-drift modules exist to police documents and every one is reachable ONLY through pytest, which is gated on `code == 'true'`. So on a docs-only PR -- the exact shape they exist to check -- none of them runs. That is the same defect the "Backlog status invariant" step above was added to fix, at ten times the surface, and this change extends that step's own argument rather than inventing a design: "gating it would skip it on exactly the pull requests it exists to police". NOT HYPOTHETICAL. On 2026-08-04 four docs-only PRs merged (#197, #198, #200, #201) and the doc guards ran on none of them. Two carried citation errors found only by reading: an ADR named ONE inbound citation of docs/releases/ where there are 23, and a docs/SECURITY.md route-table row asserted a refusal DELETE /me/mfa does not make. Neither of those two would have been caught by these particular modules -- but they are the evidence that docs-only PRs land unchecked against the guards built to check them. NO NEW REQUIRED CONTEXT, and that is deliberate. The steps ride the existing required `test (ubuntu-latest, py3.14)` job, which already RUNS on every PR -- only its steps are gated. So this needs no branch-protection change and no owner action, unlike a new job. .github/required-contexts.txt is explicit that it is "not the enforcement -- the server is", and adding a required context is all-or-nothing against a 422. `[dev]` rather than a bare `pip install pytest`, and this one would have bitten: pyproject sets asyncio_mode = "auto" (needs pytest-asyncio) and addopts "--timeout=60 --timeout-method=thread" (needs pytest-timeout), so a bare pytest ERRORS on an unknown option before collecting anything -- a step that could never pass, reding every docs-only PR. Still far lighter than the gated install, which adds console + fhir + dicom + x12 + xml that no doc scan touches. THE FLOOR HAD 23 ITEMS OF SLACK. `--min-items 277` against a corpus that has reached 300, in a guard whose entire purpose is to notice the corpus shrinking. Raised to 300 in BOTH places -- and the second place is the finding: the floor also lives as `_MIN_TOTAL_ITEMS` in tests/test_backlog_status_check.py and NOTHING compares them, so the lower copy silently becomes the only floor that binds. That is BACKLOG #1018's duplicated-constant shape. Both sites now name the other; it is not mechanised here because a test parsing workflow YAML to compare an integer is a new coupling, and the honest fix belongs with #1018's helper. 89 of the 241 tests SKIP and that is structural, recorded in the step: they assert against docs/security/THREAT-MODEL.md, which is vault-only and absent here. ADR 0156 records the class and ASVS 15.1.3 is open on it. The step prints the module list BEFORE running and uses -rs so no skip reads as a pass.⚠️ THIS PR CANNOT EXERCISE ITS OWN NEW STEPS. It changes ci.yml and a test, so `code == 'true'` here and the new steps skip. Evidence is a local run of the exact command (152 passed, 89 skipped, 5.12s) plus confirming [dev] supplies both plugins, that "Set up Python" is ungated, and that constraints.lock is tracked. The first docs-only PR is the real test, and it fails CLOSED -- a broken step reds that PR rather than passing silently. Verified: 57 workflow-structure tests pass (incl. test_workflow_shell_syntax and test_ci_venv_pinning), 27 backlog/lint-scope tests pass, ruff check and format clean, and the YAML parses with both floors reading 300.
wshallwshall
added a commit
that referenced
this pull request
Aug 7, 2026
…256 #259 #264) (#275) * feat(api): report-only TLS key-exchange groups posture field; correct stale "pinned" doc claims (BACKLOG #338) The engine's TLS key-exchange (KEX) groups are INHERITED from OpenSSL's default group list, not pinned to the approved set. harden_kex_groups pins nothing until SSLContext.set_groups lands in Python 3.15, so on every interpreter this project currently runs on the approved pin is inert. This is documentation accuracy plus observability -- it changes no live TLS behaviour (the TLS 1.2+ floor is the enforced control), and on a NOT-DEPLOYED beta there is no exposure today; the pin is a future 3.15 hardening. Two parts: 1. Report-only surfacing. New pure helper config/tls_policy.kex_groups_report() builds a throwaway probe context and asks the ONE authority, harden_kex_groups, what it manages to pin -- so the read-out can never drift from what the connectors actually do. It returns "inherited (...)" on a pre-3.15 interpreter and "pinned: ..." on 3.15+. Surfaced as an additive SecurityPosture.kex_groups field (str | None, default None), wired in create_app beside fips_attestation(), rendered as a status-page row in the web console beside the FIPS/OpenSSL rows. Report-only: it reflects, and changes, no TLS behaviour. 2. Three doc-accuracy edits correcting restatements that still read as "pinned": CONTAINER-EXPOSURE-EVALUATION.md (verification table), ASVS-L2-PHASE0-CHANGES.md (PQC roadmap row), and #200's Closes line in docs/archive/backlog/BACKLOG-CLOSED.md (11.6.2 annotated PARTIAL). Each links to PHI.md's data-in-transit section, the single source of record for the measured accepted set, rather than restating it. The two Python-3.15 tripwire tests in test_tls_policy.py that fire when set_groups/get_groups land are left untouched -- they are the signal to actually set the pin. Engine UI seam bumped 17 -> 18: the golden seam snapshot introspects SecurityPosture's field set, so a purely additive field trips the handshake; SUPPORTED_ENGINE_SEAMS and the golden snapshot updated to match. Tests: test_tls_policy.test_kex_groups_report_reports_inherited_today (the helper reports inherited + names the approved list, never "pinned:"); test_api_auth.test_security_posture_reports_kex_groups (the field flows through the MONITORING_READ-gated, audited posture route and matches the helper); a status-builder assertion that the console renders the row. All three falsified: blanking the helper reddens the two report tests (assert 'inherited' in ''); removing the console row reddens the status-builder test (assert 'key-exchange' in html). * docs(backlog): flip #338 banner to shipped (BACKLOG #338) Flip the #338 status banner from filed/not-started to shipped, now that the report-only kex_groups posture field and the three doc-accuracy corrections have landed. The banner records that the KEX groups are documented as inherited (the pin is inert until Python 3.15) plus the report-only surfacing behind engine seam v18. Banner line only, under the #338 heading, verified by number. The ranked table, the four census distribution lines, and every other item's banner are untouched. The census was NOT recomputed. * docs(research): OpenFlow step-attribute vocabulary gap-map (BACKLOG #238) A findings note comparing Windmill's seven OpenFlow step-attributes (retry, timeout, stop_after_if, skip_if, continue_on_error, mock, cache_ttl) against MessageFoundry's engine/handler vocabulary. Per attribute: what it is, the engine analogue (grounded in a real symbol/file), and the gap (covered-different-locus / partial / absent). This is a review, not a feature. OpenFlow is explicitly NOT a compatibility target; the note is an informational vocabulary map, not a gap-to-close list. Adopting a declarative artifact stays declined by ADR 0076 section 7 and BACKLOG #26. Framing is conditional throughout (not-deployed beta). Lands at docs/research/openflow-step-attributes.md, mirroring the existing docs/research/ review-note convention (config-ux-review.md, message-model-eval.md, ide-low-code-options.md). The mock row cites config/db_lookup.py / config/fhir_lookup.py for the pure-dry-run raise guard (config/db_lookup.py raise DbLookupError / config/fhir_lookup.py raise FhirLookupError), not config/wiring.py whose raise text is about the router phase. * docs(backlog): flip #238 banner to CLOSED (BACKLOG #238) The findings note (docs/research/openflow-step-attributes.md) is the item's expected output and is now delivered, so #238's banner flips from open to closed. One banner line only, under the #238 heading. Census NOT recomputed: this commit changes only #238's banner line and does not touch the ranked table or the four census distribution lines. * fix(serve): auth-off startup arm now refuses a declared terminator (BACKLOG #1013) The `[auth] enabled=false` startup arm keyed on the bind alone (`not settings.api.is_loopback`), so it did not fire for a loopback bind behind a declared upstream TLS terminator. A PHI instance with authentication entirely off behind a declared terminator would have started with no refusal and no warning on first deployment, while the same topology with auth on but MFA off is already refused by the gate #326 fixed. The two arms disagreed about what "exposed" means in the same file. Hoist the single `instance_exposed` definition (#326: an off-loopback bind OR a declared upstream TLS terminator) above the auth-off arm and widen the arm to consult it, so it refuses on a non-loopback bind OR a declared terminator. The existing loopback refusal is kept; the condition is widened, not replaced. Load order verified: `instance_exposed`'s inputs -- `settings.api.host` (through `is_loopback`) and `settings.api.tls_terminated_upstream` -- are read straight off the loaded config, and the only in-place mutation of `settings.api.*` between the hoisted definition and the former site is `serve_ui` (twice), which the predicate does not read. The definition remains defined exactly once. Tests (tests/test_cli.py): auth off + declared terminator on a loopback bind refuses (positive); auth off + true loopback with no terminator still starts (negative control); auth on + declared terminator is unaffected by the arm. Each was falsified -- reverting the arm to the bare bind check reds the positive test, firing on any auth-off reds the negative control, and dropping the auth check reds the auth-on test; each was restored. Docs updated so the contract travels with the code (CLAUDE.md 11): DEPLOYMENT.md, SYSTEM-REQUIREMENTS.md, SECURITY.md, REMOTE-CONSOLE.md and SECURITY-LOOSENING.md now describe the auth-off refusal as firing on an exposed instance (a non-loopback bind OR a declared terminator), not on the bind alone. BACKLOG #1013 * docs(backlog): mark #1013 fixed; record the resolved load-order question (BACKLOG #1013) Flip the #1013 banner from filed/open to fixed and record, in the AMENDED blockquote, that the load-order prerequisite the item flagged as unproven holds: `instance_exposed`'s inputs resolve where the auth-off arm runs. Name the single-definition pointer comment rather than pin its line number, since the hoist shifts that line. Banner flip only: the ranked table and the four census distribution lines were NOT recomputed. BACKLOG #1013 * docs(coord): the session record has no branch, and two rosters disagree about one A session was told a coordinator "might not be there" on the strength of the session-list MCP tool's `isRunning: false`, and separately the two rosters reported different branches for one checkout. Both readings were wrong the same way -- a field answering a question adjacent to the one asked -- and neither trap was written down anywhere a reader would look. MEASURED 2026-08-06. A session record holds exactly cwd, entrypoint, kind, name, nameSource, peerProtocol, pid, procStart, sessionId, startedAt, version. There is NO branch field. So any branch printed beside a session came from elsewhere, and the two sources answer different questions while both being labelled "branch": presence.ps1 / occupancy.ps1 the WORKTREE's branch, live from `git worktree list --porcelain`. Current. session-list MCP tool a SESSION attribute captured at registration. Does not follow a later `git switch`. For one checkout they reported two different names -- the live roster the branch that checkout had been switched onto, the session list the one it registered with. Neither was wrong. A disagreement is not evidence that either roster is broken, and a branch from the session list must never be quoted as a checkout's current branch. ALSO RECORDED, same family: `isRunning` means "currently EXECUTING A TURN", not "alive". An idle session between turns reads false while being perfectly reachable. It is not a liveness fence and must not be used as one -- Get-SessionLiveness is, subject to the rule already stated directly above it that ONLY THE POSITIVE ANSWER IS SAFE TO ACT ON. That rule is why the original inference was doubly wrong: it drew a negative conclusion from a signal that cannot support one, using a field that answers a different question. The concrete branch names are deliberately NOT quoted -- the leak gate refused the first attempt because a real worktree slug is an internal project name, and the lesson does not need them. Documentation only; no behaviour change. Both files parse, presence.ps1 still runs, 272 tests pass across the coord/presence/occupancy suites. * docs(supply-chain): correct at least two claims the shipped v0.3.2 release assets do not support Verified against the actual release assets, not the prose: `gh release download v0.3.2` gives an SBOM with licenses on 40/40 components and hashes on 0/40, and a VEX with `"statements": []`. Two claims did not survive that check. VEX contents. `docs/SUPPLY-CHAIN.md` described the OpenVEX asset as "our per-CVE exploitability assessments" and told the reader it "records, per CVE, whether the vulnerable code is reachable" -- a statement about the contents of a published artifact, false in any tense. It sat immediately after "Do not demand a zero-CVE clean scan", so a procurement reader who applied the VEX, saw no suppressions, and read that as an assessed all-clear would have had no assessment behind it: a compensating control resting on a false premise (CLAUDE.md section 11). The page now names the artifact, says what a statement carries when one exists, and says plainly that where we have not assessed a CVE the document is silent and the scanner's finding stands. The empty-state rule stays stated once, at security/vex/README.md:17-18 and ADR 0149, and is linked rather than re-copied -- the removed sentence was itself the divergent third copy. Component hashes. The inventory sentence enumerated "components, versions, PackageURLs, hashes, and licenses". Backfilling hashes was investigated and rejected on semantics rather than effort: CycloneDX `component.hashes` means the hash of THE file, while requirements-core.lock carries 301 `--hash=sha256:` lines over 41 packages and no package with exactly one (cryptography alone has 40). cyclonedx-py deliberately routes lock hashes to `externalReferences` for that reason, the 1.6 schema imposes no uniqueness constraint so a multi-entry set would validate clean as a silent false claim, and `pip` is an inventoried component with no lock line at all. So the sentence is corrected instead: "at least" replaces the closed enumeration, the reason given is the verified one, and no substitute integrity control is offered -- Sigstore and SLSA attest the SBOM document and our own release files, which is an adjacent question, and the lock is not a released artifact. The same false enumeration sat in scripts/security/sbom_finalize.py's docstring, one hop from the corrected page, which names that script by path in its "for auditors" section. Fixed there too rather than leaving the repo self-contradictory on the fact this commit is about. Nothing is deployed, so nobody has been misled; the defect is that the shipped page WOULD mislead a first reader who tried to verify components against hashes the SBOM does not carry. Deliberately unchanged: the true "hash-locked" phrases at :16 and :86 refer to the lock the inventory is built from, and a blanket scrub of the word would have deleted accurate claims -- the new text disambiguates them instead. No VEX statement is written here; see the notes handed to the coordinator.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A security document asserting a control that does not exist — the compensating-control-on-a-false-premise defect CLAUDE.md §11 forbids. Two-line fix, but the second half corrects a body I filed hours ago.
The false claim
docs/SECURITY.md:329, a route-table row, asserted:It is not refused. That route resolves to
disable_my_mfa(auth_routes.py:429), whose own docstring says "turn off the caller's TOTP MFA", anddisable_mfa(service.py:2083) goes straight fromget_usertodisable_totpwith no guard. Verified by reading both, not inferred.The row now states the absence and points at BACKLOG #1022. This stands independently of whether #1022 is ever built — while the guard is absent, the doc must say so.
It also corrects #1022's own body, from PR #197
That body cited
docs/SECURITY.md:752as 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. It 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("'overauth_routes.pyreturns only/me/mfa, sessions, roles and users — passkey removal is console-only, so it cannot be what a route-table row forDELETE /me/mfadescribes.So #1022's remaining doc obligation is ADR 0068 line 140, not a
SECURITY.mdline. Its body and ranked-table row now say that, and the wrong citation is recorded as corrected rather than quietly swapped — a body that mis-cites its own evidence sends the next reader to a line that reads fine.Verification
backlog_status_check.py: OK — 105 open, 194 archived, 299 total, each declaring exactly one statustests/test_security_doc_drift.pyandtests/test_backlog_status_check.py:125-128includes("DELETE", "/me/mfa"); it checks that routes are documented, not the guarantee prose, so this edit is in scope for it and it stays green