diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index c72af25f..fd7fac9d 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -8297,6 +8297,127 @@ every worker session's handoff, which is the sentence the next session bases its **Cluster:** Security / availability. **Priority:** P2. **Verdict:** build. **Severity:** conditional per CLAUDE.md section 0 -- on a first deployment with one administrator, a lockout would be unrecoverable without direct database access; **zero deployments, so nobody is locked out today.** +## 1237. require an explicit decompression ceiling on the Handler-facing primitives + +> 馃敘 **Filed 2026-08-12 - not started.** Value **6/10** 路 Difficulty **2/10**. Split out of **#1129** (ASVS 5.2.3, which stays `partial`) as the BUILD half, so the research item can close on its research. All three `*_decompress` primitives in `messagefoundry/parsing/compression.py` default `max_output_bytes=None`, so a Handler author who does not think about a ceiling silently gets none. + +**Cluster:** Security / hardening. **Priority:** P2. **Verdict:** build. +**Severity:** Conditional -- there are zero deployments (搂0). On a first deployment, a Handler calling `zip_decompress(body)` or `deflate_decompress(body)` without a ceiling would get no total-size bound. This is **not** bounded by the transports, contrary to #1129's severity sentence: the File connector decompresses single-stream gzip only (`transports/file.py:90`), so no transport ceiling ever sees a zip expansion, and `transports/file.py:73` bounds the **compressed** input at 16 MiB. Measured on `9d98f339` by loading the module directly with default arguments: 65,982 bytes in, 67,108,864 out across 8 members (1017:1), nothing raised, tracemalloc peak 76,081,609 bytes. At that ratio an admitted 16 MiB of compressed input would admit roughly 16 GiB of expansion in the transform worker. + +**What ships today.** `gzip_decompress` (`compression.py:91`) -- both in-engine callers already pass a ceiling (`pipeline/retention.py:110`, `transports/file.py:572`). `deflate_decompress` (`compression.py:125`) -- zero in-engine callers. `zip_decompress` (`compression.py:179-181`) -- zero in-engine callers, though its sibling `max_entries: int = 1024` **does** ship on and is enforced at `:196` against the central directory before the extraction loop at `:200`. + +**The change.** Make `max_output_bytes` keyword-only with **no default** on all three (`*, max_output_bytes: int | None`). Passing `None` explicitly still means "no ceiling" and stays available to a caller who has bounded the input upstream -- but it becomes a deliberate, greppable act instead of a silent default. In-tree precedent, same concern, same repo: `parsing/dicom/_inflate.py:74`, `def bounded_inflate_or_error(compressed: bytes, *, max_bytes: int) -> None:`. + +**Why this is not #1129's forbidden move.** #1129 rules out "changing the `None` default alone and scoring the cell", because swapping one unconsidered value for another leaves the clause where it was. A parameter with **no default** is a different construct: a gate that refuses when the precondition is absent. + +**Breakage.** None in tree, verified by word-boundary search -- note `zip_decompress` is a **substring** of `gzip_decompress`, so a naive grep conflates them. It **is** a breaking change to a re-exported public surface (`messagefoundry/__init__.py:92`, `:96`, `:200`, `:202`; `parsing/__init__.py:27`, `:31`); per 搂0 that cost is currently zero, so prefer the simple correct end state over a compatibility shim. + +**Tests.** Pin the signature (no default) on all three; assert a ceiling-less call raises `TypeError`; update the existing default-argument call sites at `tests/test_compression.py:34`, `:68`, `:73`, `:79`, `:86`, `:101`, `:107`, `:115`, `:133`, `:138`, passing `None` where the test means "unbounded" so each test's intent is preserved. Do **not** add any test that reads the archive's declared uncompressed size as the control. + +**Deliberately out of scope.** #1129's research also identified a bounded, output-**discarding** pre-pass for `zip_decompress` -- the shape shipped at `parsing/dicom/_inflate.py:74-107` -- which would satisfy the 5.2.3 verb's "before uncompressing" clause literally. Excluded here because it costs roughly one extra decompression pass on every accepted archive, while the existing incremental check is already the stronger control (it cannot be lied to by a false declared size). Buying literal verb-shape with real runtime cost is an owner call; file separately if wanted. + +**Relationship to the ASVS cell.** This closes the "ships off" limb for the size clause. It does **not** by itself move ASVS 5.2.3 to `pass` -- the verdict is the assessor's, the vault scorecard is the record of record, and the "before uncompressing" reading question is unresolved without the excluded pre-pass. Do not cite this item as evidence of a verdict move. + +**Source:** split from #1129 during its 2026-08-12 research pass. That pass also found #1129's own central premise false -- `parsing/dicom/peek.py:81-85` calls a default-on, output-discarding guard that reads no declared field and cites ASVS 5.2.3 in its own comment -- which is a separate ledger amendment to #1129. + +## 1238. contain the server-supplied remote listing name by rejecting it, never by rewriting it + +> 馃敘 **Filed 2026-08-13 - not started. OWNER RULING RECORDED 2026-08-13: reject, never mutate.** Value **7/10** 路 Difficulty **3/10**. The BUILD half of **#1130** (ASVS 5.3.2, which holds at `partial`). `RemoteFileSource` uses a filename chosen by a remote SFTP/FTP server with no containment check. **The obvious fix -- `posixpath.basename()` -- is actively harmful and must not be used;** the reasons are recorded below because a later reader will otherwise re-propose it. + +**Cluster:** Security / hardening. **Priority:** P1. **Verdict:** build. +**Severity:** Conditional -- there are zero deployments (搂0). On a first deployment, a malicious or compromised partner file server could return a listing entry that escapes the configured directory. It needs a hostile server rather than mere drop-directory write access, which bounds it without removing it. + +**What ships today.** `transports/remotefile.py:918` takes the listing, `:920` iterates it, and the **only** filter before use is `fnmatch` at `:923` -- a glob on the *name*, never a containment check on the resulting *path*. Measured on Python 3.14.6: `fnmatch` treats `*` as matching `/` (unlike `glob`), so `'../../etc/passwd.hl7'`, `'/etc/passwd.hl7'`, `'.error/quarantined.hl7'` and `'..\..\etc\passwd.hl7'` **all match the default `*.hl7`**. The suffix is attacker-chosen, so the glob bounds nothing that matters. The local `File` source has the guard the remote one lacks (`_within_root`, `transports/file.py:358`, `:792`, `:803`). + +**The ruling.** A single-path-component **reject** at the loop head -- after the pattern filter, before the join at `:925`, so it dominates every consumer. Refuse: non-`str`, `""`, `"."`, `".."`, any `/` or `\`, control characters, and the drive-relative form (`C:x.hl7`, which carries no separator at all). **The ratio the ruling turns on:** for a healthcare feed, refusing one file is strictly better than silently reading the wrong one, and mutation cannot give that property. + +**Why `basename()` is rejected -- measured, not preferred.** It **mutates rather than refuses**, which makes it an *aliasing* primitive: `posixpath.basename('../../etc/adt_20260812.hl7')` is `'adt_20260812.hl7'`, which rejoins to a **real file** in the poll directory. One hostile entry would then drive the retrieve and the `after_read` action against a file it does not name -- a move under the default, or a **remote delete** under `after_read="delete"`. That hands a hostile server a move-or-delete primitive against the partner's own drop directory. It is also a **complete no-op on the Windows-separator form** (`basename('..\..\etc\passwd.hl7')` returns the string unchanged, because `posixpath` tokenizes only `/`), and its output can never contain `/`, so **any containment check placed after it is unreachable by construction and always passes** -- a test asserting "traversal is rejected" would go green for the wrong reason. + +**The count-and-log consequence, which is the strongest single argument and is not a security argument.** Under `after_read="leave"` the dedup key hashes the joined name (`:1028`, `:1029`). The alias and the real file produce the **identical** key -- both computed as `1c2195def3461a12` -- so one of the two would be silently skipped as already-ingested. That is a **搂2 count-and-log invariant violation (never accept-and-drop), reached by way of the security fix.** + +**The NLST arm stays, and must never be cited as containment.** `transports/remotefile.py:284` already applies `basename` in the FTP NLST fallback. That is **load-bearing for protocol reasons** -- some servers return full pathnames from NLST -- so do not remove it. But it is *incidentally aliasing*, not incidentally contained, and a containment test landing on that arm would certify a control that does not exist. Checked 2026-08-13: **no scorecard cell anchors it and 5.3.2's residual does not mention `basename`**, so the record is clean today. Recorded here so a future pass does not helpfully add it as evidence. + +**The honest limit.** Name containment is fully in our power; **resolved-path containment is not.** No client-side check constrains a server that resolves a symlink its own way, lies about size or type, or simply serves different bytes for a benign-looking name. SFTP `normalize()` exists but is SFTP-only and asks the hostile party for the truth. Any residual must say this or it overclaims. + +**Why one chokepoint rather than per-site fixes.** The consumer set **grew at every pass and never shrank**: `posixpath.join` sites, then consumers of an already-joined path, then caller gating (three of `_move`'s four callers are error paths not gated on `after_read`), then `:944`'s retrieve and `:972`'s operator-supplied scan hook. Four passes, four expansions, no contractions -- each because the measuring instrument had drawn its boundary too tightly. **No closed count is given here deliberately**; that growth pattern is itself the argument for containing the name where it *enters* rather than at each place it is used. + +**Tests.** Write against the **MLSD** or **SFTP** arm, never NLST -- `:279` and `:434` pass the server's string through unchanged, while NLST is already incidentally aliasing and would prove nothing. Cover at least: `../` traversal, an absolute path, the backslash form, the drive-relative form, `.` / `..` / empty, a control character, and a name that still matches the configured pattern. + +**Relationship to the ASVS cell.** ASVS 5.3.2 holds at `partial`. This is a **build item, not a rescore** -- the verdict is the assessor's and the vault scorecard is the record of record. Do not cite this item as evidence of a verdict move. + +**Source:** split from #1130's research pass, 2026-08-12/13; owner ruling recorded 2026-08-13. Open and **not** settled by this item: whether a syntactic check meets the verb's "strict validation and sanitization" language. The 6.1.1 adjudication did not settle it (the control-property reading was refuted 4 of 4), so it inherits no general ruling and stays open on #1130. + +## 1239. `fhir.py` carries two identical control-char predicates behind different wrappers -- a latent drift, not a current divergence + +> 馃敘 **Filed 2026-08-13 - not started. READ THE FRAMING BEFORE ACTING: the two predicates are byte-identical TODAY. This item records a LATENT hazard, not a live defect, and it must not be cited as evidence of a current gap.** Value **3/10** 路 Difficulty **2/10**. Noticed during #1107's ASVS 1.2.2 surface enumeration. + +**Cluster:** Code quality / drift hazard. **Priority:** P3. **Verdict:** build (small). +**Severity:** none today. There is no behavioural difference to exploit and nothing is mis-screened. The cost is future-tense and conditional: a later hardening applied to one predicate would silently not apply to the other. + +**What is there.** `messagefoundry/transports/fhir.py` defines two control-character predicates whose test expressions are **identical**: +- `:166` `def _reject_control_chars(value: str, field: str) -> str:` -- raises a permanent, PHI-safe `NegativeAckError` naming only the field. Used on the **path** context (`:424`, `:463`). +- `:658` `def _has_control_char(text: str) -> bool:` -- returns a bool. Used on the **flat query** context (`:739` on the raw string, `:748` on the percent-decoded string). + +Both compute `any(ord(ch) < 0x20 or ord(ch) == 0x7F for ch in ...)`. + +**Why this is NOT filed as a duplication defect.** The differing wrappers are appropriate: a raise suits the path context (the delivery is classified permanent), a bool suits the query context (the caller raises `ValueError` with its own message). Applying them to different inputs is also deliberate and good -- the query path deliberately screens **both** the raw string and the percent-decoded one, which a single call could not do. **Do not "fix" this by collapsing the two wrappers.** + +**The actual hazard, stated conditionally.** The shared thing is the *predicate*, and it is written out twice. A future widening -- C1 controls (U+0080-U+009F), Unicode line separators (U+2028/U+2029), or a bidi-override class -- applied to one site would leave the other screening the older, narrower set, with nothing in the tree comparing them. The fix is to extract the predicate once and have both wrappers call it, keeping the wrappers exactly as they are. + +**What would NOT be an honest fix.** Collapsing the two call sites into one helper with a flag, or changing where either is applied. The wrappers and their application points are correct; only the duplicated expression is at issue. + +**Test.** Assert the two predicates agree across a shared character corpus, so a future widening of one without the other fails. That test is the durable control here and is worth more than the extraction itself. + +**Source:** observed during #1107's dynamic-URL surface enumeration, 2026-08-13, while establishing that `fhir.py` context-encodes the path and structured-query contexts but not the flat-query one. Recorded because "two hand-rolled treatments of one threat class in one file" is a drift shape worth tracking -- **and recorded with its current-identity measured**, so a later reader does not mistake it for a live divergence. + +## 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). + +**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. + +**The defect.** `messagefoundry/transports/fhir.py:104-105`: +``` +_FHIR_TYPE_RE = re.compile(r"^[A-Za-z]+$") +_FHIR_ID_RE = re.compile(r"^[A-Za-z0-9.\-]{1,64}$") +``` +Python's `$` matches **before a trailing newline**. Measured on 3.14.6: +``` +_FHIR_ID_RE.match("abc\n") -> True ACCEPTED +_FHIR_TYPE_RE.match("Patient\n") -> True ACCEPTED +_FHIR_ID_RE.match("ab\nc") -> False (embedded newline correctly refused) +_FHIR_ID_RE.fullmatch("abc\n") -> False the fix +``` + +**Reachability, stated plainly because an item that overstates it gets discounted.** Not exploitable on this tree. On the **write** path `_reject_control_chars` runs *before* the gate (`:424`, `:463`), so a newline never reaches it. On **both** paths `urllib.parse.quote(value, safe="")` encodes it to `%0A` (`:428`, `:450`, `:454`, `:712`, `:718`), so no request splitting or header injection follows. **But** the `fhir_lookup` **read** path (`_resolve_read_url`, `:705-718`) has the gate and `quote` and **no** control-char screen, so there the gate is the sole grammar control and it admits a value its own grammar excludes. + +**The fix.** `match` to `fullmatch` on both regexes, and add `_reject_control_chars` to the read path so it matches the write path's two-control posture. Both are one-line changes. + +**Test.** Assert each regex refuses a trailing-newline value directly, not through the URL builder -- routing the assertion through `quote` would pass for the wrong reason and certify nothing, which is the same trap #1238 records for `basename`. + +**Source:** #1107's ASVS 1.2.2 dynamic-URL surface enumeration, 2026-08-13. + +## 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. + +**Cluster:** Security / input validation. **Priority:** P2. **Verdict:** build. +**Severity:** Conditional and lower than the message-derived sites, because these values come from operator config rather than from an inbound message -- an operator can already choose the endpoint. It is filed because the treatment is *inconsistent within the same file*, which is how the weaker one survives review. + +**The sites.** +- `transports/fhir.py:431` -- `f"{base}/{type_seg}?{self.conditional_query}"`. `conditional_query` is read at `:231` and reaches the URL query with **no screen and no encoding at all** -- weaker than the flat-search path 300 lines below, which at least screens. +- `transports/fhir.py:436` -- the same value into the `If-None-Exist` **HTTP header**, also unscreened. **The asymmetry is in the same file:** the `meta.versionId` limb at `:446`/`:449` deliberately *rejects rather than encodes* for exactly this threat, with the reason in-comment ("quoting is wrong for a header value, so reject rather than encode"). +- `transports/dicomweb.py:241` -- `f"{base}/studies/{self.study_uid}"`. `study_uid` gets a control-char screen at `:153` but **no grammar gate and no percent-encode**, asymmetric with `fhir.py:428` which does both. + +**Encoding is the WRONG fix for `conditional_query`, and this is the trap.** The value is *meant* to carry FHIR search syntax: `docs/CONNECTIONS.md:1479` documents `identifier=sys|val` as the intended shape and `tests/test_fhir_transport.py:220` pins `assert url == f"{BASE}/Patient?identifier=sys|val"`. Percent-encoding it would break the documented feature and fail that test. **The right control is a construction-time screen** (at `:231`, where the value is read) covering both sinks -- rejecting control characters and header-splitting sequences while leaving FHIR search syntax intact. For `study_uid`, a DICOM UID grammar gate plus `quote(safe="")` matches the `fhir.py:428` treatment. + +**What would NOT be an honest fix.** Screening only `:431` and leaving `:436`. The header sink is the one with the sharper failure mode and it is the one the existing `versionId` precedent already tells us how to handle. + +**Test.** One test per sink per site, asserting refusal at construction time rather than at the URL layer. + +**Source:** #1107's ASVS 1.2.2 dynamic-URL surface enumeration, 2026-08-13. Related: #1240 (the grammar gates on the message-derived path), #1239 (duplicated control-char predicate in the same file). + ## 1242. `asvs-apply-cells.py` is a lossy writer in four distinct ways, and its own preservation guard is blind to three of them > 馃敘 **Filed 2026-08-13 -- PENDING ACTIVE DATA LOSS, not a latent hazard. The next routine `--apply` destroys a whole commit's worth of structural evidence and prints a clean bill of health.** Value **8/10** -- Difficulty **4/10**. Vault-only security tooling, so there is no deployment axis; what it destroys is the **evidence record** the ASVS cells rest on. Findings below were adversarially verified (4 of 4 lenses), which corrected two errors in the original framing -- see the amendment note.