From 230df57aa5ac781effb243cb4beeb2d30cc6a8bf Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Tue, 9 Jun 2026 20:20:56 +0200 Subject: [PATCH] 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 | 6 +++ bins/resolver/src/index.ts | 8 +++- bins/resolver/test/index.ts | 20 ++++++++ global/commands/test/globalRemove.test.ts | 47 +++++++++++++++++++ pacquet/crates/cmd-shim/src/bin_resolver.rs | 6 ++- .../crates/cmd-shim/src/bin_resolver/tests.rs | 22 +++++++++ 6 files changed, 107 insertions(+), 2 deletions(-) create mode 100644 .changeset/strange-bin-segments.md create mode 100644 global/commands/test/globalRemove.test.ts diff --git a/.changeset/strange-bin-segments.md b/.changeset/strange-bin-segments.md new file mode 100644 index 0000000000..35951c1863 --- /dev/null +++ b/.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. diff --git a/bins/resolver/src/index.ts b/bins/resolver/src/index.ts index dc97de1f47..601832fe43 100644 --- a/bins/resolver/src/index.ts +++ b/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 } diff --git a/bins/resolver/test/index.ts b/bins/resolver/test/index.ts index ac39a60a9a..b473113fe9 100644 --- a/bins/resolver/test/index.ts +++ b/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 diff --git a/global/commands/test/globalRemove.test.ts b/global/commands/test/globalRemove.test.ts new file mode 100644 index 0000000000..0323e9d608 --- /dev/null +++ b/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>().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')) +}) diff --git a/pacquet/crates/cmd-shim/src/bin_resolver.rs b/pacquet/crates/cmd-shim/src/bin_resolver.rs index 8dc928269d..d10505749e 100644 --- a/pacquet/crates/cmd-shim/src/bin_resolver.rs +++ b/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| { diff --git a/pacquet/crates/cmd-shim/src/bin_resolver/tests.rs b/pacquet/crates/cmd-shim/src/bin_resolver/tests.rs index 13a72ea734..471f908d83 100644 --- a/pacquet/crates/cmd-shim/src/bin_resolver/tests.rs +++ b/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::(&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