Conversation
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughThe pull request introduces three new SQL functions to enforce 2FA and permission checks across organizations, apps, channels, and API keys, supporting both RBAC and legacy permission validation paths. Comprehensive tests verify the functions' behavior under various 2FA enforcement scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller/Application
participant CheckFn as check_min_rights()
participant OrgDeriv as Org Derivation
participant TwoFA as 2FA Check
participant RBACPath as RBAC Path
participant LegacyPath as Legacy Path
participant Logging as Logging/Audit
Caller->>CheckFn: check_min_rights(min_right, user_id, org_id, app_id, channel_id)
CheckFn->>OrgDeriv: Derive effective org from app/channel if needed
OrgDeriv-->>CheckFn: effective_org_id
CheckFn->>TwoFA: Check if org enforces 2FA
TwoFA-->>CheckFn: 2fa_enforced boolean
alt 2FA Enforced and User Lacks 2FA
TwoFA->>Logging: Log denial event
Logging-->>CheckFn: failure
CheckFn-->>Caller: false
else Proceed with Permission Check
alt RBAC Enabled for Org
CheckFn->>RBACPath: Check RBAC permission with user/apikey
RBACPath-->>CheckFn: permission_result
else Legacy System
CheckFn->>LegacyPath: Check legacy min_right
LegacyPath-->>CheckFn: permission_result
end
alt Permission Check Failed
CheckFn->>Logging: Log denial event
Logging-->>CheckFn: logged
end
CheckFn-->>Caller: permission_result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1319c62dbd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| -- Enforce 2FA if the org requires it. | ||
| IF v_effective_org_id IS NOT NULL THEN | ||
| SELECT enforcing_2fa INTO v_org_enforcing_2fa FROM public.orgs WHERE id = v_effective_org_id; | ||
| IF v_org_enforcing_2fa = true AND (user_id IS NULL OR NOT public.has_2fa_enabled(user_id)) THEN |
There was a problem hiding this comment.
Avoid blocking RBAC API keys on 2FA-only null user_id
The new 2FA gate returns false whenever user_id is NULL, but RBAC API key requests typically call check_min_rights with auth.uid() (NULL for anon) and rely on the later API key principal check. With this early return, any API key access to an org that enforces 2FA will be denied even if the API key belongs to a user who has 2FA enabled—this is a regression compared to legacy flows that resolve the user ID from the key. Consider resolving the user from the API key header before the 2FA check (or deferring the 2FA check until after key resolution) so valid API keys remain usable in 2FA-enforced orgs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@supabase/migrations/20260127120000_enforce_2fa_in_permission_checks.sql`:
- Around line 66-74: The APikey lookup uses a direct comparison WHERE key =
v_apikey which breaks for hashed keys; replace that query with the proper lookup
function public.find_apikey_by_value(v_apikey) (or the project’s equivalent) to
resolve the apikey record and extract rbac_id into v_apikey_principal, i.e.,
call public.find_apikey_by_value with v_apikey and assign its rbac_id result to
v_apikey_principal before calling public.rbac_has_permission; update the block
that references v_apikey and v_apikey_principal to use the function instead of
the direct WHERE key = v_apikey lookup.
- Around line 61-74: When falling back to API-key principal (the block that
reads get_apikey_header() into v_apikey and rbac_id into v_apikey_principal),
resolve the API key's associated user ID (e.g., SELECT owner_user_id or
equivalent from public.apikeys INTO a new v_apikey_user_id) and, if
v_org_enforcing_2fa is true, run the same 2FA verification logic used earlier
for the incoming user_id (reuse the exact check applied to user_id) against that
resolved user before allowing the API-key permission; only call
public.rbac_has_permission(public.rbac_principal_apikey(), v_apikey_principal,
...) and set v_allowed when both the RBAC check and the 2FA compliance for the
API-key owner pass (mirror the behavior in rbac_check_permission_direct).
- Around line 213-222: Replace the direct lookup of an API key by its plain
"key" column with the helper that handles hashed keys: instead of querying
public.apikeys WHERE key = p_apikey to set v_apikey_principal, call the existing
find_apikey_by_value(p_apikey) (or the module's equivalent) to resolve the
apikey row and obtain its rbac_id; then use that rbac_id to call
public.rbac_has_permission (keeping v_apikey_principal, p_apikey,
p_permission_key, v_effective_org_id, p_app_id, p_channel_id and
public.rbac_principal_apikey() unchanged). Ensure the replacement mirrors the
earlier usage at line ~180 so hashed keys (key_hash) are correctly handled.
🧹 Nitpick comments (3)
supabase/tests/43_test_rbac_permission_2fa.sql (2)
85-93: Consider addingowner_orgto API key for completeness.The API key is created without
owner_org, which is typically required to associate the key with an organization. While Test 4 validates 2FA denial (which happens before org-based permission checks), addingowner_orgwould make the fixture more realistic and enable additional test scenarios.INSERT INTO public.apikeys (id, user_id, key, mode, name) VALUES ( 9101, tests.get_supabase_uid('test_rbac_no2fa_user'), 'test-rbac-no2fa-key', 'all'::public.key_mode, - 'Test RBAC No 2FA Key' + 'Test RBAC No 2FA Key', + current_setting('test.rbac_org_with_2fa')::uuid -- owner_org );Note: This requires moving the API key INSERT after the org setup block, or restructuring the setup.
100-158: Good test coverage for core 2FA enforcement scenarios.The four tests cover the essential matrix:
- ✓ 2FA user + 2FA-enforcing org → allowed
- ✓ non-2FA user + 2FA-enforcing org → denied
- ✓ non-2FA user + non-2FA org → allowed
- ✓ API key (non-2FA owner) + 2FA-enforcing org → denied
Consider adding a positive test for API key authentication when the key's owner has 2FA enabled, to verify API keys work correctly for compliant users in 2FA-enforcing orgs.
supabase/migrations/20260127120000_enforce_2fa_in_permission_checks.sql (1)
3-12: Consider restricting function access to prevent permission enumeration.These functions accept
user_idas a parameter and could allow callers to probe permissions for arbitrary users. Per retrieved learnings, inner functions with user_id parameters should not be granted to anon/authenticated roles if they allow querying other users' data.If these functions are meant to be called only by RLS policies or other trusted functions, consider either:
- Not granting execute to
anon/authenticatedroles- Adding an ownership check that
user_idmatches the callerRun the following script to check existing GRANT patterns for similar functions:
#!/bin/bash # Check if similar permission functions have restricted grants rg -n 'GRANT.*ON FUNCTION.*check_min_rights' supabase/migrations/ rg -n 'GRANT.*ON FUNCTION.*rbac_' supabase/migrations/
| IF user_id IS NOT NULL THEN | ||
| v_allowed := public.rbac_has_permission(public.rbac_principal_user(), user_id, v_perm, v_effective_org_id, app_id, channel_id); | ||
| END IF; | ||
|
|
||
| -- Also consider apikey principal when RBAC is enabled (API keys can hold roles directly). | ||
| IF NOT v_allowed THEN | ||
| SELECT public.get_apikey_header() INTO v_apikey; | ||
| IF v_apikey IS NOT NULL THEN | ||
| SELECT rbac_id INTO v_apikey_principal FROM public.apikeys WHERE key = v_apikey LIMIT 1; | ||
| IF v_apikey_principal IS NOT NULL THEN | ||
| v_allowed := public.rbac_has_permission(public.rbac_principal_apikey(), v_apikey_principal, v_perm, v_effective_org_id, app_id, channel_id); | ||
| END IF; | ||
| END IF; | ||
| END IF; |
There was a problem hiding this comment.
API key principal not verified for 2FA compliance.
When the user permission check fails at line 62 and the function falls back to API key permission check at lines 66-74, the API key's associated user is not verified for 2FA. The 2FA check at line 34 only validates the passed-in user_id parameter, not the API key owner.
Compare with rbac_check_permission_direct (lines 177-182, 190) which resolves the effective user from API key and then verifies 2FA for that resolved user.
Consider resolving the API key's user and checking their 2FA status before granting permission via API key path when v_org_enforcing_2fa = true.
🤖 Prompt for AI Agents
In `@supabase/migrations/20260127120000_enforce_2fa_in_permission_checks.sql`
around lines 61 - 74, When falling back to API-key principal (the block that
reads get_apikey_header() into v_apikey and rbac_id into v_apikey_principal),
resolve the API key's associated user ID (e.g., SELECT owner_user_id or
equivalent from public.apikeys INTO a new v_apikey_user_id) and, if
v_org_enforcing_2fa is true, run the same 2FA verification logic used earlier
for the incoming user_id (reuse the exact check applied to user_id) against that
resolved user before allowing the API-key permission; only call
public.rbac_has_permission(public.rbac_principal_apikey(), v_apikey_principal,
...) and set v_allowed when both the RBAC check and the 2FA compliance for the
API-key owner pass (mirror the behavior in rbac_check_permission_direct).
| IF NOT v_allowed THEN | ||
| SELECT public.get_apikey_header() INTO v_apikey; | ||
| IF v_apikey IS NOT NULL THEN | ||
| SELECT rbac_id INTO v_apikey_principal FROM public.apikeys WHERE key = v_apikey LIMIT 1; | ||
| IF v_apikey_principal IS NOT NULL THEN | ||
| v_allowed := public.rbac_has_permission(public.rbac_principal_apikey(), v_apikey_principal, v_perm, v_effective_org_id, app_id, channel_id); | ||
| END IF; | ||
| END IF; | ||
| END IF; |
There was a problem hiding this comment.
Same inconsistent API key lookup issue as in rbac_check_permission_direct.
Line 69 uses direct key comparison WHERE key = v_apikey which may fail for hashed keys. If find_apikey_by_value() exists to handle key lookups properly, it should be used here as well.
🔧 Suggested fix
IF NOT v_allowed THEN
SELECT public.get_apikey_header() INTO v_apikey;
IF v_apikey IS NOT NULL THEN
- SELECT rbac_id INTO v_apikey_principal FROM public.apikeys WHERE key = v_apikey LIMIT 1;
+ SELECT rbac_id INTO v_apikey_principal FROM public.find_apikey_by_value(v_apikey) LIMIT 1;
IF v_apikey_principal IS NOT NULL THEN🤖 Prompt for AI Agents
In `@supabase/migrations/20260127120000_enforce_2fa_in_permission_checks.sql`
around lines 66 - 74, The APikey lookup uses a direct comparison WHERE key =
v_apikey which breaks for hashed keys; replace that query with the proper lookup
function public.find_apikey_by_value(v_apikey) (or the project’s equivalent) to
resolve the apikey record and extract rbac_id into v_apikey_principal, i.e.,
call public.find_apikey_by_value with v_apikey and assign its rbac_id result to
v_apikey_principal before calling public.rbac_has_permission; update the block
that references v_apikey and v_apikey_principal to use the function instead of
the direct WHERE key = v_apikey lookup.
| IF NOT v_allowed AND p_apikey IS NOT NULL THEN | ||
| SELECT rbac_id INTO v_apikey_principal | ||
| FROM public.apikeys | ||
| WHERE key = p_apikey | ||
| LIMIT 1; | ||
|
|
||
| IF v_apikey_principal IS NOT NULL THEN | ||
| v_allowed := public.rbac_has_permission(public.rbac_principal_apikey(), v_apikey_principal, p_permission_key, v_effective_org_id, p_app_id, p_channel_id); | ||
| END IF; | ||
| END IF; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the specific migration file
git ls-files supabase/migrations/ | grep -E '20260127120000.*enforce_2fa'Repository: Cap-go/capgo
Length of output: 127
🏁 Script executed:
# Also search for find_apikey_by_value to understand its implementation
rg -n 'find_apikey_by_value' supabase/migrations/ supabase/functions/ --context 5Repository: Cap-go/capgo
Length of output: 33572
🏁 Script executed:
# Check the apikeys table definition
rg -n 'CREATE TABLE.*apikeys' supabase/migrations/ -A 20Repository: Cap-go/capgo
Length of output: 1798
🏁 Script executed:
# Look for the specific migration file with broader search
fd '20260127' supabase/migrations/Repository: Cap-go/capgo
Length of output: 127
🏁 Script executed:
# Read the specific migration file around the mentioned lines
head -n 260 supabase/migrations/20260127120000_enforce_2fa_in_permission_checks.sql | tail -n 130Repository: Cap-go/capgo
Length of output: 4563
🏁 Script executed:
# Search for the rbac_check_permission_direct function to see the full context
rg -n 'rbac_check_permission_direct' supabase/migrations/20260127120000_enforce_2fa_in_permission_checks.sql -A 130Repository: Cap-go/capgo
Length of output: 4849
🏁 Script executed:
# Check if apikeys table has key_hash column added by hashed_api_keys migration
rg -n 'key_hash' supabase/migrations/20251228080032_hashed_api_keys.sql -A 2 -B 2Repository: Cap-go/capgo
Length of output: 1996
🏁 Script executed:
# Check the find_apikey_by_value implementation to confirm it handles hashing
rg -n 'CREATE OR REPLACE FUNCTION.*find_apikey_by_value' supabase/migrations/20260105150626_fix_is_allowed_capgkey_hashed_apikeys.sql -A 20Repository: Cap-go/capgo
Length of output: 899
🏁 Script executed:
# Check if apikeys table has an rbac_id column
rg -n 'rbac_id' supabase/migrations/ | grep -i apikeys | head -20Repository: Cap-go/capgo
Length of output: 1007
🏁 Script executed:
# Get full apikeys table structure from migrations to see all columns
rg -n 'ALTER TABLE.*apikeys' supabase/migrations/20251228080032_hashed_api_keys.sql -A 5 -B 5Repository: Cap-go/capgo
Length of output: 739
🏁 Script executed:
# Verify the flow: confirm find_apikey_by_value returns rbac_id column
rg -n 'find_apikey_by_value' supabase/functions/_backend/utils/supabase.types.ts -A 15Repository: Cap-go/capgo
Length of output: 641
Use find_apikey_by_value() for consistent hashed API key lookup in RBAC permission check.
At line 216, the code queries WHERE key = p_apikey to retrieve the rbac_id for permission checking. However, hashed API keys have key = NULL and store the hash in key_hash, so this direct comparison will never match hashed keys. Line 180 correctly uses find_apikey_by_value() which handles both plain and hashed keys—the same function should be used at line 216 for consistency.
🔧 Suggested fix
IF NOT v_allowed AND p_apikey IS NOT NULL THEN
- SELECT rbac_id INTO v_apikey_principal
- FROM public.apikeys
- WHERE key = p_apikey
- LIMIT 1;
+ SELECT rbac_id INTO v_apikey_principal
+ FROM public.find_apikey_by_value(p_apikey)
+ LIMIT 1;
IF v_apikey_principal IS NOT NULL THEN📝 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.
| IF NOT v_allowed AND p_apikey IS NOT NULL THEN | |
| SELECT rbac_id INTO v_apikey_principal | |
| FROM public.apikeys | |
| WHERE key = p_apikey | |
| LIMIT 1; | |
| IF v_apikey_principal IS NOT NULL THEN | |
| v_allowed := public.rbac_has_permission(public.rbac_principal_apikey(), v_apikey_principal, p_permission_key, v_effective_org_id, p_app_id, p_channel_id); | |
| END IF; | |
| END IF; | |
| IF NOT v_allowed AND p_apikey IS NOT NULL THEN | |
| SELECT rbac_id INTO v_apikey_principal | |
| FROM public.find_apikey_by_value(p_apikey) | |
| LIMIT 1; | |
| IF v_apikey_principal IS NOT NULL THEN | |
| v_allowed := public.rbac_has_permission(public.rbac_principal_apikey(), v_apikey_principal, p_permission_key, v_effective_org_id, p_app_id, p_channel_id); | |
| END IF; | |
| END IF; |
🤖 Prompt for AI Agents
In `@supabase/migrations/20260127120000_enforce_2fa_in_permission_checks.sql`
around lines 213 - 222, Replace the direct lookup of an API key by its plain
"key" column with the helper that handles hashed keys: instead of querying
public.apikeys WHERE key = p_apikey to set v_apikey_principal, call the existing
find_apikey_by_value(p_apikey) (or the module's equivalent) to resolve the
apikey row and obtain its rbac_id; then use that rbac_id to call
public.rbac_has_permission (keeping v_apikey_principal, p_apikey,
p_permission_key, v_effective_org_id, p_app_id, p_channel_id and
public.rbac_principal_apikey() unchanged). Ensure the replacement mirrors the
earlier usage at line ~180 so hashed keys (key_hash) are correctly handled.
|
* fix(db): enforce 2fa in permission checks * fix(db): restore password policy checks
Summary (AI generated)
Test plan (AI generated)
Screenshots (AI generated)
Checklist (AI generated)
Generated with AI
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.