diff --git a/crates/socket-patch-cli/src/commands/vex_consumed.rs b/crates/socket-patch-cli/src/commands/vex_consumed.rs index b1cb2254e..94af3f22a 100644 --- a/crates/socket-patch-cli/src/commands/vex_consumed.rs +++ b/crates/socket-patch-cli/src/commands/vex_consumed.rs @@ -641,17 +641,21 @@ mod tests { ..GlobalArgs::default() }; let purl = "pkg:npm/left-pad@1.3.0".to_string(); - // The resolver never takes a link as a copy. - assert!(installed_copies(&common, &purl) + let mut expected = vec![alias.clone(), peers[1].clone()]; + expected.sort(); + // The resolver takes a root link only for a purl with no copy + // (#1494), and expands it to the same copies. + let mut resolved = installed_copies(&common, &purl) .await - .get(&purl) - .is_none_or(Vec::is_empty)); + .remove(&purl) + .unwrap(); + resolved.sort(); + assert_eq!(resolved, expected, "the resolver's own copy set"); + // An empty installed entry still falls back to the alias identity. let installed = HashMap::from([(purl, Vec::new())]); let (mut paths, calls) = tracked_npm_hosted(&common, &installed).await; - assert_eq!(calls, vec![vec![alias.clone()]]); - let mut expected = vec![alias, peers[1].clone()]; + assert_eq!(calls, vec![vec![alias]]); paths.sort(); - expected.sort(); assert_eq!(paths, expected); } @@ -746,9 +750,12 @@ mod tests { &purls[0], ) .await; - assert!( - resolved.get(&purls[0]).is_none_or(Vec::is_empty), - "the resolver skips symlinks: {resolved:?}" + // The crawler skips symlinks; the resolver takes the root link + // only because the purl has no other copy (#1494). + assert_eq!( + resolved.get(&purls[0]), + Some(&vec![root.join("node_modules/lp")]), + "{resolved:?}" ); let (_, _, _, snapshot) = diff --git a/crates/socket-patch-cli/src/ecosystem_dispatch.rs b/crates/socket-patch-cli/src/ecosystem_dispatch.rs index d516c0341..82595ae59 100644 --- a/crates/socket-patch-cli/src/ecosystem_dispatch.rs +++ b/crates/socket-patch-cli/src/ecosystem_dispatch.rs @@ -619,8 +619,17 @@ pub async fn find_all_packages_for_purls( // `merge_qualified`'s `push_path`. Cargo keeps every registry index // copy (`merge_cargo_copies`); the single-copy ecosystems keep true // first-wins via their own `merge_first_wins` wiring in - // `dispatch_find`. - dispatch_find(partitioned, options, silent, merge_variant_copies, None).await + // `dispatch_find`. An npm purl with no copy can still be reached + // through a workspace member's alias link (#1494). + dispatch_find_with_npm_links( + partitioned, + options, + silent, + merge_variant_copies, + None, + NpmLinks::ForUnresolved, + ) + .await } /// Multi-copy variant of `find_packages_for_rollback` (qualified-aware @@ -634,42 +643,88 @@ pub async fn find_all_packages_for_rollback( } /// [`find_all_packages_for_rollback`], taking the npm `node_modules` roots -/// from `prior` as [`find_packages_for_rollback_reusing`] does. +/// from `prior` as [`find_packages_for_rollback_reusing`] does. An npm purl +/// with no copy can still be reached through a workspace member's alias +/// link (#1494), as for apply. pub async fn find_all_packages_for_rollback_reusing( partitioned: &HashMap>, options: &CrawlerOptions, silent: bool, prior: Option<&NpmCrawlSnapshot>, ) -> HashMap> { - let npm_roots = prior - .filter(|p| p.taken_with(options)) - .map(|p| p.roots.as_slice()); - dispatch_find(partitioned, options, silent, merge_qualified, npm_roots).await + dispatch_find_with_npm_links( + partitioned, + options, + silent, + merge_qualified, + prior, + NpmLinks::ForUnresolved, + ) + .await } /// The install paths that reach each purl, for `scan`'s and `rollback`'s -/// path targets: [`find_all_packages_for_rollback_reusing`]'s physical -/// copies, plus each npm package link directly in a `node_modules` root -/// (scoped ones one level down) whose target's `package.json` names one -/// of the npm purls asked for. +/// path targets: [`find_all_packages_for_rollback_reusing`]'s copies plus +/// every matching npm root link, not only those of purls with no copy +/// (#1266). The result is for path matching only: it can name one +/// physical copy more than once. +pub async fn find_package_paths_for_path_scope( + partitioned: &HashMap>, + options: &CrawlerOptions, + silent: bool, + prior: Option<&NpmCrawlSnapshot>, +) -> HashMap> { + dispatch_find_with_npm_links( + partitioned, + options, + silent, + merge_qualified, + prior, + NpmLinks::All, + ) + .await +} + +/// Which npm root links [`dispatch_find_with_npm_links`] adds. +#[derive(Clone, Copy, PartialEq, Eq)] +enum NpmLinks { + /// Every link to a purl asked for: path scoping matches by install + /// path, so each spelling counts. + All, + /// Only links to purls the resolver found no copy of. The commands + /// that act per copy already reach every store peer variant of a + /// found copy through the apply engine's fan-out, so a link to one + /// would be a second visit (#633); and a link into first-party source + /// beside a found registry copy must not turn into a refusal. + ForUnresolved, +} + +/// [`dispatch_find`]'s copies, plus each npm package link directly in a +/// `node_modules` root (scoped ones one level down) whose target's +/// `package.json` names one of the npm purls asked for (see [`NpmLinks`] +/// for which), after the copies. /// /// The crawler records a plain-name link at its own path /// (`packages/a/node_modules/ms`), but never an alias link /// (`packages/a/node_modules/lp -> …/.pnpm/left-pad@1.3.0/node_modules/left-pad`): -/// links are not physical copies, and apply and VEX must see each copy -/// once (#856). Path scoping asks which paths install the package -/// instead, and pnpm's isolated linker or npm's `install-strategy=linked` -/// reach a workspace member's ALIAS dependency only through such a link -/// (#1266). A link is matched by its target's own identity, not against -/// the crawled copies, so a link to a store peer variant the crawl -/// skipped still counts, and the cost is one pass over the root links -/// however many purls are asked for. The result is for path matching -/// only: it can name one physical copy more than once. -pub async fn find_package_paths_for_path_scope( +/// links are not physical copies, and the resolver must see each copy +/// once (#856). Yet pnpm's isolated linker, vlt and npm's +/// `install-strategy=linked` reach a workspace member's ALIAS dependency +/// only through such a link. Path scoping needs it to select the member +/// (#1266), and a run from inside the member, whose `node_modules` is the +/// only root, has no other route to the copy at all (#1494). A link is +/// matched by its target's own identity, not against the crawled copies, +/// so a link to a store peer variant the crawl skipped still counts, and +/// the cost is one pass over the root links however many purls are asked +/// for (none when every purl already has a copy and only the unresolved +/// ones' links are wanted). +async fn dispatch_find_with_npm_links( partitioned: &HashMap>, options: &CrawlerOptions, silent: bool, + variant_merge: MergeFn, prior: Option<&NpmCrawlSnapshot>, + links: NpmLinks, ) -> HashMap> { let mut wanted: HashMap> = HashMap::new(); for purl in partitioned.get(&Ecosystem::Npm).into_iter().flatten() { @@ -687,13 +742,23 @@ pub async fn find_package_paths_for_path_scope( partitioned, options, silent, - merge_qualified, + variant_merge, npm_roots.as_deref(), ) .await; let Some(roots) = npm_roots else { return found; }; + if links == NpmLinks::ForUnresolved { + let resolved = |purl: &String| found.get(purl).is_some_and(|paths| !paths.is_empty()); + wanted.retain(|_, purls| { + purls.retain(|purl| !resolved(purl)); + !purls.is_empty() + }); + if wanted.is_empty() { + return found; + } + } // A failed enumeration only loses the extra links; the copies stand. let links = tokio::task::spawn_blocking(move || npm_links_to(&wanted, &roots)) .await @@ -794,17 +859,25 @@ pub async fn find_packages_for_rollback( /// tree nothing has touched since) instead of walking the tree for them /// again. Only the root discovery is reused: each root is still searched /// by `find_by_purls`, so copy choice and order are unchanged. A snapshot -/// taken with other options is ignored. +/// taken with other options is ignored. An npm purl with no copy can still +/// be reached through a workspace member's alias link (#1494). pub async fn find_packages_for_rollback_reusing( partitioned: &HashMap>, options: &CrawlerOptions, silent: bool, prior: Option<&NpmCrawlSnapshot>, ) -> HashMap { - let npm_roots = prior - .filter(|p| p.taken_with(options)) - .map(|p| p.roots.as_slice()); - collapse_to_first(dispatch_find(partitioned, options, silent, merge_qualified, npm_roots).await) + collapse_to_first( + dispatch_find_with_npm_links( + partitioned, + options, + silent, + merge_qualified, + prior, + NpmLinks::ForUnresolved, + ) + .await, + ) } /// The npm half of one [`crawl_ecosystems_with_npm`] run: the packages @@ -977,6 +1050,9 @@ pub(crate) fn npm_paths_by_identity_in( /// [`find_packages_for_rollback_reusing`]. Only the root discovery is /// reused: each root is still searched by `find_by_purls`, so copy choice /// and order are unchanged. A snapshot taken with other options is ignored. +/// An npm purl with no copy can still be reached through a workspace +/// member's alias link (#1494), so `apply --check` and `vex` see the copy +/// `apply` patched. /// /// Read-only by contract (its one caller is `vex`), so it also searches the /// gem stores under a refused out-of-tree `.bundle/config` path @@ -990,13 +1066,13 @@ pub async fn find_manifest_package_copies_reusing( ) -> HashMap> { let partitioned = partition_purls(purls, common.ecosystems.as_deref()); let crawler_options = common.crawler_options(); - let npm_roots = prior.and_then(|p| p.roots_for(&crawler_options)); - let mut copies = dispatch_find( + let mut copies = dispatch_find_with_npm_links( &partitioned, &crawler_options, quiet, merge_qualified, - npm_roots, + prior, + NpmLinks::ForUnresolved, ) .await; // `apply` also writes every store variant of each npm copy (a pnpm peer @@ -2058,6 +2134,59 @@ mod tests { assert_eq!(single.get("pkg:npm/dup@1.0.0"), Some(&root_copy)); } + /// #1494: run from a workspace member, a member's alias link into the + /// root store (`packages/a/node_modules/lp`, vlt's layout here) is the + /// only route to the copy, so the copy lookups must return it; a link + /// to another version must not count. From the root the store copy is + /// found and the link is not added: the apply engine's store fan-out + /// already reaches a link's peer variant, and a second visit would be + /// #633's phantom `already_patched`. Path scoping still sees it. + #[cfg(unix)] + #[tokio::test] + async fn multi_copy_lookups_take_a_member_alias_link_into_the_root_store() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + let store = root.join("node_modules/.vlt"); + let store_copy = write_npm_package(&store.join("~npm~left-pad@1.3.0"), "left-pad", "1.3.0"); + write_npm_package(&store.join("~npm~left-pad@1.2.0"), "left-pad", "1.2.0"); + let member_nm = root.join("packages/a/node_modules"); + std::fs::create_dir_all(&member_nm).unwrap(); + std::os::unix::fs::symlink( + "../../../node_modules/.vlt/~npm~left-pad@1.3.0/node_modules/left-pad", + member_nm.join("lp"), + ) + .unwrap(); + std::os::unix::fs::symlink( + "../../../node_modules/.vlt/~npm~left-pad@1.2.0/node_modules/left-pad", + member_nm.join("old"), + ) + .unwrap(); + let purl = "pkg:npm/left-pad@1.3.0".to_string(); + let partitioned = partition_purls(std::slice::from_ref(&purl), None); + + let member = local_options(root.join("packages/a")); + let link = vec![member_nm.join("lp")]; + let applied = find_all_packages_for_purls(&partitioned, &member, true).await; + assert_eq!(applied.get(&purl), Some(&link), "{applied:?}"); + let rolled = find_all_packages_for_rollback(&partitioned, &member, true).await; + assert_eq!(rolled.get(&purl), Some(&link), "{rolled:?}"); + let one = find_packages_for_rollback(&partitioned, &member, true).await; + assert_eq!(one.get(&purl), Some(&link[0]), "{one:?}"); + + let at_root = local_options(root.to_path_buf()); + let store_only = vec![store_copy.clone()]; + let applied = find_all_packages_for_purls(&partitioned, &at_root, true).await; + assert_eq!(applied.get(&purl), Some(&store_only), "{applied:?}"); + let rolled = find_all_packages_for_rollback(&partitioned, &at_root, true).await; + assert_eq!(rolled.get(&purl), Some(&store_only), "{rolled:?}"); + let scoped = find_package_paths_for_path_scope(&partitioned, &at_root, true, None).await; + assert_eq!( + scoped.get(&purl), + Some(&vec![store_copy, member_nm.join("lp")]), + "{scoped:?}" + ); + } + /// Multi-copy for gem (mirrors the npm test above): bundler's scoped /// (`//gems`) and flat (`gems/`) store layouts coexist under /// one `vendor/bundle` root — a bundler-2 `--path` install beside a diff --git a/crates/socket-patch-cli/tests/apply/in_process_npm_multicopy.rs b/crates/socket-patch-cli/tests/apply/in_process_npm_multicopy.rs index 6b6f02919..1ab246a34 100644 --- a/crates/socket-patch-cli/tests/apply/in_process_npm_multicopy.rs +++ b/crates/socket-patch-cli/tests/apply/in_process_npm_multicopy.rs @@ -811,6 +811,228 @@ fn apply_and_rollback_visit_a_pnpm_workspace_member_link_once() { ); } +/// #1494: run from inside a workspace member, a member's ALIAS dependency +/// (`"dv": "npm:dupvuln@1.0.0"`) under a root-store layout is only the link +/// `packages/a/node_modules/dv` into the root store, and the member's +/// `node_modules` is the only root the resolver walks. Apply used to call +/// it `package_not_installed` (exit 1) and rollback could not restore it, +/// though scan offered the patch through that link. From the root the same +/// copy is reached twice (store entry and member link) and must still be +/// visited once (#633). +#[cfg(unix)] +#[test] +fn apply_and_rollback_reach_a_member_alias_link_from_the_member() { + for (store, entry) in [ + (".vlt", "~npm~dupvuln@1.0.0"), + (".pnpm", "dupvuln@1.0.0"), + (".store", "dupvuln@1.0.0-0a1b2c3d"), + ] { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + let name = "dupvuln"; + let purl = "pkg:npm/dupvuln@1.0.0"; + let original = b"module.exports = function(){ return 'VULNERABLE'; };\n"; + let mut patched = original.to_vec(); + patched.extend_from_slice(b"// SOCKET-PATCHED-MULTICOPY\n"); + std::fs::write( + root.join("package.json"), + r#"{ "name": "root", "version": "1.0.0", "private": true }"#, + ) + .unwrap(); + std::fs::write(root.join("vlt.json"), r#"{ "workspaces": ["packages/*"] }"#).unwrap(); + let store_copy = write_copy( + &root + .join("node_modules") + .join(store) + .join(entry) + .join("node_modules") + .join(name), + name, + "1.0.0", + original, + ); + let member = root.join("packages").join("a"); + std::fs::create_dir_all(member.join("node_modules")).unwrap(); + std::fs::write( + member.join("package.json"), + r#"{ "name": "a", "version": "1.0.0", "dependencies": { "dv": "npm:dupvuln@1.0.0" } }"#, + ) + .unwrap(); + std::os::unix::fs::symlink( + format!("../../../node_modules/{store}/{entry}/node_modules/{name}"), + member.join("node_modules").join("dv"), + ) + .unwrap(); + for socket_root in [root, member.as_path()] { + stage_manifest_and_blob( + socket_root, + purl, + &git_sha256(original), + &git_sha256(&patched), + &patched, + ); + std::fs::write( + socket_root + .join(".socket") + .join("blobs") + .join(git_sha256(original)), + original, + ) + .unwrap(); + } + + let (code, v) = run_apply(&member); + assert_eq!(code, 0, "{store}: member apply; envelope={v}"); + assert_eq!(std::fs::read(&store_copy).unwrap(), patched, "{store}"); + assert_eq!( + dupvuln_events(&v), + vec![("applied".to_string(), String::new())], + "{store}: member apply; envelope={v}" + ); + + // `apply --check` (and `vex`, through the same lookup) must see the + // copy apply just patched, not report it missing. + let (code, v) = run_apply_with(&member, &["--check".as_ref()]); + assert_eq!(code, 0, "{store}: member check; envelope={v}"); + assert_eq!( + dupvuln_events(&v), + vec![("skipped".to_string(), "already_patched".to_string())], + "{store}: member check; envelope={v}" + ); + + let (code, v) = run_apply(&member); + assert_eq!(code, 0, "{store}: member re-apply; envelope={v}"); + assert_eq!( + dupvuln_events(&v), + vec![("skipped".to_string(), "already_patched".to_string())], + "{store}: member re-apply; envelope={v}" + ); + + let (code, v) = run_rollback_with(&member, &["--preserve-state".as_ref()]); + assert_eq!(code, 0, "{store}: member rollback; envelope={v}"); + assert_eq!(std::fs::read(&store_copy).unwrap(), original, "{store}"); + assert_eq!( + v["summary"]["rolledBack"], 1, + "{store}: member rollback; envelope={v}" + ); + + // From the root: the store entry and the member's link are one copy. + let (code, v) = run_apply(root); + assert_eq!(code, 0, "{store}: root apply; envelope={v}"); + assert_eq!(std::fs::read(&store_copy).unwrap(), patched, "{store}"); + assert_eq!( + dupvuln_events(&v), + vec![("applied".to_string(), String::new())], + "{store}: root apply; envelope={v}" + ); + let (code, v) = run_rollback(root); + assert_eq!(code, 0, "{store}: root rollback; envelope={v}"); + assert_eq!(std::fs::read(&store_copy).unwrap(), original, "{store}"); + assert_eq!( + v["summary"]["rolledBack"], 1, + "{store}: root rollback; envelope={v}" + ); + assert_eq!( + crate::rollback_json::already_original(&v), + 0, + "{store}: root rollback; envelope={v}" + ); + } +} + +/// #1494 / #633: the root's `dupvuln` link resolves to one pnpm peer +/// variant and member a's alias `dv` to another. Run from the root, the +/// apply engine's store fan-out from the root copy already patches the +/// member's variant, so the member's alias link must not add a second +/// visit (a phantom `already_patched` / already-original event). +#[cfg(unix)] +#[test] +fn apply_and_rollback_visit_a_member_alias_peer_variant_once() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + let name = "dupvuln"; + let purl = "pkg:npm/dupvuln@1.0.0"; + let original = b"module.exports = function(){ return 'VULNERABLE'; };\n"; + let mut patched = original.to_vec(); + patched.extend_from_slice(b"// SOCKET-PATCHED-MULTICOPY\n"); + std::fs::write( + root.join("package.json"), + r#"{ "name": "root", "version": "1.0.0", "private": true, "dependencies": { "dupvuln": "1.0.0" } }"#, + ) + .unwrap(); + std::fs::write( + root.join("pnpm-workspace.yaml"), + "packages:\n - 'packages/*'\n", + ) + .unwrap(); + let variants = ["dupvuln@1.0.0_react@17.0.2", "dupvuln@1.0.0_react@18.2.0"].map(|v| { + write_copy( + &root + .join("node_modules/.pnpm") + .join(v) + .join("node_modules") + .join(name), + name, + "1.0.0", + original, + ) + }); + std::os::unix::fs::symlink( + ".pnpm/dupvuln@1.0.0_react@17.0.2/node_modules/dupvuln", + root.join("node_modules").join(name), + ) + .unwrap(); + let member = root.join("packages").join("a"); + std::fs::create_dir_all(member.join("node_modules")).unwrap(); + std::fs::write( + member.join("package.json"), + r#"{ "name": "a", "version": "1.0.0", "dependencies": { "dv": "npm:dupvuln@1.0.0" } }"#, + ) + .unwrap(); + std::os::unix::fs::symlink( + "../../../node_modules/.pnpm/dupvuln@1.0.0_react@18.2.0/node_modules/dupvuln", + member.join("node_modules").join("dv"), + ) + .unwrap(); + stage_manifest_and_blob( + root, + purl, + &git_sha256(original), + &git_sha256(&patched), + &patched, + ); + std::fs::write( + root.join(".socket") + .join("blobs") + .join(git_sha256(original)), + original, + ) + .unwrap(); + + let (code, v) = run_apply(root); + assert_eq!(code, 0, "apply; envelope={v}"); + for variant in &variants { + assert_eq!(std::fs::read(variant).unwrap(), patched, "{variant:?}"); + } + assert_eq!( + dupvuln_events(&v), + vec![("applied".to_string(), String::new())], + "apply; envelope={v}" + ); + + let (code, v) = run_rollback(root); + assert_eq!(code, 0, "rollback; envelope={v}"); + for variant in &variants { + assert_eq!(std::fs::read(variant).unwrap(), original, "{variant:?}"); + } + assert_eq!(v["summary"]["rolledBack"], 1, "rollback; envelope={v}"); + assert_eq!( + crate::rollback_json::already_original(&v), + 0, + "rollback; envelope={v}" + ); +} + /// One pnpm 11+ isolated global install, `//node_modules`, with /// its own `.pnpm` holding a real `dupvuln@1.0.0`, linked at the top level /// (`direct`) or reached only through `wrapper` (transitive-only).