fix(nodejs): fix infinity loops for pnpm with cyclic imports (#6857)

DmitriyLewen · web-flow · commit 7d083bc890ec · 2024-06-05T07:38:42.000Z
diff --git a/pkg/dependency/parser/nodejs/pnpm/parse.go b/pkg/dependency/parser/nodejs/pnpm/parse.go
@@ -183,7 +183,7 @@ func (p *Parser) parseV9(lockFile LockFile) ([]ftypes.Package, []ftypes.Dependen
 		if dep, ok := lockFile.Importers.Root.DevDependencies[name]; ok && dep.Version == ver {
 			relationship = ftypes.RelationshipDirect
 		}
-		if dep, ok := lockFile.Importers.Root.Dependencies[name]; ok && dep.Version == ver {
+		if dep, ok := lockFile.Importers.Root.Dependencies[name]; ok && p.trimPeerDeps(dep.Version, lockVer) == ver {
 			relationship = ftypes.RelationshipDirect
 			dev = false // mark root direct deps to update `dev` field of their child deps.
 		}
@@ -208,29 +208,34 @@ func (p *Parser) parseV9(lockFile LockFile) ([]ftypes.Package, []ftypes.Dependen
 		}
 	}
 
+	visited := make(map[string]struct{})
 	// Overwrite the `Dev` field for dev deps and their child dependencies.
 	for _, pkg := range resolvedPkgs {
 		if !pkg.Dev {
-			p.markRootPkgs(pkg.ID, resolvedPkgs, resolvedDeps)
+			p.markRootPkgs(pkg.ID, resolvedPkgs, resolvedDeps, visited)
 		}
 	}
 
 	return maps.Values(resolvedPkgs), maps.Values(resolvedDeps)
 }
 
 // markRootPkgs sets `Dev` to false for non dev dependency.
-func (p *Parser) markRootPkgs(id string, pkgs map[string]ftypes.Package, deps map[string]ftypes.Dependency) {
+func (p *Parser) markRootPkgs(id string, pkgs map[string]ftypes.Package, deps map[string]ftypes.Dependency, visited map[string]struct{}) {
+	if _, ok := visited[id]; ok {
+		return
+	}
 	pkg, ok := pkgs[id]
 	if !ok {
 		return
 	}
 
 	pkg.Dev = false
 	pkgs[id] = pkg
+	visited[id] = struct{}{}
 
 	// Update child deps
 	for _, depID := range deps[id].DependsOn {
-		p.markRootPkgs(depID, pkgs, deps)
+		p.markRootPkgs(depID, pkgs, deps, visited)
 	}
 	return
 }
diff --git a/pkg/dependency/parser/nodejs/pnpm/parse_test.go b/pkg/dependency/parser/nodejs/pnpm/parse_test.go
@@ -59,6 +59,18 @@ func TestParse(t *testing.T) {
 			want:     pnpmV9,
 			wantDeps: pnpmV9Deps,
 		},
+		{
+			name:     "v9",
+			file:     "testdata/pnpm-lock_v9.yaml",
+			want:     pnpmV9,
+			wantDeps: pnpmV9Deps,
+		},
+		{
+			name:     "v9 with cyclic dependencies import",
+			file:     "testdata/pnpm-lock_v9_cyclic_import.yaml",
+			want:     pnpmV9CyclicImport,
+			wantDeps: pnpmV9CyclicImportDeps,
+		},
 	}
 
 	for _, tt := range tests {
diff --git a/pkg/dependency/parser/nodejs/pnpm/parse_testcase.go b/pkg/dependency/parser/nodejs/pnpm/parse_testcase.go
@@ -900,4 +900,68 @@ var (
 			},
 		},
 	}
+
+	pnpmV9CyclicImport = []ftypes.Package{
+		{
+			ID:           "update-browserslist-db@1.0.16",
+			Name:         "update-browserslist-db",
+			Version:      "1.0.16",
+			Relationship: ftypes.RelationshipDirect,
+		},
+		{
+			ID:           "browserslist@4.23.0",
+			Name:         "browserslist",
+			Version:      "4.23.0",
+			Relationship: ftypes.RelationshipIndirect,
+		},
+		{
+			ID:           "caniuse-lite@1.0.30001627",
+			Name:         "caniuse-lite",
+			Version:      "1.0.30001627",
+			Relationship: ftypes.RelationshipIndirect,
+		},
+		{
+			ID:           "electron-to-chromium@1.4.789",
+			Name:         "electron-to-chromium",
+			Version:      "1.4.789",
+			Relationship: ftypes.RelationshipIndirect,
+		},
+		{
+			ID:           "escalade@3.1.2",
+			Name:         "escalade",
+			Version:      "3.1.2",
+			Relationship: ftypes.RelationshipIndirect,
+		},
+		{
+			ID:           "node-releases@2.0.14",
+			Name:         "node-releases",
+			Version:      "2.0.14",
+			Relationship: ftypes.RelationshipIndirect,
+		},
+		{
+			ID:           "picocolors@1.0.1",
+			Name:         "picocolors",
+			Version:      "1.0.1",
+			Relationship: ftypes.RelationshipIndirect,
+		},
+	}
+	pnpmV9CyclicImportDeps = []ftypes.Dependency{
+		{
+			ID: "browserslist@4.23.0",
+			DependsOn: []string{
+				"caniuse-lite@1.0.30001627",
+				"electron-to-chromium@1.4.789",
+				"node-releases@2.0.14",
+				"update-browserslist-db@1.0.16",
+			},
+		},
+		{
+			ID: "update-browserslist-db@1.0.16",
+			DependsOn: []string{
+				"browserslist@4.23.0",
+				"escalade@3.1.2",
+				"picocolors@1.0.1",
+			},
+		},
+	}
 )
diff --git a/pkg/dependency/parser/nodejs/pnpm/testdata/pnpm-lock_v9_cyclic_import.yaml b/pkg/dependency/parser/nodejs/pnpm/testdata/pnpm-lock_v9_cyclic_import.yaml
@@ -0,0 +1,67 @@
+lockfileVersion: '9.0'
+
+settings:
+  autoInstallPeers: true
+  excludeLinksFromLockfile: false
+
+importers:
+
+  .:
+    dependencies:
+      update-browserslist-db:
+        specifier: 1.0.16
+        version: 1.0.16(browserslist@4.23.0)
+
+packages:
+
+  browserslist@4.23.0:
+    resolution: {integrity: sha512-QW8HiM1shhT2GuzkvklfjcKDiWFXHOeFCIA/huJPwHsslwcydgk7X+z2zXpEijP98UCY7HbubZt5J2Zgvf0CaQ==}
+    engines: {node: ^6 || ^7 || ^8 || ^9 || ^10 || ^11 || ^12 || >=13.7}
+    hasBin: true
+
+  caniuse-lite@1.0.30001627:
+    resolution: {integrity: sha512-4zgNiB8nTyV/tHhwZrFs88ryjls/lHiqFhrxCW4qSTeuRByBVnPYpDInchOIySWknznucaf31Z4KYqjfbrecVw==}
+
+  electron-to-chromium@1.4.789:
+    resolution: {integrity: sha512-0VbyiaXoT++Fi2vHGo2ThOeS6X3vgRCWrjPeO2FeIAWL6ItiSJ9BqlH8LfCXe3X1IdcG+S0iLoNaxQWhfZoGzQ==}
+
+  escalade@3.1.2:
+    resolution: {integrity: sha512-ErCHMCae19vR8vQGe50xIsVomy19rg6gFu3+r3jkEO46suLMWBksvVyoGgQV+jOfl84ZSOSlmv6Gxa89PmTGmA==}
+    engines: {node: '>=6'}
+
+  node-releases@2.0.14:
+    resolution: {integrity: sha512-y10wOWt8yZpqXmOgRo77WaHEmhYQYGNA6y421PKsKYWEK8aW+cqAphborZDhqfyKrbZEN92CN1X2KbafY2s7Yw==}
+
+  picocolors@1.0.1:
+    resolution: {integrity: sha512-anP1Z8qwhkbmu7MFP5iTt+wQKXgwzf7zTyGlcdzabySa9vd0Xt392U0rVmz9poOaBj0uHJKyyo9/upk0HrEQew==}
+
+  update-browserslist-db@1.0.16:
+    resolution: {integrity: sha512-KVbTxlBYlckhF5wgfyZXTWnMn7MMZjMu9XG8bPlliUOP9ThaF4QnhP8qrjrH7DRzHfSk0oQv1wToW+iA5GajEQ==}
+    hasBin: true
+    peerDependencies:
+      browserslist: '>= 4.21.0'
+
+snapshots:
+
+  browserslist@4.23.0:
+    dependencies:
+      caniuse-lite: 1.0.30001627
+      electron-to-chromium: 1.4.789
+      node-releases: 2.0.14
+      update-browserslist-db: 1.0.16(browserslist@4.23.0)
+
+  caniuse-lite@1.0.30001627: {}
+
+  electron-to-chromium@1.4.789: {}
+
+  escalade@3.1.2: {}
+
+  node-releases@2.0.14: {}
+
+  picocolors@1.0.1: {}
+
+  update-browserslist-db@1.0.16(browserslist@4.23.0):
+    dependencies:
+      browserslist: 4.23.0
+      escalade: 3.1.2
+      picocolors: 1.0.1

Original file line number	Diff line number	Diff line change
`@@ -183,7 +183,7 @@ func (p *Parser) parseV9(lockFile LockFile) ([]ftypes.Package, []ftypes.Dependen`
`183`	`183`	`if dep, ok := lockFile.Importers.Root.DevDependencies[name]; ok && dep.Version == ver {`
`184`	`184`	`relationship = ftypes.RelationshipDirect`
`185`	`185`	`}`
`186`		`- if dep, ok := lockFile.Importers.Root.Dependencies[name]; ok && dep.Version == ver {`
	`186`	`+ if dep, ok := lockFile.Importers.Root.Dependencies[name]; ok && p.trimPeerDeps(dep.Version, lockVer) == ver {`
`187`	`187`	`relationship = ftypes.RelationshipDirect`
`188`	`188`	dev = false // mark root direct deps to update `dev` field of their child deps.
`189`	`189`	`}`
`@@ -208,29 +208,34 @@ func (p *Parser) parseV9(lockFile LockFile) ([]ftypes.Package, []ftypes.Dependen`
`208`	`208`	`}`
`209`	`209`	`}`
`210`	`210`
	`211`	`+ visited := make(map[string]struct{})`
`211`	`212`	// Overwrite the `Dev` field for dev deps and their child dependencies.
`212`	`213`	`for _, pkg := range resolvedPkgs {`
`213`	`214`	`if !pkg.Dev {`
`214`		`- p.markRootPkgs(pkg.ID, resolvedPkgs, resolvedDeps)`
	`215`	`+ p.markRootPkgs(pkg.ID, resolvedPkgs, resolvedDeps, visited)`
`215`	`216`	`}`
`216`	`217`	`}`
`217`	`218`
`218`	`219`	`return maps.Values(resolvedPkgs), maps.Values(resolvedDeps)`
`219`	`220`	`}`
`220`	`221`
`221`	`222`	// markRootPkgs sets `Dev` to false for non dev dependency.
`222`		`-func (p *Parser) markRootPkgs(id string, pkgs map[string]ftypes.Package, deps map[string]ftypes.Dependency) {`
	`223`	`+func (p *Parser) markRootPkgs(id string, pkgs map[string]ftypes.Package, deps map[string]ftypes.Dependency, visited map[string]struct{}) {`
	`224`	`+ if _, ok := visited[id]; ok {`
	`225`	`+ return`
	`226`	`+ }`
`223`	`227`	`pkg, ok := pkgs[id]`
`224`	`228`	`if !ok {`
`225`	`229`	`return`
`226`	`230`	`}`
`227`	`231`
`228`	`232`	`pkg.Dev = false`
`229`	`233`	`pkgs[id] = pkg`
	`234`	`+ visited[id] = struct{}{}`
`230`	`235`
`231`	`236`	`// Update child deps`
`232`	`237`	`for _, depID := range deps[id].DependsOn {`
`233`		`- p.markRootPkgs(depID, pkgs, deps)`
	`238`	`+ p.markRootPkgs(depID, pkgs, deps, visited)`
`234`	`239`	`}`
`235`	`240`	`return`
`236`	`241`	`}`