Fix SARIF formatter silently dropping findings and missing locations (#393)

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

Signed-off-by: Mikaël Barbero <mikael.barbero@eclipse-foundation.org>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>

Author: 3 people

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...)
+				}
 			}
 		}
 

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{

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)
 }

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)

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{},