install: refuse a dependency name that node_modules cannot hold (.bin, node_modules, bare @scope)

src/install/PackageInstaller.rs

@@ -364,25 +364,26 @@ fn abs_node_modules_path(
     abs
 }
 
-/// A dependency alias becomes the install destination inside `node_modules`
-/// (the existing entry is renamed aside, deleted, and re-created). Reject
-/// anything that could escape `node_modules`: empty names, `.`/`..`
-/// components, absolute paths, drive letters, backslashes, NUL bytes, and any
-/// separator other than the single `/` in a scoped name (`@scope/name`).
+/// A dependency alias is the install destination inside `node_modules`.
 pub(crate) fn alias_is_safe_install_target(alias: &[u8]) -> bool {
-    if alias.is_empty() || alias.len() >= MAX_PATH_BYTES || strings::contains_any(alias, b"\\:\0") {
+    alias.len() < MAX_PATH_BYTES && crate::dependency::is_valid_node_modules_entry_name(alias)
+}
+
+/// The path rules alone, for the own name of the project, a workspace or a folder dependency.
+pub(crate) fn name_is_single_path_entry(name: &[u8]) -> bool {
+    if name.is_empty() || name.len() >= MAX_PATH_BYTES || strings::contains_any(name, b"\\:\0") {
         return false;
     }
 
     let mut component_count = 0usize;
-    for component in strings::split(alias, b"/") {
+    for component in strings::split(name, b"/") {
         component_count += 1;
         if component.is_empty() || component == b"." || component == b".." {
             return false;
         }
     }
 
-    component_count == 1 || (component_count == 2 && alias[0] == b'@')
+    component_count == 1 || (component_count == 2 && name[0] == b'@')
 }
 
 /// Formats the version label `PackageInstall` verifies and hashes patches

src/install/PackageManager/install_with_manager.rs

@@ -685,6 +685,10 @@ pub fn install_with_manager(
         }
     };
     let lockfile_before_clean = core::mem::replace(&mut manager.lockfile, new_lockfile);
+    // The log was reset above, so this is a name the tree builder refused: stop before the linker.
+    if manager.log_mut().has_errors() {
+        manager.crash();
+    }
     if manager.subcommand == Subcommand::Update && !manager.options.dry_run {
         Output::flush();
         crate::update_transitive::warn_orphaned_patches(manager);

src/install/dependency.rs

@@ -551,6 +551,27 @@ pub(crate) fn is_safe_install_folder_name(name: &[u8]) -> bool {
     true
 }
 
+/// `name` or `@scope/name`, where neither half is a directory the installer owns.
+pub(crate) fn is_valid_node_modules_entry_name(name: &[u8]) -> bool {
+    if !is_safe_install_folder_name(name) {
+        return false;
+    }
+
+    let package = match name.strip_prefix(b"@") {
+        Some(scoped) => match strings::index_of_char_usize(scoped, b'/') {
+            Some(slash) if slash > 0 => &scoped[slash + 1..],
+            _ => return false,
+        },
+        None => name,
+    };
+
+    // On a case-insensitive volume `NODE_MODULES` is `node_modules`.
+    !strings::contains_char(package, b'/')
+        && !name.starts_with(b".")
+        && !package.starts_with(b".")
+        && !package.eq_ignore_ascii_case(b"node_modules")
+}
+
 /// assumes version is valid
 pub fn without_build_tag(version: &[u8]) -> &[u8] {
     if let Some(plus) = strings::index_of_char(version, b'+') {

src/install/extract_tarball.rs

@@ -471,7 +471,7 @@ impl ExtractTarball {
             // The entire body lives inside the thread_local borrow closure.
             let folder_name: &[u8] = match self.resolution.tag {
                 ResolutionTag::Npm => {
-                    if !bun_install::dependency::is_safe_install_folder_name(name) {
+                    if !bun_install::dependency::is_valid_node_modules_entry_name(name) {
                         log.add_error_fmt(
                             None,
                             bun_ast::Loc::EMPTY,

src/install/isolated_install.rs

@@ -2121,8 +2121,19 @@ pub(crate) fn install_isolated_packages(
             {
                 let mut unsafe_folder_name: Option<&[u8]> = None;
                 let name = pkg_name.slice(string_buf);
-                if !name.is_empty() && !crate::package_installer::alias_is_safe_install_target(name)
-                {
+                // Only these kinds are also linked under their own name, in `.bun/node_modules`.
+                let own_name_is_safe = name.is_empty()
+                    || match pkg_res.tag {
+                        ResolutionTag::Npm
+                        | ResolutionTag::Git
+                        | ResolutionTag::Github
+                        | ResolutionTag::LocalTarball
+                        | ResolutionTag::RemoteTarball => {
+                            crate::package_installer::alias_is_safe_install_target(name)
+                        }
+                        _ => crate::package_installer::name_is_single_path_entry(name),
+                    };
+                if !own_name_is_safe {
                     unsafe_folder_name = Some(name);
                 } else {
                     for dep in entry_dependencies[entry_id.get() as usize].slice() {

src/install/lockfile/Tree.rs

@@ -287,9 +287,7 @@ impl<'a, const PATH_STYLE: IteratorPathStyle> Iterator<'a, PATH_STYLE> {
     }
 }
 
-/// Tree folder names are joined into install destinations as
-/// `node_modules/<name>/...`; this path and the tree builder must agree on the
-/// same validator.
+/// A tree folder name is joined onto `node_modules/`, so it has to stay inside it.
 pub(crate) fn folder_name_is_safe(name: &[u8]) -> bool {
     crate::dependency::is_safe_install_folder_name(name)
 }
@@ -748,14 +746,17 @@ impl Tree {
                 .name
                 .slice(lockfile.buffers.string_bytes.as_slice());
             if !dependency_name.is_empty()
-                && !crate::dependency::is_safe_install_folder_name(dependency_name)
+                && !crate::dependency::is_valid_node_modules_entry_name(dependency_name)
             {
-                builder.maybe_report_error(format_args!(
-                    "Invalid dependency name \"{}\"",
-                    dependency
-                        .name
-                        .fmt(lockfile.buffers.string_bytes.as_slice()),
-                ));
+                // An unresolved dependency gets no folder, and the resolver has reported it.
+                if pkg_id != invalid_package_id {
+                    builder.maybe_report_error(format_args!(
+                        "Invalid dependency name \"{}\"",
+                        dependency
+                            .name
+                            .fmt(lockfile.buffers.string_bytes.as_slice()),
+                    ));
+                }
                 continue 'dep;
             }
 

src/install/lockfile/bun.lock.rs

@@ -3262,7 +3262,9 @@ pub(crate) fn parse_into_binary_lockfile(
                 .is_none_or(|manager| manager.options.link_workspace_packages),
         );
 
-        if let Err(tree::SubtreeError::OutOfMemory) = lockfile.resolve(log) {
+        // The install reports a refused name itself, after it applies package.json.
+        let mut refused_names = bun_ast::Log::init();
+        if let Err(tree::SubtreeError::OutOfMemory) = lockfile.resolve(&mut refused_names) {
             return Err(ParseError::OutOfMemory);
         }
     }

test/cli/install/bun-install.test.ts

@@ -10275,6 +10275,227 @@ it("rejects dependency aliases containing '..' path segments", async () => {
   });
 });
 
+// Serves `bar` as a registry package whose manifest declares `dependencies`,
+// and every other name as a package with no dependencies.
+function registryWhereBarDependsOn(ctx: TestContext, urls: string[], dependencies: Record<string, string>) {
+  const bar = dummyRegistryForContext(ctx, urls, { "0.0.2": { dependencies } });
+  const others = dummyRegistryForContext(ctx, urls, { "0.0.3": {} });
+  return (request: Request) => (new URL(request.url).pathname.endsWith("/bar") ? bar(request) : others(request));
+}
+
+// The key of a dependency becomes a path under node_modules, and a registry
+// package chooses the keys of its own dependencies. A key that names an entry
+// the installer owns would unpack the target there: `.bin` is on the PATH of
+// every lifecycle script, so is the `.bin` of a nested `node_modules`, and a
+// bare `@scope` is the directory the project's own `@scope/*` packages live in.
+// Every key here is a directory the installer owns, or a shape that would make
+// one. `@barn/node_modules` matters for the same reason `node_modules` does: a
+// lifecycle script runs with the `node_modules/.bin` of every directory above
+// it on its PATH, so the payload would reach the scripts of `@barn/*`.
+describe.concurrent.each([
+  ".bin",
+  "@barn/.bin",
+  "node_modules",
+  "Node_Modules",
+  "@barn/node_modules",
+  "@barn",
+  "@/baz",
+  "@barn/baz/extra",
+])("a dependency named %p", key => {
+  it.each([
+    ["a registry package", "hoisted"],
+    ["a registry package", "isolated"],
+    ["the project", "hoisted"],
+    ["the project", "isolated"],
+  ])("declared by %s is refused before anything is installed (%s linker)", async (declarer, linker) => {
+    await withContext({ linker: linker as "hoisted" | "isolated" }, async ctx => {
+      const urls: string[] = [];
+      const target = { [key]: "npm:baz@0.0.3" };
+      const fromRegistry = declarer === "a registry package";
+      setContextHandler(ctx, registryWhereBarDependsOn(ctx, urls, fromRegistry ? target : {}));
+      await Promise.all([
+        writeFile(
+          join(ctx.package_dir, "package.json"),
+          JSON.stringify({
+            name: "foo",
+            version: "0.0.1",
+            // `bar` is a normal package that must install when nothing is refused
+            dependencies: fromRegistry ? { bar: "0.0.2" } : { ...target, bar: "0.0.2" },
+            scripts: { postinstall: [bunExe(), "foo-postinstall.js"].join(" ") },
+          }),
+        ),
+        writeFile(
+          join(ctx.package_dir, "foo-postinstall.js"),
+          'require("fs").writeFileSync("foo-postinstall.txt", "ran");',
+        ),
+      ]);
+
+      await using proc = spawn({
+        cmd: [bunExe(), "install"],
+        cwd: ctx.package_dir,
+        stdout: "pipe",
+        stderr: "pipe",
+        env,
+      });
+      const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+
+      expect({
+        errors: [...new Set(err.split(/\r?\n/).filter(line => line.startsWith("error:")))],
+        // nothing is unpacked at the reserved path
+        unpacked: await exists(join(ctx.package_dir, "node_modules", key, "package.json")),
+        // the install stops before the linker, so the good package beside the
+        // refused one is not laid out either
+        otherInstalled: await exists(join(ctx.package_dir, "node_modules", "bar", "package.json")),
+        // and no lifecycle script runs
+        postinstall: await exists(join(ctx.package_dir, "foo-postinstall.txt")),
+        installed: out.includes("installed"),
+        exitCode,
+      }).toEqual({
+        errors: [`error: Invalid dependency name "${key}"`],
+        unpacked: false,
+        otherInstalled: false,
+        postinstall: false,
+        installed: false,
+        exitCode: 1,
+      });
+    });
+  });
+});
+
+it("installs after package.json drops a name that bun.lock still holds", async () => {
+  // bun.lock as an older bun wrote it, when the key `@src` was accepted. The
+  // project has since renamed that dependency. The stale name is in the file
+  // that is loaded, not in the graph that is installed.
+  using dir = tempDir("stale-lockfile-name", {
+    "package.json": JSON.stringify({ name: "app", version: "1.0.0", dependencies: { "src-alias": "file:./src" } }),
+    "src/package.json": JSON.stringify({ name: "src", version: "1.0.0" }),
+    "bun.lock": `{
+  "lockfileVersion": 2,
+  "configVersion": 1,
+  "workspaces": {
+    "": {
+      "name": "app",
+      "dependencies": {
+        "@src": "file:./src",
+      },
+    },
+  },
+  "packages": {
+    "@src": ["src@file:src", {}],
+  }
+}
+`,
+  });
+
+  await using proc = spawn({
+    cmd: [bunExe(), "install"],
+    cwd: String(dir),
+    stdout: "pipe",
+    stderr: "pipe",
+    env,
+  });
+  const [, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+
+  expect({
+    errors: err.split(/\r?\n/).filter(line => line.startsWith("error:")),
+    linked: await exists(join(String(dir), "node_modules", "src-alias", "package.json")),
+    // the lockfile is written again, without the name
+    lockfileHoldsStaleName: (await file(join(String(dir), "bun.lock")).text()).includes('"@src"'),
+    exitCode,
+  }).toEqual({ errors: [], linked: true, lockfileHoldsStaleName: false, exitCode: 0 });
+});
+
+it("refuses a registry package whose own name is an entry of the installer", async () => {
+  await withContext(defaultOpts, async ctx => {
+    const urls: string[] = [];
+    // The alias is fine, so the tree builder accepts it. The name the registry
+    // serves is the one that would become the cache folder.
+    const manifests = dummyRegistryForContext(ctx, urls, { "0.0.3": {} });
+    setContextHandler(ctx, request =>
+      request.url.endsWith(".tgz") ? new Response(file(join(import.meta.dir, "baz-0.0.3.tgz"))) : manifests(request),
+    );
+    await writeFile(
+      join(ctx.package_dir, "package.json"),
+      JSON.stringify({ name: "foo", version: "0.0.1", dependencies: { helper: "npm:.bin@0.0.3" } }),
+    );
+
+    await using proc = spawn({
+      cmd: [bunExe(), "install"],
+      cwd: ctx.package_dir,
+      stdout: "pipe",
+      stderr: "pipe",
+      env,
+    });
+    const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+
+    expect(err).toContain('Refusing to install package with invalid name ".bin"');
+    expect(out).not.toContain("installed");
+    expect(exitCode).not.toBe(0);
+  });
+});
+
+it("still installs an alias that names no entry of the installer", async () => {
+  await withContext(defaultOpts, async ctx => {
+    const urls: string[] = [];
+    setContextHandler(ctx, dummyRegistryForContext(ctx, urls, { "0.0.3": {} }));
+    // npm refuses a leading underscore and a leading hyphen too. Neither names
+    // a directory the installer uses, so these keep working.
+    const aliases = ["_baz", "-baz", "baz.js", "@barn/baz"];
+    await writeFile(
+      join(ctx.package_dir, "package.json"),
+      JSON.stringify({
+        name: "foo",
+        version: "0.0.1",
+        dependencies: Object.fromEntries(aliases.map(alias => [alias, "npm:baz@0.0.3"])),
+      }),
+    );
+
+    await using proc = spawn({
+      cmd: [bunExe(), "install"],
+      cwd: ctx.package_dir,
+      stdout: "pipe",
+      stderr: "pipe",
+      env,
+    });
+    const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+
+    expect(err).not.toContain("error:");
+    // One package, under four names.
+    expect(out).toContain("1 package installed");
+    expect(
+      await Promise.all(aliases.map(alias => exists(join(ctx.package_dir, "node_modules", alias, "package.json")))),
+    ).toEqual([true, true, true, true]);
+    expect(exitCode).toBe(0);
+  });
+});
+
+it("still links a bin whose name starts with a period", async () => {
+  await withContext(defaultOpts, async ctx => {
+    const urls: string[] = [];
+    // A bin name is not a node_modules entry. npm links this one, and so does bun.
+    setContextHandler(ctx, dummyRegistryForContext(ctx, urls, { "0.0.3": { bin: { ".baz-run": "index.js" } } }));
+    await writeFile(
+      join(ctx.package_dir, "package.json"),
+      JSON.stringify({ name: "foo", version: "0.0.1", dependencies: { baz: "0.0.3" } }),
+    );
+
+    await using proc = spawn({
+      cmd: [bunExe(), "install"],
+      cwd: ctx.package_dir,
+      stdout: "pipe",
+      stderr: "pipe",
+      env,
+    });
+    const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+
+    expect(err).not.toContain("error:");
+    expect(out).toContain("1 package installed");
+    expect(await readdirSorted(join(ctx.package_dir, "node_modules", ".bin"))).toHaveBins([".baz-run"]);
+    expect(join(ctx.package_dir, "node_modules", ".bin", ".baz-run")).toBeValidBin(join("..", "baz", "index.js"));
+    expect(exitCode).toBe(0);
+  });
+});
+
 it("does not extract a tarball for a dependency alias containing '..' path segments", async () => {
   await withContext(defaultOpts, async ctx => {
     const urls: string[] = [];