Conversation
another-rex
left a comment
another-rex
left a comment
There was a problem hiding this comment.
I like the refactor! I think we might need to adjust it a bit though so we're not dealing with all these manual checks within each strategy.
| Name() string | ||
| // Extract attempts to extract OSV version ranges from a CVE5 Versions entry. | ||
| // Returns the extracted ranges, the detected VersionRangeType, and true if this strategy handled the entry. | ||
| Extract(vers models.Versions, affected models.Affected, metrics *models.ConversionMetrics) (ranges []models.RangeWithMetadata, vrt VersionRangeType, handled bool) |
There was a problem hiding this comment.
I like the strategy system, but the contract seems to be in the wrong place?
I.e. it looks like you are being called per version, but you also have strategies that extract at the "affected" level.
So you end up with the same thing extracted multiple times (e.g. if versions extraction always fails, but CPE extraction succeeds).
…ted-level contract Refactor VersionStrategy from a per-version interface with numeric priorities to a slice-ordered, Affected-level contract using ExtractionState to atomically track consumed version entries and prevent duplicate extractions. Also unify CPE fallback extraction across cpeApplicability and affected[].cpes, memoize per-repo version tag resolution in ProcessRanges, and fix Linux conversion outcome counting in AddAffected.
another-rex
left a comment
another-rex
left a comment
There was a problem hiding this comment.
Nice, looking great! Some minor comments, otherwise LGTM
| } | ||
|
|
||
| func (s *ZeroIntroducedSingleVersionStrategy) Extract(state *ExtractionState, metrics *models.ConversionMetrics) { | ||
| if len(state.Affected.Versions) != 1 || state.IsConsumed(0) { |
There was a problem hiding this comment.
Quick question, this is specifically checking for a single version field right? I.e. if we have a range, and then a single version field after that, we specifically don't want to interpret that as 0..version
There was a problem hiding this comment.
Yes - the check on line 29 is checking there's only one range/version and that its not already consumed. If affected.versions contains a range followed by a single version (or multiple discrete versions like ["3.0", "3.1"]), ZeroIntroducedSingleVersionStrategy is skipped, the range is consumed by StandardRangeStrategy, and the remaining single version falls through to StandaloneSingleVersionStrategy, which treats it as an exact point (introduced: v, last_affected: v).
| @@ -0,0 +1,80 @@ | |||
| package strategies | |||
There was a problem hiding this comment.
Might be worth it to link to an example for each of these (Link meaning the path to the test file for each CNA) (I'm assuming we have at least 1 test file per CNA strategy?)
There was a problem hiding this comment.
It's a bit unclear right now for ones you don't have StandaloneSingleVersionStrategy as a fallback whether that's intentional or not.
|
|
||
| // GetVersionExtractor returns the appropriate VersionExtractor for a given CNA. | ||
| // GetVersionExtractor returns the appropriate VersionExtractor configured with CNA-specific strategies. | ||
| func GetVersionExtractor(cna string) VersionExtractor { |
There was a problem hiding this comment.
Should the wordpress stuff be here? Or as we wiring those up later?
There was a problem hiding this comment.
This will happen later, so we dont bloat this pr
This branch has not been deployed
2 participants
Refactors CVE5 version extraction from nested fallback branches into a modular, ordered strategy pipeline under
conversion/cve5/strategies. This will be so much easier to extend as we get better insights on CNA quirks.VersionStrategy&ExtractionStateContract: IntroducedVersionStrategyoperating onExtractionState, which wraps anAffectedblock with a per-entryconsumedbitmap and accumulated ranges. This allows whole-block (EmitAll), multi-entry (Emit(ranges, i, i+1)), and single-entry (ExtractPerVersion) strategies to compose seamlessly within a single ordered pipeline (ExtractAffectedRanges) and CNA-specific presets (Default(),GitHub(),MITRE(),Linux()).conversion/cve5/strategies/): Isolated modular strategies into dedicated files for inverse affected ranges (defaultStatus == "affected"),changes[]blocks, standard bounds, split/unspecified range pairs, string range expressions, git commit hashes (full & short SHAs), inline CPE strings, free-text version extraction, single-version heuristics, and CVE-level CPE applicability (cpeApplicability&affected[].cpes).VersionRangeTypetallying andFindNormalAffectedRangesfromVersionExtractor, unifying Linux and default extraction throughExtractAffectedRangesand inspectingosvschema.Range_GITdirectly on extracted ranges.IsDirectGitRange, and fall back to extracting fix/introduced commits from CVEreferences(ExtractCommitsFromRefs) when structured ranges cannot be resolved.ProcessRangesto avoid redundant tag lookups across strategies, and exit early todatabase_specific.unresolved_rangeswhen no repositories are available.conversion/cve5/strategiescovering individual strategies,ExtractionStateconsumption semantics, CNA preset ordering, and git/split/inverse ranges.