Skip to content

Add server-authoritative security logging and close rate-limit gaps - #100

Merged
AlvanChow merged 1 commit into
mainfrom
claude/llm-security-audit-cvz9uy
Jul 28, 2026
Merged

Add server-authoritative security logging and close rate-limit gaps#100
AlvanChow merged 1 commit into
mainfrom
claude/llm-security-audit-cvz9uy

Conversation

@AlvanChow

Copy link
Copy Markdown
Owner

Follow-up to a security audit of the codebase. The access-control model held up well — RLS is enabled with correct policies on all ten tables, every RPC re-applies its grants after CREATE OR REPLACE, and share creation derives public copy from a server-owned catalog. The gaps were in observability and in one DoS path introduced by the most recent hardening migration.

1. Security event logging

There was no trustworthy record of anything. Auth events existed only as browser-emitted analytics — suppressible via the opt-out, silently droppable by the limiters, and forgeable by any anon caller (src/utils/analytics.js says so in its own TRUST NOTE). Authorization denials and rate-limit denials were recorded nowhere at all, and rate_limit_log — the only place a client IP ever appeared — is purged after ten minutes.

Adds security_events plus log_security_event(), which writes to two channels on purpose. Most denial paths end in RAISE EXCEPTION, which rolls back a table insert made earlier in the same transaction, and Postgres has no autonomous transactions. So the function also emits RAISE LOG, which reaches the server log immediately and is not rolled back.

Verified directly: a cross-account write attempt leaves 0 table rows and a complete server-log line carrying the attacker's uid, client IP, function, and target user.

Wired into check_rate_limit, upsert_quiz_result, upsert_quiz_results_bulk, respond_connection, and the admin RPCs; successful admin email exports are recorded too. Failed sign-ins belong to GoTrue, so they are surfaced read-only from auth.audit_log_entries rather than duplicated into the public schema. Retention is 90 days — shorter than product analytics, because these rows carry IPs. Surfaced in a new admin Security panel.

2. Worker share path: one shared bucket, and a site-wide 503

worker/index.js calls get_shared_result server-to-server, so Supabase's edge stamped cf-connecting-ip with the Worker's own egress address and every visitor to every share link shared one 120/min bucket. Once 20260723000001 changed the over-limit behaviour from "return empty" to RAISE EXCEPTION, one client at ~2 req/sec could make the Worker return 503 for the whole site. The rate-limit slot is also consumed before the row lookup, so non-existent IDs cost the same.

The Worker now forwards the real visitor IP, honoured only alongside a shared secret and normalised through inet so a leaked secret still cannot mint unlimited buckets. Fail-safe: with no secret configured (the state this PR leaves behind) the headers are not sent and are ignored, and behaviour is identical to today.

Separately, a failed lookup no longer returns 503. It is not proof the share is missing, so the shell is served with generic metadata at 200 and the SPA repeats the lookup from the visitor's own browser. Only a successful empty lookup is still a 404, so the smoke test's expectations hold.

3. Remaining unrated paths

vote_hot_take capped its write branch but not the aggregate that runs on every call — including the p_choice: null hydration the Hot Takes page issues per debate. Now 60/min per IP. list_circle and the admin RPCs had no cap either; now 30/min and 20/min.

4. Privileges

  • request_client_ip relied on the default PUBLIC EXECUTE. Harmless while it only echoed the caller's own IP, but it now compares the edge secret — making it a guess-confirmation oracle. Revoked; every caller is a SECURITY DEFINER function and is unaffected.
  • Supabase's defaults grant ALL on public tables to anon, and RLS is what denies — except for TRUNCATE, which bypasses RLS entirely. Verified on a stock Postgres: as anon, DELETE FROM public.profiles removed 0 rows while TRUNCATE public.profiles succeeded. Not reachable through PostgREST, which never issues TRUNCATE, so latent rather than live — but TRUNCATE and REFERENCES are now revoked and the defaults updated.

5. Error handling

AdminDashboard rendered raw PostgREST error text (SQLSTATE, hints, constraint names); UserMenu, Circle, and AuthNudgeBanner rendered raw GoTrue messages. All now generic. Around 15 console.error calls shipped raw Supabase errors to the production console while 6 other sites correctly gated on DEV — all now route through src/utils/devLog.js, enforced by a no-console ESLint rule.

Verification

All 24 migrations were applied to a scratch Postgres 16 with Supabase's primitives stubbed, and the behaviour above was exercised there: IP forwarding across all four secret states, rate-limit denial recording and dedupe, the rollback/server-log split, RLS isolation per role, and the TRUNCATE finding before and after.

supabase/migrations/migrations.test.js guards the invariants the whole model rests on, since there is no server tier: RLS on every created table, search_path pinned on every SECURITY DEFINER, no surviving USING (true) read policy, and grants restated on the last CREATE OR REPLACE of each function. Two of its assertions were wrong on the first pass — they treated the migration log as a snapshot rather than a history — and fixing them is what surfaced the request_client_ip oracle above.

Lint clean, 361 tests across 42 files, share catalog verified, build succeeds.

Known-failing check, pre-existing

npm audit --omit=dev --audit-level=high fails on react-router GHSA-qwww-vcr4-c8h2. This PR does not touch package.json or package-lock.json — the advisory is pre-existing on main.

It is also not exploitable here. The advisory states it "only affects your application if you are using the unstable RSC APIs", and its predecessor is explicit that declarative mode is not impacted. The fix (PR remix-run/react-router#15311) changes exactly one source file, packages/react-router/lib/rsc/server.rsc.ts, reachable only through the react-server conditional export that a browser SPA bundle never resolves. This app imports nine declarative APIs only — BrowserRouter, Routes, Route, Navigate, Link, MemoryRouter, useNavigate, useLocation, useParams — with no SSR, no RSC, no data router, no loaders or actions.

Clearing it is not a version bump: react-router-dom ends at 7.18.1 with no v8, so the only patched path is migrating to react-router@8.3.0 — a dependency swap plus an import rename across ~32 files, and raising the react/react-dom floors to ^19.2.7 and Node engines to >=22.22.0. That is a separate change from this one and is deliberately not bundled here.

Deploy notes

Apply both migrations before deploying the Worker. The share-proxy secret is optional and off by default — until both halves are set, behaviour is unchanged:

openssl rand -hex 32
# UPDATE public.edge_config SET share_proxy_secret = '<value>';
npx wrangler secret put SHARE_PROXY_SECRET

The Privacy page is updated, since the new log stores IPs for 90 days and the analytics opt-out deliberately does not suppress security records.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PmuxLEfqmMeqBg8gpuXNHa


Generated by Claude Code

Addresses the findings from the security audit.

Security event logging (was: no trustworthy record of anything)
- Auth events existed only as browser-emitted analytics: opt-out-able,
  silently droppable by the limiters, and forgeable by any anon caller.
  Authorization denials and rate-limit denials were recorded nowhere.
- Adds security_events plus log_security_event(), which writes to two
  channels on purpose. Most denial paths end in RAISE EXCEPTION, which
  rolls back a table insert made earlier in the same transaction, so the
  function also emits RAISE LOG — server-log writes are not rolled back.
  Verified: a cross-account write attempt leaves 0 table rows and a full
  server-log line carrying actor uid, client IP, function, and target.
- Wires up denials in check_rate_limit, upsert_quiz_result,
  upsert_quiz_results_bulk, respond_connection, and the admin RPCs, and
  records successful admin email exports.
- Failed sign-ins belong to GoTrue, so they are surfaced read-only from
  auth.audit_log_entries rather than duplicated into the public schema.
- Retention is 90 days, shorter than product analytics, because these
  rows carry IPs. Privacy page updated to say so.

Worker share path (was: one shared bucket, and a site-wide 503)
- The Worker calls get_shared_result server-to-server, so Supabase saw
  the Worker's egress IP and every visitor shared one 120/min bucket.
  Since the over-limit path started raising, one client at ~2 req/sec
  could 503 every share link. The Worker now forwards the visitor IP,
  honoured only alongside a shared secret and normalised through inet.
  Fail-safe: with no secret configured, behaviour is unchanged.
- A failed lookup no longer returns 503. It is not proof the share is
  missing, so the shell is served with generic metadata and the SPA
  repeats the lookup. Only a successful empty lookup is still a 404.

Remaining unrated paths
- vote_hot_take capped its write branch but not the aggregate every call
  runs, including the p_choice: null page hydration. Now 60/min per IP.
- list_circle and the admin RPCs had no cap. Now 30/min and 20/min.

Privileges
- request_client_ip relied on the default PUBLIC EXECUTE. Harmless while
  it only echoed the caller's IP, but it now compares the edge secret,
  making it a guess-confirmation oracle. Revoked; every caller is a
  SECURITY DEFINER function and is unaffected.
- Supabase's defaults grant ALL on public tables to anon, and RLS is what
  denies — except for TRUNCATE, which bypasses RLS entirely. Verified on
  a stock Postgres: as anon, DELETE FROM profiles removed 0 rows while
  TRUNCATE profiles succeeded. Not reachable through PostgREST, but
  TRUNCATE and REFERENCES are now revoked and defaults updated.

Error handling
- AdminDashboard rendered raw PostgREST error text; UserMenu, Circle, and
  AuthNudgeBanner rendered raw GoTrue messages. All now generic.
- ~15 console.error calls shipped raw Supabase errors to the production
  console while 6 other sites correctly gated on DEV. All now route
  through src/utils/devLog.js, enforced by a no-console ESLint rule.

Tests
- Worker: degraded-lookup behaviour and both IP-forwarding branches.
- supabase/migrations/migrations.test.js guards the invariants the whole
  model rests on: RLS on every created table, search_path pinned on every
  SECURITY DEFINER, no surviving USING (true) read policy, and grants
  restated on the last CREATE OR REPLACE of each function. The last two
  caught real issues while being written.

All 24 migrations were applied to a scratch Postgres with Supabase's
primitives stubbed, and the behaviour above was verified there.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PmuxLEfqmMeqBg8gpuXNHa

Copy link
Copy Markdown
Owner Author

CI is red, and it is not this PR. Lint, tests (361 across 42 files), share-catalog verification, and build all passed on the runner; the job failed on the final step, npm audit --omit=dev --audit-level=high, against react-router GHSA-qwww-vcr4-c8h2. This branch does not touch package.json or package-lock.json, so the advisory is pre-existing on main.

One consequence worth surfacing before merge: deploy.yml runs the same audit at step 4, before build and wrangler deploy. So merging this as-is lands the fixes on main but they will not reach production — the deploy job stops at the audit gate. Clearing the advisory is effectively a prerequisite for shipping any of this, not just for a green checkmark.

That said, the advisory is not exploitable in this codebase. It states it "only affects your application if you are using the unstable RSC APIs", and the predecessor advisory (GHSA-h5cw-625j-3rxh) is explicit that declarative mode is unaffected. The fix, remix-run/react-router#15311, changes exactly one source file — packages/react-router/lib/rsc/server.rsc.ts — reachable only through the react-server conditional export that a browser SPA bundle never resolves. This app imports nine declarative APIs and nothing else: BrowserRouter, Routes, Route, Navigate, Link, MemoryRouter, useNavigate, useLocation, useParams. No SSR, no RSC, no data router, no loaders or actions.

Clearing it is a migration rather than a bump: react-router-dom ends at 7.18.1 with no v8 line, so the only patched path is react-router@8.3.0 — swap the dependency, rename the import in ~32 files, and raise the react/react-dom floors to ^19.2.7 (already resolving to 19.2.8) plus engines.node to >=22.22.0. None of the nine APIs above have a breaking change in v8.

Holding here rather than bundling a router major into a security PR — the two have different risk profiles and, if the upgrade misbehaves in production, you want to revert it without reverting the security fixes.


Generated by Claude Code

@AlvanChow
AlvanChow merged commit b0b3f2c into main Jul 28, 2026
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants