[PPSC-717] fix(sarif): stabilize ruleId to prevent recurring false positive alerts#147
Open
yiftach-armis wants to merge 4 commits intomainfrom
Open
[PPSC-717] fix(sarif): stabilize ruleId to prevent recurring false positive alerts#147yiftach-armis wants to merge 4 commits intomainfrom
yiftach-armis wants to merge 4 commits intomainfrom
Conversation
…sitive alerts Derive ruleId from CWE/CVE/category instead of the server-assigned finding.ID which embeds line numbers and causes duplicate alerts when lines shift. Add partialFingerprints for cross-run dedup.
|
| Severity | Count |
|---|---|
| 🟡 MEDIUM | 1 |
Total: 1
View all 1 findings
🟡 MEDIUM (1)
CWE-253_armis-cli_38295677_internal/output/sarif.go_237_3_237_105 - In the `computeFingerprint` function the code builds a string that is fed into a SHA‑256 hash
Location: internal/output/sarif.go:237
In the computeFingerprint function the code builds a string that is fed into a SHA‑256 hash. It does this by calling fmt.Fprintf and then completely ignores the error value that fmt.Fprintf returns. If, for some reason, the write to the hash failed, the function would keep going and produce a fingerprint that might be incomplete or wrong, and the program would not notice the problem. This pattern matches the description of “incorrect check of function return value” (CWE‑253). The function is part of a command‑line tool that creates SARIF output, so it runs locally when the tool is invoked. The data used to build the fingerprint comes from the scan results, not from a direct network request, so an attacker cannot directly trigger the problem through a public interface. However, because the tool could be called by other scripts or programs, the issue is considered to have a modest exposure level. To fix the issue, the code should capture the error returned by fmt.Fprintf and handle it (for example, by returning the error up the call chain or logging it). This ensures that any unexpected write failure is not silently ignored.
CWEs: CWE-253: Incorrect Check of Function Return Value
Test Coverage Reporttotal: (statements) 79.3% Coverage by function |
There was a problem hiding this comment.
Pull request overview
Stabilizes SARIF ruleId generation and adds partial fingerprints to reduce recurring duplicate GitHub Code Scanning alerts when line numbers/CWE classifications shift between runs.
Changes:
- Introduces
stableRuleID()and updates SARIF rule/result generation to use it (CWE → CVE → category → finding.ID fallback). - Adds
partialFingerprints.primaryLocationLineHashcomputed viacomputeFingerprint()to improve cross-run deduplication. - Preserves the original server-assigned
finding.IDunderresult.properties.findingIdand adds unit/end-to-end tests for the new behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
internal/output/sarif.go |
Switches SARIF ruleId to a stable classification-based ID, adds partial fingerprints, and preserves server finding ID in properties. |
internal/output/sarif_test.go |
Adds unit tests for stable rule IDs and fingerprints, plus regression/e2e coverage for SARIF output fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…antees - stableRuleID: filter empty/whitespace values, sort for deterministic selection regardless of API ordering - computeFingerprint: use length-prefixed fields to prevent separator collision between different tuples - generateHelpURI: align priority with stableRuleID (CWE first, then CVE) - Compute fingerprint from sanitized path to match emitted artifact URI
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
internal/output/sarif.go:272
- buildRules() now deduplicates by stableRuleID, but the rule metadata (ShortDescription, DefaultConfiguration.Level, security-severity, descriptions, helpUri) is taken from whichever finding for that ruleID appears first in the findings slice. If multiple findings share the same CWE/CVE but differ in title/severity/description, the rule metadata becomes arbitrary and can vary across runs depending on finding ordering. Consider deriving rule metadata deterministically from the ruleID (or aggregating consistently, e.g., choose max severity / lexicographically smallest title).
for _, finding := range findings {
ruleID := stableRuleID(finding)
if _, exists := ruleIndexMap[ruleID]; !exists {
ruleIndexMap[ruleID] = len(rules)
rule := sarifRule{
ID: ruleID,
ShortDescription: sarifMessage{
Text: finding.Title,
},
DefaultConfiguration: sarifRuleConfig{
Level: severityToSarifLevel(finding.Severity),
},
Properties: &sarifRuleProperties{
SecuritySeverity: severityToSecurityScore(finding.Severity),
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Trim whitespace on FindingCategory before using as ruleId - Use single resolvedFile for both fingerprint and artifact URI, including placeholder on sanitization failure - Capture and log sanitization error for debuggability
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
Type of Change
Problem
The SARIF formatter uses the server-assigned
finding.IDasruleId, which bakes CWE, file path, and exact line/column numbers into the rule identity (e.g.,CWE-253_armis-cli_38295677_scripts/install.ps1_95_17_95_46). When lines shift or CWE classifications evolve between scans, GitHub Code Scanning creates new alerts and ignores prior dismissals. Historical analysis of 893 alerts showed 72% (650) were duplicates caused by this unstable ruleId.Solution
Three changes in
internal/output/sarif.go:ruleId: NewstableRuleID()derives rule identity from CWE → CVE → FindingCategory → finding.ID fallback chain, per the SARIF 2.1.0 spec (ruleId identifies the type of check, not the instance).partialFingerprints: NewcomputeFingerprint()addsprimaryLocationLineHash(SHA-256 of ruleId + file + snippet/startLine) for GitHub's cross-run deduplication.finding.IDmoved toproperties.findingIdso no debugging context is lost.Testing
Automated Tests
4 new tests added:
TestStableRuleID— 6-case table covering the full CWE→CVE→category→ID fallback chainTestComputeFingerprint— determinism, snippet-vs-startLine behavior, collision resistance, hash formatTestBuildRulesStableCollapsing— two findings with same CWE collapse into one ruleTestSARIFFormatter_FindingIDAndFingerprints— end-to-end: ruleId=CWE, fingerprints present, findingId preservedAll existing SARIF tests pass including SARIF 2.1.0 schema validation.
Manual Testing
After merge, the next daily security scan should produce SARIF with stable ruleIds. Previously dismissed alerts should stop reappearing.
Reviewer Notes
partialFingerprints.primaryLocationLineHashkey is what GitHub Code Scanning uses for cross-run dedup — this is their documented mechanism.Checklist