Refactoring Maven manifest reading (#1159)

There are two places we read Maven pom.xml:
 - Transitive scanning in `internal/manifest`
 - Guided remediation in `internal/resolution/manifest`

Both share the same logic to merge parents, so this PR consolidates the
implementation in `internal/manifest`.

This PR also updates `deps.dev` dependencies to the latest version.

go.mod

@@ -3,10 +3,10 @@ module github.com/google/osv-scanner
 go 1.21.12
 
 require (
-	deps.dev/api/v3 v3.0.0-20240711010811-c5a1406b0470
-	deps.dev/util/maven v0.0.0-20240711010811-c5a1406b0470
-	deps.dev/util/resolve v0.0.0-20240711010811-c5a1406b0470
-	deps.dev/util/semver v0.0.0-20240711010811-c5a1406b0470
+	deps.dev/api/v3 v3.0.0-20240730004939-e80e6658c33b
+	deps.dev/util/maven v0.0.0-20240730004939-e80e6658c33b
+	deps.dev/util/resolve v0.0.0-20240730004939-e80e6658c33b
+	deps.dev/util/semver v0.0.0-20240730004939-e80e6658c33b
 	github.com/BurntSushi/toml v1.4.0
 	github.com/CycloneDX/cyclonedx-go v0.9.0
 	github.com/charmbracelet/bubbles v0.18.0

go.sum

@@ -1,13 +1,13 @@
 dario.cat/mergo v1.0.0 h1:AGCNq9Evsj31mOgNPcLyXc+4PNABt905YmuqPYYpBWk=
 dario.cat/mergo v1.0.0/go.mod h1:uNxQE+84aUszobStD9th8a29P2fMDhsBdgRYvZOxGmk=
-deps.dev/api/v3 v3.0.0-20240711010811-c5a1406b0470 h1:AXexCbWOahap0SWSsyDbEkeGJt2KxX4+1zWWZ+RsWQU=
-deps.dev/api/v3 v3.0.0-20240711010811-c5a1406b0470/go.mod h1:DyBY3wNVqRCwvb4tLvz6LL/FupH3FMflEROyQAv2Vi0=
-deps.dev/util/maven v0.0.0-20240711010811-c5a1406b0470 h1:T990wxZ1ph9gKl8K4/qzorVmS24rNj2pNBTl5BYMxXA=
-deps.dev/util/maven v0.0.0-20240711010811-c5a1406b0470/go.mod h1:SBW3EribdkZYk6zxi5oVn/ZECvi4ixb7EGgEWfSimNk=
-deps.dev/util/resolve v0.0.0-20240711010811-c5a1406b0470 h1:/djBEKpc04deGl+68W1nv7GnsygaVnmAFZCgadZEWPs=
-deps.dev/util/resolve v0.0.0-20240711010811-c5a1406b0470/go.mod h1:XXi6yRYqhtxw5DvGX/mbG6fHSLn8OgoPowNd8EAxDgk=
-deps.dev/util/semver v0.0.0-20240711010811-c5a1406b0470 h1:3wJIdP0Z4so7k3PjqwxVIhCOrrYDsTOe2GBsInNEZWM=
-deps.dev/util/semver v0.0.0-20240711010811-c5a1406b0470/go.mod h1:jkcH+k02gWHBiZ7G4OnUOkSZ6WDq54Pt5DrOA8FN8Uo=
+deps.dev/api/v3 v3.0.0-20240730004939-e80e6658c33b h1:uWv66hsFIMA+4mfvYroVOpJ4+trL7PVc4zTiJ+FVUpE=
+deps.dev/api/v3 v3.0.0-20240730004939-e80e6658c33b/go.mod h1:DyBY3wNVqRCwvb4tLvz6LL/FupH3FMflEROyQAv2Vi0=
+deps.dev/util/maven v0.0.0-20240730004939-e80e6658c33b h1:4/2szyn/8mZhaI3PW/JkRRDpv0aVMILL/R0rICgAA50=
+deps.dev/util/maven v0.0.0-20240730004939-e80e6658c33b/go.mod h1:SBW3EribdkZYk6zxi5oVn/ZECvi4ixb7EGgEWfSimNk=
+deps.dev/util/resolve v0.0.0-20240730004939-e80e6658c33b h1:MTE07TVpmsX13qjSHiVxLPR2u52R7w8m0TBlk7rNvF8=
+deps.dev/util/resolve v0.0.0-20240730004939-e80e6658c33b/go.mod h1:XXi6yRYqhtxw5DvGX/mbG6fHSLn8OgoPowNd8EAxDgk=
+deps.dev/util/semver v0.0.0-20240730004939-e80e6658c33b h1:kGG4/rm/slq+X/SfMVS7JnDBWeJhX2u2EudaPJeHyHI=
+deps.dev/util/semver v0.0.0-20240730004939-e80e6658c33b/go.mod h1:jkcH+k02gWHBiZ7G4OnUOkSZ6WDq54Pt5DrOA8FN8Uo=
 github.com/BurntSushi/toml v1.4.0 h1:kuoIxZQy2WRRk1pttg9asf+WVv6tWQuBNVmK8+nqPr0=
 github.com/BurntSushi/toml v1.4.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho=
 github.com/CycloneDX/cyclonedx-go v0.9.0 h1:inaif7qD8bivyxp7XLgxUYtOXWtDez7+j72qKTMQTb8=

internal/manifest/fixtures/maven/my-app/pom.xml

@@ -0,0 +1,8 @@
+<!-- For testing parent relative path -->
+<project>
+
+  <groupId>org.test</groupId>
+  <artifactId>my-app</artifactId>
+  <version>1.0.0</version>
+
+</project>

internal/resolution/manifest/fixtures/maven/pom.xml → internal/manifest/fixtures/maven/pom.xml

@@ -1,8 +1,6 @@
 <!-- For testing parent relative path -->
 <project>
 
-  <modelVersion>4.0.0</modelVersion>
-
   <groupId>org.test</groupId>
   <artifactId>test</artifactId>
   <version>1.0.0</version>

internal/manifest/maven.go

@@ -14,12 +14,18 @@ import (
 	mavenresolve "deps.dev/util/resolve/maven"
 	"github.com/google/osv-scanner/internal/resolution/client"
 	"github.com/google/osv-scanner/internal/resolution/datasource"
-	"github.com/google/osv-scanner/internal/resolution/manifest"
 	"github.com/google/osv-scanner/internal/resolution/util"
 	"github.com/google/osv-scanner/pkg/lockfile"
 	"golang.org/x/exp/maps"
 )
 
+const (
+	OriginManagement = "management"
+	OriginParent     = "parent"
+	OriginPlugin     = "plugin"
+	OriginProfile    = "profile"
+)
+
 type MavenResolverExtractor struct {
 	client.DependencyClient
 	datasource.MavenRegistryAPIClient
@@ -37,7 +43,7 @@ func (e MavenResolverExtractor) Extract(f lockfile.DepFile) ([]lockfile.PackageD
 		return []lockfile.PackageDetails{}, fmt.Errorf("could not extract from %s: %w", f.Path(), err)
 	}
 	// Merging parents data by parsing local parent pom.xml or fetching from upstream.
-	if err := e.mergeParents(ctx, &project, project.Parent, 1, true, f.Path()); err != nil {
+	if err := MergeMavenParents(ctx, e.MavenRegistryAPIClient, &project, project.Parent, 1, f.Path(), true); err != nil {
 		return []lockfile.PackageDetails{}, fmt.Errorf("failed to merge parents: %w", err)
 	}
 	// Process the dependencies:
@@ -47,7 +53,7 @@ func (e MavenResolverExtractor) Extract(f lockfile.DepFile) ([]lockfile.PackageD
 	project.ProcessDependencies(func(groupID, artifactID, version maven.String) (maven.DependencyManagement, error) {
 		root := maven.Parent{ProjectKey: maven.ProjectKey{GroupID: groupID, ArtifactID: artifactID, Version: version}}
 		var result maven.Project
-		if err := e.mergeParents(ctx, &result, root, 0, false, f.Path()); err != nil {
+		if err := MergeMavenParents(ctx, e.MavenRegistryAPIClient, &result, root, 0, f.Path(), false); err != nil {
 			return maven.DependencyManagement{}, err
 		}
 
@@ -91,7 +97,7 @@ func (e MavenResolverExtractor) Extract(f lockfile.DepFile) ([]lockfile.PackageD
 				VersionType: resolve.Requirement,
 				Version:     string(d.Version),
 			},
-			Type: resolve.MavenDepType(d, manifest.OriginManagement),
+			Type: resolve.MavenDepType(d, OriginManagement),
 		}
 	}
 	overrideClient.AddVersion(root, reqs)
@@ -130,7 +136,7 @@ func (e MavenResolverExtractor) Extract(f lockfile.DepFile) ([]lockfile.PackageD
 // MaxParent sets a limit on the number of parents to avoid indefinite loop.
 const MaxParent = 100
 
-// mergeParents parses local accessible parent pom.xml or fetches it from
+// MergeMavenParents parses local accessible parent pom.xml or fetches it from
 // upstream, merges into root project, then interpolate the properties.
 // result holds the merged Maven project.
 // current holds the current parent project to merge.
@@ -139,7 +145,7 @@ const MaxParent = 100
 // allowLocal indicates whether parsing local parent pom.xml is allowed.
 // path holds the path to the current pom.xml, which is used to compute the
 // relative path of parent.
-func (e MavenResolverExtractor) mergeParents(ctx context.Context, result *maven.Project, current maven.Parent, start int, allowLocal bool, path string) error {
+func MergeMavenParents(ctx context.Context, mavenClient datasource.MavenRegistryAPIClient, result *maven.Project, current maven.Parent, start int, path string, allowLocal bool) error {
 	currentPath := path
 	visited := make(map[maven.ProjectKey]bool, MaxParent)
 	for n := start; n < MaxParent; n++ {
@@ -154,7 +160,7 @@ func (e MavenResolverExtractor) mergeParents(ctx context.Context, result *maven.
 
 		var proj maven.Project
 		parentFound := false
-		if parentPath := manifest.MavenParentPOMPath(currentPath, string(current.RelativePath)); allowLocal && parentPath != "" {
+		if parentPath := MavenParentPOMPath(currentPath, string(current.RelativePath)); allowLocal && parentPath != "" {
 			currentPath = parentPath
 			f, err := os.Open(parentPath)
 			if err != nil {
@@ -174,7 +180,7 @@ func (e MavenResolverExtractor) mergeParents(ctx context.Context, result *maven.
 			allowLocal = false
 
 			var err error
-			proj, err = e.MavenRegistryAPIClient.GetProject(ctx, string(current.GroupID), string(current.ArtifactID), string(current.Version))
+			proj, err = mavenClient.GetProject(ctx, string(current.GroupID), string(current.ArtifactID), string(current.Version))
 			if err != nil {
 				return fmt.Errorf("failed to get Maven project %s:%s:%s: %w", current.GroupID, current.ArtifactID, current.Version, err)
 			}
@@ -198,6 +204,28 @@ func (e MavenResolverExtractor) mergeParents(ctx context.Context, result *maven.
 	return result.Interpolate()
 }
 
+// Maven looks for the parent POM first in 'relativePath',
+// then the local repository '../pom.xml',
+// and lastly in the remote repo.
+func MavenParentPOMPath(currentPath, relativePath string) string {
+	if relativePath == "" {
+		relativePath = "../pom.xml"
+	}
+	path := filepath.Join(filepath.Dir(currentPath), relativePath)
+	if info, err := os.Stat(path); err == nil {
+		if !info.IsDir() {
+			return path
+		}
+		// Current path is a directory, so look for pom.xml in the directory.
+		path = filepath.Join(path, "pom.xml")
+		if _, err := os.Stat(path); err == nil {
+			return path
+		}
+	}
+
+	return ""
+}
+
 func ParseMavenWithResolver(depClient client.DependencyClient, mavenClient datasource.MavenRegistryAPIClient, pathToLockfile string) ([]lockfile.PackageDetails, error) {
 	f, err := lockfile.OpenLocalDepFile(pathToLockfile)
 	if err != nil {

internal/manifest/maven_test.go

@@ -2,6 +2,8 @@ package manifest_test
 
 import (
 	"io/fs"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/google/osv-scanner/internal/manifest"
@@ -358,3 +360,65 @@ func TestParseMavenWithResolver_Transitive(t *testing.T) {
 		},
 	})
 }
+
+func TestParentPOMPath(t *testing.T) {
+	t.Parallel()
+	dir, err := os.Getwd()
+	if err != nil {
+		t.Fatalf("failed to get current directory: %v", err)
+	}
+	tests := []struct {
+		currentPath, relativePath string
+		want                      string
+	}{
+		// fixtures
+		// |- maven
+		// |  |- my-app
+		// |  |  |- pom.xml
+		// |  |- parent
+		// |  |  |- pom.xml
+		// |- pom.xml
+		{
+			// Parent path is specified correctly.
+			currentPath:  filepath.Join(dir, "fixtures", "maven", "my-app", "pom.xml"),
+			relativePath: "../parent/pom.xml",
+			want:         filepath.Join(dir, "fixtures", "maven", "parent", "pom.xml"),
+		},
+		{
+			// Wrong file name is specified in relative path.
+			currentPath:  filepath.Join(dir, "fixtures", "maven", "my-app", "pom.xml"),
+			relativePath: "../parent/abc.xml",
+			want:         "",
+		},
+		{
+			// Wrong directory is specified in relative path.
+			currentPath:  filepath.Join(dir, "fixtures", "maven", "my-app", "pom.xml"),
+			relativePath: "../not-found/pom.xml",
+			want:         "",
+		},
+		{
+			// Only directory is specified.
+			currentPath:  filepath.Join(dir, "fixtures", "maven", "my-app", "pom.xml"),
+			relativePath: "../parent",
+			want:         filepath.Join(dir, "fixtures", "maven", "parent", "pom.xml"),
+		},
+		{
+			// Parent relative path is default to '../pom.xml'.
+			currentPath:  filepath.Join(dir, "fixtures", "maven", "my-app", "pom.xml"),
+			relativePath: "",
+			want:         filepath.Join(dir, "fixtures", "maven", "pom.xml"),
+		},
+		{
+			// No pom.xml is found even in the default path.
+			currentPath:  filepath.Join(dir, "fixtures", "maven", "pom.xml"),
+			relativePath: "",
+			want:         "",
+		},
+	}
+	for _, test := range tests {
+		got := manifest.MavenParentPOMPath(test.currentPath, test.relativePath)
+		if got != test.want {
+			t.Errorf("parentPOMPath(%s, %s): got %s, want %s", test.currentPath, test.relativePath, got, test.want)
+		}
+	}
+}

internal/remediation/override.go

@@ -9,9 +9,10 @@ import (
 	"deps.dev/util/resolve"
 	"deps.dev/util/resolve/dep"
 	"deps.dev/util/semver"
+	"github.com/google/osv-scanner/internal/manifest"
 	"github.com/google/osv-scanner/internal/resolution"
 	"github.com/google/osv-scanner/internal/resolution/client"
-	"github.com/google/osv-scanner/internal/resolution/manifest"
+	resolutionmanifest "github.com/google/osv-scanner/internal/resolution/manifest"
 	"github.com/google/osv-scanner/internal/resolution/util"
 	"github.com/google/osv-scanner/internal/utility/vulns"
 )
@@ -76,9 +77,9 @@ func ComputeOverridePatches(ctx context.Context, cl client.ResolutionClient, res
 		// CalculateDiff does not compute override manifest patches correctly, manually fill it out.
 		// TODO: CalculateDiff maybe should not be reconstructing patches.
 		// Refactor CalculateDiff, Relaxer, Override to make patches in a more sane way.
-		diff.Deps = make([]manifest.DependencyPatch, len(res.patches))
+		diff.Deps = make([]resolutionmanifest.DependencyPatch, len(res.patches))
 		for i, p := range res.patches {
-			diff.Deps[i] = manifest.DependencyPatch{
+			diff.Deps[i] = resolutionmanifest.DependencyPatch{
 				Pkg:          p.PackageKey,
 				Type:         dep.Type{},
 				OrigRequire:  "", // Using empty original to signal this is an override patch
@@ -279,9 +280,9 @@ func getVersionsGreater(ctx context.Context, cl client.DependencyClient, vk reso
 }
 
 // patchManifest applies the overridePatches to the manifest in-memory. Returns a copy of the manifest that has been patched.
-func patchManifest(patches []overridePatch, m manifest.Manifest) (manifest.Manifest, error) {
+func patchManifest(patches []overridePatch, m resolutionmanifest.Manifest) (resolutionmanifest.Manifest, error) {
 	if m.System() != resolve.Maven {
-		return manifest.Manifest{}, errors.New("unsupported ecosystem")
+		return resolutionmanifest.Manifest{}, errors.New("unsupported ecosystem")
 	}
 
 	// TODO: The overridePatch does not have an artifact's type or classifier, which is part of what uniquely identifies them.