Repository navigation
test: keep audit fixture advisory range clear of Bun.version - #40039
Conversation
normalizeBunSnapshot rewrites the current Bun version, so the mime advisory's "<1.4.1" range broke the snapshot once Bun hit 1.4.1. No-Verification-Needed: test fixture only
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughChangesMIME audit fixture
Merge Risk: ⚪ Minimal · up to This change only updates an audit test fixture and its expected snapshot to avoid a Bun version collision; no actionable merge-blocking product or production risk remains beyond normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-fixture-only change that correctly unblocks CI; mime@1.3.4 still satisfies <1.3.5 and the snapshot is updated to match. The inline nit about the underlying normalizeBunSnapshot collision class is a fair follow-up but not blocking here.
Extended reasoning...
Overview
This PR changes a single string in a test fixture (audit-fixtures.json) from <1.4.1 to <1.3.5 and updates the corresponding bun-audit.test.ts.snap snapshot in two places (text and JSON output). No production code is touched.
The root cause is that normalizeBunSnapshot in test/harness.ts does a blanket .replaceAll(Bun.version, "<bun-version>") over the whole audit report, and once Bun reached 1.4.1 that started matching the mime advisory's (<1.4.1) range, corrupting the snapshot on every platform.
Correctness
- The fixture is served by a mock advisory endpoint (
audit-fixtures.ts→resolveBulkAdvisoryFixture);bun audittrusts the server response and printsvulnerable_versionsverbatim in both text (audit_command.rs:1188-1189) and JSON output, so changing the string only changes what's rendered. The installed fixture version ismime@1.3.4, which still satisfies<1.3.5, so any code path that does evaluate the range (e.g.audit_fix.rs) behaves identically. - The snapshot deltas exactly mirror the fixture delta; nothing else in the snapshot moved.
audit-fixtures.jsonis technically generated bygenerate-audit-fixtures.tsagainst the real npm registry, so a future regeneration would restore<1.4.1. That reinforces the inline nit's point that the durable fix belongs in the normalizer — but regeneration is manual/rare and doesn't block this CI unblock.
Security risks
None. Test fixture data only; no runtime, install, or network code paths touched.
Level of scrutiny
Low. Two-line test-data tweak plus snapshot update, no logic changes, intent and effect are self-evident from the diff.
Other factors
The one finding is explicitly a nit suggesting a follow-up (anchor the version replacement or normalize only the header line) rather than a defect in this change. The next collision candidate in the snapshot (1.9.1) is several releases away, so there's no urgency forcing that follow-up into this PR.
| "title": "mime Regular Expression Denial of Service when MIME lookup performed on untrusted user input", | ||
| "severity": "high", | ||
| "vulnerable_versions": "<1.4.1", | ||
| "vulnerable_versions": "<1.3.5", |
There was a problem hiding this comment.
🟡 nit: this dodges the current collision but the same snapshot still contains other version strings (<1.9.1, <1.16.0, <1.20.3, <2.0.0, and installed-version literals like ms@…2.0.0 / debug@2.2.0) that normalizeBunSnapshot's blanket .replaceAll(Bun.version, …) will hit when Bun's version reaches them. A durable follow-up would be to normalize only the bun audit <version> (<revision>) header line (or anchor the version replacement in normalizeBunSnapshot) rather than bumping one fixture range each time — fine to do separately since this correctly unblocks CI now.
Extended reasoning...
What the bug is
normalizeBunSnapshot in test/harness.ts:2163 performs a blanket .replaceAll(Bun.version, "<bun-version>") over the entire string it's given. bun-audit.test.ts passes the full bun audit stdout — including every advisory body — through this helper before calling .toMatchSnapshot(). That means any substring in the audit report that happens to equal the current Bun.version gets rewritten, corrupting the snapshot.
This PR fixes the immediate instance by moving the mime advisory range from <1.4.1 to <1.3.5 so it no longer collides with Bun 1.4.1. That's correct for today, but it treats one symptom rather than the class.
The specific code path
bun-audit.test.tsrunsbun auditagainst the express@3 fixture and captures stdout.- It calls
normalizeBunSnapshot(stdout)on the full report. harness.ts:2162first replacesBun.version_with_sha→"<version> (<revision>)", which correctly handles the header linebun audit 1.4.1 (abc1234).harness.ts:2163then does.replaceAll(Bun.version, "<bun-version>")over the entire remaining body, catching any incidental occurrence of the bare version string in advisory ranges or installed-version lists.- The result is snapshotted.
Why existing code doesn't prevent it
Line 2162's version_with_sha replacement already neutralizes the only place Bun's version legitimately appears in this output (the header). Line 2163's bare-version replacement is what causes the false positives — it has no anchoring, so it matches inside (<1.4.1), ms@0.7.1, 0.7.2, 2.0.0, etc.
Remaining collision candidates in the snapshot
After this PR the text snapshot still contains all of these, each of which will break identically when Bun.version reaches it:
| String | Where it appears | Can it be dodged by editing the fixture? |
|---|---|---|
1.9.1 |
morgan (<1.9.1) |
yes |
1.16.0 |
serve-static (<1.16.0) |
yes |
1.20.3 |
body-parser (<1.20.3) |
yes |
2.0.0 |
base64-url/ms (<2.0.0) and ms@0.7.1, 0.7.2, 2.0.0 (installed version) |
no — installed versions come from the lockfile, not audit-fixtures.json |
2.2.0 |
debug@2.2.0, 2.6.9 (installed version) |
no |
2.6.9 |
debug (<2.6.9) and installed version |
no |
The installed-version cases are the important ones: they can't be fixed by editing an advisory range in audit-fixtures.json, so the per-fixture-bump approach hits a wall at Bun 2.0.0.
Step-by-step proof
Suppose Bun ships 2.0.0:
- The audit report body contains the line
ms@0.7.1, 0.7.2, 2.0.0(these are the installed versions ofmsin the express@3 tree — from the lockfile, not the advisory fixture). normalizeBunSnapshotruns.replaceAll("2.0.0", "<bun-version>").- The line becomes
ms@0.7.1, 0.7.2, <bun-version>(and(<2.0.0)becomes(<<bun-version>)in two other places). - Snapshot mismatch → test fails on every platform, exactly like the
1.4.1failure this PR fixes.
Impact
Test-only. CI will go red again on a future version bump. No runtime behavior is affected. The next collision (1.9.1) is several minor releases out, so this is not urgent.
How to fix durably
Per REVIEW.md — "Fix bugs at the layer that owns the violated invariant" and "Fix the whole class in the same PR" — either:
- Local: in
bun-audit.test.ts, strip/normalize just the first line (bun audit …) and the timing line before snapshotting, and skipnormalizeBunSnapshotfor the report body; or - Shared: tighten
harness.ts:2163so the bareBun.versionreplacement only fires in anchored contexts (e.g. afterBun v,bun@,Bun), not as a blanket substring replace.
Either avoids playing whack-a-mole with fixture ranges. Given this PR is a targeted CI unblock and the shared-harness change has broader blast radius, doing it as a follow-up is reasonable — hence nit.
1 participant
normalizeBunSnapshot replaces the current Bun version string. The mime advisory fixture used
<1.4.1, which collided once Bun became 1.4.1 and broke thebun-audit.test.tssnapshot on every platform. Moved the range to<1.3.5(mime@1.3.4 still matches).