From 6b629ae967def162a5cdebb236bd24fb2e145dd0 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 17 Sep 2026 17:22:15 +0000 Subject: [PATCH 1/4] install: keep bundleDependencies from every dependency group when parsing a registry manifest `PackageManifest::parse` filtered the `bundleDependencies` array against each dependency group while it walked them, and each group assigned the version's list to only the names it had matched. A version with `dependencies` and `optionalDependencies` kept the bundled names of the last group only, so a bundled name in `dependencies` was installed from the registry next to the copy the tarball ships. A bundled name in `peerDependencies` was never bundled. The parser now stores the array as published, once per version. `Package::from_npm` is the only reader and already matches the list against the dependencies of every group by name hash. The contents of `bundled_deps_buf` change, so the manifest cache header moves to v0.0.8. This is the parser change of #38857, rebased. --- src/install/npm.rs | 127 ++++++----------- test/cli/install/bun-install-registry.test.ts | 133 ++++++++++++++++++ .../bundled-peer/bundled-peer-1.0.0.tgz | Bin 0 -> 283 bytes .../packages/bundled-peer/package.json | 44 ++++++ .../bundled-with-optional-1.0.0.tgz | Bin 0 -> 364 bytes .../bundled-with-optional/package.json | 50 +++++++ 6 files changed, 273 insertions(+), 81 deletions(-) create mode 100644 test/cli/install/registry/packages/bundled-peer/bundled-peer-1.0.0.tgz create mode 100644 test/cli/install/registry/packages/bundled-peer/package.json create mode 100644 test/cli/install/registry/packages/bundled-with-optional/bundled-with-optional-1.0.0.tgz create mode 100644 test/cli/install/registry/packages/bundled-with-optional/package.json diff --git a/src/install/npm.rs b/src/install/npm.rs index 0addc627fc1f..2a27333782a5 100644 --- a/src/install/npm.rs +++ b/src/install/npm.rs @@ -5,7 +5,7 @@ use crate::Error; use crate::bun_json as JSON; use crate::bun_schema::api; use bun_alloc::AllocError; -use bun_collections::{HashMap, IdentityContext, StringSet, index_sort}; +use bun_collections::{HashMap, IdentityContext, index_sort}; use bun_core::{Global, Output, fmt as bun_fmt}; use bun_core::{MutableString, strings}; use bun_dotenv::Loader as DotEnv; @@ -933,8 +933,9 @@ pub mod package_manifest { // - v0.0.5: added bundled dependencies // - v0.0.6: changed semver major/minor/patch to each use u64 instead of u32 // - v0.0.7: added version publish times and extended manifest flag for minimum release age + // - v0.0.8: fixed bundled dependencies being dropped from all but one dependency group const HEADER_BYTES: &'static str = - concat!("#!/usr/bin/env bun\n", "bun-npm-manifest-cache-v0.0.7\n"); + concat!("#!/usr/bin/env bun\n", "bun-npm-manifest-cache-v0.0.8\n"); // Field order is hardcoded (descending alignment). Re-verify if the // layout changes. @@ -1972,35 +1973,6 @@ impl PackageManifest { } } -/// Fills `set` with the names listed in `bundle(d)Dependencies`; returns whether it was `true` (bundle everything). -fn collect_bundled_deps( - version_obj: Option<&JSON::E::ObjectJSON>, - set: &mut StringSet, -) -> Result { - set.map.clear_retaining_capacity(); - let mut bundle_all_deps = false; - if let Some(bundled_deps_value) = version_obj - .and_then(|o| o.get(b"bundleDependencies")) - .or_else(|| version_obj.and_then(|o| o.get(b"bundledDependencies"))) - { - match bundled_deps_value { - JSON::E::JsonValue::Boolean(boolean) => { - bundle_all_deps = *boolean; - } - JSON::E::JsonValue::Array(arr) => { - for bundled_dep in arr.get().items() { - let Some(s) = bundled_dep.as_str() else { - continue; - }; - set.insert(s)?; - } - } - _ => {} - } - } - Ok(bundle_all_deps) -} - // Keys are pre-hashed string hashes, so don't re-hash them. type ExternalStringMapDeduper = HashMap>; @@ -2012,6 +1984,16 @@ const DEPENDENCY_GROUPS: [DependencyGroup; 3] = [ DependencyGroup::PEER, ]; +/// npm accepts both spellings; `bundleDependencies` is the documented one. +fn bundled_dependencies_field( + version_obj: Option<&JSON::E::ObjectJSON>, +) -> Option<&JSON::E::JsonValue> { + let version_obj = version_obj?; + version_obj + .get(b"bundleDependencies") + .or_else(|| version_obj.get(b"bundledDependencies")) +} + impl PackageManifest { /// This parses [Abbreviated metadata](https://github.com/npm/registry/blob/master/docs/responses/package-metadata.md#abbreviated-metadata-format) pub fn parse( @@ -2065,9 +2047,6 @@ impl PackageManifest { let mut version_extern_strings_dedupe_map = ExternalStringMapDeduper::default(); let mut optional_peer_dep_names: Vec = Vec::new(); - let mut bundled_deps_set = StringSet::init(); - let mut bundle_all_deps: bool; - let mut bundled_deps_count: usize = 0; let mut string_builder = Semver::semver_string::Builder { @@ -2200,7 +2179,16 @@ impl PackageManifest { } } - bundle_all_deps = collect_bundled_deps(version_obj, &mut bundled_deps_set)?; + if let Some(JSON::E::JsonValue::Array(arr)) = + bundled_dependencies_field(version_obj) + { + bundled_deps_count += arr + .get() + .items() + .iter() + .filter(|bundled_dep| bundled_dep.as_str().is_some()) + .count(); + } for pair in &DEPENDENCY_GROUPS { if let Some(obj) = version_obj @@ -2211,11 +2199,6 @@ impl PackageManifest { dependency_sum += properties.len(); for property in properties { let key = property.key.slice(); - if !bundle_all_deps && bundled_deps_set.swap_remove(key) { - // swap remove the dependency name because it could exist in - // multiple behavior groups. - bundled_deps_count += 1; - } string_builder.count(key); string_builder.count(property.value.as_str().unwrap_or(b"")); } @@ -2404,10 +2387,30 @@ impl PackageManifest { let version_obj = prop.value.as_object(); - bundle_all_deps = collect_bundled_deps(version_obj, &mut bundled_deps_set)?; - let mut package_version: PackageVersion = empty_version; + // `Package::from_npm` matches these against the dependencies of every group. + package_version.bundled_dependencies = match bundled_dependencies_field(version_obj) + { + Some(JSON::E::JsonValue::Boolean(true)) => ExternalPackageNameHashList::INVALID, + Some(JSON::E::JsonValue::Array(arr)) => { + let bundled_deps_begin = bundled_deps_offset; + for bundled_dep in arr.get().items() { + let Some(name) = bundled_dep.as_str() else { + continue; + }; + bundled_deps_buf[bundled_deps_offset] = + Semver::semver_string::Builder::string_hash(name); + bundled_deps_offset += 1; + } + ExternalPackageNameHashList::init( + &bundled_deps_buf, + &bundled_deps_buf[bundled_deps_begin..bundled_deps_offset], + ) + } + _ => ExternalPackageNameHashList::default(), + }; + if let Some(cpu) = version_obj.and_then(|o| o.get(b"cpu")) { package_version.cpu = negatable_from_json_value::(cpu); } @@ -2629,13 +2632,7 @@ impl PackageManifest { // For peer deps, fall through with an empty `items` // slice when `peerDependencies` is absent so that // `peerDependenciesMeta`-only entries (synthesised - // below) still get a build pass. The fallthrough must - // stay scoped to packages that actually have a - // `peerDependenciesMeta`: the body sets - // `package_version.bundled_dependencies` from this - // iteration's slice of `bundled_deps_buf`, so an - // unconditional empty pass would clobber the value the - // `dependencies` iteration just produced. + // below) still get a build pass. let items: &[JSON::E::PropertyJSON] = version_obj .and_then(|o| o.get(pair.prop)) .and_then(|deps| deps.as_object()) @@ -2684,8 +2681,6 @@ impl PackageManifest { } } - let bundled_deps_begin = bundled_deps_offset; - let mut i: usize = 0; for item in items { @@ -2706,15 +2701,6 @@ impl PackageManifest { version_extern_strings[values_base + i] = string_builder.append::(version_str); - if !bundle_all_deps && bundled_deps_set.swap_remove(name_str) { - // SAFETY: bundled_deps_buf sized in counting pass - unsafe { - *bundled_deps_buf.as_mut_ptr().add(bundled_deps_offset) = - all_extern_strings[names_base + i].hash; - } - bundled_deps_offset += 1; - } - if is_peer { if optional_peer_dep_names .iter() @@ -2814,27 +2800,6 @@ impl PackageManifest { let this_versions = &version_extern_strings[values_base..values_base + count]; - // Bundled deps are matched against the - // `dependencies`/`optionalDependencies` groups - // only; the peer pass never adds to - // `bundled_deps_buf`. With the meta-only - // synthesis above the peer body now runs even - // when `peerDependencies` is absent, so writing - // here would clobber the value the dependencies - // pass already produced with an empty slice. - if !is_peer { - if bundle_all_deps { - package_version.bundled_dependencies = - ExternalPackageNameHashList::INVALID; - } else { - package_version.bundled_dependencies = - ExternalPackageNameHashList::init( - &bundled_deps_buf, - &bundled_deps_buf[bundled_deps_begin..bundled_deps_offset], - ); - } - } - let mut name_list = ExternalStringList::init(&all_extern_strings, this_names); let mut version_list = diff --git a/test/cli/install/bun-install-registry.test.ts b/test/cli/install/bun-install-registry.test.ts index 08bdf5c9bd11..82b5a3263107 100644 --- a/test/cli/install/bun-install-registry.test.ts +++ b/test/cli/install/bun-install-registry.test.ts @@ -1376,6 +1376,139 @@ describe("bundledDependencies", () => { await check(); }); + test(`(${textLockfile ? "bun.lock" : "bun.lockb"}) bundled names from dependencies and optionalDependencies`, async () => { + // bundled-with-optional@1.0.0 has dependencies { no-deps }, optionalDependencies { a-dep, basic-1 } + // and bundleDependencies [no-deps, a-dep, not-a-dependency]. Its tarball ships no-deps and a-dep. + // Only basic-1 should be installed from the registry; not-a-dependency is declared in no group + // (and does not exist in the registry), so listing it must have no effect. + await write( + packageJson, + JSON.stringify({ + name: "bundled-mixed-groups", + dependencies: { + "bundled-with-optional": "1.0.0", + }, + }), + ); + + await runBunInstall(env, packageDir, { saveTextLockfile: textLockfile }); + + async function check() { + expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual(["basic-1", "bundled-with-optional"]); + const bundledNodeModules = join(packageDir, "node_modules", "bundled-with-optional", "node_modules"); + expect( + await Promise.all([ + file(join(bundledNodeModules, "no-deps", "package.json")).json(), + file(join(bundledNodeModules, "a-dep", "package.json")).json(), + ]), + ).toEqual([ + { name: "no-deps", version: "1.0.0" }, + { name: "a-dep", version: "1.0.1" }, + ]); + } + + await check(); + + let lockfile: string | undefined; + if (textLockfile) { + lockfile = await file(join(packageDir, "bun.lock")).text(); + expect(lockfile.replaceAll(/localhost:\d+/g, "localhost:1234")).toMatchInlineSnapshot(` + "{ + "lockfileVersion": 2, + "configVersion": 1, + "workspaces": { + "": { + "name": "bundled-mixed-groups", + "dependencies": { + "bundled-with-optional": "1.0.0", + }, + }, + }, + "packages": { + "basic-1": ["basic-1@1.0.0", "http://localhost:1234/basic-1/-/basic-1-1.0.0.tgz", {}, "sha512-NW5qBU1Kn7DzCjfVfnAbBBRGuQ7krbBtrnezZwOXutA9NvrCT4SI4EJMog3AGsNeK/1OygErysF8RN/FqDYunA=="], + + "bundled-with-optional": ["bundled-with-optional@1.0.0", "http://localhost:1234/bundled-with-optional/-/bundled-with-optional-1.0.0.tgz", { "dependencies": { "no-deps": "1.0.0" }, "optionalDependencies": { "a-dep": "1.0.1", "basic-1": "1.0.0" } }, "sha512-rq4Jtdsk53QE4T00/KaOP5lyv3d8Gnt4VABtara3PFos+8POKMkRIQPYCXVRRBey7K+ttzkiA5MkHhe+T19eYA=="], + + "bundled-with-optional/a-dep": ["a-dep@1.0.1", "http://localhost:1234/a-dep/-/a-dep-1.0.1.tgz", { "bundled": true }, "sha512-6nmTaPgO2U/uOODqOhbjbnaB4xHuZ+UB7AjKUA3g2dT4WRWeNxgp0dC8Db4swXSnO5/uLLUdFmUJKINNBO/3wg=="], + + "bundled-with-optional/no-deps": ["no-deps@1.0.0", "http://localhost:1234/no-deps/-/no-deps-1.0.0.tgz", { "bundled": true }, "sha512-v4w12JRjUGvfHDUP8vFDwu0gUWu04j0cv9hLb1Abf9VdaXu4XcrddYFTMVBVvmldKViGWH7jrb6xPJRF0wq6gw=="], + } + } + " + `); + } + + // The lockfile must record the bundled edges too: a cold install from it + // must not pull no-deps or a-dep from the registry either. + await rm(join(packageDir, "node_modules"), { recursive: true, force: true }); + await runBunInstall(env, packageDir, { frozenLockfile: true }); + + await check(); + if (textLockfile) { + expect(await file(join(packageDir, "bun.lock")).text()).toBe(lockfile!); + } + }); + + test(`(${textLockfile ? "bun.lock" : "bun.lockb"}) bundled name declared in peerDependencies`, async () => { + // bundled-peer@1.0.0 has peerDependencies { no-deps } and bundleDependencies [no-deps], and its + // tarball ships no-deps. The peer is satisfied by the bundled copy, so nothing else is installed. + await write( + packageJson, + JSON.stringify({ + name: "bundled-peer-root", + dependencies: { + "bundled-peer": "1.0.0", + }, + }), + ); + + await runBunInstall(env, packageDir, { saveTextLockfile: textLockfile }); + + async function check() { + expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual(["bundled-peer"]); + expect( + await file( + join(packageDir, "node_modules", "bundled-peer", "node_modules", "no-deps", "package.json"), + ).json(), + ).toEqual({ name: "no-deps", version: "1.0.0" }); + } + + await check(); + + let lockfile: string | undefined; + if (textLockfile) { + lockfile = await file(join(packageDir, "bun.lock")).text(); + expect(lockfile.replaceAll(/localhost:\d+/g, "localhost:1234")).toMatchInlineSnapshot(` + "{ + "lockfileVersion": 2, + "configVersion": 1, + "workspaces": { + "": { + "name": "bundled-peer-root", + "dependencies": { + "bundled-peer": "1.0.0", + }, + }, + }, + "packages": { + "bundled-peer": ["bundled-peer@1.0.0", "http://localhost:1234/bundled-peer/-/bundled-peer-1.0.0.tgz", { "peerDependencies": { "no-deps": "1.0.0" } }, "sha512-rmaahOTDrcPuhmBskBLcKEEfTotY+xH6bDPnQopcNlmdePtEvAbAehSpK8JQAegpBir4omjiFqBy1LFqXQBfgA=="], + + "bundled-peer/no-deps": ["no-deps@1.0.0", "http://localhost:1234/no-deps/-/no-deps-1.0.0.tgz", { "bundled": true }, "sha512-v4w12JRjUGvfHDUP8vFDwu0gUWu04j0cv9hLb1Abf9VdaXu4XcrddYFTMVBVvmldKViGWH7jrb6xPJRF0wq6gw=="], + } + } + " + `); + } + + await rm(join(packageDir, "node_modules"), { recursive: true, force: true }); + await runBunInstall(env, packageDir, { frozenLockfile: true }); + + await check(); + if (textLockfile) { + expect(await file(join(packageDir, "bun.lock")).text()).toBe(lockfile!); + } + }); + test(`(${textLockfile ? "bun.lock" : "bun.lockb"}) git dependencies`, async () => { await Promise.all([ write( diff --git a/test/cli/install/registry/packages/bundled-peer/bundled-peer-1.0.0.tgz b/test/cli/install/registry/packages/bundled-peer/bundled-peer-1.0.0.tgz new file mode 100644 index 0000000000000000000000000000000000000000..40e294dbcb5899a9129d2e975f94030c47e0ef5e GIT binary patch literal 283 zcmV+$0p$K4iwFP!000021MSp7Yr-%T2k@NxDMHV)CT&u|^S;Fx(wWC_(?L-}BA#vw5c}yCV_Jv`%X@T+_Q$S}7$&nd-Bu5{zs10A2WM zTF%)&R0uD zS*k(*%UUY*|HJ$Lt+QrLX5xw3GeaO%`~7F~WDVKnibCA(@8QP@avU0*Z_}Cm-Gfsv hr>ElAE2azp00000000000002M)pn8QOIrXa005UIlnwv@ literal 0 HcmV?d00001 diff --git a/test/cli/install/registry/packages/bundled-peer/package.json b/test/cli/install/registry/packages/bundled-peer/package.json new file mode 100644 index 000000000000..670e3938bcff --- /dev/null +++ b/test/cli/install/registry/packages/bundled-peer/package.json @@ -0,0 +1,44 @@ +{ + "name": "bundled-peer", + "versions": { + "1.0.0": { + "name": "bundled-peer", + "version": "1.0.0", + "peerDependencies": { + "no-deps": "1.0.0" + }, + "bundleDependencies": [ + "no-deps" + ], + "_id": "bundled-peer@1.0.0", + "integrity": "sha512-rmaahOTDrcPuhmBskBLcKEEfTotY+xH6bDPnQopcNlmdePtEvAbAehSpK8JQAegpBir4omjiFqBy1LFqXQBfgA==", + "shasum": "32b15c8ab065db1e6b9383043f0dd002077cb65d", + "dist": { + "integrity": "sha512-rmaahOTDrcPuhmBskBLcKEEfTotY+xH6bDPnQopcNlmdePtEvAbAehSpK8JQAegpBir4omjiFqBy1LFqXQBfgA==", + "shasum": "32b15c8ab065db1e6b9383043f0dd002077cb65d", + "tarball": "http://localhost:4873/bundled-peer/-/bundled-peer-1.0.0.tgz" + }, + "contributors": [] + } + }, + "time": { + "modified": "2024-12-30T18:14:22.143Z", + "created": "2024-12-30T18:14:22.143Z", + "1.0.0": "2024-12-30T18:14:22.143Z" + }, + "users": {}, + "dist-tags": { + "latest": "1.0.0" + }, + "_uplinks": {}, + "_distfiles": {}, + "_attachments": { + "bundled-peer-1.0.0.tgz": { + "shasum": "32b15c8ab065db1e6b9383043f0dd002077cb65d", + "version": "1.0.0" + } + }, + "_rev": "", + "_id": "bundled-peer", + "readme": "" +} diff --git a/test/cli/install/registry/packages/bundled-with-optional/bundled-with-optional-1.0.0.tgz b/test/cli/install/registry/packages/bundled-with-optional/bundled-with-optional-1.0.0.tgz new file mode 100644 index 0000000000000000000000000000000000000000..940fd676286ea3f223aef91cafce7d2f22824810 GIT binary patch literal 364 zcmV-y0h9h8iwFP!000021MQa4PJ=KMhPmb`8m?h$p@n4EzJ)QSgGNzS$N-Hodv{CO zoRTd@Wh2@C?}CzZ=t+~G^Y>ITeU=Yu?wk;6NQAJfVYu3N#s%k`($L0}D54G#&KYRy zMy?5Qu9Rx4zb-R%zbdn~Q1yIj{h4_R=s%hMGRRalpHlxMiJa0*%@R~P|L0L7X{bg~)4Q$l7h0>YQX|8IM>WY!QjqII+kd6I*SM%oJzJC1t zoH+eHx|DUE2JAq!{ Date: Thu, 17 Sep 2026 17:22:32 +0000 Subject: [PATCH 2/4] install: resolve a bundled file: dependency whose path leaves the package A registry, git or tarball package can list a `file:` dependency in `bundleDependencies`. `npm publish` from a monorepo makes this shape: `"inner": "file:../inner"` plus the copy in the tarball's node_modules. The resolver refused the edge because the path leaves the package, and the whole install failed with `Could not find package.json for "file:../inner" dependency "inner"`. A bundled edge is never installed. Both linkers skip it, and the copy in the parent's tarball is the dependency. The resolver now records the path for a bundled edge without the escape check, as 1.3.14 did. The check still applies to every edge that is not bundled. A recorded path must stay unread, because another dependency of the same name (an optional peer of a nested package) can resolve to the recorded package. Both installers refuse such a folder. `bun patch` and `bun patch --commit` read a folder package from its path too, and now refuse it with the same rule (`Lockfile::is_trusted_folder_package`). --- .../PackageManager/PackageManagerEnqueue.rs | 8 +- src/install/PackageManager/patchPackage.rs | 28 ++- test/cli/install/bun-install-registry.test.ts | 183 ++++++++++++++++++ test/cli/install/bun-patch.test.ts | 62 ++++++ .../bundled-file/bundled-file-3.0.0.tgz | Bin 0 -> 253 bytes .../bundled-file/bundled-file-4.0.0.tgz | Bin 0 -> 346 bytes .../bundled-file/bundled-file-5.0.0.tgz | Bin 0 -> 365 bytes .../packages/bundled-file/package.json | 50 ++++- .../packages/create-bundled-file-packages.ts | 55 ++++-- 9 files changed, 367 insertions(+), 19 deletions(-) create mode 100644 test/cli/install/registry/packages/bundled-file/bundled-file-3.0.0.tgz create mode 100644 test/cli/install/registry/packages/bundled-file/bundled-file-4.0.0.tgz create mode 100644 test/cli/install/registry/packages/bundled-file/bundled-file-5.0.0.tgz diff --git a/src/install/PackageManager/PackageManagerEnqueue.rs b/src/install/PackageManager/PackageManagerEnqueue.rs index a7c46c834667..9a860b708bf2 100644 --- a/src/install/PackageManager/PackageManagerEnqueue.rs +++ b/src/install/PackageManager/PackageManagerEnqueue.rs @@ -2908,8 +2908,12 @@ fn get_or_put_resolved_package( ); } - // transitive folder dependencies do not have their dependencies resolved - if crate::bin::bin_target_escapes_package_dir(this.lockfile.str(&folder)) + // transitive folder dependencies do not have their dependencies resolved. + // A bundled one is never installed (the parent's tarball ships the copy), so + // its path is only recorded. Every reader of a folder path refuses it + // (`is_trusted_folder_package`), because another dependency can bind to it. + if !behavior.is_bundled() + && crate::bin::bin_target_escapes_package_dir(this.lockfile.str(&folder)) && !this.lockfile.is_trusted_folder_dependency(dependency_id) { break 'res FolderResolutionValue::Err(crate::Error::MissingPackageJSON); diff --git a/src/install/PackageManager/patchPackage.rs b/src/install/PackageManager/patchPackage.rs index 22f89dfd3146..627ed98671b7 100644 --- a/src/install/PackageManager/patchPackage.rs +++ b/src/install/PackageManager/patchPackage.rs @@ -22,8 +22,8 @@ use crate::package_manager_real::package_manager_directories::{ compute_cache_dir_and_subpath, get_temporary_directory, }; use crate::{ - BuntagHashBuf, DependencyID, Features, PackageID, Resolution, buntaghashbuf_make, - initialize_store, invalid_package_id, + BuntagHashBuf, DependencyID, Features, PackageID, Resolution, ResolutionTag, + buntaghashbuf_make, initialize_store, invalid_package_id, }; #[inline] @@ -46,6 +46,26 @@ fn print_resolution_label<'a>( label } +/// `bun patch` copies a `file:` package out of its folder, and `--commit` diffs +/// against it, so both refuse the folders the installers refuse. +fn refuse_unsafe_folder_package(lockfile: &Lockfile, pkg: &Package) { + if pkg.resolution.tag != ResolutionTag::Folder { + return; + } + let folder = lockfile.str(pkg.resolution.folder()); + if !crate::bin::bin_target_escapes_package_dir(folder) + || lockfile.is_trusted_folder_package(pkg.meta.id) + { + return; + } + bun_core::pretty_errorln!( + "error: refusing to patch {} with unsafe folder path \"{}\"", + bstr::BStr::new(lockfile.str(&pkg.name)), + bstr::BStr::new(folder), + ); + Global::crash(); +} + #[derive(Default)] pub struct PatchCommitResult { pub(crate) patch_key: Box<[u8]>, @@ -269,6 +289,7 @@ pub fn do_patch_commit( // `compute_cache_dir_and_subpath` resolves `pkg.resolution`'s strings against `manager.lockfile`. manager.lockfile = lockfile; + refuse_unsafe_folder_package(&manager.lockfile, &pkg); let name = manager.lockfile.str(&pkg.name).to_vec(); let cache_result = compute_cache_dir_and_subpath(manager, &name, &pkg.resolution, &mut folder_path_buf, None); @@ -833,6 +854,8 @@ pub fn prepare_patch(manager: &mut PackageManager) -> Result<(), crate::Error> { } }; + refuse_unsafe_folder_package(lockfile, &actual_package); + let name = lockfile.str(&package.name).to_vec(); let existing_patchfile_hash: Option = 'existing_patchfile_hash: { let mut name_and_version = Vec::new(); @@ -889,6 +912,7 @@ pub fn prepare_patch(manager: &mut PackageManager) -> Result<(), crate::Error> { let strbuf = manager.lockfile.buffers.string_bytes.as_slice(); let pkg = *manager.lockfile.packages.get(pkg_id as usize); + refuse_unsafe_folder_package(&manager.lockfile, &pkg); let pkg_name = pkg.name.slice(strbuf).to_vec(); let existing_patchfile_hash: Option = 'existing_patchfile_hash: { diff --git a/test/cli/install/bun-install-registry.test.ts b/test/cli/install/bun-install-registry.test.ts index 82b5a3263107..dd90f51b4e91 100644 --- a/test/cli/install/bun-install-registry.test.ts +++ b/test/cli/install/bun-install-registry.test.ts @@ -1718,6 +1718,189 @@ describe("bundledDependencies", () => { expect(stdout).toBe("bundled-file-dep\n"); expect(exitCode).toBe(0); }); + + // bundled-file@4.0.0 has the shape of a package that `npm publish` makes from a + // monorepo: the `file:` path of its bundled dependency leaves the package, and + // the tarball ships only the bundled copy in its node_modules. The copy is the + // dependency. The install records the path in the lockfile and never follows it. + // bundled-file@5.0.0 also has `optionalDependencies` ("no-deps"). The bundled + // name is in `dependencies`, and the later group must not drop its bundled flag. + describe.each(["hoisted", "isolated"] as const)( + "(%s) bundled file: dependency with a path that leaves the package", + linker => { + test.each([ + ["4.0.0", "1 package installed"], + ["5.0.0", "2 packages installed"], + ])("bundled-file@%s", async (version, installed) => { + await Promise.all([ + registry.writeBunfig(packageDir, { saveTextLockfile: false, linker }), + write( + packageJson, + JSON.stringify({ + name: "bundled-file-outside-root", + dependencies: { + "bundled-file": version, + }, + }), + ), + ]); + + const bundledDepDir = join(packageDir, "node_modules", "bundled-file", "node_modules", "bundled-file-dep"); + + async function check() { + const [pkgJsonStat, indexStat] = await Promise.all([ + lstat(join(bundledDepDir, "package.json")), + lstat(join(bundledDepDir, "index.js")), + ]); + expect([pkgJsonStat.isFile(), indexStat.isFile()]).toEqual([true, true]); + // `../bundled-file-dep`, seen from the installed package + expect(await exists(join(packageDir, "node_modules", "bundled-file-dep"))).toBeFalse(); + + await using proc = spawn({ + cmd: [bunExe(), "-e", `console.log(require("bundled-file"))`], + cwd: packageDir, + stdout: "pipe", + stderr: "pipe", + env, + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout).toBe("bundled-file-dep\n"); + expect(exitCode).toBe(0); + } + + let { out } = await runBunInstall(env, packageDir, { saveTextLockfile: true }); + expect(out).toContain(installed); + await check(); + + const lockfile = await file(join(packageDir, "bun.lock")).text(); + expect(lockfile).toContain( + `"bundled-file/bundled-file-dep": ["bundled-file-dep@file:../bundled-file-dep", { "bundled": true }],`, + ); + + await rm(join(packageDir, "node_modules"), { recursive: true, force: true }); + + ({ out } = await runBunInstall(env, packageDir, { frozenLockfile: true })); + expect(out).toContain(installed); + await check(); + expect(await file(join(packageDir, "bun.lock")).text()).toBe(lockfile); + }); + }, + ); + + // bundled-file@3.0.0 declares the same path and does not bundle the + // dependency. A registry package must not link a directory outside itself. + test("file: dependency with a path that leaves the package is refused when it is not bundled", async () => { + await write( + packageJson, + JSON.stringify({ + name: "unbundled-file-outside-root", + dependencies: { + "bundled-file": "3.0.0", + }, + }), + ); + + await using proc = spawn({ + cmd: [bunExe(), "install", "--save-text-lockfile"], + cwd: packageDir, + stdout: "pipe", + stderr: "pipe", + env, + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr.split(/\r?\n/).filter(line => line.startsWith("error:"))).toEqual([ + `error: Could not find package.json for "file:../bundled-file-dep" dependency "bundled-file-dep"`, + "error: bundled-file-dep@file:../bundled-file-dep failed to resolve", + ]); + expect(stdout).not.toContain("installed"); + expect(await exists(join(packageDir, "bun.lock"))).toBeFalse(); + expect(exitCode).toBe(1); + }); + + // The path recorded for a bundled dependency must stay unread when another + // dependency resolves to the same package. "mid" is nested next to the bundled + // "inner" and has an optional peer on it, so the hoister binds that peer to the + // recorded `file:../inner`, which names a directory outside the project. + for (const linker of ["hoisted", "isolated"] as const) { + test(`(${linker}) the path of a bundled file: dependency is not followed for another dependent`, async () => { + const packages = [ + { + json: { + name: "outer", + version: "1.0.0", + dependencies: { inner: "file:../inner", mid: "1.0.0" }, + bundleDependencies: ["inner"], + }, + files: { "package/node_modules/inner/package.json": JSON.stringify({ name: "inner", version: "1.0.0" }) }, + }, + { + json: { + name: "mid", + version: "1.0.0", + peerDependencies: { inner: "*" }, + peerDependenciesMeta: { inner: { optional: true } }, + }, + }, + // the root depends on this one, so mid@1.0.0 stays nested under "outer" + { json: { name: "mid", version: "2.0.0" } }, + ]; + const tarballs = new Map(); + for (const { json, files } of packages) { + tarballs.set( + `/${json.name}-${json.version}.tgz`, + await new Bun.Archive( + { "package/package.json": JSON.stringify(json), ...files }, + { compress: "gzip" }, + ).bytes(), + ); + } + using server = Bun.serve({ + port: 0, + fetch(request) { + const { origin, pathname } = new URL(request.url); + const tarball = tarballs.get(pathname); + if (tarball) return new Response(tarball); + const versions = packages.filter(({ json }) => `/${json.name}` === pathname); + if (versions.length === 0) return new Response("not found", { status: 404 }); + return Response.json({ + name: versions[0].json.name, + "dist-tags": { latest: versions.at(-1)!.json.version }, + versions: Object.fromEntries( + versions.map(({ json }) => [ + json.version, + { ...json, dist: { tarball: `${origin}/${json.name}-${json.version}.tgz` } }, + ]), + ), + }); + }, + }); + + using dir = tempDir("bundled-file-outside-peer", { + "inner/package.json": JSON.stringify({ name: "inner", version: "9.9.9" }), + "inner/outside.txt": "outside the project", + "project/package.json": JSON.stringify({ name: "app", dependencies: { outer: "1.0.0", mid: "2.0.0" } }), + "project/bunfig.toml": `[install]\nregistry = "${server.url.href}"\nlinker = "${linker}"\n`, + }); + const projectDir = join(String(dir), "project"); + + await using proc = spawn({ + cmd: [bunExe(), "install"], + cwd: projectDir, + stdout: "ignore", + stderr: "pipe", + env: { ...env, BUN_INSTALL_CACHE_DIR: join(String(dir), ".bun-cache") }, + }); + const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); + + expect(stderr).toContain(`error: refusing to install dependency inner with unsafe folder path "../inner"`); + expect([...new Bun.Glob("**/outside.txt").scanSync({ cwd: projectDir, dot: true, onlyFiles: false })]).toEqual( + [], + ); + expect(exitCode).toBe(1); + }); + } }); describe("optionalDependencies", () => { diff --git a/test/cli/install/bun-patch.test.ts b/test/cli/install/bun-patch.test.ts index 0e61d4526398..aabaf1ad0e51 100644 --- a/test/cli/install/bun-patch.test.ts +++ b/test/cli/install/bun-patch.test.ts @@ -177,6 +177,68 @@ describe("packages whose label is longer than 1024 bytes", () => { }); }); +// bundled-file@4.0.0 bundles a `file:../bundled-file-dep` dependency. The lockfile +// records that path and the install never reads it: the copy in the tarball is +// the dependency. `bun patch` takes a `file:` package from its folder, which for +// this path is a directory outside the project, so it has to refuse. +describe("a bundled file: dependency with a path that leaves the package", () => { + const registry = new VerdaccioRegistry(); + + beforeAll(async () => { + await registry.start(); + }); + + afterAll(() => { + registry.stop(); + }); + + const bundledCopy = "node_modules/bundled-file/node_modules/bundled-file-dep"; + + test.concurrent.each([ + ["bun patch ", ["bundled-file-dep"]], + ["bun patch ", [bundledCopy]], + ["bun patch --commit ", ["--commit", bundledCopy]], + ])("%s", async (_, args) => { + const packageJson = { name: "app", dependencies: { "bundled-file": "4.0.0" } }; + const { packageDir } = await registry.createTestDir({ + bunfigOpts: { linker: "hoisted" }, + files: { + "bundled-file-dep/package.json": JSON.stringify({ name: "bundled-file-dep", version: "9.9.9" }), + "bundled-file-dep/index.js": `module.exports = "outside the project";\n`, + "project/package.json": JSON.stringify(packageJson), + }, + }); + const projectDir = join(packageDir, "project"); + await registry.writeBunfig(projectDir, { linker: "hoisted" }); + + async function runBun(...cmd: string[]) { + await using proc = Bun.spawn({ + cmd: [bunExe(), ...cmd], + cwd: projectDir, + env: { ...bunEnv, BUN_INSTALL_CACHE_DIR: join(packageDir, ".bun-cache") }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; + } + + const install = await runBun("install"); + expect(install.stderr).not.toContain("error:"); + expect(install.exitCode).toBe(0); + + const patch = await runBun("patch", ...args); + expect(patch.stderr).toContain( + `error: refusing to patch bundled-file-dep with unsafe folder path "../bundled-file-dep"`, + ); + expect(patch.exitCode).toBe(1); + expect(await Bun.file(join(projectDir, bundledCopy, "index.js")).text()).toBe( + `module.exports = "bundled-file-dep";\n`, + ); + expect(await Bun.file(join(projectDir, "package.json")).json()).toEqual(packageJson); + }); +}); + describe("bun patch ", async () => { describe("workspace interactions", async () => { /** diff --git a/test/cli/install/registry/packages/bundled-file/bundled-file-3.0.0.tgz b/test/cli/install/registry/packages/bundled-file/bundled-file-3.0.0.tgz new file mode 100644 index 0000000000000000000000000000000000000000..97d3ad927f94b682e84c4bccfbe9fe5fdc155994 GIT binary patch literal 253 zcmb2|=3oGW|8FmE^gC=Iz?N{nvMX3^!ir+Q*)Cj<5}j%nOu1rM)~vN~pY^6wPF4mC zOQ-(Jw>-UkbLrvtyT5vW*J-s6+uOM~cz@5#*H^oOQlC|(n)&)J(^|bLZ--Ia4V~VV zA{(umHQ()8@Z)&>+3CB5-b)6)+qL$I?$iFso11SO4Yv2Fn{Jgalw`l_N=(+Znmt<| z9gQq}9hY=3sH%CNpZ=O3CR5I>y>`@7U)d^!o%R14aM_m23Q^zccy@d6i##cP)qH2pv7g`Uf+4;`kOhf$4621O(-|}v7yz!lci#X2 literal 0 HcmV?d00001 diff --git a/test/cli/install/registry/packages/bundled-file/bundled-file-4.0.0.tgz b/test/cli/install/registry/packages/bundled-file/bundled-file-4.0.0.tgz new file mode 100644 index 0000000000000000000000000000000000000000..513e146b464fcaecf96f91007e3f0e4ad799f9a7 GIT binary patch literal 346 zcmV-g0j2&QiwFP!00000|LxY%YJ)Ho24Js!ii1}htoFo43mv=LTZ}PWb2jYS=u91j z(RUx#l~JX1&`xmt-%TVT5y3y_6zTjeeU)+1jN!uN*1IrfPPC?aWajqB93`4m9K}&G z@*$2@{zsf75%S-MqxF9m`F0PWetLjan(e0cg(E;+9-`mn|Ge(t sWc}B1JO8XT^52I}{qJ4^DEj~a0000000000000B>1dY;id7<{id0N; zmd=zQ8Otd01bHK9>UCCmbo8_OA9o^NeIWvqtvHyA;Jz`sU_E)r3l1*a$O~6>ZjBpM zA%)cSYA)GmHjA9=U@qJm;_K{*=F&}a*Yhx(4#s>Tw(VNBT<6wg#r3yq8td+M9f540 z|1qyyu{Y~x&ztx{-1=J&2HtZ&&5hEKZQrR>%Jh|-xv{>QMl{KZZg5A8rCD`v3p{000000000004L-P L0WC5$04M+e3Bj_o literal 0 HcmV?d00001 diff --git a/test/cli/install/registry/packages/bundled-file/package.json b/test/cli/install/registry/packages/bundled-file/package.json index 3190852462f2..2935ae1b7902 100644 --- a/test/cli/install/registry/packages/bundled-file/package.json +++ b/test/cli/install/registry/packages/bundled-file/package.json @@ -2,7 +2,7 @@ "_id": "bundled-file", "name": "bundled-file", "dist-tags": { - "latest": "2.0.0" + "latest": "5.0.0" }, "versions": { "1.0.0": { @@ -33,6 +33,54 @@ "shasum": "cd4c6b205128b53321bad299e60d7c6afb1f6c0e", "tarball": "http://localhost:4873/bundled-file/-/bundled-file-2.0.0.tgz" } + }, + "3.0.0": { + "name": "bundled-file", + "version": "3.0.0", + "dependencies": { + "bundled-file-dep": "file:../bundled-file-dep" + }, + "_id": "bundled-file@3.0.0", + "dist": { + "integrity": "sha512-gQMoo88Glz7mlN93q5VrcTwW7lGobaWASIYXAq+JJ1mVDpVawkKKOkfAkv9CaLlsd/96HhuavU516+ldQvsEig==", + "shasum": "acec9a255f909d6c07f07d0654150ec87594a181", + "tarball": "http://localhost:4873/bundled-file/-/bundled-file-3.0.0.tgz" + } + }, + "4.0.0": { + "name": "bundled-file", + "version": "4.0.0", + "dependencies": { + "bundled-file-dep": "file:../bundled-file-dep" + }, + "bundleDependencies": [ + "bundled-file-dep" + ], + "_id": "bundled-file@4.0.0", + "dist": { + "integrity": "sha512-x18GKenWM1RFAs5LjQF6wyolc2UIuBjZHntGawm9XIaAjzVvKEVFmQZjZYb9QOrVLwGphMWVXVQKUPDnTzYubw==", + "shasum": "6a4539872405e60b6ca02a4f9ed7c69b9dae0a84", + "tarball": "http://localhost:4873/bundled-file/-/bundled-file-4.0.0.tgz" + } + }, + "5.0.0": { + "name": "bundled-file", + "version": "5.0.0", + "dependencies": { + "bundled-file-dep": "file:../bundled-file-dep" + }, + "optionalDependencies": { + "no-deps": "1.0.0" + }, + "bundleDependencies": [ + "bundled-file-dep" + ], + "_id": "bundled-file@5.0.0", + "dist": { + "integrity": "sha512-aOIi7rw4XaXuHYwHE0bvPGmC0lTpzdcSw5bS0BGxXVIDJkG3vW29IiWcTcQg539P7ZEQgZqqFpwC48ZDaHNiDA==", + "shasum": "29cb868fd9146c0c519ac07832aed444550d73a3", + "tarball": "http://localhost:4873/bundled-file/-/bundled-file-5.0.0.tgz" + } } } } \ No newline at end of file diff --git a/test/cli/install/registry/packages/create-bundled-file-packages.ts b/test/cli/install/registry/packages/create-bundled-file-packages.ts index 836d009ea924..83345a4b7379 100644 --- a/test/cli/install/registry/packages/create-bundled-file-packages.ts +++ b/test/cli/install/registry/packages/create-bundled-file-packages.ts @@ -2,10 +2,12 @@ /** * Generates the `bundled-file` fixture used by bun-install-registry.test.ts. * - * Both versions depend on `bundled-file-dep` through a `file:` spec - * (`file:vendor/bundled-file-dep`) and ship both `vendor/bundled-file-dep/` - * and a copy under `node_modules/bundled-file-dep/`, the way `npm pack` does - * for a bundled `file:` dependency. + * Every version depends on `bundled-file-dep` through a `file:` spec. + * + * 1.0.0 and 2.0.0 point the spec inside the package + * (`file:vendor/bundled-file-dep`) and ship both `vendor/bundled-file-dep/` and + * a copy under `node_modules/bundled-file-dep/`, the way `npm pack` does for a + * bundled `file:` dependency. * * - bundled-file@1.0.0 lists the dependency in `bundleDependencies`. A `file:` * resolution has its `bundled: true` marker at a different index in bun.lock @@ -13,6 +15,19 @@ * the dependency as bundled. * - bundled-file@2.0.0 does not bundle it. The install has to link the * `file:` dependency over the files the tarball already put in place. + * + * 3.0.0 and 4.0.0 point the spec outside the package + * (`file:../bundled-file-dep`), the way a package published from a monorepo + * does. The tarball cannot ship that directory. + * + * - bundled-file@3.0.0 does not bundle it. A registry package must not link a + * directory outside itself, so the install must refuse the path. + * - bundled-file@4.0.0 lists the dependency in `bundleDependencies` and ships + * the copy under `node_modules/bundled-file-dep/`. The copy is the + * dependency, so the install must not refuse the path. + * - bundled-file@5.0.0 is 4.0.0 plus an `optionalDependencies` group. The + * bundled name is in `dependencies`, and the manifest parser must keep it + * bundled when a later dependency group exists. */ import { mkdir, writeFile } from "fs/promises"; @@ -31,28 +46,40 @@ const depIndex = `module.exports = "${depName}";\n`; const versions: Record = {}; let latest = ""; -for (const [version, bundled] of [ - ["1.0.0", true], - ["2.0.0", false], -] as const) { +for (const { version, inside, bundled, optional } of [ + { version: "1.0.0", inside: true, bundled: true }, + { version: "2.0.0", inside: true, bundled: false }, + { version: "3.0.0", inside: false, bundled: false }, + { version: "4.0.0", inside: false, bundled: true }, + { version: "5.0.0", inside: false, bundled: true, optional: true }, +]) { const pkgJson = { name, version, - dependencies: { [depName]: `file:vendor/${depName}` }, + dependencies: { [depName]: inside ? `file:vendor/${depName}` : `file:../${depName}` }, + ...(optional ? { optionalDependencies: { "no-deps": "1.0.0" } } : {}), ...(bundled ? { bundleDependencies: [depName] } : {}), }; const files: Record = { "package/package.json": JSON.stringify(pkgJson, null, 2), "package/index.js": `module.exports = require("${depName}");\n`, - [`package/vendor/${depName}/package.json`]: depPkgJson, - [`package/vendor/${depName}/index.js`]: depIndex, - [`package/node_modules/${depName}/package.json`]: depPkgJson, - [`package/node_modules/${depName}/index.js`]: depIndex, }; + if (inside) { + files[`package/vendor/${depName}/package.json`] = depPkgJson; + files[`package/vendor/${depName}/index.js`] = depIndex; + } + if (inside || bundled) { + files[`package/node_modules/${depName}/package.json`] = depPkgJson; + files[`package/node_modules/${depName}/index.js`] = depIndex; + } + // Bun.Archive stamps the current time on every entry, so a second write + // changes the integrity. Keep a tarball that already exists. const tarball = join(dir, `${name}-${version}.tgz`); - await Bun.Archive.write(tarball, files, { compress: "gzip" }); + if (!(await Bun.file(tarball).exists())) { + await Bun.Archive.write(tarball, files, { compress: "gzip" }); + } const bytes = await Bun.file(tarball).bytes(); versions[version] = { From 621154cbab4cf4801623223a4870bc5722ffbf31 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 17 Sep 2026 18:04:46 +0000 Subject: [PATCH 3/4] ci: retrigger From 0b1d51f558df3513ad241fbe365213d21ecf79cc Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 17 Sep 2026 18:09:36 +0000 Subject: [PATCH 4/4] install: shorten two comments --- src/install/PackageManager/PackageManagerEnqueue.rs | 6 ++---- src/install/PackageManager/patchPackage.rs | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/install/PackageManager/PackageManagerEnqueue.rs b/src/install/PackageManager/PackageManagerEnqueue.rs index 9a860b708bf2..24ad0b4dc089 100644 --- a/src/install/PackageManager/PackageManagerEnqueue.rs +++ b/src/install/PackageManager/PackageManagerEnqueue.rs @@ -2908,10 +2908,8 @@ fn get_or_put_resolved_package( ); } - // transitive folder dependencies do not have their dependencies resolved. - // A bundled one is never installed (the parent's tarball ships the copy), so - // its path is only recorded. Every reader of a folder path refuses it - // (`is_trusted_folder_package`), because another dependency can bind to it. + // transitive folder dependencies do not have their dependencies resolved + // Nothing installs a bundled one, and every reader of the recorded path refuses it. if !behavior.is_bundled() && crate::bin::bin_target_escapes_package_dir(this.lockfile.str(&folder)) && !this.lockfile.is_trusted_folder_dependency(dependency_id) diff --git a/src/install/PackageManager/patchPackage.rs b/src/install/PackageManager/patchPackage.rs index 627ed98671b7..b7865c882226 100644 --- a/src/install/PackageManager/patchPackage.rs +++ b/src/install/PackageManager/patchPackage.rs @@ -46,8 +46,7 @@ fn print_resolution_label<'a>( label } -/// `bun patch` copies a `file:` package out of its folder, and `--commit` diffs -/// against it, so both refuse the folders the installers refuse. +/// `bun patch` reads a `file:` package from its folder, so it refuses what the installers refuse. fn refuse_unsafe_folder_package(lockfile: &Lockfile, pkg: &Package) { if pkg.resolution.tag != ResolutionTag::Folder { return;