fix(api): RBAC follow-ups for PR #2403#2548
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![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: bf02583c3a
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08a0488357
ℹ️ 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".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| public.rbac_perm_app_create_channel(), public.rbac_perm_app_read_channels(), public.rbac_perm_app_read_logs(), public.rbac_perm_app_manage_devices(), public.rbac_perm_app_read_devices(), | ||
| public.rbac_perm_app_build_native(), public.rbac_perm_app_read_audit(), public.rbac_perm_app_update_user_roles(), | ||
| public.rbac_perm_channel_read(), public.rbac_perm_channel_update_settings(), public.rbac_perm_channel_read_history(), | ||
| public.rbac_perm_channel_read(), public.rbac_perm_channel_read_history(), |
There was a problem hiding this comment.
org_admin seed migration drift
Medium Severity
The seed.sql RBAC repopulation block removes the channel.update_settings permission for the org_admin role. This makes org_admin users in fresh db reset environments unable to update channel settings, while existing production environments still allow it due to prior migrations. This inconsistency can lead to local failures for actions that succeed in production.
Reviewed by Cursor Bugbot for commit bad1237. Configure here.
| ON role_permissions.role_id = role_closure.effective_role_id | ||
| INNER JOIN public.permissions | ||
| ON permissions.id = role_permissions.permission_id | ||
| WHERE permissions.key = public.rbac_perm_app_read() |
There was a problem hiding this comment.
Bundle RLS ignores JWT key owner
Medium Severity
The rewritten app_versions_readable_app_ids() treats a present capgkey header as the RBAC principal without requiring auth.uid() to match the key owner. That differs from rbac_check_permission_direct, which rejects mismatched user and API key on the same request.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit bad1237. Configure here.
Introduce org.manage_apikeys and apikey_manager so legacy broad keys and dedicated CI keys can create and manage sibling keys without user-role assignment rights. Skip 2FA enforcement on API-key auth paths, optimize manifest/app_versions RLS with readable app id helpers, and align seed data with app_uploader channel promote and channel_admin legacy mapping. Co-authored-by: Cursor <cursoragent@cursor.com>
Seed API key 113 with the apikey_manager role and add vitest/SQL checks that CI keys can manage siblings without role escalation privileges. Co-authored-by: Cursor <cursoragent@cursor.com>
Re-apply org-scoped RBAC bindings for the dedicated apikey management seed keys after permissions are repopulated, and assert by key UUID in the SQL test instead of a fixed apikeys.id. Co-authored-by: Cursor <cursoragent@cursor.com>
Grant apikey_manager org.read for expiration policy enforcement, block admin-tier role assignment from apikey_manager callers, and restore narrow channel_developer/uploader legacy mappings with first-class roles. Co-authored-by: Cursor <cursoragent@cursor.com>
riderx
force-pushed
the
fix/rbac-apikey-hardening-followups
branch
from
bad1237 to
7ee64b2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Only evaluate denied assignable roles for bindings in orgs where the caller lacks org.update_user_roles. Co-authored-by: Cursor <cursoragent@cursor.com>
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a4fa688. Configure here.
| if (APIKEY_MANAGER_DENIED_ASSIGNABLE_ROLES.has(binding.role_name)) { | ||
| throw quickError(403, 'forbidden_binding', `Forbidden - API key managers cannot assign the ${binding.role_name} role`) | ||
| } | ||
| } |
There was a problem hiding this comment.
Client allowSystemRole bypasses deny
High Severity
The assertApiKeyManagerCanAssignBindings function allows client-supplied allowSystemRole: true in POST /apikey requests to bypass the denied roles check. This enables an apikey_manager to create sibling API keys with roles like app_admin or channel_admin, which are otherwise forbidden.
Reviewed by Cursor Bugbot for commit a4fa688. Configure here.
1 participant
Summary (AI generated)
org.manage_apikeyspermission and assignableapikey_managerrole; granted it toorg_admin/org_super_adminso migrated legacyallkeys keep sibling key management in CI.POST /apikeyvia API key auth when the caller hasorg.manage_apikeys(bindings/global-permission updates remain JWT-only; self-update remains blocked).rbac_check_permission_direct.channel_admininstead of nonexistentchannel_developer/channel_uploader.app_uploaderchannel promote/read withoutchannel.update_settings.app_versions+manifestviaapp_versions_readable_app_ids()(bundle read permission).seed.sqlso post-seed RBAC repopulation keeps the new permission/grants.Motivation (AI generated)
PR #2403 correctly hardened RBAC, but review follow-ups asked to preserve CI/API-key management for migrated broad keys, add a dedicated manage-keys permission, skip 2FA on API keys, fix channel legacy mapping, and reduce manifest RLS cost.
Business Impact (AI generated)
Customers with legacy broad API keys in GitHub Actions/Fastlane can keep provisioning and rotating sibling keys without JWT sessions, while scoped upload keys still cannot escalate privileges.
Test Plan (AI generated)
bun run supabase:db:resetbun test:backend -- tests/apikeys.test.ts tests/rbac-apikey-request-identity-rpc.test.tsGenerated with AI
Made with Cursor