diff --git a/crates/socket-patch-cli/tests/in_process_vendor.rs b/crates/socket-patch-cli/tests/in_process_vendor.rs index 93dbf0c24..427ff467f 100644 --- a/crates/socket-patch-cli/tests/in_process_vendor.rs +++ b/crates/socket-patch-cli/tests/in_process_vendor.rs @@ -543,6 +543,88 @@ async fn rollback_after_dependency_removed_cleans_up_and_converges() { assert!(events(&env).is_empty(), "nothing left to revert: {env:#}"); } +/// What `npm install left-pad@1.3.1` leaves behind after vendoring 1.3.0: +/// the same lock key, now resolving the new version from the registry. +fn upgrade_vendored_left_pad(fx: &NpmFixture) -> Vec { + let mut lock = fx.lock_value(); + lock["packages"]["node_modules/left-pad"] = json!({ + "version": "1.3.1", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.1.tgz", + "integrity": "sha512-upgraded==" + }); + let mut upgraded = serde_json::to_vec_pretty(&lock).unwrap(); + upgraded.push(b'\n'); + std::fs::write(fx.lock_path(), &upgraded).unwrap(); + upgraded +} + +/// #1155: moving a vendored package off its patched version (`npm install +/// left-pad@1.3.1`, a Dependabot bump) takes the vendored version out of +/// the lock graph just like `npm uninstall`. `rollback` used to call that +/// drift, keep the artifact and ledger entry and exit 1 on every run, so +/// `vendor --check` stayed red. Now the first rollback cleans up, leaves +/// the user's upgraded lock alone, and later runs are clean no-ops. +#[tokio::test] +async fn rollback_after_version_upgrade_cleans_up_and_converges() { + let fx = npm_fixture(); + assert_eq!(vendor_run(vendor_args(fx.root())).await, 0); + let upgraded = upgrade_vendored_left_pad(&fx); + + let cwd = fx.root().to_str().unwrap(); + let (code, stdout, stderr) = run_cli( + fx.root(), + &["rollback", "--json", "--yes", "--offline", "--cwd", cwd], + &[], + ); + assert_eq!(code, 0, "rollback must succeed:\n{stdout}\n{stderr}"); + assert!( + !stdout.contains("vendor_artifact_kept") && !stdout.contains("drifted"), + "nothing is kept as drift:\n{stdout}" + ); + assert!( + !fx.vendor_dir().exists(), + "the unreferenced artifact and ledger are cleaned up:\n{stdout}" + ); + assert_eq!(fx.lock_bytes(), upgraded, "the user's lock is untouched"); + + let (code, env) = vendor_cli(fx.root(), &["--revert"]); + assert_eq!(code, 0, "{env:#}"); + assert!(events(&env).is_empty(), "nothing left to revert: {env:#}"); + let (code, env) = vendor_cli(fx.root(), &["--check"]); + assert_eq!(code, 0, "vendor --check is green again: {env:#}"); +} + +/// #1155, `remove` leg: it used to end on "drift-kept …; re-run `scan +/// --mode vendored` to normalize, then remove again", a remedy that +/// changes nothing. Now it reverts the entry and exits 0. +#[tokio::test] +async fn remove_after_version_upgrade_reverts_vendoring() { + let fx = npm_fixture(); + assert_eq!(vendor_run(vendor_args(fx.root())).await, 0); + let upgraded = upgrade_vendored_left_pad(&fx); + + let (code, stdout, stderr) = run_cli( + fx.root(), + &[ + "remove", + PURL, + "--json", + "--offline", + "--yes", + "--cwd", + fx.root().to_str().unwrap(), + ], + &[], + ); + assert_eq!(code, 0, "remove must succeed:\n{stdout}\n{stderr}"); + let env: Value = serde_json::from_str(&stdout).unwrap(); + let reverted = find_event(&env, "removed", Some("vendor_reverted")); + assert_eq!(reverted["purl"], PURL); + assert!(!stdout.contains("drift-kept"), "{env:#}"); + assert!(!fx.vendor_dir().exists(), "vendor tree fully removed"); + assert_eq!(fx.lock_bytes(), upgraded, "the user's lock is untouched"); +} + // ───────────────────────────────────────────────────────────────────── // 5. revert works without a manifest // ───────────────────────────────────────────────────────────────────── diff --git a/crates/socket-patch-cli/tests/scan_vendor_e2e.rs b/crates/socket-patch-cli/tests/scan_vendor_e2e.rs index 8c0850eb0..a471e4ede 100644 --- a/crates/socket-patch-cli/tests/scan_vendor_e2e.rs +++ b/crates/socket-patch-cli/tests/scan_vendor_e2e.rs @@ -1161,6 +1161,97 @@ async fn scan_prune_reverts_unused_vendored_entry() { ); } +/// #1155: `npm install left-pad@1.3.1` after vendoring 1.3.0 keeps the +/// `node_modules/left-pad` key but locks the new version from the +/// registry. The vendored version left the lock graph just as after +/// `npm uninstall`, so `scan --prune` must revert the entry in one run. +/// It used to call the moved entry drift and keep it forever, so the +/// prune remedy `vendor --check` names never converged. +#[tokio::test] +async fn scan_prune_reverts_vendored_entry_after_version_upgrade() { + let mock = MockServer::start().await; + mount_patch_api(&mock, UUID).await; + let tmp = tempfile::tempdir().unwrap(); + write_fixture(tmp.path()); + + let (code, stdout, stderr) = run_scan_vendor(tmp.path(), &mock.uri(), &[]); + assert_eq!(code, 0, "stdout={stdout}; stderr={stderr}"); + assert!(tmp + .path() + .join(format!(".socket/vendor/npm/{UUID}")) + .exists()); + + // What `npm install left-pad@1.3.1` leaves behind. + let lock = serde_json::json!({ + "name": "scan-vendor-test", + "version": "0.0.0", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { + "name": "scan-vendor-test", + "version": "0.0.0", + "dependencies": { "left-pad": "^1.3.1" } + }, + "node_modules/left-pad": { + "version": "1.3.1", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.1.tgz", + "integrity": "sha512-upgraded==", + "license": "WTFPL" + } + } + }); + let mut lock_bytes = serde_json::to_vec_pretty(&lock).unwrap(); + lock_bytes.push(b'\n'); + std::fs::write(tmp.path().join("package-lock.json"), &lock_bytes).unwrap(); + std::fs::write( + tmp.path().join("node_modules/left-pad/package.json"), + br#"{"name":"left-pad","version":"1.3.1"}"#, + ) + .unwrap(); + + let out = Command::new(binary()) + .args([ + "scan", + "--json", + "--prune", + "--yes", + "--api-url", + &mock.uri(), + "--api-token", + "fake-token", + "--org", + ORG_SLUG, + ]) + .current_dir(tmp.path()) + .output() + .expect("run"); + let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); + assert_eq!(out.status.code(), Some(0), "stdout={stdout}"); + let v: serde_json::Value = serde_json::from_str(stdout.trim()).expect("valid JSON"); + assert_eq!( + v["gc"]["revertedVendoredEntries"], + serde_json::json!([PURL]), + "gc must revert the upgraded-away entry: {v}" + ); + assert_eq!( + v["gc"]["keptVendoredEntries"], + serde_json::json!([]), + "nothing resolves through the artifact, so nothing is kept: {v}" + ); + assert!( + !tmp.path() + .join(format!(".socket/vendor/npm/{UUID}")) + .exists(), + "artifact dir removed" + ); + assert_eq!( + std::fs::read(tmp.path().join("package-lock.json")).unwrap(), + lock_bytes, + "the user's upgraded lock is left exactly as they wrote it" + ); +} + /// #541, npm package-lock flavor: after `npm uninstall left-pad` re-locks /// the project without the vendored dependency, a vendored rescan skips /// the stale ledger entry with a `vendor_ledger_entry_unwired` warning diff --git a/crates/socket-patch-core/src/vendor/bun_binary.rs b/crates/socket-patch-core/src/vendor/bun_binary.rs index 6137e82e0..3697e7a47 100644 --- a/crates/socket-patch-core/src/vendor/bun_binary.rs +++ b/crates/socket-patch-core/src/vendor/bun_binary.rs @@ -644,10 +644,14 @@ pub(crate) async fn revert(entry: &VendorEntry, root: &Path, opts: RevertOpts) - if let RevertLock::Migrated(lines) = &mut lock { if rec.file == TEXT_LOCK && rec.kind == super::bun_lock::KIND_LOCK_PACKAGE { let mut dirty = false; + // `false`: bun.lockb migrations stay outside the #1155 + // upgrade path, so a moved default-registry tuple keeps + // its drift verdict here. super::bun_lock::revert_one_record( lines, rec, &entry.uuid, + false, &mut dirty, &mut outcome.warnings, ); diff --git a/crates/socket-patch-core/src/vendor/bun_lock.rs b/crates/socket-patch-core/src/vendor/bun_lock.rs index 283f0fb12..e9706f914 100644 --- a/crates/socket-patch-core/src/vendor/bun_lock.rs +++ b/crates/socket-patch-core/src/vendor/bun_lock.rs @@ -1029,10 +1029,23 @@ async fn revert_bun_wiring( } } + // An empty registry field means "the default registry", which a + // committed bunfig.toml or .npmrc can rebind (`registry`, a scope + // table); with either configuring one, an empty field proves nothing + // about where an upgrade installs from (#1155). + let default_registry_pinned = + !super::npm_common::project_may_redirect_registry(project_root).await; let mut dirty = false; if let Some(lines) = lines.as_mut() { for rec in entry.wiring.iter().rev().filter(|r| r.file == BUN_LOCK) { - revert_one_record(lines, rec, &entry.uuid, &mut dirty, &mut outcome.warnings); + revert_one_record( + lines, + rec, + &entry.uuid, + default_registry_pinned, + &mut dirty, + &mut outcome.warnings, + ); } if dirty && !dry_run { if let Err(e) = atomic_write_bytes_preserving_mode( @@ -1090,6 +1103,7 @@ pub(super) fn revert_one_record( lines: &mut [String], rec: &WiringRecord, entry_uuid: &str, + default_registry_pinned: bool, dirty: &mut bool, warnings: &mut Vec, ) { @@ -1141,6 +1155,22 @@ pub(super) fn revert_one_record( .and_then(|path| parse_vendor_path(&path)) .is_some_and(|p| p.eco == "npm" && p.uuid == entry_uuid); if !exact && !ours_uuid { + // MOVED OFF THE VENDORED VERSION, not drifted (#1155): `bun + // update` / `bun add pkg@other` re-locked the entry at another + // registry version, so the vendored version left the lock graph + // exactly as after `bun remove`. Nothing to restore; the caller + // keeps the artifact only while the lock still resolves through + // it. A same-version re-resolution stays drift. + if let Some(live_version) = version_moved_off(rec, &parsed, default_registry_pinned) { + warnings.push(VendorWarning::new( + super::LOCK_ENTRY_REMOVED_CODE, + format!( + "lock entry `{key}` now locks version {live_version}; the vendored \ + version is no longer installed, so there is nothing to restore" + ), + )); + return; + } warnings.push(drifted(format!( "lock entry `{key}` was re-resolved since vendoring; left alone" ))); @@ -1172,6 +1202,41 @@ pub(super) fn revert_one_record( )); } +/// The package name and registry version an entry tuple locks +/// (`"left-pad@1.3.1"` → `("left-pad", "1.3.1")`), or `None` for any other +/// spec (a vendored path, a URL, `file:`/`github:`). +fn registry_name_version(entry: &BunEntry) -> Option<(String, String)> { + let spec = decode_json_string(entry.elems.first()?)?; + let (name, version) = split_name_spec(&spec)?; + let plain = !version.is_empty() + && version + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '+')); + plain.then(|| (name.to_string(), version.to_string())) +} + +/// The live entry's registry version when the user moved the package to +/// another version from the same registry since vendoring: same package +/// name, same registry field (the tuple's second element) as the pre-vendor +/// tuple in `rec.original`, different version. `None` otherwise, which +/// keeps the caller's drift verdict: an `original: None` record, a URL or +/// `file:` spec, another package or another registry is not a plain upgrade +/// and must keep the artifact and leave `vendor --check` red. +fn version_moved_off( + rec: &WiringRecord, + live: &BunEntry, + default_registry_pinned: bool, +) -> Option { + let (live_name, live_version) = registry_name_version(live)?; + let original = parse_entry_line(rec.original.as_ref().and_then(Value::as_str)?).ok()?; + let (original_name, original_version) = registry_name_version(&original)?; + let live_registry = decode_json_string(live.elems.get(1)?)?; + let same_registry = Some(&live_registry) == decode_json_string(original.elems.get(1)?).as_ref() + && (!live_registry.is_empty() || default_registry_pinned); + (same_registry && live_name == original_name && live_version != original_version) + .then_some(live_version) +} + // ───────────────────────── vendor-specific classification ───────────────── // The conservative line grammar (`BunEntry`, `parse_*`, `scan_*`, …) lives in // `crate::vendor::bun_lock_text`; this module keeps only the vendor tuple @@ -3650,14 +3715,143 @@ mod tests { ); } + /// #1155: `bun update` / `bun add left-pad@1.3.1` moved the vendored + /// package off its patched version. The vendored version left the lock + /// graph exactly as after `bun remove`, so the revert has nothing to + /// restore and the artifact goes once nothing resolves through it, + /// instead of a drift-keep that `vendor --check` can never clear. + #[tokio::test] + async fn revert_after_version_change_drops_the_unreferenced_artifact() { + let fx = fixture_with(BN3_BEFORE_LOCK, "node_modules/left-pad").await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + + let upgraded_line = " \"left-pad\": [\"left-pad@1.3.1\", \"\", {}, \"sha512-other==\"],"; + let live = fx.read_lock().await; + let new_line = entry.wiring[0] + .new + .as_ref() + .and_then(Value::as_str) + .unwrap(); + let upgraded_lock = live.replace(new_line, upgraded_line); + assert_ne!(upgraded_lock, live, "test setup must move the entry"); + tokio::fs::write(fx.root().join(BUN_LOCK), &upgraded_lock) + .await + .unwrap(); + + let outcome = revert_bun(&entry, fx.root(), false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(!outcome.drift_skipped(), "{:?}", outcome.warnings); + assert!(!outcome.kept_artifact, "{:?}", outcome.warnings); + assert!( + outcome + .warnings + .iter() + .any(|w| w.code == "vendor_lock_entry_removed" + && w.detail.contains("left-pad") + && w.detail.contains("1.3.1")), + "the version change is surfaced: {:?}", + outcome.warnings + ); + assert_eq!( + fx.read_lock().await, + upgraded_lock, + "the user's upgraded lock is left byte-identical" + ); + assert!( + !fx.root() + .join(format!(".socket/vendor/npm/{UUID}")) + .exists(), + "nothing resolves through the artifact, so it is removed" + ); + } + + /// #1155 provenance guard: a version change counts as an upgrade only + /// for the same package from the same registry field as the pre-vendor + /// tuple. Another registry or another package name stays drift. + #[tokio::test] + async fn revert_keeps_version_change_from_another_registry_as_drift() { + for moved_line in [ + " \"left-pad\": [\"left-pad@1.3.1\", \"https://evil.example.com/\", {}, \"sha512-other==\"],", + " \"left-pad\": [\"not-left-pad@1.3.1\", \"\", {}, \"sha512-other==\"],", + ] { + let fx = fixture_with(BN3_BEFORE_LOCK, "node_modules/left-pad").await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + let live = fx.read_lock().await; + let new_line = entry.wiring[0] + .new + .as_ref() + .and_then(Value::as_str) + .unwrap(); + let moved_lock = live.replace(new_line, moved_line); + assert_ne!(moved_lock, live, "test setup must move the entry"); + tokio::fs::write(fx.root().join(BUN_LOCK), &moved_lock) + .await + .unwrap(); + + let outcome = revert_bun(&entry, fx.root(), false).await; + assert!(outcome.success, "{moved_line}: {:?}", outcome.error); + assert!(outcome.drift_skipped(), "{moved_line}: {:?}", outcome.warnings); + assert!(outcome.kept_artifact, "{moved_line}: {:?}", outcome.warnings); + assert_eq!(fx.read_lock().await, moved_lock, "left alone"); + } + } + + /// #1155 provenance guard: an empty registry field means "the default + /// registry", and a committed bunfig.toml or .npmrc can rebind that. + /// With either naming a registry, a version change from `""` is not + /// proven to come from the pre-vendor registry, so it stays drift. + #[tokio::test] + async fn revert_keeps_default_registry_upgrade_as_drift_when_the_registry_is_configured() { + for (file, text) in [ + ( + "bunfig.toml", + "[install]\nregistry = \"https://evil.example.com/\"\n", + ), + (".npmrc", "registry=https://evil.example.com/\n"), + ( + "bunfig.toml", + "[install.scopes]\n\"@s\" = \"https://evil.example.com/\"\n", + ), + ] { + let fx = fixture_with(BN3_BEFORE_LOCK, "node_modules/left-pad").await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + let upgraded_line = + " \"left-pad\": [\"left-pad@1.3.1\", \"\", {}, \"sha512-other==\"],"; + let live = fx.read_lock().await; + let new_line = entry.wiring[0] + .new + .as_ref() + .and_then(Value::as_str) + .unwrap(); + tokio::fs::write( + fx.root().join(BUN_LOCK), + live.replace(new_line, upgraded_line), + ) + .await + .unwrap(); + tokio::fs::write(fx.root().join(file), text).await.unwrap(); + + let outcome = revert_bun(&entry, fx.root(), false).await; + assert!(outcome.success, "{file}: {:?}", outcome.error); + assert!(outcome.drift_skipped(), "{file}: {:?}", outcome.warnings); + assert!(outcome.kept_artifact, "{file}: {:?}", outcome.warnings); + } + } + #[tokio::test] async fn revert_leaves_drifted_entries_alone_with_warning() { let fx = fixture_with(BN3_BEFORE_LOCK, "node_modules/left-pad").await; let (_, entry, _) = expect_done(fx.vendor(false).await); let entry = entry.unwrap(); - // The user re-resolved the entry behind our back (`bun update`). - let drifted_line = " \"left-pad\": [\"left-pad@1.3.1\", \"\", {}, \"sha512-other==\"],"; + // The user re-resolved the entry behind our back at the SAME + // version (a fork tarball). A version change is not drift (#1155); + // see `revert_after_version_change_drops_the_unreferenced_artifact`. + let drifted_line = + " \"left-pad\": [\"left-pad@https://example.com/left-pad-1.3.0.tgz\", {}],"; let live = fx.read_lock().await; let new_line = entry.wiring[0] .new diff --git a/crates/socket-patch-core/src/vendor/npm_common.rs b/crates/socket-patch-core/src/vendor/npm_common.rs index 0a82bf702..11e92e26a 100644 --- a/crates/socket-patch-core/src/vendor/npm_common.rs +++ b/crates/socket-patch-core/src/vendor/npm_common.rs @@ -67,6 +67,45 @@ pub(super) struct NpmCoords { /// vendor, arbitrary delete on revert) — reject fail-closed before any disk /// access. `Err` carries a ready [`VendorOutcome::Refused`] to bubble /// verbatim. +/// True when npm or Bun might fetch a registry package from somewhere other +/// than the URL its lock records, as far as the project and the process +/// environment can tell: the project's own `.npmrc` or `bunfig.toml` sets +/// anything at all (a registry, a scope table, a proxy, TLS settings, ...) +/// or can't be read, or `NPM_CONFIG_REGISTRY` / `npm_config_registry` / +/// `BUN_CONFIG_REGISTRY` is set. Deliberately coarse: a revert that would +/// delete a vendored copy because a version moved "within the same +/// registry" trusts that move only when this is false (#1155). User- and +/// machine-level config files (`~/.npmrc`, `~/.bunfig.toml`, the global +/// npmrc) are not read. +pub(super) async fn project_may_redirect_registry(project_root: &Path) -> bool { + let env_registry = [ + "NPM_CONFIG_REGISTRY", + "npm_config_registry", + "BUN_CONFIG_REGISTRY", + ] + .iter() + .any(|var| std::env::var_os(var).is_some_and(|v| !v.is_empty())); + if env_registry { + return true; + } + for name in [".npmrc", "bunfig.toml"] { + match crate::utils::fs::read_regular_to_string(&project_root.join(name)).await { + Ok(text) => { + let configures_something = text.lines().any(|line| { + let line = line.trim(); + !line.is_empty() && !line.starts_with('#') && !line.starts_with(';') + }); + if configures_something { + return true; + } + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(_) => return true, + } + } + false +} + pub(super) fn guard_coordinates( purl: &str, record: &PatchRecord, diff --git a/crates/socket-patch-core/src/vendor/npm_lock.rs b/crates/socket-patch-core/src/vendor/npm_lock.rs index d4e40749f..59f11b5c4 100644 --- a/crates/socket-patch-core/src/vendor/npm_lock.rs +++ b/crates/socket-patch-core/src/vendor/npm_lock.rs @@ -31,7 +31,9 @@ use super::npm_common::{ guard_revert_uuid_dir, vendor_npm_family, NpmCommit, NpmCoords, NpmLockBackend, NpmStagedPack, NpmVendorRequest, WireCx, }; -use super::npm_origin::{npm_non_registry_entries, npm_shrinkwrapped_entries, NpmOverrides}; +use super::npm_origin::{ + legacy_packages_key, npm_non_registry_entries, npm_shrinkwrapped_entries, NpmOverrides, +}; use super::parse_memo::ParseMemo; use super::path::parse_vendor_path; use super::source::PackageSource; @@ -745,6 +747,11 @@ pub async fn revert_npm_opts( } } + // An entry npm installs from a non-registry spec (git, URL, `file:`) is + // never a registry upgrade, whatever its `resolved` says (#326). + let overrides = NpmOverrides::read(project_root).await; + let upgrades_trusted = overrides.is_empty() + && !super::npm_common::project_may_redirect_registry(project_root).await; for lock_name in lock_files { let lock_path = project_root.join(lock_name); let lock_bytes = match read_regular_to_bytes(&lock_path).await { @@ -771,12 +778,32 @@ pub async fn revert_npm_opts( } }; + let mut non_registry = npm_non_registry_entries(&lock, &overrides); + // Any `overrides` rule (it can swap a registry edge, alias edges + // included, for a git / URL / `file:` spec npm ci installs instead + // of the lock's `resolved`) or a project `.npmrc` that can rebind + // the registry host keeps every record off the upgrade path; the + // revert then drift-keeps as before #1155. + if !upgrades_trusted { + for rec in entry.wiring.iter().filter(|r| r.file == lock_name) { + if let Some(key) = rec.key.as_deref() { + let key = match rec.kind.as_str() { + KIND_LOCK_LEGACY_ENTRY => legacy_pointer_packages_key(key), + _ => Some(key.to_string()), + }; + if let Some(key) = key { + non_registry.insert(key, "project config can redirect it".to_string()); + } + } + } + } let mut changed = false; // Reverse application order, like every backend's revert. for rec in entry.wiring.iter().rev().filter(|r| r.file == lock_name) { revert_one_record( &mut lock, rec, + &non_registry, &entry.uuid, &mut changed, &mut outcome.warnings, @@ -1152,12 +1179,109 @@ fn drifted_resolved_note(resolved: Option<&str>) -> String { } } +/// The live entry's `version` when the user moved the package to another +/// version from the same registry since vendoring: the version differs +/// from both the one we wired (`rec.new`) and the pre-vendor one +/// (`rec.original`), and `resolved` is the same package's tarball on the +/// registry the pre-vendor entry used (same `//-/` prefix). +/// `None` otherwise, which keeps the caller's drift verdict: a missing +/// version or pre-vendor `resolved`, or a resolution anywhere else (another +/// host, another package, a URL or `file:` spec) is not a plain upgrade +/// and must keep the artifact and leave `vendor --check` red. +fn version_moved_off<'a>(rec: &WiringRecord, live: &'a Value) -> Option<&'a str> { + let live_version = live.get("version").and_then(Value::as_str)?; + let recorded: Vec<&str> = [rec.new.as_ref(), rec.original.as_ref()] + .into_iter() + .flatten() + .filter_map(|v| v.get("version").and_then(Value::as_str)) + .collect(); + if recorded.is_empty() || recorded.contains(&live_version) { + return None; + } + let original_resolved = rec.original.as_ref()?.get("resolved")?.as_str()?; + let (original_scheme, prefix) = registry_tarball_prefix(original_resolved)?; + let live_resolved = live.get("resolved").and_then(Value::as_str)?; + let (live_scheme, live_rest) = split_http_scheme(live_resolved)?; + // npm rewrites an old lock's `http://` registry URLs to `https://` on + // the next install, so that upgrade is the same registry; a move from + // `https://` down to `http://` is not. + if live_scheme != original_scheme && (original_scheme, live_scheme) != ("http", "https") { + return None; + } + // The tarball file must be exactly `-.tgz`, with the + // basename taken from the pre-vendor tarball and a plain version, so + // no separator, `..`, `\`, query or fragment can steer npm's fetch to + // another package after it normalizes the URL. + let original_leaf = split_http_scheme(original_resolved)? + .1 + .strip_prefix(prefix)?; + let original_version = tarball_version( + rec.original + .as_ref()? + .get("version") + .and_then(Value::as_str)?, + ); + let basename = original_leaf.strip_suffix(&format!("-{original_version}.tgz"))?; + let tarball = tarball_version(live_version); + let plain_version = !tarball.is_empty() + && tarball + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '+')); + let leaf = live_rest.strip_prefix(prefix)?; + (plain_version && leaf == format!("{basename}-{tarball}.tgz")).then_some(live_version) +} + +/// The `packages` key a legacy `dependencies` wiring pointer mirrors +/// (`/dependencies/a/dependencies/b` → `node_modules/a/node_modules/b`), or +/// `None` for a pointer of any other shape. +fn legacy_pointer_packages_key(pointer: &str) -> Option { + let mut tokens = pointer.strip_prefix('/')?.split('/'); + let mut key = String::new(); + while let Some(field) = tokens.next() { + if field != "dependencies" { + return None; + } + let name = tokens.next()?.replace("~1", "/").replace("~0", "~"); + key = legacy_packages_key(&key, &name); + } + (!key.is_empty()).then_some(key) +} + +/// The version a lock `version` field names in its tarball file: itself, +/// or for a legacy (lockfile v1) alias row's `npm:left-pad@1.3.0` / +/// `npm:@scope/pkg@1.0.0` spelling, the part after the last `@`. +fn tarball_version(version: &str) -> &str { + match version.strip_prefix("npm:") { + Some(spec) => spec.rsplit_once('@').map_or(spec, |(_, v)| v), + None => version, + } +} + +/// `https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz` → +/// `("https", "registry.npmjs.org/left-pad/-/")`: the scheme and the +/// registry tarball directory of one package, which every version of it +/// shares. +fn registry_tarball_prefix(resolved: &str) -> Option<(&str, &str)> { + let (scheme, rest) = split_http_scheme(resolved)?; + let at = rest.rfind("/-/")?; + Some((scheme, &rest[..at + 3])) +} + +/// `https://host/path` → `("https", "host/path")`; `None` for anything but +/// an `http`/`https` URL. +fn split_http_scheme(url: &str) -> Option<(&str, &str)> { + ["https", "http"] + .into_iter() + .find_map(|scheme| Some((scheme, url.strip_prefix(scheme)?.strip_prefix("://")?))) +} + /// Apply one wiring record in reverse: restore `original` iff the live /// fragment is still ours (drift = third party re-resolved it; leave theirs /// alone, with a warning). fn revert_one_record( lock: &mut Value, rec: &WiringRecord, + non_registry: &BTreeMap, entry_uuid: &str, changed: &mut bool, warnings: &mut Vec, @@ -1216,6 +1340,28 @@ fn revert_one_record( None => false, }; if !ours { + // MOVED OFF THE VENDORED VERSION, not drifted (#1155): `npm install + // pkg@other` (or Dependabot) re-locked the entry at another + // version, so the vendored version left the lock graph exactly as + // after `npm uninstall`. Nothing to restore; the caller keeps the + // artifact only while a lock still resolves through it. A + // same-version re-resolution stays drift: re-vendoring can undo it. + let packages_key = match rec.kind.as_str() { + KIND_LOCK_LEGACY_ENTRY => legacy_pointer_packages_key(key), + _ => Some(key.to_string()), + }; + let registry_install = packages_key.is_some_and(|k| !non_registry.contains_key(&k)); + if let Some(live_version) = version_moved_off(rec, live).filter(|_| registry_install) { + warnings.push(VendorWarning::new( + super::LOCK_ENTRY_REMOVED_CODE, + format!( + "lock entry `{key}` now locks version {live_version} ({}); the vendored \ + version is no longer installed, so there is nothing to restore", + drifted_resolved_note(live_resolved) + ), + )); + return; + } warnings.push(VendorWarning::new( "vendor_lock_entry_drifted", format!( @@ -3875,6 +4021,434 @@ mod tests { ); } + /// #1155: the user moved the vendored package off its patched version + /// (`npm install left-pad@1.3.1`, or Dependabot doing the same), so + /// every recorded entry still exists but now resolves another version + /// from the registry. The vendored version left the lock graph just as + /// it does after `npm uninstall`: nothing to restore, and the artifact + /// goes once nothing resolves through it. Keeping it as drift would + /// leave `vendor --check` red with remedies that can never converge. + #[tokio::test] + async fn revert_after_version_change_drops_the_unreferenced_artifact() { + let fx = fixture().await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + + let upgraded = json!({ + "version": "1.3.1", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.1.tgz", + "integrity": "sha512-upgraded==" + }); + let mut live = fx.read_lock().await; + live["packages"]["node_modules/left-pad"] = upgraded.clone(); + live["packages"]["node_modules/foo/node_modules/left-pad"] = upgraded; + tokio::fs::write(fx.lock_path(), serialize_json(&live, " ").unwrap()) + .await + .unwrap(); + let after_upgrade = tokio::fs::read(fx.lock_path()).await.unwrap(); + + let outcome = revert_npm(&entry, fx.root(), false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(!outcome.drift_skipped(), "{:?}", outcome.warnings); + assert!(!outcome.kept_artifact, "{:?}", outcome.warnings); + assert!( + outcome + .warnings + .iter() + .any(|w| w.code == "vendor_lock_entry_removed" + && w.detail.contains("node_modules/left-pad") + && w.detail.contains("1.3.1")), + "the version change is surfaced: {:?}", + outcome.warnings + ); + assert!( + !fx.root() + .join(format!(".socket/vendor/npm/{UUID}")) + .exists(), + "nothing resolves through the artifact, so it is removed" + ); + assert_eq!( + tokio::fs::read(fx.lock_path()).await.unwrap(), + after_upgrade, + "the user's upgraded lock is left byte-identical" + ); + } + + /// #1155, downgrade of one instance: the direct copy moved to 1.2.0 + /// while the nested copy is still wired. The nested copy is restored, + /// the direct one is left as the user locked it, and the artifact goes + /// because the restore leaves nothing resolving through it. + #[tokio::test] + async fn revert_after_partial_downgrade_restores_the_rest_and_drops_artifact() { + let fx = fixture().await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + + let downgraded = json!({ + "version": "1.2.0", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.2.0.tgz", + "integrity": "sha512-older==" + }); + let mut live = fx.read_lock().await; + live["packages"]["node_modules/left-pad"] = downgraded.clone(); + tokio::fs::write(fx.lock_path(), serialize_json(&live, " ").unwrap()) + .await + .unwrap(); + + let outcome = revert_npm(&entry, fx.root(), false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(!outcome.drift_skipped(), "{:?}", outcome.warnings); + assert!(!outcome.kept_artifact, "{:?}", outcome.warnings); + let after = fx.read_lock().await; + assert_eq!(after["packages"]["node_modules/left-pad"], downgraded); + assert_eq!( + after["packages"]["node_modules/foo/node_modules/left-pad"], + default_lock()["packages"]["node_modules/foo/node_modules/left-pad"], + "the still-wired instance is restored" + ); + assert!(!fx + .root() + .join(format!(".socket/vendor/npm/{UUID}")) + .exists()); + } + + /// #1155 provenance guard: an edge npm installs from a git, URL or + /// `file:` spec is not a registry upgrade even when its `packages` entry + /// is written in the registry tarball shape: npm ci installs it from the + /// spec. It stays drift and the artifact is kept. + #[tokio::test] + async fn revert_keeps_version_change_behind_a_non_registry_spec_as_drift() { + for spec in [ + "github:stevemao/left-pad#v1.3.1", + "https://example.com/left-pad-1.3.1.tgz", + "file:../left-pad", + ] { + let fx = fixture().await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + + let forged = json!({ + "version": "1.3.1", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.1.tgz", + "integrity": "sha512-upgraded==" + }); + let mut live = fx.read_lock().await; + live["packages"][""]["dependencies"]["left-pad"] = json!(spec); + live["packages"]["node_modules/left-pad"] = forged.clone(); + live["packages"]["node_modules/foo/node_modules/left-pad"] = forged; + tokio::fs::write(fx.lock_path(), serialize_json(&live, " ").unwrap()) + .await + .unwrap(); + + let outcome = revert_npm(&entry, fx.root(), false).await; + assert!(outcome.success, "{spec}: {:?}", outcome.error); + assert!(outcome.drift_skipped(), "{spec}: {:?}", outcome.warnings); + assert!(outcome.kept_artifact, "{spec}: {:?}", outcome.warnings); + } + } + + /// #1155 provenance guard: an override can swap a registry edge (alias + /// edges included) for a git / URL / `file:` spec that npm ci installs + /// instead of the lock's `resolved`. While the project declares any + /// override, a version change is not trusted as a registry upgrade and + /// stays drift. + #[tokio::test] + async fn revert_keeps_version_change_as_drift_while_any_override_is_declared() { + for overrides in [ + json!({ "left-pad": "file:../left-pad" }), + json!({ "foo": { "left-pad": "github:evil/left-pad" } }), + json!({ "left-pad@1.3.0": "https://example.com/left-pad.tgz" }), + json!({ "foo": { "aliased": "git+file:///evil" } }), + ] { + let fx = fixture().await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + let manifest = json!({ "name": "fixture", "version": "1.0.0", "overrides": overrides }); + tokio::fs::write( + fx.root().join("package.json"), + serialize_json(&manifest, " ").unwrap(), + ) + .await + .unwrap(); + + let upgraded = json!({ + "version": "1.3.1", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.1.tgz", + "integrity": "sha512-upgraded==" + }); + let mut live = fx.read_lock().await; + live["packages"]["node_modules/left-pad"] = upgraded.clone(); + live["packages"]["node_modules/foo/node_modules/left-pad"] = upgraded; + tokio::fs::write(fx.lock_path(), serialize_json(&live, " ").unwrap()) + .await + .unwrap(); + + let outcome = revert_npm(&entry, fx.root(), false).await; + assert!(outcome.success, "{overrides}: {:?}", outcome.error); + assert!( + outcome.drift_skipped(), + "{overrides}: {:?}", + outcome.warnings + ); + assert!(outcome.kept_artifact, "{overrides}: {:?}", outcome.warnings); + } + } + + /// #1155 provenance guard: npm rewrites a `registry.npmjs.org` dist URL + /// to the project `.npmrc` registry at fetch time + /// (`replace-registry-host`), so with a registry configured there a + /// version move's recorded host proves nothing. It stays drift. + #[tokio::test] + async fn revert_keeps_version_change_as_drift_while_a_project_npmrc_sets_anything() { + for npmrc in [ + "registry=https://evil.example.com/\n", + "@s:registry=https://evil.example.com/\n", + "replace-registry-host=always\n", + "https-proxy=http://evil.example.com:8080\nstrict-ssl=false\n", + "cafile=./evil-ca.pem\n", + ] { + let fx = fixture().await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + tokio::fs::write(fx.root().join(".npmrc"), npmrc) + .await + .unwrap(); + let upgraded = json!({ + "version": "1.3.1", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.1.tgz", + "integrity": "sha512-upgraded==" + }); + let mut live = fx.read_lock().await; + live["packages"]["node_modules/left-pad"] = upgraded.clone(); + live["packages"]["node_modules/foo/node_modules/left-pad"] = upgraded; + tokio::fs::write(fx.lock_path(), serialize_json(&live, " ").unwrap()) + .await + .unwrap(); + + let outcome = revert_npm(&entry, fx.root(), false).await; + assert!(outcome.success, "{npmrc}: {:?}", outcome.error); + assert!(outcome.drift_skipped(), "{npmrc}: {:?}", outcome.warnings); + assert!(outcome.kept_artifact, "{npmrc}: {:?}", outcome.warnings); + } + } + + /// A project `.npmrc` that holds only comments or blank lines changes + /// nothing about where npm fetches, so the upgrade is still trusted. + #[tokio::test] + async fn revert_after_version_change_ignores_a_comment_only_npmrc() { + let fx = fixture().await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + tokio::fs::write(fx.root().join(".npmrc"), "# nothing here\n\n; nor here\n") + .await + .unwrap(); + let upgraded = json!({ + "version": "1.3.1", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.1.tgz", + "integrity": "sha512-upgraded==" + }); + let mut live = fx.read_lock().await; + live["packages"]["node_modules/left-pad"] = upgraded.clone(); + live["packages"]["node_modules/foo/node_modules/left-pad"] = upgraded; + tokio::fs::write(fx.lock_path(), serialize_json(&live, " ").unwrap()) + .await + .unwrap(); + + let outcome = revert_npm(&entry, fx.root(), false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(!outcome.drift_skipped(), "{:?}", outcome.warnings); + assert!(!outcome.kept_artifact, "{:?}", outcome.warnings); + } + + #[test] + fn legacy_pointer_maps_to_its_packages_key() { + assert_eq!( + legacy_pointer_packages_key("/dependencies/foo/dependencies/@s~1pad").as_deref(), + Some("node_modules/foo/node_modules/@s/pad") + ); + assert_eq!(legacy_pointer_packages_key("/packages/x"), None); + assert_eq!(legacy_pointer_packages_key("/dependencies"), None); + } + + /// #1155, old lock: npm 6 recorded `http://registry.npmjs.org/...` + /// tarball URLs, and the next `npm install` writes `https://`. That + /// upgrade is from the same registry, so it reverts like any other. + /// The reverse (an `https` entry moved to `http`) stays drift. + #[tokio::test] + async fn revert_after_version_change_accepts_http_to_https_upgrade_only() { + for (original, moved, reverts) in [ + ( + "http://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz", + "https://registry.npmjs.org/left-pad/-/left-pad-1.3.1.tgz", + true, + ), + ( + "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz", + "http://registry.npmjs.org/left-pad/-/left-pad-1.3.1.tgz", + false, + ), + ] { + let mut lock = default_lock(); + lock["packages"]["node_modules/left-pad"]["resolved"] = json!(original); + lock["packages"]["node_modules/foo/node_modules/left-pad"]["resolved"] = + json!(original); + let fx = fixture_with("left-pad", "1.3.0", lock).await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + + let upgraded = json!({ + "version": "1.3.1", + "resolved": moved, + "integrity": "sha512-upgraded==" + }); + let mut live = fx.read_lock().await; + live["packages"]["node_modules/left-pad"] = upgraded.clone(); + live["packages"]["node_modules/foo/node_modules/left-pad"] = upgraded; + tokio::fs::write(fx.lock_path(), serialize_json(&live, " ").unwrap()) + .await + .unwrap(); + + let outcome = revert_npm(&entry, fx.root(), false).await; + assert!(outcome.success, "{moved}: {:?}", outcome.error); + assert_eq!( + outcome.drift_skipped(), + !reverts, + "{moved}: {:?}", + outcome.warnings + ); + assert_eq!( + fx.root() + .join(format!(".socket/vendor/npm/{UUID}")) + .exists(), + !reverts, + "{moved}" + ); + } + } + + /// #1155, legacy alias: a lockfile v1 alias row spells its version + /// `npm:left-pad@1.3.0`. Upgrading the alias (`npm install + /// pad@npm:left-pad@1.3.1`) writes `npm:left-pad@1.3.1` with the 1.3.1 + /// registry tarball, which is the same plain upgrade. An alias pointed + /// at another package's tarball stays drift. + #[test] + fn version_moved_off_reads_legacy_alias_versions() { + let rec = WiringRecord { + file: PACKAGE_LOCK.to_string(), + kind: KIND_LOCK_LEGACY_ENTRY.to_string(), + action: WiringAction::Rewritten, + key: Some("/dependencies/pad".to_string()), + original: Some(json!({ + "version": "npm:left-pad@1.3.0", + "resolved": REG_RESOLVED, + "integrity": "sha512-orig==" + })), + new: Some(json!({ + "version": "npm:left-pad@1.3.0", + "resolved": format!("file:.socket/vendor/npm/{UUID}/left-pad-1.3.0.tgz"), + })), + }; + let upgraded = json!({ + "version": "npm:left-pad@1.3.1", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.1.tgz", + }); + assert_eq!( + version_moved_off(&rec, &upgraded), + Some("npm:left-pad@1.3.1") + ); + let elsewhere = json!({ + "version": "npm:left-pad@1.3.1", + "resolved": "https://registry.npmjs.org/left-pad/-/other-1.3.1.tgz", + }); + assert_eq!(version_moved_off(&rec, &elsewhere), None); + assert_eq!(tarball_version("npm:@scope/pkg@1.0.0"), "1.0.0"); + assert_eq!(tarball_version("1.0.0"), "1.0.0"); + } + + /// #1155 provenance guard: a version change is only an upgrade when the + /// new tarball is the same package on the registry the pre-vendor entry + /// used. A version change that resolves anywhere else (another host, + /// another package's tarball, a bare URL) is not something `npm + /// install pkg@x` writes, so it stays drift: the artifact is kept and + /// `vendor --check` stays red. + #[tokio::test] + async fn revert_keeps_version_change_resolved_off_the_recorded_registry_as_drift() { + for resolved in [ + "https://evil.example.com/left-pad/-/left-pad-1.3.1.tgz", + "https://registry.npmjs.org/not-left-pad/-/not-left-pad-1.3.1.tgz", + "https://example.com/left-pad-1.3.1.tgz", + "file:../left-pad-1.3.1.tgz", + "https://registry.npmjs.org/left-pad/-/..\\..\\not-left-pad\\-\\not-left-pad-1.3.1.tgz", + "https://registry.npmjs.org/left-pad/-/left-pad-1.3.1.tgz?x=/../../evil", + "https://registry.npmjs.org/left-pad/-/other-1.3.1.tgz", + ] { + let fx = fixture().await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + + let moved = json!({ + "version": "1.3.1", + "resolved": resolved, + "integrity": "sha512-upgraded==" + }); + let mut live = fx.read_lock().await; + live["packages"]["node_modules/left-pad"] = moved.clone(); + live["packages"]["node_modules/foo/node_modules/left-pad"] = moved; + tokio::fs::write(fx.lock_path(), serialize_json(&live, " ").unwrap()) + .await + .unwrap(); + + let outcome = revert_npm(&entry, fx.root(), false).await; + assert!(outcome.success, "{resolved}: {:?}", outcome.error); + assert!( + outcome.drift_skipped(), + "{resolved}: {:?}", + outcome.warnings + ); + assert!(outcome.kept_artifact, "{resolved}: {:?}", outcome.warnings); + assert!(fx + .root() + .join(format!(".socket/vendor/npm/{UUID}")) + .exists()); + } + } + + /// #1155 guard: the recorded entry moved to another version, but the + /// lock still resolves through the artifact under a key the wiring + /// never recorded. The artifact may be the only copy that install + /// needs, so it is kept. + #[tokio::test] + async fn revert_keeps_artifact_when_version_changed_but_uuid_still_referenced() { + let fx = fixture().await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + + let mut live = fx.read_lock().await; + let packages = live["packages"].as_object_mut().unwrap(); + let wired = packages["node_modules/left-pad"].clone(); + packages.insert("node_modules/bar/node_modules/left-pad".into(), wired); + packages.insert( + "node_modules/left-pad".into(), + json!({ + "version": "1.3.1", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.1.tgz", + "integrity": "sha512-upgraded==" + }), + ); + packages.remove("node_modules/foo/node_modules/left-pad"); + tokio::fs::write(fx.lock_path(), serialize_json(&live, " ").unwrap()) + .await + .unwrap(); + + let outcome = revert_npm(&entry, fx.root(), false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(outcome.kept_artifact, "{:?}", outcome.warnings); + assert!(fx + .root() + .join(format!(".socket/vendor/npm/{UUID}")) + .exists()); + } + /// #665 guard: a recorded entry vanished but the lock still resolves /// through the artifact under a key the wiring never recorded (npm /// re-hoisted it). The artifact may be the only copy that install diff --git a/crates/socket-patch-core/src/vendor/npm_origin.rs b/crates/socket-patch-core/src/vendor/npm_origin.rs index 51ef5ecf3..362bf57bd 100644 --- a/crates/socket-patch-core/src/vendor/npm_origin.rs +++ b/crates/socket-patch-core/src/vendor/npm_origin.rs @@ -89,6 +89,11 @@ impl NpmOverrides { } } + /// True when the root `package.json` declares no `overrides` rule. + pub(crate) fn is_empty(&self) -> bool { + self.rules.is_empty() + } + /// 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.