feat(guided remediation): add --upgrade-config flag (#1191)

closes #1177

- Adds `--upgrade-config` flag for configuring allowed upgrades on a
per-package basis.
- Hide & deprecate previous `--disallow-major-upgrades` and
`--disallow-package-upgrades` flags.
- Update docs & example script to use new flag.

cmd/osv-scanner/fix/main.go

@@ -6,9 +6,11 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"deps.dev/util/resolve"
 	"github.com/google/osv-scanner/internal/remediation"
+	"github.com/google/osv-scanner/internal/remediation/upgrade"
 	"github.com/google/osv-scanner/internal/resolution/client"
 	"github.com/google/osv-scanner/internal/resolution/lockfile"
 	"github.com/google/osv-scanner/internal/resolution/manifest"
@@ -110,16 +112,23 @@ func Command(stdout, stderr io.Writer, r *reporter.Reporter) *cli.Command {
 				Value:    -1,
 			},
 
+			&cli.StringSliceFlag{
+				Category:    upgradeCategory,
+				Name:        "upgrade-config",
+				Usage:       "the allowed package upgrades, in the format `[package-name:]level`. If package-name is omitted, level is applied to all packages. level must be one of (major, minor, patch, none).",
+				DefaultText: "major",
+			},
 			&cli.BoolFlag{
-				// TODO: allow for finer control e.g. specific packages, major/minor/patch
 				Category: upgradeCategory,
 				Name:     "disallow-major-upgrades",
 				Usage:    "disallow major version changes to dependencies",
+				Hidden:   true,
 			},
 			&cli.StringSliceFlag{
 				Category: upgradeCategory,
 				Name:     "disallow-package-upgrades",
 				Usage:    "list of packages to disallow version changes",
+				Hidden:   true,
 			},
 
 			&cli.IntFlag{
@@ -160,20 +169,75 @@ func Command(stdout, stderr io.Writer, r *reporter.Reporter) *cli.Command {
 	}
 }
 
+func parseUpgradeConfig(ctx *cli.Context, r reporter.Reporter) upgrade.Config {
+	config := upgrade.NewConfig()
+
+	if ctx.IsSet("disallow-major-upgrades") {
+		r.Warnf("WARNING: `--disallow-major-upgrades` flag is deprecated, use `--upgrade-config minor` instead\n")
+		if ctx.Bool("disallow-major-upgrades") {
+			config.SetDefault(upgrade.Minor)
+		} else {
+			config.SetDefault(upgrade.Major)
+		}
+	}
+	if ctx.IsSet("disallow-package-upgrades") {
+		r.Warnf("WARNING: `--disallow-package-upgrades` flag is deprecated, use `--upgrade-config PKG:none` instead\n")
+		for _, pkg := range ctx.StringSlice("disallow-package-upgrades") {
+			config.Set(pkg, upgrade.None)
+		}
+	}
+
+	for _, spec := range ctx.StringSlice("upgrade-config") {
+		idx := strings.LastIndex(spec, ":")
+		if idx == 0 {
+			r.Warnf("WARNING: `--upgrade-config %s` - skipping empty package name\n", spec)
+			continue
+		}
+		pkg := ""
+		levelStr := spec
+		if idx > 0 {
+			pkg = spec[:idx]
+			levelStr = spec[idx+1:]
+		}
+		var level upgrade.Level
+		switch levelStr {
+		case "major":
+			level = upgrade.Major
+		case "minor":
+			level = upgrade.Minor
+		case "patch":
+			level = upgrade.Patch
+		case "none":
+			level = upgrade.None
+		default:
+			r.Warnf("WARNING: `--upgrade-config %s` - invalid level string '%s'\n", spec, levelStr)
+			continue
+		}
+		if config.Set(pkg, level) { // returns true if was previously set
+			r.Warnf("WARNING: `--upgrade-config %s` - config for package specified multiple times\n", spec)
+		}
+	}
+
+	return config
+}
+
 func action(ctx *cli.Context, stdout, stderr io.Writer) (reporter.Reporter, error) {
 	if !ctx.IsSet("manifest") && !ctx.IsSet("lockfile") {
 		return nil, errors.New("manifest or lockfile is required")
 	}
 
+	// TODO: This isn't what the reporter is designed for.
+	// Only using r.Infof()/r.Warnf()/r.Errorf() to print to stdout/stderr.
+	r := reporter.NewTableReporter(stdout, stderr, reporter.InfoLevel, false, 0)
+
 	opts := osvFixOptions{
 		RemediationOptions: remediation.RemediationOptions{
 			IgnoreVulns:   ctx.StringSlice("ignore-vulns"),
 			ExplicitVulns: ctx.StringSlice("vulns"),
 			DevDeps:       !ctx.Bool("ignore-dev"),
 			MinSeverity:   ctx.Float64("min-severity"),
 			MaxDepth:      ctx.Int("max-depth"),
-			AvoidPkgs:     ctx.StringSlice("disallow-package-upgrades"),
-			AllowMajor:    !ctx.Bool("disallow-major-upgrades"),
+			UpgradeConfig: parseUpgradeConfig(ctx, r),
 		},
 		Manifest:  ctx.String("manifest"),
 		Lockfile:  ctx.String("lockfile"),
@@ -241,9 +305,6 @@ func action(ctx *cli.Context, stdout, stderr io.Writer) (reporter.Reporter, erro
 		return nil, interactiveMode(ctx.Context, opts)
 	}
 
-	// TODO: This isn't what the reporter is designed for.
-	// Only using r.Infof() and r.Errorf() to print to stdout & stderr respectively.
-	r := reporter.NewTableReporter(stdout, stderr, reporter.InfoLevel, false, 0)
 	maxUpgrades := ctx.Int("apply-top")
 
 	strategy := ctx.String("strategy")

cmd/osv-scanner/fix/main_test.go

@@ -0,0 +1,138 @@
+package fix
+
+import (
+	"slices"
+	"testing"
+
+	"github.com/google/osv-scanner/internal/remediation/upgrade"
+	"github.com/google/osv-scanner/pkg/reporter"
+	"github.com/urfave/cli/v2"
+)
+
+func parseFlags(t *testing.T, flags []string, arguments []string) (*cli.Context, error) {
+	// This is a bit hacky: make a mock App with only the flags we care about.
+	// Then use app.Run() to parse the flags into the cli.Context, which is returned.
+	t.Helper()
+	appFlags := make([]cli.Flag, 0, len(flags))
+	for _, f := range Command(nil, nil, nil).Flags {
+		if slices.ContainsFunc(f.Names(), func(s string) bool { return slices.Contains(flags, s) }) {
+			appFlags = append(appFlags, f)
+		}
+	}
+	var parsedContext *cli.Context
+	app := cli.App{
+		Flags: appFlags,
+		Action: func(ctx *cli.Context) error {
+			t.Helper()
+			parsedContext = ctx
+
+			return nil
+		},
+	}
+	err := app.Run(append([]string{""}, arguments...))
+
+	return parsedContext, err
+}
+
+func TestParseUpgradeConfig(t *testing.T) {
+	t.Parallel()
+	flags := []string{"upgrade-config", "disallow-major-upgrades", "disallow-package-upgrades"}
+
+	tests := []struct {
+		name string
+		args []string
+		want map[string]upgrade.Level
+	}{
+		{
+			name: "default behaviour",
+			args: []string{},
+			want: map[string]upgrade.Level{
+				"foo": upgrade.Major,
+				"bar": upgrade.Major,
+			},
+		},
+		{
+			name: "general level config",
+			args: []string{"--upgrade-config=minor"},
+			want: map[string]upgrade.Level{
+				"foo": upgrade.Minor,
+				"bar": upgrade.Minor,
+			},
+		},
+		{
+			name: "all levels",
+			args: []string{
+				"--upgrade-config", "major:major",
+				"--upgrade-config", "minor:minor",
+				"--upgrade-config", "patch:patch",
+				"--upgrade-config", "none:none",
+			},
+			want: map[string]upgrade.Level{
+				"major": upgrade.Major,
+				"minor": upgrade.Minor,
+				"patch": upgrade.Patch,
+				"none":  upgrade.None,
+				"other": upgrade.Major,
+			},
+		},
+		{
+			name: "package takes precedence over general",
+			args: []string{
+				"--upgrade-config", "pkg1:minor",
+				"--upgrade-config", "none",
+				"--upgrade-config", "pkg2:major",
+			},
+			want: map[string]upgrade.Level{
+				"pkg1": upgrade.Minor,
+				"pkg2": upgrade.Major,
+				"pkg3": upgrade.None,
+			},
+		},
+		{
+			name: "package names with colons",
+			args: []string{
+				"--upgrade-config=none:patch:minor:major",
+				"--upgrade-config=none:patch:minor",
+				"--upgrade-config=none:patch",
+				"--upgrade-config=none",
+			},
+			want: map[string]upgrade.Level{
+				"none:patch:minor": upgrade.Major,
+				"none:patch":       upgrade.Minor,
+				"none":             upgrade.Patch,
+				"other":            upgrade.None,
+			},
+		},
+		{
+			name: "deprecated flag",
+			args: []string{
+				"--disallow-major-upgrades",
+				"--disallow-package-upgrades=pkg1,pkg2",
+				"--upgrade-config=pkg3:patch",
+			},
+			want: map[string]upgrade.Level{
+				"pkg1": upgrade.None,
+				"pkg2": upgrade.None,
+				"pkg3": upgrade.Patch,
+				"pkg4": upgrade.Minor,
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			ctx, err := parseFlags(t, flags, tt.args)
+			if err != nil {
+				t.Fatalf("error parsing flags: %v", err)
+			}
+			config := parseUpgradeConfig(ctx, &reporter.VoidReporter{})
+			for pkg, want := range tt.want {
+				if got := config.Get(pkg); got != want {
+					t.Errorf("Config.Get(%s) got = %v, want %v", pkg, got, want)
+				}
+			}
+		})
+	}
+}

docs/guided-remediation.md

@@ -224,10 +224,26 @@ A vulnerability is only considered if it satisfies all the conditions set by the
 
 ### Dependency upgrade options
 
-The following flags may be used to limit the patches allowed for your dependencies:
+The following flag may be used to limit the patches allowed for your dependencies:
 
-- `--disallow-major-upgrades`: Do no allow patches that would result in the major version number of any dependency from being changed.
-- `--disallow-package-upgrades=<comma-separated list of package names>`: Do no allow patches to any of the listed packages.
+- `--upgrade-config=<[package-name:]level>` Sets the maximum upgrade level allowed for a package. Can be repeated for multiple packages.
+
+  `level` is the SemVer component to allow updates to, can be one of `major`, `minor`, `patch`, or `none`. e.g. If a package was at version `1.2.3`
+
+  - `major` allows for updates to any version `>=1.2.3`
+  - `minor` allows for updates `>=1.2.3, <2.0.0`
+  - `patch` allows for updates `>=1.2.3, <1.3.0`
+  - `none` disallows any updates
+
+  If `package-name:` is omitted, `level` is applied to all packages. The specific `package-name:level` will take precedence over the general `level` (e.g. specifying both `minor` `pkg:none` will use `none` as the allowed level for `pkg`).
+
+  Default behaviour is `--upgrade-config=major`.
+
+  Example usage:
+
+  - `--upgrade-config=minor` - disallow any patches that would bump a major version of any package.
+  - `--upgrade-config=foo:minor` - disallow any patches that bumps package `foo` by a major version. Other packages may receive major version-updating patches.
+  - `--upgrade-config=none --upgrade-config=foo:patch` - only allow patches to package `foo`, and only allow changes to `foo`'s SemVer patch level.
 
 ### Data source
 

internal/remediation/in_place.go

@@ -9,6 +9,7 @@ import (
 	"deps.dev/util/resolve"
 	"deps.dev/util/resolve/dep"
 	"deps.dev/util/semver"
+	"github.com/google/osv-scanner/internal/remediation/upgrade"
 	"github.com/google/osv-scanner/internal/resolution"
 	"github.com/google/osv-scanner/internal/resolution/client"
 	lf "github.com/google/osv-scanner/internal/resolution/lockfile"
@@ -135,17 +136,15 @@ func ComputeInPlacePatches(ctx context.Context, cl client.ResolutionClient, grap
 				continue
 			}
 			// Consider vulns affecting packages we don't want to change unfixable
-			if slices.Contains(opts.AvoidPkgs, vk.Name) {
+			if opts.UpgradeConfig.Get(vk.Name) == upgrade.None {
 				result.Unfixable = append(result.Unfixable, vuln)
 				continue
 			}
 			newVK, err := findFixedVersion(ctx, cl, vk.PackageKey, func(newVK resolve.VersionKey) bool {
-				// Check if this is a disallowed major version bump
-				if !opts.AllowMajor {
-					_, diff, err := vk.Semver().Difference(vk.Version, newVK.Version)
-					if err != nil || diff == semver.DiffMajor {
-						return false
-					}
+				// Check if this is a disallowed version bump
+				_, diff, err := vk.Semver().Difference(vk.Version, newVK.Version)
+				if err != nil || !opts.UpgradeConfig.Get(vk.Name).Allows(diff) {
+					return false
 				}
 				// Check if dependent packages are still satisfied by new version
 				ok, err := vkDependentConstraint[vk].Match(newVK.Version)

internal/remediation/in_place_test.go

@@ -8,6 +8,7 @@ import (
 
 	"deps.dev/util/resolve"
 	"github.com/google/osv-scanner/internal/remediation"
+	"github.com/google/osv-scanner/internal/remediation/upgrade"
 	"github.com/google/osv-scanner/internal/resolution"
 	"github.com/google/osv-scanner/internal/resolution/client"
 	"github.com/google/osv-scanner/internal/resolution/clienttest"
@@ -111,9 +112,9 @@ func TestComputeInPlacePatches(t *testing.T) {
 	t.Parallel()
 
 	basicOpts := remediation.RemediationOptions{
-		DevDeps:    true,
-		MaxDepth:   -1,
-		AllowMajor: true,
+		DevDeps:       true,
+		MaxDepth:      -1,
+		UpgradeConfig: upgrade.NewConfig(),
 	}
 
 	tests := []struct {

internal/remediation/override.go

@@ -8,8 +8,8 @@ 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/remediation/upgrade"
 	"github.com/google/osv-scanner/internal/resolution"
 	"github.com/google/osv-scanner/internal/resolution/client"
 	resolutionmanifest "github.com/google/osv-scanner/internal/resolution/manifest"
@@ -169,7 +169,7 @@ func overridePatchVulns(ctx context.Context, cl client.ResolutionClient, result
 		// For each VersionKey, try fix as many of the vulns affecting it as possible.
 		for vk, vulnerabilities := range vkVulns {
 			// Consider vulns affecting packages we don't want to change unfixable
-			if slices.Contains(opts.AvoidPkgs, vk.Name) {
+			if opts.UpgradeConfig.Get(vk.Name) == upgrade.None {
 				continue
 			}
 
@@ -182,11 +182,9 @@ func overridePatchVulns(ctx context.Context, cl client.ResolutionClient, result
 
 			// Find the minimal greater version that fixes as many vulnerabilities as possible.
 			for _, ver := range versions {
-				if !opts.AllowMajor {
-					// Major version updates are not allowed - break if we've encountered a major update.
-					if _, diff, _ := vk.System.Semver().Difference(vk.Version, ver.Version); diff == semver.DiffMajor {
-						break
-					}
+				// Break if we've encountered a disallowed version update.
+				if _, diff, _ := vk.System.Semver().Difference(vk.Version, ver.Version); !opts.UpgradeConfig.Get(vk.Name).Allows(diff) {
+					break
 				}
 
 				// Count the remaining known vulns that affect this version.

internal/remediation/override_test.go

@@ -5,15 +5,16 @@ import (
 	"testing"
 
 	"github.com/google/osv-scanner/internal/remediation"
+	"github.com/google/osv-scanner/internal/remediation/upgrade"
 )
 
 func TestComputeOverridePatches(t *testing.T) {
 	t.Parallel()
 
 	basicOpts := remediation.RemediationOptions{
-		DevDeps:    true,
-		MaxDepth:   -1,
-		AllowMajor: true,
+		DevDeps:       true,
+		MaxDepth:      -1,
+		UpgradeConfig: upgrade.NewConfig(),
 	}
 
 	tests := []struct {