From 5627b1212aa62252f4542ef1b96e4c074b473490 Mon Sep 17 00:00:00 2001 From: Mariano Fuentes Date: Wed, 29 Apr 2026 14:48:55 -0700 Subject: [PATCH] fix(security): admin escalation, secrets RBAC, oauth session check (#2712) * fix(api): prevent admin self-escalation to owner role PATCH /v1/people/:id allowed admins to grant themselves the owner role, modify their own member record, and assign roles higher than their own. Add authorizeRoleChange() in people/utils/role-authorization.ts enforcing: - self-modification block (route role changes through transfer-ownership) - owner-role assignment block in both directions - role-hierarchy check (caller must hold every role being assigned) Co-Authored-By: Claude Opus 4.7 (1M context) * fix(auth): scope secret access to dedicated permission Secrets controller previously gated on organization:read, granting auditors (read-only compliance reviewers) plaintext access to credentials and using the same permission for read and write operations. Introduce a 'secret' RBAC resource with create/read/update/delete actions. Grant full CRUD to owner and admin only. Auditor, employee, and contractor no longer have any secret access. Switch the secrets controller to gate on secret:* actions and update frontend route + button gating to match. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(api): bind integration OAuth callback to initiating session The integration OAuth callback relied solely on the random state token to identify the initiating user/org. Add defense-in-depth session validation so the same logged-in user (and active organization) that started the flow must be the one completing it; otherwise the state is consumed, the failure is logged, and the user is redirected with session_mismatch. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- .../src/auth/require-permission.decorator.ts | 3 +- .../controllers/oauth.controller.spec.ts | 184 +++++++++++++-- .../controllers/oauth.controller.ts | 101 +++++++- apps/api/src/people/people.controller.spec.ts | 1 + apps/api/src/people/people.controller.ts | 1 + apps/api/src/people/people.service.spec.ts | 215 +++++++++++++++++- apps/api/src/people/people.service.ts | 19 +- apps/api/src/people/utils/member-validator.ts | 4 +- .../src/people/utils/role-authorization.ts | 78 +++++++ .../src/secrets/secrets-permissions.spec.ts | 117 ++++++++++ .../src/secrets/secrets.controller.spec.ts | 68 +++++- apps/api/src/secrets/secrets.controller.ts | 10 +- .../training/permissions-regression.spec.ts | 17 ++ .../secrets/components/AddSecretDialog.tsx | 2 +- .../secrets/components/EditSecretDialog.tsx | 2 +- .../secrets/components/table/SecretsTable.tsx | 48 ++-- apps/app/src/lib/permissions.ts | 3 +- packages/auth/src/permissions.ts | 8 + 18 files changed, 823 insertions(+), 58 deletions(-) create mode 100644 apps/api/src/people/utils/role-authorization.ts create mode 100644 apps/api/src/secrets/secrets-permissions.spec.ts diff --git a/apps/api/src/auth/require-permission.decorator.ts b/apps/api/src/auth/require-permission.decorator.ts index 65045ebbdc..0946ac45ad 100644 --- a/apps/api/src/auth/require-permission.decorator.ts +++ b/apps/api/src/auth/require-permission.decorator.ts @@ -70,7 +70,8 @@ export type GRCResource = | 'training' | 'app' | 'trust' - | 'portal'; + | 'portal' + | 'secret'; /** * Action types available for GRC resources — CRUD only diff --git a/apps/api/src/integration-platform/controllers/oauth.controller.spec.ts b/apps/api/src/integration-platform/controllers/oauth.controller.spec.ts index a48d75e5b2..01cf1192c0 100644 --- a/apps/api/src/integration-platform/controllers/oauth.controller.spec.ts +++ b/apps/api/src/integration-platform/controllers/oauth.controller.spec.ts @@ -1,5 +1,6 @@ import { Test, TestingModule } from '@nestjs/testing'; import { HttpException } from '@nestjs/common'; +import type { Request } from 'express'; import { OAuthController } from './oauth.controller'; import { HybridAuthGuard } from '../../auth/hybrid-auth.guard'; import { PermissionGuard } from '../../auth/permission.guard'; @@ -12,10 +13,21 @@ import { OAuthCredentialsService } from '../services/oauth-credentials.service'; import { AutoCheckRunnerService } from '../services/auto-check-runner.service'; import { CloudSecurityService } from '../../cloud-security/cloud-security.service'; +jest.mock('@db', () => ({ + ...jest.requireActual('@prisma/client'), + db: {}, +})); + jest.mock('../../auth/auth.server', () => ({ auth: { api: { getSession: jest.fn() } }, })); +import { auth } from '../../auth/auth.server'; + +const mockedGetSession = auth.api.getSession as jest.MockedFunction< + typeof auth.api.getSession +>; + jest.mock('../../auth/hybrid-auth.guard', () => ({ HybridAuthGuard: class HybridAuthGuard {}, })); @@ -306,8 +318,36 @@ describe('OAuthController', () => { redirect: jest.fn(), } as unknown as import('express').Response; + const buildRequest = (overrides?: Partial) => + ({ + headers: { + cookie: 'better-auth.session_token=valid_cookie', + ...overrides, + }, + }) as unknown as Request; + + const mockRequest = buildRequest(); + + const setMatchingSession = (overrides?: { + userId?: string; + activeOrganizationId?: string | null; + }) => { + mockedGetSession.mockResolvedValue({ + user: { id: overrides?.userId ?? 'user_1' }, + session: { + id: 'sess_1', + activeOrganizationId: + overrides?.activeOrganizationId === null + ? undefined + : (overrides?.activeOrganizationId ?? 'org_1'), + }, + } as never); + }; + beforeEach(() => { (mockResponse.redirect as jest.Mock).mockClear(); + mockedGetSession.mockReset(); + setMatchingSession(); }); it('should redirect with error when OAuth error is present', async () => { @@ -318,6 +358,7 @@ describe('OAuthController', () => { error: 'access_denied', error_description: 'User denied access', }, + mockRequest, mockResponse, ); @@ -327,7 +368,11 @@ describe('OAuthController', () => { }); it('should redirect with error when code or state is missing', async () => { - await controller.oauthCallback({ code: '', state: '' }, mockResponse); + await controller.oauthCallback( + { code: '', state: '' }, + mockRequest, + mockResponse, + ); expect(mockResponse.redirect).toHaveBeenCalled(); const redirectUrl = (mockResponse.redirect as jest.Mock).mock.calls[0][0]; @@ -339,6 +384,7 @@ describe('OAuthController', () => { await controller.oauthCallback( { code: 'auth_code', state: 'invalid_state' }, + mockRequest, mockResponse, ); @@ -359,6 +405,7 @@ describe('OAuthController', () => { await controller.oauthCallback( { code: 'auth_code', state: 'expired_state' }, + mockRequest, mockResponse, ); @@ -385,6 +432,7 @@ describe('OAuthController', () => { await controller.oauthCallback( { code: 'auth_code', state: 'valid_state' }, + mockRequest, mockResponse, ); @@ -396,7 +444,7 @@ describe('OAuthController', () => { expect(redirectUrl).toContain('error=token_exchange_failed'); }); - it('should trigger initial GCP service discovery scan on successful first connect', async () => { + it('should redirect to success URL for GCP without triggering service detection or scan (GCP auto-detection runs after project selection, not after OAuth)', async () => { const futureDate = new Date(Date.now() + 600000); mockOAuthStateRepository.findByState.mockResolvedValue({ state: 'valid_gcp_state', @@ -455,23 +503,18 @@ describe('OAuthController', () => { await controller.oauthCallback( { code: 'auth_code', state: 'valid_gcp_state' }, + mockRequest, mockResponse, ); await new Promise((resolve) => setImmediate(resolve)); - expect(mockCloudSecurityService.detectServices).toHaveBeenCalledWith( - 'conn_1', - 'org_1', - ); - expect(mockedTriggerTask).toHaveBeenCalledWith( + // GCP service detection / scan is now triggered AFTER the user picks + // projects on the integrations page, not automatically after OAuth. + expect(mockCloudSecurityService.detectServices).not.toHaveBeenCalled(); + expect(mockedTriggerTask).not.toHaveBeenCalledWith( 'run-cloud-security-scan', - { - connectionId: 'conn_1', - organizationId: 'org_1', - providerSlug: 'gcp', - connectionName: 'conn_1', - }, + expect.anything(), ); expect(mockResponse.redirect).toHaveBeenCalled(); const redirectUrl = (mockResponse.redirect as jest.Mock).mock.calls[0][0]; @@ -546,6 +589,7 @@ describe('OAuthController', () => { await controller.oauthCallback( { code: 'auth_code', state: 'valid_gcp_state' }, + mockRequest, mockResponse, ); @@ -558,5 +602,119 @@ describe('OAuthController', () => { fetchSpy.mockRestore(); }); + + describe('session defense-in-depth', () => { + const futureDate = new Date(Date.now() + 600000); + const validState = { + state: 'valid_state', + providerSlug: 'github', + organizationId: 'org_1', + userId: 'user_1', + codeVerifier: null, + redirectUrl: null, + expiresAt: futureDate, + }; + + it('redirects with session_mismatch when no session cookie/auth header is present', async () => { + mockOAuthStateRepository.findByState.mockResolvedValue(validState); + const reqWithoutCookie = { + headers: {}, + } as unknown as Request; + + await controller.oauthCallback( + { code: 'auth_code', state: 'valid_state' }, + reqWithoutCookie, + mockResponse, + ); + + // getSession must not even be called when no auth headers are present + expect(mockedGetSession).not.toHaveBeenCalled(); + expect(mockOAuthStateRepository.delete).toHaveBeenCalledWith( + 'valid_state', + ); + const redirectUrl = (mockResponse.redirect as jest.Mock).mock + .calls[0][0]; + expect(redirectUrl).toContain('error=session_mismatch'); + }); + + it('redirects with session_mismatch when getSession returns null', async () => { + mockOAuthStateRepository.findByState.mockResolvedValue(validState); + mockedGetSession.mockResolvedValue(null); + + await controller.oauthCallback( + { code: 'auth_code', state: 'valid_state' }, + mockRequest, + mockResponse, + ); + + expect(mockOAuthStateRepository.delete).toHaveBeenCalledWith( + 'valid_state', + ); + const redirectUrl = (mockResponse.redirect as jest.Mock).mock + .calls[0][0]; + expect(redirectUrl).toContain('error=session_mismatch'); + }); + + it('redirects with session_mismatch when session.user.id does not match oauthState.userId', async () => { + mockOAuthStateRepository.findByState.mockResolvedValue(validState); + setMatchingSession({ userId: 'different_user' }); + + await controller.oauthCallback( + { code: 'auth_code', state: 'valid_state' }, + mockRequest, + mockResponse, + ); + + expect(mockOAuthStateRepository.delete).toHaveBeenCalledWith( + 'valid_state', + ); + // We do NOT proceed to token exchange when session doesn't match + expect( + mockOAuthCredentialsService.getCredentials, + ).not.toHaveBeenCalled(); + const redirectUrl = (mockResponse.redirect as jest.Mock).mock + .calls[0][0]; + expect(redirectUrl).toContain('error=session_mismatch'); + }); + + it('redirects with session_mismatch when session.activeOrganizationId is set and does not match oauthState.organizationId', async () => { + mockOAuthStateRepository.findByState.mockResolvedValue(validState); + setMatchingSession({ activeOrganizationId: 'org_other' }); + + await controller.oauthCallback( + { code: 'auth_code', state: 'valid_state' }, + mockRequest, + mockResponse, + ); + + expect(mockOAuthStateRepository.delete).toHaveBeenCalledWith( + 'valid_state', + ); + const redirectUrl = (mockResponse.redirect as jest.Mock).mock + .calls[0][0]; + expect(redirectUrl).toContain('error=session_mismatch'); + }); + + it('proceeds when session.user.id matches and activeOrganizationId is absent', async () => { + mockOAuthStateRepository.findByState.mockResolvedValue(validState); + // Session with userId match but no activeOrganizationId — still allowed, + // since the state itself already binds the organization. + setMatchingSession({ activeOrganizationId: null }); + mockedGetManifest.mockReturnValue(undefined as never); + + await controller.oauthCallback( + { code: 'auth_code', state: 'valid_state' }, + mockRequest, + mockResponse, + ); + + // Session check passed → we reach the manifest lookup, fail there, + // redirect with token_exchange_failed (NOT session_mismatch). + const redirectUrl = (mockResponse.redirect as jest.Mock).mock + .calls[0][0]; + expect(redirectUrl).toContain('error=token_exchange_failed'); + expect(redirectUrl).not.toContain('error=session_mismatch'); + }); + }); }); }); diff --git a/apps/api/src/integration-platform/controllers/oauth.controller.ts b/apps/api/src/integration-platform/controllers/oauth.controller.ts index 6b92e1e82b..3d3d5e8120 100644 --- a/apps/api/src/integration-platform/controllers/oauth.controller.ts +++ b/apps/api/src/integration-platform/controllers/oauth.controller.ts @@ -4,6 +4,7 @@ import { Post, Query, Body, + Req, Res, HttpException, HttpStatus, @@ -11,8 +12,9 @@ import { UseGuards, } from '@nestjs/common'; import { ApiTags, ApiSecurity, ApiOperation } from '@nestjs/swagger'; -import type { Response } from 'express'; +import type { Request, Response } from 'express'; import { randomBytes, createHash } from 'crypto'; +import { auth } from '../../auth/auth.server'; import { HybridAuthGuard } from '../../auth/hybrid-auth.guard'; import { PermissionGuard } from '../../auth/permission.guard'; import { RequirePermission } from '../../auth/require-permission.decorator'; @@ -212,12 +214,21 @@ export class OAuthController { } /** - * OAuth callback - exchanges code for tokens + * OAuth callback - exchanges code for tokens. + * + * Note: this endpoint is intentionally not protected by HybridAuthGuard so + * that external OAuth providers can redirect the user's browser back to it. + * Defense-in-depth session validation is performed manually below: after the + * server-side `state` token is validated, we additionally require that the + * caller's session matches the user/org that initiated the flow. If the + * session is missing or mismatched, the flow is rejected — the user must + * restart it from the originating browser/session. */ @Get('callback') @ApiOperation({ summary: 'Handle OAuth provider callback' }) async oauthCallback( @Query() query: OAuthCallbackQuery, + @Req() req: Request, @Res() res: Response, ): Promise { const { code, state, error, error_description } = query; @@ -267,6 +278,28 @@ export class OAuthController { return; } + // Defense-in-depth: ensure the same browser session that started the flow + // is the one completing it. The `state` token alone provides CSRF + // protection, but a session match guards against state-token leakage and + // ensures the completing user still has an active session for the org. + const sessionMismatch = await this.checkSessionMatchesState(req, oauthState); + if (sessionMismatch) { + await this.oauthStateRepository.delete(state); + this.logger.warn( + `OAuth callback session mismatch (${sessionMismatch.reason}) for state belonging to user ${oauthState.userId}, org ${oauthState.organizationId}`, + ); + const errorUrl = this.buildRedirectUrl( + oauthState.redirectUrl, + { + error: 'session_mismatch', + error_description: sessionMismatch.message, + }, + oauthState.organizationId, + ); + res.redirect(errorUrl); + return; + } + try { // Get manifest and OAuth config const manifest = getManifest(oauthState.providerSlug); @@ -400,6 +433,70 @@ export class OAuthController { } } + /** + * Verify that the caller's better-auth session matches the user (and active + * org, when present) recorded on the OAuthState. Returns a reason on + * mismatch, or `null` if the session is OK to continue. We intentionally + * resolve the session manually (rather than via HybridAuthGuard) so we can + * return a friendly redirect with `error=session_mismatch` instead of a 401. + */ + private async checkSessionMatchesState( + req: Request, + oauthState: { userId: string; organizationId: string }, + ): Promise<{ reason: string; message: string } | null> { + const headers = new Headers(); + const authHeader = req.headers['authorization']; + if (typeof authHeader === 'string' && authHeader) { + headers.set('authorization', authHeader); + } + const cookieHeader = req.headers['cookie']; + if (typeof cookieHeader === 'string' && cookieHeader) { + headers.set('cookie', cookieHeader); + } + + if (!authHeader && !cookieHeader) { + return { + reason: 'no_session', + message: + 'No active session. Please sign in and restart the integration flow.', + }; + } + + const session = await auth.api.getSession({ headers }); + const sessionUserId = session?.user?.id; + if (!sessionUserId) { + return { + reason: 'no_session', + message: + 'No active session. Please sign in and restart the integration flow.', + }; + } + + if (sessionUserId !== oauthState.userId) { + return { + reason: 'user_mismatch', + message: + 'OAuth flow can only be completed by the user who initiated it.', + }; + } + + const sessionData = session.session as Record | undefined; + const activeOrgRaw = sessionData?.activeOrganizationId; + const activeOrganizationId = + typeof activeOrgRaw === 'string' ? activeOrgRaw : undefined; + if ( + activeOrganizationId && + activeOrganizationId !== oauthState.organizationId + ) { + return { + reason: 'organization_mismatch', + message: 'OAuth flow organization does not match the active session.', + }; + } + + return null; + } + private async exchangeCodeForTokens( config: OAuthConfig, credentials: { clientId: string; clientSecret: string }, diff --git a/apps/api/src/people/people.controller.spec.ts b/apps/api/src/people/people.controller.spec.ts index c7111a8b53..d418cd8be5 100644 --- a/apps/api/src/people/people.controller.spec.ts +++ b/apps/api/src/people/people.controller.spec.ts @@ -217,6 +217,7 @@ describe('PeopleController', () => { 'mem_1', 'org_123', dto, + 'usr_123', ); }); }); diff --git a/apps/api/src/people/people.controller.ts b/apps/api/src/people/people.controller.ts index 899ada7376..464f58d448 100644 --- a/apps/api/src/people/people.controller.ts +++ b/apps/api/src/people/people.controller.ts @@ -399,6 +399,7 @@ export class PeopleController { memberId, organizationId, updateData, + authContext.userId, ); return { diff --git a/apps/api/src/people/people.service.spec.ts b/apps/api/src/people/people.service.spec.ts index ea0a77b7d5..a3003b5f56 100644 --- a/apps/api/src/people/people.service.spec.ts +++ b/apps/api/src/people/people.service.spec.ts @@ -48,6 +48,12 @@ jest.mock('@db', () => ({ update: jest.fn(), }, }, + FindingType: { soc2: 'soc2', iso27001: 'iso27001' }, + FindingStatus: { open: 'open', closed: 'closed' }, + PhaseCompletionType: { manual: 'manual', auto: 'auto' }, + TimelinePhaseStatus: { pending: 'pending', completed: 'completed' }, + TimelineStatus: { draft: 'draft', active: 'active' }, + Departments: { it: 'it', none: 'none' }, })); jest.mock('@trycompai/auth', () => ({ @@ -223,17 +229,40 @@ describe('PeopleService', () => { }); describe('updateById', () => { + const setupCallerMember = ( + callerUserId: string, + organizationId: string, + role: string, + ) => { + (db.member.findFirst as jest.Mock).mockImplementation( + (args: { where: { userId?: string; organizationId?: string } }) => { + if ( + args?.where?.userId === callerUserId && + args?.where?.organizationId === organizationId + ) { + return Promise.resolve({ + id: 'mem_caller', + userId: callerUserId, + organizationId, + role, + }); + } + return Promise.resolve(null); + }, + ); + }; + it('should update a member', async () => { - const updateData = { role: 'admin' }; + const updateData = { role: 'auditor' }; const existingMember = { id: 'mem_1', - userId: 'usr_1', + userId: 'usr_target', role: 'employee', }; const updatedMember = { id: 'mem_1', user: { name: 'Alice' }, - role: 'admin', + role: 'auditor', }; (MemberValidator.validateOrganization as jest.Mock).mockResolvedValue( @@ -245,11 +274,13 @@ describe('PeopleService', () => { (MemberQueries.updateMember as jest.Mock).mockResolvedValue( updatedMember, ); + setupCallerMember('usr_caller', 'org_123', 'admin,auditor'); const result = await service.updateById( 'mem_1', 'org_123', updateData as any, + 'usr_caller', ); expect(result).toEqual(updatedMember); @@ -286,8 +317,14 @@ describe('PeopleService', () => { (MemberQueries.updateMember as jest.Mock).mockResolvedValue( updatedMember, ); + setupCallerMember('usr_caller', 'org_123', 'admin'); - await service.updateById('mem_1', 'org_123', updateData as any); + await service.updateById( + 'mem_1', + 'org_123', + updateData as any, + 'usr_caller', + ); expect(MemberValidator.validateUser).toHaveBeenCalledWith('usr_new'); expect(MemberValidator.validateUserNotMember).toHaveBeenCalledWith( @@ -304,11 +341,179 @@ describe('PeopleService', () => { (MemberValidator.validateMemberExists as jest.Mock).mockRejectedValue( new NotFoundException('Member not found'), ); + setupCallerMember('usr_caller', 'org_123', 'admin'); await expect( - service.updateById('mem_none', 'org_123', {} as any), + service.updateById('mem_none', 'org_123', {} as any, 'usr_caller'), ).rejects.toThrow(NotFoundException); }); + + describe('role authorization', () => { + const targetMember = { + id: 'mem_target', + userId: 'usr_target', + role: 'employee', + }; + + beforeEach(() => { + (MemberValidator.validateOrganization as jest.Mock).mockResolvedValue( + undefined, + ); + (MemberValidator.validateMemberExists as jest.Mock).mockResolvedValue( + targetMember, + ); + (MemberQueries.updateMember as jest.Mock).mockResolvedValue({ + id: 'mem_target', + user: { name: 'Target' }, + role: 'employee', + }); + }); + + it('should forbid admin from assigning owner role to themselves', async () => { + const callerMember = { + id: 'mem_self', + userId: 'usr_caller', + role: 'admin', + }; + (MemberValidator.validateMemberExists as jest.Mock).mockResolvedValue( + callerMember, + ); + setupCallerMember('usr_caller', 'org_123', 'admin'); + + await expect( + service.updateById( + 'mem_self', + 'org_123', + { role: 'owner' } as any, + 'usr_caller', + ), + ).rejects.toThrow(ForbiddenException); + }); + + it('should forbid admin from assigning owner role to another user', async () => { + setupCallerMember('usr_caller', 'org_123', 'admin'); + + await expect( + service.updateById( + 'mem_target', + 'org_123', + { role: 'owner' } as any, + 'usr_caller', + ), + ).rejects.toThrow(ForbiddenException); + }); + + it('should forbid admin from changing their own role at all', async () => { + const callerMember = { + id: 'mem_self', + userId: 'usr_caller', + role: 'admin', + }; + (MemberValidator.validateMemberExists as jest.Mock).mockResolvedValue( + callerMember, + ); + setupCallerMember('usr_caller', 'org_123', 'admin'); + + await expect( + service.updateById( + 'mem_self', + 'org_123', + { role: 'auditor' } as any, + 'usr_caller', + ), + ).rejects.toThrow(ForbiddenException); + }); + + it('should forbid admin from assigning a role they do not have', async () => { + setupCallerMember('usr_caller', 'org_123', 'admin'); + + await expect( + service.updateById( + 'mem_target', + 'org_123', + { role: 'custom_special' } as any, + 'usr_caller', + ), + ).rejects.toThrow(ForbiddenException); + }); + + it('should allow admin to assign auditor to another user', async () => { + setupCallerMember('usr_caller', 'org_123', 'admin,auditor'); + + await expect( + service.updateById( + 'mem_target', + 'org_123', + { role: 'auditor' } as any, + 'usr_caller', + ), + ).resolves.toBeDefined(); + + expect(MemberQueries.updateMember).toHaveBeenCalled(); + }); + + it('should allow owner to demote/promote others without using owner role', async () => { + setupCallerMember('usr_caller', 'org_123', 'owner'); + + await expect( + service.updateById( + 'mem_target', + 'org_123', + { role: 'admin' } as any, + 'usr_caller', + ), + ).resolves.toBeDefined(); + + expect(MemberQueries.updateMember).toHaveBeenCalled(); + }); + + it('should forbid demoting the owner via this endpoint (downgrade requires transfer-ownership)', async () => { + const ownerTarget = { + id: 'mem_target', + userId: 'usr_target', + role: 'owner', + }; + (MemberValidator.validateMemberExists as jest.Mock).mockResolvedValue( + ownerTarget, + ); + setupCallerMember('usr_caller', 'org_123', 'owner'); + + await expect( + service.updateById( + 'mem_target', + 'org_123', + { role: 'admin' } as any, + 'usr_caller', + ), + ).rejects.toThrow(ForbiddenException); + }); + + it('should forbid update when caller is not a member of the org', async () => { + (db.member.findFirst as jest.Mock).mockResolvedValue(null); + + await expect( + service.updateById( + 'mem_target', + 'org_123', + { role: 'auditor' } as any, + 'usr_caller', + ), + ).rejects.toThrow(ForbiddenException); + }); + + it('should skip role authorization when role is not being changed', async () => { + // When role is not in the update payload, no caller-role lookup or + // self-mod check happens — non-role updates flow through unchanged. + await expect( + service.updateById( + 'mem_target', + 'org_123', + { jobTitle: 'Director' } as any, + 'usr_caller', + ), + ).resolves.toBeDefined(); + }); + }); }); describe('deleteById', () => { diff --git a/apps/api/src/people/people.service.ts b/apps/api/src/people/people.service.ts index e0ab4cc2e5..c0b89448ce 100644 --- a/apps/api/src/people/people.service.ts +++ b/apps/api/src/people/people.service.ts @@ -14,6 +14,7 @@ import type { UpdatePeopleDto } from './dto/update-people.dto'; import type { BulkCreatePeopleDto } from './dto/bulk-create-people.dto'; import { MemberValidator } from './utils/member-validator'; import { MemberQueries } from './utils/member-queries'; +import { authorizeRoleChange } from './utils/role-authorization'; import { collectAssignedItems, clearAssignments, @@ -292,6 +293,7 @@ export class PeopleService { memberId: string, organizationId: string, updateData: UpdatePeopleDto, + callerUserId?: string, ): Promise { try { await MemberValidator.validateOrganization(organizationId); @@ -300,6 +302,20 @@ export class PeopleService { organizationId, ); + if (updateData.role !== undefined) { + if (!callerUserId) { + throw new ForbiddenException( + 'Role changes require an authenticated session caller', + ); + } + await authorizeRoleChange({ + callerUserId, + organizationId, + targetMember: existingMember, + newRole: updateData.role, + }); + } + // If userId is being updated, validate the new user if (updateData.userId && updateData.userId !== existingMember.userId) { await MemberValidator.validateUser(updateData.userId); @@ -323,7 +339,8 @@ export class PeopleService { } catch (error) { if ( error instanceof NotFoundException || - error instanceof BadRequestException + error instanceof BadRequestException || + error instanceof ForbiddenException ) { throw error; } diff --git a/apps/api/src/people/utils/member-validator.ts b/apps/api/src/people/utils/member-validator.ts index c71ee29340..a412eb0b92 100644 --- a/apps/api/src/people/utils/member-validator.ts +++ b/apps/api/src/people/utils/member-validator.ts @@ -42,14 +42,14 @@ export class MemberValidator { static async validateMemberExists( memberId: string, organizationId: string, - ): Promise<{ id: string; userId: string }> { + ): Promise<{ id: string; userId: string; role: string }> { const member = await db.member.findFirst({ where: { id: memberId, organizationId, deactivated: false, }, - select: { id: true, userId: true }, + select: { id: true, userId: true, role: true }, }); if (!member) { diff --git a/apps/api/src/people/utils/role-authorization.ts b/apps/api/src/people/utils/role-authorization.ts new file mode 100644 index 0000000000..7fe29efd63 --- /dev/null +++ b/apps/api/src/people/utils/role-authorization.ts @@ -0,0 +1,78 @@ +import { ForbiddenException } from '@nestjs/common'; +import { db } from '@db'; + +const OWNER_ROLE = 'owner'; + +/** + * Parse a comma-separated role string into a clean list of role names. + */ +export function parseRoles(role: string | null | undefined): string[] { + return (role ?? '') + .split(',') + .map((r) => r.trim()) + .filter(Boolean); +} + +interface AuthorizeRoleChangeParams { + callerUserId: string; + organizationId: string; + targetMember: { id: string; userId: string; role: string }; + newRole: string; +} + +/** + * Centralized authorization for role changes via PATCH /v1/people/:id. + * + * Enforces: + * - Caller cannot change their OWN role (use /organization/transfer-ownership for owner moves). + * - The 'owner' role can only be granted/revoked via /organization/transfer-ownership. + * - A caller cannot assign a role they do not themselves possess. + */ +export async function authorizeRoleChange({ + callerUserId, + organizationId, + targetMember, + newRole, +}: AuthorizeRoleChangeParams): Promise { + const callerMember = await db.member.findFirst({ + where: { userId: callerUserId, organizationId }, + select: { id: true, role: true }, + }); + + if (!callerMember) { + throw new ForbiddenException( + 'Caller is not a member of this organization', + ); + } + + const callerRoles = parseRoles(callerMember.role); + const existingRoles = parseRoles(targetMember.role); + const newRoles = parseRoles(newRole); + + if (targetMember.userId === callerUserId) { + throw new ForbiddenException( + 'You cannot change your own role. Use /organization/transfer-ownership to change ownership.', + ); + } + + const newHasOwner = newRoles.includes(OWNER_ROLE); + const existingHasOwner = existingRoles.includes(OWNER_ROLE); + + if (newHasOwner !== existingHasOwner) { + throw new ForbiddenException( + 'Owner role can only be assigned via /organization/transfer-ownership', + ); + } + + if (callerRoles.includes(OWNER_ROLE)) { + return; + } + + for (const role of newRoles) { + if (!callerRoles.includes(role)) { + throw new ForbiddenException( + `You cannot assign the role "${role}" because you do not hold it`, + ); + } + } +} diff --git a/apps/api/src/secrets/secrets-permissions.spec.ts b/apps/api/src/secrets/secrets-permissions.spec.ts new file mode 100644 index 0000000000..b6f660d5c5 --- /dev/null +++ b/apps/api/src/secrets/secrets-permissions.spec.ts @@ -0,0 +1,117 @@ +/** + * AUTHZ-VULN-01 regression: ensure the `secret` resource is granted only to + * roles that should see DECRYPTED plaintext credentials. + * + * The secrets manager surfaces decrypted values to the user — read access on + * this resource MUST never be implicit (e.g. via `organization:read`) because + * read-only auditors would otherwise gain access to plaintext credentials. + * + * We mock the `better-auth/plugins/access` and + * `better-auth/plugins/organization/access` ESM modules using their resolved + * paths from inside `@trycompai/auth` (otherwise Jest would try to evaluate + * the real `.mjs` files as CommonJS and crash). Then we import the role + * definitions directly from `permissions.ts` — bypassing `server.ts`, which + * pulls in the rest of better-auth. + */ + +jest.mock( + require.resolve('better-auth/plugins/access', { + paths: [require.resolve('@trycompai/auth')], + }), + () => ({ + createAccessControl: (_stmt: Record) => ({ + newRole: (statements: Record) => ({ + statements, + }), + }), + }), +); + +jest.mock( + require.resolve('better-auth/plugins/organization/access', { + paths: [require.resolve('@trycompai/auth')], + }), + () => ({ + defaultStatements: { + organization: ['update', 'delete'], + member: ['create', 'update', 'delete'], + invitation: ['create', 'delete'], + team: ['create', 'update', 'delete'], + }, + ownerAc: { + statements: { + organization: ['update', 'delete'], + member: ['create', 'update', 'delete'], + invitation: ['create', 'delete'], + team: ['create', 'update', 'delete'], + }, + }, + adminAc: { + statements: { + organization: ['update'], + member: ['create', 'update', 'delete'], + invitation: ['create', 'delete'], + team: ['create', 'update', 'delete'], + }, + }, + }), +); + +// Import permissions.ts directly — going through the package barrel +// (@trycompai/auth) would also load server.ts which pulls the entire +// better-auth surface and breaks the test. +import { + BUILT_IN_ROLE_PERMISSIONS, + statement, +} from '../../../../packages/auth/src/permissions'; + +describe('Secrets resource — role grants', () => { + const fullCrud = ['create', 'read', 'update', 'delete']; + + it('declares secret in the permission statement schema', () => { + expect(statement.secret).toEqual( + expect.arrayContaining(['create', 'read', 'update', 'delete']), + ); + }); + + describe('owner role', () => { + it('should be granted secret CRUD', () => { + const perms = BUILT_IN_ROLE_PERMISSIONS.owner; + expect(perms.secret).toEqual(expect.arrayContaining(fullCrud)); + }); + }); + + describe('admin role', () => { + it('should be granted secret CRUD', () => { + const perms = BUILT_IN_ROLE_PERMISSIONS.admin; + expect(perms.secret).toEqual(expect.arrayContaining(fullCrud)); + }); + }); + + describe('auditor role', () => { + // Read-only compliance reviewer. They must NEVER see DECRYPTED secrets. + it('MUST NOT be granted any secret action', () => { + const perms = BUILT_IN_ROLE_PERMISSIONS.auditor; + expect(perms.secret).toBeUndefined(); + }); + + it('MUST NOT have secret:read (regression for AUTHZ-VULN-01)', () => { + const perms = BUILT_IN_ROLE_PERMISSIONS.auditor; + expect(perms.secret ?? []).not.toContain('read'); + }); + }); + + describe('employee role', () => { + it('MUST NOT be granted any secret action', () => { + const perms = BUILT_IN_ROLE_PERMISSIONS.employee; + expect(perms.secret).toBeUndefined(); + }); + }); + + describe('contractor role', () => { + it('MUST NOT be granted any secret action', () => { + const perms = BUILT_IN_ROLE_PERMISSIONS.contractor; + expect(perms.secret).toBeUndefined(); + }); + }); +}); diff --git a/apps/api/src/secrets/secrets.controller.spec.ts b/apps/api/src/secrets/secrets.controller.spec.ts index 776d5478f4..e59b633749 100644 --- a/apps/api/src/secrets/secrets.controller.spec.ts +++ b/apps/api/src/secrets/secrets.controller.spec.ts @@ -1,9 +1,9 @@ import { Test, TestingModule } from '@nestjs/testing'; -import { HybridAuthGuard } from '../auth/hybrid-auth.guard'; -import { PermissionGuard } from '../auth/permission.guard'; -import type { AuthContext } from '../auth/types'; -import { SecretsController } from './secrets.controller'; -import { SecretsService } from './secrets.service'; + +// Mock @db before any controller imports load Prisma client lazily. +jest.mock('@db', () => ({ + db: {}, +})); // Mock auth.server to avoid importing better-auth ESM in Jest jest.mock('../auth/auth.server', () => ({ @@ -15,6 +15,12 @@ jest.mock('@trycompai/auth', () => ({ BUILT_IN_ROLE_PERMISSIONS: {}, })); +import { HybridAuthGuard } from '../auth/hybrid-auth.guard'; +import { PermissionGuard, PERMISSIONS_KEY } from '../auth/permission.guard'; +import type { AuthContext } from '../auth/types'; +import { SecretsController } from './secrets.controller'; +import { SecretsService } from './secrets.service'; + describe('SecretsController', () => { let controller: SecretsController; let secretsService: jest.Mocked; @@ -186,4 +192,56 @@ describe('SecretsController', () => { }); }); }); + + describe('RBAC - permission decorators', () => { + // AUTHZ-VULN-01: Secrets endpoints previously gated on `organization` + // permissions, which let read-only auditors fetch DECRYPTED plaintext. + // Each endpoint must now require the dedicated `secret` resource so the + // owner/admin grant is the only path to decrypted credentials. + it('listSecrets should require secret:read (NOT organization:read)', () => { + const permissions = Reflect.getMetadata( + PERMISSIONS_KEY, + controller.listSecrets, + ); + expect(permissions).toEqual([{ resource: 'secret', actions: ['read'] }]); + }); + + it('getSecret should require secret:read', () => { + const permissions = Reflect.getMetadata( + PERMISSIONS_KEY, + controller.getSecret, + ); + expect(permissions).toEqual([{ resource: 'secret', actions: ['read'] }]); + }); + + it('createSecret should require secret:create', () => { + const permissions = Reflect.getMetadata( + PERMISSIONS_KEY, + controller.createSecret, + ); + expect(permissions).toEqual([ + { resource: 'secret', actions: ['create'] }, + ]); + }); + + it('updateSecret should require secret:update', () => { + const permissions = Reflect.getMetadata( + PERMISSIONS_KEY, + controller.updateSecret, + ); + expect(permissions).toEqual([ + { resource: 'secret', actions: ['update'] }, + ]); + }); + + it('deleteSecret should require secret:delete', () => { + const permissions = Reflect.getMetadata( + PERMISSIONS_KEY, + controller.deleteSecret, + ); + expect(permissions).toEqual([ + { resource: 'secret', actions: ['delete'] }, + ]); + }); + }); }); diff --git a/apps/api/src/secrets/secrets.controller.ts b/apps/api/src/secrets/secrets.controller.ts index 8eb09428e4..992ea826b9 100644 --- a/apps/api/src/secrets/secrets.controller.ts +++ b/apps/api/src/secrets/secrets.controller.ts @@ -30,7 +30,7 @@ export class SecretsController { constructor(private readonly secretsService: SecretsService) {} @Get() - @RequirePermission('organization', 'read') + @RequirePermission('secret', 'read') @ApiOperation({ summary: 'List all secrets (metadata only, no values)' }) async listSecrets( @OrganizationId() organizationId: string, @@ -53,7 +53,7 @@ export class SecretsController { } @Get(':id') - @RequirePermission('organization', 'read') + @RequirePermission('secret', 'read') @ApiOperation({ summary: 'Get a secret with decrypted value' }) @ApiParam({ name: 'id', description: 'Secret ID' }) async getSecret( @@ -77,7 +77,7 @@ export class SecretsController { } @Post() - @RequirePermission('organization', 'update') + @RequirePermission('secret', 'create') @ApiOperation({ summary: 'Create a new secret' }) @ApiBody({ schema: { @@ -118,7 +118,7 @@ export class SecretsController { } @Put(':id') - @RequirePermission('organization', 'update') + @RequirePermission('secret', 'update') @ApiOperation({ summary: 'Update a secret' }) @ApiParam({ name: 'id', description: 'Secret ID' }) async updateSecret( @@ -153,7 +153,7 @@ export class SecretsController { } @Delete(':id') - @RequirePermission('organization', 'update') + @RequirePermission('secret', 'delete') @ApiOperation({ summary: 'Delete a secret' }) @ApiParam({ name: 'id', description: 'Secret ID' }) async deleteSecret( diff --git a/apps/api/src/training/permissions-regression.spec.ts b/apps/api/src/training/permissions-regression.spec.ts index 1ec83db5f2..d563c67229 100644 --- a/apps/api/src/training/permissions-regression.spec.ts +++ b/apps/api/src/training/permissions-regression.spec.ts @@ -122,6 +122,12 @@ describe('Built-in role permissions — regression', () => { expect.arrayContaining(['create', 'read', 'update', 'delete']), ); }); + + it('should have secret CRUD', () => { + expect(perms.secret).toEqual( + expect.arrayContaining(['create', 'read', 'update', 'delete']), + ); + }); }); // ─── Admin ────────────────────────────────────────────────────────── @@ -176,6 +182,12 @@ describe('Built-in role permissions — regression', () => { expect.arrayContaining(['create', 'read', 'delete']), ); }); + + it('should have secret CRUD', () => { + expect(perms.secret).toEqual( + expect.arrayContaining(['create', 'read', 'update', 'delete']), + ); + }); }); // ─── Auditor ──────────────────────────────────────────────────────── @@ -232,6 +244,10 @@ describe('Built-in role permissions — regression', () => { it('should NOT have training permissions', () => { expect(perms.training).toBeUndefined(); }); + + it('should NOT have secret permissions (auditors must never see decrypted credentials)', () => { + expect(perms.secret).toBeUndefined(); + }); }); // ─── Employee ─────────────────────────────────────────────────────── @@ -269,6 +285,7 @@ describe('Built-in role permissions — regression', () => { 'apiKey', 'pentest', 'training', + 'secret', ]) { expect(perms[resource]).toBeUndefined(); } diff --git a/apps/app/src/app/(app)/[orgId]/settings/secrets/components/AddSecretDialog.tsx b/apps/app/src/app/(app)/[orgId]/settings/secrets/components/AddSecretDialog.tsx index fd2daf2221..98f03eebd0 100644 --- a/apps/app/src/app/(app)/[orgId]/settings/secrets/components/AddSecretDialog.tsx +++ b/apps/app/src/app/(app)/[orgId]/settings/secrets/components/AddSecretDialog.tsx @@ -40,7 +40,7 @@ export function AddSecretDialog() { const [open, setOpen] = useState(false); const { createSecret } = useSecrets(); const { hasPermission } = usePermissions(); - const canManageSecrets = hasPermission('organization', 'update'); + const canManageSecrets = hasPermission('secret', 'create'); const { handleSubmit, diff --git a/apps/app/src/app/(app)/[orgId]/settings/secrets/components/EditSecretDialog.tsx b/apps/app/src/app/(app)/[orgId]/settings/secrets/components/EditSecretDialog.tsx index 4073cef062..3ac2be339c 100644 --- a/apps/app/src/app/(app)/[orgId]/settings/secrets/components/EditSecretDialog.tsx +++ b/apps/app/src/app/(app)/[orgId]/settings/secrets/components/EditSecretDialog.tsx @@ -53,7 +53,7 @@ export function EditSecretDialog({ }: EditSecretDialogProps) { const { updateSecret } = useSecrets(); const { hasPermission } = usePermissions(); - const canManageSecrets = hasPermission('organization', 'update'); + const canManageSecrets = hasPermission('secret', 'update'); const { handleSubmit, control, diff --git a/apps/app/src/app/(app)/[orgId]/settings/secrets/components/table/SecretsTable.tsx b/apps/app/src/app/(app)/[orgId]/settings/secrets/components/table/SecretsTable.tsx index 2a0a3d1f5e..c884e9d3e3 100644 --- a/apps/app/src/app/(app)/[orgId]/settings/secrets/components/table/SecretsTable.tsx +++ b/apps/app/src/app/(app)/[orgId]/settings/secrets/components/table/SecretsTable.tsx @@ -73,7 +73,9 @@ function formatDate(date: string): string { export function SecretsTable({ initialSecrets }: SecretsTableProps) { const { secrets, deleteSecret } = useSecrets({ initialData: initialSecrets }); const { hasPermission } = usePermissions(); - const canUpdate = hasPermission('organization', 'update'); + const canEdit = hasPermission('secret', 'update'); + const canDelete = hasPermission('secret', 'delete'); + const canUpdate = canEdit || canDelete; const [revealedSecrets, setRevealedSecrets] = useState>({}); const [loadingSecrets, setLoadingSecrets] = useState>({}); @@ -296,26 +298,30 @@ export function SecretsTable({ initialSecrets }: SecretsTableProps) { - { - e.stopPropagation(); - setEditingSecret(secret); - }} - > - - Edit - - - { - e.stopPropagation(); - handleDeleteClick(secret); - }} - > - - Delete - + {canEdit && ( + { + e.stopPropagation(); + setEditingSecret(secret); + }} + > + + Edit + + )} + {canEdit && canDelete && } + {canDelete && ( + { + e.stopPropagation(); + handleDeleteClick(secret); + }} + > + + Delete + + )} diff --git a/apps/app/src/lib/permissions.ts b/apps/app/src/lib/permissions.ts index 79c96e4db3..60c442c1c3 100644 --- a/apps/app/src/lib/permissions.ts +++ b/apps/app/src/lib/permissions.ts @@ -80,10 +80,11 @@ export const ROUTE_PERMISSIONS: Record