From cc8e8a72c18bb862db1ee753f5c384f62eea703b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mika=C3=ABl=20Barbero?= Date: Thu, 5 Mar 2026 14:14:28 +0100 Subject: [PATCH] Fix SARIF formatter silently dropping findings and missing locations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SARIF formatter only iterated PackageDependencies when collecting findings, causing all findings from BuildDependencies to be silently omitted. Additionally, two rego rules produced findings with purls that could never match a package in the formatter's purl-based lookup: - github_action_from_unverified_creator_used used the action purl as the finding identifier and emitted coarse "Used in N repo(s)" aggregates with no file path or line number, resulting in empty SARIF locations. Rewritten to emit per-step findings with path, line, job, step, and event triggers, keyed by pkg.purl. - known_vulnerability_in_build_platform used input.provider (e.g. "gitlab") as the finding purl. Changed to use pkg.purl so findings are discoverable by the formatter. The formatter now iterates both PackageDependencies and BuildDependencies for purl lookup, with deduplication via a seen set, and avoids append into a foreign slice's backing array. Fixes #390 Co-Authored-By: Claude Opus 4.6 Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Signed-off-by: Mikaël Barbero --- formatters/sarif/sarif.go | 15 +++- formatters/sarif/sarif_test.go | 86 +++++++++++++++++++ ...b_action_from_unverified_creator_used.rego | 35 ++++++-- ...known_vulnerability_in_build_platform.rego | 3 +- scanner/inventory_test.go | 78 ++++++++++++++++- 5 files changed, 199 insertions(+), 18 deletions(-) diff --git a/formatters/sarif/sarif.go b/formatters/sarif/sarif.go index f67e9df1..c4d84044 100644 --- a/formatters/sarif/sarif.go +++ b/formatters/sarif/sarif.go @@ -99,10 +99,17 @@ func (f *Format) Format(ctx context.Context, packages []*models.PackageInsights) } pkgFindings := findingsByPurl[pkg.Purl] - for _, depPurl := range pkg.PackageDependencies { - normalizedDepPurl := normalizePurl(depPurl) - if depFindings, exists := findingsByPurl[normalizedDepPurl]; exists { - pkgFindings = append(pkgFindings, depFindings...) + seenPurls := map[string]bool{normalizePurl(pkg.Purl): true} + for _, depSlice := range [][]string{pkg.PackageDependencies, pkg.BuildDependencies} { + for _, depPurl := range depSlice { + normalizedDepPurl := normalizePurl(depPurl) + if seenPurls[normalizedDepPurl] { + continue + } + seenPurls[normalizedDepPurl] = true + if depFindings, exists := findingsByPurl[normalizedDepPurl]; exists { + pkgFindings = append(pkgFindings, depFindings...) + } } } diff --git a/formatters/sarif/sarif_test.go b/formatters/sarif/sarif_test.go index 84dcb4c5..6203ab7d 100644 --- a/formatters/sarif/sarif_test.go +++ b/formatters/sarif/sarif_test.go @@ -13,6 +13,92 @@ import ( "github.com/stretchr/testify/require" ) +// TestSarifFormatBuildDependencyFindings validates that findings keyed by a +// build dependency purl are included in the SARIF output. This exercises +// the formatter's BuildDependencies lookup path. +func TestSarifFormatBuildDependencyFindings(t *testing.T) { + actionPurl := "pkg:githubactions/unverified-owner/some-action" + pkgPurl := "pkg:github/test/repo@main" + pkg := &models.PackageInsights{ + Purl: pkgPurl, + SourceGitRepo: "test/repo", + SourceGitRef: "main", + SourceScmType: "github", + BuildDependencies: []string{actionPurl + "@v1.0"}, + FindingsResults: results.FindingsResult{ + Findings: []results.Finding{ + { + RuleId: "github_action_from_unverified_creator_used", + // Finding keyed by the build dependency purl (version stripped + // by the rego rule). This tests the BuildDependencies lookup. + Purl: actionPurl, + Meta: results.FindingMeta{ + Path: ".github/workflows/ci.yml", + Line: 5, + Job: "build", + Step: "2", + Details: "unverified-owner/some-action@v1.0", + }, + }, + }, + Rules: map[string]results.Rule{ + "github_action_from_unverified_creator_used": { + Id: "github_action_from_unverified_creator_used", + Title: "Github Action from Unverified Creator used", + Description: "Usage of GitHub Actions from unverified creators was detected.", + Level: "note", + }, + }, + }, + } + + var buf bytes.Buffer + formatter := NewFormat(&buf, "1.0.0") + err := formatter.Format(context.Background(), []*models.PackageInsights{pkg}) + require.NoError(t, err) + + var sarifOutput map[string]interface{} + err = json.Unmarshal(buf.Bytes(), &sarifOutput) + require.NoError(t, err) + + runs, ok := sarifOutput["runs"].([]interface{}) + require.True(t, ok) + require.Len(t, runs, 1) + + run, ok := runs[0].(map[string]interface{}) + require.True(t, ok) + + sarifResults, ok := run["results"].([]interface{}) + require.True(t, ok, "results should be present in the SARIF run") + require.Len(t, sarifResults, 1, "build dependency finding should appear in SARIF output") + + result, ok := sarifResults[0].(map[string]interface{}) + require.True(t, ok) + require.Equal(t, "github_action_from_unverified_creator_used", result["ruleId"]) + + locations, ok := result["locations"].([]interface{}) + require.True(t, ok, "locations should be present") + require.Len(t, locations, 1) + + location, ok := locations[0].(map[string]interface{}) + require.True(t, ok) + + physicalLocation, ok := location["physicalLocation"].(map[string]interface{}) + require.True(t, ok) + + artifactLocation, ok := physicalLocation["artifactLocation"].(map[string]interface{}) + require.True(t, ok) + uri, ok := artifactLocation["uri"].(string) + require.True(t, ok, "uri should be a string") + require.Equal(t, ".github/workflows/ci.yml", uri, "uri should be the workflow file path") + + region, ok := physicalLocation["region"].(map[string]interface{}) + require.True(t, ok) + startLine, ok := region["startLine"].(float64) + require.True(t, ok, "startLine should be a number") + require.InDelta(t, float64(5), startLine, 0.0001, "startLine should match the finding line") +} + func TestSarifFormat(t *testing.T) { // Create a test package with findings pkg := &models.PackageInsights{ diff --git a/opa/rego/rules/github_action_from_unverified_creator_used.rego b/opa/rego/rules/github_action_from_unverified_creator_used.rego index a59f2e44..0e9641ef 100644 --- a/opa/rego/rules/github_action_from_unverified_creator_used.rego +++ b/opa/rego/rules/github_action_from_unverified_creator_used.rego @@ -17,17 +17,34 @@ github_verified_partners contains p if some p in ["1password", "42crunch", "acti # Consider input package namespaces as verified github_verified_partners contains input.packages[_].package_namespace -results contains poutine.finding( - rule, - repo_purl, - {"details": sprintf("Used in %d repo(s)", [count(unverified_github_actions[repo_purl])])}, -) - -unverified_github_actions[action_repo] contains pkg.purl if { +results contains poutine.finding(rule, pkg.purl, { + "path": workflow.path, + "line": step.lines.uses, + "job": job.id, + "step": i, + "details": step.uses, + "event_triggers": [event | event := workflow.events[j].name], +}) if { pkg := input.packages[_] - dep := array.concat(pkg.build_dependencies, pkg.package_dependencies)[_] + workflow := pkg.github_actions_workflows[_] + job := workflow.jobs[_] + step := job.steps[i] + dep := purl.parse_github_actions(step.uses, pkg.source_git_repo, pkg.source_git_ref) startswith(dep, "pkg:githubactions/") + not regex.match(sprintf("pkg:githubactions/(%s)/", [concat("|", github_verified_partners)]), dep) +} - action_repo := split(dep, "@")[0] +results contains poutine.finding(rule, pkg.purl, { + "path": action.path, + "line": step.lines.uses, + "step": i, + "details": step.uses, +}) if { + pkg := input.packages[_] + action := pkg.github_actions_metadata[_] + action.runs.using == "composite" + step := action.runs.steps[i] + dep := purl.parse_github_actions(step.uses, pkg.source_git_repo, pkg.source_git_ref) + startswith(dep, "pkg:githubactions/") not regex.match(sprintf("pkg:githubactions/(%s)/", [concat("|", github_verified_partners)]), dep) } diff --git a/opa/rego/rules/known_vulnerability_in_build_platform.rego b/opa/rego/rules/known_vulnerability_in_build_platform.rego index 5edbe334..69ce6e94 100644 --- a/opa/rego/rules/known_vulnerability_in_build_platform.rego +++ b/opa/rego/rules/known_vulnerability_in_build_platform.rego @@ -15,10 +15,11 @@ import rego.v1 rule := poutine.rule(rego.metadata.chain()) -results contains poutine.finding(rule, input.provider, { +results contains poutine.finding(rule, pkg.purl, { "osv_id": advisory.osv_id, "details": sprintf("Provider: %s", [input.provider]), }) if { + pkg := input.packages[_] advisory := advisories[input.provider][osv_id] regex.match("^[0-9]+(\\.[0-9]+)*?$", input.version) semver.constraint_check(advisory.vulnerable_version_ranges[_], input.version) diff --git a/scanner/inventory_test.go b/scanner/inventory_test.go index 1a3988d0..ab2f8d93 100644 --- a/scanner/inventory_test.go +++ b/scanner/inventory_test.go @@ -335,9 +335,38 @@ func TestFindings(t *testing.T) { }, { RuleId: "github_action_from_unverified_creator_used", - Purl: "pkg:githubactions/kartverket/github-workflows", + Purl: purl, Meta: results.FindingMeta{ - Details: "Used in 1 repo(s)", + Path: ".github/workflows/valid.yml", + Line: 35, + Job: "build", + Step: "4", + Details: "kartverket/github-workflows/.github/workflows/run-terraform.yml@main", + EventTriggers: []string{"push", "pull_request_target"}, + }, + }, + { + RuleId: "github_action_from_unverified_creator_used", + Purl: purl, + Meta: results.FindingMeta{ + Path: ".github/workflows/valid.yml", + Line: 39, + Job: "build", + Step: "5", + Details: "kartverket/github-workflows/.github/workflows/run-terraform.yml@v2.7.1", + EventTriggers: []string{"push", "pull_request_target"}, + }, + }, + { + RuleId: "github_action_from_unverified_creator_used", + Purl: purl, + Meta: results.FindingMeta{ + Path: ".github/workflows/valid.yml", + Line: 43, + Job: "build", + Step: "6", + Details: "kartverket/github-workflows/.github/workflows/run-terraform.yml@v2.2", + EventTriggers: []string{"push", "pull_request_target"}, }, }, { @@ -617,9 +646,14 @@ func TestFindings(t *testing.T) { }, { RuleId: "github_action_from_unverified_creator_used", - Purl: "pkg:githubactions/some/action", + Purl: purl, Meta: results.FindingMeta{ - Details: "Used in 1 repo(s)", + Path: ".github/workflows/test_new_fields.yml", + Line: 44, + Job: "vulnerable_checkout", + Step: "3", + Details: "some/action@v1", + EventTriggers: []string{"pull_request_target"}, }, }, { @@ -747,6 +781,42 @@ func TestRulesConfig(t *testing.T) { assert.Empty(t, labels) } +// TestKnownVulnerabilityInBuildPlatformFinding validates that findings from the +// known_vulnerability_in_build_platform rule use pkg.purl as the finding purl (not +// input.provider), so they are not silently dropped from SARIF output. +func TestKnownVulnerabilityInBuildPlatformFinding(t *testing.T) { + o, _ := opa.NewOpa(context.TODO(), &models.Config{ + Include: []models.ConfigInclude{}, + }) + // provider="gitlab", version="17.3.0" matches advisory PVE-2024-00001 (>=17.3, <17.3.3) + i := NewInventory(o, nil, "gitlab", "17.3.0") + purl := "pkg:github/org/owner" + pkg := &models.PackageInsights{ + Purl: purl, + SourceGitRepo: "org/owner", + SourceGitRef: "main", + } + _ = pkg.NormalizePurl() + + scannedPackage, err := i.ScanPackage(context.Background(), *pkg, "testdata") + require.NoError(t, err) + + var platformFinding *results.Finding + for idx, f := range scannedPackage.FindingsResults.Findings { + if f.RuleId == "known_vulnerability_in_build_platform" { + platformFinding = &scannedPackage.FindingsResults.Findings[idx] + break + } + } + + require.NotNil(t, platformFinding, "expected a known_vulnerability_in_build_platform finding") + // The finding purl must be pkg.purl (not input.provider like "gitlab"), so it + // is not dropped from SARIF output by the formatter's purl-based lookup. + assert.Equal(t, purl, platformFinding.Purl, "finding purl should be pkg.purl, not the provider string") + assert.NotEmpty(t, platformFinding.Meta.OsvId, "osv_id should be populated") + assert.NotEmpty(t, platformFinding.Meta.Details, "details should be populated") +} + func TestStructuredFindingFields(t *testing.T) { o, _ := opa.NewOpa(context.TODO(), &models.Config{ Include: []models.ConfigInclude{},