Restrict global_stats access to platform admins#1706
Conversation
📝 WalkthroughWalkthroughThe PR restricts access to public.global_stats from permissive anonymous read to admin-only read access. A database migration updates RLS policies to deny non-admin users, while tests verify the new permission constraints across anonymous, authenticated non-admin, and authenticated admin user roles. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 SQLFluff (4.0.4)supabase/migrations/20260227000000_restrict_global_stats_public_access.sqlUser Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects: 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66390bc97b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,27 @@ | |||
| -- ============================================================================= | |||
There was a problem hiding this comment.
This migration uses the 20260226000000 version prefix, but that prefix is already used by 20260226000000_org_rls_require_self_2fa_update.sql; Supabase migration ordering/history is version-based, so this collision can make one migration unapplied or non-deterministic across environments, which risks leaving global_stats access policy changes out of sync.
Useful? React with 👍 / 👎.
| throws_ok ( | ||
| 'SELECT COUNT(*) FROM public.global_stats', | ||
| '42501', | ||
| 'permission denied', | ||
| 'Authenticated non-admin users should not be able to select from global_stats' |
There was a problem hiding this comment.
This assertion expects authenticated non-admin reads to raise 42501, but the migration grants SELECT to authenticated and enforces admin-only visibility via RLS, which yields an empty result set rather than a permission error; as written, this test will fail even when the policy is working as intended.
Useful? React with 👍 / 👎.
![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 the current code and only fix it if needed.
Inline comments:
In `@supabase/migrations/20260227000000_restrict_global_stats_public_access.sql`:
- Around line 25-27: The grant/revoke setup on table public.global_stats creates
asymmetric behavior: anon lacks SELECT and gets error 42501 while authenticated
has SELECT but is blocked by the RLS policy and therefore returns an empty
result (not an error), causing Test 4b in the RLS scenarios to fail; fix by
either adjusting the test expectation for authenticated non-admin users in the
test named Test 4b of the RLS scenarios to assert is_empty() instead of
throws_ok(42501), or change the privilege setup in the migration
(public.global_stats) to revoke SELECT from authenticated entirely and grant
SELECT only to a specific role that admins have so the test can continue to
expect a 42501 permission denied error.
In `@supabase/tests/27_test_rls_scenarios.sql`:
- Around line 67-77: The test currently sets "SET LOCAL role TO authenticated"
and calls throws_ok('SELECT COUNT(*) FROM public.global_stats', ...) expecting
error 42501, but RLS grants SELECT and returns zero rows (no error) and
auth.uid() is NULL because no JWT claims are set; update the test to assert
empty results instead of expecting an error (replace throws_ok with an
emptiness/count check such as is_empty or assert count = 0 for the query 'SELECT
COUNT(*) FROM public.global_stats') and before running the query set the JWT
claims/user context so auth.uid() is populated (e.g., set appropriate jwt.claims
or session context for a non-admin user) to mirror the intended non-admin
scenario rather than relying only on SET LOCAL role TO authenticated.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
supabase/migrations/20260227000000_restrict_global_stats_public_access.sqlsupabase/tests/26_test_rls_policies.sqlsupabase/tests/27_test_rls_scenarios.sql
| -- Remove table privileges for low-trust roles. | ||
| REVOKE ALL PRIVILEGES ON TABLE public.global_stats FROM anon, authenticated; | ||
| GRANT SELECT ON TABLE public.global_stats TO authenticated; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the migration file exists and read it
fd "20260227000000_restrict_global_stats_public_access.sql" supabase/migrations/Repository: Cap-go/capgo
Length of output: 130
🏁 Script executed:
# Check the test file mentioned in the review
fd "27_test_rls_scenarios.sql" supabase/tests/Repository: Cap-go/capgo
Length of output: 96
🏁 Script executed:
# Search for is_admin function definition
rg -n "CREATE.*FUNCTION.*is_admin" supabase/migrations/ -A 20Repository: Cap-go/capgo
Length of output: 6595
🏁 Script executed:
# Look for the global_stats table and any RLS policies
rg -n "global_stats" supabase/migrations/ -B 5 -A 10Repository: Cap-go/capgo
Length of output: 32179
🏁 Script executed:
# Read the full migration file
cat -n supabase/migrations/20260227000000_restrict_global_stats_public_access.sqlRepository: Cap-go/capgo
Length of output: 1279
🏁 Script executed:
# Read the test file to check Test 4b expectations
cat -n supabase/tests/27_test_rls_scenarios.sqlRepository: Cap-go/capgo
Length of output: 5134
🏁 Script executed:
# Check if there are more details about the test setup
wc -l supabase/tests/27_test_rls_scenarios.sqlRepository: Cap-go/capgo
Length of output: 100
Test expectation mismatch: authenticated non-admin users will receive empty results, not error 42501.
The privilege and policy configuration creates asymmetric behavior:
anon→ REVOKE removes SELECT privilege → error 42501 (permission denied)authenticatednon-admin → GRANT SELECT grants privilege, but RLS policy denies access → returns 0 rows (not an error)
The test at lines 67-77 in supabase/tests/27_test_rls_scenarios.sql (Test 4b) expects throws_ok with error 42501 for authenticated non-admin users, but will fail because non-admin authenticated users will receive an empty result set instead of an error. RLS policies return empty results when a role has the base privilege but fails the policy condition, not an error.
Either update Test 4b to use is_empty() for non-admin authenticated users, or revoke SELECT from authenticated entirely and grant it only to a role that admins possess.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260227000000_restrict_global_stats_public_access.sql`
around lines 25 - 27, The grant/revoke setup on table public.global_stats
creates asymmetric behavior: anon lacks SELECT and gets error 42501 while
authenticated has SELECT but is blocked by the RLS policy and therefore returns
an empty result (not an error), causing Test 4b in the RLS scenarios to fail;
fix by either adjusting the test expectation for authenticated non-admin users
in the test named Test 4b of the RLS scenarios to assert is_empty() instead of
throws_ok(42501), or change the privilege setup in the migration
(public.global_stats) to revoke SELECT from authenticated entirely and grant
SELECT only to a specific role that admins have so the test can continue to
expect a 42501 permission denied error.
| -- Test 4b: Authenticated users should not be able to read global_stats | ||
| SET | ||
| LOCAL role TO authenticated; | ||
|
|
||
| SELECT | ||
| throws_ok ( | ||
| 'SELECT COUNT(*) FROM public.global_stats', | ||
| '42501', | ||
| 'permission denied', | ||
| 'Authenticated non-admin users should not be able to select from global_stats' | ||
| ); |
There was a problem hiding this comment.
Test 4b will fail: authenticated non-admin users get empty results, not an error.
The migration grants SELECT to authenticated users, so they won't receive a "permission denied" error. Instead, the RLS policy will evaluate to FALSE for non-admins, returning 0 rows. The throws_ok assertion expects error 42501, which won't be thrown.
Additionally, no JWT claims are set before this test, so auth.uid() returns NULL.
🧪 Proposed fix: use is_empty instead of throws_ok
-- Test 4b: Authenticated users should not be able to read global_stats
SET
LOCAL role TO authenticated;
+SET
+ LOCAL request.jwt.claims TO '{"sub": "6aa76066-55ef-4238-ade6-0b32334a4097"}';
+
SELECT
- throws_ok (
- 'SELECT COUNT(*) FROM public.global_stats',
- '42501',
- 'permission denied',
+ is_empty (
+ 'SELECT * FROM public.global_stats',
'Authenticated non-admin users should not be able to select from global_stats'
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -- Test 4b: Authenticated users should not be able to read global_stats | |
| SET | |
| LOCAL role TO authenticated; | |
| SELECT | |
| throws_ok ( | |
| 'SELECT COUNT(*) FROM public.global_stats', | |
| '42501', | |
| 'permission denied', | |
| 'Authenticated non-admin users should not be able to select from global_stats' | |
| ); | |
| -- Test 4b: Authenticated users should not be able to read global_stats | |
| SET | |
| LOCAL role TO authenticated; | |
| SET | |
| LOCAL request.jwt.claims TO '{"sub": "6aa76066-55ef-4238-ade6-0b32334a4097"}'; | |
| SELECT | |
| is_empty ( | |
| 'SELECT * FROM public.global_stats', | |
| 'Authenticated non-admin users should not be able to select from global_stats' | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/tests/27_test_rls_scenarios.sql` around lines 67 - 77, The test
currently sets "SET LOCAL role TO authenticated" and calls throws_ok('SELECT
COUNT(*) FROM public.global_stats', ...) expecting error 42501, but RLS grants
SELECT and returns zero rows (no error) and auth.uid() is NULL because no JWT
claims are set; update the test to assert empty results instead of expecting an
error (replace throws_ok with an emptiness/count check such as is_empty or
assert count = 0 for the query 'SELECT COUNT(*) FROM public.global_stats') and
before running the query set the JWT claims/user context so auth.uid() is
populated (e.g., set appropriate jwt.claims or session context for a non-admin
user) to mirror the intended non-admin scenario rather than relying only on SET
LOCAL role TO authenticated.
|
1 participant
Summary (AI generated)
global_statsand restrictsSELECTto authenticated admins viapublic.is_admin(auth.uid()).supabase/tests/26_test_rls_policies.sqlto match the newAllow admin users to select global_statspolicy.supabase/tests/27_test_rls_scenarios.sqlto enforce denied access foranonand non-admin users, and allow access fortest_admin.Test plan (AI generated)
bun lint.bun lint:backend.Screenshots (AI generated)
Checklist (AI generated)
Summary by CodeRabbit