fix(evaluator): create package linkages for all CVE types#2183
Open
mtclinton wants to merge 1 commit intoRedHatInsights:masterfrom
Open
fix(evaluator): create package linkages for all CVE types#2183mtclinton wants to merge 1 commit intoRedHatInsights:masterfrom
mtclinton wants to merge 1 commit intoRedHatInsights:masterfrom
Conversation
|
Referenced Jiras: |
Reviewer's GuideExtends the evaluator’s VMAAS handling so that playbook and manually-fixable CVEs also produce package-level linkages (including module stream inference), preventing orphaned vulnerabilities where systems are marked vulnerable without associated packages. Sequence diagram for updated VMAAS evaluation and package linkage creationsequenceDiagram
participant Evaluator
participant VMAASService
participant Database
Evaluator->>VMAASService: vmaas_request(vmaas_vulnerabilities_endpoint, vmaas_json)
VMAASService-->>Evaluator: vmaas_response
alt vmaas_response missing package_list or repository_list
Evaluator-->>Evaluator: return empty lists
else vmaas_response valid
Evaluator-->>Evaluator: _perform_vmaas_request
loop build_playbook_and_manual_lists
Evaluator-->>Evaluator: create CveAdvisories for playbook_cves
Evaluator-->>Evaluator: create CveUnpatched for playbook_cve_packages
Evaluator-->>Evaluator: create CveAdvisories for manually_fixable_cves
Evaluator-->>Evaluator: create CveUnpatched for manually_fixable_cve_packages
end
loop build_unpatched_list
Evaluator-->>Evaluator: create CveUnpatched for unpatched_cves
end
Evaluator-->>Evaluator: _evaluate_vmaas_res(..., playbook_cve_packages, manually_fixable_cve_packages)
Evaluator-->>Evaluator: all_cve_packages = unpatched_cves + playbook_cve_packages + manually_fixable_cve_packages
loop for each cve_package in all_cve_packages
alt missing package_name or cpe
Evaluator-->>Evaluator: skip cve_package
else
alt module info missing
Evaluator-->>Evaluator: _get_module_from_system(package_name, system_platform.vmaas_json)
end
Evaluator-->>Evaluator: _get_or_upsert_cve(cve)
Evaluator-->>Evaluator: pn_cpes[(package_name, cpe, module_name, module_stream)] add cve_id
end
end
loop for each (package_name, cpe, module_name, module_stream) in pn_cpes
Evaluator-->>Evaluator: _get_or_upsert_vulnerable_package(...)
Evaluator->>Database: upsert system_vulnerable_package rows
Database-->>Evaluator: success
end
end
Updated class diagram for evaluator VMAAS CVE and package handlingclassDiagram
class Evaluator {
+_perform_vmaas_request(vmaas_json: dict) Tuple~List~CveAdvisories~~,List~CveAdvisories~~,List~CveUnpatched~~,List~CveUnpatched~~,List~CveUnpatched~~~
+_get_module_from_system(package_name: str, vmaas_json: dict) Tuple~Optional~str~~,Optional~str~~
+_evaluate_vmaas_res(playbook_cves: List~CveAdvisories~, manually_fixable_cves: List~CveAdvisories~, unpatched_cves: List~CveUnpatched~, playbook_cve_packages: List~CveUnpatched~, manually_fixable_cve_packages: List~CveUnpatched~, sys_vuln_rows: dict, system_platform: SystemPlatform, conn: AsyncConnection) dict
+evaluate_vulnerabilities(system_platform: SystemPlatform, conn: AsyncConnection)
+_get_or_upsert_cve(cve_name: str) CveCache
+_get_or_upsert_vulnerable_package(package_name_id: int, cpe_id: int, module_id: int) VulnerablePackage
}
class CveAdvisories {
+cve: str
+advisories: Optional~str~
}
class CveUnpatched {
+cve: str
+package_name: str
+cpe: str
+module_name: Optional~str~
+module_stream: Optional~str~
}
class SystemPlatform {
+vmaas_json: dict
}
class CveCache {
+id: int
+cve: str
}
class VulnerablePackage {
+id: int
+package_name_id: int
+cpe_id: int
+module_id: Optional~int~
}
class AsyncConnection
Evaluator --> CveAdvisories : uses
Evaluator --> CveUnpatched : uses
Evaluator --> SystemPlatform : reads vmaas_json
Evaluator --> CveCache : upserts
Evaluator --> VulnerablePackage : upserts
Evaluator --> AsyncConnection : database operations
CveUnpatched --> SystemPlatform : module inference via vmaas_json
File-Level Changes
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 - I've found 2 issues, and left some high level feedback:
- In
_perform_vmaas_request, you constructCveUnpatchedwithpackage_name/cpedefaulting to empty strings and later skip entries where those fields are falsy; consider either skipping such entries at creation time or representing missing values asNoneso it's clearer when data is actually absent and you avoid creating objects that are immediately discarded. - The
_get_module_from_systemheuristic of matching modules bypackage_name.startswith(module_name)could easily mis-associate packages to modules with similar prefixes; it may be worth tightening this logic (e.g., delimiter-aware matching or explicit mapping) or at least constraining it to known-safe patterns to reduce incorrect module assignments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_perform_vmaas_request`, you construct `CveUnpatched` with `package_name`/`cpe` defaulting to empty strings and later skip entries where those fields are falsy; consider either skipping such entries at creation time or representing missing values as `None` so it's clearer when data is actually absent and you avoid creating objects that are immediately discarded.
- The `_get_module_from_system` heuristic of matching modules by `package_name.startswith(module_name)` could easily mis-associate packages to modules with similar prefixes; it may be worth tightening this logic (e.g., delimiter-aware matching or explicit mapping) or at least constraining it to known-safe patterns to reduce incorrect module assignments.
## Individual Comments
### Comment 1
<location> `evaluator/logic.py:310-312` </location>
<code_context>
@time(EVAL_PART_TIME.labels(part="vmaas_request"))
- async def _perform_vmaas_request(self, vmaas_json: dict) -> (List[CveAdvisories], List[CveAdvisories], List[CveUnpatched]):
- """Perform VMAAS request for package based evaluation"""
+ async def _perform_vmaas_request(
+ self, vmaas_json: dict
+ ) -> (List[CveAdvisories], List[CveAdvisories], List[CveUnpatched], List[CveUnpatched], List[CveUnpatched]):
+ """Perform VMAAS request for package based evaluation
+
</code_context>
<issue_to_address>
**suggestion:** Use a proper Tuple type for the multi-value return annotation instead of a bare parenthesized type list.
`-> (List[...], ...)` is parsed as just the last type, not a multi-value return. Please use a proper tuple annotation, e.g. `-> Tuple[List[CveAdvisories], List[CveAdvisories], List[CveUnpatched], List[CveUnpatched], List[CveUnpatched]]`, so type checkers and editors understand the structure and can catch regressions if the return shape changes.
Suggested implementation:
```python
@time(EVAL_PART_TIME.labels(part="vmaas_request"))
async def _perform_vmaas_request(
self, vmaas_json: dict
) -> Tuple[
List[CveAdvisories],
List[CveAdvisories],
List[CveUnpatched],
List[CveUnpatched],
List[CveUnpatched],
]:
```
1. Ensure `Tuple` is imported from `typing` at the top of `evaluator/logic.py`, e.g. extend an existing `from typing import ...` line to include `Tuple`.
2. If `List` and the CVE types are not already imported in this file, verify they are properly imported or aliased to keep type checking consistent.
</issue_to_address>
### Comment 2
<location> `evaluator/logic.py:510-519` </location>
<code_context>
to_delete,
)
+ def _get_module_from_system(self, package_name: str, vmaas_json: dict) -> Tuple[Optional[str], Optional[str]]:
+ """Try to match package to module from system's modules_list"""
+
+ if not vmaas_json or not vmaas_json.get("modules_list"):
+ return None, None
+
+ # Check if package name matches any module name in the system's modules_list
+ for module in vmaas_json["modules_list"]:
+ if isinstance(module, dict):
+ module_name = module.get("module_name")
+ module_stream = module.get("module_stream")
+ # if package name starts with module name, likely from that module
+ if module_name and package_name.startswith(module_name):
+ return module_name, module_stream
+ return None, None
</code_context>
<issue_to_address>
**issue:** Prefix-based module detection may mis-assign modules for similarly named packages.
Using `package_name.startswith(module_name)` risks matching unrelated modules that share a prefix (e.g., module `foo` vs package `foo-bar`). If this mapping affects correctness (e.g., stream selection or CVE attribution), consider tightening the condition (exact match, valid separators, or additional metadata) and/or restricting it to known-safe naming patterns in your environment.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+310
to
+312
| async def _perform_vmaas_request( | ||
| self, vmaas_json: dict | ||
| ) -> (List[CveAdvisories], List[CveAdvisories], List[CveUnpatched], List[CveUnpatched], List[CveUnpatched]): |
There was a problem hiding this comment.
suggestion: Use a proper Tuple type for the multi-value return annotation instead of a bare parenthesized type list.
-> (List[...], ...) is parsed as just the last type, not a multi-value return. Please use a proper tuple annotation, e.g. -> Tuple[List[CveAdvisories], List[CveAdvisories], List[CveUnpatched], List[CveUnpatched], List[CveUnpatched]], so type checkers and editors understand the structure and can catch regressions if the return shape changes.
Suggested implementation:
@time(EVAL_PART_TIME.labels(part="vmaas_request"))
async def _perform_vmaas_request(
self, vmaas_json: dict
) -> Tuple[
List[CveAdvisories],
List[CveAdvisories],
List[CveUnpatched],
List[CveUnpatched],
List[CveUnpatched],
]:- Ensure
Tupleis imported fromtypingat the top ofevaluator/logic.py, e.g. extend an existingfrom typing import ...line to includeTuple. - If
Listand the CVE types are not already imported in this file, verify they are properly imported or aliased to keep type checking consistent.
Comment on lines
+510
to
+519
| def _get_module_from_system(self, package_name: str, vmaas_json: dict) -> Tuple[Optional[str], Optional[str]]: | ||
| """Try to match package to module from system's modules_list""" | ||
|
|
||
| if not vmaas_json or not vmaas_json.get("modules_list"): | ||
| return None, None | ||
|
|
||
| # Check if package name matches any module name in the system's modules_list | ||
| for module in vmaas_json["modules_list"]: | ||
| if isinstance(module, dict): | ||
| module_name = module.get("module_name") |
There was a problem hiding this comment.
issue: Prefix-based module detection may mis-assign modules for similarly named packages.
Using package_name.startswith(module_name) risks matching unrelated modules that share a prefix (e.g., module foo vs package foo-bar). If this mapping affects correctness (e.g., stream selection or CVE attribution), consider tightening the condition (exact match, valid separators, or additional metadata) and/or restricting it to known-safe naming patterns in your environment.
mtclinton
force-pushed
the
nodesjs-false-positive-fix
branch
from
ef8b5e9 to
9a65cc4
Compare
Contributor
Author
|
/retest |
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
1 participant
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.
Secure Coding Practices Checklist GitHub Link
RHINENG-22053
The evaluator was creating VULNERABLE_BY_PACKAGE entries but failing to create
corresponding system_vulnerable_package entries for playbook_cves and
manually_fixable_cves. This caused orphaned vulnerabilities where systems
were marked vulnerable but had no package linkages.
Now extracts package information from all CVE types and creates proper
package linkages with module stream information
Secure Coding Checklist
Summary by Sourcery
Ensure evaluator creates package linkages for all CVE types using extended VMAAS response data.
Bug Fixes:
Enhancements: