Skip to content

Commit ec66d92

Browse files
wshallwshallwshallwshallclaude
authored
test(multipart): replace the growth ratio that ejected PR 669 (BACKLOG #1385) (#816)
The hostile-disposition arm was waved through as "already a ratio assertion". It is one, and it ejected PR 669 at ratio 8.02 against a bound of 8.0. Re-run 15 times on an idle box the same statistic ranged 3.73 to 6.80. A ratio is steadier than a wall-clock budget only where the budget would have thin headroom. Here it would not: through parse_single_file_upload a 12,000-char hostile header costs 0.16 ms guarded and 0.7 s unguarded, so a 0.05 s budget sits 290x above one and 13x below the other. Time the shipping entry point against one budget, with the pre-guard pattern swapped into that same path as a live positive control that must exceed it. Timing the real parse also let the size-cap arm assert its ordering claim instead of describing it, with a sentinel that trips on any use of the pattern. Co-authored-by: wshallwshall <mefordev@messagefoundry.org> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 502798d commit ec66d92

3 files changed

Lines changed: 198 additions & 30 deletions

File tree

docs/BACKLOG.md

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18777,6 +18777,45 @@ unmergeable, with the reason recorded only in a run list nobody looks at.** Read
1877718777
the PR. A dequeue looks identical to never having been queued -- and `autoMergeRequest` reads
1877818778
`null` in both states, so the obvious field cannot distinguish them either (Instruments 4.15c).
1877918779

18780+
**2026-09-03 -- the hostile-disposition limb is fixed, and limb 1's own recommendation above is the
18781+
thing that was wrong.** That line says a linear-time assertion "can be restated as a ratio between two
18782+
input sizes rather than an absolute bound". **It already was one**, and the ratio is what ejected PR
18783+
669: **8.02 against a bound of 8.0, a 0.25 percent margin.** Re-run 15 times on an IDLE box, that same
18784+
statistic ranged **3.73 to 6.80** -- 85 percent of the way to its own bound with nothing else competing
18785+
for the core. A ratio is steadier than a wall-clock budget only where the budget would have thin
18786+
headroom, and here it would not. On a 12,000-character hostile header `parse_single_file_upload` costs
18787+
**0.16 ms** with the shipped pattern and **0.7 s** with the pre-guard one, a separation of about
18788+
**3,900x**, so a 0.05 s budget sits **290x** above the shipped cost and **13x** below the quadratic
18789+
one. The ratio compressed that separation into 4-versus-16 and then put the noise in the NUMERATOR,
18790+
where a `min` over reps clamps deflation and nothing clamps inflation.
18791+
18792+
**The fix is a different measurement, not a wider number.** `tests/test_multipart.py` now times the
18793+
shipping `parse_single_file_upload` against one `_SCAN_BUDGET_SECONDS` line, with the pre-guard pattern
18794+
swapped into that same path as a live positive control that must EXCEED the same line -- so a budget
18795+
nothing could ever breach, or an input that quietly stopped being hostile, reds instead of passing
18796+
quietly. Timing the real entry point also let the size-cap arm stop describing its ordering claim and
18797+
start asserting it, with a sentinel pattern that raises if the header scan reaches it. The arm is
18798+
renamed `test_a_hostile_disposition_header_stays_inside_the_scan_budget`; the run tables above name
18799+
`test_hostile_disposition_header_parses_in_linear_time`, which is a record of what failed on
18800+
2026-08-29 rather than a live path.
18801+
18802+
**The mutation pair, one variable changed.** The mutation moves the `_MAX_PART_HEADER_BYTES` refusal
18803+
below `_disposition`, which is the regression that hands the header scan a body-sized input again:
18804+
18805+
| Test body | Result under the SAME mutation |
18806+
|---|---|
18807+
| new | **1 failed, 20 passed** in 1.3 s -- the sentinel names the scan that should not have run |
18808+
| old | **19 passed** in 0.8 s -- completely invisible |
18809+
18810+
A second mutation, removing the `(?<!\w)` guard itself, reds under both bodies but not comparably. The
18811+
new body reports `0.6031s against a 0.05s budget` in **4.6 s**; the old body needs **101 s** to reach
18812+
its assertion, and under the configured 60 s per-test watchdog it never reaches it at all -- it is
18813+
killed mid-`findall` inside a C call the thread-method watchdog cannot interrupt, which is this row's
18814+
own "process-level deadlock below pytest" shape.
18815+
18816+
**Still open on this row:** the SQL Server limb, the `merge_group` visibility half, and
18817+
`tests/test_api_request_timeout.py` (open in PR 789, not on `main`).
18818+
1878018819
**Do not close this by re-running the queue until it passes.** That is what happened three times
1878118820
already, and it produces a green that means "this attempt got lucky", not "the change is sound".
1878218821

docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,8 @@ Scorecard runs on the same mirror and surfaced **48 findings**. These are **repo
6868
`tests/test_ci_venv_pinning.py`
6969
- **AC-6** — WHEN the multipart parser reads an uploaded part, THE SYSTEM SHALL bound that part's header block before parsing it, so the header scan's input size is set by the parser and not by the request-body cap.
7070
`tests/test_multipart.py::test_oversized_part_header_is_refused_not_parsed`
71-
- **AC-7** — WHEN the multipart parser scans a `Content-Disposition` line, THE SYSTEM SHALL do so in time linear in the line's length.
72-
`tests/test_multipart.py::test_hostile_disposition_header_parses_in_linear_time`
71+
- **AC-7** — WHEN the multipart parser scans a `Content-Disposition` line, THE SYSTEM SHALL do so without super-linear cost: the largest header block the parser admits SHALL be scanned in a small, bounded slice of the asyncio event loop. *(Reworded 2026-09-03 under BACKLOG #1385. The criterion used to say "in time linear in the line's length" and was verified by a growth-ratio assertion, which ejected PR 669 at ratio 8.02 against a bound of 8.0. A budget on the admitted worst case carries ~290x of headroom where that ratio carried 1.9x, and it is paired with a positive control so it cannot pass vacuously.)*
72+
`tests/test_multipart.py::test_a_hostile_disposition_header_stays_inside_the_scan_budget` · `tests/test_multipart.py::test_the_unguarded_pattern_blows_that_same_budget`
7373
- **AC-8** — WHERE a CI job installs a third-party quality or security tool that this repo routes through a PEP 735 dependency group, THE SYSTEM SHALL install it from a hash-pinned `uv export` with `--require-hashes`, and THAT lock SHALL be regenerated and diff-gated by the DEP-1 step rather than hand-maintained.
7474
`tests/test_ci_venv_pinning.py` (`test_lock_installed_toolchain_*`, `test_moved_tools_are_declared_in_a_dependency_group`, `test_no_moved_tool_is_reinstalled_inline`) · `tests/test_dep1_lock_resync_lockstep.py`
7575

tests/test_multipart.py

Lines changed: 157 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@
99

1010
import pytest
1111

12+
from messagefoundry.api import multipart
1213
from messagefoundry.api.multipart import (
13-
_DISPOSITION_PARAM,
1414
_MAX_PART_HEADER_BYTES,
1515
MultipartError,
1616
MultipartTooLargeError,
@@ -89,8 +89,34 @@ def test_no_file_part_rejected() -> None:
8989

9090
#: The pre-guard pattern. Kept here so the equivalence test proves the ``(?<!\w)`` guard is a pure
9191
#: performance change; if the production regex ever drifts, this test is what catches the semantic gap.
92+
#: It is also the live positive control for the cost arms below: swapped into the shipping parse path,
93+
#: it shows that the budget those arms assert against is a line a quadratic scan cannot get under.
9294
_LEGACY_DISPOSITION_PARAM = re.compile(r'(\w+)="([^"]*)"')
9395

96+
#: Length of the hostile word run the cost arms measure. 12,000 word characters and no further ``="``
97+
#: is 73 percent of ``_MAX_PART_HEADER_BYTES``, so the arms measure close to the largest header the
98+
#: parser will actually admit rather than an arbitrary size. Measured 2026-09-03 on a dev box:
99+
#: ``parse_single_file_upload`` handles this body in ~0.16 ms with the shipped pattern and ~0.7 s with
100+
#: the unguarded one -- a separation of about 3,900x, which is the room the budget below spends.
101+
_HOSTILE_HEADER_CHARS = 12_000
102+
103+
#: The one line both cost arms are measured against, sitting between the two costs recorded above. The
104+
#: margins run in OPPOSITE directions across it, which is why a single number does the work of two.
105+
#:
106+
#: **Above the line (the production arm) a stall turns a pass into a failure, so the margin is huge:**
107+
#: ~290x, and the estimate is best-of-5, so a runner would have to stall for 50 ms five separate times
108+
#: inside a 0.16 ms window to fake a red.
109+
#:
110+
#: **Below the line (the control arm) a stall can only help, so the margin stays tight:** ~13x, and one
111+
#: sample is enough there because noise cannot deflate it.
112+
#:
113+
#: **Deliberately one number rather than a ceiling plus a lower floor.** A floor beneath the ceiling
114+
#: would open a band in which a half-fixed scan clears both arms; sharing the line makes the control
115+
#: prove that this exact budget discriminates. If some future CPython makes the unguarded scan fast
116+
#: enough to slip under it, the control reds -- which is the correct thing to be told, because at that
117+
#: point the instrument has stopped discriminating and the number needs re-deriving.
118+
_SCAN_BUDGET_SECONDS = 0.05
119+
94120

95121
@pytest.mark.parametrize(
96122
"line",
@@ -108,48 +134,138 @@ def test_no_file_part_rejected() -> None:
108134
],
109135
)
110136
def test_disposition_param_regex_matches_legacy_semantics(line: str) -> None:
111-
"""The ``(?<!\\w)`` ReDoS guard must not change which parameters are extracted."""
112-
assert _DISPOSITION_PARAM.findall(line) == _LEGACY_DISPOSITION_PARAM.findall(line)
137+
"""The ``(?<!\\w)`` ReDoS guard must not change which parameters are extracted.
138+
139+
Read through the module rather than a from-import, so this file has exactly ONE way to name the
140+
pattern and it is the one the arms below substitute. A second, permanently-real binding would sit
141+
right beside three tests that replace it, and nothing would say which is which."""
142+
assert multipart._DISPOSITION_PARAM.findall(line) == _LEGACY_DISPOSITION_PARAM.findall(line)
143+
144+
145+
def _hostile_upload() -> tuple[str, bytes]:
146+
"""A well-formed single-file upload whose Content-Disposition carries a long word run and no
147+
further ``="``. That is the shape an unguarded ``(\\w+)="…"`` walks quadratically: the scan
148+
restarts at every offset inside the run and each restart walks the rest of it.
149+
150+
The size invariant lives here rather than in one arm, because it is a property of the fixture and
151+
every arm depends on it."""
152+
assert _MAX_PART_HEADER_BYTES // 2 < _HOSTILE_HEADER_CHARS < _MAX_PART_HEADER_BYTES, (
153+
f"the hostile header must sit near the cap the parser admits, not merely under it: "
154+
f"{_HOSTILE_HEADER_CHARS} against a cap of {_MAX_PART_HEADER_BYTES}. Over the cap and the "
155+
f"arms time the refusal path; far under it and they stop measuring the worst case the engine "
156+
f"actually accepts. Moving the cap means re-deriving BOTH this size and the budget below: "
157+
f"the control arm's cost grows with the square of this number."
158+
)
159+
part = (
160+
b'Content-Disposition: form-data; name="file"; filename="acme.hl7"; '
161+
+ b"a" * _HOSTILE_HEADER_CHARS
162+
+ b"\r\n\r\nMSH|body"
163+
)
164+
return f"multipart/form-data; boundary={_B}", _body([part])
165+
166+
167+
def _parse_seconds(reps: int) -> float:
168+
"""Best-of-``reps`` seconds for the real ``parse_single_file_upload`` on the hostile body.
169+
170+
The MINIMUM is the noise-free estimate: a scheduling hiccup can only inflate a sample, never
171+
deflate one, so one slow slice on a loaded runner cannot fake a red.
113172
173+
The parse is asserted to SUCCEED on every rep. Without that an arm could pass by timing an early
174+
error path — a refused boundary, or a header the size cap rejected — instead of the scan it claims
175+
to measure."""
176+
content_type, body = _hostile_upload()
177+
best = float("inf")
178+
for _ in range(reps):
179+
start = time.perf_counter()
180+
file = parse_single_file_upload(content_type, body, max_file_bytes=1024)
181+
best = min(best, time.perf_counter() - start)
182+
assert file.filename == "acme.hl7"
183+
return best
114184

115-
def test_hostile_disposition_header_parses_in_linear_time() -> None:
185+
186+
def test_a_hostile_disposition_header_stays_inside_the_scan_budget() -> None:
116187
"""A Content-Disposition line of many word chars and no ``="`` must not blow up quadratically.
117188
118189
The header block is attacker-supplied and ``parse_single_file_upload`` runs synchronously on the
119190
asyncio event loop, so a quadratic scan here is a whole-engine denial of service, not a slow
120-
request. ``_MAX_PART_HEADER_BYTES`` now bounds the input as well, but the two controls are
121-
independent on purpose: the cap is a size policy someone could reasonably raise, while this asserts
122-
the scan itself stays linear at any size. Assert the growth ratio rather than a wall-clock budget so
123-
the test is not flaky on a loaded CI runner: quadratic scaling multiplies by ~16 when the input
124-
quadruples; linear scaling stays near ~4.
191+
request. ``_MAX_PART_HEADER_BYTES`` bounds the input as well, but the two controls are independent
192+
on purpose: the cap is a size policy someone could reasonably raise, while this bounds what the
193+
scan costs at the size the cap admits today.
194+
195+
**This replaced a growth-ratio assertion, and the ratio is why BACKLOG #1385 has this row.** The
196+
old arm timed the bare regex at n and 4n and required the quotient under 8.0, reasoning that
197+
quadratic scaling multiplies by ~16 and linear stays near ~4. It ejected PR 669 anyway, at ratio
198+
**8.02**. A ratio is not inherently steadier than a wall-clock budget. It is steadier only where a
199+
budget would have thin headroom, and here a budget has enormous headroom. The old statistic put the
200+
whole test inside a 2x window (4 measured, 8 asserted) and put the noise in the NUMERATOR, where a
201+
``min`` over reps clamps deflation and nothing clamps inflation — so every scheduling hiccup pushed
202+
the quotient one way, up. Re-run 15 times on an IDLE box on 2026-09-03 it ranged **3.73 to 6.80**,
203+
already 85 percent of the way to its own bound with nothing else competing for the core.
204+
205+
The separation between linear and quadratic on this input is ~3,900x, not 4x, so an absolute
206+
budget carries ~290x of headroom where the ratio carried 1.9x. It also times the shipping entry
207+
point rather than a detached regex, so the cost claim is about the parse an upload really runs.
125208
"""
209+
seconds = _parse_seconds(5)
210+
assert seconds < _SCAN_BUDGET_SECONDS, (
211+
f"the hostile header cost {seconds:.4f}s of the event loop against a "
212+
f"{_SCAN_BUDGET_SECONDS}s budget"
213+
)
214+
215+
216+
def test_the_unguarded_pattern_blows_that_same_budget(monkeypatch: pytest.MonkeyPatch) -> None:
217+
"""Live positive control: the SAME body and the SAME budget, with the ``(?<!\\w)`` guard removed
218+
from the shipping parse path.
219+
220+
Without it the arm above is unfalsifiable. A budget nothing on this input could ever exceed would
221+
pass while measuring nothing, and so would an input that had quietly stopped being hostile — which
222+
the old ratio arm had no way to notice either, since it never ran a pattern it expected to be slow.
223+
This one does: it patches the module global ``_disposition`` reads, so the whole shipping path runs
224+
unguarded rather than a regex held off to one side.
225+
226+
One rep, deliberately. The assertion is that the scan is SLOW, so noise moves it the safe way."""
227+
monkeypatch.setattr(multipart, "_DISPOSITION_PARAM", _LEGACY_DISPOSITION_PARAM)
228+
seconds = _parse_seconds(1)
229+
assert seconds > _SCAN_BUDGET_SECONDS, (
230+
f"the unguarded pattern parsed the hostile header in {seconds:.4f}s, inside the "
231+
f"{_SCAN_BUDGET_SECONDS}s budget. The budget no longer separates a linear scan from a "
232+
f"quadratic one, so the arm above is not measuring anything"
233+
)
126234

127-
def elapsed(n: int, reps: int = 3) -> float:
128-
"""Best-of-``reps``: a scheduling hiccup can only inflate a sample, never deflate one, so the
129-
MINIMUM is the noise-free estimate — one slow slice on a loaded runner cannot fake a red."""
130-
line = "content-disposition: " + "a" * n
131-
best = float("inf")
132-
for _ in range(reps):
133-
start = time.perf_counter()
134-
_DISPOSITION_PARAM.findall(line)
135-
best = min(best, time.perf_counter() - start)
136-
return best
137-
138-
base_n = 20_000
139-
elapsed(base_n) # warm the regex cache / JIT-free interpreter paths
140-
small = max(elapsed(base_n), 1e-6)
141-
large = elapsed(base_n * 4)
142-
assert large / small < 8.0, f"scaling looks super-linear: {small=} {large=}"
143-
144-
145-
def test_oversized_part_header_is_refused_not_parsed() -> None:
235+
236+
class _ScanSentinel:
237+
"""Stands in for the disposition pattern and fails loudly on ANY use of it.
238+
239+
It trips from ``__getattr__`` rather than by implementing ``finditer``, because a stand-in pinned
240+
to one method name degrades into an ``AttributeError`` the day the scan is rewritten to call
241+
something else — and this file already calls ``findall`` on that same object. An
242+
``AttributeError`` would still red, but it would name the wrong problem."""
243+
244+
def __init__(self, reason: str) -> None:
245+
self.reason = reason
246+
247+
def __getattr__(self, name: str) -> object:
248+
raise AssertionError(f"{self.reason} (the scan asked the pattern for {name!r})")
249+
250+
251+
def test_oversized_part_header_is_refused_not_parsed(monkeypatch: pytest.MonkeyPatch) -> None:
146252
"""A part header block past ``_MAX_PART_HEADER_BYTES`` is rejected before ``_disposition`` runs.
147253
148254
The per-part ``max_file_bytes`` cap applies to a part's *content*, and only AFTER its header has
149255
been parsed — so without this bound the header scan's input is the whole request body (25 MiB by
150256
default, 512 MiB at the ceiling) on the asyncio event loop. A real client never approaches it: a
151257
Content-Disposition plus a Content-Type is a couple hundred bytes.
258+
259+
**"Before" is the whole claim, so it is asserted rather than described.** A sentinel stands in for
260+
the pattern and raises if the scan reaches it, so the refusal has to arrive with the header
261+
unscanned. Checking only that ``MultipartError`` is raised passes identically with the cap moved
262+
below the scan — and that reordering is exactly the regression that hands the header scan a
263+
body-sized input again, which is the reason the cap exists.
152264
"""
265+
sentinel = _ScanSentinel(
266+
"the size cap should have refused this header before anything scanned it"
267+
)
268+
monkeypatch.setattr(multipart, "_DISPOSITION_PARAM", sentinel)
153269
fat = b"X" * (_MAX_PART_HEADER_BYTES + 1)
154270
part = (
155271
b'Content-Disposition: form-data; name="file"; filename="a.hl7"\r\nX-Pad: '
@@ -162,6 +278,19 @@ def test_oversized_part_header_is_refused_not_parsed() -> None:
162278
)
163279

164280

281+
def test_the_scan_sentinel_fires_when_a_header_is_scanned(monkeypatch: pytest.MonkeyPatch) -> None:
282+
"""Negative control for the sentinel: it has to be able to report a scan that DID happen, or the
283+
arm above passes for the wrong reason — a sentinel that could never fire proves nothing about
284+
ordering. An ordinary under-cap header reaches ``_disposition`` normally, so the same substitution
285+
must raise there."""
286+
monkeypatch.setattr(multipart, "_DISPOSITION_PARAM", _ScanSentinel("reached"))
287+
part = b'Content-Disposition: form-data; name="file"; filename="a.hl7"\r\n\r\nMSH|body'
288+
with pytest.raises(AssertionError, match="asked the pattern for"):
289+
parse_single_file_upload(
290+
f"multipart/form-data; boundary={_B}", _body([part]), max_file_bytes=1024
291+
)
292+
293+
165294
def test_realistic_part_header_is_well_under_the_cap() -> None:
166295
"""Non-vacuity for the cap: an ordinary upload's header must be nowhere near the limit, so the
167296
bound can never start rejecting legitimate traffic."""

0 commit comments

Comments
 (0)