diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index cc9f868f..0d03908b 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -173,8 +173,8 @@ jobs: # requirements.lock drags PySide6/dev tooling the wheel never requires); pip-audit still audits # the all-extras set. See docs/SUPPLY-CHAIN.md + ADR 0149. python -m pip install --upgrade "cyclonedx-bom~=7.3" + # No unpinned pip bootstrap: --require-hashes resolves nothing (tests/test_ci_venv_pinning.py). python -m venv /tmp/sbomenv - /tmp/sbomenv/bin/pip install --upgrade pip /tmp/sbomenv/bin/pip install --require-hashes -r docker/locks/requirements-core.lock python -m cyclonedx_py environment /tmp/sbomenv/bin/python \ --pyproject pyproject.toml --mc-type application \ diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index f1839433..b7328c43 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -74,7 +74,7 @@ jobs: # reproducible, tamper-evident install. Exercises the lockfile as an actual install path # instead of only auditing it, so a lockfile that doesn't resolve/install is caught (low-26). python -m venv /tmp/lockcheck - /tmp/lockcheck/bin/pip install --upgrade pip + # No unpinned pip bootstrap: --require-hashes resolves nothing (tests/test_ci_venv_pinning.py). /tmp/lockcheck/bin/pip install --require-hashes -r requirements.lock - name: Audit the locked dependencies (DEP-1) run: | @@ -136,8 +136,8 @@ jobs: # pulls; the all-extras requirements.lock stays covered by the pip-audit job above. cyclonedx-bom # ~=7.3 → lxml 6.x (cp314 wheels) so the 3.14 runner doesn't source-build lxml. ADR 0149. python -m pip install --upgrade pip "cyclonedx-bom~=7.3" + # No unpinned pip bootstrap: --require-hashes resolves nothing (tests/test_ci_venv_pinning.py). python -m venv /tmp/sbomenv - /tmp/sbomenv/bin/pip install --upgrade pip /tmp/sbomenv/bin/pip install --require-hashes -r docker/locks/requirements-core.lock python -m cyclonedx_py environment /tmp/sbomenv/bin/python \ --pyproject pyproject.toml --mc-type application \ diff --git a/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md b/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md index e20761bb..9b5e9f47 100644 --- a/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md +++ b/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md @@ -64,6 +64,12 @@ Scorecard runs on the same mirror and surfaced **48 findings**. These are **repo - **AC-3** — IF a static-analysis finding is triaged as a non-issue (false positive / test-only) or an accepted risk, THEN THE SYSTEM SHALL record it as a dismissal with a written justification rather than leave it open or silently filter it. → `docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md` (this register) + the mirror's code-scanning dismissal log - **AC-4** — IF a finding is in the PHI-to-log (`clear-text-logging`) or `path-injection` class, THEN it SHALL NOT be dismissed without first confirming the untrusted-source→sink dataflow is mitigated. +- **AC-5** — WHERE a CI step builds a scratch venv whose only install is a committed lockfile, THE SYSTEM SHALL install into it exclusively with `--require-hashes`, and SHALL NOT bootstrap it with an unpinned `pip install --upgrade pip` (in any spelling) or hide that fetch behind `venv --upgrade-deps`. + → `tests/test_ci_venv_pinning.py` +- **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. + → `tests/test_multipart.py::test_oversized_part_header_is_refused_not_parsed` +- **AC-7** — WHEN the multipart parser scans a `Content-Disposition` line, THE SYSTEM SHALL do so in time linear in the line's length. + → `tests/test_multipart.py::test_hostile_disposition_header_parses_in_linear_time` ## Options considered @@ -78,3 +84,123 @@ Scorecard runs on the same mirror and surfaced **48 findings**. These are **repo **Negative / risks** — a register can go stale: it MUST be updated whenever new findings are triaged, or it misleads. The accepted risk (#5) remains a cleartext-at-rest credential — mitigated by owner-only perms + forced first-login rotation, but a residual to revisit if the bootstrap flow changes. **Out of scope** — enabling GHAS on the private repo; pursuing the *proper* Docker/Fuzzing/badge hardening above (deferred, not warranted now); and the operational mirror **publish** that re-runs CodeQL/Scorecard and auto-closes the fixed/stale findings (`publish.ps1`, owner-run). + +--- + +## Amendment — 2026-07-28: second triage round (32 open findings) + +Everything above records the **2026-06-26** state and is left intact as the record of its day. This +section is the delta. Where the two disagree, this section governs. + +### Topology correction — the "read-only mirror" premise is retired + +The Context and the Scorecard register above are written against a topology that no longer exists: +`MEFORORG/MessageFoundry` was a read-only publish target fed by force-pushed snapshots. Since the +**cutover (2026-07-27)** it is the **primary development repo** — `scripts/publish/publish.ps1` and the +release-sync check are gone, and changes arrive as reviewed PRs with branch protection and required +checks. Scorecard therefore now measures the **right** repo. + +Consequence, and it is not cosmetic: the three Scorecard dismissals whose recorded reason rests +*entirely* on that premise — **`BranchProtectionID` (#33)**, **`CodeReviewID` (#77)**, +**`MaintainedID` (#78)**, all reasoned "measured on the read-only mirror … enforced on the private +upstream" — now carry a justification that is no longer true. Under the Decision above, a dismissal +with a false reason is worse than an open finding. **They must be re-triaged against the real repo, not +renewed.** They were out of scope for this round (they are not among the 32 open findings). + +### Outcome of the second triage (32 findings): 7 fixed, 25 dismissed + +**Fixed (7):** + +| Rule | Where | Why it was real | +|---|---|---| +| `py/polynomial-redos` | `messagefoundry/api/multipart.py` | `(\w+)="([^"]*)"` scanned a part's `Content-Disposition` line quadratically (`=` is not a word char, so every offset inside a word run walked to the run's end before failing). The header block is attacker-supplied and was bounded only by the request-body cap, and it is parsed **synchronously on the asyncio event loop** that also drives every listener, router, transform and delivery worker — so one request could wedge the whole engine. Fixed with a `(?/bin/pip install --upgrade pip` — an unpinned, unverified PyPI fetch that bought nothing +(`--require-hashes` rejects any un-hashed requirement and so performs no resolution at all, making the +`ensurepip` pip sufficient). Two were open alerts 115/118 and are fixed. The third is +`security.yml`'s `/tmp/lockcheck`, which carries **already-dismissed alert #71** whose recorded reason +is the editable-install text — factually wrong for that line, since the very next line *is* a +`--require-hashes` install. It has been fixed here too, and **#71 must be closed as fixed rather than +renewed**. All three are pinned by `tests/test_ci_venv_pinning.py`, which also blocks the +`/bin/python -m pip` spelling and the `venv --upgrade-deps` variant (option 3's invisible filter). + +**3. A version pin does not satisfy this check.** Proven by the repo's own alert data: `bandit==1.9.4` +is exactly pinned and still flagged (#74), as is `zizmor==1.5.2` (alert 96), while the two +`--require-hashes` installs are flagged in neither the open nor the dismissed set. Closing the remaining +`PinnedDependenciesID` findings therefore needs a **hash-pinned lock for CI tooling**, which is coupled +to DEP-1: the four committed lock artifacts are all `uv export`ed from `uv.lock`, diff-gated in CI and +auto-resynced by Dependabot, so a hand-maintained fifth lock outside that machinery would rot into a +pinned, **stale, unpatched** toolchain — worse posture than floating. The correct fix is to route CI +tooling through a `pyproject` dependency group so it flows into `uv.lock` and the exports. Deferred, +recorded here as the convergence target. + +### Convergence rule: line drift re-fires a dismissal as a new alert + +This repo's scanner raises the **same expression at a new line** as a **new alert number**, so a +dismissal does not survive the file growing above it. Two confirmed instances: dismissed **#18** +(`__main__.py:976`) re-fired as open **122** (`__main__.py:1535`, byte-identical expression), and +dismissed **#39** (`dependabot-auto-merge.yml:28`) re-fired as open **87** (line 44) because a comment +block above it grew. Both re-fires are pure line drift, no behaviour change. + +Therefore: **when editing a file that carries dismissed alerts, keep the edit line-neutral** where +practical, or expect to re-dismiss every anchor below it. The two workflow fixes in this round were +deliberately made line-neutral — one line deleted, one comment line added — which is why their rationale +lives in `tests/test_ci_venv_pinning.py`'s module docstring rather than in the workflow. + +### Recommended hardening — identified, NOT done + +Recorded here because a `won't fix` dismissal makes an item invisible, and these were found *while* +justifying those dismissals. None of them closes its alert; each reduces residual risk. + +| Where | Recommendation | Why it matters | +|---|---|---| +| `release.yml` `pip install sigstore` | Pin `sigstore==` | The **highest residual in the group**: a completely unpinned install inside the job holding `contents: write` + `id-token: write` + `attestations: write`, resolved immediately before it signs the wheel, sdist, SBOM and VEX. A malicious release fetched at that moment runs with the OIDC identity used to publish. | +| `release.yml` `pip install --upgrade pip build` | Pin `build==` | Unpinned PEP 517 frontend that produces the published wheel/sdist. | +| `release.yml` `pip install --quiet packaging` (harness job) | Pin `packaging==`; install into a throwaway venv as the engine job already does | Resolved into the **publishing** job's main interpreter rather than a scratch venv. | +| `release.yml` `pip install --quiet packaging` (`/tmp/relsmoke`) | Pin `packaging==` | Contained (disposable venv, version-compare only), but free to pin. | +| `dependabot-auto-merge.yml` `security-events: read` | Remove the scope | Dead. Its comment claims it reads Dependabot alerts, but the gate calls the **global** `/advisories` endpoint, which is repo-scope-independent. Verified; least-privilege hygiene only. | + +`sigstore`/`build`/`packaging` pins touch the **release critical path**, which no PR CI leg executes — +see below — so they are an owner decision, not a drive-by. + +### What no test can see + +Both workflow fixes land on paths **no PR CI leg runs**: `security.yml`'s SBOM job is +`schedule`/`workflow_dispatch` only *and* `continue-on-error: true` (a failure there is yellow and +swallowed), and `release.yml` runs only on a tag push. So the first real execution of either edit is a +nightly or **a release**. `tests/test_ci_venv_pinning.py` is a text guard over the workflow source, not +an execution. Before the next tag, run `security.yml`'s sbom job via `workflow_dispatch` and read its +log — the install command there is byte-identical to `release.yml`'s. diff --git a/ide/src/symbolIndex.ts b/ide/src/symbolIndex.ts index faf7545c..18ff0264 100644 Binary files a/ide/src/symbolIndex.ts and b/ide/src/symbolIndex.ts differ diff --git a/ide/src/test/suite/symbol-index.test.ts b/ide/src/test/suite/symbol-index.test.ts index 29d1aa91..cdcc2dcb 100644 --- a/ide/src/test/suite/symbol-index.test.ts +++ b/ide/src/test/suite/symbol-index.test.ts @@ -5,6 +5,11 @@ import * as path from "node:path"; import { buildSymbolIndex, matchSymbols, scanModuleSymbols, type SymbolDef } from "../../symbolIndex"; +// The REAL node:fs module object, for the descriptor test's spies. `import * as fs` compiles +// (esModuleInterop, module=commonjs) to a namespace COPY whose members are forwarding getters onto this +// object — so the copy cannot be assigned to, while a spy installed HERE is what symbolIndex.ts calls. +const FS_MODULE: Record = require("node:fs"); + // Pure (vscode-free) symbol scan for the sidebar name search (BACKLOG #228): find top-level // handler/router/transform `def`s so a search reveals a transform / differently-named handler that is // a Python symbol inside a role-combined feed module — not a connection filename or a graph element. @@ -142,3 +147,88 @@ suite("symbolIndex — buildSymbolIndex (recurse, include _-prefixed, skip vendo assert.deepStrictEqual(buildSymbolIndex(path.join(root, "does-not-exist")), []); }); }); + +// CodeQL js/file-system-race: the scan used to size-check with `statSync(path)` and then read with +// `readFileSync(path)` — two independent path resolutions, so the file that was READ need not be the +// file that was CHECKED. That voids the maxBytes guard with no attacker involved (a save, a formatter +// or a codegen step rewriting a module between the two calls is routine in a live workspace). +suite("symbolIndex — buildSymbolIndex resolves each file once (js/file-system-race)", () => { + let root: string; + + suiteSetup(() => { + root = fs.mkdtempSync(path.join(os.tmpdir(), "mfsym-race-")); + fs.writeFileSync(path.join(root, "small.py"), "def xform_small(m):\n return m\n"); + // Comfortably over the 64-byte cap the tests below pass, so it takes the oversize path. + fs.writeFileSync(path.join(root, "big.py"), `def xform_big(m):\n return m\n# ${"p".repeat(4096)}\n`); + }); + + suiteTeardown(() => { + fs.rmSync(root, { recursive: true, force: true }); + }); + + test("a file over maxBytes is skipped; smaller siblings still index", () => { + assert.deepStrictEqual( + buildSymbolIndex(root, { maxBytes: 64 }).map((d) => d.name), + ["xform_small"], + ); + }); + + test("size-checks and reads the SAME descriptor, and closes every one it opens", () => { + const readArgs: fs.PathOrFileDescriptor[] = []; + const statPaths: string[] = []; + const opened: number[] = []; + const closed: number[] = []; + + // Captured through the namespace import BEFORE patching, so they are the real, fully-typed + // functions; the spies delegate to these rather than to the (now patched) module members. + const origRead = fs.readFileSync; + const origStat = fs.statSync; + const origOpen = fs.openSync; + const origClose = fs.closeSync; + + try { + FS_MODULE.readFileSync = (p: fs.PathOrFileDescriptor, o: BufferEncoding): string => { + readArgs.push(p); + return origRead(p, o); + }; + FS_MODULE.statSync = (p: fs.PathLike): fs.Stats => { + statPaths.push(String(p)); + return origStat(p); + }; + FS_MODULE.openSync = (p: fs.PathLike, flags: fs.OpenMode): number => { + const fd = origOpen(p, flags); + opened.push(fd); + return fd; + }; + FS_MODULE.closeSync = (fd: number): void => { + closed.push(fd); + origClose(fd); + }; + + assert.deepStrictEqual( + buildSymbolIndex(root, { maxBytes: 64 }).map((d) => d.name), + ["xform_small"], + ); + } finally { + FS_MODULE.readFileSync = origRead; + FS_MODULE.statSync = origStat; + FS_MODULE.openSync = origOpen; + FS_MODULE.closeSync = origClose; + } + + // Fail loudly rather than pass vacuously: if the scan read nothing, every check below is empty. + assert.ok(readArgs.length > 0, "the index build read no file — the checks below would be vacuous"); + for (const a of readArgs) { + assert.strictEqual( + typeof a, + "number", + `readFileSync was handed a PATH (${String(a)}); the size check and the read must share one fd`, + ); + } + assert.deepStrictEqual(statPaths, [], "a path-based statSync re-opens the TOCTOU window"); + // Both files are opened (the oversize one too, to fstat it), so this also proves the `finally` in + // readCapped releases the descriptor on the skip path — the leak the fd rewrite could have added. + assert.strictEqual(opened.length, 2, "expected one open per .py file in the fixture tree"); + assert.deepStrictEqual(closed, opened, "every opened descriptor must be closed, oversize path included"); + }); +}); diff --git a/messagefoundry/api/multipart.py b/messagefoundry/api/multipart.py index 5821df1f..2e7cffcd 100644 --- a/messagefoundry/api/multipart.py +++ b/messagefoundry/api/multipart.py @@ -19,7 +19,30 @@ import re from dataclasses import dataclass, field -_DISPOSITION_PARAM = re.compile(r'(\w+)="([^"]*)"') +# The leading ``(? _MAX_PART_HEADER_BYTES: + # Refuse rather than skip: a header this size is never a real client, and skipping would + # surface as the confusing "no file part" error instead of naming the actual problem. + raise MultipartError( + f"multipart part header block is {len(head)} bytes; the limit is " + f"{_MAX_PART_HEADER_BYTES}" + ) if content.endswith(b"\r\n"): content = content[:-2] # trailing CRLF before the next delimiter name, filename = _disposition(head) diff --git a/tests/test_cert_cli.py b/tests/test_cert_cli.py index 529dcfb3..f163a259 100644 --- a/tests/test_cert_cli.py +++ b/tests/test_cert_cli.py @@ -218,8 +218,11 @@ def test_inventory_lists_facts_and_flags_expired( certs = {c["path"]: c for c in json.loads(capsys.readouterr().out)["certs"]} g = certs[str(good_path)] - assert "good.example.org" in g["subject"] - assert "good.example.org" in g["issuer"] + # Exact DN, not a substring: a substring match also passes for a lookalike CN (the SAN + # `www.good.example.org` contains `good.example.org`), so it would not catch the wrong name + # being reported. `_make_cert` is self-issued, so issuer == subject. + assert g["subject"] == "CN=good.example.org" + assert g["issuer"] == "CN=good.example.org" assert g["sans"] == ["good.example.org", "www.good.example.org"] assert g["expired"] is False assert g["days_remaining"] >= 40 @@ -238,7 +241,10 @@ def test_inventory_human_output_renders_facts( assert main(["cert", "inventory", "--cert", str(cert_path)]) == 0 printed = capsys.readouterr().out - assert "human.example.org" in printed + # Assert the CN on the SUBJECT line specifically: a bare `"human.example.org" in printed` is + # satisfied by the SAN line alone, so it would still pass if the subject stopped being rendered. + subject_line = next(ln for ln in printed.splitlines() if ln.strip().startswith("subject:")) + assert subject_line.split(":", 1)[1].strip() == "CN=human.example.org" assert "SAN(DNS)" in printed assert "notAfter" in printed diff --git a/tests/test_ci_venv_pinning.py b/tests/test_ci_venv_pinning.py new file mode 100644 index 00000000..480b4daf --- /dev/null +++ b/tests/test_ci_venv_pinning.py @@ -0,0 +1,97 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Guard CI's lock-only scratch venvs against an unpinned toolchain install (Scorecard +PinnedDependencies). + +Three CI steps build a throwaway venv whose ONLY install is a committed lockfile — fully `==`-pinned +and hash-verified: the DEP-1 install check (`security.yml`, `/tmp/lockcheck` <- `requirements.lock`) +and the two CycloneDX SBOM builds (`release.yml` + `security.yml`, `/tmp/sbomenv` <- +`docker/locks/requirements-core.lock`). All three used to precede that with +`/bin/pip install --upgrade pip`, an UNPINNED, unverified PyPI fetch; in `release.yml` it sat +inside the job holding `contents: write` + `id-token: write` + `attestations: write`. It bought nothing +— `--require-hashes` rejects any requirement without a hash and therefore performs no dependency +resolution at all, so the pip `ensurepip` provisions is sufficient — so it was deleted rather than +pinned. + +Two regressions this pins, neither of which any other test can see (nothing in the suite executes a +workflow — they need a tag push, a schedule, or GitHub OIDC): + +1. **Reinstating the bootstrap**, in either spelling — `/bin/pip install --upgrade pip` or + `/bin/python -m pip install --upgrade pip`, the form used elsewhere in these same files. +2. **Hiding it** behind `python -m venv --upgrade-deps`, which downloads pip/setuptools from PyPI + exactly as unpinned but is invisible to the scanner. That is ADR 0034's rejected option 3 — a + visible dismissal-with-reason beats an invisible filter. + +Deliberately scoped to the LOCK-ONLY venvs. `/tmp/relsmoke` (`release.yml`) legitimately installs +unpinned `packaging` — it exists to prove the freshly built wheel's own declared closure resolves, so +feeding it a lock would defeat its purpose — and is dismissed separately. Pure text checks, no network. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest + +_REPO = Path(__file__).resolve().parents[1] +_WORKFLOWS = _REPO / ".github" / "workflows" + +#: ``(workflow, scratch venv)`` pairs whose every install must come from a hash-verified lock. +LOCK_ONLY_VENVS = ( + ("release.yml", "/tmp/sbomenv"), + ("security.yml", "/tmp/sbomenv"), + ("security.yml", "/tmp/lockcheck"), +) + +#: Workflows carrying at least one lock-only scratch venv (for the file-wide ``--upgrade-deps`` check). +_WORKFLOW_FILES = tuple(dict.fromkeys(wf for wf, _ in LOCK_ONLY_VENVS)) + + +def _code_lines(wf: Path) -> list[str]: + """The workflow's non-comment lines — the rationale comments name the very commands under test.""" + return [ + ln.strip() + for ln in wf.read_text(encoding="utf-8").splitlines() + if not ln.strip().startswith("#") + ] + + +def _install_re(venv: str) -> re.Pattern[str]: + """Match an install into ``venv`` in EITHER spelling: ``/bin/pip install ...`` and + ``/bin/python -m pip install ...``. Matching only the first would leave the second — the form + used for the interpreter-level installs in these same workflows — a silent way back in.""" + return re.compile(rf"{re.escape(venv)}/bin/(?:pip|python\s+-m\s+pip)\s+install\b") + + +@pytest.mark.parametrize(("workflow", "venv"), LOCK_ONLY_VENVS) +def test_lock_only_scratch_venv_installs_are_hash_pinned(workflow: str, venv: str) -> None: + wf = _WORKFLOWS / workflow + lines = _code_lines(wf) + + # Non-vacuity: if a scratch venv is restructured away, fail loudly rather than pass by finding + # nothing to check. + assert any(f"python -m venv {venv}" in ln for ln in lines), ( + f"{workflow} no longer creates the {venv} scratch venv — re-point this guard at whatever " + f"replaced it instead of letting it pass vacuously" + ) + + installs = [ln for ln in lines if _install_re(venv).search(ln)] + assert installs, f"{workflow} creates {venv} but installs nothing into it" + for ln in installs: + assert "--require-hashes" in ln, ( + f"{workflow} installs into the lock-only scratch venv {venv} WITHOUT --require-hashes: " + f"{ln!r}. Every install into this venv must come from a hash-verified lock — an unpinned " + f"fetch here runs in a release publishing/signing context or in the DEP-1 gate itself." + ) + + +@pytest.mark.parametrize("workflow", _WORKFLOW_FILES) +def test_scratch_venvs_do_not_hide_an_unpinned_pip_fetch(workflow: str) -> None: + """``--upgrade-deps`` is the same unpinned pip download, just invisible to Scorecard.""" + offenders = [ln for ln in _code_lines(_WORKFLOWS / workflow) if "--upgrade-deps" in ln] + assert not offenders, ( + f"{workflow} uses `venv --upgrade-deps` ({offenders}) — it downloads pip/setuptools from PyPI " + f"unpinned exactly like the deleted `pip install --upgrade pip`, but the scanner cannot see it. " + f"ADR 0034 requires a visible dismissal over an invisible filter." + ) diff --git a/tests/test_multipart.py b/tests/test_multipart.py index 4a4dd40b..3ede813b 100644 --- a/tests/test_multipart.py +++ b/tests/test_multipart.py @@ -4,9 +4,14 @@ from __future__ import annotations +import re +import time + import pytest from messagefoundry.api.multipart import ( + _DISPOSITION_PARAM, + _MAX_PART_HEADER_BYTES, MultipartError, MultipartTooLargeError, parse_boundary, @@ -78,3 +83,93 @@ def test_no_file_part_rejected() -> None: parse_single_file_upload( f"multipart/form-data; boundary={_B}", _body([part]), max_file_bytes=1024 ) + + +# --- ReDoS guard on the Content-Disposition parameter regex (CodeQL py/polynomial-redos) --------- + +#: The pre-guard pattern. Kept here so the equivalence test proves the ``(? None: + """The ``(? None: + """A Content-Disposition line of many word chars and no ``="`` must not blow up quadratically. + + The header block is attacker-supplied and ``parse_single_file_upload`` runs synchronously on the + asyncio event loop, so a quadratic scan here is a whole-engine denial of service, not a slow + request. ``_MAX_PART_HEADER_BYTES`` now bounds the input as well, but the two controls are + independent on purpose: the cap is a size policy someone could reasonably raise, while this asserts + the scan itself stays linear at any size. Assert the growth ratio rather than a wall-clock budget so + the test is not flaky on a loaded CI runner: quadratic scaling multiplies by ~16 when the input + quadruples; linear scaling stays near ~4. + """ + + def elapsed(n: int, reps: int = 3) -> float: + """Best-of-``reps``: a scheduling hiccup can only inflate a sample, never deflate one, so the + MINIMUM is the noise-free estimate — one slow slice on a loaded runner cannot fake a red.""" + line = "content-disposition: " + "a" * n + best = float("inf") + for _ in range(reps): + start = time.perf_counter() + _DISPOSITION_PARAM.findall(line) + best = min(best, time.perf_counter() - start) + return best + + base_n = 20_000 + elapsed(base_n) # warm the regex cache / JIT-free interpreter paths + small = max(elapsed(base_n), 1e-6) + large = elapsed(base_n * 4) + assert large / small < 8.0, f"scaling looks super-linear: {small=} {large=}" + + +def test_oversized_part_header_is_refused_not_parsed() -> None: + """A part header block past ``_MAX_PART_HEADER_BYTES`` is rejected before ``_disposition`` runs. + + The per-part ``max_file_bytes`` cap applies to a part's *content*, and only AFTER its header has + been parsed — so without this bound the header scan's input is the whole request body (25 MiB by + default, 512 MiB at the ceiling) on the asyncio event loop. A real client never approaches it: a + Content-Disposition plus a Content-Type is a couple hundred bytes. + """ + fat = b"X" * (_MAX_PART_HEADER_BYTES + 1) + part = ( + b'Content-Disposition: form-data; name="file"; filename="a.hl7"\r\nX-Pad: ' + + fat + + b"\r\n\r\nbody" + ) + with pytest.raises(MultipartError, match="header block"): + parse_single_file_upload( + f"multipart/form-data; boundary={_B}", _body([part]), max_file_bytes=10_000_000 + ) + + +def test_realistic_part_header_is_well_under_the_cap() -> None: + """Non-vacuity for the cap: an ordinary upload's header must be nowhere near the limit, so the + bound can never start rejecting legitimate traffic.""" + head = b'Content-Disposition: form-data; name="file"; filename="acme.hl7"\r\nContent-Type: application/octet-stream' + assert len(head) * 50 < _MAX_PART_HEADER_BYTES + file = parse_single_file_upload( + f"multipart/form-data; boundary={_B}", + _body([head + b"\r\n\r\nMSH|body"]), + max_file_bytes=1024, + ) + assert file.filename == "acme.hl7"