install: fail closed on unusable npm integrity (pin computed sha512; reject invalid manifest/lockfile hashes) - #31327
Conversation
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
WalkthroughBun now computes and persists integrity checksums for npm registry tarballs that lack usable integrity metadata. The implementation integrates npm packages into the existing tarball extraction and streaming integrity pipeline, enabling sha512 fallback computation and lockfile pinning similar to GitHub and remote tarball sources. ChangesNPM Tarball Integrity Pinning
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/install/bun-install-tarball-integrity.test.ts`:
- Around line 651-660: Replace the manual for (...) loop that generates the
it(...) cases with a Jest parameterized block using describe.each([...]) so the
integrity matrix uses the repository's test convention; take the existing array
of tuples (label, dist, extraEnv) currently iterated in the for loop and pass it
into describe.each, move the it(...) body inside the describe.each callback, use
the label for the test name interpolation (e.g. `(${label})`), and preserve the
tuple names so the test still references dist and extraEnv in the same way as in
the original test function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fd40f88a-1c24-408d-ab2c-7f8df1cffc9b
📒 Files selected for processing (4)
src/install/PackageManager/processDependencyList.rssrc/install/TarballStream.rssrc/install/extract_tarball.rstest/cli/install/bun-install-tarball-integrity.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find correctness bugs, but this changes integrity-verification and lockfile write-back behavior for npm packages in bun install (security-adjacent), so it's worth a human look — note also the build-rust CI failures on aarch64-musl and FreeBSD.
Extended reasoning...
Overview
This PR extends the existing SHA-512 fallback (already applied to GitHub / remote / local tarballs) to npm registry packages whose manifest dist.integrity is missing, uses an unsupported algorithm, or is malformed. It touches three Rust files in src/install/ (extract_tarball.rs, TarballStream.rs, PackageManager/processDependencyList.rs) plus ~210 lines of new tests. The Rust changes are small and closely mirror the existing arms for other resolution tags; the new write-back in process_extracted_tarball_package only fires when resolution.tag == Npm, package_id is valid, the extract produced a supported digest, and the lockfile entry's existing integrity is not already supported.
Security risks
This is squarely in security-sensitive territory: it governs whether and how downloaded package bytes are pinned and verified. The change is additive (it adds verification where there was previously none) and is guarded so it never overwrites an existing supported integrity, and it respects --no-verify. I don't see a way it weakens verification, but because it alters the trust/verification surface for the most common dependency source (npm registry), a human should confirm the semantics — particularly the resolve-phase write-back into lockfile.packages[...].meta.integrity and its interaction with lockfile dirty/re-save logic.
Level of scrutiny
High. bun install integrity verification and lockfile persistence are production-critical, supply-chain-relevant paths. Even though the diff is mechanically simple (adding ResolutionTag::Npm to existing match arms), the behavioral implications (every npm package without usable integrity now gets a computed pin) and the new mutation site in processDependencyList.rs warrant maintainer review.
Other factors
- CI (robobun) reports build-rust failures on aarch64-musl and FreeBSD x64 for commit 7459f19; an autofix commit followed but the status comment still shows red.
- Good test coverage added: parametrized over unsupported-algorithm / malformed-base64 / streaming-extractor variants, plus a no-resave-on-second-install check.
- CodeRabbit's only comment is a stylistic nit (
describe.eachvsforloop) — not blocking. - No CODEOWNERS entry for
src/install/.
There was a problem hiding this comment.
No issues found, but this changes integrity-pinning semantics for npm registry packages (a supply-chain security path) and writes back into lockfile package metadata, so it's worth a human look.
Extended reasoning...
Overview
This PR extends the existing SHA-512 fallback (already applied to GitHub/remote/local tarballs) to npm registry packages whose manifest dist.integrity is missing, uses an unsupported algorithm, or fails to parse. It touches three Rust files in src/install/ (the buffered extractor, the streaming extractor, and the resolve-phase lockfile write-back) plus adds ~210 lines of tests. The core change is small (~30 lines) and mechanically mirrors the adjacent Github | RemoteTarball | LocalTarball arms.
Security risks
This is a supply-chain security path: it introduces TOFU pinning for npm packages that previously went permanently unverified. The change is strictly additive (it only writes a computed integrity when !meta.integrity.tag.is_supported(), never overwriting a registry-provided value) and respects --no-verify. I see no way it weakens existing verification, but because it changes when/how integrity is recorded and later enforced for the most common package source, it deserves human sign-off.
Level of scrutiny
Medium-high. The diff is small and pattern-following, but integrity verification and lockfile persistence are correctness-critical. Subtle issues (e.g., the resolve-phase write-back interacting with lockfile dirtying/re-save, or the package_id != INVALID_PACKAGE_ID guard) are the kind of thing a maintainer familiar with the install pipeline should confirm.
Other factors
Test coverage is solid: it exercises both buffered and streaming extractors, verifies swap detection on reinstall, and asserts the lockfile is not re-saved on a second install. The one CodeRabbit comment (style nit on for vs describe.each) was reasonably declined and resolved. No CODEOWNERS apply to these files.
77035b5 to
f3fe5f6
Compare
|
Classified build #57590: the two failing checks are |
There was a problem hiding this comment.
I didn't find any issues, but this changes integrity-verification and lockfile-persistence behavior for the most common package type (npm registry), so it's worth a human sign-off on the TOFU-pinning semantics.
Extended reasoning...
Overview
This PR extends the existing SHA-512 fallback (already applied to GitHub/remote/local tarballs) to npm registry packages whose manifest dist.integrity is missing, uses an unsupported algorithm, or is malformed. Three small Rust hunks:
extract_tarball.rs(~10 lines): adds aResolutionTag::Npmarm mirroring the existing GitHub/remote/local arm, guarded by!self.skip_verify.TarballStream.rs(~6 lines): includes Npm incompute_if_missing(when notskip_verify) and in thepopulate_resultintegrity match.processDependencyList.rs(~13 lines): after extract, writes the computed integrity back intolockfile.packages[id].meta.integrityonly when the existing value is unsupported.
Plus ~210 lines of new end-to-end tests covering buffered + streaming paths, swap detection on reinstall, and no-resave on a second install.
Security risks
This is security-relevant code — package integrity verification — but the change strengthens it (TOFU pinning where previously there was none). The write-back is guarded by !meta.integrity.tag.is_supported(), so a registry-provided integrity is never overwritten. --no-verify opts out, matching the existing contract. I don't see a way this weakens verification; the risk surface is a wrong hash being pinned (causing spurious failures on reinstall) or unintended lockfile churn, both of which the new tests cover.
Level of scrutiny
Medium-high. The diff is small and mechanically extends an established pattern, but it changes user-visible lockfile behavior for the dominant resolution type and lives on the integrity-verification path that every bun install exercises. A human should confirm the design decision (TOFU pinning for npm packages whose manifest integrity is unusable) and that the resolve-phase write-back into lockfile.packages composes correctly with later lockfile serialization / dirty-tracking.
Other factors
- Bug-hunting system found nothing; the only review feedback was a CodeRabbit style nit (for-loop vs describe.each) that the author reasonably declined.
- No CODEOWNERS coverage for
src/install/. - Author reports the new tests pass locally on the rebased branch and fail under
USE_SYSTEM_BUN=1as expected; CI build #58769 was still running at the time of the last timeline update.
… usable integrity When a registry manifest's dist.integrity is missing, uses an unsupported algorithm, or fails to parse (and there is no shasum), npm packages were recorded in the lockfile with an empty integrity and never verified on any install. Extend the existing GitHub/remote/local tarball fallback to npm: compute a SHA-512 of the downloaded tarball and persist it into the lockfile during the resolve phase, so the first install's lockfile already carries the pin and subsequent installs verify against it. Skipped under --no-verify. Adds tests covering the pin being recorded for unparseable manifest integrity (buffered and streaming extract paths), a swapped tarball being rejected on reinstall, and the lockfile not being re-saved on a second install.
- npm.rs: a registry manifest whose dist.integrity / dist.shasum is present but unparseable now marks the version Tag::INVALID instead of silently degrading to UNKNOWN - Package::from_npm refuses a version with Tag::INVALID so resolving that version is an install error - TarballStream / extract_tarball compute SHA-512 for npm tarballs when no usable integrity was supplied (missing entirely), and runTasks writes the computed hash back to the lockfile package and forces a save so the pin persists - bun.lock: a non-empty unparseable npm integrity is a parse error at lockfileVersion 2+; an empty npm integrity is a parse error under --frozen-lockfile and a warning otherwise (back-filled on download)
f3fe5f6 to
2e5eab5
Compare
robobun
changed the title
|
Rebased onto current main (was ~1040 commits behind) and extended to the full fail-closed policy:
11 tests cover the four faces; 9 of them fail with |
| function octal(n: number, width: number) { | ||
| return n.toString(8).padStart(width - 1, "0") + "\0"; | ||
| } | ||
| function tarHeader(name: string, size: number) { | ||
| const buf = Buffer.alloc(512, 0); | ||
| buf.write(name, 0, 100, "utf8"); | ||
| buf.write(octal(0o644, 8), 100); | ||
| buf.write(octal(0, 8), 108); | ||
| buf.write(octal(0, 8), 116); | ||
| buf.write(octal(size, 12), 124); | ||
| buf.write(octal(0, 12), 136); | ||
| buf.fill(" ", 148, 156); | ||
| buf.write("0", 156); | ||
| buf.write("ustar\0", 257); | ||
| buf.write("00", 263); | ||
| let sum = 0; | ||
| for (let i = 0; i < 512; i++) sum += buf[i]; | ||
| buf.write(octal(sum, 8), 148); | ||
| return buf; | ||
| } |
There was a problem hiding this comment.
🟡 This new describe block adds a third near-identical copy of the octal / tarHeader / pad512 / buildTarball helpers — the same ~30 lines already appear in the "tarball integrity mismatch" describe (~line 470) and the "tarball integrity metadata forms" describe (~line 580) of this file. Consider hoisting them to file scope and reusing them across all three blocks. Nit only — no functional impact, and the two prior copies are pre-existing, so a follow-up dedup would be fine too.
Extended reasoning...
What this is
The new describe.concurrent("npm registry without usable integrity metadata") block re-defines four small tar-builder helpers inline at lines 756-787:
octal(n, width)— format a number as a NUL-terminated octal fieldtarHeader(name, size)— build a 512-byte ustar header with a computed checksumpad512(len)— pad to a 512-byte boundarybuildTarball(body)— assemble a single-entry gzipped tarball from a package.json body
These same helpers already exist twice in this file, byte-for-byte or near enough:
- Inside
it("should fail (not hang) when tarball bytes don't match manifest SHA-512")in the "tarball integrity mismatch (%s)" describe (around line 470), which definesoctal,tarHeader,pad512, and abuildTarballthat returns{ tgz, integrity }. - At the top of the "tarball integrity metadata forms" describe (around line 580), which defines
octal,tarHeader, and abuildTarballthat inlines the pad and returns{ tgz, sha512, sha384 }.
This PR adds a third copy whose buildTarball returns just the gzipped bytes.
Step-by-step comparison
Taking tarHeader as the concrete example — all three copies are identical: a 512-byte zero-filled buffer, the name at offset 0, mode 0o644 at 100, uid/gid zeros at 108/116, size at 124, mtime zero at 136, checksum field blanked to spaces at 148-156, typeflag '0' at 156, ustar\0 magic at 257, version 00 at 263, then the byte-sum written back into 148. The octal and pad512 helpers are likewise identical across the copies that have them. Only buildTarball's return shape differs between the three sites, and each variant is a one-liner over the shared pieces.
Why this doesn't block
REVIEW.md's dedup rule reads "The second time a multi-line block appears in your diff, extract a named helper" — strictly, only one copy is in this diff; the other two are pre-existing. REVIEW.md also cautions "don't ride file-wide standardization on a focused bugfix", which cuts the other way: hoisting would touch two describe blocks this PR otherwise doesn't change. So this sits in the discretionary zone rather than being a hard rule violation, and there is zero functional impact — the tests are correct as written.
Why it's still worth mentioning
The broader principle — "grep for the in-tree helper before hand-writing anything" — does apply: the helper already existed twice in the very file being edited. Three copies of a 30-line block in one file is a maintenance smell (the next person who needs to tweak the tar checksum has three places to find), and hoisting octal / tarHeader / pad512 plus a base buildTgz(body): Buffer to file scope is low-risk (pure functions, no shared state) and would delete ~60 lines net.
Suggested fix
Hoist octal, tarHeader, pad512, and a base buildTgz(body: Buffer): Buffer to file scope (just after the imports / withContext helper), then have each describe block wrap buildTgz locally for its own return shape ({ tgz, integrity }, { tgz, sha512, sha384 }, or bare Buffer). Alternatively, land as-is and dedup in a follow-up — either is fine for a nit.
| const lockPath = join(String(dir), "bun.lock"); | ||
| const lock = readFileSync(lockPath, "utf8"); | ||
| writeFileSync(lockPath, lock.replace(/"sha\d+-[A-Za-z0-9+/]+=*"]/, '""]')); |
There was a problem hiding this comment.
🟡 This test writes lock.replace(...) directly without asserting the regex matched, so if the lockfile writer's output shape ever changes, replace becomes a silent no-op and the test fails at a later assertion with a misleading message. The sibling test immediately below ("rejects an unsupported-algorithm integrity in the lockfile") uses the identical regex and does expect(tampered).not.toBe(lock) before writing — worth doing the same here for consistency and per REVIEW.md's "assert that setup created the precondition".
Extended reasoning...
What this is
The Face C test tampers with the lockfile by blanking the npm package's integrity string via a regex replacement:
const lock = readFileSync(lockPath, "utf8");
writeFileSync(lockPath, lock.replace(/"sha\d+-[A-Za-z0-9+/]+=*"]/, '""]'));String.prototype.replace returns the original string unchanged when the pattern doesn't match. There's no assertion that the replacement actually happened, so if the lockfile serializer's quoting/spacing/line-wrapping around the integrity field ever changes and this regex stops matching, the setup step silently writes back an unmodified lockfile with a valid sha512-… still in place.
Why it matters (and why it's only a nit)
REVIEW.md's test rules say: "assert that setup created the precondition". The precondition this test needs is "the lockfile now has an empty integrity string for pkg", and that precondition is currently unverified.
To be clear about impact: the test would not silently pass if the replace became a no-op. Walking through it — the lockfile keeps its valid sha512, the --frozen-lockfile install would then succeed (exit 0, no warning), and the very next assertion expect(r.stderr).toContain("Package pkg has no integrity pin in the frozen lockfile") would fail. So the test would still catch the problem, just with a confusing diagnostic ("expected stderr to contain … but it was empty") that points at the wrong line and makes the reader think Bun stopped emitting the message rather than that the fixture setup drifted.
Consistency with the sibling test
The immediately-adjacent Face D test in this same PR uses the identical regex and already guards it:
const tampered = lock.replace(/"sha\d+-[A-Za-z0-9+/]+=*"]/, '"md5-AAAAAAAAAAAAAAAAAAAAAA=="]');
expect(tampered).not.toBe(lock);
writeFileSync(lockPath, tampered);Two adjacent tests using the same fragile regex, one asserting the precondition and one not, is the kind of inconsistency that gets copy-pasted forward. Since the guard exists 30 lines below, this is a one-line addition.
Fix
const lock = readFileSync(lockPath, "utf8");
const tampered = lock.replace(/"sha\d+-[A-Za-z0-9+/]+=*"]/, '""]');
expect(tampered).not.toBe(lock);
writeFileSync(lockPath, tampered);Not blocking — the regex matches today's lockfile format and the test as written cannot pass vacuously — but worth the one line for parity with the sibling and for a clearer failure mode down the road.
| await using proc = spawn({ | ||
| cmd: [bunExe(), "install", "--save-text-lockfile"], | ||
| cwd: String(dir), | ||
| env: { ...env, BUN_INSTALL_CACHE_DIR: cacheDir }, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); |
There was a problem hiding this comment.
🟡 The first-install spawn sets stdout: "pipe" but the Promise.all only drains [proc.stderr.text(), proc.exited] — stdout is piped but never read, unlike the second spawn in this same test (line 1074) and the runInstall helper which drain all three. Add proc.stdout.text() to the Promise.all (or drop stdout: "pipe") to match the harness convention in REVIEW.md; a single tiny-package install won't fill the 64KB pipe buffer today, so this is consistency-only, not a hang risk.
Extended reasoning...
What the issue is
In the new "does not re-save the lockfile on reinstall" test, the first-install block spawns bun install --save-text-lockfile with stdout: "pipe" and stderr: "pipe", but the subsequent Promise.all only reads proc.stderr.text() and proc.exited:
await using proc = spawn({
cmd: [bunExe(), "install", "--save-text-lockfile"],
...
stdout: "pipe",
stderr: "pipe",
});
const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);stdout is piped but never drained.
Why this matters (per REVIEW.md)
REVIEW.md states: "Subprocess tests: drain pipes concurrently. Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]) — an unread pipe fills the ~64KB OS buffer and deadlocks the child." Piping a stream and never reading it is the canonical shape that convention exists to prevent.
Step-by-step trace
- Line 1045-1051:
spawn({..., stdout: "pipe", stderr: "pipe"})creates the subprocess with kernel pipes attached to both stdout and stderr. - Line 1052:
Promise.all([proc.stderr.text(), proc.exited])starts consuming stderr and awaits exit, but nothing is ever registered as a reader on the stdout pipe. - The child (
bun install) writes its normal summary ("+ pkg@1.0.0", "1 package installed", timing) to stdout. For a single tiny package this is on the order of tens of bytes — nowhere near the ~64KB kernel pipe buffer — so the child's writes never block and it exits cleanly. await using procdisposes on scope exit, so the unread stdout is eventually torn down and there is no leak.
So there is no realistic deadlock for this specific test's output volume; the issue is purely a harness-convention / consistency violation.
Why it's still worth fixing
The second spawn in the same test (line 1074) and the runInstall helper defined for this describe block both do Promise.all([proc.stderr.text(), proc.stdout.text(), proc.exited]). This block is the odd one out inside its own test, which invites copy-paste of the wrong shape into a future test where stdout is large enough to wedge. Note that the same undrained-stdout pattern already exists in a couple of pre-existing tests in this file (e.g. the 404 test's first install), so this is not unique to the PR — but this is new code in the diff and matching the convention here is a one-token change.
Fix
Either add proc.stdout.text() to the Promise.all:
const [stderr, , exitCode] = await Promise.all([proc.stderr.text(), proc.stdout.text(), proc.exited]);or, since stdout isn't asserted on here, drop stdout: "pipe" from the spawn options so it inherits and there's nothing to drain.
| // SHA-512 once the tarball is fetched, so a non-frozen | ||
| // install self-heals; under --frozen-lockfile the lockfile | ||
| // cannot be updated and the unverified entry is refused. | ||
| // Gated to v2+ so lockfiles written before the back-fill | ||
| // existed (which always persisted `""` for a registry with | ||
| // no integrity) keep loading silently. | ||
| if lockfile_version.at_least(Version::V2) && integrity_str.is_empty() { | ||
| let frozen = manager | ||
| .as_deref() | ||
| .is_some_and(|m| m.options.enable.frozen_lockfile()); | ||
| if frozen { | ||
| log.add_error_fmt( | ||
| Some(source), | ||
| item_loc(source, key_loc, i), | ||
| format_args!( | ||
| "Package {} has no integrity pin in the frozen lockfile", | ||
| bstr::BStr::new(name_str), | ||
| ), | ||
| ); | ||
| return Err(ParseError::InvalidPackageInfo); | ||
| } | ||
| log.add_warning_fmt( | ||
| Some(source), | ||
| item_loc(source, key_loc, i), | ||
| format_args!( | ||
| "Package {} has no integrity pin in the lockfile; it will be computed after download", | ||
| bstr::BStr::new(name_str), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 The comment says this check is "Gated to v2+ so lockfiles written before the back-fill existed… keep loading silently", but Version::CURRENT was already V2 on the base commit (ae4b17d introduced V2 on 2026-07-24; this PR's base is 59242d6) — so pre-PR Bun already writes v2 lockfiles, and per this PR's own Face B those v2 lockfiles persist "" for npm packages whose registry omits integrity. After upgrading, bun install --frozen-lockfile / --production on such a lockfile fails with "Package X has no integrity pin in the frozen lockfile", breaking CI with no code change. Either bump to a new Version::V3 and gate on it (matching how the pre-existing V2 invariant at line 2656 was introduced alongside the V2 bump), or rewrite the comment and PR description to state the retroactive enforcement is intentional.
Extended reasoning...
What the bug is
The new empty-integrity check at bun.lock.rs:2676 refuses a v2 lockfile npm entry with "" integrity under --frozen-lockfile. The gating comment (lines 2673-2675) justifies at_least(Version::V2) with:
Gated to v2+ so lockfiles written before the back-fill existed (which always persisted
""for a registry with no integrity) keep loading silently.
That premise is false. Version::CURRENT was already Version::V2 on the base commit — git show 59242d6c:src/install/lockfile/bun.lock.rs line 120 reads pub const CURRENT: Version = Version::V2;, and git log -S 'Version::V2' shows V2 was introduced in ae4b17d (2026-07-24), 44 commits before this PR's base. So pre-PR Bun already writes v2 lockfiles, and — per this PR's own problem statement, Face B: "bun.lock records """ — those v2 lockfiles persist "" for npm packages whose registry omitted dist.integrity/dist.shasum. The v0/v1 gate protects nothing the comment claims: v0/v1 lockfiles were written before the back-fill existed, but so were v2 lockfiles.
Step-by-step proof
- A user runs a Bun built from main between ae4b17d and this PR (e.g.
e532ad91f, the "release-asan main" the PR description itself was verified against) against a private registry (Artifactory / Nexus / self-hosted) that omits bothdist.integrityanddist.shasum. - That Bun writes
bun.lockwith"lockfileVersion": 2(sinceCURRENT = V2at bun.lock.rs:120 on that build) and, per Face B,""for the affected npm package's integrity slot. - The user upgrades to a Bun containing this PR.
- CI runs
bun install --frozen-lockfile(orbun install --production, which setsFROZEN_LOCKFILEat PackageManagerOptions.rs:812). parse_into_binary_lockfilereaches line 2676:lockfile_version.at_least(Version::V2)istrue(lockfile is v2),integrity_str.is_empty()istrue, andfrozenistrue→add_error_fmt("Package {name} has no integrity pin in the frozen lockfile")andreturn Err(ParseError::InvalidPackageInfo).- Install fails with a non-zero exit; CI breaks on upgrade with no code change.
The PR description repeats the same claim — "v0/v1 lockfiles, written before the back-fill existed, keep loading silently" — so this is the author's stated design intent, and the code fails to deliver it.
Why the existing pattern doesn't cover this
Contrast with the pre-existing off-registry-URL check at line 2656 (npm_url_needs_integrity): that check was introduced in the same commit (ae4b17d) that bumped CURRENT from V1 to V2, so every v2 lockfile ever written satisfies it by construction. This PR adds a new invariant after V2 became CURRENT, so existing v2 lockfiles do not satisfy it. REVIEW.md is explicit here: "Any change to cached/serialized output bumps the format version constant."
The same reasoning applies more weakly to the non-empty-unparseable check at 2633-2640, though that is far less likely to appear in a real pre-PR v2 lockfile since Bun's own writer never emitted an unparseable hash — only hand-editing or third-party tooling would produce one.
Impact
Users with a private registry that omits integrity metadata — exactly the population #19519 targets — have their --frozen-lockfile / --production CI break on Bun upgrade with no repo change. The non-frozen path self-heals (warns, back-fills, re-saves), so the blast radius is limited to frozen/production installs, but that is precisely the CI path.
Fix
Two acceptable resolutions:
- If backward compat is intended (which the comment and PR description both say it is): add
Version::V3, bumpCURRENTtoV3, gate both new checks onat_least(Version::V3), and updatefrom_int/ theV2doc-comment accordingly. This matches exactly how the pre-existing V2 invariants were introduced in ae4b17d. - If retroactive enforcement is intentional (defensible for a fail-closed security fix, since the non-frozen path self-heals and the affected population is narrow): rewrite the comment at 2673-2675 and the PR-description bullet for Face C to say so explicitly, and drop the misleading "lockfiles written before the back-fill existed keep loading silently" claim. Optionally include the remediation ("run
bun installonce without--frozen-lockfileto back-fill") in the error message.
Either way, the current state — code whose behavior contradicts its own documented rationale — should be resolved before merge.
| let mut had_invalid_integrity = false; | ||
| if let Some(sri_str) = dist.get(b"integrity").and_then(|v| v.as_str()) { | ||
| package_version.integrity = Integrity::parse(sri_str); | ||
| if package_version.integrity.tag.is_supported() { | ||
| break 'integrity; | ||
| } | ||
| had_invalid_integrity = !sri_str.is_empty(); | ||
| } | ||
|
|
||
| if let Some(shasum_str) = dist.get(b"shasum").and_then(|v| v.as_str()) { | ||
| package_version.integrity = | ||
| Integrity::parse_sha_sum(shasum_str).unwrap_or_default(); | ||
| if package_version.integrity.tag.is_supported() { | ||
| break 'integrity; | ||
| } | ||
| had_invalid_integrity |= !shasum_str.is_empty(); | ||
| } | ||
|
|
||
| if had_invalid_integrity { | ||
| package_version.integrity = Integrity { | ||
| tag: crate::integrity::Tag::INVALID, | ||
| ..Default::default() | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔴 The new Tag::INVALID path breaks test/cli/install/minimum-release-age.test.ts on all 9 CI platforms (see robobun's comment): its mock registry serves ~48 placeholder dist.integrity values like "sha512-fake1==" which now trip had_invalid_integrity and cause Package::from_npm to fail every install with error: Registry provided an invalid integrity hash for <pkg>@<ver>. Per REVIEW.md ("When changing output/defaults/messages, grep the suite for assertions on the old behavior and update them in the same PR"), the fix is to update that test's mock manifests to omit dist.integrity (so the version falls into the compute-and-pin path) or supply real hashes.
Extended reasoning...
What breaks
test/cli/install/minimum-release-age.test.ts fails with exit code 1 on every CI platform. The robobun comment on this PR's HEAD (d6c4ed9) confirms it as the sole non-flaky failure across 🐧 13 x64/aarch64/asan, 🐧 25.04 x64/aarch64, 🐧 3.23 x64/aarch64, and 🪟 x64/aarch64 — nine platforms, same file, same code 1.
Mechanism
The mock registry in that test file serves manifests whose dist.integrity is a placeholder string, e.g. "sha512-fake1==" (line 106), "sha512-fake2==" (114), "sha512-bugfix1==" (167), "sha512-limit1==" (219) — roughly 48 such entries across the file. There is no dist.shasum on any of them.
Before this PR, Integrity::parse returned Tag::UNKNOWN for these strings and the manifest reader silently treated the version as having no integrity — fine for tests that only exercise version-selection logic and never fetch the tarball. After this PR, the new logic in src/install/npm.rs sets had_invalid_integrity = !sri_str.is_empty() when the parsed tag is unsupported, finds no shasum to fall back to, and stamps the version with Tag::INVALID. Package::from_npm (src/install/lockfile/Package.rs:847-858) then emits error: Registry provided an invalid integrity hash for <pkg>@<ver>, which fails resolve → the install exits non-zero → every test in the file fails.
Step-by-step proof
Take "sha512-fake1==" from line 106:
Integrity::parse_entrycallsTag::parse, which returns(Tag::SHA512, 7)—"sha512-"is a recognized prefix.- The remaining input after offset 7 is
"fake1=="; trailing=padding is stripped, leaving"fake1"(5 chars). base64.decoder.calc_size_for_slice("fake1")fails: 5 unpadded base64 chars is length ≡ 1 mod 4, which is invalid.parse_entryreturnsTag::UNKNOWN.- Back in npm.rs:
is_supported()is false,sri_stris non-empty →had_invalid_integrity = true. - There is no
"shasum"field in the mock manifest, so the fallback branch is skipped. had_invalid_integrityis true →package_version.integrity.tag = Tag::INVALID.Package::from_npmseesTag::INVALIDand callslog.add_error_fmt("Registry provided an invalid integrity hash for test-pkg@1.0.0").- The install fails; the test's
expect(exitCode).toBe(0)(or equivalent) fails.
Even placeholders whose length happens to be valid mod-4 (e.g. "bugfix1", 7 chars → decodes to 5 bytes) still fail: 5 bytes ≠ the expected 64-byte SHA-512 digest length, so decoded_size > expected_len is false but the decoded size doesn't match — actually, re-reading parse_entry, it only checks decoded_size > expected_len, so a 5-byte decode into a 64-byte buffer would succeed with Tag::SHA512. However, the majority of the placeholders (fake1–fake4, limit1–limit9, beta1, beta2, etc.) have length ≡ 1 mod 4 after stripping ==, which is definitively rejected by calc_size_for_slice. Since each mock package advertises multiple such versions and the test resolves against them, at least one INVALID version per package is enough to fail the install — and the CI result confirms it does.
Why nothing else catches this
The PR's own test file (bun-install-tarball-integrity.test.ts) uses a separate in-process registry with real or intentionally-invalid integrity values, so it never touches minimum-release-age.test.ts's mock. The PR description's verification section says "Neighboring lockfile-version-2.test.ts, bun-install.test.ts, and bun-install-retry.test.ts are unchanged" — but doesn't mention grepping for other tests that serve fake dist.integrity. REVIEW.md is explicit here: "When changing output/defaults/messages, grep the suite for assertions on the old behavior and update them in the same PR."
Fix
Update test/cli/install/minimum-release-age.test.ts's mock registry to either:
- Omit
dist.integrityentirely from each version'sdistobject. The version then falls into the new compute-and-pin path (Tag::UNKNOWN, notTag::INVALID), and since these tests never actually download the tarball, nothing else changes. This is the minimal diff. - Or supply a real SHA-512 of the served tarball bytes for each version.
The first option is a mechanical ~48-line deletion and keeps the test focused on what it actually exercises (version selection by release age), so it's the recommended fix.
| /// The registry manifest carried a non-empty `dist.integrity` that did not | ||
| /// parse as a supported SRI value (unknown algorithm, bad base64, or wrong | ||
| /// digest length). Distinguished from `UNKNOWN` so the resolve step can | ||
| /// refuse the version instead of treating the hash as merely absent. | ||
| /// Never written to disk: not `is_supported()`, so `Display` emits nothing | ||
| /// and `verify()` rejects it. | ||
| pub const INVALID: Tag = Tag(5); |
There was a problem hiding this comment.
🟡 The "Never written to disk" claim here (and in the PR description's "runtime sentinel only … so it never reaches disk") is not quite true: npm.rs:2589-2594 sets Tag::INVALID on PackageVersion.integrity, and PackageVersion is byte-serialized to the on-disk manifest cache via Serializer::write_array(writer, &this.package_versions, …) (npm.rs:1038). It correctly never reaches a lockfile (from_npm replaces it before append; Display emits nothing), but discriminant 5 does land in ~/.bun/install/cache/*.npm. Worth rewording so a future maintainer doesn't treat Tag(5) as freely reusable — e.g. "Never reaches a lockfile; may be persisted in the manifest cache."
Extended reasoning...
What the comment claims vs. what happens
The new doc comment on Tag::INVALID (integrity.rs:294-296) states:
Never written to disk: not
is_supported(), soDisplayemits nothing andverify()rejects it.
The PR description repeats this: "Tag::INVALID is a runtime sentinel only … so it never reaches disk."
The justification given — Display emits nothing, verify() rejects — only covers the text-lockfile serialization path (bun.lock writes format!("{}", integrity)) and the verify path. It does not cover the raw-byte manifest-cache serialization.
Step-by-step trace to disk
PackageManifest::parsehits a version whosedist.integrityis non-empty but unparseable and whoseshasumis absent/invalid. Atnpm.rs:2589-2594it sets:package_version.integrity = Integrity { tag: Tag::INVALID, ..Default::default() };
PackageVersionis#[repr(C)]with anIntegrityfield that isbytemuck::NoUninit(integrity.rs:23), and the file explicitly documents atnpm.rs:685-686that "Serializer::write_arrayreinterprets the whole slice as&[u8]".- After parse,
save_async→Serializer::save→Self::write_array(writer, &this.package_versions, &mut pos)atnpm.rs:1038writes thePackageVersionarray — including theTag(5)byte — to~/.bun/install/cache/<pkg>.npm.
So Tag(5) reaches disk in the manifest cache.
What is true
The tag correctly never reaches a lockfile: Package::from_npm (Package.rs:847-861) checks == Tag::INVALID, logs the error, and stores Integrity::default() on the lockfile package instead. And on the text-lockfile side, Display for Integrity emits "" for any unsupported tag. So the security/verification story is intact.
Why it's worth fixing (and why it's only a nit)
There is no functional bug today: a Tag(5) in a cached .npm file round-trips fine — new bun re-reads it as INVALID and errors in from_npm; older bun reads Tag(5) as an unrecognized tag (the newtype accepts any u8), is_supported() is false, and it's treated as absent.
But per REVIEW.md, "Comments carry only durable non-obvious content: invariants" — and this comment states an invariant that is false. The concrete risk: a future maintainer reading "Never written to disk" might treat discriminant 5 as freely reusable (e.g. for a new supported algorithm like SHA3). Cached manifests written by this version would then be misread as that new algorithm with an all-zero digest, and verify() would reject every tarball for those versions until the manifest cache expires. A false invariant comment is worse than no comment.
Suggested fix
Reword to scope the claim to what the justification actually proves, e.g.:
/// Never reaches a lockfile: `from_npm` replaces it with `default()` before
/// append, and `Display` emits nothing. May be persisted in the raw-byte
/// manifest cache; do not reuse discriminant 5.
(and drop the "so it never reaches disk" from the PR description if convenient). Nit only — not blocking.
2 participants
Problem
bun installintegrity is fail-open when the registry'sdist.integrityis missing or unparseable, and when a lockfile entry carries an empty or malformed hash. Four faces, all verified against 1.4.0-canary.1 (5b98630) and release-asan main (e532ad9):dist.integrity(and no usableshasum): install succeeds silently,bun.lockrecords""for that package, no warning.dist.integrityanddist.shasumentirely:bun.lockrecords"". No hash of the downloaded bytes is computed or pinned (npm's lockfile pins a computed sha512 here).""is never verified: wipe the cache, let the registry serve different bytes for the same version, and they are linked cleanly with exit 0.md5-…) or garbage integrity in the lockfile produceswarn: Unsupported or malformed integrity hash; ignoringand proceeds unverified with exit 0.A real sha1/sha256/sha384/sha512 in
bun.lockis enforced correctly (swapped tarball:error: Integrity check failed, exit 1); the hole is only the empty/unparseable path.Cause
Integrity::parse/Integrity::parse_entryreturnTag::UNKNOWNfor every failure (short input, unknown algorithm viaTag::parse, bad base64, wrong length, decode error) and the npm manifest reader maps a garbagedist.integrityand a garbage/absentdist.shasumtoIntegrity::default()with no log line. The lockfile writer printsDisplay for Integrity, which emits nothing forUNKNOWN, so""is persisted. The compute-and-pin machinery that already exists for GitHub/remote/local tarballs (compute_if_missing) excludesResolutionTag::Npm, so the extractor never hashes the npm bytes. At verify time,!tag.is_supported()short-circuits to success, and the lockfile reader onlyadd_warnings on a malformed value.Fix
Fail closed on each face:
src/install/npm.rs,src/install/integrity.rs,src/install/lockfile/Package.rs): a registry manifest whosedist.integrity(anddist.shasum) is present but unparseable now tags the versionTag::INVALIDinstead ofUNKNOWN.Package::from_npmrefuses that version witherror: Registry provided an invalid integrity hash for <pkg>@<ver>. A validshasumstill salvages an unparseableintegrity.src/install/TarballStream.rs,src/install/extract_tarball.rs,src/install/PackageManager/runTasks.rs): when neither field is usable, both extract paths compute SHA-512 over the downloaded bytes forResolutionTag::Npm(as they already did for GitHub/remote/local tarballs).runTaskswrites the computed hash back tolockfile.packages[id].meta.integrityand setsFORCE_SAVE_LOCKFILE, so the first install's lockfile already carries the pin and every subsequent install verifies against it. Skipped under--no-verify.src/install/lockfile/bun.lock.rs): atlockfileVersion: 2, an empty npm integrity string is refused witherror: Package <x> has no integrity pin in the frozen lockfileunder--frozen-lockfile, and warned about otherwise (the download path then back-fills the SHA-512 via the same write-back and re-saves). v0/v1 lockfiles, written before the back-fill existed, keep loading silently.src/install/lockfile/bun.lock.rs): atlockfileVersion: 2, a non-empty npm integrity string that doesn't parse as a supported SRI value is a parse error (Unsupported or malformed integrity hash for npm package) instead of the warn-and-ignore. v0/v1 keep the old warn.Tag::INVALIDis a runtime sentinel only:is_supported()is false for it,Displayemits nothing, andverify()rejects it, so it never reaches disk.Verification
test/cli/install/bun-install-tarball-integrity.test.tsadds adescribe("npm registry without usable integrity metadata")block with an in-process loopback registry:Tag::INVALID(md5-…,sha512-!!!,not-an-sri-string!!!, non-hexshasum) each fail with the new install error;integritywith a validshasumfalls through to SHA-1;""for an npm package is refused under--frozen-lockfileand warned + back-filled otherwise;md5-…for an npm package is refused; a v1 lockfile with the same entry still warns and proceeds;With
src/reverted toorigin/main, 9 of the 11 new tests fail; with this change the full file (27 tests) passes.rust:check-allis clean on all ten targets. Neighboringlockfile-version-2.test.ts,bun-install.test.ts, andbun-install-retry.test.tsare unchanged.Fixes #19519