install: warn when a registry manifest advertises an integrity hash bun cannot use - #38484
install: warn when a registry manifest advertises an integrity hash bun cannot use#38484robobun wants to merge 3 commits into
Conversation
A dist.shasum that failed to parse was replaced with an empty Integrity, so the version installed with no verification and nothing said so. Log a warning naming the package version instead, and drop the corresponding mordant baseline entry.
|
Updated 7:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 0f77818 has some failures in 🧪 To try this PR locally: bunx bun-pr 38484That installs a local version of the PR into your bun-38484 --bun |
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: ready for review; CI is blocked only by macOS queue capacity. Reproduced with the new cases in Build 96000 on the current head: 177 of 179 jobs passed with no test failures (the annotated ones are retried flakes on unrelated lanes). The remaining 2 jobs are both |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the inline nit is a pre-existing sibling gap, not a regression, and doesn't block.
Extended reasoning...
Overview
Replaces Integrity::parse_sha_sum(shasum_str).unwrap_or_default() in PackageManifest::parse (src/install/npm.rs:2609) with an explicit match that emits a warning to the manifest task's Log on Err. Removes the corresponding defaulted_failure entry from mordant-baseline.toml. Adds two tests to the existing tarball integrity metadata forms describe block and generalizes its serveManifest helper from integrity: string to dist: { integrity?, shasum? } (all three existing callers updated consistently).
Security risks
None introduced. The install-time behavior is unchanged: on parse failure the integrity remains Tag::UNKNOWN (skip verification) exactly as before — the only new effect is a stderr warning. On the Ok path the assignment is byte-identical. I checked the one non-obvious case: when dist.integrity was present-but-unsupported and dist.shasum is malformed, the old code overwrote package_version.integrity with Integrity::default() while the new code leaves the value assigned at line 2603 — but Integrity::parse already returns { tag: UNKNOWN } on every failure path, so the retained value is the same UNKNOWN default and there is no observable difference in verification or lockfile output.
Level of scrutiny
Low-to-medium. ~12 lines of Rust in a diagnostic-only path, following the exact log.add_warning_fmt(None, Loc::EMPTY, format_args!(...)) pattern already used throughout src/install/ (NetworkTask.rs, OverrideMap.rs, etc.). expected_name (fn parameter, &[u8]) and version_name (prop.key.slice()) are both in scope; bstr::BStr::new matches how the same file formats them at lines 2042/2085. The mordant baseline removal is correct — this was the only defaulted_failure finding in the file.
Other factors
Tests follow the file's conventions (local Bun.serve on port 0, await using, concurrent pipe drain, per-test cache dir). The malformed-shasum test asserts the exact warning string, exit 0, and that no sha*- pin lands in bun.lock; the mismatch test asserts the warning does NOT fire on a well-formed shasum. The PR description confirms both fail/pass appropriately under USE_SYSTEM_BUN=1 vs the debug build.
The inline nit (malformed dist.integrity with no usable shasum still installs silently) is a real adjacent gap per REVIEW.md's fix-the-whole-class guidance, but it's pre-existing, requires a different fix shape (Integrity::parse encodes failure in the return value rather than the type and can't distinguish malformed from unsupported-algorithm), and the PR already scopes the broader fail-closed question to #31327. Strict improvement over main; not blocking.
…ages it Integrity::parse reports failure through Tag::UNKNOWN rather than an error, so a manifest whose integrity field is malformed or uses an algorithm we do not support went just as quiet as the malformed shasum case. Emit one warning per version whenever the registry advertised a hash and none of it was usable; a manifest that advertises nothing still installs silently as before.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
There was a problem hiding this comment.
LGTM — the follow-up in 7302b4b covers the sibling dist.integrity case I flagged, and 0f77818 drops the redundant comment.
Checked the 'integrity: block against each new test case and its negatives: unusable integrity salvaged by a valid shasum breaks out before the warning, an empty shasum hits Ok(UNKNOWN) and still warns via the non-empty integrity_str arm, and no-hash-at-all stays silent. On the Err(_) arm package_version.integrity is left at its default (Tag::UNKNOWN), so the stored value is byte-identical to before. Also confirmed the concurrent it.each reusing the integrity-unusable prefix is fine — tempDir uses mkdtempSync so each case gets a unique dir.
Extended reasoning...
Overview
Replaces Integrity::parse_sha_sum(shasum_str).unwrap_or_default() in PackageManifest::parse (src/install/npm.rs:2602-2636) with an explicit try-integrity-then-shasum block that emits one warning per version when the registry advertised a hash bun cannot use and neither field yielded a supported tag. Removes the corresponding defaulted_failure entry from mordant-baseline.toml. Adds six subprocess tests to the existing tarball integrity metadata forms describe block in test/cli/install/bun-install-tarball-integrity.test.ts (three that expect the warning, three guards that pin no-warning behavior for shasum-mismatch-still-fails, shasum-salvages-bad-integrity, and no-hash-advertised).
Security risks
None introduced. This is integrity-verification-adjacent, but the change is strictly additive visibility: the stored Integrity value on every path is byte-identical to before (verified against Integrity::default() → Tag::UNKNOWN), so the binary manifest cache, lockfile output, and verification behavior are unchanged. The only new effect is a stderr warning for a case that was previously silent. No check is loosened.
Level of scrutiny
Low-medium. ~30 lines of Rust confined to one labeled block inside a per-version parsing loop, plus test additions. The public npm registry always serves sha512- SRI or 40-hex shasum, so the new path fires only against private/mirror registries with bad metadata. The one in-repo test that serves placeholder hashes (minimum-release-age.test.ts) is called out in the description as still passing. Traced each of the six new test cases through the block by hand and each hits the intended arm.
Other factors
My prior review pointed out the unhandled sibling (non-empty malformed dist.integrity with no/empty shasum); 7302b4b restructured the block to cover both fields under one condition (!integrity_str.is_empty() || malformed_shasum), and the new it.each covers exactly that case ({ integrity: "sha512-!!!", shasum: "" }). The comment-cop bot's feedback was addressed in 0f77818. Both threads are marked resolved. Tests follow harness conventions: describe.concurrent, Bun.serve({port: 0}), tempDir with using, Promise.all draining stdout/stderr/exited, Buffer.alloc(40, "x") over .repeat, exit-code asserted last. Integrity is Copy so reading integrity.tag after the assignment on line 2617-2618 is fine.
2 participants
Problem
defaulted_failurelint in the mordant bump (Bump mordant to 3bcf116 #38460):src/install/npm.rsbuilt a version's integrity withIntegrity::parse_sha_sum(shasum_str).unwrap_or_default().parse_sha_sumrejects adist.shasumthat is not hex or has an odd length (src/install/integrity.rs:51). Theunwrap_or_default()turned that rejection into anIntegritywithTag::UNKNOWN, which every verification path reads as "nothing to verify" (Streaming::verifyreturns true for an unsupported tag).dist.integrityfield six lines above had the same outcome by a different route:Integrity::parsereports a malformed value or an unsupported algorithm asTag::UNKNOWNrather than an error, so with noshasumto fall back on it went equally quiet.""intobun.lock, with nothing on stderr. A well-formed hash that does not match the bytes already fails the install, so the unusable case was the only one that went quiet.Fix
PackageManifest::parsenow triesdist.integrity, thendist.shasum(matching on theResult), and breaks out as soon as one yields a supported hash. If it reaches the end having been given a non-emptyintegrityor a malformedshasum, it adds one warning for that version to the manifest task's log, whichrun_tasksprints when the task resolves:warn: Unsupported or malformed integrity hash in registry metadata for pkg@1.0.0; its tarball will not be verified. A manifest that advertises no hash at all stays silent, as before.UNKNOWNvalue), so the binary manifest cache and lockfile output are byte-identical to before; the only new behavior is the warning.bun.lock.rsalready treats an unparseable integrity string in the lockfile. Propagating the error would fail the whole manifest and make every version of the package uninstallable because one version's metadata is bad. The warning is per version, at manifest parse time, so it names the offending versions even when a different version ends up installed. The public registry always serves sha512 SRI or 40 hex chars, so it never fires there; in this repo onlyminimum-release-age.test.tsserves placeholder values (sha512-fake1==), and it still passes (49 tests) since it does not assert on the absence of warnings.defaulted_failure:src/install/npm.rsentry frommordant-baseline.toml; it was the only finding of that lint in the file.test/cli/install/bun-install-tarball-integrity.test.ts("tarball integrity metadata forms"). Three new cases (malformed shasum, unsupported integrity algorithm, malformed integrity plus empty shasum) fail on the released build, where stderr is just the progress lines, and pass with this change. Three guards pin the surrounding behavior: a mismatching well-formed shasum still fails the install with no warning, an unusable integrity backed by a valid shasum installs with asha1-pin and no warning, and a manifest with no hash installs silently. The whole file (22 tests),minimum-release-age.test.ts, andbun-install-streaming-extract.test.tspass on the debug build.Background
PackageManifest::parse(src/install/npm.rs) turns a registry packument into bun's binary manifest. For each version it readsdist.integrity(an SRI string such assha512-...) and, when that is absent or unsupported, falls back todist.shasum, the legacy 40 character hex sha1 that older packages carry instead.Integrityis a tag plus digest bytes;Tag::UNKNOWNmeans "no hash available". Every verification path skips versions tagged UNKNOWN, which is why a silent parse failure is equivalent to switching verification off for that version.Log;run_tasksprints that log on the main thread when the task completes, so warnings added during parsing show up in the normalbun installoutput and respect the usual log level flags.mordant-baseline.tomlis the ratchet for the mordant lint pack: it records how many findings of each lint per file are accepted as pre-existing, and CI fails a PR only when a file exceeds its count. Removing an entry means CI now requires the file to have zero findings of that lint.Earlier shape of this PR
The first revision only matched on
parse_sha_sumand warned about a malformedshasum("Malformed shasum in registry metadata for ..."). Review pointed out that an unusabledist.integritywith noshasumreached the same silent outcome, so the warning now covers both fields with one message per version.[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file