fix(tests): restore the environment before recomputing the site-code globals - #35
Merged
Conversation
…globals test_anon_parity's engine/tee divergence guard failed in any full-suite run on a box with a real token source configured, while passing in isolation. The cause was a teardown ordering bug three modules earlier. synthetic_site_prefix patches MEFOR_FORBIDDEN_TOKENS and recomputes the surrogates module globals from it. Its teardown called delenv and reloaded BEFORE monkeypatch restored the real value, so _SITE_PREFIXES was left derived from an environment that no longer existed -- and nothing recomputed it once monkeypatch put the real value back. The engine's globals then stayed stale for the rest of the session while the vendored tee/anon copy kept its import-time value, so the two diverged on an unrelated MDM^T01 hundreds of tests later. monkeypatch.undo() restores the real environment first, so the reload sees the same source the module saw at import. Confirmed by prediction rather than inspection: the failure disappears when MEFOR_FORBIDDEN_TOKENS is unset, which is exactly the condition that decides whether the stale value differs from the restored one. The regression guard is declared last in test_anon_core.py so it runs after every fixture user, and asserts the live globals still agree with a fresh recomputation. Verified it can fail: reverting monkeypatch.undo() to the old delenv reddens it (with the mutation proven applied by an exact line count first -- a replace that silently matches nothing reads as a pass). Note it also REPAIRS as it detects, since it reloads. That is deliberate: a reintroduction surfaces as one named failure at the true site instead of a distant, confusing parity failure. This never reddened CI because the test job sets no token source, so the guard the anonymization design depends on is weaker there than on a developer box. Worth closing separately; this change does not address it.
This was referenced Jul 29, 2026
wshallwshall
enabled auto-merge (squash)
July 29, 2026 03:01
wshallwshall
added a commit
that referenced
this pull request
Jul 29, 2026
…DR 0034 triage register (#37) * test(cert-cli): assert the exact DN instead of a hostname substring 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. * 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 (?<!\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. * fix(ide): resolve each scanned module once, by descriptor, not twice 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. * fix(ci): drop the unpinned pip bootstrap from the SBOM scratch venv 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. * test(api): make the ReDoS growth-ratio check noise-proof 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. * fix(ci): the DEP-1 lock-check venv had the same unpinned pip bootstrap 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. * fix(api): bound a multipart part's header block before parsing it 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. * docs(ide): claim only the identity guarantee the fd rewrite actually 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. * docs(adr): record the second static-analysis triage round in ADR 0034 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.
wshallwshall
added a commit
that referenced
this pull request
Aug 7, 2026
…napshot, file #1095 (#276) Three corrections to the ledger's own accuracy, in one commit because they cross-reference: #1094 and the ranking note both point at #1095, so splitting them leaves an intermediate commit citing an item that does not exist yet. 1. #1094 CLOSED as already satisfied when filed; no work performed. Its premise is false on origin/main. The repoint it asks for merged as befe997 (PR #271) ONE COMMIT BEFORE the item itself landed (7ecff8a, PR #272) -- a filing race, not a wrong finding. Re-verified after both: CLAUDE.md section 12 now reads "BACKLOG #26 -- closed, so it lives in docs/archive/backlog/BACKLOG-CLOSED.md, not in the live ledger", same for #27. Banner flipped from the OPEN glyph to a CLOSED one -- replaced, not added, so the item still declares exactly one status. The analysis is kept: its point that no gate in this repo can catch the class is the argument any future check has to answer, and it is now attached to #1095 at true scale. 2. The "Connector & feature-breadth gaps vs. Mirth Connect" section marked a historical snapshot. All TEN backlog numbers it cites -- #7, #20-#27, #35 -- have closed and moved to the archive; none is in this file. So "#7 above" and "#35 below" are false directions out of the document, and "P1 -- close first" names work that shipped: #20 (FHIR, ADR 0022) and #21 (observability, PR #407). The section marks #24 and #35 SHIPPED inline, which makes the unmarked #20/#21 read as still open. A reader planning from this picks up finished work. Deliberately NOT repointed per-number. Every cited item is archived, so attaching an archive path to only the two decline-by-design lines would assert by contrast that the other eight are live. Uniform staleness is at least detectable; differentiated staleness is not. 3. #1095 filed for the systemic class. Number allocated via scripts/coord/alloc.ps1, never grepped. Measured on origin/main with parse_items (imported, not re-derived): of 129 path-bearing BACKLOG.md citations, AT LEAST 69 distinct sites across AT LEAST 35 files name the live ledger for an archived item. Plus 13 hrefs that do not resolve at all, 12 line anchors past EOF (file is 6318 lines; one cites 8429), and 31 in-range anchors that drifted onto unrelated text. The item's central point is DETECTABILITY, because getting this wrong means someone closes it with a linter having fixed a third of it: the 13 broken hrefs and 12 past-EOF anchors are catchable, but the 69 wrong-file citations and the 31 drifted anchors are NOT -- those links resolve perfectly, and what rots is the number or the line beside them. It also records that the test is "does the cited FILE contain the item", not "is the item CLOSED". Those differ: #1073 is closed and still legitimately in the live ledger, so a sweep keyed on closure would corrupt correct citations. Prior art found and named rather than duplicated: MIG-35 is already "the BACKLOG-reference classifier" folding into MIG-74 in the master test plan (:128). The item notes MIG-74 as worded -- "every doc path resolves" -- would pass the largest class untouched, since those paths do resolve. Verification: - parse_items diffed before and after: exactly two items changed state, #1094 (open -> closed) and #1095 (new). No unintended banner churn. - backlog_status_check.py: OK, 365 items, each declaring exactly one status. - All 7 link targets introduced were resolved from docs/, with a known-missing path run through the same checker to prove it can report a miss. - The MIG-74 quote was confirmed verbatim in the source file, not paraphrased from an agent's summary. - Line endings normalized to CRLF to match the file; diff stayed at 50/2 rather than whole-file churn. Not included: the ~69-site sweep itself and any gate. Those are #1095's scope, and a partial repoint is worse than none for the reason given in item 2.
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.
test_anon_parity's engine/tee divergence guard failed in any full-suite run on a box with a real token source configured, while passing in isolation. The cause was a teardown ordering bug three modules earlier.synthetic_site_prefixpatchesMEFOR_FORBIDDEN_TOKENSand recomputes thesurrogatesmodule globals from it. Its teardown calleddelenvand reloaded before monkeypatch restored the real value, leaving_SITE_PREFIXESderived from an environment that no longer existed — and nothing recomputed it once monkeypatch put the real value back. The engine's globals then stayed stale for the rest of the session while the vendoredtee/anoncopy kept its import-time value, so the two diverged on an unrelatedMDM^T01hundreds of tests later.monkeypatch.undo()restores the real environment first, so the reload sees the same source the module saw at import.Confirmed by prediction, not inspection
The failure disappears when
MEFOR_FORBIDDEN_TOKENSis unset — exactly the condition that decides whether the stale value differs from the restored one. Verified on two independent checkouts.The regression guard can actually fail
Declared last in the module so it runs after every fixture user. Reverting
monkeypatch.undo()to the olddelenvreddens it — with the mutation proven applied by an exact line count first, since a replace that silently matches nothing reads as a pass.It also repairs as it detects (it reloads), which is deliberate: a reintroduction surfaces as one named failure at the true site instead of a distant, confusing parity failure.
Why CI never caught it
The pytest legs set no token source;
MEFOR_FORBIDDEN_TOKENSis supplied only to the leak-gate job. So the guard the anonymization design depends on is weaker in CI than on a developer box. This PR does not address that — it is worth closing separately.Full suite on this branch: 9038 passed, 816 skipped, 0 failed.