feat: introduce Sarif to UFM - #684
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
This comment has been minimized.
This comment has been minimized.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
| {{- /* TODO: Add properties section for SAST (coverage) and upload results | ||
| {{- $coverage := getCoverageFromTestResult $result }} | ||
| {{- $reportURL := index $metadata "report-url" }} | ||
| {{- if or $coverage $reportURL }} |
There was a problem hiding this comment.
Should Fix: This {{- if or $coverage $reportURL }} guard omits the entire run-level "properties" key whenever there's no coverage data and no report URL. local_finding.sarif.tmpl's equivalent block has no such guard — it unconditionally renders "properties": {"coverage": [...]} (empty array when there's no coverage). Since Test_UfmPresenter_SarifFromSarifInput exists specifically to validate UFM/LocalFindings SARIF-output parity, any input with zero coverage entries and no report metadata will produce structurally different SARIF between the two pipelines.
Suggest: drop this guard and instead render "coverage": [] unconditionally when $coverage is empty, keeping uploadResult conditional only on $reportURL — mirroring the LF template.
| // processSecretsRuleProblem extracts data from a secrets rule problem | ||
| func (b *issueBuilder) processSecretsRuleProblem(problem *Problem) { | ||
| if id := problem.GetID(); id != "" { | ||
| if id := problem.GetID(); id != "" && b.problemID == "" { |
There was a problem hiding this comment.
Suggestion: This changes processSecretsRuleProblem's problemID assignment from last-wins (id != "") to first-wins (id != "" && b.problemID == ""), and the new processSnykCodeRuleProblem below uses the same first-wins guard — but the untouched processSnykVulnProblem/processSnykLicenseProblem still overwrite unconditionally (last-wins) for findings with multiple matching problems. This inconsistency isn't explained by the PR description and doesn't look required for SAST/code-rule support.
Suggest: either revert this guard to match the vuln/license last-wins convention, or add a comment explaining why secrets/code-rule problems intentionally differ.
| sarif_utils "github.com/snyk/go-application-framework/pkg/utils/sarif" | ||
| ) | ||
|
|
||
| type TransformOption func(*transformConfig) |
There was a problem hiding this comment.
Suggestion: TransformOption/WithSeverityThreshold add a public option to TransformToUFMFromSarif, but the only caller in this PR (TransformSarifToUFM in pkg/local_workflows/data_transformation_workflow.go) never passes it — no code in this diff calls WithSeverityThreshold. Per this repo's API stability rules, new pkg/ surface is load-bearing once shipped. The equivalent LocalFindings path has no such option; severity filtering there is a separate downstream workflow step.
Suggest: drop this option until a concrete caller needs it (YAGNI), or wire it through TransformSarifToUFM now if one is already planned.
| fnMap["isPendingFinding"] = isPendingFinding | ||
| fnMap["isIgnoredFinding"] = isIgnoredFinding | ||
| fnMap["hasSuppression"] = hasSuppression | ||
| fnMap["getExecutionFlowsFromIssue"] = getExecutionFlowsFromIssue |
There was a problem hiding this comment.
Suggestion: getExecutionFlowsFromIssue is registered here in getCliTemplateFuncMap (used by CLI/human-readable templates), but it's only ever called from ufm.sarif.tmpl:140, which uses getSarifTemplateFuncMap (where it's already registered at line 542). No template using getCliTemplateFuncMap calls it. Looks like a copy-paste leftover.
Suggest: remove this registration.
This comment has been minimized.
This comment has been minimized.
| Arguments: res.Message.Arguments, | ||
| MessageText: res.Message.Text, | ||
| MessageMarkdown: res.Message.Markdown, | ||
| RuleIndex: res.RuleIndex, |
There was a problem hiding this comment.
Critical: RuleIndex: res.RuleIndex copies the index from the input SARIF's full tool.driver.rules array. ufm.sarif.tmpl:144 renders this value verbatim as the output document's result.ruleIndex, but the output's rules array is deduplicateIssues $issues "problemID" — only the rules that actually produced findings, in a different order/size than the input driver's full rule catalog. Once the input driver lists rules beyond the emitted subset (the normal case for real Snyk Code scans), the original index no longer points at the correct entry in the output rules array — an invalid SARIF document per spec, breaking any consumer that resolves rule metadata via ruleIndex instead of ruleId.
Fix: build a problemID → outputIndex map while iterating the deduplicated rules (in the template or in Go) and use that mapped index instead of the original SARIF's index.
| } | ||
|
|
||
| if suppression.Properties.Expiration != nil { | ||
| if t, err := time.Parse(time.RFC3339, *suppression.Properties.Expiration); err == nil { |
There was a problem hiding this comment.
Critical: This only sets ExpiresAt if err == nil from time.Parse(time.RFC3339, ...). Snyk's own relative-duration expirations (e.g. "15 days", present in this PR's own internal/presenters/testdata/with-ignores.json:678 fixture) fail this parse and are silently dropped — no error, no log, no fallback. The SARIF-render path avoids this because the template reads the raw string from FindingExtra.Suppression.Expiration instead, so the structured API (TestResult.Findings()) and the rendered SARIF diverge on identical input. The sibling Local Findings transform keeps Expiration as a plain string with no parsing/loss.
Fix: at minimum, don't silently discard unparseable values — log/surface the parse failure. Ideally interpret Snyk's relative-duration format relative to IgnoredOn to compute a real ExpiresAt.
| return testapi.FindingData{}, fmt.Errorf("failed to map locations: %w", err) | ||
| } | ||
|
|
||
| key := res.Fingerprints.Identity |
There was a problem hiding this comment.
Should Fix: key falls back to res.RuleID alone when there's no fingerprint. selectGrouper in issues.go always uses keyBasedIssueGrouper for SAST findings (this transform always sets FindingType: FindingTypeSast), which groups strictly by Attributes.Key. Two distinct SARIF results for the same rule without fingerprints (common for tools that don't emit stable fingerprints) get the same key and merge into one Issue; getFindingExtraFromIssue then only returns the first finding's message/arguments/fingerprints, silently dropping the second's data from the rendered output.
Fix: derive the fallback key from rule ID plus location (mirroring generateFindingID a few lines below), not RuleID alone.
| {{- if $physicalLoc.Region.StartColumn }} | ||
| "startColumn": {{ $physicalLoc.Region.StartColumn }}{{if $physicalLoc.Region.EndLine}},{{end}} | ||
| {{- end }} | ||
| "startLine": {{ $physicalLoc.Region.StartLine }}{{if $physicalLoc.Region.EndLine}},{{end}} |
There was a problem hiding this comment.
Should Fix: The comma after startLine is gated on $physicalLoc.Region.EndLine being present, and after endLine on StartColumn being present — but if EndLine is absent while StartColumn is set (a common single-line-span SARIF shape), you get "startLine": 5 "startColumn": 3 with no separating comma — invalid JSON. This is a variant of a pre-existing pattern (gate on the next field's presence, not on what was actually emitted), now reachable under a different, more common trigger since the field emission order changed in this PR.
Fix: track whether a field was actually emitted and gate each comma on that, rather than looking ahead to the next field.
|
|
||
| // findingExtraLocal mirrors the JSON shape of ufm.FindingExtra for deserialization | ||
| // when metadata arrives as map[string]interface{} (e.g. after JSON round-trip). | ||
| // Cannot import pkg/utils/ufm directly due to an import cycle. |
There was a problem hiding this comment.
Suggestion: This comment claims importing pkg/utils/ufm directly would create an import cycle, but tracing pkg/utils/ufm's non-test imports (internal/utils/findings, pkg/apiclients/testapi, pkg/configuration, pkg/local_workflows/json_schemas, pkg/utils/sarif, pkg/local_workflows/content_type, pkg/workflow) finds no path back to internal/presenters — only a _test.go file in ufm imports presenters, which doesn't create a build cycle. If that holds, findingExtraLocal/suppressionExtraLocal/priorityScoreFactorLocal are an unnecessary hand-synced duplicate of ufm.FindingExtra/SuppressionExtra/PriorityScoreFactor that will silently drift if a field is added to one side and not the other.
Suggest confirming whether the cycle is real; if not, import the types directly and drop the duplicates. If a cycle is found, please document the exact offending import path in the comment.
| "properties": { | ||
| {{- if $policyMods }} | ||
| "snykPolicy/v1": { | ||
| {{- range $pmod := $policyMods }} |
There was a problem hiding this comment.
Suggestion: This branch (reached when $findingExtra is absent but $policyMods is non-empty — a real, reachable combination for pre-existing Local-Findings-sourced UFM data) ranges over $policyMods emitting one unkeyed "reason" entry per element with no comma/separator. More than one policy modification produces {"reason": "a" "reason": "b"} — invalid JSON with duplicate keys.
Suggest using an index-based range with a comma condition, and confirming the intended SARIF shape when more than one policy modification exists on an issue.
9481aff to
8672a24
Compare
This comment has been minimized.
This comment has been minimized.
# Conflicts: # internal/presenters/presenter_ufm_test.go
This comment has been minimized.
This comment has been minimized.
bbc03ac to
a8de92a
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a8de92a. Configure here.
This comment has been minimized.
This comment has been minimized.
octavian-snyk
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| {{- if $codeRule }} | ||
| "name": {{ getQuotedString $codeRule.Name }}, | ||
| {{- end }} | ||
| "shortDescription": { | ||
| "text": {{ getQuotedString (buildRuleShortDescription $issue) }} | ||
| }, | ||
| {{- if not $codeRule }} | ||
| {{- with buildRuleFullDescription $issue }} | ||
| "fullDescription": { | ||
| "text": {{ getQuotedString . }} | ||
| }, | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
can we use "else" here in line 36?
| {{- if $codeRule }} | ||
| "markdown": {{ getQuotedString (derefStr $codeRule.Help.Markdown) }}, | ||
| "text": {{ getQuotedString (derefStr $codeRule.Help.Text) }} | ||
| {{- else }} | ||
| "text": "", | ||
| "markdown": {{ getQuotedString (buildRuleHelpMarkdown $issue $issue.GetFindingType) }} | ||
| {{- end }} |
There was a problem hiding this comment.
does the order of elements matter? in the first, markdown comes before text, in the second text comes before markdown
There was a problem hiding this comment.
Object properties order shouldn't matter in theory 😅
| {{- if $codeRule }} | ||
| {{- $origTags := $codeRule.Properties.Tags }} | ||
| {{- $origTagsSize := sub (len $origTags) 1 }} | ||
| {{- range $tagIndex, $tag := $origTags }} | ||
| {{ getQuotedString $tag }}{{if lt $tagIndex $origTagsSize}},{{end}} | ||
| {{- end }} | ||
| {{- else }} |
There was a problem hiding this comment.
it seems this can be improved to:
{{- if $codeRule }}
{{- range $i, $tag := $codeRule.Properties.Tags }}
{{- if $i }}, {{ end }}{{ getQuotedString $tag }}
{{- end }}
{{- else }}
| {{- if ge $cvssScore 0.0}}, | ||
| "cvssv3_baseScore": {{ $cvssScore }}, | ||
| "security-severity": {{ getQuotedString (printf "%.1f" $cvssScore) }}{{end}} | ||
| {{- end }} |
There was a problem hiding this comment.
should we have these fields present, even when condition doesn't satisfy? i.e. do we need to add an "else" here?
There was a problem hiding this comment.
I believe it's better to not have them unless the test provides these values.
| "text": {{ getQuotedString $findingExtra.MessageText }}, | ||
| {{- end }} | ||
| {{- if $findingExtra.MessageMarkdown }} | ||
| "markdown": {{ getQuotedString $findingExtra.MessageMarkdown }}, |
There was a problem hiding this comment.
instead of just calling "markdown", should we call it "messageMarkdown"? otherwise is not straightforward to understand what that means
There was a problem hiding this comment.
I believe this is the §3.11.9 property of SARIF, we cannot rename it.
| {{- if $findingExtra.Arguments }} | ||
| {{- $argSize := sub (len $findingExtra.Arguments) 1 }} | ||
| {{- range $argIndex, $arg := $findingExtra.Arguments }} | ||
| {{ getQuotedString $arg }}{{if lt $argIndex $argSize}},{{end}} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
same here, can be improved by a suggestion I wrote in some previous comment
| {{- $argSize := sub (len $findingExtra.Arguments) 1 }} | ||
| {{- range $argIndex, $arg := $findingExtra.Arguments }} | ||
| {{ getQuotedString $arg }}{{if lt $argIndex $argSize}},{{end}} | ||
| {{- end }} |
| func getExecutionFlowsFromIssue(issue testapi.Issue) []testapi.ExecutionFlowEvidence { | ||
| var flows []testapi.ExecutionFlowEvidence | ||
| for _, finding := range issue.GetFindings() { | ||
| if finding.Attributes == nil { | ||
| continue | ||
| } | ||
| for _, ev := range finding.Attributes.Evidence { | ||
| discriminator, err := ev.Discriminator() | ||
| if err != nil || discriminator != "execution_flow" { | ||
| continue | ||
| } | ||
| execFlow, err := ev.AsExecutionFlowEvidence() | ||
| if err == nil { | ||
| flows = append(flows, execFlow) | ||
| } | ||
| } | ||
| } | ||
| return flows | ||
| } |
There was a problem hiding this comment.
I see this function was moved and changed something as well e.g.
for _, evidence := range finding.Attributes.Evidenceto:
for _, ev := range finding.Attributes.Evidencebut the diff is showing as just pure addition.
There was a problem hiding this comment.
maybe you can try to keep the same as before as only variable names have changes. This might reduce the diff a bit?
PR Reviewer Guide 🔍
|
3 participants
Description
This PR introduces a new transformation from Sarif to UFM to eventually replace Sarif to Local Findings.
Checklist
make test)make generate)make lint)go get github.com/snyk/go-application-framework@YOUR_LATEST_GAF_COMMITin thecliv2directory.go.modto point to your local GAF code.go mod tidyin thecliv2directory.go.modandgo.sumchanges.Note
Medium Risk
Touches user-visible SARIF and human-readable output and introduces a new findings pipeline that will replace Local Findings; regressions could change rule indexes, scores, or metadata shape.
Overview
Adds a SARIF → UFM path (
TransformToUFMFromSarif,TransformSarifToUFM, and atestapi.TestResultimplementation) so Code scan SARIF can be modeled as UFM findings with summaries, suppressions, code flows, policy/risk metadata, and per-finding finding-extras stashed for round-trip rendering.UFM SARIF output is expanded to track the Local Findings SARIF template: Snyk Code rule metadata,
codeFlows, richer suppressions/fingerprints/messages, run-level coverage and uploadResult, remapped ruleIndex, priorityScore (withtoInthandlinguint16risk scores), and effective severity. Presenter template funcs were refactored to read that metadata (local mirror types avoid an import cycle withufm).Supporting changes: export shared
FilterSeverityASC, handlesnyk_code_rulein issue building, and add parity/regression tests (SARIF→UFM→SARIF vs SARIF→LF→SARIF, JSON round-trip, focused field tests).Reviewed by Cursor Bugbot for commit 4dc3280. Bugbot is set up for automated code reviews on this repo. Configure here.