From d893eb65aac033afdf57b0d0ee06b70d9710e2b7 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 11 Jun 2026 16:28:50 -0400 Subject: [PATCH 1/2] fix(cloud-security): don't let a fully-failed GCP/Azure scan wipe good results MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cloud scan that errored on every scope/unit silently returned [], which was stored as a fresh status:'success' run with 0 findings. The UI shows only the latest run per connection, so the empty run hid the previous good results ("0 total — Looking good"). This is what a customer hit on GCP after a scan. GCP: scanSecurityFindings swallowed every per-scope SCC error and returned []. Add an all-scopes-failed guard mirroring AWS (aws-security.service.ts:396-405): track successful/failed scopes and re-throw the underlying SCC error when every scope failed. A thrown scan skips storeFindings, so the prior good run stays visible and the actionable error (SCC_NOT_ACTIVATED / PERMISSION_DENIED) finally reaches the UI. A scope that succeeds with 0 findings does not throw. Azure: same class of bug, no guard at all. A non-403 total failure (transient 500s / throttling) returned []. Refactor scanDefender to report whether any query succeeded, and throw when the result is empty AND a unit failed — a clean subscription still emits passing findings, so empty+failure is the real tell. 403 permission-findings and genuinely-empty subscriptions are unaffected. AWS already handles this (regional all-failed guard, always active because baseline regional services are always scanned) — verified, left unchanged. Tests: +4 GCP, +4 Azure (new azure-security.service.spec.ts). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../providers/azure-security.service.spec.ts | 72 ++++++++++++ .../providers/azure-security.service.ts | 83 ++++++++++--- .../providers/gcp-security.service.spec.ts | 110 ++++++++++++++++++ .../providers/gcp-security.service.ts | 29 ++++- 4 files changed, 275 insertions(+), 19 deletions(-) create mode 100644 apps/api/src/cloud-security/providers/azure-security.service.spec.ts diff --git a/apps/api/src/cloud-security/providers/azure-security.service.spec.ts b/apps/api/src/cloud-security/providers/azure-security.service.spec.ts new file mode 100644 index 0000000000..51510079e5 --- /dev/null +++ b/apps/api/src/cloud-security/providers/azure-security.service.spec.ts @@ -0,0 +1,72 @@ +// AzureSecurityService scans Defender + a suite of ARM service adapters, all of +// which go through the global `fetch`. These tests mock `fetch` to exercise the +// all-units-failed guard: a scan where every unit errors must THROW (so it is +// not stored as a fresh empty "success" run that hides the prior good results), +// while a genuinely-clean subscription (everything succeeds, 0 findings) must +// return [] without throwing. +jest.mock('@db', () => ({ db: {} })); + +import { AzureSecurityService } from './azure-security.service'; + +function azureOkEmpty(): { ok: true; json: () => Promise } { + return { ok: true, json: async () => ({ value: [] }) }; +} + +function azureError( + status: number, + text = 'error', +): { ok: false; status: number; text: () => Promise } { + return { ok: false, status, text: async () => text }; +} + +describe('AzureSecurityService.scanSecurityFindings — all-units-failed guard', () => { + let service: AzureSecurityService; + let fetchMock: jest.Mock; + const originalFetch = global.fetch; + + const creds = { access_token: 'tok' }; + const vars = { subscription_id: 'sub-1' }; + + beforeEach(() => { + fetchMock = jest.fn(); + // @ts-expect-error replacing global fetch with a mock for these tests + global.fetch = fetchMock; + service = new AzureSecurityService(); + }); + + afterAll(() => { + global.fetch = originalFetch; + }); + + // The bug: a total (non-403) failure used to return [] silently, which got + // stored as a fresh "success" run with 0 findings and hid the prior results. + it('throws when the whole scan errors out and produces no findings, instead of returning []', async () => { + fetchMock.mockResolvedValue(azureError(500, 'internal server error')); + + await expect(service.scanSecurityFindings(creds, vars)).rejects.toThrow(); + }); + + it('does NOT throw on a healthy subscription — adapters emit passing findings', async () => { + fetchMock.mockResolvedValue(azureOkEmpty()); + + const findings = await service.scanSecurityFindings(creds, vars); + expect(findings.length).toBeGreaterThan(0); + }); + + it('does NOT throw when results are produced even though one adapter fails', async () => { + fetchMock.mockImplementation(async (url: string) => { + // Storage adapter fails; everything else succeeds (and emits findings). + if (url.includes('Microsoft.Storage')) return azureError(500, 'boom'); + return azureOkEmpty(); + }); + + const findings = await service.scanSecurityFindings(creds, vars); + expect(findings.length).toBeGreaterThan(0); + }); + + it('still enforces required inputs (missing subscription throws AZURE_SUB_MISSING)', async () => { + await expect(service.scanSecurityFindings(creds, {})).rejects.toThrow( + /AZURE_SUB_MISSING/, + ); + }); +}); diff --git a/apps/api/src/cloud-security/providers/azure-security.service.ts b/apps/api/src/cloud-security/providers/azure-security.service.ts index cb5e22823e..604d9c5824 100644 --- a/apps/api/src/cloud-security/providers/azure-security.service.ts +++ b/apps/api/src/cloud-security/providers/azure-security.service.ts @@ -106,41 +106,82 @@ export class AzureSecurityService { this.logger.log(`Scanning Azure subscription ${subscriptionId}`); const findings: SecurityFinding[] = []; + // Track unit failures so a scan that comes back EMPTY because every unit + // (Defender + each adapter) errored fails loudly instead of being stored as + // a fresh "success" run with 0 findings — which would hide the previous good + // results (the UI shows only the latest run). A healthy subscription still + // emits passing findings, so "empty result + a failure" is the real tell. + let failedUnits = 0; + let firstError: Error | null = null; // 1. Defender alerts + assessments (always runs) if (!enabledServices || enabledServices.includes('defender')) { - const defenderFindings = await this.scanDefender(token, subscriptionId); - findings.push(...defenderFindings); + const defender = await this.scanDefender(token, subscriptionId); + findings.push(...defender.findings); + if (!defender.anySucceeded) { + failedUnits++; + if (defender.firstError && !firstError) firstError = defender.firstError; + } } // 2. Run service adapters in parallel - const adapterPromises = SERVICE_ADAPTERS.filter( + const activeAdapters = SERVICE_ADAPTERS.filter( (a) => !enabledServices || enabledServices.includes(a.serviceId), - ).map(async (adapter) => { - try { - return await adapter.scan({ accessToken: token, subscriptionId }); - } catch (error) { - const msg = error instanceof Error ? error.message : String(error); - this.logger.warn(`Azure ${adapter.serviceId} scan failed: ${msg}`); - return []; + ); + const adapterResults = await Promise.all( + activeAdapters.map(async (adapter) => { + try { + const scanned = await adapter.scan({ + accessToken: token, + subscriptionId, + }); + return { ok: true as const, findings: scanned }; + } catch (error) { + const err = error instanceof Error ? error : new Error(String(error)); + this.logger.warn(`Azure ${adapter.serviceId} scan failed: ${err.message}`); + return { ok: false as const, error: err }; + } + }), + ); + for (const result of adapterResults) { + if (result.ok) { + findings.push(...result.findings); + } else { + failedUnits++; + if (!firstError) firstError = result.error; } - }); + } - const adapterResults = await Promise.all(adapterPromises); - for (const result of adapterResults) { - findings.push(...result); + // An empty result combined with a failed unit is a degenerate/broken scan, + // not a genuinely-clean subscription (a clean scan still emits passing + // findings). Throw so nothing is stored and the prior good run stays + // visible. Mirrors the AWS all-regions-failed and GCP all-scopes-failed + // guards. A truly-empty subscription (every unit succeeded, no failures) + // still returns [] normally. + if (findings.length === 0 && failedUnits > 0) { + throw firstError ?? new Error('All Azure service scans failed'); } this.logger.log(`Azure scan complete: ${findings.length} total findings`); return findings; } - /** Scan Defender for Cloud alerts and assessments. */ + /** + * Scan Defender for Cloud alerts and assessments. Reports whether at least + * one query succeeded so the caller can tell a genuinely-clean subscription + * (anySucceeded=true, no findings) apart from a total failure. + */ private async scanDefender( accessToken: string, subscriptionId: string, - ): Promise { + ): Promise<{ + findings: SecurityFinding[]; + anySucceeded: boolean; + firstError: Error | null; + }> { const findings: SecurityFinding[] = []; + let anySucceeded = false; + let firstError: Error | null = null; // Alerts try { @@ -173,6 +214,7 @@ export class AzureSecurityService { createdAt: alert.properties.startTimeUtc || new Date().toISOString(), }); } + anySucceeded = true; } catch (error) { this.handlePermissionError( findings, @@ -180,6 +222,9 @@ export class AzureSecurityService { 'Security Alerts', subscriptionId, ); + if (!firstError) { + firstError = error instanceof Error ? error : new Error(String(error)); + } } // Assessments @@ -227,6 +272,7 @@ export class AzureSecurityService { createdAt: new Date().toISOString(), }); } + anySucceeded = true; } catch (error) { this.handlePermissionError( findings, @@ -234,9 +280,12 @@ export class AzureSecurityService { 'Security Assessments', subscriptionId, ); + if (!firstError) { + firstError = error instanceof Error ? error : new Error(String(error)); + } } - return findings; + return { findings, anySucceeded, firstError }; } private handlePermissionError( diff --git a/apps/api/src/cloud-security/providers/gcp-security.service.spec.ts b/apps/api/src/cloud-security/providers/gcp-security.service.spec.ts index e18d0f8bea..9d79c6452b 100644 --- a/apps/api/src/cloud-security/providers/gcp-security.service.spec.ts +++ b/apps/api/src/cloud-security/providers/gcp-security.service.spec.ts @@ -698,3 +698,113 @@ describe('resolveGcpServiceId — Vertex AI grouping', () => { ); }); }); + +// ── SCC findings response helpers ─────────────────────────────────────────── +function sccPage( + findings: Array>, + nextPageToken?: string, +): { ok: true; json: () => Promise } { + return { + ok: true, + json: async () => ({ + listFindingsResults: findings, + ...(nextPageToken ? { nextPageToken } : {}), + }), + }; +} + +function sccError( + status: number, + text: string, +): { ok: false; status: number; text: () => Promise } { + return { ok: false, status, text: async () => text }; +} + +function sccFinding(project: string): Record { + return { + finding: { + name: `organizations/1/sources/-/findings/${project}-1`, + category: 'PUBLIC_BUCKET_ACL', + description: 'desc', + severity: 'HIGH', + state: 'ACTIVE', + resourceName: `//storage.googleapis.com/projects/${project}/buckets/b`, + eventTime: '2026-01-01T00:00:00Z', + createTime: '2026-01-01T00:00:00Z', + }, + resource: { + type: 'storage.googleapis.com/Bucket', + projectDisplayName: project, + }, + }; +} + +describe('GCPSecurityService.scanSecurityFindings — all-scopes-failed guard', () => { + let service: GCPSecurityService; + let fetchMock: jest.Mock; + const originalFetch = global.fetch; + + beforeEach(() => { + fetchMock = jest.fn(); + // @ts-expect-error replacing global fetch with a mock for these tests + global.fetch = fetchMock; + service = new GCPSecurityService(); + }); + + afterAll(() => { + global.fetch = originalFetch; + }); + + // The bug: when the only scope errored, the scan used to swallow the error and + // return [] — which got stored as a fresh "success" run with 0 findings, + // hiding the previous good results. It must now throw so nothing is stored. + it('throws (re-throwing the actionable SCC error) when the only scope errors, instead of returning []', async () => { + fetchMock.mockResolvedValue( + sccError(403, 'PERMISSION_DENIED: caller lacks securitycenter.findings.list'), + ); + + await expect( + service.scanSecurityFindings( + { access_token: 'tok' }, + { project_ids: ['wasimil-prod'] }, + ), + ).rejects.toThrow(/Permission denied/); + }); + + it('throws when EVERY configured scope errors', async () => { + fetchMock.mockResolvedValue(sccError(500, 'internal error')); + + await expect( + service.scanSecurityFindings( + { access_token: 'tok' }, + { project_ids: ['p1', 'p2'] }, + ), + ).rejects.toThrow(); + }); + + it('does NOT throw when at least one scope succeeds — returns the surviving scope’s findings', async () => { + fetchMock.mockImplementation(async (url: string) => { + if (url.includes('projects/p1')) return sccError(403, 'PERMISSION_DENIED'); + return sccPage([sccFinding('p2')]); + }); + + const findings = await service.scanSecurityFindings( + { access_token: 'tok' }, + { project_ids: ['p1', 'p2'] }, + ); + + expect(findings).toHaveLength(1); + expect(findings[0].resourceId).toContain('projects/p2'); + }); + + it('does NOT throw when a scope succeeds with zero findings (genuinely clean account)', async () => { + fetchMock.mockResolvedValue(sccPage([])); + + const findings = await service.scanSecurityFindings( + { access_token: 'tok' }, + { project_ids: ['wasimil-prod'] }, + ); + + expect(findings).toEqual([]); + }); +}); diff --git a/apps/api/src/cloud-security/providers/gcp-security.service.ts b/apps/api/src/cloud-security/providers/gcp-security.service.ts index cf78ff86ba..2516395bd9 100644 --- a/apps/api/src/cloud-security/providers/gcp-security.service.ts +++ b/apps/api/src/cloud-security/providers/gcp-security.service.ts @@ -1375,6 +1375,11 @@ export class GCPSecurityService { const allFindings: SecurityFinding[] = []; const enabledServiceSet = enabledServices ? new Set(enabledServices) : null; const seenIds = new Set(); + // Track per-scope outcomes so a scan where EVERY scope errored fails loudly + // instead of silently returning [] (see the all-scopes-failed guard below). + const successfulScopes = new Set(); + const failedScopes = new Set(); + let firstScopeError: Error | null = null; for (const scope of scopes) { try { @@ -1434,14 +1439,34 @@ export class GCPSecurityService { pageToken = response.nextPageToken; } while (pageToken); + successfulScopes.add(scope.id); } catch (err) { - // Log and continue with remaining projects — don't fail the whole scan + // Log and continue with the remaining scopes — one bad project must not + // abort the others. But remember the failure: if EVERY scope fails we + // surface it below instead of reporting a misleading "0 findings". + const error = err instanceof Error ? err : new Error(String(err)); this.logger.warn( - `GCP SCC query failed for ${scope.type} ${scope.id}: ${err instanceof Error ? err.message : String(err)}`, + `GCP SCC query failed for ${scope.type} ${scope.id}: ${error.message}`, ); + failedScopes.add(scope.id); + if (!firstScopeError) firstScopeError = error; } } + // If every configured scope errored, fail the scan instead of returning []. + // A silent [] is stored as a fresh "success" run with 0 findings, which + // (a) hides the previous good results — the UI shows only the latest run — + // and (b) makes reconciliation mark every prior finding as falsely + // "resolved". Re-throw the underlying SCC error so its actionable message + // (SCC_NOT_ACTIVATED / PERMISSION_DENIED / …) reaches the user. Mirrors the + // AWS all-regions-failed guard in aws-security.service.ts. + if (successfulScopes.size === 0 && failedScopes.size > 0) { + throw ( + firstScopeError ?? + new Error(`All ${failedScopes.size} GCP scope(s) failed to scan`) + ); + } + this.logger.log(`Found ${allFindings.length} GCP security findings`); return allFindings; } From d36d9089bb59d3a6a481a58f02e65642a87fa6df Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 11 Jun 2026 16:39:46 -0400 Subject: [PATCH 2/2] fix(cloud-security): map SCC 404 NOT_FOUND to the actionable SCC_NOT_ACTIVATED error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verified in prod logs: the customer's project-scoped SCC query fails with 404 "Requested entity was not found" — their GCP account has no organization and SCC was never activated for the selected project. Without this mapping the new throw-guard would surface a raw "GCP API error (404)"; with it the user gets the existing SCC_NOT_ACTIVATED flow (400 + persistent inline error with the activation link). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../providers/gcp-security.service.spec.ts | 15 +++++++++++++++ .../providers/gcp-security.service.ts | 12 ++++++++++++ 2 files changed, 27 insertions(+) diff --git a/apps/api/src/cloud-security/providers/gcp-security.service.spec.ts b/apps/api/src/cloud-security/providers/gcp-security.service.spec.ts index 9d79c6452b..d03c50c9a8 100644 --- a/apps/api/src/cloud-security/providers/gcp-security.service.spec.ts +++ b/apps/api/src/cloud-security/providers/gcp-security.service.spec.ts @@ -771,6 +771,21 @@ describe('GCPSecurityService.scanSecurityFindings — all-scopes-failed guard', ).rejects.toThrow(/Permission denied/); }); + // The exact case from the customer report: project-scoped SCC query on a + // no-org account returns 404 NOT_FOUND (SCC never activated for the project). + it('maps a 404 NOT_FOUND to the actionable SCC_NOT_ACTIVATED error', async () => { + fetchMock.mockResolvedValue( + sccError(404, '{"error":{"code":404,"message":"Requested entity was not found.","status":"NOT_FOUND"}}'), + ); + + await expect( + service.scanSecurityFindings( + { access_token: 'tok' }, + { project_ids: ['wasimil-prod'] }, + ), + ).rejects.toThrow(/SCC_NOT_ACTIVATED/); + }); + it('throws when EVERY configured scope errors', async () => { fetchMock.mockResolvedValue(sccError(500, 'internal error')); diff --git a/apps/api/src/cloud-security/providers/gcp-security.service.ts b/apps/api/src/cloud-security/providers/gcp-security.service.ts index 2516395bd9..a034bb6b4d 100644 --- a/apps/api/src/cloud-security/providers/gcp-security.service.ts +++ b/apps/api/src/cloud-security/providers/gcp-security.service.ts @@ -1516,6 +1516,18 @@ export class GCPSecurityService { 'Enable it at https://console.cloud.google.com/security/command-center — the Standard tier is free.', ); } + // SCC v2 returns 404 NOT_FOUND when the scoped resource has no SCC + // activation record — e.g. a project-scoped query on a no-org account + // where SCC Standard was never activated for the project. + if ( + response.status === 404 || + errorText.includes('Requested entity was not found') + ) { + throw new Error( + `SCC_NOT_ACTIVATED: Security Command Center is not activated for ${parent.replace('/', ' ')}. ` + + 'Enable it at https://console.cloud.google.com/security/command-center — the Standard tier is free.', + ); + } if ( errorText.includes('PERMISSION_DENIED') || errorText.includes('403')