Skip to content

fix: enable RLS with deny-all policy on all public tables#145

Open
bmersereau wants to merge 3 commits into
willchen96:mainfrom
bmersereau:fix/144-enable-rls-on-public-tables
Open

fix: enable RLS with deny-all policy on all public tables#145
bmersereau wants to merge 3 commits into
willchen96:mainfrom
bmersereau:fix/144-enable-rls-on-public-tables

Conversation

@bmersereau
Copy link
Copy Markdown

Summary

Adds Row Level Security as a defense-in-depth second wall against accidental GRANT statements on application tables.

  • Appends a DO $$ ... END$$ block to backend/schema.sql that, for every public base table, runs ALTER TABLE ... ENABLE ROW LEVEL SECURITY and creates a deny_client_access_<tbl> policy with USING (false) WITH CHECK (false) for anon, authenticated.
  • Ships the same block as backend/migrations/20260516_enable_rls_deny_all.sql so existing deployments can apply it incrementally (matches the convention from prior migration commits).
  • Adds backend/scripts/verify-rls.sql — a psql-runnable assertion script that fails non-zero if any public table is missing RLS or the deny-all policy.

Closes #144

Changes

File Change
backend/schema.sql +41 — append RLS + deny-all policy block at end of file
backend/migrations/20260516_enable_rls_deny_all.sql +36 — new incremental migration with the same block
backend/scripts/verify-rls.sql +57 — verification harness asserting RLS + policy on every public table

Why this matters

The current authorization model is a single REVOKE ALL ... FROM anon, authenticated block. One accidental GRANT SELECT ON public.documents TO authenticated in a future migration, hotfix, or Supabase dashboard click undoes it for that table with zero pushback. RLS with a using (false) deny-all policy is a second wall: even if a grant lands, the policy still blocks the row.

The service role bypasses RLS, so the backend (createServerSupabase() uses SUPABASE_SECRET_KEY) is unaffected.

Test plan

  • Backend tsc build passes locally.
  • Schema block is idempotent (re-runs are no-ops thanks to the if not exists guard on policy creation; enable row level security is naturally idempotent).
  • backend/scripts/verify-rls.sql runs cleanly against the post-migration schema. On a database without the migration it raises verify-rls: N assertion(s) failed.
  • Reviewer to apply the migration on a Supabase staging instance and confirm:
    • The verify script exits clean.
    • The backend continues to serve normal API traffic (service role bypasses RLS).
    • A direct PostgREST call from the browser anon client to e.g. from('documents').select('*') returns no rows (or fails) — confirming the second wall.

Notes for reviewers

  • The frontend eslint and next build failures on this branch reproduce identically on origin/main — they are pre-existing tooling debt (missing build-time env vars, accumulated lint errors), not regressions from this PR. Backend tsc is the only gate that applies to a SQL-only change and it is clean.
  • npm audit reports unrelated high/moderate transitive CVEs (@xmldom/xmldom, protobufjs, postcss) — out of scope here; tracked in this codebase's security audit.

- Add event trigger enforce_rls_on_public_tables so any future CREATE TABLE
  in public automatically gets RLS + deny-all policy. SECURITY DEFINER,
  covers both regular and partitioned tables (relkind 'r','p').
- Add 20260516_enable_rls_deny_all.down.sql rollback that drops the policy,
  function, event trigger, and disables RLS.
- Tighten verify-rls.sql: also assert with_check (write wall), accept both
  'false' and '(false)' for Postgres-rendering tolerance, cleaner pg_class
  join through pg_namespace.
- Document the convention in CONTRIBUTING.md: Database Migrations section
  with rollback expectation, RLS policy expectation, and verify-rls command.
@bmersereau
Copy link
Copy Markdown
Author

Updated based on self-review (commit ea8a44b). Five changes:

verify-rls.sql — tightened assertions

  • Now also asserts pg_policies.with_check (the write wall — previous script only checked qual, so a policy with USING (false) WITH CHECK (true) would have passed while permitting inserts).
  • Accepts both 'false' and '(false)' for the qual/with_check text comparison, so a future Postgres expression-rendering change doesn't produce false negatives.
  • Cleaner pg_class join via pg_namespace so a same-named table in another schema can never accidentally match.

Rollback migration — backend/migrations/20260516_enable_rls_deny_all.down.sql

  • Drops the deny-all policies, the event-trigger function, and the event trigger; disables RLS on every public base table. Idempotent.
  • The REVOKE statements in schema.sql remain in force, so the rollback removes the second wall only — clients still cannot reach these tables via PostgREST.

Event trigger — enforce_rls_on_public_tables

  • Fires on ddl_command_end when tag in ('CREATE TABLE'). For any new table in the public schema, it auto-runs ENABLE ROW LEVEL SECURITY and creates the matching deny_client_access_<tbl> policy.
  • SECURITY DEFINER + set search_path = public so it runs as its owner (postgres) and has the privileges to ALTER/CREATE POLICY on the new table inside the same transaction.
  • Covers relkind in ('r', 'p') — regular and partitioned tables. Temp tables (different schema) and views/materialized views (different relkind) are correctly excluded.
  • Eliminates the "developer forgot to add RLS" foot-gun raised in the review.

CONTRIBUTING.md — new "Database Migrations" section

  • Documents the schema.sql / migrations directory split, the paired .down.sql rollback expectation, and the RLS-by-default convention.
  • Notes that the event trigger handles this automatically in normal flow, and points to the verify-rls.sql smoke command for post-migration checks.

Suggested staging-side verification step added to the rollout list

  • CREATE TABLE public.test_evt_trigger (id int); then assert pg_class.relrowsecurity = true and one row in pg_policies for test_evt_trigger. Drop the table after. Confirms the event trigger fires.

Pre-existing items not addressed in this PR (intentional, out of scope):

  • handle_new_user trigger functional test under RLS — staging-only verification, no code change needed (function is SECURITY DEFINER; loadProfile repairs missing rows via service role as a fallback).
  • DO-block duplication between schema.sql and the migration — preserved per the existing convention in the schema.sql header comment.

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.

[Security][Critical] No Row Level Security on any application table — one accidental GRANT = total multi-tenant data breach

1 participant