diff --git a/CHANGELOG.md b/CHANGELOG.md index 3572a298c..e20322059 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -102,6 +102,12 @@ limits, and required install commands. ### Fixed +- `vex` no longer attests an npm or Bun patch as `not_affected` while a + second entry for the same `name@version` in the same lockfile still + resolves from the registry (for example a workspace member added after + vendoring). That copy installs unpatched, so the patch is now reported + as contested. `vendor --check` reports the same lockfile entry as drift + (#588). - Global mode (`-g`) finds npm, yarn, pnpm, bun, RubyGems and Composer on Windows, where they install as `.cmd` / `.bat` shims, instead of reporting an empty scan. The yarn and npm-family global lookups no longer run from the diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index 69fb09df9..b9f6641a1 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -376,7 +376,7 @@ Recognition rules that hold for every ecosystem: * **Patch hosts.** A hosted reference counts only on `https://patch.socket.dev` or the `--patch-server-url` / `SOCKET_PATCH_SERVER_URL` origin, with no userinfo. The uuid is the URL's LAST canonical-uuid path segment, because grant tokens may themselves be uuid-shaped. The Go module prefix is fixed. `socket-patch-` registry / repository / source names count only through a pin. For a URL on any other host, see **Patch hosts** above. * **Pins, not definitions.** A registry, index or source *definition* alone (cargo `[registries]`, nuget ``, pom ``, uv index tables, `.npmrc`) never makes a reference, because it survives a reverted pin. Sections the package manager ignores are not read: npm's v2 `dependencies` mirror, a `.cargo/config.toml` shadowed by `.cargo/config`. A Socket pin inside a maven `` is diagnosed, never a reference. -* **Contested locks.** When one lock wires a package to a patch and another lock resolves the same `name@version` from a non-Socket source, the build's bytes depend on which package manager runs. The reference is then dropped with a `patched_ref_unattributable` diagnostic naming both files. This applies across npm / pnpm / yarn / bun and across uv / pylock / poetry / pdm / Pipfile.lock / requirements. PEP 723 script locks neither contest nor are contested. A **bundled** npm copy (`inBundle: true`, or v1 `bundled: true`) of the same `name@version` contests the reference too, in the same lock, in the other npm lock of a shrinkwrap/package-lock pair, or in any other lock. npm unpacks it from the parent package's tarball, so no rewire reaches it and it stays unpatched. Bun and vlt unpack bundled copies the same way (#469, #471). For Bun, that is a `bun.lock` entry whose meta is `{ "bundled": true }`, or a `bun.lockb` record that a dependency edge with the `bundled` behavior bit reaches, including one Bun shares with a regular install. Such an entry is never a reference, and it contests the reference the same way. For vlt, the lock records no node for a bundled copy, so the copy is found in the installed store: a real package directory inside a store package's own `node_modules`. Hosted and vendored scans skip these copies with `redirect_bun_bundled_instance_skipped` / `redirect_vlt_bundled_instance_skipped` / `vendor_bundled_instance_skipped`. When a bundled copy is the only instance, vendoring refuses with `vendor_lock_entry_not_rewritable`. +* **Contested locks.** When one lock wires a package to a patch and another lock resolves the same `name@version` from a non-Socket source, the build's bytes depend on which package manager runs. The reference is then dropped with a `patched_ref_unattributable` diagnostic naming both files. This applies across npm / pnpm / yarn / bun and across uv / pylock / poetry / pdm / Pipfile.lock / requirements. PEP 723 script locks neither contest nor are contested. A **bundled** npm copy (`inBundle: true`, or v1 `bundled: true`) of the same `name@version` contests the reference too, in the same lock, in the other npm lock of a shrinkwrap/package-lock pair, or in any other lock. npm unpacks it from the parent package's tarball, so no rewire reaches it and it stays unpatched. Bun and vlt unpack bundled copies the same way (#469, #471). For Bun, that is a `bun.lock` entry whose meta is `{ "bundled": true }`, or a `bun.lockb` record that a dependency edge with the `bundled` behavior bit reaches, including one Bun shares with a regular install. Such an entry is never a reference, and it contests the reference the same way. For vlt, the lock records no node for a bundled copy, so the copy is found in the installed store: a real package directory inside a store package's own `node_modules`. Hosted and vendored scans skip these copies with `redirect_bun_bundled_instance_skipped` / `redirect_vlt_bundled_instance_skipped` / `vendor_bundled_instance_skipped`. When a bundled copy is the only instance, vendoring refuses with `vendor_lock_entry_not_rewritable`. Another entry of the **same** npm or Bun lock that resolves the wired `name@version` from a non-Socket source (for example a workspace member added after the rewire, then `npm install` / `bun install`) contests the reference too (#588). The package manager installs both entries, and that copy stays unpatched. Re-running `scan` / `vendor` rewires every copy. * **Lockless pins.** With no lock to name a version, a `Cargo.toml` pin (every declaration on `socket-patch-`, that registry defined on the patch host for the same uuid) or an exclusive nuget exact-id mapping is never a reference on its own, so v5.0 does not attest it (nor does `list` show it, or `rollback` / `remove` restore it — restore those files from version control). Only a pre-v5 redirect-ledger record naming a version the pin admits keeps it live. The same holds for a gem wired only in the `Gemfile` (the pre-bundler-2.6 mixed state, lock not converged). **Record resolution.** A candidate's record must carry the patch uuid the lockfile actually **wires**. It is taken from the first source that has one: the manifest (matched qualifier-insensitively), the hosted records above (this run's, then a pre-v5 ledger's), then the vendor ledger's embedded records. If none has it and the run is online, `vex` fetches the patch view by uuid from the patch API — for a v5 hosted checkout this is the normal path. The fetch uses `get`'s API client: the public proxy when no token is configured, and a one-shot 401/403 fallback to the proxy (free patches only). At most 10 fetches run concurrently. Fetched records stay in memory: `vex` never writes the manifest. A candidate still has no record under `--offline`, after a transport error or a 404, or when the patch is refused (paid without an entitled token); it is then omitted as `record_unavailable`, and the run is not aborted. A record whose uuid or package disagrees with the wiring is omitted as `record_mismatch`. The informational `socket-patch.vendor.json` marker is never a record source. When the lockfile wires a package to patch U, a manifest or ledger record for that package under another uuid is superseded, and a human-mode `Note:` says so. @@ -1633,7 +1633,10 @@ See [the JVM design](../../docs/design/maven-vendoring.md) for supported shapes. `vendor --check` is an offline, read-only audit. Healthy entries emit `verified` with `vendor_check_ok`; drift emits `failed` with `vendor_check_failed`, a -`partialFailure` envelope and exit 1. Missing ledger entries fail with +`partialFailure` envelope and exit 1. For a package-lock entry, drift includes a +`package-lock.json` / `npm-shrinkwrap.json` entry for the vendored `name@version` +that `vendor` would rewire but that does not resolve to the vendored artifact +(#588); the reason names that entry. Missing ledger entries fail with `vendor_ledger_missing`. Offline upstream metadata is reported as the run warning `vendor_jvm_upstream_unverified`. The check never starts an API client or writes lock/recovery files. `--check` conflicts with `--revert`. diff --git a/crates/socket-patch-cli/src/commands/vendor.rs b/crates/socket-patch-cli/src/commands/vendor.rs index 59be95b85..a59d29efe 100644 --- a/crates/socket-patch-cli/src/commands/vendor.rs +++ b/crates/socket-patch-cli/src/commands/vendor.rs @@ -955,6 +955,11 @@ async fn run_check(args: &VendorArgs) -> i32 { if failure.is_none() && vendor::jvm::apply::is_jvm_entry(entry) { failure = vendor::jvm::apply::check_entry(root, entry, local_repo.as_deref()).err(); } + if failure.is_none() && entry.ecosystem == "npm" { + failure = vendor::npm_flavor::check_npm_wiring(entry, root) + .await + .err(); + } if vendor::jvm::apply::upstream_unverified(entry) { env.warnings.push(RunWarning {code: "vendor_jvm_upstream_unverified".into(), detail: format!("{key}: upstream metadata was accepted offline; run vendor online to verify registry checksums")}); } diff --git a/crates/socket-patch-cli/tests/e2e_vex_vendor.rs b/crates/socket-patch-cli/tests/e2e_vex_vendor.rs index 253a6fb9c..0e5f46792 100644 --- a/crates/socket-patch-cli/tests/e2e_vex_vendor.rs +++ b/crates/socket-patch-cli/tests/e2e_vex_vendor.rs @@ -1472,6 +1472,142 @@ fn vendored_npm_patch_with_an_unpatched_bundled_copy_is_not_attested() { } } +/// REGRESSION (#588): the lock rewires the hoisted `lodash@4.17.21` to the +/// vendored tarball, but a workspace member added after vendoring (then +/// `npm install`) put a SECOND `lodash@4.17.21` entry in the same lock +/// that still resolves from the registry. `npm ci` installs that copy +/// unpatched, so `vex` must not attest the purl and `vendor --check` must +/// report the drift (re-running the install cannot heal it). The same lock +/// without the second entry is the control: it attests and checks clean. +#[test] +fn vendored_npm_patch_with_an_unwired_registry_copy_in_the_same_lock() { + let purl = "pkg:npm/lodash@4.17.21"; + let uuid = "0a0a0a0a-2222-4222-8222-0a0a0a0a0a0a"; + let patched = b"patched npm bytes\n"; + let after_hash = compute_git_sha256_from_bytes(patched); + for (label, second_copy) in [("control", false), ("second registry copy", true)] { + let tmp = tempfile::tempdir().expect("create tempdir"); + let cwd = tmp.path(); + let rel = format!(".socket/vendor/npm/{uuid}/lodash-4.17.21.tgz"); + let sha256 = sha256_hex(&write_member_tgz( + &cwd.join(&rel), + "package/index.js", + patched, + )); + let record = make_record( + uuid, + "package/index.js", + &after_hash, + "GHSA-dupe-aaaa", + &["CVE-2026-588"], + ); + let wiring = write_matrix_wiring(cwd, "npm", uuid, &rel); + if second_copy { + let lock_path = cwd.join("package-lock.json"); + let mut lock: Value = + serde_json::from_str(&std::fs::read_to_string(&lock_path).unwrap()).unwrap(); + let packages = lock["packages"].as_object_mut().unwrap(); + packages.insert( + "packages/b".to_string(), + serde_json::json!({ "name": "b", "version": "1.0.0" }), + ); + packages.insert( + "node_modules/b".to_string(), + serde_json::json!({ "resolved": "packages/b", "link": true }), + ); + packages.insert( + "packages/b/node_modules/lodash".to_string(), + serde_json::json!({ + "version": "4.17.21", + "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz", + "integrity": "sha512-T1JJR0lOQUw=" + }), + ); + std::fs::write(&lock_path, lock.to_string()).unwrap(); + } + let mut state = VendorState::new(); + state.entries.insert( + purl.to_string(), + detached_matrix_entry("npm", purl, uuid, &rel, sha256, record, wiring), + ); + let dir = cwd.join(".socket/vendor"); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write( + dir.join("state.json"), + serde_json::to_string_pretty(&state).unwrap(), + ) + .unwrap(); + + let vex_path = cwd.join("out.vex.json"); + let out = cli() + .args([ + "vex", + "--cwd", + cwd.to_str().unwrap(), + "--json", + "--output", + vex_path.to_str().unwrap(), + "--product", + "pkg:npm/app@1.0.0", + ]) + .output() + .expect("invoke vex"); + let env: Value = serde_json::from_slice(&out.stdout).unwrap_or_else(|e| { + panic!( + "{label}: vex envelope JSON on stdout ({e}): {}", + String::from_utf8_lossy(&out.stdout) + ) + }); + let check = cli() + .args([ + "vendor", + "--check", + "--cwd", + cwd.to_str().unwrap(), + "--json", + ]) + .output() + .expect("invoke vendor --check"); + let check_env: Value = serde_json::from_slice(&check.stdout).unwrap_or_else(|e| { + panic!( + "{label}: vendor --check envelope JSON on stdout ({e}): {}", + String::from_utf8_lossy(&check.stdout) + ) + }); + if !second_copy { + assert!(out.status.success(), "{label}: {env}"); + let doc: Value = + serde_json::from_str(&std::fs::read_to_string(&vex_path).unwrap()).unwrap(); + assert_eq!( + doc["statements"].as_array().unwrap().len(), + 1, + "{label}: {doc}" + ); + assert!(check.status.success(), "{label}: {check_env}"); + continue; + } + assert_eq!(out.status.code(), Some(1), "{label}: {env}"); + assert!( + !vex_path.exists(), + "{label}: no VEX document may attest the purl: {env}" + ); + assert!( + env.to_string().contains("packages/b/node_modules/lodash"), + "{label}: the envelope names the unwired copy: {env}" + ); + assert_eq!(check.status.code(), Some(1), "{label}: {check_env}"); + let event = &check_env["events"][0]; + assert_eq!(event["errorCode"], "vendor_check_failed", "{check_env}"); + assert!( + event["reason"] + .as_str() + .is_some_and(|r| r.contains("packages/b/node_modules/lodash") + && r.contains("re-run `socket-patch vendor`")), + "{label}: the check names the unwired copy: {check_env}" + ); + } +} + // ────────────────────────────────────────────────────────────────────── // 8. an applied, byte-verified agent-mode patch attests whether or not its // ecosystem has an install hook (there is no setup-state filter). diff --git a/crates/socket-patch-core/src/vendor/lock_inventory/mod.rs b/crates/socket-patch-core/src/vendor/lock_inventory/mod.rs index 6e93465e3..8adcdf67f 100644 --- a/crates/socket-patch-core/src/vendor/lock_inventory/mod.rs +++ b/crates/socket-patch-core/src/vendor/lock_inventory/mod.rs @@ -70,7 +70,7 @@ pub(crate) mod vlt; pub(crate) mod wired; pub(crate) mod yarn; -pub(crate) use self::npm::{npm_lock_bundled_nodes, npm_lock_nodes, NpmLockNode}; +pub(crate) use self::npm::{npm_lock_bundled_nodes, npm_lock_located_nodes, NpmLockNode}; #[cfg(test)] pub(crate) use self::npm_family::inventory_npm_lock; pub(crate) use self::pypi::pipfile_lock_entries; diff --git a/crates/socket-patch-core/src/vendor/lock_inventory/npm.rs b/crates/socket-patch-core/src/vendor/lock_inventory/npm.rs index 114bf4182..66a401a1c 100644 --- a/crates/socket-patch-core/src/vendor/lock_inventory/npm.rs +++ b/crates/socket-patch-core/src/vendor/lock_inventory/npm.rs @@ -44,12 +44,20 @@ const MAX_LEGACY_NPM_DEPTH: usize = 64; /// through nested `dependencies`, `bundled: true` entries skipped (their /// nested trees are still walked). pub(crate) fn npm_lock_nodes(doc: &Value) -> Vec> { - walk_npm_lock(doc, Bundled::Skip) + walk_npm_lock(doc, Bundled::Skip, false) .into_iter() .map(|(_, node)| node) .collect() } +/// [`npm_lock_nodes`], each with where the lock puts it (the `packages` +/// key, or the `>`-joined v1 dependency chain — the +/// [`npm_lock_bundled_nodes`] spelling), for diagnostics that must name +/// the entry. +pub(crate) fn npm_lock_located_nodes(doc: &Value) -> Vec<(String, NpmLockNode<'_>)> { + walk_npm_lock(doc, Bundled::Skip, true) +} + /// The BUNDLED entries of a parsed npm lock, each with where the lock puts /// it: `inBundle: true` in `packages` (lockfileVersion 2/3; the location is /// the `packages` key), `bundled: true` in the v1 `dependencies` tree (the @@ -61,7 +69,7 @@ pub(crate) fn npm_lock_nodes(doc: &Value) -> Vec> { /// warn `*_bundled_instance_skipped`), and lockfile discovery /// (`vex::discover::npm`) weighs it against the rewired entries. pub(crate) fn npm_lock_bundled_nodes(doc: &Value) -> Vec<(String, NpmLockNode<'_>)> { - walk_npm_lock(doc, Bundled::Only) + walk_npm_lock(doc, Bundled::Only, true) } /// Which side of the bundled split [`walk_npm_lock`] returns. @@ -71,10 +79,11 @@ enum Bundled { Only, } -/// [`npm_lock_nodes`] / [`npm_lock_bundled_nodes`]: one walk, split on the -/// bundled flag. Locations are built only for [`Bundled::Only`] (empty -/// otherwise), so the common walk allocates nothing extra. -fn walk_npm_lock(doc: &Value, bundled: Bundled) -> Vec<(String, NpmLockNode<'_>)> { +/// [`npm_lock_nodes`] / [`npm_lock_located_nodes`] / +/// [`npm_lock_bundled_nodes`]: one walk, split on the bundled flag. +/// Locations are built only when `locate` is set (empty otherwise), so the +/// common walk allocates nothing extra. +fn walk_npm_lock(doc: &Value, bundled: Bundled, locate: bool) -> Vec<(String, NpmLockNode<'_>)> { let mut out = Vec::new(); if let Some(packages) = doc.get("packages").and_then(Value::as_object) { for (key, node) in packages { @@ -85,14 +94,11 @@ fn walk_npm_lock(doc: &Value, bundled: Bundled) -> Vec<(String, NpmLockNode<'_>) continue; } let name = node.get("name").and_then(Value::as_str).unwrap_or(key_name); - let location = match bundled { - Bundled::Only => key.clone(), - Bundled::Skip => String::new(), - }; + let location = if locate { key.clone() } else { String::new() }; out.push((location, NpmLockNode::of(name, node))); } } else if let Some(deps) = doc.get("dependencies").and_then(Value::as_object) { - walk_npm_legacy_dependencies(deps, 0, bundled, "", &mut out); + walk_npm_legacy_dependencies(deps, 0, bundled, locate, "", &mut out); } out } @@ -125,6 +131,7 @@ fn walk_npm_legacy_dependencies<'a>( deps: &'a serde_json::Map, depth: usize, bundled: Bundled, + locate: bool, parent: &str, out: &mut Vec<(String, NpmLockNode<'a>)>, ) { @@ -132,16 +139,16 @@ fn walk_npm_legacy_dependencies<'a>( return; } for (name, node) in deps { - let location = match bundled { - Bundled::Only if parent.is_empty() => name.clone(), - Bundled::Only => format!("{parent} > {name}"), - Bundled::Skip => String::new(), + let location = match locate { + true if parent.is_empty() => name.clone(), + true => format!("{parent} > {name}"), + false => String::new(), }; if npm_flag(node, "bundled") == (bundled == Bundled::Only) { out.push((location.clone(), NpmLockNode::of(name, node))); } if let Some(nested) = node.get("dependencies").and_then(Value::as_object) { - walk_npm_legacy_dependencies(nested, depth + 1, bundled, &location, out); + walk_npm_legacy_dependencies(nested, depth + 1, bundled, locate, &location, out); } } } diff --git a/crates/socket-patch-core/src/vendor/npm_flavor.rs b/crates/socket-patch-core/src/vendor/npm_flavor.rs index 667b6ff51..0c1af62e4 100644 --- a/crates/socket-patch-core/src/vendor/npm_flavor.rs +++ b/crates/socket-patch-core/src/vendor/npm_flavor.rs @@ -674,6 +674,18 @@ pub async fn revert_npm_any( revert_npm_any_opts(entry, project_root, RevertOpts::new(dry_run)).await } +/// `vendor --check`'s wiring audit for an npm-family entry: `Err` (the +/// human reason) when the lock installs a copy of the entry's +/// `name@version` the vendored artifact does not reach (#588). Only the +/// package-lock flavor audits its lock today; the other flavors' wiring is +/// left to the artifact check and `vex`. +pub async fn check_npm_wiring(entry: &VendorEntry, project_root: &Path) -> Result<(), String> { + match NpmLockFlavor::from_recorded(entry.flavor.as_deref()) { + Some(NpmLockFlavor::PackageLock) => npm_lock::check_wiring(entry, project_root).await, + _ => Ok(()), + } +} + /// [`revert_npm_any`] with full [`RevertOpts`], threaded through to the /// flavor backend that wired the entry. pub async fn revert_npm_any_opts( diff --git a/crates/socket-patch-core/src/vendor/npm_lock.rs b/crates/socket-patch-core/src/vendor/npm_lock.rs index b1911ba90..726805356 100644 --- a/crates/socket-patch-core/src/vendor/npm_lock.rs +++ b/crates/socket-patch-core/src/vendor/npm_lock.rs @@ -849,6 +849,56 @@ enum LockScan { }, } +/// `vendor --check`'s wiring audit for a package-lock entry (#588): every +/// rewritable `packages` instance of the entry's `name@version` (the set +/// [`vendor_npm`] rewires) in each present npm lock must resolve to the +/// vendored artifact. Another entry for the same version — e.g. a +/// workspace member added after vendoring, then `npm install` — resolves +/// from the registry and installs unpatched, and a fresh install cannot +/// heal it: the lock itself names the unpatched source. `Err` is the +/// human reason. +pub(super) async fn check_wiring(entry: &VendorEntry, project_root: &Path) -> Result<(), String> { + // Unparseable coordinates never vendored; the artifact check owns that. + let Some((name, version)) = super::npm_common::parse_npm_purl(&entry.base_purl) else { + return Ok(()); + }; + let wired = format!("file:{}", entry.artifact.path); + let overrides = NpmOverrides::read(project_root).await; + let mut unwired = Vec::new(); + for lock_name in NPM_LOCKS { + let bytes = match read_regular_to_bytes(&project_root.join(lock_name)).await { + Ok(bytes) => bytes, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => continue, + Err(e) => return Err(format!("{lock_name} cannot be read: {e}")), + }; + let lock = parse_json_manifest(&bytes) + .map_err(|e| format!("{lock_name} is not parseable JSON: {e}"))?; + // The skip advisories are vendor's to raise; a bundled / link / + // non-registry copy is not one vendor can rewire. + let mut skipped = Vec::new(); + let LockScan::Matches(matches) = + scan_lock_matches(&lock, &overrides, &name, &version, &mut skipped) + else { + continue; + }; + unwired.extend( + matches + .iter() + .filter(|m| m.original.get("resolved").and_then(Value::as_str) != Some(&wired)) + .map(|m| format!("{lock_name} `{}`", m.key)), + ); + } + if unwired.is_empty() { + return Ok(()); + } + Err(format!( + "vendored wiring drifted: {} still resolve {name}@{version} outside the vendored \ + artifact, so that copy installs unpatched; re-run `socket-patch vendor` to rewire \ + every copy", + unwired.join(", ") + )) +} + /// Scan `packages` for instances of `name@version`, pushing skip warnings /// for the link / inBundle instances that cannot be rewritten. fn scan_lock_matches( diff --git a/crates/socket-patch-core/src/vex/discover/bun.rs b/crates/socket-patch-core/src/vex/discover/bun.rs index 954d3eb01..b219ebaae 100644 --- a/crates/socket-patch-core/src/vex/discover/bun.rs +++ b/crates/socket-patch-core/src/vex/discover/bun.rs @@ -124,6 +124,7 @@ async fn extract_text(ctx: &DiscoverCtx<'_>, out: &mut Discovery) { } }; let mut bundled = Bundled::default(); + let mut unwired = Unwired::default(); for entry in &entries { let elems = &entry.elems; let Some(spec) = elems.first().and_then(|e| decode_json_string(e)) else { @@ -153,10 +154,11 @@ async fn extract_text(ctx: &DiscoverCtx<'_>, out: &mut Discovery) { if is_bundled_entry(entry) { bundled.record(ctx, BUN_LOCK, classified, out); } else { - classify(ctx, BUN_LOCK, classified, out); + unwired.record(classify(ctx, BUN_LOCK, classified, out), &entry.key); } } bundled.contest(BUN_LOCK, out); + unwired.contest(BUN_LOCK, out); } // ── bundled copies ─────────────────────────────────────────────────────── @@ -259,6 +261,7 @@ async fn extract_binary(ctx: &DiscoverCtx<'_>, out: &mut Discovery) { } }; let mut bundled = Bundled::default(); + let mut unwired = Unwired::default(); for p in &packages { let integrity = p .integrity @@ -283,10 +286,11 @@ async fn extract_binary(ctx: &DiscoverCtx<'_>, out: &mut Discovery) { if p.bundled { bundled.record(ctx, BUN_LOCKB, classified, out); } else { - classify(ctx, BUN_LOCKB, classified, out); + unwired.record(classify(ctx, BUN_LOCKB, classified, out), &label); } } bundled.contest(BUN_LOCKB, out); + unwired.contest(BUN_LOCKB, out); } // ── shared classification ──────────────────────────────────────────────── @@ -309,8 +313,15 @@ struct Entry<'a> { } /// Push `entry`'s ref when it is Socket-wired (see the module docs); stay -/// silent for anything else. -fn classify(ctx: &DiscoverCtx<'_>, file: &str, entry: Entry<'_>, out: &mut Discovery) { +/// silent for anything else. Returns the purl of a registry entry (an exact +/// version resolved from a non-Socket source), which contests a ref for the +/// same version in this lock ([`Unwired::contest`]) and in any other. +fn classify( + ctx: &DiscoverCtx<'_>, + file: &str, + entry: Entry<'_>, + out: &mut Discovery, +) -> Option { let Entry { label, name, @@ -334,7 +345,7 @@ fn classify(ctx: &DiscoverCtx<'_>, file: &str, entry: Entry<'_>, out: &mut Disco .socket/vendor/npm// path; it is ignored" ), ); - return; + return None; } if vendored.is_none() && hosted_uuid.is_none() { // Registry / git / workspace / user tarball dependency: not ours. A @@ -345,10 +356,9 @@ fn classify(ctx: &DiscoverCtx<'_>, file: &str, entry: Entry<'_>, out: &mut Disco .starts_with(|c: char| c.is_ascii_digit()) .then_some(target) }); - if let Some(version) = version { - out.resolved_elsewhere(file, npm_purl(name, version)); - } - return; + let purl = version.and_then(|version| npm_purl(name, version)); + out.resolved_elsewhere(file, purl.clone()); + return purl; } let invalid = |out: &mut Discovery, why: String| { out.diag(DIAG_REF_INVALID, file, format!("{file}: {label}: {why}")); @@ -360,12 +370,12 @@ fn classify(ctx: &DiscoverCtx<'_>, file: &str, entry: Entry<'_>, out: &mut Disco "{name}@{target} is not in bun's tarball tuple shape [spec, {{meta}}, integrity]" ), ); - return; + return None; } let version = match &vendored { Some(vref) if vref.eco != "npm" => { invalid(out, format!("{target:?} is not a vendored npm tarball")); - return; + return None; } Some(vref) => tgz_leaf_version(name, &vref.leaf) .filter(|version| semver::Version::parse(version).is_ok()), @@ -376,7 +386,7 @@ fn classify(ctx: &DiscoverCtx<'_>, file: &str, entry: Entry<'_>, out: &mut Disco out, format!("{target:?} is not an artifact of {name:?} (its leaf must be the package's own -.tgz)"), ); - return; + return None; }; if recorded_version.is_some_and(|recorded| recorded != version) { invalid( @@ -386,14 +396,14 @@ fn classify(ctx: &DiscoverCtx<'_>, file: &str, entry: Entry<'_>, out: &mut Disco recorded_version.unwrap_or_default() ), ); - return; + return None; } let Some(purl) = npm_purl(name, version) else { invalid( out, format!("Socket-wired entry {name:?}@{version:?} has unsafe coordinates"), ); - return; + return None; }; // Hosted: both bun rewriters always write the sha512 (see module docs). if let Some(vref) = vendored { @@ -408,6 +418,54 @@ fn classify(ctx: &DiscoverCtx<'_>, file: &str, entry: Entry<'_>, out: &mut Disco true, )); } + None +} + +/// The registry copies one lock records, keyed by purl (#588): bun installs +/// every entry, so a second entry resolving a wired `name@version` from the +/// registry (e.g. a workspace member added after the rewire, then `bun +/// install`) installs unpatched beside the rewired one. +#[derive(Default)] +struct Unwired { + /// purl → the first such entry's label. + copies: std::collections::BTreeMap, +} + +impl Unwired { + fn record(&mut self, purl: Option, label: &str) { + if let Some(purl) = purl { + self.copies.entry(purl).or_insert_with(|| label.to_string()); + } + } + + /// Withdraw every ref of `file` whose `name@version` another entry of + /// the same lock resolves from the registry. + fn contest(&self, file: &str, out: &mut Discovery) { + if self.copies.is_empty() { + return; + } + let refs = std::mem::take(&mut out.refs); + for r in refs { + let unwired_at = (r.source_file == std::path::Path::new(file)) + .then(|| self.copies.get(&r.purl)) + .flatten(); + let Some(label) = unwired_at else { + out.refs.push(r); + continue; + }; + out.diag( + DIAG_REF_UNATTRIBUTABLE, + file, + format!( + "{file}: {} is wired to a Socket patch but another entry of the same lock, \ + {label:?}, still resolves that version elsewhere; bun installs both, so \ + that copy stays unpatched and the patch is not attested — re-run \ + `socket-patch vendor` / `scan --mode hosted` to rewire every copy", + r.purl, + ), + ); + } + } } #[cfg(test)] @@ -653,6 +711,65 @@ mod tests { assert_eq!(minimist.locked_integrity, None); } + /// REGRESSION (#588, the Bun twin): `bun.lock` rewires one copy of + /// `left-pad@1.3.0` (`a/left-pad`), but a second entry for the SAME + /// version (`b/left-pad`, a workspace member added after the rewire) + /// still resolves from the registry. bun installs that copy unpatched, + /// so the ref is not attested in either mode; a registry copy of a + /// DIFFERENT version contests nothing. + #[tokio::test] + async fn issue_588_registry_copy_in_the_same_lock_contests_the_ref() { + let hosted = hosted_url("npm", "left-pad", "1.3.0", UUID_A, "left-pad-1.3.0.tgz"); + let vendored = format!("left-pad@.socket/vendor/npm/{UUID_A}/left-pad-1.3.0.tgz"); + let registry = |key: &str, version: &str| { + format!("\"{key}\": [\"left-pad@{version}\", \"\", {{}}, \"{SRI}\"]") + }; + let contests = |out: &Discovery| { + out.diagnostics + .iter() + .filter(|d| { + d.code == DIAG_REF_UNATTRIBUTABLE + && d.detail.contains("another entry") + && d.detail.contains("b/left-pad") + }) + .count() + }; + for (label, spec) in [ + ("hosted", format!("left-pad@{hosted}")), + ("vendored", vendored), + ] { + let p = Project::new(); + p.write( + "bun.lock", + text_lock( + 1, + &[ + tuple("a/left-pad", &spec, Some(SRI)), + registry("b/left-pad", "1.3.0"), + ], + ), + ); + let out = run(&p).await; + assert!(out.refs.is_empty(), "{label}: {:#?}", out.refs); + assert_eq!(contests(&out), 1, "{label}: {:#?}", out.diagnostics); + + let p = Project::new(); + p.write( + "bun.lock", + text_lock( + 1, + &[ + tuple("a/left-pad", &spec, Some(SRI)), + registry("b/left-pad", "1.2.0"), + ], + ), + ); + let out = run(&p).await; + assert_eq!(out.refs.len(), 1, "{label} control: {:#?}", out.refs); + assert_eq!(contests(&out), 0, "{label} control: {:#?}", out.diagnostics); + } + } + /// The `DIAG_REF_UNATTRIBUTABLE` diagnostics that name a bundled copy. fn bundled_contests(out: &Discovery) -> usize { out.diagnostics diff --git a/crates/socket-patch-core/src/vex/discover/npm.rs b/crates/socket-patch-core/src/vex/discover/npm.rs index 920932076..a7599bdbc 100644 --- a/crates/socket-patch-core/src/vex/discover/npm.rs +++ b/crates/socket-patch-core/src/vex/discover/npm.rs @@ -56,7 +56,7 @@ use crate::formats::pnpm::{ use crate::utils::digest::is_sri_pin; use crate::vendor::lock_inventory::pnpm::rush_lock_rels; use crate::vendor::lock_inventory::{ - npm_lock_bundled_nodes, npm_lock_nodes, LockIntegrity, NpmLockNode, + npm_lock_bundled_nodes, npm_lock_located_nodes, LockIntegrity, NpmLockNode, }; use crate::vendor::npm_origin::{npm_non_registry_entries, NpmOverrides}; @@ -73,11 +73,11 @@ pub(crate) async fn extract(ctx: &DiscoverCtx<'_>, out: &mut Discovery) { /// What one parsed npm lock wires, plus the packages it resolves ELSEWHERE /// (an entry whose `resolved` is not a Socket reference) and the packages it -/// installs BUNDLED (purl → the first bundled entry's lock location). +/// installs BUNDLED (each purl → the first such entry's lock location). struct NpmLockRefs { file: &'static str, refs: Vec, - unwired: BTreeSet, + unwired: BTreeMap, bundled: BTreeMap, } @@ -95,7 +95,11 @@ struct NpmLockRefs { /// /// A bundled copy of the ref's `name@version` in either lock contests it /// too, the same lock included (#325): the rewired entry and the bundled -/// copy install side by side, and the bundled one stays unpatched. +/// copy install side by side, and the bundled one stays unpatched. So does +/// another entry of the ref's OWN lock that resolves the same +/// `name@version` elsewhere (#588 — e.g. a workspace member added after +/// the rewire, then `npm install`): npm installs every entry, and that one +/// fetches the unpatched registry bytes. fn push_uncontested(locks: Vec, out: &mut Discovery) { let wired: Vec> = locks .iter() @@ -124,8 +128,23 @@ fn push_uncontested(locks: Vec, out: &mut Discovery) { ); continue; } + if let Some(location) = lock.unwired.get(&r.purl) { + out.diag( + DIAG_REF_UNATTRIBUTABLE, + lock.file, + format!( + "{}: {} is wired to Socket patch {} but another entry of the same \ + lock, {location:?}, still resolves that version elsewhere; npm \ + installs both, so that copy stays unpatched and the patch is not \ + attested — re-run `socket-patch vendor` / `scan --mode hosted` to \ + rewire every copy", + lock.file, r.purl, r.uuid, + ), + ); + continue; + } let contested_by = locks.iter().enumerate().find(|(j, other)| { - *j != i && other.unwired.contains(&r.purl) && !wired[*j].contains(&r.purl) + *j != i && other.unwired.contains_key(&r.purl) && !wired[*j].contains(&r.purl) }); if let Some((_, other)) = contested_by { out.diag( @@ -159,7 +178,7 @@ async fn extract_package_lock( let mut read = NpmLockRefs { file, refs: Vec::new(), - unwired: BTreeSet::new(), + unwired: BTreeMap::new(), bundled: BTreeMap::new(), }; let doc: Value = match parse_json(file, &bytes) { @@ -175,8 +194,8 @@ async fn extract_package_lock( // registry must not become a ref (with no install it would attest from // the lockfile basis) — the shared walk reads the mirror only for a v1 // lock. - for node in npm_lock_nodes(&doc) { - entry_ref(ctx, file, &node, &mut read, out); + for (location, node) in npm_lock_located_nodes(&doc) { + entry_ref(ctx, file, &location, &node, &mut read, out); } // Bundled entries are never refs (a Socket url written there wires // nothing), but each one IS an install of that `name@version` from a @@ -233,7 +252,9 @@ fn drop_non_registry_installs( continue; }; out.resolved_elsewhere(file, Some(purl.clone())); - read.unwired.insert(purl.clone()); + read.unwired + .entry(purl.clone()) + .or_insert_with(|| key.clone()); unpatched.push((purl, key, reason)); } read.refs.retain(|r| { @@ -259,6 +280,7 @@ fn drop_non_registry_installs( fn entry_ref( ctx: &DiscoverCtx<'_>, file: &str, + location: &str, node: &NpmLockNode<'_>, read: &mut NpmLockRefs, out: &mut Discovery, @@ -292,7 +314,9 @@ fn entry_ref( // (the npm pair here, any other lock by the orchestrator). if let Some(purl) = node.version.and_then(|v| npm_purl(name, v)) { out.resolved_elsewhere(file, Some(purl.clone())); - read.unwired.insert(purl); + read.unwired + .entry(purl) + .or_insert_with(|| location.to_string()); } return; }; @@ -1098,6 +1122,83 @@ mod tests { assert_eq!(bundled_contests(&out).len(), 2, "{:#?}", out.diagnostics); } + /// The `DIAG_REF_UNATTRIBUTABLE` diagnostics that name an unwired copy + /// in the ref's own lock. + fn same_lock_contests(out: &Discovery) -> Vec<&Diag> { + out.diagnostics + .iter() + .filter(|d| d.code == DIAG_REF_UNATTRIBUTABLE && d.detail.contains("another entry")) + .collect() + } + + /// REGRESSION (#588): the lock rewires one copy of `name@version` + /// (`packages/a/node_modules/is-number`), but a second entry for the + /// SAME `name@version` in the SAME lock (a workspace member added after + /// vendoring, then `npm install`) still resolves from the registry. + /// `npm ci` installs that copy unpatched, so the ref is not attested, in + /// either mode, and the uuid stays recognized (the ledger record is dead + /// too). A registry copy of a DIFFERENT version contests nothing. + #[tokio::test] + async fn issue_588_unwired_registry_copy_in_the_same_lock_contests_the_ref() { + let hosted = hosted_url("npm", "is-number", "6.0.0", UUID_A, "is-number-6.0.0.tgz"); + let vendored = format!("file:.socket/vendor/npm/{UUID_B}/is-number-6.0.0.tgz"); + let registry = + |v: &str| format!("https://registry.npmjs.org/is-number/-/is-number-{v}.tgz"); + for (label, resolved, uuid, mode) in [ + ("hosted", hosted.clone(), UUID_A, WiringMode::Hosted), + ("vendored", vendored.clone(), UUID_B, WiringMode::Vendored), + ] { + let p = Project::new(); + p.write( + "package-lock.json", + lock_with_packages(serde_json::json!({ + "packages/a": { "name": "a", "version": "1.0.0" }, + "packages/b": { "name": "b", "version": "1.0.0" }, + "node_modules/a": { "resolved": "packages/a", "link": true }, + "node_modules/b": { "resolved": "packages/b", "link": true }, + "node_modules/is-number": { "version": "7.0.0", "resolved": registry("7.0.0"), "integrity": "sha512-SEVEN" }, + "packages/a/node_modules/is-number": { "version": "6.0.0", "resolved": resolved, "integrity": SRI }, + "packages/b/node_modules/is-number": { "version": "6.0.0", "resolved": registry("6.0.0"), "integrity": "sha512-ORIG" }, + })), + ); + let out = run(&p).await; + assert!(out.refs.is_empty(), "{label}: {:#?}", out.refs); + let contested = same_lock_contests(&out); + assert_eq!(contested.len(), 1, "{label}: {:#?}", out.diagnostics); + assert!( + contested[0].detail.contains("pkg:npm/is-number@6.0.0") + && contested[0] + .detail + .contains("packages/b/node_modules/is-number") + && contested[0].detail.contains("re-run"), + "{label}: {:#?}", + contested[0] + ); + assert!(out.recognizes(uuid, mode), "{label}: {:#?}", out.recognized); + } + + // Control: the only other copy is a different version (7.0.0 above), + // so the wired 6.0.0 entry is still attested. + let p = Project::new(); + p.write( + "package-lock.json", + lock_with_packages(serde_json::json!({ + "node_modules/is-number": { "version": "7.0.0", "resolved": registry("7.0.0"), "integrity": "sha512-SEVEN" }, + "packages/a/node_modules/is-number": { "version": "6.0.0", "resolved": hosted, "integrity": SRI }, + })), + ); + let out = run(&p).await; + assert_refs( + &out, + &[("pkg:npm/is-number@6.0.0", UUID_A, WiringMode::Hosted)], + ); + assert!( + same_lock_contests(&out).is_empty(), + "{:#?}", + out.diagnostics + ); + } + /// #490: a git edge the project's `overrides` send to the registry is /// a registry install, so its Socket wiring is attested. #[tokio::test]