fix(fhir): anchor path-segment patterns, catch InvalidURL, and screen operator config at construction (#1240, #1241)

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.**
 >

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:

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(

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"):

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) ------------------------------------------