Update package-lock.json parsing strategy

- Start by finding any version changes
- Then find the package name whose version was changed

Fix #1631

internal/engine/eval/vulncheck/pkgdb.go

@@ -100,12 +100,13 @@ type packageJson struct {
 	} `json:"dist"`
 }
 
-func (pj *packageJson) IndentedString(indent int, _ string, _ *pb.Dependency) string {
+func (pj *packageJson) IndentedString(indent int, oldDepLine string, _ *pb.Dependency) string {
 	padding := fmt.Sprintf("%*s", indent, "")
 	innerPadding := padding + "  " // Add 2 extra spaces
 
+	// use the old dependency to get the correct package path
+	data := fmt.Sprintf("%s\n", oldDepLine)
 	// format each line with leadingWhitespace and 2 extra spaces
-	data := padding + fmt.Sprintf("\"%s\": {\n", pj.Name)
 	data += innerPadding + fmt.Sprintf("\"version\": \"%s\",\n", pj.Version)
 	data += innerPadding + fmt.Sprintf("\"resolved\": \"%s\",\n", pj.Dist.Tarball)
 	data += innerPadding + fmt.Sprintf("\"integrity\": \"%s\",", pj.Dist.Integrity)
@@ -114,7 +115,7 @@ func (pj *packageJson) IndentedString(indent int, _ string, _ *pb.Dependency) st
 }
 
 func (pj *packageJson) LineHasDependency(line string) bool {
-	pkgLine := fmt.Sprintf(`"%s": {`, pj.Name)
+	pkgLine := fmt.Sprintf(`%s": {`, pj.Name)
 	return strings.Contains(line, pkgLine)
 }
 

internal/engine/eval/vulncheck/pkgdb_test.go

@@ -163,6 +163,54 @@ func TestNpmPkgDb(t *testing.T) {
 	}
 }
 
