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-cli/tests/e2e_redirect_npm_build.rs b/crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs index fb7a7332f..a10fa957e 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,110 @@ 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() { + 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{text}" + ), + ); + 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 +284,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 +303,30 @@ 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() { + // 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) { + return None; + } + extra_files.push(PKGA_TGZ); + } } let expected_lock_version = match major { ..=6 => 1, @@ -566,9 +693,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 +716,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 +724,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 +891,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 +919,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 +1005,7 @@ async fn npm_redirect_shrinkwrap_fresh_checkout_and_manifestless_vex() { false, RedirectCli::ScanRedirectVex, LockFlavor::Shrinkwrap, + Setup::Direct, ) .await else { @@ -865,6 +1028,7 @@ async fn npm_redirect_tampered_hosted_tarball_fails_fresh_npm_ci() { true, RedirectCli::ScanRedirectVex, LockFlavor::PackageLock, + Setup::Direct, ) .await else { @@ -904,6 +1068,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 +1094,7 @@ async fn npm_get_ghsa_hosted_narrows_and_installs() { false, RedirectCli::GetGhsaHosted, LockFlavor::PackageLock, + Setup::Direct, ) .await else { 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..d02bfa337 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,116 @@ 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 } }), + json!({ "left-pad": "1.3.0", "pkga@1.0.0+build.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` + // 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..9c8a31840 100644 --- a/crates/socket-patch-core/src/vendor/npm_origin.rs +++ b/crates/socket-patch-core/src/vendor/npm_origin.rs @@ -23,13 +23,310 @@ //! * 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. As in npm, the + /// rule scoped to the closest ancestor wins (then the most deeply + /// 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, + from: &str, + dep_name: &str, + spec: &str, + ) -> Option { + if self.rules.is_empty() { + return None; + } + let chain = dependent_chain(packages, from); + 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_some(best.value) + } + + /// Search `rules` (nested `depth` levels deep; the enclosing rules + /// matched ancestors up to `chain[from_ix - 1]`) for the edge's rule. + #[allow(clippy::too_many_arguments)] + fn visit( + &self, + rules: &Map, + chain: &[(String, Option)], + from_ix: usize, + depth: usize, + dep_name: &str, + spec: &str, + search: &mut RuleSearch, + ) { + 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 { + 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) { + if anc_name != name { + continue; + } + let applies = match selector { + None => true, + // 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. + Some(_) => { + if mentions(children, dep_name) { + search.unclear = true; + } + false + } + }; + 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 { + 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()), + } + } +} + +/// 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)) + }) +} + +/// `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 { + 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. +#[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('@') { + 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 +350,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 +380,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 +552,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 +568,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 +582,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 +595,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 +619,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 +634,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 +652,361 @@ 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_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_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 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"] { + 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 + // 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