Skip to content

Commit 48085ce

Browse files
Fix npm VEX attesting a patch the twin lock lacks (#798) (#799)
* Start fix for #798 Assisted-by: Claude Code:claude-opus-5-5 * Refuse npm VEX when the twin lock lacks the pkg With both npm-shrinkwrap.json and package-lock.json committed, lockfile-only `vex` attested a patch that only one lock wired when the other lock had no entry for the package at all. npm 12 installs from package-lock.json and re-resolves a missing entry from the registry, so the checkout installed unpatched bytes while the VEX document said `not_affected`. A twin lock with no entry for the package now contests the wiring the same way a registry entry does (`patched_ref_unattributable`), in both directions and for hosted and vendored wiring. A twin that holds the package only at another version still contests nothing: npm installs that version, not unpatched bytes of the patched one. Fixes #798 Assisted-by: Claude Code:claude-opus-5-5 * Make the dual-lock read test use agreeing twins The test that proves both npm locks are read wired each package in only one lock. After #798 such a pair is contested (npm re-resolves the package missing from the other lock), so the fixture now has each lock wire both packages. It still proves both locks are read (4 refs) and that the v2 legacy mirror adds nothing. Refs #798 Assisted-by: Claude Code:claude-opus-5-5 * Keep patch refs out of a vex test's messages CodeQL flagged the new dual-lock test for printing the wired patch reference (which holds the patch uuid) in an assertion message. The message now names the wiring mode instead. Refs #798 Assisted-by: Claude Code:claude-opus-5-5 * Fix vex alias tests broken by store-copy merge #605 taught the name-keyed npm resolver to probe bundled store trees, so it now finds aliased copies (node_modules/lp) and a nested host's store peers itself. Two vex_consumed tests from #738 assumed that set never held aliases, so main's CI went red after both merged. The tests now feed the alias-free set explicitly to keep covering alias expansion, and also check the resolver's own set reaches the same copies with no duplicates. No production code changes. Assisted-by: Claude Code:claude-opus-5-5 (cherry picked from commit 40dac07) * Contest a twin npm lock that lacks the wired version A twin lock that held the package only at another version still let the wired ref through. npm keeps that entry only while it satisfies package.json, and otherwise fetches the wired version unpatched from the registry, so the lock alone can't vouch for it. The twin now contests unless it has an entry for the same name@version at any path. Lock pairs the rewriters keep in sync share that set, so they are unaffected. Refs #798 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TtZrsd52E6vxhvF9hpLVSw --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 75ba9e1 commit 48085ce

3 files changed

Lines changed: 237 additions & 11 deletions

File tree

‎crates/socket-patch-cli/tests/e2e_vex_lockfile/npm.rs‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -361,6 +361,40 @@ fn dual_lock_with_one_lock_on_the_registry_attests_nothing() {
361361
}
362362
}
363363

364+
/// REGRESSION (#798): the same holds when the other lock has NO entry for
365+
/// the package (a stale twin an npm <= 11 teammate left behind): npm
366+
/// re-resolves the missing entry from the registry, so whichever npm major
367+
/// reads that lock installs unpatched bytes. Nothing attests.
368+
#[test]
369+
fn dual_lock_with_one_lock_missing_the_package_attests_nothing() {
370+
let api = api_for(UUID, PURL);
371+
let stale = json!({
372+
"name": "app", "version": "1.0.0", "lockfileVersion": 3, "requires": true,
373+
"packages": { "": { "name": "app", "version": "1.0.0" } },
374+
});
375+
for (mode, wired) in [
376+
("hosted", hosted_url("patch.socket.dev", UUID)),
377+
("vendored", format!("file:{}", vendored_rel(UUID))),
378+
] {
379+
for stale_file in ["package-lock.json", "npm-shrinkwrap.json"] {
380+
let tmp = tempfile::tempdir().unwrap();
381+
let p = tmp.path();
382+
write_locks(p, Shape::Dual, &wired, PIN);
383+
std::fs::write(p.join(stale_file), stale.to_string()).unwrap();
384+
write_artifact(p, UUID, PATCHED);
385+
let out = run_vex(&binary(), p, &VexRun::online(&api));
386+
assert_ne!(out.code, Some(0), "{mode}, stale {stale_file}:\n{out}");
387+
assert_absent(out.doc.as_ref(), PURL);
388+
let text = out.stdout.clone() + &out.stderr;
389+
assert!(
390+
text.contains("patched_ref_unattributable")
391+
&& text.contains("has no entry for the package"),
392+
"the contested wiring must be explained:\n{out}"
393+
);
394+
}
395+
}
396+
}
397+
364398
/// Standalone `vex` over a manifest-less checkout honors the output
365399
/// conventions every `vex` run does: `-O -` prints the document to stdout
366400
/// (never a file named `-`) with the summary on stderr, and `--dry-run`

