Skip to content

Commit 5dff221

Browse files
committed
Keep npm overrides that may not apply cautious
Two override shapes still let socket-patch treat a git or URL dependency as a registry install while npm ci kept fetching the original spec: - A "*" or empty override is a no-op in npm, which keeps the dependent's raw spec, but socket-patch read "*" as a registry range and patched the entry. - A rule nested under a range selector such as "pkga@^1" was ignored, so a broader top-level registry rule cleared the entry even though npm uses the narrower rule. A winning "*", empty or nested-only rule now leaves the raw spec in effect. A range selector that may apply, or a target selector other than the edge's own spec, makes the result unclear, so the copy stays reported as unpatched. Refs #490 Assisted-by: Claude Code:claude-opus-5-5
1 parent c657128 commit 5dff221

2 files changed

Lines changed: 237 additions & 13 deletions

File tree

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

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12284,6 +12284,59 @@ mod tests {
1228412284
/// the dependent's spec and ignores the lock's `resolved`, so rewiring
1228512285
/// that entry would report (and VEX-attest) a patch `npm ci` never
1228612286
/// installs. It must be skipped loudly, like a bundled copy.
12287+
#[test]
12288+
fn issue_490_unclear_overrides_leave_a_url_dependency_unredirected() {
12289+
// The review probes: npm keeps the URL spec for a `*` override, and
12290+
// picks the narrower rule under a range selector, so `npm ci` still
12291+
// fetches the URL. The rewriter must skip the entry loudly.
12292+
let url = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz";
12293+
let lock = json!({
12294+
"name": "app",
12295+
"lockfileVersion": 3,
12296+
"packages": {
12297+
"": { "name": "app", "version": "0.0.0", "dependencies": { "pkga": "^1.0.0" } },
12298+
"node_modules/pkga": {
12299+
"version": "1.0.0",
12300+
"resolved": "https://registry.npmjs.org/pkga/-/pkga-1.0.0.tgz",
12301+
"dependencies": { "left-pad": url }
12302+
},
12303+
"node_modules/left-pad": {
12304+
"version": "1.3.0",
12305+
"resolved": url,
12306+
"integrity": "sha512-UPSTREAM=="
12307+
}
12308+
}
12309+
});
12310+
let overrides = vec![npm_override(
12311+
"left-pad",
12312+
"1.3.0",
12313+
"http://patch.test/lp.tgz",
12314+
"sha512-PATCHED==",
12315+
)];
12316+
for manifest_overrides in [
12317+
json!({ "left-pad": "*" }),
12318+
json!({ "left-pad": "1.3.0", "pkga@^1": { "left-pad": url } }),
12319+
] {
12320+
let mut files = BTreeMap::new();
12321+
files.insert(
12322+
"package-lock.json".to_string(),
12323+
serde_json::to_string_pretty(&lock).unwrap(),
12324+
);
12325+
files.insert(
12326+
"package.json".to_string(),
12327+
json!({ "name": "app", "dependencies": { "pkga": "^1.0.0" }, "overrides": manifest_overrides })
12328+
.to_string(),
12329+
);
12330+
let r = rewrite_registry_redirect(&files, &overrides);
12331+
assert!(r.files.is_empty(), "{manifest_overrides}: {:?}", r.edits);
12332+
assert!(
12333+
warning_codes(&r).contains(&"redirect_npm_non_registry_entry_skipped"),
12334+
"{manifest_overrides}: {:?}",
12335+
r.warnings
12336+
);
12337+
}
12338+
}
12339+
1228712340
#[test]
1228812341
fn issue_490_a_git_edge_overridden_to_the_registry_is_redirected() {
1228912342
// `pkga` depends on left-pad from git; the project's `overrides`

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

Lines changed: 184 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,14 @@ impl NpmOverrides {
9898
/// the dependent or one of its physical ancestors (outermost first),
9999
/// with no selector or that package's exact version. As in npm, the
100100
/// rule scoped to the closest ancestor wins (then the most deeply
101-
/// nested one). Two equally close rules that disagree make the answer
102-
/// unclear, and so does a version-range selector: neither applies.
101+
/// nested one), and a winning `*` or empty value is a no-op that leaves
102+
/// the dependent's own spec in effect.
103+
///
104+
/// Anything this can't decide exactly makes the answer unclear, and
105+
/// then no override applies: two equally close rules that disagree, a
106+
/// target selector other than `spec`, or an enclosing selector that is
107+
/// a range (it may match the ancestor, so a rule beneath it may be the
108+
/// one npm picks).
103109
fn replacement(
104110
&self,
105111
packages: &Map<String, Value>,
@@ -111,9 +117,15 @@ impl NpmOverrides {
111117
return None;
112118
}
113119
let chain = dependent_chain(packages, from);
114-
let mut best: Option<BestRule> = None;
115-
self.visit(&self.rules, &chain, 0, 0, dep_name, spec, &mut best);
116-
best.filter(|b| !b.contested).map(|b| b.value)
120+
let mut search = RuleSearch::default();
121+
self.visit(&self.rules, &chain, 0, 0, dep_name, spec, &mut search);
122+
if search.unclear {
123+
return None;
124+
}
125+
let best = search.best.filter(|b| !b.contested)?;
126+
// npm ignores a `*` (or empty) replacement: the raw spec stays.
127+
let value = best.value.trim();
128+
(!value.is_empty() && value != "*").then(|| best.value)
117129
}
118130

119131
/// Search `rules` (nested `depth` levels deep; the enclosing rules
@@ -127,35 +139,67 @@ impl NpmOverrides {
127139
depth: usize,
128140
dep_name: &str,
129141
spec: &str,
130-
best: &mut Option<BestRule>,
142+
search: &mut RuleSearch,
131143
) {
132144
for (key, value) in rules {
133145
if key == "." {
134146
continue;
135147
}
136148
let (name, selector) = split_selector(key);
137149
// A rule for the edge's own package.
138-
if name == dep_name && selector.is_none_or(|sel| sel == spec) {
139-
if let Some(replacement) = self.rule_value(value) {
140-
BestRule::offer(best, (from_ix, depth), replacement);
150+
if name == dep_name {
151+
match selector {
152+
None => self.offer(value, (from_ix, depth), search),
153+
Some(sel) if sel == spec => self.offer(value, (from_ix, depth), search),
154+
// npm matches other selectors against the spec by
155+
// semver intersection, which isn't modelled here.
156+
Some(_) => search.unclear = true,
141157
}
142158
}
143159
// A rule scoped to a package on the dependent's chain: every
144160
// matching ancestor, so the closest one is ranked too.
145161
if let Some(children) = value.as_object() {
146162
for (ix, (anc_name, anc_version)) in chain.iter().enumerate().skip(from_ix) {
147-
let version_ok = match selector {
163+
if anc_name != name {
164+
continue;
165+
}
166+
let applies = match selector {
148167
None => true,
149-
Some(sel) => anc_version.as_deref() == Some(sel),
168+
Some(sel) if anc_version.as_deref() == Some(sel) => true,
169+
// Another exact version: the rule can't apply.
170+
Some(sel) if is_exact_version(sel) && anc_version.is_some() => false,
171+
// A range (or an unknown ancestor version): it may
172+
// apply, so a rule for the edge beneath it may be
173+
// the one npm picks.
174+
Some(_) => {
175+
if mentions(children, dep_name) {
176+
search.unclear = true;
177+
}
178+
false
179+
}
150180
};
151-
if anc_name == name && version_ok {
152-
self.visit(children, chain, ix + 1, depth + 1, dep_name, spec, best);
181+
if applies {
182+
self.visit(children, chain, ix + 1, depth + 1, dep_name, spec, search);
153183
}
154184
}
155185
}
156186
}
157187
}
158188

189+
/// Rank a rule for the edge; one whose value can't be resolved (a
190+
/// `$name` the root doesn't declare, a non-string) makes it unclear.
191+
fn offer(&self, value: &Value, rank: (usize, usize), search: &mut RuleSearch) {
192+
match self.rule_value(value) {
193+
Some(replacement) => BestRule::offer(&mut search.best, rank, replacement),
194+
// An object with only nested rules overrides nothing itself,
195+
// but still shadows a farther rule: a no-op, like `*`.
196+
None if value.as_object().is_some_and(|o| !o.contains_key(".")) => {
197+
BestRule::offer(&mut search.best, rank, String::new())
198+
}
199+
None => search.unclear = true,
200+
}
201+
}
202+
159203
/// A rule's replacement spec: the string itself, or an object's `"."`,
160204
/// with a `$name` reference resolved against the root's dependencies.
161205
fn rule_value(&self, value: &Value) -> Option<String> {
@@ -171,6 +215,39 @@ impl NpmOverrides {
171215
}
172216
}
173217

218+
/// What [`NpmOverrides::visit`] has found for one edge.
219+
#[derive(Debug, Default)]
220+
struct RuleSearch {
221+
best: Option<BestRule>,
222+
/// A rule that may apply couldn't be evaluated exactly.
223+
unclear: bool,
224+
}
225+
226+
/// Whether `rules` (at any depth) holds a rule keyed by `dep_name`.
227+
fn mentions(rules: &Map<String, Value>, dep_name: &str) -> bool {
228+
rules.iter().any(|(key, value)| {
229+
split_selector(key).0 == dep_name
230+
|| value
231+
.as_object()
232+
.is_some_and(|children| mentions(children, dep_name))
233+
})
234+
}
235+
236+
/// A plain `major.minor.patch` version, optionally with a prerelease or
237+
/// build suffix: never a range.
238+
fn is_exact_version(selector: &str) -> bool {
239+
let core_end = selector.find(['-', '+']).unwrap_or(selector.len());
240+
let (core, suffix) = selector.split_at(core_end);
241+
let parts: Vec<&str> = core.split('.').collect();
242+
parts.len() == 3
243+
&& parts
244+
.iter()
245+
.all(|p| !p.is_empty() && p.bytes().all(|b| b.is_ascii_digit()))
246+
&& suffix
247+
.bytes()
248+
.all(|b| b.is_ascii_alphanumeric() || matches!(b, b'-' | b'+' | b'.'))
249+
}
250+
174251
/// The winning override rule so far: ranked by how close its innermost
175252
/// enclosing ancestor is to the dependent (`chain` entries consumed), then
176253
/// by nesting depth. `contested` when an equally ranked rule disagrees.
@@ -745,6 +822,100 @@ mod tests {
745822
assert!(!found.contains_key("node_modules/left-pad"), "{found:?}");
746823
}
747824

825+
#[test]
826+
fn issue_490_wildcard_and_empty_overrides_leave_the_raw_spec() {
827+
// npm ignores a `*` (or empty) replacement, so the dependent's URL
828+
// or git spec stays in effect (npm 10.9.4 `edge.js`). Covered for a
829+
// remote-tarball spec and a git spec, with `"."` objects too.
830+
let url = REGISTRY_TGZ;
831+
let url_lock = lock(json!({
832+
"": { "dependencies": { "left-pad": url } },
833+
"node_modules/left-pad": { "version": "1.3.0", "resolved": url }
834+
}));
835+
let git_lock = overridden_git_lock(REGISTRY_TGZ);
836+
for value in [json!("*"), json!(""), json!(" * "), json!({ ".": "*" })] {
837+
for (lock, label) in [(&url_lock, "url"), (&git_lock, "git")] {
838+
let found =
839+
npm_non_registry_entries(lock, &manifest(json!({ "left-pad": value.clone() })));
840+
assert!(
841+
found.contains_key("node_modules/left-pad"),
842+
"{label} / {value}: {found:?}"
843+
);
844+
}
845+
}
846+
// A closer no-op shadows a farther registry rule, as in npm.
847+
let found = npm_non_registry_entries(
848+
&git_lock,
849+
&manifest(json!({ "left-pad": "1.3.0", "pkga": { "left-pad": "*" } })),
850+
);
851+
assert!(found.contains_key("node_modules/left-pad"), "{found:?}");
852+
let found = npm_non_registry_entries(
853+
&git_lock,
854+
&manifest(json!({
855+
"left-pad": "1.3.0",
856+
"pkga": { "left-pad": { "nested": "1.0.0" } }
857+
})),
858+
);
859+
assert!(found.contains_key("node_modules/left-pad"), "{found:?}");
860+
}
861+
862+
#[test]
863+
fn issue_490_range_selectors_that_may_apply_keep_the_edge() {
864+
let lock = overridden_git_lock(REGISTRY_TGZ);
865+
let tgz = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz";
866+
for scoped in ["pkga@^1", "pkga@1.x", "pkga@>=1", "pkga@*", "pkga@1"] {
867+
// npm picks the narrower rule under `pkga@^1` (here a URL) over
868+
// the top-level registry rule, so the broader one can't clear
869+
// the edge while the selector is unevaluated.
870+
let found = npm_non_registry_entries(
871+
&lock,
872+
&manifest(json!({ "left-pad": "1.3.0", scoped: { "left-pad": tgz } })),
873+
);
874+
assert!(
875+
found.contains_key("node_modules/left-pad"),
876+
"{scoped}: {found:?}"
877+
);
878+
}
879+
// A range selector whose subtree never names the dependency, and an
880+
// exact selector for another version, don't block the clear.
881+
for overrides in [
882+
json!({ "left-pad": "1.3.0", "pkga@^1": { "other": "1.0.0" } }),
883+
json!({ "left-pad": "1.3.0", "pkga@2.0.0": { "left-pad": tgz } }),
884+
] {
885+
let found = npm_non_registry_entries(&lock, &manifest(overrides.clone()));
886+
assert!(
887+
!found.contains_key("node_modules/left-pad"),
888+
"{overrides}: {found:?}"
889+
);
890+
}
891+
// A target selector other than the edge's own spec is unclear too.
892+
let found = npm_non_registry_entries(
893+
&lock,
894+
&manifest(json!({ "left-pad": "1.3.0", "left-pad@^1": "github:x/y" })),
895+
);
896+
assert!(found.contains_key("node_modules/left-pad"), "{found:?}");
897+
}
898+
899+
#[test]
900+
fn exact_versions_are_told_from_ranges() {
901+
for exact in ["1.0.0", "10.2.33", "1.0.0-beta.1", "1.0.0+build.5"] {
902+
assert!(is_exact_version(exact), "{exact}");
903+
}
904+
for range in [
905+
"^1",
906+
"1",
907+
"1.x",
908+
"1.0",
909+
"~1.0.0",
910+
">=1.0.0",
911+
"*",
912+
"1.0.0 || 2.0.0",
913+
"v1.0.0",
914+
] {
915+
assert!(!is_exact_version(range), "{range}");
916+
}
917+
}
918+
748919
#[test]
749920
fn issue_490_a_nested_dependent_matches_its_physical_ancestors() {
750921
// `@scope/b` under `a` depends on left-pad from git; the override is

0 commit comments

Comments
 (0)