feat: Make skip git the default behavior (#1584)

another-rex · web-flow · commit fd2ef0f49ca5 · 2025-02-07T16:42:58.000+11:00
Also removes the `--json` flag since that has been deprecated for a while now. The json tests are not deleted, as there are the same tests with the `--format json` flag. Succeeds: #1311 Closes: #1277
diff --git a/.github/workflows/osv-scanner-reusable-pr.yml b/.github/workflows/osv-scanner-reusable-pr.yml
@@ -26,7 +26,6 @@ on:
         type: string
         default: |-
           -r
-          --skip-git
           ./
       results-file-name:
         description: "File name of the result SARIF file"
diff --git a/.github/workflows/osv-scanner-reusable.yml b/.github/workflows/osv-scanner-reusable.yml
@@ -26,7 +26,6 @@ on:
         type: string
         default: |-
           -r
-          --skip-git
           ./
       results-file-name:
         description: "File name of the result SARIF file"
diff --git a/.github/workflows/osv-scanner-unified-action.yml b/.github/workflows/osv-scanner-unified-action.yml
@@ -38,7 +38,6 @@ jobs:
     with:
       # Just scan the root directory and docs, since everything else is fixtures
       scan-args: |-
-        --skip-git
         ./
         ./docs/
   scan-pr:
@@ -52,6 +51,5 @@ jobs:
     with:
       # Just scan the root directory and docs, since everything else is fixtures
       scan-args: |-
-        --skip-git
         ./
         ./docs/
diff --git a/.github/workflows/prerelease-check.yml b/.github/workflows/prerelease-check.yml
@@ -27,7 +27,6 @@ jobs:
       # Only scan the top level go.mod file without recursively scanning directories since
       # this is pipeline is about releasing the go module and binary
       scan-args: |-
-        --skip-git
         ./
 
   format:
diff --git a/actions/scanner/action.yml b/actions/scanner/action.yml
@@ -5,7 +5,6 @@ inputs:
   scan-args:
     description: "Arguments to osv-scanner, separated by new line"
     default: |-
-      --skip-git
       --recursive
       ./
 runs:
diff --git a/cmd/osv-scanner/__snapshots__/main_test.snap b/cmd/osv-scanner/__snapshots__/main_test.snap
@@ -873,7 +873,7 @@ invalid verbosity level "unknown" - must be one of: error, warn, info, verbose
 
 ---
 
-[TestRun/json_output_1 - 1]
+[TestRun/json_output - 1]
 {
   "results": [],
   "experimental_config": {
@@ -886,27 +886,7 @@ invalid verbosity level "unknown" - must be one of: error, warn, info, verbose
 
 ---
 
-[TestRun/json_output_1 - 2]
-Scanning dir ./fixtures/locks-many/composer.lock
-Scanned <rootdir>/fixtures/locks-many/composer.lock file and found 1 package
-Loaded filter from: <rootdir>/fixtures/locks-many/osv-scanner.toml
-
----
-
-[TestRun/json_output_2 - 1]
-{
-  "results": [],
-  "experimental_config": {
-    "licenses": {
-      "summary": false,
-      "allowlist": null
-    }
-  }
-}
-
----
-
-[TestRun/json_output_2 - 2]
+[TestRun/json_output - 2]
 Scanning dir ./fixtures/locks-many/composer.lock
 Scanned <rootdir>/fixtures/locks-many/composer.lock file and found 1 package
 Loaded filter from: <rootdir>/fixtures/locks-many/osv-scanner.toml
@@ -2633,48 +2613,6 @@ Loaded Packagist local db from <tempdir>/osv-scanner/Packagist/all.zip
 
 ---
 
-[TestRun_LocalDatabases/output_with_json#01 - 1]
-{
-  "results": [],
-  "experimental_config": {
-    "licenses": {
-      "summary": false,
-      "allowlist": null
-    }
-  }
-}
-
----
-
-[TestRun_LocalDatabases/output_with_json#01 - 2]
-Scanning dir ./fixtures/locks-many/composer.lock
-Scanned <rootdir>/fixtures/locks-many/composer.lock file and found 1 package
-Loaded filter from: <rootdir>/fixtures/locks-many/osv-scanner.toml
-Loaded Packagist local db from <tempdir>/osv-scanner/Packagist/all.zip
-
----
-
-[TestRun_LocalDatabases/output_with_json#01 - 3]
-{
-  "results": [],
-  "experimental_config": {
-    "licenses": {
-      "summary": false,
-      "allowlist": null
-    }
-  }
-}
-
----
-
-[TestRun_LocalDatabases/output_with_json#01 - 4]
-Scanning dir ./fixtures/locks-many/composer.lock
-Scanned <rootdir>/fixtures/locks-many/composer.lock file and found 1 package
-Loaded filter from: <rootdir>/fixtures/locks-many/osv-scanner.toml
-Loaded Packagist local db from <tempdir>/osv-scanner/Packagist/all.zip
-
----
-
 [TestRun_LocalDatabases_AlwaysOffline/a_bunch_of_different_lockfiles_and_ecosystem - 1]
 Scanning dir ./fixtures/locks-requirements
 Scanned <rootdir>/fixtures/locks-requirements/my-requirements.txt file and found 1 package
diff --git a/cmd/osv-scanner/internal/helper/helper.go b/cmd/osv-scanner/internal/helper/helper.go
@@ -21,7 +21,7 @@ import (
 
 // flags that require network access and values to disable them.
 var OfflineFlags = map[string]string{
-	"skip-git":                             "true",
+	"include-git-root":                     "true",
 	"experimental-offline-vulnerabilities": "true",
 	"experimental-no-resolve":              "true",
 	"experimental-licenses-summary":        "false",
diff --git a/cmd/osv-scanner/main_test.go b/cmd/osv-scanner/main_test.go
@@ -244,14 +244,8 @@ func TestRun(t *testing.T) {
 			args: []string{"", "--recursive", "--no-ignore", "./fixtures/locks-gitignore"},
 			exit: 0,
 		},
-		// output with json
 		{
-			name: "json output 1",
-			args: []string{"", "--json", "./fixtures/locks-many/composer.lock"},
-			exit: 0,
-		},
-		{
-			name: "json output 2",
+			name: "json output",
 			args: []string{"", "--format", "json", "./fixtures/locks-many/composer.lock"},
 			exit: 0,
 		},
@@ -631,11 +625,6 @@ func TestRun_LocalDatabases(t *testing.T) {
 			args: []string{"", "--experimental-offline", "--experimental-download-offline-databases", "--recursive", "--no-ignore", "./fixtures/locks-gitignore"},
 			exit: 0,
 		},
-		{
-			name: "output with json",
-			args: []string{"", "--experimental-offline", "--experimental-download-offline-databases", "--json", "./fixtures/locks-many/composer.lock"},
-			exit: 0,
-		},
 		{
 			name: "output with json",
 			args: []string{"", "--experimental-offline", "--experimental-download-offline-databases", "--format", "json", "./fixtures/locks-many/composer.lock"},
diff --git a/cmd/osv-scanner/scan/image/main.go b/cmd/osv-scanner/scan/image/main.go
@@ -76,7 +76,7 @@ func action(context *cli.Context, stdout, stderr io.Writer) (reporter.Reporter,
 		Image:                      context.Args().First(),
 		ConfigOverridePath:         context.String("config"),
 		IsImageArchive:             context.Bool("archive"),
-		SkipGit:                    context.Bool("skip-git"),
+		IncludeGitRoot:             context.Bool("include-git-root"),
 		ExperimentalScannerActions: helper.GetExperimentalScannerActions(context, scanLicensesAllowlist),
 	}
 
diff --git a/cmd/osv-scanner/scan/source/main.go b/cmd/osv-scanner/scan/source/main.go
@@ -27,15 +27,6 @@ var projectScanFlags = []cli.Flag{
 		Usage:     "scan sbom file on this path",
 		TakesFile: true,
 	},
-	&cli.BoolFlag{
-		Name:  "json",
-		Usage: "sets output to json (deprecated, use --format json instead)",
-	},
-	&cli.BoolFlag{
-		Name:  "skip-git",
-		Usage: "skip scanning git repositories",
-		Value: false,
-	},
 	&cli.BoolFlag{
 		Name:    "recursive",
 		Aliases: []string{"r"},
@@ -55,6 +46,11 @@ var projectScanFlags = []cli.Flag{
 		Name:  "no-call-analysis",
 		Usage: "disables call graph analysis",
 	},
+	&cli.BoolFlag{
+		Name:  "include-git-root",
+		Usage: "include scanning git root (non-submoduled) repositories",
+		Value: false,
+	},
 }
 
 var projectScanExperimentalFlags = []cli.Flag{
@@ -106,10 +102,6 @@ func Command(stdout, stderr io.Writer, r *reporter.Reporter) *cli.Command {
 func Action(context *cli.Context, stdout, stderr io.Writer) (reporter.Reporter, error) {
 	format := context.String("format")
 
-	if context.Bool("json") {
-		format = "json"
-	}
-
 	outputPath := context.String("output")
 	serve := context.Bool("serve")
 	if serve {
@@ -158,7 +150,7 @@ func Action(context *cli.Context, stdout, stderr io.Writer) (reporter.Reporter,
 		LockfilePaths:              context.StringSlice("lockfile"),
 		SBOMPaths:                  context.StringSlice("sbom"),
 		Recursive:                  context.Bool("recursive"),
-		SkipGit:                    context.Bool("skip-git"),
+		IncludeGitRoot:             context.Bool("include-git-root"),
 		NoIgnore:                   context.Bool("no-ignore"),
 		ConfigOverridePath:         context.String("config"),
 		DirectoryPaths:             context.Args().Slice(),
diff --git a/docs/github-action.md b/docs/github-action.md
@@ -138,7 +138,6 @@ jobs:
       # Only scan the top level go.mod file without recursively scanning directories since
       # this is pipeline is about releasing the go module and binary
       scan-args: |-
-        --skip-git
         ./
     permissions:
       # Require writing security events to upload SARIF file to security tab
@@ -167,7 +166,6 @@ The GitHub Actions have the following optional inputs:
   Default:
   ```bash
     --recursive # Recursively scan subdirectories
-    --skip-git=true # Skip commit scanning to focus on dependencies
     ./ # Start the scan from the root of the repository
   ```
 - `results-file-name`: This is the name of the final SARIF file uploaded to Github.
@@ -202,7 +200,6 @@ jobs:
     with:
       scan-args: |-
         --recursive
-        --skip-git=true
         ./
 ```
 
diff --git a/docs/scan-source.md b/docs/scan-source.md
@@ -30,8 +30,6 @@ The preceding command will find lockfiles, SBOMs, and git directories in your ta
 
 The recursive flag `-r` or `--recursive` will tell the scanner to search all subdirectories in addition to the specified directory. It can find additional lockfiles, dependencies, and vulnerabilities. If your project has deeply nested subdirectories, a recursive search may take a long time.
 
-Git directories are searched for the latest commit hash. Searching for git commit hash is intended to work with projects that use git submodules or a similar mechanism where dependencies are checked out as real git repositories.
-
 ## Ignored files
 
 By default, OSV-Scanner will not scan files that are ignored by `.gitignore` files. All recursively scanned files are matched to a git repository (if it exists) and any matching `.gitignore` files within that repository are taken into account.
@@ -87,6 +85,8 @@ osv-scanner scan source --lockfile ':/path/to/my:projects/package-lock.json'
 
 OSV-Scanner will automatically scan git submodules and vendored directories for C/C++ code and try to attribute them to specific dependencies and versions. See [C/C++ Scanning](<supported_languages_and_lockfiles#C/C++ scanning>) for more details.
 
+By default, root git directories (i.e. git repositories that are not a submodule of a bigger git repo) are skipped. You can include those repositories by setting the `--include-git-root` flag.
+
 ## Scanning with call analysis
 
 Call stack analysis can be performed on some languages to check if the
diff --git a/internal/scalibrextract/vcs/gitrepo/extractor.go b/internal/scalibrextract/vcs/gitrepo/extractor.go
@@ -14,7 +14,9 @@ import (
 
 // Extractor extracts git repository hashes including submodule hashes.
 // This extractor will not return an error, and will just return no results if we fail to extract
-type Extractor struct{}
+type Extractor struct {
+	IncludeRootGit bool
+}
 
 var _ filesystem.Extractor = Extractor{}
 
@@ -96,12 +98,14 @@ func (e Extractor) Extract(_ context.Context, input *filesystem.ScanInput) ([]*e
 	//nolint:prealloc // Not sure how many there will be in advance.
 	var packages []*extractor.Inventory
 
-	commitSHA, err := getCommitSHA(repo)
+	if e.IncludeRootGit {
+		commitSHA, err := getCommitSHA(repo)
 
-	// If error is not nil, then ignore this and continue, as it is not fatal.
-	// The error could be because there are no commits in the repository
-	if err == nil {
-		packages = append(packages, createCommitQueryInventory(commitSHA, input.Path))
+		// If error is not nil, then ignore this and continue, as it is not fatal.
+		// The error could be because there are no commits in the repository
+		if err == nil {
+			packages = append(packages, createCommitQueryInventory(commitSHA, input.Path))
+		}
 	}
 
 	// If we can't get submodules, just return with what we have.
diff --git a/internal/scalibrextract/vcs/gitrepo/extractor_test.go b/internal/scalibrextract/vcs/gitrepo/extractor_test.go
@@ -50,7 +50,9 @@ func TestExtractor_Extract(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.Name, func(t *testing.T) {
 			t.Parallel()
-			extr := gitrepo.Extractor{}
+			extr := gitrepo.Extractor{
+				IncludeRootGit: true,
+			}
 			parent := filepath.Dir(tt.InputConfig.Path)
 			err := os.Rename(path.Join(parent, "git-hidden"), path.Join(parent, ".git"))
 			if err != nil {
diff --git a/pkg/osvscanner/internal/scanners/extractorbuilder.go b/pkg/osvscanner/internal/scanners/extractorbuilder.go
@@ -126,21 +126,24 @@ func BuildSBOMExtractors() []filesystem.Extractor {
 // BuildWalkerExtractors returns all relevant extractors for directory scanning given the required clients
 // All clients can be nil, and if nil the extractors requiring those clients will not be returned.
 func BuildWalkerExtractors(
-	skipGit bool,
+	includeRootGit bool,
 	osvdevClient *osvdev.OSVClient,
 	dependencyClients map[osvschema.Ecosystem]client.DependencyClient,
 	mavenAPIClient *datasource.MavenRegistryAPIClient) []filesystem.Extractor {
 	relevantExtractors := []filesystem.Extractor{}
 
-	if !skipGit {
-		relevantExtractors = append(relevantExtractors, gitrepo.Extractor{})
+	if includeRootGit {
+		relevantExtractors = append(relevantExtractors, gitrepo.Extractor{
+			IncludeRootGit: includeRootGit,
+		})
 	}
 	relevantExtractors = append(relevantExtractors, lockfileExtractors...)
 	relevantExtractors = append(relevantExtractors, sbomExtractors...)
 
 	if osvdevClient != nil {
 		relevantExtractors = append(relevantExtractors, vendored.Extractor{
-			ScanGitDir: skipGit,
+			// Only attempt to vendor check git directories if we are not skipping scanning root git directories
+			ScanGitDir: !includeRootGit,
 			OSVClient:  osvdevClient,
 		})
 	}
diff --git a/pkg/osvscanner/osvscanner.go b/pkg/osvscanner/osvscanner.go
@@ -39,7 +39,7 @@ type ScannerActions struct {
 	DirectoryPaths     []string
 	GitCommits         []string
 	Recursive          bool
-	SkipGit            bool
+	IncludeGitRoot     bool
 	NoIgnore           bool
 	Image              string
 	IsImageArchive     bool
@@ -182,8 +182,6 @@ func DoScan(actions ScannerActions, r reporter.Reporter) (models.VulnerabilityRe
 	// --- Sanity check flags ----
 	// TODO(v2): Move the logic of the offline flag changing other flags into here from the main.go/scan.go
 	if actions.CompareOffline {
-		actions.SkipGit = true
-
 		if len(actions.ScanLicensesAllowlist) > 0 || actions.ScanLicensesSummary {
 			return models.VulnerabilityResults{}, errors.New("cannot retrieve licenses locally")
 		}
diff --git a/pkg/osvscanner/scan.go b/pkg/osvscanner/scan.go
@@ -37,7 +37,7 @@ func scan(r reporter.Reporter, accessors ExternalAccessors, actions ScannerActio
 
 	// --- Directories ---
 	dirExtractors := scanners.BuildWalkerExtractors(
-		actions.SkipGit,
+		actions.IncludeGitRoot,
 		accessors.OSVDevClient,
 		accessors.DependencyClients,
 		accessors.MavenRegistryAPIClient,

Original file line number	Diff line number	Diff line change
`@@ -76,7 +76,7 @@ func action(context *cli.Context, stdout, stderr io.Writer) (reporter.Reporter,`
`76`	`76`	`Image: context.Args().First(),`
`77`	`77`	`ConfigOverridePath: context.String("config"),`
`78`	`78`	`IsImageArchive: context.Bool("archive"),`
`79`		`- SkipGit: context.Bool("skip-git"),`
	`79`	`+ IncludeGitRoot: context.Bool("include-git-root"),`
`80`	`80`	`ExperimentalScannerActions: helper.GetExperimentalScannerActions(context, scanLicensesAllowlist),`
`81`	`81`	`}`
`82`	`82`