Conversation
📝 WalkthroughWalkthroughRevokes PUBLIC EXECUTE on API-key oracle functions and grants EXECUTE to service_role only via a migration; updates tests to authenticate as service_role; adds a new pgTAP test that verifies access control for anonymous, authenticated, and service-role callers. Changes
Sequence Diagram(s)(omitted) 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/migrations/20260308203222_restrict_apikey_oracle_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: 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.
🧹 Nitpick comments (1)
supabase/tests/45_test_apikey_oracle_permissions.sql (1)
31-52: Consider adding a test for the two-argumentget_user_idwith service role.The authenticated role tests cover both
get_user_id(apikey)andget_user_id(apikey, app_id)(lines 33-45), but the service_role section only tests the single-argument version (lines 56-61). For completeness, you might add a test forget_user_id(apikey, app_id)under service_role.🔧 Optional: Add service_role test for two-argument get_user_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' ); +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 tests.clear_authentication();Note: Update
plan(8)toplan(9)if adding this test.🤖 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 31 - 52, Add a service_role test that calls get_user_id('ae6e7458-c46d-4c00-aa3b-153b0b8520ea', 'com.demo.app') and asserts the expected user id (same as the single-argument service_role test), and increment the test harness plan from plan(8) to plan(9); locate the service_role block in the file (near the existing get_user_id(apikey) service_role test) and add the two-argument invocation for get_user_id and its corresponding is(...) assertion.
🤖 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 31-52: Add a service_role test that calls
get_user_id('ae6e7458-c46d-4c00-aa3b-153b0b8520ea', 'com.demo.app') and asserts
the expected user id (same as the single-argument service_role test), and
increment the test harness plan from plan(8) to plan(9); locate the service_role
block in the file (near the existing get_user_id(apikey) service_role test) and
add the two-argument invocation for get_user_id and its corresponding is(...)
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9258340b-a9e4-4860-b61a-397dbfb15be7
📒 Files selected for processing (4)
supabase/migrations/20260308203222_restrict_apikey_oracle_public_access.sqlsupabase/tests/07_auth_functions.sqlsupabase/tests/19_test_identity_functions.sqlsupabase/tests/45_test_apikey_oracle_permissions.sql
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/tests/45_test_apikey_oracle_permissions.sql`:
- Around line 59-64: The test compares the uuid returned by get_user_id(...)
against a text literal, causing a type mismatch; update the assertion in
45_test_apikey_oracle_permissions.sql to cast the expected literal to uuid
(e.g., append ::uuid) so the comparison matches the function return type,
mirroring the approach used in 10_utility_functions.sql and ensuring
get_user_id(...) is compared as uuid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d925803d-2c90-4999-8f80-06bb185d3ed4
📒 Files selected for processing (3)
supabase/migrations/20260308203222_restrict_apikey_oracle_public_access.sqlsupabase/tests/10_utility_functions.sqlsupabase/tests/45_test_apikey_oracle_permissions.sql
| SELECT | ||
| is( | ||
| get_user_id('ae6e7458-c46d-4c00-aa3b-153b0b8520ea'), | ||
| '6aa76066-55ef-4238-ade6-0b32334a4097', | ||
| 'service role should still call get_user_id' | ||
| ); |
There was a problem hiding this comment.
Potential type mismatch: UUID vs text comparison.
get_user_id() returns uuid, but the expected value is a text literal. In 10_utility_functions.sql, the comparison uses explicit ::uuid cast. Consider adding the cast for consistency and to avoid potential comparison issues.
Proposed fix
SELECT
is(
get_user_id('ae6e7458-c46d-4c00-aa3b-153b0b8520ea'),
- '6aa76066-55ef-4238-ade6-0b32334a4097',
+ '6aa76066-55ef-4238-ade6-0b32334a4097'::uuid,
'service role should still call get_user_id'
);📝 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 | |
| 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'), | |
| '6aa76066-55ef-4238-ade6-0b32334a4097'::uuid, | |
| 'service role should still call get_user_id' | |
| ); |
🤖 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 59 - 64,
The test compares the uuid returned by get_user_id(...) against a text literal,
causing a type mismatch; update the assertion in
45_test_apikey_oracle_permissions.sql to cast the expected literal to uuid
(e.g., append ::uuid) so the comparison matches the function return type,
mirroring the approach used in 10_utility_functions.sql and ensuring
get_user_id(...) is compared as uuid.
|
1 participant
Summary (AI generated)
authenticated, keepingget_user_id(both overloads) andget_org_perm_for_apikeyavailable only toservice_role.service_roleis allowed.get_user_idutility coverage executes underservice_roleinsupabase/tests/10_utility_functions.sql.Motivation (AI generated)
Granting authenticated access to key-disclosure helpers preserves an API-key oracle for all logged-in users. Restricting these RPCs to service role only closes that gap and aligns privileges with internal-only usage.
Business Impact (AI generated)
This reduces key-identity leakage risk and strengthens API security posture without changing public endpoint behavior. It also keeps CI stable by matching tests to current permission controls.
Test Plan (AI generated)
bun lint:backend.07_auth_functions.sql,10_utility_functions.sql,19_test_identity_functions.sql,45_test_apikey_oracle_permissions.sql.get_user_id/get_org_perm_for_apikeyby non-service-role callers in SQL tests.Generated with AI
Summary by CodeRabbit
Security
Tests