Skip to content

fix(security): lock users privileged columns + pgTAP policy tests in CI - #38

Merged
Hanu2908 merged 3 commits into
mainfrom
fix/lock-users-columns-and-db-tests
Oct 11, 2026
Merged

Hanu2908 merged 3 commits into
mainfrom
fix/lock-users-columns-and-db-tests

Conversation

@Hanu2908

@Hanu2908 Hanu2908 commented Oct 11, 2026 •

Copy link
Copy Markdown
Owner

Summary

Any student could make themselves a developer from the browser console:

await supabase.from('users').update({ is_developer: true }).eq('id', myId)

That passes requireDeveloper() in edge functions. get-analytics-summary then uses the service role key to return names and emails for any sectionId. The UPDATE policy only pinned role and section_id.

The fix replaces "block bad columns" with "allow good columns":

-GRANT INSERT, UPDATE ON public.users TO anon, authenticated   -- Supabase default
+REVOKE INSERT, UPDATE ON public.users FROM anon, authenticated;
+GRANT UPDATE (day_scholar, sub_batch, phone, branch, notifications_enabled)
+  ON public.users TO authenticated;

All 12 database functions that write users (create/join hub, CR promote/transfer, etc.) are SECURITY DEFINER, so they're unaffected. The client writes only those five columns.

Pipeline change:

 PR → main
   lint, typecheck, test, build
+  db-tests: supabase start → supabase test db
 push to main (supabase/**)
+  db-tests
-  deploy            (cancel-in-progress: true)
+  deploy needs db-tests   (cancel-in-progress: false)
supabase/
├── migrations/20261011120000_lock_users_privileged_columns.sql
└── tests/
    ├── users_privileges.test.sql    # 12 checks: role, section, is_developer, cr_rank, roll, insert, anon
    ├── section_isolation.test.sql   # 6 checks: members + announcements across sections
    └── poll_anonymity.test.sql      # 8 checks: general votes carry no student_id, CR can't see them

Evidence

  • Before: rls.test.sql was SELECT 1. No policy tests ran anywhere, and migrations went straight to prod.
    After: see the Database Policy Tests check on this PR. This is the first time all migrations are applied to an empty database. If supabase start fails, a migration depends on something created by hand in the dashboard, and that needs fixing before merge.
  • Regression guard: users_privileges.test.sql check 1 (update users set is_developer = true → expects 42501) would fail against the current prod schema.

Found by the first CI runs

  1. Migration replay (2eca65e). 20260606000000 recreated four announcement_comments policies without dropping them first, so an empty DB couldn't be built. I added DROP POLICY IF EXISTS. Both definitions are identical, so prod is unchanged.
  2. Recursive users UPDATE policy (a9f81fe). The 20260909 policy selects from users inside a users policy. Every client UPDATE on users fails with 42P17 infinite recursion (the same bug 20260524105400 fixed once). This is probably live in prod since Sept 9: the notification toggle, profile preferences and the onboarding phone/batch step. The policy is now a plain own-row check, and the column grant handles which columns are writable.

Merge Danger

Door: two-way

Rolling back means re-granting INSERT, UPDATE ON public.users TO authenticated. No data changes.

Blast Radius: profile-writes

If the client writes a users column that isn't in the allowlist, that write fails with "permission denied". I grepped src/ and found only those five columns. Before merging, run select id, email from users where is_developer; in prod to see whether the hole was already used.

🤖 Generated with Claude Code

Any student could run `update users set is_developer = true` on their own
row and pass requireDeveloper() in edge functions, which read names and
emails of any section with the service role key. The UPDATE policy pinned
role and section_id only.

- Revoke INSERT/UPDATE on users from anon and authenticated; grant UPDATE
  back on the five columns the client writes. Enrollment and CR changes
  use SECURITY DEFINER RPCs and are unaffected.
- Add pgTAP tests in supabase/tests: privileged columns, cross-section
  isolation, general-poll anonymity.
- CI: new db-tests job runs `supabase start` + `supabase test db`.
- CD: deploy now needs db-tests; stop cancelling an in-progress db push.
- Remove the SELECT 1 placeholder RLS test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@vercel

vercel Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
class-hub Ready Ready Preview Oct 11, 2026 9:27am UTC

@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: f23545f4-82df-4e70-a755-110498f2b5fa

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Hanu2908 and others added 2 commits October 11, 2026 14:51
20260606000000 recreated the four announcement_comments policies that
20260601000000 already created, without dropping them first. Prod applied
it fine, but `supabase start` on an empty DB fails with 42710 (policy
already exists). Both definitions are identical, so dropping first leaves
the final state unchanged; prod won't re-run this migration.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The 20260909 policy selected from public.users inside a users policy, so
every client UPDATE on users fails with 42P17 (infinite recursion), the
same bug 20260524105400 fixed once before. pgTAP caught it on the first
real run.

The column grant now makes role/section_id unwritable, so the policy only
pins the row to its owner.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Hanu2908
Hanu2908 merged commit 905af3e into main Oct 11, 2026
10 checks passed

This branch was successfully deployed

1 active deployment
Preview — a9f81fef Deployed Oct 11, 2026 by vercel[bot]
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.

1 participant