From 7c4e92ec33bfe50147c2d2d409f0eefb5eb44ae9 Mon Sep 17 00:00:00 2001
From: anshul23102 <anshul23102@iiitd.ac.in>
Date: Mon, 25 May 2026 07:24:31 +0530
Subject: [PATCH] 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<Result<{ id: n
   });
   if (!rateRes.ok) return err('rate_limited', 'slow down', true);
 
-  // Enforce 3-claim limit: count currently claimed recs before allowing a new one.
-  const { count: claimedCount } = await service
-    .from('recommendations')
-    .select('id', { count: 'exact', head: true })
-    .eq('user_id', user.id)
-    .eq('status', 'claimed');
-  if ((claimedCount ?? 0) >= 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<Result<{ id: n
     detail: { recId } as never,
   });
 
-  return ok({ id: data.id });
+  return ok({ id: claimedId });
 }
 
 const PR_URL_RE = /^https:\/\/github\.com\/[^/]+\/[^/]+\/pull\/\d+$/;
diff --git a/supabase/migrations/0012_claim_recommendation_atomic.sql b/supabase/migrations/0012_claim_recommendation_atomic.sql
new file mode 100644
index 0000000..a772a68
--- /dev/null
+++ b/supabase/migrations/0012_claim_recommendation_atomic.sql
@@ -0,0 +1,48 @@
+-- Migration: atomic claim function to close TOCTOU race condition (issue #205)
+--
+-- The previous TypeScript implementation enforced the 3-claim cap with a COUNT
+-- query followed by a separate UPDATE. Because these are two independent database
+-- round-trips, concurrent requests for the same user can both read count < 3,
+-- both pass the guard, and both commit, allowing a user to exceed 3 active claims.
+--
+-- This function moves both operations into a single UPDATE statement. The
+-- subquery that checks the count and the row that gets written are evaluated
+-- atomically by the database engine, so concurrent calls are serialised at
+-- the row level. If the count is already 3 when the UPDATE runs, zero rows
+-- are returned and the claim is rejected without a separate round-trip.
+
+CREATE OR REPLACE FUNCTION public.claim_recommendation_atomic(
+  p_rec_id  bigint,
+  p_user_id uuid
+)
+RETURNS TABLE(id bigint)
+LANGUAGE plpgsql
+SECURITY DEFINER
+SET search_path = public
+AS $$
+BEGIN
+  RETURN QUERY
+  UPDATE recommendations AS r
+  SET
+    status     = 'claimed',
+    claimed_at = now()
+  WHERE
+    r.id      = p_rec_id
+    AND r.user_id = p_user_id
+    AND r.status  = 'open'
+    AND (
+      SELECT COUNT(*)
+      FROM   recommendations r2
+      WHERE  r2.user_id = p_user_id
+        AND  r2.status  = 'claimed'
+    ) < 3
+  RETURNING r.id;
+END;
+$$;
+
+-- Grant execute to authenticated users (server actions run under their session)
+-- and to the service role (used by Inngest functions and webhook handlers).
+GRANT EXECUTE ON FUNCTION public.claim_recommendation_atomic(bigint, uuid)
+  TO authenticated;
+GRANT EXECUTE ON FUNCTION public.claim_recommendation_atomic(bigint, uuid)
+  TO service_role;