From ad84fffd46d8569b3adfba914f3b715fb359d19d Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Tue, 26 May 2026 17:25:25 +0200 Subject: [PATCH] fix: reject path-traversal segments in dependency aliases (#11954) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: reject path-traversal segments in dependency aliases A transitive registry package can use a dependency-alias key like `@x/../../../../../.git/hooks` to make `pnpm install` create a symlink outside the intended `node_modules` directory, since pnpm passes the alias straight into `path.join(modulesDir, alias)` without checking that the joined path stays inside `modulesDir`. Reject aliases that aren't a single `name` or `@scope/name` shape at manifest-read time (both the importer's manifest and every transitive package manifest) and re-check at the symlink layer as defense in depth. Mirror the fix in pacquet's deps-resolver. --- Written by an agent (Claude Code, claude-opus-4-7). * fix(pacquet): use raw strings in alias validator tests for dylint Perfectionist's `prefer-raw-string` lint rejects the two backslash-escaped test inputs. --- Written by an agent (Claude Code, claude-opus-4-7). * refactor: tighten dependency-alias validator to validate-npm-package-name An alias is the directory name pnpm creates inside `node_modules`, so the only valid shapes are a single `name` or `@scope/name` consisting of URL-friendly characters with no leading `.` / `_`, and not equal to reserved names such as `node_modules`. That's the same `validForOldPackages` rule `parseWantedDependency` already applies to CLI-given names — the manifest-read path should match. Route both stacks through it so `.bin`, `.pnpm`, `node_modules`, `favicon.ico`, whitespace, and non-URL-friendly characters are all rejected alongside the path-traversal shapes the narrow validator caught. --- Written by an agent (Claude Code, claude-opus-4-7). * refactor: collapse symlink-layer assertion + path.join into safeJoinModulesDir The two-step pattern of "assert the alias stays in the dir" then "join the dir and the alias" left it possible for a caller to use the join without the assertion. Fold them into a single `safeJoinModulesDir` that returns the joined path and throws on escape, so the check is unmissable. --- Written by an agent (Claude Code, claude-opus-4-7). * test(symlink-dependency): cover the path-equals-dir guard branch The earlier tests only exercised the `!startsWith` branch with `'../sibling'` and `'@x/../../../etc'`. Add `''` and `'.'` as alias cases — both resolve to the modules dir itself and hit the `resolvedLink === resolvedDir` branch of `safeJoinModulesDir`. --- Written by an agent (Claude Code, claude-opus-4-7). --- .../reject-traversal-dependency-aliases.md | 7 ++ Cargo.lock | 1 + fs/symlink-dependency/src/index.ts | 8 +-- .../src/safeJoinModulesDir.ts | 19 +++++ .../src/symlinkDirectRootDependency.ts | 5 +- .../test/safeJoinModulesDir.test.ts | 36 ++++++++++ installing/deps-resolver/package.json | 4 +- .../src/getNonDevWantedDependencies.ts | 12 +++- .../src/getWantedDependencies.ts | 6 ++ .../src/validateDependencyAlias.ts | 31 ++++++++ .../test/validateDependencyAlias.test.ts | 67 +++++++++++++++++ .../crates/resolving-deps-resolver/Cargo.toml | 13 ++-- .../crates/resolving-deps-resolver/src/lib.rs | 2 + .../src/resolve_dependency_tree.rs | 72 +++++++++++++++---- .../src/resolve_importer.rs | 20 ++++-- .../resolving-deps-resolver/src/tests.rs | 46 ++++++++++++ .../src/validate_dependency_alias.rs | 60 ++++++++++++++++ pnpm-lock.yaml | 6 ++ 18 files changed, 379 insertions(+), 36 deletions(-) create mode 100644 .changeset/reject-traversal-dependency-aliases.md create mode 100644 fs/symlink-dependency/src/safeJoinModulesDir.ts create mode 100644 fs/symlink-dependency/test/safeJoinModulesDir.test.ts create mode 100644 installing/deps-resolver/src/validateDependencyAlias.ts create mode 100644 installing/deps-resolver/test/validateDependencyAlias.test.ts create mode 100644 pacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rs diff --git a/.changeset/reject-traversal-dependency-aliases.md b/.changeset/reject-traversal-dependency-aliases.md new file mode 100644 index 0000000000..b587dafa2f --- /dev/null +++ b/.changeset/reject-traversal-dependency-aliases.md @@ -0,0 +1,7 @@ +--- +"@pnpm/installing.deps-resolver": patch +"@pnpm/fs.symlink-dependency": patch +"pnpm": patch +--- + +Reject dependency aliases that contain path-traversal segments (such as `@x/../../../../../.git/hooks`) when reading them from a package manifest or symlinking them into `node_modules`. A malicious registry package could otherwise use a transitive dependency key to make `pnpm install` create symlinks at attacker-chosen paths outside the intended `node_modules` directory. diff --git a/Cargo.lock b/Cargo.lock index c5d4d860f0..4f630f50b0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2660,6 +2660,7 @@ dependencies = [ "pacquet-lockfile", "pacquet-package-manifest", "pacquet-patching", + "pacquet-resolving-parse-wanted-dependency", "pacquet-resolving-resolver-base", "pipe-trait", "pretty_assertions", diff --git a/fs/symlink-dependency/src/index.ts b/fs/symlink-dependency/src/index.ts index 7862a04be0..5f80c69a58 100644 --- a/fs/symlink-dependency/src/index.ts +++ b/fs/symlink-dependency/src/index.ts @@ -1,8 +1,8 @@ -import path from 'node:path' - import { linkLogger } from '@pnpm/core-loggers' import { symlinkDir, symlinkDirSync } from 'symlink-dir' +import { safeJoinModulesDir } from './safeJoinModulesDir.js' + export { symlinkDirectRootDependency } from './symlinkDirectRootDependency.js' export async function symlinkDependency ( @@ -10,7 +10,7 @@ export async function symlinkDependency ( destModulesDir: string, importAs: string ): Promise<{ reused: boolean, warn?: string }> { - const link = path.join(destModulesDir, importAs) + const link = safeJoinModulesDir(destModulesDir, importAs) linkLogger.debug({ target: dependencyRealLocation, link }) return symlinkDir(dependencyRealLocation, link) } @@ -20,7 +20,7 @@ export function symlinkDependencySync ( destModulesDir: string, importAs: string ): { reused: boolean, warn?: string } { - const link = path.join(destModulesDir, importAs) + const link = safeJoinModulesDir(destModulesDir, importAs) linkLogger.debug({ target: dependencyRealLocation, link }) return symlinkDirSync(dependencyRealLocation, link) } diff --git a/fs/symlink-dependency/src/safeJoinModulesDir.ts b/fs/symlink-dependency/src/safeJoinModulesDir.ts new file mode 100644 index 0000000000..5017b363bc --- /dev/null +++ b/fs/symlink-dependency/src/safeJoinModulesDir.ts @@ -0,0 +1,19 @@ +import path from 'node:path' + +// `path.join(modulesDir, alias)` paired with a containment check, so a +// caller can't accidentally use the joined path without verifying that +// it lives inside `modulesDir`. Earlier passes reject path-traversal +// aliases at manifest-read time, but this layer also runs for paths +// reconstructed from lockfiles and snapshots, so the check stays here +// as a final guarantee. +export function safeJoinModulesDir (modulesDir: string, alias: string): string { + const link = path.join(modulesDir, alias) + const resolvedDir = path.resolve(modulesDir) + const resolvedLink = path.resolve(link) + if (resolvedLink === resolvedDir || !resolvedLink.startsWith(resolvedDir + path.sep)) { + const error = new Error(`Refusing to symlink dependency outside ${modulesDir}: alias ${JSON.stringify(alias)} resolves to ${resolvedLink}`) as Error & { code: string } + error.code = 'ERR_PNPM_INVALID_DEPENDENCY_NAME' + throw error + } + return link +} diff --git a/fs/symlink-dependency/src/symlinkDirectRootDependency.ts b/fs/symlink-dependency/src/symlinkDirectRootDependency.ts index 57703a9818..808259ec27 100644 --- a/fs/symlink-dependency/src/symlinkDirectRootDependency.ts +++ b/fs/symlink-dependency/src/symlinkDirectRootDependency.ts @@ -1,5 +1,4 @@ import { promises as fs } from 'node:fs' -import path from 'node:path' import util from 'node:util' import { @@ -9,6 +8,8 @@ import { import type { DependenciesField } from '@pnpm/types' import { symlinkDir } from 'symlink-dir' +import { safeJoinModulesDir } from './safeJoinModulesDir.js' + const DEP_TYPE_BY_DEPS_FIELD_NAME = { dependencies: 'prod', devDependencies: 'dev', @@ -45,7 +46,7 @@ export async function symlinkDirectRootDependency ( } } - const dest = path.join(destModulesDirReal, importAs) + const dest = safeJoinModulesDir(destModulesDirReal, importAs) const { reused } = await symlinkDir(dependencyLocation, dest) if (reused) return // if the link was already present, don't log rootLogger.debug({ diff --git a/fs/symlink-dependency/test/safeJoinModulesDir.test.ts b/fs/symlink-dependency/test/safeJoinModulesDir.test.ts new file mode 100644 index 0000000000..edbda76131 --- /dev/null +++ b/fs/symlink-dependency/test/safeJoinModulesDir.test.ts @@ -0,0 +1,36 @@ +import fs from 'node:fs' +import path from 'node:path' + +import { expect, test } from '@jest/globals' +import { symlinkDependency, symlinkDependencySync, symlinkDirectRootDependency } from '@pnpm/fs.symlink-dependency' +import { tempDir } from '@pnpm/prepare' + +const escapeAliases = ['@x/../../../etc', '../sibling', '', '.'] + +test.each(escapeAliases)('symlinkDependency refuses alias %p', async (alias) => { + const tmp = tempDir(false) + const destModulesDir = path.join(tmp, 'node_modules') + fs.mkdirSync(destModulesDir) + await expect( + symlinkDependency(path.join(tmp, 'dep'), destModulesDir, alias) + ).rejects.toThrow(expect.objectContaining({ code: 'ERR_PNPM_INVALID_DEPENDENCY_NAME' })) +}) + +test.each(escapeAliases)('symlinkDependencySync refuses alias %p', (alias) => { + const tmp = tempDir(false) + const destModulesDir = path.join(tmp, 'node_modules') + fs.mkdirSync(destModulesDir) + expect(() => { + symlinkDependencySync(path.join(tmp, 'dep'), destModulesDir, alias) + }).toThrow(expect.objectContaining({ code: 'ERR_PNPM_INVALID_DEPENDENCY_NAME' })) +}) + +test.each(escapeAliases)('symlinkDirectRootDependency refuses alias %p', async (alias) => { + const tmp = tempDir(false) + const destModulesDir = path.join(tmp, 'node_modules') + fs.mkdirSync(destModulesDir) + await expect(symlinkDirectRootDependency(path.join(tmp, 'dep'), destModulesDir, alias, { + linkedPackage: { name: 'dep', version: '1.0.0' }, + prefix: '', + })).rejects.toThrow(expect.objectContaining({ code: 'ERR_PNPM_INVALID_DEPENDENCY_NAME' })) +}) diff --git a/installing/deps-resolver/package.json b/installing/deps-resolver/package.json index 0b29a85eeb..b29ffa569b 100644 --- a/installing/deps-resolver/package.json +++ b/installing/deps-resolver/package.json @@ -70,6 +70,7 @@ "safe-promise-defer": "catalog:", "semver": "catalog:", "semver-range-intersect": "catalog:", + "validate-npm-package-name": "catalog:", "version-selector-type": "catalog:" }, "peerDependencies": { @@ -81,7 +82,8 @@ "@pnpm/logger": "workspace:*", "@types/normalize-path": "catalog:", "@types/ramda": "catalog:", - "@types/semver": "catalog:" + "@types/semver": "catalog:", + "@types/validate-npm-package-name": "catalog:" }, "engines": { "node": ">=22.13" diff --git a/installing/deps-resolver/src/getNonDevWantedDependencies.ts b/installing/deps-resolver/src/getNonDevWantedDependencies.ts index d8ac98f539..611aae2d25 100644 --- a/installing/deps-resolver/src/getNonDevWantedDependencies.ts +++ b/installing/deps-resolver/src/getNonDevWantedDependencies.ts @@ -1,6 +1,8 @@ import type { Dependencies, DependenciesMeta, DependencyManifest } from '@pnpm/types' import { pickBy } from 'ramda' +import { assertValidDependencyAliases } from './validateDependencyAlias.js' + export interface WantedDependency { alias: string bareSpecifier: string // package reference @@ -10,9 +12,17 @@ export interface WantedDependency { saveCatalogName?: string } -type GetNonDevWantedDependenciesManifest = Pick +type GetNonDevWantedDependenciesManifest = Pick & { + name?: string + version?: string +} export function getNonDevWantedDependencies (pkg: GetNonDevWantedDependenciesManifest): WantedDependency[] { + const pkgDescription = pkg.name != null + ? `Package "${pkg.name}${pkg.version != null ? `@${pkg.version}` : ''}"` + : 'Package' + assertValidDependencyAliases(pkg.dependencies, pkgDescription) + assertValidDependencyAliases(pkg.optionalDependencies, pkgDescription) let bd = pkg.bundledDependencies ?? pkg.bundleDependencies if (bd === true) { bd = pkg.dependencies != null ? Object.keys(pkg.dependencies) : [] diff --git a/installing/deps-resolver/src/getWantedDependencies.ts b/installing/deps-resolver/src/getWantedDependencies.ts index 56f7aa9b2f..12832cb59a 100644 --- a/installing/deps-resolver/src/getWantedDependencies.ts +++ b/installing/deps-resolver/src/getWantedDependencies.ts @@ -6,6 +6,8 @@ import type { ProjectManifest, } from '@pnpm/types' +import { assertValidDependencyAliases } from './validateDependencyAlias.js' + export interface WantedDependency { alias: string bareSpecifier: string // package reference @@ -23,6 +25,10 @@ export function getWantedDependencies ( includeDirect?: IncludedDependencies } ): WantedDependency[] { + assertValidDependencyAliases(pkg.dependencies, 'The current package') + assertValidDependencyAliases(pkg.devDependencies, 'The current package') + assertValidDependencyAliases(pkg.optionalDependencies, 'The current package') + assertValidDependencyAliases(pkg.peerDependencies, 'The current package') let depsToInstall = filterDependenciesByType(pkg, opts?.includeDirect ?? { dependencies: true, diff --git a/installing/deps-resolver/src/validateDependencyAlias.ts b/installing/deps-resolver/src/validateDependencyAlias.ts new file mode 100644 index 0000000000..e23cbfcde9 --- /dev/null +++ b/installing/deps-resolver/src/validateDependencyAlias.ts @@ -0,0 +1,31 @@ +import { PnpmError } from '@pnpm/error' +import validateNpmPackageName from 'validate-npm-package-name' + +// An alias is the directory name pnpm creates inside `node_modules`, so +// it must be a valid npm package name. Anything else (path-traversal +// shapes such as `@x/../../../../../.git/hooks`, control characters, +// names that collide with pnpm's own `node_modules` layout such as +// `.bin` / `.pnpm` / `node_modules`) is rejected. Matches the +// `validForOldPackages` check `parseWantedDependency` applies to +// CLI-given names. +export function isValidDependencyAlias (alias: string): boolean { + return typeof alias === 'string' && validateNpmPackageName(alias).validForOldPackages +} + +export function assertValidDependencyAliases ( + deps: Record | undefined, + parentPkgDescription: string +): void { + if (deps == null) return + for (const alias of Object.keys(deps)) { + if (!isValidDependencyAlias(alias)) { + throw new PnpmError( + 'INVALID_DEPENDENCY_NAME', + `${parentPkgDescription} contains a dependency with an invalid name: ${JSON.stringify(alias)}`, + { + hint: 'A dependency name must be a valid npm package name — a single `name` or `@scope/name` consisting of URL-friendly characters, with no leading `.` or `_`, and not equal to reserved names such as `node_modules`.', + } + ) + } + } +} diff --git a/installing/deps-resolver/test/validateDependencyAlias.test.ts b/installing/deps-resolver/test/validateDependencyAlias.test.ts new file mode 100644 index 0000000000..c161db58e4 --- /dev/null +++ b/installing/deps-resolver/test/validateDependencyAlias.test.ts @@ -0,0 +1,67 @@ +import { expect, test } from '@jest/globals' + +import { assertValidDependencyAliases, isValidDependencyAlias } from '../lib/validateDependencyAlias.js' + +test.each([ + ['foo'], + ['Foo'], + ['@scope/name'], + ['@s/x'], + ['lodash.merge'], + ['a_b'], + ['a-b'], + ['x'], + ['underscore'], +])('accepts %p', (alias) => { + expect(isValidDependencyAlias(alias)).toBe(true) +}) + +test.each([ + ['', 'empty string'], + ['..', 'parent traversal'], + ['.', 'current dir'], + ['/foo', 'absolute posix'], + ['foo/bar', 'unscoped slash'], + ['@scope/name/extra', 'scoped with extra segment'], + ['@scope/../etc', 'scope with parent traversal'], + ['@x/../../../../../.git/hooks', 'PoC payload'], + ['foo\\bar', 'backslash'], + ['C:\\Windows\\System32', 'windows absolute'], + ['foo\0bar', 'null byte'], + ['scope/name', 'two segments without @'], + ['./foo', 'current dir prefix'], + ['.bin', 'leading dot (collides with pnpm .bin)'], + ['.pnpm', 'leading dot (collides with pnpm .pnpm)'], + ['_foo', 'leading underscore'], + ['node_modules', 'reserved name'], + ['favicon.ico', 'reserved name'], + [' foo ', 'leading/trailing whitespace'], + ['foo bar', 'embedded whitespace'], + ['foo?bar', 'non-url-friendly character'], +])('rejects %s (%s)', (alias) => { + expect(isValidDependencyAlias(alias)).toBe(false) +}) + +test('assertValidDependencyAliases throws ERR_PNPM_INVALID_DEPENDENCY_NAME for malicious aliases', () => { + expect(() => { + assertValidDependencyAliases({ '@x/../../../../../.git/hooks': '1.0.0' }, 'Package "bad@1.0.0"') + }).toThrow(expect.objectContaining({ + code: 'ERR_PNPM_INVALID_DEPENDENCY_NAME', + message: expect.stringContaining('Package "bad@1.0.0" contains a dependency with an invalid name'), + })) +}) + +test('assertValidDependencyAliases is a no-op for undefined and empty input', () => { + expect(() => { + assertValidDependencyAliases(undefined, 'pkg') + }).not.toThrow() + expect(() => { + assertValidDependencyAliases({}, 'pkg') + }).not.toThrow() +}) + +test('assertValidDependencyAliases is a no-op for valid aliases', () => { + expect(() => { + assertValidDependencyAliases({ foo: '1.0.0', '@scope/bar': '2.0.0' }, 'pkg') + }).not.toThrow() +}) diff --git a/pacquet/crates/resolving-deps-resolver/Cargo.toml b/pacquet/crates/resolving-deps-resolver/Cargo.toml index ce13d6adb0..42fa9f987d 100644 --- a/pacquet/crates/resolving-deps-resolver/Cargo.toml +++ b/pacquet/crates/resolving-deps-resolver/Cargo.toml @@ -11,12 +11,13 @@ license.workspace = true repository.workspace = true [dependencies] -pacquet-catalogs-resolver = { workspace = true } -pacquet-catalogs-types = { workspace = true } -pacquet-deps-path = { workspace = true } -pacquet-package-manifest = { workspace = true } -pacquet-patching = { workspace = true } -pacquet-resolving-resolver-base = { workspace = true } +pacquet-catalogs-resolver = { workspace = true } +pacquet-catalogs-types = { workspace = true } +pacquet-deps-path = { workspace = true } +pacquet-package-manifest = { workspace = true } +pacquet-patching = { workspace = true } +pacquet-resolving-parse-wanted-dependency = { workspace = true } +pacquet-resolving-resolver-base = { workspace = true } async-recursion = { workspace = true } derive_more = { workspace = true } diff --git a/pacquet/crates/resolving-deps-resolver/src/lib.rs b/pacquet/crates/resolving-deps-resolver/src/lib.rs index eb0d8930bb..8c6de3bbd3 100644 --- a/pacquet/crates/resolving-deps-resolver/src/lib.rs +++ b/pacquet/crates/resolving-deps-resolver/src/lib.rs @@ -56,6 +56,7 @@ mod resolve_dependency_tree; mod resolve_importer; mod resolve_peers; mod resolved_tree; +mod validate_dependency_alias; pub use dependencies_graph::{ DependenciesGraph, DependenciesGraphNode, MissingPeer, PeerDependencyIssue, @@ -78,6 +79,7 @@ pub use resolved_tree::{ ChildEdge, DependenciesTree, DependenciesTreeNode, DirectDep, PeerDep, ResolvedPackage, ResolvedTree, TreeChildren, }; +pub use validate_dependency_alias::is_valid_dependency_alias; #[cfg(test)] mod tests; diff --git a/pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs b/pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs index 7e3745c400..261349e846 100644 --- a/pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs +++ b/pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs @@ -102,6 +102,20 @@ pub enum ResolveDependencyTreeError { specifier: String, resolved_via: String, }, + + /// A dependency alias contained a path-separator segment that would + /// escape the intended `node_modules` directory when joined onto a + /// modules path. Mirrors pnpm's + /// [`INVALID_DEPENDENCY_NAME`](https://github.com/pnpm/pnpm/blob/main/installing/deps-resolver/src/validateDependencyAlias.ts). + #[display( + "{parent} contains a dependency with an invalid name: {alias:?}. Dependency names must be a single package name or \"@scope/name\" — they cannot contain path-separator segments such as \"..\"." + )] + #[diagnostic(code(INVALID_DEPENDENCY_NAME))] + InvalidDependencyName { + #[error(not(source))] + parent: String, + alias: String, + }, } impl From for ResolveDependencyTreeError { @@ -143,13 +157,17 @@ where { let ctx = TreeCtx::new(opts.base_opts).with_patched_dependencies(opts.patched_dependencies); let optional_names = importer_optional_dependency_names(manifest); - let wanted: Vec<(String, String, bool)> = manifest - .dependencies(dependency_groups) - .map(|(name, range)| { - let optional = optional_names.contains(name); - (name.to_string(), range.to_string(), optional) - }) - .collect(); + let mut wanted: Vec<(String, String, bool)> = Vec::new(); + for (name, range) in manifest.dependencies(dependency_groups) { + if !crate::is_valid_dependency_alias(name) { + return Err(ResolveDependencyTreeError::InvalidDependencyName { + parent: "The current package".to_string(), + alias: name.to_string(), + }); + } + let optional = optional_names.contains(name); + wanted.push((name.to_string(), range.to_string(), optional)); + } let direct = extend_tree(&ctx, resolver, wanted).await?; Ok(ctx.into_resolved_tree(direct)) } @@ -580,7 +598,7 @@ where let child_specs = match child_specs { Some(specs) => specs, None => { - let specs = Arc::new(extract_children(&result)); + let specs = Arc::new(extract_children(&result)?); lock_recoverable(&ctx.children_specs_by_id) .entry(id.clone()) .or_insert_with(|| Arc::clone(&specs)); @@ -770,21 +788,45 @@ fn render_specifier(wanted: &WantedDependency) -> String { /// `optionalDependencies`. The walker propagates this through /// `current_is_optional` so [`ResolvedPackage::optional`] reflects /// whether every path to the node went through an optional edge. -fn extract_children(result: &pacquet_resolving_resolver_base::ResolveResult) -> Vec { - let Some(manifest) = result.manifest.as_ref() else { return Vec::new() }; +fn extract_children( + result: &pacquet_resolving_resolver_base::ResolveResult, +) -> Result, ResolveDependencyTreeError> { + let Some(manifest) = result.manifest.as_ref() else { return Ok(Vec::new()) }; + let parent = render_parent(result); let mut out = Vec::new(); - collect_deps(manifest, "dependencies", false, &mut out); - collect_deps(manifest, "optionalDependencies", true, &mut out); - out + collect_deps(manifest, "dependencies", false, &parent, &mut out)?; + collect_deps(manifest, "optionalDependencies", true, &parent, &mut out)?; + Ok(out) } -fn collect_deps(manifest: &Value, key: &str, optional: bool, out: &mut Vec) { - let Some(map) = manifest.get(key).and_then(Value::as_object) else { return }; +fn collect_deps( + manifest: &Value, + key: &str, + optional: bool, + parent: &str, + out: &mut Vec, +) -> Result<(), ResolveDependencyTreeError> { + let Some(map) = manifest.get(key).and_then(Value::as_object) else { return Ok(()) }; for (name, range) in map { if let Some(range_str) = range.as_str() { + if !crate::is_valid_dependency_alias(name) { + return Err(ResolveDependencyTreeError::InvalidDependencyName { + parent: parent.to_string(), + alias: name.clone(), + }); + } out.push((name.clone(), range_str.to_string(), optional)); } } + Ok(()) +} + +fn render_parent(result: &pacquet_resolving_resolver_base::ResolveResult) -> String { + if let Some(name_ver) = result.name_ver.as_ref() { + format!("Package \"{}@{}\"", name_ver.name, name_ver.suffix) + } else { + format!("Package \"{}\"", result.id) + } } /// Extract `peerDependencies` from a resolved package's manifest, with diff --git a/pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs b/pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs index 6cfe2a6a44..506faeb04a 100644 --- a/pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs +++ b/pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs @@ -155,13 +155,19 @@ where // direct dep with the right `wanted.optional` flag for the // `ResolvedPackage.optional` propagation. let optional_names = importer_optional_dependency_names(manifest); - let initial_wanted: Vec<(String, String, bool)> = manifest - .dependencies(groups) - .map(|(name, range)| { - let optional = optional_names.contains(name); - (name.to_string(), range.to_string(), optional) - }) - .collect(); + let mut initial_wanted: Vec<(String, String, bool)> = Vec::new(); + for (name, range) in manifest.dependencies(groups) { + if !crate::is_valid_dependency_alias(name) { + return Err(ResolveImporterError::Resolve( + ResolveDependencyTreeError::InvalidDependencyName { + parent: "The current package".to_string(), + alias: name.to_string(), + }, + )); + } + let optional = optional_names.contains(name); + initial_wanted.push((name.to_string(), range.to_string(), optional)); + } let initial_wanted = resolve_catalog_specifiers(initial_wanted, &catalogs)?; let mut direct = extend_tree(&ctx, resolver, initial_wanted).await?; update_preferred_versions_with_ctx(&ctx, &mut all_preferred_versions); diff --git a/pacquet/crates/resolving-deps-resolver/src/tests.rs b/pacquet/crates/resolving-deps-resolver/src/tests.rs index faa0fd2285..e9370fff99 100644 --- a/pacquet/crates/resolving-deps-resolver/src/tests.rs +++ b/pacquet/crates/resolving-deps-resolver/src/tests.rs @@ -317,6 +317,52 @@ async fn declined_specifier_surfaces_spec_not_supported_error() { } } +/// A transitive dependency whose alias contains `..` segments would +/// escape the `node_modules` directory when joined onto a modules +/// path. The walker rejects it before any further resolution work. +/// Mirrors the pnpm-side fix in +/// `installing/deps-resolver/src/validateDependencyAlias.ts`. +#[tokio::test] +async fn transitive_dep_with_traversal_alias_is_rejected() { + let mut table = HashMap::new(); + table.insert( + ("normal".to_string(), "1.0.0".to_string()), + fake_result( + "normal", + "1.0.0", + serde_json::json!({ + "name": "normal", + "version": "1.0.0", + "dependencies": { "@x/../../../../../.git/hooks": "1.0.0" }, + }), + ), + ); + let resolver = StubResolver { table, calls: Mutex::new(Vec::new()) }; + let (_tmp, manifest) = fake_manifest(serde_json::json!({ "normal": "1.0.0" })); + + let err = resolve_dependency_tree( + &resolver, + &manifest, + [DependencyGroup::Prod], + ResolveDependencyTreeOptions { + base_opts: ResolveOptions::default(), + patched_dependencies: None, + }, + ) + .await + .expect_err("traversal alias must error"); + match err { + ResolveDependencyTreeError::InvalidDependencyName { parent, alias } => { + assert_eq!(alias, "@x/../../../../../.git/hooks"); + assert!( + parent.contains("normal"), + "parent must name the offending package, got {parent:?}", + ); + } + other => panic!("expected InvalidDependencyName, got {other:?}"), + } +} + mod block_exotic_subdeps { use std::collections::HashMap; use std::sync::Mutex; diff --git a/pacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rs b/pacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rs new file mode 100644 index 0000000000..19d3c0a965 --- /dev/null +++ b/pacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rs @@ -0,0 +1,60 @@ +//! Reject dependency aliases that aren't valid npm package names — +//! anything else, joined with a `node_modules` path, can either escape +//! the intended directory (`@x/../../../../../.git/hooks`) or collide +//! with pnpm's own layout (`.bin`, `.pnpm`, `node_modules`). Mirrors +//! pnpm's +//! [`isValidDependencyAlias`](https://github.com/pnpm/pnpm/blob/main/installing/deps-resolver/src/validateDependencyAlias.ts), +//! which routes through the same `validate-npm-package-name` +//! `validForOldPackages` check that `parse_wanted_dependency` applies +//! to CLI-given names. + +use pacquet_resolving_parse_wanted_dependency::is_valid_old_npm_package_name; + +/// `true` when `alias` is a valid npm package name that pnpm can safely +/// use as a `node_modules/` directory. +pub fn is_valid_dependency_alias(alias: &str) -> bool { + is_valid_old_npm_package_name(alias) +} + +#[cfg(test)] +mod tests { + use super::is_valid_dependency_alias; + + #[test] + fn accepts_valid_aliases() { + for alias in + ["foo", "Foo", "@scope/name", "@s/x", "lodash.merge", "a_b", "a-b", "underscore"] + { + assert!(is_valid_dependency_alias(alias), "expected valid: {alias:?}"); + } + } + + #[test] + fn rejects_invalid_aliases() { + for alias in [ + "", + "..", + ".", + "/foo", + "foo/bar", + "@scope/name/extra", + "@scope/../etc", + "@x/../../../../../.git/hooks", + r"foo\bar", + r"C:\Windows\System32", + "foo\0bar", + "scope/name", + "./foo", + ".bin", + ".pnpm", + "_foo", + "node_modules", + "favicon.ico", + " foo ", + "foo bar", + "foo?bar", + ] { + assert!(!is_valid_dependency_alias(alias), "expected invalid: {alias:?}"); + } + } +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 5295b86a15..01be27275a 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -6063,6 +6063,9 @@ importers: semver-range-intersect: specifier: 'catalog:' version: 0.3.1 + validate-npm-package-name: + specifier: 'catalog:' + version: 7.0.2 version-selector-type: specifier: 'catalog:' version: 3.0.0 @@ -6085,6 +6088,9 @@ importers: '@types/semver': specifier: 'catalog:' version: 7.7.1 + '@types/validate-npm-package-name': + specifier: 'catalog:' + version: 4.0.2 installing/deps-restorer: dependencies: