diff --git a/crates/socket-patch-cli/src/ecosystem_dispatch.rs b/crates/socket-patch-cli/src/ecosystem_dispatch.rs index a733b3179..913c1181e 100644 --- a/crates/socket-patch-cli/src/ecosystem_dispatch.rs +++ b/crates/socket-patch-cli/src/ecosystem_dispatch.rs @@ -142,8 +142,8 @@ pub fn partition_purls( /// Standard scan-one-ecosystem pattern: discover source paths, run /// `find_by_purls` on each, and merge results into `$out` keyed by PURL -/// (first wins). Used by every ecosystem except pypi (which dedups -/// PURLs and, on rollback, remaps base PURLs back to qualified ones). +/// with the ecosystem's `$on_match` merge (first-wins, or every copy for +/// npm, cargo and the release-variant ecosystems). /// /// `$using_label` is the noun in "Using at: " for global /// scans; pass `""` to suppress that line. The banner is progress chrome @@ -209,7 +209,8 @@ macro_rules! scan_ecosystem { }}; } -/// Signature shared by `merge_first_wins` and `merge_qualified`. +/// Signature shared by the `dispatch_find` merges (`merge_first_wins`, +/// `merge_cargo_copies`, `merge_variant_copies`, `merge_qualified`). /// `dispatch_find` swaps between them so the rollback path can fan one /// crawler result back out to every caller-supplied qualified PURL. The /// output map holds a `Vec` of paths per PURL: most ecosystems install one @@ -228,8 +229,8 @@ fn push_path(out: &mut HashMap>, purl: String, path: PathBu } } -/// Default merge for the single-copy ecosystems (cargo / go / composer / -/// nuget / deno): keep the FIRST path discovered per PURL, matching the +/// Default merge for the single-copy ecosystems (go / composer / nuget / +/// deno): keep the FIRST path discovered per PURL, matching the /// historical `HashMap` first-wins contract exactly. These /// ecosystems resolve one logical install per `name@version`, but the same /// install is legitimately reachable from several source roots (e.g. NuGet's @@ -237,9 +238,9 @@ fn push_path(out: &mut HashMap>, purl: String, path: PathBu /// would re-apply to what is effectively the same package — a scope-expanding /// behavior change that is out of scope for the npm multi-copy fix and would, /// for a shared global cache, mutate state other projects rely on. Fanning out -/// to genuinely-distinct installs is npm-only (`merge_npm_copies`); if a -/// per-root fan-out is ever wanted for these ecosystems it must be an explicit, -/// separately-tested decision. +/// to genuinely-distinct installs is an explicit, separately-tested decision +/// per ecosystem: npm (`merge_npm_copies`) and cargo's registry index dirs +/// (`merge_cargo_copies`). fn merge_first_wins( out: &mut HashMap>, _purls: &[String], @@ -277,6 +278,51 @@ fn merge_variant_copies( } } +/// Cargo merge: keep EVERY crates.io copy, one per registry index dir. +/// The shared registry cache extracts a crate once per index dir +/// (`index.crates.io-6f17d22bba15001f` for cargo 1.70–1.84, +/// `index.crates.io-1949cf8c6b5b557f` for cargo ≥ 1.85, the pre-1.70 git +/// index), and each cargo builds from its own. First-wins patched, checked +/// and restored only the copy in the first dir in listing order, so a +/// project on the other cargo kept building the unpatched crate while +/// `apply` reported success and `vex` attested it (#339). +/// +/// A `pkg:cargo` PURL names a crates.io crate, so a same-`name@version` +/// copy in another registry's dir (a private registry, a hosted Socket +/// patch registry) is a different crate: it is kept only when no crates.io +/// copy exists, as the single first-found copy it always was. A +/// `cargo vendor` tree or `--global-prefix` is one root, so one copy. +fn merge_cargo_copies( + out: &mut HashMap>, + _purls: &[String], + packages: HashMap, +) { + for (purl, pkg) in packages { + let paths = out.entry(purl).or_default(); + if paths.is_empty() { + paths.push(pkg.path); + } else if is_crates_io_copy(&pkg.path) && !paths.contains(&pkg.path) { + if paths.iter().all(|p| is_crates_io_copy(p)) { + paths.push(pkg.path); + } else { + // The first-found copy belongs to another registry. + *paths = vec![pkg.path]; + } + } + } +} + +/// Whether `crate_dir` sits in a crates.io index dir of the registry +/// cache: `index.crates.io-` (sparse, cargo ≥ 1.70) or the git +/// index's `github.com-1ecc6299db9ec823` (cargo < 1.70). +fn is_crates_io_copy(crate_dir: &std::path::Path) -> bool { + crate_dir + .parent() + .and_then(|p| p.file_name()) + .and_then(|n| n.to_str()) + .is_some_and(|n| n.starts_with("index.crates.io-") || n == "github.com-1ecc6299db9ec823") +} + /// npm merge: the npm crawler returns EVERY physical copy of each PURL /// (nested duplicates, diamonds, `file:` dups), so fold every path in. /// A one-path `HashMap` would silently drop every copy @@ -372,8 +418,8 @@ fn passthrough_purls(purls: &[String]) -> Vec { /// into one `purl -> path` map. /// /// `variant_merge` lets the rollback variant fan a single crawler result -/// out to every caller-supplied qualified PURL; everything else just -/// inserts the crawler-returned PURL with first-wins semantics. It is +/// out to every caller-supplied qualified PURL; every other ecosystem +/// inserts the crawler-returned PURL with its own fixed merge. It is /// applied to the release-variant ecosystems (PyPI / RubyGems / Maven), /// which are also queried with deduped base PURLs. /// @@ -430,7 +476,8 @@ async fn dispatch_find( using_label = "cargo crate sources", err_label = "Cargo crates", purls_override = passthrough_purls, - on_match = merge_first_wins, + // One copy per registry index dir, each built by another cargo. + on_match = merge_cargo_copies, ); scan_ecosystem!( @@ -559,7 +606,8 @@ pub async fn find_all_packages_for_purls( // (base-PURL keyed) instead of first-wins: the gem crawler surfaces // coexisting bundler stores whose copies apply must ALL patch. The // rollback variant below gets the same multi-copy carry from - // `merge_qualified`'s `push_path`. Single-copy ecosystems keep true + // `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 @@ -1172,7 +1220,7 @@ mod tests { // keeps ONLY the first — matching the historical first-wins contract. // Fanning out to every root re-patches what is effectively one install // (the docker_e2e_nuget `already_patched` regression); genuine - // multi-copy fan-out is npm-only via `merge_npm_copies`. + // multi-copy fan-out is opt-in per ecosystem (npm, cargo). let mut out: HashMap> = HashMap::new(); merge_first_wins(&mut out, &[], packages(&[("pkg:cargo/foo@1.0", "/first")])); merge_first_wins(&mut out, &[], packages(&[("pkg:cargo/foo@1.0", "/second")])); @@ -1283,6 +1331,59 @@ mod tests { ); } + // ---- merge_cargo_copies ------------------------------------------------ + + #[test] + fn merge_cargo_copies_prefers_crates_io_over_another_registry() { + // A same-name@version crate from a private registry is another + // crate: never patched beside crates.io copies, whichever order the + // index dirs list in, but still the copy when it is the only one. + let purl = "pkg:cargo/foo@1.0.0"; + let corp = "/src/corp.example-0123456789abcdef/foo-1.0.0"; + let new = "/src/index.crates.io-1949cf8c6b5b557f/foo-1.0.0"; + let git = "/src/github.com-1ecc6299db9ec823/foo-1.0.0"; + let mut out: HashMap> = HashMap::new(); + for path in [corp, new, corp, git] { + merge_cargo_copies(&mut out, &[], packages(&[(purl, path)])); + } + assert_eq!( + out.get(purl), + Some(&vec![PathBuf::from(new), PathBuf::from(git)]) + ); + let mut out: HashMap> = HashMap::new(); + merge_cargo_copies(&mut out, &[], packages(&[(purl, corp)])); + assert_eq!(out.get(purl), Some(&vec![PathBuf::from(corp)])); + } + + #[test] + fn merge_cargo_copies_keeps_every_registry_index_copy() { + // #339: the macro calls on_match once per registry index dir; each + // dir's copy is built by a different cargo, so all are carried in + // discovery order, and a path re-observed is recorded once. + let mut out: HashMap> = HashMap::new(); + for path in [ + "/src/index.crates.io-1949cf8c6b5b557f/cfg-if-1.0.0", + "/src/index.crates.io-6f17d22bba15001f/cfg-if-1.0.0", + ] { + merge_cargo_copies(&mut out, &[], packages(&[("pkg:cargo/cfg-if@1.0.0", path)])); + } + merge_cargo_copies( + &mut out, + &[], + packages(&[( + "pkg:cargo/cfg-if@1.0.0", + "/src/index.crates.io-6f17d22bba15001f/cfg-if-1.0.0", + )]), + ); + assert_eq!( + out.get("pkg:cargo/cfg-if@1.0.0"), + Some(&vec![ + PathBuf::from("/src/index.crates.io-1949cf8c6b5b557f/cfg-if-1.0.0"), + PathBuf::from("/src/index.crates.io-6f17d22bba15001f/cfg-if-1.0.0"), + ]) + ); + } + // ---- merge_variant_copies ---------------------------------------------- #[test] diff --git a/crates/socket-patch-cli/tests/e2e_cargo.rs b/crates/socket-patch-cli/tests/e2e_cargo.rs index 5cdb171e4..fb5004047 100644 --- a/crates/socket-patch-cli/tests/e2e_cargo.rs +++ b/crates/socket-patch-cli/tests/e2e_cargo.rs @@ -552,3 +552,199 @@ async fn sync_keeps_shared_cache_entry_behind_a_cargo_vendor_dir() { ); assert_eq!(std::fs::read(itoa_old.join("src/lib.rs")).unwrap(), orig); } + +/// #339: `$CARGO_HOME/registry/src` holds one extracted copy of a crate +/// per registry index dir (`index.crates.io-6f17d22bba15001f` for cargo +/// 1.70–1.84, `index.crates.io-1949cf8c6b5b557f` for cargo ≥ 1.85), so +/// two toolchains sharing a `CARGO_HOME` each build their own copy. Agent +/// mode must patch, check, attest and restore EVERY copy, not just the +/// first index dir in listing order: otherwise the project built by the +/// other cargo compiles the unpatched crate while `apply` reports success +/// and `vex` attests `not_affected`. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn agent_mode_covers_every_registry_index_copy() { + let server = start_proxy().await; + let proxy_url = server.uri(); + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + let cwd = root.to_str().unwrap(); + + std::fs::write( + root.join("Cargo.toml"), + "[package]\nname = \"myapp\"\nversion = \"0.1.0\"\n\n[dependencies]\ncfg-if = \"=1.0.0\"\n", + ) + .unwrap(); + std::fs::write( + root.join("Cargo.lock"), + "version = 3\n\n[[package]]\nname = \"cfg-if\"\nversion = \"1.0.0\"\n\ + source = \"registry+https://github.com/rust-lang/crates.io-index\"\n\n\ + [[package]]\nname = \"myapp\"\nversion = \"0.1.0\"\ndependencies = [\n \"cfg-if\",\n]\n", + ) + .unwrap(); + + let orig: &[u8] = b"pub fn cfg() {}\n"; + let patched: &[u8] = b"pub fn cfg() {}\npub fn socket_patched() {}\n"; + let src = root.join(".cargo/registry/src"); + let copies = [ + fake_registry_crate( + &src.join("index.crates.io-1949cf8c6b5b557f"), + "cfg-if", + "1.0.0", + orig, + ), + fake_registry_crate( + &src.join("index.crates.io-6f17d22bba15001f"), + "cfg-if", + "1.0.0", + orig, + ), + ]; + // A private registry's crate of the same name@version is another + // crate: agent mode must leave it alone. + let corp = fake_registry_crate( + &src.join("corp.example-0123456789abcdef"), + "cfg-if", + "1.0.0", + b"pub fn corp() {}\n", + ); + let lib = |copy: &PathBuf| std::fs::read(copy.join("src/lib.rs")).unwrap(); + + let socket = root.join(".socket"); + let purl = "pkg:cargo/cfg-if@1.0.0"; + // vex attests only patches that carry vulnerability metadata. + let mut record = manifest_record( + &socket, + "33900000-0000-4000-8000-000000000001", + orig, + patched, + ); + record["vulnerabilities"] = serde_json::json!({ "GHSA-xxxx-xxxx-xxxx": { + "cves": ["CVE-2024-0001"], + "summary": "s", + "severity": "high", + "description": "d", + }}); + let manifest = serde_json::json!({ "patches": { purl: record } }); + std::fs::write( + socket.join("manifest.json"), + serde_json::to_string_pretty(&manifest).unwrap(), + ) + .unwrap(); + + // apply patches both copies. + let out = run( + &["apply", "--json", "--offline", "--cwd", cwd], + root, + &proxy_url, + ) + .await; + let stdout = String::from_utf8_lossy(&out.stdout); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + out.status.success(), + "apply must exit 0\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); + for copy in &copies { + assert_eq!( + lib(copy), + patched, + "apply left {} unpatched\nstdout:\n{stdout}", + copy.display() + ); + } + assert_eq!( + lib(&corp), + b"pub fn corp() {}\n", + "apply patched another registry's crate" + ); + + // Baselines: with every copy patched the check passes and vex attests. + let out = run( + &["apply", "--check", "--json", "--offline", "--cwd", cwd], + root, + &proxy_url, + ) + .await; + let stdout = String::from_utf8_lossy(&out.stdout); + assert!( + out.status.success(), + "apply --check must pass with every copy patched\nstdout:\n{stdout}" + ); + let vex_args = [ + "vex", + "--offline", + "--product", + "pkg:cargo/myapp@0.1.0", + "--cwd", + cwd, + ]; + let out = run(&vex_args, root, &proxy_url).await; + let stdout = String::from_utf8_lossy(&out.stdout); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + out.status.success() && stdout.contains("not_affected"), + "vex must attest with every copy patched\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); + + // A copy re-extracted pristine (another cargo fetched it again) must + // fail the CI gate instead of reading as in sync. + std::fs::write(copies[1].join("src/lib.rs"), orig).unwrap(); + let out = run( + &["apply", "--check", "--json", "--offline", "--cwd", cwd], + root, + &proxy_url, + ) + .await; + let stdout = String::from_utf8_lossy(&out.stdout); + assert!( + !out.status.success(), + "apply --check must fail while {} is unpatched\nstdout:\n{stdout}", + copies[1].display() + ); + + // ... and must keep vex from attesting the patch. + let out = run(&vex_args, root, &proxy_url).await; + let stdout = String::from_utf8_lossy(&out.stdout); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + !stdout.contains("not_affected"), + "vex attested a patch one built copy lacks\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); + + // Re-apply fixes the pristine copy; rollback then restores both. + let out = run( + &["apply", "--json", "--offline", "--cwd", cwd], + root, + &proxy_url, + ) + .await; + assert!(out.status.success(), "re-apply must exit 0"); + for copy in &copies { + assert_eq!(lib(copy), patched, "re-apply left {}", copy.display()); + } + let out = run( + &["rollback", purl, "--json", "--offline", "--cwd", cwd], + root, + &proxy_url, + ) + .await; + let stdout = String::from_utf8_lossy(&out.stdout); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + out.status.success(), + "rollback must exit 0\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); + for copy in &copies { + assert_eq!( + lib(copy), + orig, + "rollback left {} patched\nstdout:\n{stdout}", + copy.display() + ); + } + assert_eq!( + lib(&corp), + b"pub fn corp() {}\n", + "rollback touched another registry's crate" + ); +} diff --git a/crates/socket-patch-cli/tests/in_process_rollback_all_ecosystems.rs b/crates/socket-patch-cli/tests/in_process_rollback_all_ecosystems.rs index faf628011..d0f39ddec 100644 --- a/crates/socket-patch-cli/tests/in_process_rollback_all_ecosystems.rs +++ b/crates/socket-patch-cli/tests/in_process_rollback_all_ecosystems.rs @@ -26,7 +26,7 @@ //! merge functions behave identically, so the test would prove nothing. //! //! npm / cargo / golang / composer / nuget are NOT release-variant -//! ecosystems (their crawlers wire `merge_first_wins` in both resolvers), so a +//! ecosystems (their crawlers wire a fixed merge in both resolvers), so a //! qualified PURL there is genuinely unsupported and those fixtures keep //! bare PURLs. diff --git a/docs/ecosystems.md b/docs/ecosystems.md index d79318605..367b8c0dc 100644 --- a/docs/ecosystems.md +++ b/docs/ecosystems.md @@ -1099,6 +1099,12 @@ crate that means the **shared** `$CARGO_HOME/registry` cache: the patch affects project on the machine, and is silently reset by `cargo clean` or a cache prune. Use `--mode vendored` for a project-local, committable patch. +The cache holds one extracted copy of a crate per registry index directory under +`registry/src` (cargo 1.70–1.84 and cargo 1.85+ use different ones), so two toolchains +sharing a `CARGO_HOME` each build their own copy. `apply`, `apply --check`, `vex` and +`rollback` cover every copy: an unpatched one fails the check and keeps `vex` from +attesting the patch. + Nothing deletes a crate from the cache when the project stops using it, so the manifest record stays the only way to restore that copy: `rollback` (and `remove`) also restore it after the crate was moved to vendored mode, and `scan --prune` /