fix: vuln correlation fixes - #2582
Open
rh-jfuller wants to merge 2 commits into
Open
Conversation
Contributor
Reviewer's GuideThis PR ensures CPE-only SBOM components (nodes with no PURL but with CPE that matches vulnerabilities) are correctly surfaced in advisory and vulnerability detail endpoints instead of being silently dropped, by making qualified_purl optional across the Rust models and SQL, relaxing joins/filters, and emitting empty purl arrays when appropriate. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="modules/fundamental/src/sbom/model/details.rs" line_range="53" />
<code_context>
Ok(Self {
advisory_id: res.try_get("", "advisory_id")?,
- qualified_purl_id: res.try_get("", "qualified_purl_id")?,
+ qualified_purl_id: res.try_get("", "qualified_purl_id").ok(),
sbom_id: res.try_get("", "sbom_id")?,
sbom_node_id: res.try_get("", "node_id")?,
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `.ok()` here converts all DB errors (not just NULLs) into `None`, potentially hiding genuine query/schema issues.
If you only want to map NULLs to `None`, consider using `try_get::<Option<Uuid>>` (if available) or explicitly handling `DbErr::RecordNotFound`/NULL. The current `.ok()` call turns any failure to read `qualified_purl_id` (including type/column errors) into `None`, which can hide real bugs and lead to inconsistent data.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@rh-jfuller Shouldn't this PR include some tests to prove the list and detail endpoints produce consistent results for CPE only and CPE + PURL components? |
… backlink endpoints
SBOM list endpoint correctly counts vulnerabilities matched via
package level CPE identity (cpe_status) on nodes without a PURL but
both detail endpoints silently dropped them:
- /sbom/{id}/advisory: cpe_advisory_info_sql() filtered out rows /w
NULL qualified_purl_id and IdSet required it to be non-optional Uuid
- /vulnerability/{id}: cpe_status sub-query INNER JOINed
sbom_node_purl_ref and qualified_purl eliminating CPE-only nodes
This causes list page to show higher vuln count detail pages
…ysis get_product_statuses_for_purl joins version_range but never applies version_matches(), returning all version ranges for package name regardless of whether queried version falls within range or uses a compatible version scheme which causes false-positive vuln match This commit passes pURL version into get_product_statuses_for_purl and add version_matches(purl_version, version_range.*) filter, matching pattern already used by sibling purl_status query
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The affected-vulnerability count for an SBOM is inconsistent across endpoints. A vuln matching an SBOM only through a package-level CPE (on a component with no PURL) is counted by the list query but
silently dropped by both detail endpoints:
The discrepancy appears when an SBOM contains a purl-less CPE component whose CPE matches vulnerabilities via
cpe_status.Both detail endpoints required matched SBOM nodes to have a
qualified_purl_id:cpe_advisory_info_sql()had an explicitWHERE p.qualified_purl_id IS NOT NULLfilter.INNER JOIN sbom_node_purl_refand
INNER JOIN qualified_purl, eliminating CPE-only nodes.IdSet/QueryCatcher/SbomStatusCatcherall declaredqualified_purlas mandatory, so even if the SQL returned NULLs thedeserialization would fail.
fwiw - list endpoint's counting CTE had no such requirement, producing correct (higher) count.
Fix
IS NOT NULLfilter fromcpe_advisory_info_sql()INNER JOIN→LEFT JOINforsbom_node_purl_refandqualified_purlin the vulnerability detail cpe_status queryqualified_purloptional inIdSet,QueryCatcher, andSbomStatusCatcherpurlarray for CPE-only packages instead of skippingentire row
which fixes TC-5630
Also fixes TC-5170 by adding version_matches filter to product status query for PURL analysis.
Summary by Sourcery
Include CPE-only SBOM components in advisory and vulnerability detail responses so affected vulnerability counts are consistent with list endpoints.
Bug Fixes:
Enhancements:
Summary by Sourcery
Include CPE-only SBOM components in vulnerability and advisory detail results.
Bug Fixes:
Enhancements: