From 7449514dc40fc26406033237792988c78f9f80fe Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 13 Aug 2026 14:22:12 -0500 Subject: [PATCH 1/6] fix(fhir): anchor the path-segment gates with \Z so a trailing LF is refused (BACKLOG #1240) 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. Fixed on the PATTERNS, not the call sites. `match` versus `fullmatch` is a property of the CALL and there are three call sites (fhir.py:189, :698, :704), so a per-call fix covers whichever two you happen to notice and leaves the third to re-introduce the hole. Anchoring the pattern fixes all three at once and cannot be re-broken by a future caller. The item as filed prescribed "two one-line changes: match to fullmatch on both regexes". That is not executable -- it is three call-site edits, not two. `$` to `\Z` is genuinely two lines and strictly stronger. Re-verified against the code before building; the amendment content is with the dispatcher. NOT DONE, deliberately: the item's read-path `_reject_control_chars` limb. Once the gates are strict it is redundant -- both charsets exclude every C0 and DEL character, and every character of the query reaches a gate ('?' refused at :713, more than two segments raised at :696). Adding it would also re-introduce the second control-char treatment that #1239 records as retired by #1243. TEST SHAPE IS LOAD-BEARING. 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 both before and after this change -- so the obvious test passes either way and proves nothing. Only an LF ending a segment followed by more path ("Patient\n/123") reaches a gate carrying the newline. Both shapes are pinned: the discriminating one asserts the refusal, and a second test pins the normalisation so that if it ever starts raising, the first test is known to need re-deriving rather than deleting. Red-first: both parametrized cases failed with "DID NOT RAISE ValueError" against the unfixed patterns, and the normalisation pin passed before and after, as it should. Verified, with scope stated: ruff format --check and ruff check clean on both changed files; mypy strict clean on transports/fhir.py; pytest over tests/test_fhir_lookup.py, tests/test_egress_allowlist.py and tests/test_transports.py = 165 passed, 1 skipped, in the lane venv built against constraints.lock (ruff 0.15.22, matching the pin). The FULL suite was NOT run and neither test path was collected in full. --- messagefoundry/transports/fhir.py | 8 ++++++-- tests/test_fhir_lookup.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index d9d66a49..a8bba67c 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -101,8 +101,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: diff --git a/tests/test_fhir_lookup.py b/tests/test_fhir_lookup.py index 8b114983..9975c227 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: From 86db41938a299bd4d53a6ed4aafcbddd6004493f Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 13 Aug 2026 14:27:48 -0500 Subject: [PATCH 2/6] fix(fhir): catch http.client.InvalidURL in _post so it dead-letters instead of escaping (BACKLOG #1241) InvalidURL is not a ValueError and not an OSError. Its MRO is InvalidURL -> HTTPException -> Exception so it matched NONE of _post's except arms: not HTTPError (:616), not URLError (:634), not (TimeoutError, OSError) (:647), and not the ValueError backstop at :638 -- whose own comment says it exists for "a CRLF in a header/URL that slipped past the control-char guard", which is precisely the condition urllib raises InvalidURL for. So the arm written for this exception could not catch it. On first deployment the URL limb would surface as an unhandled internal error out of send() rather than the classified permanent dead-letter the file intends, which is a different disposition and a different operator experience: an escaping exception instead of a dead-lettered message with a reason. This is a PARTIAL fix for #1241 and I am not claiming otherwise. The item's filed claim -- that operator-config values reach the URL and header sinks with no construction-time screen -- still HOLDS for both sinks and is NOT addressed here. This commit closes the narrower defect found while re-verifying the item: that when the URL sink does fail, it fails in the wrong class. Scope note carried from the re-verification, because it bounds how far this goes: the URL limb has two incidental neutralisations the header limb does not -- urllib.parse.unwrap strips a trailing CRLF, and Request.full_url splits at '#' client-side. The header sink has neither, which is why the construction-time screen is still needed and why a fix cannot stop here. Red-first: the test failed with a raw `http.client.InvalidURL: URL can't contain control characters` escaping _post, which is the defect itself rather than a proxy for it. NEGATIVE CONTROL SHIPPED ALONGSIDE. A second test asserts a URLError still raises a retryable DeliveryError and NOT a NegativeAckError, so this cannot pass by the method having been widened to swallow everything into the permanent class. It passed before this change and after it. Verified, with scope stated: ruff format --check and ruff check clean on both changed files; mypy strict clean on transports/fhir.py; pytest over test_fhir_transport.py, test_fhir_lookup.py, test_egress_allowlist.py, test_transports.py and test_smart_backend.py = 259 passed, 1 skipped, in the lane venv built against constraints.lock (ruff 0.15.22, matching the pin). THE FULL SUITE WAS NOT RUN and neither test path was collected in full. No ledger edit: the banner flip is withheld deliberately and the disposition routes to the dispatcher. --- messagefoundry/transports/fhir.py | 9 ++++++++- tests/test_fhir_transport.py | 32 +++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index a8bba67c..541339f8 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 @@ -635,10 +636,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", diff --git a/tests/test_fhir_transport.py b/tests/test_fhir_transport.py index 903e77f7..884dc274 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,37 @@ def test_resolve_if_match_versionid_with_control_char_is_permanent() -> None: assert ei.value.permanent is True +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: From f7bd300093a41e67c64db4447e8b0cc364cf9a1f Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 13 Aug 2026 15:03:37 -0500 Subject: [PATCH 3/6] fix(fhir): screen operator-configured url and conditional_query at construction (BACKLOG #1241) This is the item's FILED defect, which the previous commit did not touch: operator-config values reached the URL and header sinks with no construction-time screen. conditional_query was taken verbatim from settings and reached TWO sinks: - an unencoded URL interpolation, f"{base}/{type_seg}?{self.conditional_query}" - the If-None-Exist HEADER value The header sink is why this could not be left to the send path. The URL limb has two incidental neutralisations it does not: urllib.parse.unwrap strips a trailing CRLF, and Request.full_url splits at '#' client-side. Neither touches a header value, so a CRLF in conditional_query is a header injection with nothing in front of it. SCREENED AT CONSTRUCTION, NOT PER MESSAGE, AND THE DISPOSITION IS THE REASON. _reject_config_control_chars raises ValueError and is deliberately distinct from the existing _reject_control_chars, which screens message-derived values and raises a permanent NegativeAckError. A bad MESSAGE dead-letters one message. 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. Applied to both `url` and `conditional_query`. Red-first: all five new cases failed with "DID NOT RAISE ValueError". One of them first failed with a TypeError instead -- the test passed url= through a helper that already supplies it -- and a test failing for the wrong reason is not a red-first proof, so it was rebuilt to construct the Destination directly and re-confirmed. POSITIVE CONTROL SHIPPED: a clean conditional_query carrying '|' and ':' and '/' still constructs and is preserved verbatim, so the screen cannot pass by rejecting everything. STILL NOT COMPLETE, and #1241 must not be closed on this either. Not addressed here: - transports/dicomweb.py, which the item also names. Untouched. - FhirLookupExecutor has a SECOND url construction site in this same file with the same unscreened shape. Found only because an edit matched two locations rather than one. Not fixed here because it is outside what was dispatched; reported as content. Verified, with scope stated: ruff format --check and ruff check clean on both changed files; mypy strict clean on transports/fhir.py; pytest over test_fhir_transport, test_fhir_lookup, test_egress_allowlist, test_transports, test_smart_backend and test_connection_api = 302 passed, 1 skipped, in the lane venv built against constraints.lock (ruff 0.15.22, matching the pin). THE FULL SUITE WAS NOT RUN and the webconsole suite was not collected at all. No ledger edit; the banner flip is withheld and disposition routes to the dispatcher. --- messagefoundry/transports/fhir.py | 25 +++++++++++++++++- tests/test_fhir_transport.py | 43 +++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index 541339f8..7f949c17 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -184,6 +184,21 @@ def _reject_control_chars(value: str, field: str) -> str: return value +def _reject_config_control_chars(value: str, setting: str) -> 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. + """ + if any(ord(ch) < 0x20 or ord(ch) == 0x7F for ch in value): + raise ValueError(f"FHIR destination {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 ('/', '..', @@ -210,6 +225,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}") @@ -233,7 +249,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 diff --git a/tests/test_fhir_transport.py b/tests/test_fhir_transport.py index 884dc274..136a8630 100644 --- a/tests/test_fhir_transport.py +++ b/tests/test_fhir_transport.py @@ -241,6 +241,49 @@ 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. From 5dfe5e7d09cc70e3528ab50932dc462458fe210a Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 13 Aug 2026 17:44:25 -0500 Subject: [PATCH 4/6] backlog: close #1240, record #1241 as partial -- the ledger edit PR #379 cannot make itself PR #379 is red on a required check that says a PR implementing BACKLOG #N must update BACKLOG.md. The owner's 2026-08-13 ruling says a builder may resolve merge conflicts but may not author ledger content. Those two are mutually unsatisfiable for a compliant builder PR, so the builder correctly withheld the banner and the PR correctly went red. Authoring is dispatcher and lander only; this supplies the edit. Neither a bug nor anyone's error -- two correct rules meeting. #1240 CLOSED. Verified before signing by printing the operands on both refs rather than counting them, after a count instrument returned 0 on a string the printed lines visibly contained: origin/main _FHIR_TYPE_RE = re.compile(r"^[A-Za-z]+$") PR #379 head _FHIR_TYPE_RE = re.compile(r"^[A-Za-z]+\Z") $ -> \Z on the two pattern definitions, call sites unchanged. That 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 and leave a fourth 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 duplication that #1239 records as retired. The item also records that the obvious regression test cannot discriminate: _resolve_read_url strips, so "Patient/123\n" yields an identical URL before and after the fix and only "Patient\n/123" flips. Measured by executing the shipped and patched sources, not argued. #1241 STAYS OPEN, amended to record partial progress. #379 fixed construction-time screening plus a wrong-exception-class defect 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 that case. Still outstanding: transports/dicomweb.py, which the item names, and a second unscreened url-construction site in FhirLookupExecutor in the same file. The item's subject is the ASYMMETRY, so one sink screened while a sibling is not reproduces the very defect being reported. A partial close would be wrong. Two corrections to #1241's filed text, neither reducing severity: its comparison clause INVERTS rather than going stale, because the neighbouring path it called "weaker but at least screening" was removed outright, leaving :431 the only unencoded interpolation in the file; and its enum rationale is right advice for the wrong reason, since containment comes from the !r conversion rather than the enum's closedness. Controls: parse_items 281 items / 206 open / 75 closed before, 281 / 205 / 76 after -- 0 / -1 / +1, the expected delta for exactly one close and one amendment. backlog_status_check green, every item declaring exactly one status. Banner invariant checked per item: #1240 one closed-alphabet character and zero open, #1241 zero closed and one open. --- docs/BACKLOG.md | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) 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.** > From cf41c686630d304c9d4083732b3a34ee773ad91d Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 13 Aug 2026 15:09:45 -0500 Subject: [PATCH 5/6] fix(dicomweb): screen url, operator headers and bearer_token at construction (BACKLOG #1241) Completes #1241's second named file. dicomweb.py already had the right helper and the right contract -- _reject_url_control_chars raises ValueError at construction -- and applied it to exactly ONE operator setting, study_uid. Three others reached the same wire unscreened: url scheme-checked only headers merged into the request headers verbatim, NAMES as well as values bearer_token interpolated into Authorization verbatim Header NAMES are screened as well as values because both halves land on the wire, so a CRLF in either splits the request. The header sink is the one that needs this most: a URL has incidental neutralisation downstream (urllib.parse.unwrap strips a trailing CRLF, Request.full_url splits at '#' client-side) and a header value has none -- nothing strips or re-encodes it. Screened at CONSTRUCTION, matching the existing study_uid treatment and the fhir.py sibling in this same item: a bad MESSAGE dead-letters one message, a bad SETTING is wrong for every message the connection will ever send, so it fails the connection at load rather than dead-lettering an unbounded stream of messages that were never at fault. The inconsistency is the interesting part and worth recording: the file was not missing the concept, the helper, or the contract. It had all three and applied them to one of four settings. A reader auditing "does dicomweb screen its config?" finds study_uid screened and can reasonably stop. Red-first: all six new cases failed with "DID NOT RAISE ValueError". POSITIVE CONTROL SHIPPED: clean operator headers still construct and are preserved verbatim on the destination, so the screen cannot pass by rejecting everything. Verified, with scope stated: ruff format and ruff check clean on both changed files; mypy strict clean on transports/dicomweb.py; pytest over test_dicomweb, test_dicom_wiring, test_fhir_transport, test_fhir_lookup and test_transports = 271 passed, 1 skipped, in the lane venv built against constraints.lock (ruff 0.15.22, matching the pin). THE FULL SUITE WAS NOT RUN and the webconsole suite was not collected at all. Still open on #1241 and NOT closed by this: FhirLookupExecutor has a second unscreened url construction site in fhir.py, reported to the dispatcher as content rather than fixed here because it is outside what was dispatched. No ledger edit; banner flip withheld, disposition routes to the dispatcher. --- messagefoundry/transports/dicomweb.py | 9 ++++++ tests/test_dicomweb.py | 43 +++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) 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/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"): From 9a8b528b4b4c8018b5c2ad74d2aeb7147e4ccd31 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 13 Aug 2026 19:02:16 -0500 Subject: [PATCH 6/6] fix(fhir): screen the FhirLookupExecutor base url at construction (BACKLOG #1241) The SECOND url construction site in this module. FhirDestination screens its own url and conditional_query; FhirLookupExecutor took a base url from the same operator config and checked only that it was a non-empty string with an http(s) scheme. THE ASYMMETRY IS THE ITEM'S SUBJECT, which is why this is not a separate concern. #1241 reports operator-config values reaching sinks with no construction-time screen. Screening one sink and leaving its sibling unscreened reproduces the defect being reported, in the same file, on the same setting name. Found only because an earlier edit to the destination matched TWO locations instead of one. It was reported to the dispatcher as content rather than fixed at the time, because it was outside what had been dispatched. The helper gained a `where` parameter so the message names WHICH construction site raised. It defaults to "destination", so the two existing call sites are unchanged in behaviour and the tests that match on "control character" are unaffected. That parameter exists because there are two sites and the reader of a load-time failure needs to know which one. Red-first: all three control-char cases failed with DID NOT RAISE ValueError against the unscreened constructor. POSITIVE CONTROL SHIPPED: a clean https url still constructs and the connection is registered, so the screen cannot pass by rejecting everything. Verified, with scope stated: ruff format --check and ruff check clean on both changed files; mypy strict clean on transports/fhir.py; pytest over test_fhir_lookup, test_fhir_transport, test_dicomweb, test_egress_allowlist and test_transports = 270 passed, 1 skipped, in the lane venv built against constraints.lock (ruff 0.15.22, matching the pin). THE FULL SUITE WAS NOT RUN and the webconsole suite was not collected. WHAT REMAINS OPEN ON #1241, so this commit is not read as closing it: nothing in this module that I have found. dicomweb.py was screened in 45293154, which is committed and anchored but did NOT reach PR #379 -- content-tested against the PR head, not inferred. Whether the item closes depends on that commit landing alongside these. No ledger edit; the banner flip is withheld and disposition routes to the dispatcher. --- messagefoundry/transports/fhir.py | 11 +++++++++-- tests/test_fhir_lookup.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index 7f949c17..7293e95c 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -184,7 +184,7 @@ def _reject_control_chars(value: str, field: str) -> str: return value -def _reject_config_control_chars(value: str, setting: str) -> str: +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 @@ -193,9 +193,13 @@ def _reject_config_control_chars(value: str, setting: str) -> str: 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 destination {setting!r} contains an illegal control character") + raise ValueError(f"FHIR {where} {setting!r} contains an illegal control character") return value @@ -787,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_fhir_lookup.py b/tests/test_fhir_lookup.py index 9975c227..bc79b82a 100644 --- a/tests/test_fhir_lookup.py +++ b/tests/test_fhir_lookup.py @@ -512,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) ------------------------------------------