From 3d1dec258babfb1285bb8bb31f9a32049ea32fba Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 28 Jul 2026 19:13:40 -0500 Subject: [PATCH 1/9] test(cert-cli): assert the exact DN instead of a hostname substring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL py/incomplete-url-substring-sanitization (alerts 119/120/121) flagged three `"" in ` assertions in the cert-inventory tests. There is no URL and no sanitization here — `cert inventory` is a read-only report and nothing in the engine makes a trust decision from the rendered subject/issuer — so it is not the vulnerability the query models. The assertions were genuinely weak, though: * `"good.example.org" in g["subject"]` also passes for a lookalike CN, and the test's own SAN list contains `www.good.example.org`; `read_cert_facts` returns `cert.subject.rfc4514_string()`, so the exact expected value is `CN=good.example.org`. * `"human.example.org" in printed` is satisfied by the SAN line alone, so it would keep passing if the human renderer stopped emitting the subject line at all. Tightened to an exact DN comparison and to a subject-line-specific check. No coverage is removed; both tests now fail on a defect they previously accepted. --- tests/test_cert_cli.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) 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 From 89c73d0d77ad7c300d682d721ce2b9e0d4dd17c1 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 28 Jul 2026 19:15:14 -0500 Subject: [PATCH 2/9] fix(api): kill the quadratic Content-Disposition scan in the multipart parser CodeQL alert 125 (py/polynomial-redos, high). `(\w+)="([^"]*)"` restarts at every offset inside a word run and walks the rest of that run before failing, so a Content-Disposition line of n word characters costs O(n^2). The header block is attacker-supplied, bounded only by [store].max_upload_bytes (25 MiB default), and parse_single_file_upload runs synchronously on the asyncio event loop -- so one POST /uploads could wedge the entire engine. Measured before: 2k->10ms, 4k->39ms, 8k->156ms, 16k->613ms, 32k->2895ms (clean quadratic); extrapolated to the 25 MiB cap, ~22 days of blocked event loop. After: the same 25 MiB hostile body parses in 353 ms. The fix is a leading (? 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 bounded only by ``[store].max_upload_bytes`` (25 MiB + default), 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. 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) -> float: + line = "content-disposition: " + "a" * n + start = time.perf_counter() + _DISPOSITION_PARAM.findall(line) + return time.perf_counter() - start + + 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=}" From b58ea56e06caf11b986d3d4c9bbeee36b2353e3c Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 28 Jul 2026 19:46:12 -0500 Subject: [PATCH 3/9] fix(ide): resolve each scanned module once, by descriptor, not twice by path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL js/file-system-race (alert 111). buildSymbolIndex size-checked with statSync(path) and then read with readFileSync(path) — two independent path resolutions with a window between them, so the file that was READ need not be the file that was CHECKED. Exploitability is low (same-privilege, same extension host, over a config dir the extension itself enumerated, and the checked property is a resource guard rather than an authorization decision — an attacker who can swap the file can just write a large .py directly). The reason to fix is non-adversarial correctness: in a live workspace a save, a formatter or a codegen step rewrites a module between the two calls routinely, so the maxBytes guard was unsound as written. The read now goes through readCapped(), which opens once and does fstatSync + readFileSync on that descriptor; a finally closes it on every exit, including the oversize skip (readFileSync does not close a descriptor it is handed). Behaviour is otherwise unchanged: still never throws, still skips an unreadable or oversized file. Pinned by two tests: the size cap still holds, and fs spies assert readFileSync is only ever handed a NUMBER (never a path), that no path-based statSync runs, and that every opened descriptor is closed. Refs ADR 0034. --- ide/src/symbolIndex.ts | Bin 6624 -> 8015 bytes ide/src/test/suite/symbol-index.test.ts | 90 ++++++++++++++++++++++++ 2 files changed, 90 insertions(+) diff --git a/ide/src/symbolIndex.ts b/ide/src/symbolIndex.ts index faf7545c73d059850578d9a17b77818809c45f12..ac209dcb42566b02a972b9529e786adc22e210c4 100644 GIT binary patch delta 1534 zcmZWp!H(lZ5S0)YazR?b1r8K*Az3n$1DDmT1_9ZukU+a)c36p;X{!2L-+v>Xup6@;FX=3cwm%CWOr59d#_&A-(URsWu1`8$V4)MYU>fN)+I~G+T=NViiNV2Y}NHQZRC)gP3R#$bX71_ zDi)IGIk zjp@>9{^5K2IV^<2X=p>_?WtEO&&~z}o_EI8fsznae9_u;DxhWweNKtzPU)ve9KWxW z@zg+z*f^hq*)^gG@NsQF#DN5a7Uh(Pj+tu>B)f0OA>+c6?Ai&NU4O~#}C^zM`^127(>WJ8u zRE#R!us;WO04&Xp>E#O&t)x)n{3cY82xnbTJ4Qn&bAF3F_I>jer3lVJ2YOcIaWkAy zwrFJI4Z4hst{o#$A4dxSoO@B5^~8S?(4 z?wn5Qm^~UU-vexfqyv=y-6vF!qrKQEYXbBhI`H=9xA&*r9uGb5!yU*N7bNXqif7NS zC@mfIP@_Q=am3eH6PopL`rVGbcwOf3K0&OaHBNhiDAc&RD>XssO}_)s6>st)_QP@P z2lhpP9pPjq#%y9W9WIV)19Y5aoW{d!VkxCP09w0Pz*+d~8=&dai;JVyvw?4i<6|!+ z7>f5)hhFMK4~gzeEs=|dq)HL1rCe5+j5JJNz^mYtVY#_GAHm;}f@}I;`QA_VlgXQp mAH;XQN?pGT`8vW&WBVUHKk`!SMVWc&)?5leP*PN>Pz`1&C?x0S6_)^I z6H63q71E0JK&;@(ykw0uO^9Lzh0HVs4Uq8UU7}Kxw@3+SDJaxJb%V@?=qpN1OmWN1 ZNrmXuQBW!^NwZLz9LTS*dB4;I7640>FBJd) 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"); + }); +}); From 72fffd773817c29a5f7da6061d076a05de6340aa Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 28 Jul 2026 19:46:42 -0500 Subject: [PATCH 4/9] fix(ci): drop the unpinned pip bootstrap from the SBOM scratch venv MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scorecard PinnedDependenciesID (alerts 115 and 118): release.yml:177 and security.yml:140 each ran `/tmp/sbomenv/bin/pip install --upgrade pip` — an unpinned, unverified PyPI install — immediately before the only thing that venv ever installs, a fully ==-pinned, hash-verified lock. --require-hashes performs no dependency resolution at all, so the pip ensurepip provisions is sufficient and the upgrade bought nothing. In release.yml it ran inside the job holding contents/id-token/attestations: write. Deleting the command both removes an unpinned install from the release path and closes the alert, so these two are the only PinnedDependencies findings in the group that resolve without a CI-tool lock (the rest stay open, blocked on DEP-1). Deliberately NOT replaced with `python -m venv --upgrade-deps`, which performs the same unpinned fetch while hiding it from the scanner — ADR 0034 option 3, rejected in favour of a visible dismissal over an invisible filter. The new test fails on either regression, and both halves were mutation-checked by reintroducing the deleted line and the --upgrade-deps variant. Refs ADR 0034. --- .github/workflows/release.yml | 7 ++- .github/workflows/security.yml | 6 ++- tests/test_sbom_toolchain_pinning.py | 72 ++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 tests/test_sbom_toolchain_pinning.py diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index cc9f868f..a27240b8 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -173,8 +173,13 @@ 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 `pip install --upgrade pip` in this scratch venv (Scorecard PinnedDependencies): the only + # thing it ever installs is a fully `==`-pinned, hash-verified lock, and --require-hashes needs + # no resolution at all, so ensurepip's bundled pip suffices. The upgrade bought nothing while + # adding an UNPINNED, unverified PyPI install to the job that holds contents/id-token/ + # attestations: write. Do NOT swap in `python -m venv --upgrade-deps` — same unpinned fetch, + # just invisible to the scanner (the "invisible filter" ADR 0034 rejected). 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..a47d8e4d 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -136,8 +136,12 @@ 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 `pip install --upgrade pip` in this scratch venv (Scorecard PinnedDependencies): its only + # install is a fully `==`-pinned, hash-verified lock, and --require-hashes performs no + # resolution, so ensurepip's bundled pip is enough — the upgrade was a redundant UNPINNED PyPI + # fetch. Do NOT swap in `python -m venv --upgrade-deps`: same unpinned fetch, just invisible to + # the scanner (the "invisible filter" ADR 0034 rejected). 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/tests/test_sbom_toolchain_pinning.py b/tests/test_sbom_toolchain_pinning.py new file mode 100644 index 00000000..e829b7af --- /dev/null +++ b/tests/test_sbom_toolchain_pinning.py @@ -0,0 +1,72 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Guard the SBOM scratch venv against an unpinned toolchain install (Scorecard PinnedDependencies). + +`release.yml` and `security.yml` each build the CycloneDX SBOM from a throwaway venv whose ONLY install +is `docker/locks/requirements-core.lock` — fully `==`-pinned and hash-verified. Both used to precede that +with `/tmp/sbomenv/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` performs no resolution, so ensurepip's bundled pip suffices), so it was +deleted rather than pinned. + +Nothing in the suite executes either workflow (they need a tag push / a schedule + GitHub OIDC), so a +refactor could reinstate the unpinned install — or "fix" it with `python -m venv --upgrade-deps`, which +performs the SAME unpinned fetch while hiding it from the scanner (ADR 0034 option 3, rejected: a visible +dismissal beats an invisible filter). Pure text checks, no network. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +_REPO = Path(__file__).resolve().parents[1] +_WORKFLOWS = _REPO / ".github" / "workflows" + +# Both workflows that build an SBOM from the hash-locked core runtime in a scratch venv. +SBOM_WORKFLOWS = (_WORKFLOWS / "release.yml", _WORKFLOWS / "security.yml") + +VENV_CMD = "python -m venv /tmp/sbomenv" +VENV_PIP = "/tmp/sbomenv/bin/pip install" + + +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("#") + ] + + +@pytest.mark.parametrize("wf", SBOM_WORKFLOWS, ids=lambda p: p.name) +def test_sbom_scratch_venv_installs_only_hash_pinned_requirements(wf: Path) -> None: + lines = _code_lines(wf) + + # Non-vacuity: if the scratch venv is ever restructured away, this test must fail rather than pass + # by finding nothing to check. + assert any(VENV_CMD in ln for ln in lines), ( + f"{wf.name} no longer creates the /tmp/sbomenv scratch venv — re-point this guard at whatever " + f"replaced it instead of letting it pass vacuously" + ) + + installs = [ln for ln in lines if VENV_PIP in ln] + assert installs, f"{wf.name} creates /tmp/sbomenv but installs nothing into it" + for ln in installs: + assert "--require-hashes" in ln, ( + f"{wf.name} installs into the SBOM scratch venv WITHOUT --require-hashes: {ln!r}. Every " + f"install in this venv must come from a hash-verified lock — an unpinned fetch here runs in " + f"the release job's publishing/signing context." + ) + + +@pytest.mark.parametrize("wf", SBOM_WORKFLOWS, ids=lambda p: p.name) +def test_sbom_scratch_venv_does_not_hide_an_unpinned_pip_fetch(wf: Path) -> None: + """`--upgrade-deps` is the same unpinned pip download, just invisible to Scorecard.""" + offenders = [ln for ln in _code_lines(wf) if "--upgrade-deps" in ln] + assert not offenders, ( + f"{wf.name} 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." + ) From 9bd23ebee10094781353a106224cd5fd264b680d Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 28 Jul 2026 19:46:55 -0500 Subject: [PATCH 5/9] test(api): make the ReDoS growth-ratio check noise-proof MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The linear-time guard on the Content-Disposition regex (alert 125) compares a 20k-char scan against an 80k one and fails above 8x. Both samples were single shots of ~0.2ms/0.9ms, so one scheduling slice on a loaded CI runner could inflate the large sample past the threshold and red the build for a timing hiccup rather than a real regression. Take the best of three per size instead: a hiccup can only inflate a sample, never deflate one, so the minimum is the noise-free estimate. Measured here at 219us / 875us (ratio 4.0) with the guard, and 1.0s / 15.7s (ratio 15.8 — the gate fires) against the pre-guard pattern, so the check still detects the very regression it exists for. --- tests/test_multipart.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/tests/test_multipart.py b/tests/test_multipart.py index 729eb67a..92cf7612 100644 --- a/tests/test_multipart.py +++ b/tests/test_multipart.py @@ -121,11 +121,16 @@ def test_hostile_disposition_header_parses_in_linear_time() -> None: scaling multiplies by ~16 when the input quadruples; linear scaling stays near ~4. """ - def elapsed(n: int) -> float: + 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 - start = time.perf_counter() - _DISPOSITION_PARAM.findall(line) - return time.perf_counter() - start + 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 From f9b60c40654274cb8075d6ab28819ba7478cb8d6 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 28 Jul 2026 20:55:22 -0500 Subject: [PATCH 6/9] fix(ci): the DEP-1 lock-check venv had the same unpinned pip bootstrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous pass deleted `/bin/pip install --upgrade pip` from the two SBOM scratch venvs but missed the third instance of the identical construct, 63 lines above one of them: security.yml's `/tmp/lockcheck`, whose only install is `--require-hashes -r requirements.lock`. Every word of that pass's rationale applies verbatim — --require-hashes rejects any un-hashed requirement and so performs no resolution at all, making the pip ensurepip provisions sufficient — so the bootstrap bought nothing while adding an unpinned, unverified PyPI fetch to the DEP-1 gate itself. It is code-scanning alert #71, currently dismissed "won't fix" with the reason "CI uses editable installs (pip install -e .[extras]) for testing, which cannot use --require-hashes." That is factually wrong for this line: nothing here is an editable install, and the very next line IS a --require-hashes install. ADR 0034 requires a recorded reason; a wrong one is worse than an open finding, so #71 must be closed as fixed rather than renewed. Both earlier edits are also made LINE-NEUTRAL. This scanner re-raises the same expression at a new line as a NEW alert number — dismissed #18 (__main__.py:976) re-fired as open 122 (:1535), dismissed #39 (dependabot-auto-merge.yml:28) as open 87 (:44), both pure line drift. The 11 added comment lines would have shifted dismissed #35/#69 in release.yml and #74/#75/#76 in security.yml onto new lines, re-opening ~5 findings to close 2. Each rationale block is now one line; the argument lives in the test module's docstring, where it cannot move an anchor. The guard is generalized accordingly and renamed: it covers all three lock-only scratch venvs, and matches `/bin/python -m pip install` as well as `/bin/pip install` — the former is the spelling used elsewhere in these same workflows and the old guard was blind to it. Verified by mutation: reinstating the bootstrap in either spelling, and hiding it behind `venv --upgrade-deps`, each turn the guard red. --- .github/workflows/release.yml | 7 +- .github/workflows/security.yml | 8 +-- tests/test_ci_venv_pinning.py | 97 ++++++++++++++++++++++++++++ tests/test_sbom_toolchain_pinning.py | 72 --------------------- 4 files changed, 100 insertions(+), 84 deletions(-) create mode 100644 tests/test_ci_venv_pinning.py delete mode 100644 tests/test_sbom_toolchain_pinning.py diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index a27240b8..0d03908b 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -173,12 +173,7 @@ 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 `pip install --upgrade pip` in this scratch venv (Scorecard PinnedDependencies): the only - # thing it ever installs is a fully `==`-pinned, hash-verified lock, and --require-hashes needs - # no resolution at all, so ensurepip's bundled pip suffices. The upgrade bought nothing while - # adding an UNPINNED, unverified PyPI install to the job that holds contents/id-token/ - # attestations: write. Do NOT swap in `python -m venv --upgrade-deps` — same unpinned fetch, - # just invisible to the scanner (the "invisible filter" ADR 0034 rejected). + # No unpinned pip bootstrap: --require-hashes resolves nothing (tests/test_ci_venv_pinning.py). python -m venv /tmp/sbomenv /tmp/sbomenv/bin/pip install --require-hashes -r docker/locks/requirements-core.lock python -m cyclonedx_py environment /tmp/sbomenv/bin/python \ diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index a47d8e4d..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,11 +136,7 @@ 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 `pip install --upgrade pip` in this scratch venv (Scorecard PinnedDependencies): its only - # install is a fully `==`-pinned, hash-verified lock, and --require-hashes performs no - # resolution, so ensurepip's bundled pip is enough — the upgrade was a redundant UNPINNED PyPI - # fetch. Do NOT swap in `python -m venv --upgrade-deps`: same unpinned fetch, just invisible to - # the scanner (the "invisible filter" ADR 0034 rejected). + # No unpinned pip bootstrap: --require-hashes resolves nothing (tests/test_ci_venv_pinning.py). python -m venv /tmp/sbomenv /tmp/sbomenv/bin/pip install --require-hashes -r docker/locks/requirements-core.lock python -m cyclonedx_py environment /tmp/sbomenv/bin/python \ 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_sbom_toolchain_pinning.py b/tests/test_sbom_toolchain_pinning.py deleted file mode 100644 index e829b7af..00000000 --- a/tests/test_sbom_toolchain_pinning.py +++ /dev/null @@ -1,72 +0,0 @@ -# SPDX-License-Identifier: AGPL-3.0-or-later -# Copyright (C) 2026 MessageFoundry Organization and contributors -"""Guard the SBOM scratch venv against an unpinned toolchain install (Scorecard PinnedDependencies). - -`release.yml` and `security.yml` each build the CycloneDX SBOM from a throwaway venv whose ONLY install -is `docker/locks/requirements-core.lock` — fully `==`-pinned and hash-verified. Both used to precede that -with `/tmp/sbomenv/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` performs no resolution, so ensurepip's bundled pip suffices), so it was -deleted rather than pinned. - -Nothing in the suite executes either workflow (they need a tag push / a schedule + GitHub OIDC), so a -refactor could reinstate the unpinned install — or "fix" it with `python -m venv --upgrade-deps`, which -performs the SAME unpinned fetch while hiding it from the scanner (ADR 0034 option 3, rejected: a visible -dismissal beats an invisible filter). Pure text checks, no network. -""" - -from __future__ import annotations - -from pathlib import Path - -import pytest - -_REPO = Path(__file__).resolve().parents[1] -_WORKFLOWS = _REPO / ".github" / "workflows" - -# Both workflows that build an SBOM from the hash-locked core runtime in a scratch venv. -SBOM_WORKFLOWS = (_WORKFLOWS / "release.yml", _WORKFLOWS / "security.yml") - -VENV_CMD = "python -m venv /tmp/sbomenv" -VENV_PIP = "/tmp/sbomenv/bin/pip install" - - -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("#") - ] - - -@pytest.mark.parametrize("wf", SBOM_WORKFLOWS, ids=lambda p: p.name) -def test_sbom_scratch_venv_installs_only_hash_pinned_requirements(wf: Path) -> None: - lines = _code_lines(wf) - - # Non-vacuity: if the scratch venv is ever restructured away, this test must fail rather than pass - # by finding nothing to check. - assert any(VENV_CMD in ln for ln in lines), ( - f"{wf.name} no longer creates the /tmp/sbomenv scratch venv — re-point this guard at whatever " - f"replaced it instead of letting it pass vacuously" - ) - - installs = [ln for ln in lines if VENV_PIP in ln] - assert installs, f"{wf.name} creates /tmp/sbomenv but installs nothing into it" - for ln in installs: - assert "--require-hashes" in ln, ( - f"{wf.name} installs into the SBOM scratch venv WITHOUT --require-hashes: {ln!r}. Every " - f"install in this venv must come from a hash-verified lock — an unpinned fetch here runs in " - f"the release job's publishing/signing context." - ) - - -@pytest.mark.parametrize("wf", SBOM_WORKFLOWS, ids=lambda p: p.name) -def test_sbom_scratch_venv_does_not_hide_an_unpinned_pip_fetch(wf: Path) -> None: - """`--upgrade-deps` is the same unpinned pip download, just invisible to Scorecard.""" - offenders = [ln for ln in _code_lines(wf) if "--upgrade-deps" in ln] - assert not offenders, ( - f"{wf.name} 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." - ) From 9899ba7a03e4eaef148b7b407e4c4e59b38d9c3f Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 28 Jul 2026 20:55:39 -0500 Subject: [PATCH 7/9] fix(api): bound a multipart part's header block before parsing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `(? _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_multipart.py b/tests/test_multipart.py index 92cf7612..3ede813b 100644 --- a/tests/test_multipart.py +++ b/tests/test_multipart.py @@ -11,6 +11,7 @@ from messagefoundry.api.multipart import ( _DISPOSITION_PARAM, + _MAX_PART_HEADER_BYTES, MultipartError, MultipartTooLargeError, parse_boundary, @@ -114,11 +115,13 @@ def test_disposition_param_regex_matches_legacy_semantics(line: str) -> None: def test_hostile_disposition_header_parses_in_linear_time() -> None: """A Content-Disposition line of many word chars and no ``="`` must not blow up quadratically. - The header block is attacker-supplied and bounded only by ``[store].max_upload_bytes`` (25 MiB - default), 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. 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. + 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: @@ -137,3 +140,36 @@ def elapsed(n: int, reps: int = 3) -> float: 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" From f6c4d0cc2344f44fdab73c1f12f72969623d5d84 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 28 Jul 2026 20:55:54 -0500 Subject: [PATCH 8/9] docs(ide): claim only the identity guarantee the fd rewrite actually buys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit readCapped's docstring implied the fd rewrite restored the maxBytes cap. It does not. `fs.readFileSync(fd)` re-stats the descriptor itself and reads whatever length it then finds, so a file appended to in place between the `fstatSync` and the read is still read in full — the size check is an early-out, not a bound. What the rewrite does buy is real and worth stating exactly: one path resolution instead of two, so `fstatSync(fd)` and `readFileSync(fd)` cannot disagree about WHICH file they touched. That is the identity guarantee, and it is what CodeQL js/file-system-race flagged. The residual is recorded rather than papered over: maxBytes has always been a best-effort resource guard ("a generated blob isn't a feed"), never an authorization decision; the cost of losing it is memory for one oversized regex pass; and scanModuleSymbols returns only {name, kind, file, line}, never file content, so nothing leaks through it. Making the cap sound would take a bounded readSync into a pre-sized buffer — deliberately not done here, since it would also rewrite the fd-spy test and this worktree cannot run the ide mocha suite. Comment-only: the 127 non-comment lines are byte-identical before and after. --- ide/src/symbolIndex.ts | Bin 8015 -> 8571 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/ide/src/symbolIndex.ts b/ide/src/symbolIndex.ts index ac209dcb42566b02a972b9529e786adc22e210c4..18ff026463d3128140f3ffb0bc9019be3b531177 100644 GIT binary patch delta 805 zcmYLHy>1jS5bm!f4=_qYf-aX9QBXvHbVWkJHFP_>9`9Q5*(?8W*&uYZ)bJok^t=Ks zHLpR#H%3u9@7nYI&aan$ufG3$ap1 zMrGncE)xgZzIuJL+wR`eSUGVhB5V9M|>8Ms15@{rtIVgh{@Z&itM=v;36 z@<9@L31gX5BBlaa7ffDLjv%!_C!S{s23|`f7id0u9)x-+2f=e?dd#V7Zt?GdM{~)= zdEZ!d-d67+e$RJTOOf2+nwS4_R0I1%`{dlRWf0|ZA=buJfRST8CeUlXhR#k|iAD)6 zEJMAIJt)Id0Ff0jr{`-5%_iF6TBexXJr@@P@)=y#gLbGTsTD9dQ`xKTA%?;(h|6&hwEsn*0k@Vhv6iprjRk|7e(zp z2Hs;)Zjt=DWxgg4-cQ|w_SS>SkxS2>_Q7W}9g5plsL070lJE)jx)-`Hw`f9Bt-ObCDtzl@n{rG)ySp5Tknj8}V delta 230 zcmW-bF-`+P3`JX%HWKwO0Y#)m6ckBI7m0e!j5j;V>^Pe7?yyx-ae;6UBzmrZ#1Xgy z6%B^!+xpM`|7Z1a_xe6R;OqDTN*>ygVwB*Ejf|$Jr`4of8f*v39yMqugBm-5RLSi) z-y$RsVi*|@8?43i3|FXfeT7TIrPoMLfGpG!%WB>Mj{yh5ym8( Date: Tue, 28 Jul 2026 20:56:16 -0500 Subject: [PATCH 9/9] docs(adr): record the second static-analysis triage round in ADR 0034 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR 0034 AC-3 requires the class-level rationale and the accepted-risk register to live in-repo so re-scans converge instead of re-litigating. 32 open findings were triaged and the register had not been touched, so the whole round existed only in per-alert comments. Appended as a dated amendment rather than an edit: the repo's own slug-rot guard treats docs/adr/ as historical by construction — an ADR should describe the topology of ITS day — so the 2026-06-26 text stays intact and this section governs where they disagree. Four things the amendment records that a re-scan would otherwise lose: Topology correction. The original Context and the whole Scorecard register rest on MEFORORG being a read-only mirror fed by force-pushed snapshots. Since the cutover it is the primary development repo; publish.ps1 and the release-sync check are gone. Three dismissals reasoned ENTIRELY on that premise — BranchProtectionID (#33), CodeReviewID (#77), MaintainedID (#78) — now carry a justification that is no longer true and must be re-triaged, not renewed. log-injection, narrowed. The register claims control characters "in every emitted record" are neutralized by ControlCharScrubFilter. Precisely: that filter scrubs the RENDERED MESSAGE (record.getMessage() -> record.msg, so the %-args are covered) and does NOT touch record.exc_text or record.stack_info. RedactionFilter renders and PHI-redacts those but does not escape control characters, and the text formatter then appends exc_text verbatim. Verified by execution: a ValueError carrying a newline, logged with exc_info=True, lands on its own physical line. All four log-injection alerts this round are lazy %-arg sinks with no exc_info so the dismissals stand on their own traces — but the class rationale must not be inherited by a future finding on a log.exception site, and the engine has many. PinnedDependenciesID, corrected. "CI installs editably, which cannot use --require-hashes" is structurally true of the editable installs and was applied too widely: three scratch venvs whose only install is a hash-verified lock carried an unpinned pip bootstrap that bought nothing. Also records the proof, from this repo's own alert data, that an == pin does NOT satisfy the check (bandit==1.9.4 is still flagged) — only --require-hashes does, which couples to DEP-1. Convergence and residuals. The line-drift rule (same expression, new line, new alert number, with both confirmed instances), and a table of five hardening items found while justifying won't-fix dismissals — an unpinned sigstore inside the signing job foremost — which a dismissal would otherwise make invisible. AC-5/6/7 added for the guards this round introduced. --- ...is-triage-policy-accepted-risk-register.md | 126 ++++++++++++++++++ 1 file changed, 126 insertions(+) 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.