From 54e0272579410110f6333b831b3d8a9a215ee058 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 28 Jul 2026 21:57:31 -0500 Subject: [PATCH 1/6] test(security): the threat-model guard could not see a bound described 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. --- tests/test_threat_model_doc_drift.py | 62 ++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/tests/test_threat_model_doc_drift.py b/tests/test_threat_model_doc_drift.py index 8e692f70..a29ec9e8 100644 --- a/tests/test_threat_model_doc_drift.py +++ b/tests/test_threat_model_doc_drift.py @@ -874,6 +874,7 @@ def _doc_side_bounds() -> list[tuple[str, object, str]]: ("**HL7 parse**", peek.DEFAULT_MAX_MESSAGE_BYTES // mib, "16 MiB"), ("**HL7 parse**", peek.DEFAULT_MAX_SEGMENTS, "10,000"), ("**HL7 escape expansion**", _builtin_hl7.MAX_ESCAPE_REPEAT, "512"), + ("**HL7 escape expansion**", _builtin_hl7.MAX_COUNTED_ESCAPE_OPENERS, "100,000"), ("**MLLP listener**", mllp.DEFAULT_MAX_CONNECTIONS, "256"), ("**MLLP listener**", int(mllp.DEFAULT_RECEIVE_TIMEOUT), "60"), ("**HTTP inbound listener**", http_listener.DEFAULT_MAX_BODY_BYTES // mib, "16 MiB"), @@ -981,6 +982,67 @@ def test_every_quoted_bound_appears_in_its_own_doc_row(heading: str, which: str) ) +#: Each entry: (import path, dotted symbol, what it implements, phrases the doc must NOT contain). +#: Every guard above pins a value that IS rendered, so all of them are structurally blind to a +#: sentence asserting a bound's ABSENCE. That is not hypothetical: the HL7 escape-expansion row +#: asserted "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 +#: (verified 2026-07-28). Because ASVS 15.1.3 scores the DOCUMENT, the stale sentence was itself the +#: control defect — a false claim about the most attacker-exposed bound in the product. +_ABSENCE_CLAIMS: list[tuple[str, str, str, tuple[str, ...]]] = [ + ( + "messagefoundry.parsing.peek", + "enforce_expansion_budget", + "aggregate HL7 escape-expansion budget (ASVS 1.3.3)", + ( + "no aggregate per-message budget", + "there is no aggregate", + "has no aggregate", + ), + ), +] + + +def test_the_document_does_not_deny_a_bound_the_code_implements() -> None: + """A shipped bound must not be described as absent. + + The symbol's existence is ASSERTED, not merely probed, and that is deliberate. If the guard + skipped when the symbol went missing, deleting the control would silently disable the only test + that notices — the guard would pass hardest exactly when the bound disappeared. Asserting instead + means removing the budget reds this test and forces the author to say so in the document, which is + the outcome the requirement wants either way. + + Scope honesty: this catches a fixed phrase list, not paraphrase. It is a tripwire for the specific + claims we have already seen rot, not a proof that the document contains no false negative claim. + """ + import importlib + + text = _doc_text().lower() + scanned: list[str] = [] + problems: list[str] = [] + for module_path, symbol, description in ((c[0], c[1], c[2]) for c in _ABSENCE_CLAIMS): + module = importlib.import_module(module_path) + assert hasattr(module, symbol), ( + f"{module_path}.{symbol} is gone — it implements {description}. If the bound was removed " + "on purpose, update docs/security/THREAT-MODEL.md to say so and retire this entry; do not " + "delete the assertion and leave the document claiming a control that no longer exists." + ) + scanned.append(f"{module_path}.{symbol}") + for _module_path, symbol, description, phrases in _ABSENCE_CLAIMS: + for phrase in phrases: + if phrase.lower() in text: + problems.append(f"{phrase!r} contradicts the shipped {description} ({symbol})") + # Liveness receipt: name what was examined, so a future reader can tell an empty scan from a + # clean one. A guard that reports "nothing found" without saying what it looked at is the + # failure mode this file exists to prevent. + assert scanned, "the absence-claim table is empty — this guard examined nothing" + assert not problems, ( + f"docs/security/THREAT-MODEL.md denies a bound the code implements: {problems}. Examined " + f"{len(scanned)} shipped control(s): {scanned}. ASVS 15.1.3 scores this document, so a stale " + "claim that a control is missing is a control defect, not a typo." + ) + + def test_every_registered_source_connector_is_covered_or_explicitly_exempt() -> None: """Code-derived completeness for the data plane, the half the anchor list cannot supply. From 1ce3a1a1206380a0cae0605b49cfbf6f7cd7b5cf Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 06:13:38 -0500 Subject: [PATCH 2/6] feat(auth): assert the id_token's declared class, not just its signature (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. --- docs/SECURITY.md | 3 +- ...ode-pkce-relying-party-hybrid-ad-backed.md | 20 ++- messagefoundry/auth/oidc/claims.py | 84 ++++++++++-- messagefoundry/verify/federation.py | 21 ++- tests/test_auth_oidc.py | 129 ++++++++++++++++++ tests/test_security_doc_drift.py | 2 +- tests/test_verify_federation.py | 7 + 7 files changed, 245 insertions(+), 21 deletions(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index c5879ce8..7252140b 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -1084,6 +1084,7 @@ one-to-one — that is why the bind/exposure posture occupies two rows and the A | Gated operation × requester-vs-approver identity × hold age | the pending-approval record: the operation name, the requesting identity, and the hold's creation time | `[approvals].enabled` **and** the operation is in `[approvals].operations` and has no approved unexpired release; the approver is the requester; the hold is older than `expiry_hours` | **DENY** the immediate execution — **202** hold + `approval.requested` audit; **403** on self-approval; **409** once expired or already decided | off; `['connection_purge','dead_letter_replay']`; 72 h | `[approvals].enabled`, `operations`, `expiry_hours` | | mTLS client-certificate subject | the qualified subject-RDN / SAN names of a **verified** peer certificate | exact match against a deny-by-default map (empty map = feature off) | **ALLOW** — resolve to that principal's Identity (RBAC then authorizes); a disabled account grants none | `{}` = off | `[api].tls_client_cert_identities` (requires `tls_client_ca_file`) | | Operator-listener peer client certificate | the TLS peer certificate presented at the API / `/ui` handshake | `[api].tls_client_ca_file` set (requires `tls_cert_file`) → `ssl.CERT_REQUIRED` plus strict RFC 5280 verify flags (`api/tls.py:47-50`); no client certificate, or one not issued by that CA | **DENY** — the TLS handshake fails, so the request never reaches the ASGI stack at all: no middleware runs, no route matches, no identity is resolved, and no 403 body is produced | unset = off (server-only TLS, no peer-certificate decision on the control plane) | `[api].tls_client_ca_file` | +| Declared token class of a federated assertion | the `typ` JOSE header, and the presence of an `events` claim, on a **signature-verified** JWS | `typ` present and — normalised `.strip().lower()` then `application/`-stripped — not `jwt`, so `at+jwt` (RFC 9068 access token), `logout+jwt` and `secevent+jwt` are refused while an **absent** `typ` is allowed (RFC 7519 §5.1 makes the header advisory); or the claim set carries `events`, i.e. an RFC 8417 security event token. Every such token is minted by the **same issuer under the same key**, so no signature or key rung distinguishes it | **DENY** the sign-in — `ClaimsError("wrong_token_type")` at the key-selection rung, `ClaimsError("unexpected_events_claim")` ahead of the nonce compare (a logout token carries no nonce, so a later check would misreport it as a browser-binding failure) | on | (no knob) | | Federated authentication-context claims (`amr` / `acr`) | the `amr` list / `acr` string of a **signature-verified** `id_token` | `oidc_require_mfa_claim` on **and** neither an `amr` value in `[auth].oidc_mfa_amr_values` (default `["mfa"]`) nor an `acr` in `oidc_required_acr_values` (default `[]`, so the `amr` arm alone decides) | **DENY** the sign-in — `ClaimsError("mfa_claim_missing")`. An IdP **assertion**, never a proof | on, `["mfa"]` / `[]` | `[auth].oidc_require_mfa_claim`, `oidc_mfa_amr_values`, `oidc_required_acr_values` | | UPN suffix of the federated username claim | the suffix after the FIRST `@` of the username claim | `oidc_username_strip_domain` on (default) **and** the suffix is not in `oidc_allowed_username_domains` (or `[auth].ad_domain`). With stripping **off** the claim is used verbatim and no suffix check runs | **DENY** the sign-in — `ClaimsError("username_domain_not_allowed")` | on | `[auth].oidc_allowed_username_domains`, `oidc_username_strip_domain` | | Bootstrap-admin age × admin population | `users.created_at` for the built-in bootstrap account × whether a second enabled Administrator exists | still unclaimed (`must_change_password` set) **and** (`now ≥ created_at + bootstrap_expiry_hours × 3600` **or** another enabled admin exists); `0` = no time expiry | **DENY** — the account is disabled, **all** its sessions revoked, `auth.bootstrap_admin_retired` audited. A *claimed* (password-changed) bootstrap account is never touched | 72 h | `[auth].bootstrap_expiry_hours` | @@ -1398,7 +1399,7 @@ Comparative properties on the dimensions the table's four columns cannot carry: | **Local** | passkeys only (WebAuthn origin-bound, `attestation=none`, `user_verification=preferred`); password/TOTP are phishable | TOTP is single-use per 30 s step (`totp_skew_steps` default `0`); recovery codes single-use; passkey challenges are 64-byte CSPRNG, single-use, 120 s TTL, with a strict sign-counter compare-and-set | argon2id password hash (t=3, m=64 MiB, p=4); TOTP secret **cipher-encrypted**; recovery codes argon2id-hashed; COSE public keys **plaintext by design** | built (TOTP + passkeys) | disable the account or revoke sessions — immediate | | **AD** | none | none beyond TLS | **none** — only the service-account bind password (env or a `[secrets]` reference, fail-closed) | delegated and **unverifiable** — the simple-bind leg issues MFA-satisfied under a signed relaxation; the engine receives no evidence (contrast OIDC's signature-verified `amr`/`acr`, which is enforced) | disabling in AD does **not** end a live session; `[auth].ad_session_recheck_seconds` (default **0 = off**) closes it, bounded by interval × strikes | | **Kerberos** | none (single-leg, no channel binding) | ticket lifetime is the domain's | **none** — the acceptor keytab/SPN is OS-owned | delegated, unverifiable | as AD | -| **OIDC** | the IdP's, not the engine's | strongest of the four: server-side PKCE verifier + `state` (constant-time compare) + `nonce`, single-use flow, a `__Host-`-prefixed browser-binding cookie the callback requires, and a kid/alg/signature/`iss`/`aud`/`exp`/`iat`/`nbf` ladder under a bounded clock skew | **none** — only the confidential-client secret (env-only or a `[secrets]` reference, resolved eagerly at startup) | asserted via `amr`/`acr` **and enforced** — with `[auth].oidc_require_mfa_claim` on (default) a token carrying no configured `amr`/`acr` is refused at claims validation, and only then is the session minted MFA-verified; switch it off and the federated session is minted **un**verified, which `mfa_satisfied` refuses. This is the one directory leg whose factor the engine actually verifies | as AD, plus the `id_token.exp` cap; no refresh tokens and no RP-initiated logout | +| **OIDC** | the IdP's, not the engine's | strongest of the four: server-side PKCE verifier + `state` (constant-time compare) + `nonce`, single-use flow, a `__Host-`-prefixed browser-binding cookie the callback requires, and a `typ`/kid/alg/signature/`events`/`iss`/`aud`/`exp`/`iat`/`nbf`/`nonce`/`sub` ladder under a bounded clock skew — `typ` and `events` assert the token **class** (an access token or a logout token carries the same issuer and key), and `sub`/`iat` are required rather than optional | **none** — only the confidential-client secret (env-only or a `[secrets]` reference, resolved eagerly at startup) | asserted via `amr`/`acr` **and enforced** — with `[auth].oidc_require_mfa_claim` on (default) a token carrying no configured `amr`/`acr` is refused at claims validation, and only then is the session minted MFA-verified; switch it off and the federated session is minted **un**verified, which `mfa_satisfied` refuses. This is the one directory leg whose factor the engine actually verifies | as AD, plus the `id_token.exp` cap; no refresh tokens and no RP-initiated logout | | **mTLS** | n/a (no interactive ceremony) | n/a | **none** — the engine holds only the pinned client CA and the name map | none, structurally | **no revocation checking** — `VERIFY_X509_STRICT` is strict path validation, not OCSP/CRL; live revocation is the org's PKI. Engine-side: remove the allow-list entry (config change → restart) or disable the mapped account | **Where each pathway is enforced, and what turns it on:** Local → `POST /auth/login` + `POST /ui/login` diff --git a/docs/adr/0142-federated-sso-oidc-authorization-code-pkce-relying-party-hybrid-ad-backed.md b/docs/adr/0142-federated-sso-oidc-authorization-code-pkce-relying-party-hybrid-ad-backed.md index 33f16412..7972d0ca 100644 --- a/docs/adr/0142-federated-sso-oidc-authorization-code-pkce-relying-party-hybrid-ad-backed.md +++ b/docs/adr/0142-federated-sso-oidc-authorization-code-pkce-relying-party-hybrid-ad-backed.md @@ -143,10 +143,24 @@ Server-side `state` is a CSRF/mix-up defence, **not** a browser binding — whoe - **AC-2** — WHEN a federated login completes, THE SYSTEM SHALL resolve roles from on-prem AD via `resolve_principal`, never from a token claim. → `tests/test_auth_oidc_service.py` -- **AC-3** — IF the `id_token` fails any verification rung (signature, `iss`, `aud`/`azp`, `exp`/`iat` - skew, `nonce`, `kid` unknown/ambiguous, key below the floor), THEN THE SYSTEM SHALL refuse the login, - mint no session, and audit a closed-set reason slug. +- **AC-3** — IF the `id_token` fails any verification rung (declared token class via `typ`/`events`, + signature, `iss`, `aud`/`azp`, `exp`/`iat` skew, required `sub`, `nonce`, `kid` unknown/ambiguous, + key below the floor), THEN THE SYSTEM SHALL refuse the login, mint no session, and audit a + closed-set reason slug. → `tests/test_auth_oidc.py` + + *Amended 2026-07-28 (ASVS 9.2.2).* The class rungs were added because an access token + (`typ: at+jwt`), 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 signature and key + rung passes on them, so without an explicit class assertion the only thing between an access token + and an accepted login is nonce equality. An **absent** `typ` is still accepted — RFC 7519 §5.1 + makes the header advisory and refusing it would lock out conforming IdPs. + + > **Forward note for cell 10.5.5 (back-channel logout receiver).** A logout token legitimately + > carries `events` and no `nonce`, so `validate_id_token` will refuse it — correctly. Build that + > receiver its **own** logout-token ladder (OIDC Back-Channel Logout §2.4: require `events`, forbid + > `nonce`, require `sid`/`sub`). Do **not** relax `unexpected_events_claim` to reuse this function: + > that silently reopens ASVS 9.2.2 and no test outside this ADR note would catch it. - **AC-4** — IF the protected header declares `alg` outside the configured allow-list, THEN THE SYSTEM SHALL raise before any signature computation (`none` and `HS*` are unrepresentable in `SignatureAlgorithm`). diff --git a/messagefoundry/auth/oidc/claims.py b/messagefoundry/auth/oidc/claims.py index 023d4654..a9b31180 100644 --- a/messagefoundry/auth/oidc/claims.py +++ b/messagefoundry/auth/oidc/claims.py @@ -37,6 +37,9 @@ "malformed_token", "malformed_payload", "claim_not_numeric", + "claim_sub_missing", + "wrong_token_type", + "unexpected_events_claim", "unknown_kid", "ambiguous_kid", "key_rejected", @@ -100,6 +103,37 @@ class FederatedPrincipal: expires_at: float +#: The ``typ`` values an ``id_token`` may declare, after normalisation. RFC 7519 §5.1 makes the header +#: advisory and OIDC Core does not mandate it, so an ABSENT ``typ`` is accepted — refusing it would +#: lock out conforming IdPs that omit it. +_ID_TOKEN_TYPS: frozenset[str] = frozenset({"jwt"}) + + +def _assert_id_token_typ(header: Mapping[str, object]) -> None: + """Refuse a JWS whose header DECLARES a class other than ``id_token`` (ASVS 9.2.2). + + The tokens this excludes — an access token (``at+jwt``, RFC 9068), a back-channel logout token + (``logout+jwt``), a security event token (``secevent+jwt``) — are minted by the **same issuer + under the same key**, so every check downstream of key selection passes on them. Without this the + only thing between an access token and an accepted federated login is nonce equality. + + Normalisation is ``.strip().lower().removeprefix("application/")``, lower-casing **before** the + prefix strip: RFC 7515 §4.1.9 lets the ``application/`` prefix be omitted and media types are + case-insensitive, so ``Application/JWT`` is a legal spelling of ``jwt``. Stripping first would + leave ``Application/JWT`` un-normalised and refuse a conforming token. + + Called at the TOP of key selection, before the kid guard and the alg pin, so a wrong-class token + is audited as ``wrong_token_type`` rather than as whichever unrelated rung it happens to trip. + """ + typ = header.get("typ") + if typ is None: + return + if not isinstance(typ, str): + raise ClaimsError("wrong_token_type", "id_token header typ is not a string") + if typ.strip().lower().removeprefix("application/") not in _ID_TOKEN_TYPS: + raise ClaimsError("wrong_token_type", f"id_token header declares typ {typ!r}") + + def _select_key_and_alg( id_token: str, policy: OidcClaimPolicy, jwks: JwksCache ) -> tuple[object, SignatureAlgorithm]: @@ -107,6 +141,7 @@ def _select_key_and_alg( header = unverified_jws_header(id_token) except SigningError as exc: raise ClaimsError("malformed_token", str(exc)) from exc + _assert_id_token_typ(header) kid = header.get("kid") if not isinstance(kid, str) or kid == "": raise ClaimsError("unknown_kid", "id_token header carries no kid") @@ -146,13 +181,26 @@ def _verify_signature(id_token: str, key: object, policy: OidcClaimPolicy) -> Ma raise ClaimsError("malformed_payload", str(exc)) from exc -def _check_core_claims(claims: Mapping[str, object], policy: OidcClaimPolicy, now: float) -> float: - """Walk the core OIDC claim checks and **return the verified ``exp``** (ADR 0142 AC-6). +def _check_core_claims( + claims: Mapping[str, object], policy: OidcClaimPolicy, now: float +) -> tuple[float, str]: + """Walk the core OIDC claim checks and **return the verified ``(exp, sub)``** (ADR 0142 AC-6). - The `exp` is returned rather than discarded so the session cap has a second operand that came - from the *signature-verified* claims. A caller must never re-parse the token to recover it — that - is the second-read bug class ``verify_compact_jws`` exists to foreclose. + Both are returned rather than discarded so the session cap and the audit subject have operands + that came from the *signature-verified* claims. A caller must never re-parse the token to recover + either — that is the second-read bug class ``verify_compact_jws`` exists to foreclose. """ + # ASVS 9.2.2, ahead of every other claim check. A claim set carrying ``events`` is a Security + # Event Token (RFC 8417) — a back-channel logout token or similar — not an ``id_token``, and the + # two must not be interchangeable. This runs FIRST because a logout token carries no ``nonce``: + # checked in ladder order it would be refused as ``nonce_mismatch``, which indicts the browser + # binding and tells the operator the wrong thing about why the login failed. + if "events" in claims: + raise ClaimsError( + "unexpected_events_claim", + "claim set carries an events claim (RFC 8417 SET, not an id_token)", + ) + if claims.get("iss") != policy.issuer: raise ClaimsError("claim_iss", "iss does not match the pinned issuer") @@ -168,10 +216,13 @@ def _check_core_claims(claims: Mapping[str, object], policy: OidcClaimPolicy, no exp = _require_number(claims, "exp", "expired") if now > exp + skew: raise ClaimsError("expired", "id_token exp is in the past") - if "iat" in claims: - iat = _require_number(claims, "iat", "issued_in_future") - if iat > now + skew: - raise ClaimsError("issued_in_future", "id_token iat is in the future") + # ``iat`` is REQUIRED of an id_token by OIDC Core 2, so it is read unconditionally rather than + # behind an `if "iat" in claims` guard. A token omitting it is not an id_token; the omission + # surfaces as ``claim_not_numeric`` at this rung, the same slug a non-numeric ``iat`` already + # raised, so no new reason enters the closed set. + iat = _require_number(claims, "iat", "issued_in_future") + if iat > now + skew: + raise ClaimsError("issued_in_future", "id_token iat is in the future") if "nbf" in claims: nbf = _require_number(claims, "nbf", "not_yet_valid") if now + skew < nbf: @@ -181,7 +232,15 @@ def _check_core_claims(claims: Mapping[str, object], policy: OidcClaimPolicy, no if not isinstance(token_nonce, str) or not hmac.compare_digest(token_nonce, policy.nonce): raise ClaimsError("nonce_mismatch", "id_token nonce does not match the flow nonce") - return exp + # ``sub`` is REQUIRED of an id_token by OIDC Core 2 and is the only stable identifier the + # assertion carries. It was previously read with an `else ""` fallback at construction, so a + # token without one minted a principal whose subject was the empty string — and that empty + # subject was written into the ``auth.login_success`` audit as if it were evidence. + subject = claims.get("sub") + if not isinstance(subject, str) or subject == "": + raise ClaimsError("claim_sub_missing", "id_token carries no usable sub claim") + + return exp, subject def _require_number(claims: Mapping[str, object], field_name: str, _reason: str) -> float: @@ -272,14 +331,13 @@ def validate_id_token( """ key, _alg = _select_key_and_alg(id_token, policy, jwks) claims = _verify_signature(id_token, key, policy) - expires_at = _check_core_claims(claims, policy, clock()) + expires_at, subject = _check_core_claims(claims, policy, clock()) amr, acr = _check_mfa_gate(claims, policy) username = _resolve_username(claims, policy) - subject = claims.get("sub") return FederatedPrincipal( username=username, - subject=subject if isinstance(subject, str) else "", + subject=subject, amr=amr, acr=acr, expires_at=expires_at, diff --git a/messagefoundry/verify/federation.py b/messagefoundry/verify/federation.py index f435b022..e9ee780c 100644 --- a/messagefoundry/verify/federation.py +++ b/messagefoundry/verify/federation.py @@ -33,8 +33,18 @@ _RUNGS: tuple[tuple[str, str, frozenset[str]], ...] = ( ( "fed.replay.key", - "id_token key selection (kid -> JWKS)", - frozenset({"malformed_token", "unknown_kid", "ambiguous_kid", "key_rejected"}), + "id_token type + key selection (typ -> kid -> JWKS)", + frozenset( + { + "malformed_token", + # The typ assertion runs at the top of key selection, so a token declaring a + # non-id_token class indicts THIS rung and no other (ASVS 9.2.2). + "wrong_token_type", + "unknown_kid", + "ambiguous_kid", + "key_rejected", + } + ), ), ( "fed.replay.signature", @@ -43,14 +53,19 @@ ), ( "fed.replay.claims", - "iss / aud / azp / exp / iat / nbf", + "token class (events) / iss / aud / azp / exp / iat / nbf / sub", frozenset( { + # An ``events`` claim means an RFC 8417 SET, not an id_token. Checked at the TOP of + # this rung, before the nonce compare, so a logout token is not misreported as a + # browser-binding failure (ASVS 9.2.2). + "unexpected_events_claim", "claim_iss", "claim_aud", "claim_azp", "expired", "claim_not_numeric", + "claim_sub_missing", "not_yet_valid", "issued_in_future", } diff --git a/tests/test_auth_oidc.py b/tests/test_auth_oidc.py index 54daf607..4447d313 100644 --- a/tests/test_auth_oidc.py +++ b/tests/test_auth_oidc.py @@ -330,6 +330,135 @@ def test_every_reason_slug_is_declared() -> None: claims_mod.ClaimsError("not_a_real_reason") +# --- claims: token-class assertion (ASVS 9.2.2) ---------------------------------------------------- + +#: Sentinel for "mint this token with NO typ header at all" — distinct from ``typ: null``. +_OMIT_TYP = object() + + +def _mint_with_typ( + key: rsa.RSAPrivateKey, kid: str, claims: Mapping[str, Any], typ: Any = _OMIT_TYP +) -> str: + """Mint a **properly signed** compact JWS carrying an arbitrary ``typ`` header. + + ``CompactJwtSigner.sign`` hardcodes ``{"alg": ..., "typ": "JWT"}``, so the shipped signer cannot + express the wrong-class token this rung exists to refuse. The signature below is computed over + the real header bytes, so a refusal proves the **typ assertion** fired — not that a retargeted or + hand-spliced signature failed to verify, which would make every test here pass for the wrong + reason and stay green if the assertion were deleted. + """ + from messagefoundry.transports.signing import _b64u_encode, _sign + + header: dict[str, Any] = {"alg": SignatureAlgorithm.RS256.value, "kid": kid} + if typ is not _OMIT_TYP: + header["typ"] = typ + header_b64 = _b64u_encode(json.dumps(header, separators=(",", ":"), sort_keys=True).encode()) + claims_b64 = _b64u_encode( + json.dumps(dict(claims), separators=(",", ":"), sort_keys=True).encode() + ) + signature = _sign(key, SignatureAlgorithm.RS256, f"{header_b64}.{claims_b64}".encode("ascii")) + return f"{header_b64}.{claims_b64}.{_b64u_encode(signature)}" + + +def test_the_hand_mint_helper_produces_a_genuinely_valid_token(rsa_key: rsa.RSAPrivateKey) -> None: + """Guard-the-guard: without this, every ``_mint_with_typ`` test could be passing because the + helper mints garbage rather than because the assertion under test fired.""" + jws = _mint_with_typ(rsa_key, "k1", _good_claims(), typ="JWT") + principal = oidc.validate_id_token(jws, _policy(), _cache_for(rsa_key), clock=lambda: 1_000_100) + assert principal.username == "jdoe" + + +@pytest.mark.parametrize( + "typ", + [ + "at+jwt", # RFC 9068 access token — same issuer, same key, passes every other rung + "application/at+jwt", # the RFC 7515 §4.1.9 long form of the same + "logout+jwt", # OIDC back-channel logout token + "secevent+jwt", # RFC 8417 security event token + "JOSE", + "", # declared-but-empty is a declaration, not an absence + 123, # non-string + ], +) +def test_a_declared_non_id_token_typ_is_refused(rsa_key: rsa.RSAPrivateKey, typ: Any) -> None: + jws = _mint_with_typ(rsa_key, "k1", _good_claims(), typ=typ) + with pytest.raises(oidc.ClaimsError) as exc: + oidc.validate_id_token(jws, _policy(), _cache_for(rsa_key), clock=lambda: 1_000_100) + assert exc.value.reason == "wrong_token_type" + + +def test_an_absent_typ_still_validates(rsa_key: rsa.RSAPrivateKey) -> None: + """RFC 7519 §5.1 makes ``typ`` advisory and OIDC Core does not mandate it, so a conforming IdP + that omits it must still authenticate. Making ``typ`` mandatory is a federation outage.""" + jws = _mint_with_typ(rsa_key, "k1", _good_claims()) + principal = oidc.validate_id_token(jws, _policy(), _cache_for(rsa_key), clock=lambda: 1_000_100) + assert principal.username == "jdoe" + + +@pytest.mark.parametrize("typ", ["JWT", "jwt", "application/jwt", "Application/JWT", " JWT "]) +def test_a_media_type_prefixed_or_differently_cased_typ_still_validates( + rsa_key: rsa.RSAPrivateKey, typ: str +) -> None: + """``Application/JWT`` is the case that pins the normalisation ORDER: lower-case must happen + BEFORE ``removeprefix("application/")``. Swap the two and this value alone still carries the + prefix, so a conforming token is refused — the other four spellings pass either way.""" + jws = _mint_with_typ(rsa_key, "k1", _good_claims(), typ=typ) + principal = oidc.validate_id_token(jws, _policy(), _cache_for(rsa_key), clock=lambda: 1_000_100) + assert principal.username == "jdoe" + + +def test_an_events_bearing_claim_set_is_refused_under_its_own_slug( + rsa_key: rsa.RSAPrivateKey, +) -> None: + """A Security Event Token (RFC 8417) is not an id_token even when the issuer and key match.""" + jws = _mint( + rsa_key, + "k1", + _good_claims(events={"http://schemas.openid.net/event/backchannel-logout": {}}), + ) + with pytest.raises(oidc.ClaimsError) as exc: + oidc.validate_id_token(jws, _policy(), _cache_for(rsa_key), clock=lambda: 1_000_100) + assert exc.value.reason == "unexpected_events_claim" + + +def test_the_events_rejection_precedes_the_nonce_rung(rsa_key: rsa.RSAPrivateKey) -> None: + """A real back-channel logout token carries ``events`` and NO ``nonce``. If the events check ran + in ladder order it would be refused as ``nonce_mismatch`` — indicting the browser binding and + telling the operator the wrong thing about why the login failed.""" + logout_ish = _good_claims(events={"http://schemas.openid.net/event/backchannel-logout": {}}) + del logout_ish["nonce"] + jws = _mint(rsa_key, "k1", logout_ish) + with pytest.raises(oidc.ClaimsError) as exc: + oidc.validate_id_token(jws, _policy(), _cache_for(rsa_key), clock=lambda: 1_000_100) + assert exc.value.reason == "unexpected_events_claim" + + +@pytest.mark.parametrize( + ("drop", "reason"), + [("sub", "claim_sub_missing"), ("iat", "claim_not_numeric")], +) +def test_a_token_missing_a_required_id_token_claim_is_refused( + rsa_key: rsa.RSAPrivateKey, drop: str, reason: str +) -> None: + """The positive arm. ``sub`` and ``iat`` are REQUIRED of an id_token by OIDC Core 2. Before this, + a token with no ``sub`` minted ``FederatedPrincipal(subject="")`` and that empty string was + written into the ``auth.login_success`` audit as though it were evidence.""" + without = _good_claims() + del without[drop] + jws = _mint(rsa_key, "k1", without) + with pytest.raises(oidc.ClaimsError) as exc: + oidc.validate_id_token(jws, _policy(), _cache_for(rsa_key), clock=lambda: 1_000_100) + assert exc.value.reason == reason + + +@pytest.mark.parametrize("bad_sub", ["", 12345, None]) +def test_a_non_string_or_empty_sub_is_refused(rsa_key: rsa.RSAPrivateKey, bad_sub: Any) -> None: + jws = _mint(rsa_key, "k1", _good_claims(sub=bad_sub)) + with pytest.raises(oidc.ClaimsError) as exc: + oidc.validate_id_token(jws, _policy(), _cache_for(rsa_key), clock=lambda: 1_000_100) + assert exc.value.reason == "claim_sub_missing" + + # --- flow: PKCE + the flow cache ------------------------------------------------------------------- diff --git a/tests/test_security_doc_drift.py b/tests/test_security_doc_drift.py index 4be2fbca..f4b0eae0 100644 --- a/tests/test_security_doc_drift.py +++ b/tests/test_security_doc_drift.py @@ -447,7 +447,7 @@ #: Body-row counts of the two decision tables. Row-scoping alone cannot catch the deletion of a row #: whose tokens are shared with a sibling row (Sec-Fetch, bind/exposure, the DICOM construction #: gate), so the counts are pinned too: removing ANY row reds CI. -_CONTEXT_TABLE_A_ROWS = 34 +_CONTEXT_TABLE_A_ROWS = 35 _CONTEXT_TABLE_B_ROWS = 9 #: The closed action vocabulary the section declares. Every Action cell in BOTH tables must OPEN with diff --git a/tests/test_verify_federation.py b/tests/test_verify_federation.py index 1bc381c5..bfdd69f5 100644 --- a/tests/test_verify_federation.py +++ b/tests/test_verify_federation.py @@ -225,6 +225,13 @@ def test_without_a_nonce_the_binding_rung_skips_and_later_rungs_are_not_claimed_ [ ({"iss": "https://evil.example"}, "fed.replay.claims"), ({"aud": "someone-else"}, "fed.replay.claims"), + # ASVS 9.2.2: an RFC 8417 SET verifies its signature and passes key selection, so the report + # must indict the CLAIMS rung. Mapped to the signature rung instead, `verify` would tell the + # operator the token was forged. + ( + {"events": {"http://schemas.openid.net/event/backchannel-logout": {}}}, + "fed.replay.claims", + ), ({"amr": ["pwd"]}, "fed.replay.mfa"), ({"preferred_username": "Administrator@attacker.example"}, "fed.replay.username"), ], From 17d8c8229d012441fb4b389b2df37e4fa76b1859 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 06:39:29 -0500 Subject: [PATCH 3/6] feat(transports): bound what actually ships, not what was configured (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. --- messagefoundry/apiclient/client.py | 25 +++ messagefoundry/store/keyprovider_vault.py | 9 + messagefoundry/transports/dicomweb.py | 4 + messagefoundry/transports/fhir.py | 58 +++++++ messagefoundry/transports/http_auth.py | 5 + messagefoundry/transports/rest.py | 196 +++++++++++++++++++++- messagefoundry/transports/smart.py | 6 + messagefoundry/transports/soap.py | 23 +++ tests/test_apiclient.py | 39 +++++ tests/test_fhir_lookup.py | 26 +++ tests/test_rest_transport.py | 156 +++++++++++++++++ 11 files changed, 538 insertions(+), 9 deletions(-) diff --git a/messagefoundry/apiclient/client.py b/messagefoundry/apiclient/client.py index 3303d520..bb06a032 100644 --- a/messagefoundry/apiclient/client.py +++ b/messagefoundry/apiclient/client.py @@ -72,6 +72,15 @@ _log = logging.getLogger(__name__) _LOOPBACK_HOSTS = {"127.0.0.1", "localhost", "::1"} +# ASVS 4.2.5, the client half. Deliberately DUPLICATED from transports/rest.py's +# MAX_OUTBOUND_URL_LEN / MAX_OUTBOUND_HEADER_VALUE_LEN rather than imported: ADR 0088 makes this +# package Qt-free AND engine-free, so a GUI or harness process can depend on it without dragging in +# transports/. Importing the constants would reintroduce exactly that coupling. The values are pinned +# equal by test_apiclient_length_bounds_match_the_transport_constants, which imports both sides in a +# TEST process (where the coupling is harmless) and fails if either drifts. +MAX_REQUEST_URL_LEN = 8192 +MAX_REQUEST_HEADER_VALUE_LEN = 8192 + class ApiError(RuntimeError): """An API call failed (transport error, a non-2xx response, or an undecodable 2xx body).""" @@ -273,6 +282,22 @@ def _request( **kw: object, ) -> httpx.Response: headers = {"Authorization": f"Bearer {self._token}"} if self._token else None + # ASVS 4.2.5: bound the request line and the bearer this client emits. The limits are + # DUPLICATED from transports/rest.py rather than imported: ADR 0088 makes this package + # engine-free (a GUI/harness process must not pull transports/ in), so the import that would + # share them is exactly the coupling this package exists to avoid. Kept in step by + # ``test_apiclient_length_bounds_match_the_transport_constants``. + if len(self.base_url) + len(path) > MAX_REQUEST_URL_LEN: + raise ApiError( + f"request URL is {len(self.base_url) + len(path)} chars, over the " + f"{MAX_REQUEST_URL_LEN}-char limit" + ) + if headers is not None and len(headers["Authorization"]) > MAX_REQUEST_HEADER_VALUE_LEN: + # Never echo the value: it is a live session bearer. + raise ApiError( + f"the session Authorization header is {len(headers['Authorization'])} chars, over " + f"the {MAX_REQUEST_HEADER_VALUE_LEN}-char limit" + ) try: response = self._http.request(method, path, headers=headers, **kw) # type: ignore[arg-type] except httpx.HTTPError as exc: diff --git a/messagefoundry/store/keyprovider_vault.py b/messagefoundry/store/keyprovider_vault.py index e393f05d..fcd89baa 100644 --- a/messagefoundry/store/keyprovider_vault.py +++ b/messagefoundry/store/keyprovider_vault.py @@ -75,6 +75,15 @@ def _build_client(addr: str | None, token: str | None) -> Any: a live Vault. ``addr``/``token`` are passed through; when ``None``, hvac falls back to its own ``VAULT_ADDR``/``VAULT_TOKEN`` environment conventions.""" hvac = _import_hvac() + # ASVS 4.2.5: ``token`` ships as an ``X-Vault-Token`` request header on EVERY Transit + # encrypt/decrypt/HMAC call, and ``addr`` becomes the request URL -- both from MEFOR_* env, and + # neither was ever measured (the outbound length gate lived only in transports/). An env value + # that resolved to an unexpected blob is exactly the misconfiguration the bound exists to surface + # early, and here it would otherwise surface as an opaque Vault-side failure on the first + # store read. Imported lazily so store/ does not take a transports/ import at module scope. + from messagefoundry.transports.rest import enforce_outbound_length_limits + + enforce_outbound_length_limits(addr or "", {"X-Vault-Token": token} if token else {}) # hvac.Client() reads VAULT_ADDR/VAULT_TOKEN from the environment when url/token are None. client: Any = hvac.Client(url=addr, token=token) return client diff --git a/messagefoundry/transports/dicomweb.py b/messagefoundry/transports/dicomweb.py index 7bc24d0c..6df16dce 100644 --- a/messagefoundry/transports/dicomweb.py +++ b/messagefoundry/transports/dicomweb.py @@ -206,6 +206,10 @@ def __init__(self, config: Destination) -> None: ) self._opener = _insecure_opener(*proxy_handlers) self._target_url = self._resolve_target_url() + # ASVS 4.2.5: the gate above measured base_url, but _target_url is DERIVED from it 33 lines + # later and is what actually ships on every STOW-RS POST and OPTIONS probe. Bounding only the + # base left the derived URL unmeasured -- an ordering gap, not a missing call. + enforce_outbound_length_limits(self._target_url, self._headers) def _resolve_target_url(self) -> str: """``{base}/studies`` (server assigns the study) or ``{base}/studies/{study_uid}`` when set.""" diff --git a/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index 16d33542..1c888f03 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -71,6 +71,9 @@ capture_response_headers, egress_route_from_settings, enforce_outbound_length_limits, + enforce_send_time_length_limits, + enforce_signature_header_limits, + find_outbound_length_violation, normalize_header_allowlist, outbound_headers_from_metadata, refuse_cleartext_credentials, @@ -279,6 +282,9 @@ def __init__(self, config: Destination) -> None: # ASVS 4.1.5 (ADR 0018): opt-in detached-JWS signing; None = off (byte-identical). Built here so # a bad key fails loud at construction; the signature is minted in _post over the body bytes. self._signer: MessageSigner | None = signer_from_destination(config) + # ASVS 4.2.5: the detached-JWS headers are added after the URL/header gate above. + # Message-independent, so they are bounded once here rather than on every send. + enforce_signature_header_limits(self._signer, connector="FHIR destination") # ADR 0024 + #65: opt-in bearer-token auth — SMART (asymmetric JWT) OR OAuth2 client-credentials # (symmetric secret), unified behind the one bearer seam. None = off (byte-identical). Lazy import # breaks the rest <-> http_auth/smart cycle; built here so a bad key/secret/token_url fails loud. @@ -519,6 +525,28 @@ def _post( # ASVS 4.1.5 (ADR 0018): detached JWS over the body bytes, minted off-loop past the queue # boundary so a retry re-mints it (re-run purity holds). headers = {**headers, **self._signer.signature_headers(data)} + # ASVS 4.2.5: FHIR is the one destination whose URL is genuinely per-message -- _resolve_request + # builds it from the resource type and id, and _FHIR_TYPE_RE bounds that grammar but not its + # LENGTH. The construction gate only ever saw base_url, so without this a crafted resourceType + # ships an unbounded request line. Placed before the try below because these raise + # NegativeAckError/DeliveryError, which its handlers deliberately do not catch. + try: + enforce_send_time_length_limits( + url, + headers, + connector=f"FHIR {_redact_url(self.base_url)}", + url_is_message_derived=True, + message_header_names=frozenset(extra_headers), + minted_credential_names=( + frozenset({"Authorization"}) + if self._token_provider is not None + else frozenset() + ), + ) + except NegativeAckError as exc: + if exc.credential_fault and self._token_provider is not None: + self._token_provider.invalidate() + raise try: req = urllib.request.Request( # noqa: S310 # nosec B310 — scheme constrained to http(s) in __init__ url, @@ -742,6 +770,10 @@ def __init__( # ASVS 12.2.1: a cleartext read pulls the PHI resource/searchset back over the wire, so a # cleartext http read to a non-loopback host is refused too (loopback stays byte-identical). self._hop_guard[cname] = refuse_cleartext_egress(scheme, url, attested=attested) + # ASVS 4.2.5: this executor never called the construction gate at all -- fhir.py's only + # call site was the DESTINATION's __init__, so a lookup connection's base URL and static + # headers were unbounded on both sides. + enforce_outbound_length_limits(url, headers) self._headers[cname] = headers self._token[cname] = token if bool(s.get("verify_tls", True)): @@ -834,6 +866,19 @@ def _get(self, connection: str, url: str) -> tuple[str, int]: token = self._token[connection] if token is not None: headers["Authorization"] = f"Bearer {token.access_token()}" + # ASVS 4.2.5. ``url`` here is built per call, so it is the most message-derived URL in the + # engine, and the minted bearer is added just above -- neither was ever measured. A + # FhirLookupError (not a delivery error) because this read runs inside a Handler: there is no + # message to dead-letter, the Handler sees the failure directly. The message carries the class + # and the length only, never the URL or the header name -- a lookup query can carry PHI. + violation = find_outbound_length_violation(url, headers) + if violation is not None: + if violation.name == "Authorization" and token is not None: + token.invalidate() # cached; without this every retry re-sends the same bad token + raise FhirLookupError( + f"fhir_lookup on {connection!r}: the built {violation.kind} is {violation.length} " + f"chars, over the {violation.limit}-char limit" + ) req = urllib.request.Request( # noqa: S310 # nosec B310 — scheme constrained to http(s) in __init__ url, headers=headers, method="GET" ) @@ -886,6 +931,19 @@ def _probe(self, connection: str) -> None: token = self._token[connection] if token is not None: headers["Authorization"] = f"Bearer {token.access_token()}" + # ASVS 4.2.5. ``url`` here is built per call, so it is the most message-derived URL in the + # engine, and the minted bearer is added just above -- neither was ever measured. A + # FhirLookupError (not a delivery error) because this read runs inside a Handler: there is no + # message to dead-letter, the Handler sees the failure directly. The message carries the class + # and the length only, never the URL or the header name -- a lookup query can carry PHI. + violation = find_outbound_length_violation(url, headers) + if violation is not None: + if violation.name == "Authorization" and token is not None: + token.invalidate() # cached; without this every retry re-sends the same bad token + raise FhirLookupError( + f"fhir_lookup on {connection!r}: the built {violation.kind} is {violation.length} " + f"chars, over the {violation.limit}-char limit" + ) req = urllib.request.Request( # noqa: S310 # nosec B310 — scheme constrained to http(s) in __init__ url, headers=headers, method="GET" ) diff --git a/messagefoundry/transports/http_auth.py b/messagefoundry/transports/http_auth.py index 92a9c773..76a6ebad 100644 --- a/messagefoundry/transports/http_auth.py +++ b/messagefoundry/transports/http_auth.py @@ -51,6 +51,7 @@ ProxyConfig, _no_redirect_opener, _redact_url, + enforce_outbound_length_limits, proxy_auth_handler_from_settings, refuse_cleartext_credential_hop, ) @@ -229,6 +230,10 @@ def _fetch_token(self) -> tuple[str, float]: form["client_id"] = self.client_id form["client_secret"] = self._client_secret data = urllib.parse.urlencode(form).encode("ascii") + # ASVS 4.2.5: the token URL and the Basic client-credential header are operator-supplied via + # env(), so an env value that resolved to an unexpected blob would otherwise surface as an + # opaque IdP-side failure on the first mint rather than as a clear config error. + enforce_outbound_length_limits(self.token_url, dict(headers)) req = urllib.request.Request( # noqa: S310 # nosec B310 — scheme constrained to http(s) above self.token_url, data=data, headers=headers, method="POST" ) diff --git a/messagefoundry/transports/rest.py b/messagefoundry/transports/rest.py index 53fd9cc6..eb26e60e 100644 --- a/messagefoundry/transports/rest.py +++ b/messagefoundry/transports/rest.py @@ -194,6 +194,12 @@ def outbound_headers_from_metadata(metadata: Mapping[str, str] | None) -> dict[s # the first delivery. 8 KiB comfortably exceeds any legitimate endpoint URL or Basic/Bearer credential. MAX_OUTBOUND_URL_LEN = 8192 MAX_OUTBOUND_HEADER_VALUE_LEN = 8192 +# Header NAMES are bounded too. Values were bounded from the start, but a name is equally +# attacker-influenceable at send time -- ``outbound_headers_from_metadata`` derives it from a +# message-metadata key suffix, and ``_HEADER_NAME_TOKEN`` bounds its CHARSET, not its length. An +# unbounded name overflows the same header block the value bound exists to protect. Generous: the +# longest name anything here emits is ``X-Idempotency-Key``. +MAX_OUTBOUND_HEADER_NAME_LEN = 256 # 4xx statuses worth retrying anyway: the server is up but momentarily unwilling, not a hard reject. _RETRYABLE_4XX = frozenset({408, 429}) @@ -491,23 +497,164 @@ def refuse_unrevoked_verified_hop( ).enforce_construction() +@dataclass(frozen=True, slots=True) +class OutboundLengthViolation: + """One over-length outbound value: which kind, which name, how long, against what limit. + + Measurement is separated from raising because the SAME measurement serves two gates with + different disclosure rules. At construction every value is operator-supplied, so the message may + name the offending header. At send time a header name can be message-derived, so it may not — + see :func:`enforce_send_time_length_limits`. + """ + + kind: str # "url" | "header-name" | "header-value" + name: str + length: int + limit: int + + +def find_outbound_length_violation( + url: str, headers: Mapping[str, str] +) -> OutboundLengthViolation | None: + """The pure measurement behind both length gates: the FIRST violation, or ``None``.""" + if len(url) > MAX_OUTBOUND_URL_LEN: + return OutboundLengthViolation("url", "", len(url), MAX_OUTBOUND_URL_LEN) + for name, value in headers.items(): + if len(name) > MAX_OUTBOUND_HEADER_NAME_LEN: + return OutboundLengthViolation( + "header-name", name, len(name), MAX_OUTBOUND_HEADER_NAME_LEN + ) + if len(value) > MAX_OUTBOUND_HEADER_VALUE_LEN: + return OutboundLengthViolation( + "header-value", name, len(value), MAX_OUTBOUND_HEADER_VALUE_LEN + ) + return None + + def enforce_outbound_length_limits(url: str, headers: dict[str, str]) -> None: """Reject an over-length outbound URL or request-header value at connector construction (ASVS 4.2.5). Shared by the REST and SOAP destinations (SOAP reuses REST's HTTP plumbing). Raises :class:`ValueError` with a PHI-free message naming only the limit and the offending header name — - never the value (a header may carry a credential).""" - if len(url) > MAX_OUTBOUND_URL_LEN: + never the value (a header may carry a credential). + + This gate sees only what is **statically configured**. Everything added later — per-message + headers, a per-call FHIR URL, the server-minted SMART bearer, the detached-JWS headers — is + bounded by :func:`enforce_send_time_length_limits` instead. + """ + violation = find_outbound_length_violation(url, headers) + if violation is None: + return + if violation.kind == "url": raise ValueError( - f"outbound URL is {len(url)} chars, over the {MAX_OUTBOUND_URL_LEN}-char limit; " + f"outbound URL is {violation.length} chars, over the {violation.limit}-char limit; " "check the configured 'url' / its env() value" ) - for name, value in headers.items(): - if len(value) > MAX_OUTBOUND_HEADER_VALUE_LEN: - raise ValueError( - f"outbound header {name!r} is {len(value)} chars, over the " - f"{MAX_OUTBOUND_HEADER_VALUE_LEN}-char limit; check the configured header / " - "credential value" + if violation.kind == "header-name": + # The name itself is the over-length value, so it is summarised rather than echoed. + raise ValueError( + f"an outbound header NAME is {violation.length} chars, over the " + f"{violation.limit}-char limit; check the configured headers" + ) + raise ValueError( + f"outbound header {violation.name!r} is {violation.length} chars, over the " + f"{violation.limit}-char limit; check the configured header / credential value" + ) + + +def enforce_send_time_length_limits( + url: str, + headers: Mapping[str, str], + *, + connector: str, + message_header_names: frozenset[str] = frozenset(), + url_is_message_derived: bool = False, + minted_credential_names: frozenset[str] = frozenset(), +) -> None: + """Re-measure the FINAL request line and header block, immediately before it goes on the wire. + + Three classes of value are added AFTER the construction gate and are unbounded without this: + per-message headers merged from message metadata, the per-call URL a FHIR read builds, and the + server-minted SMART bearer / detached-JWS headers stamped in just before the request is built. + + **The failure class decides the disposition, so it is computed, not guessed:** + + * *message-derived* → a **permanent** reject. The same message overflows on every retry, so + retrying is a guaranteed-futile loop that also keeps the lane busy. + * *server-minted credential* → **permanent + credential_fault**, so the delivery worker applies + ``credential_fault_policy`` instead of hammering the IdP. A plain transient would re-sign a + client assertion and POST the token endpoint on every retry, forever. + * *anything else* → a plain :class:`DeliveryError`. Static values are already caught at + construction, so this arm is belt-and-braces. + + **The message-derived arm must not name the offending header.** At construction a header name is + operator-static; at send time ``outbound_headers_from_metadata`` derives it from a message- + metadata key suffix (``_HEADER_NAME_TOKEN`` bounds the charset, not the content). This message + travels through ``safe_exc`` into ``last_error`` and ``message_events.detail``, and on the + ``DeliveryError`` arm through ``_note_lane_unhealthy`` → ``connection_error`` → the webhook + AlertSink, i.e. **off-box**. ``redaction.py`` concedes single-token identifiers are its + acknowledged residual, so only the class and the length may leave. + """ + violation = find_outbound_length_violation(url, headers) + if violation is None: + return + + if violation.kind == "url": + if url_is_message_derived: + raise NegativeAckError( + f"{connector} built a {violation.length}-char request URL, over the " + f"{violation.limit}-char limit", + code="MF-URI-LEN", + permanent=True, ) + raise DeliveryError( + f"{connector} request URL is {violation.length} chars, over the " + f"{violation.limit}-char limit" + ) + + if violation.name in minted_credential_names: + raise NegativeAckError( + f"{connector} minted a {violation.length}-char {violation.name} header, over the " + f"{violation.limit}-char limit", + code="MF-HDR-LEN", + permanent=True, + credential_fault=True, + ) + + if violation.name in message_header_names: + # Class + length ONLY -- never violation.name. See the docstring. + kind = "name" if violation.kind == "header-name" else "value" + raise NegativeAckError( + f"{connector} built a per-message request-header {kind} of {violation.length} chars, " + f"over the {violation.limit}-char limit", + code="MF-HDR-LEN", + permanent=True, + ) + + raise DeliveryError( + f"{connector} request header {violation.name!r} is {violation.length} chars, over the " + f"{violation.limit}-char limit" + ) + + +def enforce_signature_header_limits(signer: object | None, *, connector: str) -> None: + """Bound the detached-JWS headers at **construction** (ASVS 4.2.5). + + Construction is the right place because the signature header's length is **message-independent**: + the JWS is detached, so the body contributes only its fixed-width hash. An RS256 header measured + over an empty body is the same length as one over a 100 KB body, which is exactly what + ``test_signature_header_length_is_message_independent`` pins. Re-measuring per send would burn a + signing operation on the hot path to re-derive an answer that cannot change. + + An over-length signature header is therefore a **configuration** fault (an absurd ``sign_key_id`` + is the reachable cause), and surfaces as the same ``ValueError`` the rest of the construction gate + raises — before the connector is ever handed a message. + """ + if signer is None: + return + headers = getattr(signer, "signature_headers", None) + if headers is None: # pragma: no cover - every MessageSigner implements it + return + enforce_outbound_length_limits("", dict(headers(b""))) # --- forward/egress web proxy (BACKLOG #112/#127/#128, ADR 0126) ----------------------------------- @@ -881,6 +1028,9 @@ def __init__(self, config: Destination) -> None: # identical). Built here so a bad key/algorithm fails loud at connector construction (check/ # dry-run/start), like a bad TLS cert; the per-request signature is minted in _post (off-loop). self._signer: MessageSigner | None = signer_from_destination(config) + # ASVS 4.2.5: the detached-JWS headers are added after the URL/header gate above. + # Message-independent, so they are bounded once here rather than on every send. + enforce_signature_header_limits(self._signer, connector="REST destination") # ADR 0024 + #65: opt-in bearer-token auth — SMART Backend Services (asymmetric JWT) OR OAuth2 # client-credentials (symmetric secret), unified behind the one bearer seam. None = off (byte- # identical). Lazy import breaks the rest <-> http_auth/smart cycle (they reuse rest's opener); @@ -1087,6 +1237,34 @@ def _post( self.url, data=data, headers=headers, method=self.method ) ) + # ASVS 4.2.5: bound what ACTUALLY ships. The construction gate saw only the static config; the + # per-message headers, the SMART bearer and the JWS headers above are all added after it. + # Measured on req.full_url rather than self.url because _ech_request re-addresses the request + # at the sidecar, so self.url is not what goes on the wire. + try: + enforce_send_time_length_limits( + req.full_url, + headers, + connector=f"REST {_redact_url(self.url)}", + message_header_names=frozenset(dynamic_headers or ()), + # Classification keys on the header NAME, which is sound only because + # outbound_headers_from_metadata REFUSES to emit Authorization (see + # _HEADER_NAME_TOKEN's call site). If that refusal is ever removed, a message could + # impersonate the credential class and convert its own permanent failure into an + # unbounded retry. + minted_credential_names=( + frozenset({"Authorization"}) + if self._token_provider is not None + else frozenset() + ), + ) + except NegativeAckError as exc: + if exc.credential_fault and self._token_provider is not None: + # The provider CACHES (see smart.py's access_token). Without dropping it, every retry + # re-sends the byte-identical over-length token and the failure is unfixable by any + # amount of retrying. + self._token_provider.invalidate() + raise try: with self._opener.open(req, timeout=self.timeout) as resp: # Read the body (drains the connection for clean close; returned for capture). 2xx ⇒ diff --git a/messagefoundry/transports/smart.py b/messagefoundry/transports/smart.py index b7847a9c..7c922afd 100644 --- a/messagefoundry/transports/smart.py +++ b/messagefoundry/transports/smart.py @@ -55,6 +55,7 @@ ProxyConfig, _no_redirect_opener, _redact_url, + enforce_outbound_length_limits, ) from messagefoundry.transports.signing import CompactJwtSigner @@ -203,6 +204,11 @@ def _fetch_token(self) -> tuple[str, float]: if self.scope: form["scope"] = self.scope data = urllib.parse.urlencode(form).encode("ascii") + # ASVS 4.2.5: the token URL is operator-supplied via env(), and the signed client assertion + # rides the FORM body (not a header), so only the URL and the proxy credential are measurable + # here -- both are config, so a blob-valued env() surfaces as a config error rather than an + # opaque IdP failure on the first mint. + enforce_outbound_length_limits(self.token_url, dict(self._proxy_auth)) req = urllib.request.Request( # noqa: S310 # nosec B310 — scheme constrained to http(s) in __init__ self.token_url, data=data, diff --git a/messagefoundry/transports/soap.py b/messagefoundry/transports/soap.py index 45137388..9a9174cb 100644 --- a/messagefoundry/transports/soap.py +++ b/messagefoundry/transports/soap.py @@ -88,6 +88,8 @@ capture_response_headers, egress_route_from_settings, enforce_outbound_length_limits, + enforce_send_time_length_limits, + enforce_signature_header_limits, normalize_header_allowlist, refuse_cleartext_credential_hop, refuse_cleartext_credentials, @@ -356,6 +358,9 @@ def __init__(self, config: Destination) -> None: # (byte-identical). Built here so a bad key/algorithm fails loud at connector construction; the # signature is minted in _post over the FINAL wire bytes (the WS-* wrapped envelope, ADR 0015). self._signer: MessageSigner | None = signer_from_destination(config) + # ASVS 4.2.5: the detached-JWS headers are added after the URL/header gate above. + # Message-independent, so they are bounded once here rather than on every send. + enforce_signature_header_limits(self._signer, connector="SOAP destination") # #201 (ADR 0078 amendment): the verify-ON https hops below (mTLS + the shared verifying opener) # validate the peer cert but do no OCSP/CRL revocation (stdlib ssl has none) — refuse an @@ -746,6 +751,24 @@ def _post(self, payload: str) -> tuple[str, int, dict[str, str]]: headers=headers, method="POST", ) + # ASVS 4.2.5: bound what actually ships. SOAP has no message-derived header class (see send() + # -- metadata is documented as unused here), so the only values added after the construction + # gate are the minted bearer and the JWS headers. + try: + enforce_send_time_length_limits( + req.full_url, + headers, + connector=f"SOAP {_redact_url(self.url)}", + minted_credential_names=( + frozenset({"Authorization"}) + if self._token_provider is not None + else frozenset() + ), + ) + except NegativeAckError as exc: + if exc.credential_fault and self._token_provider is not None: + self._token_provider.invalidate() + raise try: with self._opener.open(req, timeout=self.timeout) as resp: body = resp.read().decode(self.encoding, errors="replace") diff --git a/tests/test_apiclient.py b/tests/test_apiclient.py index a942a927..a6e76e34 100644 --- a/tests/test_apiclient.py +++ b/tests/test_apiclient.py @@ -85,3 +85,42 @@ def test_decode_helpers_map_bad_body_to_apierror() -> None: _decode(httpx.Response(200, json={"unexpected": "shape"}), EngineInfo) with pytest.raises(ApiError): _decode_list(httpx.Response(200, json={"not": "a list"}), ChannelInfo) + + +# --- ASVS 4.2.5: the client's own outbound length bound -------------------------------------------- + + +def test_apiclient_length_bounds_match_the_transport_constants() -> None: + """The constants are DUPLICATED in apiclient rather than imported, because ADR 0088 keeps this + package engine-free — a GUI/harness process must not pull `transports/` in just to make an HTTP + call. This test is where the duplication is kept honest: it imports both sides in a TEST process, + where the coupling is harmless, and reds if either drifts. + + Mutation: change either constant on either side. Red: the assertion names both values.""" + from messagefoundry.apiclient.client import ( + MAX_REQUEST_HEADER_VALUE_LEN, + MAX_REQUEST_URL_LEN, + ) + from messagefoundry.transports.rest import ( + MAX_OUTBOUND_HEADER_VALUE_LEN, + MAX_OUTBOUND_URL_LEN, + ) + + assert MAX_REQUEST_URL_LEN == MAX_OUTBOUND_URL_LEN, ( + f"apiclient bounds the URL at {MAX_REQUEST_URL_LEN} but the transports bound it at " + f"{MAX_OUTBOUND_URL_LEN}; the duplication has drifted" + ) + assert MAX_REQUEST_HEADER_VALUE_LEN == MAX_OUTBOUND_HEADER_VALUE_LEN, ( + f"apiclient bounds a header value at {MAX_REQUEST_HEADER_VALUE_LEN} but the transports " + f"bound it at {MAX_OUTBOUND_HEADER_VALUE_LEN}; the duplication has drifted" + ) + + +def test_apiclient_refuses_an_over_length_request_path() -> None: + """`_request` builds `base_url + path`; nothing measured it before. Mutation: delete the URL + check in `_request`. Red: DID NOT RAISE (the request reaches httpx instead).""" + from messagefoundry.apiclient.client import ApiError, EngineClient + + client = EngineClient("http://127.0.0.1:8765") + with pytest.raises(ApiError, match="over the 8192-char limit"): + client._request("GET", "/messages?q=" + "a" * 9000) diff --git a/tests/test_fhir_lookup.py b/tests/test_fhir_lookup.py index 8e5426b5..75dc740a 100644 --- a/tests/test_fhir_lookup.py +++ b/tests/test_fhir_lookup.py @@ -564,3 +564,29 @@ def test_no_lookup_declared_is_unchanged() -> None: # AC-7 assert reg.fhir_lookups == {} with pytest.raises(FhirLookupError): fhir_lookup("epic", "Patient/123") + + +# --- ASVS 4.2.5: the read URL is the most message-derived URL in the engine ------------------------ + + +async def test_fhir_lookup_over_length_read_url_is_refused() -> None: + """This executor never called the construction length gate at ALL — fhir.py's only call site was + the DESTINATION's __init__ — so both the configured base and the per-call read URL were unbounded. + The query is Handler-supplied and reaches `{base}/{query}` verbatim. + + Mutation: replace the `find_outbound_length_violation` call in `_get` with `violation = None`. + Red: the `match=` below. **That clause is mandatory, not decorative** — with the guard gone the + oversize GET reaches the fake opener and `_parse` raises `FhirLookupError` anyway, so a bare + `pytest.raises(FhirLookupError)` would pass either way and prove nothing.""" + ex, opener = _executor() + with pytest.raises(FhirLookupError, match="over the 8192-char limit"): + await ex.read("epic", "Patient?name=" + "a" * 9000) + assert opener.requests == [], "the over-length request must never reach the opener" + + +async def test_fhir_lookup_ordinary_read_still_reaches_the_opener() -> None: + """Byte-identity control for the gate above: a normal read is untouched. Mutation: drop + MAX_OUTBOUND_URL_LEN to 8 → this reds, proving the gate sits on the ordinary path.""" + ex, opener = _executor(body=b'{"resourceType": "Patient", "id": "123"}') + await ex.read("epic", "Patient/123") + assert len(opener.requests) == 1 diff --git a/tests/test_rest_transport.py b/tests/test_rest_transport.py index cc909e1e..71a30993 100644 --- a/tests/test_rest_transport.py +++ b/tests/test_rest_transport.py @@ -20,6 +20,7 @@ from messagefoundry.config.wiring import Rest, WiringError from messagefoundry.pipeline.wiring_runner import check_egress_allowed from messagefoundry.transports import build_destination +from messagefoundry.transports import rest as rest_mod from messagefoundry.transports.base import DeliveryError, NegativeAckError from messagefoundry.transports.rest import RestDestination @@ -141,6 +142,161 @@ def test_rest_rejects_over_length_header_value() -> None: _dest(bearer_token="x" * 9000) +# --- ASVS 4.2.5: the SEND-TIME half of the length bound ------------------------------------------- +# +# The construction gate above sees only statically configured values. Three classes are added AFTER +# it — per-message headers, the server-minted SMART bearer, and the detached-JWS headers — and were +# unbounded until the send-time gate. Each test below names the mutation that reds it. + + +async def test_rest_over_length_message_header_is_a_permanent_nak() -> None: + """Mutation: delete the `enforce_send_time_length_limits` call in `_post`. Red: DID NOT RAISE — + the send completes and ships a 9000-char header. Permanent, not transient: the same message + overflows on every retry, so retrying is a guaranteed-futile loop that also holds the lane.""" + dest = _dest() + dest._opener = _FakeOpener() # type: ignore[assignment] + with pytest.raises(NegativeAckError) as exc: + await dest.send("x", metadata={"http.header.X-Case-Ref": "v" * 9000}) + assert exc.value.permanent is True + assert exc.value.credential_fault is False + assert "over the 8192-char limit" in str(exc.value) + + +async def test_the_message_header_arm_never_names_the_header() -> None: + """PHI egress. At construction a header name is operator-static, so naming it is safe. At send + time `outbound_headers_from_metadata` derives it from a message-metadata key suffix, and this + string reaches `last_error`, `message_events.detail` and — on the DeliveryError arm — the webhook + AlertSink, i.e. OFF-BOX. Only the class and the length may leave. + + Mutation: put `violation.name` back in the message-derived branch. Red: the `not in` below.""" + dest = _dest() + dest._opener = _FakeOpener() # type: ignore[assignment] + with pytest.raises(NegativeAckError) as exc: + await dest.send("x", metadata={"http.header.X-Patient-MRN-12345": "v" * 9000}) + text = str(exc.value) + assert "X-Patient-MRN-12345" not in text + assert "MRN" not in text + assert "per-message request-header value of 9000 chars" in text + + +async def test_rest_send_time_gate_does_not_disturb_the_ordinary_path() -> None: + """Byte-identity control. Mutation: drop MAX_OUTBOUND_HEADER_VALUE_LEN to 64 — the 100-char + header below reds, proving the gate is on the ordinary path and not dead code.""" + dest = _dest(headers={"X-Idempotency-Key": "k" * 100}) + opener = _FakeOpener() + dest._opener = opener # type: ignore[assignment] + await dest.send("x", metadata={"http.header.X-Case-Ref": "ok"}) + assert len(opener.requests) == 1 + assert opener.requests[0].full_url == URL + + +async def test_the_first_violation_found_is_the_dynamic_one_not_content_type( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Positive control, and it is fiddly for a reason. `_build_headers` seeds `Content-Type: + application/json` FIRST and `_post` does `dict(self._headers)` then `.update(dynamic)`, so with + the limit patched below Content-Type's own length the FIRST violation found is Content-Type — a + connection-static value, which raises the *base* DeliveryError and would make this test assert + the wrong arm entirely. `application/json` is 16 chars, so the limit is patched to 18 and the + dynamic value made 20. Construct FIRST, patch SECOND: patching before construction would trip the + construction gate instead.""" + dest = _dest() + dest._opener = _FakeOpener() # type: ignore[assignment] + monkeypatch.setattr(rest_mod, "MAX_OUTBOUND_HEADER_VALUE_LEN", 18) + with pytest.raises(NegativeAckError) as exc: + await dest.send("x", metadata={"http.header.X-Case-Ref": "v" * 20}) + assert "20 chars" in str(exc.value) + assert "Content-Type" not in str(exc.value) + + +async def test_rest_over_length_minted_bearer_is_a_credential_fault_and_is_invalidated() -> None: + """The only test that proves PLACEMENT. Mutation: move the guard from `_post` back into `send` + (the naive fix). Red: DID NOT RAISE — `send` cannot see a bearer the provider mints inside + `_post`. + + `credential_fault=True`, not a plain transient: the provider CACHES, so without `invalidate()` + every retry re-sends the byte-identical over-length token forever; and with `invalidate()` but no + `credential_fault`, every retry re-signs a client assertion and POSTs the IdP forever. The + delivery worker's `credential_fault_policy` exists to stop exactly that re-auth storm.""" + + class _Provider: + def __init__(self) -> None: + self.token: str | None = "T" * 9000 + self.invalidated = False + + def access_token(self) -> str: + return self.token or "" + + def invalidate(self) -> None: + self.invalidated = True + self.token = None + + dest = _dest() + provider = _Provider() + dest._token_provider = provider # type: ignore[assignment] + dest._opener = _FakeOpener() # type: ignore[assignment] + with pytest.raises(NegativeAckError) as exc: + await dest.send("x") + assert exc.value.permanent is True + assert exc.value.credential_fault is True + assert provider.invalidated is True + assert "T" * 20 not in str(exc.value) # never echo the credential + + +def _signing_pem() -> str: + from cryptography.hazmat.primitives import serialization + from cryptography.hazmat.primitives.asymmetric import rsa + + return ( + rsa.generate_private_key(public_exponent=65537, key_size=2048) + .private_bytes( + encoding=serialization.Encoding.PEM, + format=serialization.PrivateFormat.PKCS8, + encryption_algorithm=serialization.NoEncryption(), + ) + .decode("ascii") + ) + + +def _signing_dest(pem: str, key_id: str) -> RestDestination: + """Built from FLAT ``sign_*`` settings — the `Rest()` spec has no signing arm, and + ``signer_from_destination`` falls back to these for a directly-built Destination.""" + d = build_destination( + Destination( + name="OB_REST_SIGNED", + type=ConnectorType.REST, + settings={"url": URL, "sign_private_key": pem, "sign_key_id": key_id}, + ) + ) + assert isinstance(d, RestDestination) + return d + + +def test_over_length_sign_key_id_is_refused_at_construction() -> None: + """An absurd `sign_key_id` is the reachable cause of an over-length signature header, and it is a + CONFIG fault — caught before the connector is ever handed a message. + + Mutation: delete the `enforce_signature_header_limits` call in `RestDestination.__init__`. Red: + DID NOT RAISE.""" + with pytest.raises(ValueError, match="over the 8192-char limit"): + _signing_dest(_signing_pem(), "k" * 9000) + + +def test_signature_header_length_is_message_independent() -> None: + """This is what legitimises bounding the signature at CONSTRUCTION rather than per send: the JWS + is detached, so the body contributes only its fixed-width hash. + + Mutation: make the JWS attached (sign over the body instead of its digest). Red: the set below + grows and `assert len(...) == 1` fails.""" + signer = _signing_dest(_signing_pem(), "k1")._signer + assert signer is not None + lengths = { + len(next(iter(signer.signature_headers(body).values()))) + for body in (b"", b"x" * 10, b"y" * 100_000) + } + assert len(lengths) == 1, f"signature header length varies with the body: {lengths}" + + def test_rest_verify_tls_false_refused_without_escape(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.delenv("MEFOR_ALLOW_INSECURE_TLS", raising=False) with pytest.raises(ValueError): From 8a61e4a418de935ece2ed8b2aa6990aa21b39aa8 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 08:59:48 -0500 Subject: [PATCH 4/6] feat(transports): close the last five ASVS 4.2.5 surfaces, and name the sixth Completes the change set begun in 17d8c822. 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. --- .../0022-fhir-resource-codec-rest-client.md | 10 +++++-- docs/adr/0025-dicom-codec-store-connectors.md | 3 +- docs/testing/FEATURE-COVERAGE-PLAN.md | 2 +- messagefoundry/pipeline/alert_sinks.py | 7 +++++ messagefoundry/transports/ai_broker.py | 16 +++++++++- messagefoundry/transports/fhir.py | 20 +++++++++++++ messagefoundry/transports/rest.py | 20 +++++++++++++ messagefoundry/transports/soap.py | 4 +++ messagefoundry/tray/probe.py | 9 ++++++ tests/test_ai_broker.py | 30 +++++++++++++++++++ tests/test_alert_sinks.py | 15 ++++++++++ 11 files changed, 130 insertions(+), 6 deletions(-) diff --git a/docs/adr/0022-fhir-resource-codec-rest-client.md b/docs/adr/0022-fhir-resource-codec-rest-client.md index 5eec6ac7..4f3e220e 100644 --- a/docs/adr/0022-fhir-resource-codec-rest-client.md +++ b/docs/adr/0022-fhir-resource-codec-rest-client.md @@ -183,8 +183,10 @@ in → `fhir.resources` resource out, hand-authored (no pure-Python v2↔FHIR co that **reuses the shared module-level helpers** in [transports/rest.py](../../messagefoundry/transports/rest.py) — **exactly as the SOAP destination does**. `SoapDestination` ([transports/soap.py](../../messagefoundry/transports/soap.py)) is *not* a wrapper of `RestDestination`; it is a sibling `DestinationConnector` that imports rest.py's -`_NO_REDIRECT_OPENER`/`_NoRedirectHandler`, `_insecure_opener`, `_redact_url`, `enforce_outbound_length_limits`, -`refuse_cleartext_credentials`, plus `signer_from_destination` — and follows rest.py's status→retry idiom. +`_NO_REDIRECT_OPENER`/`_NoRedirectHandler`, `_insecure_opener`, `_redact_url`, `enforce_outbound_length_limits` ++ `enforce_send_time_length_limits` + `enforce_signature_header_limits` (ASVS 4.2.5 -- the construction gate +sees only static config; the send-time gate bounds the per-call URL, the per-message headers and the minted +bearer), `refuse_cleartext_credentials`, plus `signer_from_destination` — and follows rest.py's status→retry idiom. `FhirDestination` does the **same**: it **does not compose or instantiate `RestDestination`**, and it does **not** re-implement HTTP. It implements the `DestinationConnector` contract: one `async def send(self, payload: str) -> DeliveryResponse | None`, optional `aclose`/`test_connection` overrides, and it raises **only** @@ -195,7 +197,9 @@ str) -> DeliveryResponse | None`, optional `aclose`/`test_connection` overrides, - The TLS posture: the no-redirect, TLS-verifying opener (`_NO_REDIRECT_OPENER`/`_NoRedirectHandler` — a 3xx is raised, never followed: the PHI-redirect defense, ASVS 15.3.2) and the `verify_tls=False` escape gated by `MEFOR_ALLOW_INSECURE_TLS` (`insecure_tls_allowed()`), plus the cleartext-credential refusal. -- `enforce_outbound_length_limits`, `refuse_cleartext_credentials`, `_redact_url`, and the optional JWS signer +- `enforce_outbound_length_limits` / `enforce_send_time_length_limits` / `enforce_signature_header_limits` + (ASVS 4.2.5; the FHIR per-call URL is message-derived, so the send-time arm is load-bearing here in a way it + is not for REST), `refuse_cleartext_credentials`, `_redact_url`, and the optional JWS signer hook (`signer_from_destination`, ADR 0018) for signed bodies. - The retry classification **idiom** from rest.py's `_post`: **2xx → delivered**; status in `_RETRYABLE_4XX = {408, 429}` **or** `5xx` → `DeliveryError` (transient → pipeline retries with backoff); **any other 4xx** (and a diff --git a/docs/adr/0025-dicom-codec-store-connectors.md b/docs/adr/0025-dicom-codec-store-connectors.md index 0bdf81de..407afe7f 100644 --- a/docs/adr/0025-dicom-codec-store-connectors.md +++ b/docs/adr/0025-dicom-codec-store-connectors.md @@ -292,7 +292,8 @@ right; built behind the Phase-1 slice: [transports/rest.py](../../messagefoundry/transports/rest.py) — **exactly as the SOAP and FHIR destinations do**. `SoapDestination`/`FhirDestination` are *not* wrappers of `RestDestination`; each is a sibling `DestinationConnector` that imports rest.py's `_NO_REDIRECT_OPENER`/`_NoRedirectHandler`, `_insecure_opener`, `_redact_url`, - `enforce_outbound_length_limits`, `refuse_cleartext_credentials`, the `_RETRYABLE_4XX` retry idiom, plus + `enforce_outbound_length_limits` (ASVS 4.2.5 -- applied to the DERIVED `_target_url`, not just `base_url`), + `refuse_cleartext_credentials`, the `_RETRYABLE_4XX` retry idiom, plus `signer_from_destination` — and follows rest.py's status→retry idiom. `DicomWebDestination` does the **same**: it **does not compose or instantiate `RestDestination`**, and it does **not** re-implement HTTP. It implements the `DestinationConnector` contract: one `async def send(self, payload: str) -> DeliveryResponse | None`, optional diff --git a/docs/testing/FEATURE-COVERAGE-PLAN.md b/docs/testing/FEATURE-COVERAGE-PLAN.md index 6c7a213f..c630f760 100644 --- a/docs/testing/FEATURE-COVERAGE-PLAN.md +++ b/docs/testing/FEATURE-COVERAGE-PLAN.md @@ -760,7 +760,7 @@ Recommended tests to close gaps: | HTTPFHIR-3 | REST/SOAP/FHIR response capture (accepted/no_reply, encrypted at rest) | 0013,0003 | test_response_capture.py, test_response_headers_capture.py, test_fhir_transport.py, test_soap_wssecurity.py | covered | backend: encryption-at-rest SQLite-only | med | M | | HTTPFHIR-4 | Captured response-header allow-list (#154) | 0013 | test_response_headers_capture.py | covered | SOAP/FHIR wiring not separately driven | low | S | | HTTPFHIR-5 | Per-message dynamic HTTP headers (#68): projection + injection safety | 0081 | test_rest_transport.py, test_fhir_transport.py | covered | — | med | S | -| HTTPFHIR-6 | Outbound URL/header length limits (ASVS 4.2.5) | 0003 | test_rest_transport.py over-length url/header | covered | SOAP/FHIR share helper, no own assertion | low | S | +| HTTPFHIR-6 | Outbound URL/header length limits, construction **and send time** (ASVS 4.2.5) | 0003 | test_rest_transport.py over-length url/header + the send-time gate (message header, minted bearer, signature); test_fhir_lookup.py read-URL bound; test_apiclient.py constant parity | covered | SOAP/FHIR share the helper and have no own send-time assertion; _probe paths asserted only indirectly | low | S | | HTTPFHIR-7 | SOAP plain mode: envelope POST + version headers | 0003,0015 | test_soap_transport.py 1.1/1.2 headers + send | covered | — | med | S | | HTTPFHIR-8 | SOAP Fault classification + no fault-body echo | 0015 | test_soap_transport.py, test_soap_wssecurity.py | covered | phi: no canary assertion on fault body | med | S | | HTTPFHIR-9 | SOAP WS-Addressing/WS-Security stamping (purity, PasswordText/Digest, escaping) | 0015 | test_soap_wssecurity.py | covered | — | high | S | diff --git a/messagefoundry/pipeline/alert_sinks.py b/messagefoundry/pipeline/alert_sinks.py index 7e346572..187b1ecd 100644 --- a/messagefoundry/pipeline/alert_sinks.py +++ b/messagefoundry/pipeline/alert_sinks.py @@ -298,6 +298,13 @@ def __init__( "(cleartext, MITM-able — trusted-network/dev use only)", INSECURE_TLS_ESCAPE_ENV, ) + # ASVS 4.2.5: bound the webhook URL. Construction-only is sufficient here and not a shortcut: + # the URL is operator config and the sole header is a fixed ``Content-Type``, so nothing is + # added between here and the wire. Imported lazily to keep the module's import cost unchanged + # for the far commoner non-webhook sinks. + from messagefoundry.transports.rest import enforce_outbound_length_limits + + enforce_outbound_length_limits(url, {"Content-Type": "application/json"}) self.url = url self.timeout = timeout # Optional egress allowlist (lower-cased); empty = any host. SSRF defense-in-depth (1.3.6). diff --git a/messagefoundry/transports/ai_broker.py b/messagefoundry/transports/ai_broker.py index 4f270a62..450aae66 100644 --- a/messagefoundry/transports/ai_broker.py +++ b/messagefoundry/transports/ai_broker.py @@ -45,7 +45,11 @@ # Reuse rest.py's hardened, TLS-verifying, no-redirect opener + URL redaction (no new HTTP plumbing) — # exactly as smart.py / fhir.py / soap.py do. No import cycle: rest.py never imports this module. -from messagefoundry.transports.rest import _NO_REDIRECT_OPENER, _redact_url +from messagefoundry.transports.rest import ( + _NO_REDIRECT_OPENER, + _redact_url, + find_outbound_length_violation, +) if TYPE_CHECKING: # only for the from-settings factory annotation from messagefoundry.config.settings import AiSettings @@ -138,6 +142,16 @@ def __init__( "[ai].endpoint over cleartext http would expose the api_key; refused unless " f"{INSECURE_TLS_ESCAPE_ENV} is set (dev/trusted-network only) — use https" ) + # ASVS 4.2.5. Both values are operator-supplied via env() and both ship on every provider call + # -- the endpoint as the request line, the key as the ``x-api-key`` header. Bounded here rather + # than per call because neither varies per prompt. Raised as AiBrokerError (a ValueError + # subclass) so the API surface keeps its single error type, and the key is NEVER echoed. + violation = find_outbound_length_violation(endpoint, {"x-api-key": api_key}) + if violation is not None: + raise AiBrokerError( + f"[ai] {violation.kind} is {violation.length} chars, over the " + f"{violation.limit}-char limit; check [ai].endpoint / the api_key env() value" + ) self.endpoint = endpoint self.api_key = api_key self.provider = provider or "claude" diff --git a/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index 1c888f03..5849f929 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -489,6 +489,26 @@ def _probe(self) -> None: req = urllib.request.Request( # noqa: S310 # nosec B310 — scheme constrained to http(s) in __init__ url, headers=headers, method="GET" ) + # ASVS 4.2.5: this probe MINTS a real bearer above (so reachability reflects the actual + # credentials), and that token is the one value the construction gate could not see -- the URL + # and the static headers were already bounded there. Gated here so an operator's "test + # connection" fails with the real reason rather than an opaque wire error. The SOAP probe + # deliberately has no equivalent: it mints nothing, so a gate there could not fire. + try: + enforce_send_time_length_limits( + req.full_url, + headers, + connector=f"FHIR {_redact_url(self.base_url)} probe", + minted_credential_names=( + frozenset({"Authorization"}) + if self._token_provider is not None + else frozenset() + ), + ) + except NegativeAckError as exc: + if exc.credential_fault and self._token_provider is not None: + self._token_provider.invalidate() + raise try: with self._opener.open(req, timeout=self.timeout) as resp: resp.read() diff --git a/messagefoundry/transports/rest.py b/messagefoundry/transports/rest.py index eb26e60e..ee3a04c3 100644 --- a/messagefoundry/transports/rest.py +++ b/messagefoundry/transports/rest.py @@ -1188,6 +1188,26 @@ def _probe(self) -> None: self.url, headers=headers, method="HEAD" ) ) + # ASVS 4.2.5: this probe MINTS a real bearer above (so reachability reflects the actual + # credentials), and that token is the one value the construction gate could not see -- the URL + # and the static headers were already bounded there. Gated here so an operator's "test + # connection" fails with the real reason rather than an opaque wire error. The SOAP probe + # deliberately has no equivalent: it mints nothing, so a gate there could not fire. + try: + enforce_send_time_length_limits( + req.full_url, + headers, + connector=f"REST {_redact_url(self.url)} probe", + minted_credential_names=( + frozenset({"Authorization"}) + if self._token_provider is not None + else frozenset() + ), + ) + except NegativeAckError as exc: + if exc.credential_fault and self._token_provider is not None: + self._token_provider.invalidate() + raise try: with self._opener.open(req, timeout=self.timeout) as resp: resp.read() diff --git a/messagefoundry/transports/soap.py b/messagefoundry/transports/soap.py index 9a9174cb..2e60e2f0 100644 --- a/messagefoundry/transports/soap.py +++ b/messagefoundry/transports/soap.py @@ -707,6 +707,10 @@ def _probe(self) -> None: # Reachability only: a HEAD reaches the endpoint without POSTing an envelope. An HTTP response # means the host answered (a 405 is still a pass), but a 401/403 means the configured # credentials would be rejected — surface that as a failure. Connection/DNS/TLS/timeout fails. + # ASVS 4.2.5: deliberately NO send-time length gate here. Unlike the REST and FHIR probes, + # this one does not mint a bearer -- it ships ``self.url`` and ``self._headers`` verbatim, and + # both were bounded at construction. A gate here could not fire on any input, and a guard that + # cannot fail reads as coverage without being it. req = urllib.request.Request( # noqa: S310 # nosec B310 — scheme constrained to http(s) in __init__ self.url, headers=self._headers, method="HEAD" ) diff --git a/messagefoundry/tray/probe.py b/messagefoundry/tray/probe.py index f1394702..d86ba4e4 100644 --- a/messagefoundry/tray/probe.py +++ b/messagefoundry/tray/probe.py @@ -128,6 +128,15 @@ def make_probe_client(engine_url: str, timeout: float = DEFAULT_TIMEOUT_S) -> ht engine URL gets a verifying TLS context (see :func:`build_verify`); a failed verification surfaces as an ``httpx.HTTPError`` and therefore as ``DOWN``/``UNKNOWN``, never as a silent downgrade to an unverified connection. + + **ASVS 4.2.5 — a NAMED residual, not an oversight.** ``engine_url`` is not length-bounded the way + the outbound transports and ``apiclient`` are. Three facts make that proportionate rather than a + gap, and all three must stay true or this needs revisiting: the client is **tokenless** (no + credential can overflow), the URL is **local operator config** pointing at this host's own engine + (not attacker-influenceable and not message-derived), and ``tray/`` is deliberately stdlib+httpx + only (ADR 0113) — importing ``transports/`` to share the constant would breach the same layering + ADR 0088 protects for ``apiclient``, and a third copy of an 8192 that nothing pins is worse than + a documented absence. If the tray ever carries a token or takes a remote URL, close it. """ return httpx.Client( base_url=engine_url.rstrip("/"), diff --git a/tests/test_ai_broker.py b/tests/test_ai_broker.py index 4e4973a5..7bd1c257 100644 --- a/tests/test_ai_broker.py +++ b/tests/test_ai_broker.py @@ -325,3 +325,33 @@ async def test_ai_chat_requires_ai_assist_permission( ) assert denied.status_code == 403 # viewer lacks ai:assist assert ok.status_code == 200 # coding role holds ai:assist + + +# --- ASVS 4.2.5: outbound length bound -------------------------------------- + + +def test_broker_refuses_an_over_length_api_key() -> None: + """The key ships as `x-api-key` on every provider call and is operator-supplied via env(), so an + env value that resolved to an unexpected blob would otherwise surface as an opaque provider-side + failure on the first chat rather than as a config error. + + Mutation: delete the `find_outbound_length_violation` block in `AiBroker.__init__`. Red: DID NOT + RAISE. The assertion below also pins that the KEY is never echoed.""" + with pytest.raises(AiBrokerError, match="over the 8192-char limit") as exc: + AiBroker(endpoint=_ENDPOINT, api_key="k" * 9000, allowed_endpoints=["ai.internal"]) + assert "kkkkkkkkkkkkkkkkkkkk" not in str(exc.value) + + +def test_broker_refuses_an_over_length_endpoint() -> None: + with pytest.raises(AiBrokerError, match="over the 8192-char limit"): + AiBroker( + endpoint=_ENDPOINT + "?q=" + "a" * 9000, + api_key="k", + allowed_endpoints=["ai.internal"], + ) + + +def test_broker_ordinary_construction_still_works() -> None: + """Byte-identity control. Mutation: drop MAX_OUTBOUND_URL_LEN to 8 -> this reds.""" + broker = AiBroker(endpoint=_ENDPOINT, api_key="k", allowed_endpoints=["ai.internal"]) + assert broker.endpoint_host == "ai.internal" diff --git a/tests/test_alert_sinks.py b/tests/test_alert_sinks.py index 4ca87fab..ca297758 100644 --- a/tests/test_alert_sinks.py +++ b/tests/test_alert_sinks.py @@ -325,3 +325,18 @@ def test_alerts_email_recipients_split_from_env_string() -> None: def test_alerts_password_via_env(monkeypatch: pytest.MonkeyPatch) -> None: settings = load_settings(environ={"MEFOR_ALERTS_EMAIL_PASSWORD": "s3cret"}) assert settings.alerts.email_password == "s3cret" + + +def test_webhook_url_length_is_bounded_at_construction() -> None: + """ASVS 4.2.5. Construction-only 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. + + Mutation: delete the `enforce_outbound_length_limits` call in `WebhookTransport.__init__`. Red: + DID NOT RAISE.""" + with pytest.raises(ValueError, match="over the 8192-char limit"): + WebhookTransport("https://hooks.example/x?q=" + "a" * 9000, timeout=5.0) + + +def test_webhook_ordinary_url_still_constructs() -> None: + """Byte-identity control. Mutation: drop MAX_OUTBOUND_URL_LEN to 8 -> this reds.""" + assert WebhookTransport("https://hooks.example/x", timeout=5.0).name == "webhook" From fdb3f19469824b98598f8a20385b5231b8fbd716 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 09:19:40 -0500 Subject: [PATCH 5/6] fix(secrets): the one credential the docs say to rotate was never fingerprinted 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. --- messagefoundry/pipeline/secret_rotation.py | 16 +++++ tests/test_secret_rotation_inventory.py | 72 ++++++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/messagefoundry/pipeline/secret_rotation.py b/messagefoundry/pipeline/secret_rotation.py index 6263454e..00deaa5c 100644 --- a/messagefoundry/pipeline/secret_rotation.py +++ b/messagefoundry/pipeline/secret_rotation.py @@ -116,8 +116,24 @@ def overdue(self) -> bool: ("MEFOR_API_TLS_KEY_PASSWORD", "off-loopback TLS private-key passphrase"), ("MEFOR_STORE_VAULT_TOKEN", "Vault token — store DEK provider"), ("MEFOR_SECRETS_VAULT_TOKEN", "Vault token — connector KV provider"), + ("MEFOR_AI_API_KEY", "engine-broker LLM provider credential"), ) +# Deliberately NOT rotation-inventoried, recorded here so the omissions are decisions rather than +# oversights a later reader re-litigates. The test that pins this list checks the classification, not +# just the membership: +# MEFOR_STORE_ENCRYPTION_KEY the DEK itself -- it has its own arm (_maybe_escalate_dek), +# which reasons over the wrapped key's age, not an env +# fingerprint. Adding it here would double-count it. +# MEFOR_STORE_ENCRYPTION_KEYS_RETIRED a DECRYPT-ONLY tail of superseded keys. Rotating it is +# meaningless -- it exists precisely so old ciphertext stays +# readable -- and flagging it as "due" would tell an operator to +# destroy their own recovery path. +# MEFOR_STORE_TRANSIT_KEY Transit key NAMES, not secret values. The secret lives in +# MEFOR_STORE_TRANSIT_AUDIT_KEY 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. + _FP_HEX_LEN = ( 32 # hex chars of the HMAC-SHA256 fingerprint kept (128-bit — ample, keeps the row compact) ) diff --git a/tests/test_secret_rotation_inventory.py b/tests/test_secret_rotation_inventory.py index 317a8d5a..6f8177f6 100644 --- a/tests/test_secret_rotation_inventory.py +++ b/tests/test_secret_rotation_inventory.py @@ -287,3 +287,75 @@ def test_rotation_docstring_guard_self_test() -> None: "The store DEK is tracked live-by-default off a persisted tracked-since stamp." ) assert not any(lie in fixed for lie in ("deny-by-default", "NOT tracked here yet")) + + +# --- ASVS 13.3.4: the fingerprinting arm must cover the fixed env secrets ------------------------- + +#: Fixed ``MEFOR_*`` critical secrets deliberately NOT fingerprinted by the rotation watcher, each with +#: the reason. Kept HERE rather than only in the module so the exclusion is a reviewed decision with a +#: test behind it: dropping a name into ``_ENV_SECRET_CLASSES``'s exclusion comment alone changes +#: nothing, but silently *removing* a real secret from the tracked set would otherwise go unnoticed. +_NOT_FINGERPRINTED: dict[str, str] = { + "MEFOR_STORE_ENCRYPTION_KEY": ( + "the DEK itself — tracked by its own arm (_maybe_escalate_dek), which reasons over the wrapped " + "key's age rather than an env fingerprint; listing it here too would double-count it" + ), + "MEFOR_STORE_ENCRYPTION_KEYS_RETIRED": ( + "a decrypt-only tail of superseded keys — rotating it is meaningless, and flagging it 'due' " + "would tell an operator to destroy their own recovery path" + ), + "MEFOR_STORE_VAULT_TRANSIT_KEY": "a Vault Transit KEK NAME, not a secret value", + "MEFOR_STORE_TRANSIT_KEY": "a Vault Transit data-key NAME, not a secret value", + "MEFOR_STORE_TRANSIT_AUDIT_KEY": "a Vault Transit audit-key NAME, not a secret value", + "MEFOR_PFX_PASSWORD": ( + "a one-shot passphrase for the `cert import` CLI — the running service never holds it, so " + "there is nothing to fingerprint" + ), +} + + +def test_every_fixed_env_secret_is_fingerprinted_or_explicitly_excused() -> None: + """The gap this closes was live and silent. + + ``MEFOR_AI_API_KEY`` was a registered critical secret **with a documented rotation cadence** in the + schedule above, and yet was absent from ``_ENV_SECRET_CLASSES`` — so the watcher never fingerprinted + the one credential the documentation explicitly tells operators to rotate. Enumeration completeness + (which this module already guarded) and *rotation* coverage are different properties, and nothing + checked the second. + + Mutation: remove ``MEFOR_AI_API_KEY`` from ``_ENV_SECRET_CLASSES``. Red: it appears in the + "registered ... but neither fingerprinted nor excused" list below. + """ + from messagefoundry.pipeline.secret_rotation import _ENV_SECRET_CLASSES + + fingerprinted = {name for name, _label in _ENV_SECRET_CLASSES} + fixed_env = {k for k in CRITICAL_SECRETS if k.startswith("MEFOR_")} + + unaccounted = sorted(fixed_env - fingerprinted - set(_NOT_FINGERPRINTED)) + assert not unaccounted, ( + f"registered critical secret(s) neither fingerprinted by the rotation watcher nor excused: " + f"{unaccounted}. Add each to _ENV_SECRET_CLASSES in messagefoundry/pipeline/secret_rotation.py, " + f"or to _NOT_FINGERPRINTED here WITH the reason it cannot be rotated. A documented rotation " + f"cadence with no fingerprint is a reminder nothing can ever emit." + ) + + # The exclusion list must not rot into a place to park real secrets: every name in it has to still + # be a registered critical secret, and must NOT also be fingerprinted (which would be contradictory). + stale = sorted(set(_NOT_FINGERPRINTED) - fixed_env) + assert not stale, f"_NOT_FINGERPRINTED names that are no longer registered secrets: {stale}" + both = sorted(set(_NOT_FINGERPRINTED) & fingerprinted) + assert not both, f"names both excused AND fingerprinted — the excuse is false: {both}" + + +def test_every_fingerprinted_env_secret_is_a_registered_critical_secret() -> None: + """The other direction: the watcher must not fingerprint something the registry does not know + about, or the rotation alert names a secret with no documented cadence to measure it against. + + Mutation: add a fabricated ``MEFOR_NOT_REAL`` to ``_ENV_SECRET_CLASSES``. Red: named below.""" + from messagefoundry.pipeline.secret_rotation import _ENV_SECRET_CLASSES + + unregistered = sorted({n for n, _ in _ENV_SECRET_CLASSES} - set(CRITICAL_SECRETS)) + assert not unregistered, ( + f"fingerprinted secret(s) missing from the CRITICAL_SECRETS registry: {unregistered}. " + f"Register each (and give it a rotation-schedule row) so the alert has a cadence to cite." + ) From aa861ee15de8930ac97a9d2c24352d0e3b1077c4 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 10:32:45 -0500 Subject: [PATCH 6/6] fix(packaging): our own docs pointed users at an unclaimed PyPI name (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. --- README.md | 2 +- docs/INSTALL-GUIDE.md | 5 +- docs/MENTAL-MODEL.md | 2 +- docs/SERVICE.md | 2 +- docs/USER-GUIDE.md | 2 +- messagefoundry/api/app.py | 12 +- packaging/messagefoundry-webconsole/README.md | 9 +- tests/test_install_instruction_provenance.py | 182 ++++++++++++++++++ 8 files changed, 206 insertions(+), 10 deletions(-) create mode 100644 tests/test_install_instruction_provenance.py diff --git a/README.md b/README.md index b5dc87c5..b527315b 100644 --- a/README.md +++ b/README.md @@ -102,7 +102,7 @@ deliberate — replace `` with the current release shown at the top of needs (each is opt-in and lazy-imported): ```bash -pip install "messagefoundry-webconsole==" # the browser web console (/ui) — the operator UI; most operators want this +pip install -e packaging/messagefoundry-webconsole # the browser web console (/ui) — the operator UI; most operators want this (source tree: not on PyPI yet) pip install "messagefoundry[postgres]==" # PostgreSQL store backend (production server DB) pip install "messagefoundry[sqlserver]==" # SQL Server store backend (+ OS-level ODBC Driver 18) pip install "messagefoundry[sftp]==" # SFTP transport for the REMOTEFILE connector diff --git a/docs/INSTALL-GUIDE.md b/docs/INSTALL-GUIDE.md index b77cb3bf..d047ccbd 100644 --- a/docs/INSTALL-GUIDE.md +++ b/docs/INSTALL-GUIDE.md @@ -249,7 +249,10 @@ It ships as a separate, version-matched wheel (`messagefoundry-webconsole`) that in-process; install it alongside the engine and turn on `[api].serve_ui`: ```powershell -pip install "messagefoundry-webconsole==0.1.0" # the /ui web console, into the same venv +pip install -e packaging/messagefoundry-webconsole # the /ui web console, into the same venv +# NOTE: the console is NOT on PyPI yet — install it from the source tree, as above. Installing it +# by bare name from an index would resolve an UNCLAIMED distribution, i.e. whatever a third party +# has uploaded under that name, with its build backend executing at install time (ASVS 15.2.4). # then set [api].serve_ui = true in your service settings and (re)start the engine ``` diff --git a/docs/MENTAL-MODEL.md b/docs/MENTAL-MODEL.md index 9792e010..e5f2c481 100644 --- a/docs/MENTAL-MODEL.md +++ b/docs/MENTAL-MODEL.md @@ -340,7 +340,7 @@ Keep the message store on a fast *local* disk, not a network share — the stage ## 13. Deployment & operations -- **Install:** the supported production artifact is the signed, version-pinned PyPI wheel (pip install "messagefoundry==0.1.0"); then messagefoundry init scaffolds your own config repo (ADR 0017). Extras are opt-in: \[postgres\], \[sqlserver\], \[harness\] (the PySide6 test harness), \[sftp\]. The `/ui` web console installs alongside as the separate `messagefoundry-webconsole` distribution. +- **Install:** the supported production artifact is the signed, version-pinned PyPI wheel (pip install "messagefoundry==0.1.0"); then messagefoundry init scaffolds your own config repo (ADR 0017). Extras are opt-in: \[postgres\], \[sqlserver\], \[harness\] (the PySide6 test harness), \[sftp\]. The `/ui` web console installs alongside as the separate `messagefoundry-webconsole` distribution — **from the source tree** (`pip install -e packaging/messagefoundry-webconsole`) until the release phase publishes it; the name is not yet claimed on PyPI. - **Run headless:** python -m messagefoundry serve --config samples/config --db ./messagefoundry.db --env dev — API on http://127.0.0.1:8765 (GET /connections, /messages, /stats, WS /ws/stats). diff --git a/docs/SERVICE.md b/docs/SERVICE.md index 7a6be31b..bb523c3a 100644 --- a/docs/SERVICE.md +++ b/docs/SERVICE.md @@ -374,7 +374,7 @@ same-origin at `/ui` (not part of the service runtime — a separate, version-ma mounts in-process): ```powershell -pip install "messagefoundry-webconsole" # into the engine venv +pip install -e packaging/messagefoundry-webconsole # into the engine venv (not on PyPI yet) # set [api].serve_ui = true in the service settings, then (re)start the service ``` diff --git a/docs/USER-GUIDE.md b/docs/USER-GUIDE.md index 482d9c19..84af5b35 100644 --- a/docs/USER-GUIDE.md +++ b/docs/USER-GUIDE.md @@ -110,7 +110,7 @@ pip install -e ".[dicom]" # DICOM C-STORE SCP + codec — headers/SR only pip install -e ".[otel]" # OpenTelemetry/OTLP export seam (the /metrics endpoint itself needs no extra) ``` -(For a deployment wheel, the same extras apply: `pip install "messagefoundry[harness]==0.1.0"`, and the web console installs as its own wheel `pip install "messagefoundry-webconsole==0.1.0"`.) SQLite is the zero-dependency default — you need no extra to run the sample config. +(For a deployment wheel, the same extras apply: `pip install "messagefoundry[harness]==0.1.0"`, and the web console installs from the source tree with `pip install -e packaging/messagefoundry-webconsole` — it is **not published to an index yet**, so installing it by bare name would resolve an unclaimed distribution.) SQLite is the zero-dependency default — you need no extra to run the sample config. ### 3. Run the engine headless (dev) diff --git a/messagefoundry/api/app.py b/messagefoundry/api/app.py index 64c72ca4..685a2326 100644 --- a/messagefoundry/api/app.py +++ b/messagefoundry/api/app.py @@ -4934,9 +4934,17 @@ async def _reauthorize() -> Identity | None: try: from messagefoundry_webconsole import assert_engine_seam, mount_ui except ImportError as exc: # pragma: no cover + # ASVS 15.2.4: this string is an INSTALL INSTRUCTION the operator will paste. It named a + # `webconsole` EXTRA that does not exist (pyproject deliberately withholds it until the + # wheel is published — see the note beside [project.optional-dependencies]), + # so the command failed; and an instruction to fetch an UNPUBLISHED distribution name from + # a public index is the dependency-confusion surface this cell is about. Point at the path + # install, which is what actually works today and resolves no index at all. raise RuntimeError( - "serve_ui requires the web console — install it: " - "pip install messagefoundry[webconsole]" + "serve_ui requires the web console, which is not installed. It ships as a separate " + "distribution: install it from the source tree with " + "`pip install -e packaging/messagefoundry-webconsole`, or set [api].serve_ui=false " + "to run JSON-only." ) from exc # Assert the seam BEFORE building the deps bundle (review fix): a package that changed the diff --git a/packaging/messagefoundry-webconsole/README.md b/packaging/messagefoundry-webconsole/README.md index ae0f27e8..bebe63e4 100644 --- a/packaging/messagefoundry-webconsole/README.md +++ b/packaging/messagefoundry-webconsole/README.md @@ -16,11 +16,14 @@ surface (`security`/`models`/`auth_models`/`_ui_seam`), `messagefoundry.auth`, a ## Install ``` -pip install messagefoundry[webconsole] # engine + console -# or, explicitly: -pip install messagefoundry messagefoundry-webconsole +pip install -e packaging/messagefoundry-webconsole # from the source tree, alongside the engine ``` +> **Not on PyPI yet.** This distribution has never been published, so its name is **unclaimed**. +> Installing it by bare name from an index would resolve whatever a third party has uploaded under +> that name — and an sdist executes its build backend during `pip install`, before any engine process +> exists. Install from the source tree until the name is registered (ASVS 15.2.4). + A plain `pip install messagefoundry` stays byte-identical: with the console absent and `serve_ui` default-off, the JSON API is unchanged; `serve_ui=true` without the console fails LOUD at startup. diff --git a/tests/test_install_instruction_provenance.py b/tests/test_install_instruction_provenance.py new file mode 100644 index 00000000..fd43df78 --- /dev/null +++ b/tests/test_install_instruction_provenance.py @@ -0,0 +1,182 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""ASVS 15.2.4: no shipped text may instruct an install that resolves an UNCLAIMED name. + +The defect this guards was live in **shipped code**, not just docs: ``api/app.py``'s ``serve_ui`` +RuntimeError told the operator to run ``pip install messagefoundry[webconsole]`` — an extra +``pyproject.toml`` deliberately withholds until the wheel is published, so the command simply failed. +Three docs additionally told users to ``pip install "messagefoundry-webconsole"`` from an index the +distribution has never been published to. + +That second shape is the one the requirement is actually about. An install instruction naming a +distribution nobody has claimed is a **dependency-confusion primitive**: whoever registers the name on +PyPI first gets their 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. + +**Scope, honestly.** Correcting our instructions removes *our contribution* to that risk. It does not +remove the risk: only claiming the name does. This module is the half that can be automated. + +Two properties, both derived from source rather than hardcoded: + +1. **Every extra named in an install instruction must exist** in ``[project.optional-dependencies]``. +2. **No unpublished distribution may be named in an index-resolving install command.** A path install + (``pip install -e packaging/...``) resolves no index and is always fine; a bare-name install is + only fine once the name is published. +""" + +from __future__ import annotations + +import re +import tomllib +from pathlib import Path + +import pytest + +_ROOT = Path(__file__).resolve().parent.parent + +#: Distributions this repository builds that are **not yet published to any index**. Until a name is +#: claimed, no shipped text may tell a user to install it by bare name. Remove an entry here the day +#: the name is registered -- and note that reserving it is sufficient; an empty project cannot be +#: squatted. +_UNPUBLISHED_DISTRIBUTIONS = frozenset({"messagefoundry-webconsole"}) + +#: Files whose install commands are shipped to, or executed by, someone other than a maintainer. +#: CI workflows and internal handoffs are excluded deliberately: they install from the source tree by +#: path, run only in our own checkout, and are not instructions anyone pastes into a deployment. +_SHIPPED_TEXT_GLOBS = ( + "docs/*.md", + "README.md", + "messagefoundry/**/*.py", + "packaging/messagefoundry-webconsole/README.md", +) + +#: An install command naming a bare distribution -- i.e. one pip will resolve against an INDEX. +#: ``-e `` / a path argument is deliberately not matched: it resolves no index. +_BARE_NAME_INSTALL = re.compile( + r"""(?:pip|uv\s+pip|uv)\s+(?:install|add) # the verb + (?P(?:\s+--?[\w-]+(?:[= ]\S+)?)*) # any flags + \s+["']?(?P[A-Za-z][\w.-]*) # the distribution name + (?P\[[^\]]*\])? # optional [extras] + """, + re.VERBOSE, +) + +#: ``messagefoundry[...]`` where the extras are LITERAL. ``[{extra}]`` / ``[{_EXTRA}]`` are f-string +#: placeholders whose value is filled at runtime from the module's own constant, so they name no extra +#: at rest and are excluded by the ``{`` exclusion in the character class -- not by an allow-list, +#: which would have to be maintained. +_EXTRA_REF = re.compile(r"messagefoundry\[(?P[^\]{}]+)\]") + + +def _declared_extras() -> frozenset[str]: + data = tomllib.loads((_ROOT / "pyproject.toml").read_text(encoding="utf-8")) + return frozenset(data.get("project", {}).get("optional-dependencies", {})) + + +#: Not install instructions, and excluded with a reason rather than silently. +#: ``BACKLOG.md`` is a historical ledger -- it records what past items PROPOSED (including a +#: ``[console]`` extra that was never declared), and rewriting history to satisfy a lint would destroy +#: the record this project relies on. Nobody pastes an install command out of a closed backlog item. +_NOT_INSTRUCTIONS = frozenset({"BACKLOG.md"}) + + +def _shipped_files() -> list[Path]: + seen: list[Path] = [] + for pattern in _SHIPPED_TEXT_GLOBS: + seen.extend(sorted(_ROOT.glob(pattern))) + # docs/security/** is vaulted and not shipped; skip it if a local checkout has it materialized. + return [ + p + for p in seen + if "security" not in p.parts and p.name not in _NOT_INSTRUCTIONS and p.is_file() + ] + + +def test_the_scan_actually_examined_something() -> None: + """Liveness receipt. Every assertion below is a `not found` over a file set built from globs, and + a glob that stops matching turns this module into a wall of green that checks nothing.""" + files = _shipped_files() + assert len(files) >= 20, f"the shipped-text scan matched only {len(files)} files: {files}" + assert _declared_extras(), "pyproject declares no optional-dependencies -- the parse broke" + + +def test_every_extra_named_in_shipped_text_is_declared() -> None: + """Mutation: restore ``pip install messagefoundry[webconsole]`` in ``api/app.py``. Red: named + below, because ``webconsole`` is not in ``[project.optional-dependencies]``.""" + declared = _declared_extras() + problems: list[str] = [] + for path in _shipped_files(): + for lineno, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): + for m in _EXTRA_REF.finditer(line): + for extra in (e.strip() for e in m["extras"].split(",")): + if extra and extra not in declared: + rel = path.relative_to(_ROOT).as_posix() + problems.append(f"{rel}:{lineno} names undeclared extra [{extra}]") + assert not problems, ( + f"shipped text names extras pyproject does not declare: {problems}. Declared: " + f"{sorted(declared)}. An install instruction for a non-existent extra simply fails for the " + f"operator who pastes it." + ) + + +def test_no_shipped_text_installs_an_unpublished_distribution_by_name() -> None: + """The dependency-confusion half (ASVS 15.2.4). + + Mutation: restore ``pip install "messagefoundry-webconsole==0.1.0"`` in docs/INSTALL-GUIDE.md. + Red: named below. A path install of the same distribution stays green, which is the distinction + that matters -- ``-e packaging/messagefoundry-webconsole`` resolves no index and cannot be hijacked. + """ + problems: list[str] = [] + for path in _shipped_files(): + for lineno, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): + for m in _BARE_NAME_INSTALL.finditer(line): + if "-e" in m["flags"] or "--editable" in m["flags"]: + continue + name = m["name"].lower().replace("_", "-") + if name in _UNPUBLISHED_DISTRIBUTIONS: + rel = path.relative_to(_ROOT).as_posix() + problems.append(f"{rel}:{lineno} installs unpublished {name!r} by name") + assert not problems, ( + f"shipped text instructs an INDEX install of a distribution we have not published: " + f"{problems}. Whoever registers that name on PyPI first executes code at install time on " + f"every user who follows it. Use a path install until the name is claimed, then remove it " + f"from _UNPUBLISHED_DISTRIBUTIONS here." + ) + + +def test_the_unpublished_list_does_not_rot() -> None: + """An entry that has since been published would silently keep flagging correct instructions, and + one that was deleted without being published would silently stop guarding. Pin it to a name this + repository actually builds.""" + packaging_dirs = {p.name for p in (_ROOT / "packaging").iterdir() if p.is_dir()} + unknown = sorted(_UNPUBLISHED_DISTRIBUTIONS - packaging_dirs) + assert not unknown, ( + f"_UNPUBLISHED_DISTRIBUTIONS names distributions this repo does not build: {unknown}" + ) + + +@pytest.mark.parametrize( + ("line", "flagged"), + [ + ('pip install "messagefoundry-webconsole==0.1.0"', True), + ("pip install messagefoundry-webconsole", True), + ("uv pip install messagefoundry-webconsole", True), + ("pip install -e packaging/messagefoundry-webconsole", False), + ("uv pip install --system -e packaging/messagefoundry-webconsole", False), + ('pip install "messagefoundry==0.1.0"', False), + ], +) +def test_the_detector_separates_index_installs_from_path_installs(line: str, flagged: bool) -> None: + """Guard-the-guard, and the reason it is worth having: the whole value of the check above is the + path-vs-index distinction. A detector that flagged both would be turned off; one that flagged + neither would be decorative. This pins the boundary in both directions, with no file I/O, so it + keeps working wherever the suite runs.""" + hits = [ + m + for m in _BARE_NAME_INSTALL.finditer(line) + if "-e" not in m["flags"] + and "--editable" not in m["flags"] + and m["name"].lower().replace("_", "-") in _UNPUBLISHED_DISTRIBUTIONS + ] + assert bool(hits) is flagged, f"{line!r}: expected flagged={flagged}, got {bool(hits)}"