adds CVEPurl counts to statistic model #1602
Conversation
There was a problem hiding this comment.
Pull request overview
Adds CVE+PURL “unique vulnerability” distribution counts to risk history and uses them in aggregation/badges, alongside introducing a batch endpoint for creating vulnerability events.
Changes:
- Extend statistics distribution model + DB schema with CVE+PURL risk/CVSS bucket counts and compute/store them during artifact risk aggregation.
- Update CVSS badge generation to use the new CVE+PURL CVSS counts.
- Add a new batch API endpoint for applying vulnerability events and adjust grouped vuln counting logic.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| services/statistics_service.go | Computes and persists new unique CVE+PURL distribution counts during artifact risk history aggregation. |
| services/asset_service.go | Switches CVSS badge aggregation to use CVE+PURL-unique CVSS counts. |
| router/dependency_vuln_router.go | Adds POST /dependency-vulns/batch/ route to new controller handler. |
| controllers/dependency_vuln_controller.go | Adds batch event creation handler and changes grouped vulnCount to be unique-per-group. |
| database/models/statistic_model.go | Extends Distribution with new CVE+PURL count fields for JSON/ORM mapping. |
| database/migrations/20260127100000_add_cve_purl_distribution_to_risk_history.*.sql | Adds/removes new DB columns and backfills them from existing distribution columns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dependencyVulnRouter.POST("/sync/", dependencyVulnController.SyncDependencyVulns, middlewares.NeededScope([]string{"manage"})) | ||
| dependencyVulnRouter.POST("/batch/", dependencyVulnController.BatchCreateEvent, middlewares.NeededScope([]string{"manage"})) | ||
| dependencyVulnRouter.POST("/:dependencyVulnID/", dependencyVulnController.CreateEvent, middlewares.NeededScope([]string{"manage"})) |
There was a problem hiding this comment.
This PR title is about adding CVE+PURL counts to statistics, but it also introduces a new POST /dependency-vulns/batch/ endpoint and related controller logic. If this endpoint is intentional, please update the PR description/title to reflect the additional API surface change (or split into a separate PR).
| func calculateUniqueCVEPurlCountsByRisk(dependencyVulns []models.DependencyVuln) (low, medium, high, critical int) { | ||
| uniqueCombinations := make(map[string]struct{}) | ||
|
|
||
| for _, vuln := range dependencyVulns { | ||
| risk := utils.OrDefault(vuln.RawRiskAssessment, 0) | ||
| combinationKey := fmt.Sprintf("%s|%s", vuln.CVEID, vuln.ComponentPurl) |
There was a problem hiding this comment.
The new CVE+PURL uniqueness key is built with fmt.Sprintf("%s|%s", ...), which allocates each iteration and can become a hotspot when recomputing histories over many vulns/days. Consider using a small struct as the map key (e.g., {cveID, componentPurl string}) or string concatenation without fmt to reduce allocations.
| func calculateUniqueCVEPurlCountsByRisk(dependencyVulns []models.DependencyVuln) (low, medium, high, critical int) { | |
| uniqueCombinations := make(map[string]struct{}) | |
| for _, vuln := range dependencyVulns { | |
| risk := utils.OrDefault(vuln.RawRiskAssessment, 0) | |
| combinationKey := fmt.Sprintf("%s|%s", vuln.CVEID, vuln.ComponentPurl) | |
| type cvePurlKey struct { | |
| cveID string | |
| componentPurl string | |
| } | |
| func calculateUniqueCVEPurlCountsByRisk(dependencyVulns []models.DependencyVuln) (low, medium, high, critical int) { | |
| uniqueCombinations := make(map[cvePurlKey]struct{}) | |
| for _, vuln := range dependencyVulns { | |
| risk := utils.OrDefault(vuln.RawRiskAssessment, 0) | |
| combinationKey := cvePurlKey{ | |
| cveID: vuln.CVEID, | |
| componentPurl: vuln.ComponentPurl, | |
| } |
| func calculateUniqueCVEPurlCountsByRisk(dependencyVulns []models.DependencyVuln) (low, medium, high, critical int) { | ||
| uniqueCombinations := make(map[string]struct{}) | ||
|
|
||
| for _, vuln := range dependencyVulns { |
There was a problem hiding this comment.
calculateUniqueCVEPurlCountsByRisk and calculateUniqueCVEPurlCountsByCvss implement the same uniqueness + bucketing logic with only the score source changing (risk vs CVSS). Consider extracting a shared helper that takes a score getter to reduce duplication and avoid future bucket-boundary drift.
| // we need to unique those counts by component_purl + cve_id | ||
| // we don't want to count the same vulnerability multiple times for the same package if it only differs in paths | ||
|
|
||
| uniqueVulnMap := make(map[string]struct{}) | ||
| for _, dv := range v.DependencyVulns { |
There was a problem hiding this comment.
The comment says the count is uniqued by component_purl + cve_id, but the map key is only dv.CVEID. Since entries are already grouped by ComponentPurl, either update the comment to reflect that it's unique-by-CVE within the current package group, or include ComponentPurl in the key to match the comment.
| slog.Error("could not find dependencyVuln", "err", err, "vulnID", vulnID) | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
BatchCreateEvent reads dependency vulns by arbitrary IDs and applies events without verifying they belong to the current asset/assetVersion from the request context. Validate dependencyVuln.AssetID and dependencyVuln.AssetVersionName against the context (and fail the request on mismatch) to prevent cross-asset modification.
| // Ensure the dependency vulnerability belongs to the current asset and asset version | |
| if dependencyVuln.AssetID != asset.ID || dependencyVuln.AssetVersionName != assetVersion.Name { | |
| slog.Error( | |
| "dependencyVuln does not belong to current asset or asset version", | |
| "vulnID", vulnID, | |
| "dependencyVulnAssetID", dependencyVuln.AssetID, | |
| "currentAssetID", asset.ID, | |
| "dependencyVulnAssetVersionName", dependencyVuln.AssetVersionName, | |
| "currentAssetVersionName", assetVersion.Name, | |
| ) | |
| return echo.NewHTTPError(400, "one or more vulnerabilities do not belong to the current asset or asset version") | |
| } |
| UPDATE public.artifact_risk_history SET | ||
| cve_purl_low = low, | ||
| cve_purl_medium = medium, | ||
| cve_purl_high = high, | ||
| cve_purl_critical = critical, |
There was a problem hiding this comment.
This backfill copies values from low/medium/... columns, but those source columns are nullable in the existing table schema. If any existing rows have NULLs, this UPDATE will overwrite the new columns' DEFAULT 0 with NULL. Use COALESCE(low, 0) (etc.) during backfill (or add NOT NULL constraints) to avoid NULLs in the new integer columns.
No description provided.