feat(guided remediation): add non-interactive Maven remediation by ov…

…erride (#1136)

#1141 
Adds `--strategy=override` for `osv-scanner fix --non-interactive` for
`pom.xml` manifest files. For now interactive mode will print an error
telling you to use non-interactive mode if you try `pom.xml`.

Also, made non-interactive mode decide which strategy to use if not
explicitly specified based on the provided manifests / lockfiles.

cmd/osv-scanner/__snapshots__/fix_test.snap

@@ -1742,6 +1742,72 @@ Warning: `fix` exists as both a subcommand of OSV-Scanner and as a file on the f
 
 ---
 
+[TestRun_Fix/fix_non-interactive_override_pom.xml - 1]
+Resolving <tempdir>/pom.xml...
+Found 11 vulnerabilities matching the filter
+Can fix 11/11 matching vulnerabilities by overriding 4 dependencies
+OVERRIDE-PACKAGE: org.apache.httpcomponents:httpclient,4.5.13
+OVERRIDE-PACKAGE: org.codehaus.plexus:plexus-utils,3.0.24
+OVERRIDE-PACKAGE: org.jsoup:jsoup,1.15.3
+OVERRIDE-PACKAGE: commons-io:commons-io,2.7
+REMAINING-VULNS: 0
+UNFIXABLE-VULNS: 0
+Rewriting <tempdir>/pom.xml...
+
+---
+
+[TestRun_Fix/fix_non-interactive_override_pom.xml - 2]
+Warning: `fix` exists as both a subcommand of OSV-Scanner and as a file on the filesystem. `fix` is assumed to be a subcommand here. If you intended for `fix` to be an argument to `fix`, you must specify `fix fix` in your command line.
+
+---
+
+[TestRun_Fix/fix_non-interactive_override_pom.xml - 3]
+<project>
+  <modelVersion>4.0.0</modelVersion>
+
+  <groupId>dev.osv</groupId>
+  <artifactId>osv-fix</artifactId>
+  <version>1</version>
+
+  <properties>
+    <httpclient.version>4.5.13</httpclient.version>
+  </properties>
+
+  <dependencyManagement>
+    <dependencies>
+    <dependency>
+      <groupId>commons-io</groupId>
+      <artifactId>commons-io</artifactId>
+      <version>2.7</version>
+    </dependency>
+      <dependency>
+        <groupId>org.jsoup</groupId>
+        <artifactId>jsoup</artifactId>
+        <version>1.15.3</version>
+      </dependency>
+      <dependency>
+        <groupId>org.apache.httpcomponents</groupId>
+        <artifactId>httpclient</artifactId>
+        <version>${httpclient.version}</version>
+      </dependency>
+    </dependencies>
+  </dependencyManagement>
+
+  <dependencies>
+    <dependency>
+      <groupId>org.apache.maven.wagon</groupId>
+      <artifactId>wagon-http</artifactId>
+      <version>3.0.0</version>
+    </dependency>
+    <dependency>
+      <groupId>org.codehaus.plexus</groupId>
+      <artifactId>plexus-utils</artifactId>
+      <version>3.0.24</version>
+    </dependency>
+  </dependencies>
+</project>
+---
+
 [TestRun_Fix/fix_non-interactive_relock_package.json - 1]
 Resolving <tempdir>/package.json...
 Found 5 vulnerabilities matching the filter

cmd/osv-scanner/fix/fixtures/override-maven/pom.xml

@@ -0,0 +1,39 @@
+<project>
+  <modelVersion>4.0.0</modelVersion>
+
+  <groupId>dev.osv</groupId>
+  <artifactId>osv-fix</artifactId>
+  <version>1</version>
+
+  <properties>
+    <httpclient.version>4.0</httpclient.version>
+  </properties>
+
+  <dependencyManagement>
+    <dependencies>
+      <dependency>
+        <groupId>org.jsoup</groupId>
+        <artifactId>jsoup</artifactId>
+        <version>1.14.1</version>
+      </dependency>
+      <dependency>
+        <groupId>org.apache.httpcomponents</groupId>
+        <artifactId>httpclient</artifactId>
+        <version>${httpclient.version}</version>
+      </dependency>
+    </dependencies>
+  </dependencyManagement>
+
+  <dependencies>
+    <dependency>
+      <groupId>org.apache.maven.wagon</groupId>
+      <artifactId>wagon-http</artifactId>
+      <version>3.0.0</version>
+    </dependency>
+    <dependency>
+      <groupId>org.codehaus.plexus</groupId>
+      <artifactId>plexus-utils</artifactId>
+      <version>3.0</version>
+    </dependency>
+  </dependencies>
+</project>

cmd/osv-scanner/fix/interactive.go

@@ -13,6 +13,10 @@ import (
 
 func interactiveMode(ctx context.Context, opts osvFixOptions) error {
 	if !remediation.SupportsRelax(opts.ManifestRW) && !remediation.SupportsInPlace(opts.LockfileRW) {
+		if remediation.SupportsOverride(opts.ManifestRW) {
+			return errors.New("override strategy is not supported in interactive mode, please use --non-interactive")
+		}
+
 		return errors.New("no supported remediation strategies found")
 	}
 

cmd/osv-scanner/fix/main.go

@@ -76,8 +76,7 @@ func Command(stdout, stderr io.Writer, r *reporter.Reporter) *cli.Command {
 			&cli.StringFlag{
 				Category: autoModeCategory,
 				Name:     "strategy",
-				Usage:    "remediation approach to use; value can be: in-place, relock",
-				Value:    "relock",
+				Usage:    "remediation approach to use; value can be: in-place, relock, override",
 				Action: func(ctx *cli.Context, s string) error {
 					if !ctx.Bool("non-interactive") {
 						// This flag isn't used in interactive mode
@@ -92,6 +91,10 @@ func Command(stdout, stderr io.Writer, r *reporter.Reporter) *cli.Command {
 						if !ctx.IsSet("manifest") {
 							return errors.New("relock strategy requires manifest file")
 						}
+					case "override":
+						if !ctx.IsSet("manifest") {
+							return errors.New("override strategy requires manifest file")
+						}
 					default:
 						return fmt.Errorf("unsupported strategy \"%s\" - must be one of: in-place, relock", s)
 					}
@@ -157,11 +160,6 @@ func Command(stdout, stderr io.Writer, r *reporter.Reporter) *cli.Command {
 }
 
 func action(ctx *cli.Context, stdout, stderr io.Writer) (reporter.Reporter, error) {
-	// The Action on strategy isn't run when using the default values. Check if the manifest is set.
-	if ctx.Bool("non-interactive") && ctx.String("strategy") == "relock" && !ctx.IsSet("manifest") {
-		return nil, errors.New("relock strategy requires manifest file")
-	}
-
 	if !ctx.IsSet("manifest") && !ctx.IsSet("lockfile") {
 		return nil, errors.New("manifest or lockfile is required")
 	}
@@ -232,11 +230,29 @@ func action(ctx *cli.Context, stdout, stderr io.Writer) (reporter.Reporter, erro
 	r := reporter.NewTableReporter(stdout, stderr, reporter.InfoLevel, false, 0)
 	maxUpgrades := ctx.Int("apply-top")
 
-	switch ctx.String("strategy") {
+	strategy := ctx.String("strategy")
+
+	if !ctx.IsSet("strategy") {
+		// Choose a default strategy based on the manifest/lockfile provided.
+		switch {
+		case remediation.SupportsRelax(opts.ManifestRW):
+			strategy = "relock"
+		case remediation.SupportsOverride(opts.ManifestRW):
+			strategy = "override"
+		case remediation.SupportsInPlace(opts.LockfileRW):
+			strategy = "in-place"
+		default:
+			return nil, errors.New("no supported remediation strategies for manifest/lockfile")
+		}
+	}
+
+	switch strategy {
 	case "relock":
 		return r, autoRelock(ctx.Context, r, opts, maxUpgrades)
 	case "in-place":
 		return r, autoInPlace(ctx.Context, r, opts, maxUpgrades)
+	case "override":
+		return r, autoOverride(ctx.Context, r, opts, maxUpgrades)
 	default:
 		// The strategy flag should already be validated by this point.
 		panic(fmt.Sprintf("non-interactive mode attempted to run with unhandled strategy: \"%s\"", ctx.String("strategy")))

cmd/osv-scanner/fix/noninteractive.go

@@ -247,6 +247,117 @@ func relockUnfixableVulns(diffs []resolution.ResolutionDiff) []*resolution.Resol
 	return unfixable
 }
 
+func autoOverride(ctx context.Context, r reporter.Reporter, opts osvFixOptions, maxUpgrades int) error {
+	if !remediation.SupportsOverride(opts.ManifestRW) {
+		return errors.New("override strategy is not supported for manifest")
+	}
+
+	r.Infof("Resolving %s...\n", opts.Manifest)
+	f, err := lockfile.OpenLocalDepFile(opts.Manifest)
+	if err != nil {
+		return err
+	}
+
+	manif, err := opts.ManifestRW.Read(f)
+	f.Close()
+	if err != nil {
+		return err
+	}
+
+	opts.Client.PreFetch(ctx, manif.Requirements, manif.FilePath)
+	res, err := resolution.Resolve(ctx, opts.Client, manif)
+	if err != nil {
+		return err
+	}
+
+	if errs := res.Errors(); len(errs) > 0 {
+		r.Warnf("WARNING: encountered %d errors during dependency resolution:\n", len(errs))
+		r.Warnf(resolutionErrorString(res, errs))
+	}
+
+	res.FilterVulns(opts.MatchVuln)
+	// TODO: count vulnerabilities per unique version as scan action does
+	totalVulns := len(res.Vulns)
+	r.Infof("Found %d vulnerabilities matching the filter\n", totalVulns)
+
+	allPatches, err := remediation.ComputeOverridePatches(ctx, opts.Client, res, opts.RemediationOptions)
+	if err != nil {
+		return err
+	}
+
+	if err := opts.Client.WriteCache(manif.FilePath); err != nil {
+		r.Warnf("WARNING: failed to write resolution cache: %v\n", err)
+	}
+
+	if len(allPatches) == 0 {
+		r.Infof("No dependency patches are possible\n")
+		r.Infof("REMAINING-VULNS: %d\n", totalVulns)
+		r.Infof("UNFIXABLE-VULNS: %d\n", totalVulns)
+
+		return nil
+	}
+
+	depPatches, nFixed := autoChooseOverridePatches(allPatches, maxUpgrades)
+	nUnfixable := len(relockUnfixableVulns(allPatches))
+	r.Infof("Can fix %d/%d matching vulnerabilities by overriding %d dependencies\n", nFixed, totalVulns, len(depPatches))
+	for _, p := range depPatches {
+		r.Infof("OVERRIDE-PACKAGE: %s,%s\n", p.Pkg.Name, p.NewRequire)
+	}
+	r.Infof("REMAINING-VULNS: %d\n", totalVulns-nFixed)
+	r.Infof("UNFIXABLE-VULNS: %d\n", nUnfixable)
+	// TODO: Print the FIXED-VULN-IDS, REMAINING-VULN-IDS, UNFIXABLE-VULN-IDS
+	// TODO: Consider potentially introduced vulnerabilities
+
+	r.Infof("Rewriting %s...\n", opts.Manifest)
+	if err := manifest.Overwrite(opts.ManifestRW, opts.Manifest, manifest.ManifestPatch{Manifest: &manif, Deps: depPatches}); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func autoChooseOverridePatches(diffs []resolution.ResolutionDiff, maxUpgrades int) ([]manifest.DependencyPatch, int) {
+	if maxUpgrades == 0 {
+		return nil, 0
+	}
+
+	var patches []manifest.DependencyPatch
+	pkgChanged := make(map[resolve.PackageKey]bool) // dependencies we've already applied a patch to
+	fixedVulns := make(map[string]bool)             // vulns that have already been fixed by a patch
+	for _, diff := range diffs {
+		// If this patch is incompatible with existing patches, skip adding it to the patch list.
+
+		// A patch is incompatible if any of its changed packages have already been changed by an existing patch.
+		if slices.ContainsFunc(diff.Deps, func(dp manifest.DependencyPatch) bool { return pkgChanged[dp.Pkg] }) {
+			continue
+		}
+		// A patch is also incompatible if any fixed vulnerability has already been fixed by another patch.
+		// This would happen if updating the version of one package has a side effect of also updating or removing one of its vulnerable dependencies.
+		// e.g. We have {foo@1 -> bar@1}, and two possible patches [foo@3, bar@2].
+		// Patching foo@3 makes {foo@3 -> bar@3}, which also fixes the vulnerability in bar.
+		// Applying both patches would force {foo@3 -> bar@2}, which is less desirable.
+		if slices.ContainsFunc(diff.RemovedVulns, func(rv resolution.ResolutionVuln) bool { return fixedVulns[rv.Vulnerability.ID] }) {
+			continue
+		}
+
+		// Add all individual package patches to the final patch list, and track the vulns this is anticipated to fix.
+		for _, dp := range diff.Deps {
+			patches = append(patches, dp)
+			pkgChanged[dp.Pkg] = true
+		}
+		for _, rv := range diff.RemovedVulns {
+			fixedVulns[rv.Vulnerability.ID] = true
+		}
+
+		maxUpgrades--
+		if maxUpgrades == 0 {
+			break
+		}
+	}
+
+	return patches, len(fixedVulns)
+}
+
 func resolutionErrorString(res *resolution.ResolutionResult, errs []resolution.ResolutionError) string {
 	// we pass in the []ResolutionErrors because calling res.Errors() is costly
 	s := strings.Builder{}

cmd/osv-scanner/fix_test.go

@@ -54,6 +54,12 @@ func TestRun_Fix(t *testing.T) {
 			exit:     0,
 			manifest: "./fix/fixtures/relock-npm/package.json",
 		},
+		{
+			name:     "fix non-interactive override pom.xml",
+			args:     []string{"", "fix", "--non-interactive", "--strategy=override"},
+			exit:     0,
+			manifest: "./fix/fixtures/override-maven/pom.xml",
+		},
 		// TODO: add tests with the cli flags
 	}
 	for _, tt := range tests {

internal/remediation/override.go

@@ -68,8 +68,10 @@ func ComputeOverridePatches(ctx context.Context, cl client.ResolutionClient, res
 		}
 
 		if res.err != nil {
-			// TODO: stop goroutines
-			return nil, res.err
+			// Resolution errors seem to happen when a package/version cannot be found, which isn't uncommon.
+			// Just silently skip for now, treating it the same as unfixable.
+			// TODO: Log the error somehow.
+			continue
 		}
 
 		diff := result.CalculateDiff(res.result)

internal/resolution/manifest/maven.go

@@ -312,6 +312,11 @@ func (m MavenDependencyPatches) addPatch(changedDep DependencyPatch, fromBase bo
 		return fmt.Errorf("MavenDepTypeToDependency: %w", err)
 	}
 
+	// If this dependency did not already exist in the base project, we want to add it to the dependencyManagement section
+	if !fromBase {
+		o = manifest.OriginManagement
+	}
+
 	substrings := strings.Split(changedDep.Pkg.Name, ":")
 	if len(substrings) != 2 {
 		return fmt.Errorf("invalid Maven name: %s", changedDep.Pkg.Name)