Skip to content

Disclose database migrations and Supabase config#130

Open
hoogvliets wants to merge 1 commit into
willchen96:mainfrom
hoogvliets:disclose/database-migrations
Open

Disclose database migrations and Supabase config#130
hoogvliets wants to merge 1 commit into
willchen96:mainfrom
hoogvliets:disclose/database-migrations

Conversation

@hoogvliets

@hoogvliets hoogvliets commented May 14, 2026

Copy link
Copy Markdown

Part of the downstream AGPL disclosure. This PR isolates database migration and Supabase configuration changes for easier review.

Scope

  • Database schema management, migration files, one-shot schema, Supabase local config, and package/env updates needed for migrations.
  • Full disclosure PR: Disclose downstream AGPL changes #127.

Migration System

  • Adds node-pg-migrate scripts for migrating up, rolling back one migration, and creating new migrations.
  • Adds migration dependencies and lockfile updates.
  • Replaces the previous monolithic backend/schema.sql setup path.

Schema Changes

  • Adds baseline migration scaffolding.
  • Adds auth user lookup RPC support.
  • Adds PDF conversion status fields.
  • Adds UUID foreign key and billing cleanup changes.
  • Adds review document count selection helpers.

Security and Account Lifecycle

  • Adds RLS policies for cross-tenant protection.
  • Adds workflow sharing checks.
  • Adds encrypted API key schema support.
  • Adds soft-delete user profile support.
  • Adds account deletion job schema support.

Fresh Database Setup

  • Adds backend/migrations/000_one_shot_schema.sql for fresh Supabase databases.
  • Adds supabase/config.toml and supabase/.gitignore for local Supabase CLI workflows.

Environment Updates

  • Documents direct DATABASE_URL usage for migration execution.
  • Adds env slots for anon-key testing, encryption, restore tokens, and rate limiting.

Review Note

@bmersereau

Copy link
Copy Markdown

Note: RLS strategy conflict with PR #145

This PR proposes fine-grained per-user RLS policies for authenticated (owner-only mutations, share-aware SELECTs via SECURITY DEFINER helpers). PR #145 takes the opposite approach — deny-all for anon and authenticated, with all data access going through the backend service role exclusively.

These are philosophically incompatible. In Postgres, permissive policies combine with OR, so if both land, this PR's allow-policies would silently nullify #145's deny-all for authenticated users (and vice versa — #145's deny-all becomes dead weight if this PR's policies are present).

Current state on main: RLS is already enabled (with no policies, so effectively deny-all) on user_api_keys, courtlistener_citation_index, and courtlistener_opinion_cluster_index. No other tables have RLS. Neither PR has landed yet.

The existing codebase uses no frontend direct Supabase queries — all data access goes through the backend API via service role. This means:

A decision on which model to adopt should be made before either PR is merged to avoid silent policy conflicts.

@bmersereau

Copy link
Copy Markdown

PR Review: Disclose database migrations and Supabase config

Summary

This PR discloses a batch of database migrations (baseline through account-deletion-jobs) and Supabase config, as part of the downstream AGPL disclosure series. The centrepiece security change is 0005_rls_policies.ts — 638 lines of fine-grained RLS policies for 14 tables with 5 SECURITY DEFINER helper functions. The approach is technically well-considered, but two blockers need resolution before this can merge.

Risk Assessment

  • Risk Level: High
  • Blast Radius: 17 files, 4937 additions — touches schema, all migrations, package deps, Supabase config. Any bug in the RLS migration could lock out legitimate authenticated queries or silently break cross-tenant isolation.
  • Rollback Plan: down() function present in 0005_rls_policies.ts.

Branch Health


Review by Category

Architecture — RLS Strategy Conflict with PR #145

  • [severity:blocker] Incompatible RLS strategy

    PR fix: enable RLS with deny-all policy on all public tables #145 (open, rebased on main) takes a deny-all approach — USING (false) for anon and authenticated, all data access via service role. This PR takes the opposite approach — fine-grained per-user policies that allow authenticated to read/write their own rows directly.

    In Postgres, permissive policies combine with OR. If both PRs land, this PR's allow-policies silently nullify fix: enable RLS with deny-all policy on all public tables #145's deny-all for authenticated users. Neither would hard-error, but one approach defeats the other.

    Current architecture reality: The frontend uses Supabase only for auth operations (sign-in/out, MFA). All data queries go through the backend API via service role — no frontend component issues direct table queries. This matches fix: enable RLS with deny-all policy on all public tables #145's deny-all model, not this PR's per-user policy model.

    Required action: Agree on one strategy before either PR merges. If the intent is to eventually enable direct PostgREST access from the frontend, document that architectural decision here.

Security

  • [severity:praise] Helper functions use language sql (not plpgsql) — correctly allows the planner to inline them and push @> filters down into GIN indexes. The comment explaining this is valuable.
  • [severity:praise] All helpers use SECURITY DEFINER + set search_path = public — correctly locked down.
  • [severity:praise] with check clauses on all mutation policies — write wall is explicit.
  • [severity:major] is_workflow_visible relies on lower(auth.email()) cross-table join without a functional index on workflow_shares.shared_with_email — queries filtering by email against workflow_shares will table-scan unless lower(shared_with_email) is indexed. Under load this becomes an N-scan per authenticated request. A CREATE INDEX ... ON workflow_shares (lower(shared_with_email)) should accompany 0005.
  • [severity:minor] is_project_member stores shared emails as lowercase in shared_with JSONB (jsonb_build_array(lower(auth.email()))), but the projects_insert_owner / projects_update_owner policies don't validate that any emails added to shared_with are lowercase-normalised. A client could insert shared_with = '["User@Example.com"]' and the membership check would silently miss them.

Correctness

  • [severity:major] CourtListener tables not covered
    courtlistener_citation_index and courtlistener_opinion_cluster_index (added by PR Add courtlistener intergration, liquid glass redesign, UI improvements, version control, various fixes #171, which this branch is missing) are read-only lookup tables, but they exist in the public schema and are unaddressed by this migration. Post-merge they'd have no RLS.

  • [severity:minor] 0005_rls_policies.ts down() drops is_document_member before document_edits_select_member policy — the policy references the function, but since DROP POLICY doesn't have a dependency on the function, this is fine at the SQL level. No actual bug, but the rollback order comment implies otherwise.

PR Size

  • [severity:major] 4937 additions across 17 files is too large to review safely in one pass. The migration system setup (migrations 0000–0004, 0006–0009), the Supabase config, and the security-critical 0005_rls_policies.ts should be separate PRs. The disclosure context is understood, but a reviewer cannot confidently sign off on security-critical RLS logic buried in a 5k-line diff.

PostgreSQL Checklist

  • ✅ Idempotent up/down migrations
  • ✅ GIN indexes (jsonb_path_ops) added for @> queries
  • ✅ SECURITY DEFINER + search_path on all helpers
  • ⚠️ Missing functional index on lower(workflow_shares.shared_with_email) (see above)
  • with check clauses on all insert/update policies

Issue Lifecycle


Verdict

What I Verified

  • Checked branch staleness — 16 commits behind main
  • Read 0005_rls_policies.ts in full (638 lines via GitHub API)
  • Verified CourtListener tables were added after this branch diverged
  • Checked workflow_shares for email index gap
  • Confirmed frontend has no direct table queries (auth-only Supabase usage)
  • Reviewed interaction with PR fix: enable RLS with deny-all policy on all public tables #145

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.

3 participants