[codex] fix scoped API key org boundary bypass#1951
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughEnforces API-key org/app scoping in RBAC checks via two new DB functions, simplifies organization deletion by removing storage cleanup and only deleting the org record, tightens member deletion to verify the removed row, and adds tests asserting scoped API keys cannot act across org boundaries. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant TS as "TypeScript Handler"
participant DB as "Database"
participant RBAC as "public.rbac_check_permission_direct"
participant Key as "public.find_apikey_by_value"
participant Scope as "Org/App Scope Check"
participant TwoFA as "2FA Enforcement"
participant Perm as "RBAC / Legacy Permission Check"
Client->>TS: DELETE /organization?orgId=...
TS->>DB: call rbac_check_permission_direct(..., p_apikey)
DB->>RBAC: evaluate permission
RBAC->>Key: validate apikey
alt apikey missing/expired
Key-->>RBAC: not found / expired
RBAC->>DB: log denial
RBAC-->>TS: false
else apikey valid
Key-->>RBAC: apikey data (limited_to_orgs/apps, user_id)
RBAC->>Scope: check limited_to_orgs/apps
alt outside scope
Scope-->>RBAC: denied
RBAC-->>TS: false
else within scope
Scope-->>RBAC: allowed
RBAC->>TwoFA: check org requires 2FA / user 2FA
alt 2FA required and user lacks it
TwoFA-->>RBAC: denied
RBAC->>DB: log denial
RBAC-->>TS: false
else 2FA ok or not required
TwoFA-->>RBAC: ok
RBAC->>Perm: evaluate RBAC (and channel overrides) or legacy fallback
Perm-->>RBAC: permission result
RBAC-->>TS: true/false
end
end
end
alt permitted
TS->>DB: delete org record
DB-->>TS: success
TS-->>Client: 200 / success
else denied
TS-->>Client: 403 invalid_org_id
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Member
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
supabase/migrations/20260424094101_enforce_apikey_scope_in_rbac_check.sql (1)
1-3: Consider adding explicit privilege controls for these SECURITY DEFINER functions.Per coding guidelines, non-trigger PostgreSQL RPC functions should apply minimum privileges: REVOKE ALL FROM PUBLIC and grant only to required roles. These callable SECURITY DEFINER functions run with elevated privileges and should restrict who can invoke them.
♻️ Suggested privilege controls (append after each function)
-- After rbac_check_permission_direct (line 250) ALTER FUNCTION "public"."rbac_check_permission_direct"("text", "uuid", "uuid", character varying, bigint, "text") OWNER TO "postgres"; REVOKE ALL ON FUNCTION "public"."rbac_check_permission_direct"("text", "uuid", "uuid", character varying, bigint, "text") FROM PUBLIC; GRANT EXECUTE ON FUNCTION "public"."rbac_check_permission_direct"("text", "uuid", "uuid", character varying, bigint, "text") TO "service_role"; GRANT EXECUTE ON FUNCTION "public"."rbac_check_permission_direct"("text", "uuid", "uuid", character varying, bigint, "text") TO "authenticated"; -- After rbac_check_permission_direct_no_password_policy (line 426) ALTER FUNCTION "public"."rbac_check_permission_direct_no_password_policy"("text", "uuid", "uuid", character varying, bigint, "text") OWNER TO "postgres"; REVOKE ALL ON FUNCTION "public"."rbac_check_permission_direct_no_password_policy"("text", "uuid", "uuid", character varying, bigint, "text") FROM PUBLIC; GRANT EXECUTE ON FUNCTION "public"."rbac_check_permission_direct_no_password_policy"("text", "uuid", "uuid", character varying, bigint, "text") TO "service_role"; GRANT EXECUTE ON FUNCTION "public"."rbac_check_permission_direct_no_password_policy"("text", "uuid", "uuid", character varying, bigint, "text") TO "authenticated";As per coding guidelines: "For PostgreSQL RPC and helper functions, apply minimum privileges explicitly: start from deny-by-default, set OWNER explicitly, use REVOKE ALL ... FROM PUBLIC, and grant only required roles."
Also applies to: 253-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260424094101_enforce_apikey_scope_in_rbac_check.sql` around lines 1 - 3, Add explicit privilege controls for the SECURITY DEFINER functions: for "rbac_check_permission_direct" and "rbac_check_permission_direct_no_password_policy" set the OWNER to postgres, revoke all privileges from PUBLIC, and grant EXECUTE only to the required roles (service_role and authenticated); append these ALTER/REVOKE/GRANT statements right after each function definition so the functions are deny-by-default and only callable by the intended roles.supabase/functions/_backend/public/organization/members/delete.ts (1)
58-67: Note: Explicit role_bindings delete may be redundant with database trigger.Per
supabase/migrations/20260311162400_sync_org_user_delete_role_bindings.sql, thesync_org_user_role_binding_on_deletetrigger already callsresync_org_user_role_bindingsto cascade-delete role_bindings when anorg_usersrecord is deleted.This explicit delete serves as a safety net, which is fine. Just noting for awareness that both mechanisms will attempt the cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/public/organization/members/delete.ts` around lines 58 - 67, The explicit delete of role_bindings in the block using supabaseAdmin(...).from('role_bindings').delete() is potentially redundant with the DB trigger sync_org_user_role_binding_on_delete defined in the migration, so either remove this explicit delete to rely on the trigger, or keep it but add a clear comment above the delete referencing the trigger/migration (sync_org_user_role_binding_on_delete in 20260311162400_sync_org_user_delete_role_bindings.sql) and that it is a safety-net; if you keep it, leave the existing error handling (throw simpleError('error_deleting_role_bindings', ...)) as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/rbac-permissions.test.ts`:
- Around line 256-297: The test fails to grant any RBAC role to the scoped API
key for allowedOrgId, so rbac_check_permission_direct finds no role_bindings;
insert a role_bindings entry for principal_type='apikey' with principal_id equal
to the apikey's rbac_id (lookup by scopedKey) and org_id = allowedOrgId,
pointing at a role that contains the 'org.delete' permission (or create a role +
role_permissions mapping that includes 'org.delete') before calling
rbac_check_permission_direct in the allowedResult block so the API key check can
succeed (references: rbac_check_permission_direct, scopedKey, allowedOrgId,
USER_ID, role_bindings, apikeys, roles/role_permissions).
---
Nitpick comments:
In `@supabase/functions/_backend/public/organization/members/delete.ts`:
- Around line 58-67: The explicit delete of role_bindings in the block using
supabaseAdmin(...).from('role_bindings').delete() is potentially redundant with
the DB trigger sync_org_user_role_binding_on_delete defined in the migration, so
either remove this explicit delete to rely on the trigger, or keep it but add a
clear comment above the delete referencing the trigger/migration
(sync_org_user_role_binding_on_delete in
20260311162400_sync_org_user_delete_role_bindings.sql) and that it is a
safety-net; if you keep it, leave the existing error handling (throw
simpleError('error_deleting_role_bindings', ...)) as-is.
In `@supabase/migrations/20260424094101_enforce_apikey_scope_in_rbac_check.sql`:
- Around line 1-3: Add explicit privilege controls for the SECURITY DEFINER
functions: for "rbac_check_permission_direct" and
"rbac_check_permission_direct_no_password_policy" set the OWNER to postgres,
revoke all privileges from PUBLIC, and grant EXECUTE only to the required roles
(service_role and authenticated); append these ALTER/REVOKE/GRANT statements
right after each function definition so the functions are deny-by-default and
only callable by the intended roles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56aebb43-f5fe-41a6-be42-b4f0a041f918
📒 Files selected for processing (5)
supabase/functions/_backend/public/organization/delete.tssupabase/functions/_backend/public/organization/members/delete.tssupabase/migrations/20260424094101_enforce_apikey_scope_in_rbac_check.sqltests/organization-api.test.tstests/rbac-permissions.test.ts
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/apikeys-expiration.test.ts (1)
14-16: Optional: defaultauthHeadersinapiFetchto remove repetition.This can reduce boilerplate and prevent future missed headers in new test cases.
♻️ Proposed refactor
function apiFetch(path: string, init?: RequestInit) { - return fetchWithRetry(`${BASE_URL}${path}`, init) + return fetchWithRetry(`${BASE_URL}${path}`, { + ...init, + headers: init?.headers ?? authHeaders, + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/apikeys-expiration.test.ts` around lines 14 - 16, The apiFetch helper currently calls fetchWithRetry without default headers; update the apiFetch(path: string, init?: RequestInit) function to merge a default authHeaders (e.g., Authorization: `Bearer ${TEST_API_KEY}` or reuse existing authHeaders constant) into init.headers so tests don’t need to repeat headers; ensure merging preserves any headers passed in by callers and still forwards init to fetchWithRetry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/apikeys-expiration.test.ts`:
- Around line 14-16: The apiFetch helper currently calls fetchWithRetry without
default headers; update the apiFetch(path: string, init?: RequestInit) function
to merge a default authHeaders (e.g., Authorization: `Bearer ${TEST_API_KEY}` or
reuse existing authHeaders constant) into init.headers so tests don’t need to
repeat headers; ensure merging preserves any headers passed in by callers and
still forwards init to fetchWithRetry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4dbbd539-d81a-497a-8ad3-d902e0b45745
📒 Files selected for processing (1)
tests/apikeys-expiration.test.ts
This was referenced Apr 27, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary (AI generated)
rbac_check_permission_directbefore any owner-user fallback can authorize out-of-scope accessDELETE /organizationfrom touching storage until the org row delete actually succeedsMotivation (AI generated)
Scoped API keys were able to inherit the owning user's broader permissions and cross organization boundaries on destructive organization-management routes.
Business Impact (AI generated)
This closes a high-severity tenant-isolation bug that could let delegated integrations damage another organization's branding or RBAC state, which directly affects customer trust and incident risk.
Test Plan (AI generated)
bun run lint:backendbunx eslint tests/rbac-permissions.test.ts tests/organization-api.test.tsbun typecheckbun run supabase:with-env -- bunx vitest run tests/rbac-permissions.test.ts tests/organization-api.test.tsGenerated with AI
Summary by CodeRabbit
Bug Fixes
Security
Tests