From 9bf3a3b1f6fe2c08b3880fe8f1e299f820ca0bc9 Mon Sep 17 00:00:00 2001 From: dcccrypto Date: Mon, 10 Aug 2026 02:56:34 +0100 Subject: [PATCH] =?UTF-8?q?fix(waitlist):=20scope=20table=20writes=20to=20?= =?UTF-8?q?service=5Frole=20=E2=80=94=20anon=20insert=20bypassed=20every?= =?UTF-8?q?=20control=20(GH#2503)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../api/waitlist-signup-write-authz.test.ts | 109 ++++++++++++++++++ app/app/api/waitlist/signup/route.ts | 9 +- supabase-waitlist-schema.sql | 44 +++++-- 3 files changed, 152 insertions(+), 10 deletions(-) create mode 100644 app/__tests__/api/waitlist-signup-write-authz.test.ts diff --git a/app/__tests__/api/waitlist-signup-write-authz.test.ts b/app/__tests__/api/waitlist-signup-write-authz.test.ts new file mode 100644 index 000000000..b88909f23 --- /dev/null +++ b/app/__tests__/api/waitlist-signup-write-authz.test.ts @@ -0,0 +1,109 @@ +/** + * GH#2503 — the waitlist table must have no anon write path. + * + * 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. + * + * It was not. The RLS policy read `for insert to anon with check (true)` — an + * unconditional grant to the role the PUBLISHABLE key maps to — so a direct + * PostgREST INSERT skipped every control above and could choose `created_at` + * (waitlist_position orders by created_at ASC, so: position #1), `tier`, + * `referral_code` and `referred_by_code` (leaderboard fraud). + * + * These assertions guard the source in the same style as the sibling + * waitlist-signup-*.test.ts files, because the property is structural: it is + * about which credential may write, which no request-level test can observe. + */ + +import { describe, it, expect } from "vitest"; +import * as fs from "fs"; +import * as path from "path"; + +const ROUTE_PATH = path.resolve( + __dirname, + "../../app/api/waitlist/signup/route.ts", +); +const SCHEMA_PATH = path.resolve( + __dirname, + "../../../supabase-waitlist-schema.sql", +); + +/** + * Strip `--` line comments before matching statements. + * + * This file is mostly prose: it explains the anon/publishable-key model at + * length, and the GH#2503 note deliberately quotes `create policy ... to anon` + * to describe the regression it is guarding against. Matching statements + * against the raw text picks that sentence up as a policy and reads forward to + * the next `;`, which is how the first version of this test failed. Parse the + * SQL, not the commentary. + */ +function sqlStatements(schema: string): string[] { + const code = schema + .split("\n") + .map((line) => line.replace(/--.*$/, "")) + .join("\n"); + return code.match(/create policy[\s\S]*?;/gi) ?? []; +} + +describe("GH#2503: waitlist writes are server-only", () => { + it("grants the waitlist INSERT policy to service_role, not anon", () => { + const schema = fs.readFileSync(SCHEMA_PATH, "utf8"); + + const insertPolicies = sqlStatements(schema).filter((p) => + /for\s+insert/i.test(p), + ); + + expect(insertPolicies.length).toBeGreaterThan(0); + for (const policy of insertPolicies) { + expect(policy).toMatch(/to\s+service_role/i); + expect(policy).not.toMatch(/to\s+anon/i); + } + }); + + it("leaves no INSERT/UPDATE/DELETE policy granted to anon", () => { + const schema = fs.readFileSync(SCHEMA_PATH, "utf8"); + const anonWritePolicies = sqlStatements(schema).filter( + (p) => /to\s+anon/i.test(p) && /for\s+(insert|update|delete)/i.test(p), + ); + + expect(anonWritePolicies).toEqual([]); + }); + + it("inserts through the service-role client, not the publishable one", () => { + const source = fs.readFileSync(ROUTE_PATH, "utf8"); + + // The binding the insert is performed on must come from the service client. + const insertBinding = source.match( + /const\s+(\w+)\s*=\s*getWaitlistServiceSupabase\(\);[\s\S]*?await\s+\1\s*\.from\("waitlist"\)\s*\.insert\(/, + ); + expect(insertBinding).not.toBeNull(); + + // And no insert may be performed on a binding built from the anon client. + const anonInsert = source.match( + /const\s+(\w+)\s*=\s*getWaitlistSupabase\(\);[\s\S]*?await\s+\1\s*\.from\("waitlist"\)\s*\.insert\(/, + ); + expect(anonInsert).toBeNull(); + }); + + it("still uses the anon client only for the anon-granted RPC", () => { + const source = fs.readFileSync(ROUTE_PATH, "utf8"); + const schema = fs.readFileSync(SCHEMA_PATH, "utf8"); + + // getWaitlistSupabase() may remain for reads that go through a + // SECURITY DEFINER function anon is explicitly granted — that is the + // designed use of the publishable key and is not a write path. + const anonUses = source.match(/getWaitlistSupabase\(\)\s*\.\s*(\w+)/g) ?? []; + for (const use of anonUses) { + expect(use).toMatch(/\.rpc$/); + } + + // Guard the grant this depends on, so revoking it upstream is not silent. + expect(schema).toMatch( + /grant execute on function public\.waitlist_referral_code_exists\(text\) to anon;/, + ); + }); +}); diff --git a/app/app/api/waitlist/signup/route.ts b/app/app/api/waitlist/signup/route.ts index 9b125037a..4640d0728 100644 --- a/app/app/api/waitlist/signup/route.ts +++ b/app/app/api/waitlist/signup/route.ts @@ -758,7 +758,14 @@ export async function POST(req: Request) { // fresh random 8-char Crockford code; retry with a new one // • waitlist_pubkey_key or waitlist_email_unique_idx — the same user // re-submitting; idempotent, mark as duplicate and move on - const supabase = getWaitlistSupabase(); + // GH#2503: this insert must go through the SERVICE-ROLE client. The waitlist + // table's INSERT policy is scoped to service_role; the publishable/anon key + // has no write policy, because every anti-abuse control this route applies + // (referral code, Turnstile, honeypot, dwell time, disposable-email block, + // bot-UA filter, per-IP and per-code caps, signature verification) is only a + // boundary if this route is the ONLY write path. It was not, while anon could + // INSERT directly against PostgREST with a key that is public by design. + const supabase = getWaitlistServiceSupabase(); const baseRow: Record = { twitter_handle, source, diff --git a/supabase-waitlist-schema.sql b/supabase-waitlist-schema.sql index 82a952e2b..1e3af878d 100644 --- a/supabase-waitlist-schema.sql +++ b/supabase-waitlist-schema.sql @@ -6,7 +6,9 @@ -- end state. -- -- Design: --- - Anonymous users insert via the publishable key (RLS allows insert only). +-- - Inserts are server-only, via WAITLIST_SUPABASE_SERVICE_ROLE_KEY (GH#2503). +-- The publishable key can no longer write; it is only used for anon-callable +-- SECURITY DEFINER RPCs. -- - Server-side route /api/waitlist/signup verifies the wallet signature -- BEFORE inserting, so RLS-allowed inserts are gated on real ownership. -- - SELECT is denied to anon (privacy: don't leak the email-list-equivalent). @@ -216,12 +218,33 @@ alter table public.waitlist enable row level security; drop policy if exists "anon insert" on public.waitlist; drop policy if exists "deny select" on public.waitlist; +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 with check (true); -- Anon cannot read individual rows. Intentionally no select policy → @@ -325,13 +348,16 @@ grant execute on function public.waitlist_referral_code_exists(text) to anon; -- WHY IT'S TAMPER-RESISTANT: -- • referred_by_code is set at INSERT time by the signup route. The route -- never UPDATEs it afterwards. --- • The RLS policy on the waitlist table grants anon INSERT only — no --- UPDATE or DELETE policy exists, so anon cannot mutate the column. +-- • The RLS policy on the waitlist table grants INSERT to service_role only +-- (GH#2503) — anon holds no write policy at all, and no UPDATE or DELETE +-- policy exists for any role, so the column cannot be mutated after insert. -- • This function reads `count(*)` of rows that reference each code; an -- attacker would need INSERT access to a row with a chosen --- referred_by_code (which they have — but their inserted row is then --- subject to the same RLS, and the route validates `referred_by_code` --- points at a real code before accepting). They cannot decrement, edit +-- referred_by_code. Before GH#2503 they had exactly that: this rationale +-- leaned on "the route validates referred_by_code", but the route was not +-- the only write path — a publishable-key INSERT never reached it. With +-- writes scoped to service_role the route IS the only path, so that +-- sentence is now true rather than assumed. They cannot decrement, edit -- ownership of, or hide existing referrals. -- • Service role bypasses RLS but the service-role key lives in operator -- env (Vercel project + local .env), not in the browser bundle.