Fix npm crawler missing relocated dependency stores (#359, #362)

CHANGELOG.md

@@ -169,6 +169,11 @@ limits, and required install commands.
 - Transient apply locks are removed on normal command exit; no-op scans and full
   reversal avoid leaving unused `.socket/` state. Terminal output, telemetry
   timeouts, and update-check handling are more consistent.
+- Agent mode finds transitive npm packages in npm's linked store
+  (`install-strategy=linked`, `node_modules/.store`) and in a relocated pnpm
+  `virtualStoreDir`, instead of reporting them `package_not_installed` (#359,
+  #362). A store outside the project, such as pnpm's global virtual store, is
+  shared with other projects and is still not patched in place.
 - npm locks keep their own layout when edited. `scan --mode hosted`,
   `scan --mode vendored`, `rollback` and `vendor --revert`
   re-serialized `package-lock.json` / `npm-shrinkwrap.json` with LF line

crates/socket-patch-cli/tests/e2e_vendor_npm_build.rs

@@ -1273,3 +1273,124 @@ fn npm6_installs_a_vendored_v2_lock_from_its_legacy_mirror() {
         &[VexVia::Apply, VexVia::Vendor],
     );
 }
+
+/// The one real package dir npm's linked store holds for `name@version`
+/// (`node_modules/.store/<name>@<version>-<hash>/node_modules/<name>`).
+fn linked_store_copy(proj: &Path, name: &str, version: &str) -> Option<PathBuf> {
+    let prefix = format!("{name}@{version}-");
+    std::fs::read_dir(proj.join("node_modules/.store"))
+        .ok()?
+        .flatten()
+        .find(|e| e.file_name().to_string_lossy().starts_with(&prefix))
+        .map(|e| e.path().join("node_modules").join(name))
+}
+
+/// What Node loads for `dep` when `from` requires it.
+fn node_loads(proj: &Path, from: &str, dep: &str) -> String {
+    let script = format!(
+        "const p=require('path');process.stdout.write(require('fs').readFileSync(\
+         require.resolve('{dep}',{{paths:[p.dirname(require.resolve('{from}'))]}}),'utf8'))"
+    );
+    let out = Command::new("node")
+        .args(["-e", &script])
+        .current_dir(proj)
+        .output()
+        .expect("node runs");
+    assert!(
+        out.status.success(),
+        "{}",
+        npm_e2e_common::output_text(&out)
+    );
+    String::from_utf8_lossy(&out.stdout).into_owned()
+}
+
+/// #359: with `install-strategy=linked` (npm 9.4+), a transitive package
+/// is a real dir ONLY in `node_modules/.store`. `apply` must patch the
+/// copy Node loads, `rollback` must restore it, and `vendor` must build
+/// its tarball from it; before the fix all three reported
+/// `package_not_installed`.
+#[test]
+fn npm_linked_strategy_transitive_package_is_patched_rolled_back_and_vendored() {
+    let suite = "e2e_vendor_npm_build (linked)";
+    let Some(major) = npm_major_or_skip(suite) else {
+        return;
+    };
+    // `install-strategy=linked` arrived in npm 9.4.0 (measured: 9.0-9.3
+    // ignore it and install the hoisted tree).
+    let minor: u32 = npm_e2e_common::npm_version()
+        .and_then(|v| v.split('.').nth(1)?.parse().ok())
+        .unwrap_or(0);
+    if major < 9 || (major == 9 && minor < 4) {
+        println!("SKIP {suite}: npm {major}.{minor} has no install-strategy=linked");
+        return;
+    }
+    let tmp = tempfile::tempdir().unwrap();
+    let proj = tmp.path().join("proj");
+    std::fs::create_dir_all(&proj).unwrap();
+    std::fs::write(
+        proj.join("package.json"),
+        r#"{"name":"linked-capstone","version":"0.0.0","private":true}"#,
+    )
+    .unwrap();
+    std::fs::write(proj.join(".npmrc"), "install-strategy=linked\n").unwrap();
+    let cache = tmp.path().join("npm-cache");
+    // is-odd@3.0.1 depends on is-number@6.0.0: transitive, so store-only.
+    if !npm_e2e_common::install_fixture(suite, &proj, &cache, "is-odd@3.0.1") {
+        return;
+    }
+    let copy = linked_store_copy(&proj, "is-number", "6.0.0")
+        .expect("npm's linked strategy put is-number in node_modules/.store");
+    assert!(!proj.join("node_modules/is-number").exists());
+    let index = copy.join("index.js");
+    let orig = std::fs::read(&index).unwrap();
+    let patched: Vec<u8> = [MARKER.as_bytes(), orig.as_slice()].concat();
+    stage_patch_with_vuln(
+        &proj,
+        "pkg:npm/is-number@6.0.0",
+        "package/index.js",
+        &orig,
+        &patched,
+        "GHSA-link-npm-real",
+    );
+    // Rollback restores from the before-blob.
+    std::fs::write(proj.join(".socket/blobs").join(git_sha256(&orig)), &orig).unwrap();
+    let cwd = proj.to_str().unwrap();
+
+    let (code, stdout, stderr) = run_socket(&proj, &["apply", "--json", "--offline", "--cwd", cwd]);
+    assert_eq!(code, 0, "apply failed.\n{stdout}\n{stderr}");
+    let env = parse_envelope(&stdout);
+    assert_eq!(env["status"], "success", "{env}");
+    assert_eq!(env["summary"]["applied"], 1, "{env}");
+    assert_eq!(std::fs::read(&index).unwrap(), patched);
+    assert!(node_loads(&proj, "is-odd", "is-number").starts_with(MARKER));
+
+    let (code, stdout, stderr) = run_socket(&proj, &["rollback", "--json", "--cwd", cwd]);
+    assert_eq!(code, 0, "rollback failed.\n{stdout}\n{stderr}");
+    assert_eq!(std::fs::read(&index).unwrap(), orig);
+
+    // Rollback drops the record from the manifest; stage it again.
+    stage_patch_with_vuln(
+        &proj,
+        "pkg:npm/is-number@6.0.0",
+        "package/index.js",
+        &orig,
+        &patched,
+        "GHSA-link-npm-real",
+    );
+    let lock_before = std::fs::read(proj.join("package-lock.json")).unwrap();
+    let (code, stdout, stderr) =
+        run_socket(&proj, &["vendor", "--json", "--offline", "--cwd", cwd]);
+    assert_eq!(code, 0, "vendor failed.\n{stdout}\n{stderr}");
+    assert_eq!(parse_envelope(&stdout)["status"], "success", "{stdout}");
+    assert_ne!(
+        std::fs::read(proj.join("package-lock.json")).unwrap(),
+        lock_before,
+        "vendor must rewire the lock: {stdout}"
+    );
+    let (code, stdout, stderr) = run_socket(&proj, &["vendor", "--revert", "--json", "--cwd", cwd]);
+    assert_eq!(code, 0, "vendor --revert failed.\n{stdout}\n{stderr}");
+    assert_eq!(
+        std::fs::read(proj.join("package-lock.json")).unwrap(),
+        lock_before
+    );
+}

crates/socket-patch-cli/tests/e2e_vendor_pnpm_build.rs

@@ -1799,3 +1799,85 @@ fn run_legacy_capstone(pm: &str, lock_head: &str) {
     assert!(!proj.join(".socket/vendor").exists());
     eprintln!("REVERT OK ({pm})");
 }
+
+/// #362: pnpm's `virtualStoreDir` moves the virtual store, and a
+/// transitive dependency lives only there. Agent-mode `apply` must find
+/// it through `node_modules/.modules.yaml` and patch the copy Node
+/// loads, and `rollback` must restore it: both for a store next to
+/// `node_modules` and for one inside it under a custom hidden name.
+#[test]
+fn pnpm_agent_apply_patches_a_transitive_dep_in_a_relocated_virtual_store() {
+    if !has_corepack_pm(PNPM_PRIMARY) {
+        println!("SKIP: `corepack {PNPM_PRIMARY}` unavailable");
+        return;
+    }
+    for (setting, store_rel) in [
+        (".vstore", ".vstore"),
+        ("node_modules/.custom", "node_modules/.custom"),
+    ] {
+        let tmp = tempfile::tempdir().unwrap();
+        let proj = tmp.path().join("proj");
+        std::fs::create_dir_all(&proj).unwrap();
+        std::fs::write(
+            proj.join("package.json"),
+            r#"{"name":"vsd","version":"0.0.0","private":true,"dependencies":{"is-odd":"3.0.1"}}"#,
+        )
+        .unwrap();
+        std::fs::write(
+            proj.join("pnpm-workspace.yaml"),
+            format!("virtualStoreDir: {setting}\n"),
+        )
+        .unwrap();
+        let store = tmp.path().join("pnpm-store");
+        let install = corepack(
+            &proj,
+            PNPM_PRIMARY,
+            &["install", "--store-dir", store.to_str().unwrap()],
+        );
+        if !install.status.success() {
+            assert!(!pnpm_required(), "fixture install failed: {install:?}");
+            println!("SKIP: fixture `pnpm install` failed: {install:?}");
+            return;
+        }
+        // is-odd@3.0.1 depends on is-number@6.0.0: transitive, store-only.
+        let copy = proj
+            .join(store_rel)
+            .join("is-number@6.0.0/node_modules/is-number");
+        let index = copy.join("index.js");
+        let orig = std::fs::read(&index)
+            .unwrap_or_else(|e| panic!("{setting}: pnpm put is-number at {}: {e}", copy.display()));
+        let patched: Vec<u8> = [MARKER.as_bytes(), orig.as_slice()].concat();
+        stage_patch(
+            &proj,
+            "pkg:npm/is-number@6.0.0",
+            "package/index.js",
+            &orig,
+            &patched,
+        );
+        std::fs::write(proj.join(".socket/blobs").join(git_sha256(&orig)), &orig).unwrap();
+        let cwd = proj.to_str().unwrap();
+
+        let (code, stdout, stderr) =
+            run_socket(&proj, &["apply", "--json", "--offline", "--cwd", cwd]);
+        assert_eq!(code, 0, "{setting}: apply failed.\n{stdout}\n{stderr}");
+        let env = parse_envelope(&stdout);
+        assert_eq!(env["summary"]["applied"], 1, "{setting}: {env}");
+        assert_eq!(std::fs::read(&index).unwrap(), patched, "{setting}");
+        // Node loads that very copy.
+        let script = "const p=require('path');process.stdout.write(require('fs').readFileSync(\
+             require.resolve('is-number',{paths:[p.dirname(require.resolve('is-odd'))]}),'utf8'))";
+        let out = Command::new("node")
+            .args(["-e", script])
+            .current_dir(&proj)
+            .output()
+            .expect("node runs");
+        assert!(
+            String::from_utf8_lossy(&out.stdout).starts_with(MARKER),
+            "{setting}: {out:?}"
+        );
+
+        let (code, stdout, stderr) = run_socket(&proj, &["rollback", "--json", "--cwd", cwd]);
+        assert_eq!(code, 0, "{setting}: rollback failed.\n{stdout}\n{stderr}");
+        assert_eq!(std::fs::read(&index).unwrap(), orig, "{setting}");
+    }
+}

crates/socket-patch-cli/tests/scan_pnpm_relocated_store_cwd_e2e.rs

@@ -0,0 +1,152 @@
+//! `scan` with the default `--cwd .` must not walk a pnpm `virtualStoreDir`
+//! outside the project.
+//!
+//! The default cwd makes the importer the empty relative path, which a
+//! bare `strip_prefix` accepts as a prefix of every path: an absolute
+//! recorded store (pnpm's global virtual store, `<store-dir>/v10/links`,
+//! shared by every project on the machine) then passed the in-project
+//! check and was crawled, so `apply` would patch the other projects too
+//! (#361). A store inside the project is still walked from the same cwd,
+//! whether it is recorded relative or absolute.
+
+use std::path::{Path, PathBuf};
+use std::process::Command;
+
+use wiremock::matchers::{method, path};
+use wiremock::{Mock, MockServer, ResponseTemplate};
+
+const ORG: &str = "test-org";
+
+fn binary() -> PathBuf {
+    env!("CARGO_BIN_EXE_socket-patch").into()
+}
+
+fn write_pkg(dir: &Path, name: &str) {
+    std::fs::create_dir_all(dir).unwrap();
+    std::fs::write(
+        dir.join("package.json"),
+        format!(r#"{{ "name": "{name}", "version": "1.0.0" }}"#),
+    )
+    .unwrap();
+}
+
+/// A project under `tmp/proj` whose `.modules.yaml` records
+/// `virtual_store_dir`, plus a pnpm-shaped store entry for `name` at
+/// `store`.
+fn stage(tmp: &Path, virtual_store_dir: &str, store: &Path, name: &str) -> PathBuf {
+    let proj = tmp.join("proj");
+    std::fs::create_dir_all(proj.join("node_modules")).unwrap();
+    std::fs::write(
+        proj.join("package.json"),
+        r#"{ "name": "cwd-root", "version": "0.0.0" }"#,
+    )
+    .unwrap();
+    std::fs::write(
+        proj.join("node_modules/.modules.yaml"),
+        serde_json::to_string(&serde_json::json!({
+            "layoutVersion": 5,
+            "virtualStoreDir": virtual_store_dir,
+        }))
+        .unwrap(),
+    )
+    .unwrap();
+    write_pkg(
+        &store.join(format!("{name}@1.0.0/node_modules/{name}")),
+        name,
+    );
+    proj
+}
+
+/// `scan --json` from `cwd` with no `--cwd` flag, returning the batch
+/// request bodies the crawl produced.
+async fn scan_bodies(cwd: &Path) -> String {
+    let server = MockServer::start().await;
+    Mock::given(method("POST"))
+        .and(path(format!("/v0/orgs/{ORG}/patches/batch")))
+        .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({
+            "packages": [], "canAccessPaidPatches": false,
+        })))
+        .mount(&server)
+        .await;
+    let mut cmd = Command::new(binary());
+    cmd.arg("scan").current_dir(cwd);
+    for (key, _) in std::env::vars_os() {
+        if key.to_string_lossy().starts_with("SOCKET_")
+            && key.to_string_lossy() != "SOCKET_NO_CONFIG"
+        {
+            cmd.env_remove(&key);
+        }
+    }
+    cmd.env_remove("VIRTUAL_ENV");
+    cmd.env("CARGO_HOME", cwd.join(".cargo-home"));
+    cmd.env("SOCKET_TELEMETRY_DISABLED", "1");
+    let out = cmd
+        .args([
+            "--json",
+            "-e",
+            "npm",
+            "--api-url",
+            &server.uri(),
+            "--api-token",
+            "fake-token-for-test",
+            "--org",
+            ORG,
+        ])
+        .output()
+        .expect("run socket-patch");
+    assert!(
+        out.status.success(),
+        "stdout={}; stderr={}",
+        String::from_utf8_lossy(&out.stdout),
+        String::from_utf8_lossy(&out.stderr)
+    );
+    server
+        .received_requests()
+        .await
+        .unwrap_or_default()
+        .iter()
+        .filter(|r| r.url.path().ends_with("/patches/batch"))
+        .map(|r| String::from_utf8_lossy(&r.body).into_owned())
+        .collect::<Vec<_>>()
+        .join("\n")
+}
+
+#[tokio::test]
+async fn default_cwd_scan_skips_an_absolute_store_outside_the_project() {
+    let tmp = tempfile::tempdir().unwrap();
+    let global = tmp.path().join("pnpm-store/v10/links");
+    let proj = stage(
+        tmp.path(),
+        &global.display().to_string(),
+        &global,
+        "shared-dep",
+    );
+    let bodies = scan_bodies(&proj).await;
+    assert!(!bodies.contains("shared-dep"), "{bodies}");
+}
+
+#[tokio::test]
+async fn default_cwd_scan_walks_a_store_inside_the_project() {
+    let tmp = tempfile::tempdir().unwrap();
+    let store = tmp.path().join("proj/.vstore");
+    let proj = stage(tmp.path(), "../.vstore", &store, "inproj-dep");
+    let bodies = scan_bodies(&proj).await;
+    assert!(bodies.contains("pkg:npm/inproj-dep@1.0.0"), "{bodies}");
+}
+
+/// Old pnpm records `virtualStoreDir` as an absolute path. From the
+/// default cwd the importer is the empty path, which is no lexical prefix
+/// of an absolute store, but a store inside the project is still walked.
+#[tokio::test]
+async fn default_cwd_scan_walks_an_absolute_store_inside_the_project() {
+    let tmp = tempfile::tempdir().unwrap();
+    let store = tmp.path().join("proj/.vstore");
+    let proj = stage(
+        tmp.path(),
+        &store.display().to_string(),
+        &store,
+        "absolute-dep",
+    );
+    let bodies = scan_bodies(&proj).await;
+    assert!(bodies.contains("pkg:npm/absolute-dep@1.0.0"), "{bodies}");
+}