From b90775d8944a15d663e96dc4ef0e71d74e454da8 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 21 May 2026 17:08:28 -0400 Subject: [PATCH 1/4] fix(cloud-tests): show folder-nested GCP projects in connection picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Customers with an org→folder→project hierarchy (a common SOC2-friendly layout) reported that their production projects never appeared in our GCP connection picker. Two bugs combined to cause it: 1. detectProjectsForOrg filtered with `parent.id:`, which GCP's v1/projects API interprets as "immediate org children only". A project whose immediate parent is a folder under the org never matches, even if the user has full IAM access to it. 2. Both detectProjects and detectProjectsForOrg called the v1/projects list endpoint with pageSize=50 and never followed nextPageToken. For accounts with many sandboxes / Gemini default projects / etc., the first 50 results consumed the whole page and the production projects on later pages were silently dropped. Fix: - New `listProjectsPaginated(token, filter)` helper that follows nextPageToken until exhaustion, uses pageSize=200, and stops at a 1000-project safety cap. On a mid-pagination error it returns what it has so far — matches the prior "best-effort" picker posture and prevents a single transient 5xx from blanking the dropdown. - detectProjectsForOrg now issues two parallel paginated calls: one for direct org children (existing behavior) and one for any folder-nested project the caller has access to (`parent.type:folder`). Results merged + deduped. GCP's v1 list API has no "descendants-of-org" query, so the user's IAM scope is the effective filter for the folder-nested arm. - detectProjects refactored to use the same paginator. Direct list is paginated; org-scoped fallback is paginated. Backward compatibility: customers whose projects all live directly under an org see no behavior change (the folder-nested call returns 0 for them). Existing connections aren't touched — detection only runs at connect/re-auth. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../providers/gcp-security.service.spec.ts | 280 ++++++++++++++++++ .../providers/gcp-security.service.ts | 206 ++++++++----- 2 files changed, 406 insertions(+), 80 deletions(-) create mode 100644 apps/api/src/cloud-security/providers/gcp-security.service.spec.ts 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 new file mode 100644 index 0000000000..7af1a5aebd --- /dev/null +++ b/apps/api/src/cloud-security/providers/gcp-security.service.spec.ts @@ -0,0 +1,280 @@ +// `gcp-security.service.ts` pulls in heavy SCC/IAM clients via the +// constructor flow at import time. We only need to test the OAuth-fetch +// based project-detection paths, so stub @db (Prisma client) before +// importing the service. +jest.mock('@db', () => ({ db: {} })); + +import { GCPSecurityService } from './gcp-security.service'; + +/** + * Helper: build a Response-like object for a single page of the GCP + * v1/projects list endpoint. Mirrors the exact shape the real API + * returns so the production code path is exercised verbatim. + */ +function gcpPage(opts: { + projects: Array<{ projectId: string; name: string; projectNumber: string }>; + nextPageToken?: string; +}): { ok: true; json: () => Promise } { + return { + ok: true, + json: async () => ({ + projects: opts.projects, + ...(opts.nextPageToken ? { nextPageToken: opts.nextPageToken } : {}), + }), + }; +} + +function makeProject(suffix: string) { + return { + projectId: `proj-${suffix}`, + name: `Project ${suffix}`, + projectNumber: `100${suffix}`, + }; +} + +describe('GCPSecurityService — project detection', () => { + 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; + }); + + // ─── Pagination through nextPageToken ────────────────────────────────── + + describe('listProjectsPaginated (via detectProjects)', () => { + it('returns all projects from a single page when no nextPageToken is set', async () => { + fetchMock.mockResolvedValueOnce( + gcpPage({ projects: [makeProject('1'), makeProject('2')] }), + ); + + const result = await service.detectProjects('token'); + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(result).toEqual([ + { id: 'proj-1', name: 'Project 1', number: '1001' }, + { id: 'proj-2', name: 'Project 2', number: '1002' }, + ]); + }); + + it('follows nextPageToken across multiple pages until exhaustion', async () => { + // Three pages — this is the Greg scenario: a customer with more + // accessible projects than fit in a single page. Pre-fix, only + // the first page came back and the folder-nested production + // projects on later pages were silently dropped. + fetchMock + .mockResolvedValueOnce( + gcpPage({ + projects: [makeProject('1'), makeProject('2')], + nextPageToken: 'page-2-token', + }), + ) + .mockResolvedValueOnce( + gcpPage({ + projects: [makeProject('3'), makeProject('4')], + nextPageToken: 'page-3-token', + }), + ) + .mockResolvedValueOnce( + gcpPage({ + projects: [makeProject('5')], + // no nextPageToken → end of pagination + }), + ); + + const result = await service.detectProjects('token'); + + expect(fetchMock).toHaveBeenCalledTimes(3); + expect(result.map((p) => p.id)).toEqual([ + 'proj-1', + 'proj-2', + 'proj-3', + 'proj-4', + 'proj-5', + ]); + }); + + it('passes pageToken through to subsequent requests', async () => { + fetchMock + .mockResolvedValueOnce( + gcpPage({ + projects: [makeProject('1')], + nextPageToken: 'token-for-page-2', + }), + ) + .mockResolvedValueOnce(gcpPage({ projects: [makeProject('2')] })); + + await service.detectProjects('access-token-xyz'); + + const secondCallUrl = fetchMock.mock.calls[1]?.[0] as string; + expect(secondCallUrl).toContain('pageToken=token-for-page-2'); + }); + + it('returns the projects collected so far when a mid-pagination page fails', async () => { + // Mid-pagination 500 from GCP — we keep what we got rather than + // throwing and blanking the picker. Matches the prior failure + // posture of "best-effort results". + fetchMock + .mockResolvedValueOnce( + gcpPage({ + projects: [makeProject('1'), makeProject('2')], + nextPageToken: 'page-2', + }), + ) + .mockResolvedValueOnce({ + ok: false, + status: 503, + text: async () => 'Service Unavailable', + }); + + const result = await service.detectProjects('token'); + + expect(result.map((p) => p.id)).toEqual(['proj-1', 'proj-2']); + // The fallback path doesn't fire because direct returned ≥1. + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it('stops paginating at the safety cap (1000 projects) even if more pages remain', async () => { + // Five 200-project pages → exactly 1000 → cap hit. + const bigPage = (start: number, count: number, more: boolean) => + gcpPage({ + projects: Array.from({ length: count }, (_, i) => + makeProject(String(start + i)), + ), + ...(more ? { nextPageToken: `next-${start + count}` } : {}), + }); + fetchMock + .mockResolvedValueOnce(bigPage(1, 200, true)) + .mockResolvedValueOnce(bigPage(201, 200, true)) + .mockResolvedValueOnce(bigPage(401, 200, true)) + .mockResolvedValueOnce(bigPage(601, 200, true)) + .mockResolvedValueOnce(bigPage(801, 200, true)) + // Sixth page never requested because cap hit. + .mockResolvedValueOnce(bigPage(1001, 200, false)); + + const result = await service.detectProjects('token'); + + expect(result).toHaveLength(1000); + expect(fetchMock).toHaveBeenCalledTimes(5); + }); + }); + + // ─── detectProjectsForOrg: org direct + folder-nested merge ─────────── + + describe('detectProjectsForOrg', () => { + it('returns the union of direct org children and folder-nested projects', async () => { + // Two parallel calls expected: + // 1. parent.id:43356919874 → direct children + // 2. parent.type:folder → folder-nested + // The fix: previously only call (1) ran, so projects under a + // folder ("propperai-prod", "propperai-demo" in Greg's report) + // never appeared. After the fix, both run and merge. + fetchMock.mockImplementation(async (url: string) => { + if (url.includes('parent.id%3A43356919874')) { + // Direct children of the org. + return gcpPage({ + projects: [ + { + projectId: 'org-root-1', + name: 'Root Project', + projectNumber: '111', + }, + ], + }); + } + if (url.includes('parent.type%3Afolder')) { + // Folder-nested production projects. + return gcpPage({ + projects: [ + { + projectId: 'propperai-prod', + name: 'Propper Prod', + projectNumber: '222', + }, + { + projectId: 'propperai-demo', + name: 'Propper Demo', + projectNumber: '333', + }, + ], + }); + } + throw new Error(`Unexpected fetch URL in test: ${url}`); + }); + + const result = await service.detectProjectsForOrg( + 'token', + '43356919874', + ); + + const ids = result.map((p) => p.id).sort(); + expect(ids).toEqual(['org-root-1', 'propperai-demo', 'propperai-prod']); + }); + + it('dedupes when the same project appears in both direct and folder lists', async () => { + // A project legitimately matched by both filters (unusual but + // possible if GCP's filter semantics overlap in some edge) must + // not be returned twice. + fetchMock.mockImplementation(async (url: string) => { + if (url.includes('parent.id%3A123')) { + return gcpPage({ projects: [makeProject('shared')] }); + } + if (url.includes('parent.type%3Afolder')) { + return gcpPage({ + projects: [makeProject('shared'), makeProject('unique')], + }); + } + throw new Error(`Unexpected URL: ${url}`); + }); + + const result = await service.detectProjectsForOrg('token', '123'); + + expect(result.map((p) => p.id)).toEqual(['proj-shared', 'proj-unique']); + }); + + it('fires the two list calls in parallel', async () => { + const seenUrls: string[] = []; + let resolveCount = 0; + const resolvers: Array<() => void> = []; + + fetchMock.mockImplementation((url: string) => { + seenUrls.push(url); + return new Promise((resolve) => { + resolvers.push(() => resolve(gcpPage({ projects: [] }))); + }); + }); + + const pending = service.detectProjectsForOrg('token', '999'); + + // Yield a microtask so the Promise.all schedules both fetches. + await Promise.resolve(); + await Promise.resolve(); + + expect(seenUrls).toHaveLength(2); + expect(seenUrls.some((u) => u.includes('parent.id%3A999'))).toBe(true); + expect(seenUrls.some((u) => u.includes('parent.type%3Afolder'))).toBe( + true, + ); + + // Resolve both pending fetches so detectProjectsForOrg can complete. + resolvers.forEach((r) => r()); + resolveCount = resolvers.length; + await pending; + expect(resolveCount).toBe(2); + }); + + it('returns empty array when both calls return no projects', async () => { + fetchMock.mockResolvedValue(gcpPage({ projects: [] })); + const result = await service.detectProjectsForOrg('token', 'org-no-projects'); + expect(result).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 79f1a844f0..d0b30c44ff 100644 --- a/apps/api/src/cloud-security/providers/gcp-security.service.ts +++ b/apps/api/src/cloud-security/providers/gcp-security.service.ts @@ -803,45 +803,45 @@ export class GCPSecurityService { /** * Detect active GCP projects scoped to a specific organization. - * Returns only projects whose parent is the given org ID. + * Returns: + * - projects whose IMMEDIATE parent is the given org ID, AND + * - projects nested inside any folder the user has access to. + * + * Background: GCP's `v1/projects` list endpoint does not support a + * "descendants of org" query — `parent.id:` only matches direct + * children, so customers whose production projects live under a + * folder (org → folder → project, a common SOC2-friendly layout) + * never see those projects in our picker. We compensate by also + * listing every accessible folder-nested project; the user's IAM + * scope already limits what they can see. */ async detectProjectsForOrg( accessToken: string, organizationId: string, ): Promise> { - const params = new URLSearchParams({ - pageSize: '50', - filter: `lifecycleState:ACTIVE AND parent.id:${organizationId}`, - }); - const response = await fetch( - `https://cloudresourcemanager.googleapis.com/v1/projects?${params.toString()}`, - { - headers: { - Authorization: `Bearer ${accessToken}`, - 'Content-Type': 'application/json', - }, - }, - ); + const [directChildren, folderNested] = await Promise.all([ + this.listProjectsPaginated( + accessToken, + `lifecycleState:ACTIVE AND parent.id:${organizationId}`, + ), + this.listProjectsPaginated( + accessToken, + 'lifecycleState:ACTIVE AND parent.type:folder', + ), + ]); - if (!response.ok) { - this.logger.warn( - `Failed to list GCP projects for org ${organizationId}: ${await response.text()}`, - ); - return []; + const seen = new Set(); + const merged: Array<{ id: string; name: string; number: string }> = []; + for (const p of [...directChildren, ...folderNested]) { + if (seen.has(p.id)) continue; + seen.add(p.id); + merged.push(p); } - const data = await response.json(); - return ( - (data.projects ?? []) as Array<{ - projectId: string; - name: string; - projectNumber: string; - }> - ).map((p) => ({ - id: p.projectId, - name: p.name, - number: p.projectNumber, - })); + this.logger.log( + `GCP detectProjectsForOrg(${organizationId}): ${directChildren.length} direct + ${folderNested.length} folder-nested → ${merged.length} unique`, + ); + return merged; } /** @@ -852,54 +852,10 @@ export class GCPSecurityService { async detectProjects( accessToken: string, ): Promise> { - const mapRow = (p: { - projectId: string; - name: string; - projectNumber: string; - }) => ({ - id: p.projectId, - name: p.name, - number: p.projectNumber, - }); - - const listProjectsWithFilter = async ( - filter: string, - ): Promise< - Array<{ id: string; name: string; number: string }> - > => { - const params = new URLSearchParams({ - pageSize: '50', - filter, - }); - const response = await fetch( - `https://cloudresourcemanager.googleapis.com/v1/projects?${params.toString()}`, - { - headers: { - Authorization: `Bearer ${accessToken}`, - 'Content-Type': 'application/json', - }, - }, - ); - - if (!response.ok) { - const errorText = await response.text(); - this.logger.warn( - `Failed to list GCP projects (filter=${filter}): ${errorText}`, - ); - return []; - } - - const data = await response.json(); - return ( - (data.projects ?? []) as Array<{ - projectId: string; - name: string; - projectNumber: string; - }> - ).map(mapRow); - }; - - const direct = await listProjectsWithFilter('lifecycleState:ACTIVE'); + const direct = await this.listProjectsPaginated( + accessToken, + 'lifecycleState:ACTIVE', + ); if (direct.length > 0) { this.logger.log( `GCP detectProjects: ${direct.length} project(s) via direct list`, @@ -919,7 +875,8 @@ export class GCPSecurityService { const merged: Array<{ id: string; name: string; number: string }> = []; for (const org of orgs) { - const underOrg = await listProjectsWithFilter( + const underOrg = await this.listProjectsPaginated( + accessToken, `lifecycleState:ACTIVE AND parent.id:${org.id}`, ); for (const p of underOrg) { @@ -945,6 +902,95 @@ export class GCPSecurityService { return merged; } + /** + * Paginated wrapper around GCP's `cloudresourcemanager.projects.list`. + * + * The v1 list endpoint paginates via `nextPageToken`. The previous + * implementation requested a single page with `pageSize=50` and never + * followed `nextPageToken`, which silently truncated the result for + * any customer with more than ~50 accessible projects (large orgs, + * accounts with many sandboxes/Gemini default projects, etc.) and + * caused critical production projects to be missing from our picker. + * + * Behavior: + * - Follows `nextPageToken` until exhaustion. + * - Uses `pageSize=200` (well under GCP's 500 max) to keep + * round-trips low. + * - Stops at `SAFE_MAX_PROJECTS=1000` to bound API usage; if a + * customer legitimately has more accessible projects, they + * should narrow with a filter rather than load all of them in + * the picker. + * - On non-OK response from any page, logs and returns what was + * collected so far instead of throwing — matches the prior + * failure mode (UI gets best-effort results) and prevents one + * transient page error from blanking the whole picker. + */ + private async listProjectsPaginated( + accessToken: string, + filter: string, + ): Promise> { + const PAGE_SIZE = 200; + const SAFE_MAX_PROJECTS = 1000; + + const collected: Array<{ id: string; name: string; number: string }> = []; + let pageToken: string | undefined; + let pages = 0; + + do { + const params = new URLSearchParams({ + pageSize: String(PAGE_SIZE), + filter, + }); + if (pageToken) params.set('pageToken', pageToken); + + const response = await fetch( + `https://cloudresourcemanager.googleapis.com/v1/projects?${params.toString()}`, + { + headers: { + Authorization: `Bearer ${accessToken}`, + 'Content-Type': 'application/json', + }, + }, + ); + + if (!response.ok) { + this.logger.warn( + `Failed to list GCP projects (filter="${filter}", page=${pages + 1}, collected=${collected.length}): ${await response.text()}`, + ); + return collected; + } + + const data = (await response.json()) as { + projects?: Array<{ + projectId: string; + name: string; + projectNumber: string; + }>; + nextPageToken?: string; + }; + + for (const p of data.projects ?? []) { + collected.push({ + id: p.projectId, + name: p.name, + number: p.projectNumber, + }); + } + + pageToken = data.nextPageToken; + pages++; + + if (collected.length >= SAFE_MAX_PROJECTS) { + this.logger.warn( + `GCP projects: hit safety cap of ${SAFE_MAX_PROJECTS} for filter="${filter}" — consider a narrower filter if more results are needed`, + ); + break; + } + } while (pageToken); + + return collected; + } + /** * Detect which GCP services the customer actually uses by querying * the Service Usage API for each project. Maps GCP API names to From 22a441f9e9c2f6a8e998e9b371cacc8af97d7d8b Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 21 May 2026 17:22:21 -0400 Subject: [PATCH 2/4] fix(cloud-tests): isolate folder arm failures from direct arm in GCP picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switches `detectProjectsForOrg` from `Promise.all` → `Promise.allSettled` so a transient failure on the new folder-nested arm (e.g., GCP rejects the `parent.type:folder`-alone filter, DNS blip, transient 5xx during pagination) cannot blank the entire picker. The direct arm matches the prior production code path, so as long as it succeeds we are guaranteed to be no worse than today's prod even if the folder arm fails outright. Two new tests lock in the isolation: - direct arm succeeds, folder arm throws → returns direct results - folder arm succeeds, direct arm throws → returns folder results Co-Authored-By: Claude Opus 4.7 (1M context) --- .../providers/gcp-security.service.spec.ts | 58 +++++++++++++++++++ .../providers/gcp-security.service.ts | 22 ++++++- 2 files changed, 79 insertions(+), 1 deletion(-) 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 7af1a5aebd..cc6985c9fa 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 @@ -276,5 +276,63 @@ describe('GCPSecurityService — project detection', () => { const result = await service.detectProjectsForOrg('token', 'org-no-projects'); expect(result).toEqual([]); }); + + it('still returns direct-arm projects when the folder arm throws (no-regression guarantee)', async () => { + // Hard guarantee: even if GCP rejects the parent.type:folder query + // outright or the connection drops mid-call, the picker must + // remain at least as functional as production. This locks in the + // promise-allSettled isolation. + fetchMock.mockImplementation(async (url: string) => { + if (url.includes('parent.id%3A555')) { + return gcpPage({ + projects: [ + { + projectId: 'direct-only', + name: 'Direct Only', + projectNumber: '777', + }, + ], + }); + } + if (url.includes('parent.type%3Afolder')) { + // Simulate fetch throwing — e.g., DNS failure, TLS error, + // GCP returning an invalid response that breaks .json(). + throw new Error('simulated network failure on folder arm'); + } + throw new Error(`Unexpected URL: ${url}`); + }); + + const result = await service.detectProjectsForOrg('token', '555'); + + expect(result).toEqual([ + { id: 'direct-only', name: 'Direct Only', number: '777' }, + ]); + }); + + it('still returns folder-arm projects when the direct arm throws', async () => { + fetchMock.mockImplementation(async (url: string) => { + if (url.includes('parent.id%3A666')) { + throw new Error('simulated failure on direct arm'); + } + if (url.includes('parent.type%3Afolder')) { + return gcpPage({ + projects: [ + { + projectId: 'folder-only', + name: 'Folder Only', + projectNumber: '888', + }, + ], + }); + } + throw new Error(`Unexpected URL: ${url}`); + }); + + const result = await service.detectProjectsForOrg('token', '666'); + + expect(result).toEqual([ + { id: 'folder-only', name: 'Folder Only', number: '888' }, + ]); + }); }); }); 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 d0b30c44ff..210e82a7c9 100644 --- a/apps/api/src/cloud-security/providers/gcp-security.service.ts +++ b/apps/api/src/cloud-security/providers/gcp-security.service.ts @@ -819,7 +819,11 @@ export class GCPSecurityService { accessToken: string, organizationId: string, ): Promise> { - const [directChildren, folderNested] = await Promise.all([ + // Promise.allSettled (not Promise.all) so a transient failure on + // the folder-nested arm cannot blank the entire picker. The direct + // arm matches the prior production behavior — if THAT works, we + // are at minimum no worse than today's prod. + const [directResult, folderResult] = await Promise.allSettled([ this.listProjectsPaginated( accessToken, `lifecycleState:ACTIVE AND parent.id:${organizationId}`, @@ -830,6 +834,22 @@ export class GCPSecurityService { ), ]); + const directChildren = + directResult.status === 'fulfilled' ? directResult.value : []; + const folderNested = + folderResult.status === 'fulfilled' ? folderResult.value : []; + + if (directResult.status === 'rejected') { + this.logger.warn( + `GCP detectProjectsForOrg(${organizationId}): direct arm threw — ${directResult.reason}`, + ); + } + if (folderResult.status === 'rejected') { + this.logger.warn( + `GCP detectProjectsForOrg(${organizationId}): folder arm threw — ${folderResult.reason}`, + ); + } + const seen = new Set(); const merged: Array<{ id: string; name: string; number: string }> = []; for (const p of [...directChildren, ...folderNested]) { From 39d233fb810dcf09bdd024c6cf14eac8e6090de3 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 21 May 2026 17:28:09 -0400 Subject: [PATCH 3/4] fix(cloud-tests): properly scope folder-nested GCP projects to the target org MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses cubic P2 on PR #2899: the previous folder arm filter (`parent.type:folder` alone) would have returned projects from ANY org the caller had access to, violating the `detectProjectsForOrg` contract for multi-org users. The folder arm now recursively enumerates folders under the target org via `v2/folders?parent=organizations/` (and recursively under each child folder), then queries each discovered folder with `parent.type:folder AND parent.id:` — the paired filter shape GCP explicitly documents for by-parent project queries and which triggers their alternate consistent index. Behaviors preserved: - Promise.allSettled isolation so a failure on either arm cannot blank the picker. - Per-folder query failures are isolated (one bad folder doesn't kill the rest). - Safety cap of 500 folders to bound API usage. New tests: - Greg's exact scenario: only this org's folder gets queried. - Recursive sub-folder traversal (org → folder → sub-folder → projects). - Dedupe across arms. - Folder arm failure → direct arm still works. - Direct arm failure → folder arm still works. - Per-folder failure isolation. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../providers/gcp-security.service.spec.ts | 253 +++++++++++------- .../providers/gcp-security.service.ts | 152 ++++++++++- 2 files changed, 298 insertions(+), 107 deletions(-) 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 cc6985c9fa..3b8c9def20 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 @@ -167,67 +167,130 @@ describe('GCPSecurityService — project detection', () => { }); }); - // ─── detectProjectsForOrg: org direct + folder-nested merge ─────────── + // ─── detectProjectsForOrg: org direct + folder-tree projects ────────── + + /** + * Build a Response-like for the v2/folders endpoint. + */ + function foldersPage(opts: { + folders: string[]; // folder IDs (numeric) + nextPageToken?: string; + }): { ok: true; json: () => Promise } { + return { + ok: true, + json: async () => ({ + folders: opts.folders.map((id) => ({ name: `folders/${id}` })), + ...(opts.nextPageToken ? { nextPageToken: opts.nextPageToken } : {}), + }), + }; + } describe('detectProjectsForOrg', () => { - it('returns the union of direct org children and folder-nested projects', async () => { - // Two parallel calls expected: - // 1. parent.id:43356919874 → direct children - // 2. parent.type:folder → folder-nested - // The fix: previously only call (1) ran, so projects under a - // folder ("propperai-prod", "propperai-demo" in Greg's report) - // never appeared. After the fix, both run and merge. + it("only queries projects whose parent is this org's direct children OR a folder inside this org's tree (cubic P2)", async () => { + // Greg's exact scenario: + // org 43356919874 (propper.ai) + // ├── folder 9724350536 (propper) + // │ ├── propperai-prod + // │ └── propperai-demo + // └── org-root-1 (direct org child) + // + // The folder-nested arm previously used `parent.type:folder` + // alone, which would have ALSO returned projects under folders + // in OTHER orgs the caller had access to. The fix enumerates + // folders under THIS org and queries each by ID, matching GCP's + // documented happy-path filter shape and properly scoping the + // result. + const seenFolderIdsQueried: string[] = []; fetchMock.mockImplementation(async (url: string) => { - if (url.includes('parent.id%3A43356919874')) { - // Direct children of the org. - return gcpPage({ - projects: [ - { - projectId: 'org-root-1', - name: 'Root Project', - projectNumber: '111', - }, - ], - }); + // Folder enumeration: top-level folders under the org. + if (url.includes('v2/folders') && url.includes('organizations%2F43356919874')) { + return foldersPage({ folders: ['9724350536'] }); } - if (url.includes('parent.type%3Afolder')) { - // Folder-nested production projects. + // Folder enumeration: sub-folders (none in this tree). + if (url.includes('v2/folders') && url.includes('folders%2F9724350536')) { + return foldersPage({ folders: [] }); + } + // Direct org children projects. + if (url.includes('v1/projects') && url.includes('parent.id%3A43356919874')) { return gcpPage({ projects: [ - { - projectId: 'propperai-prod', - name: 'Propper Prod', - projectNumber: '222', - }, - { - projectId: 'propperai-demo', - name: 'Propper Demo', - projectNumber: '333', - }, + { projectId: 'org-root-1', name: 'Root Project', projectNumber: '111' }, ], }); } + // Per-folder project queries — extract folder ID from filter. + if (url.includes('v1/projects') && url.includes('parent.type%3Afolder')) { + const m = url.match(/parent\.id%3A(\d+)/); + if (m) seenFolderIdsQueried.push(m[1]); + if (m && m[1] === '9724350536') { + return gcpPage({ + projects: [ + { projectId: 'propperai-prod', name: 'Propper Prod', projectNumber: '222' }, + { projectId: 'propperai-demo', name: 'Propper Demo', projectNumber: '333' }, + ], + }); + } + return gcpPage({ projects: [] }); + } throw new Error(`Unexpected fetch URL in test: ${url}`); }); - const result = await service.detectProjectsForOrg( - 'token', - '43356919874', - ); + const result = await service.detectProjectsForOrg('token', '43356919874'); const ids = result.map((p) => p.id).sort(); expect(ids).toEqual(['org-root-1', 'propperai-demo', 'propperai-prod']); + // ONLY this org's folder was queried — cubic P2 fix. + expect(seenFolderIdsQueried).toEqual(['9724350536']); + }); + + it('recursively traverses nested folders (org → folder → sub-folder → projects)', async () => { + // Layout: + // org 1000 + // └── folder 2000 (top) + // └── folder 3000 (nested) + // └── project deep-prod + fetchMock.mockImplementation(async (url: string) => { + if (url.includes('v2/folders') && url.includes('organizations%2F1000')) { + return foldersPage({ folders: ['2000'] }); + } + if (url.includes('v2/folders') && url.includes('folders%2F2000')) { + return foldersPage({ folders: ['3000'] }); + } + if (url.includes('v2/folders') && url.includes('folders%2F3000')) { + return foldersPage({ folders: [] }); + } + if (url.includes('v1/projects') && url.includes('parent.id%3A1000')) { + return gcpPage({ projects: [] }); // no direct org children + } + if (url.includes('v1/projects') && url.includes('parent.id%3A3000')) { + return gcpPage({ + projects: [ + { projectId: 'deep-prod', name: 'Deep', projectNumber: '999' }, + ], + }); + } + if (url.includes('v1/projects') && url.includes('parent.id%3A2000')) { + return gcpPage({ projects: [] }); + } + throw new Error(`Unexpected URL: ${url}`); + }); + + const result = await service.detectProjectsForOrg('token', '1000'); + expect(result.map((p) => p.id)).toEqual(['deep-prod']); }); - it('dedupes when the same project appears in both direct and folder lists', async () => { - // A project legitimately matched by both filters (unusual but - // possible if GCP's filter semantics overlap in some edge) must - // not be returned twice. + it('dedupes when the same project would appear in both arms', async () => { fetchMock.mockImplementation(async (url: string) => { - if (url.includes('parent.id%3A123')) { + if (url.includes('v2/folders') && url.includes('organizations%2F123')) { + return foldersPage({ folders: ['folder-a'] }); + } + if (url.includes('v2/folders') && url.includes('folders%2Ffolder-a')) { + return foldersPage({ folders: [] }); + } + if (url.includes('v1/projects') && url.includes('parent.id%3A123')) { return gcpPage({ projects: [makeProject('shared')] }); } - if (url.includes('parent.type%3Afolder')) { + if (url.includes('v1/projects') && url.includes('parent.id%3Afolder-a')) { return gcpPage({ projects: [makeProject('shared'), makeProject('unique')], }); @@ -236,74 +299,39 @@ describe('GCPSecurityService — project detection', () => { }); const result = await service.detectProjectsForOrg('token', '123'); - expect(result.map((p) => p.id)).toEqual(['proj-shared', 'proj-unique']); }); - it('fires the two list calls in parallel', async () => { - const seenUrls: string[] = []; - let resolveCount = 0; - const resolvers: Array<() => void> = []; - - fetchMock.mockImplementation((url: string) => { - seenUrls.push(url); - return new Promise((resolve) => { - resolvers.push(() => resolve(gcpPage({ projects: [] }))); - }); + it('returns empty array when the org has no direct projects and no folders', async () => { + fetchMock.mockImplementation(async (url: string) => { + if (url.includes('v2/folders')) return foldersPage({ folders: [] }); + if (url.includes('v1/projects')) return gcpPage({ projects: [] }); + throw new Error(`Unexpected URL: ${url}`); }); - const pending = service.detectProjectsForOrg('token', '999'); - - // Yield a microtask so the Promise.all schedules both fetches. - await Promise.resolve(); - await Promise.resolve(); - - expect(seenUrls).toHaveLength(2); - expect(seenUrls.some((u) => u.includes('parent.id%3A999'))).toBe(true); - expect(seenUrls.some((u) => u.includes('parent.type%3Afolder'))).toBe( - true, - ); - - // Resolve both pending fetches so detectProjectsForOrg can complete. - resolvers.forEach((r) => r()); - resolveCount = resolvers.length; - await pending; - expect(resolveCount).toBe(2); - }); - - it('returns empty array when both calls return no projects', async () => { - fetchMock.mockResolvedValue(gcpPage({ projects: [] })); - const result = await service.detectProjectsForOrg('token', 'org-no-projects'); + const result = await service.detectProjectsForOrg('token', 'empty-org'); expect(result).toEqual([]); }); - it('still returns direct-arm projects when the folder arm throws (no-regression guarantee)', async () => { - // Hard guarantee: even if GCP rejects the parent.type:folder query - // outright or the connection drops mid-call, the picker must - // remain at least as functional as production. This locks in the - // promise-allSettled isolation. + it('still returns direct-arm projects when the folder arm throws entirely (no-regression guarantee)', async () => { + // If GCP's v2/folders endpoint throws or returns 4xx, the folder + // arm collapses to [] gracefully — the direct arm still works + // and we are at minimum no worse than prod. fetchMock.mockImplementation(async (url: string) => { + if (url.includes('v2/folders')) { + throw new Error('simulated v2/folders network failure'); + } if (url.includes('parent.id%3A555')) { return gcpPage({ projects: [ - { - projectId: 'direct-only', - name: 'Direct Only', - projectNumber: '777', - }, + { projectId: 'direct-only', name: 'Direct Only', projectNumber: '777' }, ], }); } - if (url.includes('parent.type%3Afolder')) { - // Simulate fetch throwing — e.g., DNS failure, TLS error, - // GCP returning an invalid response that breaks .json(). - throw new Error('simulated network failure on folder arm'); - } throw new Error(`Unexpected URL: ${url}`); }); const result = await service.detectProjectsForOrg('token', '555'); - expect(result).toEqual([ { id: 'direct-only', name: 'Direct Only', number: '777' }, ]); @@ -311,17 +339,17 @@ describe('GCPSecurityService — project detection', () => { it('still returns folder-arm projects when the direct arm throws', async () => { fetchMock.mockImplementation(async (url: string) => { - if (url.includes('parent.id%3A666')) { - throw new Error('simulated failure on direct arm'); + if (url.includes('v1/projects') && url.includes('parent.id%3A666') && !url.includes('parent.type%3Afolder')) { + throw new Error('simulated direct-arm failure'); } - if (url.includes('parent.type%3Afolder')) { + if (url.includes('v2/folders') && url.includes('organizations%2F666')) { + return foldersPage({ folders: ['folder-x'] }); + } + if (url.includes('v2/folders')) return foldersPage({ folders: [] }); + if (url.includes('v1/projects') && url.includes('parent.id%3Afolder-x')) { return gcpPage({ projects: [ - { - projectId: 'folder-only', - name: 'Folder Only', - projectNumber: '888', - }, + { projectId: 'folder-only', name: 'Folder Only', projectNumber: '888' }, ], }); } @@ -329,10 +357,37 @@ describe('GCPSecurityService — project detection', () => { }); const result = await service.detectProjectsForOrg('token', '666'); - expect(result).toEqual([ { id: 'folder-only', name: 'Folder Only', number: '888' }, ]); }); + + it('isolates per-folder query failures so one bad folder does not blank the rest', async () => { + // Two folders, the first one's project list throws. The second + // one should still return its projects. + fetchMock.mockImplementation(async (url: string) => { + if (url.includes('v2/folders') && url.includes('organizations%2F777')) { + return foldersPage({ folders: ['bad-folder', 'good-folder'] }); + } + if (url.includes('v2/folders')) return foldersPage({ folders: [] }); + if (url.includes('v1/projects') && url.includes('parent.id%3A777') && !url.includes('parent.type%3Afolder')) { + return gcpPage({ projects: [] }); + } + if (url.includes('v1/projects') && url.includes('parent.id%3Abad-folder')) { + throw new Error('bad folder query exploded'); + } + if (url.includes('v1/projects') && url.includes('parent.id%3Agood-folder')) { + return gcpPage({ + projects: [ + { projectId: 'good-proj', name: 'Good', projectNumber: '101' }, + ], + }); + } + throw new Error(`Unexpected URL: ${url}`); + }); + + const result = await service.detectProjectsForOrg('token', '777'); + expect(result.map((p) => p.id)).toEqual(['good-proj']); + }); }); }); 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 210e82a7c9..d9b40a37af 100644 --- a/apps/api/src/cloud-security/providers/gcp-security.service.ts +++ b/apps/api/src/cloud-security/providers/gcp-security.service.ts @@ -819,19 +819,24 @@ export class GCPSecurityService { accessToken: string, organizationId: string, ): Promise> { - // Promise.allSettled (not Promise.all) so a transient failure on - // the folder-nested arm cannot blank the entire picker. The direct - // arm matches the prior production behavior — if THAT works, we - // are at minimum no worse than today's prod. + // Two arms, isolated via Promise.allSettled so a failure on one + // cannot blank the picker: + // 1. Direct org children: existing behavior. + // 2. Folder-nested projects, scoped to THIS org's folder tree: + // recursively enumerate folders under the org, then query + // each with `parent.type:folder AND parent.id:`, + // which is GCP's documented happy path (uses the alternate + // consistent search index). + // + // The folder arm is properly org-scoped — a user with access to + // projects in OTHER organizations will not see those projects + // here. This honors the "ForOrg" contract. const [directResult, folderResult] = await Promise.allSettled([ this.listProjectsPaginated( accessToken, `lifecycleState:ACTIVE AND parent.id:${organizationId}`, ), - this.listProjectsPaginated( - accessToken, - 'lifecycleState:ACTIVE AND parent.type:folder', - ), + this.listProjectsInOrgFolderTree(accessToken, organizationId), ]); const directChildren = @@ -864,6 +869,137 @@ export class GCPSecurityService { return merged; } + /** + * Recursively enumerate all folders under an org, then list projects + * inside each folder. The per-folder project query uses the paired + * `parent.type:folder AND parent.id:` filter, which is the + * shape GCP explicitly documents for by-parent project queries + * ("the filter must contain both a parent.type and a parent.id + * restriction") and which triggers their alternate consistent index. + * + * Per-folder failures are isolated: if one folder's project list + * fails (transient 5xx, permission edge case), the rest still + * succeed. + */ + private async listProjectsInOrgFolderTree( + accessToken: string, + organizationId: string, + ): Promise> { + const folderIds = await this.listFoldersUnderOrg( + accessToken, + organizationId, + ); + if (folderIds.length === 0) return []; + + const perFolder = await Promise.all( + folderIds.map((folderId) => + this.listProjectsPaginated( + accessToken, + `lifecycleState:ACTIVE AND parent.type:folder AND parent.id:${folderId}`, + ).catch((err) => { + this.logger.warn( + `GCP folder ${folderId}: project list failed — ${err instanceof Error ? err.message : String(err)}`, + ); + return []; + }), + ), + ); + + return perFolder.flat(); + } + + /** + * Breadth-first walk of the folder tree under an organization. + * Returns every folder ID the caller has visibility into, no matter + * how deeply nested. Bounded by SAFE_MAX_FOLDERS to keep API usage + * predictable in pathological cases. + */ + private async listFoldersUnderOrg( + accessToken: string, + organizationId: string, + ): Promise { + const SAFE_MAX_FOLDERS = 500; + + const collected: string[] = []; + const seenParents = new Set(); + const queue: string[] = [`organizations/${organizationId}`]; + + while (queue.length > 0 && collected.length < SAFE_MAX_FOLDERS) { + const parent = queue.shift(); + if (!parent || seenParents.has(parent)) continue; + seenParents.add(parent); + + const children = await this.listChildFolders(accessToken, parent); + for (const folderId of children) { + if (collected.includes(folderId)) continue; + collected.push(folderId); + queue.push(`folders/${folderId}`); + if (collected.length >= SAFE_MAX_FOLDERS) { + this.logger.warn( + `GCP folder enumeration: hit safety cap of ${SAFE_MAX_FOLDERS} folders for org ${organizationId}`, + ); + break; + } + } + } + + return collected; + } + + /** + * One paginated call to `v2/folders?parent=` returning the + * immediate child folder IDs (stripped of the "folders/" prefix). + * Errors are non-fatal — log and return what we collected so far so + * one bad page doesn't kill the whole tree walk. + */ + private async listChildFolders( + accessToken: string, + parent: string, + ): Promise { + const PAGE_SIZE = 100; + const collected: string[] = []; + let pageToken: string | undefined; + + do { + const params = new URLSearchParams({ + parent, + pageSize: String(PAGE_SIZE), + }); + if (pageToken) params.set('pageToken', pageToken); + + const response = await fetch( + `https://cloudresourcemanager.googleapis.com/v2/folders?${params.toString()}`, + { + headers: { + Authorization: `Bearer ${accessToken}`, + 'Content-Type': 'application/json', + }, + }, + ); + + if (!response.ok) { + this.logger.warn( + `Failed to list child folders of ${parent}: ${await response.text()}`, + ); + return collected; + } + + const data = (await response.json()) as { + folders?: Array<{ name: string }>; + nextPageToken?: string; + }; + + for (const f of data.folders ?? []) { + // f.name has shape "folders/123456". + const id = f.name.replace(/^folders\//, ''); + collected.push(id); + } + pageToken = data.nextPageToken; + } while (pageToken); + + return collected; + } + /** * Auto-detect active GCP projects accessible by the OAuth token. * Tries a direct project list first; if empty (common for org-centric accounts), From d333f59bab11494009e8c1f78f6489a26ed6ea76 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 21 May 2026 17:36:28 -0400 Subject: [PATCH 4/4] =?UTF-8?q?fix(cloud-tests):=20cap=20concurrency=20for?= =?UTF-8?q?=20GCP=20folder=E2=86=92projects=20fan-out?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses cubic P2 on PR #2899: the previous `Promise.all` over every folder ID fires N simultaneous requests to cloudresourcemanager. For tenants with many folders, that can trip GCP's per-user read-quota, and because `listProjectsPaginated` returns the projects-collected-so- far on a non-OK response (including 429 Too Many Requests), a throttled folder query LOOKS like an empty folder — silently truncating the picker with no visible error. The fix bounds concurrent in-flight folder queries to 5 via a small inlined `mapWithConcurrency` helper. GCP's read quota (~600 req/min/user on cloudresourcemanager) is well above this, so the cap stays comfortably below throttling thresholds even for very deep folder hierarchies. Test added that builds a 20-folder tenant, holds the per-folder project queries open, and verifies: - peak in-flight count never exceeds 5 - all 20 folder queries eventually run (cap doesn't truncate work) Co-Authored-By: Claude Opus 4.7 (1M context) --- .../providers/gcp-security.service.spec.ts | 87 +++++++++++++++++++ .../providers/gcp-security.service.ts | 53 ++++++++++- 2 files changed, 137 insertions(+), 3 deletions(-) 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 3b8c9def20..fc8627ec40 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 @@ -362,6 +362,93 @@ describe('GCPSecurityService — project detection', () => { ]); }); + it('caps concurrent folder→projects queries to avoid GCP throttling (cubic P2)', async () => { + // Pathological tenant: 20 folders under the org. Without a + // concurrency cap, all 20 project-list calls would fire at once, + // and any 429s would be silently treated as "empty folder", + // dropping projects from the picker. + const FOLDER_COUNT = 20; + const folderIds = Array.from( + { length: FOLDER_COUNT }, + (_, i) => `f${i}`, + ); + + let inFlight = 0; + let maxInFlight = 0; + const pending: Array<() => void> = []; + + fetchMock.mockImplementation((url: string) => { + if ( + url.includes('v2/folders') && + url.includes('organizations%2Fbig-org') + ) { + return Promise.resolve(foldersPage({ folders: folderIds })); + } + if (url.includes('v2/folders')) { + return Promise.resolve(foldersPage({ folders: [] })); // no sub-folders + } + if ( + url.includes('v1/projects') && + url.includes('parent.id%3Abig-org') && + !url.includes('parent.type%3Afolder') + ) { + return Promise.resolve(gcpPage({ projects: [] })); // direct arm + } + if ( + url.includes('v1/projects') && + url.includes('parent.type%3Afolder') + ) { + // Per-folder project queries — hold the response so we can + // observe concurrency. + inFlight++; + maxInFlight = Math.max(maxInFlight, inFlight); + return new Promise((resolve) => { + pending.push(() => { + inFlight--; + resolve(gcpPage({ projects: [] })); + }); + }); + } + throw new Error(`Unexpected URL: ${url}`); + }); + + const promise = service.detectProjectsForOrg('token', 'big-org'); + + // Wait for the concurrency cap to be reached. setTimeout(0) drains + // the full microtask queue and lets pending I/O settle, which is + // necessary because the folder enumeration does 21 sequential + // fetches (1 org-level + 20 per-folder) before per-folder project + // queries can begin. + const start = Date.now(); + while (Date.now() - start < 2000 && inFlight < 5) { + await new Promise((r) => setTimeout(r, 5)); + } + + // The cap is 5 — observed peak must not exceed it. + expect(maxInFlight).toBeGreaterThan(0); + expect(maxInFlight).toBeLessThanOrEqual(5); + + // Drain — release pending project queries one at a time so the + // remaining folder workers can pick up the next IDs. + while (pending.length > 0 || inFlight > 0) { + const resolver = pending.shift(); + if (resolver) resolver(); + await new Promise((r) => setTimeout(r, 5)); + } + await promise; + + // After full drain, every folder's project query must have run + // exactly once. Confirms the concurrency cap didn't prematurely + // truncate the work. + const perFolderCalls = fetchMock.mock.calls.filter( + (c) => + typeof c[0] === 'string' && + c[0].includes('v1/projects') && + c[0].includes('parent.type%3Afolder'), + ).length; + expect(perFolderCalls).toBe(FOLDER_COUNT); + }); + it('isolates per-folder query failures so one bad folder does not blank the rest', async () => { // Two folders, the first one's project list throws. The second // one should still return its projects. 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 d9b40a37af..13160b9c00 100644 --- a/apps/api/src/cloud-security/providers/gcp-security.service.ts +++ b/apps/api/src/cloud-security/providers/gcp-security.service.ts @@ -891,8 +891,17 @@ export class GCPSecurityService { ); if (folderIds.length === 0) return []; - const perFolder = await Promise.all( - folderIds.map((folderId) => + // Bound the fan-out: an unbounded `Promise.all(folderIds.map(...))` + // can trigger GCP rate limiting (`429 Too Many Requests`) on + // tenants with many folders. Because `listProjectsPaginated` + // returns the projects collected so far on a non-OK response, a + // throttled folder query LOOKS like an empty folder to the caller + // — silently truncating the picker with no visible error. A modest + // concurrency limit avoids the problem entirely. + const perFolder = await mapWithConcurrency( + folderIds, + FOLDER_QUERY_CONCURRENCY, + (folderId) => this.listProjectsPaginated( accessToken, `lifecycleState:ACTIVE AND parent.type:folder AND parent.id:${folderId}`, @@ -902,7 +911,6 @@ export class GCPSecurityService { ); return []; }), - ), ); return perFolder.flat(); @@ -1456,3 +1464,42 @@ export class GCPSecurityService { return map[gcpSeverity] ?? 'medium'; } } + +/** + * Max simultaneous in-flight folder→projects queries when expanding an + * organization's folder tree. GCP's `cloudresourcemanager` quota + * (~600 read req/min/user) is well above this, but a small cap keeps us + * comfortably below throttling thresholds even for tenants with deep + * folder hierarchies, and prevents bursts that could starve other + * concurrent GCP work on the same account. + */ +const FOLDER_QUERY_CONCURRENCY = 5; + +/** + * Map `items` through `fn` with at most `concurrency` promises in flight + * at any moment. Preserves input order in the result array. No deps — + * inlined here because the only call site is the GCP folder fan-out. + */ +export async function mapWithConcurrency( + items: T[], + concurrency: number, + fn: (item: T) => Promise, +): Promise { + const results: R[] = new Array(items.length); + let cursor = 0; + + const worker = async (): Promise => { + while (true) { + const idx = cursor++; + if (idx >= items.length) return; + results[idx] = await fn(items[idx]); + } + }; + + const workers = Array.from( + { length: Math.min(concurrency, items.length) }, + () => worker(), + ); + await Promise.all(workers); + return results; +}