fix(db): enforce API key expiry in get_user_org_ids#1809
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new SECURITY DEFINER PostgreSQL function Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DB as Postgres (public.get_user_org_ids)
participant ApikeyTbl as apikeys table
participant RBAC as role_bindings / groups / apps / channels / org_users
participant Audit as denial_logging
Client->>DB: CALL public.get_user_org_ids() (with or without X-Client-Api-Key)
DB->>ApikeyTbl: lookup API key (hashed/plain)
alt API key found
ApikeyTbl-->>DB: apikey record (includes expiry, limited_orgs)
DB->>DB: validate expiry (is_apikey_expired)
alt expired
DB->>Audit: INSERT denial event
DB-->>Client: return no orgs / error (per logic)
else valid
DB->>RBAC: collect org_ids from role_bindings, groups, apps, channels, org_users
DB-->>DB: union all orgs, apply limited_orgs filter if present
DB-->>Client: return setof org_id
end
else no API key
DB->>RBAC: collect org_ids for current identity/session
DB-->>Client: return setof org_id
end
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)
📝 Coding Plan
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/20260317021715_fix_get_user_org_ids_apikey_expiry.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
🧹 Nitpick comments (1)
supabase/tests/42_test_apikey_expiration.sql (1)
416-444: Strengthen the happy-path regression forget_user_org_ids().
count(*) > 0only proves the RPC returned something. A regression that returns the wrong org set, or breaks the no-header authenticated fallback introduced by the new control flow, would still pass. Please assert the expected org IDs/count for the seeded user and add one session-authenticated case with nocapgkeyheader.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/tests/42_test_apikey_expiration.sql` around lines 416 - 444, The happy-path assertion for get_user_org_ids() is too weak—replace the loose check (count(*) > 0) with assertions that verify the exact expected org IDs and/or exact count for the seeded user when using the valid API key 'test-key-orgs-valid' (e.g., compare the result set or join to an expected array), and add an additional test case that clears the capgkey header (set_config('request.headers','{}', true) or omit capgkey) to exercise the session-authenticated fallback path and assert the same expected org IDs/count; target the test blocks that call get_user_org_ids() and the set_config calls for 'test-key-orgs-valid'/'test-key-orgs-expired' so you update those SELECT ... ok(...) checks to perform precise equality checks against the expected org id list or count.
🤖 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/20260317021715_fix_get_user_org_ids_apikey_expiry.sql`:
- Around line 1-4: The migration creates/updates the SECURITY DEFINER function
get_user_org_ids but does not make its ACL explicit; modify the migration to
harden the function ACL by first revoking all privileges from PUBLIC for the
specific function signature (the get_user_org_ids() overload) and then granting
only the minimal required role(s) (e.g., the application role and/or specific
admin role) the EXECUTE privilege; ensure the REVOKE ALL and GRANT EXECUTE
statements reference the exact function name and signature (get_user_org_ids())
and are executed after the CREATE OR REPLACE FUNCTION to enforce consistent
permissions across all instances.
---
Nitpick comments:
In `@supabase/tests/42_test_apikey_expiration.sql`:
- Around line 416-444: The happy-path assertion for get_user_org_ids() is too
weak—replace the loose check (count(*) > 0) with assertions that verify the
exact expected org IDs and/or exact count for the seeded user when using the
valid API key 'test-key-orgs-valid' (e.g., compare the result set or join to an
expected array), and add an additional test case that clears the capgkey header
(set_config('request.headers','{}', true) or omit capgkey) to exercise the
session-authenticated fallback path and assert the same expected org IDs/count;
target the test blocks that call get_user_org_ids() and the set_config calls for
'test-key-orgs-valid'/'test-key-orgs-expired' so you update those SELECT ...
ok(...) checks to perform precise equality checks against the expected org id
list or count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23e33c6d-6871-4b6e-a53f-b12a9c9bcfbb
📒 Files selected for processing (2)
supabase/migrations/20260317021715_fix_get_user_org_ids_apikey_expiry.sqlsupabase/tests/42_test_apikey_expiration.sql
|
1 participant
Summary (AI generated)
public.get_user_org_ids()find_apikey_by_value()andis_apikey_expired()get_user_org_ids()Motivation (AI generated)
The original PR fixed the logic in the wrong place by editing an already-applied migration. This follow-up ships the same auth hardening through a new migration, which is the only way to patch existing databases without breaking fresh migration replay.
Business Impact (AI generated)
This closes an API-key-expiration bypass in a user/org lookup RPC while keeping deployment safe. It reduces stale-key exposure and avoids rollout regressions caused by historical-migration drift.
Test Plan (AI generated)
git diff --checkget_user_org_ids()bun run supabase:db:resetbunx supabase test dbGenerated with AI
Summary by CodeRabbit
New Features
Tests