Code Review Bench PR #25434 - fix: get bookings handler for pbac and fallback roles

[ketkarameya]

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#1

fix: get bookings handler for pbac and fallback roles

---

Summary by Gitar

• Permission-based access control:
  • Replaced role checks with PermissionCheckService using booking.read permission and [ADMIN, OWNER] fallback roles
• Organization scoping:
  • Added optional orgId parameter to IPermissionRepository for multi-org filtering
• Repository refactor:
  • Extracted SQL into getTeamsWithPBACPermissions and getTeamsWithFallbackRoles private methods
• Test coverage:
  • Added 127 lines of integration tests and 383 lines of unit tests in get.handler.test.ts

_{This will update automatically on new commits.}

---

[gitar-bot-staging]

🚨 Bug: orgId silently dropped in getTeamIdsWithPermission

In PermissionRepository.getTeamIdsWithPermission(), the orgId parameter is destructured from the input but is not forwarded to the delegated call to this.getTeamIdsWithPermissions(). This means organization-scoped filtering never takes effect when calling the singular getTeamIdsWithPermission method — which is exactly the entry point used by the getBookings handler (via permissionCheckService.getTeamIdsWithPermission).

Impact: When a user belongs to multiple organizations, the query will return team IDs from all organizations instead of only the user's current organization. This is a privilege escalation / data leakage issue — an org admin of Org A could see bookings from Org B's teams if they also have a membership in Org B.

The fix is to include orgId in the forwarded call.

_{Was this helpful? React with 👍 / 👎}

Suggested change

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

• Apply suggested fix

[gitar-bot-staging]

⚠️ Security: Relaxed isCurrentUser check enables permission bypass

The isCurrentUser check was changed from:

const isCurrentUser = filters.userIds.length === 1 && user.id === filters.userIds[0];

to:

const isCurrentUser = filters.userIds.includes(user.id);

The old logic required that the only requested user was the current user. The new logic only checks that the current user is among the requested users. This changes the security semantics: if a user requests userIds: [self, otherUser] where otherUser is outside their org/team, the old code would correctly reject the request (length > 1, so isCurrentUser = false, and areUserIdsWithinUserOrgOrTeam = false → FORBIDDEN). The new code would set isCurrentUser = true, bypassing the permission check entirely and allowing the user to see bookings for otherUser.

Impact: A user can include their own ID alongside any other user ID(s) to bypass the permission check and access bookings they shouldn't be able to see.

The fix should restore the original stricter semantics, or ensure the condition properly validates all requested userIds (not just that the current user is among them).

_{Was this helpful? React with 👍 / 👎}

Suggested change

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

• Apply suggested fix

[gitar-bot-staging]

⚠️ This PR is blocked due to unresolved code review findings.

_{Configure merge blocking · Maintainers can dismiss this review. Gitar never approves changes.}

[gitar-bot-staging]

Code Review 🚫 Blocked 0 resolved / 2 findings

PBAC refactoring has a critical bug where orgId is silently dropped in getTeamIdsWithPermission (defeating org-scoped filtering), and a security issue where the relaxed isCurrentUser check allows bypassing the permission gate by including your own user ID alongside unauthorized ones.

🚨 Bug: orgId silently dropped in getTeamIdsWithPermission

📄 packages/features/pbac/infrastructure/repositories/PermissionRepository.ts:222

In PermissionRepository.getTeamIdsWithPermission(), the orgId parameter is destructured from the input but is not forwarded to the delegated call to this.getTeamIdsWithPermissions(). This means organization-scoped filtering never takes effect when calling the singular getTeamIdsWithPermission method — which is exactly the entry point used by the getBookings handler (via permissionCheckService.getTeamIdsWithPermission).

Impact: When a user belongs to multiple organizations, the query will return team IDs from all organizations instead of only the user's current organization. This is a privilege escalation / data leakage issue — an org admin of Org A could see bookings from Org B's teams if they also have a membership in Org B.

The fix is to include orgId in the forwarded call.

Suggested fix

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

⚠️ Security: Relaxed isCurrentUser check enables permission bypass

📄 packages/trpc/server/routers/viewer/bookings/get.handler.ts:149

The isCurrentUser check was changed from:

const isCurrentUser = filters.userIds.length === 1 && user.id === filters.userIds[0];

to:

const isCurrentUser = filters.userIds.includes(user.id);

The old logic required that the only requested user was the current user. The new logic only checks that the current user is among the requested users. This changes the security semantics: if a user requests userIds: [self, otherUser] where otherUser is outside their org/team, the old code would correctly reject the request (length > 1, so isCurrentUser = false, and areUserIdsWithinUserOrgOrTeam = false → FORBIDDEN). The new code would set isCurrentUser = true, bypassing the permission check entirely and allowing the user to see bookings for otherUser.

Impact: A user can include their own ID alongside any other user ID(s) to bypass the permission check and access bookings they shouldn't be able to see.

The fix should restore the original stricter semantics, or ensure the condition properly validates all requested userIds (not just that the current user is among them).

Suggested fix

    const isCurrentUser = filters.userIds.length === 1 && filters.userIds[0] === user.id;

Rules 🎸 2 actions taken

Gitar Rules

🎸 Linear Auto-Linking: Searched for Linear issues and determined no high-confidence match exists for this PBAC refactoring work

🎸 Summary Enhancement: Enhanced PR description with technical summary covering PermissionCheckService refactoring, SQL extraction, organization scoping, and test coverage

_{4 rules not applicable. Show all rules by commenting gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}