-
Notifications
You must be signed in to change notification settings - Fork 5k
install: fail closed on unusable npm integrity (pin computed sha512; reject invalid manifest/lockfile hashes) #31327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
20008c3
10785f3
ab26d89
2e5eab5
d6c4ed9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,6 +110,9 @@ | |
| /// lockfile keeps loading: | ||
| /// - an npm package resolved to a tarball URL outside the configured | ||
| /// registry must carry a supported integrity hash | ||
| /// - an npm package's integrity string, when non-empty, must parse as a | ||
| /// supported SRI hash (unparseable/unknown-algorithm entries are | ||
| /// rejected rather than warned-and-ignored) | ||
| /// - a git `.bun-tag` must be a safe path/checkout component (the same | ||
| /// check on a `github` tag is enforced at every version, since its | ||
| /// download path has no checkout-time re-validation) | ||
|
|
@@ -2622,11 +2625,19 @@ | |
|
|
||
| pkg.meta.integrity = Integrity::parse(integrity_str); | ||
| if !integrity_str.is_empty() && !pkg.meta.integrity.tag.is_supported() { | ||
| // Surface — don't fail — for npm parity (`npm install` | ||
| // proceeds on a malformed lockfile integrity, treating | ||
| // it as absent). The download path still applies any | ||
| // registry-supplied integrity, so this only loses the | ||
| // *lockfile* pin. | ||
| // A non-empty value that doesn't parse as a supported | ||
| // SRI hash is refused so a tampered or unknown-algorithm | ||
| // entry cannot silently disable verification. Gated to | ||
| // v2+ so lockfiles written before this check keep | ||
| // loading with the prior warn-and-ignore behaviour. | ||
| if lockfile_version.at_least(Version::V2) { | ||
| log.add_error( | ||
| Some(source), | ||
| item_loc(source, key_loc, i), | ||
| b"Unsupported or malformed integrity hash for npm package", | ||
| ); | ||
| return Err(ParseError::InvalidPackageInfo); | ||
| } | ||
| log.add_warning( | ||
| Some(source), | ||
| item_loc(source, key_loc, i), | ||
|
|
@@ -2653,6 +2664,39 @@ | |
| ); | ||
| return Err(ParseError::InvalidPackageInfo); | ||
| } | ||
|
|
||
| // An empty integrity string means this entry vouches for | ||
| // nothing. The download path computes and back-fills a | ||
| // 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), | ||
| ), | ||
| ); | ||
| } | ||
|
Check failure on line 2699 in src/install/lockfile/bun.lock.rs
|
||
|
Comment on lines
+2670
to
+2699
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 The comment says this check is "Gated to v2+ so lockfiles written before the back-fill existed… keep loading silently", but Extended reasoning...What the bug isThe new empty-integrity check at
That premise is false. Step-by-step proof
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 thisContrast with the pre-existing off-registry-URL check at line 2656 ( 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. ImpactUsers with a private registry that omits integrity metadata — exactly the population #19519 targets — have their FixTwo acceptable resolutions:
Either way, the current state — code whose behavior contradicts its own documented rationale — should be resolved before merge. |
||
| } | ||
| ResolutionTag::LocalTarball | ResolutionTag::RemoteTarball => { | ||
| // integrity is optional for tarball deps (backward compat) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2568,17 +2568,30 @@ | |
| package_version.unpacked_size = n.value() as u32; | ||
| } | ||
|
|
||
| if let Some(shasum_str) = dist.get(b"integrity").and_then(|v| v.as_str()) { | ||
| package_version.integrity = Integrity::parse(shasum_str); | ||
| 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() | ||
| }; | ||
| } | ||
|
Check failure on line 2594 in src/install/npm.rs
|
||
|
Comment on lines
+2571
to
2594
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 The new Extended reasoning...What breaks
MechanismThe mock registry in that test file serves manifests whose Before this PR, Step-by-step proofTake
Even placeholders whose length happens to be valid mod-4 (e.g. Why nothing else catches thisThe PR's own test file ( FixUpdate
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. |
||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 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-2594setsTag::INVALIDonPackageVersion.integrity, andPackageVersionis byte-serialized to the on-disk manifest cache viaSerializer::write_array(writer, &this.package_versions, …)(npm.rs:1038). It correctly never reaches a lockfile (from_npmreplaces it before append;Displayemits nothing), but discriminant 5 does land in~/.bun/install/cache/*.npm. Worth rewording so a future maintainer doesn't treatTag(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:The PR description repeats this: "
Tag::INVALIDis a runtime sentinel only … so it never reaches disk."The justification given —
Displayemits nothing,verify()rejects — only covers the text-lockfile serialization path (bun.lockwritesformat!("{}", 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: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]".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 storesIntegrity::default()on the lockfile package instead. And on the text-lockfile side,Display for Integrityemits""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.npmfile round-trips fine — new bun re-reads it asINVALIDand errors infrom_npm; older bun readsTag(5)as an unrecognized tag (the newtype accepts anyu8),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
5as 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, andverify()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.:
(and drop the "so it never reaches disk" from the PR description if convenient). Nit only — not blocking.