feat(security): close ASVS 4.2.5 and 9.2.2, and the guards that could not see them - #39
Merged
Merged
Conversation
…d as absent Every assertion in the drift suite pins a value that IS rendered, so a sentence asserting a bound's ABSENCE is invisible by construction. That is not hypothetical: THREAT-MODEL.md stated "There is no aggregate per-message budget" for the entire life of peek.enforce_expansion_budget, and re-inserting that exact sentence left all 81 tests green. ASVS 15.1.3 scores the document, so the stale claim was the control defect -- a false statement about the most attacker-exposed bound in the product. Two guards, each mutation-verified to red for the right reason: - MAX_COUNTED_ESCAPE_OPENERS pinned into the escape-expansion row, so the measurement's own cap cannot drift doc-side (mutating 100,000 -> 250,000 reds). - test_the_document_does_not_deny_a_bound_the_code_implements: a (shipped symbol, forbidden phrases) table. The symbol is ASSERTED rather than probed -- a guard that skipped when the control vanished would pass hardest exactly when the bound disappeared. Limits stated in the docstrings rather than implied: it matches a fixed phrase list, not paraphrase; and docs/security/** is untracked here, so _doc_text() takes its accessor skip and both guards are inert on origin/main. They run in the vault and in a local checkout with the docs materialized. The document correction itself is vault-bound and not in this commit.
…ure (ASVS 9.2.2) An access token (typ: at+jwt, RFC 9068), a back-channel logout token (logout+jwt) and an RFC 8417 security event token are minted by the SAME issuer under the SAME key as the id_token. Every key and signature rung passes on them, so the only thing standing between an access token and an accepted federated login was nonce equality. Three rungs added, each with its own closed-set slug so `verify --section federation` still indicts exactly one rung: - wrong_token_type, at the TOP of key selection (before the kid guard and the alg pin, so a wrong-class token is not audited as unknown_kid). An ABSENT typ is still accepted: RFC 7519 5.1 makes the header advisory and refusing it would lock out conforming IdPs. Normalisation lower-cases BEFORE stripping the application/ prefix -- the other order refuses a legal Application/JWT. - unexpected_events_claim, ahead of every other claim check. A logout token carries no nonce, so in ladder order it would be refused as nonce_mismatch, telling the operator the browser binding failed when the IdP actually sent the wrong token class. - claim_sub_missing, plus iat made mandatory (reusing claim_not_numeric). Both are REQUIRED of an id_token by OIDC Core 2. sub was previously read with an `else ""` fallback, so a token without one minted FederatedPrincipal(subject="") and that empty string was written into the auth.login_success audit as though it were evidence. Six mutations, each verified red for its own reason: delete the typ call -> DID NOT RAISE; swap the normalisation order -> Application/JWT refused; move the events check below the nonce compare -> 'nonce_mismatch' == 'unexpected_events_claim'; restore the optional-iat guard -> DID NOT RAISE; restore the empty-sub fallback -> DID NOT RAISE; map a slug to two rungs -> the closed-set guard names both. The hand-mint helper signs the real header bytes rather than splicing, because CompactJwtSigner hardcodes typ:"JWT" and a retargeted signature would make every test here pass for the wrong reason and stay green if the assertion were deleted. A guard-the-guard test pins that the helper mints a VALID token. ADR 0142 AC-3 amended, with a forward note: a back-channel-logout receiver (cell 10.5.5) needs its own logout-token ladder -- relaxing unexpected_events_claim to reuse validate_id_token would silently reopen this. Fail-closed on a live login path: an IdP stamping a non-jwt typ is refused after upgrade. Blast radius is bounded by [auth].oidc_enabled defaulting off. The scorecard flip for this cell is vault-bound and not in this commit.
…(ASVS 4.2.5) The 8 KiB URL/header bound was imposed only at connector CONSTRUCTION, so it covered only statically configured values. Three classes are added after it and were entirely unbounded: - per-message headers merged from message metadata, - the per-call URL a FHIR read or write builds (_FHIR_TYPE_RE bounds that grammar but not its LENGTH), - the server-minted SMART bearer and the detached-JWS headers, stamped in just before the request is built -- i.e. AFTER the point any naive fix would guard. Split into a pure detector (find_outbound_length_violation) plus two enforcers, so the same measurement serves both gates under different disclosure rules. The failure CLASS is computed, not guessed, because it decides the disposition: - message-derived -> permanent NAK. The same message overflows on every retry, so retrying is a guaranteed-futile loop that also holds the lane. - server-minted credential -> permanent + credential_fault, and the provider is invalidated. The provider caches: without invalidate() every retry re-sends a byte-identical over-length token forever; with invalidate() but no credential_fault, every retry re-signs a client assertion and POSTs the IdP forever. credential_fault_policy exists to stop exactly that re-auth storm. - anything else -> a plain DeliveryError. PHI egress: the message-derived arm emits the class and the length ONLY, never the header name. At construction a name is operator-static; at send time outbound_headers_from_metadata derives it from a message-metadata key suffix, and that string reaches last_error, message_events.detail and -- on the DeliveryError arm -- the webhook AlertSink, i.e. off-box. redaction.py concedes single-token identifiers are its acknowledged residual. Also closed, each a distinct gap rather than a missing call: - FhirLookupExecutor never called the gate AT ALL (fhir.py's only call site was the destination's __init__), so a lookup's base, its per-call read URL and its minted bearer were unmeasured on both sides. Raises FhirLookupError, not a delivery error: the read runs inside a Handler, where there is no message to dead-letter. - dicomweb bounded base_url but _target_url is derived from it 33 lines later and is what actually ships -- an ordering gap. - store/keyprovider_vault.py: the Vault token ships as X-Vault-Token on every Transit call and the address becomes the URL; both from MEFOR_* env, neither ever measured. - apiclient/client.py: base_url + path and the session bearer. Constants are DUPLICATED rather than imported -- ADR 0088 keeps that package engine-free, so the sharing import is the coupling it exists to avoid -- and pinned equal by a test that imports both sides in a test process. - The SMART and OAuth2-CC token endpoints. Signature headers are bounded at CONSTRUCTION because their length is message-independent: the JWS is detached, so the body contributes only its fixed-width hash. test_signature_header_length_is_message_independent pins that, and is what legitimises the placement. Six mutations verified red for their own reason: neutralise the send-time gate -> DID NOT RAISE; restore the header name -> the PHI assertion; drop credential_fault -> assert False is True; delete the signature bound -> DID NOT RAISE; shrink the limit to 64 -> the byte-identity control reds, proving the gate is on the ordinary path; delete the FHIR read gate -> the match= clause (mandatory, not decorative: without the guard the oversize GET reaches the opener and _parse raises FhirLookupError anyway, so a bare raises() would pass either way). Behaviour change, not pure hardening: a message that today ships a >8 KiB URL or header will dead-letter or retry instead. No legitimate config is affected -- a normal RS256 signature header is 364 chars -- but it belongs in the release note. Residual: one engine-wide 8 KiB constant, not a per-receiver negotiated limit -- the same bar already credited at construction. Not yet covered: the three _probe paths, alert_sinks, ai_broker and tray/probe.py. The scorecard flip for this cell is vault-bound and not in this commit.
…he sixth Completes the change set begun in 17d8c82. Each of these ships an outbound URL or header that nothing measured. - ai_broker: the endpoint is the request line and the api_key rides x-api-key on every provider call, both from env(). Raised as AiBrokerError (already a ValueError subclass) so the API surface keeps its single error type, and the key is never echoed -- the test pins that too. - The REST and FHIR probe paths. Both MINT a real bearer so reachability reflects the actual credentials, and that token is the one value the construction gate could not see. Bounded so an operator's "test connection" fails with the real reason instead of an opaque wire error. The SOAP probe deliberately gets NO gate, and says so where it would have gone. It mints nothing -- it ships self.url and self._headers verbatim, both already bounded at construction -- so a gate there could not fire on any input. The first draft of this commit did add one, with an Authorization arm that would have called invalidate() on a provider the probe never touches; a guard that cannot fail reads as coverage without being it, which is the exact failure mode the rest of this work exists to remove. - The webhook AlertSink URL. Construction-only here is sufficient and not a shortcut: the URL is operator config and the sole header is a fixed Content-Type, so nothing is added between construction and the wire. The import is lazy to keep the module's cost unchanged for the commoner sinks. tray/probe.py is deliberately NOT closed, and says so in its own docstring rather than being silently skipped. Three facts make that proportionate and all three must stay true: the client is tokenless (no credential can overflow), the URL is local operator config pointing at this host's own engine (neither attacker-influenceable nor message-derived), and tray/ is stdlib+httpx only (ADR 0113) -- importing transports/ to share the constant would breach the same layering ADR 0088 protects for apiclient, and a third unpinned copy of an 8192 is worse than a documented absence. If the tray ever carries a token or takes a remote URL, close it. Two new mutations, both verified red for their own reason: delete the ai_broker bound -> DID NOT RAISE AiBrokerError; delete the webhook bound -> DID NOT RAISE ValueError. Each has a byte-identity control beside it that reds when the limit is shrunk, so neither guard can pass as dead code. The harness now runs eight. Docs reconciled where they enumerate the shared rest.py helpers and would otherwise still describe a construction-only bound: ADR 0022 (twice -- the FHIR per-call URL is message-derived, so the send-time arm is load-bearing there in a way it is not for REST), ADR 0025 (the DERIVED _target_url, not just base_url), and FEATURE-COVERAGE-PLAN's HTTPFHIR-6 row, whose "no own assertion" gap note is now accurate rather than stale.
…gerprinted
MEFOR_AI_API_KEY was a registered critical secret WITH a documented rotation
cadence in the ASVS-L2-PHASE0-CHANGES schedule ("Per provider / org policy; on
compromise"), and yet was absent from _ENV_SECRET_CLASSES. So the rotation
watcher never fingerprinted it: the documentation told operators to rotate the
credential, and nothing in the engine could ever emit the reminder.
Enumeration completeness and rotation COVERAGE are different properties.
test_secret_rotation_inventory.py already guarded the first -- the secret was
correctly registered and correctly documented -- and nothing guarded the second,
which is why this sat green.
Adds the class, and adds the gate that would have caught it: every fixed MEFOR_*
critical secret must be either fingerprinted or explicitly excused WITH its
reason. Three mutations, each red for its own reason:
- un-fingerprint MEFOR_AI_API_KEY (recreates the live gap) -> named as
"neither fingerprinted nor excused"
- fingerprint a name the registry does not know -> named as unregistered, so the
alert cannot cite a cadence that does not exist
- park a real secret in the excuse list while also tracking it -> "both excused
AND fingerprinted -- the excuse is false"
The five exclusions are recorded with reasons rather than left as absences,
because each is a decision a later reader would otherwise re-litigate:
- MEFOR_STORE_ENCRYPTION_KEY: the DEK has its own arm (_maybe_escalate_dek),
which reasons over the wrapped key's age, not an env fingerprint. Listing it
here as well would double-count it.
- MEFOR_STORE_ENCRYPTION_KEYS_RETIRED: a decrypt-only tail. Rotating it is
meaningless -- it exists so old ciphertext stays readable -- and flagging it
"due" would tell an operator to destroy their own recovery path.
- MEFOR_STORE_TRANSIT_KEY / _AUDIT_KEY / MEFOR_STORE_VAULT_TRANSIT_KEY: Vault
Transit key NAMES, not secret values. The secret lives in Vault and rotates
there; the engine holds only the label.
- MEFOR_PFX_PASSWORD: a one-shot passphrase for the `cert import` CLI. The
running service never holds it, so there is nothing to fingerprint.
This does NOT move ASVS 13.3.4. That cell is Partial because the enforce arm
only alerts where the requirement says EXPIRE -- _maybe_escalate_dek emits
secret_rotation_due(enforced=True) past the grace window and nothing expires or
refuses. This is coverage parity within the detect-and-remind half. Anyone
re-scoring 13.3.4 on the strength of this commit is wrong.
Second-order note kept honest: under vault_transit no non-DEK class is
fingerprinted at all (crypto_transit returns no MAC key), so on that provider
this new class is inert along with the rest.
main moved from c66f6be to 3ecceb0 while this branch was in flight, and 3ecceb0 (ADR 0153 -- no data label may allow a cleartext hop, plus the shipped-posture default flips) touches the same three transports this branch does. Merging rather than leaving it stale: branch protection is strict:false, so a stale green would merge without revalidation. Three conflicts, all collisions rather than disagreements -- the two change sets are orthogonal (cleartext-hop posture vs outbound length bounds): - smart.py, http_auth.py: import-block collisions in the shared `from messagefoundry.transports.rest import (...)`. Both sides additive; kept both, sorted. - fhir.py: ADR 0153 rewrote the refuse_cleartext_egress call in FhirLookupExecutor's per-connection loop from one line to a six-argument form. My enforce_outbound_length_limits call sat immediately after it and is unaffected by the rewrite, so main's version of the call is taken verbatim and the length gate follows it unchanged. Verified against the merged tree, not assumed: - ruff format / ruff check / mypy strict: clean. - 1261 tests across every suite touching the merged regions. - BOTH mutation harnesses re-run: all 8 ASVS 4.2.5 mutations and all 6 ASVS 9.2.2 mutations still red for their own reason. That is the check that matters -- a merge can leave tests passing while silently disarming the guard they cover, and reverting each fix to confirm it still reds is the only way to know it did not. Recorded, not resolved: ADR 0153 is a default-flip wave on the cleartext-hop decision, i.e. exactly the lever that moves ASVS cells, and its effect on the open Partial set has NOT been assessed. The correction doc's 237/47 is anchored at c66f6be and is a floor for those cells, not a verdict. Noted in docs/security/ASVS-L3-RESCORE-CORRECTION-2026-07-28.md section 8 with a candidate cell list for the re-score writer.
…(ASVS 15.2.4)
Six shipped sites told users to install the web console in a way that either
fails outright or resolves a distribution nobody has registered.
The one that matters is the second shape. `messagefoundry-webconsole` has never
been published, so the name is UNCLAIMED. An install instruction naming an
unclaimed distribution is a dependency-confusion primitive: whoever registers it
on PyPI first gets code executed at install time -- an sdist runs its build
backend during `pip install` -- on every user who follows OUR OWN documentation,
before any engine process exists and therefore beyond the reach of every runtime
control in the product. README.md:105 is the front-page quick-start, so that is
the first install command a new user meets.
The other shape is simply broken: api/app.py's serve_ui RuntimeError told the
operator to run `pip install messagefoundry[webconsole]`, an extra pyproject
DELIBERATELY withholds until the wheel is published (see the note beside
[project.optional-dependencies]). Shipped code, not docs -- the operator hits it
at startup and the remedy it prints does not work.
Corrected to the path install, which resolves no index and is what CI has always
used: README.md, docs/INSTALL-GUIDE.md, docs/SERVICE.md, docs/USER-GUIDE.md,
docs/MENTAL-MODEL.md, packaging/messagefoundry-webconsole/README.md, and the
runtime error in api/app.py.
The guard derives both properties from source rather than hardcoding them:
- every extra named in shipped text must exist in [project.optional-dependencies]
- no unpublished distribution may appear in an INDEX-resolving install command
The path-vs-index distinction is the whole value, so it is pinned in both
directions by a no-I/O parametrized test: `-e packaging/...` stays green,
`pip install messagefoundry-webconsole` reds. A detector that flagged both would
be turned off within a week; one that flagged neither would be decorative.
Writing the guard immediately found two sites the manual sweep had missed --
README.md:105 and packaging/.../README.md:19 -- plus, pleasingly, my own first
correction note, which explained the hazard using a literal pasteable copy of
the bad command. Three mutations verified red: restore the non-existent extra in
shipped code; restore the index install in README; restore it in INSTALL-GUIDE.
Two exclusions, each recorded rather than silent: f-string `[{extra}]`
placeholders name no extra at rest (excluded by regex shape, not an allow-list
needing upkeep), and docs/BACKLOG.md is a historical ledger -- it records what
past items PROPOSED, including a `[console]` extra never declared, and rewriting
history to satisfy a lint would destroy the record.
Scope, stated honestly in the module docstring: this removes OUR contribution to
the risk. It does not remove the risk. Only claiming the name does -- and
reserving it is sufficient, since an empty project cannot be squatted. ASVS
15.2.4 therefore stays Partial until the reservation happens; remove the entry
from _UNPUBLISHED_DISTRIBUTIONS that day and this guard stops flagging index
installs of it.
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.
Closes two ASVS 5.0 L3 cells and fixes three defects found while building them. Every guard here was
mutation-verified: the fix was reverted and the test confirmed to red for its own reason, because a
green dot on a test that would pass either way is what let the original gaps ship.
ASVS 9.2.2 — assert the id_token's declared class (
1ce3a1a1)An access token (
typ: at+jwt, RFC 9068), a back-channel logout token, and an RFC 8417 security eventtoken are minted by the same issuer under the same key as the
id_token. Every key and signaturerung passes on them, so the only thing between an access token and an accepted federated login was
nonce equality.
Three rungs, each with its own closed-set slug so
verify --section federationstill indicts exactlyone rung:
wrong_token_typeat the top of key selection, so a wrong-class token is not audited asunknown_kid. An absenttypis still accepted — RFC 7519 §5.1 makes it advisory and refusingit would lock out conforming IdPs. Normalisation lower-cases before stripping
application/; theother order refuses a legal
Application/JWT.unexpected_events_claimahead of the nonce compare. A logout token carries no nonce, so inladder order it would be refused as
nonce_mismatch— telling the operator the browser bindingfailed when the IdP actually sent the wrong token class.
claim_sub_missing, plusiatmade mandatory. Both are REQUIRED of anid_tokenby OIDC Core 2.Defect fixed in passing:
subwas read with anelse ""fallback, so a token without one mintedFederatedPrincipal(subject="")— and that empty string was written into theauth.login_successaudit as though it were evidence.
ASVS 4.2.5 — bound what actually ships (
17d8c822,8a61e4a4)The 8 KiB bound ran only at connector construction, so it covered only statically configured
values. Three classes are added after it and were unbounded: per-message headers from message
metadata, the per-call URL a FHIR read/write builds, and the server-minted SMART bearer / detached-JWS
headers — stamped in after the point any naive fix would guard.
The failure class is computed, not guessed, because it decides the disposition:
guaranteed-futile loop that also holds the lane).
credential_fault, and the provider is invalidated. Theprovider caches, so without
invalidate()every retry re-sends a byte-identical bad token; withinvalidate()but nocredential_fault, every retry re-signs a client assertion and POSTs the IdP.credential_fault_policyexists to stop exactly that re-auth storm.PHI egress: the message-derived arm emits the class and length only, never the header name.
At construction a name is operator-static; at send time it is derived from a message-metadata key, and
that string reaches
last_error,message_events.detail, and — on theDeliveryErrorarm — thewebhook AlertSink, i.e. off-box.
Also closed, each a distinct gap rather than a missing call:
FhirLookupExecutornever called the gate at all —fhir.py's only call site was thedestination's
__init__, so a lookup's base, its per-call read URL (the most message-derived URLin the engine) and its minted bearer were unmeasured on both sides.
base_urlwhile_target_urlis derived from it 33 lines later and is whatactually ships — an ordering gap.
store/keyprovider_vault.py(the Vault token ridesX-Vault-Tokenon every Transit call),apiclient/client.py, the SMART and OAuth2-CC token endpoints,ai_broker, the webhook sink, andthe REST/FHIR probes.
apiclient's constants are duplicated rather than imported — ADR 0088 keeps that packageengine-free, so the sharing import is the coupling it exists to avoid — and pinned equal by a test
that imports both sides in a test process.
One residual is named rather than closed:
tray/probe.py, documented in its own docstring withthe three conditions that make it proportionate (tokenless, local operator config, stdlib-only per ADR
0113) and the trigger that reopens it.
Rotation coverage (
fdb3f194)MEFOR_AI_API_KEYwas a registered critical secret with a documented rotation cadence, and wasabsent from
_ENV_SECRET_CLASSES— so the watcher never fingerprinted the one credential thedocumentation tells operators to rotate. Enumeration completeness was already guarded; rotation
coverage was not, which is why it sat green.
Adds the class plus the gate: every fixed
MEFOR_*critical secret must be fingerprinted orexplicitly excused with its reason. The five exclusions are recorded rather than left as absences.
This does not move ASVS 13.3.4 — that cell is Partial because the enforce arm alerts where the
requirement says expire.
Guard-the-guard (
54e02725)Every assertion in the threat-model drift suite pins a value that is rendered, so a sentence
asserting a bound's absence was invisible by construction.
THREAT-MODEL.mdclaimed there was noaggregate HL7 escape-expansion budget for the entire life of
peek.enforce_expansion_budget, andre-inserting that exact sentence left all 81 tests green. Two guards added; the shipped symbol is
asserted rather than probed, because a guard that skipped when the control vanished would pass
hardest exactly when the bound disappeared.
Behaviour changes worth a release note
legitimate config is affected (a normal RS256 signature header is 364 chars).
jwttypis refused after upgrade. Blast radius bounded by[auth].oidc_enableddefaulting off.Verification
test_installed_metadata_matches_dunder_version)is a local-venv artifact — this worktree has no
.venv, so the run used a stale0.3.0installagainst
0.3.2source. It passes on a fresh CI install.point of merging rather than deferring: a merge can leave tests passing while silently disarming the
guard they cover.
ruff format/ruff check/mypystrict clean; bandit and the leak gate green with a real tokensource loaded.
Not claimed here
ADR 0153's effect on the ASVS score is unassessed. It is a default-flip wave on the cleartext-hop
decision — the class of change that moves cells — and it names no ASVS cells, so the mapping is real
work. The scorecard correction accompanying this branch is anchored before it and is a floor for
those cells, not a verdict.
🤖 Generated with Claude Code