diff --git a/src/install/PackageInstaller.rs b/src/install/PackageInstaller.rs index d80a7151486a..75472ae62335 100644 --- a/src/install/PackageInstaller.rs +++ b/src/install/PackageInstaller.rs @@ -1456,19 +1456,8 @@ impl<'a> PackageInstaller<'a> { } else { // transitive folder dependencies are not hoisted if folder.len() >= self.folder_path_buf.len() - || (bin::bin_target_escapes_package_dir(folder) && { - // overrides/resolutions are only ever parsed from the root - // package.json, so a folder path that reached here via an - // override was written by the user and is trusted the same - // as a direct dependency of the root. - let dep = &self.lockfile().buffers.dependencies.as_slice() - [dependency_id as usize]; - !self.lockfile().overrides.contains_name( - dep.name_hash, - dep.name.slice(string_buf!()), - string_buf!(), - ) - }) + || (bin::bin_target_escapes_package_dir(folder) + && !self.lockfile().is_trusted_folder_dependency(dependency_id)) { if log_level != Options::LogLevel::Silent { bun_core::pretty_errorln!( diff --git a/src/install/PackageManager/PackageManagerEnqueue.rs b/src/install/PackageManager/PackageManagerEnqueue.rs index adf08bded22a..bce4bec8097f 100644 --- a/src/install/PackageManager/PackageManagerEnqueue.rs +++ b/src/install/PackageManager/PackageManagerEnqueue.rs @@ -2849,19 +2849,10 @@ 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)) { - // overrides/resolutions are only ever parsed from the root - // package.json, so a folder path that reached here via an - // override was written by the user and is trusted the same - // as a direct dependency of the root. - let buf = this.lockfile.buffers.string_bytes.as_slice(); - if !this.lockfile.overrides.contains_name( - dependency.name_hash, - dependency.name.slice(buf), - buf, - ) { - break 'res FolderResolutionValue::Err(crate::Error::MissingPackageJSON); - } + if 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); } let mut package = Package::default(); diff --git a/src/install/PackageManager/PackageManagerResolution.rs b/src/install/PackageManager/PackageManagerResolution.rs index 2bfc156cd7ab..78ece7d050b1 100644 --- a/src/install/PackageManager/PackageManagerResolution.rs +++ b/src/install/PackageManager/PackageManagerResolution.rs @@ -326,11 +326,10 @@ impl PackageManager { continue; } - let features = match pkg_resolutions[parent_id].tag { - ResolutionTag::Root | ResolutionTag::Workspace | ResolutionTag::Folder => { - self.options.local_package_features - } - _ => self.options.remote_package_features, + let features = if pkg_resolutions[parent_id].tag.is_local_package() { + self.options.local_package_features + } else { + self.options.remote_package_features }; // even if optional dependencies are enabled, it's still allowed to fail if failed_dep.behavior.is_optional() || !failed_dep.behavior.is_enabled(features) { diff --git a/src/install/lockfile.rs b/src/install/lockfile.rs index 4c721ac55ae6..30b7e495c2cb 100644 --- a/src/install/lockfile.rs +++ b/src/install/lockfile.rs @@ -860,6 +860,50 @@ impl Lockfile { 0 } + /// Does the root or a workspace depend on package `id` directly? + pub(crate) fn is_workspace_declared_package(&self, id: PackageID) -> bool { + let packages = self.packages.slice(); + let resolutions = self.buffers.resolutions.as_slice(); + for (pkg_id, res_list) in packages.items_resolutions().iter().enumerate() { + let tag = packages.items_resolution()[pkg_id].tag; + if tag != ResolutionTag::Workspace && tag != ResolutionTag::Root { + continue; + } + if res_list.get(resolutions).contains(&id) { + return true; + } + } + false + } + + /// Is dependency `id` declared by the root, a workspace, or a `file:` package + /// one of them depends on directly? Checked, not assumed: a migrated lockfile + /// can carry dependencies for a folder that a registry package shipped. + pub(crate) fn is_dependency_of_local_package(&self, id: DependencyID) -> bool { + let Some(parent_id) = self.get_parent_pkg_of_dependency(id) else { + return false; + }; + match self.packages.items_resolution()[parent_id as usize].tag { + ResolutionTag::Root | ResolutionTag::Workspace => true, + ResolutionTag::Folder => self.is_workspace_declared_package(parent_id), + _ => false, + } + } + + /// May the folder path of dependency `id` leave its package directory? Yes + /// when a local package declares the dependency, or when a plain override or + /// resolution supplies the path (those are only ever parsed from the root + /// package.json). Both are user authored, like a root `file:` dependency. + pub(crate) fn is_trusted_folder_dependency(&self, id: DependencyID) -> bool { + if self.is_dependency_of_local_package(id) { + return true; + } + let buf = self.buffers.string_bytes.as_slice(); + let dep = &self.buffers.dependencies[id as usize]; + self.overrides + .contains_name(dep.name_hash, dep.name.slice(buf), buf) + } + /// Does this tree id belong to a workspace (including workspace root)? /// TODO(dylan-conway) fix! pub(crate) fn is_workspace_tree_id(&self, id: tree::Id) -> bool { diff --git a/src/install/lockfile/Tree.rs b/src/install/lockfile/Tree.rs index c17ca42f3151..2376a83d1e1e 100644 --- a/src/install/lockfile/Tree.rs +++ b/src/install/lockfile/Tree.rs @@ -602,11 +602,10 @@ pub(crate) fn is_filtered_dependency_or_workspace( return true; } - let dep_features = match parent_res.tag { - crate::resolution::Tag::Root - | crate::resolution::Tag::Workspace - | crate::resolution::Tag::Folder => manager.options.local_package_features, - _ => manager.options.remote_package_features, + let dep_features = if parent_res.tag.is_local_package() { + manager.options.local_package_features + } else { + manager.options.remote_package_features }; if !dep.behavior.is_enabled(dep_features) { diff --git a/src/install/migration/npm_lock.rs b/src/install/migration/npm_lock.rs index 263077dfa258..f1e28d8fa5a7 100644 --- a/src/install/migration/npm_lock.rs +++ b/src/install/migration/npm_lock.rs @@ -108,6 +108,8 @@ struct Migrator<'a> { link_entries: DynamicBitSet, shadowed: DynamicBitSet, skipped_external: DynamicBitSet, + /// Targets the root or a workspace depends on directly. + local_declared: DynamicBitSet, entry_package_ids: Vec, queue: Vec<(u32, PackageID)>, probe: Vec, @@ -142,6 +144,7 @@ pub(super) fn migrate_packages( link_entries: DynamicBitSet::init_empty(entry_count)?, shadowed: DynamicBitSet::init_empty(entry_count)?, skipped_external: DynamicBitSet::init_empty(entry_count)?, + local_declared: DynamicBitSet::init_empty(entry_count)?, entry_package_ids: vec![INVALID_PACKAGE_ID; entry_count], queue: Vec::new(), probe: Vec::new(), @@ -765,9 +768,20 @@ impl<'a> Migrator<'a> { } let version_tag = version.tag; + // Trust a `file:` spec only in a `file:` package the root or a workspace + // declares, not in a folder a registry package ships (`is_trusted_folder_dependency`). + let declares_folder = res_tag == resolution::Tag::Folder + && version_tag == DepTag::Folder + && self.local_declared.is_set(j as usize); let mut found = self.find_target(key, name); + if let Some((t, _)) = found + && is_local + { + self.local_declared.set(t as usize); + } if let Some((t, through_link)) = found && !is_local + && !declares_folder && self.is_external_folder(t, through_link) { self.skip_external(t, name); diff --git a/src/install/resolution.rs b/src/install/resolution.rs index d1e6fa7424e4..f6b89e01bb2f 100644 --- a/src/install/resolution.rs +++ b/src/install/resolution.rs @@ -1014,6 +1014,13 @@ impl Tag { self == Tag::Git || self == Tag::Github } + /// The root, a workspace, or a `file:` folder package: a package.json on + /// disk rather than content fetched from a registry, git host, or tarball. + /// These get `local_package_features`. + pub(crate) fn is_local_package(self) -> bool { + self == Tag::Root || self == Tag::Workspace || self == Tag::Folder + } + pub(crate) fn can_enqueue_install_task(self) -> bool { self == Tag::Npm || self == Tag::LocalTarball diff --git a/test/cli/install/bun-install.test.ts b/test/cli/install/bun-install.test.ts index d41a2311f467..791a8587e777 100644 --- a/test/cli/install/bun-install.test.ts +++ b/test/cli/install/bun-install.test.ts @@ -10240,56 +10240,203 @@ it("does not extract a tarball for a dependency alias containing '..' path segme }); }); -it("does not install transitive file: dependencies that point outside their package", async () => { - // A dependency declared by a non-workspace package (here: a folder dependency - // of the project) uses a file: specifier pointing at an absolute path outside - // of that package and outside the project. That directory must not be linked - // into node_modules. - using dir = tempDir("transitive-file-dep", { - "secret/credentials.txt": "do-not-link-me", +it("does not install a registry package's transitive file: dependency that points outside its package", async () => { + // A dependency declared by a registry package uses a file: specifier + // pointing at an absolute path outside of that package and outside the + // project. Registry content is not written by the user, so that directory + // must not be resolved or linked into node_modules. (file: dependencies + // declared by the project's own local file: packages are user authored and + // are allowed to point outside the project; see the tests below.) + await withContext(defaultOpts, async ctx => { + const urls: string[] = []; + using dir = tempDir("registry-transitive-file-dep", { + "secret/credentials.txt": "do-not-link-me", + }); + const secretDir = join(String(dir), "secret"); + setContextHandler( + ctx, + dummyRegistryForContext(ctx, urls, { + "0.0.3": { + dependencies: { + loot: "file:" + secretDir.replaceAll("\\", "/"), + }, + }, + }), + ); + await writeFile( + join(ctx.package_dir, "package.json"), + JSON.stringify({ + name: "my-app", + version: "1.0.0", + dependencies: { + baz: "0.0.3", + }, + }), + ); + + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install"], + cwd: ctx.package_dir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + const [err, out, exitCode] = await Promise.all([stderr.text(), stdout.text(), exited]); + + // The directory outside the package must not appear under node_modules, + // neither hoisted nor nested under the declaring package. + expect(await exists(join(ctx.package_dir, "node_modules", "loot"))).toBe(false); + expect(await exists(join(ctx.package_dir, "node_modules", "baz", "node_modules", "loot"))).toBe(false); + // The dependency is reported as unresolvable instead of silently linking local files. + expect(err).toContain("Could not find package.json"); + expect(out).not.toContain("2 packages installed"); + expect(exitCode).toBe(1); + }); +}); + +it("refuses to install an escaping file: dependency that a registry package's own folder declares in the lockfile", async () => { + // Bun's resolver records no dependencies for a folder that a registry package + // ships, but a lockfile written elsewhere (a migration) can. Trust for a + // folder parent is anchored at the root or a workspace, so the installer still + // refuses the escaping path. + await withContext(defaultOpts, async ctx => { + const urls: string[] = []; + using dir = tempDir("registry-folder-declares-file-dep", { + "secret/package.json": JSON.stringify({ name: "loot", version: "1.0.0" }), + "secret/credentials.txt": "do-not-link-me", + }); + const secretDir = join(String(dir), "secret").replaceAll("\\", "/"); + setContextHandler( + ctx, + dummyRegistryForContext(ctx, urls, { + "0.0.3": { + dependencies: { + inner: "file:./inner", + }, + }, + }), + ); + await writeFile( + join(ctx.package_dir, "package.json"), + JSON.stringify({ + name: "my-app", + version: "1.0.0", + dependencies: { + baz: "0.0.3", + }, + }), + ); + await writeFile( + join(ctx.package_dir, "bun.lock"), + JSON.stringify({ + lockfileVersion: 1, + workspaces: { + "": { + name: "my-app", + dependencies: { + baz: "0.0.3", + }, + }, + }, + packages: { + "baz": ["baz@0.0.3", `${ctx.registry_url}baz-0.0.3.tgz`, { dependencies: { inner: "file:./inner" } }, ""], + "baz/inner": ["inner@file:node_modules/baz/inner", { dependencies: { loot: "file:" + secretDir } }], + "baz/inner/loot": ["loot@file:" + secretDir, {}], + }, + }), + ); + + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install"], + cwd: ctx.package_dir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + const [err, out, exitCode] = await Promise.all([stderr.text(), stdout.text(), exited]); + + expect(err).toContain("refusing to install dependency loot with unsafe folder path"); + expect(await exists(join(ctx.package_dir, "node_modules", "loot"))).toBe(false); + expect( + await exists(join(ctx.package_dir, "node_modules", "baz", "node_modules", "inner", "node_modules", "loot")), + ).toBe(false); + expect(out).not.toContain("3 packages installed"); + expect(exitCode).toBe(1); + }); +}); + +it("installs transitive file: dependencies of a local file: package that point outside the project", async () => { + // A file: package referenced by the root package.json lives outside the + // project and declares its own relative folder dependencies that also land + // outside the project. The declaring package is local (not from a registry), + // so its file: paths are user authored and must not be rejected by the + // escape check that constrains registry packages. + using dir = tempDir("local-file-dep-outside", { + "packages/plugin/package.json": JSON.stringify({ + name: "plugin", + version: "1.0.0", + dependencies: { + "shared-lib": "../shared-lib", + }, + devDependencies: { + "shared-dev-lib": "file:../shared-dev-lib", + }, + }), + "packages/plugin/index.js": "module.exports = 'plugin';", + "packages/shared-lib/package.json": JSON.stringify({ name: "shared-lib", version: "1.0.0" }), + "packages/shared-dev-lib/package.json": JSON.stringify({ name: "shared-dev-lib", version: "1.0.0" }), "project/package.json": JSON.stringify({ name: "my-app", version: "1.0.0", dependencies: { - "evil-folder-dep": "file:./evil-folder-dep", + plugin: "file:../packages/plugin", }, }), - "project/evil-folder-dep/index.js": "module.exports = 1;", }); const projectDir = join(String(dir), "project"); - const secretDir = join(String(dir), "secret"); - await write( - join(projectDir, "evil-folder-dep", "package.json"), - JSON.stringify({ - name: "evil-folder-dep", - version: "1.0.0", - dependencies: { - loot: "file:" + secretDir.replaceAll("\\", "/"), - }, - }), - ); + // 1) `bun install` with no lockfile and 2) `bun update` exercise the + // resolve/enqueue path; 3) `bun install` with the saved lockfile and an + // empty node_modules exercises the package installer path. + for (const [i, cmd] of ["install", "update", "install"].entries()) { + if (i === 2) { + await rm(join(projectDir, "node_modules"), { recursive: true, force: true }); + } + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), cmd], + cwd: projectDir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + const [err, out, exitCode] = await Promise.all([stderr.text(), stdout.text(), exited]); - const { stdout, stderr, exited } = spawn({ - cmd: [bunExe(), "install"], - cwd: projectDir, - stdout: "pipe", - stdin: "pipe", - stderr: "pipe", - env, - }); - const err = await stderr.text(); - const out = await stdout.text(); - const exitCode = await exited; + expect(err).not.toContain("Could not find package.json"); + expect(err).not.toContain("failed to resolve"); + expect(err).not.toContain("unsafe folder path"); + expect(err).not.toContain("refusing to install"); + expect(out).toContain("plugin"); + expect(exitCode).toBe(0); + } - // The directory outside the package must not appear under node_modules, - // neither hoisted nor nested under the declaring package. - expect(await exists(join(projectDir, "node_modules", "loot"))).toBe(false); - expect(await exists(join(projectDir, "node_modules", "evil-folder-dep", "node_modules", "loot"))).toBe(false); - // The dependency is reported as unresolvable instead of silently linking local files. - expect(err).toContain("Could not find package.json"); - expect(out).not.toContain("2 packages installed"); - expect(exitCode).toBe(1); + // Both transitive folder dependencies resolve, relative to the project root. + // On Windows the stored folder path uses backslashes (JSON-escaped in the + // lockfile); normalize them before asserting. + const lock = (await file(join(projectDir, "bun.lock")).text()).replaceAll("\\\\", "/"); + expect(lock).toContain('"plugin/shared-lib"'); + expect(lock).toContain("shared-lib@file:../packages/shared-lib"); + expect(lock).toContain('"plugin/shared-dev-lib"'); + expect(await exists(join(projectDir, "node_modules", "plugin", "package.json"))).toBe(true); + // And both are linked under the declaring package. + expect(await exists(join(projectDir, "node_modules", "plugin", "node_modules", "shared-lib", "package.json"))).toBe( + true, + ); + expect( + await exists(join(projectDir, "node_modules", "plugin", "node_modules", "shared-dev-lib", "package.json")), + ).toBe(true); }); it("does not install transitive file: dependencies with overlong folder targets", async () => { @@ -10478,55 +10625,212 @@ for (const field of ["resolutions", "overrides"]) { expect(await exists(join(projectDir, "node_modules", "shared", "package.json"))).toBe(true); }); - it(`still rejects transitive file: dependencies that escape their package when a different name is in "${field}"`, async () => { + it(`still rejects a registry package's transitive file: dependency that escapes when a different name is in "${field}"`, async () => { // An override for a different name must not whitelist an unrelated - // transitive file: dependency that points outside its package. - using dir = tempDir("override-file-dep-unrelated", { - "secret/credentials.txt": "do-not-link-me", - "shared/package.json": JSON.stringify({ name: "shared", version: "1.0.0" }), - "project/package.json": JSON.stringify({ - name: "my-app", - version: "1.0.0", - dependencies: { - "evil-folder-dep": "file:./evil-folder-dep", - }, - [field]: { - shared: "file:../shared", - }, - }), - "project/evil-folder-dep/index.js": "module.exports = 1;", - "project/evil-folder-dep/package.json": JSON.stringify({ - name: "evil-folder-dep", - version: "1.0.0", - dependencies: { - loot: "file:../../secret", - }, - }), - }); - const projectDir = join(String(dir), "project"); + // transitive file: dependency of a registry package that points outside + // its package. The target exists and is installable, so a bypassed check + // would link it. + await withContext(defaultOpts, async ctx => { + const urls: string[] = []; + using dir = tempDir("registry-transitive-file-dep-unrelated-override", { + "secret/package.json": JSON.stringify({ name: "loot", version: "1.0.0" }), + "secret/credentials.txt": "do-not-link-me", + }); + const secretDir = join(String(dir), "secret"); + setContextHandler( + ctx, + dummyRegistryForContext(ctx, urls, { + "0.0.3": { + dependencies: { + loot: "file:" + secretDir.replaceAll("\\", "/"), + }, + }, + }), + ); + await write( + join(ctx.package_dir, "shared", "package.json"), + JSON.stringify({ name: "shared", version: "1.0.0" }), + ); + await writeFile( + join(ctx.package_dir, "package.json"), + JSON.stringify({ + name: "my-app", + version: "1.0.0", + dependencies: { + baz: "0.0.3", + }, + [field]: { + shared: "file:./shared", + }, + }), + ); - const { stdout, stderr, exited } = spawn({ - cmd: [bunExe(), "install"], - cwd: projectDir, - stdout: "pipe", - stdin: "pipe", - stderr: "pipe", - env, + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install"], + cwd: ctx.package_dir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + const [err, out, exitCode] = await Promise.all([stderr.text(), stdout.text(), exited]); + + expect(await exists(join(ctx.package_dir, "node_modules", "loot"))).toBe(false); + expect(await exists(join(ctx.package_dir, "node_modules", "baz", "node_modules", "loot"))).toBe(false); + expect(err).toContain("Could not find package.json"); + expect(out).not.toContain("2 packages installed"); + expect(exitCode).toBe(1); }); - const [err, out, exitCode] = await Promise.all([stderr.text(), stdout.text(), exited]); + }); - expect(await exists(join(projectDir, "node_modules", "loot"))).toBe(false); - expect(await exists(join(projectDir, "node_modules", "evil-folder-dep", "node_modules", "loot"))).toBe(false); - expect(err).toContain("Could not find package.json"); - expect(out).not.toContain("2 packages installed"); - expect(exitCode).toBe(1); + it(`applies a root "${field}" file: path to a registry package's dependency`, async () => { + // overrides/resolutions are declared in the root package.json, so a file: + // path applied through them is trusted even for a dependency declared by a + // registry package (which is otherwise constrained; see the test above). + await withContext(defaultOpts, async ctx => { + const urls: string[] = []; + using dir = tempDir(`override-registry-file-dep-${field}`, { + "shared/package.json": JSON.stringify({ name: "shared", version: "1.0.0" }), + "shared/index.js": "module.exports = 'shared';", + }); + const sharedDir = join(String(dir), "shared"); + setContextHandler( + ctx, + dummyRegistryForContext(ctx, urls, { + "0.0.3": { + dependencies: { + shared: "1.0.0", + }, + }, + }), + ); + await writeFile( + join(ctx.package_dir, "package.json"), + JSON.stringify({ + name: "my-app", + version: "1.0.0", + dependencies: { + baz: "0.0.3", + }, + [field]: { + shared: "file:" + sharedDir.replaceAll("\\", "/"), + }, + }), + ); + + // The first install resolves the override (enqueue path); the second + // starts from the saved lockfile with an empty node_modules (installer path). + for (let i = 0; i < 2; i++) { + if (i === 1) { + await rm(join(ctx.package_dir, "node_modules"), { recursive: true, force: true }); + } + const { stdout, stderr, exited } = spawn({ + cmd: i === 0 ? [bunExe(), "install", "--save-text-lockfile"] : [bunExe(), "install"], + cwd: ctx.package_dir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + const [err, out, exitCode] = await Promise.all([stderr.text(), stdout.text(), exited]); + + expect(err).not.toContain("Could not find package.json"); + expect(err).not.toContain("failed to resolve"); + expect(err).not.toContain("unsafe folder path"); + expect(err).not.toContain("refusing to install"); + expect(out).toContain("baz"); + expect(exitCode).toBe(0); + } + + // The registry package's dependency resolved to the override's file: path + // (no registry request for it was made) and is linked under baz, since + // transitive folder dependencies are not hoisted. + expect(urls.filter(url => url.includes("shared"))).toEqual([]); + const lock = (await file(join(ctx.package_dir, "bun.lock")).text()).replaceAll("\\\\", "/"); + expect(lock).toContain('"baz/shared"'); + expect(lock).toContain("shared@file:"); + expect(await exists(join(ctx.package_dir, "node_modules", "baz", "node_modules", "shared", "index.js"))).toBe( + true, + ); + }); }); - const nestedRule = (value: string) => - field === "overrides" ? { "pkg-a": { shared: value } } : { "pkg-a/shared": value }; + const nestedRule = (parent: string, value: string) => + field === "overrides" ? { [parent]: { shared: value } } : { [`${parent}/shared`]: value }; + + it(`rejects a nested "${field}" rule pointing at a file: path outside the project for a registry package's dependency`, async () => { + // A nested rule only replaces one parent -> child edge, so it does not + // make every edge of that name root authored. The trust checks therefore + // only consult plain rules, and a nested file: path that escapes the + // project is still rejected when the parent is a registry package. + await withContext(defaultOpts, async ctx => { + const urls: string[] = []; + setContextHandler( + ctx, + dummyRegistryForContext(ctx, urls, { + "0.0.3": { + dependencies: { + shared: "1.0.0", + }, + }, + }), + ); + // The project is a subdirectory so that `../shared` exists and is outside it. + const projectDir = join(ctx.package_dir, "project"); + await write( + join(ctx.package_dir, "shared", "package.json"), + JSON.stringify({ name: "shared", version: "1.0.0" }), + ); + await write(join(ctx.package_dir, "shared", "index.js"), "module.exports = 'shared';"); + await write( + join(projectDir, "bunfig.toml"), + ` +[install] +cache = false +registry = "${ctx.registry_url}" +`, + ); + await write( + join(projectDir, "package.json"), + JSON.stringify({ + name: "my-app", + version: "1.0.0", + dependencies: { + baz: "0.0.3", + }, + [field]: nestedRule("baz", "file:../shared"), + }), + ); - it(`rejects a nested "${field}" rule pointing at a file: path outside the project`, async () => { - using dir = tempDir("nested-override-file-dep-outside", { + await using proc = spawn({ + cmd: [bunExe(), "install"], + cwd: projectDir, + stdout: "pipe", + stdin: "ignore", + stderr: "pipe", + env, + }); + const [err, out, exitCode] = await Promise.all([proc.stderr.text(), proc.stdout.text(), proc.exited]); + + // stderr also carries the progress lines of the registry fetch of `baz`. + expect(err.split(/\r?\n/).filter(line => line.startsWith("error:"))).toEqual([ + 'error: Could not find package.json for "file:../shared" dependency "shared"', + "error: shared@1.0.0 failed to resolve", + ]); + expect(out).not.toContain("packages installed"); + expect(await exists(join(projectDir, "node_modules", "shared"))).toBe(false); + expect(await exists(join(projectDir, "node_modules", "baz", "node_modules", "shared"))).toBe(false); + expect(await exists(join(projectDir, "bun.lock"))).toBe(false); + expect(urls.filter(url => url.includes("shared"))).toEqual([]); + expect(exitCode).toBe(1); + }); + }); + + it(`installs a nested "${field}" rule pointing at a file: path outside the project for a local file: package's dependency`, async () => { + // The parent is a file: package of the root, so its dependencies' file: + // paths are trusted like root dependencies (see the transitive file: tests + // above), nested rule or not. + using dir = tempDir("nested-override-file-dep-outside-local", { "shared/package.json": JSON.stringify({ name: "shared", version: "1.0.0" }), "shared/index.js": "module.exports = 'shared';", "project/package.json": JSON.stringify({ @@ -10535,7 +10839,7 @@ for (const field of ["resolutions", "overrides"]) { dependencies: { "pkg-a": "file:./pkg-a", }, - [field]: nestedRule("file:../shared"), + [field]: nestedRule("pkg-a", "file:../shared"), }), "project/pkg-a/package.json": JSON.stringify({ name: "pkg-a", @@ -10548,25 +10852,44 @@ for (const field of ["resolutions", "overrides"]) { }); const projectDir = join(String(dir), "project"); - await using proc = spawn({ - cmd: [bunExe(), "install"], - cwd: projectDir, - stdout: "pipe", - stdin: "ignore", - stderr: "pipe", - env, - }); - const [err, out, exitCode] = await Promise.all([proc.stderr.text(), proc.stdout.text(), proc.exited]); + // The first pass resolves the rule (enqueue path); the second installs + // from the lockfile it wrote (package installer path). + for (const args of [["install"], ["install", "--frozen-lockfile"]]) { + await rm(join(projectDir, "node_modules"), { recursive: true, force: true }); - expect(normalizeBunSnapshot(err, projectDir)).toMatchInlineSnapshot(` - "error: Could not find package.json for "file:../shared" dependency "shared" - error: shared@1.0.0 failed to resolve" - `); - expect(out).not.toContain("packages installed"); - expect(await exists(join(projectDir, "node_modules", "shared"))).toBe(false); - expect(await exists(join(projectDir, "node_modules", "pkg-a", "node_modules", "shared"))).toBe(false); - expect(await exists(join(projectDir, "bun.lock"))).toBe(false); - expect(exitCode).toBe(1); + await using proc = spawn({ + cmd: [bunExe(), ...args], + cwd: projectDir, + stdout: "pipe", + stdin: "ignore", + stderr: "pipe", + env, + }); + const [err, out, exitCode] = await Promise.all([proc.stderr.text(), proc.stdout.text(), proc.exited]); + + expect(err).not.toContain("error:"); + expect(out).toContain("2 packages installed"); + expect(exitCode).toBe(0); + + await using runProc = spawn({ + cmd: [bunExe(), "-e", "console.log(require('pkg-a'))"], + cwd: projectDir, + env, + stdout: "pipe", + stderr: "pipe", + }); + const [runOut, runErr, runExit] = await Promise.all([ + runProc.stdout.text(), + runProc.stderr.text(), + runProc.exited, + ]); + expect(runErr).toBe(""); + expect(runOut).toBe("shared\n"); + expect(runExit).toBe(0); + } + + const lock = (await file(join(projectDir, "bun.lock")).text()).replaceAll("\\\\", "/"); + expect(lock).toContain('"pkg-a/shared": ["shared@file:../shared", {}]'); }); it(`installs a nested "${field}" rule pointing at a file: path inside the project`, async () => { @@ -10577,7 +10900,7 @@ for (const field of ["resolutions", "overrides"]) { dependencies: { "pkg-a": "file:./pkg-a", }, - [field]: nestedRule("file:./vendor/shared"), + [field]: nestedRule("pkg-a", "file:./vendor/shared"), }), "vendor/shared/package.json": JSON.stringify({ name: "shared", version: "2.0.0" }), "vendor/shared/index.js": "module.exports = 'vendored shared';", diff --git a/test/cli/install/migration/migrate.test.ts b/test/cli/install/migration/migrate.test.ts index 0a90dee296c5..0ba190efb16f 100644 --- a/test/cli/install/migration/migrate.test.ts +++ b/test/cli/install/migration/migrate.test.ts @@ -1643,6 +1643,82 @@ describe("package-lock.json migration fixes", () => { expect(storeEntries(project.dir)).toStrictEqual(linker === "isolated" ? [project.o.replaceAll(/[/:]/g, "+")] : []); }); + // A file: path written in an out-of-tree file: package's own package.json is trusted like a + // root dependency, the same as a fresh `bun install` does, even when it leaves the project. + test.concurrent("a file: dependency declared by an out-of-tree file: package migrates", async () => { + const dependencies = { plugin: "file:../packages/plugin" }; + using copy = tempDir("npm-migrate-folder-declares-file-dep", { + "root/package.json": JSON.stringify({ name: "root", dependencies }), + "root/package-lock.json": npmLock("root", { + "": { name: "root", dependencies }, + "../packages/plugin": { version: "1.0.0", dependencies: { "shared-lib": "file:../shared-lib" } }, + "../packages/plugin/node_modules/shared-lib": { resolved: "../packages/shared-lib", link: true }, + "../packages/shared-lib": { version: "1.0.0" }, + "node_modules/plugin": { resolved: "../packages/plugin", link: true }, + }), + "packages/plugin/package.json": JSON.stringify({ + name: "plugin", + version: "1.0.0", + dependencies: { "shared-lib": "file:../shared-lib" }, + }), + "packages/shared-lib/package.json": JSON.stringify({ name: "shared-lib", version: "1.0.0" }), + }); + const dir = join(String(copy), "root"); + writeExtra(dir, {}); + + const { stderr, exitCode, lock } = await migrate(dir); + expect(stderr).not.toContain("outside the project"); + expect(exitCode).toBe(0); + expect(lock.packages).toStrictEqual({ + plugin: ["plugin@file:../packages/plugin", { dependencies: { "shared-lib": "file:../shared-lib" } }], + "plugin/shared-lib": ["shared-lib@file:../packages/shared-lib", {}], + }); + await frozen(dir); + + // The same lockfile a fresh `bun install` writes, and the installer links the nested package. + const install = await run(dir, "install", "--frozen-lockfile"); + expect(install.stderr).not.toContain("error"); + expect(install.exitCode).toBe(0); + expect( + await Bun.file(join(dir, "node_modules", "plugin", "node_modules", "shared-lib", "package.json")).json(), + ).toHaveProperty("name", "shared-lib"); + }); + + // A folder that a registry package ships inside itself is a `Folder` entry too, but the registry + // package declared it, so a `file:` path in its package.json that leaves the project is still skipped. + test.concurrent("a file: dependency declared by a registry package's own folder is still skipped", async () => { + const dependencies = { evil: "1.0.0" }; + using copy = tempDir("npm-migrate-registry-folder-declares-file-dep", { + "root/package.json": JSON.stringify({ name: "root", dependencies }), + "root/package-lock.json": npmLock("root", { + "": { name: "root", dependencies }, + "node_modules/evil": { + version: "1.0.0", + resolved: `${OFFLINE_REGISTRY}evil/-/evil-1.0.0.tgz`, + dependencies: { inner: "file:./inner" }, + }, + "node_modules/evil/node_modules/inner": { resolved: "node_modules/evil/inner", link: true }, + "node_modules/evil/inner": { version: "1.0.0", dependencies: { loot: "file:../../../../secret" } }, + "node_modules/evil/inner/node_modules/loot": { resolved: "../secret", link: true }, + "../secret": { version: "1.0.0" }, + }), + "secret/package.json": JSON.stringify({ name: "loot", version: "1.0.0" }), + "secret/credentials.txt": "do-not-link-me", + }); + const dir = join(String(copy), "root"); + writeExtra(dir, {}); + + const { stderr, exitCode, text, lock } = await migrate(dir); + expect(stderr).toContain( + 'skipped "loot" from package-lock.json: transitive folder dependency "../secret" is outside the project', + ); + expect(exitCode).toBe(0); + expect(Object.keys(lock.packages).sort()).toStrictEqual(["evil", "evil/inner"]); + expect(lock.packages["evil/inner"]).toStrictEqual(["inner@file:node_modules/evil/inner", {}]); + expect(text).not.toContain("../secret"); + expect(text).not.toContain("loot"); + }); + test.concurrent("lockfileVersion 5 is refused, and install falls back to a fresh resolve", async () => { using dir = synthetic("npm-migrate-v5", { "package.json": JSON.stringify({ name: "v5", dependencies: { "dep-1": "file:dep-1" } }),