refactor(lang): use MatchVersion instead of IsVulnerable for Comparer interface #9735
refactor(lang): use MatchVersion instead of IsVulnerable for Comparer interface #9735DmitriyLewen wants to merge 6 commits intoaquasecurity:mainfrom
MatchVersion instead of IsVulnerable for Comparer interface #9735Conversation
- use MatchVersion instead of IsVulnerable function - move GenericComparer to separate package - move common IsVulnerable function into driver.go
github-actions
bot
added
the
apidiff
📊 API Changes DetectedSemver impact: |
|
|
||
| // IsVulnerable checks if the package version is vulnerable to the advisory. | ||
| func (n Comparer) IsVulnerable(ver string, advisory dbTypes.Advisory) bool { | ||
| return compare.IsVulnerable(ver, advisory, n.MatchVersion) |
There was a problem hiding this comment.
Here’s a more detailed description of the issue in #9427.
It’s related to an npm package.
Suppose we have a package with version 4.17.20+root.io.1 and an advisory:
• PatchedVersions: "4.17.20+root.io.5", "4.17.21"
• VulnerableVersions: "=4.17.20", ">4.17.20, <4.17.21"
When we check VulnerableVersions, the package is considered vulnerable.
But when we check PatchedVersions, 4.17.20+root.io.1 == 4.17.20+root.io.5 (because build metadata isn’t compared), so we don’t show this vulnerability.
Test for this case:
https://github.com/chait-slim/trivy/blob/790eaa7a430f9b95370d9a13695174b13c09b6d3/pkg/detector/library/rootio/rootio_test.go#L123-L154
There was a problem hiding this comment.
I haven’t fully followed the vulnerability detection for Root.io’s language-specific libraries yet, but it feels weird that other ecosystems would need to change just because Root.io requires special handling for version comparisons. Wouldn’t it be possible to implement the special processing only for the root ecosystem?
There was a problem hiding this comment.
Let me explain in more detail:
Root.io will have multiple ecosystems, and we already have comparators for those ecosystems.
The issue is that these comparators have the IsVulnerable logic hard-coded.
This means that when comparing Root.io packages, we always check both VulnerableVersions and PatchedVersions.
However, if we want to check only VulnerableVersions (as an example), we’d have to write a new comparator/function for custom comparators.
e.g.
func (n Comparer) IsVulnerableForRootIO(ver string, advisory dbTypes.Advisory) bool {
return root.io.IsVulnerable(ver, advisory, n.MatchVersion)
}I think it’s more reasonable if comparators only compare versions/ranges,
and the vulnerability evaluation logic is separated: a shared (current) default implementation and a dedicated Root.io-specific one.
There was a problem hiding this comment.
When I was answering your question, I realized that this doesn’t fully solve the issue with npm packages.
So I asked a follow-up question to the Root.io team - aquasecurity/trivy-db#570 (comment)
There was a problem hiding this comment.
Because it doesn’t follow the ecosystem’s versioning, Root.io’s PatchedVersions can be considered inaccurate. Wouldn’t it work to perform some preprocessing? For example, we could remove only PatchedVersions before passing the data to IsVulnerable() and then add them back later.
However, I may be suggesting something off the mark since I don’t fully understand the details yet.
There was a problem hiding this comment.
Yeah, you’re right. It looks like that could help.
I’ll think about it, thanks!
There was a problem hiding this comment.
However, I am not opposed to this proposal. Regardless of Root.io’s handling, if greater flexibility is needed, I think it’s best to make the changes as your PR suggests.
In this particular case, it just seemed that Root.io was not following the ecosystem’s versioning, so I thought it would be more natural to give them special handling before passing them to each ecosystem’s Comparer.
There was a problem hiding this comment.
When I was exploring the Root.io ecosystems, I kept “jumping” between different ecosystems and tried to think globally — that’s how the global solution appeared 😄
I wanted to add more flexibility and configuration options.
But as you correctly pointed out, Root.io didn’t consider that npm doesn’t compare build metadata, so our current approach won’t work.
Let’s leave the working logic as is for now, and if we ever need it, we can revisit this PR later.
|
I’ll close this PR for now, since it looks like there’s a simpler solution. |
2 participants
Description
Refactored the
Comparerinterface in the library detection system to use a cleanerMatchVersionfunction instead ofIsVulnerable. This change simplifies version comparison logic by separating constraint matching from vulnerabilityassessment.
Changes
Comparerinterface fromIsVulnerable(currentVersion, advisory)toMatchVersion(currentVersion, constraint)GenericComparerfrompkg/detector/library/compare/topkg/detector/library/compare/generic/IsVulnerablefunction intodriver.go, centralizing vulnerability assessment logicRelated PRs
IsVulnerablethat checks onlyVulnerableVersionsand doesn’t checkPatchedVersions.However, the current compares are tied to the main IsVulnerable and don’t allow reusing them.
Checklist