Skip to content

Commit 806fd8e

Browse files
emrysaldevin-ai-integration[bot]
emrysal
and
devin-ai-integration[bot]
authored
fix: prevent IDOR in PBAC updateRole and deleteRole tRPC endpoints (calcom#28569)
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
1 parent ee973c6 commit 806fd8e

File tree

2 files changed

+253
-5
lines changed

2 files changed

+253
-5
lines changed

‎packages/trpc/server/routers/viewer/pbac/_router.idor-prevention.test.ts‎

Lines changed: 236 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,236 @@
1+
import { beforeEach, describe, expect, it, vi } from "vitest";
2+
3+
const mockCheckPermission = vi.fn();
4+
const mockRoleBelongsToTeam = vi.fn();
5+
const mockUpdate = vi.fn();
6+
const mockDeleteRole = vi.fn();
7+
const mockCreateRole = vi.fn();
8+
const mockGetTeamRoles = vi.fn();
9+
10+
vi.mock("@calcom/features/pbac/services/permission-check.service", () => ({
11+
PermissionCheckService: class {
12+
checkPermission = mockCheckPermission;
13+
},
14+
}));
15+
16+
vi.mock("@calcom/features/pbac/services/role.service", () => ({
17+
RoleService: class {
18+
roleBelongsToTeam = mockRoleBelongsToTeam;
19+
update = mockUpdate;
20+
deleteRole = mockDeleteRole;
21+
createRole = mockCreateRole;
22+
getTeamRoles = mockGetTeamRoles;
23+
},
24+
}));
25+
26+
vi.mock("@calcom/prisma", () => ({
27+
prisma: {
28+
membership: { findFirst: vi.fn() },
29+
},
30+
default: {},
31+
}));
32+
33+
vi.mock("@calcom/features/di/containers/TeamFeatureRepository", () => ({
34+
getTeamFeatureRepository: vi.fn(() => ({
35+
checkIfTeamHasFeature: vi.fn(),
36+
upsert: vi.fn(),
37+
})),
38+
}));
39+
40+
// Mock authedProcedure to be a passthrough that injects a fake user
41+
vi.mock("../../../procedures/authedProcedure", async () => {
42+
const { procedure } = await vi.importActual<typeof import("../../../trpc")>("../../../trpc");
43+
const { middleware } = await vi.importActual<typeof import("../../../trpc")>("../../../trpc");
44+
45+
const fakeAuthed = procedure.use(
46+
middleware(({ next }) => {
47+
return next({
48+
ctx: {
49+
user: { id: 1 },
50+
session: { user: { id: 1 }, upId: "usr-1" },
51+
},
52+
});
53+
})
54+
);
55+
56+
return {
57+
default: fakeAuthed,
58+
authedAdminProcedure: fakeAuthed,
59+
authedOrgAdminProcedure: fakeAuthed,
60+
};
61+
});
62+
63+
import type { InnerContext } from "../../../createContext";
64+
import { permissionsRouter } from "./_router";
65+
66+
function createTestCaller() {
67+
const mockContext: InnerContext = {
68+
prisma: {} as InnerContext["prisma"],
69+
insightsDb: {} as InnerContext["insightsDb"],
70+
locale: "en",
71+
traceContext: {
72+
traceId: "test-trace-id",
73+
spanId: "test-span-id",
74+
operation: "test",
75+
},
76+
};
77+
return permissionsRouter.createCaller(mockContext);
78+
}
79+
80+
describe("PBAC permissions router – IDOR prevention", () => {
81+
const TEAM_A_ID = 100;
82+
const ROLE_IN_TEAM_A = "role-team-a-uuid";
83+
const ROLE_IN_TEAM_B = "role-team-b-uuid";
84+
85+
beforeEach(() => {
86+
vi.clearAllMocks();
87+
});
88+
89+
describe("updateRole", () => {
90+
it("should succeed when roleId belongs to the specified teamId", async () => {
91+
mockCheckPermission.mockResolvedValue(true);
92+
mockRoleBelongsToTeam.mockResolvedValue(true);
93+
mockUpdate.mockResolvedValue({ id: ROLE_IN_TEAM_A, name: "Updated Role" });
94+
95+
const caller = createTestCaller();
96+
const result = await caller.updateRole({
97+
teamId: TEAM_A_ID,
98+
roleId: ROLE_IN_TEAM_A,
99+
name: "Updated Role",
100+
permissions: [],
101+
});
102+
103+
expect(result).toEqual({ id: ROLE_IN_TEAM_A, name: "Updated Role" });
104+
expect(mockRoleBelongsToTeam).toHaveBeenCalledWith(ROLE_IN_TEAM_A, TEAM_A_ID);
105+
expect(mockUpdate).toHaveBeenCalledWith({
106+
roleId: ROLE_IN_TEAM_A,
107+
permissions: [],
108+
updates: { name: "Updated Role", color: undefined },
109+
});
110+
});
111+
112+
it("should throw when roleId does NOT belong to the specified teamId (IDOR attempt)", async () => {
113+
mockCheckPermission.mockResolvedValue(true);
114+
mockRoleBelongsToTeam.mockResolvedValue(false);
115+
116+
const caller = createTestCaller();
117+
118+
await expect(
119+
caller.updateRole({
120+
teamId: TEAM_A_ID,
121+
roleId: ROLE_IN_TEAM_B,
122+
name: "Hijacked",
123+
permissions: [],
124+
})
125+
).rejects.toThrow("Role not found in this team");
126+
127+
expect(mockRoleBelongsToTeam).toHaveBeenCalledWith(ROLE_IN_TEAM_B, TEAM_A_ID);
128+
expect(mockUpdate).not.toHaveBeenCalled();
129+
});
130+
131+
it("should not reach ownership check if the user lacks permission", async () => {
132+
mockCheckPermission.mockResolvedValue(false);
133+
134+
const caller = createTestCaller();
135+
136+
await expect(
137+
caller.updateRole({
138+
teamId: TEAM_A_ID,
139+
roleId: ROLE_IN_TEAM_A,
140+
name: "Test",
141+
permissions: [],
142+
})
143+
).rejects.toThrow("You don't have permission to update roles");
144+
145+
expect(mockRoleBelongsToTeam).not.toHaveBeenCalled();
146+
expect(mockUpdate).not.toHaveBeenCalled();
147+
});
148+
});
149+
150+
describe("deleteRole", () => {
151+
it("should succeed when roleId belongs to the specified teamId", async () => {
152+
mockCheckPermission.mockResolvedValue(true);
153+
mockRoleBelongsToTeam.mockResolvedValue(true);
154+
mockDeleteRole.mockResolvedValue(undefined);
155+
156+
const caller = createTestCaller();
157+
const result = await caller.deleteRole({
158+
teamId: TEAM_A_ID,
159+
roleId: ROLE_IN_TEAM_A,
160+
});
161+
162+
expect(result).toEqual({ success: true });
163+
expect(mockRoleBelongsToTeam).toHaveBeenCalledWith(ROLE_IN_TEAM_A, TEAM_A_ID);
164+
expect(mockDeleteRole).toHaveBeenCalledWith(ROLE_IN_TEAM_A);
165+
});
166+
167+
it("should throw when roleId does NOT belong to the specified teamId (IDOR attempt)", async () => {
168+
mockCheckPermission.mockResolvedValue(true);
169+
mockRoleBelongsToTeam.mockResolvedValue(false);
170+
171+
const caller = createTestCaller();
172+
173+
await expect(
174+
caller.deleteRole({
175+
teamId: TEAM_A_ID,
176+
roleId: ROLE_IN_TEAM_B,
177+
})
178+
).rejects.toThrow("Role not found in this team");
179+
180+
expect(mockRoleBelongsToTeam).toHaveBeenCalledWith(ROLE_IN_TEAM_B, TEAM_A_ID);
181+
expect(mockDeleteRole).not.toHaveBeenCalled();
182+
});
183+
184+
it("should not reach ownership check if the user lacks permission", async () => {
185+
mockCheckPermission.mockResolvedValue(false);
186+
187+
const caller = createTestCaller();
188+
189+
await expect(
190+
caller.deleteRole({
191+
teamId: TEAM_A_ID,
192+
roleId: ROLE_IN_TEAM_A,
193+
})
194+
).rejects.toThrow("You don't have permission to delete roles");
195+
196+
expect(mockRoleBelongsToTeam).not.toHaveBeenCalled();
197+
expect(mockDeleteRole).not.toHaveBeenCalled();
198+
});
199+
});
200+
201+
describe("cross-team IDOR scenario", () => {
202+
it("should prevent an admin of Team A from updating a role belonging to Team B", async () => {
203+
mockCheckPermission.mockResolvedValue(true);
204+
mockRoleBelongsToTeam.mockResolvedValue(false);
205+
206+
const caller = createTestCaller();
207+
208+
await expect(
209+
caller.updateRole({
210+
teamId: TEAM_A_ID,
211+
roleId: ROLE_IN_TEAM_B,
212+
name: "Malicious Update",
213+
permissions: [],
214+
})
215+
).rejects.toThrow("Role not found in this team");
216+
217+
expect(mockUpdate).not.toHaveBeenCalled();
218+
});
219+
220+
it("should prevent an admin of Team A from deleting a role belonging to Team B", async () => {
221+
mockCheckPermission.mockResolvedValue(true);
222+
mockRoleBelongsToTeam.mockResolvedValue(false);
223+
224+
const caller = createTestCaller();
225+
226+
await expect(
227+
caller.deleteRole({
228+
teamId: TEAM_A_ID,
229+
roleId: ROLE_IN_TEAM_B,
230+
})
231+
).rejects.toThrow("Role not found in this team");
232+
233+
expect(mockDeleteRole).not.toHaveBeenCalled();
234+
});
235+
});
236+
});

‎packages/trpc/server/routers/viewer/pbac/_router.tsx‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,11 @@
1-
import { z } from "zod";
2-
31
import { FeaturesRepository } from "@calcom/features/flags/features.repository";
4-
import { isValidPermissionString } from "@calcom/features/pbac/domain/types/permission-registry";
52
import type { PermissionString } from "@calcom/features/pbac/domain/types/permission-registry";
3+
import { isValidPermissionString } from "@calcom/features/pbac/domain/types/permission-registry";
64
import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service";
75
import { RoleService } from "@calcom/features/pbac/services/role.service";
86
import { prisma } from "@calcom/prisma";
9-
import { RoleType, MembershipRole } from "@calcom/prisma/enums";
10-
7+
import { MembershipRole, RoleType } from "@calcom/prisma/enums";
8+
import { z } from "zod";
119
import authedProcedure from "../../../procedures/authedProcedure";
1210
import { router } from "../../../trpc";
1311

@@ -129,6 +127,13 @@ export const permissionsRouter = router({
129127
}
130128

131129
const roleService = new RoleService();
130+
131+
// Verify the role belongs to the specified team to prevent IDOR
132+
const roleBelongsToTeam = await roleService.roleBelongsToTeam(input.roleId, input.teamId);
133+
if (!roleBelongsToTeam) {
134+
throw new Error("Role not found in this team");
135+
}
136+
132137
return roleService.update({
133138
roleId: input.roleId,
134139
permissions: input.permissions,
@@ -165,6 +170,13 @@ export const permissionsRouter = router({
165170
}
166171

167172
const roleService = new RoleService();
173+
174+
// Verify the role belongs to the specified team to prevent IDOR
175+
const roleBelongsToTeam = await roleService.roleBelongsToTeam(input.roleId, input.teamId);
176+
if (!roleBelongsToTeam) {
177+
throw new Error("Role not found in this team");
178+
}
179+
168180
await roleService.deleteRole(input.roleId);
169181
return { success: true };
170182
}),

0 commit comments

Comments
 (0)