security(db): enable RLS on all public tables - #131
Open
jon-bell wants to merge 4 commits into
Open
Conversation
Supabase's PostgREST auto-exposes every table in the `public` schema over HTTP, authorized by the public anon key that ships to every browser. With RLS disabled, anyone holding that key could read or modify all rows (workspaces, prompts, documents, prolific study IDs, and the orphaned `generations` table). This migration closes that hole. - Enables RLS on every base table in `public` via a loop, so current and future orphan tables (e.g. `generations`, dropped from the Drizzle schema when the generation panel was removed) are locked to default-deny rather than silently exposed. - Adds owner-scoped `authenticated` policies to the known application tables, rooted at `workspaces.user_id = auth.uid()::text`; children inherit via `workspace_id` or `chart_id -> charts -> workspace`. - `workshops` and orphan tables get RLS with no policy (server/admin only). - No FORCE, so the `postgres` (Drizzle) and `service_role` (tests) roles keep bypassing — the app and test suite are unaffected. Verified against real Postgres (PGlite): anon sees nothing, an authenticated user sees only their own rows, cross-owner writes are blocked, service_role bypasses, and rowsecurity is on for all tables. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughChangesThe change replaces caller-supplied user IDs with server-derived identity checks across workspace and owned-data operations, adds development identity support and authorization tests, updates lens-run creation, and enables ownership-based RLS policies across public Supabase tables. Ownership authorization and RLS
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@supabase/migrations/20260716192819_enable_rls.sql`:
- Around line 132-145: Update the RLS policy conditions for lens_runs in both
using and with check to validate that the referenced chart_id belongs to the
same workspace_id as the run, and that this workspace is owned by auth.uid().
Preserve the existing ownership validation while adding the chart/workspace
consistency check.
- Around line 33-40: Update the orphan-table protection around the migration’s
“ORPHAN TABLES” logic so newly created public tables are also prevented from
remaining without RLS. Add an appropriate event trigger that enables RLS for
future base tables in public, or add a CI validation that rejects public tables
lacking RLS; ensure the existing migration-time coverage remains intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a4fa5f9-95b7-4ff5-babb-ad6c24dad95c
📒 Files selected for processing (1)
supabase/migrations/20260716192819_enable_rls.sql
Comment on lines
+33
to
+40
| -- ORPHAN TABLES | ||
| -- Some tables exist in the live DB but not in the Drizzle schema (e.g. | ||
| -- `generations`, left over from the removed generation panel). PostgREST | ||
| -- exposes those too. So rather than enable RLS on a hand-listed set, we enable | ||
| -- it on EVERY base table in `public` — this self-heals against current and | ||
| -- future orphans, locking them to default-deny (bypass roles only) until | ||
| -- someone gives them an explicit policy. The owner-scoped policies below then | ||
| -- layer onto the known application tables. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the migration and nearby related files first.
git ls-files 'supabase/migrations/*' 'supabase/**' | sed -n '1,200p'
echo
echo '--- migration outline ---'
ast-grep outline supabase/migrations/20260716192819_enable_rls.sql --view expanded || true
echo
echo '--- related references to RLS / default privileges / future tables ---'
rg -n --hidden --glob '!*node_modules*' --glob '!*dist*' --glob '!*build*' \
-e 'enable row level security|row level security|default privileges|revoke .*anon|revoke .*authenticated|event trigger|future orphans|ORPHAN TABLES|policy' \
supabase .github . 2>/dev/null | sed -n '1,240p'
echo
echo '--- target migration with line numbers ---'
cat -n supabase/migrations/20260716192819_enable_rls.sql | sed -n '1,220p'Repository: ndif-team/workbench
Length of output: 17129
Future tables still need explicit RLS. The loop only enables RLS on tables that exist when this migration runs; any later CREATE TABLE public.* will still start without RLS, so the “future orphans” claim is too broad. Add an event trigger or a CI check that rejects new public tables without RLS.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260716192819_enable_rls.sql` around lines 33 - 40,
Update the orphan-table protection around the migration’s “ORPHAN TABLES” logic
so newly created public tables are also prevented from remaining without RLS.
Add an appropriate event trigger that enables RLS for future base tables in
public, or add a CI validation that rejects public tables lacking RLS; ensure
the existing migration-time coverage remains intact.
Contributor
|
🚀 Preview deployed
|
The RLS work runs Drizzle as the postgres role (bypasses RLS), so authorization for the app path lives in the server actions. This converts the query RPCs to derive the caller from the session (requireUserId / requireWorkspaceOwner / requireChartOwner) instead of trusting a client-supplied userId, and closes the gaps found in review: - workspaceQueries: getWorkspaces / updateWorkspace / deleteWorkspace / createWorkspace now derive identity from the session; drop the client-supplied userId args and the dead prop/state plumbing that fed them (WorkspaceList, CreateWorkspaceDialog, AutoWorkspaceCreator, WorkspaceNameEditor, ModelLaunchDialog, LandingPage, page.tsx, workspaceApi hooks). setWorkspaceProlificIfEmpty now requires workspace ownership — previously it had no authorization at all, allowing anyone to stamp Prolific attribution onto any workspace. - setConfig: only data/type are mutable; never rewrite workspaceId, so an owned config can't be relocated into another workspace. - updateView: a zero-row update is a benign no-op (returns undefined), not a ForbiddenError — the ownedByChart predicate already blocks cross-user writes, and the debounced autosave races legitimately with clear/reset, which previously surfaced a spurious error toast. - copyChart: single ownership-scoped full-row fetch (drop the redundant requireChartOwner + re-select round-trip). - pushTutorialChart: drop the redundant pre-flight ownership check (the create helpers already guard before any write). - lensRunQueries: drop the vestigial CreateLensRunInput.workspaceId (the stored value is derived from requireChartOwner). - migration: drop the redundant workshops RLS ALTER (the do-loop already enables it); keep the deny-by-default intent comment. - ownership: note the per-RPC getUser() round-trip as a follow-up. Tests: convert the DB suites to the session-derived signatures (switch identity via setDevUserId for cross-user seeding), rewrite the wrong-user update/delete tests, and add ownership-rejection coverage for getWorkspaces / createWorkspace / setWorkspaceProlificIfEmpty. 76/76 pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Formatting-only follow-up to satisfy the Run Prettier CI gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workbench/_web/src/app/workbench/components/WorkspaceList.tsx`:
- Around line 122-127: Replace the hardcoded ["workspaces"] queryKey in
WorkspaceList with the centralized queryKeys.workspaces.all key, adding the
queryKeys import if needed. Ensure the corresponding key is defined in
src/lib/queryKeys.ts if it does not already exist.
In `@workbench/_web/src/db/__tests__/lensRuns.test.ts`:
- Around line 84-96: Add cross-user fixtures and tests in the lens-run test
setup around createLensRun, using a separate attacker user and attacker-owned
chart. Verify createLensRun rejects the victim’s chart, and that fetch, patch,
clear, and delete operations cannot access runs belonging to that chart; retain
the existing owner-path coverage unchanged.
In `@workbench/_web/src/lib/auth/ownership.ts`:
- Around line 44-49: Update requireUserId so NEXT_PUBLIC_DISABLE_AUTH=true is
honored only in non-production environments; in production, reject the
configuration instead of importing getDevUserId or returning a synthetic
identity. Prefer a server-only environment variable for this development bypass
while preserving normal session authentication and the existing devUser lazy
import behavior.
In `@workbench/_web/src/lib/queries/configQueries.ts`:
- Around line 37-40: Enforce workspace ownership for chart/config links: in
addChartConfigLink, reuse the workspace returned by requireChartOwner(chartId)
and verify configId belongs to that workspace before inserting. In
workbench/_web/src/lib/queries/configQueries.ts lines 37-40, add the validation;
in workbench/_web/src/lib/queries/chartQueries.ts lines 268-282, scope
downstream config lookup to the authorized chart workspace, reject mismatched
links, and audit existing links created by the unguarded endpoint.
In `@workbench/_web/src/lib/queries/internal.ts`:
- Around line 25-37: Update getNextWorkspaceItemPosition and the corresponding
chart/document creation flow so position allocation and insertion are serialized
atomically, using a transaction with appropriate locking or a database-managed
workspace counter with conflict retry. Ensure concurrent creations cannot
receive the same position across charts and documents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1af1e6e8-78b3-404f-aa24-b153ead2cfa5
📒 Files selected for processing (25)
supabase/migrations/20260716192819_enable_rls.sqlworkbench/_web/src/actions/workshop.tsworkbench/_web/src/app/workbench/components/AutoWorkspaceCreator.tsxworkbench/_web/src/app/workbench/components/WorkspaceList.tsxworkbench/_web/src/app/workbench/page.tsxworkbench/_web/src/components/CreateWorkspaceDialog.tsxworkbench/_web/src/components/LandingPage.tsxworkbench/_web/src/components/WorkspaceNameEditor.tsxworkbench/_web/src/components/models/ModelLaunchDialog.tsxworkbench/_web/src/db/__tests__/lensRuns.test.tsworkbench/_web/src/db/__tests__/local-db.test.tsworkbench/_web/src/db/__tests__/prolific.test.tsworkbench/_web/src/db/__tests__/workshops.test.tsworkbench/_web/src/lib/api/patchLensApi.tsworkbench/_web/src/lib/api/workspaceApi.tsworkbench/_web/src/lib/auth/devUser.tsworkbench/_web/src/lib/auth/ownership.tsworkbench/_web/src/lib/queries/chartQueries.tsworkbench/_web/src/lib/queries/configQueries.tsworkbench/_web/src/lib/queries/documentQueries.tsworkbench/_web/src/lib/queries/internal.tsworkbench/_web/src/lib/queries/lensRunQueries.tsworkbench/_web/src/lib/queries/tutorialChart.tsworkbench/_web/src/lib/queries/viewQueries.tsworkbench/_web/src/lib/queries/workspaceQueries.ts
💤 Files with no reviewable changes (1)
- workbench/_web/src/lib/api/patchLensApi.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- supabase/migrations/20260716192819_enable_rls.sql
…-closed) Follow-up hardening from the PR #131 review pass: - ownership: requireUserId() fails closed in production — a build with NEXT_PUBLIC_DISABLE_AUTH=true throws instead of handing every RPC the synthetic dev identity. - addChartConfigLink: verify the config lives in the chart's owned workspace (chart ownership alone didn't establish config ownership, letting a caller link — and via copyChart, read — another tenant's config). copyChart's config lookup is likewise scoped to the chart's workspace and refuses a cross-workspace link. - RLS: tighten the lens_runs policy so chart_id and workspace_id must resolve to the same owned workspace, and the chart_config_links policy so the config shares the chart's owned workspace (defense-in-depth behind the app-layer checks above). - internal.ts: allocate the next list position inside the INSERT via a portable subquery instead of a separate read-then-write, removing the app-level window where concurrent creates collide on position. - WorkspaceList / workspaceApi: use the centralized queryKeys.workspaces.all instead of a hardcoded ["workspaces"] key. Tests: add lens-run cross-user coverage (createLensRun rejects another user's chart; list / heatmap / patch / clear / delete can't reach that chart's runs). 79/79 DB tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supabase's PostgREST auto-exposes every table in the
publicschema over HTTP, authorized by the public anon key that ships to every browser. With RLS disabled, anyone holding that key could read or modify all rows. This migration closes that hole.publicvia a loop, so current and future orphan tables (e.g.generations, dropped from the Drizzle schema when the generation panel was removed) are locked to default-deny rather than silently exposed.authenticatedpolicies to the known application tables, rooted atworkspaces.user_id = auth.uid()::text; children inherit viaworkspace_idorchart_id -> charts -> workspace.workshopsand orphan tables get RLS with no policy (server/admin only).postgres(Drizzle) andservice_role(tests) roles keep bypassing — the app and test suite are unaffected.Due to the nature of this change, this is currently deployed.
Summary by CodeRabbit