From 7c4e92ec33bfe50147c2d2d409f0eefb5eb44ae9 Mon Sep 17 00:00:00 2001 From: anshul23102 Date: Mon, 25 May 2026 07:24:31 +0530 Subject: [PATCH 1/2] fix: close TOCTOU race condition in claimRecommendation Fixes #205 The previous implementation enforced the 3-claim cap with two independent database round-trips: a COUNT query, then a separate UPDATE. Between those two steps, a second concurrent request for the same user could also read count < 3, pass the guard, and commit its own UPDATE. Sending multiple claim requests in parallel (trivially via Promise.all) reproduced the bug reliably -- the user ended up with more than 3 active claims. Fix: replaced the two-step sequence with a call to a new database function, claim_recommendation_atomic, added in migration 0012. The function merges the count check and the write into a single UPDATE statement. The subquery that evaluates the claim count and the row that gets written are handled atomically by the database engine, so concurrent calls are serialised at the row level and only one can succeed when the count is at the limit. Migration 0012 (supabase/migrations/0012_claim_recommendation_atomic.sql): - Defines claim_recommendation_atomic(p_rec_id bigint, p_user_id uuid) returning TABLE(id bigint). - The UPDATE only proceeds if the caller's current claimed count is strictly less than 3 AND the target rec is still open. Zero rows returned covers both rejection cases (limit reached or rec not open). - SECURITY DEFINER with SET search_path = public prevents search-path injection. - GRANT EXECUTE given to both authenticated and service_role so server actions and background functions can call it without privilege changes. recommendations.ts: - Removed the COUNT query and the subsequent maybeSingle UPDATE. - Added service.rpc('claim_recommendation_atomic', ...) in their place. - Interprets zero returned rows as a unified claim_limit_or_not_open error; the UI already re-fetches state on any error so the distinction between the two rejection reasons is not needed client-side. - data.id reference updated to claimedId extracted from the RPC result. --- src/app/actions/recommendations.ts | 47 +++++++++--------- .../0012_claim_recommendation_atomic.sql | 48 +++++++++++++++++++ 2 files changed, 73 insertions(+), 22 deletions(-) create mode 100644 supabase/migrations/0012_claim_recommendation_atomic.sql diff --git a/src/app/actions/recommendations.ts b/src/app/actions/recommendations.ts index cd8253f..0a48575 100644 --- a/src/app/actions/recommendations.ts +++ b/src/app/actions/recommendations.ts @@ -112,29 +112,32 @@ export async function claimRecommendation(recId: number): Promise= 3) { - return err('claim_limit', 'you already have 3 active claims - merge or close them first'); + // Atomic claim: the count check and the write are merged into a single UPDATE + // inside claim_recommendation_atomic (see migration 0012). This eliminates the + // TOCTOU window that existed when they were separate round-trips -- concurrent + // requests can no longer both pass a count of 2 and both commit, because the + // subquery that evaluates the count and the row that gets written are handled + // atomically by the database engine. + // + // Zero rows returned means one of two things: the user already holds 3 active + // claims, or this specific rec is no longer open. Both outcomes are safe to + // surface with the same error because the UI re-fetches state after either. + const { data: rpcData, error: rpcErr } = await service.rpc( + 'claim_recommendation_atomic', + { p_rec_id: recId, p_user_id: user.id }, + ); + + if (rpcErr) return err('persist_failed', rpcErr.message); + + const rows = rpcData as Array<{ id: number }> | null; + if (!rows || rows.length === 0) { + return err( + 'claim_limit_or_not_open', + 'claim rejected: you may already have 3 active claims, or this rec is no longer open', + ); } - // Atomic claim: UPDATE ... WHERE status='open' AND user_id=auth.uid() - // Zero rows affected = already claimed or doesn't exist. - const { data, error: updateErr } = await service - .from('recommendations') - .update({ status: 'claimed', claimed_at: new Date().toISOString() }) - .eq('id', recId) - .eq('user_id', user.id) - .eq('status', 'open') - .select('id') - .maybeSingle(); - - if (updateErr) return err('persist_failed', updateErr.message); - if (!data) return err('already_claimed', 'this rec is no longer open'); + const claimedId = rows[0].id; // Invalidate cache so next dashboard load is fresh. await cacheDel(`recs:${user.id}`); @@ -146,7 +149,7 @@ export async function claimRecommendation(recId: number): Promise Date: Mon, 25 May 2026 07:39:33 +0530 Subject: [PATCH 2/2] fix: stop embedding OAuth token in Inngest audit event payload bootstrapProfile was passing the user's live GitHub OAuth provider token directly inside inngest.send() for the audit/run event. Inngest persists event payloads to third-party cloud storage, so this exposed long-lived credentials outside the application's trust boundary. The audit-run function already supports a three-tier auth fallback: 1. installationId from the event (minted into a short-lived install token at execution time) 2. Live DB lookup of github_installations at execution time 3. accessToken from the event (legacy path, retained for backward compat) bootstrapProfile now looks up the user's active installation at event-send time and passes installationId instead of accessToken. If no installation exists yet (common at first login), installationId is omitted and the function performs the DB lookup at execution time, by which point the install webhook will typically have landed. Also removes the getSession() call that was only needed to retrieve the provider token, and fixes a TypeScript strict-null error in claimRecommendation (rows[0] after an explicit length guard). Closes #204 --- src/app/actions/profile.ts | 38 ++++++++++++++++++++---------- src/app/actions/recommendations.ts | 10 ++++---- src/inngest/functions/audit-run.ts | 16 ++++++++----- 3 files changed, 40 insertions(+), 24 deletions(-) diff --git a/src/app/actions/profile.ts b/src/app/actions/profile.ts index 7f80828..d928096 100644 --- a/src/app/actions/profile.ts +++ b/src/app/actions/profile.ts @@ -69,19 +69,31 @@ export async function bootstrapProfile(): Promise> { let auditQueued = false; if (!profile.audit_completed) { - const providerToken = (await sb.auth.getSession()).data.session?.provider_token; - if (providerToken) { - await inngest.send({ - name: 'audit/run', - data: { - userId: profile.id, - githubHandle: profile.github_handle, - githubId, - accessToken: providerToken, - }, - }); - auditQueued = true; - } + // Look up the user's active GitHub App installation. Passing the installation + // ID lets audit-run mint a short-lived install token at execution time rather + // than embedding a long-lived OAuth token in the Inngest event payload (which + // is persisted to third-party storage). If no installation exists yet the + // function performs the same DB lookup at execution time and may find one by + // then; if still nothing it exits cleanly with reason 'no_auth_source'. + const { data: installs } = await service + .from('github_installations') + .select('id') + .eq('user_id', profile.id) + .is('uninstalled_at', null) + .order('installed_at', { ascending: false }) + .limit(1); + const installationId = (installs?.[0]?.id as number | undefined) ?? undefined; + + await inngest.send({ + name: 'audit/run', + data: { + userId: profile.id, + githubHandle: profile.github_handle, + githubId, + installationId, + }, + }); + auditQueued = true; } // Fire-and-forget maintainer discovery so this user picks up admin diff --git a/src/app/actions/recommendations.ts b/src/app/actions/recommendations.ts index 0a48575..1136b72 100644 --- a/src/app/actions/recommendations.ts +++ b/src/app/actions/recommendations.ts @@ -122,10 +122,10 @@ export async function claimRecommendation(recId: number): Promise