PR Decorator Fix - Same CVEs should be aggregated in the same record#1067
PR Decorator Fix - Same CVEs should be aggregated in the same record#1067orto17 wants to merge 6 commits intojfrog:v3_erfrom
Conversation
orto17
added
the
safe to test
github-actions
bot
removed
the
safe to test
orto17
changed the title
eranturgeman
left a comment
eranturgeman
left a comment
There was a problem hiding this comment.
approved! see my comments
github-actions
bot
removed
the
safe to test
github-actions
bot
removed
the
safe to test
github-actions
bot
removed
the
safe to test
github-actions
bot
removed
the
safe to test
github-actions
bot
removed
the
safe to test
| if pathErr != nil { | ||
| return fmt.Errorf("failed to rollback descriptor: %w (original error: %v)", pathErr, err) | ||
| } | ||
| // #nosec G703 -- path validated under working directory by getAbsolutePathUnderWd |
There was a problem hiding this comment.
#nosec G703 annotation is incorrect.
gosec G703 is "Deferring a method which returns an error" — that's unrelated.
The gosec rule being triggered here is G304 ("File path provided as taint input").
| return regexp.MustCompile(regexpCompleteFormat) | ||
| } | ||
|
|
||
| func getAbsolutePathUnderWd(path string) (string, error) { |
There was a problem hiding this comment.
please add unit test
| if err != nil { | ||
| return "", fmt.Errorf("failed to get working directory: %w", err) | ||
| } | ||
| wdAbs, err := filepath.Abs(wd) |
There was a problem hiding this comment.
I think Getwd already returns absolute path so calling filepath.Abs is redundant
| return "", fmt.Errorf("failed to resolve working directory: %w", err) | ||
| } | ||
| rel, err := filepath.Rel(wdAbs, absPath) | ||
| if err != nil || strings.HasPrefix(rel, "..") || rel == ".." { |
There was a problem hiding this comment.
strings.HasPrefix(rel, "..") already covers the case of "rel == ".."" so you dont need the ' rel == ".." '
| agg.ImpactPaths = append([][]formats.ComponentRow(nil), v.ImpactPaths...) | ||
| agg.FixedVersions = append([]string(nil), v.FixedVersions...) | ||
| heapCopy := new(formats.VulnerabilityOrViolationRow) | ||
| *heapCopy = agg |
There was a problem hiding this comment.
why do you need this second copy? this is similar to the copy made here 'agg := *v'
| order[key] = i | ||
| } | ||
| } | ||
| sort.Slice(result, func(i, j int) bool { |
There was a problem hiding this comment.
The order-preservation pass re-iterates vulnerabilities to build the
order map after the aggregation map is already built. This can be
folded into the first loop (track first-seen index inside the main loop)
to avoid the second O(n) scan — minor but cleaner.
| return out | ||
| } | ||
|
|
||
| func aggregateVulnerabilitiesOrViolationsByCve(vulnerabilities []formats.VulnerabilityOrViolationRow) []formats.VulnerabilityOrViolationRow { |
There was a problem hiding this comment.
The violations aggregation is drops important data when two rows for
the same CVE have different Watch names (the "Watch Name" column in the
violations table is kept from the first occurrence, silently discarding
the others). If two different Watch policies trigger on the same CVE,
merging them may cause confusion.
There was a problem hiding this comment.
Another issue with the violation aggregation is when two violations rows for the same CVE have different impacted dependency versions (e.g., lib:1.5.15 and lib:1.5.1), the merged row will only show lib:1.5.15 in the "Impacted Dependency" column. The second version is silently dropped — it won't appear in the table at all (unlike vulnerabilities where ImpactPaths carries that info implicitly).
The Components field (used for "Direct Dependencies" via getDirectDependenciesCellData) has the same problem — it's also only kept from the first occurrence in the aggregation merge step.
eranturgeman
left a comment
eranturgeman
left a comment
There was a problem hiding this comment.
Please add description to PR and fix failing test in Scan Pull Request suit as they are all suppose to be green now
2 participants
The fix: