osv-scanner: fix reports of vulnerabilities when multiple versions of @grafana packages are installed (#419)

* osv-scanner: fix multiple versions of a same grafana package reporting vulnerabilities

* simplify code

* replace express by body-parser

* Merge main

* restore files

* simplify lock file

* reduce lock file to a min

* fix string

Author: academo

pkg/analysis/passes/osvscanner/cache-grafana-packages.go

@@ -6,16 +6,10 @@ import (
 	"github.com/grafana/plugin-validator/pkg/analysis/passes/osvscanner/lockfile"
 )
 
-func packageExists(name string, items []lockfile.PackageDetails) bool {
-	for _, aPackage := range items {
-		if aPackage.Name == name {
-			return true
-		}
-	}
-	return false
-}
-
-func IncludedByGrafanaPackage(packageName string, cache []lockfile.PackageFlattened) (bool, string) {
+func IncludedByGrafanaPackage(
+	packageName string,
+	cache []lockfile.PackageFlattened,
+) (bool, string) {
 	for _, item := range cache {
 		for _, dependency := range item.Dependencies {
 			if dependency.Package.Name == packageName {
@@ -26,24 +20,105 @@ func IncludedByGrafanaPackage(packageName string, cache []lockfile.PackageFlatte
 	return false, ""
 }
 
-func CacheGrafanaPackages(allPackages []lockfile.PackageDetails) ([]lockfile.PackageFlattened, error) {
+func CacheGrafanaPackages(
+	allPackages []lockfile.PackageDetails,
+) ([]lockfile.PackageFlattened, error) {
 	cache := make([]lockfile.PackageFlattened, 0)
-	for grafanaPackage := range GrafanaPackages {
-		// check if the package is in the list parsed
-		if packageExists(grafanaPackage, allPackages) {
-			// get dependencies of grafanaPackage
-			expanded, err := lockfile.ExpandPackage(grafanaPackage, allPackages)
-			if err != nil {
-				// failed to parse
-				return nil, err
+	processedNames := make(map[string]bool)
+
+	for _, pkg := range allPackages {
+		if GrafanaPackages[pkg.Name] && !processedNames[pkg.Name] {
+			processedNames[pkg.Name] = true
+
+			allVersions := make([]lockfile.PackageDetails, 0)
+			for _, p := range allPackages {
+				if p.Name == pkg.Name {
+					allVersions = append(allVersions, p)
+				}
+			}
+
+			// should never happen
+			if len(allVersions) == 0 {
+				continue
+			}
+
+			// create a custom flattened package
+			// with all dependencies for all versions of this package
+			merged := lockfile.PackageFlattened{
+				Name:    pkg.Name,
+				Version: allVersions[0].Version,
+			}
+
+			for _, version := range allVersions {
+				for _, dep := range version.Dependencies {
+					// do not add them twice
+					if !dependencyExists(dep.Name, merged.Dependencies) {
+						state := lockfile.DependencyState{
+							Package:   dep,
+							Processed: false,
+						}
+						merged.Dependencies = append(merged.Dependencies, state)
+					}
+				}
 			}
-			cache = append(cache, *expanded)
+
+			// find all transitive dependencies of this package
+			// aka deps of deps of deps of deps to the infinite
+			expandDependenciesRecursively(&merged, allPackages)
+
+			cache = append(cache, merged)
 		}
 	}
-	// sort cache
+
 	sort.SliceStable(cache, func(i, j int) bool {
 		return cache[i].Name < cache[j].Name
 	})
 
 	return cache, nil
 }
+
+func expandDependenciesRecursively(
+	topLevel *lockfile.PackageFlattened,
+	allPackages []lockfile.PackageDetails,
+) {
+	for i := 0; i < len(topLevel.Dependencies); i++ {
+		if topLevel.Dependencies[i].Processed {
+			continue
+		}
+
+		topLevel.Dependencies[i].Processed = true
+		depName := topLevel.Dependencies[i].Package.Name
+
+		var depPackage *lockfile.PackageDetails
+		for j := range allPackages {
+			if allPackages[j].Name == depName {
+				depPackage = &allPackages[j]
+				break
+			}
+		}
+
+		if depPackage == nil {
+			continue
+		}
+
+		for _, subDep := range depPackage.Dependencies {
+			// do not add them twice
+			if !dependencyExists(subDep.Name, topLevel.Dependencies) {
+				state := lockfile.DependencyState{
+					Package:   subDep,
+					Processed: false,
+				}
+				topLevel.Dependencies = append(topLevel.Dependencies, state)
+			}
+		}
+	}
+}
+
+func dependencyExists(depName string, dependencies []lockfile.DependencyState) bool {
+	for _, dep := range dependencies {
+		if dep.Package.Name == depName {
+			return true
+		}
+	}
+	return false
+}

pkg/analysis/passes/osvscanner/cache-grafana-packages_test.go

@@ -32,3 +32,100 @@ func TestCacheHitMiss(t *testing.T) {
 	cacheMiss, _ := IncludedByGrafanaPackage("notmoment", cachedPackages)
 	require.False(t, cacheMiss)
 }
+
+func TestCacheGrafanaPackagesMultipleVersionsNPM(t *testing.T) {
+	t.Parallel()
+
+	aLockfile := filepath.Join("testdata", "node", "multi-version-npm", "package-lock.json")
+	packages, err := lockfile.ParseNpmLock(aLockfile)
+	require.NoError(t, err)
+
+	cachedPackages, err := CacheGrafanaPackages(packages)
+	require.NoError(t, err)
+
+	// This test verifies the fix: when multiple versions of @grafana/data exist (9.5.1 with dompurify and 9.3.8 without), both versions should be cached so dompurify is found
+	var foundGrafanaData bool
+	var hasDompurify bool
+	for _, pkg := range cachedPackages {
+		if pkg.Name == "@grafana/data" {
+			foundGrafanaData = true
+			for _, dep := range pkg.Dependencies {
+				if dep.Package.Name == "dompurify" {
+					hasDompurify = true
+					break
+				}
+			}
+			break
+		}
+	}
+
+	require.True(t, foundGrafanaData, "@grafana/data should be in the cache")
+	require.True(t, hasDompurify, "dompurify should be in @grafana/data dependencies")
+
+	cacheHit, includedBy := IncludedByGrafanaPackage("dompurify", cachedPackages)
+	require.True(t, cacheHit, "dompurify should be found in the cache")
+	require.Equal(t, "@grafana/data", includedBy, "dompurify should be included by @grafana/data")
+}
+
+func TestCacheGrafanaPackagesSingleVersionNPM(t *testing.T) {
+	t.Parallel()
+
+	aLockfile := filepath.Join("testdata", "node", "critical-npm", "package-lock.json")
+	packages, err := lockfile.ParseNpmLock(aLockfile)
+	require.NoError(t, err)
+
+	cachedPackages, err := CacheGrafanaPackages(packages)
+	require.NoError(t, err)
+
+	require.NotEmpty(t, cachedPackages, "cache should have entries")
+
+	cacheHit, includedBy := IncludedByGrafanaPackage("moment", cachedPackages)
+	require.True(t, cacheHit, "moment should be found in the cache")
+	require.Equal(t, "@grafana/data", includedBy, "moment should be included by @grafana/data")
+}
+
+func TestCacheGrafanaPackagesYarnLock(t *testing.T) {
+	t.Parallel()
+
+	aLockfile := filepath.Join("testdata", "node", "circular-yarn", "yarn.lock")
+	packages, err := lockfile.ParseYarnLock(aLockfile)
+	require.NoError(t, err)
+
+	cachedPackages, err := CacheGrafanaPackages(packages)
+	require.NoError(t, err)
+
+	grafanaPackageCount := 0
+	grafanaPackages := make(map[string]bool)
+	for _, pkg := range cachedPackages {
+		if pkg.Name == "@grafana/data" || pkg.Name == "@grafana/runtime" || pkg.Name == "@grafana/toolkit" || pkg.Name == "@grafana/ui" {
+			grafanaPackageCount++
+			grafanaPackages[pkg.Name] = true
+		}
+	}
+
+	require.GreaterOrEqual(t, grafanaPackageCount, 2, "should have at least 2 different @grafana/* packages in cache")
+	require.True(t, grafanaPackages["@grafana/data"], "@grafana/data should be in cache")
+	require.True(t, grafanaPackages["@grafana/runtime"], "@grafana/runtime should be in cache")
+
+	cacheHit, includedBy := IncludedByGrafanaPackage("moment", cachedPackages)
+	require.True(t, cacheHit, "moment should be found in the cache")
+	require.Equal(t, "@grafana/data", includedBy, "moment should be included by @grafana/data")
+}
+
+func TestNonGrafanaDependencyVulnerabilitiesAreNotFiltered(t *testing.T) {
+	t.Parallel()
+
+	aLockfile := filepath.Join("testdata", "node", "multi-version-npm", "package-lock.json")
+	packages, err := lockfile.ParseNpmLock(aLockfile)
+	require.NoError(t, err)
+
+	cachedPackages, err := CacheGrafanaPackages(packages)
+	require.NoError(t, err)
+
+	bodyParserInCache, _ := IncludedByGrafanaPackage("body-parser", cachedPackages)
+	require.False(t, bodyParserInCache, "body-parser should NOT be included by any Grafana package")
+
+	dompurifyInCache, includedByDompurify := IncludedByGrafanaPackage("dompurify", cachedPackages)
+	require.True(t, dompurifyInCache, "dompurify should be included by a Grafana package")
+	require.Equal(t, "@grafana/data", includedByDompurify, "dompurify should be included by @grafana/data")
+}

pkg/analysis/passes/osvscanner/osvscanner.go

@@ -97,7 +97,10 @@ func run(pass *analysis.Pass) (interface{}, error) {
 			scanningPerformed = true
 			data, err := doScanInternal(lockFile)
 			if err != nil {
-				logme.DebugFln("osv-scanner returned error (vulnerabilities found): %s", err.Error())
+				logme.DebugFln(
+					"osv-scanner returned error (vulnerabilities found): %s",
+					err.Error(),
+				)
 			}
 
 			filteredResults := FilterOSVResults(data, lockFile)
@@ -138,19 +141,20 @@ func run(pass *analysis.Pass) (interface{}, error) {
 							messagesReported[message] = true
 							switch severity {
 							case SeverityCritical:
+								logme.DebugFln("osv-scanner detected a critical severity issue: %s", message)
 								pass.ReportResult(
 									pass.AnalyzerName,
 									osvScannerCriticalSeverityDetected,
 									"osv-scanner detected a critical severity issue",
 									message)
 								criticalSeverityCount++
 							case SeverityHigh:
+								logme.DebugFln("osv-scanner detected a high severity issue: %s", message)
 								pass.ReportResult(
 									pass.AnalyzerName,
 									osvScannerHighSeverityDetected,
 									"osv-scanner detected a high severity issue",
-									message,
-								)
+									message)
 								highSeverityCount++
 							}
 						}

pkg/analysis/passes/osvscanner/osvscanner_test.go

@@ -2,6 +2,7 @@ package osvscanner
 
 import (
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/google/osv-scanner/v2/pkg/models"
@@ -111,7 +112,7 @@ func TestOSVScannerAsLibrary(t *testing.T) {
 	require.NoError(t, err)
 	require.Len(t, interceptor.Diagnostics, 4)
 
-	// this results in three issues: 1 individual critical, 1 critical summary, 1 individual high, 1 high summary
+	// this results in four issues: 1 individual critical, 1 critical summary, 1 individual high, 1 high summary
 	messages := []string{
 		"osv-scanner detected a critical severity issue",
 		"osv-scanner detected critical severity issues",
@@ -154,3 +155,31 @@ func TestOSVScannerAsLibraryInvalidLockfile(t *testing.T) {
 	require.NoError(t, err)
 	require.Len(t, interceptor.Diagnostics, 0)
 }
+
+func TestOSVScannerMultiVersionNPM(t *testing.T) {
+	var interceptor testpassinterceptor.TestPassInterceptor
+	pass := &analysis.Pass{
+		RootDir: filepath.Join("./"),
+		ResultOf: map[*analysis.Analyzer]interface{}{
+			archive.Analyzer:    filepath.Join("testdata", "node", "multi-version-npm"),
+			sourcecode.Analyzer: filepath.Join("testdata", "node", "multi-version-npm"),
+		},
+		Report: interceptor.ReportInterceptor(),
+	}
+
+	_, err := Analyzer.Run(pass)
+	require.NoError(t, err)
+
+	titles := interceptor.GetTitles()
+	require.Contains(t, titles, "osv-scanner detected high severity issues")
+
+	details := interceptor.GetDetails()
+	hasBodyParserVulnerability := false
+	for _, detail := range details {
+		if strings.Contains(detail, "body-parser") && strings.Contains(detail, "SEVERITY: HIGH") {
+			hasBodyParserVulnerability = true
+			break
+		}
+	}
+	require.True(t, hasBodyParserVulnerability, "body-parser high severity vulnerability should be reported")
+}