Route RLS admin checks through is_platform_admin#1780
Conversation
📝 WalkthroughWalkthroughThis PR refactors and splits admin authentication logic, introducing a distinction between platform admin (vault-based via Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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/20260311164503_split_is_admin_platform_admin_and_rls.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: 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: 126fa066da
ℹ️ 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".
| 'ALTER POLICY %I ON %I.%I%s USING (%s) WITH CHECK (%s)', | ||
| v_policy.policyname, | ||
| v_policy.schemaname, | ||
| v_policy.tablename, | ||
| v_roles_sql, | ||
| v_with_check |
There was a problem hiding this comment.
The format() call for policies that have WITH CHECK is missing an argument: the SQL template has six placeholders (... USING (%s) WITH CHECK (%s)) but only five values are provided, and no v_using value is passed at all. When this loop reaches any matching policy with with_check (for example INSERT policies), Postgres raises too few arguments for format() and the migration aborts before rewriting RLS predicates.
Useful? React with 👍 / 👎.
| let client: PoolClient | ||
|
|
||
| const query = (text: string, values: Array<string | boolean> = []) => { | ||
| return client.query(text, values) |
There was a problem hiding this comment.
These tests are marked it.concurrent, but they share a single mutable client variable through beforeEach/afterEach and query(). Parallel executions can overwrite that shared reference, causing one test to run queries or rollbacks on another test's connection, which makes the suite flaky and nondeterministic under parallel scheduling.
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: 6
🧹 Nitpick comments (1)
src/modules/auth.ts (1)
10-10: Use the~/servicesalias here.This file already imports the same module via
~/services/supabase; keeping a second relative specifier makes refactors noisier and breaks the frontend import convention.As per coding guidelines,
Import using \~/` alias for `src/` directory in frontend code instead of relative paths`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/auth.ts` at line 10, The import uses a relative path for getPlans and isPlatformAdmin; replace the relative import "import { getPlans, isPlatformAdmin } from './../services/supabase'" with the project alias import from the same module (use the ~/services/supabase specifier) so both imports in this file consistently use the ~/ alias; update the import statement referencing getPlans and isPlatformAdmin to use '~/services/supabase'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 342: Fix the typo in the regression-prevention rule by replacing the
misspelled token "REGRSSION" with the correct spelling "REGRESSION" in the
comment string "COMMENT THE BEHAVIOR (E.G., LEGACY VS PLATFORM SECRET CHECK) TO
PREVENT REGRSSION." so the line reads "COMMENT THE BEHAVIOR (E.G., LEGACY VS
PLATFORM SECRET CHECK) TO PREVENT REGRESSION."; this touches the exact text
"REGRSSION" and should only change that word to "REGRESSION".
In `@src/modules/auth.ts`:
- Around line 161-168: The main.isAdmin flag is only initialized once and can
persist across session/role changes; update the code paths that set main.isAdmin
(the block using isPlatformAdmin() and the similar block at lines ~220-227) so
they always re-evaluate and assign main.isAdmin = await isPlatformAdmin()
whenever the active session/account/role changes (or explicitly clear
main.isAdmin on sign-out/session-switch) instead of only setting it when
undefined; ensure any session-change handler or auth state watcher calls
isPlatformAdmin() and updates main.isAdmin (and keep the existing error handling
that sets main.isAdmin = false on failure).
In `@supabase/migrations/20260311000000_split_is_admin_platform_admin.sql`:
- Around line 77-85: The is_admin() RBAC check ignores role_bindings.expires_at
so time-limited platform_super_admin grants never expire; update the SQL that
sets is_admin_from_rbac (the SELECT joining public.role_bindings rb and
public.roles r) to only consider bindings where rb.expires_at IS NULL OR
rb.expires_at > now() (or current_timestamp), keeping the existing joins/filters
(rb.principal_type = public.rbac_principal_user(), rb.principal_id = userid,
rb.scope_type = public.rbac_scope_platform(), r.name =
public.rbac_role_platform_super_admin()) so expired bindings do not satisfy the
admin check.
In
`@supabase/migrations/20260313000000_replace_is_admin_rls_with_is_platform_admin.sql`:
- Around line 43-71: The ALTER POLICY generation for the branch where
v_policy.with_check IS NOT NULL is missing v_using in the format call and always
emits a "USING (...)" clause (producing "USING ()" when v_using IS NULL); update
the branch that handles v_policy.with_check to 1) include v_using as an argument
to the format() call and 2) conditionally include the "USING (<v_using>)"
fragment only when v_using IS NOT NULL (otherwise omit the USING clause and emit
only "WITH CHECK (<v_with_check>)"), using the existing variables
v_policy.policyname, v_policy.schemaname, v_policy.tablename, v_roles_sql,
v_using, and v_with_check to build the correct ALTER POLICY statement.
In `@supabase/tests/35_test_is_admin_rbac.sql`:
- Line 3: The pgTAP plan count is incorrect: update the test's plan invocation
(currently SELECT plan(7);) to match the actual number of assertions (6) so the
test does not fail due to an off-by-one; locate the SELECT plan(...) statement
in the test file (e.g., the plan call at the top of the test) and change the
numeric argument to 6.
In `@tests/is-admin-functions.test.ts`:
- Around line 8-13: The tests share a mutable module-level PoolClient (`client`)
and a `query` helper that closes over it, causing race conditions when using
it.concurrent; refactor so each concurrent test gets its own client/transaction
helper: remove reliance on the module-level `client` and `query`, and instead
create a per-test client/transaction (or a factory that returns a `query`
function bound to that test's client) inside each it.concurrent block or its
beforeEach for that test; ensure `pool` is still shared safely but each test
opens/releases its own client (via Pool.connect()/client.release()) so
transactions/rollbacks don't cross tests and tests remain parallelizable.
---
Nitpick comments:
In `@src/modules/auth.ts`:
- Line 10: The import uses a relative path for getPlans and isPlatformAdmin;
replace the relative import "import { getPlans, isPlatformAdmin } from
'./../services/supabase'" with the project alias import from the same module
(use the ~/services/supabase specifier) so both imports in this file
consistently use the ~/ alias; update the import statement referencing getPlans
and isPlatformAdmin to use '~/services/supabase'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe71dd02-fd83-4e11-9388-ec0ed0e073ce
📒 Files selected for processing (15)
AGENTS.mdsrc/modules/auth.tssrc/services/supabase.tssrc/types/supabase.types.tssupabase/functions/_backend/private/admin_credits.tssupabase/functions/_backend/private/admin_stats.tssupabase/functions/_backend/private/log_as.tssupabase/functions/_backend/public/replication.tssupabase/functions/_backend/utils/supabase.tssupabase/functions/_backend/utils/supabase.types.tssupabase/migrations/20260311000000_split_is_admin_platform_admin.sqlsupabase/migrations/20260313000000_replace_is_admin_rls_with_is_platform_admin.sqlsupabase/tests/07_auth_functions.sqlsupabase/tests/35_test_is_admin_rbac.sqltests/is-admin-functions.test.ts
💤 Files with no reviewable changes (1)
- supabase/functions/_backend/utils/supabase.ts
| - APPLY `REVOKE ALL ... FROM PUBLIC` TO EVERY OVERLOAD. | ||
| - GRANT `service_role` TO `uuid` ONLY; GRANT `authenticated` ONLY TO `()` IF NEEDED. | ||
| - KEEP `SET search_path = ''` AND `SECURITY DEFINER` EXPLICIT. | ||
| - COMMENT THE BEHAVIOR (E.G., LEGACY VS PLATFORM SECRET CHECK) TO PREVENT REGRSSION. |
There was a problem hiding this comment.
Fix the typo in the regression-prevention rule.
Line 342 says REGRSSION; it should be REGRESSION.
🧰 Tools
🪛 LanguageTool
[grammar] ~342-~342: Ensure spelling is correct
Context: ...CY VS PLATFORM SECRET CHECK) TO PREVENT REGRSSION. ```sql ALTER FUNCTION public.is_platf...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` at line 342, Fix the typo in the regression-prevention rule by
replacing the misspelled token "REGRSSION" with the correct spelling
"REGRESSION" in the comment string "COMMENT THE BEHAVIOR (E.G., LEGACY VS
PLATFORM SECRET CHECK) TO PREVENT REGRSSION." so the line reads "COMMENT THE
BEHAVIOR (E.G., LEGACY VS PLATFORM SECRET CHECK) TO PREVENT REGRESSION."; this
touches the exact text "REGRSSION" and should only change that word to
"REGRESSION".
| try { | ||
| // isPlatformAdmin() is the only frontend admin-rights source. | ||
| main.isAdmin = await isPlatformAdmin() | ||
| } | ||
| catch (error) { | ||
| console.error('Failed to resolve platform admin status:', error) | ||
| main.isAdmin = false | ||
| } |
There was a problem hiding this comment.
Don't let main.isAdmin survive session or role changes.
This flag is only refreshed on first load or when it is still undefined. If the user is promoted/demoted mid-session, or the session switches accounts without tearing down the store, /admin routing can keep using the previous user's admin bit.
Also applies to: 220-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/auth.ts` around lines 161 - 168, The main.isAdmin flag is only
initialized once and can persist across session/role changes; update the code
paths that set main.isAdmin (the block using isPlatformAdmin() and the similar
block at lines ~220-227) so they always re-evaluate and assign main.isAdmin =
await isPlatformAdmin() whenever the active session/account/role changes (or
explicitly clear main.isAdmin on sign-out/session-switch) instead of only
setting it when undefined; ensure any session-change handler or auth state
watcher calls isPlatformAdmin() and updates main.isAdmin (and keep the existing
error handling that sets main.isAdmin = false on failure).
| SELECT EXISTS ( | ||
| SELECT 1 | ||
| FROM public.role_bindings rb | ||
| JOIN public.roles r ON r.id = rb.role_id | ||
| WHERE rb.principal_type = public.rbac_principal_user() | ||
| AND rb.principal_id = userid | ||
| AND rb.scope_type = public.rbac_scope_platform() | ||
| AND r.name = public.rbac_role_platform_super_admin() | ||
| ) INTO is_admin_from_rbac; |
There was a problem hiding this comment.
Expired platform bindings still satisfy is_admin().
This lookup ignores role_bindings.expires_at, so a time-boxed platform_super_admin grant never stops authorizing once inserted.
Suggested fix
SELECT EXISTS (
SELECT 1
FROM public.role_bindings rb
JOIN public.roles r ON r.id = rb.role_id
WHERE rb.principal_type = public.rbac_principal_user()
AND rb.principal_id = userid
+ AND (rb.expires_at IS NULL OR rb.expires_at > pg_catalog.now())
AND rb.scope_type = public.rbac_scope_platform()
AND r.name = public.rbac_role_platform_super_admin()
) INTO is_admin_from_rbac;📝 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.
| SELECT EXISTS ( | |
| SELECT 1 | |
| FROM public.role_bindings rb | |
| JOIN public.roles r ON r.id = rb.role_id | |
| WHERE rb.principal_type = public.rbac_principal_user() | |
| AND rb.principal_id = userid | |
| AND rb.scope_type = public.rbac_scope_platform() | |
| AND r.name = public.rbac_role_platform_super_admin() | |
| ) INTO is_admin_from_rbac; | |
| SELECT EXISTS ( | |
| SELECT 1 | |
| FROM public.role_bindings rb | |
| JOIN public.roles r ON r.id = rb.role_id | |
| WHERE rb.principal_type = public.rbac_principal_user() | |
| AND rb.principal_id = userid | |
| AND (rb.expires_at IS NULL OR rb.expires_at > pg_catalog.now()) | |
| AND rb.scope_type = public.rbac_scope_platform() | |
| AND r.name = public.rbac_role_platform_super_admin() | |
| ) INTO is_admin_from_rbac; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260311000000_split_is_admin_platform_admin.sql` around
lines 77 - 85, The is_admin() RBAC check ignores role_bindings.expires_at so
time-limited platform_super_admin grants never expire; update the SQL that sets
is_admin_from_rbac (the SELECT joining public.role_bindings rb and public.roles
r) to only consider bindings where rb.expires_at IS NULL OR rb.expires_at >
now() (or current_timestamp), keeping the existing joins/filters
(rb.principal_type = public.rbac_principal_user(), rb.principal_id = userid,
rb.scope_type = public.rbac_scope_platform(), r.name =
public.rbac_role_platform_super_admin()) so expired bindings do not satisfy the
admin check.
| IF v_using = v_policy.qual AND v_with_check = COALESCE(v_policy.with_check, '') THEN | ||
| CONTINUE; | ||
| END IF; | ||
|
|
||
| IF array_length(v_policy.roles, 1) > 0 THEN | ||
| SELECT string_agg(format('%I', role), ', ') | ||
| INTO v_roles | ||
| FROM unnest(v_policy.roles) AS role; | ||
| v_roles_sql := format(' TO %s', v_roles); | ||
| END IF; | ||
|
|
||
| IF v_policy.with_check IS NOT NULL THEN | ||
| EXECUTE format( | ||
| 'ALTER POLICY %I ON %I.%I%s USING (%s) WITH CHECK (%s)', | ||
| v_policy.policyname, | ||
| v_policy.schemaname, | ||
| v_policy.tablename, | ||
| v_roles_sql, | ||
| v_with_check | ||
| ); | ||
| ELSE | ||
| EXECUTE format( | ||
| 'ALTER POLICY %I ON %I.%I%s USING (%s)', | ||
| v_policy.policyname, | ||
| v_policy.schemaname, | ||
| v_policy.tablename, | ||
| v_roles_sql, | ||
| v_using | ||
| ); |
There was a problem hiding this comment.
The WITH CHECK rewrite branch currently generates invalid ALTER POLICY SQL.
The format() call is missing v_using, so any matching policy with with_check will fail immediately. Even after fixing that, this path always emits USING (...), which breaks policies whose qual is NULL by producing USING ().
Suggested fix
- IF v_using = v_policy.qual AND v_with_check = COALESCE(v_policy.with_check, '') THEN
+ IF v_using = COALESCE(v_policy.qual, '') AND v_with_check = COALESCE(v_policy.with_check, '') THEN
CONTINUE;
END IF;
- IF v_policy.with_check IS NOT NULL THEN
+ IF v_policy.qual IS NOT NULL AND v_policy.with_check IS NOT NULL THEN
EXECUTE format(
'ALTER POLICY %I ON %I.%I%s USING (%s) WITH CHECK (%s)',
v_policy.policyname,
v_policy.schemaname,
v_policy.tablename,
v_roles_sql,
+ v_using,
v_with_check
);
+ ELSIF v_policy.with_check IS NOT NULL THEN
+ EXECUTE format(
+ 'ALTER POLICY %I ON %I.%I%s WITH CHECK (%s)',
+ v_policy.policyname,
+ v_policy.schemaname,
+ v_policy.tablename,
+ v_roles_sql,
+ v_with_check
+ );
ELSE
EXECUTE format(
'ALTER POLICY %I ON %I.%I%s USING (%s)',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@supabase/migrations/20260313000000_replace_is_admin_rls_with_is_platform_admin.sql`
around lines 43 - 71, The ALTER POLICY generation for the branch where
v_policy.with_check IS NOT NULL is missing v_using in the format call and always
emits a "USING (...)" clause (producing "USING ()" when v_using IS NULL); update
the branch that handles v_policy.with_check to 1) include v_using as an argument
to the format() call and 2) conditionally include the "USING (<v_using>)"
fragment only when v_using IS NOT NULL (otherwise omit the USING clause and emit
only "WITH CHECK (<v_with_check>)"), using the existing variables
v_policy.policyname, v_policy.schemaname, v_policy.tablename, v_roles_sql,
v_using, and v_with_check to build the correct ALTER POLICY statement.
| -- Test is_admin() function with RBAC integration | ||
| -- Test is_admin() with RBAC integration | ||
| BEGIN; | ||
| SELECT plan(7); |
There was a problem hiding this comment.
The pgTAP plan is off by one.
This file now has six assertions, but plan(7) still expects seven, so the test will fail even when every check passes.
Suggested fix
-SELECT plan(7);
+SELECT plan(6);📝 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.
| SELECT plan(7); | |
| SELECT plan(6); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/tests/35_test_is_admin_rbac.sql` at line 3, The pgTAP plan count is
incorrect: update the test's plan invocation (currently SELECT plan(7);) to
match the actual number of assertions (6) so the test does not fail due to an
off-by-one; locate the SELECT plan(...) statement in the test file (e.g., the
plan call at the top of the test) and change the numeric argument to 6.
| let pool: Pool | ||
| let client: PoolClient | ||
|
|
||
| const query = (text: string, values: Array<string | boolean> = []) => { | ||
| return client.query(text, values) | ||
| } |
There was a problem hiding this comment.
client cannot be shared across it.concurrent() tests.
query() closes over one mutable client, and each hook overwrites or releases that same variable. Once both tests start, one test can run inside the other test's transaction or roll back the other test's connection. If this suite must stay concurrent, give each test its own local client/transaction helper instead of module-level state.
As per coding guidelines, Design all tests for parallel execution; use \it.concurrent()` instead of `it()` to maximize parallelism; create dedicated seed data for tests that modify shared resources to avoid conflicts`.
Also applies to: 19-33, 40-40, 75-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/is-admin-functions.test.ts` around lines 8 - 13, The tests share a
mutable module-level PoolClient (`client`) and a `query` helper that closes over
it, causing race conditions when using it.concurrent; refactor so each
concurrent test gets its own client/transaction helper: remove reliance on the
module-level `client` and `query`, and instead create a per-test
client/transaction (or a factory that returns a `query` function bound to that
test's client) inside each it.concurrent block or its beforeEach for that test;
ensure `pool` is still shared safely but each test opens/releases its own client
(via Pool.connect()/client.release()) so transactions/rollbacks don't cross
tests and tests remain parallelizable.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supabase/migrations/20260311164503_split_is_admin_platform_admin_and_rls.sql (1)
142-158: Consolidate replacement rules to reduce drift and missed variants.The
replace(...)list is duplicated forv_usingandv_with_check(also flagged by Sonar), which is brittle. A single pattern loop is easier to maintain and can include missing no-arg variants (is_admin()/public.is_admin()).♻️ Refactor sketch
+ FOR old_pattern IN + SELECT unnest(ARRAY[ + 'public.is_admin()', + 'is_admin()', + 'public.is_admin(auth_user.uid)', + 'public.is_admin(auth.uid())', + '"public"."is_admin"("auth_user"."uid")', + 'public.is_admin((SELECT auth.uid()))', + '"public"."is_admin"((SELECT auth.uid()))', + 'is_admin(auth_user.uid)', + 'is_admin(auth.uid())', + 'is_admin((SELECT auth.uid()))' + ]) + LOOP + v_using := replace(v_using, old_pattern, 'public.is_platform_admin()'); + v_with_check := replace(v_with_check, old_pattern, 'public.is_platform_admin()'); + END LOOP; - - v_using := replace(v_using, 'public.is_admin(auth_user.uid)', 'public.is_platform_admin()'); - ... - v_with_check := replace(v_with_check, 'is_admin((SELECT auth.uid()))', 'is_platform_admin()');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260311164503_split_is_admin_platform_admin_and_rls.sql` around lines 142 - 158, The replacement logic duplicates many replace(...) calls for v_using and v_with_check and misses no-arg variants; consolidate into a single array/list of patterns (e.g., patterns = ['public.is_admin(auth_user.uid)', '"public"."is_admin"("auth_user"."uid")', 'is_admin(auth.uid())', 'is_admin((SELECT auth.uid()))', 'is_admin()', 'public.is_admin()' , ...]) and loop over that list to apply replace to both v_using and v_with_check (call the existing replace function/method inside the loop), then replace matches with 'public.is_platform_admin()' (and include both quoted/unquoted forms if needed) so all variants are handled in one place and duplication is removed.
🤖 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/20260311164503_split_is_admin_platform_admin_and_rls.sql`:
- Around line 171-189: The dynamic SQL in the ALTER POLICY branch is malformed:
the format string expects six placeholders but only five args are passed, and it
always emits a USING clause even when v_using is empty; update the ALTER POLICY
logic (the block that builds SQL using v_policy.policyname, v_policy.schemaname,
v_policy.tablename, v_roles_sql, v_using, v_with_check) to handle three cases
explicitly: (A) both v_using (from v_policy.qual) and v_with_check present — use
format with six placeholders and pass v_using as the 5th and v_with_check as the
6th to produce "USING (%s) WITH CHECK (%s)"; (B) only v_with_check present —
emit SQL without a USING clause (use a format that includes only the WITH CHECK
part); and (C) only v_using present — emit SQL with only the USING clause (use a
format with five placeholders). Ensure you reference and use the existing
variables v_using and v_with_check in the correct order when calling format().
---
Nitpick comments:
In
`@supabase/migrations/20260311164503_split_is_admin_platform_admin_and_rls.sql`:
- Around line 142-158: The replacement logic duplicates many replace(...) calls
for v_using and v_with_check and misses no-arg variants; consolidate into a
single array/list of patterns (e.g., patterns =
['public.is_admin(auth_user.uid)', '"public"."is_admin"("auth_user"."uid")',
'is_admin(auth.uid())', 'is_admin((SELECT auth.uid()))', 'is_admin()',
'public.is_admin()' , ...]) and loop over that list to apply replace to both
v_using and v_with_check (call the existing replace function/method inside the
loop), then replace matches with 'public.is_platform_admin()' (and include both
quoted/unquoted forms if needed) so all variants are handled in one place and
duplication is removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c9b96a6-91f6-4a74-8940-c1f1c6fd1f9b
📒 Files selected for processing (1)
supabase/migrations/20260311164503_split_is_admin_platform_admin_and_rls.sql
| IF v_policy.with_check IS NOT NULL THEN | ||
| EXECUTE format( | ||
| 'ALTER POLICY %I ON %I.%I%s USING (%s) WITH CHECK (%s)', | ||
| v_policy.policyname, | ||
| v_policy.schemaname, | ||
| v_policy.tablename, | ||
| v_roles_sql, | ||
| v_with_check | ||
| ); | ||
| ELSE | ||
| EXECUTE format( | ||
| 'ALTER POLICY %I ON %I.%I%s USING (%s)', | ||
| v_policy.policyname, | ||
| v_policy.schemaname, | ||
| v_policy.tablename, | ||
| v_roles_sql, | ||
| v_using | ||
| ); | ||
| END IF; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
import re
path = Path("supabase/migrations/20260311164503_split_is_admin_platform_admin_and_rls.sql")
text = path.read_text()
m = re.search(
r"EXECUTE format\(\s*'ALTER POLICY %I ON %I\.%I%s USING \(%s\) WITH CHECK \(%s\)'\s*,(?P<args>.*?)\);\s*",
text,
re.S,
)
if not m:
print("WITH CHECK format() call not found")
raise SystemExit(1)
args = [a.strip() for a in m.group("args").split(",") if a.strip()]
print("expected_placeholders = 6")
print(f"actual_args = {len(args)}")
print("args =", args)
if len(args) != 6:
print("FAIL: placeholder/argument mismatch confirmed")
PYRepository: Cap-go/capgo
Length of output: 250
🏁 Script executed:
cat -n supabase/migrations/20260311164503_split_is_admin_platform_admin_and_rls.sql | head -200 | tail -100Repository: Cap-go/capgo
Length of output: 4709
Fix malformed dynamic SQL in the WITH CHECK branch (migration-blocking).
Line 173 has 6 format() placeholders (%I, %I, %I, %s, %s, %s), but only 5 arguments are provided (v_policy.policyname, v_policy.schemaname, v_policy.tablename, v_roles_sql, v_with_check). The missing argument is v_using, which should be the 5th placeholder. Currently, v_with_check is being passed as the 5th argument (the USING slot), leaving the WITH CHECK slot empty. This will fail at runtime with a format string mismatch error.
Additionally, this branch unconditionally emits USING (...) even when v_policy.qual is null. When qual is null, v_using becomes an empty string (line 138), resulting in invalid SQL like USING (). Handle three cases: (1) both qual and with_check exist, (2) only with_check exists (check-only policies need no USING clause), and (3) only qual exists.
Proposed fix
- IF v_policy.with_check IS NOT NULL THEN
+ IF v_policy.with_check IS NOT NULL AND v_policy.qual IS NOT NULL THEN
EXECUTE format(
'ALTER POLICY %I ON %I.%I%s USING (%s) WITH CHECK (%s)',
v_policy.policyname,
v_policy.schemaname,
v_policy.tablename,
v_roles_sql,
+ v_using,
v_with_check
);
+ ELSIF v_policy.with_check IS NOT NULL THEN
+ EXECUTE format(
+ 'ALTER POLICY %I ON %I.%I%s WITH CHECK (%s)',
+ v_policy.policyname,
+ v_policy.schemaname,
+ v_policy.tablename,
+ v_roles_sql,
+ v_with_check
+ );
ELSE
EXECUTE format(
'ALTER POLICY %I ON %I.%I%s USING (%s)',
v_policy.policyname,
v_policy.schemaname,
v_policy.tablename,
v_roles_sql,
v_using
);
END IF;📝 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.
| IF v_policy.with_check IS NOT NULL THEN | |
| EXECUTE format( | |
| 'ALTER POLICY %I ON %I.%I%s USING (%s) WITH CHECK (%s)', | |
| v_policy.policyname, | |
| v_policy.schemaname, | |
| v_policy.tablename, | |
| v_roles_sql, | |
| v_with_check | |
| ); | |
| ELSE | |
| EXECUTE format( | |
| 'ALTER POLICY %I ON %I.%I%s USING (%s)', | |
| v_policy.policyname, | |
| v_policy.schemaname, | |
| v_policy.tablename, | |
| v_roles_sql, | |
| v_using | |
| ); | |
| END IF; | |
| IF v_policy.with_check IS NOT NULL AND v_policy.qual IS NOT NULL THEN | |
| EXECUTE format( | |
| 'ALTER POLICY %I ON %I.%I%s USING (%s) WITH CHECK (%s)', | |
| v_policy.policyname, | |
| v_policy.schemaname, | |
| v_policy.tablename, | |
| v_roles_sql, | |
| v_using, | |
| v_with_check | |
| ); | |
| ELSIF v_policy.with_check IS NOT NULL THEN | |
| EXECUTE format( | |
| 'ALTER POLICY %I ON %I.%I%s WITH CHECK (%s)', | |
| v_policy.policyname, | |
| v_policy.schemaname, | |
| v_policy.tablename, | |
| v_roles_sql, | |
| v_with_check | |
| ); | |
| ELSE | |
| EXECUTE format( | |
| 'ALTER POLICY %I ON %I.%I%s USING (%s)', | |
| v_policy.policyname, | |
| v_policy.schemaname, | |
| v_policy.tablename, | |
| v_roles_sql, | |
| v_using | |
| ); | |
| END IF; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260311164503_split_is_admin_platform_admin_and_rls.sql`
around lines 171 - 189, The dynamic SQL in the ALTER POLICY branch is malformed:
the format string expects six placeholders but only five args are passed, and it
always emits a USING clause even when v_using is empty; update the ALTER POLICY
logic (the block that builds SQL using v_policy.policyname, v_policy.schemaname,
v_policy.tablename, v_roles_sql, v_using, v_with_check) to handle three cases
explicitly: (A) both v_using (from v_policy.qual) and v_with_check present — use
format with six placeholders and pass v_using as the 5th and v_with_check as the
6th to produce "USING (%s) WITH CHECK (%s)"; (B) only v_with_check present —
emit SQL without a USING clause (use a format that includes only the WITH CHECK
part); and (C) only v_using present — emit SQL with only the USING clause (use a
format with five placeholders). Ensure you reference and use the existing
variables v_using and v_with_check in the correct order when calling format().
1 participant
Summary (AI generated)
is_admintois_platform_admin()with a migration that rewrites existing public RLS predicates.is_adminavailable only for legacy/internal behavior while enforcing platform-admin decisions throughis_platform_adminfor user-facing authorization.Test Plan (AI generated)
bun lintbun lint:backendis_adminandis_platform_adminbehaviorSummary by CodeRabbit
New Features
Documentation
Tests