Skip to content

Update npm lockfile 3 ancestor traversal to properly detect dependency roots #1677

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
FernandoRojo wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update npm lockfile 3 ancestor traversal to properly detect dependency roots #1677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 13 additions & 18 deletions src/Microsoft.ComponentDetection.Detectors/npm/NpmLockfile3Detector.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
namespace Microsoft.ComponentDetection.Detectors.Npm;

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.Json;
Expand Down Expand Up @@ -102,7 +103,7 @@ protected override void ProcessLockfile(
this.RecordComponent(singleFileComponentRecorder, component, lockPackage.Dev ?? false, component);

// Enqueue nested dependencies
this.EnqueueNestedDependencies(subQueue, path, lockPackage, packageLookup, singleFileComponentRecorder, component);
this.EnqueueNestedDependencies(subQueue, path, lockPackage, packageLookup, component);

// Process sub-dependencies
while (subQueue.Count > 0)
Expand All @@ -127,7 +128,7 @@ protected override void ProcessLockfile(

this.RecordComponent(singleFileComponentRecorder, subComponent, subPackage.Dev ?? false, component, parentComponent.Id);

this.EnqueueNestedDependencies(subQueue, subPath, subPackage, packageLookup, singleFileComponentRecorder, subComponent);
this.EnqueueNestedDependencies(subQueue, subPath, subPackage, packageLookup, subComponent);
}
}
}
Expand Down Expand Up @@ -157,7 +158,6 @@ private void EnqueueNestedDependencies(
string currentPath,
PackageLockV3Package package,
Dictionary<string, (string Path, PackageLockV3Package Package)> packageLookup,
ISingleFileComponentRecorder componentRecorder,
TypedComponent parent)
{
if (package.Dependencies is null)
Expand All @@ -167,31 +167,26 @@ private void EnqueueNestedDependencies(

foreach (var dep in package.Dependencies)
{
// First, check if there is an entry in the lockfile for this dependency nested in its ancestors
var ancestors = componentRecorder.DependencyGraph.GetAncestors(parent.Id);
ancestors.Add(parent.Id);

// Remove version information from ancestor IDs
ancestors = ancestors.Select(x => x.Split(' ')[0]).ToList();

var found = false;

// Depth-first search through ancestors
for (var i = 0; i < ancestors.Count && !found; i++)
// Resolve using lockfile paths: current package path first, then each ancestor path, then top-level.
var currentAncestorPath = currentPath;
while (!string.IsNullOrEmpty(currentAncestorPath) && !found)
{
var possiblePath = ancestors.Skip(i).ToList();
var ancestorNodeModulesPath = string.Format(
"{0}/{1}/{0}/{2}",
NodeModules,
string.Join($"/{NodeModules}/", possiblePath),
dep.Key);
var ancestorNodeModulesPath = $"{currentAncestorPath}/{NodeModules}/{dep.Key}";

if (packageLookup.TryGetValue(ancestorNodeModulesPath, out var nestedPkg))
{
this.Logger.LogDebug("Found nested dependency {Dependency} in {AncestorNodeModulesPath}", dep.Key, ancestorNodeModulesPath);
queue.Enqueue((nestedPkg.Path, nestedPkg.Package, parent));
found = true;
continue;
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The continue statement here is redundant because found is set to true and the while loop condition checks !found, which will cause the loop to exit naturally. The continue will skip the remaining iterations, but since the loop is already exiting, this doesn't add value. Consider removing this line for clarity.

Suggested change
continue;

Copilot uses AI. Check for mistakes.
}

var parentNodeModulesIndex = currentAncestorPath.LastIndexOf($"/{NodeModules}/", StringComparison.Ordinal);
currentAncestorPath = parentNodeModulesIndex >= 0
? currentAncestorPath[..parentNodeModulesIndex]
: null;
}

if (found)
Expand Down
76 changes: 76 additions & 0 deletions test/Microsoft.ComponentDetection.Detectors.Tests/NpmLockfile3DetectorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,82 @@ public async Task TestNpmDetector_NestedNodeModulesV3Async()
dependencyGraph.GetDependenciesForComponent(componentBId).Should().BeEmpty();
}

[TestMethod]
public async Task TestNpmDetector_ResolvesDependencyFromAncestorPathV3Async()
{
var componentA = (Name: "componentA", Version: "1.0.0");
var componentB = (Name: "componentB", Version: "1.0.0");
var componentC = (Name: "componentC", Version: "1.0.0");

var packageLockJson = @"{{
""name"": ""test"",
""version"": ""0.0.0"",
""lockfileVersion"": 3,
""requires"": true,
""packages"": {{
"""": {{
""name"": ""test"",
""version"": ""0.0.0"",
""dependencies"": {{
""{0}"": ""{1}""
}}
}},
""node_modules/{0}"": {{
""version"": ""{1}"",
""resolved"": ""https://registry.npmjs.org/{0}/-/{0}-{1}.tgz"",
""integrity"": ""sha512-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="",
""dependencies"": {{
""{2}"": ""{3}""
}}
}},
""node_modules/{0}/node_modules/{2}"": {{
""version"": ""{3}"",
""resolved"": ""https://registry.npmjs.org/{2}/-/{2}-{3}.tgz"",
""integrity"": ""sha512-BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB="",
""dependencies"": {{
""{4}"": ""{5}""
}}
}},
""node_modules/{0}/node_modules/{4}"": {{
""version"": ""{5}"",
""resolved"": ""https://registry.npmjs.org/{4}/-/{4}-{5}.tgz"",
""integrity"": ""sha512-CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC=""
}}
}}
}}";

var packageLockTemplate = string.Format(packageLockJson, componentA.Name, componentA.Version, componentB.Name, componentB.Version, componentC.Name, componentC.Version);

var packagejson = @"{{
""name"": ""test"",
""version"": ""0.0.0"",
""dependencies"": {{
""{0}"": ""{1}""
}}
}}";

var packageJsonTemplate = string.Format(packagejson, componentA.Name, componentA.Version);

var (scanResult, componentRecorder) = await this.DetectorTestUtility
.WithFile(this.packageLockJsonFileName, packageLockTemplate, this.packageLockJsonSearchPatterns)
.WithFile(this.packageJsonFileName, packageJsonTemplate, this.packageJsonSearchPattern)
.ExecuteDetectorAsync();

scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);

var detectedComponents = componentRecorder.GetDetectedComponents();
detectedComponents.Should().HaveCount(3);

var componentAId = detectedComponents.First(c => ((NpmComponent)c.Component).Name.Equals(componentA.Name)).Component.Id;
var componentBId = detectedComponents.First(c => ((NpmComponent)c.Component).Name.Equals(componentB.Name)).Component.Id;
var componentCId = detectedComponents.First(c => ((NpmComponent)c.Component).Name.Equals(componentC.Name)).Component.Id;

var dependencyGraph = componentRecorder.GetDependencyGraphsByLocation().Values.First();

dependencyGraph.GetDependenciesForComponent(componentAId).Should().Contain(componentBId);
dependencyGraph.GetDependenciesForComponent(componentBId).Should().Contain(componentCId);
}

[TestMethod]
public async Task TestNpmDetector_PackageLockWithoutPackagesObject_ShouldHandleGracefully()
{
Expand Down
Loading