Fix some Maven manifest & resolver issues (#1008)

Some fixes for a few Maven resolver issues I've come across:
1. Requirement origins weren't being tracked correctly if a package key
was set by a property. Fixed by checking dependency keys before and
after interpolation, and updating the map if they changed. (I've
modified one of the tests to check for this case)
2. To work around the resolver not resolving test or optional
dependencies, made it so the pom.xml parser removes the test scope and
optional flags.
3. `resolve.PackageKey` was not sufficient to uniquely key the
requirements for the `Groups` map in the `Manifest`. Made a new
`RequirementKey` type with ecosystem-specific information for both npm
and maven to solve this.

internal/remediation/suggest/maven.go

@@ -32,7 +32,7 @@ func (ms *MavenSuggester) Suggest(ctx context.Context, client resolve.Client, mf
 		if slices.Contains(opts.NoUpdates, req.Name) {
 			continue
 		}
-		if opts.IgnoreDev && lockfile.MavenEcosystem.IsDevGroup(mf.Groups[req.PackageKey]) {
+		if opts.IgnoreDev && lockfile.MavenEcosystem.IsDevGroup(mf.Groups[manifest.MakeRequirementKey(req)]) {
 			// Skip the update if the dependency is of development group
 			// and updates on development dependencies are not desired
 			continue

internal/remediation/suggest/maven_test.go

@@ -28,6 +28,27 @@ func depTypeWithOrigin(origin string) dep.Type {
 	return result
 }
 
+func mavenReqKey(t *testing.T, name, artifactType, classifier string) manifest.RequirementKey {
+	t.Helper()
+	var typ dep.Type
+	if artifactType != "" {
+		typ.AddAttr(dep.MavenArtifactType, artifactType)
+	}
+	if classifier != "" {
+		typ.AddAttr(dep.MavenClassifier, classifier)
+	}
+
+	return manifest.MakeRequirementKey(resolve.RequirementVersion{
+		VersionKey: resolve.VersionKey{
+			PackageKey: resolve.PackageKey{
+				Name:   name,
+				System: resolve.Maven,
+			},
+		},
+		Type: typ,
+	})
+}
+
 func TestSuggest(t *testing.T) {
 	t.Parallel()
 	ctx := context.Background()
@@ -98,7 +119,7 @@ func TestSuggest(t *testing.T) {
 					VersionType: resolve.Requirement,
 					Version:     "4.12",
 				},
-				Type: dep.NewType(dep.Test),
+				// Type: dep.NewType(dep.Test), test scope is ignored to make resolution work.
 			},
 			{
 				VersionKey: resolve.VersionKey{
@@ -223,9 +244,9 @@ func TestSuggest(t *testing.T) {
 				Type: depPlugin,
 			},
 		},
-		Groups: map[resolve.PackageKey][]string{
-			{System: resolve.Maven, Name: "junit:junit"}:    {"test"},
-			{System: resolve.Maven, Name: "org.import:xyz"}: {"import"},
+		Groups: map[manifest.RequirementKey][]string{
+			mavenReqKey(t, "junit:junit", "", ""):    {"test"},
+			mavenReqKey(t, "org.import:xyz", "", ""): {"import"},
 		},
 		EcosystemSpecific: manifest.MavenManifestSpecific{
 			Properties: []manifest.PropertyWithOrigin{

internal/resolution/dependency_chain.go

@@ -6,6 +6,7 @@ import (
 
 	"deps.dev/util/resolve"
 	"deps.dev/util/resolve/dep"
+	"github.com/google/osv-scanner/internal/resolution/manifest"
 	"github.com/google/osv-scanner/internal/resolution/util"
 	vulnUtil "github.com/google/osv-scanner/internal/utility/vulns"
 	"github.com/google/osv-scanner/pkg/lockfile"
@@ -34,21 +35,24 @@ func (dc DependencyChain) End() (resolve.VersionKey, string) {
 	return dc.At(0)
 }
 
-func ChainIsDev(dc DependencyChain, groups map[resolve.PackageKey][]string) bool {
+func ChainIsDev(dc DependencyChain, groups map[manifest.RequirementKey][]string) bool {
 	edge := dc.Edges[len(dc.Edges)-1]
 	// This check only applies to the graphs created from the in-place lockfile scanning.
 	// TODO: consider dev dependencies in e.g. workspaces that aren't direct
 	if edge.Type.HasAttr(dep.Dev) {
 		return true
 	}
 
-	vk := dc.Graph.Nodes[edge.To].Version
-	ecosystem, ok := util.OSVEcosystem[vk.System]
+	req := resolve.RequirementVersion{
+		VersionKey: dc.Graph.Nodes[edge.To].Version,
+		Type:       edge.Type.Clone(),
+	}
+	ecosystem, ok := util.OSVEcosystem[req.System]
 	if !ok {
 		return false
 	}
 
-	return lockfile.Ecosystem(ecosystem).IsDevGroup(groups[vk.PackageKey])
+	return lockfile.Ecosystem(ecosystem).IsDevGroup(groups[manifest.MakeRequirementKey(req)])
 }
 
 // ComputeChains computes all paths from each specified NodeID to the root node.

internal/resolution/manifest/manifest.go

@@ -15,17 +15,17 @@ import (
 )
 
 type Manifest struct {
-	FilePath          string                          // Path to the manifest file on disk
-	Root              resolve.Version                 // Version representing this package
-	Requirements      []resolve.RequirementVersion    // All direct requirements, including dev
-	Groups            map[resolve.PackageKey][]string // Dependency groups that the imports belong to
-	LocalManifests    []Manifest                      // manifests of local packages
-	EcosystemSpecific any                             // Any ecosystem-specific information needed
+	FilePath          string                       // Path to the manifest file on disk
+	Root              resolve.Version              // Version representing this package
+	Requirements      []resolve.RequirementVersion // All direct requirements, including dev
+	Groups            map[RequirementKey][]string  // Dependency groups that the imports belong to
+	LocalManifests    []Manifest                   // manifests of local packages
+	EcosystemSpecific any                          // Any ecosystem-specific information needed
 }
 
 func newManifest() Manifest {
 	return Manifest{
-		Groups: make(map[resolve.PackageKey][]string),
+		Groups: make(map[RequirementKey][]string),
 	}
 }
 
@@ -101,3 +101,23 @@ func GetManifestIO(pathToManifest string) (ManifestIO, error) {
 		return nil, fmt.Errorf("unsupported manifest type: %s", base)
 	}
 }
+
+// A RequirementKey is a comparable type that uniquely identifies a package dependency in a manifest.
+// It does not include the version specification.
+type RequirementKey struct {
+	resolve.PackageKey
+	EcosystemSpecific any
+}
+
+func MakeRequirementKey(requirement resolve.RequirementVersion) RequirementKey {
+	switch requirement.System {
+	case resolve.NPM:
+		return npmRequirementKey(requirement)
+	case resolve.Maven:
+		return mavenRequirementKey(requirement)
+	case resolve.UnknownSystem:
+		fallthrough
+	default:
+		return RequirementKey{PackageKey: requirement.PackageKey}
+	}
+}

internal/resolution/manifest/maven.go

@@ -13,6 +13,7 @@ import (
 
 	"deps.dev/util/maven"
 	"deps.dev/util/resolve"
+	"deps.dev/util/resolve/dep"
 	"github.com/google/osv-scanner/internal/resolution/datasource"
 	"github.com/google/osv-scanner/pkg/lockfile"
 )
@@ -25,6 +26,20 @@ const (
 	OriginProfile    = "profile"
 )
 
+func mavenRequirementKey(requirement resolve.RequirementVersion) RequirementKey {
+	// Maven dependencies must have unique groupId:artifactId:type:classifier.
+	artifactType, _ := requirement.Type.GetAttr(dep.MavenArtifactType)
+	classifier, _ := requirement.Type.GetAttr(dep.MavenClassifier)
+
+	return RequirementKey{
+		PackageKey: requirement.PackageKey,
+		EcosystemSpecific: struct{ ArtifactType, Classifier string }{
+			ArtifactType: artifactType,
+			Classifier:   classifier,
+		},
+	}
+}
+
 type MavenManifestIO struct {
 	datasource.MavenRegistryAPIClient
 }
@@ -76,6 +91,58 @@ func (m MavenManifestIO) Read(df lockfile.DepFile) (Manifest, error) {
 		}
 	}
 
+	interpolate := func(project *maven.Project) error {
+		// Interpolating can change a dependency's key if it contained properties.
+		// If it is changed we need to update the requirementOrigins map with the new key.
+		allKeys := func() []maven.DependencyKey {
+			// Get all the dependencyKeys of the project.
+			// This order shouldn't change after calling Interpolate.
+			var keys []maven.DependencyKey
+			for _, dep := range project.Dependencies {
+				keys = append(keys, dep.Key())
+			}
+			for _, dep := range project.DependencyManagement.Dependencies {
+				keys = append(keys, dep.Key())
+			}
+			for _, profile := range project.Profiles {
+				for _, dep := range profile.Dependencies {
+					keys = append(keys, dep.Key())
+				}
+				for _, dep := range profile.DependencyManagement.Dependencies {
+					keys = append(keys, dep.Key())
+				}
+			}
+			for _, plugin := range project.Build.PluginManagement.Plugins {
+				for _, dep := range plugin.Dependencies {
+					keys = append(keys, dep.Key())
+				}
+			}
+
+			return keys
+		}
+
+		prevKeys := allKeys()
+		if err := project.Interpolate(); err != nil {
+			return err
+		}
+
+		newKeys := allKeys()
+		if len(prevKeys) != len(newKeys) {
+			// The length can change if properties fail to resolve, which should be rare.
+			// It's difficult to determine which dependencies were removed in these cases, so just error.
+			return errors.New("number of dependencies changed after interpolation")
+		}
+
+		for i, prevKey := range prevKeys {
+			newKey := newKeys[i]
+			if newKey != prevKey {
+				requirementOrigins[newKey] = requirementOrigins[prevKey]
+			}
+		}
+
+		return nil
+	}
+
 	var project maven.Project
 	if err := xml.NewDecoder(df).Decode(&project); err != nil {
 		return Manifest{}, fmt.Errorf("failed to unmarshal project: %w", err)
@@ -86,6 +153,10 @@ func (m MavenManifestIO) Read(df lockfile.DepFile) (Manifest, error) {
 	if err := m.MergeParents(ctx, &project, project.Parent, 1, df.Path(), addAllRequirements, OriginParent); err != nil {
 		return Manifest{}, fmt.Errorf("failed to merge parents: %w", err)
 	}
+	// Interpolate to resolve properties.
+	if err := interpolate(&project); err != nil {
+		return Manifest{}, fmt.Errorf("failed to merge parents: %w", err)
+	}
 
 	// Process the dependencies:
 	//  - dedupe dependencies and dependency management
@@ -97,6 +168,10 @@ func (m MavenManifestIO) Read(df lockfile.DepFile) (Manifest, error) {
 		if err := m.MergeParents(ctx, &result, root, 0, df.Path(), addAllRequirements, OriginImport); err != nil {
 			return maven.DependencyManagement{}, err
 		}
+		// Interpolate to resolve properties.
+		if err := interpolate(&result); err != nil {
+			return maven.DependencyManagement{}, err
+		}
 
 		return result.DependencyManagement, nil
 	})
@@ -112,21 +187,19 @@ func (m MavenManifestIO) Read(df lockfile.DepFile) (Manifest, error) {
 
 	var requirements []resolve.RequirementVersion
 	var otherRequirements []resolve.RequirementVersion
-	groups := make(map[resolve.PackageKey][]string)
+	groups := make(map[RequirementKey][]string)
 	addRequirements := func(deps []maven.Dependency) {
 		for _, dep := range deps {
 			origin := requirementOrigins[dep.Key()]
+			reqVer := makeRequirementVersion(dep, origin)
 			if strings.HasPrefix(origin, OriginParent+"@") || strings.HasPrefix(origin, OriginImport) {
-				otherRequirements = append(otherRequirements, makeRequirementVersion(dep, origin))
+				otherRequirements = append(otherRequirements, reqVer)
 			} else {
-				requirements = append(requirements, makeRequirementVersion(dep, origin))
+				requirements = append(requirements, reqVer)
 			}
 			if dep.Scope != "" {
-				pk := resolve.PackageKey{
-					System: resolve.Maven,
-					Name:   dep.Name(),
-				}
-				groups[pk] = append(groups[pk], string(dep.Scope))
+				reqKey := mavenRequirementKey(reqVer)
+				groups[reqKey] = append(groups[reqKey], string(dep.Scope))
 			}
 		}
 	}
@@ -228,8 +301,8 @@ func (m MavenManifestIO) MergeParents(ctx context.Context, result *maven.Project
 		result.MergeParent(proj)
 		current = proj.Parent
 	}
-	// Interpolate the project to resolve the properties.
-	return result.Interpolate()
+
+	return nil
 }
 
 // For dependencies in profiles and plugins, we use origin to indicate where they are from.
@@ -238,6 +311,12 @@ func (m MavenManifestIO) MergeParents(ctx context.Context, result *maven.Project
 //   - identifier to locate the profile/plugin which is profile ID or plugin name
 //   - (optional) suffix indicates if this is a dependency management
 func makeRequirementVersion(dep maven.Dependency, origin string) resolve.RequirementVersion {
+	// Treat test & optional dependencies as regular dependencies to force the resolver to resolve them.
+	if dep.Scope == "test" {
+		dep.Scope = ""
+	}
+	dep.Optional = ""
+
 	return resolve.RequirementVersion{
 		VersionKey: resolve.VersionKey{
 			PackageKey: resolve.PackageKey{

internal/resolution/manifest/maven_test.go

@@ -35,6 +35,27 @@ func depTypeWithOrigin(origin string) dep.Type {
 	return result
 }
 
+func mavenReqKey(t *testing.T, name, artifactType, classifier string) manifest.RequirementKey {
+	t.Helper()
+	var typ dep.Type
+	if artifactType != "" {
+		typ.AddAttr(dep.MavenArtifactType, artifactType)
+	}
+	if classifier != "" {
+		typ.AddAttr(dep.MavenClassifier, classifier)
+	}
+
+	return manifest.MakeRequirementKey(resolve.RequirementVersion{
+		VersionKey: resolve.VersionKey{
+			PackageKey: resolve.PackageKey{
+				Name:   name,
+				System: resolve.Maven,
+			},
+		},
+		Type: typ,
+	})
+}
+
 func TestMavenRead(t *testing.T) {
 	t.Parallel()
 
@@ -46,13 +67,14 @@ func TestMavenRead(t *testing.T) {
 	  <version>1.2.3</version>
 	  <packaging>pom</packaging>
 	  <properties>
+			<bbb.artifact>bbb</bbb.artifact>
 		  <bbb.version>2.2.2</bbb.version>
 	  </properties>
 	  <dependencyManagement>
 		<dependencies>
 		  <dependency>
 			<groupId>org.example</groupId>
-			<artifactId>bbb</artifactId>
+			<artifactId>${bbb.artifact}</artifactId>
 			<version>${bbb.version}</version>
 		  </dependency>
 		</dependencies>
@@ -138,7 +160,7 @@ func TestMavenRead(t *testing.T) {
 					VersionType: resolve.Requirement,
 					Version:     "4.12",
 				},
-				Type: dep.NewType(dep.Test),
+				// Type: dep.NewType(dep.Test), test scope is ignored to make resolution work.
 			},
 			{
 				VersionKey: resolve.VersionKey{
@@ -206,12 +228,13 @@ func TestMavenRead(t *testing.T) {
 				Type: depPlugin,
 			},
 		},
-		Groups: map[resolve.PackageKey][]string{
-			{System: resolve.Maven, Name: "junit:junit"}:    {"test"},
-			{System: resolve.Maven, Name: "org.import:xyz"}: {"import"},
+		Groups: map[manifest.RequirementKey][]string{
+			mavenReqKey(t, "junit:junit", "", ""):       {"test"},
+			mavenReqKey(t, "org.import:xyz", "pom", ""): {"import"},
 		},
 		EcosystemSpecific: manifest.MavenManifestSpecific{
 			Properties: []manifest.PropertyWithOrigin{
+				{Property: maven.Property{Name: "bbb.artifact", Value: "bbb"}},
 				{Property: maven.Property{Name: "bbb.version", Value: "2.2.2"}},
 				{Property: maven.Property{Name: "aaa.version", Value: "1.1.1"}},
 
@@ -231,7 +254,7 @@ func TestMavenRead(t *testing.T) {
 						VersionType: resolve.Requirement,
 						Version:     "${junit.version}",
 					},
-					Type: dep.NewType(dep.Test),
+					// Type: dep.NewType(dep.Test), test scope is ignored to make resolution work.
 				},
 				{
 					VersionKey: resolve.VersionKey{
@@ -259,7 +282,7 @@ func TestMavenRead(t *testing.T) {
 					VersionKey: resolve.VersionKey{
 						PackageKey: resolve.PackageKey{
 							System: resolve.Maven,
-							Name:   "org.example:bbb",
+							Name:   "org.example:${bbb.artifact}",
 						},
 						VersionType: resolve.Requirement,
 						Version:     "${bbb.version}",