The rate limiter on the two anonymous endpoints fails open under concurrency, and keys on a header the caller controls #1237

Status 2026-09-24. Defect 1 is fixed in #1245 stage A1, which is live on production (applied 2026-09-24 00:39Z and measured): SKIP LOCKED is gone, and a new consume_rate_limit increments and decides in one statement (twenty concurrent consumes allow exactly five). One correction to the diagnosis below, found by writing the red test: the bug does not show while the lock is held — the reset path's ON CONFLICT waits for the row too — it shows when the lock is released, when the caller that already chose "no history" wakes, zeroes the count and answers allowed: true. Reproduced end to end on main's migration.

Both halves settled on production, 2026-09-24.

• Defect 1 is fixed and live end to end. The SQL (A1) and both Edge Functions (Limiter and oracle definers were anon-callable: A1+A2 fixed and live; A3 (Edge Functions to consume_rate_limit) and A4 (final revoke) remain #1245 A3, merged in fix(#1245, #1237): the Edge Functions count and decide in one call (stage A3) #1291, deployed as create-lead v9 and contact-message v12 from 587ae2f1) now make one consume_rate_limit call before the side effect. Measured with the concurrent probe this issue asks for: after one earlier attempt, 8 concurrent create-lead POSTs answered exactly 4×409 then 4×429 — five allowed in total, the rest refused. The old check-then-record would have admitted all eight. The probe reused an existing lead's id, so no lead was written (1 before, 1 after), and its limiter row was deleted afterwards.
• Defect 2 does not reproduce on Supabase's edge. Measured on the same probe: a request carrying X-Forwarded-For: 203.0.113.9 (TEST-NET) was keyed on this machine's real public IPv4, not on the spoofed value and not on any internal or shared address. The platform puts the true client address first, so trusting entry [0] is correct there and a caller cannot choose their bucket with that header. What is still wrong is the code comment's reasoning ("taking the last would let a caller prepend…"): the entry is trustworthy because the edge sets it, not because of which end a client can reach. That correction is the last thing here.

check_rate_limit is the only gate in front of two anonymous service-role endpoints, and it does not hold. Two independent defects, either one sufficient on its own. The first needs no credentials, no header control, and no spoofing — only concurrency.

1. FOR UPDATE SKIP LOCKED turns contention into a free pass, and clears the lockout

supabase/migrations/20251006_complete_monolithic_setup.sql:810-813

SELECT * INTO v_record
FROM rate_limit_attempts
WHERE identifier = p_identifier AND attempt_type = p_attempt_type
FOR UPDATE SKIP LOCKED;

SKIP LOCKED returns no row when a concurrent transaction holds that row's lock. The very next branch reads v_record.id IS NULL as "first attempt in this window" and takes the INSERT path:

ON CONFLICT (identifier, attempt_type) DO UPDATE
  SET window_start = v_now, attempt_count = 0, locked_until = NULL, updated_at = v_now;
RETURN json_build_object('allowed', TRUE, ...);

So a concurrent caller is not merely allowed through — attempt_count is reset to 0 and locked_until is set to NULL, wiping any lockout already in force. The limiter is strictest against sequential callers and absent against parallel ones, which inverts what it is for.

Measured in a throwaway postgres:17 container reproducing the table and function verbatim: 100% pass-through when requests are concurrent, versus the intended 5-per-15-minutes when sequential. No project, database or API was touched.

SKIP LOCKED is correct for a work queue, where skipping a locked row means another worker has it. It is wrong here, where the row is shared state and skipping it means "pretend there is no history". The gate should block on the lock (plain FOR UPDATE), or the whole check-and-increment should be one atomic upsert.

2. The x-forwarded-for trust order is inverted, and the comment arguing for it describes the opposite mechanism

supabase/functions/contact-message/index.ts:65-79, supabase/functions/create-lead/resolve.ts:125-142 (deliberately duplicated, so one fix must land in both)

const fwd = req.headers.get('x-forwarded-for');
if (fwd) {
  const first = fwd.split(',')[0]?.trim();
  if (first) return first;
}
return req.headers.get('cf-connecting-ip') ?? null;

A proxy appends the peer it received from. A caller who supplies X-Forwarded-For: evil therefore produces evil, <real client ip> at the function — the caller's value is first. Taking [0] takes exactly the attacker-controlled entry, and since XFF is always present in production the cf-connecting-ip fallback is unreachable.

The in-repo justification defeats itself. The comment reads:

Taking the last would let a caller prepend their own header and rotate identifiers at will

Prepending puts a value first, not last. The sentence describes the attack that the chosen branch enables and then concludes in favour of that branch. tests/unit/create-lead-resolve.test.ts:127-135 pins the behaviour as correct, so the test suite ratifies it.

A fresh identifier is a fresh bucket and a fresh row (UNIQUE(identifier, attempt_type)), so rotating the header rotates the limit.

One thing I did not close. Whether the Supabase edge appends to a caller-supplied XFF or replaces it outright. Replacing it would neutralise this half. I did not probe, because the only probe that would settle it writes a row to leads on production. It does not change the fix — the platform-supplied header should be preferred over the caller-supplied list regardless — and it does not change this ticket's severity, because defect 1 stands alone.

Note these functions are called at ${NEXT_PUBLIC_SUPABASE_URL}/functions/v1/... (src/lib/leads/record-lead.ts:44, src/utils/email/providers/supabase-resend.ts:29) — *.supabase.co directly. The Cloudflare rules in CLAUDE.md front the Pages site, not these, so cf-connecting-ip is not being set by Cloudflare here at all.

Why nothing else catches it

• No second gate. Both handlers run CORS → method → parse → validate → limiter, and nothing else. No shared secret, no signature, no captcha. Turnstile exists only on the auth forms.
• RLS cannot help. Both build a service-role client explicitly (contact-message/index.ts:83-90, create-lead/index.ts:25-32) and both RPCs are SECURITY DEFINER. create-lead's own header comment says it is the sole writer to leads precisely because anon and authenticated are REVOKEd.
• verify_jwt is false on both. Measured live today against GET /v1/projects/{ref}/functions: 17 of 18 functions are false; only delete-account is true. So not even the public anon key is required. (A config.toml with no [functions] block implies a default of true — the deployed project disagrees with that inference, which is the whole point of An Edge Function deploy has no provenance — a complete but stale tree ships silently, and a 200 is not evidence #1188.)
• Validation bounds shape, never volume, and volume is the entire claim. Nothing is idempotent: omit id and Postgres mints one via gen_random_uuid(); the 409-on-23505 path only fires if the attacker reuses a UUID.

What it costs

create-lead — unbounded anonymous INSERTs into leads. Row content stays constrained (caller-chosen v4 UUID, source from three literals, product_id must be an existing SKU); no PII and no utm_* can be written. The damage is quantity: fabricated clicks poisoning the lead list and every conversion number derived from it, plus one rate_limit_attempts row per distinct identifier. On a free-tier 500 MB database, filling the disk takes the whole project down.

contact-message — mail through Resend on demand. Not an open relay: to: [CONTACT_TO] is a server-side constant at index.ts:209 and nothing from the request body reaches it, so this cannot spam strangers or burn the domain's reputation on non-consenting recipients. What it is: a flood of the owner's own inbox with attacker-written subject (200 chars) and body (5000 chars) and a caller-chosen Reply-To, consuming the shared RESEND_API_KEY quota — the same key send-payment-email/index.ts:9 uses for receipts, so a sustained flood degrades transactional mail. Given #361's suppression and bounce history, quota and sender health are the real exposure.

Low-severity aside, worth fixing in the same pass: clientIp returns an arbitrary string that is passed to p_ip_address INET. A non-INET value raises inside the RPC and both functions answer 503. It only breaks the sender's own request, so it is a robustness bug rather than an availability one, but it is an uncaught error path on the money-adjacent side of the app.

Fix, and why it is not in this ticket

1. Drop SKIP LOCKED, or restructure check-and-increment as a single atomic upsert. Migration change — needs applying to the live project, not merely merging. Production drift: the user_profiles privilege-escalation fix was never applied to the live database #1038 and A blocked user could still send for 19 days: #352's C30 clause was never applied to production #1071 are both cases where a merged migration sat unapplied (3 days, 19 days) while the drift gate printed green.
2. Prefer the platform header over the caller-supplied list; treat XFF as trusted only from the right-hand side. Fix both copies and rewrite tests/unit/create-lead-resolve.test.ts:127-135, which currently pins the defect.
3. Validate the identifier as an INET before passing it, and fail closed.

Not opened as a PR tonight, deliberately. Every part of the fix needs a deploy that merging does not perform — an Edge Function deploy for (2) and (3), a live migration apply for (1) — and landing the code without the deploy reproduces exactly the drift that #1038 and #1071 cost weeks to.

Verification must be a concurrent probe. A sequential test passes today against the broken function and would go on passing against the fix, proving nothing.

Found by an adversarial audit of all 18 Edge Functions. The other 17 gate their callers correctly — sweep-intake-orphans compares a shared secret before any side effect, the subscription functions verify the JWT and check ownership before touching a provider. Three other candidate exposures were raised and refuted.