Repository navigation
Draft: Move core matching logic out of analysers - #8854
neilscallywag wants to merge 4 commits into
Conversation
… Currently, these tests fail which indicates 3 different issues with iteration order of getMatchingSoftware(), stale attribution in matchedVulnerableSoftware and incorrect removal. Test class was scaffoleded with the help of GPT
…ic. This class currently only contains basic scaffolding.
| } | ||
|
|
||
| } | ||
| else if (reason != null) { |
There was a problem hiding this comment.
this can be considered WIP, I want a kind of a "reason" to be populated when the match is inconclusive so that we can analyse especially when there are false positives due to ecosystem or whatnot.
| } | ||
|
|
||
| public static MatchResult match(Cpe dependencyCpe, Vulnerability candidateCve, | ||
| String dependencyEcosystem) { |
There was a problem hiding this comment.
This dependency ecosystem is from the package manager. We can consider changing this to Ecosystem Class depending on where we want to call this method from.
Dependency.getEcosystem() currently returns a string instead of the Ecosystem Object. That means if we enforce this fn to take in Ecosystem as param, we need to enforce Ecosystem to be used everywhere else OR cast the string to Ecosystem Object before passing it in here.
Or we can take in string and cast it internally within this method. Ideally, we want to take option A but that could introduce other unintended effects.
| * @return <code>true</code> if there is a match; otherwise | ||
| * <code>false</code> | ||
| */ | ||
| private boolean ecosystemMatchesTargetSoftware(String ecosystem, String targetSoftware) { |
There was a problem hiding this comment.
This is moved to Ecosystem
There was a problem hiding this comment.
It could also be its own separate PR.
| switch (normalized) { | ||
| case "nodejs": | ||
| case "npm": | ||
| return NODEJS; |
There was a problem hiding this comment.
I dont have sufficient understanding of js ecosystem to know if js and javascript can be added here or in javascript
| Outcome.NO_MATCH, null, null, matches, candidates); | ||
| } | ||
|
|
||
| matches.sort(Comparator |
There was a problem hiding this comment.
Regardless of how we recieve the matches, we want a deterministic order to it.
There was a problem hiding this comment.
This makes the microsoftAuthenticationLibrarySelectionShouldNotDependOnSetIterationOrder() test pass
For context, CVEDB.getMatchingSoftware() returns items based on the order in which they appear in the list which should not happen i.e, functionally same list should return the same item [A,B,C] should be considered the same as [B,C,A] and should return the same item.
This branch has not been deployed
1 participant
Description of Change
As per discussions in #8852 and #8834, We want to move some of the behaviours out of the matching behaviours out of analysers and cveDB and allocate it to a deterministic class. This is a working PR so maintainers can add inputs.
We want to first establish baseline behaviour that currently exists and then incremently introduce changes. Then we will compare the baseline behaviour to the new behaviour and determine if that is indeed what we want. Then finally, we will switch to the new changes once we are satisfied.
Ill probably wait on some inputs from the maintainers before I work on this further so that we can kind of align on the overall architecture, things we wanna consolidate and scope.
Related issues
List<Vulnerability>when filtering by ecosystem #8852 fix: skip NVD entries whose target_sw names another language runtime #8834Have test cases been added to cover the new functionality?
yes