+func TestPackageJsonLineHasDependency(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name         string
+		line         string
+		pj           *packageJson
+		expectRetval bool
+	}{
+		{
+			name: "MatchWithScope",
+			line: "\"node_modules/@types/node\": {",
+			pj: &packageJson{
+				Name:    "@types/node",
+				Version: "20.9.0",
+			},
+			expectRetval: true,
+		},
+		{
+			name: "MatchNoScope",
+			line: "\"node_modules/lodash\": {",
+			pj: &packageJson{
+				Name:    "lodash",
+				Version: "4.17.21",
+			},
+			expectRetval: true,
+		},
+		{
+			name: "NoMatch",
+			line: "\"node_modules/other\": {",
+			pj: &packageJson{
+				Name:    "lodash",
+				Version: "4.17.21",
+			},
+			expectRetval: false,
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+		})
+
+		require.Equal(t, tt.expectRetval, tt.pj.LineHasDependency(tt.line), "expected reply to match mock data")
+	}
+}
+
 func TestPyPiReplyLineHasDependency(t *testing.T) {
 	t.Parallel()
 

internal/engine/eval/vulncheck/review_test.go

@@ -151,7 +151,7 @@ func TestReviewPrHandlerVulnerabilitiesDifferentIdentities(t *testing.T) {
 
 	expBody, err := createReviewBody(vulnsFoundText)
 	require.NoError(t, err)
-	expCommentBody := reviewBodyWithSuggestion(patchPackage.IndentedString(0, "", nil))
+	expCommentBody := reviewBodyWithSuggestion(patchPackage.IndentedString(0, fmt.Sprintf(`"%s": {`, patchPackage.Name), nil))
 
 	mockClient.EXPECT().
 		ListReviews(gomock.Any(), pr.RepoOwner, pr.RepoName, int(pr.Number), nil).
@@ -424,7 +424,7 @@ func TestCommitStatusPrHandlerWithVulnerabilities(t *testing.T) {
 
 	expBody, err := createReviewBody(vulnsFoundText)
 	require.NoError(t, err)
-	expCommentBody := reviewBodyWithSuggestion(patchPackage.IndentedString(0, "", nil))
+	expCommentBody := reviewBodyWithSuggestion(patchPackage.IndentedString(0, fmt.Sprintf(`"%s": {`, patchPackage.Name), nil))
 
 	mockClient.EXPECT().
 		ListReviews(gomock.Any(), pr.RepoOwner, pr.RepoName, int(pr.Number), nil).

internal/engine/ingester/diff/parse.go

@@ -17,15 +17,19 @@ package diff
 
 import (
 	"bufio"
-	"encoding/json"
-	"fmt"
+	"path/filepath"
 	"regexp"
 	"strings"
 
 	"github.com/stacklok/minder/internal/util"
 	pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
 )
 
+var (
+	versionRegex        = regexp.MustCompile(`^\+\s*"version"\s*:\s*"([^"\n]*)"\s*(?:,|$)`)
+	dependencyNameRegex = regexp.MustCompile(`\s*"([^"]+)"\s*:\s*{\s*`)
+)
+
 type ecosystemParser func(string) ([]*pb.Dependency, error)
 
 func newEcosystemParser(eco DependencyEcosystem) ecosystemParser {
@@ -161,62 +165,67 @@ func goParse(patch string) ([]*pb.Dependency, error) {
 	return deps, nil
 }
 
-type npmDependency struct {
-	Version      string                    `json:"version"`
-	Resolved     string                    `json:"resolved"`
-	Integrity    string                    `json:"integrity"`
-	Requires     map[string]string         `json:"requires,omitempty"`
-	Dependencies map[string]*npmDependency `json:"dependencies,omitempty"`
-	Optional     bool                      `json:"optional,omitempty"`
-}
-
-type npmRoot struct {
-	Deps map[string]*npmDependency
-}
-
-// TODO(jakub): this is a hacky way to parse the npm patch file
 func npmParse(patch string) ([]*pb.Dependency, error) {
 	lines := strings.Split(patch, "\n")
-	var output strings.Builder
-
-	// Write the start of the JSON object to the output
-	output.WriteString("{\n")
-
-	// Start of crazy code to parse the patch file
-	// What we do here is first grab all the lines that start with "+"
-	// Then we remove the "+" symbol and write the modified line to the output
-	// We then add a { to the start of the output and a } to the end, so we have a valid JSON object
-	// Then we convert the string builder to a string and unmarshal it into the Package struct
-	for _, line := range lines {
-		// Check if the line starts with "+"
-		if strings.HasPrefix(line, "+") {
-			// Remove the "+" symbol and write the modified line to the output
-			line = strings.TrimPrefix(line, "+")
-			output.WriteString(line + "\n")
+
+	var deps []*pb.Dependency
+
+	for i, line := range lines {
+		// Check if the line contains a version
+		matches := versionRegex.FindStringSubmatch(line)
+		if len(matches) > 1 {
+			version := matches[1]
+			name := findDependencyName(i, lines)
+			// The version is not always a dependency version. It may also be the version of the package in this repo,
+			// or the version of the root project. See https://docs.npmjs.com/cli/v10/configuring-npm/package-lock-json
+			if name != "" {
+				deps = append(deps, &pb.Dependency{
+					Ecosystem: pb.DepEcosystem_DEP_ECOSYSTEM_NPM,
+					Name:      name,
+					Version:   version,
+				})
+			}
 		}
 	}
 
-	//if string ends with a comma, remove it
-	outputString := strings.TrimSuffix(output.String(), ",\n")
-	outputString = outputString + "\n}"
+	return deps, nil
+}
 
-	// Convert the string builder to a string and unmarshal it into the Package struct
-	root := &npmRoot{
-		Deps: make(map[string]*npmDependency),
+// findDependencyName iterates over all the previous lines to find the JSON key containing the parent dependency name.
+// If the parent key does not correspond to a dependency (i.e. it could be the root project), then an empty string is
+// returned.
+func findDependencyName(i int, lines []string) string {
+	closingBraces := 0
+	for j := i - 1; j >= 0; j-- {
+		if strings.Contains(lines[j], "}") {
+			closingBraces = closingBraces + 1
+		}
+		if strings.Contains(lines[j], "{") {
+			if closingBraces == 0 {
+				matches := dependencyNameRegex.FindStringSubmatch(lines[j])
+				if len(matches) > 1 {
+					// extract the dependency name from the key, which is the dependency path
+					dependencyPath := matches[1]
+					return getDependencyName(dependencyPath)
+				}
+				return ""
+			}
+			closingBraces = closingBraces - 1
+		}
 	}
+	return ""
+}
 
-	err := json.Unmarshal([]byte(outputString), &root.Deps)
-	if err != nil {
-		return nil, fmt.Errorf("failed to unmarshal npm package: %w", err)
-	}
+func getDependencyName(dependencyPath string) string {
+	dependencyName := filepath.Base(dependencyPath)
+	dir := filepath.Dir(dependencyPath)
 
-	deps := make([]*pb.Dependency, 0, len(root.Deps))
-	for name, dep := range root.Deps {
-		deps = append(deps, &pb.Dependency{
-			Ecosystem: pb.DepEcosystem_DEP_ECOSYSTEM_NPM,
-			Name:      name,
-			Version:   dep.Version,
-		})
+	// Check if the parent directory starts with "@", meaning that the dependency has a scope.
+	// See https://docs.npmjs.com/cli/v10/using-npm/scope
+	if strings.HasPrefix(filepath.Base(dir), "@") {
+		// Prefix the dependency name with the scope
+		return filepath.Join(filepath.Base(dir), dependencyName)
 	}
-	return deps, nil
+
+	return dependencyName
 }