‎crates/socket-patch-core/src/vex/discover/npm.rs‎

Lines changed: 199 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -72,13 +72,30 @@ pub(crate) async fn extract(ctx: &DiscoverCtx<'_>, out: &mut Discovery) {
7272
}
7373

7474
/// What one parsed npm lock wires, plus the packages it resolves ELSEWHERE
75-
/// (an entry whose `resolved` is not a Socket reference) and the packages it
76-
/// installs BUNDLED (each purl → the first such entry's lock location).
75+
/// (an entry whose `resolved` is not a Socket reference), the packages it
76+
/// installs BUNDLED (each purl → the first such entry's lock location) and
77+
/// every `name@version` it has any entry for, at any path.
7778
struct NpmLockRefs {
7879
file: &'static str,
7980
refs: Vec<PatchedRef>,
8081
unwired: BTreeMap<String, String>,
8182
bundled: BTreeMap<String, String>,
83+
mentioned: BTreeSet<String>,
84+
}
85+
86+
impl NpmLockRefs {
87+
/// Remember that this lock has an entry for `name@version`. An entry
88+
/// without a version vouches for no version.
89+
fn mention(&mut self, name: &str, version: Option<&str>) {
90+
if let Some(purl) = version.and_then(|v| npm_purl(name, v)) {
91+
self.mentioned.insert(purl);
92+
}
93+
}
94+
95+
/// Whether this lock has an entry for exactly `purl`'s `name@version`.
96+
fn mentions(&self, purl: &str) -> bool {
97+
self.mentioned.contains(purl)
98+
}
8299
}
83100

