Skip to content

Commit 46b3bf7

Browse files
cursoragentcursoragent
committed
Fix: Check version when claiming npm lock edits in takeover revert
The revert_npm_redirect_purl function was claiming redirect_npm_lock_dep and redirect_npm_lock_entry edits by package name only, but the rewriter that recorded them matched both name AND version. This could cause reverting one version to incorrectly claim and replay edits for a different version of the same package (e.g., hoisted + nested versions). Changes: - Load disk_locks for both redirect_npm_lock_entry and redirect_npm_lock_dep - Add deps_contains_name_version helper to check dependencies tree recursively - Update redirect_npm_lock_dep claiming to verify version in dependencies tree - Update redirect_npm_lock_entry claiming to verify version in packages map This ensures version-scoped ownership matching the original rewriter logic.
1 parent b04ad42 commit 46b3bf7

4 files changed

Lines changed: 114 additions & 40 deletions

File tree

‎crates/socket-patch-core/src/patch/redirect/takeover.rs‎

Lines changed: 51 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -343,6 +343,28 @@ const NPM_TEXT_KINDS: [&str; 3] = [
343343
"redirect_pnpm_resolution",
344344
];
345345

346+
/// Check if the legacy npm v2 `dependencies` tree contains any entry with the
347+
/// given name and version (recursively).
348+
fn deps_contains_name_version(deps: &Value, name: &str, version: &str) -> bool {
349+
let Some(deps_obj) = deps.as_object() else {
350+
return false;
351+
};
352+
for (dep_name, entry) in deps_obj {
353+
if dep_name == name
354+
&& entry.get("version").and_then(Value::as_str) == Some(version)
355+
&& entry.get("bundled").and_then(Value::as_bool) != Some(true)
356+
{
357+
return true;
358+
}
359+
if let Some(nested) = entry.get("dependencies") {
360+
if deps_contains_name_version(nested, name, version) {
361+
return true;
362+
}
363+
}
364+
}
365+
false
366+
}
367+
346368
/// Revert every hosted-redirect edit the ledger records for `purl` (an npm
347369
/// package), then drop that purl's record and edits from `state`. The caller
348370
/// persists the mutated ledger (see `persist_redirect_state`).
@@ -369,14 +391,17 @@ pub async fn revert_npm_redirect_purl(
369391
let (name, version) = (name.to_string(), version.to_string());
370392
let lock_key = format!("{name}@{version}");
371393

372-
// The package-lock/shrinkwrap files any `redirect_npm_lock_entry` edits
373-
// touch, parsed once from disk: an ALIAS install (`npm i alias@npm:name`)
374-
// keys its entry by the alias, so ownership is resolved through the
375-
// entry's `name` field — exactly how the rewriter matched it (the rewrite
376-
// never touches name/version, so the probe is symmetric).
394+
// The package-lock/shrinkwrap files any `redirect_npm_lock_entry` or
395+
// `redirect_npm_lock_dep` edits touch, parsed once from disk: an ALIAS
396+
// install (`npm i alias@npm:name`) keys its entry by the alias, so
397+
// ownership is resolved through the entry's `name` field — exactly how
398+
// the rewriter matched it (the rewrite never touches name/version, so the
399+
// probe is symmetric).
377400
let mut disk_locks: BTreeMap<String, Option<Value>> = BTreeMap::new();
378401
for e in &state.edits {
379-
if e.kind == "redirect_npm_lock_entry" && !disk_locks.contains_key(&e.path) {
402+
if (e.kind == "redirect_npm_lock_entry" || e.kind == "redirect_npm_lock_dep")
403+
&& !disk_locks.contains_key(&e.path)
404+
{
380405
let parsed = read_rel(project_root, &e.path)
381406
.await?
382407
.and_then(|c| serde_json::from_str::<Value>(&c).ok());
@@ -386,30 +411,45 @@ pub async fn revert_npm_redirect_purl(
386411

387412
// Claim this purl's edits. Text-fragment kinds and the berry/classic/pnpm
388413
// rewriters key edits by `<name>@<version>`; the legacy npm v2
389-
// `dependencies` tree keys by bare name; the v3 `packages` map keys by
390-
// the lock path. A bun.lock edit that may belong to this purl is a hard
391-
// refusal: bun edits key by the lock's package key (not name@version)
414+
// `dependencies` tree and the v3 `packages` map also match by version
415+
// (not just bare name). A bun.lock edit that may belong to this purl is a
416+
// hard refusal: bun edits key by the lock's package key (not name@version)
392417
// and their revert is not implemented, so vendoring over one would drop
393418
// the record while stranding its edits — half a takeover.
394419
let mut mine: Vec<usize> = Vec::new();
395420
for (i, e) in state.edits.iter().enumerate() {
396421
let key = e.key.as_deref().unwrap_or_default();
397422
let claimed = match e.kind.as_str() {
398423
k if NPM_TEXT_KINDS.contains(&k) => key == lock_key,
399-
"redirect_npm_lock_dep" => key == name,
424+
"redirect_npm_lock_dep" => {
425+
key == name
426+
&& disk_locks
427+
.get(&e.path)
428+
.and_then(|l| l.as_ref())
429+
.and_then(|l| l.get("dependencies"))
430+
.is_some_and(|deps| deps_contains_name_version(deps, &name, &version))
431+
}
400432
"redirect_npm_lock_entry" => {
401433
let key_name = key
402434
.rsplit_once("node_modules/")
403435
.map(|(_, n)| n)
404436
.unwrap_or(key);
405-
key_name == name
437+
(key_name == name
406438
|| disk_locks
407439
.get(&e.path)
408440
.and_then(|l| l.as_ref())
409441
.and_then(|l| l.get("packages"))
410442
.and_then(|p| p.get(key))
411443
.is_some_and(|entry| {
412444
entry.get("name").and_then(Value::as_str) == Some(name.as_str())
445+
}))
446+
&& disk_locks
447+
.get(&e.path)
448+
.and_then(|l| l.as_ref())
449+
.and_then(|l| l.get("packages"))
450+
.and_then(|p| p.get(key))
451+
.is_some_and(|entry| {
452+
entry.get("version").and_then(Value::as_str) == Some(version.as_str())
413453
})
414454
}
415455
"redirect_bun_lock_package" => {

‎crates/socket-patch-core/src/vendor/cargo.rs‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1434,7 +1434,9 @@ mod tests {
14341434
"marker must survive"
14351435
);
14361436
assert_eq!(
1437-
tokio::fs::read(root.join(".cargo/config.toml")).await.unwrap(),
1437+
tokio::fs::read(root.join(".cargo/config.toml"))
1438+
.await
1439+
.unwrap(),
14381440
cfg1,
14391441
"config untouched"
14401442
);
@@ -1519,10 +1521,15 @@ mod tests {
15191521
let copy = dir.path().join("cfg-if-1.0.4");
15201522
let stage = stage_dir_for(&copy);
15211523
tokio::fs::create_dir_all(&stage).await.unwrap();
1522-
tokio::fs::write(stage.join("lib.rs"), b"new\n").await.unwrap();
1524+
tokio::fs::write(stage.join("lib.rs"), b"new\n")
1525+
.await
1526+
.unwrap();
15231527

15241528
swap_stage_into_place(&stage, &copy).await.unwrap();
1525-
assert_eq!(tokio::fs::read(copy.join("lib.rs")).await.unwrap(), b"new\n");
1529+
assert_eq!(
1530+
tokio::fs::read(copy.join("lib.rs")).await.unwrap(),
1531+
b"new\n"
1532+
);
15261533
assert!(!backup_dir_for(&copy).exists());
15271534
assert!(!stage.exists());
15281535
}
@@ -1576,7 +1583,9 @@ mod tests {
15761583
[[package]]\nname = \"cfg-if\"\nversion = \"1.0.4\"\nsource = \"{SOURCE}\"\nchecksum = \"{CHECKSUM}\"\n\n\
15771584
[[package]]\nname = \"cfg-if\"\nversion = \"1.0.4\"\nsource = \"git+https://example.com/fork/cfg-if#abcdef\"\n"
15781585
);
1579-
tokio::fs::write(root.join("Cargo.lock"), &lock).await.unwrap();
1586+
tokio::fs::write(root.join("Cargo.lock"), &lock)
1587+
.await
1588+
.unwrap();
15801589

15811590
let detail = expect_refused(
15821591
run_vendor(PURL, root, &blobs, &pristine, &record, false).await,
@@ -1603,7 +1612,9 @@ mod tests {
16031612
async fn test_refuses_user_entry_through_foreign_socket_dir() {
16041613
let (dir, blobs, pristine, record) = fixture().await;
16051614
let root = dir.path();
1606-
tokio::fs::create_dir_all(root.join(".cargo")).await.unwrap();
1615+
tokio::fs::create_dir_all(root.join(".cargo"))
1616+
.await
1617+
.unwrap();
16071618
let user_cfg = format!(
16081619
"[patch.crates-io]\ncfg-if = {{ path = \"../shared-fork/.socket/vendor/cargo/{UUID2}/cfg-if-1.0.4\" }}\n"
16091620
);

‎crates/socket-patch-core/src/vendor/cargo_config.rs‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -236,10 +236,12 @@ fn path_is_socket_owned(path: &str) -> bool {
236236
if segments.contains(&"..") {
237237
return false;
238238
}
239-
[CARGO_VENDOR_DIR, LEGACY_CARGO_PATCHES_DIR].iter().any(|dir| {
240-
let prefix: Vec<&str> = dir.split('/').collect();
241-
segments.len() > prefix.len() && segments[..prefix.len()] == prefix[..]
242-
})
239+
[CARGO_VENDOR_DIR, LEGACY_CARGO_PATCHES_DIR]
240+
.iter()
241+
.any(|dir| {
242+
let prefix: Vec<&str> = dir.split('/').collect();
243+
segments.len() > prefix.len() && segments[..prefix.len()] == prefix[..]
244+
})
243245
}
244246

245247
/// The `path` string of a `[patch]` entry (inline table or sub-table), if any.
@@ -384,9 +386,7 @@ mod tests {
384386
));
385387
// A `..` INSIDE the owned prefix escapes it.
386388
assert!(!path_is_socket_owned(".socket/vendor/cargo/../../../etc"));
387-
assert!(!path_is_socket_owned(
388-
".socket/cargo-patches/../../secrets"
389-
));
389+
assert!(!path_is_socket_owned(".socket/cargo-patches/../../secrets"));
390390
// A nested sub-checkout's socket dir is not THIS project's.
391391
assert!(!path_is_socket_owned("sub/.socket/vendor/cargo/u/x-1.0.0"));
392392
// The bare owned dir itself (no copy segment) is not an entry we write.

‎crates/socket-patch-core/src/vendor/pnpm_lock.rs‎

Lines changed: 40 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -194,8 +194,7 @@ pub async fn vendor_pnpm(
194194
if let Err(detail) = check_lock_override(&lines, name, version, &effective_key) {
195195
return refused("vendor_override_conflict", detail);
196196
}
197-
if let Err(detail) =
198-
check_workspace_override(ws_text.as_deref(), name, version, &effective_key)
197+
if let Err(detail) = check_workspace_override(ws_text.as_deref(), name, version, &effective_key)
199198
{
200199
return refused("vendor_override_conflict", detail);
201200
}
@@ -282,11 +281,11 @@ pub async fn vendor_pnpm(
282281
// The pnpm >= 11 override surface. Mirrors the package.json override
283282
// key-for-key so whichever surface the installed pnpm reads matches the
284283
// lock's `overrides:` section.
285-
let ws_edit = match apply_workspace_override(ws_text.as_deref(), &effective_key, &spec, &mut wiring)
286-
{
287-
Ok(edit) => edit,
288-
Err(e) => return done_failure(purl, format!("{PNPM_WORKSPACE} surgery failed: {e}")),
289-
};
284+
let ws_edit =
285+
match apply_workspace_override(ws_text.as_deref(), &effective_key, &spec, &mut wiring) {
286+
Ok(edit) => edit,
287+
Err(e) => return done_failure(purl, format!("{PNPM_WORKSPACE} surgery failed: {e}")),
288+
};
290289

291290
if !pkg_changed && !lock_changed && ws_edit.new_text.is_none() {
292291
// Everything already carries this uuid + the packed integrity: the
@@ -573,7 +572,10 @@ pub async fn revert_pnpm(entry: &VendorEntry, project_root: &Path, dry_run: bool
573572
.find(|r| r.file == PNPM_WORKSPACE && r.kind == KIND_WS_OVERRIDE)
574573
{
575574
let (created_file, created_overrides) = match &entry.pnpm {
576-
Some(meta) => (meta.created_workspace_file, meta.created_workspace_overrides),
575+
Some(meta) => (
576+
meta.created_workspace_file,
577+
meta.created_workspace_overrides,
578+
),
577579
None => (false, false),
578580
};
579581
if let Err(e) = revert_workspace(
@@ -688,7 +690,11 @@ fn revert_ws_record(
688690
}
689691
match rec.original.as_ref().and_then(Value::as_str) {
690692
Some(orig) => {
691-
lines[i] = format!("{}{}: {orig}", " ".repeat(indent), yaml_key_like(key, &repr));
693+
lines[i] = format!(
694+
"{}{}: {orig}",
695+
" ".repeat(indent),
696+
yaml_key_like(key, &repr)
697+
);
692698
}
693699
None => {
694700
lines.remove(i);
@@ -1326,7 +1332,10 @@ fn apply_workspace_override(
13261332
lines[i] = format!("{pad}{}: {spec}", yaml_key_like(our_key, &repr));
13271333
wiring.push(ws_record(our_key, spec, WiringAction::Rewritten, original));
13281334
} else {
1329-
lines.insert(last_entry + 1, format!("{pad}{}: {spec}", yaml_key(our_key)));
1335+
lines.insert(
1336+
last_entry + 1,
1337+
format!("{pad}{}: {spec}", yaml_key(our_key)),
1338+
);
13301339
wiring.push(ws_record(our_key, spec, WiringAction::Added, None));
13311340
}
13321341
return Ok(WorkspaceEdit {
@@ -1346,7 +1355,10 @@ fn apply_workspace_override(
13461355
.unwrap_or(lines.len());
13471356
lines.splice(
13481357
anchor..anchor,
1349-
["overrides:".to_string(), format!(" {}: {spec}", yaml_key(our_key))],
1358+
[
1359+
"overrides:".to_string(),
1360+
format!(" {}: {spec}", yaml_key(our_key)),
1361+
],
13501362
);
13511363
wiring.push(ws_record(our_key, spec, WiringAction::Added, None));
13521364
Ok(WorkspaceEdit {
@@ -4103,7 +4115,10 @@ snapshots:
41034115
#[tokio::test]
41044116
async fn workspace_file_is_created_with_root_scaffold_and_revert_deletes_it() {
41054117
let fx = fixture_with(P1_BEFORE_PKG, P1_BEFORE_LOCK).await;
4106-
assert!(!ws_exists(&fx).await, "fixture starts with no workspace file");
4118+
assert!(
4119+
!ws_exists(&fx).await,
4120+
"fixture starts with no workspace file"
4121+
);
41074122

41084123
let (_, entry, _) = expect_done(fx.vendor(false).await);
41094124
let entry = entry.unwrap();
@@ -4114,9 +4129,10 @@ snapshots:
41144129
"created workspace carries `packages: ['.']` + the override"
41154130
);
41164131
// The three surfaces agree on the same key → value (no config mismatch).
4117-
assert!(fx.read(PNPM_WORKSPACE).await.contains(&format!(
4118-
"overrides:\n left-pad@1.3.0: {spec}"
4119-
)));
4132+
assert!(fx
4133+
.read(PNPM_WORKSPACE)
4134+
.await
4135+
.contains(&format!("overrides:\n left-pad@1.3.0: {spec}")));
41204136
assert!(fx
41214137
.read(PNPM_LOCK)
41224138
.await
@@ -4240,7 +4256,10 @@ snapshots:
42404256
assert!(pnpm_meta.created_pnpm_table && pnpm_meta.created_overrides_table);
42414257
assert!(prev.wiring.iter().any(|r| r.file == PACKAGE_JSON));
42424258
assert!(prev.wiring.iter().any(|r| r.file == PNPM_LOCK));
4243-
assert!(!ws_exists(&fx).await, "downgraded state has no workspace file");
4259+
assert!(
4260+
!ws_exists(&fx).await,
4261+
"downgraded state has no workspace file"
4262+
);
42444263

42454264
// 2. Re-vendor under the current code: package.json + lock are already
42464265
// in sync, so ONLY the workspace mirror is written and the fresh
@@ -4267,7 +4286,11 @@ snapshots:
42674286
P1_BEFORE_PKG,
42684287
"package.json byte-restored"
42694288
);
4270-
assert_eq!(fx.read(PNPM_LOCK).await, P1_BEFORE_LOCK, "lock byte-restored");
4289+
assert_eq!(
4290+
fx.read(PNPM_LOCK).await,
4291+
P1_BEFORE_LOCK,
4292+
"lock byte-restored"
4293+
);
42714294
assert!(
42724295
!ws_exists(&fx).await,
42734296
"the workspace file the re-vendor created is deleted"

0 commit comments

Comments
 (0)