From 33436301715ee21f6bb341296979ab22580a27a1 Mon Sep 17 00:00:00 2001 From: Anso Date: Fri, 26 Jun 2026 23:35:57 -0400 Subject: [PATCH] fix: always reconcile the scan banner with the current policy verdict (#1488) The scan-detail banner only recomputed its verdict when honor-suppressions was enabled; otherwise it returned the snapshot stored at scan time. The deploy gate always re-evaluates current policies, so with honor-suppressions off a policy lifecycle change drifted the banner from the gate: disabling (or editing) an enabled policy left the banner claiming a violation the gate would now pass, and tightening a passing policy left the banner reporting a pass the gate would block. Recompute the banner verdict unconditionally so it agrees with the gate across the full policy and suppression lifecycle, regardless of the honor-suppressions setting. The recompute already reads that setting itself (so a raw or suppression-filtered verdict is chosen correctly), returns no verdict when no policy matches (clearing the banner to match a passing gate), stays read-only, and still falls back to the stored snapshot if it throws. --- ...ity-scan-banner-suppressions-route.test.ts | 59 +++++++++++++++---- backend/src/routes/security.ts | 39 ++++++------ 2 files changed, 66 insertions(+), 32 deletions(-) diff --git a/backend/src/__tests__/security-scan-banner-suppressions-route.test.ts b/backend/src/__tests__/security-scan-banner-suppressions-route.test.ts index 38ccf43e..c2f14770 100644 --- a/backend/src/__tests__/security-scan-banner-suppressions-route.test.ts +++ b/backend/src/__tests__/security-scan-banner-suppressions-route.test.ts @@ -1,14 +1,13 @@ /** * GET /api/security/scans/:scanId banner consistency with the deploy gate. * - * The verdict stored on a scan at scan time is a one-time snapshot. Once the - * deploy gate is set to honor suppressions, the gate re-reads current - * suppressions on every deploy while the stored verdict does not, so creating, - * deleting, or expiring a suppression used to leave the scan-detail banner - * disagreeing with what the gate would actually do. The detail route now - * recomputes the banner verdict against current suppressions; these tests pin - * that the banner tracks the gate across the suppression lifecycle, and that the - * stored snapshot is still used verbatim when honor-suppressions is off. + * The verdict stored on a scan at scan time is a one-time snapshot, while the + * deploy gate always re-evaluates current policies and suppressions. The detail + * route therefore recomputes the banner verdict on every read so it tracks the + * gate across the full lifecycle: creating, deleting, editing, or expiring a + * suppression, and enabling, disabling, or tightening a policy, with + * honor-suppressions both on and off. These tests pin that agreement; the stored + * snapshot is used only as a fallback if the recompute throws. */ import { describe, it, expect, beforeAll, afterAll, beforeEach, vi } from 'vitest'; import request from 'supertest'; @@ -150,11 +149,51 @@ describe('GET /api/security/scans/:scanId banner vs deploy gate', () => { } }); - it('uses the stored snapshot verbatim when honor-suppressions is off', async () => { + it('recomputes the raw verdict when honor-suppressions is off, ignoring suppressions like the gate', async () => { const scanId = seedViolatingScan(); suppress(null); DatabaseService.getInstance().updateGlobalSetting('deploy_block_honor_suppressions', '0'); - // Setting off: the gate ignores suppressions too, so the raw stored verdict stands. + // Honor off: the gate ignores suppressions, so the recomputed banner still + // flags the KEV (matching the raw gate), rather than honoring the suppression. + expect(await bannerViolated(scanId)).toBe(true); + }); + + it('clears the banner when the policy is disabled, even with honor-suppressions off', async () => { + const scanId = seedViolatingScan(); + const db = DatabaseService.getInstance(); + db.updateGlobalSetting('deploy_block_honor_suppressions', '0'); + // The stored snapshot says violated, but a disabled policy makes the gate pass. + // The banner must recompute (to no verdict), not echo the stale snapshot. + db.getScanPolicies().forEach((p) => db.updateScanPolicy(p.id, { enabled: 0 })); + expect(await bannerViolated(scanId)).toBeFalsy(); + // The stored snapshot is unchanged, proving the banner was recomputed at read time. + expect(parsePolicyEvaluation(db.getVulnerabilityScan(scanId)?.policy_evaluation)?.violated).toBe(true); + // Re-enabling the policy brings the violation back, matching the gate again. + db.getScanPolicies().forEach((p) => db.updateScanPolicy(p.id, { enabled: 1 })); + expect(await bannerViolated(scanId)).toBe(true); + }); + + it('shows a violation after a passing policy is tightened to block, with honor-suppressions off', async () => { + const db = DatabaseService.getInstance(); + db.updateGlobalSetting('deploy_block_honor_suppressions', '0'); + const policy = db.createScanPolicy({ + name: 'sev', node_id: null, node_identity: '', stack_pattern: 'web', + max_severity: 'CRITICAL', block_on_deploy: 1, enabled: 1, + block_on_severity: 1, block_on_kev: 0, block_on_fixable: 0, replicated_from_control: 0, + }); + const scanId = db.createVulnerabilityScan({ + node_id: 1, image_ref: 'web:1', image_digest: null, scanned_at: Date.now(), + total_vulnerabilities: 1, critical_count: 0, high_count: 1, medium_count: 0, low_count: 0, + unknown_count: 0, fixable_count: 0, secret_count: 0, misconfig_count: 0, scanners_used: 'vuln', + highest_severity: 'HIGH', os_info: null, trivy_version: '0.50.0', scan_duration_ms: null, + triggered_by: 'manual', status: 'completed', error: null, stack_context: 'web', + }); + const scan = db.getVulnerabilityScan(scanId) as VulnerabilityScan; + db.setScanPolicyEvaluation(scanId, db.evaluateScanAgainstPolicies(1, scan, FleetSyncService.getSelfIdentity())); + // HIGH is within a CRITICAL threshold, so the stored verdict passes. + expect(await bannerViolated(scanId)).toBeFalsy(); + // Tightening the threshold to HIGH makes the gate block; the banner must follow. + db.updateScanPolicy(policy.id, { max_severity: 'HIGH' }); expect(await bannerViolated(scanId)).toBe(true); }); }); diff --git a/backend/src/routes/security.ts b/backend/src/routes/security.ts index c66ad21e..c3e25ca2 100644 --- a/backend/src/routes/security.ts +++ b/backend/src/routes/security.ts @@ -114,39 +114,28 @@ function parseScannersInput(raw: unknown): readonly ('vuln' | 'secret')[] | unde return Array.from(out) as readonly ('vuln' | 'secret')[]; } -function shapeScanForResponse( - scan: VulnerabilityScan, - // The scan-detail endpoint passes a verdict recomputed against current - // suppressions (resolveBannerEvaluation) so the banner matches the deploy - // gate; other callers omit it and the snapshot stored at scan time is used. - evaluationOverride?: ReturnType, -): Omit & { +function shapeScanForResponse(scan: VulnerabilityScan): Omit & { policy_evaluation: ReturnType; } { const { policy_evaluation, ...rest } = scan; - return { - ...rest, - policy_evaluation: evaluationOverride !== undefined ? evaluationOverride : parsePolicyEvaluation(policy_evaluation), - }; + return { ...rest, policy_evaluation: parsePolicyEvaluation(policy_evaluation) }; } /** * The policy verdict the scan-detail banner shows. The verdict stored at scan - * time drifts from the deploy gate once the gate is set to honor suppressions, - * because the gate always re-reads current suppressions while the stored value - * is a one-time snapshot: creating, editing, deleting, or expiring a suppression - * leaves the banner claiming a violation the gate would now pass, or the reverse. - * Recompute against current suppressions so the banner agrees with the gate. - * With honor-suppressions off, suppressions affect neither the gate nor the - * verdict, so the stored snapshot is authoritative and returned without a reread. + * time is a one-time snapshot, while the deploy gate always re-evaluates current + * policies and suppressions: enabling, disabling, or editing a policy (and + * creating, editing, deleting, or expiring a suppression) leaves the stored + * snapshot claiming a violation the gate would now pass, or the reverse. Always + * recompute so the banner agrees with the gate across the full policy and + * suppression lifecycle. evaluateScanAgainstPolicies reads the honor-suppressions + * setting itself, so this is correct whether or not suppressions are honored, and + * returns null when no policy matches (the banner clears, matching a passing gate). */ function resolveBannerEvaluation( db: DatabaseService, scan: VulnerabilityScan, ): ReturnType { - if (db.getGlobalSettings()['deploy_block_honor_suppressions'] !== '1') { - return parsePolicyEvaluation(scan.policy_evaluation); - } try { return db.evaluateScanAgainstPolicies(scan.node_id, scan, FleetSyncService.getSelfIdentity()); } catch (err) { @@ -612,7 +601,13 @@ securityRouter.get('/scans/:scanId', authMiddleware, (req: Request, res: Respons }).filter(Boolean), ); const publiclyExposed = exposedMap.get(scan.image_ref) ?? null; - res.json({ ...shapeScanForResponse(scan, resolveBannerEvaluation(db, scan)), publicly_exposed: publiclyExposed }); + // The banner verdict is recomputed against current policies/suppressions so it + // matches the deploy gate; the list endpoint keeps the stored snapshot. + res.json({ + ...shapeScanForResponse(scan), + policy_evaluation: resolveBannerEvaluation(db, scan), + publicly_exposed: publiclyExposed, + }); }); securityRouter.get(