fix: get bookings handler for pbac and fallback roles

[tomerqodo]

Benchmark PR from agentic-review-benchmarks#1

[claude]

🔴 Security: The isCurrentUser check on line 149 was changed from filters.userIds.length === 1 && user.id === filters.userIds[0] to filters.userIds.includes(user.id), introducing an authorization bypass. A user can include their own ID alongside unauthorized IDs (e.g., userIds: [ownId, targetId]) to pass the guard and view other users' bookings, since the full filters.userIds array is used in subsequent queries without filtering out unauthorized IDs.

Extended reasoning...

What the bug is

The isCurrentUser variable on line 149 of get.handler.ts is used as part of an authorization guard that prevents users from querying bookings of other users they do not have permission to access. The old code required filters.userIds.length === 1 && user.id === filters.userIds[0], meaning only a single-element array containing exactly the current user's ID would pass. The new code uses filters.userIds.includes(user.id), which passes for any array that contains the current user's ID regardless of what other IDs are present.

The specific code path

When a user calls the bookings endpoint with a userIds filter, the handler checks two conditions before throwing FORBIDDEN:

1. areUserIdsWithinUserOrgOrTeam — whether all requested user IDs belong to teams/orgs the caller has booking.read permission for
2. isCurrentUser — whether the caller is looking up their own bookings

The guard at line 155 is: if (!areUserIdsWithinUserOrgOrTeam && !isCurrentUser). If either condition is true, the guard passes.

Step-by-step proof of exploit

1. User ID 1 (a regular member with no admin rights) sends a request with filters.userIds = [1, 999]
2. areUserIdsWithinUserOrgOrTeam evaluates: does allAccessibleUserIds include both 1 and 999? User 999 is not in any team the caller administers, so this is false
3. isCurrentUser evaluates: does [1, 999].includes(1)? Yes → true
4. The guard becomes if (!false && !true) → if (true && false) → false, so no error is thrown
5. The query at line 170 uses filters.userIds directly: .where("userId", "in", filters.userIds) which expands to WHERE userId IN (1, 999)
6. Bookings for user 999 are returned to user 1 without authorization

Why existing code doesn't prevent it

There is no secondary filtering that strips unauthorized IDs from filters.userIds after the guard passes. The full array is passed directly into the SQL query. The guard was the sole line of defense, and the relaxed includes check defeats it.

Impact

This is a data exposure vulnerability. Any authenticated user can view any other user's bookings by appending the target's user ID to their own in the userIds filter. This leaks potentially sensitive scheduling data including booking titles, descriptions, attendee information, locations, and custom responses.

How to fix

Restore the original strict check: const isCurrentUser = filters.userIds.length === 1 && user.id === filters.userIds[0];. This ensures the self-lookup exemption only applies when the user is exclusively querying their own bookings. Alternatively, if the intent is to allow mixed queries, filter filters.userIds to only include authorized IDs before building the SQL queries.

[claude]

🔴 getTeamIdsWithPermission destructures orgId from its params (line 215) but does not forward it to the delegated getTeamIdsWithPermissions call on line 222. This means any caller passing orgId (e.g., get.handler.ts passing user.orgId) has the org-scoping silently ignored, causing the query to return teams from ALL organizations — a cross-org data leak. Fix: add orgId to the forwarded params object on line 222.

Extended reasoning...

Bug Analysis

The getTeamIdsWithPermission method in PermissionRepository.ts is a convenience wrapper that delegates to the plural getTeamIdsWithPermissions. On line 215, it correctly destructures orgId from its parameters:

async getTeamIdsWithPermission({
    userId,
    permission,
    fallbackRoles,
    orgId,  // <-- destructured here
  }: { ... }): Promise<number[]> {
    return this.getTeamIdsWithPermissions({ userId, permissions: [permission], fallbackRoles });
    //                                     ^^^^^ orgId is NOT included ^^^^^
  }

However, the delegation call on line 222 only passes { userId, permissions: [permission], fallbackRoles } — orgId is missing.

Code Path That Triggers This

In get.handler.ts, the getBookings function calls:

const teamIdsWithBookingPermission = await permissionCheckService.getTeamIdsWithPermission({
    userId: user.id,
    permission: "booking.read",
    fallbackRoles,
    orgId: user.orgId ?? undefined,
});

The PermissionCheckService.getTeamIdsWithPermission correctly forwards orgId to this.repository.getTeamIdsWithPermission(...). But when the repository’s singular method delegates to the plural method, orgId is dropped.

Why Existing Code Doesn’t Prevent This

The plural method getTeamIdsWithPermissions correctly accepts and uses orgId — it passes it to both getTeamsWithPBACPermissions and getTeamsWithFallbackRoles, which apply the SQL filter AND (${orgId}::bigint IS NULL OR t."id" = ${orgId} OR t."parentId" = ${orgId}). But since orgId arrives as undefined (because it was never forwarded), the SQL condition ${orgId}::bigint IS NULL evaluates to TRUE, effectively disabling the org filter.

