feat: include all scores in PurlStatus #2166
Conversation
Reviewer's GuideExtend PurlStatus to expose the full list of CVSS scores for each vulnerability, adjust the service layer to propagate raw CVSS models instead of only an aggregated score, and update tests and OpenAPI schema to reflect the new scores field. Sequence diagram for propagation of all CVSS scores into PurlStatussequenceDiagram
participant Client
participant VulnerabilityService
participant Cvss3Store
participant PurlStatus
Client->>VulnerabilityService: analyze_vulnerability()
VulnerabilityService->>Cvss3Store: load_cvss3_scores()
Cvss3Store-->>VulnerabilityService: Vec cvss3::Model
VulnerabilityService->>VulnerabilityService: build cvss3_map(HashMap<(Uuid, String), Vec cvss3::Model>)
loop for each advisory and purl status
VulnerabilityService->>VulnerabilityService: lookup scores = cvss3_map[(advisory_id, vulnerability_id)]
VulnerabilityService->>PurlStatus: from_head(vuln_head, status, version_range, cpe, &scores)
activate PurlStatus
PurlStatus->>PurlStatus: average_score = Cvss3Score::from_iter(scores.iter().map(Cvss3Base::from))
PurlStatus->>PurlStatus: all_scores = scores.iter().cloned().filter_map(Score::try_from).collect()
PurlStatus-->>VulnerabilityService: PurlStatus{average_severity, average_score, scores: all_scores, ...}
deactivate PurlStatus
end
VulnerabilityService-->>Client: AnalysisDetails including Vec PurlStatus with scores[]
Class diagram for updated PurlStatus score handlingclassDiagram
class PurlStatus {
+VulnerabilityHead vulnerability
+Severity average_severity
+f64 average_score
+Vec~Score~ scores
+String status
+Option~StatusContext~ context
+Option~VersionRange~ version_range
+from_vulnerability(vuln, version_range, status, cpe, tx) Result~PurlStatus, Error~
+from_head(vuln_head, status, version_range, cpe, scores) Result~PurlStatus, Error~
}
class Score {
+f64 value
+Severity severity
+try_from(cvss3_model) Result~Score, Error~
}
class Cvss3Score {
+f64 value()
+Severity severity()
+from_iter(iterator) Cvss3Score
}
class Cvss3Base {
+from(cvss3_model) Cvss3Base
}
class Cvss3Model {
}
class VulnerabilityService {
+build_analysis_details(...)
}
PurlStatus "1" --> "*" Score : has
PurlStatus "1" --> "1" Cvss3Score : derives_average
Cvss3Score "1" --> "*" Cvss3Base : aggregates
Cvss3Base "1" --> "1" Cvss3Model : wraps
VulnerabilityService --> PurlStatus : constructs
VulnerabilityService --> Cvss3Model : uses
Score "1" --> "1" Cvss3Model : try_from
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
VulnerabilityService, when populatingcvss3_map, you can avoid the extra clone by insertingscoredirectly into theHashMapifcvss3_scoresisn’t reused later (i.e., replacelet score_clone = score.clone(); ... .push(score_clone);with.push(score);). - The construction of
all_scoresinPurlStatus::from_vulnerability/from_headclones the CVSS3 models and thentry_froms them; consider adding aTryFrom<&cvss3::Model>implementation forScoreso you can map over references and avoid cloning.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `VulnerabilityService`, when populating `cvss3_map`, you can avoid the extra clone by inserting `score` directly into the `HashMap` if `cvss3_scores` isn’t reused later (i.e., replace `let score_clone = score.clone(); ... .push(score_clone);` with `.push(score);`).
- The construction of `all_scores` in `PurlStatus::from_vulnerability`/`from_head` clones the CVSS3 models and then `try_from`s them; consider adding a `TryFrom<&cvss3::Model>` implementation for `Score` so you can map over references and avoid cloning.
## Individual Comments
### Comment 1
<location> `modules/fundamental/src/purl/model/details/purl.rs:340-344` </location>
<code_context>
let cvss3 = vuln.find_related(cvss3::Entity).all(tx).await?;
- let average_score = Score::from_iter(cvss3.iter().map(Cvss3Base::from));
+ let average_score = Cvss3Score::from_iter(cvss3.iter().map(Cvss3Base::from));
+ let all_scores = cvss3
+ .iter()
+ .cloned()
+ .filter_map(|cvss3| Score::try_from(cvss3).ok())
+ .collect();
+
Ok(Self {
</code_context>
<issue_to_address>
**issue:** Consider whether silently dropping scores that fail `Score::try_from` is acceptable.
This `filter_map` will drop any CVSS3 values that fail `Score::try_from`, so `scores` may exclude items that contributed to `average_score`. If conversion failures are expected in real data, consider logging them or propagating an error instead of skipping them silently.
</issue_to_address>
### Comment 2
<location> `modules/fundamental/tests/advisory/csaf/reingest.rs:164-175` </location>
<code_context>
},
average_severity: Severity::Medium,
average_score: 5.3f64,
+ scores: vec![
+ Score {
+ severity: Severity::Medium,
+ value: 5.3,
</code_context>
<issue_to_address>
**suggestion (testing):** Use distinct scores in the expectation to prove that all individual scores (not just count) are preserved
The new expectations only verify that `scores` contains multiple entries, not that distinct CVSS scores are preserved. Please make the entries differ (e.g., one V3.1 and one V2/V3.0, or with different `value`s) so the test would fail if only a single or averaged score were returned.
```suggestion
scores: vec![
Score {
severity: Severity::Medium,
value: 5.3,
r#type: ScoreType::V3_1,
},
Score {
severity: Severity::Medium,
value: 7.5,
r#type: ScoreType::V3_0,
}
],
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| let all_scores = cvss3 | ||
| .iter() | ||
| .cloned() | ||
| .filter_map(|cvss3| Score::try_from(cvss3).ok()) | ||
| .collect(); |
There was a problem hiding this comment.
issue: Consider whether silently dropping scores that fail Score::try_from is acceptable.
This filter_map will drop any CVSS3 values that fail Score::try_from, so scores may exclude items that contributed to average_score. If conversion failures are expected in real data, consider logging them or propagating an error instead of skipping them silently.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2166 +/- ##
==========================================
+ Coverage 68.24% 68.27% +0.02%
==========================================
Files 376 376
Lines 21208 21221 +13
Branches 21208 21221 +13
==========================================
+ Hits 14473 14488 +15
+ Misses 5868 5862 -6
- Partials 867 871 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| pub vulnerability: VulnerabilityHead, | ||
| pub average_severity: Severity, | ||
| pub average_score: f64, | ||
| pub scores: Vec<Score>, |
There was a problem hiding this comment.
I know we're doing a bad job with this so far. But it would be good to improve in the future and document fields, so that it would also end up in the OpenAPI spec
There was a problem hiding this comment.
Updated the description. Made me notice that it feels a bit odd for the scores/severity to be in the PurlStatus type given that theres a 1:N from vuln->score, but no direct relationship between purl_status->score from what I saw in the seaorm schema
Strum355
force-pushed
the
nsc/v3-analyze-scores
branch
from
d32a3d9 to
ff91aeb
Compare
Strum355
force-pushed
the
nsc/v3-analyze-scores
branch
from
ff91aeb to
d5b0d65
Compare
carlosthe19916
left a comment
carlosthe19916
left a comment
There was a problem hiding this comment.
@Strum355 my apologies for the delayed review.
I tested this PR and it works fine. Just a minor thing to change: The current response of /api/v3/vulnerability/analyze has the following shape:
{
"purl1": {
details: [
...vulnerability fields here, // this not needed
purl_statuses: [{
vulnerability: {...vulnerability fields here},
advisory: {},
scores: []
}]
]
}
}
notice that the field purl.details contains a set of fields that belong to a Vulnerability, while at the same time the vulnerability fields are already present inside the purl_statuses field.
Could we clean up the duplicate vulnerability fields?
Thanks for the feedback! I had noticed the duplication as well, but Im not sure on which side we want to remove it from. While I am leaning towards removing it from trustify/modules/fundamental/src/vulnerability/model/analyze.rs Lines 18 to 20 in 53e6492 vulnerability
|
@Strum355 this wouldn't be a problem for the client side (UI). I'll let you take the final decision on regards of which side the fields should be removed from :) |
…our of within PurlStatus
Strum355
force-pushed
the
nsc/v3-analyze-scores
branch
from
d5b0d65 to
24547af
Compare
@ctron @dejanb I opted to remove it from the top level AnalysisDetails, lmk if you feel strongly otherwise |
3 participants
UI needs the full list of scores to adopt /api/v3/vulnerability/analyze endpoint, which was included in the v2 version in the AnalysisAdvisory struct
Summary by Sourcery
Expose full CVSS score details for PurlStatus and propagate them through the vulnerability analysis pipeline and API.
New Features:
Enhancements: