diff --git a/CHANGELOG.md b/CHANGELOG.md index 862487a57..7c88bd12c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -102,6 +102,17 @@ limits, and required install commands. ### Fixed +- **npm dependencies installed from git, a URL or `file:` are no longer + reported patched.** npm installs such a dependency from the dependent's + spec (`github:user/repo`, `https://…/x.tgz`, `file:…`) and ignores the + lock entry's `resolved`, so `npm ci` kept installing the original bytes + after `scan --mode hosted` or `vendor` rewired the entry and `vex` + attested it. Both modes now skip such an entry with a loud + stays-UNPATCHED warning (`redirect_npm_non_registry_entry_skipped` / + `vendor_non_registry_entry_skipped`; vendoring refuses with + `vendor_lock_entry_not_rewritable` when no registry copy is left), and + `vex` attests nothing for a `name@version` while such a copy is in the + lock (#326). - **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/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index 12547510d..ece909789 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -356,7 +356,7 @@ Discovery is read-only, never touches the network, and never fails the run: a ma | Ecosystem | Files read | Hosted reference | Vendored reference | Hosted pin (`integrity_required`) | |---|---|---|---|---| -| npm | `package-lock.json` and `npm-shrinkwrap.json` (both when both exist) | `resolved` on the patch host (`packages` in v2/v3; `dependencies` only in v1; `link` / `inBundle` / `bundled` entries skipped) | `resolved: file:.socket/vendor/npm//-.tgz` | `integrity`, required | +| npm | `package-lock.json` and `npm-shrinkwrap.json` (both when both exist) | `resolved` on the patch host (`packages` in v2/v3; `dependencies` only in v1; `link` / `inBundle` / `bundled` entries skipped, and so is any entry npm installs from a git, URL or `file:` spec, together with every ref for the same `name@version`) | `resolved: file:.socket/vendor/npm//-.tgz` | `integrity`, required | | pnpm | `pnpm-lock.yaml` (every `lockfileVersion`); `shrinkwrap.yaml` only when there is no `pnpm-lock.yaml`; with `rush.json`, `common/config/rush/pnpm-lock.yaml` + `common/config/subspaces/*/pnpm-lock.yaml` | `packages:` `resolution.tarball` on the patch host | `file:.socket/vendor/npm/…` tarball + key | `integrity`, required | | yarn | `yarn.lock` (classic and berry) | classic `resolved`; berry `resolution: …::__archiveUrl=` | classic `resolved "file:./.socket/vendor/npm/…#"`; berry `file:` entry **plus** a root `package.json` `resolutions` mapping onto the same artifact (without it the entry is orphaned: diagnosed, no ref) | classic `integrity` / `#sha1`, berry `checksum`, required | | bun | `bun.lock`; `bun.lockb` only when there is no `bun.lock` (bun reads exactly one) | URL tuple / binary remote-tarball resolution; version from the URL leaf | `.socket/vendor/npm//-.tgz` tuple / local-tarball resolution | `sha512-…`, required. A 2-tuple that Bun < 1.3.10 re-saved without its digest is still a reference, but it attests only from an installed tree. | diff --git a/crates/socket-patch-cli/tests/e2e_vendor_npm_build.rs b/crates/socket-patch-cli/tests/e2e_vendor_npm_build.rs index 40af54446..39b4c4a04 100644 --- a/crates/socket-patch-cli/tests/e2e_vendor_npm_build.rs +++ b/crates/socket-patch-cli/tests/e2e_vendor_npm_build.rs @@ -596,6 +596,119 @@ fn npm_vendor_vex_attests_against_vendored_tarball() { ); } +/// #326: a dependency installed from a remote-tarball spec (`"left-pad": +/// "https://…/left-pad-1.3.0.tgz"`) is fetched from that url by `npm ci`, +/// whatever the lock's `resolved` says. Vendoring used to rewire the entry +/// and report `applied` (and `vex` attested it) while `npm ci` installed +/// the original bytes. It must refuse, leave the lock untouched, and give +/// `vex` nothing to attest. +#[test] +fn npm_vendor_refuses_a_remote_tarball_dependency() { + let suite = "e2e_vendor_npm_build (remote tarball)"; + let Some(major) = npm_major_or_skip(suite) else { + return; + }; + let tmp = tempfile::tempdir().unwrap(); + let proj = tmp.path().join("proj"); + std::fs::create_dir_all(&proj).unwrap(); + std::fs::write( + proj.join("package.json"), + r#"{"name":"vendor-url-spec","version":"0.0.0","private":true}"#, + ) + .unwrap(); + let cache = tmp.path().join("npm-cache"); + let url = format!("https://registry.npmjs.org/{DEP}/-/{DEP}-{DEP_VERSION}.tgz"); + // npm 12 refuses remote-tarball specs unless `allow-remote` permits them. + let spec = if major >= 12 { + format!("{url} --allow-remote=all") + } else { + url.clone() + }; + let mut args = vec!["install", "--no-audit", "--no-fund", "--cache"]; + args.push(cache.to_str().unwrap()); + args.extend(spec.split(' ')); + let out = npm(&proj, &args); + if !out.status.success() { + npm_e2e_common::skip( + suite, + &format!( + "`npm install {spec}` failed (registry unreachable?):\n{}", + String::from_utf8_lossy(&out.stderr) + ), + ); + return; + } + let pkg: serde_json::Value = + serde_json::from_slice(&std::fs::read(proj.join("package.json")).unwrap()).unwrap(); + assert_eq!( + pkg["dependencies"][DEP], url, + "the fixture depends on the url spec: {pkg}" + ); + + let installed_index = proj.join("node_modules").join(DEP).join("index.js"); + let orig = std::fs::read(&installed_index).expect("installed index.js"); + let patched: Vec = [MARKER.as_bytes(), orig.as_slice()].concat(); + let purl = format!("pkg:npm/{DEP}@{DEP_VERSION}"); + const GHSA: &str = "GHSA-vend-npm-url"; + stage_patch_with_vuln(&proj, &purl, "package/index.js", &orig, &patched, GHSA); + if v1_lock_is_refused(&proj, major) { + return; + } + let lock_before = std::fs::read(proj.join("package-lock.json")).unwrap(); + + let (_code, stdout, stderr) = run_socket( + &proj, + &[ + "vendor", + "--json", + "--offline", + "--cwd", + proj.to_str().unwrap(), + ], + ); + let env = parse_envelope(&stdout); + assert_eq!( + env["summary"]["applied"], 0, + "a url-spec dependency must not be vendored: {env}\nstderr:\n{stderr}" + ); + assert!( + stdout.contains("vendor_lock_entry_not_rewritable") && stdout.contains("UNPATCHED"), + "the refusal must say why: {env}" + ); + assert_eq!( + std::fs::read(proj.join("package-lock.json")).unwrap(), + lock_before, + "the lock is untouched" + ); + assert!( + !proj.join(format!(".socket/vendor/npm/{UUID}")).exists(), + "no artifact is written" + ); + + let vex_path = proj.join("out.vex.json"); + let (code, stdout, stderr) = run_socket( + &proj, + &[ + "vex", + "--cwd", + proj.to_str().unwrap(), + "--output", + vex_path.to_str().unwrap(), + "--product", + "pkg:npm/app@1.0.0", + ], + ); + let attested = std::fs::read(&vex_path) + .ok() + .and_then(|b| serde_json::from_slice::(&b).ok()) + .and_then(|doc| doc["statements"].as_array().map(|s| !s.is_empty())) + .unwrap_or(false); + assert!( + !attested, + "vex must not attest an unwired patch (exit {code}).\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); +} + /// get-driven twin of the capstone (v3.6): instead of hand-staging /// `.socket/` (manifest + blob) and running `vendor --offline`, /// `get --mode vendored` resolves the SAME patch from a mocked diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index f244e0e62..ceed30f70 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -25,6 +25,7 @@ use serde_json::{json, Value}; use crate::utils::digest::is_hex64_lower; use crate::utils::line_endings::{to_lf, LineEndings}; +use crate::vendor::npm_origin::{legacy_packages_key, npm_non_registry_entries}; use crate::vendor::yarn_berry_lock::yarnrc_compression_level; mod bun_binary; @@ -863,6 +864,9 @@ fn rewrite_one_npm_lock( .collect() }) .unwrap_or_default(); + // Entries npm installs from a git / url / `file:` spec: see + // `vendor::npm_origin` (#326). + let non_registry = npm_non_registry_entries(&lock); let mut changed = false; for dep in npm { let fname = full_name(dep); @@ -910,6 +914,22 @@ fn rewrite_one_npm_lock( }); continue; } + // npm installs a git / url / `file:` dependency from the + // dependent's spec and ignores `resolved`, so a rewrite here + // would confirm (and VEX-attest) a patch that never installs. + if let Some(reason) = non_registry.get(key.as_str()) { + matched_any = true; + result.warnings.push(RewriteWarning { + code: "redirect_npm_non_registry_entry_skipped".into(), + detail: format!( + "lock entry `{key}` is not installed from the registry ({reason}) \ + and CANNOT be redirected — npm installs it from that spec, so \ + that copy stays UNPATCHED; depend on the registry release to \ + patch it" + ), + }); + continue; + } matched_any = true; if let Some(edit) = rewrite_npm_entry( entry, @@ -928,6 +948,8 @@ fn rewrite_one_npm_lock( if let Some(deps) = lock.get_mut("dependencies").and_then(Value::as_object_mut) { changed = rewrite_npm_v2_deps( deps, + "", + &non_registry, &fname, dep, &sha512, @@ -1002,8 +1024,11 @@ fn rewrite_npm_entry( }) } +#[allow(clippy::too_many_arguments)] fn rewrite_npm_v2_deps( deps: &mut serde_json::Map, + parent_key: &str, + non_registry: &BTreeMap, fname: &str, dep: &DepOverride, sha512: &str, @@ -1013,6 +1038,7 @@ fn rewrite_npm_v2_deps( ) -> bool { let mut changed = false; for (name, entry) in deps.iter_mut() { + let packages_key = legacy_packages_key(parent_key, name); if name == fname && entry.get("version").and_then(Value::as_str) == Some(dep.version.as_str()) { @@ -1028,6 +1054,12 @@ fn rewrite_npm_v2_deps( or update the bundling parent to cover it" ), }); + } else if non_registry.contains_key(&packages_key) { + // The mirror of a `packages` entry npm installs from a git / + // url / `file:` spec: that twin was skipped (and warned about) + // above, so rewriting this copy would only record an edit for + // bytes that never install. + *matched_any = true; } else { *matched_any = true; if let Some(edit) = @@ -1039,9 +1071,17 @@ fn rewrite_npm_v2_deps( } } if let Some(nested) = entry.get_mut("dependencies").and_then(Value::as_object_mut) { - changed = - rewrite_npm_v2_deps(nested, fname, dep, sha512, lockfile, result, matched_any) - || changed; + changed = rewrite_npm_v2_deps( + nested, + &packages_key, + non_registry, + fname, + dep, + sha512, + lockfile, + result, + matched_any, + ) || changed; } } changed @@ -12151,6 +12191,199 @@ mod tests { ); } + /// #326: npm installs a git, remote-tarball or `file:` dependency from + /// the dependent's spec and ignores the lock's `resolved`, so rewiring + /// that entry would report (and VEX-attest) a patch `npm ci` never + /// installs. It must be skipped loudly, like a bundled copy. + #[test] + fn npm_non_registry_entries_are_skipped_with_loud_warning() { + let url = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz"; + for (spec, resolved) in [ + ( + "github:stevemao/left-pad#v1.3.0", + "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba", + ), + (url, url), + ("file:../left-pad-1.3.0.tgz", "file:../left-pad-1.3.0.tgz"), + ] { + let lock = json!({ + "name": "app", + "lockfileVersion": 3, + "packages": { + "": { "name": "app", "version": "0.0.0", "dependencies": { "left-pad": spec } }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": resolved, + "integrity": "sha512-UPSTREAM==" + } + } + }); + let mut files = BTreeMap::new(); + files.insert( + "package-lock.json".to_string(), + serde_json::to_string_pretty(&lock).unwrap(), + ); + let overrides = vec![npm_override( + "left-pad", + "1.3.0", + "http://patch.test/lp.tgz", + "sha512-PATCHED==", + )]; + let r = rewrite_registry_redirect(&files, &overrides); + assert!( + r.files.is_empty() && r.edits.is_empty(), + "{spec}: a non-registry entry must not be rewired: {:?}", + r.edits + ); + let skipped = r + .warnings + .iter() + .find(|w| w.code == "redirect_npm_non_registry_entry_skipped") + .unwrap_or_else(|| panic!("{spec}: the skip must warn: {:?}", r.warnings)); + assert!( + skipped.detail.contains("UNPATCHED") + && skipped.detail.contains("node_modules/left-pad"), + "{spec}: {}", + skipped.detail + ); + assert!( + !warning_codes(&r).contains(&"redirect_npm_entry_not_found"), + "{spec}: the entry was found: {:?}", + r.warnings + ); + } + } + + /// #326, lockfileVersion 2: the legacy `dependencies` mirror of a + /// non-registry `packages` entry is left alone too, even when it stores + /// the plain version, while the registry copy's mirror is rewired. + #[test] + fn npm_v2_legacy_mirror_of_a_non_registry_entry_is_not_rewired() { + let registry = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz"; + let git = "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba"; + let lock = json!({ + "name": "app", + "lockfileVersion": 2, + "packages": { + "": { "name": "app", "version": "0.0.0", + "dependencies": { "a": "^1.0.0", "left-pad": "^1.3.0" } }, + "node_modules/a": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/a/-/a-1.0.0.tgz", + "integrity": "sha512-A==", + "dependencies": { "left-pad": "stevemao/left-pad#v1.3.0" } + }, + "node_modules/a/node_modules/left-pad": { "version": "1.3.0", "resolved": git }, + "node_modules/left-pad": { + "version": "1.3.0", "resolved": registry, "integrity": "sha512-UPSTREAM==" + } + }, + "dependencies": { + "a": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/a/-/a-1.0.0.tgz", + "integrity": "sha512-A==", + "requires": { "left-pad": "stevemao/left-pad#v1.3.0" }, + "dependencies": { + "left-pad": { "version": "1.3.0", "resolved": git } + } + }, + "left-pad": { + "version": "1.3.0", "resolved": registry, "integrity": "sha512-UPSTREAM==" + } + } + }); + let mut files = BTreeMap::new(); + files.insert( + "package-lock.json".to_string(), + serde_json::to_string_pretty(&lock).unwrap(), + ); + let overrides = vec![npm_override( + "left-pad", + "1.3.0", + "http://patch.test/lp.tgz", + "sha512-PATCHED==", + )]; + let r = rewrite_registry_redirect(&files, &overrides); + let keys: Vec<_> = r + .edits + .iter() + .map(|e| (e.kind.as_str(), e.key.as_deref())) + .collect(); + assert_eq!( + keys, + [ + ("redirect_npm_lock_entry", Some("node_modules/left-pad")), + ("redirect_npm_lock_dep", Some("left-pad")), + ], + "only the registry copy and its mirror are rewired" + ); + let out: Value = serde_json::from_str(&r.files["package-lock.json"]).unwrap(); + assert_eq!( + out["dependencies"]["a"]["dependencies"]["left-pad"], + lock["dependencies"]["a"]["dependencies"]["left-pad"], + "the git copy's legacy mirror is byte-untouched" + ); + assert_eq!( + out["dependencies"]["left-pad"]["resolved"], + "http://patch.test/lp.tgz" + ); + } + + /// #326, transitive: a nested git copy is skipped while the hoisted + /// registry copy of the same version is still redirected. + #[test] + fn npm_nested_git_copy_is_skipped_and_registry_copy_rewired() { + let lock = json!({ + "name": "app", + "lockfileVersion": 3, + "packages": { + "": { "name": "app", "version": "0.0.0", + "dependencies": { "a": "^1.0.0", "left-pad": "^1.3.0" } }, + "node_modules/a": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/a/-/a-1.0.0.tgz", + "integrity": "sha512-A==", + "dependencies": { "left-pad": "stevemao/left-pad#v1.3.0" } + }, + "node_modules/a/node_modules/left-pad": { + "version": "1.3.0", + "resolved": "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba" + }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz", + "integrity": "sha512-UPSTREAM==" + } + } + }); + let mut files = BTreeMap::new(); + files.insert( + "package-lock.json".to_string(), + serde_json::to_string_pretty(&lock).unwrap(), + ); + let overrides = vec![npm_override( + "left-pad", + "1.3.0", + "http://patch.test/lp.tgz", + "sha512-PATCHED==", + )]; + let r = rewrite_registry_redirect(&files, &overrides); + assert_eq!(r.edits.len(), 1, "{:?}", r.edits); + assert_eq!(r.edits[0].key.as_deref(), Some("node_modules/left-pad")); + assert!( + warning_codes(&r).contains(&"redirect_npm_non_registry_entry_skipped"), + "{:?}", + r.warnings + ); + let out: Value = serde_json::from_str(&r.files["package-lock.json"]).unwrap(); + assert_eq!( + out["packages"]["node_modules/a/node_modules/left-pad"], + lock["packages"]["node_modules/a/node_modules/left-pad"], + "the git copy is byte-untouched" + ); + } + /// When the patched dep has both a regular entry and a bundled nested /// copy, the regular entry is redirected and the bundled copy is left /// byte-untouched behind the stays-UNPATCHED warning (partial coverage diff --git a/crates/socket-patch-core/src/vendor/mod.rs b/crates/socket-patch-core/src/vendor/mod.rs index 452886d2e..d72edf292 100644 --- a/crates/socket-patch-core/src/vendor/mod.rs +++ b/crates/socket-patch-core/src/vendor/mod.rs @@ -74,6 +74,7 @@ pub(crate) mod npm_common; pub(crate) mod npm_dir; pub mod npm_flavor; pub mod npm_lock; +pub(crate) mod npm_origin; mod npm_pack; pub(crate) mod nuget_config; pub mod nuget_feed; diff --git a/crates/socket-patch-core/src/vendor/npm_lock.rs b/crates/socket-patch-core/src/vendor/npm_lock.rs index 087d2ecc8..0a8237cb8 100644 --- a/crates/socket-patch-core/src/vendor/npm_lock.rs +++ b/crates/socket-patch-core/src/vendor/npm_lock.rs @@ -14,6 +14,7 @@ //! bytes — no error, no patch. Every rewrite therefore carries the packed //! tarball's own hash, never an inherited one. +use std::collections::BTreeMap; use std::path::Path; use serde_json::Value; @@ -28,6 +29,7 @@ use super::common::{already_patched_result, detect_indent, done, refused, serial use super::npm_common::{ done_failure_unstage, guard_coordinates, guard_revert_uuid_dir, stage_patch_pack, }; +use super::npm_origin::{legacy_packages_key, npm_non_registry_entries}; use super::parse_memo::ParseMemo; use super::path::parse_vendor_path; use super::source::PackageSource; @@ -461,7 +463,9 @@ fn rewritable_matches( .filter(|w| { matches!( w.code, - "vendor_bundled_instance_skipped" | "vendor_link_entry_skipped" + "vendor_bundled_instance_skipped" + | "vendor_link_entry_skipped" + | "vendor_non_registry_entry_skipped" ) }) .map(|w| w.detail.as_str()) @@ -471,8 +475,9 @@ fn rewritable_matches( "vendor_lock_entry_not_rewritable", format!( "every {lock_name} entry for {name}@{version} is bundled inside a \ - parent's tarball or a link and cannot be rewritten — those copies \ - stay UNPATCHED and `npm install` will not help: {}", + parent's tarball, a link, or installed from a non-registry spec and \ + cannot be rewritten — those copies stay UNPATCHED and `npm install` \ + will not help: {}", skipped.join("; ") ), ))); @@ -836,6 +841,7 @@ fn scan_lock_matches( let Some(packages) = lock.get("packages").and_then(Value::as_object) else { return LockScan::Matches(matches); // validated earlier; defensive }; + let non_registry = npm_non_registry_entries(lock); for (key, entry) in packages { // The root "" entry is the project itself, never a dependency. if key.is_empty() { @@ -873,6 +879,21 @@ fn scan_lock_matches( )); continue; } + if let Some(reason) = non_registry.get(key.as_str()) { + // LOUD: npm installs a git / url / `file:` dependency from the + // dependent's spec and ignores `resolved`, so a rewrite here + // would report the patch applied while the original bytes + // install (#326). + warnings.push(VendorWarning::new( + "vendor_non_registry_entry_skipped", + format!( + "lock entry `{key}` is not installed from the registry ({reason}) and \ + CANNOT be rewritten — npm installs it from that spec, so that copy stays \ + UNPATCHED; depend on the registry release to vendor it" + ), + )); + continue; + } matches.push(LockMatch { key: key.clone(), original: entry.clone(), @@ -936,6 +957,8 @@ fn recompute_dep_fields(live: &mut serde_json::Map, staged_pkg: & fn rewrite_legacy_tree( deps: &mut serde_json::Map, pointer_base: &str, + parent_key: &str, + non_registry: &BTreeMap, name: &str, version: &str, resolved: &str, @@ -953,6 +976,7 @@ fn rewrite_legacy_tree( continue; }; let pointer = format!("{pointer_base}/{}", escape_json_pointer_token(dep_name)); + let packages_key = legacy_packages_key(parent_key, dep_name); let node_version = obj.get("version").and_then(Value::as_str); if node_version == Some(alias_version.as_str()) { // An aliased consumer of the patched package. The modern @@ -980,6 +1004,14 @@ fn rewrite_legacy_tree( // `resolved`, and rewriting it would desync the two lock halves. // (The `packages` twin carries `inBundle` and already pushed the // stays-UNPATCHED warning.) + } else if dep_name == name + && node_version == Some(version) + && non_registry.contains_key(&packages_key) + { + // The mirror of a `packages` entry npm installs from a git / url + // / `file:` spec (#326): its twin was skipped with + // `vendor_non_registry_entry_skipped`, so rewiring this copy + // would record wiring for bytes that never install. } else if dep_name == name && node_version == Some(version) && !entry_in_sync(obj, resolved, integrity) @@ -1005,6 +1037,8 @@ fn rewrite_legacy_tree( rewrite_legacy_tree( sub, &format!("{pointer}/dependencies"), + &packages_key, + non_registry, name, version, resolved, @@ -1224,6 +1258,8 @@ impl LockRewire<'_> { recomputed_deps: &mut bool, warnings: &mut Vec, ) -> Result<(), String> { + // Taken before any rewrite, for the legacy mirror below. + let non_registry = npm_non_registry_entries(lock); let Some(packages) = lock.get_mut("packages").and_then(Value::as_object_mut) else { return Err("lock `packages` object vanished mid-rewrite".to_string()); }; @@ -1274,6 +1310,8 @@ impl LockRewire<'_> { rewrite_legacy_tree( deps, "/dependencies", + "", + &non_registry, self.name, self.version, self.resolved, @@ -2026,6 +2064,90 @@ mod tests { ); } + /// #326: a git / remote-tarball / `file:` dependency is installed from + /// the dependent's spec, not the lock's `resolved`, so vendoring it + /// would report `applied` while `npm ci` installs the original bytes. + /// With no other copy the vendor refuses and writes nothing. + #[tokio::test] + async fn non_registry_only_instances_refuse_and_write_nothing() { + let url = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz"; + for (spec, resolved) in [ + ( + "github:stevemao/left-pad#v1.3.0", + "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba", + ), + (url, url), + ("file:../left-pad-1.3.0.tgz", "file:../left-pad-1.3.0.tgz"), + ] { + let lock = json!({ + "name": "fixture", + "version": "1.0.0", + "lockfileVersion": 3, + "packages": { + "": { "name": "fixture", "version": "1.0.0", + "dependencies": { "left-pad": spec } }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": resolved, + "integrity": "sha512-orig==" + } + } + }); + let fx = fixture_with("left-pad", "1.3.0", lock).await; + let detail = expect_refused(fx.vendor(false).await, "vendor_lock_entry_not_rewritable"); + assert!( + detail.contains("UNPATCHED") && detail.contains("node_modules/left-pad"), + "{spec}: {detail}" + ); + assert!( + !detail.contains("make sure the package is installed"), + "{spec}: {detail}" + ); + assert_eq!( + tokio::fs::read(fx.lock_path()).await.unwrap(), + fx.lock_bytes, + "{spec}: lock untouched by the refusal" + ); + assert!( + !fx.root().join(".socket/vendor").exists(), + "{spec}: refusal writes nothing" + ); + } + } + + /// #326, transitive: the nested git copy is skipped loudly and the + /// registry copies are still vendored. + #[tokio::test] + async fn nested_git_instance_is_skipped_with_warning() { + let mut lock = default_lock(); + lock["packages"]["node_modules/foo"]["dependencies"] = + json!({ "left-pad": "github:stevemao/left-pad#v1.3.0" }); + lock["packages"]["node_modules/foo/node_modules/left-pad"]["resolved"] = + json!("git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba"); + let fx = fixture_with("left-pad", "1.3.0", lock.clone()).await; + let (result, entry, warnings) = expect_done(fx.vendor(false).await); + assert!(result.success); + assert_eq!(entry.unwrap().wiring.len(), 1, "only the hoisted copy"); + let skipped = warnings + .iter() + .find(|w| w.code == "vendor_non_registry_entry_skipped") + .unwrap_or_else(|| panic!("{warnings:?}")); + assert!( + skipped.detail.contains("UNPATCHED") + && skipped + .detail + .contains("node_modules/foo/node_modules/left-pad"), + "{}", + skipped.detail + ); + let live = fx.read_lock().await; + assert_eq!( + live["packages"]["node_modules/foo/node_modules/left-pad"], + lock["packages"]["node_modules/foo/node_modules/left-pad"], + "the git copy is byte-untouched" + ); + } + /// When EVERY lock instance of the target is bundled or a link, the /// refusal must state the real reason (the entry IS in the lock and /// `npm install` will not help) and keep the stays-UNPATCHED advisory — @@ -2520,6 +2642,72 @@ mod tests { ); } + /// #326, v2 legacy mirror: the mirror of a non-registry `packages` + /// entry is not rewired either, even when it stores the plain version. + #[tokio::test] + async fn v2_legacy_mirror_of_a_git_instance_is_not_rewritten() { + let git = "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba"; + let lock = json!({ + "name": "fixture", + "version": "1.0.0", + "lockfileVersion": 2, + "requires": true, + "packages": { + "": { "name": "fixture", "version": "1.0.0", + "dependencies": { "foo": "^2.0.0", "left-pad": "^1.3.0" } }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": REG_RESOLVED, + "integrity": "sha512-orig==" + }, + "node_modules/foo": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/foo/-/foo-2.0.0.tgz", + "integrity": "sha512-foo==", + "dependencies": { "left-pad": "github:stevemao/left-pad#v1.3.0" } + }, + "node_modules/foo/node_modules/left-pad": { "version": "1.3.0", "resolved": git } + }, + "dependencies": { + "foo": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/foo/-/foo-2.0.0.tgz", + "integrity": "sha512-foo==", + "requires": { "left-pad": "github:stevemao/left-pad#v1.3.0" }, + "dependencies": { + "left-pad": { "version": "1.3.0", "resolved": git } + } + }, + "left-pad": { + "version": "1.3.0", + "resolved": REG_RESOLVED, + "integrity": "sha512-orig==" + } + } + }); + let fx = fixture_with("left-pad", "1.3.0", lock.clone()).await; + let (result, entry, _) = expect_done(fx.vendor(false).await); + assert!(result.success, "{:?}", result.error); + let legacy_keys: Vec = entry + .unwrap() + .wiring + .iter() + .filter(|r| r.kind == KIND_LOCK_LEGACY_ENTRY) + .filter_map(|r| r.key.clone()) + .collect(); + assert_eq!( + legacy_keys, + ["/dependencies/left-pad"], + "only the registry copy's mirror" + ); + let live = fx.read_lock().await; + assert_eq!( + live["dependencies"]["foo"]["dependencies"]["left-pad"], + lock["dependencies"]["foo"]["dependencies"]["left-pad"], + "the git copy's legacy mirror is byte-untouched" + ); + } + /// v2 legacy mirror: an alias consumer (`"aliased": {"version": /// "npm:left-pad@1.3.0"}`) has no proven equivalent rewrite — it must be /// left untouched AND loudly warned, since npm 6 reading the mirror diff --git a/crates/socket-patch-core/src/vendor/npm_origin.rs b/crates/socket-patch-core/src/vendor/npm_origin.rs new file mode 100644 index 000000000..14ef34f72 --- /dev/null +++ b/crates/socket-patch-core/src/vendor/npm_origin.rs @@ -0,0 +1,345 @@ +//! Which `package-lock.json` / `npm-shrinkwrap.json` entries npm installs +//! from the registry, and which it installs from somewhere else. +//! +//! npm installs a git (`github:user/repo`, `git+ssh://…`), remote-tarball +//! (`https://…/x.tgz`) or local (`file:…`) dependency from the DEPENDENT's +//! spec, not from the lock entry's `resolved`: rewriting that entry's +//! `resolved` / `integrity` changes nothing at install time (`npm ci` fetches +//! the git checkout or the URL again, and the next `npm install` writes the +//! original `resolved` back). The hosted rewriter +//! (`patch::redirect::rewrite_one_npm_lock`), the vendored backend +//! (`vendor::npm_lock`) and lockfile discovery (`vex::discover::npm`) all +//! share [`npm_non_registry_entries`], so a copy the rewriters refuse is +//! never attested either. +//! +//! An entry is non-registry when EITHER +//! +//! * an inbound dependency spec that resolves to it is not a registry spec +//! ([`npm_spec_is_registry`]). The edges are the `dependencies`, +//! `optionalDependencies`, `devDependencies` and `peerDependencies` of +//! every `packages` entry (the root `""`, workspace members and installed +//! packages alike), resolved with node's lookup order: the dependent's own +//! `node_modules/`, then each ancestor directory's; +//! * or its own `resolved` names a git or `file:` source. socket-patch's own +//! vendored wiring (`file:.socket/vendor/…`) is not one: the vendored +//! backend only rewrites `resolved`, never the dependent's spec. + +use std::collections::BTreeMap; + +use serde_json::Value; + +use crate::constants::SOCKET_DIR; + +/// The dependency maps whose specs npm resolves against `packages` entries. +const EDGE_FIELDS: [&str; 4] = [ + "dependencies", + "optionalDependencies", + "devDependencies", + "peerDependencies", +]; + +/// The `packages` key a lockfileVersion 2 legacy `dependencies` node +/// mirrors: `parent` is the mirrored key of the enclosing node (`""` at the +/// top of the tree), `name` the node's key in its `dependencies` map. +pub(crate) fn legacy_packages_key(parent: &str, name: &str) -> String { + if parent.is_empty() { + format!("node_modules/{name}") + } else { + format!("{parent}/node_modules/{name}") + } +} + +/// Every `packages` key npm installs from a non-registry source, mapped to +/// the reason (for the skip warnings). Empty for a lock without `packages`: +/// in a lockfileVersion 1 `dependencies` tree a git / URL / `file:` entry's +/// `version` is that spec, so it never matches a patch's `name@version`. +pub(crate) fn npm_non_registry_entries(lock: &Value) -> BTreeMap { + let mut out = BTreeMap::new(); + let Some(packages) = lock.get("packages").and_then(Value::as_object) else { + return out; + }; + for (key, entry) in packages { + if !key.contains("node_modules/") { + continue; + } + if let Some(resolved) = entry.get("resolved").and_then(Value::as_str) { + if resolved_is_non_registry(resolved) { + out.insert(key.clone(), format!("it resolves to {resolved:?}")); + } + } + } + for (from, entry) in packages { + for field in EDGE_FIELDS { + let Some(deps) = entry.get(field).and_then(Value::as_object) else { + continue; + }; + for (dep_name, spec) in deps { + let Some(spec) = spec.as_str() else { + continue; + }; + if npm_spec_is_registry(spec) { + continue; + } + let Some(target) = resolve_edge(packages, from, dep_name) else { + continue; + }; + let dependent = if from.is_empty() { + "the project".to_string() + } else { + format!("`{from}`") + }; + out.entry(target).or_insert_with(|| { + format!( + "{dependent} depends on it as {spec:?}, which npm installs from that spec" + ) + }); + } + } + } + out +} + +/// The `packages` key node's module lookup picks for `dep_name` required +/// from the package at `from`: `/node_modules/`, then the same +/// under each ancestor directory, up to the project root. +fn resolve_edge( + packages: &serde_json::Map, + from: &str, + dep_name: &str, +) -> Option { + let mut dir = from; + loop { + let candidate = if dir.is_empty() { + format!("node_modules/{dep_name}") + } else { + format!("{dir}/node_modules/{dep_name}") + }; + if packages.contains_key(&candidate) { + return Some(candidate); + } + if dir.is_empty() { + return None; + } + dir = dir.rsplit_once('/').map_or("", |(parent, _)| parent); + } +} + +/// A lock `resolved` that is a git or local source rather than a tarball +/// url (see the module docs for socket-patch's own `file:` wiring). +fn resolved_is_non_registry(resolved: &str) -> bool { + const GIT: [&str; 7] = [ + "git+", + "git:", + "git@", + "github:", + "gitlab:", + "bitbucket:", + "gist:", + ]; + if GIT.iter().any(|p| resolved.starts_with(p)) { + return true; + } + match resolved.strip_prefix("file:") { + Some(path) => { + let path = path.trim_start_matches("./"); + !path.starts_with(&format!("{SOCKET_DIR}/vendor/")) + } + None => false, + } +} + +/// Whether npm resolves `spec` against the registry: a version, a semver +/// range, a dist-tag, or an `npm:` alias of one. Everything else (git, +/// GitHub shorthand, a url, a path, a tarball file name) is installed from +/// the spec itself. Mirrors npm-package-arg's classification. +pub(crate) fn npm_spec_is_registry(spec: &str) -> bool { + let spec = spec.trim(); + if let Some(alias) = spec.strip_prefix("npm:") { + // `npm:name`, `npm:name@range`, `npm:@scope/name@range`. + let unscoped = alias.strip_prefix('@').unwrap_or(alias); + return match unscoped.split_once('@') { + Some((_, range)) => npm_spec_is_registry(range), + None => true, + }; + } + // A scheme (`git+ssh:`, `github:`, `https:`, `file:`, a `C:` drive), a + // path (`./x`, `../x`, `/x`, `~/x`), GitHub shorthand (`user/repo`) or + // a tarball file name. No semver range or dist-tag contains ':' or '/'. + if spec.contains(':') || spec.contains('/') || spec.contains('\\') { + return false; + } + if spec.starts_with('.') { + return false; + } + let lower = spec.to_ascii_lowercase(); + !(lower.ends_with(".tgz") || lower.ends_with(".tar.gz") || lower.ends_with(".tar")) +} + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::*; + + #[test] + fn registry_specs_are_recognized() { + for spec in [ + "", + "*", + "1.3.0", + "^1.3.0", + "~1.3.0", + ">=1 <2", + "1.x || 2", + "latest", + "next", + "npm:left-pad@1.3.0", + "npm:@scope/pad@^1", + "npm:left-pad", + ] { + assert!(npm_spec_is_registry(spec), "{spec:?} is a registry spec"); + } + } + + #[test] + fn non_registry_specs_are_recognized() { + for spec in [ + "github:stevemao/left-pad#v1.3.0", + "stevemao/left-pad", + "stevemao/left-pad#v1.3.0", + "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba", + "git+https://github.com/stevemao/left-pad.git", + "git://github.com/stevemao/left-pad.git", + "gitlab:user/repo", + "bitbucket:user/repo", + "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz", + "http://example.com/left-pad.tgz", + "file:../left-pad-1.3.0.tgz", + "file:vendor/left-pad", + "./left-pad", + "../left-pad", + "/abs/left-pad", + "~/left-pad", + "left-pad-1.3.0.tgz", + "C:\\pkgs\\left-pad.tgz", + "npm:left-pad@github:stevemao/left-pad", + ] { + assert!( + !npm_spec_is_registry(spec), + "{spec:?} is not a registry spec" + ); + } + } + + fn lock(packages: Value) -> Value { + json!({ "lockfileVersion": 3, "packages": packages }) + } + + #[test] + fn a_direct_git_dependency_is_non_registry() { + let lock = lock(json!({ + "": { "dependencies": { "left-pad": "github:stevemao/left-pad#v1.3.0" } }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba" + } + })); + let found = npm_non_registry_entries(&lock); + assert!(found.contains_key("node_modules/left-pad"), "{found:?}"); + } + + #[test] + fn a_rewired_git_dependency_is_still_non_registry_by_its_spec() { + // After a rewrite the entry's `resolved` is a hosted url or our own + // `file:.socket/vendor/…` tarball; the dependent's spec still says git. + for resolved in [ + "https://patch.socket.dev/npm/left-pad/-/left-pad-1.3.0.tgz", + "file:.socket/vendor/npm/11111111-2222-4333-8444-555555555555/left-pad-1.3.0.tgz", + ] { + let lock = lock(json!({ + "": { "dependencies": { "left-pad": "github:stevemao/left-pad#v1.3.0" } }, + "node_modules/left-pad": { "version": "1.3.0", "resolved": resolved } + })); + assert!(npm_non_registry_entries(&lock).contains_key("node_modules/left-pad")); + } + } + + #[test] + fn a_remote_tarball_dependency_is_non_registry() { + // The url is the registry's own tarball, but npm installs it from + // the spec: only the dependent's spec tells the two apart. + let url = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz"; + let lock = lock(json!({ + "": { "dependencies": { "left-pad": url } }, + "node_modules/left-pad": { "version": "1.3.0", "resolved": url } + })); + assert!(npm_non_registry_entries(&lock).contains_key("node_modules/left-pad")); + } + + #[test] + fn a_file_tarball_dependency_is_non_registry() { + let lock = lock(json!({ + "": { "dependencies": { "left-pad": "file:../left-pad-1.3.0.tgz" } }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": "file:../left-pad-1.3.0.tgz" + } + })); + assert!(npm_non_registry_entries(&lock).contains_key("node_modules/left-pad")); + } + + #[test] + fn transitive_edges_resolve_with_node_lookup_order() { + // `a` depends on left-pad from git and gets its own nested copy; + // the hoisted copy the root depends on is a registry install. + let lock = lock(json!({ + "": { "dependencies": { "a": "^1.0.0", "left-pad": "^1.3.0" } }, + "node_modules/a": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/a/-/a-1.0.0.tgz", + "dependencies": { "left-pad": "github:stevemao/left-pad#v1.3.0" } + }, + "node_modules/a/node_modules/left-pad": { + "version": "1.3.0", + "resolved": "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba" + }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz" + } + })); + let found = npm_non_registry_entries(&lock); + assert!(found.contains_key("node_modules/a/node_modules/left-pad")); + assert!(!found.contains_key("node_modules/left-pad"), "{found:?}"); + assert!(!found.contains_key("node_modules/a")); + } + + #[test] + fn a_workspace_member_edge_walks_up_to_the_hoisted_copy() { + let url = "https://example.com/left-pad-1.3.0.tgz"; + let lock = lock(json!({ + "": { "workspaces": ["packages/*"] }, + "packages/app": { "dependencies": { "left-pad": url } }, + "node_modules/app": { "resolved": "packages/app", "link": true }, + "node_modules/left-pad": { "version": "1.3.0", "resolved": url } + })); + assert!(npm_non_registry_entries(&lock).contains_key("node_modules/left-pad")); + } + + #[test] + fn registry_dependencies_are_not_flagged() { + let lock = lock(json!({ + "": { "dependencies": { "left-pad": "1.3.0", "alias": "npm:left-pad@1.3.0" } }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz" + }, + "node_modules/alias": { + "name": "left-pad", + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz" + } + })); + assert!(npm_non_registry_entries(&lock).is_empty()); + } +} diff --git a/crates/socket-patch-core/src/vex/discover/npm.rs b/crates/socket-patch-core/src/vex/discover/npm.rs index a6d0ca005..0cb2b7431 100644 --- a/crates/socket-patch-core/src/vex/discover/npm.rs +++ b/crates/socket-patch-core/src/vex/discover/npm.rs @@ -58,6 +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; pub(crate) async fn extract(ctx: &DiscoverCtx<'_>, out: &mut Discovery) { let mut locks: Vec = Vec::new(); @@ -188,9 +189,63 @@ async fn extract_package_lock( read.bundled.entry(purl).or_insert(location); } } + drop_non_registry_installs(file, &doc, &mut read, out); Some(read) } +/// npm installs a git / url / `file:` dependency from the dependent's spec +/// and ignores the entry's `resolved` (`vendor::npm_origin`, #326), so such +/// an entry stays unpatched whatever its `resolved` says. Every ref for the +/// same `name@version` is dropped (that copy is live beside it), and the +/// copy counts as resolved elsewhere, so other locks' wiring for it is +/// contested too. +fn drop_non_registry_installs( + file: &str, + doc: &Value, + read: &mut NpmLockRefs, + out: &mut Discovery, +) { + let non_registry = npm_non_registry_entries(doc); + if non_registry.is_empty() { + return; + } + let mut unpatched: Vec<(String, &str, &str)> = Vec::new(); + for (key, reason) in &non_registry { + let entry = &doc["packages"][key.as_str()]; + let key_name = key.rsplit_once("node_modules/").map_or("", |(_, n)| n); + let name = entry + .get("name") + .and_then(Value::as_str) + .unwrap_or(key_name); + let Some(purl) = entry + .get("version") + .and_then(Value::as_str) + .and_then(|v| npm_purl(name, v)) + else { + continue; + }; + out.resolved_elsewhere(file, Some(purl.clone())); + read.unwired.insert(purl.clone()); + unpatched.push((purl, key, reason)); + } + read.refs.retain(|r| { + let Some((_, key, reason)) = unpatched.iter().find(|(p, _, _)| *p == r.purl) else { + return true; + }; + out.diag( + DIAG_REF_UNATTRIBUTABLE, + file, + format!( + "{file}: {} is wired to a Socket patch but lock entry `{key}` is not \ + installed from the registry ({reason}); npm installs it from that spec, so \ + that copy stays UNPATCHED and nothing is attested", + r.purl + ), + ); + false + }); +} + /// Classify one lock entry: a ref (into `read.refs`), a package resolved /// elsewhere (into `read.unwired`), or nothing. fn entry_ref( @@ -1016,6 +1071,90 @@ mod tests { assert_eq!(bundled_contests(&out).len(), 2, "{:#?}", 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 + /// non-registry copy of the same version stays unpatched beside it. + #[tokio::test] + async fn entries_npm_installs_from_a_non_registry_spec_are_not_attested() { + let hosted = hosted_url("npm", "left-pad", "1.3.0", UUID_A, "left-pad-1.3.0.tgz"); + let vendored = format!("file:.socket/vendor/npm/{UUID_B}/left-pad-1.3.0.tgz"); + let tarball = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz"; + for spec in [ + "github:stevemao/left-pad#v1.3.0", + tarball, + "file:../left-pad-1.3.0.tgz", + ] { + for (wiring, resolved) in [("hosted", &hosted), ("vendored", &vendored)] { + let p = Project::new(); + p.write( + "package-lock.json", + lock_with_packages(serde_json::json!({ + "": { "name": "app", "version": "1.0.0", + "dependencies": { "left-pad": spec } }, + "node_modules/left-pad": { + "version": "1.3.0", "resolved": resolved, "integrity": SRI + }, + })), + ); + let out = run(&p).await; + assert!( + out.refs.is_empty(), + "{spec} / {wiring}: {} refs", + out.refs.len() + ); + assert!( + diag_codes(&out).contains(&DIAG_REF_UNATTRIBUTABLE), + "{spec} / {wiring}: {:?}", + diag_codes(&out) + ); + } + } + // Transitive: the hoisted copy is wired, a nested git copy of the + // same version is not. + let p = Project::new(); + p.write( + "package-lock.json", + lock_with_packages(serde_json::json!({ + "": { "name": "app", "version": "1.0.0", + "dependencies": { "a": "^1.0.0", "left-pad": "^1.3.0" } }, + "node_modules/a": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/a/-/a-1.0.0.tgz", + "dependencies": { "left-pad": "stevemao/left-pad#v1.3.0" } + }, + "node_modules/a/node_modules/left-pad": { + "version": "1.3.0", + "resolved": "git+ssh://git@github.com/stevemao/left-pad.git#ff8e7ba" + }, + "node_modules/left-pad": { "version": "1.3.0", "resolved": hosted, "integrity": SRI }, + })), + ); + let out = run(&p).await; + assert!(out.refs.is_empty(), "{} refs", out.refs.len()); + let diag = out + .diagnostics + .iter() + .find(|d| d.code == DIAG_REF_UNATTRIBUTABLE) + .unwrap_or_else(|| panic!("{:?}", out.diagnostics)); + assert!( + diag.detail.contains("node_modules/a/node_modules/left-pad"), + "{}", + diag.detail + ); + // Control: a registry spec keeps the ref. + let p = Project::new(); + p.write( + "package-lock.json", + lock_with_packages(serde_json::json!({ + "": { "name": "app", "version": "1.0.0", + "dependencies": { "left-pad": "^1.3.0" } }, + "node_modules/left-pad": { "version": "1.3.0", "resolved": hosted, "integrity": SRI }, + })), + ); + assert_eq!(run(&p).await.refs.len(), 1); + } + /// Negative shapes: a uuid on a foreign host, a placeholder token, the /// root and workspace-member keys, and an escaping vendored path. #[tokio::test]