Step-by-Step Proof

1. User is an admin in Org A (orgId=100) and also has membership in Org B (orgId=200).
2. User calls the bookings handler. get.handler.ts passes orgId: 100 to permissionCheckService.getTeamIdsWithPermission.
3. The service layer correctly passes orgId: 100 to repository.getTeamIdsWithPermission.
4. getTeamIdsWithPermission destructures orgId = 100 but calls getTeamIdsWithPermissions({ userId, permissions: [permission], fallbackRoles }) — no orgId.
5. getTeamIdsWithPermissions receives orgId = undefined.
6. The SQL queries use ${orgId}::bigint IS NULL which evaluates to TRUE since orgId is undefined/null.
7. Teams from Org B are returned alongside Org A teams.
8. The handler uses these team IDs to fetch bookings, so the user sees bookings from Org B that they should not have access to.

Impact

This is a cross-organization data leak. An org admin viewing bookings would see bookings from all organizations they belong to, rather than only their current organization. This completely defeats the purpose of the orgId scoping feature added in this PR.

Fix

Line 222 should be:

return this.getTeamIdsWithPermissions({ userId, permissions: [permission], fallbackRoles, orgId });

Note that the integration tests in this PR only test the plural getTeamIdsWithPermissions method directly with orgId, so they pass. But the singular getTeamIdsWithPermission (which is the method actually called by the handler) is not tested with orgId, which is why this bug was not caught.

[claude]

🟡 Several unused imports were left behind by this refactor: (1) Prisma as PrismaClientType in get.handler.ts:16 is dead code after the two functions that used PrismaClientType.MembershipListRelationFilter were removed, (2) TRPCError in permission-check.service.ts:6 is imported but never thrown or referenced, and (3) TRPCError in get.handler.test.ts:5 is imported but never used since error assertions use rejects.toMatchObject(). These will likely trigger lint errors (unused imports). The fix is to remove Prisma as PrismaClientType from the import on line 16, and remove the TRPCError imports from the other two files.

Extended reasoning...

What the bug is

This PR introduces or leaves behind three unused imports across three files. None of these imports are referenced anywhere in their respective files outside of the import statement itself.

Detailed analysis of each unused import

1. Prisma as PrismaClientType in get.handler.ts line 16:
The import line reads: import type { Booking, Prisma, Prisma as PrismaClientType } from "@calcom/prisma/client";

Booking is still used (line 87 for the BookingsUnionQuery type) and Prisma is still used (line 443 for Prisma.JsonValue), but PrismaClientType has zero references in the file. The PR removed both getEventTypeIdsWhereUserIsAdminOrOwner and getUserIdsAndEmailsWhereUserIsAdminOrOwner — the two functions that referenced PrismaClientType.MembershipListRelationFilter as a parameter type. The replacement function getUserIdsAndEmailsFromTeamIds takes teamIds: number[] instead, so the type alias is no longer needed.

2. TRPCError in permission-check.service.ts line 6:
The PR adds import { TRPCError } from "@trpc/server"; to this file, but TRPCError is never used anywhere in the file. All error handling in PermissionCheckService uses this.logger.error(error) followed by returning empty arrays — no throw new TRPCError(...) exists. This may also be a layer violation (features importing from @trpc/server), though that depends on project conventions.

3. TRPCError in get.handler.test.ts line 5:
The new test file imports TRPCError but never uses it. The FORBIDDEN error check uses rejects.toMatchObject({code: "FORBIDDEN", message: "..."}), which does not require the TRPCError class. While one verifier noted this overlaps with the permission-check.service.ts issue, these are distinct files with independent unused imports.

Step-by-step proof

For PrismaClientType: The import appears on line 16. Searching the file for PrismaClientType yields only the import line — zero usages in the ~900+ line file. The old code had membershipCondition: PrismaClientType.MembershipListRelationFilter in two function signatures, both of which were deleted by this PR.

For TRPCError in permission-check.service.ts: The import appears on line 6. Searching the file for TRPCError yields only the import line. The file’s catch blocks all follow the pattern this.logger.error(error); return [];.

For TRPCError in get.handler.test.ts: The import appears on line 5. Searching the file for TRPCError yields only the import line. The error assertion on line 200+ uses rejects.toMatchObject({code: "FORBIDDEN", ...}).

Impact

These are minor issues with no runtime impact, but they will likely trigger lint errors (e.g., no-unused-imports from biome/eslint) in CI. The fix is straightforward: change line 16 of get.handler.ts to import type { Booking, Prisma } from "@calcom/prisma/client";, and remove the TRPCError import lines from the other two files.