feat: prevent privilege escalation #2428
Conversation
wilsonrivera
requested review from
JivusAyrus,
StarpTech and
thisisnithin
as code owners
WalkthroughAdds RBAC checks preventing non-organization-admins from creating, updating, or deleting API keys tied to organization-admin groups; exposes API key groupId in repository responses; updates tests to cover denied operations; disables admin-group options in the group-select UI and pluralizes the invite label. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
controlplane/src/core/bufservices/api-key/updateAPIKey.ts (1)
60-71: LGTM! RBAC check prevents privilege escalation during API key updates.The authorization gate correctly mirrors the pattern in
createAPIKey.ts, blocking non-admin users from reassigning API keys to admin-controlled groups before the update executes.The RBAC check pattern (lines 60-69) is duplicated between
createAPIKey.ts(lines 74-83) and this file. Consider extracting to a shared validation function to improve maintainability.💡 Suggested refactor
Create a shared validator in a utility file:
// core/services/ApiKeyRBACValidator.ts export function validateGroupAssignment( targetGroup: OrganizationGroupDTO, requesterRBAC: RBACEvaluator ): { valid: boolean; error?: { code: EnumStatusCode; details: string } } { const groupRBAC = new RBACEvaluator([targetGroup]); if (groupRBAC.isOrganizationAdmin && !requesterRBAC.isOrganizationAdmin) { return { valid: false, error: { code: EnumStatusCode.ERR, details: "You don't have the required permissions to assign this group", }, }; } return { valid: true }; }Then use it in both files:
const validation = validateGroupAssignment(orgGroup, authContext.rbac); if (!validation.valid) { return { response: validation.error, apiKey: '', }; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
controlplane/src/core/bufservices/api-key/createAPIKey.ts(2 hunks)controlplane/src/core/bufservices/api-key/updateAPIKey.ts(2 hunks)controlplane/test/api-keys.test.ts(4 hunks)studio/src/components/group-select.tsx(3 hunks)studio/src/pages/[organizationSlug]/members.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
controlplane/src/core/bufservices/api-key/createAPIKey.ts (2)
controlplane/src/core/services/RBACEvaluator.ts (1)
RBACEvaluator(24-246)controlplane/src/core/services/ApiGenerator.ts (1)
ApiKeyGenerator(3-7)
controlplane/src/core/bufservices/api-key/updateAPIKey.ts (1)
controlplane/src/core/services/RBACEvaluator.ts (1)
RBACEvaluator(24-246)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
studio/src/pages/[organizationSlug]/members.tsx (1)
135-135: LGTM! Label updated to reflect multi-group support.The change from "What group should the member be added to?" to "What group(s) should the member be added to?" accurately reflects the MultiGroupSelect component's capability to handle multiple group assignments.
studio/src/components/group-select.tsx (1)
65-65: RBAC-based disabling logic is correct (once Line 16 is fixed).The per-item disabling logic correctly prevents non-admin users from selecting groups with
organization-adminrules. This aligns with the backend RBAC checks in the API key creation and update endpoints.controlplane/src/core/bufservices/api-key/createAPIKey.ts (1)
74-85: LGTM! RBAC check prevents privilege escalation during API key creation.The authorization gate correctly validates that:
- A new
RBACEvaluatoris instantiated with the target group to determine if it grants admin privileges- Non-admin users are blocked from assigning admin-controlled groups with a clear error message
- API key generation is deferred until after authorization passes, preventing unnecessary resource allocation
This effectively prevents
organization-apikey-managerfrom escalating toorganization-adminprivileges.controlplane/test/api-keys.test.ts (2)
114-161: LGTM! Test updated to align with new RBAC constraints.The existing test now correctly creates API keys with the
viewergroup and updates to thedevelopergroup, avoiding admin group assignment which would trigger the new authorization gate. This ensures the test validates the happy path fororganization-apikey-managerrole.
163-224: LGTM! Comprehensive test coverage for privilege escalation prevention.The two new tests thoroughly validate that
organization-apikey-managercannot:
- Create API keys with admin-controlled groups (lines 163-188)
- Update existing API keys to admin-controlled groups (lines 190-224)
Both tests correctly verify:
- Error code is
EnumStatusCode.ERR- Error message matches the expected permission denial
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2428 +/- ##
===========================================
- Coverage 61.35% 29.93% -31.43%
===========================================
Files 229 587 +358
Lines 23814 88122 +64308
Branches 0 4632 +4632
===========================================
+ Hits 14612 26376 +11764
- Misses 7970 61443 +53473
+ Partials 1232 303 -929
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
controlplane/src/core/repositories/ApiKeyRepository.ts (1)
109-110: Consider null handling for groupId in the returned object.When
groupIdisnull(which is valid per the schema'sonDelete: 'set null'), this creates{ id: null }rather thanundefined. The downstream code indeleteAPIKey.tshandles this correctly withif (apiKey.group?.id), but the type assertionas APIKeyDTOmay mask type inconsistencies.For consistency with
getAPIKeys(line 141), consider returningundefinedwhen groupId is null:🔎 Suggested improvement
return { id: key[0].id, name: key[0].name, createdAt: key[0].createdAt.toISOString(), lastUsedAt: key[0].lastUsedAt?.toISOString() ?? '', expiresAt: key[0].expiresAt?.toISOString() ?? '', createdBy: key[0].createdBy, creatorUserID: key[0].creatorUserID, - group: { id: key[0].groupId }, + group: key[0].groupId ? { id: key[0].groupId } : undefined, } as APIKeyDTO;controlplane/src/core/bufservices/api-key/deleteAPIKey.ts (1)
55-61: Remove unnecessaryapiKeyfield from error response.The
DeleteAPIKeyResponsetype only includes aresponsefield, unlikeCreateAPIKeyResponsewhich has anapiKeyfield. The error response at lines 55-61 incorrectly includesapiKey: '', while the successful response at lines 84-88 correctly omits it. This appears to be copy-paste fromcreateAPIKey.ts.return { response: { code: EnumStatusCode.ERR, details: `You don't have access to remove the API key "${apiKey.name}"`, }, - apiKey: '', };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
controlplane/src/core/bufservices/api-key/createAPIKey.ts(2 hunks)controlplane/src/core/bufservices/api-key/deleteAPIKey.ts(3 hunks)controlplane/src/core/bufservices/api-key/updateAPIKey.ts(2 hunks)controlplane/src/core/repositories/ApiKeyRepository.ts(2 hunks)controlplane/test/api-keys.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/src/core/bufservices/api-key/updateAPIKey.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/bufservices/api-key/deleteAPIKey.ts
🧬 Code graph analysis (2)
controlplane/src/core/bufservices/api-key/createAPIKey.ts (2)
controlplane/src/core/services/RBACEvaluator.ts (1)
RBACEvaluator(24-246)controlplane/src/core/services/ApiGenerator.ts (1)
ApiKeyGenerator(3-7)
controlplane/src/core/repositories/ApiKeyRepository.ts (1)
controlplane/src/db/schema.ts (1)
apiKeys(1175-1205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
controlplane/src/core/bufservices/api-key/createAPIKey.ts (1)
74-85: LGTM! Correct RBAC enforcement for privilege escalation prevention.The logic correctly prevents non-admin users from creating API keys that would grant admin privileges. Creating the
RBACEvaluatorwith the target group and checkingisOrganizationAdminbefore allowing the operation is an effective approach.Good improvement moving the API key generation (line 85) after all validation, avoiding unnecessary key generation for failed requests.
controlplane/test/api-keys.test.ts (3)
163-188: Good test coverage for create restriction.The test correctly validates that an
organization-apikey-managercannot create API keys with an admin group, and verifies both the error code and the specific error message.
190-224: Good test coverage for update restriction.The test properly sets up a non-admin API key first, then verifies the apikey-manager cannot escalate it to admin group.
226-259: Good test coverage for delete restriction.The test correctly creates an admin API key as an admin user, then verifies a non-admin apikey-manager cannot delete it. The flow of switching auth contexts is appropriate for this scenario.
controlplane/src/core/bufservices/api-key/deleteAPIKey.ts (1)
46-64: LGTM! Proper RBAC check for delete operation.The implementation correctly:
- Guards against null group IDs (line 46)
- Handles deleted groups gracefully (line 52)
- Uses the same RBAC pattern as createAPIKey for consistency
Based on learnings, authentication and authorization checks at the service layer (before repository calls) is the established pattern in this codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
controlplane/test/api-keys.test.ts (1)
163-259: Consider adding a positive test case for organization-admin with admin-group keys.While the new tests thoroughly validate that
organization-apikey-manageris blocked from admin-group operations, there's no explicit test confirming thatorganization-adminCAN still create, update, and delete admin-group API keys.The parameterized test (lines 104-161) now only uses viewer/developer groups for both roles. Consider adding a test specifically for
organization-adminperforming all operations with admin-group keys to verify the restrictions don't inadvertently block legitimate admin actions.💡 Example test structure
test('that an "organization-admin" can manage API keys with admin role', async () => { const { client, server, users, authenticator } = await SetupTest({ dbname, enableMultiUsers: true }); const orgGroupsResponse = await client.getOrganizationGroups({}); const adminGroup = orgGroupsResponse.groups.find((g) => g.name === 'admin')!; authenticator.changeUserWithSuppliedContext({ ...users[TestUser.adminAliceCompanyA], rbac: createTestRBACEvaluator(createTestGroup({ role: 'organization-admin' })), }); // Create, update, and delete admin-group key - all should succeed const apiKeyName = uid(); const createResponse = await client.createAPIKey({ name: apiKeyName, expires: ExpiresAt.NEVER, groupId: adminGroup.groupId, }); expect(createResponse.response?.code).toBe(EnumStatusCode.OK); // ... update and delete operations await server.close(); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/test/api-keys.test.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controlplane/test/api-keys.test.ts (4)
controlplane/src/core/test-util.ts (2)
createTestRBACEvaluator(173-175)createTestGroup(181-198)controlplane/src/db/models.ts (1)
OrganizationRole(32-32)studio/src/lib/constants.ts (1)
OrganizationRole(290-290)controlplane/test/test-util.ts (1)
SetupTest(66-418)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
controlplane/test/api-keys.test.ts (4)
104-161: LGTM! Test correctly adapted for new admin-group restrictions.The changes to use
viewerGroupinstead ofadminGroupproperly align this test with the new security policy. By testing with non-admin groups, this parameterized test correctly validates that bothorganization-adminandorganization-apikey-managerroles retain their general API key management capabilities without triggering the new admin-group restrictions.The grammar fixes ("have" → "has") also improve clarity.
190-224: Good coverage of the UPDATE restriction.This test correctly validates that
organization-apikey-managercannot escalate an existing API key to the admin group. The two-phase approach (create with viewer group, then attempt update to admin) properly simulates a privilege escalation attempt.
226-259: Good coverage of the DELETE restriction.This test correctly validates that
organization-apikey-managercannot delete admin-group API keys, preventing privilege escalation via key removal. The role-switching approach (create as admin, then attempt deletion as api-key-manager) is the correct way to test this scenario.Note: The past review concern about the comment on line 237 has been properly addressed—it now accurately states "admin" group.
163-188: Test correctly validates the CREATE restriction with proper error handling.This test validates that
organization-apikey-managercannot create API keys with the admin group, preventing privilege escalation at the creation stage. The implementation correctly returnsEnumStatusCode.ERRwith the error message "You don't have access to create an API key with the group "admin"", which matches the test expectations. The error code distinction is intentional:ERRsignals "you have create permission but not for this group" versusERROR_NOT_AUTHORIZED(returned byUnauthorizedError) which signals "you lack this permission entirely."
…tion-from-organization-apikey
The goal of this PR is to prevent privilege escalation from
organization-apikey-managertoorganization-adminSummary by CodeRabbit
New Features
UI
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist