Skip to content

Remove the unused custom oauth_states CSRF layer (dead code + public-write RLS) #183

Description

@TortoiseWolfe

Summary

The app ships a custom OAuth-state CSRF layer (src/lib/auth/oauth-state.ts + an oauth_states table) that has zero production callers. The real CSRF defense is Supabase's built-in PKCE state — both OAuth entry points say so in comments. The custom layer is therefore dead code, and worse, its table carries fully-public RLS (WITH CHECK (true) / USING (true) on INSERT/SELECT/UPDATE) — live attack surface (anyone with the anon key can flood, read, or overwrite rows) for a table nothing consumes.

Recommendation: delete the full surface. Surfaced by the systematic code review in #181 (deliberately not done there — touches the migration + prod DDL, bigger than a review-scoped change).

Evidence it's unused

  • Zero production importers of src/lib/auth/oauth-state.ts — the only importer is its own test file (verified: grep for generateOAuthState/validateOAuthState/oauth-state across src/ returns only the module + its __tests__).
  • src/components/auth/OAuthButtons/OAuthButtons.tsx:29 — no import; comment: "Supabase handles CSRF protection via built-in state parameter (PKCE flow) — No need to manually manage state tokens". Calls supabase.auth.signInWithOAuth({...}) with no manual state.
  • src/app/auth/callback/page.tsx:36 — no import; comment: "Supabase handles state validation internally - no manual check needed". Never validates a state token.

Why it's a liability, not defense-in-depth

  • The oauth_states RLS is fully public (supabase/migrations/20251006_complete_monolithic_setup.sql L340-361): public INSERT (WITH CHECK (true)), public SELECT (USING (true)), public UPDATE (USING (true)), public DELETE-if-expired. Anyone with the anon key can spam/read/overwrite it — pure attack surface for a table with no consumer.
  • The design is also weaker than it looks even if it were wired in: session_id is self-reported from sessionStorage by the same client, and both generateOAuthState/validateOAuthState fail open. So it wouldn't add meaningful protection beyond PKCE.
  • Keep-vs-delete (honest): the only "keep" argument is defense-in-depth if a fork disabled Supabase PKCE — but the layer isn't wired into the flow, so it provides zero protection today, and such a fork would have to re-integrate it from scratch anyway. Keeping dead code "just in case" is latent risk (public-write table), not real defense-in-depth. Net: delete.

Full removal surface (checklist)

  • Module: delete src/lib/auth/oauth-state.ts (214 lines — generateOAuthState, validateOAuthState, cleanupExpiredStates, private helpers generateUUID/getSessionId).
  • Test: delete src/lib/auth/__tests__/oauth-state.test.ts (19 tests / 6 describes; self-contained in-memory mock — no shared-fixture impact).
  • Migration (EDIT THE MONOLITH DIRECTLY — do NOT create a separate migration file, per CLAUDE.md): in supabase/migrations/20251006_complete_monolithic_setup.sql, remove the oauth_states CREATE TABLE (L317-328), 3 indexes (L330-332), 4 RLS policies (L340-361), 2 COMMENTs (L334, L363), and the summary-comment mention (L2324).
  • Prod DDL: apply a live DROP TABLE oauth_states CASCADE; to prod via the Supabase Management API (POST /v1/projects/{ref}/database/query, JSON payload built from a FILE — mirrors the Drop #49 legacy schema leftovers (profiles, audit_logs, handle_new_user, handle_updated_at) #170 legacy-table drop). It holds only transient OAuth tokens (5-min expiry) → safe to drop with no backfill. Verify post-drop that the table + its 4 policies are gone.
  • Reset script: remove the deleteAllFromTable('oauth_states', 'OAuth states') call at scripts/reset-database.ts:274.
  • Generated types: regenerate src/lib/supabase/types.ts (the oauth_states Row/Insert/Update block is L406-444) via GET /v1/projects/{ref}/types/typescript.
  • Teardown: supabase/migrations/999_drop_all_tables.sql:42 (DROP TABLE IF EXISTS oauth_states CASCADE;) — may stay as a harmless no-op or be removed for tidiness.
  • No cleanup fn/trigger/cron to remove — none exists (the table is fully inert; the only "cleanup" was the dead client-side cleanupExpiredStates).

Docs cleanup (recommended — they actively TEACH the removed pattern)

The OAuth blog post advertises the manual generate/validate flow that contradicts the real PKCE code, so it's teaching a dead (and insecure-looking) pattern:

  • public/blog/authentication-supabase-oauth.md (and its compiled copy src/lib/blog/blog-data.json) — update the code samples to the actual signInWithOAuth PKCE flow.
  • Historical spec docs (low priority, mention only): docs/specs/017-security-hardening/ (contracts/oauth-csrf.md, data-model.md, tasks.md, etc.), docs/specs/040-feature-040-code/, features/foundation/005-security-hardening/spec.md:18, scripts/audit/truth-table.json:148.

Verification

  • pnpm type-check + pnpm lint clean after the module/test/type removals.
  • Full test suite green (nothing else imports the module).
  • Prod: confirm oauth_states table + its 4 policies are dropped (query information_schema.tables / pg_policies), and OAuth sign-in still works (PKCE was always the real path).
  • Fresh-install parity: re-running the monolith on a clean DB no longer creates the table.

Estimated: S (small — deletions + one live DROP + type regen + doc touch-ups).


Origin: code review #181. Related follow-up: #182 (finish group message E2E encryption).

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestpriority:p3Low — defer (heavy work, polish, nice-to-haves)

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions