Fix audit_logs RLS timeout#1588
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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.0)supabase/migrations/20260205120000_fix_audit_logs_select_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: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical performance issue where audit_logs SELECT queries were timing out due to per-row identity resolution in the RLS policy. The solution replaces the per-row get_identity_org_allowed() call with a new statement-scoped audit_logs_allowed_orgs() function that computes the list of accessible org_ids once per query, enabling index-friendly filtering with org_id = ANY(...).
Changes:
- Introduced
audit_logs_allowed_orgs()function that performs authentication and authorization checks once per statement instead of per row - Updated the audit_logs RLS policy to use the new function with an index-friendly predicate
- Added comprehensive tests to verify the new function behavior and policy implementation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| supabase/migrations/20260205120000_fix_audit_logs_select_rls.sql | Implements new audit_logs_allowed_orgs() SECURITY DEFINER function and replaces the RLS policy to use statement-scoped authorization |
| supabase/tests/40_test_audit_log_apikey.sql | Adds 3 new tests for audit_logs_allowed_orgs() function behavior and verifies policy implementation; updates test count and renumbers existing tests |
| CREATE OR REPLACE FUNCTION "public"."audit_logs_allowed_orgs"() | ||
| RETURNS "uuid"[] | ||
| LANGUAGE "plpgsql" STABLE SECURITY DEFINER | ||
| SET "search_path" TO '' | ||
| AS $$ | ||
| DECLARE | ||
| v_user_id uuid; | ||
| v_api_key_text text; | ||
| v_api_key public.apikeys%ROWTYPE; | ||
| v_allowed uuid[] := '{}'::uuid[]; | ||
| v_org_id uuid; | ||
| v_use_rbac boolean; | ||
| v_perm text := public.rbac_permission_for_legacy( | ||
| public.rbac_right_super_admin(), | ||
| public.rbac_scope_org() | ||
| ); | ||
| v_enforcing_2fa boolean; | ||
| BEGIN | ||
| SELECT auth.uid() INTO v_user_id; | ||
|
|
||
| -- If no authenticated user, attempt Capgo API key auth (capgkey header). | ||
| IF v_user_id IS NULL THEN | ||
| SELECT public.get_apikey_header() INTO v_api_key_text; | ||
| IF v_api_key_text IS NULL THEN | ||
| RETURN v_allowed; | ||
| END IF; | ||
|
|
||
| SELECT * FROM public.find_apikey_by_value(v_api_key_text) INTO v_api_key; | ||
| IF v_api_key.id IS NULL THEN | ||
| RETURN v_allowed; | ||
| END IF; | ||
|
|
||
| IF NOT (v_api_key.mode = ANY('{read,upload,write,all}'::public.key_mode[])) THEN | ||
| RETURN v_allowed; | ||
| END IF; | ||
|
|
||
| IF public.is_apikey_expired(v_api_key.expires_at) THEN | ||
| RETURN v_allowed; | ||
| END IF; | ||
|
|
||
| v_user_id := v_api_key.user_id; | ||
| END IF; | ||
|
|
||
| -- Collect candidate orgs from legacy + RBAC bindings. | ||
| FOR v_org_id IN | ||
| SELECT DISTINCT org_id | ||
| FROM ( | ||
| SELECT ou.org_id | ||
| FROM public.org_users ou | ||
| WHERE ou.user_id = v_user_id | ||
| AND ou.org_id IS NOT NULL | ||
| AND ou.app_id IS NULL | ||
| AND ou.channel_id IS NULL | ||
| UNION | ||
| SELECT rb.org_id | ||
| FROM public.role_bindings rb | ||
| WHERE rb.principal_type = public.rbac_principal_user() | ||
| AND rb.principal_id = v_user_id | ||
| AND rb.scope_type = public.rbac_scope_org() | ||
| AND rb.org_id IS NOT NULL | ||
| UNION | ||
| SELECT rb.org_id | ||
| FROM public.role_bindings rb | ||
| WHERE v_api_key.rbac_id IS NOT NULL | ||
| AND rb.principal_type = public.rbac_principal_apikey() | ||
| AND rb.principal_id = v_api_key.rbac_id | ||
| AND rb.scope_type = public.rbac_scope_org() | ||
| AND rb.org_id IS NOT NULL | ||
| ) candidates | ||
| LOOP | ||
| -- Enforce API key org restrictions (if present). | ||
| IF v_api_key.id IS NOT NULL | ||
| AND COALESCE(array_length(v_api_key.limited_to_orgs, 1), 0) > 0 | ||
| AND NOT (v_org_id = ANY(v_api_key.limited_to_orgs)) | ||
| THEN | ||
| CONTINUE; | ||
| END IF; | ||
|
|
||
| v_use_rbac := public.rbac_is_enabled_for_org(v_org_id); | ||
|
|
||
| IF NOT v_use_rbac THEN | ||
| -- Legacy rights (also enforces org 2FA + password policy). | ||
| IF public.check_min_rights_legacy( | ||
| 'super_admin'::public.user_min_right, | ||
| v_user_id, | ||
| v_org_id, | ||
| NULL::character varying, | ||
| NULL::bigint | ||
| ) THEN | ||
| v_allowed := array_append(v_allowed, v_org_id); | ||
| END IF; | ||
| ELSE | ||
| -- Mirror check_min_rights() org gating for RBAC orgs (2FA + password policy). | ||
| SELECT o.enforcing_2fa INTO v_enforcing_2fa | ||
| FROM public.orgs o | ||
| WHERE o.id = v_org_id; | ||
|
|
||
| IF v_enforcing_2fa = true AND NOT public.has_2fa_enabled(v_user_id) THEN | ||
| CONTINUE; | ||
| END IF; | ||
|
|
||
| IF NOT public.user_meets_password_policy(v_user_id, v_org_id) THEN | ||
| CONTINUE; | ||
| END IF; | ||
|
|
||
| -- Allow if the user or the API key principal has the required RBAC permission. | ||
| IF public.rbac_has_permission( | ||
| public.rbac_principal_user(), | ||
| v_user_id, | ||
| v_perm, | ||
| v_org_id, | ||
| NULL::character varying, | ||
| NULL::bigint | ||
| ) THEN | ||
| v_allowed := array_append(v_allowed, v_org_id); | ||
| ELSIF v_api_key.id IS NOT NULL | ||
| AND v_api_key.rbac_id IS NOT NULL | ||
| AND public.rbac_has_permission( | ||
| public.rbac_principal_apikey(), | ||
| v_api_key.rbac_id, | ||
| v_perm, | ||
| v_org_id, | ||
| NULL::character varying, | ||
| NULL::bigint | ||
| ) | ||
| THEN | ||
| v_allowed := array_append(v_allowed, v_org_id); | ||
| END IF; | ||
| END IF; | ||
| END LOOP; | ||
|
|
||
| RETURN v_allowed; | ||
| END; | ||
| $$; |
There was a problem hiding this comment.
The audit_logs_allowed_orgs function is marked as SECURITY DEFINER and will be executed by the "anon" role (via RLS policy on line 148), but it calls several helper functions that explicitly deny execution to "anon":
- rbac_permission_for_legacy (line 22-25) - revoked from anon in migration 20260123140712_fix_rbac_perf_security.sql:500
- rbac_is_enabled_for_org (line 88) - revoked from anon in migration 20260123140712_fix_rbac_perf_security.sql:494
- check_min_rights_legacy (line 92) - needs verification but likely restricted
- rbac_has_permission (lines 116, 127) - revoked from anon in migration 20260123140712_fix_rbac_perf_security.sql:488
- user_meets_password_policy (line 111) - revoked from anon in migration 20251228100000_password_policy_enforcement.sql:133
- has_2fa_enabled with user_id param (line 107) - revoked from anon in migration 20251224103713_2fa_enforcement.sql:53
When an unauthenticated API key request (using anon role) triggers the RLS policy, the audit_logs_allowed_orgs function will execute with the anon role's permissions (not elevated by SECURITY DEFINER for nested calls), causing these helper function calls to fail with permission denied errors.
To fix this, either:
- Grant EXECUTE on these functions to anon (if safe given SECURITY DEFINER protections)
- Restructure audit_logs_allowed_orgs to avoid calling restricted functions
- Call the functions with explicit permission elevation where needed
| SELECT rb.org_id | ||
| FROM public.role_bindings rb | ||
| WHERE v_api_key.rbac_id IS NOT NULL | ||
| AND rb.principal_type = public.rbac_principal_apikey() | ||
| AND rb.principal_id = v_api_key.rbac_id | ||
| AND rb.scope_type = public.rbac_scope_org() | ||
| AND rb.org_id IS NOT NULL |
There was a problem hiding this comment.
In the third UNION query, the WHERE clause references v_api_key.rbac_id (line 73), but v_api_key is only populated when v_user_id is NULL and an API key is found (lines 37-50). When a user authenticates via JWT (v_user_id from auth.uid() is not NULL), v_api_key remains uninitialized as a ROWTYPE.
In PostgreSQL, when a ROWTYPE variable is declared but not assigned, its fields are NULL. This means:
- For JWT-authenticated users: v_api_key.rbac_id will be NULL, and the WHERE clause "v_api_key.rbac_id IS NOT NULL" (line 73) will correctly exclude this UNION branch
- For API key users: v_api_key is populated and the query works as intended
While this works correctly due to the IS NOT NULL check, it relies on implicit NULL behavior for uninitialized ROWTYPEs. For clarity and maintainability, consider explicitly initializing v_api_key or restructuring to make the intent clearer, such as only including this UNION branch within an IF block that checks if v_api_key.id IS NOT NULL.
Summary (AI generated)
audit_logsunfiltered selects timing out by replacing per-row identity evaluation in RLS with statement-scopedaudit_logs_allowed_orgs().Motivation (AI generated)
/rest/v1/audit_logs?select=id&limit=1timed out (57014) and parallel load caused other endpoints to time out.Business Impact (AI generated)
Test plan (AI generated)
bun lint:backendbun lintbun test:backendScreenshots (AI generated)
Checklist (AI generated)
bun run lint:backend && bun run lint.Generated with AI
Summary by CodeRabbit
Bug Fixes
Tests