fix(bins.resolver): reject reserved manifest bin names (#12289)

Manifest bin keys "", ".", "..", and scoped forms such as "@scope/.."
passed the bin-name guard because encodeURIComponent leaves them
unchanged. When joined to the global bin directory during global
remove/update/add operations, "." resolves to the bin directory itself
and ".." to its parent, which removeBin then recursively deletes.

Reject empty, ".", and ".." bin names after scope stripping in both the
TypeScript resolver and the pacquet cmd-shim bin resolver.

.changeset/strange-bin-segments.md

@@ -0,0 +1,6 @@
+---
+"@pnpm/bins.resolver": patch
+"pnpm": patch
+---
+
+Reject reserved manifest `bin` names (`""`, `"."`, `".."`, and scoped forms such as `@scope/..`) when resolving a package's bins. These names previously passed the bin-name guard and, when joined to the global bin directory during global remove/update/add operations, could resolve to the global bin directory itself or its parent and have it recursively deleted.

bins/resolver/src/index.ts

@@ -66,7 +66,13 @@ function commandsFromBin (bin: PackageBin, pkgName: string, pkgPath: string): Co
     const binName = commandName[0] === '@'
       ? commandName.slice(commandName.indexOf('/') + 1)
       : commandName
-    // Validate: must be safe (no path traversal) - only allow URL-safe chars or $
+    // Validate: must be safe (no path traversal). Reject empty and the
+    // filesystem-relative names "." and ".." (these survive encodeURIComponent
+    // unchanged but resolve to the bin directory itself or its parent when
+    // joined to a target dir), then only allow URL-safe chars or $.
+    if (binName === '' || binName === '.' || binName === '..') {
+      continue
+    }
     if (binName !== encodeURIComponent(binName) && binName !== '$') {
       continue
     }

bins/resolver/test/index.ts

@@ -147,6 +147,26 @@ test('skip scoped bin names with path traversal', async () => {
   ])
 })
 
+test('skip reserved bin names', async () => {
+  expect(
+    await getBinsFromPackageManifest({
+      name: 'malicious',
+      version: '1.0.0',
+      bin: {
+        '': './empty.js',
+        '.': './dot.js',
+        '..': './dot-dot.js',
+        '@scope/..': './scoped-dot-dot.js',
+        good: './good',
+      },
+    }, process.cwd())).toStrictEqual([
+    {
+      name: 'good',
+      path: path.resolve('good'),
+    },
+  ])
+})
+
 test('skip directories.bin with path traversal', async () => {
   // Security test: malicious packages can try to escape the package root
   // using directories.bin to chmod files at arbitrary locations

global/commands/test/globalRemove.test.ts

@@ -0,0 +1,47 @@
+import fs from 'node:fs'
+import os from 'node:os'
+import path from 'node:path'
+
+import { expect, jest, test } from '@jest/globals'
+
+const removeBin = jest.fn<(cmd: string) => Promise<void>>().mockResolvedValue(undefined)
+
+jest.unstable_mockModule('@pnpm/bins.remover', () => ({ removeBin }))
+
+const { handleGlobalRemove } = await import('../src/globalRemove.js')
+
+// A malicious global package whose manifest declares reserved bin keys must not
+// reach the deletion sink: `path.join(globalBinDir, '.')` is the bin directory
+// itself and `path.join(globalBinDir, '..')` is its parent, so removing either
+// would wipe out unrelated files. Only the safe `good` shim may be deleted.
+test('global remove ignores reserved manifest bin names', async () => {
+  const globalDir = fs.mkdtempSync(path.join(os.tmpdir(), 'global-remove-'))
+  const globalBinDir = path.join(globalDir, 'bin')
+  const installDir = path.join(globalDir, 'install')
+  const depDir = path.join(installDir, 'node_modules', 'evil')
+  fs.mkdirSync(depDir, { recursive: true })
+  fs.writeFileSync(
+    path.join(installDir, 'package.json'),
+    JSON.stringify({ name: 'global', version: '1.0.0', dependencies: { evil: '1.0.0' } })
+  )
+  fs.writeFileSync(
+    path.join(depDir, 'package.json'),
+    JSON.stringify({
+      name: 'evil',
+      version: '1.0.0',
+      bin: {
+        '': './empty.js',
+        '.': './dot.js',
+        '..': './dot-dot.js',
+        '@scope/..': './scoped-dot-dot.js',
+        good: './good.js',
+      },
+    })
+  )
+  fs.symlinkSync(installDir, path.join(globalDir, 'hash'))
+
+  await handleGlobalRemove({ globalPkgDir: globalDir, bin: globalBinDir }, ['evil'])
+
+  expect(removeBin).toHaveBeenCalledTimes(1)
+  expect(removeBin).toHaveBeenCalledWith(path.join(globalBinDir, 'good'))
+})

pacquet/crates/cmd-shim/src/bin_resolver.rs

@@ -171,11 +171,15 @@ fn commands_from_bin(bin: &Value, pkg_name: Option<&str>, pkg_path: &Path) -> Ve
 ///
 /// `encodeURIComponent` leaves the following bytes unescaped:
 /// `A-Z a-z 0-9 - _ . ! ~ * ' ( )`.
+///
+/// `.` and `..` survive `encodeURIComponent` unchanged but resolve to the bin
+/// directory itself or its parent when joined to a target dir, so they are
+/// rejected explicitly.
 fn is_safe_bin_name(name: &str) -> bool {
     if name == "$" {
         return true;
     }
-    if name.is_empty() {
+    if name.is_empty() || name == "." || name == ".." {
         return false;
     }
     name.bytes().all(|byte| {

pacquet/crates/cmd-shim/src/bin_resolver/tests.rs

@@ -362,6 +362,28 @@ fn empty_bin_key_is_rejected() {
     assert_eq!(commands[0].name, "good");
 }
 
+/// The relative names `.` and `..` survive the URL-safe guard's character
+/// set (`.` is unescaped by `encodeURIComponent`) but resolve to the bin
+/// directory itself or its parent when joined to a target dir, so
+/// `is_safe_bin_name` must reject them. Scoped forms like `@scope/..`
+/// collapse to `..` after the scope strip and must be rejected too.
+#[test]
+fn reserved_relative_bin_names_are_rejected() {
+    let manifest = json!({
+        "name": "malicious",
+        "version": "1.0.0",
+        "bin": {
+            ".": "dot.js",
+            "..": "dotdot.js",
+            "@scope/..": "scoped-dotdot.js",
+            "good": "good.js",
+        },
+    });
+    let commands = get_bins_from_package_manifest::<Host>(&manifest, Path::new("/p"));
+    assert_eq!(commands.len(), 1);
+    assert_eq!(commands[0].name, "good");
+}
+
 /// [`lexical_normalize`] drops `.` (CurDir) segments. This is a direct
 /// test on the helper. The integration-style test below covers the same
 /// arm via `directories.bin`, but a direct assertion makes the CurDir