fix: get bookings handler for pbac and fallback roles

[tomerqodo]

Benchmark PR from agentic-review-benchmarks#1

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. getTeamIdsWithPermission ignores orgId 📘 Rule violation ⛨ Security

Description

• orgId is documented and typed as a scoping parameter, but the implementation drops it when
  delegating to getTeamIdsWithPermissions().
• This can unintentionally broaden authorization scope (e.g., returning team IDs outside the
  intended organization scope), undermining access control guarantees for sensitive reads.

Code

packages/features/pbac/infrastructure/repositories/PermissionRepository.ts[R212-223]

    userId,
    permission,
    fallbackRoles,
+    orgId,
  }: {
    userId: number;
    permission: PermissionString;
    fallbackRoles: MembershipRole[];
+    orgId?: number;
  }): Promise<number[]> {
    return this.getTeamIdsWithPermissions({ userId, permissions: [permission], fallbackRoles });
  }

Evidence

The interface explicitly states orgId scopes returned team IDs, but the concrete repository
implementation does not forward orgId to the underlying permissions method, meaning the scoping
constraint is not actually applied.

Rule 6: Generic: Security-First Input Validation and Data Handling
packages/features/pbac/domain/repositories/IPermissionRepository.ts[64-73]
packages/features/pbac/infrastructure/repositories/PermissionRepository.ts[211-223]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`getTeamIdsWithPermission()` accepts `orgId` but does not pass it through to `getTeamIdsWithPermissions()`, so organization scoping is not applied.

## Issue Context
`orgId` is explicitly documented as a scoping parameter. Dropping it can lead to returning team IDs outside the intended org scope, weakening authorization boundaries.

## Fix Focus Areas
- packages/features/pbac/infrastructure/repositories/PermissionRepository.ts[211-223]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. userIds auth bypass 🐞 Bug ⛨ Security

Description

• In getBookings(), the userIds filter authorization allows bypass when the filter includes the
  current user plus unauthorized userIds.
• Because isCurrentUser is computed via includes() (not “only self”), the FORBIDDEN guard is
  skipped and the subsequent queries execute with the full filters.userIds set.
• This can expose bookings created by or attended by users outside the caller’s allowed scope.

Code

packages/trpc/server/routers/viewer/bookings/get.handler.ts[R143-152]

  // If userIds filter is provided
  if (!!filters?.userIds && filters.userIds.length > 0) {
    const areUserIdsWithinUserOrgOrTeam = filters.userIds.every((userId) =>
-      userIdsWhereUserIsAdminOrOwner.includes(userId)
+      allAccessibleUserIds.includes(userId)
    );

-    const isCurrentUser = filters.userIds.length === 1 && user.id === filters.userIds[0];
+    const isCurrentUser = filters.userIds.includes(user.id);

    //  Scope depends on `user.orgId`:
    // - Throw an error if trying to filter by usersIds that are not within your ORG

Evidence

The guard condition permits requests where filters.userIds contains the current user, even if
other IDs are outside allAccessibleUserIds, and then uses filters.userIds to query bookings.
This creates an access-control bypass: an attacker can include their own userId to avoid the
FORBIDDEN error while still querying other userIds.

packages/trpc/server/routers/viewer/bookings/get.handler.ts[143-172]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`getBookings()` allows bypassing the userIds authorization check by including the current userId alongside unauthorized userIds. The code treats `filters.userIds.includes(user.id)` as sufficient to skip the FORBIDDEN error.

### Issue Context
We must ensure that adding the current userId does not grant access to any other userIds outside the caller’s accessible set.

### Fix Focus Areas
- packages/trpc/server/routers/viewer/bookings/get.handler.ts[143-172]

### Suggested implementation
Adjust the logic to something like:
- `const isOnlyCurrentUser = filters.userIds.length === 1 && filters.userIds[0] === user.id;`
- `const otherUserIds = filters.userIds.filter((id) => id !== user.id);`
- `const areOtherUserIdsAuthorized = otherUserIds.every((id) => allAccessibleUserIds.includes(id));`
- Throw FORBIDDEN if `!isOnlyCurrentUser && !areOtherUserIdsAuthorized`.

Add a test case where `filters.userIds = [user.id, unauthorizedId]` and assert FORBIDDEN.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[qodo-code-review]

1. getteamidswithpermission ignores orgid 📘 Rule violation ⛨ Security

• orgId is documented and typed as a scoping parameter, but the implementation drops it when
  delegating to getTeamIdsWithPermissions().
• This can unintentionally broaden authorization scope (e.g., returning team IDs outside the
  intended organization scope), undermining access control guarantees for sensitive reads.

Agent prompt

## Issue description
`getTeamIdsWithPermission()` accepts `orgId` but does not pass it through to `getTeamIdsWithPermissions()`, so organization scoping is not applied.

## Issue Context
`orgId` is explicitly documented as a scoping parameter. Dropping it can lead to returning team IDs outside the intended org scope, weakening authorization boundaries.

## Fix Focus Areas
- packages/features/pbac/infrastructure/repositories/PermissionRepository.ts[211-223]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-code-review]

2. Userids auth bypass 🐞 Bug ⛨ Security

• In getBookings(), the userIds filter authorization allows bypass when the filter includes the
  current user plus unauthorized userIds.
• Because isCurrentUser is computed via includes() (not “only self”), the FORBIDDEN guard is
  skipped and the subsequent queries execute with the full filters.userIds set.
• This can expose bookings created by or attended by users outside the caller’s allowed scope.

Agent prompt

### Issue description
`getBookings()` allows bypassing the userIds authorization check by including the current userId alongside unauthorized userIds. The code treats `filters.userIds.includes(user.id)` as sufficient to skip the FORBIDDEN error.

### Issue Context
We must ensure that adding the current userId does not grant access to any other userIds outside the caller’s accessible set.

### Fix Focus Areas
- packages/trpc/server/routers/viewer/bookings/get.handler.ts[143-172]

### Suggested implementation
Adjust the logic to something like:
- `const isOnlyCurrentUser = filters.userIds.length === 1 && filters.userIds[0] === user.id;`
- `const otherUserIds = filters.userIds.filter((id) => id !== user.id);`
- `const areOtherUserIdsAuthorized = otherUserIds.every((id) => allAccessibleUserIds.includes(id));`
- Throw FORBIDDEN if `!isOnlyCurrentUser && !areOtherUserIdsAuthorized`.

Add a test case where `filters.userIds = [user.id, unauthorizedId]` and assert FORBIDDEN.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools