diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index e7f3e78d..069e14fe 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -8376,7 +8376,17 @@ Both compute `any(ord(ch) < 0x20 or ord(ch) == 0x7F for ch in ...)`. ## 1240. the FHIR grammar gates use `match` with a `$` anchor, so a trailing newline passes -> 🔢 **Filed 2026-08-13 - not started. NOT EXPLOITABLE TODAY -- the reachability analysis is in the item and it is honest about that.** Value **5/10** · Difficulty **1/10**. Both FHIR grammar gates accept a value with a trailing newline, so on the `fhir_lookup` read path the gate does not enforce the grammar it advertises. Found during #1107 (ASVS 1.2.2). +> ✅ **SHIPPED -- fixed on PR #379, banner authored by the dispatcher because a builder may not author ledger content (owner ruling 2026-08-13).** Filed 2026-08-13. **NOT EXPLOITABLE TODAY -- the reachability analysis is in the item and it is honest about that.** Value **5/10** · Difficulty **1/10**. The defect as filed: both FHIR grammar gates accepted a value with a trailing newline, so on the `fhir_lookup` read path the gate did not enforce the grammar it advertises. Found during #1107 (ASVS 1.2.2). +> +> **VERIFIED BY THE DISPATCHER BEFORE SIGNING THE CLOSURE, by printing the operands on both refs rather than counting them:** +> ``` +> origin/main _FHIR_TYPE_RE = re.compile(r"^[A-Za-z]+$") _FHIR_ID_RE = ...{1,64}$") +> PR #379 head _FHIR_TYPE_RE = re.compile(r"^[A-Za-z]+\Z") _FHIR_ID_RE = ...{1,64}\Z") +> call sites _FHIR_TYPE_RE.match(...) / _FHIR_ID_RE.match(...) UNCHANGED, correctly +> ``` +> **`$` -> `\Z` on the two pattern definitions, which is the durable form:** it covers **all three** call sites at once and cannot be re-broken by a future caller, where converting the calls to `.fullmatch` would fix three sites and leave the fourth caller free to reintroduce it. The read-path `_reject_control_chars` limb was deliberately **not** added -- redundant once the gates are strict, and it would reintroduce the duplication #1239 records as retired. +> +> ⚠️ **The obvious regression test CANNOT DISCRIMINATE and must not be written.** `_resolve_read_url:689` does `raw = query.strip()`, so `"Patient/123\n"` returns an **identical URL before and after the fix**. Only `"Patient\n/123"` flips admitted-to-refused. A test using the stripped form would ship green and prove nothing -- measured by executing the shipped and patched sources, not argued. **Cluster:** Security / input validation. **Priority:** P2. **Verdict:** build (small). **Severity:** Conditional and currently **none** -- see reachability below. The defect is that a control does not do what it claims, which matters independently of whether another control happens to cover for it. @@ -8404,7 +8414,15 @@ _FHIR_ID_RE.fullmatch("abc\n") -> False the fix ## 1241. operator-config values reach URL and header sinks with no construction-time screen -> 🔢 **Filed 2026-08-13 - not started.** Value **5/10** · Difficulty **3/10**. Several operator-configured values are interpolated into URL paths, query strings and an HTTP header with weaker treatment than the message-derived values beside them -- in one case with no screen at all. Found during #1107 (ASVS 1.2.2). **The subject is the asymmetry**, so fixing one site without the others misses the point. +> 🔢 **Filed 2026-08-13. PARTIALLY FIXED on PR #379 and DELIBERATELY STILL OPEN -- see the amendment below. DO NOT CLOSE THIS ON #379.** Value **5/10** · Difficulty **3/10**. Several operator-configured values are interpolated into URL paths, query strings and an HTTP header with weaker treatment than the message-derived values beside them -- in one case with no screen at all. Found during #1107 (ASVS 1.2.2). **The subject is the asymmetry**, so fixing one site without the others misses the point. +> +> ⚠️ **AMENDED 2026-08-13 (dispatcher) -- PARTIAL PROGRESS RECORDED, ITEM STAYS OPEN.** Banner authored by the dispatcher rather than the builder, per the owner's 2026-08-13 ruling that a builder may resolve conflicts but may not author ledger content. **The builder flagged the partiality in its own commit body; this records it in the ledger so a reader of `main` cannot mistake #379 for a closure.** +> +> **WHAT #379 FIXED:** construction-time screening of `url` and `conditional_query`, plus a wrong-exception-class defect that was worse than the filed finding -- `http.client.InvalidURL` derives from `HTTPException`, **not** `ValueError` and **not** `OSError`, so it escaped **every** except arm in `_post` including the backstop written for exactly that case. The result was an unhandled exception out of `send()` rather than the classified dead-letter the file intends. +> +> **WHAT REMAINS, and it is why this stays open:** **`transports/dicomweb.py`**, which this item names and #379 does not touch; and a **SECOND unscreened url-construction site in `FhirLookupExecutor`** in the same file, discovered only because an edit matched two locations. **The item's own framing is the reason a partial close would be wrong -- the subject is the ASYMMETRY, and one sink screened while a sibling is not reproduces exactly the defect being reported.** +> +> **TWO CORRECTIONS TO THE FILED TEXT, neither reducing severity.** The comparison clause does not merely go stale, it **INVERTS**: the neighbouring flat-search path it called "weaker but at least screening" was removed outright by `039757ff`, so `:431` became the **only** unencoded interpolation left in the file -- which **strengthens** this item rather than weakening it. And the enum rationale is **right advice for the wrong reason**: containment comes from the `!r` conversion escaping control characters, not from the enum's closedness. **Replace the reason or the next reader copies the enum argument to a site with no `!r`.** > ⚠️ **Amendment 2026-08-13 -- the item's JUSTIFICATION was the weaker of the two available, and the stronger one is a measured fact. Severity is UNCHANGED and deliberately not upgraded.** > diff --git a/messagefoundry/transports/dicomweb.py b/messagefoundry/transports/dicomweb.py index 9e25b19b..bd7f71b6 100644 --- a/messagefoundry/transports/dicomweb.py +++ b/messagefoundry/transports/dicomweb.py @@ -141,6 +141,7 @@ def __init__(self, config: Destination) -> None: raise ValueError( "DICOMweb destination requires a 'url' setting (the DICOMweb service base URL)" ) + _reject_url_control_chars(url, "url") scheme = urllib.parse.urlsplit(url).scheme.lower() if scheme not in ("http", "https"): raise ValueError( @@ -248,9 +249,17 @@ def _build_headers(self, s: dict[str, Any]) -> dict[str, str]: headers: dict[str, str] = {"Accept": _DICOM_JSON} extra = s.get("headers") or {} if isinstance(extra, dict): + # Screened for the same reason study_uid is, and NAMES as well as values: both halves land + # on the wire, so a CRLF in either splits the request. Unlike a URL a header value has no + # incidental neutralisation downstream -- nothing strips or re-encodes it. + for k, v in extra.items(): + _reject_url_control_chars(str(k), "header name") + _reject_url_control_chars(str(v), f"header {str(k)!r} value") headers.update({str(k): str(v) for k, v in extra.items()}) token = s.get("bearer_token") if token: + # PHI/secret-safe: the helper names the field, never the value. + _reject_url_control_chars(str(token), "bearer_token") headers["Authorization"] = f"Bearer {token}" user, password = s.get("basic_user"), s.get("basic_password") if user and password: diff --git a/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index d9d66a49..7293e95c 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -36,6 +36,7 @@ import asyncio import base64 +import http.client import json import logging import re @@ -101,8 +102,12 @@ # the message-derived path segments so a crafted resource can't smuggle '/', '..', '?', '#', or '@' # into the request path and redirect a PHI-bearing write to a different resource/operation on the same # allow-listed host (the [egress].allowed_http gate pins the host, not the path). -_FHIR_TYPE_RE = re.compile(r"^[A-Za-z]+$") -_FHIR_ID_RE = re.compile(r"^[A-Za-z0-9.\-]{1,64}$") +# `\Z`, never `$`: Python's `$` also matches immediately BEFORE a final newline, so `^[A-Za-z]+$` +# accepted "Patient\n" and the gate did not enforce the grammar it advertises. Anchoring the pattern +# fixes every caller at once -- `match` vs `fullmatch` is a property of the CALL, and there are three +# call sites (:189, :698, :704), so a per-call fix leaves the next one to re-introduce it. +_FHIR_TYPE_RE = re.compile(r"^[A-Za-z]+\Z") +_FHIR_ID_RE = re.compile(r"^[A-Za-z0-9.\-]{1,64}\Z") def _operation_outcome(body: str) -> dict[str, Any] | None: @@ -179,6 +184,25 @@ def _reject_control_chars(value: str, field: str) -> str: return value +def _reject_config_control_chars(value: str, setting: str, where: str = "destination") -> str: + """Reject an OPERATOR-CONFIGURED value carrying a C0/DEL control char, at CONSTRUCTION time. + + Deliberately distinct from ``_reject_control_chars``, which screens MESSAGE-derived values on the + send path and raises a permanent ``NegativeAckError``. The distinction is the disposition: a bad + *message* dead-letters one message, whereas a bad *setting* is wrong for every message the + connection will ever send -- so it must fail the connection at load rather than dead-letter an + unbounded stream of messages that were never at fault. Raises ``ValueError`` to match the other + construction-time setting checks. PHI-safe: names the setting, never the value. + + ``where`` carries the construction site because there are TWO in this module -- the destination + and the read executor -- and #1241's subject is precisely the asymmetry of screening one and not + its sibling. + """ + if any(ord(ch) < 0x20 or ord(ch) == 0x7F for ch in value): + raise ValueError(f"FHIR {where} {setting!r} contains an illegal control character") + return value + + def _validate_path_token(value: str, pattern: re.Pattern[str], field: str) -> str: """Reject a message-derived path segment that doesn't match its FHIR grammar before it flows into the request URL. ``_reject_control_chars`` blocks CRLF/NUL but NOT path metacharacters ('/', '..', @@ -205,6 +229,7 @@ def __init__(self, config: Destination) -> None: raise ValueError( "FHIR destination requires a 'url' setting (the FHIR service base URL)" ) + _reject_config_control_chars(url, "url") scheme = urllib.parse.urlsplit(url).scheme.lower() if scheme not in ("http", "https"): raise ValueError(f"FHIR destination 'url' must be http or https, got scheme {scheme!r}") @@ -228,7 +253,14 @@ def __init__(self, config: Destination) -> None: f"FHIR destination conditional must be one of {_CONDITIONALS} or unset, " f"got {self.conditional!r}" ) - self.conditional_query: str | None = s.get("conditional_query") or None + # Screened HERE rather than on the send path: it reaches an unencoded URL interpolation AND + # the If-None-Exist HEADER value, and the header sink has none of the URL limb's incidental + # neutralisations (urllib.parse.unwrap strips a trailing CRLF; Request.full_url splits at '#' + # client-side -- neither touches a header value). + _q = s.get("conditional_query") or None + self.conditional_query: str | None = ( + _reject_config_control_chars(str(_q), "conditional_query") if _q else None + ) if ( self.conditional in ("if-none-exist", "conditional-update") and not self.conditional_query @@ -631,10 +663,16 @@ def _post( raise DeliveryError( f"FHIR {_redact_url(self.base_url)} unreachable: {exc.reason}" ) from exc - except ValueError as exc: + except (ValueError, http.client.InvalidURL) as exc: # Backstop for an illegal request value urllib rejects (a CRLF in a header/URL that slipped # past the control-char guard, or a bad conditional_query) — a permanent failure (a retry # re-sends the same body), never an escaping internal error. PHI-safe: redacted url only. + # + # InvalidURL is named EXPLICITLY because it is not a ValueError: its MRO is + # InvalidURL -> HTTPException -> Exception, so it is neither a ValueError nor an OSError + # and matched none of the arms here — including this one, which was written for exactly + # the CRLF-in-a-URL case it raises. Without it the URL limb escapes send() as an + # unhandled internal error instead of the classified permanent dead-letter above. raise NegativeAckError( f"FHIR {_redact_url(self.base_url)} rejected an invalid request value", code="bad-request-value", @@ -753,6 +791,9 @@ def __init__(self, connections: Mapping[str, Mapping[str, Any]]) -> None: raise ValueError( f"FhirLookup {cname!r} requires a 'url' setting (the FHIR base URL)" ) + # #1241: the SECOND url construction site in this module. The destination screens its + # own; screening one and not its sibling reproduces the asymmetry the item reports. + _reject_config_control_chars(url, "url", f"lookup {cname!r}") scheme = urllib.parse.urlsplit(url).scheme.lower() if scheme not in ("http", "https"): raise ValueError( diff --git a/tests/test_dicomweb.py b/tests/test_dicomweb.py index b421f41a..7d02230f 100644 --- a/tests/test_dicomweb.py +++ b/tests/test_dicomweb.py @@ -126,6 +126,49 @@ def test_dicomweb_study_uid_control_char_rejected() -> None: _dest(study_uid="1.2.3\r\nHost: evil") +@pytest.mark.parametrize( + ("setting", "value"), + [ + ("headers", {"X-Site": "a\r\nX-Evil: 1"}), # CRLF in an operator header VALUE + ("headers", {"X-Site\r\nX-Evil": "1"}), # CRLF in an operator header NAME + ("headers", {"X-Site": "a\x00b"}), # NUL in a value + ("bearer_token", "tok\r\nX-Evil: 1"), # CRLF in the credential -> Authorization header + ], +) +def test_dicomweb_operator_header_control_char_rejected( + setting: str, value: object +) -> None: # #1241 + """`study_uid` was screened at construction; the other operator-configured settings that reach the + same wire were not. `headers` merged straight into the request headers and `bearer_token` went + into `Authorization` verbatim, so a CRLF in either is a header injection with nothing in front of + it -- and unlike a URL there is no incidental neutralisation on a header value. + + Screened at CONSTRUCTION for the same reason as the sibling settings: a bad SETTING is wrong for + every message the connection will ever send, so it must fail the connection at load rather than + dead-letter an unbounded stream of messages that were never at fault. + """ + with pytest.raises(ValueError, match="illegal control character"): + _dest(**{setting: value}) # type: ignore[arg-type] + + +def test_dicomweb_base_url_control_char_rejected() -> None: # #1241 + with pytest.raises(ValueError, match="illegal control character"): + build_destination( + Destination( + name="OB", + type=ConnectorType.DICOMWEB, + settings=DICOMweb(url="https://pacs.example.org/dicom-web\r\nX-Evil: 1").settings, + ) + ) + + +def test_dicomweb_clean_operator_headers_still_construct() -> None: # #1241 + """Positive control: the screen must admit what it is not screening for.""" + d = _dest(headers={"X-Site": "site-a", "X-Trace": "abc-123"}) + assert d._headers["X-Site"] == "site-a" + assert d._headers["X-Trace"] == "abc-123" + + def test_dicomweb_cleartext_credentials_refused() -> None: # Basic/bearer over plain http puts the credential on the wire — refused (mirrors REST/FHIR). with pytest.raises(ValueError, match="cleartext"): diff --git a/tests/test_fhir_lookup.py b/tests/test_fhir_lookup.py index 8b114983..bc79b82a 100644 --- a/tests/test_fhir_lookup.py +++ b/tests/test_fhir_lookup.py @@ -237,6 +237,37 @@ def test_resolve_read_url_rejects_bad_path(query: str) -> None: _resolve_read_url(BASE, query) +@pytest.mark.parametrize( + "query", + [ + "Patient\n/123", # LF ends the resourceType segment + "Patient\n/123\n", # LF ends both segments + ], +) +def test_resolve_read_url_rejects_lf_terminated_segment(query: str) -> None: # #1240 + r"""Python's `$` matches BEFORE a final newline, so `^...$` accepted a segment ending in LF. + + The patterns use `\Z` for exactly this. THE SHAPE MATTERS AND THE OBVIOUS TEST DOES NOT + DISCRIMINATE: a trailing LF on the whole query (`Patient/123\n`) is normalised away upstream and + builds a URL byte-identical to the clean input, measured before and after the fix -- so a test + written that way passes either way and proves nothing. Only an LF ending a segment that is + followed by more path reaches a gate as a newline-bearing token, and that is what flips from + BUILT to refused here. + """ + with pytest.raises(ValueError): + _resolve_read_url(BASE, query) + + +def test_lf_terminated_query_is_normalised_not_gated() -> None: # #1240 + r"""Pins WHY the sibling test uses the shape it does, so nobody 'simplifies' it back. + + `Patient/123\n` builds the same URL as `Patient/123`. That is upstream normalisation, NOT the + grammar gate doing its job -- if this ever starts raising, the sibling test above is no longer + the discriminating case and needs re-deriving rather than deleting. + """ + assert _resolve_read_url(BASE, "Patient/123\n") == _resolve_read_url(BASE, "Patient/123") + + async def test_read_rejects_bad_query_phi_safe() -> None: ex, opener = _executor(body=PATIENT.encode()) with pytest.raises(FhirLookupError) as ei: @@ -481,6 +512,35 @@ def test_executor_rejects_non_http_scheme() -> None: FhirLookupExecutor({"bad": {"url": "ftp://h/fhir"}}) +@pytest.mark.parametrize( + "url", + [ + "https://h/fhir\r\nX-Evil: 1", # CRLF -- request splitting / header injection + "https://h/fhir\n", # bare LF + "https://h/\x00fhir", # NUL + ], +) +def test_executor_rejects_control_char_in_url(url: str) -> None: # #1241 + """The READ executor screens its operator-configured base URL, exactly as the destination does. + + #1241's subject is the ASYMMETRY: one sink screened while a sibling is not reproduces the very + defect the item reports. This is that sibling -- a second url construction site in the same + module, reached from operator config, previously checked for type and scheme only. + + Screened at CONSTRUCTION rather than per call: a bad setting is wrong for every lookup this + connection will ever serve, so it fails the connection at load rather than failing an unbounded + stream of reads that were never at fault. + """ + with pytest.raises(ValueError, match="control character"): + FhirLookupExecutor({"bad": {"url": url}}) + + +def test_executor_clean_url_still_constructs() -> None: # #1241 + """Positive control: the screen must admit what it is not screening for.""" + ex = FhirLookupExecutor({"ok": {"url": "https://h/fhir"}}) + assert "ok" in ex.connections + + # --- fail-closed egress gate (AC-4) ------------------------------------------ diff --git a/tests/test_fhir_transport.py b/tests/test_fhir_transport.py index 903e77f7..136a8630 100644 --- a/tests/test_fhir_transport.py +++ b/tests/test_fhir_transport.py @@ -11,6 +11,7 @@ from __future__ import annotations import email.message +import http.client import io import json import urllib.error @@ -240,6 +241,80 @@ def test_resolve_if_match_versionid_with_control_char_is_permanent() -> None: assert ei.value.permanent is True +@pytest.mark.parametrize( + "value", + [ + "identifier=x\r\nX-Evil: 1", # CRLF -- header injection via the If-None-Exist sink + "identifier=x\nX-Evil: 1", # bare LF + "identifier=x\x00", # NUL + "identifier=x\x7f", # DEL + ], +) +def test_conditional_query_control_char_is_refused_at_construction(value: str) -> None: # #1241 + """An operator-configured `conditional_query` reaches TWO sinks with no screen between config and + wire: an unencoded URL interpolation, and the `If-None-Exist` HEADER value. + + Screened at CONSTRUCTION, not per message, and the distinction is the point. A bad *message* is a + permanent dead-letter -- one message fails. A bad *setting* is wrong for every message the + connection will ever send, so it must fail the connection at load rather than dead-letter an + unbounded stream of messages that were never at fault. + + The header sink is why this cannot be left to the send path: unlike the URL limb it has NO + incidental neutralisation -- `urllib.parse.unwrap` strips a trailing CRLF and `Request.full_url` + splits at '#' client-side, and neither touches a header value. + """ + with pytest.raises(ValueError, match="control character"): + _dest(conditional="if-none-exist", conditional_query=value) + + +def test_clean_conditional_query_still_constructs() -> None: # #1241 + """Positive control for the screen: it must admit what it is not screening for.""" + d = _dest(conditional="if-none-exist", conditional_query="identifier=http://h|123") + assert d.conditional_query == "identifier=http://h|123" + + +def test_base_url_control_char_is_refused_at_construction() -> None: # #1241 + # Built directly rather than through _dest, which already supplies url=. + bad = Destination( + name="OB_FHIR", + type=ConnectorType.FHIR, + settings={"url": "https://fhir.example.org/fhir\r\nX-Evil: 1"}, + ) + with pytest.raises(ValueError, match="control character"): + build_destination(bad) + + +def test_invalid_url_from_urllib_is_a_permanent_dead_letter() -> None: # #1241 + """`http.client.InvalidURL` must not escape `_post` as an unhandled exception. + + THE GAP IT CLOSES: InvalidURL derives from HTTPException, NOT ValueError and NOT OSError + (`InvalidURL -> HTTPException -> Exception`), so it matched none of `_post`'s arms -- including + the ValueError backstop whose own comment says it exists for "a CRLF in a header/URL that + slipped past the control-char guard". That is precisely this exception, and it escaped the arm + written for it. On first deployment the URL limb would surface as an internal error out of + `send()` rather than the classified permanent dead-letter the file intends. + + The sibling arms are asserted below so this cannot pass by the whole method being widened. + """ + dest = _dest() + dest._opener = _FakeOpener( # type: ignore[assignment] + exc=http.client.InvalidURL("URL can't contain control characters") + ) + with pytest.raises(NegativeAckError) as ei: + dest._post(PATIENT, "POST", f"{BASE}/Patient", {}) + assert ei.value.permanent is True + + +def test_invalid_url_fix_did_not_widen_the_other_arms() -> None: # #1241 + """Negative control for the test above: a connection failure must STILL be a retryable + DeliveryError, not swept into the permanent dead-letter class.""" + dest = _dest() + dest._opener = _FakeOpener(exc=urllib.error.URLError("connection refused")) # type: ignore[assignment] + with pytest.raises(DeliveryError) as ei: + dest._post(PATIENT, "POST", f"{BASE}/Patient", {}) + assert not isinstance(ei.value, NegativeAckError) + + def test_resolve_id_with_control_char_is_permanent() -> None: bad_id = json.dumps({"resourceType": "Patient", "id": "p\r\n1"}) # CRLF in the URL-path id with pytest.raises(NegativeAckError) as ei: