fix(fhir): anchor path-segment patterns, catch InvalidURL, and screen operator config at construction (#1240, #1241) by wshallwshall · Pull Request #379 · MEFORORG/MessageFoundry

wshallwshall · 2026-08-13T19:25:24Z

Anchors the two FHIR path-segment patterns with \Z instead of $ (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 -- deliberately

The item as filed prescribed "two one-line changes: match to fullmatch on both regexes". That is
not executable: match versus fullmatch is a property of the call, and there are three call
sites (:189, :698, :704), so it would be three edits and a future caller could re-introduce it.

$ to \Z is genuinely two lines, covers all three call sites, and cannot be re-broken by a new caller.

Deliberately not done

The item's read-path _reject_control_chars limb. Redundant once the gates are strict -- both charsets
exclude every C0 and DEL character, and every character of the query reaches a gate -- and adding it
would re-introduce the duplication that #1239 records as retired by #1243.

Test shape, because the obvious test proves nothing

"Patient/123\n" is normalised away upstream and builds a URL byte-identical to the clean input,
measured before and after the fix -- so the obvious test passes either way. Only "Patient\n/123", an
LF ending a segment followed by more path, reaches a gate carrying the newline.

Both shapes are pinned: the discriminating case asserts the refusal, and a second test pins the
normalisation, so if that 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; the normalisation pin passed before and after, as it must.

No banner flip, and that is compliance rather than an oversight

This branch touches exactly two files and nothing under docs/. The ledger disposition is withheld
deliberately and routes to the Dispatcher.

Flagging it explicitly because the opposite cause produces the same visible shape: earlier today PR #372
landed a fix with no ledger edit, and its item sat reading not-started while the fix was already on main.

Verification scope -- stated, not implied

Re-run against the rebased commit, not the originally tested one:

ruff format --check, ruff check   clean on both files
mypy (strict)                     clean on transports/fhir.py
pytest test_fhir_lookup.py + test_egress_allowlist.py + test_transports.py
                                  165 passed, 1 skipped

The full suite was NOT run locally and neither test path was collected in full. CI is the gate for
that. All commit hooks passed, including ruff and bandit.

ALSO CARRIES #1241 (added after this PR was opened)

http.client.InvalidURL is not a ValueError and not an OSError -- its MRO is
InvalidURL -> HTTPException -> Exception -- so it matched none of _post's arms, including 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". The arm written for this exception could not catch it.

Now caught and named explicitly, with the MRO in the comment so nobody folds it back into a bare
ValueError.

#1241 IS A PARTIAL FIX -- DO NOT CLOSE THE ITEM ON IT

The item's filed claim -- operator-config values reaching the URL and header sinks with no
construction-time screen -- still HOLDS for both sinks and is not addressed here. What is fixed is
the narrower defect found while re-verifying: when the URL sink fails, it failed in the wrong class.

Scope bound worth carrying: 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.

Red-first, with a shipped negative control

Red was the defect itself, not a proxy: the test failed with a raw http.client.InvalidURL escaping
_post.

A second test ships as the negative control -- a URLError must STILL raise a retryable
DeliveryError and NOT a NegativeAckError -- so this cannot pass by the method having been widened to
sweep everything into the permanent class. It passed before and after.

Why both fixes are on one branch

Both write transports/fhir.py, and the write serialisation on that file was explicit. Splitting them
into concurrent branches is the thing that was ruled out, and stacking a second PR on this one would
create a pre-squash base the moment either merged.

Verification for the combined branch

ruff format --check, ruff check   clean on all changed files
mypy (strict)                     clean on transports/fhir.py
pytest test_fhir_transport + test_fhir_lookup + test_egress_allowlist
       + test_transports + test_smart_backend
                                  259 passed, 1 skipped

The full suite was NOT run locally, neither test path was collected in full, and the webconsole suite
was not run at all. CI is the gate for that.

Both diff instruments agree on the tip: three-dot and two-dot are byte-identical at 3 files, 77
insertions, 3 deletions, and docs/ is untouched -- so no banner flip in either commit, deliberately.

AND #1241's ACTUAL FILED DEFECT (third commit, `f7bd300`)

The earlier commit fixed the wrong-exception-class defect. This one fixes the claim as filed.

conditional_query was taken verbatim from operator settings and reached two sinks with no screen --
an unencoded URL interpolation and the If-None-Exist header value. url was equally unscreened.

Screened at construction, and the disposition is the point

_reject_config_control_chars raises ValueError, 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 a bad setting fails the connection at load rather than dead-lettering an unbounded stream of
messages that were never at fault.

The header sink is why the send path was not enough. The URL limb has two incidental neutralisations
it does not -- unwrap strips a trailing CRLF, full_url splits at # client-side -- and neither
touches a header value.

Still incomplete -- #1241 must NOT be closed on this either

transports/dicomweb.py, which the item also names, is untouched.
FhirLookupExecutor has a SECOND unscreened url construction site in the same file, found only
because an edit matched two locations instead of one. Outside what was dispatched, so reported rather
than fixed.

Red-first, including one that had to be rebuilt

All five new cases red with DID NOT RAISE ValueError. One first failed with a TypeError -- 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 red.

Positive control shipped: a clean conditional_query carrying |, : and / still constructs and
is preserved verbatim, so the screen cannot pass by rejecting everything.

Verification

ruff format --check, ruff check   clean
mypy (strict)                     clean on transports/fhir.py
pytest over six suites            302 passed, 1 skipped

The full suite was NOT run and the webconsole suite was not collected at all. CI is the gate.

A diff disagreement that was chased, not assumed

Two-dot and three-dot disagreed after this commit (3 files vs 5, 131 deletions) -- the same signature as a
genuine squash-revert caught earlier today. The discriminator was run rather than assumed:

'\Z' anchors on origin/main        0
http.client import on origin/main  0     -> #379 has NOT merged
positive control (patterns exist)  1

So this branch is merely behind, not reverting: main gained #376 during the build, and two-dot renders
main's newer work as deletions. Intersection test, the decisive one: files changed here
(fhir.py + tests) versus files main changed since the merge-base (remotefile.py + its test) --
empty intersection, so a merge cannot lose anything. Re-verified independently before this push.

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

…nstead 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.

…nstruction (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.

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.

…ruction (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.

…CKLOG #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.

wshallwshall enabled auto-merge (squash) August 13, 2026 19:25

wshallwshall disabled auto-merge August 13, 2026 19:28

wshallwshall changed the title ~~fix(fhir): anchor the path-segment patterns with \Z so a trailing newline cannot pass (#1240)~~ fix(fhir): anchor the path-segment patterns, and catch InvalidURL in _post (#1240, #1241 partial) Aug 13, 2026

wshallwshall enabled auto-merge (squash) August 13, 2026 19:29

wshallwshall disabled auto-merge August 13, 2026 20:05

wshallwshall changed the title ~~fix(fhir): anchor the path-segment patterns, and catch InvalidURL in _post (#1240, #1241 partial)~~ fix(fhir): anchor path-segment patterns, catch InvalidURL, and screen operator config at construction (#1240, #1241) Aug 13, 2026

wshallwshall enabled auto-merge (squash) August 13, 2026 20:06

wshallwshall added 2 commits August 13, 2026 15:06

Merge branch 'main' into claude/b2-1240-fhir-anchor

b4d660d

Merge branch 'main' into claude/b2-1240-fhir-anchor

8a1fde6

wshallwshall disabled auto-merge August 13, 2026 22:45

wshallwshall enabled auto-merge (squash) August 13, 2026 22:46

wshallwshall disabled auto-merge August 13, 2026 22:46

wshallwshall enabled auto-merge (squash) August 13, 2026 22:49

Merge branch 'main' into claude/b2-1240-fhir-anchor

bc949a8

wshallwshall disabled auto-merge August 13, 2026 23:59

wshallwshall enabled auto-merge (squash) August 14, 2026 00:00

Merge branch 'main' into claude/b2-1240-fhir-anchor

0586a79

wshallwshall disabled auto-merge August 14, 2026 00:25

wshallwshall enabled auto-merge (squash) August 14, 2026 00:26

Merge branch 'main' into claude/b2-1240-fhir-anchor

0d9be74

wshallwshall merged commit 0424bfa into main Aug 14, 2026
35 of 36 checks passed

wshallwshall deleted the claude/b2-1240-fhir-anchor branch August 14, 2026 01:54

Navigation Menu

Uh oh!

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

fix(fhir): anchor path-segment patterns, catch InvalidURL, and screen operator config at construction (#1240, #1241)#379
wshallwshall merged 11 commits into
mainfrom
claude/b2-1240-fhir-anchor

wshallwshall commented Aug 13, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

wshallwshall commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixed on the patterns, not the call sites -- deliberately

Deliberately not done

Test shape, because the obvious test proves nothing

No banner flip, and that is compliance rather than an oversight

Verification scope -- stated, not implied

ALSO CARRIES #1241 (added after this PR was opened)

#1241 IS A PARTIAL FIX -- DO NOT CLOSE THE ITEM ON IT

Red-first, with a shipped negative control

Why both fixes are on one branch

Verification for the combined branch

AND #1241's ACTUAL FILED DEFECT (third commit, f7bd300)

Screened at construction, and the disposition is the point

Still incomplete -- #1241 must NOT be closed on this either

Red-first, including one that had to be rebuilt

Verification

A diff disagreement that was chased, not assumed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

wshallwshall commented Aug 13, 2026 •

edited

Loading

AND #1241's ACTUAL FILED DEFECT (third commit, `f7bd300`)