fix(security): close the open code-scanning backlog — 7 fixes + the ADR 0034 triage register - #37
Merged
Conversation
CodeQL py/incomplete-url-substring-sanitization (alerts 119/120/121) flagged three `"<host>" in <str>` 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.
…t 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 (?<!\w) lookbehind, which is O(1) and rejects every offset inside a run immediately, leaving one \w+ walk per run. It removes no match -- `=` is not a word character, so \w+ starting inside a run can only succeed at that run's end, meaning an interior offset matches iff the run's first offset does, and the leftmost scan always reaches the first offset earlier. Pinned by a differential test against the pre-guard pattern plus a growth-ratio test. Refs ADR 0034.
…by path 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.
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.
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.
The previous pass deleted `<venv>/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 `<venv>/bin/python -m pip install` as well as `<venv>/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.
The `(?<!\w)` lookbehind made the Content-Disposition scan linear, but left its INPUT sized by the attacker. `max_file_bytes` caps a part's content, and only after its header has already been parsed, so the header block's real bound was the request body cap — `[store].max_upload_bytes`, 25 MiB by default and 512 MiB at the ceiling, raised for the two upload paths by the body middleware. Linear is not free at that size: `parse_single_file_upload` runs synchronously on the asyncio event loop that also drives every listener, router worker, transform worker and delivery worker, so one request blocks the whole engine for ~0.35 s at the default and ~7 s at the ceiling. A stalled loop stops ACKing MLLP senders and stops draining the staged queue. `_MAX_PART_HEADER_BYTES` (16 KiB) is orders of magnitude above anything a real client sends — a Content-Disposition plus a Content-Type is a couple hundred bytes, pinned by a non-vacuity test asserting a realistic header is 50x under the limit. Refuse rather than skip: skipping would surface as the confusing "no file part" error instead of naming the actual problem. It maps to the existing 400. The two controls stay independent on purpose. The cap is a size policy someone could reasonably raise; the linear-time assertion holds at any size. Incidentally the bound also gives the ReDoS analysis a length-bounded source rather than an attacker-sized one, which matters because the query models a lookaround as a zero-width assertion and may not credit the lookbehind. Verified by mutation: deleting the guard block reds test_oversized_part_header_is_refused_not_parsed.
…buys
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the standing ADR 0034 violation: 32 code-scanning alerts sat silently open on a public repo, where the policy requires every finding be fixed, or dismissed with a recorded reason.
All 32 were triaged against the real code — 7 fixed here, 25 dismissed with evidence-bearing justifications (already recorded against the repo; open alerts are now 32 → 7, and those 7 are exactly this branch's fixes, which close on re-scan when this lands).
The fixes that matter
py/polynomial-redosinapi/multipart.py— the only network-reachable one. A hostileContent-Dispositiondrove a quadratic scan; now a(?<!\w)guard plus a bounded part-header cap, pinned by a linear-time test and a 10-case differential proving the new pattern keeps the old semantics.js/file-system-raceinide/src/symbolIndex.ts— TOCTOU replaced with oneopenSyncthenfstatSync(fd)/readFileSync(fd), closed infinallyon both exits.pip install --upgrade pipbootstraps deleted from SBOM scratch venvs, pinned by a source guard.py/incomplete-url-substring-sanitizationtest assertions tightened to exact DNs — the substring form also passed for a lookalike SAN minted two lines above, so these were genuinely weak assertions, not false positives.Notable dismissals
The five
clear-text-loggingalerts are all name heuristics, not credentials — e.g._DEK_SECRET_IDis the literal string"MEFOR_STORE_ENCRYPTION_KEY", an env-var name. Where the sink genuinely can carry HL7 (the off-box audit tee),redaction.safe_text()is applied at the chokepoint and pinned by tests.TokenPermissionsIDis won't-fix on hard evidence:contents: writeis required forgh pr merge --auto, and job-level relocation does not satisfy Scorecard — this repo's own dismissed #34/#35 recordjobLevel contents permission set to write.Deliberately not fixed
Five release-path pinning items (
sigstoreunpinned inside the job holdingcontents/id-token/attestations: writeimmediately before signing is the sharpest). No version was guessed — these sit on a critical path no PR CI leg executes, and a wrong pin fails a tag. They are named in the ADR so awon't fixdoes not bury them.Also recorded:
ControlCharScrubFilterdoes not coverrecord.exc_text, so the log-injection rationale must not be inherited by a future finding on alog.exception(...)site.Verification
ruff clean · mypy 0 new errors · pytest 9050 passed, 816 skipped, 1 failed — that failure is the pre-existing
test_anon_paritybug, fixed independently in #35.security.yml/release.ymledits are unexercised by PR CI (schedule/tag-only). Runsecurity.yml's sbom job viaworkflow_dispatchbefore the next tag.