diff --git a/supabase/functions/_backend/public/build/ai_analyze.ts b/supabase/functions/_backend/public/build/ai_analyze.ts index 7b329d89f2..bfd2887530 100644 --- a/supabase/functions/_backend/public/build/ai_analyze.ts +++ b/supabase/functions/_backend/public/build/ai_analyze.ts @@ -3,7 +3,7 @@ import type { Database } from '../../utils/supabase.types.ts' import { quickError, simpleError } from '../../utils/hono.ts' import { cloudlog, cloudlogErr, serializeError } from '../../utils/logging.ts' import { checkPermission } from '../../utils/rbac.ts' -import { supabaseApikey } from '../../utils/supabase.ts' +import { supabaseAdmin, supabaseApikey } from '../../utils/supabase.ts' import { sendEventToTracking } from '../../utils/tracking.ts' import { getEnv } from '../../utils/utils.ts' @@ -232,12 +232,35 @@ export async function aiAnalyzeBuild( const durationMs = Date.now() - builderStartedAt - // 4. Flip the flag after the builder succeeds (idempotency) - const { error: updateErr } = await supabase + // 4. Flip the flag after the builder succeeds (idempotency). + // + // IMPORTANT: this UPDATE must use the service-role client. The + // `build_requests` table has RLS policies for SELECT (org members) and + // ALL (service role only) — there is NO policy granting UPDATE to the + // authenticated/anon user-context client created by `supabaseApikey()`. + // Using `supabase` (user-context) here silently fails the write under + // RLS: PostgREST reports zero rows matched, the operation returns + // `{ error: null }`, the flag stays false, and the idempotency guard at + // step 2 (`row.ai_analyzed === true`) never fires for any subsequent + // request — so a single job can be re-analyzed indefinitely and run up + // unbounded Workers AI cost. + // + // We've already verified user authorization above via `checkPermission` + // and the user-context SELECT, so escalating to service-role for this + // one targeted write is safe. + // + // `.select('builder_job_id')` returns the affected rows so we can + // distinguish a real success from a 0-row no-op (e.g. if the row was + // deleted between the SELECT and the UPDATE). The analysis has already + // run by this point, so we still don't throw — but a 0-row update is + // logged loudly because it means idempotency is broken for this job. + const adminClient = supabaseAdmin(c) + const { data: updatedRows, error: updateErr } = await adminClient .from('build_requests') .update({ ai_analyzed: true, updated_at: new Date().toISOString() }) .eq('builder_job_id', jobId) .eq('app_id', appId) + .select('builder_job_id') if (updateErr) { // Log but don't throw — the analysis already happened; the user got their result. @@ -249,6 +272,16 @@ export async function aiAnalyzeBuild( error: updateErr.message, }) } + else if (!updatedRows || updatedRows.length === 0) { + // Row vanished between SELECT and UPDATE, or some other unexpected + // mismatch. Idempotency is broken for this job. Log so it's findable. + cloudlogErr({ + requestId: c.get('requestId'), + message: 'ai_analyzed UPDATE matched zero rows after successful analysis — idempotency broken for this job', + job_id: jobId, + app_id: appId, + }) + } cloudlog({ requestId: c.get('requestId'), diff --git a/tests/build-ai-analyze.test.ts b/tests/build-ai-analyze.test.ts index 95b354dddc..13ed0529b4 100644 --- a/tests/build-ai-analyze.test.ts +++ b/tests/build-ai-analyze.test.ts @@ -1,8 +1,9 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import { aiAnalyzeBuild } from '../supabase/functions/_backend/public/build/ai_analyze' -const { mockSupabaseApikey, mockCheckPermission, mockGetEnv, mockSendEventToTracking } = vi.hoisted(() => ({ +const { mockSupabaseApikey, mockSupabaseAdmin, mockCheckPermission, mockGetEnv, mockSendEventToTracking } = vi.hoisted(() => ({ mockSupabaseApikey: vi.fn(), + mockSupabaseAdmin: vi.fn(), mockCheckPermission: vi.fn(), mockGetEnv: vi.fn(), mockSendEventToTracking: vi.fn(), @@ -10,6 +11,7 @@ const { mockSupabaseApikey, mockCheckPermission, mockGetEnv, mockSendEventToTrac vi.mock('../supabase/functions/_backend/utils/supabase.ts', () => ({ supabaseApikey: mockSupabaseApikey, + supabaseAdmin: mockSupabaseAdmin, })) vi.mock('../supabase/functions/_backend/utils/rbac.ts', () => ({ checkPermission: mockCheckPermission, @@ -45,26 +47,49 @@ interface RowShape { owner_org: string } -function mockBuildRequestRow(row: RowShape | null) { +// Default admin UPDATE result: 1 row affected. Tests that exercise the +// 0-row-update edge case can override via `adminUpdateRows`. +function mockBuildRequestRow(row: RowShape | null, opts: { adminUpdateRows?: Array<{ builder_job_id: string }> } = {}) { + // ── apikey client: SELECT only ───────────────────────────────────────── + // The user-context client is used for the authorization SELECT against + // `build_requests`. It is NEVER allowed to do UPDATE — that's what this + // test file is regression-protecting. `apikeyUpdate` is wired so we can + // assert it never gets called. const eqAppId = { maybeSingle: vi.fn().mockResolvedValue({ data: row, error: null }) } const eqJob = { eq: vi.fn().mockReturnValue(eqAppId) } const select = { eq: vi.fn().mockReturnValue(eqJob) } - const updateEqApp = vi.fn().mockResolvedValue({ error: null }) - const updateEqJob = { eq: vi.fn().mockReturnValue({ eq: updateEqApp }) } - const update = vi.fn().mockReturnValue(updateEqJob) + const apikeyUpdate = vi.fn() mockSupabaseApikey.mockReturnValue({ from: vi.fn().mockImplementation((table: string) => { expect(table).toBe('build_requests') - return { select: vi.fn().mockReturnValue(select), update } + return { select: vi.fn().mockReturnValue(select), update: apikeyUpdate } }), }) - return { updateEqApp } + + // ── admin client: UPDATE only ────────────────────────────────────────── + // The service-role client is used for the post-analysis flag write. + // The chain is `.update().eq().eq().select('builder_job_id')` → resolves + // to `{ data: rows, error }`. + const updateRows = opts.adminUpdateRows ?? (row ? [{ builder_job_id: 'job-abc' }] : []) + const adminSelect = vi.fn().mockResolvedValue({ data: updateRows, error: null }) + const adminEqApp = { eq: vi.fn().mockReturnValue({ select: adminSelect }) } + const adminEqJob = vi.fn().mockReturnValue(adminEqApp) + const adminUpdate = vi.fn().mockReturnValue({ eq: adminEqJob }) + mockSupabaseAdmin.mockReturnValue({ + from: vi.fn().mockImplementation((table: string) => { + expect(table).toBe('build_requests') + return { update: adminUpdate } + }), + }) + + return { apikeyUpdate, adminUpdate, adminSelect } } const apikey = { key: 'apikey-test', user_id: 'user-1' } as any beforeEach(() => { mockSupabaseApikey.mockReset() + mockSupabaseAdmin.mockReset() mockCheckPermission.mockReset() mockGetEnv.mockReset() mockSendEventToTracking.mockReset() @@ -146,14 +171,16 @@ describe('aiAnalyzeBuild', () => { it('does NOT flip the flag when builder proxy returns non-2xx; fires Requested + Result(builder_error) with duration_ms', async () => { mockCheckPermission.mockResolvedValue(true) - const { updateEqApp } = mockBuildRequestRow({ app_id: appId, status: 'failed', ai_analyzed: false, owner_org: orgId }) + const { apikeyUpdate, adminUpdate } = mockBuildRequestRow({ app_id: appId, status: 'failed', ai_analyzed: false, owner_org: orgId }) ;(globalThis.fetch as any).mockResolvedValue(new Response('upstream broken', { status: 503 })) await expect(aiAnalyzeBuild(createContext(), jobId, appId, apikey, 'small logs')) .rejects .toThrow(/AI analysis failed/i) - expect(updateEqApp).not.toHaveBeenCalled() + // Builder failed → no UPDATE on EITHER client. The flag must stay false. + expect(apikeyUpdate).not.toHaveBeenCalled() + expect(adminUpdate).not.toHaveBeenCalled() expect(trackingCallsByEvent('AI Build Analysis Requested')).toHaveLength(1) const results = trackingCallsByEvent('AI Build Analysis Result') expect(results).toHaveLength(1) @@ -161,16 +188,24 @@ describe('aiAnalyzeBuild', () => { expect(results[0][1].tags.duration_ms).toBeDefined() }) - it('flips the flag, returns analysis on builder 200, fires Requested + Result(success); does NOT leak analysis text in tags', async () => { + it('flips the flag via SERVICE-ROLE client (never the apikey client), returns analysis, fires Requested + Result(success); does NOT leak analysis text in tags', async () => { mockCheckPermission.mockResolvedValue(true) - const { updateEqApp } = mockBuildRequestRow({ app_id: appId, status: 'failed', ai_analyzed: false, owner_org: orgId }) + const { apikeyUpdate, adminUpdate } = mockBuildRequestRow({ app_id: appId, status: 'failed', ai_analyzed: false, owner_org: orgId }) ;(globalThis.fetch as any).mockResolvedValue( new Response(JSON.stringify({ analysis: '### Likely cause\nfoo' }), { status: 200, headers: { 'content-type': 'application/json' } }), ) const result = await aiAnalyzeBuild(createContext(), jobId, appId, apikey, 'small logs') - expect(updateEqApp).toHaveBeenCalledTimes(1) + // Regression: the UPDATE that flips `ai_analyzed=true` MUST go through + // the service-role client. The `build_requests` table has no RLS policy + // granting UPDATE to the user-context apikey client — using it would + // silently fail and break idempotency. We assert both halves of this: + // the admin client got called exactly once, and the apikey client's + // .update was never invoked at all. + expect(adminUpdate).toHaveBeenCalledTimes(1) + expect(adminUpdate).toHaveBeenCalledWith(expect.objectContaining({ ai_analyzed: true })) + expect(apikeyUpdate).not.toHaveBeenCalled() expect(await result.json()).toEqual({ analysis: '### Likely cause\nfoo' }) // Verify the builder URL and headers @@ -195,6 +230,34 @@ describe('aiAnalyzeBuild', () => { } }) + it('still returns the analysis when the admin UPDATE matched zero rows (defensive — analysis already happened, but we log the broken-idempotency case)', async () => { + mockCheckPermission.mockResolvedValue(true) + // Simulate the row vanishing between the user-context SELECT and the + // service-role UPDATE. We still want the caller to get their analysis + // back (the AI work has already been done and paid for); the fix is + // that the production code logs this case loudly so it's findable. + const { adminUpdate } = mockBuildRequestRow( + { app_id: appId, status: 'failed', ai_analyzed: false, owner_org: orgId }, + { adminUpdateRows: [] }, // <-- 0-row update + ) + ;(globalThis.fetch as any).mockResolvedValue( + new Response(JSON.stringify({ analysis: '### Likely cause\nfoo' }), { status: 200, headers: { 'content-type': 'application/json' } }), + ) + + const result = await aiAnalyzeBuild(createContext(), jobId, appId, apikey, 'small logs') + + // The analysis still comes back to the caller — we don't punish the + // user for a server-side bookkeeping miss. + expect(adminUpdate).toHaveBeenCalledTimes(1) + expect(await result.json()).toEqual({ analysis: '### Likely cause\nfoo' }) + + // Success Result event still fires — the AI half of the operation + // completed; only the persistence bookkeeping was incomplete. + const results = trackingCallsByEvent('AI Build Analysis Result') + expect(results).toHaveLength(1) + expect(results[0][1].tags.result).toBe('success') + }) + it('fires Result(config_error) when BUILDER_URL is missing', async () => { mockCheckPermission.mockResolvedValue(true) mockBuildRequestRow({ app_id: appId, status: 'failed', ai_analyzed: false, owner_org: orgId })