fix(dicomweb): screen the study UID at construction -- the last limb of BACKLOG #1241 - #400
Merged
Conversation
…t (BACKLOG #1241)
The control-char screen in front of `study_uid` was the only thing between an
operator-configured value and `{base}/studies/{study_uid}`, and `has_control_char`
is a C0/DEL predicate -- it does not know what a path metacharacter is. None of
`../../metadata`, `1.2.3/series`, `1.2.3?bulk=1`, `1.2.3#frag` or
`user@evil.example.org` carries a control character, so every one of them reached
the URL unaltered.
The sharp case is traversal: `../../metadata` would redirect a PHI-bearing STOW-RS
POST off the study endpoint to another service on the same host (CWE-918), which
no egress allow-list catches because the HOST never changes.
This is the last limb of #1241 -- the item's subject is the ASYMMETRY, and the
other two limbs are already on main (`fhir.py:263` screens `conditional_query`
across both its sinks, `fhir.py:797` screens the FhirLookupExecutor url). This
file was the sibling still unscreened.
Gate the value to the DICOM PS3.5 s9.1 UID grammar -- dot-separated numeric
components, at most 64 characters -- at CONSTRUCTION, beside the existing screen:
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.
Anchored \A...\Z and never ^...$: `$` also matches immediately before a trailing
newline, which would readmit the very byte the control-char screen exists to
reject. Each component requires at least one digit, so `..`, a leading dot and a
trailing dot are excluded STRUCTURALLY rather than by enumeration -- a lazy
`[0-9.]{1,64}` class would have admitted all three.
Leading zeros within a component are DELIBERATELY admitted although PS3.5 forbids
them. That is a conformance rule, not a security one: it constrains no character
this gate exists to exclude, and refusing a real-world non-conformant UID at
startup would block a legitimate deployment for no gain in containment.
Also percent-encode at the sink, matching the fhir.py path-segment treatment this
file was asymmetric with. It is a NO-OP while the grammar holds -- digits and dots
are both RFC 3986 unreserved -- and a test pins that, so the encode can never be
blamed for a changed URL. It is defence-in-depth against a later loosening of the
grammar, not a control doing work today.
Nineteen tests: twelve refusals, six positive controls, one pinning the encode as a
no-op. NEGATIVE CONTROL, run with the source reverted to origin/main: the twelve
refusal cases FAIL and the seven controls still PASS, so the tests detect this
defect specifically rather than passing vacuously.
No configured value changes behaviour -- the only `study_uid` set anywhere in the
repo is "1.2.3", which passes. The connections.toml surface is covered free: it
desugars through the same DICOMweb() factory, so the factory is the schema and
there is no second source of truth.
…s strict (BACKLOG #1241) The UID gate I added one commit ago used the natural pattern for this grammar, \A[0-9]+(?:\.[0-9]+)*\Z. That nests '+' inside '*', which is the shape test_security_static.py::test_no_catastrophic_regex_in_source rejects, and it failed the suite. MEASURED BEFORE DECIDING, against a known-catastrophic control in the same harness, because "it looks fine to me" is not evidence: (a+)+b, failing input n=14 -> 0.3 ms n=18 -> 5 ms n=22 -> 81 ms mine, worst case 100 -> 0.020 ms ... 20000 -> 3.669 ms The control goes exponential; mine is linear -- 200x the input for 183x the time. The pattern is genuinely NOT catastrophic, because '.' and [0-9] are disjoint, so each iteration's boundary is forced and there is no ambiguous split to backtrack through. The gate is matching the SHAPE. A FALSE POSITIVE IS A REASON TO CHANGE THE CODE, NOT TO WEAKEN THE CHECK. Taking an exception would have spent the gate's strictness -- which is deliberate, per the floor-conservatism rationale recorded under #1235 -- to buy nothing: the split-and-check below is simpler than the regex it replaces, obviously linear, and needs no exception at all. str.isdigit() is NOT usable for the rewrite. It is Unicode-aware and returns True for superscript and Arabic-Indic digits, so it would admit non-ASCII into the URL path and reopen the exact hole this screen closes. The explicit ASCII frozenset is the control, not an optimisation -- both cases are verified rejected. Requiring every component to be non-empty is what keeps '..', a leading dot and a trailing dot illegal, so they stay excluded structurally rather than by enumeration. A trailing newline is now excluded for free -- it lands inside the final component and is not an ASCII digit -- which is the hole a $-anchored regex would have left open (#1240's defect). That property is pinned by a test calling the screen directly, because through the connector the control-char screen catches it first and the two failures are indistinguishable from outside. Behaviour is otherwise unchanged: all 19 existing cases still hold. NEGATIVE CONTROL RE-RUN COLD against the new implementation rather than inherited from the last one -- with the source reverted to origin/main, 13 fail and the 7 positive controls still pass.
…tement THE PAIRED LEDGER COMMIT FOR THIS PR, under the sanctioned interim (c): a builder PR implementing a BACKLOG item cannot make itself green, and ledger authoring is not a builder's lane. TWO CORRECTIONS TO THE ITEM'S OWN TEXT, verified against origin/main rather than relayed from a report: 1. IT NAMED FhirLookupExecutor AS OUTSTANDING AND IT IS NOT. fhir.py:797 screens its url with a comment naming the item; fhir.py:263 screens conditional_query at construction, covering both the URL and If-None-Exist sinks. The 'what remains' paragraph listed two things and one of them had shipped. 2. THE ANCHORS WERE STALE. It cited fhir.py:428 -- an Authorization-header line with nothing to do with url construction -- and dicomweb.py:241/:153. The real sites are dicomweb.py:243/:155. A file:line citation into code is validated by nothing, which is exactly the subject of #1263. CLOSURE DELIBERATELY NOT TAKEN. All five limbs now appear complete and two seats independently verified the other four on main, so closing is defensible -- but it is a judgement about COMPLETENESS, and the seat supplying a paired commit to unblock a gate is the wrong one to make it. Same line held on #1249 and #1026. Verified: status check OK, 524 items each declaring exactly one status; #1241 still OPEN; U+26A0 count UNCHANGED at 121, so no new glyph vocabulary.
wshallwshall
enabled auto-merge (squash)
August 14, 2026 20:52
wshallwshall
added a commit
that referenced
this pull request
Aug 15, 2026
Two conflicts, both in docs/BACKLOG.md. The tail was the ordinary append collision: main filed #1268 and #1273 while this branch filed #1265, #1266 and #1267. Both sets kept, and they happen to land in numeric order. Verified no item on main is missing from the result. The #1241 conflict was substantive. A lander and this seat independently amended the same item, and the amendments overlapped: both said the "what remains" paragraph overstated itself, both said the FhirLookupExecutor limb is built at fhir.py:797, and both corrected fhir.py:428 and the dicomweb anchors. Restating a load-bearing fact rather than stating it once is the defect SDS-3.5 names, so the duplicated half is dropped rather than merged. More importantly my half had gone stale while the merge sat unmade. It said dicomweb.py alone still reached the URL path with no grammar gate and no percent-encode, and therefore that the item could not close. PR #400 landed exactly that gate at construction with a negative control. Carrying that sentence onto main would have published a false present- tense claim about shipped code, so it is cut and the cut is recorded in the amendment itself rather than left silent. What survives from this side is the part the lander's amendment does not contain: the full anchor-drift table rather than the two sites it corrects, the observation that fhir.py:231 inverts rather than drifts and that the item already carries its own refutation two paragraphs up, and the finding that the test anchor has now drifted three times, which is the measurement that a line number is the wrong instrument for that reference rather than a run of bad luck. Verified: backlog_status_check.py OK at 533 items each declaring exactly one status; 297 in BACKLOG.md (main's 294 plus 1265/1266/1267), no conflict markers remain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The fifth and last limb of BACKLOG #1241.
dicomweb.py'sstudy_uidnow carries a grammar gaterefused at construction -- dot-separated digits, max 64, anchored
\A...\Zand never^...$--plus a defence-in-depth percent-encode. The other four limbs are already on
main.Two commits, both anchored, based fresh on
origin/mainand not stacked on the author's otherbranch.
The author's condition, and why it was worth writing that way
They handed this over not-ready, with one condition: their full suite writes
PYTEST_EXIT=<n>asthe last line of its log, and "if that line is absent it did NOT finish -- absence is not a pass."
At handover the log ended mid-progress-bar with zero
PYTEST_EXIToccurrences, so it was held.It now reads:
Their wrapper had reported "exit code 0" the whole time -- and that 0 belonged to the
echothatappended the exit code, not to pytest. A wrapper's status is a claim about the run; the summary
line is the run. Had the condition been the weaker "check for failures", an unfinished run would
have satisfied it, because an unfinished run has no failures either.
Negative control
Reverted to
origin/main, 12 of 12 grammar cases FAIL while 7 positive controls still PASS -- thetests discriminate the defect rather than passing vacuously.
ruff,ruff formatandmypy --strictclean.
The paired ledger commit, and two corrections to the item's own text
A builder PR implementing a BACKLOG item cannot make itself green -- the required
a PR that implements BACKLOG #N must update BACKLOG.mdcheck reads the PR's own diff, and ledgerauthoring is not a builder's lane. So the banner commit here is the Lander's, under the sanctioned
interim (c).
Verified against
origin/mainrather than relayed, the item was overstating itself:FhirLookupExecutoras outstanding. It is not --fhir.py:797screens its urlwith a comment naming the item, and
fhir.py:263screensconditional_queryat construction,covering both the URL and
If-None-Existsinks.fhir.py:428, an Authorization-header line unrelated to urlconstruction, and
dicomweb.py:241/:153. The real sites aredicomweb.py:243/:155.A
file:linecitation into code is validated by nothing, which is the subject of#1263.Closure is deliberately not taken. All five limbs now appear complete and two seats independently
verified the other four -- so closing is defensible. It remains a judgement about completeness,
and the seat that supplies a paired commit to unblock a gate is the wrong one to make it.