fix(waitlist): scope table writes to service_role — anon insert bypassed every control (GH#2503) - #2504
fix(waitlist): scope table writes to service_role — anon insert bypassed every control (GH#2503)#2504dcccrypto wants to merge 1 commit into
Conversation
…sed every control (GH#2503) The waitlist table's only RLS policy was `for insert to anon with check (true)` -- an unconditional grant to the role the PUBLISHABLE key maps to. Every anti-abuse control on signup (referral-code requirement, Turnstile, honeypot, dwell time, disposable-email block, bot-UA filter, per-IP and per-referral-code caps, wallet-signature verification) lives in POST /api/waitlist/signup. That makes the route a security boundary only if it is the ONLY path that can write the table, and it was not: anyone holding the publishable key could INSERT straight to PostgREST and skip all of it. The schema said as much in its own words -- "server-side route verifies the wallet signature BEFORE inserting, so RLS-allowed inserts are gated on real ownership" -- which cannot be true of a path the route does not sit in front of. Because created_at, tier, referral_code and referred_by_code are all client-settable, that yields more than spam: waitlist_position() orders by (created_at ASC, id ASC), so a backdated row takes position #1, and waitlist_referral_leaderboard() groups by referred_by_code, so bulk inserts control the ranking. Two changes, which must both be present for either to mean anything: - supabase-waitlist-schema.sql: the INSERT policy is now `to service_role`. - the signup route's insert now uses getWaitlistServiceSupabase(). No new secret: WAITLIST_SUPABASE_SERVICE_ROLE_KEY already exists and the same route already uses the service client in five other places, so it is provisioned wherever this deploys. What actually closes the hole is REMOVING the anon policy, not adding the service_role one: with RLS enabled and no policy applying to anon, anon INSERT is denied by default, and service_role holds BYPASSRLS and would write either way. The policy is written out anyway so the intended writer is stated rather than implied by an absence, and so a future `create policy ... to anon` reads as the regression it is. The schema comments that asserted the old model -- including the tamper-resistance rationale that leaned on "the route validates referred_by_code" -- are corrected rather than left to mislead. getWaitlistSupabase() remains for the waitlist_referral_code_exists RPC, which is SECURITY DEFINER and explicitly granted to anon. That is the designed use of a publishable key: reads through a function, not direct table writes. Tests guard the source, matching the sibling waitlist-signup-*.test.ts files, because the property is structural -- which credential may write is not observable from a request-level test. They parse the SQL with `--` comments stripped; the first version matched the prose, since the schema discusses anon at length and the new note quotes `create policy ... to anon` to describe the regression it guards. Mutation-tested, control either side: control (both halves) 4 passed schema policy back to anon 2 failed route back to the anon client 1 failed control (both halves) 4 passed Verified: 5 waitlist test files, 37 passed; tsc --noEmit exit 0. The full app suite reports 42 failures across 5 files (oracle-advance-phase, useChartDrawingTool, useChartDrawings, useStuckSlabs, admin-session-security-v2). Those are pre-existing: clean origin/main with no changes fails the same 5 files with the same 42 failures. None are waitlist related. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe waitlist signup route now uses the service-role Supabase client. The schema permits waitlist inserts only for ChangesWaitlist write authorization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/__tests__/api/waitlist-signup-write-authz.test.ts`:
- Around line 44-50: Update sqlStatements() and the waitlist authorization
assertions to include ALTER POLICY statements affecting public.waitlist, parse
each policy’s complete TO role list, and require the role set to be exactly
service_role. Ensure policies targeting other tables or containing additional
roles such as anon do not satisfy the guard.
In `@supabase-waitlist-schema.sql`:
- Around line 221-247: Update the waitlist policy migration before creating
"service_role insert" to explicitly drop the deployed legacy anon INSERT policy
by its existing policy name. Ensure the upgrade removes the permissive anon
policy from existing databases, and add a regression check covering migration
from that legacy policy to service_role-only inserts.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 342cf893-04ae-4b70-8e58-0f3428bfca6d
📒 Files selected for processing (3)
app/__tests__/api/waitlist-signup-write-authz.test.tsapp/app/api/waitlist/signup/route.tssupabase-waitlist-schema.sql
| function sqlStatements(schema: string): string[] { | ||
| const code = schema | ||
| .split("\n") | ||
| .map((line) => line.replace(/--.*$/, "")) | ||
| .join("\n"); | ||
| return code.match(/create policy[\s\S]*?;/gi) ?? []; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect every policy declaration and mutation that can affect waitlist access.
rg -n -i -C 3 \
'^\s*(create|alter)\s+policy\b|^\s*on\s+public\.waitlist\b|^\s*for\s+(insert|update|delete)\b|^\s*to\s+' \
supabase-waitlist-schema.sqlRepository: dcccrypto/percolator-launch
Length of output: 1935
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test file outline/size =="
wc -l app/__tests__/api/waitlist-signup-write-authz.test.ts
sed -n '1,140p' app/__tests__/api/waitlist-signup-write-authz.test.ts
echo
echo "== policy references in tests =="
rg -n -i -C 2 'CREATE POLICY|CREATE\s+POLICY|ALTER POLICY|ALTER\s+POLICY|service_role|anon|to\s' app/__tests__/api/waitlist-signup-write-authz.test.ts
echo
echo "== parsed schema waitlist policy references =="
awk 'BEGIN{IGNORECASE=1} /create policy|alter policy|on public.waitlist|to service_role|to anon|to authenticated/ {print NR": "$0}' supabase-waitlist-schema.sqlRepository: dcccrypto/percolator-launch
Length of output: 9654
Parse waitlist write policies into role sets.
sqlStatements() only returns CREATE POLICY, so an ALTER POLICY public.waitlist ... TO anon changes would not be included. Current text checks would also pass TO service_role, anon because it contains to service_role. Scope checks to public.waitlist, parse every policy TO list, require exactly service_role, and include ALTER POLICY mutations in this guard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/__tests__/api/waitlist-signup-write-authz.test.ts` around lines 44 - 50,
Update sqlStatements() and the waitlist authorization assertions to include
ALTER POLICY statements affecting public.waitlist, parse each policy’s complete
TO role list, and require the role set to be exactly service_role. Ensure
policies targeting other tables or containing additional roles such as anon do
not satisfy the guard.
| drop policy if exists "service_role insert" on public.waitlist; | ||
|
|
||
| -- Anon can insert (server-side route validates the signature first). | ||
| create policy "anon insert" | ||
| -- GH#2503: writes are server-only. | ||
| -- | ||
| -- This previously read `to anon with check (true)` — an unconditional grant to | ||
| -- the role the PUBLISHABLE key maps to. Every anti-abuse control (referral-code | ||
| -- requirement, Turnstile, honeypot, dwell time, disposable-email block, bot-UA | ||
| -- filter, per-IP and per-code caps, wallet-signature verification) lives in | ||
| -- POST /api/waitlist/signup, so that route was being treated as the security | ||
| -- boundary while the database accepted writes from anyone holding a key that is | ||
| -- public by design. A direct PostgREST INSERT skipped all of it and could set | ||
| -- created_at (→ position #1, since waitlist_position orders by created_at ASC), | ||
| -- tier, referral_code and referred_by_code (→ leaderboard fraud) at will. | ||
| -- | ||
| -- Note on what actually closes it: the protection comes from REMOVING the anon | ||
| -- policy, not from adding the one below. With RLS enabled and no policy | ||
| -- applying to anon, anon INSERT is denied by default; service_role holds | ||
| -- BYPASSRLS and would write regardless. The policy is written out anyway so the | ||
| -- intended writer is stated in the schema rather than implied by an absence, | ||
| -- and so a future `create policy ... to anon` reads as the regression it is. | ||
| -- | ||
| -- The signup route uses getWaitlistServiceSupabase() for this insert. SELECT / | ||
| -- UPDATE / DELETE remain unpolicied for anon, as before. | ||
| create policy "service_role insert" | ||
| on public.waitlist | ||
| for insert | ||
| to anon | ||
| to service_role |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Drop the deployed legacy anon INSERT policy.
Line 221 drops only "service_role insert". An existing database can still retain the prior anon INSERT policy. PostgreSQL then continues to permit direct publishable-key inserts, because the legacy permissive policy remains effective.
Drop the legacy anon policy by its deployed policy name before creating the service-role policy. Add a regression check for this upgrade path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase-waitlist-schema.sql` around lines 221 - 247, Update the waitlist
policy migration before creating "service_role insert" to explicitly drop the
deployed legacy anon INSERT policy by its existing policy name. Ensure the
upgrade removes the permissive anon policy from existing databases, and add a
regression check covering migration from that legacy policy to service_role-only
inserts.
|
Caveat on this PR's green checks, so nobody over-trusts them.
It matters specifically here because the only invocation of the app suite in any of
So the verification that stands behind this PR is the local run and the mutation battery in the description, not the checkmarks above: If you want CI to actually exercise these before merging, porting #2450's |
|
Closing — out of scope for the current playground focus. This does not resolve GH#2503, and the exposure is unchanged. Recording what was verified so it isn't lost: The vulnerability is a direct PostgREST INSERT against the waitlist table, using the publishable/anon key. It never reaches create policy "anon insert" on public.waitlist for insert to anon with check (true);An attacker can therefore skip every anti-abuse control the route applies (referral code, Turnstile, honeypot, dwell time, disposable-email block, bot-UA filter, per-IP and per-code caps, wallet-signature verification) and choose Two things make this different from the sibling issue in #2500, which I checked while reviewing:
To close it later, the single required action is running the policy change against the live waitlist project. Reopen this or re-cut it when that's in scope. 🤖 Generated with Claude Code |
1 participant
Closes #2503.
The hole
The waitlist table's only RLS policy was:
with check (true)foranonis an unconditional grant to the role the publishable key maps to. Every anti-abuse control on signup — referral-code requirement, Turnstile, honeypot, dwell time, disposable-email block, bot-UA filter, per-IP and per-referral-code caps, wallet-signature verification — lives inPOST /api/waitlist/signup. That route is a security boundary only if it is the only path that can write the table, and it was not.The schema stated the old model in its own words:
That cannot be true of a write path the route does not sit in front of. In Supabase's model the publishable key is not a secret — its safety is entirely RLS, and RLS granted unconditional insert.
Because
created_at,tier,referral_codeandreferred_by_codeare all client-settable, the consequence is more than spam:waitlist_position()orders by(created_at ASC, id ASC), so a backdated row takes position #1;waitlist_referral_leaderboard()groups byreferred_by_code, so bulk inserts control the ranking.The fix
Two changes, and neither means anything without the other:
supabase-waitlist-schema.sql— the INSERT policy is nowto service_role.app/app/api/waitlist/signup/route.ts— the insert now usesgetWaitlistServiceSupabase().No new secret.
WAITLIST_SUPABASE_SERVICE_ROLE_KEYalready exists, and this same route already uses the service client in five other places (:607,:787,:867,:939,:1017) — so it is provisioned wherever this deploys.What actually closes it is removing the anon policy, not adding the service_role one. With RLS enabled and no policy applying to
anon, anon INSERT is denied by default;service_roleholdsBYPASSRLSand would write regardless. I wrote the policy out anyway so the intended writer is stated in the schema rather than implied by an absence, and so a futurecreate policy … to anonreads as the regression it is. I have said this plainly in the schema comment rather than letting the new policy look like the protection.getWaitlistSupabase()stays for thewaitlist_referral_code_existsRPC — SECURITY DEFINER and explicitlygranted to anon (schema:328). That is the designed use of a publishable key: reads through a function, not direct table writes. A test pins that grant so revoking it upstream can't silently break the route.I also corrected the schema comments that asserted the old model, including the "WHY IT'S TAMPER-RESISTANT" note, which conceded an attacker "would need INSERT access … (which they have)" and then leaned on the route validating
referred_by_code. With writes scoped to service_role that sentence is now true rather than assumed.Deploy the code first, then apply the SQL.
service_rolebypasses RLS, so the updated route works under the old policy too — deploying code first is a no-op for behaviour. Applying the SQL first would deny anon inserts while the deployed route is still using the anon client, breaking signups until the deploy lands.Base branch — deliberately
main, notplaygroundFlagging this because it is unusual for my PRs and worth a maintainer's eye: the waitlist feature does not exist on
playground. That branch hassupabase-waitlist-schema.sqlbut no routes, lib or components —getWaitlistSupabasehas zero hits there. The whole feature (app/app/api/waitlist/**,app/lib/waitlist/**, the tests) lives only onmain.So the schema is vulnerable on both branches, but the consuming code that has to change with it is on
mainalone. Targetingplaygroundwould have tightened a policy with no route to update and no way to verify the pair.(#2503 audits
bd0e7d86, which isorigin/mainat 2026-07-21;playgroundis at58db9192. The two have diverged substantially.)Verification
tsc --noEmitexit 0.anon--comments stripped. The first version matched the raw text and failed, because the schema discussesanonat length and my own new note quotescreate policy … to anonto describe the regression it guards. Worth knowing before editing them.Pre-existing failures, not from this change
The full app suite reports 42 failures across 5 files —
oracle-advance-phase,useChartDrawingTool,useChartDrawings,useStuckSlabs,admin-session-security-v2.These are not mine. Clean
origin/mainwith no changes applied fails the same 5 files with the same 42 failures. None are waitlist related. Raising it separately because a base branch sitting at 42 red tests is worth someone's attention on its own.Severity note
#2503 rates this Medium, escalating to High if the waitlist publishable key is reachable from a public bundle, and I agree with that framing. I did not probe the live deployment. In this snapshot the key is referenced only by server route handlers, so it is probably not in the browser bundle today — but the fix does not depend on where in that range it lands, and the exposure is one client-side insert away from being unauthenticated-anyone.
Needs Security review.
Summary by CodeRabbit