diff --git a/.changeset/contain-virtual-store-package-name.md b/.changeset/contain-virtual-store-package-name.md new file mode 100644 index 0000000000..b21f1b46d0 --- /dev/null +++ b/.changeset/contain-virtual-store-package-name.md @@ -0,0 +1,8 @@ +--- +"@pnpm/deps.graph-builder": patch +"@pnpm/deps.graph-hasher": patch +"@pnpm/lockfile.to-pnp": patch +"pnpm": patch +--- + +Prevent a crafted `pnpm-lock.yaml` from writing package content outside the virtual store. A dependency path key whose name reconstructs to a path-traversal sequence (e.g. `../../../tmp/x@1.0.0`) is now rejected by the isolated (virtual-store) linker and the Plug'n'Play resolver map, matching the containment already applied to the hoisted linker. Under the global virtual store, a traversal in the version-derived path segment (e.g. a snapshot `version: "../../x"`) is now rejected at `formatGlobalVirtualStorePath`, the single point every global-virtual-store slot path funnels through — closing the same escape in the isolated linker, the resolver's dependency-graph builder, and the config-dependency installer. diff --git a/.changeset/isolated-linker-name-traversal.md b/.changeset/isolated-linker-name-traversal.md new file mode 100644 index 0000000000..9d2314156b --- /dev/null +++ b/.changeset/isolated-linker-name-traversal.md @@ -0,0 +1,8 @@ +--- +"@pnpm/installing.deps-resolver": patch +"@pnpm/deps.graph-builder": patch +"@pnpm/installing.env-installer": patch +"pnpm": patch +--- + +Fixed a path traversal vulnerability where a dependency whose manifest `name` was a scoped path traversal (e.g. `@x/../../../`) could be written outside `node_modules` to an attacker-controlled location during `pnpm install`, even with `--ignore-scripts`. The isolated linker now validates the package name before using it as a directory name, matching the existing protection in the hoisted linker. diff --git a/pacquet/crates/cli/tests/install.rs b/pacquet/crates/cli/tests/install.rs index a122ed36a5..4fff33f35a 100644 --- a/pacquet/crates/cli/tests/install.rs +++ b/pacquet/crates/cli/tests/install.rs @@ -52,6 +52,52 @@ fn should_install_dependencies() { drop((root, mock_instance)); } +/// A project manifest that declares a dependency under a path-traversal +/// name is rejected by the resolver on a fresh install — before any +/// resolution or fetch, and long before the name could become a +/// `node_modules/` directory. This is the fresh-resolve +/// counterpart to the frozen-lockfile name check. Surfaces +/// `ERR_PNPM_INVALID_DEPENDENCY_NAME`. +#[test] +fn install_rejects_a_traversal_dependency_name_in_the_manifest() { + let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + let manifest_path = workspace.join("package.json"); + let package_json_content = serde_json::json!({ + "dependencies": { + "../../escaped-link": "1.0.0", + }, + }); + fs::write(&manifest_path, package_json_content.to_string()).expect("write to package.json"); + + let output = pacquet.with_arg("install").output().expect("spawn pacquet install"); + assert!( + !output.status.success(), + "the resolver must reject a traversal dependency name (stderr: {})", + String::from_utf8_lossy(&output.stderr), + ); + // The fresh-resolve path forwards the resolver's diagnostic + // transparently, so the rendered envelope carries the canonical + // `ERR_PNPM_INVALID_DEPENDENCY_NAME` code — matching the frozen path + // (see `lockfile_verification.rs`). The offending name is in the + // message too, but miette may wrap it across lines at narrow widths, + // so assert on the stable code and the unwrapped "invalid name" + // phrase instead. + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!( + stderr.contains("ERR_PNPM_INVALID_DEPENDENCY_NAME") && stderr.contains("invalid name"), + "stderr must report the invalid-dependency-name code; got:\n{stderr}", + ); + assert!( + !workspace.parent().is_some_and(|parent| parent.join("escaped-link").exists()), + "no link may be created outside the project", + ); + + drop((root, mock_instance)); +} + #[test] fn should_install_exec_files() { let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = diff --git a/pacquet/crates/cli/tests/lockfile_verification.rs b/pacquet/crates/cli/tests/lockfile_verification.rs index f3f7e9b017..bc7e360580 100644 --- a/pacquet/crates/cli/tests/lockfile_verification.rs +++ b/pacquet/crates/cli/tests/lockfile_verification.rs @@ -226,3 +226,83 @@ fn trust_lockfile_cli_flag_skips_verification() { drop((root, mock_instance)); } + +/// Regression test for the crafted-lockfile path-traversal advisory +/// (GHSA-2rx9-3g3h-c2jv). A dependency name/alias that isn't a valid npm +/// package name — here a `../../escaped-link` snapshot key — would be +/// joined into a filesystem path at install time and escape the project. +/// The dependency-name check must run **even under `--trust-lockfile`**, +/// which disables the resolution-policy verification fan-out: the two +/// tests above prove that flag skips the *policy* gate, so this test pins +/// the complementary contract that the *structural* name check is not +/// skipped with it. The install must fail with +/// `ERR_PNPM_INVALID_DEPENDENCY_NAME` before any virtual-store slot is +/// materialized. +#[test] +fn trust_lockfile_still_rejects_traversal_dependency_name() { + let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + // A legit direct dependency keeps the frozen-lockfile freshness + // check happy; the crafted entry is an extra `snapshots:` / + // `packages:` key whose name is a path traversal. The verifier + // walks every snapshot, so an unreferenced hostile key is still + // rejected. + let manifest_path = workspace.join("package.json"); + let package_json = serde_json::json!({ + "dependencies": { + "@pnpm.e2e/hello-world-js-bin": "1.0.0", + }, + }); + fs::write(&manifest_path, package_json.to_string()).expect("write package.json"); + + let lockfile = "lockfileVersion: '9.0'\n\ + importers:\n \ + .:\n \ + dependencies:\n \ + '@pnpm.e2e/hello-world-js-bin':\n \ + specifier: 1.0.0\n \ + version: 1.0.0\n\ + packages:\n \ + '@pnpm.e2e/hello-world-js-bin@1.0.0':\n \ + resolution: {integrity: sha512-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA==}\n \ + '../../escaped-link@1.0.0':\n \ + resolution: {integrity: sha512-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA==}\n\ + snapshots:\n \ + '@pnpm.e2e/hello-world-js-bin@1.0.0': {}\n \ + '../../escaped-link@1.0.0': {}\n"; + fs::write(workspace.join("pnpm-lock.yaml"), lockfile).expect("write lockfile"); + + let output = pacquet + .with_args(["install", "--frozen-lockfile", "--trust-lockfile"]) + .output() + .expect("spawn pacquet install"); + + assert!( + !output.status.success(), + "the name check must reject the install even under --trust-lockfile (stderr: {})", + String::from_utf8_lossy(&output.stderr), + ); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!( + stderr.contains("ERR_PNPM_INVALID_DEPENDENCY_NAME"), + "stderr must name the invalid-dependency-name code; got:\n{stderr}", + ); + // The rejection happens before materialization, so nothing is + // extracted — neither the legit slot nor any escaped directory. + assert!( + !workspace.join("node_modules/.pnpm").exists(), + "the check must fail before any virtual-store materialization", + ); + // Belt-and-suspenders: the traversal target of the crafted alias + // (`/node_modules/../../escaped-link`) must not exist. + if let Some(parent) = workspace.parent() { + assert!( + !parent.join("escaped-link").exists(), + "no link may be created outside the project", + ); + } + + drop((root, mock_instance)); +} diff --git a/pacquet/crates/lockfile-verification/src/lib.rs b/pacquet/crates/lockfile-verification/src/lib.rs index ac4fd140bd..441b9ece0b 100644 --- a/pacquet/crates/lockfile-verification/src/lib.rs +++ b/pacquet/crates/lockfile-verification/src/lib.rs @@ -11,6 +11,7 @@ //! JSONL stat-and-skip cache. //! //! Public surface today: [`verify_lockfile_resolutions()`], +//! [`verify_lockfile_dependency_names()`], //! [`collect_resolution_policy_violations()`], [`hash_lockfile()`], //! [`VerifyError`], and [`RenderedViolation`] — the last lets a caller //! that resolved violations out-of-process (e.g. the pnpr client @@ -35,5 +36,6 @@ pub use hash_lockfile::hash_lockfile; pub use record_lockfile_verified::record_lockfile_verified; pub use verify_lockfile_resolutions::{ RESOLUTION_SHAPE_MISMATCH_VIOLATION_CODE, VerifyLockfileResolutionsOptions, - collect_resolution_policy_violations, verify_lockfile_resolutions, + collect_resolution_policy_violations, verify_lockfile_dependency_names, + verify_lockfile_resolutions, }; diff --git a/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs b/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs index 10cdc2a983..0ac3e15803 100644 --- a/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs +++ b/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs @@ -67,6 +67,12 @@ pub async fn verify_lockfile_resolutions( verifiers: &[Arc], opts: &VerifyLockfileResolutionsOptions<'_>, ) -> Result<(), VerifyError> { + // Offline structural gate first: reject invalid dependency names + // before the `packages`-absent short-circuit and the cache lookup, + // so a lockfile that carries a path-traversal alias but no + // `packages:` section (e.g. only `link:` deps) is still rejected. + verify_lockfile_dependency_names(lockfile)?; + if lockfile.packages.is_none() { return Ok(()); } @@ -123,10 +129,7 @@ pub async fn verify_lockfile_resolutions( cache_precomputed = result.precomputed; } - let (candidates, shape_violations, invalid_aliases) = collect_candidates(lockfile); - if !invalid_aliases.is_empty() { - return Err(VerifyError::invalid_dependency_aliases(&invalid_aliases)); - } + let (candidates, shape_violations) = collect_candidates(lockfile); if !shape_violations.is_empty() { return Err(build_verification_error(shape_violations)); } @@ -205,7 +208,7 @@ pub async fn collect_resolution_policy_violations( // Shape violations and invalid aliases are deliberately not // collected here: they are hard tampering failures, not policy // picks a caller may auto-exclude. - let (candidates, _shape_violations, _invalid_aliases) = collect_candidates(lockfile); + let (candidates, _shape_violations) = collect_candidates(lockfile); // `Err(message)` is a transport failure the caller must surface rather than // treat as "no violations" — see [`run_fan_out`]. run_fan_out(candidates, verifiers, concurrency).await @@ -326,6 +329,61 @@ fn push_invalid_aliases<'alias>( } } +/// Collect every lockfile-controlled dependency name that isn't a valid +/// npm package name. Three sources, each of which becomes a +/// `node_modules/` path component at install time: +/// +/// - every importer's direct-dependency aliases, +/// - every snapshot's own package name (the `snapshots:` map key), and +/// - every snapshot's `dependencies` / `optionalDependencies` aliases. +/// +/// A name carrying a `..` traversal, a `/`, or a reserved value such as +/// `node_modules` / `.bin` / `.pnpm` could make an install write outside +/// the intended directory or overwrite pnpm-owned layout. +fn collect_invalid_dependency_names(lockfile: &Lockfile) -> std::collections::BTreeSet { + let mut invalid = std::collections::BTreeSet::new(); + for importer in lockfile.importers.values() { + for deps in + [&importer.dependencies, &importer.dev_dependencies, &importer.optional_dependencies] + { + push_invalid_aliases(deps.iter().flatten().map(|(alias, _)| alias), &mut invalid); + } + } + if let Some(snapshots) = lockfile.snapshots.as_ref() { + for (key, snapshot) in snapshots { + push_invalid_aliases(std::iter::once(&key.name), &mut invalid); + for deps in [&snapshot.dependencies, &snapshot.optional_dependencies] { + push_invalid_aliases(deps.iter().flatten().map(|(alias, _)| alias), &mut invalid); + } + } + } + invalid +} + +/// Reject a lockfile whose dependency names or aliases are not valid npm +/// package names. +/// +/// This is an offline, network-free structural check — it does **not** +/// depend on the resolution-policy verifiers, so it must run on every +/// install, **including under `trustLockfile`**, which disables the +/// policy fan-out. A crafted lockfile alias becomes a filesystem path +/// (`node_modules/`, a virtual-store slot's inner +/// `node_modules/`, a bin, a hoisted link), and pacquet's +/// [`PkgName`] parser does not validate against npm's package-name +/// rules, so an unchecked `..`/`/`-bearing name escapes the intended +/// directory. +/// +/// Surfaces [`VerifyError::InvalidDependencyAlias`] +/// (`ERR_PNPM_INVALID_DEPENDENCY_NAME`) listing every offender. +pub fn verify_lockfile_dependency_names(lockfile: &Lockfile) -> Result<(), VerifyError> { + let invalid = collect_invalid_dependency_names(lockfile); + if invalid.is_empty() { + return Ok(()); + } + let invalid: Vec = invalid.into_iter().collect(); + Err(VerifyError::invalid_dependency_aliases(&invalid)) +} + /// One `(name, version, resolution)` tuple deduplicated from /// `lockfile.packages`. struct Candidate { @@ -343,36 +401,10 @@ struct Candidate { /// `BTreeMap` over a serialized key gives deterministic iteration /// order for tests; the fan-out runs across the value iter so order /// doesn't affect correctness, only the reproducibility of failures. -fn collect_candidates( - lockfile: &Lockfile, -) -> (Vec, Vec, Vec) { +fn collect_candidates(lockfile: &Lockfile) -> (Vec, Vec) { let Some(packages) = lockfile.packages.as_ref() else { - return (Vec::new(), Vec::new(), Vec::new()); + return (Vec::new(), Vec::new()); }; - // Pacquet keeps the alias-bearing maps in `importers` / `snapshots`, - // separate from the `packages` metadata the loop below walks, so - // they're scanned here in the same pass. - let mut invalid_aliases: std::collections::BTreeSet = std::collections::BTreeSet::new(); - for importer in lockfile.importers.values() { - for deps in - [&importer.dependencies, &importer.dev_dependencies, &importer.optional_dependencies] - { - push_invalid_aliases( - deps.iter().flatten().map(|(alias, _)| alias), - &mut invalid_aliases, - ); - } - } - if let Some(snapshots) = lockfile.snapshots.as_ref() { - for snapshot in snapshots.values() { - for deps in [&snapshot.dependencies, &snapshot.optional_dependencies] { - push_invalid_aliases( - deps.iter().flatten().map(|(alias, _)| alias), - &mut invalid_aliases, - ); - } - } - } let mut deduped: BTreeMap = BTreeMap::new(); let mut shape_violations = Vec::new(); for (key, metadata) in packages { @@ -412,7 +444,7 @@ fn collect_candidates( resolution: metadata.resolution.clone(), }); } - (deduped.into_values().collect(), shape_violations, invalid_aliases.into_iter().collect()) + (deduped.into_values().collect(), shape_violations) } /// Run every active verifier against every candidate with a diff --git a/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rs b/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rs index c715aa39be..5836c34ade 100644 --- a/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rs +++ b/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rs @@ -883,6 +883,66 @@ snapshots: assert!(matches!(err, VerifyError::InvalidDependencyAlias { .. }), "got {err:?}"); } +#[tokio::test] +async fn rejects_a_traversal_snapshot_package_name() { + // The snapshot's own package name (the `snapshots:` key) becomes + // `/node_modules/` at extract time — a sink the alias + // scan of the dependency maps alone doesn't cover. + let lockfile = parse( + "lockfileVersion: '9.0' + +packages: + + ../../../escape@1.0.0: + resolution: {integrity: sha512-deadbeef} + +snapshots: + + ../../../escape@1.0.0: {} +", + ); + let err = verify_lockfile_resolutions::( + &lockfile, + &[], + &VerifyLockfileResolutionsOptions::default(), + ) + .await + .expect_err("a traversal snapshot package name must be rejected"); + let VerifyError::InvalidDependencyAlias { breakdown, .. } = err else { + panic!("expected InvalidDependencyAlias, got {err:?}"); + }; + assert!(breakdown.contains("../../../escape"), "breakdown {breakdown:?}"); +} + +#[test] +fn verify_lockfile_dependency_names_rejects_a_packages_less_lockfile() { + // A lockfile with only `link:` deps has no `packages:` section, so + // the resolution fan-out short-circuits — but the offline name check + // must still reject a traversal importer alias. + let lockfile = parse( + "lockfileVersion: '9.0' + +importers: + + .: + dependencies: + '../../escaped-link': + specifier: link:./local + version: link:local +", + ); + assert!(lockfile.packages.is_none(), "fixture must have no packages section"); + let err = super::verify_lockfile_dependency_names(&lockfile) + .expect_err("a traversal alias in a packages-less lockfile must be rejected"); + assert!(matches!(err, VerifyError::InvalidDependencyAlias { .. }), "got {err:?}"); +} + +#[test] +fn verify_lockfile_dependency_names_accepts_a_clean_lockfile() { + super::verify_lockfile_dependency_names(&parse(TWO_PKG_LOCKFILE)) + .expect("a lockfile with valid names must pass"); +} + #[tokio::test] async fn accepts_valid_scoped_and_unscoped_aliases() { let lockfile = parse( diff --git a/pacquet/crates/package-manager/src/create_symlink_layout.rs b/pacquet/crates/package-manager/src/create_symlink_layout.rs index f5788ef65c..e25f45b115 100644 --- a/pacquet/crates/package-manager/src/create_symlink_layout.rs +++ b/pacquet/crates/package-manager/src/create_symlink_layout.rs @@ -1,4 +1,7 @@ -use crate::{SkippedSnapshots, SymlinkPackageError, VirtualStoreLayout, symlink_package}; +use crate::{ + SkippedSnapshots, SymlinkPackageError, VirtualStoreLayout, + safe_join_modules_dir::safe_join_modules_dir, symlink_package, +}; use pacquet_lockfile::{PkgName, SnapshotDepRef}; use std::{collections::HashMap, path::Path}; @@ -48,13 +51,19 @@ pub fn create_symlink_layout( if skipped.contains(&target) { return Ok(()); } - let target_name_str = target.name.to_string(); - let alias_name_str = alias_name.to_string(); - symlink_package( - &layout.slot_dir(&target).join("node_modules").join(&target_name_str), - &virtual_node_modules_dir.join(&alias_name_str), + // Both names are lockfile-derived and untrusted: `target.name` + // is the resolved package's own name and `alias_name` is the + // dependency key. A traversal-shaped name (`@x/../../...`) would + // otherwise let the symlink target or the symlink itself escape + // the slot's `node_modules`, so guard each join. + let symlink_target = safe_join_modules_dir( + &layout.slot_dir(&target).join("node_modules"), + &target.name.to_string(), ) - .map(drop) + .map_err(SymlinkPackageError::InvalidAlias)?; + let symlink_path = safe_join_modules_dir(virtual_node_modules_dir, &alias_name.to_string()) + .map_err(SymlinkPackageError::InvalidAlias)?; + symlink_package(&symlink_target, &symlink_path).map(drop) }) } diff --git a/pacquet/crates/package-manager/src/create_symlink_layout/tests.rs b/pacquet/crates/package-manager/src/create_symlink_layout/tests.rs index 1e46f7cc32..c754199de1 100644 --- a/pacquet/crates/package-manager/src/create_symlink_layout/tests.rs +++ b/pacquet/crates/package-manager/src/create_symlink_layout/tests.rs @@ -1,4 +1,4 @@ -use crate::{SkippedSnapshots, VirtualStoreLayout, create_symlink_layout}; +use crate::{SkippedSnapshots, SymlinkPackageError, VirtualStoreLayout, create_symlink_layout}; use pacquet_lockfile::{PackageKey, PkgName, SnapshotDepRef}; use pretty_assertions::assert_eq; use std::{collections::HashMap, fs, path::PathBuf}; @@ -236,3 +236,42 @@ fn alias_dep_links_under_alias_but_resolves_via_target() { .expect("compute relative target"); assert_eq!(read, expected); } + +/// A dependency alias that is a scoped path traversal +/// (`@x/../../.../OUTSIDE`) must be rejected before any symlink is +/// created, rather than escaping the slot's `node_modules`. +/// `PkgName::parse` accepts such a name (its `bare` field keeps the +/// `../` segments), so the guard has to live at the join. +#[test] +fn rejects_traversal_dependency_alias() { + let tmp = tempdir().expect("tempdir"); + let virtual_store_dir = tmp.path().to_path_buf(); + let layout = VirtualStoreLayout::legacy( + virtual_store_dir, + pacquet_config::default_virtual_store_dir_max_length() as usize, + ); + + let traversal = format!("@x/{}OUTSIDE", "../".repeat(20)); + let mut deps: HashMap = HashMap::new(); + deps.insert(pkg_name(&traversal), dep_ref("1.0.0")); + + let skipped = SkippedSnapshots::default(); + let virtual_node_modules_dir = tmp.path().join("self/node_modules"); + fs::create_dir_all(&virtual_node_modules_dir).unwrap(); + + let error = create_symlink_layout( + Some(&deps), + None, + &pkg_name("self"), + &skipped, + &layout, + &virtual_node_modules_dir, + ) + .expect_err("traversal alias must be rejected"); + assert!(matches!(error, SymlinkPackageError::InvalidAlias(_)), "got {error:?}"); + + // The guard fires before any symlink is created, so nothing was + // linked into (or out of) the slot's node_modules. + let linked = fs::read_dir(&virtual_node_modules_dir).unwrap().count(); + assert_eq!(linked, 0); +} diff --git a/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot.rs b/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot.rs index 7c3fdd2fe4..8936a77c4f 100644 --- a/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot.rs +++ b/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot.rs @@ -1,6 +1,7 @@ use crate::{ ImportIndexedDirError, ImportIndexedDirOpts, SkippedSnapshots, SymlinkPackageError, VirtualStoreLayout, create_symlink_layout, import_indexed_dir, + safe_join_modules_dir::{InvalidDependencyAliasError, safe_join_modules_dir}, }; use derive_more::{Display, Error}; use miette::Diagnostic; @@ -86,6 +87,12 @@ pub enum CreateVirtualDirError { #[diagnostic(transparent)] SymlinkPackage(#[error(source)] SymlinkPackageError), + /// The snapshot's package name is not a valid npm package name, so + /// joining it under the slot's `node_modules` could escape the + /// directory. Surfaces pnpm's `ERR_PNPM_INVALID_DEPENDENCY_NAME`. + #[diagnostic(transparent)] + InvalidAlias(#[error(source)] InvalidDependencyAliasError), + #[display("Failed to remove obsolete child link at {path:?}: {error}")] #[diagnostic(code(pacquet_package_manager::remove_obsolete_child))] RemoveObsoleteChild { @@ -125,7 +132,9 @@ impl CreateVirtualDirBySnapshot<'_> { } })?; - let save_path = virtual_node_modules_dir.join(package_key.name.to_string()); + let save_path = + safe_join_modules_dir(&virtual_node_modules_dir, &package_key.name.to_string()) + .map_err(CreateVirtualDirError::InvalidAlias)?; // `rayon::join` runs both closures in parallel on rayon's pool, // returning only once both finish. `import_indexed_dir` is itself diff --git a/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot/tests.rs b/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot/tests.rs index 75ffe5889f..8566c8c040 100644 --- a/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot/tests.rs +++ b/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot/tests.rs @@ -177,6 +177,45 @@ async fn run_emits_imported_event_after_import_indexed_dir() { ); } +/// A snapshot key whose package name is a path traversal would become +/// the `/node_modules/` extraction directory, escaping the +/// store. The guard rejects it before any package content is imported. +#[test] +fn run_rejects_traversal_package_name() { + let dir = tempdir().expect("tempdir"); + let virtual_store_dir = dir.path().to_path_buf(); + let cas_paths: HashMap = HashMap::new(); + let logged_methods = AtomicU8::new(0); + let snapshot = SnapshotEntry::default(); + let package_key: PackageKey = + "../../escaped@1.0.0".parse().expect("parse traversal snapshot key"); + + let layout = crate::VirtualStoreLayout::legacy( + virtual_store_dir, + pacquet_config::default_virtual_store_dir_max_length() as usize, + ); + let skipped = crate::SkippedSnapshots::default(); + let result = CreateVirtualDirBySnapshot { + layout: &layout, + cas_paths: &cas_paths, + import_method: PackageImportMethod::Hardlink, + logged_methods: &logged_methods, + requester: "/proj", + package_id: "../../escaped@1.0.0", + package_key: &package_key, + snapshot: &snapshot, + skipped: &skipped, + removed_aliases: &[], + link_concurrency_probe: None, + } + .run::(); + + assert!( + matches!(result, Err(crate::CreateVirtualDirError::InvalidAlias(_))), + "a traversal package name must be rejected before extraction; got {result:?}", + ); +} + /// A warm reinstall that drops a child dependency unlinks the stale /// symlink (and its now-empty `@scope` directory) while leaving the /// children it still depends on in place. diff --git a/pacquet/crates/package-manager/src/hoist.rs b/pacquet/crates/package-manager/src/hoist.rs index 8a3b672a97..aa00a101ba 100644 --- a/pacquet/crates/package-manager/src/hoist.rs +++ b/pacquet/crates/package-manager/src/hoist.rs @@ -441,6 +441,7 @@ pub fn symlink_hoisted_dependencies( public_hoisted_modules_dir: &std::path::Path, skipped: &std::collections::HashSet, ) -> Result<(), crate::SymlinkPackageError> { + use crate::safe_join_modules_dir::safe_join_modules_dir; use rayon::prelude::*; use std::{ collections::HashSet, @@ -474,8 +475,16 @@ pub fn symlink_hoisted_dependencies( continue; } let Some(node) = graph.get(node_id) else { continue }; - let dep_dir = - Arc::new(layout.slot_dir(node_id).join("node_modules").join(name_to_dir(&node.name))); + // `node.name` originates from the lockfile, so a traversal-shaped + // name is guarded here before it becomes the hoist symlink's + // `/node_modules/` target. + let dep_dir = Arc::new( + safe_join_modules_dir( + &layout.slot_dir(node_id).join("node_modules"), + &node.name.to_string(), + ) + .map_err(crate::SymlinkPackageError::InvalidAlias)?, + ); for (alias, kind) in alias_map { let target_dir_root: &Path = match kind { HoistKind::Public => public_hoisted_modules_dir, @@ -548,17 +557,6 @@ pub fn symlink_hoisted_dependencies( ) } -/// Render a [`PkgName`] into its on-disk segment under `node_modules`. -/// Scoped names land at `@scope/name`, unscoped at `name`. Mirrors the -/// helper used by the bin linker (kept private to this module to avoid -/// cross-crate dependency churn for one path-join helper). -fn name_to_dir(name: &PkgName) -> std::path::PathBuf { - match &name.scope { - Some(scope) => std::path::PathBuf::from(format!("@{scope}")).join(&name.bare), - None => std::path::PathBuf::from(&name.bare), - } -} - /// Read the existing symlink at `dest` and decide whether it should /// be replaced. If it already points at `dep_dir`, leave it untouched. /// If it points inside `package_store_dir` or `internal_pnpm_dir` diff --git a/pacquet/crates/package-manager/src/hoist/tests.rs b/pacquet/crates/package-manager/src/hoist/tests.rs index 2f236050f5..ee6620207b 100644 --- a/pacquet/crates/package-manager/src/hoist/tests.rs +++ b/pacquet/crates/package-manager/src/hoist/tests.rs @@ -393,6 +393,56 @@ fn symlink_skips_dropped_nodes() { ); } +/// A hoisted node whose own (lockfile-derived) name is a path traversal +/// would become the `/node_modules/` symlink target and +/// escape the store. `symlink_hoisted_dependencies` must reject it. +#[test] +fn symlink_rejects_traversal_node_name() { + use crate::VirtualStoreLayout; + use pacquet_lockfile::PkgName; + use tempfile::tempdir; + + let dir = tempdir().unwrap(); + let virtual_store_dir = dir.path().join("node_modules/.pacquet"); + let private_hoisted = virtual_store_dir.join("node_modules"); + let public_hoisted = dir.path().join("node_modules"); + std::fs::create_dir_all(&virtual_store_dir).unwrap(); + + let node_key = key("evil", "1.0.0"); + let mut hoisted: HashMap> = HashMap::new(); + hoisted.insert(node_key.clone(), HashMap::from([("evil".to_string(), HoistKind::Private)])); + + let mut graph: HashMap = HashMap::new(); + graph.insert( + node_key, + HoistGraphNode { + name: PkgName::parse("../../escaped").unwrap(), + children: HashMap::new(), + has_bin: false, + }, + ); + + let layout = VirtualStoreLayout::legacy( + &virtual_store_dir, + pacquet_config::default_virtual_store_dir_max_length() as usize, + ); + let skipped: HashSet = HashSet::new(); + + let result = super::symlink_hoisted_dependencies( + &hoisted, + &graph, + &layout, + &private_hoisted, + &public_hoisted, + &skipped, + ); + assert!( + matches!(result, Err(crate::SymlinkPackageError::InvalidAlias(_))), + "a traversal hoisted node name must be rejected; got {result:?}", + ); + assert!(!dir.path().join("escaped").exists(), "no hoist link may be created outside the store"); +} + #[test] fn private_hoist_with_bins_collected_for_bin_link() { let (snapshots, packages) = make_lockfile_data(&[ diff --git a/pacquet/crates/package-manager/src/install_frozen_lockfile.rs b/pacquet/crates/package-manager/src/install_frozen_lockfile.rs index bf4a230e59..9ab5e1f555 100644 --- a/pacquet/crates/package-manager/src/install_frozen_lockfile.rs +++ b/pacquet/crates/package-manager/src/install_frozen_lockfile.rs @@ -890,6 +890,19 @@ where Some(&allow_build_policy), ); + // Reject a lockfile whose dependency names, aliases, or + // virtual-store slots would escape the project or the store once + // joined into a filesystem path. Runs before any materialization + // and before the warm-install skip filter, and unconditionally — + // so it is not bypassed by `trustLockfile`, which disables the + // resolution-verification fan-out where the offline name check + // would otherwise run. The slot-containment half needs the + // install-time `layout`, so it can't live in the verifier crate. + pacquet_lockfile_verification::verify_lockfile_dependency_names(lockfile) + .map_err(InstallFrozenLockfileError::LockfileVerification)?; + crate::validate_virtual_store_slot_containment(snapshots, &layout) + .map_err(InstallFrozenLockfileError::LockfileVerification)?; + // The frozen path runs no resolve-time prefetcher, so the warm // batch owns package-status progress for store hits. An empty set // leaves every warm package reported as `found_in_store`. diff --git a/pacquet/crates/package-manager/src/install_package_from_registry.rs b/pacquet/crates/package-manager/src/install_package_from_registry.rs index 539d275f09..00f5d1e11e 100644 --- a/pacquet/crates/package-manager/src/install_package_from_registry.rs +++ b/pacquet/crates/package-manager/src/install_package_from_registry.rs @@ -1,6 +1,8 @@ use crate::{ ImportIndexedDirError, ImportIndexedDirOpts, SymlinkPackageError, import_indexed_dir, - retry_config::retry_opts_from_config, symlink_package, + retry_config::retry_opts_from_config, + safe_join_modules_dir::{InvalidDependencyAliasError, safe_join_modules_dir}, + symlink_package, }; use derive_more::{Display, Error}; use miette::Diagnostic; @@ -89,6 +91,14 @@ pub enum InstallPackageFromRegistryError { ImportIndexedDir(#[error(source)] ImportIndexedDirError), SymlinkPackage(#[error(source)] SymlinkPackageError), + /// The resolved package name is not a valid npm package name. For a + /// tarball, git, or file dependency the name comes from the fetched + /// `package.json`, so a traversal-shaped name would let the import + /// escape the virtual store's `node_modules`. Surfaces pnpm's + /// `ERR_PNPM_INVALID_DEPENDENCY_NAME`. + #[diagnostic(transparent)] + InvalidAlias(#[error(source)] InvalidDependencyAliasError), + /// The resolver produced a resolution shape the npm install path /// can't materialize (today: anything other than a tarball /// resolution carrying an integrity hash). Surfaces with a @@ -138,7 +148,11 @@ impl InstallPackageFromRegistry<'_> { // key (`alias`) so an npm-alias entry and its non-aliased // counterpart can coexist in the same parent, both pointing // at the same registry-named subdirectory inside `slot_dir`. - let save_path = slot_dir.join("node_modules").join(&real_name); + // `real_name` is untrusted for tarball/git/file deps (read from + // the fetched manifest), so the join is guarded against a + // traversal-shaped name escaping `node_modules`. + let save_path = safe_join_modules_dir(&slot_dir.join("node_modules"), &real_name) + .map_err(InstallPackageFromRegistryError::InvalidAlias)?; let symlink_path = node_modules_dir.join(alias); diff --git a/pacquet/crates/package-manager/src/install_package_from_registry/tests.rs b/pacquet/crates/package-manager/src/install_package_from_registry/tests.rs index 0725d115a8..0c0aa9e3b3 100644 --- a/pacquet/crates/package-manager/src/install_package_from_registry/tests.rs +++ b/pacquet/crates/package-manager/src/install_package_from_registry/tests.rs @@ -534,3 +534,79 @@ async fn install_returns_unsupported_resolution_when_name_ver_missing() { drop((store_dir, modules_dir, virtual_store_dir)); } + +/// A tarball/git/file dep whose manifest `name` is a path traversal +/// must be refused before the import writes anything, so a malicious +/// `package.json` can't escape the virtual store's `node_modules` and +/// overwrite files at an attacker-chosen location. The guard fires +/// before the tarball download, so no network access is needed. +#[tokio::test] +async fn install_rejects_traversal_manifest_name() { + let store_dir = tempdir().unwrap(); + let modules_dir = tempdir().unwrap(); + let virtual_store_dir = tempdir().unwrap(); + + let config = create_config(store_dir.path(), modules_dir.path(), virtual_store_dir.path()); + let config: &'static Config = config.pipe(Box::new).pipe(Box::leak); + + let http_client = Arc::new(ThrottledClient::new_for_installs()); + let verified_files_cache = SharedVerifiedFilesCache::default(); + let logged_methods = AtomicU8::new(0); + + let traversal_name = "@x/../../../../../../OUTSIDE"; + let resolution = ResolveResult { + id: "https://example.com/foo.tar.gz".into(), + name_ver: None, + latest: None, + published_at: None, + manifest: Some(Arc::new(serde_json::json!({ + "name": traversal_name, + "version": "1.0.0", + }))), + resolution: LockfileResolution::Tarball(TarballResolution { + tarball: "https://example.com/foo.tar.gz".to_string(), + integrity: None, + git_hosted: Some(true), + path: None, + }), + resolved_via: "git-repository".to_string(), + normalized_bare_specifier: None, + alias: Some("bar".to_string()), + policy_violation: None, + }; + + let slot_dir = virtual_store_dir.path().join("bar@1.0.0"); + + let result = InstallPackageFromRegistry { + tarball_mem_cache: &Default::default(), + config, + http_client: &http_client, + store_index: None, + store_index_writer: None, + verified_files_cache: &verified_files_cache, + prefetched_cas_paths: None, + logged_methods: &logged_methods, + requester: "", + alias: "bar", + resolution: &resolution, + node_modules_dir: modules_dir.path(), + slot_dir: &slot_dir, + first_visit: true, + } + .run::() + .await; + + match result { + Err(InstallPackageFromRegistryError::InvalidAlias(err)) => { + assert_eq!(err.alias, traversal_name); + } + other => panic!("expected InvalidAlias, got {other:?}"), + } + + // The traversal must not have materialized anything outside the + // slot's `node_modules`. + assert!(!virtual_store_dir.path().join("OUTSIDE").exists()); + assert!(!slot_dir.join("node_modules").join("OUTSIDE").exists()); + + drop((store_dir, modules_dir, virtual_store_dir)); +} diff --git a/pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs b/pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs index 3d6ea6c2af..d2e14b4f10 100644 --- a/pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs +++ b/pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs @@ -268,17 +268,15 @@ pub enum InstallWithFreshLockfileError { LinkVirtualStoreBins(#[error(source)] LinkVirtualStoreBinsError), /// The resolver chain failed for at least one dependency. The - /// inner message carries the boxed error's `Display`. + /// diagnostic is forwarded transparently so a canonical inner code + /// (e.g. a traversal name's `ERR_PNPM_INVALID_DEPENDENCY_NAME`) + /// reaches the CLI unchanged. The `Display` still interpolates the + /// inner error so consumers that stringify the top-level error + /// (e.g. pnpr's `resolve.rs`, which forwards `err.to_string()` over + /// the wire) keep the detail. #[display("Failed to resolve dependency tree: {_0}")] - #[diagnostic(code(pacquet_package_manager::resolve_dependency_tree))] - ResolveDependencyTree(#[error(not(source))] ResolveDependencyTreeError), - - /// The hoist-loop orchestrator failed. Wraps the tree-walk error - /// (the only failure source today) plus any future orchestrator- - /// specific failures. - #[display("Failed to resolve importer: {_0}")] - #[diagnostic(code(pacquet_package_manager::resolve_importer))] - ResolveImporter(#[error(not(source))] ResolveImporterError), + #[diagnostic(transparent)] + ResolveDependencyTree(#[error(source)] ResolveDependencyTreeError), /// `minimumReleaseAgeExclude` patterns rejected at compile time. /// Surfaced as `ERR_PNPM_INVALID_MINIMUM_RELEASE_AGE_EXCLUDE`. @@ -1188,7 +1186,9 @@ impl InstallWithFreshLockfile<'_, DependencyGroupList> { }, ) .await - .map_err(InstallWithFreshLockfileError::ResolveImporter)?; + .map_err(|ResolveImporterError::Resolve(err)| { + InstallWithFreshLockfileError::ResolveDependencyTree(err) + })?; let total_nodes = workspace_result.peers.graph.len(); for (importer_id, issues) in &workspace_result.peers.peer_dependency_issues_by_importer { tracing::warn!( diff --git a/pacquet/crates/package-manager/src/lib.rs b/pacquet/crates/package-manager/src/lib.rs index caed0ead9c..fda27c1b0a 100644 --- a/pacquet/crates/package-manager/src/lib.rs +++ b/pacquet/crates/package-manager/src/lib.rs @@ -50,6 +50,7 @@ mod tarball_prefetch; mod update; mod update_project_manifest; mod update_project_manifest_object; +mod validate_lockfile_paths; mod version_policy; mod virtual_store_layout; @@ -98,6 +99,7 @@ pub use tarball_prefetch::*; pub use update::*; pub use update_project_manifest::*; pub use update_project_manifest_object::*; +pub use validate_lockfile_paths::*; pub use version_policy::*; pub use virtual_store_layout::*; diff --git a/pacquet/crates/package-manager/src/safe_join_modules_dir.rs b/pacquet/crates/package-manager/src/safe_join_modules_dir.rs index 63035d7acb..64c0d6a601 100644 --- a/pacquet/crates/package-manager/src/safe_join_modules_dir.rs +++ b/pacquet/crates/package-manager/src/safe_join_modules_dir.rs @@ -16,7 +16,7 @@ use std::path::{Path, PathBuf}; /// `ERR_PNPM_INVALID_DEPENDENCY_NAME`. #[derive(Debug, Display, Error, Diagnostic)] #[display("Refusing to place a dependency under {} with the invalid alias {alias:?}", modules.display())] -#[diagnostic(code(INVALID_DEPENDENCY_NAME))] +#[diagnostic(code(ERR_PNPM_INVALID_DEPENDENCY_NAME))] pub struct InvalidDependencyAliasError { pub modules: PathBuf, #[error(not(source))] diff --git a/pacquet/crates/package-manager/src/symlink_package.rs b/pacquet/crates/package-manager/src/symlink_package.rs index a197a2bb9e..f024292921 100644 --- a/pacquet/crates/package-manager/src/symlink_package.rs +++ b/pacquet/crates/package-manager/src/symlink_package.rs @@ -1,3 +1,4 @@ +use crate::safe_join_modules_dir::InvalidDependencyAliasError; use derive_more::{Display, Error}; use miette::Diagnostic; use pacquet_fs::{ForceSymlinkOutcome, force_symlink_dir}; @@ -23,6 +24,13 @@ pub enum SymlinkPackageError { #[error(source)] error: io::Error, }, + + /// A hoisted package's name is not a valid npm package name, so the + /// `/node_modules/` target the hoist symlink would point + /// at could escape the slot's `node_modules`. Surfaces pnpm's + /// `ERR_PNPM_INVALID_DEPENDENCY_NAME`. + #[diagnostic(transparent)] + InvalidAlias(#[error(source)] InvalidDependencyAliasError), } /// Create a `node_modules/` symlink for a direct dependency. diff --git a/pacquet/crates/package-manager/src/validate_lockfile_paths.rs b/pacquet/crates/package-manager/src/validate_lockfile_paths.rs new file mode 100644 index 0000000000..dd25bf3043 --- /dev/null +++ b/pacquet/crates/package-manager/src/validate_lockfile_paths.rs @@ -0,0 +1,52 @@ +//! Reject a lockfile whose virtual-store slots would escape the store +//! root once materialized. +//! +//! Dependency-name validation lives in the lockfile verifier +//! ([`pacquet_lockfile_verification::verify_lockfile_dependency_names`]), +//! which the install runs unconditionally. This module covers the one +//! escape that name validation alone can't: the global-virtual-store +//! slot path inserts the package name and the version-derived segment as +//! raw path components (unlike the legacy flat name, which `/`-escapes), +//! so a traversal in the version escapes the store even when the name is +//! valid. The check needs the install-time [`VirtualStoreLayout`], which +//! is why it lives here rather than in the verifier crate. + +use crate::VirtualStoreLayout; +use pacquet_fs::is_subdir; +use pacquet_lockfile::{PackageKey, SnapshotEntry}; +use pacquet_lockfile_verification::VerifyError; +use std::collections::{BTreeSet, HashMap}; + +/// Reject the install when any snapshot's computed virtual-store slot +/// resolves outside the store root. The whole `snapshots` map is +/// scanned — not just the survivors of the warm-install skip filter — +/// so a poisoned snapshot that would be skipped as unchanged is still +/// rejected before any directory is created. +/// +/// Surfaces [`VerifyError::InvalidDependencyAlias`] +/// (`ERR_PNPM_INVALID_DEPENDENCY_NAME`), the same code the name check +/// raises, listing every offending snapshot key. +pub fn validate_virtual_store_slot_containment( + snapshots: Option<&HashMap>, + layout: &VirtualStoreLayout, +) -> Result<(), VerifyError> { + let Some(snapshots) = snapshots else { + return Ok(()); + }; + let mut escaped: BTreeSet = BTreeSet::new(); + for key in snapshots.keys() { + // Lexical containment: the slot does not exist yet, so this must + // not touch the filesystem. + if !is_subdir(layout.package_store_dir(), &layout.slot_dir(key)) { + escaped.insert(key.to_string()); + } + } + if escaped.is_empty() { + return Ok(()); + } + let escaped: Vec = escaped.into_iter().collect(); + Err(VerifyError::invalid_dependency_aliases(&escaped)) +} + +#[cfg(test)] +mod tests; diff --git a/pacquet/crates/package-manager/src/validate_lockfile_paths/tests.rs b/pacquet/crates/package-manager/src/validate_lockfile_paths/tests.rs new file mode 100644 index 0000000000..07d0c2309e --- /dev/null +++ b/pacquet/crates/package-manager/src/validate_lockfile_paths/tests.rs @@ -0,0 +1,50 @@ +use super::validate_virtual_store_slot_containment; +use crate::VirtualStoreLayout; +use miette::Diagnostic; +use pacquet_lockfile::{PackageKey, SnapshotEntry}; +use std::{collections::HashMap, path::PathBuf}; + +fn assert_invalid_dependency_name_code(err: &pacquet_lockfile_verification::VerifyError) { + let code = err.code().map(|code| code.to_string()); + assert_eq!(code.as_deref(), Some("ERR_PNPM_INVALID_DEPENDENCY_NAME")); +} + +#[test] +fn accepts_snapshots_whose_slots_stay_in_the_store() { + let layout = VirtualStoreLayout::legacy( + PathBuf::from("/project/node_modules/.pnpm"), + pacquet_config::default_virtual_store_dir_max_length() as usize, + ); + let mut snapshots = HashMap::new(); + snapshots.insert("@scope/foo@1.2.3".parse::().unwrap(), SnapshotEntry::default()); + snapshots.insert("bar@4.5.6".parse::().unwrap(), SnapshotEntry::default()); + validate_virtual_store_slot_containment(Some(&snapshots), &layout) + .expect("contained slots must pass"); +} + +#[test] +fn rejects_a_global_virtual_store_version_escape() { + // Under the global virtual store the slot path inserts the version + // segment as a raw path component (unlike the legacy flat name, which + // escapes `/`). A traversal-bearing version escapes the store root + // even though the package name itself is valid, so the containment + // check — not the name check — is what rejects it. + let key: PackageKey = "evil@../../../escaped".parse().expect("parse escaping version key"); + + let mut config = pacquet_config::Config::new(); + config.enable_global_virtual_store = true; + config.global_virtual_store_dir = PathBuf::from("/store/links"); + + let mut snapshots = HashMap::new(); + snapshots.insert(key.clone(), SnapshotEntry::default()); + let layout = VirtualStoreLayout::new(&config, None, Some(&snapshots), None, None); + assert!( + !pacquet_fs::is_subdir(layout.package_store_dir(), &layout.slot_dir(&key)), + "the crafted slot must actually escape the store for this test to be meaningful", + ); + + let err = validate_virtual_store_slot_containment(Some(&snapshots), &layout) + .expect_err("a slot that escapes the store root must be rejected"); + assert_invalid_dependency_name_code(&err); + assert!(err.to_string().contains("evil@"), "offender listed: {err}"); +} 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 69631a837a..4552d0eec7 100644 --- a/pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs +++ b/pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs @@ -215,11 +215,11 @@ pub enum ResolveDependencyTreeError { /// A dependency alias contained a path-separator segment that would /// escape the intended `node_modules` directory when joined onto a - /// modules path, raised with the `INVALID_DEPENDENCY_NAME` code. + /// modules path, raised with the `ERR_PNPM_INVALID_DEPENDENCY_NAME` code. #[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))] + #[diagnostic(code(ERR_PNPM_INVALID_DEPENDENCY_NAME))] InvalidDependencyName { #[error(not(source))] parent: String, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 1953ae7442..252177d3c2 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -3285,6 +3285,9 @@ importers: '@pnpm/deps.path': specifier: workspace:* version: link:../path + '@pnpm/fs.symlink-dependency': + specifier: workspace:* + version: link:../../fs/symlink-dependency '@pnpm/hooks.types': specifier: workspace:* version: link:../../hooks/types @@ -3316,6 +3319,9 @@ importers: specifier: 'catalog:' version: '@pnpm/ramda@0.28.1' devDependencies: + '@jest/globals': + specifier: 'catalog:' + version: 30.4.1 '@pnpm/deps.graph-builder': specifier: workspace:* version: 'link:' @@ -5986,6 +5992,9 @@ importers: '@pnpm/fetching.pick-fetcher': specifier: workspace:* version: link:../../fetching/pick-fetcher + '@pnpm/fs.symlink-dependency': + specifier: workspace:* + version: link:../../fs/symlink-dependency '@pnpm/hooks.types': specifier: workspace:* version: link:../../hooks/types @@ -6294,6 +6303,9 @@ importers: '@pnpm/fs.read-modules-dir': specifier: workspace:* version: link:../../fs/read-modules-dir + '@pnpm/fs.symlink-dependency': + specifier: workspace:* + version: link:../../fs/symlink-dependency '@pnpm/installing.deps-resolver': specifier: workspace:* version: link:../deps-resolver @@ -7054,6 +7066,9 @@ importers: '@pnpm/deps.path': specifier: workspace:* version: link:../../deps/path + '@pnpm/fs.symlink-dependency': + specifier: workspace:* + version: link:../../fs/symlink-dependency '@pnpm/lockfile.fs': specifier: workspace:* version: link:../fs diff --git a/pnpm11/deps/graph-builder/package.json b/pnpm11/deps/graph-builder/package.json index ca6246d85b..355b43e45d 100644 --- a/pnpm11/deps/graph-builder/package.json +++ b/pnpm11/deps/graph-builder/package.json @@ -24,10 +24,11 @@ "!*.map" ], "scripts": { - "lint": "eslint \"src/**/*.ts\"", - "test": "pn compile", + "lint": "eslint \"src/**/*.ts\" \"test/**/*.ts\"", + "test": "pn compile && pn .test", "prepublishOnly": "tsgo --build", - "compile": "tsgo --build && pn lint --fix" + "compile": "tsgo --build && pn lint --fix", + ".test": "cross-env NODE_OPTIONS=\"$NODE_OPTIONS --experimental-vm-modules --disable-warning=ExperimentalWarning --disable-warning=DEP0169\" jest" }, "dependencies": { "@pnpm/config.package-is-installable": "workspace:*", @@ -35,6 +36,7 @@ "@pnpm/core-loggers": "workspace:*", "@pnpm/deps.graph-hasher": "workspace:*", "@pnpm/deps.path": "workspace:*", + "@pnpm/fs.symlink-dependency": "workspace:*", "@pnpm/hooks.types": "workspace:*", "@pnpm/installing.modules-yaml": "workspace:*", "@pnpm/lockfile.fs": "workspace:*", @@ -50,6 +52,7 @@ "@pnpm/logger": "catalog:" }, "devDependencies": { + "@jest/globals": "catalog:", "@pnpm/deps.graph-builder": "workspace:*", "@pnpm/logger": "workspace:*", "@types/ramda": "catalog:" diff --git a/pnpm11/deps/graph-builder/src/lockfileToDepGraph.ts b/pnpm11/deps/graph-builder/src/lockfileToDepGraph.ts index 4209f0831a..24f3a1b583 100644 --- a/pnpm11/deps/graph-builder/src/lockfileToDepGraph.ts +++ b/pnpm11/deps/graph-builder/src/lockfileToDepGraph.ts @@ -7,6 +7,7 @@ import { progressLogger, } from '@pnpm/core-loggers' import * as dp from '@pnpm/deps.path' +import { safeJoinModulesDir } from '@pnpm/fs.symlink-dependency' import type { IncludedDependencies } from '@pnpm/installing.modules-yaml' import type { LockfileObject, LockfileResolution } from '@pnpm/lockfile.fs' import { @@ -230,7 +231,11 @@ async function buildGraphFromPackages ( const depIntegrityIsUnchanged = isIntegrityEqual(pkgSnapshot.resolution, currentPackages[depPath]?.resolution) const modules = path.join(dirInVirtualStore, 'node_modules') - const dir = path.join(modules, pkgName) + // `pkgName` is reconstructed from the (attacker-controllable) lockfile + // depPath key via `dp.parse`, which does no validation. Contain it here so + // a traversal name (e.g. `../../../tmp/x`) can't make the package import + // escape the virtual store. Mirrors the guard on the hoisted linker. + const dir = safeJoinModulesDir(modules, pkgName) locationByDepPath[depPath] = dir // Track directory deps for injected workspace packages if (isDirectoryDep) { diff --git a/pnpm11/deps/graph-builder/test/lockfileToDepGraph.test.ts b/pnpm11/deps/graph-builder/test/lockfileToDepGraph.test.ts new file mode 100644 index 0000000000..4fa35a9d07 --- /dev/null +++ b/pnpm11/deps/graph-builder/test/lockfileToDepGraph.test.ts @@ -0,0 +1,124 @@ +/// +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' + +import { expect, test } from '@jest/globals' +import { depPathToFilename } from '@pnpm/deps.path' +import type { LockfileObject } from '@pnpm/lockfile.fs' + +import { lockfileToDepGraph, type LockfileToDepGraphOptions } from '../src/lockfileToDepGraph.js' + +// A crafted lockfile whose `packages` depPath *key* carries a path-traversal +// (or reserved) name. The isolated (virtual-store) linker reconstructs the +// package name from that key via `dp.parse` and joins it onto the virtual +// store to form the package's install directory, so this is the shape an +// attacker who can ship a lockfile would use to make `pnpm install` write +// package content outside the store (GHSA-c59q-g84q-2gj5). +function craftedLockfile (name: string): LockfileObject { + return { + lockfileVersion: '9.0', + importers: { + '.': { + dependencies: { 'legit-name': `${name}@1.0.0` }, + specifiers: { 'legit-name': '1.0.0' }, + }, + }, + packages: { + [`${name}@1.0.0`]: { + resolution: { integrity: 'sha512-deadbeef' }, + }, + }, + } as unknown as LockfileObject +} + +// `force: true` skips the installability check so the walk reaches the name +// sink directly; the store controller throws if touched, proving the name is +// rejected before any fetch or filesystem work. +function graphOpts (lockfileDir: string): LockfileToDepGraphOptions { + const unreachable = (name: string) => () => { + throw new Error(`${name} must not be reached for a rejected package name`) + } + return { + autoInstallPeers: false, + engineStrict: false, + force: true, + importerIds: ['.'], + include: { dependencies: true, devDependencies: true, optionalDependencies: true }, + ignoreScripts: false, + lockfileDir, + nodeVersion: process.version, + pnpmVersion: '0.0.0', + registries: { default: 'http://localhost/' }, + sideEffectsCacheRead: false, + skipped: new Set(), + storeController: { + fetchPackage: unreachable('fetchPackage'), + getFilesIndexFilePath: unreachable('getFilesIndexFilePath'), + }, + storeDir: path.join(lockfileDir, 'store'), + globalVirtualStoreDir: path.join(lockfileDir, 'store', 'v10'), + virtualStoreDir: path.join(lockfileDir, 'node_modules', '.pnpm'), + virtualStoreDirMaxLength: 120, + } as unknown as LockfileToDepGraphOptions +} + +test.each([ + '../../../escape', + '@scope/../../escape', + '.bin', + '.pnpm', + 'node_modules', +])('lockfileToDepGraph rejects package name %p', async (name) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'graph-builder-')) + await expect( + lockfileToDepGraph(craftedLockfile(name), null, graphOpts(dir)) + ).rejects.toThrow(expect.objectContaining({ code: 'ERR_PNPM_INVALID_DEPENDENCY_NAME' })) +}) + +test('lockfileToDepGraph does not create a directory outside the virtual store for a traversal name', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'graph-builder-')) + const name = '../../../escape' + const opts = graphOpts(dir) + // Reconstruct the exact install directory the unguarded code would derive: + // `depPathToFilename` folds the depPath key into a single (contained) store + // subdir, but the separately-parsed `pkgName` is joined onto its + // `node_modules` raw — that raw join is what escapes. + const dirInVirtualStore = path.join(opts.virtualStoreDir, depPathToFilename(`${name}@1.0.0`, opts.virtualStoreDirMaxLength)) + const escaped = path.join(dirInVirtualStore, 'node_modules', name) + // Guard against the test asserting on a non-escaping path: the derived + // directory must genuinely fall outside the virtual store. + expect(escaped.startsWith(opts.virtualStoreDir + path.sep)).toBe(false) + await expect( + lockfileToDepGraph(craftedLockfile(name), null, opts) + ).rejects.toThrow(expect.objectContaining({ code: 'ERR_PNPM_INVALID_DEPENDENCY_NAME' })) + expect(fs.existsSync(escaped)).toBe(false) +}) + +test('lockfileToDepGraph rejects a global-virtual-store slot whose version escapes the store', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'graph-builder-')) + // A valid package name but a traversal *version*. The global-virtual-store + // slot path `@///` inserts the version as a raw path + // segment, so it escapes the store root even though `safeJoinModulesDir` + // accepts the (valid) inner package name — the name guard alone can't catch + // this, only the slot containment can. + const lockfile = { + lockfileVersion: '9.0', + importers: { + '.': { + dependencies: { 'legit-name': 'foo@1.0.0' }, + specifiers: { 'legit-name': '1.0.0' }, + }, + }, + packages: { + 'foo@1.0.0': { + resolution: { integrity: 'sha512-deadbeef' }, + version: '../../../../escape', + }, + }, + } as unknown as LockfileObject + const opts = { ...graphOpts(dir), enableGlobalVirtualStore: true } as unknown as LockfileToDepGraphOptions + await expect( + lockfileToDepGraph(lockfile, null, opts) + ).rejects.toThrow(expect.objectContaining({ code: 'ERR_PNPM_INVALID_DEPENDENCY_NAME' })) +}) diff --git a/pnpm11/deps/graph-builder/test/tsconfig.json b/pnpm11/deps/graph-builder/test/tsconfig.json new file mode 100644 index 0000000000..67ce5e1d0e --- /dev/null +++ b/pnpm11/deps/graph-builder/test/tsconfig.json @@ -0,0 +1,18 @@ +{ + "extends": "../tsconfig.json", + "compilerOptions": { + "noEmit": false, + "outDir": "../node_modules/.test.lib", + "rootDir": "..", + "isolatedModules": true + }, + "include": [ + "**/*.ts", + "../../../__typings__/**/*.d.ts" + ], + "references": [ + { + "path": ".." + } + ] +} diff --git a/pnpm11/deps/graph-builder/tsconfig.json b/pnpm11/deps/graph-builder/tsconfig.json index f4d32f77da..dc33b6018c 100644 --- a/pnpm11/deps/graph-builder/tsconfig.json +++ b/pnpm11/deps/graph-builder/tsconfig.json @@ -24,6 +24,9 @@ { "path": "../../core/types" }, + { + "path": "../../fs/symlink-dependency" + }, { "path": "../../hooks/types" }, diff --git a/pnpm11/deps/graph-hasher/src/index.ts b/pnpm11/deps/graph-hasher/src/index.ts index 202bda0320..b3c55fc824 100644 --- a/pnpm11/deps/graph-hasher/src/index.ts +++ b/pnpm11/deps/graph-hasher/src/index.ts @@ -281,10 +281,26 @@ export function calcGlobalVirtualStorePathWithSubdeps ( // Scoped: @scope/pkg/version/hash // Unscoped: @/pkg/version/hash function formatGlobalVirtualStorePath (name: string, version: string, hexDigest: string): string { + // `version` is lockfile-controlled (`pkgSnapshot.version ?? parsed depPath`) + // and is inserted below as a raw path segment. Every global-virtual-store + // slot path funnels through here, and callers join the result onto + // `globalVirtualStoreDir` before passing it to `importPackage`, so a `..` + // segment in the version would let the slot escape the store root — even + // when the package name itself is valid (the name's own traversal is caught + // downstream by `safeJoinModulesDir`). Reject it at this single choke point. + assertNoPathTraversal(version) const prefix = name.startsWith('@') ? '' : '@/' return `${prefix}${name}/${version}/${hexDigest}` } +function assertNoPathTraversal (version: string): void { + if (version.split(/[/\\]/).includes('..')) { + const error = new Error(`Refusing to build a virtual-store path with the traversal version segment ${JSON.stringify(version)}`) as Error & { code: string } + error.code = 'ERR_PNPM_INVALID_DEPENDENCY_NAME' + throw error + } +} + export interface PkgMetaAndSnapshot extends PkgMeta { pkgSnapshot: PackageSnapshot pkgIdWithPatchHash: PkgIdWithPatchHash diff --git a/pnpm11/deps/graph-hasher/test/calcLeafGlobalVirtualStorePath.test.ts b/pnpm11/deps/graph-hasher/test/calcLeafGlobalVirtualStorePath.test.ts index b29e7921cc..e0fb55d326 100644 --- a/pnpm11/deps/graph-hasher/test/calcLeafGlobalVirtualStorePath.test.ts +++ b/pnpm11/deps/graph-hasher/test/calcLeafGlobalVirtualStorePath.test.ts @@ -6,6 +6,18 @@ describe('calcLeafGlobalVirtualStorePath', () => { const path = calcLeafGlobalVirtualStorePath('foo@1.0.0:sha512-abc', 'foo', '1.0.0') expect(path).toMatch(/^@\/foo\/1\.0\.0\/[a-f0-9]+$/) }) + + // The version segment is lockfile-controlled and inserted raw into the slot + // path; a `..` segment would let the slot escape the global virtual store + // root once joined onto `globalVirtualStoreDir`. All GVS slot builders funnel + // through `formatGlobalVirtualStorePath`, so the rejection happens here. + it.each(['../../../escape', '..', 'a/../../b', 'x/..'])( + 'rejects a traversal version segment %p', + (version) => { + expect(() => calcLeafGlobalVirtualStorePath('foo@1.0.0:sha512-abc', 'foo', version)) + .toThrow(expect.objectContaining({ code: 'ERR_PNPM_INVALID_DEPENDENCY_NAME' })) + } + ) }) describe('calcGlobalVirtualStorePathWithSubdeps', () => { diff --git a/pnpm11/installing/deps-installer/test/fixtures/pkg-with-path-traversal-name.tgz b/pnpm11/installing/deps-installer/test/fixtures/pkg-with-path-traversal-name.tgz new file mode 100644 index 0000000000..b544e35261 Binary files /dev/null and b/pnpm11/installing/deps-installer/test/fixtures/pkg-with-path-traversal-name.tgz differ diff --git a/pnpm11/installing/deps-installer/test/install/fromTarball.ts b/pnpm11/installing/deps-installer/test/install/fromTarball.ts index 9a55eb35b3..7be7705de6 100644 --- a/pnpm11/installing/deps-installer/test/install/fromTarball.ts +++ b/pnpm11/installing/deps-installer/test/install/fromTarball.ts @@ -1,10 +1,13 @@ import { expect, test } from '@jest/globals' import { addDependenciesToPackage } from '@pnpm/installing.deps-installer' import { prepareEmpty } from '@pnpm/prepare' +import { fixtures } from '@pnpm/test-fixtures' import { REGISTRY_MOCK_PORT } from '@pnpm/testing.registry-mock' import { testDefaults } from '../utils/index.js' +const f = fixtures(import.meta.dirname) + test('tarball from npm registry', async () => { const project = prepareEmpty() @@ -32,3 +35,14 @@ test('tarballs from GitHub (is-negative)', async () => { project.has('is-negative') }) + +// A tarball dependency's own manifest `name` is used as the directory name for +// the package inside the virtual store. A traversal name such as +// `@x/../../../` must not escape the store and write outside node_modules. +test('a tarball dependency whose manifest name is a path traversal is rejected', async () => { + prepareEmpty() + + await expect( + addDependenciesToPackage({}, [`file:${f.find('pkg-with-path-traversal-name.tgz')}`], testDefaults()) + ).rejects.toThrow('Refusing to place a dependency') +}) diff --git a/pnpm11/installing/deps-resolver/package.json b/pnpm11/installing/deps-resolver/package.json index cf95448b1a..860a3064f8 100644 --- a/pnpm11/installing/deps-resolver/package.json +++ b/pnpm11/installing/deps-resolver/package.json @@ -42,6 +42,7 @@ "@pnpm/deps.peer-range": "workspace:*", "@pnpm/error": "workspace:*", "@pnpm/fetching.pick-fetcher": "workspace:*", + "@pnpm/fs.symlink-dependency": "workspace:*", "@pnpm/hooks.types": "workspace:*", "@pnpm/lockfile.preferred-versions": "workspace:*", "@pnpm/lockfile.pruner": "workspace:*", diff --git a/pnpm11/installing/deps-resolver/src/index.ts b/pnpm11/installing/deps-resolver/src/index.ts index edd811f618..c78212b6c4 100644 --- a/pnpm11/installing/deps-resolver/src/index.ts +++ b/pnpm11/installing/deps-resolver/src/index.ts @@ -7,6 +7,7 @@ import { import { findRuntimeNodeVersion, iterateHashedGraphNodes } from '@pnpm/deps.graph-hasher' import { isRuntimeDepPath } from '@pnpm/deps.path' import { PnpmError } from '@pnpm/error' +import { safeJoinModulesDir } from '@pnpm/fs.symlink-dependency' import type { LockfileObject, ProjectSnapshot, @@ -611,7 +612,7 @@ function extendGraph ( const node = graph[depPath] Object.assign(node, { modules, - dir: path.join(modules, node.name), + dir: safeJoinModulesDir(modules, node.name), }) } return graph diff --git a/pnpm11/installing/deps-resolver/src/resolvePeers.ts b/pnpm11/installing/deps-resolver/src/resolvePeers.ts index adb51a5dfe..b015cee82e 100644 --- a/pnpm11/installing/deps-resolver/src/resolvePeers.ts +++ b/pnpm11/installing/deps-resolver/src/resolvePeers.ts @@ -1,6 +1,7 @@ import path from 'node:path' import { createPeerDepGraphHash, depPathToFilename, parseDepPath, type PeerId } from '@pnpm/deps.path' +import { safeJoinModulesDir } from '@pnpm/fs.symlink-dependency' import type { DepPath, ParentPackages, @@ -703,7 +704,7 @@ async function resolvePeersOfNode ( const peerDependencies = { ...resolvedPackage.peerDependencies } if (!ctx.depGraph[depPath] || ctx.depGraph[depPath].depth > node.depth) { const modules = path.join(ctx.virtualStoreDir, depPathToFilename(depPath, ctx.virtualStoreDirMaxLength), 'node_modules') - const dir = path.join(modules, resolvedPackage.name) + const dir = safeJoinModulesDir(modules, resolvedPackage.name) const transitivePeerDependencies = new Set() for (const unknownPeer of allResolvedPeers.keys()) { diff --git a/pnpm11/installing/deps-resolver/tsconfig.json b/pnpm11/installing/deps-resolver/tsconfig.json index 0d5b77c4b1..bc5546a55e 100644 --- a/pnpm11/installing/deps-resolver/tsconfig.json +++ b/pnpm11/installing/deps-resolver/tsconfig.json @@ -45,6 +45,9 @@ { "path": "../../fetching/pick-fetcher" }, + { + "path": "../../fs/symlink-dependency" + }, { "path": "../../hooks/types" }, diff --git a/pnpm11/installing/env-installer/package.json b/pnpm11/installing/env-installer/package.json index 847c1949bd..85cf68496d 100644 --- a/pnpm11/installing/env-installer/package.json +++ b/pnpm11/installing/env-installer/package.json @@ -41,6 +41,7 @@ "@pnpm/deps.graph-hasher": "workspace:*", "@pnpm/error": "workspace:*", "@pnpm/fs.read-modules-dir": "workspace:*", + "@pnpm/fs.symlink-dependency": "workspace:*", "@pnpm/installing.deps-resolver": "workspace:*", "@pnpm/lockfile.fs": "workspace:*", "@pnpm/lockfile.pruner": "workspace:*", diff --git a/pnpm11/installing/env-installer/src/installConfigDeps.ts b/pnpm11/installing/env-installer/src/installConfigDeps.ts index c9d8422168..43e7b3af5a 100644 --- a/pnpm11/installing/env-installer/src/installConfigDeps.ts +++ b/pnpm11/installing/env-installer/src/installConfigDeps.ts @@ -7,6 +7,7 @@ import { installingConfigDepsLogger, skippedOptionalDependencyLogger } from '@pn import { calcGlobalVirtualStorePathWithSubdeps, calcLeafGlobalVirtualStorePath } from '@pnpm/deps.graph-hasher' import { PnpmError } from '@pnpm/error' import { readModulesDir } from '@pnpm/fs.read-modules-dir' +import { safeJoinModulesDir } from '@pnpm/fs.symlink-dependency' import { type EnvLockfile, readEnvLockfile } from '@pnpm/lockfile.fs' import { getNpmTarballUrl } from '@pnpm/resolving.tarball-url' import type { StoreController } from '@pnpm/store.controller' @@ -292,7 +293,7 @@ async function installOptionalSubdeps (opts: InstallOptionalSubdepsOpts): Promis await Promise.all(compatibleSubdeps.map(async (subdep) => { const subdepFullPkgId = `${subdep.name}@${subdep.version}:${subdep.resolution.integrity}` const subdepRelPath = calcLeafGlobalVirtualStorePath(subdepFullPkgId, subdep.name, subdep.version) - const subdepDirInGlobalVirtualStore = path.join(opts.globalVirtualStoreDir, subdepRelPath, 'node_modules', subdep.name) + const subdepDirInGlobalVirtualStore = safeJoinModulesDir(path.join(opts.globalVirtualStoreDir, subdepRelPath, 'node_modules'), subdep.name) if (!fs.existsSync(path.join(subdepDirInGlobalVirtualStore, 'package.json'))) { opts.reportStarted() const { fetching } = await opts.store.fetchPackage({ @@ -310,7 +311,7 @@ async function installOptionalSubdeps (opts: InstallOptionalSubdepsOpts): Promis filesResponse, }) } - const linkPath = path.join(opts.parentNodeModulesDir, subdep.name) + const linkPath = safeJoinModulesDir(opts.parentNodeModulesDir, subdep.name) if (await symlinkPointsTo(linkPath, subdepDirInGlobalVirtualStore)) { return } diff --git a/pnpm11/installing/env-installer/tsconfig.json b/pnpm11/installing/env-installer/tsconfig.json index 0c66ed5e0b..23d4f3a690 100644 --- a/pnpm11/installing/env-installer/tsconfig.json +++ b/pnpm11/installing/env-installer/tsconfig.json @@ -39,6 +39,9 @@ { "path": "../../fs/read-modules-dir" }, + { + "path": "../../fs/symlink-dependency" + }, { "path": "../../lockfile/fs" }, diff --git a/pnpm11/lockfile/to-pnp/package.json b/pnpm11/lockfile/to-pnp/package.json index 83f049b443..f3a1a16996 100644 --- a/pnpm11/lockfile/to-pnp/package.json +++ b/pnpm11/lockfile/to-pnp/package.json @@ -34,6 +34,7 @@ }, "dependencies": { "@pnpm/deps.path": "workspace:*", + "@pnpm/fs.symlink-dependency": "workspace:*", "@pnpm/lockfile.fs": "workspace:*", "@pnpm/lockfile.utils": "workspace:*", "@pnpm/types": "workspace:*", diff --git a/pnpm11/lockfile/to-pnp/src/index.ts b/pnpm11/lockfile/to-pnp/src/index.ts index 2059963872..d865be998b 100644 --- a/pnpm11/lockfile/to-pnp/src/index.ts +++ b/pnpm11/lockfile/to-pnp/src/index.ts @@ -2,6 +2,7 @@ import { promises as fs } from 'node:fs' import path from 'node:path' import { depPathToFilename, refToRelative } from '@pnpm/deps.path' +import { safeJoinModulesDir } from '@pnpm/fs.symlink-dependency' import type { LockfileObject } from '@pnpm/lockfile.fs' import { nameVerFromPkgSnapshot, @@ -101,13 +102,16 @@ export function lockfileToPackageRegistry ( packageRegistry.set(name, packageStore) } - // Seems like this field should always contain a relative path - let packageLocation = normalizePath(path.relative(opts.lockfileDir, path.join( + // Seems like this field should always contain a relative path. + // `name` is reconstructed from the (attacker-controllable) lockfile depPath + // key via `dp.parse`, which does no validation, so contain it here to keep + // the PnP resolver map from pointing outside the virtual store. + const pkgModulesDir = path.join( opts.virtualStoreDir, depPathToFilename(relDepPath, opts.virtualStoreDirMaxLength), - 'node_modules', - name - ))) + 'node_modules' + ) + let packageLocation = normalizePath(path.relative(opts.lockfileDir, safeJoinModulesDir(pkgModulesDir, name))) if (!packageLocation.startsWith('../')) { packageLocation = `./${packageLocation}` } diff --git a/pnpm11/lockfile/to-pnp/test/index.ts b/pnpm11/lockfile/to-pnp/test/index.ts index 002a52f281..8a6f6d554e 100644 --- a/pnpm11/lockfile/to-pnp/test/index.ts +++ b/pnpm11/lockfile/to-pnp/test/index.ts @@ -2,8 +2,9 @@ import path from 'node:path' import { expect, test } from '@jest/globals' +import type { LockfileObject } from '@pnpm/lockfile.fs' import { dependenciesGraphToPackageMap, lockfileToPackageMap, lockfileToPackageRegistry } from '@pnpm/lockfile.to-pnp' -import type { DepPath, ProjectId } from '@pnpm/types' +import type { DepPath, ProjectId, Registries } from '@pnpm/types' test('lockfileToPackageRegistry', () => { const packageRegistry = lockfileToPackageRegistry({ @@ -701,3 +702,33 @@ test('lockfileToPackageRegistry packages that have peer deps', () => { ], ]) }) + +// A `packages` depPath *key* whose name portion is a path-traversal makes the +// PnP `packageLocation` (built by joining that name onto the virtual store) +// point outside the store. The name must be rejected so a tampered lockfile +// can't aim the `.pnp.cjs` resolver map at arbitrary paths (GHSA-c59q-g84q-2gj5). +test('lockfileToPackageRegistry rejects a package name with path-traversal', () => { + const lockfile = { + lockfileVersion: '9.0', + importers: { + ['.' as ProjectId]: { + dependencies: { 'legit-name': '../../../escape@1.0.0' }, + specifiers: { 'legit-name': '1.0.0' }, + }, + }, + packages: { + ['../../../escape@1.0.0' as DepPath]: { + resolution: { integrity: 'sha512-deadbeef' }, + }, + }, + } as unknown as LockfileObject + expect(() => + lockfileToPackageRegistry(lockfile, { + importerNames: {}, + lockfileDir: '/home/user/project', + virtualStoreDir: '/home/user/project/node_modules/.pnpm', + virtualStoreDirMaxLength: 120, + registries: { default: 'https://registry.npmjs.org/' } as Registries, + }) + ).toThrow(expect.objectContaining({ code: 'ERR_PNPM_INVALID_DEPENDENCY_NAME' })) +}) diff --git a/pnpm11/lockfile/to-pnp/tsconfig.json b/pnpm11/lockfile/to-pnp/tsconfig.json index 1646b863b3..2544d514c4 100644 --- a/pnpm11/lockfile/to-pnp/tsconfig.json +++ b/pnpm11/lockfile/to-pnp/tsconfig.json @@ -18,6 +18,9 @@ { "path": "../../deps/path" }, + { + "path": "../../fs/symlink-dependency" + }, { "path": "../fs" },