From 806fd8edd0afde1021c2284866be8b3c23367c90 Mon Sep 17 00:00:00 2001 From: Alex van Andel Date: Tue, 24 Mar 2026 22:08:35 +0000 Subject: [PATCH] fix: prevent IDOR in PBAC updateRole and deleteRole tRPC endpoints (#28569) Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../pbac/_router.idor-prevention.test.ts | 236 ++++++++++++++++++ .../server/routers/viewer/pbac/_router.tsx | 22 +- 2 files changed, 253 insertions(+), 5 deletions(-) create mode 100644 packages/trpc/server/routers/viewer/pbac/_router.idor-prevention.test.ts diff --git a/packages/trpc/server/routers/viewer/pbac/_router.idor-prevention.test.ts b/packages/trpc/server/routers/viewer/pbac/_router.idor-prevention.test.ts new file mode 100644 index 0000000000..d18bf56ed2 --- /dev/null +++ b/packages/trpc/server/routers/viewer/pbac/_router.idor-prevention.test.ts @@ -0,0 +1,236 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const mockCheckPermission = vi.fn(); +const mockRoleBelongsToTeam = vi.fn(); +const mockUpdate = vi.fn(); +const mockDeleteRole = vi.fn(); +const mockCreateRole = vi.fn(); +const mockGetTeamRoles = vi.fn(); + +vi.mock("@calcom/features/pbac/services/permission-check.service", () => ({ + PermissionCheckService: class { + checkPermission = mockCheckPermission; + }, +})); + +vi.mock("@calcom/features/pbac/services/role.service", () => ({ + RoleService: class { + roleBelongsToTeam = mockRoleBelongsToTeam; + update = mockUpdate; + deleteRole = mockDeleteRole; + createRole = mockCreateRole; + getTeamRoles = mockGetTeamRoles; + }, +})); + +vi.mock("@calcom/prisma", () => ({ + prisma: { + membership: { findFirst: vi.fn() }, + }, + default: {}, +})); + +vi.mock("@calcom/features/di/containers/TeamFeatureRepository", () => ({ + getTeamFeatureRepository: vi.fn(() => ({ + checkIfTeamHasFeature: vi.fn(), + upsert: vi.fn(), + })), +})); + +// Mock authedProcedure to be a passthrough that injects a fake user +vi.mock("../../../procedures/authedProcedure", async () => { + const { procedure } = await vi.importActual("../../../trpc"); + const { middleware } = await vi.importActual("../../../trpc"); + + const fakeAuthed = procedure.use( + middleware(({ next }) => { + return next({ + ctx: { + user: { id: 1 }, + session: { user: { id: 1 }, upId: "usr-1" }, + }, + }); + }) + ); + + return { + default: fakeAuthed, + authedAdminProcedure: fakeAuthed, + authedOrgAdminProcedure: fakeAuthed, + }; +}); + +import type { InnerContext } from "../../../createContext"; +import { permissionsRouter } from "./_router"; + +function createTestCaller() { + const mockContext: InnerContext = { + prisma: {} as InnerContext["prisma"], + insightsDb: {} as InnerContext["insightsDb"], + locale: "en", + traceContext: { + traceId: "test-trace-id", + spanId: "test-span-id", + operation: "test", + }, + }; + return permissionsRouter.createCaller(mockContext); +} + +describe("PBAC permissions router – IDOR prevention", () => { + const TEAM_A_ID = 100; + const ROLE_IN_TEAM_A = "role-team-a-uuid"; + const ROLE_IN_TEAM_B = "role-team-b-uuid"; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe("updateRole", () => { + it("should succeed when roleId belongs to the specified teamId", async () => { + mockCheckPermission.mockResolvedValue(true); + mockRoleBelongsToTeam.mockResolvedValue(true); + mockUpdate.mockResolvedValue({ id: ROLE_IN_TEAM_A, name: "Updated Role" }); + + const caller = createTestCaller(); + const result = await caller.updateRole({ + teamId: TEAM_A_ID, + roleId: ROLE_IN_TEAM_A, + name: "Updated Role", + permissions: [], + }); + + expect(result).toEqual({ id: ROLE_IN_TEAM_A, name: "Updated Role" }); + expect(mockRoleBelongsToTeam).toHaveBeenCalledWith(ROLE_IN_TEAM_A, TEAM_A_ID); + expect(mockUpdate).toHaveBeenCalledWith({ + roleId: ROLE_IN_TEAM_A, + permissions: [], + updates: { name: "Updated Role", color: undefined }, + }); + }); + + it("should throw when roleId does NOT belong to the specified teamId (IDOR attempt)", async () => { + mockCheckPermission.mockResolvedValue(true); + mockRoleBelongsToTeam.mockResolvedValue(false); + + const caller = createTestCaller(); + + await expect( + caller.updateRole({ + teamId: TEAM_A_ID, + roleId: ROLE_IN_TEAM_B, + name: "Hijacked", + permissions: [], + }) + ).rejects.toThrow("Role not found in this team"); + + expect(mockRoleBelongsToTeam).toHaveBeenCalledWith(ROLE_IN_TEAM_B, TEAM_A_ID); + expect(mockUpdate).not.toHaveBeenCalled(); + }); + + it("should not reach ownership check if the user lacks permission", async () => { + mockCheckPermission.mockResolvedValue(false); + + const caller = createTestCaller(); + + await expect( + caller.updateRole({ + teamId: TEAM_A_ID, + roleId: ROLE_IN_TEAM_A, + name: "Test", + permissions: [], + }) + ).rejects.toThrow("You don't have permission to update roles"); + + expect(mockRoleBelongsToTeam).not.toHaveBeenCalled(); + expect(mockUpdate).not.toHaveBeenCalled(); + }); + }); + + describe("deleteRole", () => { + it("should succeed when roleId belongs to the specified teamId", async () => { + mockCheckPermission.mockResolvedValue(true); + mockRoleBelongsToTeam.mockResolvedValue(true); + mockDeleteRole.mockResolvedValue(undefined); + + const caller = createTestCaller(); + const result = await caller.deleteRole({ + teamId: TEAM_A_ID, + roleId: ROLE_IN_TEAM_A, + }); + + expect(result).toEqual({ success: true }); + expect(mockRoleBelongsToTeam).toHaveBeenCalledWith(ROLE_IN_TEAM_A, TEAM_A_ID); + expect(mockDeleteRole).toHaveBeenCalledWith(ROLE_IN_TEAM_A); + }); + + it("should throw when roleId does NOT belong to the specified teamId (IDOR attempt)", async () => { + mockCheckPermission.mockResolvedValue(true); + mockRoleBelongsToTeam.mockResolvedValue(false); + + const caller = createTestCaller(); + + await expect( + caller.deleteRole({ + teamId: TEAM_A_ID, + roleId: ROLE_IN_TEAM_B, + }) + ).rejects.toThrow("Role not found in this team"); + + expect(mockRoleBelongsToTeam).toHaveBeenCalledWith(ROLE_IN_TEAM_B, TEAM_A_ID); + expect(mockDeleteRole).not.toHaveBeenCalled(); + }); + + it("should not reach ownership check if the user lacks permission", async () => { + mockCheckPermission.mockResolvedValue(false); + + const caller = createTestCaller(); + + await expect( + caller.deleteRole({ + teamId: TEAM_A_ID, + roleId: ROLE_IN_TEAM_A, + }) + ).rejects.toThrow("You don't have permission to delete roles"); + + expect(mockRoleBelongsToTeam).not.toHaveBeenCalled(); + expect(mockDeleteRole).not.toHaveBeenCalled(); + }); + }); + + describe("cross-team IDOR scenario", () => { + it("should prevent an admin of Team A from updating a role belonging to Team B", async () => { + mockCheckPermission.mockResolvedValue(true); + mockRoleBelongsToTeam.mockResolvedValue(false); + + const caller = createTestCaller(); + + await expect( + caller.updateRole({ + teamId: TEAM_A_ID, + roleId: ROLE_IN_TEAM_B, + name: "Malicious Update", + permissions: [], + }) + ).rejects.toThrow("Role not found in this team"); + + expect(mockUpdate).not.toHaveBeenCalled(); + }); + + it("should prevent an admin of Team A from deleting a role belonging to Team B", async () => { + mockCheckPermission.mockResolvedValue(true); + mockRoleBelongsToTeam.mockResolvedValue(false); + + const caller = createTestCaller(); + + await expect( + caller.deleteRole({ + teamId: TEAM_A_ID, + roleId: ROLE_IN_TEAM_B, + }) + ).rejects.toThrow("Role not found in this team"); + + expect(mockDeleteRole).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/packages/trpc/server/routers/viewer/pbac/_router.tsx b/packages/trpc/server/routers/viewer/pbac/_router.tsx index 1f9da6e4f7..2267422cda 100644 --- a/packages/trpc/server/routers/viewer/pbac/_router.tsx +++ b/packages/trpc/server/routers/viewer/pbac/_router.tsx @@ -1,13 +1,11 @@ -import { z } from "zod"; - import { FeaturesRepository } from "@calcom/features/flags/features.repository"; -import { isValidPermissionString } from "@calcom/features/pbac/domain/types/permission-registry"; import type { PermissionString } from "@calcom/features/pbac/domain/types/permission-registry"; +import { isValidPermissionString } from "@calcom/features/pbac/domain/types/permission-registry"; import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service"; import { RoleService } from "@calcom/features/pbac/services/role.service"; import { prisma } from "@calcom/prisma"; -import { RoleType, MembershipRole } from "@calcom/prisma/enums"; - +import { MembershipRole, RoleType } from "@calcom/prisma/enums"; +import { z } from "zod"; import authedProcedure from "../../../procedures/authedProcedure"; import { router } from "../../../trpc"; @@ -129,6 +127,13 @@ export const permissionsRouter = router({ } const roleService = new RoleService(); + + // Verify the role belongs to the specified team to prevent IDOR + const roleBelongsToTeam = await roleService.roleBelongsToTeam(input.roleId, input.teamId); + if (!roleBelongsToTeam) { + throw new Error("Role not found in this team"); + } + return roleService.update({ roleId: input.roleId, permissions: input.permissions, @@ -165,6 +170,13 @@ export const permissionsRouter = router({ } const roleService = new RoleService(); + + // Verify the role belongs to the specified team to prevent IDOR + const roleBelongsToTeam = await roleService.roleBelongsToTeam(input.roleId, input.teamId); + if (!roleBelongsToTeam) { + throw new Error("Role not found in this team"); + } + await roleService.deleteRole(input.roleId); return { success: true }; }),