feat: add severity to UFM SARIF rule and result properties - #595
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.
54a8d3e to
5be40ce
Compare
This comment has been minimized.
This comment has been minimized.
5be40ce to
49ca020
Compare
PR Reviewer Guide 🔍
|
| // extractEffectiveSeverity extracts effective severity from policy modifications. | ||
| // It also extracts the original (pre-policy) severity from the "prior" field of | ||
| // the policy modification, which is used to render the snykPolicy/v1 block in | ||
| // SARIF output (see ufm.sarif.tmpl). | ||
| // | ||
| // TODO(CLI-1330): The originalSeverity extraction from mod.Prior is currently | ||
| // untested — no existing test data includes a policy_modifications entry with | ||
| // a "prior" value. Add a test case with a severity override that includes | ||
| // "prior": "<original severity>" to cover the snykPolicy/v1 SARIF rendering. | ||
| // Also verify which pointer the API actually uses ("/rating/severity" per the | ||
| // spec example vs "/attributes/rating/severity" seen in some test data). | ||
| func (b *issueBuilder) extractEffectiveSeverity(finding *FindingData) { |
There was a problem hiding this comment.
TODO here.
Currently we have no tests that contains non-empty policy_modifications.
| continue | ||
| } | ||
| if b.effectiveSeverity == "" && finding.Attributes.Rating.Severity != "" { | ||
| b.effectiveSeverity = string(finding.Attributes.Rating.Severity) |
There was a problem hiding this comment.
AI review — Should Fix (behavior change): This drops the mod.Prior != nil precondition that previously gated assigning effectiveSeverity. On origin/main the assignment only happened when a prior value existed; now effectiveSeverity is set for any /rating/severity (or /attributes/rating/severity) modification regardless of whether Prior is present.
For a finding where the builder-derived b.severity differs from finding.Attributes.Rating.Severity and the policy mod has a nil Prior, GetEffectiveSeverity() now returns the rating value instead of falling back to b.severity — and that flows into the new result-level properties.severity in the SARIF output. No current fixture exercises this divergent case (the only severity-mod fixture has both values equal), so the change is invisible to tests. Please confirm this is intentional and pin it with a test for both prior-present and prior-absent mods.
— AI review
| if mod.Pointer == "/rating/severity" && mod.Prior != nil { | ||
| if b.effectiveSeverity == "" && finding.Attributes.Rating.Severity != "" { | ||
| b.effectiveSeverity = string(finding.Attributes.Rating.Severity) | ||
| isSeverityMod := mod.Pointer == "/rating/severity" || mod.Pointer == "/attributes/rating/severity" |
There was a problem hiding this comment.
AI review — Should Fix (untested path + unconfirmed contract): The dual-pointer match (/rating/severity || /attributes/rating/severity) plus the downstream originalSeverity extraction and the snykPolicy/v1 SARIF block (template lines ~204-210) ship with zero test coverage — your own TODO(CLI-1330) confirms no fixture contains a policy_modifications entry with a prior value, and snykPolicy appears in none of the regenerated testdata.
Two concrete risks on this never-exercised path: (1) the API pointer spelling is a documented guess (hence matching both), so a wrong/absent pointer silently no-ops the feature; (2) a non-string Prior (object/number) fails the (*mod.Prior).(string) assertion and silently omits the whole block. Recommend resolving the real pointer against the API contract (keep only the confirmed branch) and adding a golden fixture with a real severity override so this branch is exercised before merge — otherwise the headline feature lands dark.
— AI review
| {{- end }} | ||
| {{- end }}, | ||
| "properties": { | ||
| "severity": {{ getQuotedString $issue.GetEffectiveSeverity }} |
There was a problem hiding this comment.
AI review — Should Fix (inconsistent severity sources): This result-level properties.severity uses GetEffectiveSeverity (post-policy), but the SARIF result "level" (line ~78) and the rule-level properties.severity (line ~47) use GetSeverity (raw/pre-policy). When a policy overrides a finding's severity, the same result advertises a level derived from the original severity while properties.severity (and the snykPolicy/v1 block right below) reflect the effective one — contradictory signals in one object.
The raw-at-rule / effective-at-result split may be deliberate (rules are deduped across findings), but it is currently implicit and untested. Please add a brief comment documenting the intent, and a fixture with an actual override that pins the level vs properties.severity relationship.
— AI review
| {{- $originalSeverity := $issue.GetOriginalSeverity }} | ||
| {{- if $originalSeverity }}, | ||
| "snykPolicy/v1": { | ||
| "originalLevel": {{ getQuotedString (severityToSarifLevel $originalSeverity) }}, |
There was a problem hiding this comment.
AI review — Suggestion: severityToSarifLevel is case-sensitive and only maps low/medium/high/critical, returning "unmapped" otherwise. Here it runs on the raw originalSeverity string taken straight from mod.Prior, so a genuine override whose prior value is none, other, or non-lowercase (e.g. "High") renders "originalLevel": "unmapped" — an invalid SARIF level from otherwise-correct API data. (The result-level level at line ~78 is safe because it consumes the lowercased Severity enum; this new path is unprotected.) Worth normalizing casing / handling the full enum once the override path gets a fixture.
— AI review
| {{- if $originalSeverity }}, | ||
| "snykPolicy/v1": { | ||
| "originalLevel": {{ getQuotedString (severityToSarifLevel $originalSeverity) }}, | ||
| "severity": {{ getQuotedString $issue.GetEffectiveSeverity }}, |
There was a problem hiding this comment.
AI review — Suggestion: snykPolicy/v1.severity here emits the identical GetEffectiveSeverity value already emitted at the enclosing properties.severity (line ~202). It's redundant and a sync hazard — any future change to effective-severity semantics must be edited in both places or they silently diverge. The snykPolicy/v1 block's purpose is to describe the override (originalLevel/originalSeverity); consider dropping the duplicated effective severity from it.
— AI review
Verdict: CONTESTED — the core Should Fix (posted inline):
Suggestions (posted inline): Suggestion (out-of-diff): the regenerated Security: clean — no secrets introduced; all dynamic template values are — AI review |
| b.effectiveSeverity = string(finding.Attributes.Rating.Severity) | ||
| } | ||
| if b.originalSeverity == "" && mod.Prior != nil { | ||
| if prior, ok := (*mod.Prior).(string); ok { |
There was a problem hiding this comment.
Should Fix — the snykPolicy/v1 / originalSeverity path ships untested and on an unconfirmed contract.
This new extraction (and the SARIF block it feeds at ufm.sarif.tmpl ~204-210) is the headline of this PR, yet no fixture exercises it: no policy_modifications entry in any *.testresult.json carries a prior, snykPolicy appears in none of the regenerated SARIF outputs, and no test references GetOriginalSeverity. Your own TODO(CLI-1330) above (line 589) calls this out. Two concrete correctness risks live in this untested code:
- Unconfirmed pointer (line 600).
isSeverityModmatches both/rating/severityand/attributes/rating/severityas a guess. If the API emits a third form,originalSeveritystays empty, the template{{if $originalSeverity}}is false, and the feature is a silent no-op — with no failing test to reveal it. - Non-string
Priorsilently drops the block.(*mod.Prior).(string)is a comma-ok assertion; if the API ever serializes the prior severity as a non-string (object/number), the assertion fails,originalSeveritystays empty, and the entiresnykPolicy/v1block disappears for a finding that was policy-modified — no error, no log.
Suggest adding a testresult fixture with a real severity override carrying "prior": "<severity>", regenerating the SARIF so it contains a rendered snykPolicy/v1 block, and asserting on it — before this path goes live. Confirm the real API pointer and collapse the dual-match to the verified one.
— AI review
| {{- $originalSeverity := $issue.GetOriginalSeverity }} | ||
| {{- if $originalSeverity }}, | ||
| "snykPolicy/v1": { | ||
| "originalLevel": {{ getQuotedString (severityToSarifLevel $originalSeverity) }}, |
There was a problem hiding this comment.
Should Fix — originalLevel can render "unmapped" from otherwise-valid API data.
severityToSarifLevel (pkg/utils/sarif/sarif.go:34) is case-sensitive and maps only low/medium/high/critical, returning "unmapped" otherwise. Here it runs on the raw, unnormalized originalSeverity taken straight from mod.Prior. The testapi.Severity enum also defines none and other, so a policy override from/to one of those — or any non-lowercase value ("High") — emits "originalLevel": "unmapped", an invalid SARIF level. The result-level level field is safe because it consumes the lowercased Severity enum; this new path is unprotected. Normalize casing / handle the full enum when the override path gets its fixture.
— AI review
| {{- end }} | ||
| {{- end }}, | ||
| "properties": { | ||
| "severity": {{ getQuotedString $issue.GetEffectiveSeverity }} |
There was a problem hiding this comment.
ℹ️ Non-blocking suggestions on the new
propertiesblock.
- Rule-vs-result severity divergence (undocumented). Rule-level
properties.severity(line 47) usesGetSeverity(raw/pre-policy) while this result-levelproperties.severityusesGetEffectiveSeverity(post-policy), and the resultlevel(line 78) also uses raw severity. For a policy-overridden finding the same SARIF document then advertises a rawlevelalongside an effectiveproperties.severity— contradictory signals unless intended. If deliberate, a one-line comment at each call site stating which severity each field carries would prevent a future "cleanup" from regressing it. - Duplicate
severityin the policy block (line 207).snykPolicy/v1.severityis byte-identical to this outerproperties.severity(bothGetEffectiveSeverity). The block's purpose is the override delta (originalLevel/originalSeverity); the duplicated effective severity carries no extra info and is a sync hazard. Consider dropping it. - Empty severity renders
"severity": "".GetSeverity/GetEffectiveSeveritymay return empty (per the interface doc); an unrated finding then emits a literal empty string rather than omitting the key, while the siblinglevelmaps empty to"unmapped". Decide deliberately (omit via{{- with }}or normalize) and add an unrated-finding fixture.
— AI review
2 participants
Description
Adds severity information to the UFM SARIF template so that consumers have a single, consistent place to read the effective severity — including
critical, which has no native SARIF level (the SARIF 2.1.0 spec only defineserror,warning,note,none).Changes:
ufm.sarif.tmpl: adds"severity"to each rule'spropertiesand a new"properties"block on each result containing"severity"(effective, post-policy) and conditionally a"snykPolicy/v1"block when a policy severity override exists.issues.go: addsGetOriginalSeverity()to theIssueinterface, extracting the pre-policy severity from thepriorfield inpolicy_modifications. Also accepts both/rating/severityand/attributes/rating/severitypointers.mocks/issues.go: addsGetOriginalSeveritymock to stay in sync with the interface.Note: the
snykPolicy/v1rendering path (when a severity override includes apriorvalue) is plumbed but not yet covered by test data — there is a TODO in the code. No existing API responses observed includepriorinpolicy_modificationsyet.CLI-1330
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.