From e8d8b715368d71074cca465adc19bc1190ddfeb6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 18:23:11 +0000 Subject: [PATCH 1/9] Start fix for #490 Assisted-by: Claude Code:claude-opus-5-5 From fbaea5bd09f0f44252a605c21038d7747bcdaab8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 18:37:07 +0000 Subject: [PATCH 2/9] Patch npm deps overrides send to the registry When a dependency declares another package from git, a URL or file:, and the project's package.json "overrides" pins it back to a registry version, npm installs the registry release. socket-patch still treated that copy as installed from git: hosted scan skipped it and exited 0 with the package unpatched, vendored scan failed, and vex refused to attest a correctly patched install. The check that decides which lock entries come from the registry now reads the root package.json overrides (top-level rules, rules nested under the dependent or its ancestors, "." values and $name refs). An override that clearly sends the edge to a registry spec makes the copy patchable again in hosted mode, vendored mode and vex. Anything less clear keeps the old, cautious behavior. Fixes #490 Assisted-by: Claude Code:claude-opus-5-5 --- CHANGELOG.md | 5 +- crates/socket-patch-core/src/hosted/engine.rs | 150 +++++++ .../src/hosted/memory/select.rs | 13 +- .../redirect/lock_index_equivalence_tests.rs | 2 +- .../src/patch/redirect/mod.rs | 76 +++- .../socket-patch-core/src/vendor/npm_lock.rs | 94 ++++- .../src/vendor/npm_origin.rs | 377 +++++++++++++++++- .../socket-patch-core/src/vex/discover/mod.rs | 7 + .../socket-patch-core/src/vex/discover/npm.rs | 61 ++- 9 files changed, 755 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b03e92f8d..98e978f16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -116,7 +116,10 @@ limits, and required install commands. `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). + lock (#326). A dependency the project's `overrides` send back to a + registry version is not one of these: npm installs the override's + registry release, so hosted and vendored modes patch it again, and + `vex` attests it (#490). - **Agent mode finds Poetry's virtualenv in more setups.** Three cases missed the virtualenv Poetry installed into. Each fell back to the wrong interpreter, skipped the patch as `package_not_installed` and diff --git a/crates/socket-patch-core/src/hosted/engine.rs b/crates/socket-patch-core/src/hosted/engine.rs index a4351b939..0b8ff09bc 100644 --- a/crates/socket-patch-core/src/hosted/engine.rs +++ b/crates/socket-patch-core/src/hosted/engine.rs @@ -424,6 +424,35 @@ pub async fn read_candidate_files( } } + // The root manifest's `overrides` decide which git / url / `file:` + // dependent specs npm really installs from (#490). Only the npm lock + // rewriter reads it, as advisory input: no rewriter edits it, so a link + // or an unreadable in-memory entry is left out (the rewriter then keeps + // its conservative reading) rather than refused. + if candidates.iter().any(|c| c.dep.ecosystem == "npm") + && NPM_LOCKS.iter().any(|lock| out.files.contains_key(*lock)) + { + let rel = crate::hosted::memory::select::NPM_MANIFEST_REL; + let text = match view { + ProjectView::Disk(_) | ProjectView::Snapshot(_) => view.read_text(rel).await.ok(), + ProjectView::Memory(project) + if !project.is_symlink(rel) && !unreadable.contains(rel) => + { + match project.get(rel) { + Some(MemoryEntry::Text(text)) => Some(text.to_string()), + Some(MemoryEntry::Binary(bytes)) => { + std::str::from_utf8(bytes).ok().map(str::to_string) + } + _ => None, + } + } + ProjectView::Memory(_) => None, + }; + if let Some(text) = text { + out.files.insert(rel.to_string(), text); + } + } + // Cargo workspace members (and in-root path dependencies) declare // dependencies of their own: a member's direct `cfg-if = "1"` must be // pinned alongside the root's, or the redirected lock entry is @@ -1643,6 +1672,127 @@ mod tests { assert!(read.unreadable_reads.is_empty()); } + fn left_pad_candidate() -> Candidate { + use crate::patch::redirect::Integrity; + Candidate { + purl: "pkg:npm/left-pad@1.3.0".into(), + dep: DepOverride { + ecosystem: "npm".into(), + name: "left-pad".into(), + namespace: None, + version: "1.3.0".into(), + token: "tok".into(), + patch_uuid: "uuid".into(), + artifact_url: "https://patch.test/left-pad-1.3.0.tgz".into(), + registry_override: None, + integrity: Integrity { + sha512: Some("sha512-PATCHED==".into()), + ..Default::default() + }, + }, + } + } + + /// The #490 lock: `pkga` depends on left-pad from git. + const OVERRIDDEN_GIT_LOCK: &str = r#"{ + "name": "app", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { "name": "app", "dependencies": { "pkga": "file:pkga-1.0.0.tgz" } }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz", + "integrity": "sha512-UPSTREAM==" + }, + "node_modules/pkga": { + "version": "1.0.0", + "resolved": "file:pkga-1.0.0.tgz", + "dependencies": { "left-pad": "github:stevemao/left-pad#v1.3.0" } + } + } +} +"#; + const OVERRIDING_MANIFEST: &str = r#"{"name":"app","dependencies":{"pkga":"file:pkga-1.0.0.tgz"},"overrides":{"left-pad":"1.3.0"}}"#; + + async fn npm_rewrite( + view: &ProjectView<'_>, + unreadable: &BTreeSet, + ) -> (CandidateFiles, Rewritten) { + let outer = OuterAllowRemote::default; + let options = RewriteOptions { + dry_run: false, + targets_pipenv_lock: false, + pipenv_major: None, + pipenv_unknown_detail: String::new(), + trust_lockfile_config: true, + npm_allow_remote_config: true, + npm_outer: &outer, + blocking: false, + }; + let candidates = vec![left_pad_candidate()]; + let read = read_candidate_files(view, unreadable, &candidates).await; + let done = rewrite( + view, + read.clone(), + &candidates, + BTreeMap::new(), + &BTreeSet::new(), + &[], + options, + ) + .await; + (read, done) + } + + #[tokio::test] + async fn issue_490_the_root_manifest_overrides_reach_the_npm_rewriter() { + let redirected = |done: &Rewritten| { + done.rewrite + .files + .get("package-lock.json") + .is_some_and(|lock| lock.contains("https://patch.test/left-pad-1.3.0.tgz")) + }; + // In memory. + let mut p = MemoryProject::new(); + p.insert_text("package-lock.json", OVERRIDDEN_GIT_LOCK); + p.insert_text("package.json", OVERRIDING_MANIFEST); + let (read, done) = npm_rewrite(&ProjectView::Memory(&p), &BTreeSet::new()).await; + assert!(read.files.contains_key("package.json")); + assert!(redirected(&done), "{:?}", done.rewrite.warnings); + assert!(!done.rewrite.files.contains_key("package.json")); + + // A linked or unreadable manifest is left out, not refused: the + // rewriter keeps the conservative #326 skip. + for linked in [true, false] { + let mut p = MemoryProject::new(); + p.insert_text("package-lock.json", OVERRIDDEN_GIT_LOCK); + let mut unreadable = BTreeSet::new(); + if linked { + p.insert("package.json", MemoryEntry::Symlink); + } else { + p.insert_present("package.json"); + unreadable.insert("package.json".to_string()); + } + let (read, done) = npm_rewrite(&ProjectView::Memory(&p), &unreadable).await; + assert!(!read.files.contains_key("package.json")); + assert!(read.symlinked_reads.is_empty() && read.unreadable_reads.is_empty()); + assert!(!redirected(&done)); + assert!(done + .rewrite + .warnings + .iter() + .any(|w| w.code == "redirect_npm_non_registry_entry_skipped")); + } + + // On disk. + let tmp = tempfile::tempdir().unwrap(); + std::fs::write(tmp.path().join("package-lock.json"), OVERRIDDEN_GIT_LOCK).unwrap(); + std::fs::write(tmp.path().join("package.json"), OVERRIDING_MANIFEST).unwrap(); + let (_, done) = npm_rewrite(&ProjectView::Disk(tmp.path()), &BTreeSet::new()).await; + assert!(redirected(&done), "{:?}", done.rewrite.warnings); + } + fn gem_candidate() -> Candidate { use crate::patch::redirect::{Integrity, RegistryOverride, RegistryOverrideIdentifiers}; Candidate { diff --git a/crates/socket-patch-core/src/hosted/memory/select.rs b/crates/socket-patch-core/src/hosted/memory/select.rs index fb4dfc832..f18efa81e 100644 --- a/crates/socket-patch-core/src/hosted/memory/select.rs +++ b/crates/socket-patch-core/src/hosted/memory/select.rs @@ -45,8 +45,18 @@ pub(crate) const VENDOR_STATE_REL: &str = ".socket/vendor/state.json"; /// classifies against (a package it records is not NEW). pub(crate) const MANIFEST_REL: &str = ".socket/manifest.json"; +/// The root manifest: its `overrides` decide which npm lock entries are +/// registry installs (#490). Read as advisory input, never edited. +pub(crate) const NPM_MANIFEST_REL: &str = "package.json"; + /// Root-relative text files read beyond `REDIRECT_CANDIDATE_FILES`. -const EXTRA_TEXT_FILES: [&str; 4] = [PNPM_WORKSPACE_REL, NPMRC_REL, VENDOR_STATE_REL, MANIFEST_REL]; +const EXTRA_TEXT_FILES: [&str; 5] = [ + PNPM_WORKSPACE_REL, + NPMRC_REL, + VENDOR_STATE_REL, + MANIFEST_REL, + NPM_MANIFEST_REL, +]; /// The one directory name the disk Cargo member walk never enters (it /// follows `members`, `exclude`, path dependencies and `[patch]` paths @@ -482,6 +492,7 @@ mod tests { vec![ ".npmrc", "package-lock.json", + "package.json", "tool.py", "tool.py.lock", "web/.yarnrc.yml", 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 7e0a91647..a573ac813 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 @@ -163,7 +163,7 @@ fn indexed_npm_lock_rewrite_matches_golden() { let refs: Vec<&DepOverride> = deps.iter().collect(); for lockfile in ["package-lock.json", "npm-shrinkwrap.json"] { let mut got = RewriteResult::default(); - rewrite_one_npm_lock(&text, lockfile, &refs, &mut got); + rewrite_one_npm_lock(&text, lockfile, &refs, &NpmOverrides::default(), &mut got); record(&(&text, lockfile, &deps), &got); edits += got.edits.len(); codes.extend(got.warnings.iter().map(|w| w.code.clone())); diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 3fdda3257..c9332b389 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -26,7 +26,7 @@ use serde_json::{json, Value}; use crate::utils::digest::is_hex64_lower; use crate::utils::line_endings::{to_lf, LineEndings}; use crate::vendor::common::{parse_json_text, JsonLayout}; -use crate::vendor::npm_origin::{legacy_packages_key, npm_non_registry_entries}; +use crate::vendor::npm_origin::{legacy_packages_key, npm_non_registry_entries, NpmOverrides}; use crate::vendor::yarn_berry_lock::yarnrc_compression_level; mod bun_binary; @@ -819,8 +819,21 @@ fn rewrite_npm_lock( } return; } + // The root manifest's `overrides` decide which git / url / `file:` + // dependent specs npm actually installs from (#490); the engine reads + // it as advisory input only. + let manifest_overrides = files + .get("package.json") + .map(|text| NpmOverrides::from_manifest_text(text)) + .unwrap_or_default(); for lockfile in present { - rewrite_one_npm_lock(&files[lockfile], lockfile, &npm, result); + rewrite_one_npm_lock( + &files[lockfile], + lockfile, + &npm, + &manifest_overrides, + result, + ); } } @@ -831,6 +844,7 @@ fn rewrite_one_npm_lock( content: &str, lockfile: &str, npm: &[&DepOverride], + manifest_overrides: &NpmOverrides, result: &mut RewriteResult, ) { // npm reads past a leading UTF-8 BOM; so do we. @@ -879,7 +893,7 @@ fn rewrite_one_npm_lock( .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 non_registry = npm_non_registry_entries(&lock, manifest_overrides); let mut changed = false; for dep in npm { let fname = full_name(dep); @@ -12270,6 +12284,62 @@ mod tests { /// 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 issue_490_a_git_edge_overridden_to_the_registry_is_redirected() { + // `pkga` depends on left-pad from git; the project's `overrides` + // send it to the registry release, which is what npm installs. + let lock = json!({ + "name": "app", + "lockfileVersion": 3, + "packages": { + "": { "name": "app", "version": "0.0.0", "dependencies": { "pkga": "file:pkga-1.0.0.tgz" } }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz", + "integrity": "sha512-UPSTREAM==" + }, + "node_modules/pkga": { + "version": "1.0.0", + "resolved": "file:pkga-1.0.0.tgz", + "dependencies": { "left-pad": "github:stevemao/left-pad#v1.3.0" } + } + } + }); + let overrides = vec![npm_override( + "left-pad", + "1.3.0", + "http://patch.test/lp.tgz", + "sha512-PATCHED==", + )]; + let mut files = BTreeMap::new(); + files.insert( + "package-lock.json".to_string(), + serde_json::to_string_pretty(&lock).unwrap(), + ); + // Without the manifest's override the #326 skip still applies. + let r = rewrite_registry_redirect(&files, &overrides); + assert!(r.files.is_empty(), "{:?}", r.edits); + assert!(warning_codes(&r).contains(&"redirect_npm_non_registry_entry_skipped")); + + files.insert( + "package.json".to_string(), + r#"{"name":"app","dependencies":{"pkga":"file:pkga-1.0.0.tgz"},"overrides":{"left-pad":"1.3.0"}}"# + .to_string(), + ); + let r = rewrite_registry_redirect(&files, &overrides); + assert!( + !warning_codes(&r).contains(&"redirect_npm_non_registry_entry_skipped"), + "{:?}", + r.warnings + ); + let rewritten: Value = serde_json::from_str(&r.files["package-lock.json"]).unwrap(); + let entry = &rewritten["packages"]["node_modules/left-pad"]; + assert_eq!(entry["resolved"], "http://patch.test/lp.tgz"); + assert_eq!(entry["integrity"], "sha512-PATCHED=="); + // The manifest is input only: never written. + assert!(!r.files.contains_key("package.json")); + } + #[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"; diff --git a/crates/socket-patch-core/src/vendor/npm_lock.rs b/crates/socket-patch-core/src/vendor/npm_lock.rs index c96322bf6..b1911ba90 100644 --- a/crates/socket-patch-core/src/vendor/npm_lock.rs +++ b/crates/socket-patch-core/src/vendor/npm_lock.rs @@ -29,7 +29,7 @@ use super::common::{already_patched_result, done, parse_json_manifest, refused, use super::npm_common::{ done_failure_unstage, guard_coordinates, guard_revert_uuid_dir, stage_patch_pack, }; -use super::npm_origin::{legacy_packages_key, npm_non_registry_entries}; +use super::npm_origin::{legacy_packages_key, npm_non_registry_entries, NpmOverrides}; use super::parse_memo::ParseMemo; use super::path::parse_vendor_path; use super::source::PackageSource; @@ -147,11 +147,16 @@ pub async fn vendor_npm<'a>( Err(outcome) => return *outcome, }; + // The root manifest's `overrides` (#490): which git / url / `file:` + // dependent specs npm really installs from. + let overrides = NpmOverrides::read(project_root).await; + // ── 3. Find the rewritable lock instances ─────────────────────────── - let matches = match rewritable_matches(&lock, name, version, &lock_name, &mut warnings) { - Ok(matches) => matches, - Err(outcome) => return *outcome, - }; + let matches = + match rewritable_matches(&lock, &overrides, name, version, &lock_name, &mut warnings) { + Ok(matches) => matches, + Err(outcome) => return *outcome, + }; // ── 3b. Sibling lock (npm 12) ─────────────────────────────────────── // npm 12 removed `npm shrinkwrap`, auto-creates a package-lock.json @@ -164,7 +169,14 @@ pub async fn vendor_npm<'a>( // rewriter's rule), and one that cannot be is SAID. let mut siblings: Vec = Vec::new(); for (sib_name, sib_bytes) in sibling_locks { - match sibling_lock_target(sib_name, sib_bytes, name, version, &mut warnings) { + match sibling_lock_target( + sib_name, + sib_bytes, + &overrides, + name, + version, + &mut warnings, + ) { Ok(sib) => siblings.push(sib), Err(why) => warnings.push(VendorWarning::new( "vendor_npm_sibling_lock_unwired", @@ -231,6 +243,7 @@ pub async fn vendor_npm<'a>( resolved: &resolved, integrity: &packed.integrity, staged_pkg_json: staged_pkg_json.as_ref(), + overrides: &overrides, }; if let Err(e) = rewire.apply( &mut lock, @@ -430,12 +443,13 @@ fn lock_version_gate(lock: &Value, lock_name: &str) -> Result, Box, ) -> Result, Box> { - let matches = match scan_lock_matches(lock, name, version, warnings) { + let matches = match scan_lock_matches(lock, overrides, name, version, warnings) { LockScan::Matches(m) => m, LockScan::WorkspaceMember { key } => { // A matching key outside node_modules/ is the user's own @@ -500,6 +514,7 @@ fn rewritable_matches( pub(super) struct NpmLockProject { lock_name: String, lock: std::sync::Arc, + overrides: NpmOverrides, } /// Read the lock as [`vendor_npm`]'s step 2 does — selected, parsed, @@ -515,7 +530,12 @@ pub(super) async fn read_project(project_root: &Path) -> Result, @@ -840,7 +862,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); + let non_registry = npm_non_registry_entries(lock, overrides); for (key, entry) in packages { // The root "" entry is the project itself, never a dependency. if key.is_empty() { @@ -1203,6 +1225,7 @@ struct SiblingLock { fn sibling_lock_target( sib_name: &str, sib_bytes: std::io::Result>, + overrides: &NpmOverrides, name: &str, version: &str, warnings: &mut Vec, @@ -1220,7 +1243,7 @@ fn sibling_lock_target( "lockfileVersion {lock_version:?}; only v2/v3 locks are supported" )); } - match scan_lock_matches(&lock, name, version, warnings) { + match scan_lock_matches(&lock, overrides, name, version, warnings) { LockScan::Matches(matches) if !matches.is_empty() => Ok(SiblingLock { name: sib_name.to_string(), bytes, @@ -1242,6 +1265,7 @@ struct LockRewire<'a> { resolved: &'a str, integrity: &'a str, staged_pkg_json: Option<&'a Value>, + overrides: &'a NpmOverrides, } impl LockRewire<'_> { @@ -1258,7 +1282,7 @@ impl LockRewire<'_> { warnings: &mut Vec, ) -> Result<(), String> { // Taken before any rewrite, for the legacy mirror below. - let non_registry = npm_non_registry_entries(lock); + let non_registry = npm_non_registry_entries(lock, self.overrides); let Some(packages) = lock.get_mut("packages").and_then(Value::as_object_mut) else { return Err("lock `packages` object vanished mid-rewrite".to_string()); }; @@ -1692,7 +1716,7 @@ mod tests { let mut warnings = Vec::new(); assert!( matches!( - scan_lock_matches(&lock, "left-pad", "1.3.0", &mut warnings), + scan_lock_matches(&lock, &NpmOverrides::default(), "left-pad", "1.3.0", &mut warnings), LockScan::Matches(m) if m.is_empty() ), "defensive scan of {lock} must yield no matches" @@ -2068,6 +2092,52 @@ mod tests { /// 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 issue_490_a_git_edge_overridden_to_the_registry_is_vendored() { + // `pkga` depends on left-pad from git; the project's `overrides` + // send it to the registry release, which is what npm installs. + let lock = json!({ + "name": "fixture", + "version": "1.0.0", + "lockfileVersion": 3, + "packages": { + "": { "name": "fixture", "version": "1.0.0", + "dependencies": { "pkga": "file:pkga-1.0.0.tgz" } }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz", + "integrity": "sha512-orig==" + }, + "node_modules/pkga": { + "version": "1.0.0", + "resolved": "file:pkga-1.0.0.tgz", + "dependencies": { "left-pad": "github:stevemao/left-pad#v1.3.0" } + } + } + }); + let fx = fixture_with("left-pad", "1.3.0", lock).await; + // Without the override the #326 refusal still applies. + expect_refused(fx.vendor(true).await, "vendor_lock_entry_not_rewritable"); + tokio::fs::write( + fx.root().join("package.json"), + r#"{"name":"fixture","version":"1.0.0","dependencies":{"pkga":"file:pkga-1.0.0.tgz"},"overrides":{"left-pad":"1.3.0"}}"#, + ) + .await + .unwrap(); + let (result, entry, _warnings) = expect_done(fx.vendor(false).await); + assert!(result.success, "{:?}", result.error); + assert!(entry.is_some()); + let rewritten: Value = + serde_json::from_slice(&tokio::fs::read(fx.lock_path()).await.unwrap()).unwrap(); + let resolved = rewritten["packages"]["node_modules/left-pad"]["resolved"] + .as_str() + .unwrap(); + assert!( + resolved.starts_with("file:.socket/vendor/npm/"), + "{resolved}" + ); + } + #[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"; diff --git a/crates/socket-patch-core/src/vendor/npm_origin.rs b/crates/socket-patch-core/src/vendor/npm_origin.rs index 14ef34f72..82273da13 100644 --- a/crates/socket-patch-core/src/vendor/npm_origin.rs +++ b/crates/socket-patch-core/src/vendor/npm_origin.rs @@ -23,13 +23,198 @@ //! * 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. +//! +//! An inbound spec the project's `overrides` replace with a registry spec +//! does not count (#490): npm installs the override, and the lock records +//! the registry tarball while the dependent's entry keeps its own raw +//! spec. npm doesn't record `overrides` in the lock, so they come from the +//! root `package.json` ([`NpmOverrides`]). Only overrides that clearly apply +//! to the edge clear it (see [`NpmOverrides::replacement`]). In any unclear +//! case the edge keeps counting, which leaves the copy unpatched and +//! reported rather than wrongly attested. use std::collections::BTreeMap; -use serde_json::Value; +use serde_json::{Map, Value}; use crate::constants::SOCKET_DIR; +/// The root `package.json` fields that decide what npm installs for an +/// overridden edge: its `overrides` and its own dependency specs (for the +/// `$name` references an override value may use). Empty (no override +/// applies) when the manifest is absent or unparseable. +#[derive(Debug, Clone, Default)] +pub(crate) struct NpmOverrides { + rules: Map, + root_deps: Map, +} + +impl NpmOverrides { + /// From the parsed root `package.json`. + pub(crate) fn from_manifest(manifest: &Value) -> Self { + let rules = manifest + .get("overrides") + .and_then(Value::as_object) + .cloned() + .unwrap_or_default(); + let mut root_deps = Map::new(); + // npm resolves `$name` against the root's direct dependencies. + for field in EDGE_FIELDS { + if let Some(deps) = manifest.get(field).and_then(Value::as_object) { + for (name, spec) in deps { + root_deps + .entry(name.clone()) + .or_insert_with(|| spec.clone()); + } + } + } + NpmOverrides { rules, root_deps } + } + + /// From the root `package.json` text (a leading BOM is skipped, as npm + /// does); empty when it isn't valid JSON. + pub(crate) fn from_manifest_text(text: &str) -> Self { + super::common::parse_json_text(text) + .map(|manifest| Self::from_manifest(&manifest)) + .unwrap_or_default() + } + + /// From `/package.json`; empty when it can't be read. + pub(crate) async fn read(project_root: &std::path::Path) -> Self { + match crate::utils::fs::read_regular_to_bytes(&project_root.join("package.json")).await { + Ok(bytes) => super::common::parse_json_manifest(&bytes) + .map(|manifest| Self::from_manifest(&manifest)) + .unwrap_or_default(), + Err(_) => Self::default(), + } + } + + /// The spec an override makes npm install for the edge `dep_name@spec` + /// whose dependent sits at the lock key `from`, or `None` when no + /// override clearly applies. + /// + /// A rule applies when its key names `dep_name` with no selector (or + /// with `spec` itself as the selector), and every enclosing rule names + /// the dependent or one of its physical ancestors (outermost first), + /// with no selector or that package's exact version. The innermost + /// applicable rule wins, as in npm. Version-range selectors are not + /// evaluated, so a rule that uses one never applies here. + fn replacement( + &self, + packages: &Map, + from: &str, + dep_name: &str, + spec: &str, + ) -> Option { + if self.rules.is_empty() { + return None; + } + let chain = dependent_chain(packages, from); + let mut best: Option<(usize, String)> = None; + self.visit(&self.rules, &chain, 0, 0, dep_name, spec, &mut best); + best.map(|(_, value)| value) + } + + /// Search `rules` (nested `depth` levels deep; the enclosing rules + /// matched `chain[..from_ix]`) for the deepest rule for the edge. + #[allow(clippy::too_many_arguments)] + fn visit( + &self, + rules: &Map, + chain: &[(String, Option)], + from_ix: usize, + depth: usize, + dep_name: &str, + spec: &str, + best: &mut Option<(usize, String)>, + ) { + for (key, value) in rules { + if key == "." { + continue; + } + let (name, selector) = split_selector(key); + // A rule for the edge's own package. + if name == dep_name && selector.is_none_or(|sel| sel == spec) { + if let Some(replacement) = self.rule_value(value) { + if best.as_ref().is_none_or(|(d, _)| depth >= *d) { + *best = Some((depth, replacement)); + } + } + } + // A rule scoped to a package on the dependent's chain. + if let Some(children) = value.as_object() { + for (ix, (anc_name, anc_version)) in chain.iter().enumerate().skip(from_ix) { + let version_ok = match selector { + None => true, + Some(sel) => anc_version.as_deref() == Some(sel), + }; + if anc_name == name && version_ok { + self.visit(children, chain, ix + 1, depth + 1, dep_name, spec, best); + break; + } + } + } + } + } + + /// A rule's replacement spec: the string itself, or an object's `"."`, + /// with a `$name` reference resolved against the root's dependencies. + fn rule_value(&self, value: &Value) -> Option { + let raw = match value { + Value::String(s) => s.as_str(), + Value::Object(obj) => obj.get(".")?.as_str()?, + _ => return None, + }; + match raw.strip_prefix('$') { + Some(reference) => self.root_deps.get(reference)?.as_str().map(str::to_string), + None => Some(raw.to_string()), + } + } +} + +/// `name` or `name@selector` (a scoped `@scope/name` keeps its leading `@`). +fn split_selector(key: &str) -> (&str, Option<&str>) { + let (scope_at, rest) = match key.strip_prefix('@') { + Some(rest) => (1, rest), + None => (0, key), + }; + match rest.split_once('@') { + Some((name, selector)) => (&key[..scope_at + name.len()], Some(selector)), + None => (key, None), + } +} + +/// The packages on the path from the project root to the lock key `from`, +/// outermost first, as `(name, version)`: each `node_modules/` +/// segment's entry (its `name` field for an alias, else the segment). The +/// root and workspace members contribute nothing. +fn dependent_chain(packages: &Map, from: &str) -> Vec<(String, Option)> { + let mut chain = Vec::new(); + let mut rest = from; + let mut prefix = String::new(); + while let Some(ix) = rest.find("node_modules/") { + let after = &rest[ix + "node_modules/".len()..]; + // A package segment runs to the next nested `node_modules/`. + let seg_len = after.find("/node_modules/").unwrap_or(after.len()); + let consumed = ix + "node_modules/".len() + seg_len; + prefix.push_str(&rest[..consumed]); + let segment = &after[..seg_len]; + let entry = packages.get(&prefix); + let name = entry + .and_then(|e| e.get("name")) + .and_then(Value::as_str) + .unwrap_or(segment) + .to_string(); + let version = entry + .and_then(|e| e.get("version")) + .and_then(Value::as_str) + .map(str::to_string); + chain.push((name, version)); + rest = &rest[consumed..]; + } + chain +} + /// The dependency maps whose specs npm resolves against `packages` entries. const EDGE_FIELDS: [&str; 4] = [ "dependencies", @@ -53,7 +238,10 @@ pub(crate) fn legacy_packages_key(parent: &str, name: &str) -> String { /// 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 { +pub(crate) fn npm_non_registry_entries( + lock: &Value, + overrides: &NpmOverrides, +) -> BTreeMap { let mut out = BTreeMap::new(); let Some(packages) = lock.get("packages").and_then(Value::as_object) else { return out; @@ -80,6 +268,14 @@ pub(crate) fn npm_non_registry_entries(lock: &Value) -> BTreeMap if npm_spec_is_registry(spec) { continue; } + // An override that swaps the spec for a registry one makes + // npm install the registry release instead (#490). + if overrides + .replacement(packages, from, dep_name, spec) + .is_some_and(|replacement| npm_spec_is_registry(&replacement)) + { + continue; + } let Some(target) = resolve_edge(packages, from, dep_name) else { continue; }; @@ -244,7 +440,7 @@ mod tests { "resolved": "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba" } })); - let found = npm_non_registry_entries(&lock); + let found = npm_non_registry_entries(&lock, &NpmOverrides::default()); assert!(found.contains_key("node_modules/left-pad"), "{found:?}"); } @@ -260,7 +456,8 @@ mod tests { "": { "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")); + assert!(npm_non_registry_entries(&lock, &NpmOverrides::default()) + .contains_key("node_modules/left-pad")); } } @@ -273,7 +470,8 @@ mod tests { "": { "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")); + assert!(npm_non_registry_entries(&lock, &NpmOverrides::default()) + .contains_key("node_modules/left-pad")); } #[test] @@ -285,7 +483,8 @@ mod tests { "resolved": "file:../left-pad-1.3.0.tgz" } })); - assert!(npm_non_registry_entries(&lock).contains_key("node_modules/left-pad")); + assert!(npm_non_registry_entries(&lock, &NpmOverrides::default()) + .contains_key("node_modules/left-pad")); } #[test] @@ -308,7 +507,7 @@ mod tests { "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz" } })); - let found = npm_non_registry_entries(&lock); + let found = npm_non_registry_entries(&lock, &NpmOverrides::default()); 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")); @@ -323,7 +522,8 @@ mod tests { "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")); + assert!(npm_non_registry_entries(&lock, &NpmOverrides::default()) + .contains_key("node_modules/left-pad")); } #[test] @@ -340,6 +540,165 @@ mod tests { "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz" } })); - assert!(npm_non_registry_entries(&lock).is_empty()); + assert!(npm_non_registry_entries(&lock, &NpmOverrides::default()).is_empty()); + } + + /// The #490 lock: `pkga` depends on left-pad from git, the project + /// overrides it, and npm installs the registry release. + fn overridden_git_lock(left_pad_resolved: &str) -> Value { + lock(json!({ + "": { "dependencies": { "pkga": "file:pkga-1.0.0.tgz" } }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": left_pad_resolved, + "integrity": "sha512-XI5MPzVNApjAyhQzphX8BkmKsKUxD4LdyK24iZeQGinBN9yTQT3bFlCBy/aVx2HrNcqQGsdot8ghrjyrvMCoEA==" + }, + "node_modules/pkga": { + "version": "1.0.0", + "resolved": "file:pkga-1.0.0.tgz", + "dependencies": { "left-pad": "github:stevemao/left-pad#v1.3.0" } + } + })) + } + + fn manifest(overrides: Value) -> NpmOverrides { + NpmOverrides::from_manifest(&json!({ + "name": "app", + "dependencies": { "pkga": "file:pkga-1.0.0.tgz", "left-pad": "1.3.0" }, + "overrides": overrides + })) + } + + const REGISTRY_TGZ: &str = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz"; + + #[test] + fn issue_490_a_registry_override_of_a_git_edge_is_a_registry_install() { + // Fresh lock, and the lock after a hosted redirect. + for resolved in [ + REGISTRY_TGZ, + "https://patch.socket.dev/npm/left-pad/-/left-pad-1.3.0.tgz", + ] { + let lock = overridden_git_lock(resolved); + for overrides in [ + json!({ "left-pad": "1.3.0" }), + json!({ "left-pad": "^1.3.0" }), + json!({ "left-pad": { ".": "1.3.0" } }), + json!({ "left-pad": "$left-pad" }), + json!({ "pkga": { "left-pad": "1.3.0" } }), + json!({ "pkga@1.0.0": { "left-pad": "1.3.0" } }), + json!({ "left-pad@github:stevemao/left-pad#v1.3.0": "1.3.0" }), + ] { + let found = npm_non_registry_entries(&lock, &manifest(overrides.clone())); + assert!( + !found.contains_key("node_modules/left-pad"), + "{overrides} / {resolved}: {found:?}" + ); + // The dependent itself is still a `file:` install. + assert!(found.contains_key("node_modules/pkga"), "{found:?}"); + } + } + } + + #[test] + fn issue_490_overrides_that_do_not_clearly_apply_keep_the_edge() { + let lock = overridden_git_lock(REGISTRY_TGZ); + for overrides in [ + // No override at all (the #326 case). + json!({}), + // Scoped under a package that isn't on the dependent's chain. + json!({ "other": { "left-pad": "1.3.0" } }), + // A parent selector for another version of the dependent. + json!({ "pkga@2.0.0": { "left-pad": "1.3.0" } }), + // A selector naming a different spec. + json!({ "left-pad@1.2.0": "1.3.0" }), + // An override to another non-registry source. + json!({ "left-pad": "github:someone/left-pad" }), + json!({ "left-pad": "file:../left-pad" }), + // A `$` reference to a dependency the root doesn't declare. + json!({ "left-pad": "$missing" }), + // An object rule without its own `"."` spec. + json!({ "left-pad": { "other": "1.0.0" } }), + // A different package's override. + json!({ "right-pad": "1.0.0" }), + ] { + let found = npm_non_registry_entries(&lock, &manifest(overrides.clone())); + assert!( + found.contains_key("node_modules/left-pad"), + "{overrides}: {found:?}" + ); + } + } + + #[test] + fn issue_490_the_innermost_override_wins() { + let lock = overridden_git_lock(REGISTRY_TGZ); + // Top level says registry, but the rule scoped to pkga says git. + let found = npm_non_registry_entries( + &lock, + &manifest(json!({ + "left-pad": "1.3.0", + "pkga": { "left-pad": "github:someone/left-pad" } + })), + ); + assert!(found.contains_key("node_modules/left-pad"), "{found:?}"); + // And the other way round. + let found = npm_non_registry_entries( + &lock, + &manifest(json!({ + "left-pad": "github:someone/left-pad", + "pkga": { "left-pad": "1.3.0" } + })), + ); + assert!(!found.contains_key("node_modules/left-pad"), "{found:?}"); + } + + #[test] + fn issue_490_a_nested_dependent_matches_its_physical_ancestors() { + // `@scope/b` under `a` depends on left-pad from git; the override is + // scoped to `a`, two levels up. + let lock = lock(json!({ + "": { "dependencies": { "a": "^1.0.0" } }, + "node_modules/a": { "version": "1.0.0", "resolved": REGISTRY_TGZ }, + "node_modules/a/node_modules/@scope/b": { + "version": "2.0.0", + "resolved": REGISTRY_TGZ, + "dependencies": { "left-pad": "github:stevemao/left-pad#v1.3.0" } + }, + "node_modules/a/node_modules/left-pad": { + "version": "1.3.0", + "resolved": REGISTRY_TGZ + } + })); + let scoped = |overrides: Value| { + npm_non_registry_entries( + &lock, + &NpmOverrides::from_manifest(&json!({ "overrides": overrides })), + ) + }; + let key = "node_modules/a/node_modules/left-pad"; + assert!(!scoped(json!({ "a": { "left-pad": "1.3.0" } })).contains_key(key)); + assert!(!scoped(json!({ "a": { "@scope/b": { "left-pad": "1.3.0" } } })).contains_key(key)); + assert!(!scoped(json!({ "@scope/b@2.0.0": { "left-pad": "1.3.0" } })).contains_key(key)); + // Out of order: `@scope/b` isn't an ancestor of `a`. + assert!(scoped(json!({ "@scope/b": { "a": { "left-pad": "1.3.0" } } })).contains_key(key)); + } + + #[test] + fn overrides_parse_from_manifest_text() { + let lock = overridden_git_lock(REGISTRY_TGZ); + let text = "\u{feff}{\"overrides\":{\"left-pad\":\"1.3.0\"}}"; + let found = npm_non_registry_entries(&lock, &NpmOverrides::from_manifest_text(text)); + assert!(!found.contains_key("node_modules/left-pad"), "{found:?}"); + // Unparseable: no overrides. + let found = npm_non_registry_entries(&lock, &NpmOverrides::from_manifest_text("{")); + assert!(found.contains_key("node_modules/left-pad"), "{found:?}"); + } + + #[test] + fn selectors_split_scoped_names() { + assert_eq!(split_selector("left-pad"), ("left-pad", None)); + assert_eq!(split_selector("left-pad@1"), ("left-pad", Some("1"))); + assert_eq!(split_selector("@s/pad"), ("@s/pad", None)); + assert_eq!(split_selector("@s/pad@^2"), ("@s/pad", Some("^2"))); } } diff --git a/crates/socket-patch-core/src/vex/discover/mod.rs b/crates/socket-patch-core/src/vex/discover/mod.rs index d571a3b6e..2d39e200a 100644 --- a/crates/socket-patch-core/src/vex/discover/mod.rs +++ b/crates/socket-patch-core/src/vex/discover/mod.rs @@ -873,6 +873,13 @@ impl<'a> DiscoverCtx<'a> { } } + /// `rel`'s text when it can be read, with no diagnostic and no + /// recognition: for advisory inputs that never carry wiring (the root + /// `package.json`'s npm `overrides`). + pub(crate) async fn read_advisory_text(&self, rel: &str) -> Option { + self.view.read_text(rel).await.ok() + } + /// Bytes twin of [`DiscoverCtx::read_text`] (JSON and binary locks). A /// binary lock is swept through its lossy UTF-8 view: string pools store /// resolutions verbatim, and a stale string an older patch generation diff --git a/crates/socket-patch-core/src/vex/discover/npm.rs b/crates/socket-patch-core/src/vex/discover/npm.rs index 7d151db2e..920932076 100644 --- a/crates/socket-patch-core/src/vex/discover/npm.rs +++ b/crates/socket-patch-core/src/vex/discover/npm.rs @@ -58,7 +58,7 @@ use crate::vendor::lock_inventory::pnpm::rush_lock_rels; use crate::vendor::lock_inventory::{ npm_lock_bundled_nodes, npm_lock_nodes, LockIntegrity, NpmLockNode, }; -use crate::vendor::npm_origin::npm_non_registry_entries; +use crate::vendor::npm_origin::{npm_non_registry_entries, NpmOverrides}; pub(crate) async fn extract(ctx: &DiscoverCtx<'_>, out: &mut Discovery) { let mut locks: Vec = Vec::new(); @@ -189,7 +189,14 @@ async fn extract_package_lock( read.bundled.entry(purl).or_insert(location); } } - drop_non_registry_installs(file, &doc, &mut read, out); + // The root manifest's `overrides` (#490): which git / url / `file:` + // dependent specs npm really installs from. + let overrides = ctx + .read_advisory_text("package.json") + .await + .map(|text| NpmOverrides::from_manifest_text(&text)) + .unwrap_or_default(); + drop_non_registry_installs(file, &doc, &overrides, &mut read, out); Some(read) } @@ -202,10 +209,11 @@ async fn extract_package_lock( fn drop_non_registry_installs( file: &str, doc: &Value, + overrides: &NpmOverrides, read: &mut NpmLockRefs, out: &mut Discovery, ) { - let non_registry = npm_non_registry_entries(doc); + let non_registry = npm_non_registry_entries(doc, overrides); if non_registry.is_empty() { return; } @@ -1090,6 +1098,53 @@ mod tests { assert_eq!(bundled_contests(&out).len(), 2, "{:#?}", 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] + async fn issue_490_a_git_edge_overridden_to_the_registry_is_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"); + for (wiring, resolved) in [("hosted", &hosted), ("vendored", &vendored)] { + for (overrides, attested) in [ + (None, false), + (Some(r#"{"left-pad":"1.3.0"}"#), true), + (Some(r#"{"left-pad":"github:someone/left-pad"}"#), false), + ] { + let p = Project::new(); + p.write( + "package-lock.json", + lock_with_packages(serde_json::json!({ + "": { "name": "app", "version": "1.0.0", + "dependencies": { "pkga": "file:pkga-1.0.0.tgz" } }, + "node_modules/pkga": { + "version": "1.0.0", + "resolved": "file:pkga-1.0.0.tgz", + "dependencies": { "left-pad": "github:stevemao/left-pad#v1.3.0" } + }, + "node_modules/left-pad": { + "version": "1.3.0", "resolved": resolved, "integrity": SRI + }, + })), + ); + if let Some(overrides) = overrides { + p.write( + "package.json", + format!( + r#"{{"name":"app","dependencies":{{"pkga":"file:pkga-1.0.0.tgz"}},"overrides":{overrides}}}"# + ), + ); + } + let out = run(&p).await; + assert_eq!( + out.refs.len(), + usize::from(attested), + "{wiring} / {overrides:?}: {:#?}", + out.diagnostics + ); + } + } + } + /// #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 From 9b885bd66054f58da33e64e00d5313641961bed3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 18:58:51 +0000 Subject: [PATCH 3/9] Add real-npm e2e for overridden git deps The hosted capstone suite gains the #490 project shape: a local package depends on left-pad from git and the project's overrides pin it to the registry release. The test checks that scan redirects it, that a fresh npm ci installs the patched bytes, that vex verifies them, and that rollback restores the lock. It skips on npm older than 8.3, which has no overrides. Refs #490 Assisted-by: Claude Code:claude-opus-5-5 --- .../tests/e2e_redirect_npm_build.rs | 164 +++++++++++++++++- 1 file changed, 160 insertions(+), 4 deletions(-) diff --git a/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs b/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs index fb7a7332f..dfe7ef61c 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs @@ -150,6 +150,102 @@ struct RedirectFixture { flavor: LockFlavor, locks: Vec<&'static str>, pristine_locks: Vec<(&'static str, Vec)>, + /// Project files beyond package.json / the locks / `.socket/` that a + /// checkout carries (the #490 fixture's local `pkga` tarball). + extra_files: Vec<&'static str>, +} + +/// How the fixture project depends on [`DEP`]. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum Setup { + /// `npm install left-pad@1.3.0`: a direct registry dependency. + Direct, + /// #490: a local `pkga` depends on left-pad from git, and the project's + /// `overrides` send it back to the registry release, which is what npm + /// installs (the lock records the registry tarball; `pkga`'s entry + /// keeps its git spec). + OverriddenGitDep, +} + +/// The local tarball the [`Setup::OverriddenGitDep`] project depends on. +const PKGA_TGZ: &str = "pkga-1.0.0.tgz"; + +/// Install the [`Setup::OverriddenGitDep`] project; `false` after a skip. +fn install_overridden_git_dep(suite: &str, tmp: &Path, proj: &Path, cache: &Path) -> bool { + let pkga = tmp.join("pkga"); + std::fs::create_dir_all(&pkga).unwrap(); + std::fs::write( + pkga.join("package.json"), + format!( + r#"{{"name":"pkga","version":"1.0.0","dependencies":{{"{DEP}":"github:stevemao/left-pad#v{DEP_VERSION}"}}}}"# + ), + ) + .unwrap(); + std::fs::write( + pkga.join("index.js"), + format!("module.exports = require('{DEP}');\n"), + ) + .unwrap(); + let packed = npm_e2e_common::npm(&pkga, &["pack", "--cache", cache.to_str().unwrap()]); + assert!( + packed.status.success(), + "npm pack pkga: {}", + npm_e2e_common::output_text(&packed) + ); + std::fs::rename(pkga.join(PKGA_TGZ), proj.join(PKGA_TGZ)).unwrap(); + std::fs::write( + proj.join("package.json"), + format!( + r#"{{"name":"redirect-capstone","version":"0.0.0","private":true,"dependencies":{{"pkga":"file:{PKGA_TGZ}"}},"overrides":{{"{DEP}":"{DEP_VERSION}"}}}}"# + ), + ) + .unwrap(); + let out = npm_e2e_common::npm( + proj, + &[ + "install", + "--no-audit", + "--no-fund", + "--cache", + cache.to_str().unwrap(), + ], + ); + if !out.status.success() { + npm_e2e_common::skip( + suite, + &format!( + "`npm install` of the overrides fixture failed (registry unreachable?):\n{}", + npm_e2e_common::output_text(&out) + ), + ); + return false; + } + // The #490 premise: the registry release is installed, and the + // dependent's entry still carries its git spec. + let lock: serde_json::Value = + serde_json::from_slice(&std::fs::read(proj.join("package-lock.json")).unwrap()).unwrap(); + assert_eq!( + lock["packages"][format!("node_modules/{DEP}")]["resolved"], + format!("https://registry.npmjs.org/{DEP}/-/{DEP}-{DEP_VERSION}.tgz"), + "npm installs the override's registry release: {lock}" + ); + assert!( + lock["packages"]["node_modules/pkga"]["dependencies"][DEP] + .as_str() + .is_some_and(|spec| spec.starts_with("github:")), + "pkga keeps its git spec: {lock}" + ); + true +} + +/// Whether the npm under test honors `overrides` (added in npm 8.3). +fn npm_supports_overrides() -> bool { + let Some(version) = npm_e2e_common::npm_version() else { + return false; + }; + let mut parts = version.split('.').map(|p| p.parse::().unwrap_or(0)); + let (major, minor) = (parts.next().unwrap_or(0), parts.next().unwrap_or(0)); + (major, minor) >= (8, 3) } /// Which CLI invocation drives step 3 (the redirect itself). The scan @@ -180,6 +276,7 @@ async fn redirect_scanned_project( tamper_served_tarball: bool, cli: RedirectCli, flavor: LockFlavor, + setup: Setup, ) -> Option { let suite = format!("e2e_redirect_npm_build ({tag})"); let Some(major) = npm_e2e_common::npm_major() else { @@ -198,8 +295,28 @@ async fn redirect_scanned_project( // 1. REAL fixture: npm install (network allowed here, private cache). let cache = tmp.path().join("npm-cache"); - if !npm_e2e_common::install_fixture(&suite, &proj, &cache, &format!("{DEP}@{DEP_VERSION}")) { - return None; + let mut extra_files = Vec::new(); + match setup { + Setup::Direct => { + if !npm_e2e_common::install_fixture( + &suite, + &proj, + &cache, + &format!("{DEP}@{DEP_VERSION}"), + ) { + return None; + } + } + Setup::OverriddenGitDep => { + if !npm_supports_overrides() { + npm_e2e_common::skip(&suite, "this npm predates `overrides` (npm 8.3)"); + return None; + } + if !install_overridden_git_dep(&suite, tmp.path(), &proj, &cache) { + return None; + } + extra_files.push(PKGA_TGZ); + } } let expected_lock_version = match major { ..=6 => 1, @@ -566,9 +683,18 @@ async fn redirect_scanned_project( flavor, locks, pristine_locks, + extra_files, }) } +/// [`npm_e2e_common::fresh_checkout`] plus the fixture's extra files. +fn checkout(fx: &RedirectFixture, dst: &Path) { + npm_e2e_common::fresh_checkout(&fx.proj, dst, &fx.locks); + for extra in &fx.extra_files { + std::fs::copy(fx.proj.join(extra), dst.join(extra)).unwrap(); + } +} + /// New dir holding ONLY what a git checkout would carry — package.json, the /// committed npm lock(s), the committed `.npmrc` the hosted run wrote, /// `.socket/` — then a PLAIN `npm ci` (no `--allow-remote` flag) against an @@ -580,7 +706,7 @@ async fn redirect_scanned_project( /// twin checkout without it is refused EALLOWREMOTE and installs nothing. fn fresh_checkout_npm_ci(fx: &RedirectFixture) -> (PathBuf, Output) { let fresh = fx.tmp.path().join("fresh"); - npm_e2e_common::fresh_checkout(&fx.proj, &fresh, &fx.locks); + checkout(fx, &fresh); assert_eq!( std::fs::read_to_string(fresh.join(".npmrc")).unwrap(), "allow-remote=all\n", @@ -588,7 +714,7 @@ fn fresh_checkout_npm_ci(fx: &RedirectFixture) -> (PathBuf, Output) { ); if npm_e2e_common::needs_allow_remote(fx.major) { let bare = fx.tmp.path().join("fresh-without-npmrc"); - npm_e2e_common::fresh_checkout(&fx.proj, &bare, &fx.locks); + checkout(fx, &bare); std::fs::remove_file(bare.join(".npmrc")).unwrap(); let refused = npm_e2e_common::npm_ci(&bare, &fx.tmp.path().join("refused-npm-cache")); let text = npm_e2e_common::output_text(&refused); @@ -755,6 +881,7 @@ async fn npm_redirect_fresh_checkout_npm_ci_installs_patched_bytes_and_vex_verif false, RedirectCli::ScanRedirectVex, LockFlavor::PackageLock, + Setup::Direct, ) .await else { @@ -782,6 +909,31 @@ async fn npm_redirect_fresh_checkout_npm_ci_installs_patched_bytes_and_vex_verif manifestless_tail(&fx, &fresh, installed, &[VexVia::Apply]); } +/// #490: a git dependency the project's `overrides` send back to the +/// registry is what npm installs from the registry, so the hosted scan +/// redirects it (the shared steps assert `redirected: 1`, the lock pin and +/// the in-run VEX statement), a fresh `npm ci` installs the patched bytes, +/// the hash-verified `vex` attests it, and `rollback` restores the lock. +#[tokio::test(flavor = "multi_thread")] +#[ignore = "wall-bound real-npm install (~150s); runs on all 3 OSes as an e2e CI matrix leg"] +async fn npm_redirect_overridden_git_dependency_installs_patched_bytes() { + let Some(fx) = redirect_scanned_project( + "overrides", + false, + RedirectCli::ScanRedirectVex, + LockFlavor::PackageLock, + Setup::OverriddenGitDep, + ) + .await + else { + return; + }; + let (fresh, installed) = fresh_install_patched(&fx); + assert!(installed, "npm >= 8.3 installs the hosted pin"); + post_install_vex(&fresh, &fx.server.uri()); + rollback_removes_npmrc(&fx); +} + /// Step 5 of the capstone: the hash-verified `vex`. v5 hosted mode keeps no /// ledger, so the pin comes from the committed lock (its host named by /// `--patch-server-url`) and the record from the org-scoped patch API. @@ -843,6 +995,7 @@ async fn npm_redirect_shrinkwrap_fresh_checkout_and_manifestless_vex() { false, RedirectCli::ScanRedirectVex, LockFlavor::Shrinkwrap, + Setup::Direct, ) .await else { @@ -865,6 +1018,7 @@ async fn npm_redirect_tampered_hosted_tarball_fails_fresh_npm_ci() { true, RedirectCli::ScanRedirectVex, LockFlavor::PackageLock, + Setup::Direct, ) .await else { @@ -904,6 +1058,7 @@ async fn npm_get_uuid_hosted_fresh_checkout_npm_ci_installs_patched_bytes() { false, RedirectCli::GetUuidHosted, LockFlavor::PackageLock, + Setup::Direct, ) .await else { @@ -929,6 +1084,7 @@ async fn npm_get_ghsa_hosted_narrows_and_installs() { false, RedirectCli::GetGhsaHosted, LockFlavor::PackageLock, + Setup::Direct, ) .await else { From f170c96fbdca4a846b4deb8132766d0b58c94a0a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 19:02:31 +0000 Subject: [PATCH 4/9] Don't fail old-npm legs on the overrides e2e npm before 8.3 has no overrides, so the #490 case doesn't exist there. Return without calling skip(), which panics on the pinned (REQUIRED) CI legs for npm 6 and 7. Refs #490 Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs b/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs index dfe7ef61c..1590f1b47 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs @@ -309,7 +309,9 @@ async fn redirect_scanned_project( } Setup::OverriddenGitDep => { if !npm_supports_overrides() { - npm_e2e_common::skip(&suite, "this npm predates `overrides` (npm 8.3)"); + // Not a skip: the case doesn't exist on this npm, so the + // pinned (REQUIRED) legs for older majors still pass. + println!("N/A {suite}: this npm predates `overrides` (npm 8.3)"); return None; } if !install_overridden_git_dep(&suite, tmp.path(), &proj, &cache) { From 9356ac954d5ced6963795d4c0e3749d42da08c1c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 19:05:23 +0000 Subject: [PATCH 5/9] Treat npm 9.0's override crash as N/A in e2e npm 9.0.0 can't install the #490 project at all: it dies with "Invalid comparator: github:..." when an override covers a git spec. The case doesn't exist on that npm, so the e2e returns instead of failing CI's pinned 9.0.0 leg. npm 9.9 and later run it fully. Refs #490 Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-cli/tests/e2e_redirect_npm_build.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs b/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs index 1590f1b47..a10fa957e 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs @@ -211,11 +211,19 @@ fn install_overridden_git_dep(suite: &str, tmp: &Path, proj: &Path, cache: &Path ], ); if !out.status.success() { + let text = npm_e2e_common::output_text(&out); + // Early npm 9 releases crash on an override that covers a git spec + // (`Invalid comparator: github:…`): the #490 project can't be + // installed there at all, so the case doesn't exist (CI's pinned + // 9.0.0 leg). Not a skip, which would fail the REQUIRED legs. + if text.contains("Invalid comparator") { + println!("N/A {suite}: this npm can't install a git dependency under `overrides`"); + return false; + } npm_e2e_common::skip( suite, &format!( - "`npm install` of the overrides fixture failed (registry unreachable?):\n{}", - npm_e2e_common::output_text(&out) + "`npm install` of the overrides fixture failed (registry unreachable?):\n{text}" ), ); return false; From c657128f2872ee00702ab5472c983dc28b75b8fc Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 19:20:28 +0000 Subject: [PATCH 6/9] Let the closest npm override rule win Two override rules scoped to different ancestors at the same nesting depth tied, and the one later in key order won. npm uses the rule of the closest ancestor, so a farther registry rule could clear an edge that a closer git or file: rule keeps on that source. socket-patch could then patch or attest a copy npm still installs from the spec. Rules are now ranked by how close their ancestor is to the dependent, then by depth. Equally ranked rules that disagree count as unclear, so the copy stays reported as unpatched. Refs #490 Assisted-by: Claude Code:claude-opus-5-5 --- .../src/vendor/npm_origin.rs | 117 ++++++++++++++++-- 1 file changed, 105 insertions(+), 12 deletions(-) diff --git a/crates/socket-patch-core/src/vendor/npm_origin.rs b/crates/socket-patch-core/src/vendor/npm_origin.rs index 82273da13..c07e655df 100644 --- a/crates/socket-patch-core/src/vendor/npm_origin.rs +++ b/crates/socket-patch-core/src/vendor/npm_origin.rs @@ -96,9 +96,10 @@ impl NpmOverrides { /// A rule applies when its key names `dep_name` with no selector (or /// with `spec` itself as the selector), and every enclosing rule names /// the dependent or one of its physical ancestors (outermost first), - /// with no selector or that package's exact version. The innermost - /// applicable rule wins, as in npm. Version-range selectors are not - /// evaluated, so a rule that uses one never applies here. + /// with no selector or that package's exact version. As in npm, the + /// rule scoped to the closest ancestor wins (then the most deeply + /// nested one). Two equally close rules that disagree make the answer + /// unclear, and so does a version-range selector: neither applies. fn replacement( &self, packages: &Map, @@ -110,13 +111,13 @@ impl NpmOverrides { return None; } let chain = dependent_chain(packages, from); - let mut best: Option<(usize, String)> = None; + let mut best: Option = None; self.visit(&self.rules, &chain, 0, 0, dep_name, spec, &mut best); - best.map(|(_, value)| value) + best.filter(|b| !b.contested).map(|b| b.value) } /// Search `rules` (nested `depth` levels deep; the enclosing rules - /// matched `chain[..from_ix]`) for the deepest rule for the edge. + /// matched ancestors up to `chain[from_ix - 1]`) for the edge's rule. #[allow(clippy::too_many_arguments)] fn visit( &self, @@ -126,7 +127,7 @@ impl NpmOverrides { depth: usize, dep_name: &str, spec: &str, - best: &mut Option<(usize, String)>, + best: &mut Option, ) { for (key, value) in rules { if key == "." { @@ -136,12 +137,11 @@ impl NpmOverrides { // A rule for the edge's own package. if name == dep_name && selector.is_none_or(|sel| sel == spec) { if let Some(replacement) = self.rule_value(value) { - if best.as_ref().is_none_or(|(d, _)| depth >= *d) { - *best = Some((depth, replacement)); - } + BestRule::offer(best, (from_ix, depth), replacement); } } - // A rule scoped to a package on the dependent's chain. + // A rule scoped to a package on the dependent's chain: every + // matching ancestor, so the closest one is ranked too. if let Some(children) = value.as_object() { for (ix, (anc_name, anc_version)) in chain.iter().enumerate().skip(from_ix) { let version_ok = match selector { @@ -150,7 +150,6 @@ impl NpmOverrides { }; if anc_name == name && version_ok { self.visit(children, chain, ix + 1, depth + 1, dep_name, spec, best); - break; } } } @@ -172,6 +171,32 @@ impl NpmOverrides { } } +/// The winning override rule so far: ranked by how close its innermost +/// enclosing ancestor is to the dependent (`chain` entries consumed), then +/// by nesting depth. `contested` when an equally ranked rule disagrees. +#[derive(Debug)] +struct BestRule { + rank: (usize, usize), + value: String, + contested: bool, +} + +impl BestRule { + fn offer(best: &mut Option, rank: (usize, usize), value: String) { + match best { + Some(b) if rank < b.rank => {} + Some(b) if rank == b.rank => b.contested |= b.value != value, + _ => { + *best = Some(BestRule { + rank, + value, + contested: false, + }) + } + } + } +} + /// `name` or `name@selector` (a scoped `@scope/name` keeps its leading `@`). fn split_selector(key: &str) -> (&str, Option<&str>) { let (scope_at, rest) = match key.strip_prefix('@') { @@ -652,6 +677,74 @@ mod tests { assert!(!found.contains_key("node_modules/left-pad"), "{found:?}"); } + #[test] + fn issue_490_the_closest_ancestor_rule_wins_whatever_the_key_order() { + // `@scope/b` under `a` depends on left-pad from git. Rules scoped to + // `a` (farther) and `@scope/b` (closer) sit at the same nesting + // depth; the closer one decides, in either key order. + let lock = lock(json!({ + "": { "dependencies": { "a": "^1.0.0" } }, + "node_modules/a": { "version": "1.0.0", "resolved": REGISTRY_TGZ }, + "node_modules/a/node_modules/@scope/b": { + "version": "2.0.0", + "resolved": REGISTRY_TGZ, + "dependencies": { "left-pad": "github:stevemao/left-pad#v1.3.0" } + }, + "node_modules/a/node_modules/left-pad": { + "version": "1.3.0", + "resolved": REGISTRY_TGZ + } + })); + let key = "node_modules/a/node_modules/left-pad"; + let found = |overrides: Value| { + npm_non_registry_entries( + &lock, + &NpmOverrides::from_manifest(&json!({ "overrides": overrides })), + ) + .contains_key(key) + }; + for (closer, farther, non_registry) in [ + ("github:someone/left-pad", "1.3.0", true), + ("1.3.0", "github:someone/left-pad", false), + ] { + // `@scope/b` sorts before `a`, so the farther rule is visited + // last; swapping which ancestor holds the registry spec covers + // both outcomes. + assert_eq!( + found(json!({ + "@scope/b": { "left-pad": closer }, + "a": { "left-pad": farther } + })), + non_registry, + "closer {closer} / farther {farther}" + ); + } + } + + #[test] + fn issue_490_equally_close_rules_that_disagree_keep_the_edge() { + let lock = overridden_git_lock(REGISTRY_TGZ); + // `pkga` and `pkga@1.0.0` both scope to the dependent at the same + // depth: npm's pick between them isn't modelled, so stay cautious. + let found = npm_non_registry_entries( + &lock, + &manifest(json!({ + "pkga": { "left-pad": "1.3.0" }, + "pkga@1.0.0": { "left-pad": "github:someone/left-pad" } + })), + ); + assert!(found.contains_key("node_modules/left-pad"), "{found:?}"); + // Agreeing rules are fine. + let found = npm_non_registry_entries( + &lock, + &manifest(json!({ + "pkga": { "left-pad": "1.3.0" }, + "pkga@1.0.0": { "left-pad": "1.3.0" } + })), + ); + assert!(!found.contains_key("node_modules/left-pad"), "{found:?}"); + } + #[test] fn issue_490_a_nested_dependent_matches_its_physical_ancestors() { // `@scope/b` under `a` depends on left-pad from git; the override is From 5dff22113b43531f97701acd9186cc580b8b5d8d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 2 Oct 2026 14:28:49 +0000 Subject: [PATCH 7/9] Keep npm overrides that may not apply cautious Two override shapes still let socket-patch treat a git or URL dependency as a registry install while npm ci kept fetching the original spec: - A "*" or empty override is a no-op in npm, which keeps the dependent's raw spec, but socket-patch read "*" as a registry range and patched the entry. - A rule nested under a range selector such as "pkga@^1" was ignored, so a broader top-level registry rule cleared the entry even though npm uses the narrower rule. A winning "*", empty or nested-only rule now leaves the raw spec in effect. A range selector that may apply, or a target selector other than the edge's own spec, makes the result unclear, so the copy stays reported as unpatched. Refs #490 Assisted-by: Claude Code:claude-opus-5-5 --- .../src/patch/redirect/mod.rs | 53 +++++ .../src/vendor/npm_origin.rs | 197 ++++++++++++++++-- 2 files changed, 237 insertions(+), 13 deletions(-) diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index c9332b389..737089b08 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -12284,6 +12284,59 @@ mod tests { /// 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 issue_490_unclear_overrides_leave_a_url_dependency_unredirected() { + // The review probes: npm keeps the URL spec for a `*` override, and + // picks the narrower rule under a range selector, so `npm ci` still + // fetches the URL. The rewriter must skip the entry loudly. + let url = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz"; + let lock = json!({ + "name": "app", + "lockfileVersion": 3, + "packages": { + "": { "name": "app", "version": "0.0.0", "dependencies": { "pkga": "^1.0.0" } }, + "node_modules/pkga": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/pkga/-/pkga-1.0.0.tgz", + "dependencies": { "left-pad": url } + }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": url, + "integrity": "sha512-UPSTREAM==" + } + } + }); + let overrides = vec![npm_override( + "left-pad", + "1.3.0", + "http://patch.test/lp.tgz", + "sha512-PATCHED==", + )]; + for manifest_overrides in [ + json!({ "left-pad": "*" }), + json!({ "left-pad": "1.3.0", "pkga@^1": { "left-pad": url } }), + ] { + let mut files = BTreeMap::new(); + files.insert( + "package-lock.json".to_string(), + serde_json::to_string_pretty(&lock).unwrap(), + ); + files.insert( + "package.json".to_string(), + json!({ "name": "app", "dependencies": { "pkga": "^1.0.0" }, "overrides": manifest_overrides }) + .to_string(), + ); + let r = rewrite_registry_redirect(&files, &overrides); + assert!(r.files.is_empty(), "{manifest_overrides}: {:?}", r.edits); + assert!( + warning_codes(&r).contains(&"redirect_npm_non_registry_entry_skipped"), + "{manifest_overrides}: {:?}", + r.warnings + ); + } + } + #[test] fn issue_490_a_git_edge_overridden_to_the_registry_is_redirected() { // `pkga` depends on left-pad from git; the project's `overrides` diff --git a/crates/socket-patch-core/src/vendor/npm_origin.rs b/crates/socket-patch-core/src/vendor/npm_origin.rs index c07e655df..f89dbb900 100644 --- a/crates/socket-patch-core/src/vendor/npm_origin.rs +++ b/crates/socket-patch-core/src/vendor/npm_origin.rs @@ -98,8 +98,14 @@ impl NpmOverrides { /// the dependent or one of its physical ancestors (outermost first), /// with no selector or that package's exact version. As in npm, the /// rule scoped to the closest ancestor wins (then the most deeply - /// nested one). Two equally close rules that disagree make the answer - /// unclear, and so does a version-range selector: neither applies. + /// nested one), and a winning `*` or empty value is a no-op that leaves + /// the dependent's own spec in effect. + /// + /// Anything this can't decide exactly makes the answer unclear, and + /// then no override applies: two equally close rules that disagree, a + /// target selector other than `spec`, or an enclosing selector that is + /// a range (it may match the ancestor, so a rule beneath it may be the + /// one npm picks). fn replacement( &self, packages: &Map, @@ -111,9 +117,15 @@ impl NpmOverrides { return None; } let chain = dependent_chain(packages, from); - let mut best: Option = None; - self.visit(&self.rules, &chain, 0, 0, dep_name, spec, &mut best); - best.filter(|b| !b.contested).map(|b| b.value) + let mut search = RuleSearch::default(); + self.visit(&self.rules, &chain, 0, 0, dep_name, spec, &mut search); + if search.unclear { + return None; + } + let best = search.best.filter(|b| !b.contested)?; + // npm ignores a `*` (or empty) replacement: the raw spec stays. + let value = best.value.trim(); + (!value.is_empty() && value != "*").then(|| best.value) } /// Search `rules` (nested `depth` levels deep; the enclosing rules @@ -127,7 +139,7 @@ impl NpmOverrides { depth: usize, dep_name: &str, spec: &str, - best: &mut Option, + search: &mut RuleSearch, ) { for (key, value) in rules { if key == "." { @@ -135,27 +147,59 @@ impl NpmOverrides { } let (name, selector) = split_selector(key); // A rule for the edge's own package. - if name == dep_name && selector.is_none_or(|sel| sel == spec) { - if let Some(replacement) = self.rule_value(value) { - BestRule::offer(best, (from_ix, depth), replacement); + if name == dep_name { + match selector { + None => self.offer(value, (from_ix, depth), search), + Some(sel) if sel == spec => self.offer(value, (from_ix, depth), search), + // npm matches other selectors against the spec by + // semver intersection, which isn't modelled here. + Some(_) => search.unclear = true, } } // A rule scoped to a package on the dependent's chain: every // matching ancestor, so the closest one is ranked too. if let Some(children) = value.as_object() { for (ix, (anc_name, anc_version)) in chain.iter().enumerate().skip(from_ix) { - let version_ok = match selector { + if anc_name != name { + continue; + } + let applies = match selector { None => true, - Some(sel) => anc_version.as_deref() == Some(sel), + Some(sel) if anc_version.as_deref() == Some(sel) => true, + // Another exact version: the rule can't apply. + Some(sel) if is_exact_version(sel) && anc_version.is_some() => false, + // A range (or an unknown ancestor version): it may + // apply, so a rule for the edge beneath it may be + // the one npm picks. + Some(_) => { + if mentions(children, dep_name) { + search.unclear = true; + } + false + } }; - if anc_name == name && version_ok { - self.visit(children, chain, ix + 1, depth + 1, dep_name, spec, best); + if applies { + self.visit(children, chain, ix + 1, depth + 1, dep_name, spec, search); } } } } } + /// Rank a rule for the edge; one whose value can't be resolved (a + /// `$name` the root doesn't declare, a non-string) makes it unclear. + fn offer(&self, value: &Value, rank: (usize, usize), search: &mut RuleSearch) { + match self.rule_value(value) { + Some(replacement) => BestRule::offer(&mut search.best, rank, replacement), + // An object with only nested rules overrides nothing itself, + // but still shadows a farther rule: a no-op, like `*`. + None if value.as_object().is_some_and(|o| !o.contains_key(".")) => { + BestRule::offer(&mut search.best, rank, String::new()) + } + None => search.unclear = true, + } + } + /// A rule's replacement spec: the string itself, or an object's `"."`, /// with a `$name` reference resolved against the root's dependencies. fn rule_value(&self, value: &Value) -> Option { @@ -171,6 +215,39 @@ impl NpmOverrides { } } +/// What [`NpmOverrides::visit`] has found for one edge. +#[derive(Debug, Default)] +struct RuleSearch { + best: Option, + /// A rule that may apply couldn't be evaluated exactly. + unclear: bool, +} + +/// Whether `rules` (at any depth) holds a rule keyed by `dep_name`. +fn mentions(rules: &Map, dep_name: &str) -> bool { + rules.iter().any(|(key, value)| { + split_selector(key).0 == dep_name + || value + .as_object() + .is_some_and(|children| mentions(children, dep_name)) + }) +} + +/// A plain `major.minor.patch` version, optionally with a prerelease or +/// build suffix: never a range. +fn is_exact_version(selector: &str) -> bool { + let core_end = selector.find(['-', '+']).unwrap_or(selector.len()); + let (core, suffix) = selector.split_at(core_end); + let parts: Vec<&str> = core.split('.').collect(); + parts.len() == 3 + && parts + .iter() + .all(|p| !p.is_empty() && p.bytes().all(|b| b.is_ascii_digit())) + && suffix + .bytes() + .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'-' | b'+' | b'.')) +} + /// The winning override rule so far: ranked by how close its innermost /// enclosing ancestor is to the dependent (`chain` entries consumed), then /// by nesting depth. `contested` when an equally ranked rule disagrees. @@ -745,6 +822,100 @@ mod tests { assert!(!found.contains_key("node_modules/left-pad"), "{found:?}"); } + #[test] + fn issue_490_wildcard_and_empty_overrides_leave_the_raw_spec() { + // npm ignores a `*` (or empty) replacement, so the dependent's URL + // or git spec stays in effect (npm 10.9.4 `edge.js`). Covered for a + // remote-tarball spec and a git spec, with `"."` objects too. + let url = REGISTRY_TGZ; + let url_lock = lock(json!({ + "": { "dependencies": { "left-pad": url } }, + "node_modules/left-pad": { "version": "1.3.0", "resolved": url } + })); + let git_lock = overridden_git_lock(REGISTRY_TGZ); + for value in [json!("*"), json!(""), json!(" * "), json!({ ".": "*" })] { + for (lock, label) in [(&url_lock, "url"), (&git_lock, "git")] { + let found = + npm_non_registry_entries(lock, &manifest(json!({ "left-pad": value.clone() }))); + assert!( + found.contains_key("node_modules/left-pad"), + "{label} / {value}: {found:?}" + ); + } + } + // A closer no-op shadows a farther registry rule, as in npm. + let found = npm_non_registry_entries( + &git_lock, + &manifest(json!({ "left-pad": "1.3.0", "pkga": { "left-pad": "*" } })), + ); + assert!(found.contains_key("node_modules/left-pad"), "{found:?}"); + let found = npm_non_registry_entries( + &git_lock, + &manifest(json!({ + "left-pad": "1.3.0", + "pkga": { "left-pad": { "nested": "1.0.0" } } + })), + ); + assert!(found.contains_key("node_modules/left-pad"), "{found:?}"); + } + + #[test] + fn issue_490_range_selectors_that_may_apply_keep_the_edge() { + let lock = overridden_git_lock(REGISTRY_TGZ); + let tgz = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz"; + for scoped in ["pkga@^1", "pkga@1.x", "pkga@>=1", "pkga@*", "pkga@1"] { + // npm picks the narrower rule under `pkga@^1` (here a URL) over + // the top-level registry rule, so the broader one can't clear + // the edge while the selector is unevaluated. + let found = npm_non_registry_entries( + &lock, + &manifest(json!({ "left-pad": "1.3.0", scoped: { "left-pad": tgz } })), + ); + assert!( + found.contains_key("node_modules/left-pad"), + "{scoped}: {found:?}" + ); + } + // A range selector whose subtree never names the dependency, and an + // exact selector for another version, don't block the clear. + for overrides in [ + json!({ "left-pad": "1.3.0", "pkga@^1": { "other": "1.0.0" } }), + json!({ "left-pad": "1.3.0", "pkga@2.0.0": { "left-pad": tgz } }), + ] { + let found = npm_non_registry_entries(&lock, &manifest(overrides.clone())); + assert!( + !found.contains_key("node_modules/left-pad"), + "{overrides}: {found:?}" + ); + } + // A target selector other than the edge's own spec is unclear too. + let found = npm_non_registry_entries( + &lock, + &manifest(json!({ "left-pad": "1.3.0", "left-pad@^1": "github:x/y" })), + ); + assert!(found.contains_key("node_modules/left-pad"), "{found:?}"); + } + + #[test] + fn exact_versions_are_told_from_ranges() { + for exact in ["1.0.0", "10.2.33", "1.0.0-beta.1", "1.0.0+build.5"] { + assert!(is_exact_version(exact), "{exact}"); + } + for range in [ + "^1", + "1", + "1.x", + "1.0", + "~1.0.0", + ">=1.0.0", + "*", + "1.0.0 || 2.0.0", + "v1.0.0", + ] { + assert!(!is_exact_version(range), "{range}"); + } + } + #[test] fn issue_490_a_nested_dependent_matches_its_physical_ancestors() { // `@scope/b` under `a` depends on left-pad from git; the override is From fa440ae7a96a8e54d80f777fe53ff34e2d0ef2f0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 2 Oct 2026 14:32:14 +0000 Subject: [PATCH 8/9] Fix clippy lint in npm override check The previous commit used bool::then with a closure where then_some is enough, which fails the CI clippy gate (-D warnings). Refs #490 Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-core/src/vendor/npm_origin.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/socket-patch-core/src/vendor/npm_origin.rs b/crates/socket-patch-core/src/vendor/npm_origin.rs index f89dbb900..d04332765 100644 --- a/crates/socket-patch-core/src/vendor/npm_origin.rs +++ b/crates/socket-patch-core/src/vendor/npm_origin.rs @@ -125,7 +125,7 @@ impl NpmOverrides { let best = search.best.filter(|b| !b.contested)?; // npm ignores a `*` (or empty) replacement: the raw spec stays. let value = best.value.trim(); - (!value.is_empty() && value != "*").then(|| best.value) + (!value.is_empty() && value != "*").then_some(best.value) } /// Search `rules` (nested `depth` levels deep; the enclosing rules From 3ef00ef2f6d738e41b645e2663057fae04e37f3a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 2 Oct 2026 15:02:45 +0000 Subject: [PATCH 9/9] Match npm override selectors without build tags An override scoped to "pkga@1.0.0+build.1" applies to an installed pkga 1.0.0, because semver ignores build metadata. socket-patch compared the selector to the version as raw text, skipped the rule, and let a broader registry override patch a dependency that npm ci still fetches from its URL. Exact-version selectors now compare without build metadata: equal versions apply the nested rules, and different ones (prereleases included) still can't. Refs #490 Assisted-by: Claude Code:claude-opus-5-5 --- .../src/patch/redirect/mod.rs | 1 + .../src/vendor/npm_origin.rs | 50 +++++++++++++++++-- 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 737089b08..d02bfa337 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -12316,6 +12316,7 @@ mod tests { for manifest_overrides in [ json!({ "left-pad": "*" }), json!({ "left-pad": "1.3.0", "pkga@^1": { "left-pad": url } }), + json!({ "left-pad": "1.3.0", "pkga@1.0.0+build.1": { "left-pad": url } }), ] { let mut files = BTreeMap::new(); files.insert( diff --git a/crates/socket-patch-core/src/vendor/npm_origin.rs b/crates/socket-patch-core/src/vendor/npm_origin.rs index d04332765..9c8a31840 100644 --- a/crates/socket-patch-core/src/vendor/npm_origin.rs +++ b/crates/socket-patch-core/src/vendor/npm_origin.rs @@ -165,9 +165,13 @@ impl NpmOverrides { } let applies = match selector { None => true, - Some(sel) if anc_version.as_deref() == Some(sel) => true, - // Another exact version: the rule can't apply. - Some(sel) if is_exact_version(sel) && anc_version.is_some() => false, + // An exact version applies when it equals the + // ancestor's under semver, which ignores build + // metadata (`1.0.0+build.1` matches `1.0.0`), and + // can't apply otherwise. + Some(sel) if is_exact_version(sel) && anc_version.is_some() => anc_version + .as_deref() + .is_some_and(|v| without_build(v) == without_build(sel)), // A range (or an unknown ancestor version): it may // apply, so a rule for the edge beneath it may be // the one npm picks. @@ -233,6 +237,12 @@ fn mentions(rules: &Map, dep_name: &str) -> bool { }) } +/// `version` without its `+build` metadata, which semver ignores when +/// comparing. +fn without_build(version: &str) -> &str { + version.split_once('+').map_or(version, |(core, _)| core) +} + /// A plain `major.minor.patch` version, optionally with a prerelease or /// build suffix: never a range. fn is_exact_version(selector: &str) -> bool { @@ -896,6 +906,40 @@ mod tests { assert!(found.contains_key("node_modules/left-pad"), "{found:?}"); } + #[test] + fn issue_490_exact_selectors_compare_without_build_metadata() { + // The follow-up review probe: npm matches `pkga@1.0.0+build.1` to + // the installed `pkga@1.0.0`, so its narrower URL rule wins over + // the top-level registry rule and the edge stays non-registry. + let lock = overridden_git_lock(REGISTRY_TGZ); + let tgz = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz"; + for scoped in ["pkga@1.0.0+build.1", "pkga@1.0.0+other"] { + let found = npm_non_registry_entries( + &lock, + &manifest(json!({ "left-pad": "1.3.0", scoped: { "left-pad": tgz } })), + ); + assert!( + found.contains_key("node_modules/left-pad"), + "{scoped}: {found:?}" + ); + // And a registry rule under it applies as the exact match. + let found = npm_non_registry_entries( + &lock, + &manifest(json!({ scoped: { "left-pad": "1.3.0" } })), + ); + assert!( + !found.contains_key("node_modules/left-pad"), + "{scoped}: {found:?}" + ); + } + // A prerelease is a different version, so its rule can't apply. + let found = npm_non_registry_entries( + &lock, + &manifest(json!({ "left-pad": "1.3.0", "pkga@1.0.0-beta.1": { "left-pad": tgz } })), + ); + assert!(!found.contains_key("node_modules/left-pad"), "{found:?}"); + } + #[test] fn exact_versions_are_told_from_ranges() { for exact in ["1.0.0", "10.2.33", "1.0.0-beta.1", "1.0.0+build.5"] {