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 (1)
📝 WalkthroughWalkthroughAdds a DB trigger to prevent updates that would demote the last organization ChangesLast Super Admin Demotion Prevention
Sequence DiagramsequenceDiagram
actor Client
participant API as API Handler\n(role_bindings.ts)
participant DB as PostgreSQL\nDatabase
Client->>API: PATCH /private/role_bindings/:id (demote role)
API->>DB: compute callerMaxRank
DB-->>API: callerMaxRank
API->>DB: UPDATE role_bindings ... FROM roles WHERE roles.priority_rank <= callerMaxRank RETURNING *
DB->>DB: Trigger: prevent_last_super_admin_binding_update BEFORE UPDATE OF role_id
DB->>DB: Acquire advisory lock(org_id) and count remaining org_super_admin bindings
alt Trigger raises CANNOT_DEMOTE_LAST_SUPER_ADMIN_BINDING
DB-->>API: Error
API-->>Client: 409 Conflict (Cannot demote the last org_super_admin)
else UPDATE prevented by JOIN/WHERE (no row returned)
DB-->>API: no rows returned
API-->>Client: 403 Forbidden (higher-privileges)
else Update allowed
DB-->>API: Success
API-->>Client: 200 OK
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 48 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
supabase/functions/_backend/private/role_bindings.ts (2)
660-689:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTranslate the last-super-admin trigger failure instead of returning 500.
After this migration lands, a
super_admindemoting the finalorg_super_adminwill hitCANNOT_DEMOTE_LAST_SUPER_ADMIN_BINDINGon Line 660. The generic catch on Lines 682-689 turns that expected guard intoInternal server error, which makes an authorization/business-rule rejection look like backend failure.Suggested handling
- catch (error) { + catch (error: any) { + if (error?.message?.includes('CANNOT_DEMOTE_LAST_SUPER_ADMIN_BINDING')) { + return c.json({ error: 'At least one super_admin binding must remain in the org' }, 409) + } + cloudlogErr({ requestId: c.get('requestId'), message: 'role_binding_update_failed', bindingId, error, }) return c.json({ error: 'Internal server error' }, 500) }🤖 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 660 - 689, The catch block after the update in the role binding handler currently logs and returns a generic 500, which hides the specific business-rule error CANNOT_DEMOTE_LAST_SUPER_ADMIN_BINDING; update the catch in the function that calls drizzle.update(schema.role_bindings) (identify by bindingId, updated, cloudlog and cloudlogErr usage) to detect when error.code === 'CANNOT_DEMOTE_LAST_SUPER_ADMIN_BINDING' (or error.message contains that constant) and respond with a translated client error (e.g., c.json({ error: 'Cannot demote the last org_super_admin' }, 400 or 403) while still logging via cloudlogErr; for all other errors preserve the existing 500 logging/response behavior.
611-664:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake the current-role check atomic with the update.
loadManagedBinding()snapshotsbinding.role_id, then Lines 650-657 authorize against that stale value before Lines 660-664 update bybindingIdonly. A concurrent transaction can promote the binding after the read and before the write, and this request will still overwrite it. That reopens the demotion path as a TOCTOU authorization bug.Please fold the read + authorization + write into a single transaction with a row lock, or encode the current-role-rank predicate directly into the
UPDATE/CTE and require exactly one row to change.🤖 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 611 - 664, The current check uses a stale snapshot from loadManagedBinding then updates by bindingId, allowing a TOCTOU race; make the check and update atomic by either wrapping the read+authorize+write in a DB transaction with a row lock (SELECT ... FOR UPDATE on role_bindings joined to roles to read current roles.priority_rank, call getCallerMaxPriorityRank, enforce priority checks, then update and commit) or by encoding the authorization predicate into the UPDATE so it only succeeds when the existing role's priority_rank is <= callerMaxRank (e.g. UPDATE role_bindings rb SET role_id = :newRoleId FROM roles r WHERE rb.id = :bindingId AND rb.role_id = r.id AND r.priority_rank <= :callerMaxRank RETURNING ... and verify one row was returned); adjust the code path around loadManagedBinding, getCallerMaxPriorityRank, and the drizzle .update(...) so you verify the atomic operation affected exactly one row and return a 403 if it did not.
🧹 Nitpick comments (1)
tests/private-role-bindings.test.ts (1)
12-12: ⚡ Quick winDrop the extra seeded
super_adminfrom this endpoint regression.This test fails on the existing-role rank check before any
UPDATEhappens, so theADMIN_USER_IDrow never contributes to the behavior being asserted. Keeping it adds an unnecessary dependency on a hardcoded seeded user outsidetest-utils, which makes the test more brittle across DB resets.Possible simplification
-const ADMIN_USER_ID = 'c591b04e-cf29-4945-b9a0-776d0672061a' @@ const { error: membersError } = await supabase.from('org_users').insert([ { org_id: orgId, user_id: USER_ID, user_right: 'admin' }, - { org_id: orgId, user_id: ADMIN_USER_ID, user_right: 'super_admin' }, ])As per coding guidelines, "create dedicated seed data when tests modify resources or when resource state matters for assertions."
Also applies to: 241-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/private-role-bindings.test.ts` at line 12, Remove the hardcoded ADMIN_USER_ID constant and any uses of that seeded super_admin row in the failing test (ADMIN_USER_ID) because the existing-role rank check runs before the UPDATE so that row is irrelevant; instead create and use a dedicated test user seeded via the test utilities (e.g., call the project’s test-utils helper like createTestUser/seedTestUser or the existing test fixture) within the test setup so the test only depends on locally-seeded data, and update assertions that referenced ADMIN_USER_ID accordingly (also remove the duplicate seeded super_admin references around the other occurrences noted).
🤖 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 660-689: The catch block after the update in the role binding
handler currently logs and returns a generic 500, which hides the specific
business-rule error CANNOT_DEMOTE_LAST_SUPER_ADMIN_BINDING; update the catch in
the function that calls drizzle.update(schema.role_bindings) (identify by
bindingId, updated, cloudlog and cloudlogErr usage) to detect when error.code
=== 'CANNOT_DEMOTE_LAST_SUPER_ADMIN_BINDING' (or error.message contains that
constant) and respond with a translated client error (e.g., c.json({ error:
'Cannot demote the last org_super_admin' }, 400 or 403) while still logging via
cloudlogErr; for all other errors preserve the existing 500 logging/response
behavior.
- Around line 611-664: The current check uses a stale snapshot from
loadManagedBinding then updates by bindingId, allowing a TOCTOU race; make the
check and update atomic by either wrapping the read+authorize+write in a DB
transaction with a row lock (SELECT ... FOR UPDATE on role_bindings joined to
roles to read current roles.priority_rank, call getCallerMaxPriorityRank,
enforce priority checks, then update and commit) or by encoding the
authorization predicate into the UPDATE so it only succeeds when the existing
role's priority_rank is <= callerMaxRank (e.g. UPDATE role_bindings rb SET
role_id = :newRoleId FROM roles r WHERE rb.id = :bindingId AND rb.role_id = r.id
AND r.priority_rank <= :callerMaxRank RETURNING ... and verify one row was
returned); adjust the code path around loadManagedBinding,
getCallerMaxPriorityRank, and the drizzle .update(...) so you verify the atomic
operation affected exactly one row and return a 403 if it did not.
---
Nitpick comments:
In `@tests/private-role-bindings.test.ts`:
- Line 12: Remove the hardcoded ADMIN_USER_ID constant and any uses of that
seeded super_admin row in the failing test (ADMIN_USER_ID) because the
existing-role rank check runs before the UPDATE so that row is irrelevant;
instead create and use a dedicated test user seeded via the test utilities
(e.g., call the project’s test-utils helper like createTestUser/seedTestUser or
the existing test fixture) within the test setup so the test only depends on
locally-seeded data, and update assertions that referenced ADMIN_USER_ID
accordingly (also remove the duplicate seeded super_admin references around the
other occurrences noted).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c864e291-6a83-411a-8f77-abb71a78c766
📒 Files selected for processing (4)
supabase/functions/_backend/private/role_bindings.tssupabase/migrations/20260502134234_prevent_last_super_admin_demotion.sqltests/private-role-bindings.test.tstests/security-definer-execute-hardening.test.ts
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
1 participant
Summary (AI generated)
super_adminbinding.Motivation (AI generated)
GHSA-54mj-q77q-xwx5 reported that an
org_admincould demote anorg_super_adminbecause PATCH only checked the new role rank, and the database only protected last-super-admin deletion.Business Impact (AI generated)
This preserves org ownership integrity and prevents admins from locking every super admin out of billing, destructive app management, and other super-admin-only workflows.
Test Plan (AI generated)
bun lintbun lint:backendbunx eslint tests/private-role-bindings.test.ts tests/security-definer-execute-hardening.test.tsbun run supabase:with-env -- bunx vitest run tests/private-role-bindings.test.ts tests/security-definer-execute-hardening.test.tsbun typecheckGenerated with AI
Summary by CodeRabbit
Bug Fixes
Tests