From 4ec0049f55b3578769c2ffb1a6033d4bbb195284 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kh=E1=BA=A3i?= Date: Wed, 24 Jun 2026 18:38:38 +0700 Subject: [PATCH] fix(pacquet/git-fetcher): bundle transitive deps of bundled dependencies (#12620) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(git-fetcher): bundle transitive deps of bundled dependencies The packlist `bundleDependencies` pass only spliced in each directly-bundled package's own files; it never followed that package's own dependencies, so a bundled dep's transitive (and hoisted) dependencies were dropped from the published file set. Replace the per-dependency recursion with npm-bundled's reachability walk: seed from the root manifest's `bundleDependencies`, then transitively pull in every reachable package's `dependencies` and `optionalDependencies`, resolving each via the node module-resolution walk-up (nested `node_modules/` first, then ancestor `node_modules/`). The walk-up is what lets a hoisted transitive dep at the root `node_modules/` be found and spliced in under its real path. A visited-set keyed on the canonicalised resolved directory keeps a diamond from being processed twice and stops dependency cycles. Ports the upstream pnpm test "bundles transitive dependencies of bundled dependencies (hoisted)" from releasing/commands/test/publish/pack.ts, plus nested-wins-over-hoisted and optionalDependencies/devDependencies coverage. Resolves https://github.com/pnpm/pnpm/issues/12602 Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01XXHGos5SaEKKr7pUW4FXga * docs(git-fetcher): fix stale bundling comment and pin test citation Address review nits on the transitive-bundling change: - The comment in `collect_own_files` referred to a `bundleDependencies` pass "below", but bundling now lives in the separate `collect_bundled_files`; describe the current structure instead. - Pin the ported-test citation to a commit permalink, per pacquet/AGENTS.md's rule that code citations link to a specific SHA. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01XXHGos5SaEKKr7pUW4FXga * fix(git-fetcher): refuse bundled deps whose real path escapes the package A `node_modules/` entry can be a symlink pointing outside the package (a sibling directory or an absolute host path). The name still passes `is_safe_bundle_name` because it is a single safe segment, so the walk-up resolves it and `collect_own_files` walks the symlink target — splicing host files into the published set. The git/directory fetchers import untrusted packages, making this an information-disclosure path. Check each resolved dependency's canonical (symlink-resolved) path against the canonical package root and refuse anything that escapes, falling back to a lexical comparison when canonicalization is unavailable. Reuses the canonicalize call already used for cycle dedup. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01XXHGos5SaEKKr7pUW4FXga * fix(git-fetcher): fail closed when a bundled dep's real path is unverifiable The symlink-escape guard fell back to a lexical path check whenever `fs::canonicalize` returned `None` for the dependency. A symlink under `node_modules/` is lexically inside the package even when its target escapes, so that fallback was fail-open. When the root canonicalizes but the dependency does not, refuse the dependency instead: a genuine dependency always canonicalizes here, since `resolve_bundled_dependency` already stat'd its `package.json` through the same path. The lexical fallback now applies only when the root itself cannot be canonicalized. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01XXHGos5SaEKKr7pUW4FXga * fix(git-fetcher): correct the upstream test citation to a real commit The previous citation pinned SHA cab1c11c69, which does not exist — it was taken from the issue body without verification. Point instead at the in-repo copy of the test at pnpm11/releasing/commands/test/publish/pack.ts, pinned to a commit that actually contains it. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01XXHGos5SaEKKr7pUW4FXga --------- Co-authored-by: Claude --- pacquet/crates/git-fetcher/src/packlist.rs | 288 ++++++++++++------ .../crates/git-fetcher/src/packlist/tests.rs | 176 ++++++++++- 2 files changed, 357 insertions(+), 107 deletions(-) diff --git a/pacquet/crates/git-fetcher/src/packlist.rs b/pacquet/crates/git-fetcher/src/packlist.rs index 5f06425381..0bd30fc81d 100644 --- a/pacquet/crates/git-fetcher/src/packlist.rs +++ b/pacquet/crates/git-fetcher/src/packlist.rs @@ -18,10 +18,16 @@ //! `HISTORY*` / `NOTICE*` at the root, plus the paths declared in //! `main` / `bin`. These survive `.npmignore` rejection and the //! `files`-field filter. -//! 4. **`bundleDependencies` recursion**: for each name in +//! 4. **`bundleDependencies` closure**: starting from the names in //! `manifest.bundleDependencies` (or the legacy -//! `bundledDependencies`), recurse into `node_modules//` and -//! splice its packlist under that prefix. +//! `bundledDependencies`), transitively include every reachable +//! dependency. A bundled package pulls in its own `dependencies` +//! and `optionalDependencies` too, so the whole closure ships. +//! Each name is resolved with the node module-resolution walk-up +//! (nested `node_modules/` first, then ancestor `node_modules/`), +//! which is what lets a hoisted transitive dep at the root +//! `node_modules/` be found and spliced in under its real path. +//! Port of [`npm-bundled`](https://github.com/npm/npm-bundled). //! //! Two intentional divergences from upstream: //! @@ -43,17 +49,17 @@ use ignore::{WalkBuilder, gitignore::Gitignore}; use pacquet_package_manifest::safe_read_package_json_from_dir; use serde_json::Value; use std::{ - collections::{BTreeSet, HashSet}, + collections::{BTreeSet, HashSet, VecDeque}, fs, path::{Path, PathBuf}, }; -/// Cap on `bundleDependencies` recursion depth. Real packages bundle +/// Cap on `bundleDependencies` closure depth. Real packages bundle /// at most a handful of levels (most published packages bundle zero; -/// the rare ones bundle one or two). The cap prevents a runaway -/// recursion if a bundled dep declares its own bundle pointing at -/// itself (or a symlink loop in the source tree slips past -/// `canonicalize`). +/// the rare ones bundle one or two). The visited-set already makes +/// the walk terminate; this cap is belt-and-braces against a +/// pathological tree that keeps resolving fresh canonical paths +/// (e.g. a deep chain of `dependencies` that never repeats). const MAX_BUNDLE_DEPTH: u32 = 32; /// Case-insensitive prefix matches for files always-included at the @@ -88,47 +94,165 @@ const ALWAYS_EXCLUDED_SUFFIXES: &[&str] = &[".orig"]; /// [`fs/packlist/src/index.ts:24-29`](https://github.com/pnpm/pnpm/blob/94240bc046/fs/packlist/src/index.ts#L24-L29) /// (paths relative to `pkg_dir`, no leading `./`). pub fn packlist(pkg_dir: &Path, manifest: &Value) -> Result, PacklistError> { - let mut visited = HashSet::new(); - packlist_inner(pkg_dir, manifest, &mut visited, 0) + let mut out: BTreeSet = collect_own_files(pkg_dir, manifest)?; + collect_bundled_files(pkg_dir, manifest, &mut out)?; + Ok(out.into_iter().collect()) } -/// Inner recursive entry point that threads cycle detection and a -/// depth cap through `bundleDependencies` traversals. Each -/// recursion's canonicalised `pkg_dir` is inserted into `visited` -/// so a bundled dep that points back at an ancestor (cycle) gets -/// skipped instead of stack-overflowing. The `depth` counter is a -/// belt-and-braces guard against any cycle the canonical-path check -/// can't see (e.g. filesystem mount tricks). -fn packlist_inner( - pkg_dir: &Path, - manifest: &Value, - visited: &mut HashSet, +/// One unit of `bundleDependencies`-closure work: resolve `name` +/// starting the node module-resolution walk-up at `from_dir`, then +/// splice the resolved package's files into the output. +struct BundleTask { + name: String, + from_dir: PathBuf, depth: u32, -) -> Result, PacklistError> { - // `fs::canonicalize` resolves symlinks, which is precisely what - // we want for cycle detection — a symlink loop in the source - // tree shows up as the same canonical path. Fall back to the - // input path on canonicalisation failure (e.g. permission - // denied); the cycle check then degrades to identity on the - // raw path, which is still enough to catch trivial self-bundles. - let canonical = fs::canonicalize(pkg_dir).unwrap_or_else(|_| pkg_dir.to_path_buf()); - if !visited.insert(canonical) { - tracing::warn!( - target: "pacquet::git_fetcher::packlist", - pkg_dir = %pkg_dir.display(), - "bundleDependencies cycle: directory already visited at this canonical path; skipping", - ); - return Ok(Vec::new()); +} + +/// Build the `bundleDependencies` closure for `root` and splice each +/// bundled package's files into `out` under the package's real path +/// relative to `root` (e.g. `node_modules//...`). +/// +/// Mirrors [`npm-bundled`](https://github.com/npm/npm-bundled): seed +/// from the root manifest's bundle list, then transitively pull in +/// every reachable dependency. Once a package is bundled, its own +/// `dependencies` and `optionalDependencies` are bundled too — that +/// is how the closure reaches a hoisted transitive dep sitting at the +/// root `node_modules/`. `devDependencies` are never followed. +/// +/// The `visited` set is keyed on the canonicalised resolved directory, +/// so a diamond (two bundled deps sharing a transitive dep) processes +/// the shared package once and a `dependencies` cycle terminates +/// instead of looping forever. +fn collect_bundled_files( + root: &Path, + root_manifest: &Value, + out: &mut BTreeSet, +) -> Result<(), PacklistError> { + // Canonical form of the package root, used to reject any bundled + // dependency whose real path escapes the tree (see the symlink check + // in the loop below). `None` if `root` itself can't be canonicalised, + // in which case the escape check falls back to a lexical comparison. + let canonical_root = fs::canonicalize(root).ok(); + let mut visited: HashSet = HashSet::new(); + let mut queue: VecDeque = root_bundle_dep_names(root_manifest) + .into_iter() + .map(|name| BundleTask { name, from_dir: root.to_path_buf(), depth: 0 }) + .collect(); + + while let Some(task) = queue.pop_front() { + if task.depth > MAX_BUNDLE_DEPTH { + tracing::warn!( + target: "pacquet::git_fetcher::packlist", + bundle_name = %task.name, + depth = task.depth, + "bundleDependencies closure exceeded MAX_BUNDLE_DEPTH; refusing to descend further", + ); + continue; + } + // Defense-in-depth: a malicious manifest could carry + // `bundleDependencies: ["../../etc"]` (or an absolute path). + // Reject anything that's not a single safe segment before it + // reaches the join in `resolve_bundled_dependency`. + if !is_safe_bundle_name(&task.name) { + tracing::warn!( + target: "pacquet::git_fetcher::packlist", + bundle_name = %task.name, + "rejecting bundleDependencies entry that is not a single path segment", + ); + continue; + } + let Some(dep_dir) = resolve_bundled_dependency(&task.name, &task.from_dir, root) else { + tracing::debug!( + target: "pacquet::git_fetcher::packlist", + bundle_name = %task.name, + from_dir = %task.from_dir.display(), + "bundleDependencies entry not resolvable under node_modules/; skipping", + ); + continue; + }; + // `fs::canonicalize` resolves symlinks, giving both the dedup + // key (a symlink loop shows up as an already-visited path) and + // the real target for the escape check below. `None` on failure + // (e.g. permission denied); dedup then degrades to the raw path + // and the escape check to a lexical comparison. + let canonical_dep = fs::canonicalize(&dep_dir).ok(); + // `is_safe_bundle_name` only screens the name; a + // `node_modules/` symlink pointing at a sibling or an + // absolute host path passes that yet resolves outside the tree. + // Walking it would splice host files into the published set, so + // refuse anything whose real path is not under the root. The + // fetcher imports untrusted git-hosted packages, so this matters. + let escapes = match (&canonical_root, &canonical_dep) { + (Some(root), Some(dep)) => dep.strip_prefix(root).is_err(), + // Root resolved but the dependency's real path didn't: we + // can't prove it stays inside the tree, so fail closed. A + // genuine dependency always canonicalises here — + // `resolve_bundled_dependency` already stat'd its + // `package.json` through the same path. + (Some(_), None) => true, + // Root itself won't canonicalise (pathological): fall back + // to a best-effort lexical check. + _ => dep_dir.strip_prefix(root).is_err(), + }; + if escapes { + tracing::warn!( + target: "pacquet::git_fetcher::packlist", + bundle_name = %task.name, + dep_dir = %dep_dir.display(), + "bundled dependency resolves outside the package tree; refusing", + ); + continue; + } + if !visited.insert(canonical_dep.unwrap_or_else(|| dep_dir.clone())) { + continue; + } + let prefix = relative_forward_slash(root, &dep_dir); + let dep_manifest = safe_read_package_json_from_dir(&dep_dir) + .ok() + .flatten() + .unwrap_or_else(|| Value::Object(serde_json::Map::new())); + for rel in collect_own_files(&dep_dir, &dep_manifest)? { + out.insert(format!("{prefix}/{rel}")); + } + for name in nested_bundle_dep_names(&dep_manifest) { + queue.push_back(BundleTask { name, from_dir: dep_dir.clone(), depth: task.depth + 1 }); + } } - if depth > MAX_BUNDLE_DEPTH { - tracing::warn!( - target: "pacquet::git_fetcher::packlist", - pkg_dir = %pkg_dir.display(), - depth, - "bundleDependencies recursion exceeded MAX_BUNDLE_DEPTH; refusing to descend further", - ); - return Ok(Vec::new()); + Ok(()) +} + +/// Resolve a bundled dependency `name` to its directory using the +/// node module-resolution walk-up: check `from_dir/node_modules/name`, +/// then climb to each ancestor's `node_modules/`, stopping at `root`. +/// Returns the first directory that contains a `package.json`, or +/// `None` if the name resolves nowhere within the package tree. +/// +/// Climbing past `root` is refused so a hoisted dep always resolves to +/// the package being packed rather than to a sibling on the host. +fn resolve_bundled_dependency(name: &str, from_dir: &Path, root: &Path) -> Option { + let mut current = from_dir.to_path_buf(); + loop { + let candidate = current.join("node_modules").join(name); + if candidate.join("package.json").is_file() { + return Some(candidate); + } + if current == root { + return None; + } + let parent = current.parent()?; + if parent == current { + return None; + } + current = parent.to_path_buf(); } +} + +/// Collect the forward-slash relative paths for a single package's own +/// files — the `.npmignore` / `.gitignore` walk, the `files`-field +/// allowlist, and the always-included / `main` / `bin` force-includes. +/// This is the per-package packlist with no `bundleDependencies` +/// traversal; [`collect_bundled_files`] layers the closure on top. +fn collect_own_files(pkg_dir: &Path, manifest: &Value) -> Result, PacklistError> { let files_field = manifest.get("files").and_then(Value::as_array); let files_matcher: Option = files_field.and_then(|arr| build_files_matcher(pkg_dir, arr)); @@ -177,9 +301,9 @@ fn packlist_inner( if should_always_exclude(&rel) { continue; } - // `node_modules/` is handled by the `bundleDependencies` - // pass below — never include its contents via the general - // walk. Without this gate a manifest that publishes a stray + // `node_modules/` contents are bundled by + // `collect_bundled_files`, never via this general walk. + // Without this gate a manifest that publishes a stray // `node_modules/something` would slip through. if rel.starts_with("node_modules/") || rel == "node_modules" { continue; @@ -241,47 +365,7 @@ fn packlist_inner( } } - // Pass 4: recurse into `bundleDependencies` / - // `bundledDependencies`. Each bundled dep gets its own packlist - // pass; the result splices in under `node_modules//`. Both - // field names are accepted because some published packages use - // one and some the other (npm-packlist tolerates both). - for bundle_name in bundle_dep_names(manifest) { - // Defense-in-depth: a malicious manifest could carry - // `bundleDependencies: ["../../etc"]` (or an absolute path). - // Reject anything that's not a single safe segment before - // building the join path; let `is_safe_bundle_name` log the - // refusal so the gap is observable in install logs. - if !is_safe_bundle_name(&bundle_name) { - tracing::warn!( - target: "pacquet::git_fetcher::packlist", - bundle_name = %bundle_name, - pkg_dir = %pkg_dir.display(), - "rejecting bundleDependencies entry that is not a single path segment", - ); - continue; - } - let bundle_pkg_dir = pkg_dir.join("node_modules").join(&bundle_name); - if !bundle_pkg_dir.is_dir() { - tracing::debug!( - target: "pacquet::git_fetcher::packlist", - bundle_name = %bundle_name, - pkg_dir = %pkg_dir.display(), - "bundleDependencies entry not present under node_modules/; skipping", - ); - continue; - } - let bundle_manifest = safe_read_package_json_from_dir(&bundle_pkg_dir) - .ok() - .flatten() - .unwrap_or_else(|| Value::Object(serde_json::Map::new())); - let nested = packlist_inner(&bundle_pkg_dir, &bundle_manifest, visited, depth + 1)?; - for rel in nested { - out.insert(format!("node_modules/{bundle_name}/{rel}")); - } - } - - Ok(out.into_iter().collect()) + Ok(out) } /// Compile the `manifest.files` allowlist into a single `Gitignore` @@ -413,10 +497,11 @@ fn is_safe_bundle_name(name: &str) -> bool { true } -/// Collect names from `bundleDependencies` (or the legacy -/// `bundledDependencies`). Both spellings appear in real published -/// packages; npm-packlist accepts either. -fn bundle_dep_names(manifest: &Value) -> Vec { +/// Seed names for the bundle closure: the root manifest's +/// `bundleDependencies` (or the legacy `bundledDependencies`). Both +/// spellings appear in real published packages; npm-packlist accepts +/// either. +fn root_bundle_dep_names(manifest: &Value) -> Vec { let raw = manifest.get("bundleDependencies").or_else(|| manifest.get("bundledDependencies")); let Some(raw) = raw else { return Vec::new() }; match raw { @@ -435,6 +520,23 @@ fn bundle_dep_names(manifest: &Value) -> Vec { } } +/// Names a bundled package pulls into the closure: every key in its +/// own `dependencies` and `optionalDependencies`. A bundled package +/// ships its whole runtime closure, so these are followed regardless +/// of whether the nested package declares its own `bundleDependencies` +/// (already-bundled packages don't re-gate their deps). `peer`- and +/// `dev`-dependencies are deliberately excluded — they are not part of +/// the published closure. Mirrors `npm-bundled`'s `getDeps`. +fn nested_bundle_dep_names(manifest: &Value) -> Vec { + let mut names = Vec::new(); + for field in ["dependencies", "optionalDependencies"] { + if let Some(map) = manifest.get(field).and_then(Value::as_object) { + names.extend(map.keys().cloned()); + } + } + names +} + fn relative_forward_slash(root: &Path, full: &Path) -> String { let rel = full.strip_prefix(root).unwrap_or(full); let mut buf = PathBuf::from(rel).into_os_string().to_string_lossy().into_owned(); diff --git a/pacquet/crates/git-fetcher/src/packlist/tests.rs b/pacquet/crates/git-fetcher/src/packlist/tests.rs index a79ff97dfa..dab41f25aa 100644 --- a/pacquet/crates/git-fetcher/src/packlist/tests.rs +++ b/pacquet/crates/git-fetcher/src/packlist/tests.rs @@ -4,11 +4,15 @@ use std::{fs, path::Path}; use tempfile::tempdir; fn touch(root: &Path, rel: &str) { + write(root, rel, ""); +} + +fn write(root: &Path, rel: &str, contents: &str) { let path = root.join(rel); if let Some(parent) = path.parent() { fs::create_dir_all(parent).unwrap(); } - fs::write(path, "").unwrap(); + fs::write(path, contents).unwrap(); } #[test] @@ -265,6 +269,115 @@ fn bundle_dependencies_subtree_is_included() { ); } +#[test] +fn bundle_dependencies_pull_in_hoisted_transitive_deps() { + // Port of pnpm's `pack: bundles transitive dependencies of bundled + // dependencies (hoisted)` + // ([pnpm11/releasing/commands/test/publish/pack.ts](https://github.com/pnpm/pnpm/blob/dd79bdc08e/pnpm11/releasing/commands/test/publish/pack.ts#L161-L191)). + // `top` is bundled and + // declares `dependencies: { nested }`; `nested` is hoisted to the + // root `node_modules`. The closure must follow `top`'s + // dependencies and resolve `nested` via the walk-up to the root, + // splicing it in at `node_modules/nested/`. + let dir = tempdir().unwrap(); + let root = dir.path(); + touch(root, "package.json"); + write( + root, + "node_modules/top/package.json", + r#"{"name":"top","version":"1.0.0","dependencies":{"nested":"1.0.0"}}"#, + ); + touch(root, "node_modules/top/index.js"); + write(root, "node_modules/nested/package.json", r#"{"name":"nested","version":"1.0.0"}"#); + touch(root, "node_modules/nested/index.js"); + + let manifest = json!({ + "name": "x", + "version": "0.0.0", + "bundledDependencies": ["top"], + }); + let out = packlist(root, &manifest).unwrap(); + + assert!(out.contains(&"node_modules/top/index.js".to_string()), "{out:?}"); + assert!( + out.contains(&"node_modules/nested/index.js".to_string()), + "hoisted transitive dep `nested` must be bundled: {out:?}", + ); +} + +#[test] +fn bundle_dependencies_follow_nested_node_modules_before_hoisted() { + // A bundled dep's own `node_modules/` wins over a hoisted copy + // at the root, matching node module resolution. + let dir = tempdir().unwrap(); + let root = dir.path(); + touch(root, "package.json"); + write( + root, + "node_modules/top/package.json", + r#"{"name":"top","version":"1.0.0","dependencies":{"nested":"2.0.0"}}"#, + ); + write( + root, + "node_modules/top/node_modules/nested/package.json", + r#"{"name":"nested","version":"2.0.0"}"#, + ); + touch(root, "node_modules/top/node_modules/nested/nested-v2.js"); + write(root, "node_modules/nested/package.json", r#"{"name":"nested","version":"1.0.0"}"#); + touch(root, "node_modules/nested/hoisted-v1.js"); + + let manifest = json!({ + "name": "x", + "version": "0.0.0", + "bundleDependencies": ["top"], + }); + let out = packlist(root, &manifest).unwrap(); + + assert!( + out.contains(&"node_modules/top/node_modules/nested/nested-v2.js".to_string()), + "nested copy of `nested` must be bundled: {out:?}", + ); + assert!( + !out.contains(&"node_modules/nested/hoisted-v1.js".to_string()), + "hoisted `nested` is shadowed by the nested copy and must not ship: {out:?}", + ); +} + +#[test] +fn bundle_dependencies_optional_deps_of_bundled_dep_are_included() { + // `optionalDependencies` are part of a bundled package's runtime + // closure, so they ship; `devDependencies` do not. + let dir = tempdir().unwrap(); + let root = dir.path(); + touch(root, "package.json"); + write( + root, + "node_modules/top/package.json", + r#"{"name":"top","version":"1.0.0","optionalDependencies":{"opt":"1.0.0"},"devDependencies":{"dev":"1.0.0"}}"#, + ); + touch(root, "node_modules/top/index.js"); + write(root, "node_modules/opt/package.json", r#"{"name":"opt","version":"1.0.0"}"#); + touch(root, "node_modules/opt/index.js"); + write(root, "node_modules/dev/package.json", r#"{"name":"dev","version":"1.0.0"}"#); + touch(root, "node_modules/dev/index.js"); + + let manifest = json!({ + "name": "x", + "version": "0.0.0", + "bundleDependencies": ["top"], + }); + let out = packlist(root, &manifest).unwrap(); + + assert!( + out.contains(&"node_modules/opt/index.js".to_string()), + "optionalDependencies of a bundled dep must ship: {out:?}", + ); + assert!( + !out.iter().any(|p| p.starts_with("node_modules/dev")), + "devDependencies of a bundled dep must not ship: {out:?}", + ); +} + #[test] fn bundled_dependencies_legacy_spelling_works() { let dir = tempdir().unwrap(); @@ -352,6 +465,39 @@ fn bundle_dependencies_rejects_path_traversal() { ); } +#[cfg(unix)] +#[test] +fn bundle_dependency_symlink_escaping_pkg_dir_is_refused() { + // Defense-in-depth: the bundle name is a single safe segment + // (`is_safe_bundle_name` accepts it), but `node_modules/` is + // a symlink pointing outside the package. Resolving and walking it + // would splice host files into the published set. The fetcher + // imports untrusted git-hosted packages, so this must be refused. + let dir = tempdir().unwrap(); + let root = dir.path().join("pkg"); + fs::create_dir_all(root.join("node_modules")).unwrap(); + touch(&root, "package.json"); + // A sibling directory outside the package, made to look like a real + // package so the walk-up resolves it. + let escape = dir.path().join("escape"); + fs::create_dir_all(&escape).unwrap(); + fs::write(escape.join("package.json"), r#"{"name":"evil","version":"1.0.0"}"#).unwrap(); + fs::write(escape.join("secret.txt"), "DO NOT EXFIL\n").unwrap(); + std::os::unix::fs::symlink(&escape, root.join("node_modules/evil")).unwrap(); + + let manifest = json!({ + "name": "x", + "version": "0.0.0", + "bundleDependencies": ["evil"], + }); + let out = packlist(&root, &manifest).unwrap(); + + assert!( + !out.iter().any(|path| path.contains("secret")), + "a node_modules symlink escaping pkg_dir must not leak host files: {out:?}", + ); +} + #[test] fn always_excluded_dir_segments_only_match_vcs() { let dir = tempdir().unwrap(); @@ -412,27 +558,29 @@ fn files_field_bare_basename_matches_at_depth() { #[test] fn bundle_dependencies_self_cycle_is_caught() { - // Defense-in-depth: a bundled dep whose own manifest declares a - // bundle pointing back at itself (or any cycle reachable through - // the canonical-path chain) must not stack-overflow the fetcher. - // The visited-set + depth cap inside `packlist_inner` stops the - // recursion. + // Defense-in-depth: a bundled dep whose own manifest depends on + // itself (or any cycle reachable through the canonical-path chain) + // must not loop the closure walk forever. The visited-set keyed on + // the canonicalised resolved directory stops the re-entry. let dir = tempdir().unwrap(); let root = dir.path(); touch(root, "package.json"); touch(root, "node_modules/self/package.json"); touch(root, "node_modules/self/lib.js"); - // The bundled dep declares itself as a bundleDependency. Without - // cycle detection this becomes infinite recursion. + // The bundled dep lists itself as a runtime dependency. The + // closure follows `dependencies`, so without cycle detection this + // re-resolves `self` forever. fs::write( root.join("node_modules/self/package.json"), - r#"{"name":"self","version":"1.0.0","bundleDependencies":["self"]}"#, + r#"{"name":"self","version":"1.0.0","dependencies":{"self":"1.0.0"}}"#, ) .unwrap(); // Symlink `node_modules/self/node_modules/self` back to the - // outer `node_modules/self` so the canonical-path check has - // something to catch. (On platforms that can't symlink, the - // depth cap kicks in instead.) + // outer `node_modules/self` so the nested-first walk-up resolves + // the self-dependency to a directory the canonical-path check + // recognises as already visited. (On platforms that can't + // symlink, the walk-up falls back to the same outer directory and + // the visited-set still catches it.) fs::create_dir_all(root.join("node_modules/self/node_modules")).unwrap(); #[cfg(unix)] { @@ -453,8 +601,8 @@ fn bundle_dependencies_self_cycle_is_caught() { assert!(out.contains(&"package.json".to_string())); assert!(out.contains(&"node_modules/self/package.json".to_string())); assert!(out.contains(&"node_modules/self/lib.js".to_string())); - // No deeper paths via the cycle — the visited-set / depth cap - // refused the re-entry. + // No deeper paths via the cycle — the visited-set refused the + // re-entry. assert!( !out.iter().any(|p| p.starts_with("node_modules/self/node_modules/")), "cycle through node_modules/self/node_modules/self/... must be cut: {out:?}",