Restrict public execution of API key oracle RPCs#1758
Conversation
📝 WalkthroughWalkthroughRevoke PUBLIC execute on three apikey-related functions and re-grant execute to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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/tests/45_test_apikey_oracle_permissions.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 |
![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
🤖 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/20260308203222_restrict_apikey_oracle_public_access.sql`:
- Around line 9-11: The three GRANTs reopen parameterized oracle helpers to the
authenticated role; remove the GRANT EXECUTE statements for
"public"."get_user_id" (both overloads: ("apikey" "text") and ("apikey" "text",
"app_id" "text")) and for "public"."get_org_perm_for_apikey" ("apikey" "text",
"app_id" "text") so these functions remain service_role-only; if client access
is required, instead expose or GRANT only no-argument wrapper functions that
perform authentication checks (create/ensure wrappers that call the service_role
helpers internally and grant those wrappers to authenticated).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 673dacae-151b-486b-b317-44cd9693c3a5
📒 Files selected for processing (1)
supabase/migrations/20260308203222_restrict_apikey_oracle_public_access.sql
| GRANT EXECUTE ON FUNCTION "public"."get_user_id" ("apikey" "text") TO "authenticated"; | ||
| GRANT EXECUTE ON FUNCTION "public"."get_user_id" ("apikey" "text", "app_id" "text") TO "authenticated"; | ||
| GRANT EXECUTE ON FUNCTION "public"."get_org_perm_for_apikey" ("apikey" "text", "app_id" "text") TO "authenticated"; |
There was a problem hiding this comment.
Don't re-open these oracle helpers to all authenticated sessions.
Lines 9-11 still make the parameterized API-key lookup functions callable by any logged-in client. That preserves the oracle for authenticated users, which is the same class of exposure these low-level helpers usually need to avoid. Prefer keeping these functions service_role-only and exposing only a checked wrapper if client access is required.
🔒 Suggested change
-GRANT EXECUTE ON FUNCTION "public"."get_user_id" ("apikey" "text") TO "authenticated";
-GRANT EXECUTE ON FUNCTION "public"."get_user_id" ("apikey" "text", "app_id" "text") TO "authenticated";
-GRANT EXECUTE ON FUNCTION "public"."get_org_perm_for_apikey" ("apikey" "text", "app_id" "text") TO "authenticated";Based on learnings, "API key identities resolve through the authenticated role via JWT, not the anon role" and parameterized helper functions "should NOT be granted to anon/authenticated roles ... only the no-argument wrapper functions should be public as they perform authentication checks."
📝 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.
| GRANT EXECUTE ON FUNCTION "public"."get_user_id" ("apikey" "text") TO "authenticated"; | |
| GRANT EXECUTE ON FUNCTION "public"."get_user_id" ("apikey" "text", "app_id" "text") TO "authenticated"; | |
| GRANT EXECUTE ON FUNCTION "public"."get_org_perm_for_apikey" ("apikey" "text", "app_id" "text") TO "authenticated"; | |
| -- (previous lines in migration file) | |
| -- Remove the GRANT EXECUTE statements for parameterized helpers | |
| -- These functions should remain service_role-only to prevent oracle access |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260308203222_restrict_apikey_oracle_public_access.sql`
around lines 9 - 11, The three GRANTs reopen parameterized oracle helpers to the
authenticated role; remove the GRANT EXECUTE statements for
"public"."get_user_id" (both overloads: ("apikey" "text") and ("apikey" "text",
"app_id" "text")) and for "public"."get_org_perm_for_apikey" ("apikey" "text",
"app_id" "text") so these functions remain service_role-only; if client access
is required, instead expose or GRANT only no-argument wrapper functions that
perform authentication checks (create/ensure wrappers that call the service_role
helpers internally and grant those wrappers to authenticated).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/tests/45_test_apikey_oracle_permissions.sql (1)
54-68: Inconsistent test coverage for service_role.The authenticated tests (lines 33-52) verify all three function signatures:
get_user_id(apikey),get_user_id(apikey, app_id), andget_org_perm_for_apikey(apikey, app_id). However, the service_role tests omitget_user_id(apikey, app_id).Consider adding the missing test for consistency:
🔧 Proposed fix
SELECT tests.authenticate_as_service_role(); SELECT is( get_user_id('ae6e7458-c46d-4c00-aa3b-153b0b8520ea'), '6aa76066-55ef-4238-ade6-0b32334a4097', 'service role should still call get_user_id' ); +SELECT + is( + get_user_id('ae6e7458-c46d-4c00-aa3b-153b0b8520ea', 'com.demo.app'), + '6aa76066-55ef-4238-ade6-0b32334a4097', + 'service role should still call get_user_id with app_id' + ); + SELECT is( get_org_perm_for_apikey('ae6e7458-c46d-4c00-aa3b-153b0b8520ea', 'com.demo.app'), 'perm_owner', 'service role should still call get_org_perm_for_apikey' );Update the plan count accordingly:
-SELECT plan(8); +SELECT plan(9);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/tests/45_test_apikey_oracle_permissions.sql` around lines 54 - 68, Add the missing service_role test for the two-argument variant of get_user_id: inside the service-role authenticated block (where tests.call authenticate_as_service_role() and other assertions run) add an is(...) assertion that calls get_user_id('ae6e7458-c46d-4c00-aa3b-153b0b8520ea', 'com.demo.app') and compares it to the expected user id (same expected value used for the single-arg test), and update any test plan/expect count if present to reflect the additional assertion; reference the existing get_user_id(apikey) and get_org_perm_for_apikey(apikey, app_id) tests to mirror formatting and expected value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/tests/45_test_apikey_oracle_permissions.sql`:
- Around line 54-68: Add the missing service_role test for the two-argument
variant of get_user_id: inside the service-role authenticated block (where
tests.call authenticate_as_service_role() and other assertions run) add an
is(...) assertion that calls get_user_id('ae6e7458-c46d-4c00-aa3b-153b0b8520ea',
'com.demo.app') and compares it to the expected user id (same expected value
used for the single-arg test), and update any test plan/expect count if present
to reflect the additional assertion; reference the existing get_user_id(apikey)
and get_org_perm_for_apikey(apikey, app_id) tests to mirror formatting and
expected value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fda628bf-343a-45c7-99ab-423c1702d598
📒 Files selected for processing (1)
supabase/tests/45_test_apikey_oracle_permissions.sql
|
1 participant
Summary (AI generated)
PUBLICand lock down directget_user_idandget_org_perm_for_apikeyexecute rights, preventing anonymous caller access to API-key validation oracles.authenticatedandservice_roleonly, preserving intended internal access patterns.Motivation (AI generated)
Business Impact (AI generated)
Test plan (AI generated)
bun lint:backendScreenshots (AI generated)
Checklist (AI generated)
bun run lint:backend && bun run lint.Summary by CodeRabbit