[codex] fix RBAC mixed-scope role escalation#1942
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds role/binding scope validation, tightens user principal access to prefer accepted org_membership (rejecting invites) with RBAC fallback, introduces a SECURITY DEFINER Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as RoleBindings API
participant OrgUsers as schema.org_users
participant Bindings as schema.role_bindings
participant Roles as schema.roles
participant RBAC as public.rbac_has_permission
Client->>API: POST/PATCH role_binding request
API->>API: validateRoleScope(role.scope_type, binding.scope_type)
alt scope mismatch
API-->>Client: 400 Role scope_type does not match binding scope
else scope match
API->>OrgUsers: SELECT ... FROM schema.org_users WHERE user_id=... LIMIT 10
alt active org membership found
API-->>Client: proceed (ok)
else invite_* membership found
API-->>Client: 400 User has not accepted the org invitation yet
else no qualifying org_users rows
API->>Bindings: SELECT b JOIN Roles r ON b.role_id=r.id WHERE r.scope_type = b.scope_type AND not expired
alt matching RBAC binding found
API-->>Client: proceed (ok)
else
API-->>Client: 400 User is not a member of this org
end
end
end
Note over API,RBAC: Permission checks delegated to public.rbac_has_permission
API->>RBAC: rbac_has_permission(permission_key, caller_org, caller_app, caller_channel, target_scope, timestamp)
RBAC->>Bindings: resolve direct + group-derived bindings within computed scope catalog
RBAC->>Roles: expand via role_hierarchy and aggregate permissions
RBAC-->>API: boolean (permission granted?)
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 unit tests (beta)
Comment |
Contributor
Member
Author
|
@coderabbitai 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: 3
🤖 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/functions/_backend/private/role_bindings.ts`:
- Around line 116-129: The current membership check accepts any role_bindings
row; modify the query that produces targetRbacAccess to ignore expired or
scope-mismatched bindings by adding conditions that role_bindings.expire_at IS
NULL OR role_bindings.expire_at > now() and ensure the binding's scope_type
matches the referenced role's scope_type (join schema.roles on role_id and
require eq(schema.role_bindings.scope_type, schema.roles.scope_type) or
equivalent); this ensures only active, scope-valid direct bindings for
principal_id/org_id are treated as membership proof before returning
success/failure.
In `@supabase/migrations/20260424090111_fix_rbac_scope_mismatch_escalation.sql`:
- Around line 87-100: When p_channel_id is provided, don’t let caller-supplied
p_app_id/p_org_id bleed into scope_catalog; after resolving the channel row
(v_channel_uuid, v_channel_app_id, v_channel_org_id) always canonicalize scope
by deriving v_app_uuid (lookup apps.id by v_channel_app_id) and setting v_org_id
:= v_channel_org_id before building scope_catalog, and if the caller supplied
p_app_id or p_org_id that does not match the channel’s app/org, return FALSE (or
fail) instead of silently using the caller values; update the block that
currently only sets v_app_uuid/v_org_id when NULL to enforce this
canonicalization and early-fail on mismatch.
- Around line 50-165: The SECURITY DEFINER function public.rbac_has_permission
currently lacks explicit OWNER and still has the default PUBLIC EXECUTE
privilege; after creating the function (rbac_has_permission(text, uuid, text,
uuid, character varying, bigint)) set its owner to the intended role (e.g.
postgres), revoke all privileges from PUBLIC, and then explicitly GRANT EXECUTE
only to the specific roles that must call it (do not leave PUBLIC or extra roles
with execute); update the migration to run those ALTER FUNCTION/REVOKE/GRANT
statements immediately after the function definition.
🪄 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: 79f912db-5fdc-462a-8859-fb7a11a50b6e
📒 Files selected for processing (4)
supabase/functions/_backend/private/role_bindings.tssupabase/migrations/20260424090111_fix_rbac_scope_mismatch_escalation.sqltests/private-role-bindings.test.tstests/rbac-permissions.test.ts
…3m2-x66x # Conflicts: # tests/rbac-permissions.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/20260424090111_fix_rbac_scope_mismatch_escalation.sql`:
- Around line 177-181: The migration currently grants EXECUTE on the SECURITY
DEFINER helper rbac_has_permission(text, uuid, text, uuid, character varying,
bigint) to the authenticated role; remove that grant so only definer/service
roles can call it (retain REVOKE ALL ... FROM PUBLIC/anon and keep GRANT to
service_role or owner only), and if callers need client access implement a safe
wrapper (e.g., rbac_has_permission_for_current_user) that derives principal from
the auth context instead of accepting p_principal_type/p_principal_id as
parameters.
- Around line 151-167: The recursive CTE role_closure must carry the role's
scope_type (or scope_family) to prevent cross-scope inheritance: modify
combined_roles to select both role_id and scope_type, change role_closure to be
(role_id, scope_type) seeded from combined_roles, and in the recursive arm join
public.role_hierarchy rh ON rh.parent_role_id = rc.role_id AND rh.scope_type =
rc.scope_type (or rh.scope_family = rc.scope_type/family) so only descendants
from the same scope family are traversed before selecting permissions in
perm_set; update downstream joins (role_closure usage in role_permissions join)
to reference rc.role_id accordingly.
🪄 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: fd762378-9559-4a9f-9b01-23ea20107b87
📒 Files selected for processing (4)
supabase/functions/_backend/private/role_bindings.tssupabase/migrations/20260424090111_fix_rbac_scope_mismatch_escalation.sqltests/private-role-bindings.test.tstests/rbac-permissions.test.ts
✅ Files skipped from review due to trivial changes (1)
- supabase/functions/_backend/private/role_bindings.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/private-role-bindings.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
supabase/tests/13_test_plan_math.sql (1)
43-43: Disable specific triggers instead ofTRIGGER USERto prevent hiding unrelated regressions
DISABLE TRIGGER USERsuppresses all user-defined triggers onapps, includingon_app_updateandcleanup_onboarding_app_data_on_completewhich won't even fire during anowner_orgupdate. Onlyhandle_updated_atandaudit_apps_triggerare affected by this operation. Follow the pattern already used on line 44 and explicitly disable only the necessary triggers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/tests/13_test_plan_math.sql` at line 43, The ALTER TABLE apps DISABLE TRIGGER USER call is too broad; instead disable only the two triggers you intend to suppress by replacing that statement with explicit DISABLE TRIGGER calls for handle_updated_at and audit_apps_trigger (following the same pattern used on the next line), e.g., use ALTER TABLE apps DISABLE TRIGGER handle_updated_at; and ALTER TABLE apps DISABLE TRIGGER audit_apps_trigger; so only those triggers are disabled and other user triggers like on_app_update and cleanup_onboarding_app_data_on_complete remain active.tests/rbac-permissions.test.ts (1)
344-352: Avoid mutating the shared role hierarchy fixture in this regression.This insert rewires the seeded
public.role_hierarchygraph directly. It works under rollback, but it still couples the test to global fixture shape and makes future parallelization harder. Prefer dedicated test roles for this case, or at least make the added edge test-local and idempotent.As per coding guidelines, "When creating tests that interact with shared resources, create dedicated seed data when your test modifies resources; reuse existing seed data only when you read without modification and create your own child resources".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/rbac-permissions.test.ts` around lines 344 - 352, This test mutates the shared public.role_hierarchy by inserting an edge; instead create dedicated test roles and add the hierarchy edge only between those test-local roles (or make the insert idempotent and scoped) instead of using public.roles and public.rbac_role_*(); specifically, use the test setup to INSERT two new roles (e.g., test_parent/test_child) and then INSERT INTO public.role_hierarchy selecting those new role IDs, or wrap the insert in a conditional upsert that only affects test-local role names so you never alter the global seeded graph.
🤖 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/13_test_plan_math.sql`:
- Around line 43-47: The ALTER and UPDATE statements use unqualified table
names; change all references to schema-qualified names (e.g., replace apps and
app_versions with public.apps and public.app_versions or the appropriate schema)
so the ALTER TABLE USER/force_valid_owner_org_app_versions and UPDATE target
fully qualified tables; update the three lines referencing apps and app_versions
to use the schema prefix while keeping the same trigger names and UPDATE
predicate (owner_org/app_id) intact.
---
Nitpick comments:
In `@supabase/tests/13_test_plan_math.sql`:
- Line 43: The ALTER TABLE apps DISABLE TRIGGER USER call is too broad; instead
disable only the two triggers you intend to suppress by replacing that statement
with explicit DISABLE TRIGGER calls for handle_updated_at and audit_apps_trigger
(following the same pattern used on the next line), e.g., use ALTER TABLE apps
DISABLE TRIGGER handle_updated_at; and ALTER TABLE apps DISABLE TRIGGER
audit_apps_trigger; so only those triggers are disabled and other user triggers
like on_app_update and cleanup_onboarding_app_data_on_complete remain active.
In `@tests/rbac-permissions.test.ts`:
- Around line 344-352: This test mutates the shared public.role_hierarchy by
inserting an edge; instead create dedicated test roles and add the hierarchy
edge only between those test-local roles (or make the insert idempotent and
scoped) instead of using public.roles and public.rbac_role_*(); specifically,
use the test setup to INSERT two new roles (e.g., test_parent/test_child) and
then INSERT INTO public.role_hierarchy selecting those new role IDs, or wrap the
insert in a conditional upsert that only affects test-local role names so you
never alter the global seeded graph.
🪄 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: faa7016e-5dd6-4112-ba9e-9f072ff0a9bb
📒 Files selected for processing (4)
supabase/migrations/20260424090111_fix_rbac_scope_mismatch_escalation.sqlsupabase/tests/13_test_plan_math.sqlsupabase/tests/50_test_rbac_has_permission_execute_grants.sqltests/rbac-permissions.test.ts
✅ Files skipped from review due to trivial changes (1)
- supabase/tests/50_test_rbac_has_permission_execute_grants.sql
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ba18c7f77
ℹ️ 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".
Contributor
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/13_test_plan_math.sql`:
- Around line 44-49: The trigger force_valid_owner_org_app_versions is disabled
before the fixture updates but never re-enabled; add a matching ALTER TABLE
public.app_versions ENABLE TRIGGER force_valid_owner_org_app_versions
immediately after the UPDATE that sets app_versions (after the UPDATE where
id=7) so the trigger is restored for subsequent statements (ensure the enable
uses the exact trigger name force_valid_owner_org_app_versions and table
public.app_versions).
🪄 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: f6fd86fc-de56-49c3-8053-fc226339674e
📒 Files selected for processing (1)
supabase/tests/13_test_plan_math.sql
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/functions/_backend/private/role_bindings.ts (1)
171-194:⚠️ Potential issue | 🔴 CriticalKeep the caller rank ceiling limited to active bindings in the same scope family.
getCallerMaxPriorityRank()still takesMAX(priority_rank)over all of the caller’s bindings in the org. That means an expired high-rank binding, or a high-rank binding from another scope family, can still inflate the POST/PATCH/DELETE ceiling onceorg.update_user_rolesis satisfied by some other active role.🔒 Suggested hardening
async function getCallerMaxPriorityRank( drizzle: ReturnType<typeof getDrizzleClient>, authType: 'apikey' | 'jwt', principalId: string, orgId: string, + scopeType: RoleBindingBody['scope_type'], ): Promise<number> { const principalType = authType === 'apikey' ? 'apikey' : 'user' const result = await drizzle .select({ max_rank: sql<number>`MAX(${schema.roles.priority_rank})` }) .from(schema.role_bindings) .innerJoin(schema.roles, and( eq(schema.role_bindings.role_id, schema.roles.id), eq(schema.role_bindings.scope_type, schema.roles.scope_type), )) .where( and( eq(schema.role_bindings.principal_type, principalType), eq(schema.role_bindings.principal_id, principalId), eq(schema.role_bindings.org_id, orgId), + eq(schema.role_bindings.scope_type, scopeType), + sql`(${schema.role_bindings.expires_at} IS NULL OR ${schema.role_bindings.expires_at} > now())`, ), ) .limit(1) return result[0]?.max_rank ?? 0 }const callerMaxRank = await getCallerMaxPriorityRank(drizzle, auth.authType, callerPrincipalId, org_id, role.scope_type) const callerMaxRank = await getCallerMaxPriorityRank( drizzle, auth!.authType, callerPrincipalId, binding.org_id!, binding.scope_type as RoleBindingBody['scope_type'], )Also applies to: 331-331, 488-488, 565-565
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/role_bindings.ts` around lines 171 - 194, getCallerMaxPriorityRank currently returns MAX(priority_rank) across all bindings in the org; change it to accept an additional scopeType parameter and restrict the query to bindings with role_bindings.scope_type = scopeType and only "active" bindings (e.g., where role_bindings.expires_at IS NULL OR role_bindings.expires_at > now()) so expired or other-scope-family bindings cannot inflate the caller ceiling; update call sites (the places that call getCallerMaxPriorityRank) to pass the binding.scope_type (e.g., the binding.org_id and binding.scope_type shown in the review) and ensure you still join to schema.roles as before (use the same function name getCallerMaxPriorityRank, drizzle, schema.role_bindings and schema.roles identifiers to locate and modify the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@supabase/functions/_backend/private/role_bindings.ts`:
- Around line 171-194: getCallerMaxPriorityRank currently returns
MAX(priority_rank) across all bindings in the org; change it to accept an
additional scopeType parameter and restrict the query to bindings with
role_bindings.scope_type = scopeType and only "active" bindings (e.g., where
role_bindings.expires_at IS NULL OR role_bindings.expires_at > now()) so expired
or other-scope-family bindings cannot inflate the caller ceiling; update call
sites (the places that call getCallerMaxPriorityRank) to pass the
binding.scope_type (e.g., the binding.org_id and binding.scope_type shown in the
review) and ensure you still join to schema.roles as before (use the same
function name getCallerMaxPriorityRank, drizzle, schema.role_bindings and
schema.roles identifiers to locate and modify the code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff331ad9-0192-475d-9694-315462b8de94
📒 Files selected for processing (3)
supabase/functions/_backend/private/role_bindings.tssupabase/tests/13_test_plan_math.sqltests/private-role-bindings.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/private-role-bindings.test.ts
…3m2-x66x # Conflicts: # supabase/tests/13_test_plan_math.sql
|
This was referenced Apr 29, 2026
4 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_has_permission()and restrict its execute grant toservice_roleMotivation (AI generated)
GHSA-55q2-p3m2-x66x reports that org-scoped roles could be assigned at app scope, including for pending invitees, and that the RBAC resolver later trusted those malformed bindings. That allowed a low-privilege invited user to inherit app-level privileges after accepting an invite.
Business Impact (AI generated)
This closes a confirmed privilege-escalation path in RBAC-enabled organizations. It protects tenant boundaries, reduces the risk of unauthorized app administration, and lets Capgo remediate the advisory with both application-level and database-level defenses.
Test Plan (AI generated)
bun lintbunx eslint supabase/functions/_backend/private/role_bindings.ts tests/private-role-bindings.test.ts tests/rbac-permissions.test.tsbunx vitest run tests/private-role-bindings.test.ts tests/rbac-permissions.test.tsbunx supabase test db supabase/tests/13_test_plan_math.sqlbunx supabase test db supabase/tests/24_test_data_functions.sqlpublic.rbac_has_permission(...)on the branch-local reset databaseScreenshots (AI generated)
Checklist (AI generated)
Generated with AI
Summary by CodeRabbit
Bug Fixes
New Features
Tests