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/SECURITY.md b/docs/SECURITY.md index c9eb0078..dfd03da4 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 on its own; `[auth].ad_session_recheck_seconds` (default **300 s**) 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/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/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/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/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/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/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/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/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/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/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/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/dicomweb.py b/messagefoundry/transports/dicomweb.py index 7761b7b3..9e25b19b 100644 --- a/messagefoundry/transports/dicomweb.py +++ b/messagefoundry/transports/dicomweb.py @@ -229,6 +229,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 7813790c..189a040f 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -72,6 +72,9 @@ cleartext_acceptance_from_settings, 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, @@ -303,6 +306,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. @@ -510,6 +516,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() @@ -546,6 +572,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, @@ -795,6 +843,10 @@ def __init__( cleartext_reason=lk_reason, connection=cname, ) + # 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)): @@ -887,6 +939,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" ) @@ -939,6 +1004,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 e812df80..e6ff1b35 100644 --- a/messagefoundry/transports/http_auth.py +++ b/messagefoundry/transports/http_auth.py @@ -52,6 +52,7 @@ _no_redirect_opener, _redact_url, cleartext_acceptance_from_settings, + enforce_outbound_length_limits, proxy_auth_handler_from_settings, refuse_cleartext_credential_hop, ) @@ -243,6 +244,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 272a94a7..b562d8ff 100644 --- a/messagefoundry/transports/rest.py +++ b/messagefoundry/transports/rest.py @@ -195,6 +195,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}) @@ -641,23 +647,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) ----------------------------------- @@ -1090,6 +1237,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); @@ -1257,6 +1407,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() @@ -1306,6 +1476,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 e982b1a6..0ce3525d 100644 --- a/messagefoundry/transports/smart.py +++ b/messagefoundry/transports/smart.py @@ -56,6 +56,7 @@ _no_redirect_opener, _redact_url, cleartext_acceptance_from_settings, + enforce_outbound_length_limits, refuse_cleartext_credential_hop, ) from messagefoundry.transports.signing import CompactJwtSigner @@ -227,6 +228,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 59473ca5..72e78e7c 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, @@ -384,6 +386,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 @@ -745,6 +750,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" ) @@ -789,6 +798,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/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/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/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_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" 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_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_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_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)}" diff --git a/tests/test_rest_transport.py b/tests/test_rest_transport.py index 7314c22e..b224ee65 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 @@ -155,6 +156,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): 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." + ) diff --git a/tests/test_security_doc_drift.py b/tests/test_security_doc_drift.py index f12de451..24a7d1b0 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_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. 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"), ],