Repository: google/osv-scanner

Issue:
[Github action] exit code 0 returned on incorrect cli usage
When using the `osv-scanner` GitHub action incorrectly by providing non existing args providing args after the scan path, the action succeeds. The reason: `127` exit code is redirected to `0` [here](https://github.com/google/osv-scanner/blob/main/exit_code_redirect.sh#L45-L47).

Real example:
`--config` arg is provided after the scan path [here](https://github.com/envoyproxy/gateway/blob/71519afe106178f97066e777fcbfc6879edbf439/.github/workflows/osv-scanner.yml#L30C8-L30C17) but action succeeds although there are errors in the log:
```log
Unable to parse git ignores: stat --config: no such file or directory
Failed to walk --config: lstat --config: no such file or directory
lstat --config: no such file or directory
Exit code: 127
``` 

Discussion:
@github-actions: This issue has not had any activity for 60 days and will be automatically closed in two weeks

See https://github.com/google/osv-scanner/blob/main/CONTRIBUTING.md for how to contribute a PR if you're interested in helping out.@shahar-h: Another example: I tried to use the new `vulnerability.ignore` feature that was added in `v1.9.1`, but got an error since I used osv-scanner v1.9.0:
```log
Failed to read config file: unknown keys in config file: PackageOverrides.vulnerability.ignore
unknown keys in config file: PackageOverrides.vulnerability.ignore
```
The error code here is also 127 so the github action succeeds also in this case.@github-actions: This issue has not had any activity for 60 days and will be automatically closed in two weeks

See https://github.com/google/osv-scanner/blob/main/CONTRIBUTING.md for how to contribute a PR if you're interested in helping out.@another-rex: I think two changes are needed here:
1. 127 should not redirect to 0,
2. Return special error code for user config error, (invalid config, incorrect CLI usage...etc)@G-Rath: > Return special error code for user config error, (invalid config, incorrect CLI usage...etc)

I don't actually think we can easily do this because it doesn't look like a specific error is returned by the CLI library, and at least in the scope of this issue I don't actually know why we'd want to single that case out - even if it was to show some GH annotation, I think that'd be something we could do for all errors 🤷 @another-rex: This is now released in v2.0.3

Solution patch:
diff --git a/.golangci.yaml b/.golangci.yaml
index 8a4aadb3..00ad4b10 100644
--- a/.golangci.yaml
+++ b/.golangci.yaml
@@ -113,7 +113,6 @@ linters:
       checks:
         - all
         - -QF1006 # lift into loop condition
-        - -QF1008 # remove embedded field from selector
   exclusions:
     generated: lax
     presets:
diff --git a/cmd/osv-reporter/main.go b/cmd/osv-reporter/main.go
index 53e58228..62d5886e 100644
--- a/cmd/osv-reporter/main.go
+++ b/cmd/osv-reporter/main.go
@@ -182,7 +182,15 @@ func run(args []string, stdout, stderr io.Writer) int {
 		},
 	}
 
-	if err := app.Run(context.Background(), args); err != nil {
+	err := app.Run(context.Background(), args)
+
+	// if the config is invalid, it's possible that is why any other errors
+	// happened so that exit code takes priority
+	if logger.HasErroredBecauseInvalidConfig() {
+		return 130
+	}
+
+	if err != nil {
 		if errors.Is(err, osvscanner.ErrVulnerabilitiesFound) {
 			return 1
 		}
diff --git a/cmd/osv-scanner/fix/noninteractive.go b/cmd/osv-scanner/fix/noninteractive.go
index c011ee22..521365c1 100644
--- a/cmd/osv-scanner/fix/noninteractive.go
+++ b/cmd/osv-scanner/fix/noninteractive.go
@@ -295,7 +295,7 @@ func autoOverride(ctx context.Context, opts osvFixOptions, maxUpgrades int) erro
 					SnapshotsEnabled: repo.Snapshots.Enabled.Boolean(),
 				}
 			}
-			if err := opts.Client.DependencyClient.AddRegistries(registries); err != nil {
+			if err := opts.Client.AddRegistries(registries); err != nil {
 				return err
 			}
 		}
diff --git a/cmd/osv-scanner/internal/cmd/run.go b/cmd/osv-scanner/internal/cmd/run.go
index 2a79f351..c40631ef 100644
--- a/cmd/osv-scanner/internal/cmd/run.go
+++ b/cmd/osv-scanner/internal/cmd/run.go
@@ -90,7 +90,15 @@ func Run(args []string, stdout, stderr io.Writer, commands []CommandBuilder) int
 
 	args = insertDefaultCommand(args, app.Commands, app.DefaultCommand, stderr)
 
-	if err := app.Run(context.Background(), args); err != nil {
+	err := app.Run(context.Background(), args)
+
+	// if the config is invalid, it's possible that is why any other errors
+	// happened so that exit code takes priority
+	if logHandler.HasErroredBecauseInvalidConfig() {
+		return 130
+	}
+
+	if err != nil {
 		switch {
 		case errors.Is(err, osvscanner.ErrVulnerabilitiesFound):
 			return 1
diff --git a/internal/cmdlogger/handler.go b/internal/cmdlogger/handler.go
index c3bdce68..cedab555 100644
--- a/internal/cmdlogger/handler.go
+++ b/internal/cmdlogger/handler.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"io"
 	"log/slog"
+	"strings"
 )
 
 type Handler struct {
@@ -13,6 +14,8 @@ type Handler struct {
 	hasErrored         bool
 	everythingToStderr bool
 	Level              slog.Leveler
+
+	hasErroredBecauseInvalidConfig bool
 }
 
 // SendEverythingToStderr tells the logger to send all logs to stderr regardless
@@ -47,6 +50,10 @@ func (c *Handler) Enabled(_ context.Context, level slog.Level) bool {
 func (c *Handler) Handle(_ context.Context, record slog.Record) error {
 	if record.Level == slog.LevelError {
 		c.hasErrored = true
+
+		if strings.HasPrefix(record.Message, "Ignored invalid config file") {
+			c.hasErroredBecauseInvalidConfig = true
+		}
 	}
 
 	_, err := fmt.Fprint(c.writer(record.Level), record.Message+"\n")
@@ -60,6 +67,12 @@ func (c *Handler) HasErrored() bool {
 	return c.hasErrored
 }
 
+// HasErroredBecauseInvalidConfig returns true if there have been any calls to
+// Handle with a level of [slog.LevelError] due to a config file being invalid
+func (c *Handler) HasErroredBecauseInvalidConfig() bool {
+	return c.hasErroredBecauseInvalidConfig
+}
+
 func (c *Handler) WithAttrs(_ []slog.Attr) slog.Handler {
 	panic("not supported")
 }
diff --git a/internal/cmdlogger/interface.go b/internal/cmdlogger/interface.go
index 3ac577c0..85e031ce 100644
--- a/internal/cmdlogger/interface.go
+++ b/internal/cmdlogger/interface.go
@@ -6,6 +6,7 @@ type CmdLogger interface {
 	slog.Handler
 	SendEverythingToStderr()
 	HasErrored() bool
+	HasErroredBecauseInvalidConfig() bool
 	SetLevel(level slog.Leveler)
 }
 
diff --git a/internal/imodels/imodels.go b/internal/imodels/imodels.go
index 08d417ff..e8bb334e 100644
--- a/internal/imodels/imodels.go
+++ b/internal/imodels/imodels.go
@@ -83,7 +83,7 @@ func (pkg *PackageInfo) Name() string {
 	}
 
 	// Patch Maven archive extractor package names
-	if metadata, ok := pkg.Package.Metadata.(*archivemetadata.Metadata); ok {
+	if metadata, ok := pkg.Metadata.(*archivemetadata.Metadata); ok {
 		// Debian uses source name on osv.dev
 		// (fallback to using the normal name if source name is empty)
 		if metadata.ArtifactID != "" && metadata.GroupID != "" {
@@ -92,7 +92,7 @@ func (pkg *PackageInfo) Name() string {
 	}
 
 	// --- OS metadata ---
-	if metadata, ok := pkg.Package.Metadata.(*dpkgmetadata.Metadata); ok {
+	if metadata, ok := pkg.Metadata.(*dpkgmetadata.Metadata); ok {
 		// Debian uses source name on osv.dev
 		// (fallback to using the normal name if source name is empty)
 		if metadata.SourceName != "" {
@@ -100,7 +100,7 @@ func (pkg *PackageInfo) Name() string {
 		}
 	}
 
-	if metadata, ok := pkg.Package.Metadata.(*apkmetadata.Metadata); ok {
+	if metadata, ok := pkg.Metadata.(*apkmetadata.Metadata); ok {
 		if metadata.OriginName != "" {
 			return metadata.OriginName
 		}
@@ -158,27 +158,27 @@ func (pkg *PackageInfo) Version() string {
 }
 
 func (pkg *PackageInfo) Location() string {
-	if len(pkg.Package.Locations) > 0 {
-		return pkg.Package.Locations[0]
+	if len(pkg.Locations) > 0 {
+		return pkg.Locations[0]
 	}
 
 	return ""
 }
 
 func (pkg *PackageInfo) Commit() string {
-	if pkg.Package.SourceCode != nil {
-		return pkg.Package.SourceCode.Commit
+	if pkg.SourceCode != nil {
+		return pkg.SourceCode.Commit
 	}
 
 	return ""
 }
 
 func (pkg *PackageInfo) SourceType() models.SourceType {
-	if len(pkg.Package.Plugins) == 0 {
+	if len(pkg.Plugins) == 0 {
 		return models.SourceTypeUnknown
 	}
 
-	for _, extractorName := range pkg.Package.Plugins {
+	for _, extractorName := range pkg.Plugins {
 		if _, ok := osExtractors[extractorName]; ok {
 			return models.SourceTypeOSPackage
 		} else if _, ok := sbomExtractors[extractorName]; ok {
@@ -194,7 +194,7 @@ func (pkg *PackageInfo) SourceType() models.SourceType {
 }
 
 func (pkg *PackageInfo) DepGroups() []string {
-	if dg, ok := pkg.Package.Metadata.(scalibrosv.DepGroups); ok {
+	if dg, ok := pkg.Metadata.(scalibrosv.DepGroups); ok {
 		return dg.DepGroups()
 	}
 
@@ -202,13 +202,13 @@ func (pkg *PackageInfo) DepGroups() []string {
 }
 
 func (pkg *PackageInfo) OSPackageName() string {
-	if metadata, ok := pkg.Package.Metadata.(*apkmetadata.Metadata); ok {
+	if metadata, ok := pkg.Metadata.(*apkmetadata.Metadata); ok {
 		return metadata.PackageName
 	}
-	if metadata, ok := pkg.Package.Metadata.(*dpkgmetadata.Metadata); ok {
+	if metadata, ok := pkg.Metadata.(*dpkgmetadata.Metadata); ok {
 		return metadata.PackageName
 	}
-	if metadata, ok := pkg.Package.Metadata.(*rpmmetadata.Metadata); ok {
+	if metadata, ok := pkg.Metadata.(*rpmmetadata.Metadata); ok {
 		return metadata.PackageName
 	}
 
diff --git a/internal/remediation/override.go b/internal/remediation/override.go
index 48eb50ad..e8fcba96 100644
--- a/internal/remediation/override.go
+++ b/internal/remediation/override.go
@@ -239,7 +239,7 @@ func overridePatchVulns(ctx context.Context, cl client.ResolutionClient, result
 
 	// Sort the patches for deterministic output.
 	slices.SortFunc(effectivePatches, func(a, b overridePatch) int {
-		if c := a.PackageKey.Compare(b.PackageKey); c != 0 {
+		if c := a.Compare(b.PackageKey); c != 0 {
 			return c
 		}
 
diff --git a/internal/resolution/manifest/maven.go b/internal/resolution/manifest/maven.go
index 069c2639..1cb622c5 100644
--- a/internal/resolution/manifest/maven.go
+++ b/internal/resolution/manifest/maven.go
@@ -83,7 +83,7 @@ func (m MavenReadWriter) Read(df depfile.DepFile) (Manifest, error) {
 			VersionKey: resolve.VersionKey{
 				PackageKey: resolve.PackageKey{
 					System: resolve.Maven,
-					Name:   project.Parent.ProjectKey.Name(),
+					Name:   project.Parent.Name(),
 				},
 				// Parent version is a concrete version, but we model parent as dependency here.
 				VersionType: resolve.Requirement,
@@ -100,7 +100,7 @@ func (m MavenReadWriter) Read(df depfile.DepFile) (Manifest, error) {
 
 	// TODO: there may be properties in repo.Releases.Enabled and repo.Snapshots.Enabled
 	for _, repo := range project.Repositories {
-		if err := m.MavenRegistryAPIClient.AddRegistry(datasource.MavenRegistry{
+		if err := m.AddRegistry(datasource.MavenRegistry{
 			URL:              string(repo.URL),
 			ID:               string(repo.ID),
 			ReleasesEnabled:  repo.Releases.Enabled.Boolean(),
@@ -243,7 +243,7 @@ func buildOriginalRequirements(project maven.Project, originPrefix string) []Dep
 		for _, d := range plugin.Dependencies {
 			dependencies = append(dependencies, DependencyWithOrigin{
 				Dependency: d,
-				Origin:     mavenOrigin(originPrefix, mavenutil.OriginPlugin, plugin.ProjectKey.Name()),
+				Origin:     mavenOrigin(originPrefix, mavenutil.OriginPlugin, plugin.Name()),
 			})
 		}
 	}
@@ -805,7 +805,7 @@ func writeProject(w io.Writer, enc *internalxml.Encoder, raw, prefix, id string,
 				if err := dec.DecodeElement(&rawPlugin, &tt); err != nil {
 					return err
 				}
-				if err := writeProject(w, enc, "<plugin>"+rawPlugin.InnerXML+"</plugin>", mavenutil.OriginPlugin, rawPlugin.ProjectKey.Name(), patches, properties, updated); err != nil {
+				if err := writeProject(w, enc, "<plugin>"+rawPlugin.InnerXML+"</plugin>", mavenutil.OriginPlugin, rawPlugin.Name(), patches, properties, updated); err != nil {
 					return fmt.Errorf("updating profile: %w", err)
 				}
 
diff --git a/internal/tui/in-place-info.go b/internal/tui/in-place-info.go
index 866d89d4..774626d6 100644
--- a/internal/tui/in-place-info.go
+++ b/internal/tui/in-place-info.go
@@ -120,7 +120,7 @@ func (ip *inPlaceInfo) Update(msg tea.Msg) (ViewModel, tea.Cmd) {
 		case key.Matches(msg, Keys.Quit):
 			return ip, CloseViewModel
 		case key.Matches(msg, Keys.Select):
-			vuln := ip.vulns[ip.Model.Cursor()]
+			vuln := ip.vulns[ip.Cursor()]
 			ip.currVulnInfo = NewVulnInfo(vuln)
 			ip.currVulnInfo.Resize(ip.Width(), ip.Height())
 
diff --git a/internal/tui/vuln-list.go b/internal/tui/vuln-list.go
index 4b9c849b..fce63d6f 100644
--- a/internal/tui/vuln-list.go
+++ b/internal/tui/vuln-list.go
@@ -115,7 +115,7 @@ func (v *vulnList) Update(msg tea.Msg) (ViewModel, tea.Cmd) {
 		case key.Matches(msg, Keys.Quit):
 			return v, CloseViewModel
 		case key.Matches(msg, Keys.Select):
-			vuln := v.Model.SelectedItem().(vulnListItem)
+			vuln := v.SelectedItem().(vulnListItem)
 			v.currVulnInfo = NewVulnInfo(vuln.Vulnerability)
 			v.currVulnInfo.Resize(v.Width(), v.Height())
 
diff --git a/pkg/osvscanner/osvscanner.go b/pkg/osvscanner/osvscanner.go
index de74df19..81b88697 100644
--- a/pkg/osvscanner/osvscanner.go
+++ b/pkg/osvscanner/osvscanner.go
@@ -152,13 +152,13 @@ func initializeExternalAccessors(actions ScannerActions) (ExternalAccessors, err
 	externalAccessors.OSVDevClient = osvdev.DefaultClient()
 
 	// --- No Transitive Scanning ---
-	if actions.TransitiveScanningActions.Disabled {
+	if actions.Disabled {
 		return externalAccessors, nil
 	}
 
 	// --- Transitive Scanning Clients ---
 	externalAccessors.MavenRegistryAPIClient, err = datasource.NewMavenRegistryAPIClient(datasource.MavenRegistry{
-		URL:             actions.TransitiveScanningActions.MavenRegistry,
+		URL:             actions.MavenRegistry,
 		ReleasesEnabled: true,
 	}, "")
 
@@ -166,10 +166,10 @@ func initializeExternalAccessors(actions ScannerActions) (ExternalAccessors, err
 		return ExternalAccessors{}, err
 	}
 
-	if !actions.TransitiveScanningActions.NativeDataSource {
+	if !actions.NativeDataSource {
 		externalAccessors.DependencyClients[osvschema.EcosystemMaven], err = resolution.NewDepsDevClient(depsdev.DepsdevAPI, "osv-scanner_scan/"+version.OSVVersion)
 	} else {
-		externalAccessors.DependencyClients[osvschema.EcosystemMaven], err = resolution.NewMavenRegistryClient(actions.TransitiveScanningActions.MavenRegistry, "")
+		externalAccessors.DependencyClients[osvschema.EcosystemMaven], err = resolution.NewMavenRegistryClient(actions.MavenRegistry, "")
 	}
 
 	if err != nil {
@@ -371,7 +371,7 @@ func DoContainerScan(actions ScannerActions) (models.VulnerabilityResults, error
 	for _, psr := range scanResult.PackageScanResults {
 		if (strings.HasPrefix(psr.PackageInfo.Location(), "usr/") && psr.PackageInfo.Ecosystem().Ecosystem == osvschema.EcosystemGo) ||
 			strings.Contains(psr.PackageInfo.Location(), "dist-packages/") && psr.PackageInfo.Ecosystem().Ecosystem == osvschema.EcosystemPyPI {
-			psr.PackageInfo.Package.Annotations = append(psr.PackageInfo.Package.Annotations, extractor.InsideOSPackage)
+			psr.PackageInfo.Annotations = append(psr.PackageInfo.Annotations, extractor.InsideOSPackage)
 		}
 	}
 
diff --git a/pkg/osvscanner/vulnerability_result.go b/pkg/osvscanner/vulnerability_result.go
index a924bc9a..4136f1ec 100644
--- a/pkg/osvscanner/vulnerability_result.go
+++ b/pkg/osvscanner/vulnerability_result.go
@@ -135,7 +135,7 @@ func buildVulnerabilityResults(
 
 			groupedBySource[source].pvs = append(groupedBySource[source].pvs, pkg)
 			// Overwrite annotations as it should be the same for the same package.
-			groupedBySource[source].annotations = p.Package.Annotations
+			groupedBySource[source].annotations = p.Annotations
 		}
 	}
 



Test patch:
diff --git a/cmd/osv-scanner/scan/source/command_test.go b/cmd/osv-scanner/scan/source/command_test.go
index 33ce484e..e5a76714 100644
--- a/cmd/osv-scanner/scan/source/command_test.go
+++ b/cmd/osv-scanner/scan/source/command_test.go
@@ -243,7 +243,7 @@ func TestCommand(t *testing.T) {
 		{
 			Name: "config file is invalid",
 			Args: []string{"", "source", "./fixtures/config-invalid"},
-			Exit: 127,
+			Exit: 130,
 		},
 		// config file with unknown keys
 		{
diff --git a/internal/testlogger/handler.go b/internal/testlogger/handler.go
index 87550c6f..deda2bdb 100644
--- a/internal/testlogger/handler.go
+++ b/internal/testlogger/handler.go
@@ -108,6 +108,12 @@ func (tl *Handler) HasErrored() bool {
 	return tl.getLogger().HasErrored()
 }
 
+// HasErroredBecauseInvalidConfig returns true if there have been any calls to
+// Handle with a level of [slog.LevelError] due to a config file being invalid
+func (tl *Handler) HasErroredBecauseInvalidConfig() bool {
+	return tl.getLogger().HasErroredBecauseInvalidConfig()
+}
+
 func (tl *Handler) WithAttrs(attrs []slog.Attr) slog.Handler {
 	return tl.getLogger().WithAttrs(attrs)
 }
diff --git a/internal/testutility/mock_http.go b/internal/testutility/mock_http.go
index ba0a7662..e96057ff 100644
--- a/internal/testutility/mock_http.go
+++ b/internal/testutility/mock_http.go
@@ -25,7 +25,7 @@ func NewMockHTTPServer(t *testing.T) *MockHTTPServer {
 	t.Helper()
 	mock := &MockHTTPServer{response: make(map[string][]byte)}
 	mock.Server = httptest.NewServer(mock)
-	t.Cleanup(func() { mock.Server.Close() })
+	t.Cleanup(func() { mock.Close() })
 
 	return mock
 }