Fix cargo apply skipping other registry copies (#339)

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 <X> at: <path>" 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,18 +229,18 @@ fn push_path(out: &mut HashMap<String, Vec<PathBuf>>, 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<String, PathBuf>` 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
 /// global cache *and* a project-local packages folder). Patching each root
 /// 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<String, Vec<PathBuf>>,
     _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<String, Vec<PathBuf>>,
+    _purls: &[String],
+    packages: HashMap<String, CrawledPackage>,
+) {
+    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-<hash>` (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<String, PathBuf>` would silently drop every copy
@@ -372,8 +418,8 @@ fn passthrough_purls(purls: &[String]) -> Vec<String> {
 /// 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<String, Vec<PathBuf>> = 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<String, Vec<PathBuf>> = 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<String, Vec<PathBuf>> = 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<String, Vec<PathBuf>> = 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]

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"
+    );
+}