Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 86 additions & 4 deletions src/install/lockfile/bun.lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,18 @@

/// fixed unnecessary listing of workspace dependencies
V1 = 1,

/// Stricter parsing that rejects, rather than accepts, lockfiles the
/// earlier versions tolerated. Gated here so an already-written v0/v1
/// lockfile keeps loading:
/// - an npm package resolved to a tarball URL outside the configured
/// registry must carry a supported integrity hash
/// - a git/github `.bun-tag` must be a safe path/checkout component
V2 = 2,
}

impl Version {
pub const CURRENT: Version = Version::V1;
pub const CURRENT: Version = Version::V2;
Comment thread
claude[bot] marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.

#[inline]
pub const fn current() -> Version {
Expand All @@ -122,9 +130,17 @@
match n {
0 => Some(Version::V0),
1 => Some(Version::V1),
2 => Some(Version::V2),
_ => None,
}
}

/// `true` when this lockfile version is at least `other`. Used to gate
/// strict parse-time checks introduced in a later version.
#[inline]
pub const fn at_least(self, other: Version) -> bool {
(self as u32) >= (other as u32)
}
}

/// For sorting dependencies belonging to a node_modules folder. No duplicate names, so
Expand Down Expand Up @@ -165,6 +181,53 @@
Self::save_from_binary_inner(lockfile, load_result, options, writer)
}

/// Pick the `lockfileVersion` to stamp. `Version::CURRENT` (v2) adds
/// parse-time checks that reject entries older versions tolerated: an
/// off-registry npm tarball without a supported integrity hash, and an
/// unsafe git `.bun-tag`. The writer emits those fields verbatim (no
/// backfill), so stamping v2 on a lockfile that still carries such an entry
/// would make the *next* parse reject it. Only stamp v2 when every package
/// already satisfies the v2 invariants; otherwise stay at v1 so the file
/// round-trips (load → save → load) cleanly.
fn version_to_write(
pkg_resolutions: &[Resolution],
pkg_metas: &[Meta],
pkg_names: &[String],
buf: &[u8],
options: &PackageManagerOptions,
) -> Version {
for (i, res) in pkg_resolutions.iter().enumerate() {
match res.tag {
ResolutionTag::Npm => {
if pkg_metas[i].integrity.tag.is_supported() {
continue;
}
// No supported integrity: only v2-clean if the tarball URL
// is under the configured/default registry (mirrors the
// parser's `npm_url_needs_integrity` computation).
let url = res.npm().url.slice(buf);
let configured_registry = options
.scope_for_package_name(pkg_names[i].slice(buf))
.url
.href();
let under_registry = url_is_under_registry(url, configured_registry)
|| url_is_under_registry(url, Npm::Registry::DEFAULT_URL.as_bytes());
if !under_registry {
return Version::V1;
}
}
ResolutionTag::Git | ResolutionTag::Github => {
if !crate::repository::is_safe_resolved_tag(res.repository().resolved.slice(buf))
{
return Version::V1;
}

Check warning on line 223 in src/install/lockfile/bun.lock.rs

View check run for this annotation

Claude / Claude Code Review

Stale github references after 58bc805a split the .bun-tag gate

Commit 58bc805a made the parser reject unsafe **github** `.bun-tag` values unconditionally at every version (line 2670: `enforce_safe_tag = tag == ResolutionTag::Github || …at_least(V2)`), but two references still treat github as v2-gated: this `version_to_write` arm returns `V1` for an unsafe github tag even though stamping V1 won't make it round-trip (the parser rejects it at v1 too, so the Github half of this arm is dead/ineffective and contradicts the doc comment's "stay at v1 so the file ro
Comment thread
robobun marked this conversation as resolved.
Outdated
}
_ => {}
}
}
Version::CURRENT
}

