fix(db): harden security definer execute grants#1966
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request hardens database function access control by converting select helper functions to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
supabase/migrations/20260427105151_harden_security_definer_execute_grants.sql (2)
75-77: Inconsistent REVOKE pattern—missing ANON/AUTHENTICATED revocations.
sync_org_user_role_binding_on_delete()only revokes from PUBLIC, while similar trigger/sync functions (e.g., lines 79-91) revoke from PUBLIC, ANON, and AUTHENTICATED. For consistency and defense-in-depth, consider adding the missing revocations.Proposed fix
REVOKE ALL ON FUNCTION public.sync_org_user_role_binding_on_delete() FROM PUBLIC; +REVOKE ALL +ON FUNCTION public.sync_org_user_role_binding_on_delete() +FROM ANON; +REVOKE ALL +ON FUNCTION public.sync_org_user_role_binding_on_delete() +FROM AUTHENTICATED;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260427105151_harden_security_definer_execute_grants.sql` around lines 75 - 77, The REVOKE block for the function sync_org_user_role_binding_on_delete() only removes privileges from PUBLIC but should also revoke from ANON and AUTHENTICATED to match other trigger/sync functions; update the migration to add REVOKE ALL ON FUNCTION public.sync_org_user_role_binding_on_delete() FROM ANON; and REVOKE ALL ON FUNCTION public.sync_org_user_role_binding_on_delete() FROM AUTHENTICATED; (mirror the pattern used for the other sync/trigger functions in this file).
15-92: All functions in this migration are confirmed trigger functions; REVOKE statements are redundant but harmless.Each of the 15 functions listed (including
apikeys_force_server_key(),check_encrypted_bundle_on_insert(),noupdate(),sanitize_*(),sync_*(), andgenerate_org_user_on_org_create()) returnstriggerand cannot be invoked directly by any role. The REVOKE statements provide defense-in-depth documentation of security intent and consistency across internal helper functions, though they are technically unnecessary for trigger functions per PostgreSQL semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260427105151_harden_security_definer_execute_grants.sql` around lines 15 - 92, The REVOKE statements target functions that are trigger functions (return type trigger) and thus cannot be called as RPCs; remove the redundant REVOKE calls to simplify the migration: delete the REVOKE lines for apikeys_force_server_key(), apikeys_strip_plain_key_for_hashed(), check_encrypted_bundle_on_insert(), cleanup_onboarding_app_data_on_complete(), generate_org_user_on_org_create() (and the entire DO $$...$$ block that revokes it), generate_org_user_stripe_info_on_org_create(), noupdate(), prevent_last_super_admin_binding_delete(), sanitize_apps_text_fields(), sanitize_orgs_text_fields(), sanitize_tmp_users_text_fields(), sanitize_users_text_fields(), sync_org_has_usage_credits_from_grants(), sync_org_user_role_binding_on_delete(), sync_org_user_role_binding_on_update(), and sync_org_user_to_role_binding(); leave any non-trigger-specific security statements intact if present.tests/security-definer-execute-hardening.test.ts (1)
101-121: Add explicit existence check for queried functions.The
LEFT JOINreturns NULL values forprosecdef,anon_exec, andauth_execwhen a function doesn't exist (e.g., signature mismatch). The tests would then comparenulltofalse/true, failing with unclear error messages rather than identifying the missing function.Consider adding an existence assertion to catch signature mismatches early:
Proposed fix
for (const proc of INVOKER_PROCS) { + expect(states.get(proc)?.prosecdef, `${proc} does not exist`).not.toBeNull() expect(states.get(proc)?.prosecdef, proc).toBe(false) }Apply similar checks in other test cases, or add a shared helper:
function assertProcExists(states: Map<string, ProcState>, proc: string) { const state = states.get(proc) expect(state?.prosecdef, `${proc} does not exist or signature mismatch`).not.toBeNull() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security-definer-execute-hardening.test.ts` around lines 101 - 121, The LEFT JOIN in getProcStates returns NULL for non-existent functions causing silent null vs boolean mismatches; add an explicit existence assertion helper (e.g., assertProcExists(states: Map<string,ProcState>, proc: string)) that looks up states.get(proc) and calls expect(state?.prosecdef, `${proc} does not exist or signature mismatch`).not.toBeNull(); call this helper from the tests before comparing anon_exec/auth_exec so missing functions fail with a clear message; alternatively you can augment getProcStates to return an explicit exists flag, but ensure tests first assert existence using the ProcState.prosecdef check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/security-definer-execute-hardening.test.ts`:
- Around line 22-41: Add the missing function name to the SERVICE_ONLY_PROCS
test array: include 'public.sync_org_user_role_binding_on_delete(uuid, uuid)' in
the SERVICE_ONLY_PROCS constant so the test covers the migration that revokes
execute from sync_org_user_role_binding_on_delete; update the SERVICE_ONLY_PROCS
declaration in tests/security-definer-execute-hardening.test.ts (where
SERVICE_ONLY_PROCS is defined) to contain that entry alongside the other
public.sync_* entries.
---
Nitpick comments:
In
`@supabase/migrations/20260427105151_harden_security_definer_execute_grants.sql`:
- Around line 75-77: The REVOKE block for the function
sync_org_user_role_binding_on_delete() only removes privileges from PUBLIC but
should also revoke from ANON and AUTHENTICATED to match other trigger/sync
functions; update the migration to add REVOKE ALL ON FUNCTION
public.sync_org_user_role_binding_on_delete() FROM ANON; and REVOKE ALL ON
FUNCTION public.sync_org_user_role_binding_on_delete() FROM AUTHENTICATED;
(mirror the pattern used for the other sync/trigger functions in this file).
- Around line 15-92: The REVOKE statements target functions that are trigger
functions (return type trigger) and thus cannot be called as RPCs; remove the
redundant REVOKE calls to simplify the migration: delete the REVOKE lines for
apikeys_force_server_key(), apikeys_strip_plain_key_for_hashed(),
check_encrypted_bundle_on_insert(), cleanup_onboarding_app_data_on_complete(),
generate_org_user_on_org_create() (and the entire DO $$...$$ block that revokes
it), generate_org_user_stripe_info_on_org_create(), noupdate(),
prevent_last_super_admin_binding_delete(), sanitize_apps_text_fields(),
sanitize_orgs_text_fields(), sanitize_tmp_users_text_fields(),
sanitize_users_text_fields(), sync_org_has_usage_credits_from_grants(),
sync_org_user_role_binding_on_delete(), sync_org_user_role_binding_on_update(),
and sync_org_user_to_role_binding(); leave any non-trigger-specific security
statements intact if present.
In `@tests/security-definer-execute-hardening.test.ts`:
- Around line 101-121: The LEFT JOIN in getProcStates returns NULL for
non-existent functions causing silent null vs boolean mismatches; add an
explicit existence assertion helper (e.g., assertProcExists(states:
Map<string,ProcState>, proc: string)) that looks up states.get(proc) and calls
expect(state?.prosecdef, `${proc} does not exist or signature
mismatch`).not.toBeNull(); call this helper from the tests before comparing
anon_exec/auth_exec so missing functions fail with a clear message;
alternatively you can augment getProcStates to return an explicit exists flag,
but ensure tests first assert existence using the ProcState.prosecdef check.
🪄 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
Run ID: 3c301121-8477-4931-ab57-4929af6a9692
📒 Files selected for processing (2)
supabase/migrations/20260427105151_harden_security_definer_execute_grants.sqltests/security-definer-execute-hardening.test.ts
|
1 participant
Summary (AI generated)
SECURITY DEFINERexecute grants with a focused Supabase migrationMotivation (AI generated)
Supabase flagged a large set of exposed
SECURITY DEFINERfunctions. This PR tightens the obvious mis-grants first without breaking the existing API-key-backed product flows that still rely on a small set of privileged RPC wrappers.Business Impact (AI generated)
This reduces the externally reachable attack surface around privileged database functions while preserving core organization and statistics flows. It lowers security risk without interrupting customer usage of API keys and dashboard statistics.
Test Plan (AI generated)
bun lintbun run supabase:with-env -- bunx vitest run tests/security-definer-execute-hardening.test.ts tests/statistics.test.ts --reporter=verboseGenerated with AI
Summary by CodeRabbit
Chores
Tests