Skip to content

Commit a454c8d

Browse files
committed
fix: Skip peer-only deps in npm lockfile resolve_package for workspace entries
When a workspace package has a peer-only dependency (declared in peerDependencies but not in dependencies/devDependencies/optionalDependencies), NpmLockfile::resolve_package now returns None for that dep. This prevents the transitive closure from resolving peer deps from the workspace package's path context, which picks the wrong version when multiple versions are hoisted differently. The consumer app already includes the correct version of the peer dep in its own transitive closure, so skipping it for intermediate workspace packages produces a valid pruned lockfile. Closes #10985
1 parent ba2a830 commit a454c8d

1 file changed

Lines changed: 257 additions & 2 deletions

File tree

  • crates/turborepo-lockfiles/src

crates/turborepo-lockfiles/src/npm.rs

Lines changed: 257 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,26 @@ impl Lockfile for NpmLockfile {
5353
name: &str,
5454
_version: &str,
5555
) -> Result<Option<Package>, Error> {
56-
if !self.packages.contains_key(workspace_path) {
57-
return Err(Error::MissingWorkspace(workspace_path.to_string()));
56+
let workspace_pkg = self
57+
.packages
58+
.get(workspace_path)
59+
.ok_or_else(|| Error::MissingWorkspace(workspace_path.to_string()))?;
60+
61+
// For workspace packages with a peer-only dependency (in
62+
// peerDependencies but not dependencies/devDependencies/
63+
// optionalDependencies): skip resolution when multiple versions of
64+
// the package exist in the lockfile. The workspace would resolve to
65+
// the hoisted version, but the consumer may need a different one.
66+
// When only one version exists, resolving is safe.
67+
// See https://github.com/vercel/turborepo/issues/10985
68+
if Self::is_workspace_entry(workspace_path)
69+
&& workspace_pkg.peer_dependencies.contains_key(name)
70+
&& !workspace_pkg.dependencies.contains_key(name)
71+
&& !workspace_pkg.dev_dependencies.contains_key(name)
72+
&& !workspace_pkg.optional_dependencies.contains_key(name)
73+
&& self.has_multiple_versions(name)
74+
{
75+
return Ok(None);
5876
}
5977

6078
let possible_keys = [
@@ -192,6 +210,34 @@ impl NpmLockfile {
192210
.ok_or_else(|| Error::MissingPackage(pkg_str.to_string()))
193211
}
194212

213+
/// Returns true if the key refers to a workspace package entry
214+
/// (e.g. "packages/components", "apps/web") rather than a node_modules
215+
/// entry or the root.
216+
fn is_workspace_entry(key: &str) -> bool {
217+
!key.is_empty() && !key.contains("node_modules/")
218+
}
219+
220+
/// Returns true if the lockfile contains more than one version of a
221+
/// package. For example, both `node_modules/next` (v14) and
222+
/// `apps/app-two/node_modules/next` (v15).
223+
fn has_multiple_versions(&self, name: &str) -> bool {
224+
let suffix = format!("node_modules/{name}");
225+
self.packages
226+
.keys()
227+
.filter(|key| {
228+
// Match `node_modules/{name}` or `*/node_modules/{name}` but
229+
// not `node_modules/{name}/node_modules/...` (nested deps of
230+
// the package itself).
231+
if let Some(before) = key.strip_suffix(suffix.as_str()) {
232+
before.is_empty() || before.ends_with('/')
233+
} else {
234+
false
235+
}
236+
})
237+
.count()
238+
> 1
239+
}
240+
195241
fn possible_npm_deps(key: &str, dep: &str) -> Vec<String> {
196242
let mut possible_deps = vec![format!("{key}/node_modules/{dep}")];
197243

@@ -315,6 +361,215 @@ mod test {
315361
}
316362
}
317363

364+
#[test]
365+
fn test_peer_dep_single_version_still_resolves() {
366+
// When only one version of a peer dep exists, resolve_package should
367+
// still return it — even for workspace entries with peer-only deps.
368+
// This is the npm-peer-dep scenario.
369+
let lockfile_json = r#"{
370+
"lockfileVersion": 3,
371+
"packages": {
372+
"": { "name": "monorepo", "workspaces": ["apps/*", "packages/*"] },
373+
"apps/web": {
374+
"version": "1.0.0",
375+
"dependencies": { "@repo/ui": "*", "react": "^18.0.0" }
376+
},
377+
"packages/ui": {
378+
"name": "@repo/ui",
379+
"version": "0.1.0",
380+
"peerDependencies": { "react": "^18.0.0" }
381+
},
382+
"node_modules/@repo/ui": { "resolved": "packages/ui", "link": true },
383+
"node_modules/react": { "version": "18.3.1" }
384+
}
385+
}"#;
386+
let lockfile = NpmLockfile::load(lockfile_json.as_bytes()).unwrap();
387+
388+
// packages/ui's peer dep react should resolve because there's only
389+
// one version in the lockfile.
390+
let result = lockfile
391+
.resolve_package("packages/ui", "react", "^18.0.0")
392+
.unwrap();
393+
assert!(
394+
result.is_some(),
395+
"single-version peer dep should resolve normally"
396+
);
397+
assert_eq!(result.unwrap().key, "node_modules/react");
398+
}
399+
400+
#[test]
401+
fn test_issue_10985_peer_dep_multi_version() {
402+
// Reproduces https://github.com/vercel/turborepo/issues/10985
403+
// When two apps depend on different versions of a package and a shared
404+
// workspace package has a peerDependency satisfying both, pruning should
405+
// not include the wrong version in the subgraph.
406+
let lockfile_json = r#"{
407+
"lockfileVersion": 3,
408+
"requires": true,
409+
"packages": {
410+
"": {
411+
"name": "monorepo",
412+
"workspaces": ["apps/*", "packages/*"]
413+
},
414+
"apps/app-one": {
415+
"version": "0.0.0",
416+
"dependencies": {
417+
"@repo/components": "*",
418+
"next": "^14.0.0"
419+
}
420+
},
421+
"apps/app-two": {
422+
"version": "0.0.0",
423+
"dependencies": {
424+
"@repo/components": "*",
425+
"next": "^15.0.0"
426+
}
427+
},
428+
"apps/app-two/node_modules/next": {
429+
"version": "15.0.0",
430+
"resolved": "https://registry.npmjs.org/next/-/next-15.0.0.tgz",
431+
"dependencies": { "next-dep": "2.0.0" }
432+
},
433+
"apps/app-two/node_modules/next/node_modules/next-dep": {
434+
"version": "2.0.0"
435+
},
436+
"node_modules/@repo/components": {
437+
"resolved": "packages/components",
438+
"link": true
439+
},
440+
"node_modules/app-one": {
441+
"resolved": "apps/app-one",
442+
"link": true
443+
},
444+
"node_modules/app-two": {
445+
"resolved": "apps/app-two",
446+
"link": true
447+
},
448+
"node_modules/next": {
449+
"version": "14.2.5",
450+
"resolved": "https://registry.npmjs.org/next/-/next-14.2.5.tgz",
451+
"dependencies": { "next-dep": "1.0.0" }
452+
},
453+
"node_modules/next/node_modules/next-dep": {
454+
"version": "1.0.0"
455+
},
456+
"node_modules/next-dep": {
457+
"version": "2.0.0"
458+
},
459+
"packages/components": {
460+
"name": "@repo/components",
461+
"version": "0.0.0",
462+
"peerDependencies": { "next": "^14 || ^15" }
463+
}
464+
}
465+
}"#;
466+
467+
let lockfile = NpmLockfile::load(lockfile_json.as_bytes()).unwrap();
468+
469+
// Compute transitive closure for app-one.
470+
// app-one depends on next@^14 and @repo/components.
471+
let app_one_deps = vec![
472+
("next".to_string(), "^14.0.0".to_string()),
473+
("@repo/components".to_string(), "*".to_string()),
474+
]
475+
.into_iter()
476+
.collect::<HashMap<_, _>>();
477+
let app_one_closure =
478+
crate::transitive_closure(&lockfile, "apps/app-one", app_one_deps, false).unwrap();
479+
let app_one_keys: std::collections::HashSet<_> =
480+
app_one_closure.iter().map(|p| p.key.as_str()).collect();
481+
482+
// app-one should get node_modules/next (v14) and its deps
483+
assert!(
484+
app_one_keys.contains("node_modules/next"),
485+
"app-one should include hoisted next@14"
486+
);
487+
assert!(
488+
app_one_keys.contains("node_modules/next/node_modules/next-dep"),
489+
"app-one should include next@14's nested dep"
490+
);
491+
// app-one should NOT get apps/app-two/node_modules/next (v15)
492+
assert!(
493+
!app_one_keys.contains("apps/app-two/node_modules/next"),
494+
"app-one should NOT include next@15"
495+
);
496+
497+
// Compute transitive closure for app-two.
498+
// app-two depends on next@^15 and @repo/components.
499+
let app_two_deps = vec![
500+
("next".to_string(), "^15.0.0".to_string()),
501+
("@repo/components".to_string(), "*".to_string()),
502+
]
503+
.into_iter()
504+
.collect::<HashMap<_, _>>();
505+
let app_two_closure =
506+
crate::transitive_closure(&lockfile, "apps/app-two", app_two_deps, false).unwrap();
507+
let app_two_keys: std::collections::HashSet<_> =
508+
app_two_closure.iter().map(|p| p.key.as_str()).collect();
509+
510+
// app-two should get apps/app-two/node_modules/next (v15) and its deps
511+
assert!(
512+
app_two_keys.contains("apps/app-two/node_modules/next"),
513+
"app-two should include nested next@15"
514+
);
515+
assert!(
516+
app_two_keys.contains("apps/app-two/node_modules/next/node_modules/next-dep"),
517+
"app-two should include next@15's nested dep"
518+
);
519+
520+
// packages/components has peerDependencies: { next: "^14 || ^15" }.
521+
// resolve_package now skips peer-only deps for workspace entries,
522+
// so the transitive closure is empty even when the peer dep is
523+
// passed as an unresolved dep.
524+
let components_deps = vec![("next".to_string(), "^14 || ^15".to_string())]
525+
.into_iter()
526+
.collect::<HashMap<_, _>>();
527+
let components_closure =
528+
crate::transitive_closure(&lockfile, "packages/components", components_deps, false)
529+
.unwrap();
530+
let components_keys: std::collections::HashSet<_> =
531+
components_closure.iter().map(|p| p.key.as_str()).collect();
532+
assert!(
533+
components_keys.is_empty(),
534+
"components peer-only deps should not resolve during transitive closure"
535+
);
536+
537+
// Simulate pruning for app-two: collect external deps from app-two + components
538+
let pruned_keys: std::collections::HashSet<_> =
539+
app_two_keys.union(&components_keys).cloned().collect();
540+
let pruned_key_strings: Vec<String> = pruned_keys.iter().map(|s| s.to_string()).collect();
541+
542+
// Create the subgraph for app-two
543+
let subgraph = lockfile
544+
.subgraph(
545+
&[
546+
"apps/app-two".to_string(),
547+
"packages/components".to_string(),
548+
],
549+
&pruned_key_strings,
550+
)
551+
.unwrap();
552+
553+
// The pruned lockfile should NOT contain node_modules/next (v14)
554+
// because app-two doesn't need it. Only app-one (which was pruned away) needs
555+
// v14. Including v14 causes npm ci to fail because the lockfile tree is
556+
// inconsistent.
557+
let encoded = subgraph.encode().unwrap();
558+
let pruned: serde_json::Value = serde_json::from_slice(&encoded).unwrap();
559+
let pruned_packages = pruned["packages"].as_object().unwrap();
560+
assert!(
561+
!pruned_packages.contains_key("node_modules/next"),
562+
"pruned lockfile for app-two should NOT contain node_modules/next (v14). The \
563+
@repo/components peer dep should not pull in the wrong version. Keys in subgraph: \
564+
{:?}",
565+
pruned_packages.keys().collect::<Vec<_>>()
566+
);
567+
assert!(
568+
!pruned_packages.contains_key("node_modules/next/node_modules/next-dep"),
569+
"pruned lockfile for app-two should NOT contain next@14's nested deps"
570+
);
571+
}
572+
318573
#[test]
319574
fn test_turbo_version_rejects_non_semver() {
320575
// Malicious version strings that could be used for RCE via npx should be

0 commit comments

Comments
 (0)