pub(crate) fn save_from_binary_inner(
lockfile: &mut BinaryLockfile,
load_result: &LoadResult,
Expand Down Expand Up @@ -244,7 +307,9 @@
writer.write_all(b"{\n")?;
Self::inc_indent(writer, indent)?;
{
writeln!(writer, "\"lockfileVersion\": {},", Version::CURRENT as u32)?;
let lockfile_version =
Self::version_to_write(pkg_resolutions, pkg_metas, pkg_names, buf, options);
writeln!(writer, "\"lockfileVersion\": {},", lockfile_version as u32)?;
Self::write_indent(writer, *indent)?;

let config_version: ConfigVersion =
Expand Down Expand Up @@ -2546,7 +2611,14 @@
// Fail closed: otherwise a tampered lockfile could redirect
// the tarball URL off-registry and install arbitrary content
// under a trusted package name with verification disabled.
if npm_url_needs_integrity && !pkg.meta.integrity.tag.is_supported() {
//
// Only enforced for v2+. Older lockfiles predate this check
// and may legitimately omit integrity for an off-registry
// tarball; rejecting them would break existing installs.
if lockfile_version.at_least(Version::V2)
&& npm_url_needs_integrity
&& !pkg.meta.integrity.tag.is_supported()
{
log.add_error(
Some(source),
integrity_expr.loc,
Expand Down Expand Up @@ -2587,7 +2659,17 @@
return Err(ParseError::InvalidPackageInfo);
};

if !crate::repository::is_safe_resolved_tag(bun_tag_str) {
// Reject an unsafe `.bun-tag`. For `git`, `Repository::checkout`
// re-validates with the same guard before building any cache
// path or invoking `git`, so this parse-time check is gated to
// v2+ — older git lockfiles keep loading without reopening the
// checkout hole. For `github` there is no such re-validation
// (the tarball-download path feeds the tag straight into the
// cache folder name), so the check must stay unconditional to
// keep the path-traversal guard intact at every version.
let enforce_safe_tag =
tag == ResolutionTag::Github || lockfile_version.at_least(Version::V2);
if enforce_safe_tag && !crate::repository::is_safe_resolved_tag(bun_tag_str) {
log.add_error(Some(source), bun_tag.loc, b"Invalid git dependency tag");
return Err(ParseError::InvalidPackageInfo);
}
Expand Down
14 changes: 7 additions & 7 deletions test/cli/install/__snapshots__/bun-install-registry.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ exports[`auto-install symlinks (and junctions) are created correctly in the inst

exports[`text lockfile workspace sorting 1`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand Down Expand Up @@ -45,7 +45,7 @@ exports[`text lockfile workspace sorting 1`] = `

exports[`text lockfile workspace sorting 2`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand Down Expand Up @@ -88,7 +88,7 @@ exports[`text lockfile workspace sorting 2`] = `

exports[`text lockfile --frozen-lockfile 1`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand Down Expand Up @@ -120,7 +120,7 @@ exports[`text lockfile --frozen-lockfile 1`] = `

exports[`binaries each type of binary serializes correctly to text lockfile 1`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand Down Expand Up @@ -148,7 +148,7 @@ exports[`binaries each type of binary serializes correctly to text lockfile 1`]

exports[`binaries root resolution bins 1`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 0,
"workspaces": {
"": {
Expand All @@ -170,7 +170,7 @@ exports[`binaries root resolution bins 1`] = `

exports[`hoisting text lockfile is hoisted 1`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand Down Expand Up @@ -280,7 +280,7 @@ exports[`outdated NO_COLOR works 1`] = `

exports[`it should ignore peerDependencies within workspaces 1`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand Down
20 changes: 10 additions & 10 deletions test/cli/install/__snapshots__/bun-lock.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

exports[`should write plaintext lockfiles 1`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand All @@ -21,7 +21,7 @@ exports[`should write plaintext lockfiles 1`] = `

exports[`should escape names 1`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand All @@ -48,7 +48,7 @@ exports[`should escape names 1`] = `

exports[`should be the default save format 1`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand All @@ -67,7 +67,7 @@ exports[`should be the default save format 1`] = `

exports[`should be the default save format 2`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand All @@ -89,7 +89,7 @@ exports[`should be the default save format 2`] = `

exports[`should save the lockfile if --save-text-lockfile and --frozen-lockfile are used 1`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand All @@ -108,7 +108,7 @@ exports[`should save the lockfile if --save-text-lockfile and --frozen-lockfile

exports[`should save the lockfile if --save-text-lockfile and --frozen-lockfile are used 2`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand All @@ -130,7 +130,7 @@ exports[`should save the lockfile if --save-text-lockfile and --frozen-lockfile

exports[`should convert a binary lockfile with invalid optional peers 1`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 0,
"workspaces": {
"": {
Expand Down Expand Up @@ -300,7 +300,7 @@ exports[`should not deduplicate bundled packages with un-bundled packages 2`] =

exports[`should not change formatting unexpectedly 2`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand Down Expand Up @@ -400,7 +400,7 @@ exports[`should not change formatting unexpectedly 2`] = `

exports[`should sort overrides before comparing 1`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand Down Expand Up @@ -438,7 +438,7 @@ exports[`should sort overrides before comparing 1`] = `

exports[`should include unused resolutions in the lockfile 1`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand Down
6 changes: 3 additions & 3 deletions test/cli/install/__snapshots__/catalogs.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

exports[`basic detect changes (bun.lock) 1`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand Down Expand Up @@ -37,7 +37,7 @@ exports[`basic detect changes (bun.lock) 1`] = `

exports[`basic detect changes (bun.lock) 2`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand Down Expand Up @@ -72,7 +72,7 @@ exports[`basic detect changes (bun.lock) 2`] = `

exports[`basic detect changes (bun.lock) 3`] = `
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand Down
2 changes: 1 addition & 1 deletion test/cli/install/bun-install.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9013,7 +9013,7 @@ describe.concurrent("bun-install", () => {
expect(await exists(join(ctx.package_dir, "bun.lockb"))).toBeFalse();
expect(await file(join(ctx.package_dir, "bun.lock")).text()).toMatchInlineSnapshot(`
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand Down
22 changes: 12 additions & 10 deletions test/cli/install/bun-lock.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -616,12 +616,11 @@ it("should include unused resolutions in the lockfile", async () => {
await runBunInstall(env, packageDir, { frozenLockfile: true });
});

it("requires an integrity hash when an npm package entry points at a tarball URL outside the configured registry", async () => {
it("requires an integrity hash for an off-registry npm tarball URL at lockfileVersion 2", async () => {
const { packageDir, packageJson } = await registry.createTestDir();

// Stand-in for a host that is not the configured registry. A correct install
// must never contact it for this lockfile, because the entry carries no
// integrity hash that would pin the tarball contents.
// Stand-in for a host that is not the configured registry. Parsing fails
// before any fetch, so this is never actually contacted.
let offRegistryRequests = 0;
await using offRegistry = Bun.serve({
port: 0,
Expand All @@ -644,7 +643,7 @@ it("requires an integrity hash when an npm package entry points at a tarball URL

const lockfileWithUrl = (tarballUrl: string) =>
JSON.stringify({
lockfileVersion: 1,
lockfileVersion: 2,
configVersion: 1,
workspaces: {
"": {
Expand All @@ -659,8 +658,10 @@ it("requires an integrity hash when an npm package entry points at a tarball URL
},
});

// The entry keeps the well-known name and version but points the tarball at
// a different host and provides no integrity hash.
// The entry keeps the well-known name and version but points the tarball at a
// different host and provides no integrity hash. At lockfileVersion 2 this
// fails closed: parsing rejects it before any fetch. (The v1 backward-compat
// case — parsing accepts such an entry — is covered in lockfile-version-2.test.ts.)
await write(
join(packageDir, "bun.lock"),
lockfileWithUrl(`http://127.0.0.1:${offRegistry.port}/no-deps/-/no-deps-1.0.0.tgz`),
Expand All @@ -682,9 +683,10 @@ it("requires an integrity hash when an npm package entry points at a tarball URL
expect(await exists(join(packageDir, "node_modules", "no-deps"))).toBe(false);
expect(await exited).not.toBe(0);

// The same entry with the tarball URL under the configured registry and no
// integrity hash is still accepted (backward compat for lockfiles that omit
// the hash for registry-hosted tarballs).
// The same entry with the tarball URL *under* the configured registry and no
// integrity hash is accepted even at v2 (the off-registry gate does not apply,
// so `npm_url_needs_integrity` is false — registry-hosted tarballs may still
// omit the hash).
await write(join(packageDir, "bun.lock"), lockfileWithUrl(`${registry.registryUrl()}no-deps/-/no-deps-1.0.0.tgz`));

({ exited, stdout, stderr } = spawn({
Expand Down
2 changes: 1 addition & 1 deletion test/cli/install/bun-workspaces.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1981,7 +1981,7 @@ test("matching workspace devDependency and npm peerDependency", async () => {
expect((await file(join(packageDir, "bun.lock")).text()).replaceAll(/localhost:\d+/g, "localhost:1234"))
.toMatchInlineSnapshot(`
"{
"lockfileVersion": 1,
"lockfileVersion": 2,
"configVersion": 1,
"workspaces": {
"": {
Expand Down
Loading
Loading