Merge pull request #287 from snyk/fix/OSM-2671/npm-nested-bundled-deps

fix: [OSM-2671] resolve bundled deps with non-hoisted bundle owner

Author: JamesPatrickGill

lib/dep-graph-builders/npm-lock-v2/index.ts

@@ -399,17 +399,34 @@ export const getChildNodeKey = (
     }
     return candidateKeys[0];
   }
-  // If we are bundled we assume we are scoped by the bundle root at least
-  // otherwise the ancestry root is the root ignoring the true root
+
+  // Always use the full ancestry (excluding the true root)
+  // The lockfile paths reflect the actual file system structure, which may or may not
+  // include all ancestors depending on hoisting
   const isBundled = ancestry[ancestry.length - 1].inBundle;
-  const rootOperatingIdx = isBundled
-    ? ancestry.findIndex((el) => el.inBundle === true) - 1
-    : 1;
   const ancestryFromRootOperatingIdx = [
-    ...ancestry.slice(rootOperatingIdx),
+    ...ancestry.slice(1), // Always start from index 1 (skip only the true root)
     { id: `${name}@${version}`, name, version },
   ];
 
+  // Find the bundle owner for the bundle root check below
+  let bundleOwnerName: string | undefined;
+  if (isBundled) {
+    const firstBundledIdx = ancestry.findIndex((el) => el.inBundle === true);
+    if (firstBundledIdx > 0) {
+      const potentialBundleOwner = ancestry[firstBundledIdx - 1];
+      if (potentialBundleOwner && potentialBundleOwner.key) {
+        const ownerPkg = pkgs[potentialBundleOwner.key];
+        if (
+          ownerPkg &&
+          (ownerPkg.bundledDependencies || ownerPkg.bundleDependencies)
+        ) {
+          bundleOwnerName = potentialBundleOwner.name;
+        }
+      }
+    }
+  }
+
   // We filter on a number of cases
   let filteredCandidates = candidateKeys.filter((candidate) => {
     // This is splitting the candidate that looks like
@@ -441,13 +458,13 @@ export const getChildNodeKey = (
       return false;
     }
 
-    // If we are bundled we assume the bundle root is the first value
-    // in the candidates scoping
-    if (isBundled && ancestryFromRootOperatingIdx[0].id !== ROOT_NODE_ID) {
-      const doesBundledPkgShareBundleRoot =
-        candidateAncestry[0] === ancestryFromRootOperatingIdx[0].name;
+    // If we are bundled, check if the candidate shares the same bundle owner
+    // The bundle owner should appear in the candidate path
+    if (isBundled && bundleOwnerName) {
+      const doesCandidateIncludeBundleOwner =
+        candidateAncestry.includes(bundleOwnerName);
 
-      if (doesBundledPkgShareBundleRoot === false) {
+      if (!doesCandidateIncludeBundleOwner) {
         return false;
       }
     }
@@ -504,7 +521,18 @@ export const getChildNodeKey = (
     filteredCandidates = possibleFilteredKeys;
   }
 
-  return undefined;
+  // If we still have multiple candidates, prefer the one with the most path segments
+  // (i.e., the deepest one, which is closest to the requesting package)
+  if (filteredCandidates.length > 1) {
+    filteredCandidates.sort((a, b) => {
+      const aDepth = a.split('/node_modules/').length;
+      const bDepth = b.split('/node_modules/').length;
+      return bDepth - aDepth; // Prefer deeper paths
+    });
+    return filteredCandidates[0];
+  }
+
+  return filteredCandidates.length === 1 ? filteredCandidates[0] : undefined;
 };
 
 const checkOverrides = (

test/jest/dep-graph-builders/fixtures/npm-lock-v2/nested-bundled-deps/README.md

@@ -0,0 +1,72 @@
+# Nested Bundled Dependencies Test Fixture
+
+⚠️ **This is a manually created synthetic test fixture.** The packages and versions are not real npm packages. This fixture was created specifically to test bundled dependency resolution.
+
+🚫 **DO NOT run `npm install` in this directory.** The package.json includes a preinstall script that will error if you attempt to install. The dependencies listed do not exist in the npm registry.
+
+This minimal fixture tests the resolution of bundled dependencies when the bundle owner is not hoisted to the root level.
+
+## Structure
+
+```
+test-app (root)
+└── @myorg/wrapper-tool (not hoisted)
+    └── builder-tool (has bundledDependencies)
+        ├── semver (bundled)
+        │   └── lru-cache (bundled, nested)
+        │       └── yallist (bundled, deeply nested)
+        └── chalk (bundled)
+            ├── ansi-styles (bundled, nested)
+            │   └── color-convert (bundled, deeply nested)
+            │       └── color-name (bundled, deeply nested)
+            └── supports-color (bundled, nested)
+                └── has-flag (bundled, deeply nested)
+```
+
+## What It Tests
+
+### 1. Non-Hoisted Bundle Owner
+
+Unlike packages hoisted to `node_modules/`, `builder-tool` is nested under `@myorg/wrapper-tool`. This tests that the algorithm correctly includes the non-bundled parent in the ancestry check.
+
+**Lockfile path**:
+
+```
+node_modules/@myorg/wrapper-tool/node_modules/builder-tool/node_modules/semver/node_modules/lru-cache
+```
+
+The `@myorg/wrapper-tool` part MUST be included in the ancestry check.
+
+### 2. Nested Bundled Dependencies
+
+Tests that bundled dependencies can have their own dependencies that are also bundled:
+
+- `builder-tool` declares bundled dependencies
+- `semver` (bundled) depends on `lru-cache` (also bundled)
+- `lru-cache` depends on `yallist` (also bundled)
+
+### 3. Multiple Bundled Dependency Trees
+
+The fixture includes two bundled dependency subtrees (`semver` and `chalk`) to verify the algorithm handles multiple branches correctly.
+
+### 4. Deep Nesting
+
+With 4-5 levels of nesting, this tests that the algorithm can handle deeply nested bundled structures without performance issues.
+
+## Key Algorithm Challenge
+
+When resolving `lru-cache` from within `semver`, the algorithm must:
+
+1. **Include full ancestry**: `[@myorg/wrapper-tool, builder-tool, semver, lru-cache]`
+2. **Validate bundle owner**: Verify `builder-tool` is in the candidate path
+3. **Handle lockfile path**: Match against `node_modules/@myorg/wrapper-tool/node_modules/builder-tool/node_modules/semver/node_modules/lru-cache`
+4. **Prefer correct candidate**: Choose the bundled one over any hoisted versions
+
+This tests the exact scenario that was failing before the fix.
+
+## Comparison to Other Fixtures
+
+- **goof**: Bundle owner hoisted to root (`node_modules/nyc/...`)
+- **nested-bundled-deps**: Bundle owner NOT hoisted (`node_modules/@myorg/wrapper-tool/node_modules/builder-tool/...`)
+
+Both patterns must work correctly to handle real-world package structures.