From 75ecb0723f77f08d28aa2f76bd10e12ba90061d0 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 30 Sep 2026 17:24:29 +0000 Subject: [PATCH 1/5] Start fix for #326 Assisted-by: Claude Code:claude-opus-5-5 From ad605fe963fe2e52f117997ff1b046e551b3d99e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 30 Sep 2026 17:37:54 +0000 Subject: [PATCH 2/5] Skip npm lock entries installed from git or URLs npm installs a git, remote-tarball or file: dependency from the dependent's spec and ignores the lock entry's resolved. Hosted and vendored mode rewired those entries anyway, so the scan reported the package patched and vex attested it while npm ci installed the original bytes. Both rewriters now skip such entries with a loud stays-UNPATCHED warning (vendored refuses when no registry copy is left), and vex no longer attests a name@version while a non-registry copy of it is in the lock. Fixes #326 Assisted-by: Claude Code:claude-opus-5-5 --- .../tests/e2e_vendor_npm_build.rs | 96 +++++ .../src/patch/redirect/mod.rs | 137 +++++++ crates/socket-patch-core/src/vendor/mod.rs | 1 + .../socket-patch-core/src/vendor/npm_lock.rs | 110 +++++- .../src/vendor/npm_origin.rs | 334 ++++++++++++++++++ .../socket-patch-core/src/vex/discover/npm.rs | 135 +++++++ 6 files changed, 810 insertions(+), 3 deletions(-) create mode 100644 crates/socket-patch-core/src/vendor/npm_origin.rs diff --git a/crates/socket-patch-cli/tests/e2e_vendor_npm_build.rs b/crates/socket-patch-cli/tests/e2e_vendor_npm_build.rs index e9f6a0c60..a4f7cf841 100644 --- a/crates/socket-patch-cli/tests/e2e_vendor_npm_build.rs +++ b/crates/socket-patch-cli/tests/e2e_vendor_npm_build.rs @@ -592,6 +592,102 @@ fn npm_vendor_vex_attests_against_vendored_tarball() { ); } +/// #326: a dependency installed from a remote-tarball spec (`"left-pad": +/// "https://…/left-pad-1.3.0.tgz"`) is fetched from that url by `npm ci`, +/// whatever the lock's `resolved` says. Vendoring used to rewire the entry +/// and report `applied` (and `vex` attested it) while `npm ci` installed +/// the original bytes. It must refuse, leave the lock untouched, and give +/// `vex` nothing to attest. +#[test] +fn npm_vendor_refuses_a_remote_tarball_dependency() { + let suite = "e2e_vendor_npm_build (remote tarball)"; + let Some(major) = npm_major_or_skip(suite) else { + 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":"vendor-url-spec","version":"0.0.0","private":true}"#, + ) + .unwrap(); + let cache = tmp.path().join("npm-cache"); + let url = format!("https://registry.npmjs.org/{DEP}/-/{DEP}-{DEP_VERSION}.tgz"); + if !npm_e2e_common::install_fixture(suite, &proj, &cache, &url) { + return; + } + let pkg: serde_json::Value = + serde_json::from_slice(&std::fs::read(proj.join("package.json")).unwrap()).unwrap(); + assert_eq!( + pkg["dependencies"][DEP], url, + "the fixture depends on the url spec: {pkg}" + ); + + let installed_index = proj.join("node_modules").join(DEP).join("index.js"); + let orig = std::fs::read(&installed_index).expect("installed index.js"); + let patched: Vec = [MARKER.as_bytes(), orig.as_slice()].concat(); + let purl = format!("pkg:npm/{DEP}@{DEP_VERSION}"); + const GHSA: &str = "GHSA-vend-npm-url"; + stage_patch_with_vuln(&proj, &purl, "package/index.js", &orig, &patched, GHSA); + if v1_lock_is_refused(&proj, major) { + return; + } + let lock_before = std::fs::read(proj.join("package-lock.json")).unwrap(); + + let (_code, stdout, stderr) = run_socket( + &proj, + &[ + "vendor", + "--json", + "--offline", + "--cwd", + proj.to_str().unwrap(), + ], + ); + let env = parse_envelope(&stdout); + assert_eq!( + env["summary"]["applied"], 0, + "a url-spec dependency must not be vendored: {env}\nstderr:\n{stderr}" + ); + assert!( + stdout.contains("vendor_lock_entry_not_rewritable") && stdout.contains("UNPATCHED"), + "the refusal must say why: {env}" + ); + assert_eq!( + std::fs::read(proj.join("package-lock.json")).unwrap(), + lock_before, + "the lock is untouched" + ); + assert!( + !proj.join(format!(".socket/vendor/npm/{UUID}")).exists(), + "no artifact is written" + ); + + let vex_path = proj.join("out.vex.json"); + let (code, stdout, stderr) = run_socket( + &proj, + &[ + "vex", + "--cwd", + proj.to_str().unwrap(), + "--output", + vex_path.to_str().unwrap(), + "--product", + "pkg:npm/app@1.0.0", + ], + ); + let attested = std::fs::read(&vex_path) + .ok() + .and_then(|b| serde_json::from_slice::(&b).ok()) + .and_then(|doc| doc["statements"].as_array().map(|s| !s.is_empty())) + .unwrap_or(false); + assert!( + !attested, + "vex must not attest an unwired patch (exit {code}).\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); +} + /// get-driven twin of the capstone (v3.6): instead of hand-staging /// `.socket/` (manifest + blob) and running `vendor --offline`, /// `get --mode vendored` resolves the SAME patch from a mocked diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index abc6d41ce..3fa373e7b 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -25,6 +25,7 @@ use serde_json::{json, Value}; use crate::utils::composer_version::composer_versions_equivalent; use crate::utils::digest::is_hex64_lower; use crate::utils::line_endings::{to_lf, LineEndings}; +use crate::vendor::npm_origin::npm_non_registry_entries; use crate::vendor::yarn_berry_lock::yarnrc_compression_level; mod bun_binary; @@ -859,6 +860,9 @@ fn rewrite_one_npm_lock( .collect() }) .unwrap_or_default(); + // Entries npm installs from a git / url / `file:` spec: see + // `vendor::npm_origin` (#326). + let non_registry = npm_non_registry_entries(&lock); let mut changed = false; for dep in npm { let fname = full_name(dep); @@ -906,6 +910,22 @@ fn rewrite_one_npm_lock( }); continue; } + // npm installs a git / url / `file:` dependency from the + // dependent's spec and ignores `resolved`, so a rewrite here + // would confirm (and VEX-attest) a patch that never installs. + if let Some(reason) = non_registry.get(key.as_str()) { + matched_any = true; + result.warnings.push(RewriteWarning { + code: "redirect_npm_non_registry_entry_skipped".into(), + detail: format!( + "lock entry `{key}` is not installed from the registry ({reason}) \ + and CANNOT be redirected — npm installs it from that spec, so \ + that copy stays UNPATCHED; depend on the registry release to \ + patch it" + ), + }); + continue; + } matched_any = true; if let Some(edit) = rewrite_npm_entry( entry, @@ -13162,6 +13182,123 @@ mod tests { ); } + /// #326: npm installs a git, remote-tarball or `file:` dependency from + /// the dependent's spec and ignores the lock's `resolved`, so rewiring + /// that entry would report (and VEX-attest) a patch `npm ci` never + /// installs. It must be skipped loudly, like a bundled copy. + #[test] + fn npm_non_registry_entries_are_skipped_with_loud_warning() { + let url = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz"; + for (spec, resolved) in [ + ( + "github:stevemao/left-pad#v1.3.0", + "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba", + ), + (url, url), + ("file:../left-pad-1.3.0.tgz", "file:../left-pad-1.3.0.tgz"), + ] { + let lock = json!({ + "name": "app", + "lockfileVersion": 3, + "packages": { + "": { "name": "app", "version": "0.0.0", "dependencies": { "left-pad": spec } }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": resolved, + "integrity": "sha512-UPSTREAM==" + } + } + }); + let mut files = BTreeMap::new(); + files.insert( + "package-lock.json".to_string(), + serde_json::to_string_pretty(&lock).unwrap(), + ); + let overrides = vec![npm_override( + "left-pad", + "1.3.0", + "http://patch.test/lp.tgz", + "sha512-PATCHED==", + )]; + let r = rewrite_registry_redirect(&files, &overrides); + assert!( + r.files.is_empty() && r.edits.is_empty(), + "{spec}: a non-registry entry must not be rewired: {:?}", + r.edits + ); + let skipped = r + .warnings + .iter() + .find(|w| w.code == "redirect_npm_non_registry_entry_skipped") + .unwrap_or_else(|| panic!("{spec}: the skip must warn: {:?}", r.warnings)); + assert!( + skipped.detail.contains("UNPATCHED") + && skipped.detail.contains("node_modules/left-pad"), + "{spec}: {}", + skipped.detail + ); + assert!( + !warning_codes(&r).contains(&"redirect_npm_entry_not_found"), + "{spec}: the entry was found: {:?}", + r.warnings + ); + } + } + + /// #326, transitive: a nested git copy is skipped while the hoisted + /// registry copy of the same version is still redirected. + #[test] + fn npm_nested_git_copy_is_skipped_and_registry_copy_rewired() { + let lock = json!({ + "name": "app", + "lockfileVersion": 3, + "packages": { + "": { "name": "app", "version": "0.0.0", + "dependencies": { "a": "^1.0.0", "left-pad": "^1.3.0" } }, + "node_modules/a": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/a/-/a-1.0.0.tgz", + "integrity": "sha512-A==", + "dependencies": { "left-pad": "stevemao/left-pad#v1.3.0" } + }, + "node_modules/a/node_modules/left-pad": { + "version": "1.3.0", + "resolved": "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba" + }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz", + "integrity": "sha512-UPSTREAM==" + } + } + }); + let mut files = BTreeMap::new(); + files.insert( + "package-lock.json".to_string(), + serde_json::to_string_pretty(&lock).unwrap(), + ); + let overrides = vec![npm_override( + "left-pad", + "1.3.0", + "http://patch.test/lp.tgz", + "sha512-PATCHED==", + )]; + let r = rewrite_registry_redirect(&files, &overrides); + assert_eq!(r.edits.len(), 1, "{:?}", r.edits); + assert_eq!(r.edits[0].key.as_deref(), Some("node_modules/left-pad")); + assert!( + warning_codes(&r).contains(&"redirect_npm_non_registry_entry_skipped"), + "{:?}", + r.warnings + ); + let out: Value = serde_json::from_str(&r.files["package-lock.json"]).unwrap(); + assert_eq!( + out["packages"]["node_modules/a/node_modules/left-pad"], + lock["packages"]["node_modules/a/node_modules/left-pad"], + "the git copy is byte-untouched" + ); + } + /// When the patched dep has both a regular entry and a bundled nested /// copy, the regular entry is redirected and the bundled copy is left /// byte-untouched behind the stays-UNPATCHED warning (partial coverage diff --git a/crates/socket-patch-core/src/vendor/mod.rs b/crates/socket-patch-core/src/vendor/mod.rs index 68fcb5d2a..d6f2dc33f 100644 --- a/crates/socket-patch-core/src/vendor/mod.rs +++ b/crates/socket-patch-core/src/vendor/mod.rs @@ -73,6 +73,7 @@ pub(crate) mod npm_common; pub(crate) mod npm_dir; pub mod npm_flavor; pub mod npm_lock; +pub(crate) mod npm_origin; mod npm_pack; pub(crate) mod nuget_config; pub mod nuget_feed; diff --git a/crates/socket-patch-core/src/vendor/npm_lock.rs b/crates/socket-patch-core/src/vendor/npm_lock.rs index fc66c3b8c..02dbb5fdd 100644 --- a/crates/socket-patch-core/src/vendor/npm_lock.rs +++ b/crates/socket-patch-core/src/vendor/npm_lock.rs @@ -28,6 +28,7 @@ use super::common::{already_patched_result, detect_indent, done, refused, serial use super::npm_common::{ done_failure_unstage, guard_coordinates, guard_revert_uuid_dir, stage_patch_pack, }; +use super::npm_origin::npm_non_registry_entries; use super::parse_memo::ParseMemo; use super::path::parse_vendor_path; use super::source::PackageSource; @@ -460,7 +461,9 @@ fn rewritable_matches( .filter(|w| { matches!( w.code, - "vendor_bundled_instance_skipped" | "vendor_link_entry_skipped" + "vendor_bundled_instance_skipped" + | "vendor_link_entry_skipped" + | "vendor_non_registry_entry_skipped" ) }) .map(|w| w.detail.as_str()) @@ -470,8 +473,9 @@ fn rewritable_matches( "vendor_lock_entry_not_rewritable", format!( "every {lock_name} entry for {name}@{version} is bundled inside a \ - parent's tarball or a link and cannot be rewritten — those copies \ - stay UNPATCHED and `npm install` will not help: {}", + parent's tarball, a link, or installed from a non-registry spec and \ + cannot be rewritten — those copies stay UNPATCHED and `npm install` \ + will not help: {}", skipped.join("; ") ), ))); @@ -833,6 +837,7 @@ fn scan_lock_matches( let Some(packages) = lock.get("packages").and_then(Value::as_object) else { return LockScan::Matches(matches); // validated earlier; defensive }; + let non_registry = npm_non_registry_entries(lock); for (key, entry) in packages { // The root "" entry is the project itself, never a dependency. if key.is_empty() { @@ -870,6 +875,21 @@ fn scan_lock_matches( )); continue; } + if let Some(reason) = non_registry.get(key.as_str()) { + // LOUD: npm installs a git / url / `file:` dependency from the + // dependent's spec and ignores `resolved`, so a rewrite here + // would report the patch applied while the original bytes + // install (#326). + warnings.push(VendorWarning::new( + "vendor_non_registry_entry_skipped", + format!( + "lock entry `{key}` is not installed from the registry ({reason}) and \ + CANNOT be rewritten — npm installs it from that spec, so that copy stays \ + UNPATCHED; depend on the registry release to vendor it" + ), + )); + continue; + } matches.push(LockMatch { key: key.clone(), original: entry.clone(), @@ -2103,6 +2123,90 @@ mod tests { ); } + /// #326: a git / remote-tarball / `file:` dependency is installed from + /// the dependent's spec, not the lock's `resolved`, so vendoring it + /// would report `applied` while `npm ci` installs the original bytes. + /// With no other copy the vendor refuses and writes nothing. + #[tokio::test] + async fn non_registry_only_instances_refuse_and_write_nothing() { + let url = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz"; + for (spec, resolved) in [ + ( + "github:stevemao/left-pad#v1.3.0", + "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba", + ), + (url, url), + ("file:../left-pad-1.3.0.tgz", "file:../left-pad-1.3.0.tgz"), + ] { + let lock = json!({ + "name": "fixture", + "version": "1.0.0", + "lockfileVersion": 3, + "packages": { + "": { "name": "fixture", "version": "1.0.0", + "dependencies": { "left-pad": spec } }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": resolved, + "integrity": "sha512-orig==" + } + } + }); + let fx = fixture_with("left-pad", "1.3.0", lock).await; + let detail = expect_refused(fx.vendor(false).await, "vendor_lock_entry_not_rewritable"); + assert!( + detail.contains("UNPATCHED") && detail.contains("node_modules/left-pad"), + "{spec}: {detail}" + ); + assert!( + !detail.contains("make sure the package is installed"), + "{spec}: {detail}" + ); + assert_eq!( + tokio::fs::read(fx.lock_path()).await.unwrap(), + fx.lock_bytes, + "{spec}: lock untouched by the refusal" + ); + assert!( + !fx.root().join(".socket/vendor").exists(), + "{spec}: refusal writes nothing" + ); + } + } + + /// #326, transitive: the nested git copy is skipped loudly and the + /// registry copies are still vendored. + #[tokio::test] + async fn nested_git_instance_is_skipped_with_warning() { + let mut lock = default_lock(); + lock["packages"]["node_modules/foo"]["dependencies"] = + json!({ "left-pad": "github:stevemao/left-pad#v1.3.0" }); + lock["packages"]["node_modules/foo/node_modules/left-pad"]["resolved"] = + json!("git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba"); + let fx = fixture_with("left-pad", "1.3.0", lock.clone()).await; + let (result, entry, warnings) = expect_done(fx.vendor(false).await); + assert!(result.success); + assert_eq!(entry.unwrap().wiring.len(), 1, "only the hoisted copy"); + let skipped = warnings + .iter() + .find(|w| w.code == "vendor_non_registry_entry_skipped") + .unwrap_or_else(|| panic!("{warnings:?}")); + assert!( + skipped.detail.contains("UNPATCHED") + && skipped + .detail + .contains("node_modules/foo/node_modules/left-pad"), + "{}", + skipped.detail + ); + let live = fx.read_lock().await; + assert_eq!( + live["packages"]["node_modules/foo/node_modules/left-pad"], + lock["packages"]["node_modules/foo/node_modules/left-pad"], + "the git copy is byte-untouched" + ); + } + /// When EVERY lock instance of the target is bundled or a link, the /// refusal must state the real reason (the entry IS in the lock and /// `npm install` will not help) and keep the stays-UNPATCHED advisory — diff --git a/crates/socket-patch-core/src/vendor/npm_origin.rs b/crates/socket-patch-core/src/vendor/npm_origin.rs new file mode 100644 index 000000000..6e5a0b6f2 --- /dev/null +++ b/crates/socket-patch-core/src/vendor/npm_origin.rs @@ -0,0 +1,334 @@ +//! Which `package-lock.json` / `npm-shrinkwrap.json` entries npm installs +//! from the registry, and which it installs from somewhere else. +//! +//! npm installs a git (`github:user/repo`, `git+ssh://…`), remote-tarball +//! (`https://…/x.tgz`) or local (`file:…`) dependency from the DEPENDENT's +//! spec, not from the lock entry's `resolved`: rewriting that entry's +//! `resolved` / `integrity` changes nothing at install time (`npm ci` fetches +//! the git checkout or the URL again, and the next `npm install` writes the +//! original `resolved` back). The hosted rewriter +//! (`patch::redirect::rewrite_one_npm_lock`), the vendored backend +//! (`vendor::npm_lock`) and lockfile discovery (`vex::discover::npm`) all +//! share [`npm_non_registry_entries`], so a copy the rewriters refuse is +//! never attested either. +//! +//! An entry is non-registry when EITHER +//! +//! * an inbound dependency spec that resolves to it is not a registry spec +//! ([`npm_spec_is_registry`]). The edges are the `dependencies`, +//! `optionalDependencies`, `devDependencies` and `peerDependencies` of +//! every `packages` entry (the root `""`, workspace members and installed +//! packages alike), resolved with node's lookup order: the dependent's own +//! `node_modules/`, then each ancestor directory's; +//! * or its own `resolved` names a git or `file:` source. socket-patch's own +//! vendored wiring (`file:.socket/vendor/…`) is not one: the vendored +//! backend only rewrites `resolved`, never the dependent's spec. + +use std::collections::BTreeMap; + +use serde_json::Value; + +use crate::constants::SOCKET_DIR; + +/// The dependency maps whose specs npm resolves against `packages` entries. +const EDGE_FIELDS: [&str; 4] = [ + "dependencies", + "optionalDependencies", + "devDependencies", + "peerDependencies", +]; + +/// Every `packages` key npm installs from a non-registry source, mapped to +/// the reason (for the skip warnings). Empty for a lock without `packages`: +/// in a lockfileVersion 1 `dependencies` tree a git / URL / `file:` entry's +/// `version` is that spec, so it never matches a patch's `name@version`. +pub(crate) fn npm_non_registry_entries(lock: &Value) -> BTreeMap { + let mut out = BTreeMap::new(); + let Some(packages) = lock.get("packages").and_then(Value::as_object) else { + return out; + }; + for (key, entry) in packages { + if !key.contains("node_modules/") { + continue; + } + if let Some(resolved) = entry.get("resolved").and_then(Value::as_str) { + if resolved_is_non_registry(resolved) { + out.insert(key.clone(), format!("it resolves to {resolved:?}")); + } + } + } + for (from, entry) in packages { + for field in EDGE_FIELDS { + let Some(deps) = entry.get(field).and_then(Value::as_object) else { + continue; + }; + for (dep_name, spec) in deps { + let Some(spec) = spec.as_str() else { + continue; + }; + if npm_spec_is_registry(spec) { + continue; + } + let Some(target) = resolve_edge(packages, from, dep_name) else { + continue; + }; + let dependent = if from.is_empty() { + "the project".to_string() + } else { + format!("`{from}`") + }; + out.entry(target).or_insert_with(|| { + format!( + "{dependent} depends on it as {spec:?}, which npm installs from that spec" + ) + }); + } + } + } + out +} + +/// The `packages` key node's module lookup picks for `dep_name` required +/// from the package at `from`: `/node_modules/`, then the same +/// under each ancestor directory, up to the project root. +fn resolve_edge( + packages: &serde_json::Map, + from: &str, + dep_name: &str, +) -> Option { + let mut dir = from; + loop { + let candidate = if dir.is_empty() { + format!("node_modules/{dep_name}") + } else { + format!("{dir}/node_modules/{dep_name}") + }; + if packages.contains_key(&candidate) { + return Some(candidate); + } + if dir.is_empty() { + return None; + } + dir = dir.rsplit_once('/').map_or("", |(parent, _)| parent); + } +} + +/// A lock `resolved` that is a git or local source rather than a tarball +/// url (see the module docs for socket-patch's own `file:` wiring). +fn resolved_is_non_registry(resolved: &str) -> bool { + const GIT: [&str; 7] = [ + "git+", + "git:", + "git@", + "github:", + "gitlab:", + "bitbucket:", + "gist:", + ]; + if GIT.iter().any(|p| resolved.starts_with(p)) { + return true; + } + match resolved.strip_prefix("file:") { + Some(path) => { + let path = path.trim_start_matches("./"); + !path.starts_with(&format!("{SOCKET_DIR}/vendor/")) + } + None => false, + } +} + +/// Whether npm resolves `spec` against the registry: a version, a semver +/// range, a dist-tag, or an `npm:` alias of one. Everything else (git, +/// GitHub shorthand, a url, a path, a tarball file name) is installed from +/// the spec itself. Mirrors npm-package-arg's classification. +pub(crate) fn npm_spec_is_registry(spec: &str) -> bool { + let spec = spec.trim(); + if let Some(alias) = spec.strip_prefix("npm:") { + // `npm:name`, `npm:name@range`, `npm:@scope/name@range`. + let unscoped = alias.strip_prefix('@').unwrap_or(alias); + return match unscoped.split_once('@') { + Some((_, range)) => npm_spec_is_registry(range), + None => true, + }; + } + // A scheme (`git+ssh:`, `github:`, `https:`, `file:`, a `C:` drive), a + // path (`./x`, `../x`, `/x`, `~/x`), GitHub shorthand (`user/repo`) or + // a tarball file name. No semver range or dist-tag contains ':' or '/'. + if spec.contains(':') || spec.contains('/') || spec.contains('\\') { + return false; + } + if spec.starts_with('.') { + return false; + } + let lower = spec.to_ascii_lowercase(); + !(lower.ends_with(".tgz") || lower.ends_with(".tar.gz") || lower.ends_with(".tar")) +} + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::*; + + #[test] + fn registry_specs_are_recognized() { + for spec in [ + "", + "*", + "1.3.0", + "^1.3.0", + "~1.3.0", + ">=1 <2", + "1.x || 2", + "latest", + "next", + "npm:left-pad@1.3.0", + "npm:@scope/pad@^1", + "npm:left-pad", + ] { + assert!(npm_spec_is_registry(spec), "{spec:?} is a registry spec"); + } + } + + #[test] + fn non_registry_specs_are_recognized() { + for spec in [ + "github:stevemao/left-pad#v1.3.0", + "stevemao/left-pad", + "stevemao/left-pad#v1.3.0", + "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba", + "git+https://github.com/stevemao/left-pad.git", + "git://github.com/stevemao/left-pad.git", + "gitlab:user/repo", + "bitbucket:user/repo", + "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz", + "http://example.com/left-pad.tgz", + "file:../left-pad-1.3.0.tgz", + "file:vendor/left-pad", + "./left-pad", + "../left-pad", + "/abs/left-pad", + "~/left-pad", + "left-pad-1.3.0.tgz", + "C:\\pkgs\\left-pad.tgz", + "npm:left-pad@github:stevemao/left-pad", + ] { + assert!( + !npm_spec_is_registry(spec), + "{spec:?} is not a registry spec" + ); + } + } + + fn lock(packages: Value) -> Value { + json!({ "lockfileVersion": 3, "packages": packages }) + } + + #[test] + fn a_direct_git_dependency_is_non_registry() { + let lock = lock(json!({ + "": { "dependencies": { "left-pad": "github:stevemao/left-pad#v1.3.0" } }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba" + } + })); + let found = npm_non_registry_entries(&lock); + assert!(found.contains_key("node_modules/left-pad"), "{found:?}"); + } + + #[test] + fn a_rewired_git_dependency_is_still_non_registry_by_its_spec() { + // After a rewrite the entry's `resolved` is a hosted url or our own + // `file:.socket/vendor/…` tarball; the dependent's spec still says git. + for resolved in [ + "https://patch.socket.dev/npm/left-pad/-/left-pad-1.3.0.tgz", + "file:.socket/vendor/npm/11111111-2222-4333-8444-555555555555/left-pad-1.3.0.tgz", + ] { + let lock = lock(json!({ + "": { "dependencies": { "left-pad": "github:stevemao/left-pad#v1.3.0" } }, + "node_modules/left-pad": { "version": "1.3.0", "resolved": resolved } + })); + assert!(npm_non_registry_entries(&lock).contains_key("node_modules/left-pad")); + } + } + + #[test] + fn a_remote_tarball_dependency_is_non_registry() { + // The url is the registry's own tarball, but npm installs it from + // the spec: only the dependent's spec tells the two apart. + let url = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz"; + let lock = lock(json!({ + "": { "dependencies": { "left-pad": url } }, + "node_modules/left-pad": { "version": "1.3.0", "resolved": url } + })); + assert!(npm_non_registry_entries(&lock).contains_key("node_modules/left-pad")); + } + + #[test] + fn a_file_tarball_dependency_is_non_registry() { + let lock = lock(json!({ + "": { "dependencies": { "left-pad": "file:../left-pad-1.3.0.tgz" } }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": "file:../left-pad-1.3.0.tgz" + } + })); + assert!(npm_non_registry_entries(&lock).contains_key("node_modules/left-pad")); + } + + #[test] + fn transitive_edges_resolve_with_node_lookup_order() { + // `a` depends on left-pad from git and gets its own nested copy; + // the hoisted copy the root depends on is a registry install. + let lock = lock(json!({ + "": { "dependencies": { "a": "^1.0.0", "left-pad": "^1.3.0" } }, + "node_modules/a": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/a/-/a-1.0.0.tgz", + "dependencies": { "left-pad": "github:stevemao/left-pad#v1.3.0" } + }, + "node_modules/a/node_modules/left-pad": { + "version": "1.3.0", + "resolved": "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba" + }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz" + } + })); + let found = npm_non_registry_entries(&lock); + assert!(found.contains_key("node_modules/a/node_modules/left-pad")); + assert!(!found.contains_key("node_modules/left-pad"), "{found:?}"); + assert!(!found.contains_key("node_modules/a")); + } + + #[test] + fn a_workspace_member_edge_walks_up_to_the_hoisted_copy() { + let url = "https://example.com/left-pad-1.3.0.tgz"; + let lock = lock(json!({ + "": { "workspaces": ["packages/*"] }, + "packages/app": { "dependencies": { "left-pad": url } }, + "node_modules/app": { "resolved": "packages/app", "link": true }, + "node_modules/left-pad": { "version": "1.3.0", "resolved": url } + })); + assert!(npm_non_registry_entries(&lock).contains_key("node_modules/left-pad")); + } + + #[test] + fn registry_dependencies_are_not_flagged() { + let lock = lock(json!({ + "": { "dependencies": { "left-pad": "1.3.0", "alias": "npm:left-pad@1.3.0" } }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz" + }, + "node_modules/alias": { + "name": "left-pad", + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz" + } + })); + assert!(npm_non_registry_entries(&lock).is_empty()); + } +} diff --git a/crates/socket-patch-core/src/vex/discover/npm.rs b/crates/socket-patch-core/src/vex/discover/npm.rs index a9ddfe3a7..0ca562cde 100644 --- a/crates/socket-patch-core/src/vex/discover/npm.rs +++ b/crates/socket-patch-core/src/vex/discover/npm.rs @@ -55,6 +55,7 @@ use crate::vendor::lock_inventory::pnpm::{ use crate::vendor::lock_inventory::{ npm_lock_nodes, pnpm_registry_key, LockIntegrity, NpmLockNode, }; +use crate::vendor::npm_origin::npm_non_registry_entries; pub(crate) async fn extract(ctx: &DiscoverCtx<'_>, out: &mut Discovery) { let mut locks: Vec = Vec::new(); @@ -146,9 +147,63 @@ async fn extract_package_lock( for node in npm_lock_nodes(&doc) { entry_ref(ctx, file, &node, &mut read, out); } + drop_non_registry_installs(file, &doc, &mut read, out); Some(read) } +/// npm installs a git / url / `file:` dependency from the dependent's spec +/// and ignores the entry's `resolved` (`vendor::npm_origin`, #326), so such +/// an entry stays unpatched whatever its `resolved` says. Every ref for the +/// same `name@version` is dropped (that copy is live beside it), and the +/// copy counts as resolved elsewhere, so other locks' wiring for it is +/// contested too. +fn drop_non_registry_installs( + file: &str, + doc: &Value, + read: &mut NpmLockRefs, + out: &mut Discovery, +) { + let non_registry = npm_non_registry_entries(doc); + if non_registry.is_empty() { + return; + } + let mut unpatched: Vec<(String, &str, &str)> = Vec::new(); + for (key, reason) in &non_registry { + let entry = &doc["packages"][key.as_str()]; + let key_name = key.rsplit_once("node_modules/").map_or("", |(_, n)| n); + let name = entry + .get("name") + .and_then(Value::as_str) + .unwrap_or(key_name); + let Some(purl) = entry + .get("version") + .and_then(Value::as_str) + .and_then(|v| npm_purl(name, v)) + else { + continue; + }; + out.resolved_elsewhere(file, Some(purl.clone())); + read.unwired.insert(purl.clone()); + unpatched.push((purl, key, reason)); + } + read.refs.retain(|r| { + let Some((_, key, reason)) = unpatched.iter().find(|(p, _, _)| *p == r.purl) else { + return true; + }; + out.diag( + DIAG_REF_UNATTRIBUTABLE, + file, + format!( + "{file}: {} is wired to Socket patch {} but lock entry `{key}` is not \ + installed from the registry ({reason}); npm installs it from that spec, so \ + that copy stays UNPATCHED and nothing is attested", + r.purl, r.uuid + ), + ); + false + }); +} + /// Classify one lock entry: a ref (into `read.refs`), a package resolved /// elsewhere (into `read.unwired`), or nothing. fn entry_ref( @@ -841,6 +896,86 @@ mod tests { assert!(out.refs.is_empty(), "{:#?}", out.refs); } + /// #326: a Socket-wired entry npm installs from a git / url / `file:` + /// spec (a lock rewired before the rewriters refused these, or by + /// hand) wires nothing, and neither does a wired registry copy while a + /// non-registry copy of the same version stays unpatched beside it. + #[tokio::test] + async fn entries_npm_installs_from_a_non_registry_spec_are_not_attested() { + let hosted = hosted_url("npm", "left-pad", "1.3.0", UUID_A, "left-pad-1.3.0.tgz"); + let vendored = format!("file:.socket/vendor/npm/{UUID_B}/left-pad-1.3.0.tgz"); + let tarball = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz"; + for spec in [ + "github:stevemao/left-pad#v1.3.0", + tarball, + "file:../left-pad-1.3.0.tgz", + ] { + for resolved in [&hosted, &vendored] { + let p = Project::new(); + p.write( + "package-lock.json", + lock_with_packages(serde_json::json!({ + "": { "name": "app", "version": "1.0.0", + "dependencies": { "left-pad": spec } }, + "node_modules/left-pad": { + "version": "1.3.0", "resolved": resolved, "integrity": SRI + }, + })), + ); + let out = run(&p).await; + assert!(out.refs.is_empty(), "{spec} / {resolved}: {:#?}", out.refs); + assert!( + diag_codes(&out).contains(&DIAG_REF_UNATTRIBUTABLE), + "{spec} / {resolved}: {:?}", + out.diagnostics + ); + } + } + // Transitive: the hoisted copy is wired, a nested git copy of the + // same version is not. + let p = Project::new(); + p.write( + "package-lock.json", + lock_with_packages(serde_json::json!({ + "": { "name": "app", "version": "1.0.0", + "dependencies": { "a": "^1.0.0", "left-pad": "^1.3.0" } }, + "node_modules/a": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/a/-/a-1.0.0.tgz", + "dependencies": { "left-pad": "stevemao/left-pad#v1.3.0" } + }, + "node_modules/a/node_modules/left-pad": { + "version": "1.3.0", + "resolved": "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba" + }, + "node_modules/left-pad": { "version": "1.3.0", "resolved": hosted, "integrity": SRI }, + })), + ); + let out = run(&p).await; + assert!(out.refs.is_empty(), "{:#?}", out.refs); + let diag = out + .diagnostics + .iter() + .find(|d| d.code == DIAG_REF_UNATTRIBUTABLE) + .unwrap_or_else(|| panic!("{:?}", out.diagnostics)); + assert!( + diag.detail.contains("node_modules/a/node_modules/left-pad"), + "{}", + diag.detail + ); + // Control: a registry spec keeps the ref. + let p = Project::new(); + p.write( + "package-lock.json", + lock_with_packages(serde_json::json!({ + "": { "name": "app", "version": "1.0.0", + "dependencies": { "left-pad": "^1.3.0" } }, + "node_modules/left-pad": { "version": "1.3.0", "resolved": hosted, "integrity": SRI }, + })), + ); + assert_eq!(run(&p).await.refs.len(), 1); + } + /// Negative shapes: a uuid on a foreign host, a placeholder token, the /// root and workspace-member keys, and an escaping vendored path. #[tokio::test] From d53ef13a002207db4fe868c3610a363aca33876b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 30 Sep 2026 17:38:41 +0000 Subject: [PATCH 3/5] Document the npm non-registry entry skip Assisted-by: Claude Code:claude-opus-5-5 --- CHANGELOG.md | 11 +++++++++++ crates/socket-patch-cli/CLI_CONTRACT.md | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b85d8bf44..2cb6bcdd3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -914,6 +914,17 @@ into the new version's section — see docs/releasing.md. ### Fixed +- **npm dependencies installed from git, a URL or `file:` are no longer + reported patched.** npm installs such a dependency from the dependent's + spec (`github:user/repo`, `https://…/x.tgz`, `file:…`) and ignores the + lock entry's `resolved`, so `npm ci` kept installing the original bytes + after `scan --mode hosted` or `vendor` rewired the entry and `vex` + attested it. Both modes now skip such an entry with a loud + stays-UNPATCHED warning (`redirect_npm_non_registry_entry_skipped` / + `vendor_non_registry_entry_skipped`; vendoring refuses with + `vendor_lock_entry_not_rewritable` when no registry copy is left), and + `vex` attests nothing for a `name@version` while such a copy is in the + lock (#326). - **Hosted nuget redirects survive a `` in `nuget.config`.** The Socket source (and, in an existing ``, its mapping) was inserted ahead of the section's ``, which NuGet diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index 66d224b0d..2c8dce192 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -209,7 +209,7 @@ Discovery is read-only, never touches the network, and never fails the run: a ma | Ecosystem | Files read | Hosted reference | Vendored reference | Hosted pin (`integrity_required`) | |---|---|---|---|---| -| npm | `package-lock.json` and `npm-shrinkwrap.json` (both when both exist) | `resolved` on the patch host (`packages` in v2/v3; `dependencies` only in v1; `link` / `inBundle` / `bundled` entries skipped) | `resolved: file:.socket/vendor/npm//-.tgz` | `integrity`, required | +| npm | `package-lock.json` and `npm-shrinkwrap.json` (both when both exist) | `resolved` on the patch host (`packages` in v2/v3; `dependencies` only in v1; `link` / `inBundle` / `bundled` entries skipped, and so is any entry npm installs from a git, URL or `file:` spec, together with every ref for the same `name@version`) | `resolved: file:.socket/vendor/npm//-.tgz` | `integrity`, required | | pnpm | `pnpm-lock.yaml` (every `lockfileVersion`); `shrinkwrap.yaml` only when there is no `pnpm-lock.yaml`; with `rush.json`, `common/config/rush/pnpm-lock.yaml` + `common/config/subspaces/*/pnpm-lock.yaml` | `packages:` `resolution.tarball` on the patch host | `file:.socket/vendor/npm/…` tarball + key | `integrity`, required | | yarn | `yarn.lock` (classic and berry) | classic `resolved`; berry `resolution: …::__archiveUrl=` | classic `resolved "file:./.socket/vendor/npm/…#"`; berry `file:` entry **plus** a root `package.json` `resolutions` mapping onto the same artifact (without it the entry is orphaned: diagnosed, no ref) | classic `integrity` / `#sha1`, berry `checksum`, required | | bun | `bun.lock`; `bun.lockb` only when there is no `bun.lock` (bun reads exactly one) | URL tuple / binary remote-tarball resolution; version from the URL leaf | `.socket/vendor/npm//-.tgz` tuple / local-tarball resolution | `sha512-…`, required. A 2-tuple that Bun < 1.3.10 re-saved without its digest is still a reference, but it attests only from an installed tree. | From f07b94aa4a7a9d7233a66ecc4df26d1b53934705 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 30 Sep 2026 18:01:35 +0000 Subject: [PATCH 4/5] Let the npm 12 e2e install a URL dependency npm 12 refuses remote-tarball specs unless allow-remote permits them, so the new vendored e2e now passes --allow-remote=all there. The VEX test messages and the new diagnostic no longer print patch URLs or uuids. Assisted-by: Claude Code:claude-opus-5-5 --- .../tests/e2e_vendor_npm_build.rs | 19 ++++++++++++++++++- .../socket-patch-core/src/vex/discover/npm.rs | 18 +++++++++++------- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/crates/socket-patch-cli/tests/e2e_vendor_npm_build.rs b/crates/socket-patch-cli/tests/e2e_vendor_npm_build.rs index a4f7cf841..3e4c964a8 100644 --- a/crates/socket-patch-cli/tests/e2e_vendor_npm_build.rs +++ b/crates/socket-patch-cli/tests/e2e_vendor_npm_build.rs @@ -614,7 +614,24 @@ fn npm_vendor_refuses_a_remote_tarball_dependency() { .unwrap(); let cache = tmp.path().join("npm-cache"); let url = format!("https://registry.npmjs.org/{DEP}/-/{DEP}-{DEP_VERSION}.tgz"); - if !npm_e2e_common::install_fixture(suite, &proj, &cache, &url) { + // npm 12 refuses remote-tarball specs unless `allow-remote` permits them. + let spec = if major >= 12 { + format!("{url} --allow-remote=all") + } else { + url.clone() + }; + let mut args = vec!["install", "--no-audit", "--no-fund", "--cache"]; + args.push(cache.to_str().unwrap()); + args.extend(spec.split(' ')); + let out = npm(&proj, &args); + if !out.status.success() { + npm_e2e_common::skip( + suite, + &format!( + "`npm install {spec}` failed (registry unreachable?):\n{}", + String::from_utf8_lossy(&out.stderr) + ), + ); return; } let pkg: serde_json::Value = diff --git a/crates/socket-patch-core/src/vex/discover/npm.rs b/crates/socket-patch-core/src/vex/discover/npm.rs index 0ca562cde..b8fad24a0 100644 --- a/crates/socket-patch-core/src/vex/discover/npm.rs +++ b/crates/socket-patch-core/src/vex/discover/npm.rs @@ -194,10 +194,10 @@ fn drop_non_registry_installs( DIAG_REF_UNATTRIBUTABLE, file, format!( - "{file}: {} is wired to Socket patch {} but lock entry `{key}` is not \ + "{file}: {} is wired to a Socket patch but lock entry `{key}` is not \ installed from the registry ({reason}); npm installs it from that spec, so \ that copy stays UNPATCHED and nothing is attested", - r.purl, r.uuid + r.purl ), ); false @@ -910,7 +910,7 @@ mod tests { tarball, "file:../left-pad-1.3.0.tgz", ] { - for resolved in [&hosted, &vendored] { + for (wiring, resolved) in [("hosted", &hosted), ("vendored", &vendored)] { let p = Project::new(); p.write( "package-lock.json", @@ -923,11 +923,15 @@ mod tests { })), ); let out = run(&p).await; - assert!(out.refs.is_empty(), "{spec} / {resolved}: {:#?}", out.refs); + assert!( + out.refs.is_empty(), + "{spec} / {wiring}: {} refs", + out.refs.len() + ); assert!( diag_codes(&out).contains(&DIAG_REF_UNATTRIBUTABLE), - "{spec} / {resolved}: {:?}", - out.diagnostics + "{spec} / {wiring}: {:?}", + diag_codes(&out) ); } } @@ -952,7 +956,7 @@ mod tests { })), ); let out = run(&p).await; - assert!(out.refs.is_empty(), "{:#?}", out.refs); + assert!(out.refs.is_empty(), "{} refs", out.refs.len()); let diag = out .diagnostics .iter() From c46271c382d804825e7ca527b659d34942942be6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 30 Sep 2026 18:29:08 +0000 Subject: [PATCH 5/5] Skip the v2 legacy mirror of non-registry npm entries The hosted rewriter and the vendored backend skipped a git, URL or file: packages entry, but still rewired its lockfileVersion 2 legacy dependencies mirror when that mirror stored the plain version. Each legacy node now maps to the packages key it mirrors, and a node whose twin is non-registry is left alone. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LycgFGuki2BqZ25VNhwxJ5 --- .../redirect/lock_index_equivalence_tests.rs | 4 + .../src/patch/redirect/mod.rs | 104 +++++++++++++++++- .../socket-patch-core/src/vendor/npm_lock.rs | 86 ++++++++++++++- .../src/vendor/npm_origin.rs | 11 ++ 4 files changed, 200 insertions(+), 5 deletions(-) diff --git a/crates/socket-patch-core/src/patch/redirect/lock_index_equivalence_tests.rs b/crates/socket-patch-core/src/patch/redirect/lock_index_equivalence_tests.rs index e343e9d5b..d8539de43 100644 --- a/crates/socket-patch-core/src/patch/redirect/lock_index_equivalence_tests.rs +++ b/crates/socket-patch-core/src/patch/redirect/lock_index_equivalence_tests.rs @@ -359,8 +359,12 @@ fn rewrite_one_npm_lock_oracle( } // v2 legacy `dependencies` tree (keyed by name), recursive. if let Some(deps) = lock.get_mut("dependencies").and_then(Value::as_object_mut) { + // The randomized locks carry only registry specs, so the #326 + // non-registry guard never fires and the oracle passes none. changed = rewrite_npm_v2_deps( deps, + "", + &BTreeMap::new(), &fname, dep, &sha512, diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 3fa373e7b..6ee5ba1dd 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -25,7 +25,7 @@ use serde_json::{json, Value}; use crate::utils::composer_version::composer_versions_equivalent; use crate::utils::digest::is_hex64_lower; use crate::utils::line_endings::{to_lf, LineEndings}; -use crate::vendor::npm_origin::npm_non_registry_entries; +use crate::vendor::npm_origin::{legacy_packages_key, npm_non_registry_entries}; use crate::vendor::yarn_berry_lock::yarnrc_compression_level; mod bun_binary; @@ -944,6 +944,8 @@ fn rewrite_one_npm_lock( if let Some(deps) = lock.get_mut("dependencies").and_then(Value::as_object_mut) { changed = rewrite_npm_v2_deps( deps, + "", + &non_registry, &fname, dep, &sha512, @@ -1018,8 +1020,11 @@ fn rewrite_npm_entry( }) } +#[allow(clippy::too_many_arguments)] fn rewrite_npm_v2_deps( deps: &mut serde_json::Map, + parent_key: &str, + non_registry: &BTreeMap, fname: &str, dep: &DepOverride, sha512: &str, @@ -1029,6 +1034,7 @@ fn rewrite_npm_v2_deps( ) -> bool { let mut changed = false; for (name, entry) in deps.iter_mut() { + let packages_key = legacy_packages_key(parent_key, name); if name == fname && entry.get("version").and_then(Value::as_str) == Some(dep.version.as_str()) { @@ -1044,6 +1050,12 @@ fn rewrite_npm_v2_deps( or update the bundling parent to cover it" ), }); + } else if non_registry.contains_key(&packages_key) { + // The mirror of a `packages` entry npm installs from a git / + // url / `file:` spec: that twin was skipped (and warned about) + // above, so rewriting this copy would only record an edit for + // bytes that never install. + *matched_any = true; } else { *matched_any = true; if let Some(edit) = @@ -1055,9 +1067,17 @@ fn rewrite_npm_v2_deps( } } if let Some(nested) = entry.get_mut("dependencies").and_then(Value::as_object_mut) { - changed = - rewrite_npm_v2_deps(nested, fname, dep, sha512, lockfile, result, matched_any) - || changed; + changed = rewrite_npm_v2_deps( + nested, + &packages_key, + non_registry, + fname, + dep, + sha512, + lockfile, + result, + matched_any, + ) || changed; } } changed @@ -13245,6 +13265,82 @@ mod tests { } } + /// #326, lockfileVersion 2: the legacy `dependencies` mirror of a + /// non-registry `packages` entry is left alone too, even when it stores + /// the plain version, while the registry copy's mirror is rewired. + #[test] + fn npm_v2_legacy_mirror_of_a_non_registry_entry_is_not_rewired() { + let registry = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz"; + let git = "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba"; + let lock = json!({ + "name": "app", + "lockfileVersion": 2, + "packages": { + "": { "name": "app", "version": "0.0.0", + "dependencies": { "a": "^1.0.0", "left-pad": "^1.3.0" } }, + "node_modules/a": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/a/-/a-1.0.0.tgz", + "integrity": "sha512-A==", + "dependencies": { "left-pad": "stevemao/left-pad#v1.3.0" } + }, + "node_modules/a/node_modules/left-pad": { "version": "1.3.0", "resolved": git }, + "node_modules/left-pad": { + "version": "1.3.0", "resolved": registry, "integrity": "sha512-UPSTREAM==" + } + }, + "dependencies": { + "a": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/a/-/a-1.0.0.tgz", + "integrity": "sha512-A==", + "requires": { "left-pad": "stevemao/left-pad#v1.3.0" }, + "dependencies": { + "left-pad": { "version": "1.3.0", "resolved": git } + } + }, + "left-pad": { + "version": "1.3.0", "resolved": registry, "integrity": "sha512-UPSTREAM==" + } + } + }); + let mut files = BTreeMap::new(); + files.insert( + "package-lock.json".to_string(), + serde_json::to_string_pretty(&lock).unwrap(), + ); + let overrides = vec![npm_override( + "left-pad", + "1.3.0", + "http://patch.test/lp.tgz", + "sha512-PATCHED==", + )]; + let r = rewrite_registry_redirect(&files, &overrides); + let keys: Vec<_> = r + .edits + .iter() + .map(|e| (e.kind.as_str(), e.key.as_deref())) + .collect(); + assert_eq!( + keys, + [ + ("redirect_npm_lock_entry", Some("node_modules/left-pad")), + ("redirect_npm_lock_dep", Some("left-pad")), + ], + "only the registry copy and its mirror are rewired" + ); + let out: Value = serde_json::from_str(&r.files["package-lock.json"]).unwrap(); + assert_eq!( + out["dependencies"]["a"]["dependencies"]["left-pad"], + lock["dependencies"]["a"]["dependencies"]["left-pad"], + "the git copy's legacy mirror is byte-untouched" + ); + assert_eq!( + out["dependencies"]["left-pad"]["resolved"], + "http://patch.test/lp.tgz" + ); + } + /// #326, transitive: a nested git copy is skipped while the hoisted /// registry copy of the same version is still redirected. #[test] diff --git a/crates/socket-patch-core/src/vendor/npm_lock.rs b/crates/socket-patch-core/src/vendor/npm_lock.rs index 02dbb5fdd..b5287525d 100644 --- a/crates/socket-patch-core/src/vendor/npm_lock.rs +++ b/crates/socket-patch-core/src/vendor/npm_lock.rs @@ -14,6 +14,7 @@ //! bytes — no error, no patch. Every rewrite therefore carries the packed //! tarball's own hash, never an inherited one. +use std::collections::BTreeMap; use std::path::Path; use serde_json::Value; @@ -28,7 +29,7 @@ use super::common::{already_patched_result, detect_indent, done, refused, serial use super::npm_common::{ done_failure_unstage, guard_coordinates, guard_revert_uuid_dir, stage_patch_pack, }; -use super::npm_origin::npm_non_registry_entries; +use super::npm_origin::{legacy_packages_key, npm_non_registry_entries}; use super::parse_memo::ParseMemo; use super::path::parse_vendor_path; use super::source::PackageSource; @@ -953,6 +954,8 @@ fn recompute_dep_fields(live: &mut serde_json::Map, staged_pkg: & fn rewrite_legacy_tree( deps: &mut serde_json::Map, pointer_base: &str, + parent_key: &str, + non_registry: &BTreeMap, name: &str, version: &str, resolved: &str, @@ -970,6 +973,7 @@ fn rewrite_legacy_tree( continue; }; let pointer = format!("{pointer_base}/{}", escape_json_pointer_token(dep_name)); + let packages_key = legacy_packages_key(parent_key, dep_name); let node_version = obj.get("version").and_then(Value::as_str); if node_version == Some(alias_version.as_str()) { // An aliased consumer of the patched package. The modern @@ -997,6 +1001,14 @@ fn rewrite_legacy_tree( // `resolved`, and rewriting it would desync the two lock halves. // (The `packages` twin carries `inBundle` and already pushed the // stays-UNPATCHED warning.) + } else if dep_name == name + && node_version == Some(version) + && non_registry.contains_key(&packages_key) + { + // The mirror of a `packages` entry npm installs from a git / url + // / `file:` spec (#326): its twin was skipped with + // `vendor_non_registry_entry_skipped`, so rewiring this copy + // would record wiring for bytes that never install. } else if dep_name == name && node_version == Some(version) && !entry_in_sync(obj, resolved, integrity) @@ -1022,6 +1034,8 @@ fn rewrite_legacy_tree( rewrite_legacy_tree( sub, &format!("{pointer}/dependencies"), + &packages_key, + non_registry, name, version, resolved, @@ -1241,6 +1255,8 @@ impl LockRewire<'_> { recomputed_deps: &mut bool, warnings: &mut Vec, ) -> Result<(), String> { + // Taken before any rewrite, for the legacy mirror below. + let non_registry = npm_non_registry_entries(lock); let Some(packages) = lock.get_mut("packages").and_then(Value::as_object_mut) else { return Err("lock `packages` object vanished mid-rewrite".to_string()); }; @@ -1291,6 +1307,8 @@ impl LockRewire<'_> { rewrite_legacy_tree( deps, "/dependencies", + "", + &non_registry, self.name, self.version, self.resolved, @@ -2762,6 +2780,72 @@ mod tests { ); } + /// #326, v2 legacy mirror: the mirror of a non-registry `packages` + /// entry is not rewired either, even when it stores the plain version. + #[tokio::test] + async fn v2_legacy_mirror_of_a_git_instance_is_not_rewritten() { + let git = "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba"; + let lock = json!({ + "name": "fixture", + "version": "1.0.0", + "lockfileVersion": 2, + "requires": true, + "packages": { + "": { "name": "fixture", "version": "1.0.0", + "dependencies": { "foo": "^2.0.0", "left-pad": "^1.3.0" } }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": REG_RESOLVED, + "integrity": "sha512-orig==" + }, + "node_modules/foo": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/foo/-/foo-2.0.0.tgz", + "integrity": "sha512-foo==", + "dependencies": { "left-pad": "github:stevemao/left-pad#v1.3.0" } + }, + "node_modules/foo/node_modules/left-pad": { "version": "1.3.0", "resolved": git } + }, + "dependencies": { + "foo": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/foo/-/foo-2.0.0.tgz", + "integrity": "sha512-foo==", + "requires": { "left-pad": "github:stevemao/left-pad#v1.3.0" }, + "dependencies": { + "left-pad": { "version": "1.3.0", "resolved": git } + } + }, + "left-pad": { + "version": "1.3.0", + "resolved": REG_RESOLVED, + "integrity": "sha512-orig==" + } + } + }); + let fx = fixture_with("left-pad", "1.3.0", lock.clone()).await; + let (result, entry, _) = expect_done(fx.vendor(false).await); + assert!(result.success, "{:?}", result.error); + let legacy_keys: Vec = entry + .unwrap() + .wiring + .iter() + .filter(|r| r.kind == KIND_LOCK_LEGACY_ENTRY) + .filter_map(|r| r.key.clone()) + .collect(); + assert_eq!( + legacy_keys, + ["/dependencies/left-pad"], + "only the registry copy's mirror" + ); + let live = fx.read_lock().await; + assert_eq!( + live["dependencies"]["foo"]["dependencies"]["left-pad"], + lock["dependencies"]["foo"]["dependencies"]["left-pad"], + "the git copy's legacy mirror is byte-untouched" + ); + } + /// v2 legacy mirror: an alias consumer (`"aliased": {"version": /// "npm:left-pad@1.3.0"}`) has no proven equivalent rewrite — it must be /// left untouched AND loudly warned, since npm 6 reading the mirror diff --git a/crates/socket-patch-core/src/vendor/npm_origin.rs b/crates/socket-patch-core/src/vendor/npm_origin.rs index 6e5a0b6f2..14ef34f72 100644 --- a/crates/socket-patch-core/src/vendor/npm_origin.rs +++ b/crates/socket-patch-core/src/vendor/npm_origin.rs @@ -38,6 +38,17 @@ const EDGE_FIELDS: [&str; 4] = [ "peerDependencies", ]; +/// The `packages` key a lockfileVersion 2 legacy `dependencies` node +/// mirrors: `parent` is the mirrored key of the enclosing node (`""` at the +/// top of the tree), `name` the node's key in its `dependencies` map. +pub(crate) fn legacy_packages_key(parent: &str, name: &str) -> String { + if parent.is_empty() { + format!("node_modules/{name}") + } else { + format!("{parent}/node_modules/{name}") + } +} + /// Every `packages` key npm installs from a non-registry source, mapped to /// the reason (for the skip warnings). Empty for a lock without `packages`: /// in a lockfileVersion 1 `dependencies` tree a git / URL / `file:` entry's