84101
/// Push every ref no OTHER npm lock contests. npm <= 11 installs from
@@ -89,9 +106,16 @@ struct NpmLockRefs {
89106
/// patched by some npm majors and unpatched by others — not decidable from
90107
/// the files, so it is diagnosed and not attested (the same call as the v2
91108
/// legacy mirror: a lock section some npm reads must not attest bytes
92-
/// another npm installs). The lock that does not mention the package at all
93-
/// contests nothing. Two locks wiring DIFFERENT patches are both emitted
94-
/// (the CLI's `wiring_conflict` gate).
109+
/// another npm installs). A lock with NO entry for the ref's
110+
/// `name@version` contests it too (#798): npm re-resolves a missing entry
111+
/// from the registry (npm 12 with a stale package-lock.json twin, npm <= 11
112+
/// with a stale shrinkwrap). An entry for another version holds only while
113+
/// that version still satisfies `package.json`, which the lock alone cannot
114+
/// tell, so a lock holding the package only at other versions (at any
115+
/// path) contests it as well. Twins the rewriters keep in sync share their
116+
/// `name@version` set and contest nothing. Two
117+
/// locks wiring DIFFERENT patches are both emitted (the CLI's
118+
/// `wiring_conflict` gate).
95119
///
96120
/// A bundled copy of the ref's `name@version` in either lock contests it
97121
/// too, the same lock included (#325): the rewired entry and the bundled
@@ -159,6 +183,25 @@ fn push_uncontested(locks: Vec<NpmLockRefs>, out: &mut Discovery) {
159183
lock.file, r.purl, r.uuid, other.file, NPM_LOCKS[0], NPM_LOCKS[1],
160184
),
161185
);
186+
} else if let Some(other) = locks
187+
.iter()
188+
.enumerate()
189+
.find(|(j, other)| *j != i && !other.mentions(&r.purl))
190+
.map(|(_, other)| other)
191+
{
192+
out.diag(
193+
DIAG_REF_UNATTRIBUTABLE,
194+
lock.file,
195+
format!(
196+
"{}: {} is wired to Socket patch {} but {} has no entry for the \
197+
package at that version, so npm can re-resolve it from the registry \
198+
when it installs from that lock — npm <= 11 installs from {}, npm >= 12 from {}, so \
199+
whether the patched bytes install depends on the npm version; \
200+
rewire both locks (re-run `socket-patch vendor` / `scan --mode \
201+
hosted`) to attest it",
202+
lock.file, r.purl, r.uuid, other.file, NPM_LOCKS[0], NPM_LOCKS[1],
203+
),
204+
);
162205
} else {
163206
out.push(r.clone());
164207
}
@@ -180,6 +223,7 @@ async fn extract_package_lock(
180223
refs: Vec::new(),
181224
unwired: BTreeMap::new(),
182225
bundled: BTreeMap::new(),
226+
mentioned: BTreeSet::new(),
183227
};
184228
let doc: Value = match parse_json(file, &bytes) {
185229
Ok(doc) => doc,
@@ -203,6 +247,7 @@ async fn extract_package_lock(
203247
// the same version, here ([`push_uncontested`]) and in any other lock
204248
// (the orchestrator).
205249
for (location, node) in npm_lock_bundled_nodes(&doc) {
250+
read.mention(node.name, node.version);
206251
if let Some(purl) = node.version.and_then(|v| npm_purl(node.name, v)) {
207252
out.resolved_elsewhere(file, Some(purl.clone()));
208253
read.bundled.entry(purl).or_insert(location);
@@ -251,6 +296,7 @@ fn drop_non_registry_installs(
251296
else {
252297
continue;
253298
};
299+
read.mentioned.insert(purl.clone());
254300
out.resolved_elsewhere(file, Some(purl.clone()));
255301
read.unwired
256302
.entry(purl.clone())
@@ -286,6 +332,7 @@ fn entry_ref(
286332
out: &mut Discovery,
287333
) {
288334
let name = node.name;
335+
read.mention(name, node.version);
289336
let resolved = node.resolved;
290337
let Located {
291338
vendored,
@@ -753,8 +800,10 @@ mod tests {
753800
}
754801

755802
/// Both npm locks are read (npm 12 installs from package-lock.json beside a
756-
/// committed shrinkwrap); a v2 lock's legacy `dependencies` mirror is
757-
/// not (its agreeing twin adds nothing).
803+
/// committed shrinkwrap), each yielding its own refs; a v2 lock's legacy
804+
/// `dependencies` mirror is not (its agreeing twin adds nothing). The
805+
/// twins agree: a lock with no entry for a package would contest it
806+
/// (#798).
758807
#[tokio::test]
759808
async fn both_locks_are_read_and_the_agreeing_mirror_adds_nothing() {
760809
let a = hosted_url("npm", "left-pad", "1.3.0", UUID_A, "left-pad-1.3.0.tgz");
@@ -764,13 +813,15 @@ mod tests {
764813
"npm-shrinkwrap.json",
765814
lock_with_packages(serde_json::json!({
766815
"node_modules/left-pad": { "version": "1.3.0", "resolved": a, "integrity": SRI },
816+
"node_modules/minimist": { "version": "1.2.5", "resolved": b, "integrity": SRI },
767817
})),
768818
);
769819
p.write(
770820
"package-lock.json",
771821
serde_json::json!({
772822
"lockfileVersion": 2,
773823
"packages": {
824+
"node_modules/left-pad": { "version": "1.3.0", "resolved": a, "integrity": SRI },
774825
"node_modules/minimist": { "version": "1.2.5", "resolved": b, "integrity": SRI },
775826
},
776827
"dependencies": {
@@ -796,8 +847,9 @@ mod tests {
796847
);
797848
assert_eq!(
798849
out.refs.len(),
799-
2,
800-
"the v2 mirror and its nested legacy copy add nothing: {:#?}",
850+
4,
851+
"one ref per package from each lock; the v2 mirror and its nested \
852+
legacy copy add nothing: {:#?}",
801853
out.refs
802854
);
803855
}
@@ -863,7 +915,7 @@ mod tests {
863915

864916
/// The agreeing dual-lock state (both locks wired identically — what the
865917
/// hosted rewriter and the vendor backend now write) is a ref from each
866-
/// lock; a lock that does not mention the package contests nothing.
918+
/// lock.
867919
#[tokio::test]
868920
async fn agreeing_dual_npm_locks_both_wire_the_patch() {
869921
let hosted = hosted_url("npm", "left-pad", "1.3.0", UUID_A, "left-pad-1.3.0.tgz");
@@ -882,6 +934,143 @@ mod tests {
882934
assert!(out.diagnostics.is_empty(), "{:?}", out.diagnostics);
883935
}
884936

937+
/// REGRESSION (#798): a twin npm lock with NO entry for the package
938+
/// contests it too. npm 12 reads package-lock.json beside a committed
939+
/// shrinkwrap and re-resolves the missing entry from the registry, so
940+
/// the bytes it installs are unpatched, in either direction, hosted or
941+
/// vendored. The uuids stay recognized (a ledger claim is dead too).
942+
#[tokio::test]
943+
async fn a_sibling_npm_lock_without_the_package_contests_it() {
944+
let hosted = hosted_url("npm", "left-pad", "1.3.0", UUID_A, "left-pad-1.3.0.tgz");
945+
let vendored = format!("file:.socket/vendor/npm/{UUID_B}/minimist-1.2.5.tgz");
946+
let registry = |n: &str, v: &str| format!("https://registry.npmjs.org/{n}/-/{n}-{v}.tgz");
947+
let p = Project::new();
948+
// Shrinkwrap wires left-pad (hosted); package-lock has no left-pad.
949+
// package-lock wires minimist (vendored); shrinkwrap has no minimist.
950+
p.write(
951+
"npm-shrinkwrap.json",
952+
lock_with_packages(serde_json::json!({
953+
"node_modules/left-pad": { "version": "1.3.0", "resolved": hosted, "integrity": SRI },
954+
"node_modules/other": { "version": "1.0.0", "resolved": registry("other", "1.0.0"), "integrity": SRI },
955+
})),
956+
);
957+
p.write(
958+
"package-lock.json",
959+
lock_with_packages(serde_json::json!({
960+
"node_modules/minimist": { "version": "1.2.5", "resolved": vendored, "integrity": SRI },
961+
})),
962+
);
963+
let out = run(&p).await;
964+
assert!(
965+
out.refs.is_empty(),
966+
"contested refs emitted: {:#?}",
967+
out.refs
968+
);
969+
let contested: Vec<&Diag> = out
970+
.diagnostics
971+
.iter()
972+
.filter(|d| d.code == DIAG_REF_UNATTRIBUTABLE)
973+
.collect();
974+
assert_eq!(contested.len(), 2, "{:#?}", out.diagnostics);
975+
assert!(contested
976+
.iter()
977+
.any(|d| d.file == std::path::Path::new("npm-shrinkwrap.json")
978+
&& d.detail.contains("pkg:npm/left-pad@1.3.0")
979+
&& d.detail.contains("no entry")
980+
&& d.detail.contains("npm >= 12")));
981+
assert!(contested
982+
.iter()
983+
.any(|d| d.file == std::path::Path::new("package-lock.json")
984+
&& d.detail.contains("pkg:npm/minimist@1.2.5")
985+
&& d.detail.contains("no entry")));
986+
for (label, uuid) in [("UUID_A", UUID_A), ("UUID_B", UUID_B)] {
987+
assert!(
988+
out.recognized.iter().any(|r| r.uuid == uuid),
989+
"{label} must stay recognized (authoritative, dead): {:#?}",
990+
out.recognized
991+
);
992+
}
993+
}
994+
995+
/// REGRESSION (#798 review): a sibling npm lock holding the wired
996+
/// package only at ANOTHER version contests it as well. npm keeps that
997+
/// entry only while it satisfies `package.json` (`^1.3.0` here rejects
998+
/// 1.2.0), and otherwise fetches the wired version unpatched from the
999+
/// registry. Covers a top-level and a nested wired copy.
1000+
#[tokio::test]
1001+
async fn a_sibling_npm_lock_with_only_another_version_contests_it() {
1002+
let hosted = hosted_url("npm", "left-pad", "1.3.0", UUID_A, "left-pad-1.3.0.tgz");
1003+
let registry = |n: &str, v: &str| format!("https://registry.npmjs.org/{n}/-/{n}-{v}.tgz");
1004+
for wired_at in [
1005+
"node_modules/left-pad",
1006+
"node_modules/foo/node_modules/left-pad",
1007+
] {
1008+
let p = Project::new();
1009+
p.write(
1010+
"package.json",
1011+
serde_json::json!({
1012+
"name": "app", "version": "1.0.0",
1013+
"dependencies": { "left-pad": "^1.3.0" },
1014+
})
1015+
.to_string(),
1016+
);
1017+
p.write(
1018+
"npm-shrinkwrap.json",
1019+
lock_with_packages(serde_json::json!({
1020+
wired_at: { "version": "1.3.0", "resolved": hosted, "integrity": SRI },
1021+
})),
1022+
);
1023+
p.write(
1024+
"package-lock.json",
1025+
lock_with_packages(serde_json::json!({
1026+
"node_modules/left-pad": { "version": "1.2.0", "resolved": registry("left-pad", "1.2.0"), "integrity": SRI },
1027+
})),
1028+
);
1029+
let out = run(&p).await;
1030+
assert!(out.refs.is_empty(), "{wired_at}: {:#?}", out.refs);
1031+
assert_eq!(
1032+
diag_codes(&out),
1033+
vec![DIAG_REF_UNATTRIBUTABLE],
1034+
"{wired_at}: {:#?}",
1035+
out.diagnostics
1036+
);
1037+
assert!(
1038+
out.diagnostics[0]
1039+
.detail
1040+
.contains("has no entry for the package at that version"),
1041+
"{:#?}",
1042+
out.diagnostics
1043+
);
1044+
}
1045+
}
1046+
1047+
/// The same `name@version` at a different path in the sibling lock
1048+
/// vouches for the wired one (the lock pair still agrees on what
1049+
/// installs).
1050+
#[tokio::test]
1051+
async fn a_sibling_npm_lock_with_the_version_at_another_path_contests_nothing() {
1052+
let hosted = hosted_url("npm", "left-pad", "1.3.0", UUID_A, "left-pad-1.3.0.tgz");
1053+
let p = Project::new();
1054+
p.write(
1055+
"npm-shrinkwrap.json",
1056+
lock_with_packages(serde_json::json!({
1057+
"node_modules/left-pad": { "version": "1.3.0", "resolved": hosted, "integrity": SRI },
1058+
})),
1059+
);
1060+
p.write(
1061+
"package-lock.json",
1062+
lock_with_packages(serde_json::json!({
1063+
"node_modules/foo/node_modules/left-pad": { "version": "1.3.0", "resolved": hosted, "integrity": SRI },
1064+
})),
1065+
);
1066+
let out = run(&p).await;
1067+
assert_refs(
1068+
&out,
1069+
&[("pkg:npm/left-pad@1.3.0", UUID_A, WiringMode::Hosted)],
1070+
);
1071+
assert!(out.diagnostics.is_empty(), "{:?}", out.diagnostics);
1072+
}
1073+
8851074
/// REGRESSION: a v2 lock whose `packages` entry was reverted to the
8861075
/// registry while its stale legacy `dependencies` mirror still carries
8871076
/// the Socket url wires NOTHING — npm >= 7 installs from `packages`, so

‎docs/testing/npm-compatibility.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,10 @@ npm 12 notes:
5151
both present, npm 12 installs from package-lock.json while npm <= 11 installs
5252
from the shrinkwrap — so hosted and vendored rewrites wire BOTH, and
5353
manifest-less VEX refuses to attest a package one lock wires while the other
54-
still resolves it from the registry (`patched_ref_unattributable`).
54+
still resolves it from the registry or has no entry for that `name@version`
55+
(missing, or only another version that may no longer satisfy
56+
`package.json`), which npm can re-resolve from the registry
57+
(`patched_ref_unattributable`).
5558
- `allow-file` defaults to `all`: vendored `file:` tarballs install unchanged.
5659

5760
## Suites

0 commit comments

Comments
 (0)