fix(security): prevent stale RBAC demotion privileges#2020
fix(security): prevent stale RBAC demotion privileges#2020
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds RBAC synchronization and encrypted bundle cleanup functionality. A database migration introduces trigger-based synchronization between legacy ChangesRBAC Legacy Sync & Bundle Cleanup
Sequence Diagram(s)sequenceDiagram
participant Client as Client / Handler
participant DB as Database Transaction
participant SyncFunc as sync_org_user_legacy_from_rbac()
participant OrgUsers as org_users Table
Client->>DB: BEGIN transaction
Note over DB: DELETE role_binding
DB->>DB: delete role_bindings row
alt binding.principal_type = 'user' AND binding.scope_type = 'org'
Client->>SyncFunc: Check if sync needed
SyncFunc->>OrgUsers: UPDATE user_right/rbac_role_name to NULL
OrgUsers-->>SyncFunc: Row updated
end
DB->>DB: COMMIT
DB-->>Client: Success
sequenceDiagram
participant Trigger as role_bindings Trigger
participant SyncFunc as sync_org_user_legacy_from_rbac_trigger()
participant SyncImpl as sync_org_user_legacy_from_rbac()
participant OrgUsers as org_users Table
participant RoleBindings as role_bindings Table
Note over RoleBindings: INSERT/UPDATE/DELETE event
RoleBindings->>Trigger: Fire AFTER trigger
Trigger->>SyncFunc: Invoke trigger wrapper
SyncFunc->>SyncImpl: Call sync implementation
SyncImpl->>RoleBindings: SELECT best active org-scope binding
RoleBindings-->>SyncImpl: Return binding or null
SyncImpl->>OrgUsers: UPDATE user_right/rbac_role_name
OrgUsers-->>SyncImpl: Success
SyncImpl-->>SyncFunc: Complete
SyncFunc-->>Trigger: Done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
Review rate limit: 0/5 reviews remaining, refill in 8 minutes and 46 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/migrations/20260502134355_fix_rbac_role_binding_demoted_super_admin.sql (1)
176-180: 💤 Low valueComplex key_id matching logic could use a clarifying comment.
The bidirectional prefix matching (
key_id = LEFT(required_key, 20)ORLEFT(key_id, length) = required_key) handles both truncated and full key comparisons, but the intent isn't immediately obvious. Consider adding an inline comment explaining why both directions are checked.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260502134355_fix_rbac_role_binding_demoted_super_admin.sql` around lines 176 - 180, Add a clarifying inline comment next to the complex key matching condition that explains the bidirectional prefix checks: that av.key_id may be a 20-char truncated ID or a full-length required_key, so the condition checks either that the truncated required_key matches av.key_id (av.key_id = LEFT(required_key, 20)) or that a truncated av.key_id matches the full required_key (LEFT(av.key_id, LENGTH(required_key)) = required_key); place this comment immediately above or inline with the condition containing av.key_id and required_key to make the intent clear for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@supabase/migrations/20260502134355_fix_rbac_role_binding_demoted_super_admin.sql`:
- Around line 176-180: Add a clarifying inline comment next to the complex key
matching condition that explains the bidirectional prefix checks: that av.key_id
may be a 20-char truncated ID or a full-length required_key, so the condition
checks either that the truncated required_key matches av.key_id (av.key_id =
LEFT(required_key, 20)) or that a truncated av.key_id matches the full
required_key (LEFT(av.key_id, LENGTH(required_key)) = required_key); place this
comment immediately above or inline with the condition containing av.key_id and
required_key to make the intent clear for future maintainers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bf208f1-263f-4ff0-84d5-0b7d44a1de5e
📒 Files selected for processing (5)
supabase/functions/_backend/private/role_bindings.tssupabase/migrations/20260502134355_fix_rbac_role_binding_demoted_super_admin.sqltests/enforce-encrypted-bundles.test.tstests/private-role-bindings.test.tstests/security-definer-execute-hardening.test.ts
riderx
force-pushed
the
codex/fix-rbac-demotion-advisory
branch
from
a945562 to
b6e7694
Compare
riderx
force-pushed
the
codex/fix-rbac-demotion-advisory
branch
from
b6e7694 to
7e4e1fc
Compare
|
1 participant
Summary (AI generated)
org_users.user_rightvalues from preserving super-admin access after RBAC demotion.role_bindingsback to the legacy org membership row and updates encrypted-bundle cleanup RPCs to authorize through RBAC.Motivation (AI generated)
RBAC-enabled orgs could delete or downgrade a role binding while leaving the legacy
org_users.user_rightrow assuper_admin. The bundle cleanup RPCs trusted that stale legacy value, so a demoted user could still count or delete non-compliant bundles.Business Impact (AI generated)
This closes a high-severity authorization gap that could allow demoted org users to perform destructive bundle cleanup actions, protecting customer OTA delivery and org access controls.
Test Plan (AI generated)
bun lintbun lint:backendbun run supabase:with-env -- bunx vitest run tests/private-role-bindings.test.ts tests/enforce-encrypted-bundles.test.ts tests/security-definer-execute-hardening.test.tsbun run typecheckgit diff --checkGenerated with AI
Summary by CodeRabbit
Bug Fixes
Security