Skip to content

Comments

fix: ignore userIds form filter segment if no permission#24177

Closed
zhyd1997 wants to merge 5 commits intocalcom:mainfrom
zhyd1997:fix/issue-24172-1759237004
Closed

fix: ignore userIds form filter segment if no permission#24177
zhyd1997 wants to merge 5 commits intocalcom:mainfrom
zhyd1997:fix/issue-24172-1759237004

Conversation

@zhyd1997
Copy link
Contributor

What does this PR do?

Visual Demo (For contributors especially)

A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).

Video Demo (if applicable):

  • Show screen recordings of the issue or feature.
  • Demonstrate how to reproduce the issue, the behavior before and after the change.

Image Demo (if applicable):

  • Add side-by-side screenshots of the original and updated change.
  • Highlight any significant change(s).

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Are there environment variables that should be set?
  • What are the minimal test data to have?
  • What is expected (happy path) to have (input and output)?
  • Any other important info that could help to test that PR

Checklist

  • I haven't read the contributing guide
  • My code doesn't follow the style guidelines of this project
  • I haven't commented my code, particularly in hard-to-understand areas
  • I haven't checked if my changes generate no new warnings

@zhyd1997 zhyd1997 requested a review from a team as a code owner September 30, 2025 13:05
@vercel
Copy link

vercel bot commented Sep 30, 2025

@zhyd1997 is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 30, 2025

Walkthrough

Introduces an internal helper getFilteredUserIds to constrain requested userIds to the caller's permitted scope (returning permitted IDs, the current user, or empty). Replaces prior pre-check that threw FORBIDDEN with filtering and logs FORBIDDEN when out-of-scope IDs were requested. Refactors the bookings get handler to derive attendee emails and other query inputs from the filtered IDs, replaces inline boolean-guard patterns with explicit if checks, and conditions each booking-query branch on the presence of its derived data.

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues Check ✅ Passed The code changes implement a helper to filter out unauthorized userIds and update the bookings handler to ignore these IDs instead of returning an error, directly addressing the primary objective of both issue #24172 and issue CAL-6491 by replacing the strict pre-check with scoped filtering logic and ensuring the system silently drops unauthorized filters.
Out of Scope Changes Check ✅ Passed The pull request modifies only the get.handler.ts file to adjust userIds filtering logic and does not introduce any unrelated features or modifications outside the linked issues’ scope, with no additional files or functionality altered.
Description Check ✅ Passed The pull request description clearly outlines the fixed issue numbers, provides context on the change objective, and relates directly to the modifications introduced in the code, remaining on-topic and relevant to the changeset even with its template sections and demonstration prompts.
Title Check ✅ Passed Despite a minor typo, the title clearly summarizes the main change of ignoring unauthorized userIds in filter segments when the viewer lacks permission, making it directly related and specific to the PR’s purpose.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@graphite-app graphite-app bot added the community Created by Linear-GitHub Sync label Sep 30, 2025
@graphite-app graphite-app bot requested a review from a team September 30, 2025 13:05
@dosubot dosubot bot added bookings area: bookings, availability, timezones, double booking 🐛 bug Something isn't working labels Sep 30, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/bookings/get.handler.ts (1)

260-273: LGTM: Explicit conditionals improve readability.

The conversion from short-circuit boolean guards to explicit if blocks maintains the same semantics while improving code clarity. These changes ensure query fragments are only added when the corresponding data arrays are non-empty, making the conditional logic more explicit and easier to follow.

Also applies to: 277-291, 296-309, 314-326

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between fea7e66 and 65ea4ba.

📒 Files selected for processing (1)
  • packages/trpc/server/routers/viewer/bookings/get.handler.ts (7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

**/*.ts: For Prisma queries, only select data you need; never use include, always use select
Ensure the credential.key field is never returned from tRPC endpoints or APIs

Files:

  • packages/trpc/server/routers/viewer/bookings/get.handler.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js .utc() in hot paths like loops

Files:

  • packages/trpc/server/routers/viewer/bookings/get.handler.ts
**/*.{ts,tsx,js,jsx}

⚙️ CodeRabbit configuration file

Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.

Files:

  • packages/trpc/server/routers/viewer/bookings/get.handler.ts
🧠 Learnings (2)
📚 Learning: 2025-08-26T08:08:23.395Z
Learnt from: SinghaAnirban005
PR: calcom/cal.com#23343
File: packages/features/insights/server/trpc-router.ts:1080-1101
Timestamp: 2025-08-26T08:08:23.395Z
Learning: In packages/features/insights/server/trpc-router.ts, when filtering personal event types (userId provided, no teamId, not isAll), the query correctly uses user.id (authenticated user) instead of the input userId parameter for security reasons. This prevents users from accessing other users' personal event types by passing arbitrary user IDs.

Applied to files:

  • packages/trpc/server/routers/viewer/bookings/get.handler.ts
📚 Learning: 2025-09-03T11:54:05.409Z
Learnt from: supalarry
PR: calcom/cal.com#23514
File: apps/api/v2/src/ee/bookings/2024-08-13/services/bookings.service.ts:579-582
Timestamp: 2025-09-03T11:54:05.409Z
Learning: In calcom/cal.com bookings repository methods, when Prisma select uses `eventType: true`, all eventType fields including seatsShowAttendees are automatically included in the selection. Explicit field selection is not required when using `true` for nested relations.

Applied to files:

  • packages/trpc/server/routers/viewer/bookings/get.handler.ts

@github-actions github-actions bot added consumer High priority Created by Linear-GitHub Sync labels Sep 30, 2025
@zhyd1997 zhyd1997 changed the title fix(apps/web): ignore userIds form filter segment if no permission fix(packages/trpc): ignore userIds form filter segment if no permission Sep 30, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
packages/trpc/server/routers/viewer/bookings/get.handler.ts (1)

189-219: Security issue: attendee email filter uses original userIds before permission narrowing.

attendeeEmailsFromUserIdsFilter is computed at line 135 using the original filters.userIds before the permission-narrowing logic (lines 161-171) executes. This means Queries 2 and 3 (lines 189-202, 205-219) filter by attendee emails of ALL users in the original filter, including those the current user doesn't have permission to view.

Attack scenario:

  1. User creates filter segment with userIds = [unauthorizedUser.id, currentUser.id].
  2. Permission logic narrows userIds to [currentUser.id].
  3. Query 1 correctly uses narrowed userIds.
  4. Queries 2 and 3 still use attendeeEmailsFromUserIdsFilter containing unauthorizedUser.email.
  5. User can see bookings where unauthorizedUser is an attendee, bypassing permission checks.

Recompute attendeeEmailsFromUserIdsFilter after the permission-narrowing block:

     let userIds = filters.userIds;

     //  Scope depends on `user.orgId`:
     // - Throw an error if trying to filter by usersIds that are not within your ORG
     // - Throw an error if trying to filter by usersIds that are not within your TEAM
     if (!areUserIdsWithinUserOrgOrTeam) {
       if (!hasCurrentUser) {
         userIds = [];
         log.error({
           code: "FORBIDDEN",
           message: "You do not have permissions to fetch bookings for specified userIds",
         });
+      } else {
+        userIds = [user.id];
       }
-
-      userIds = [user.id];
     }
+
+    // Recompute attendee emails using narrowed userIds
+    const attendeeEmailsFromNarrowedUserIds = await prisma.user
+      .findMany({
+        where: { id: { in: userIds } },
+        select: { email: true },
+      })
+      .then((users) => users.map((user) => user.email));

Then use attendeeEmailsFromNarrowedUserIds in Queries 2 and 3 instead of attendeeEmailsFromUserIdsFilter.

🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/bookings/get.handler.ts (1)

263-329: LGTM: Explicit guards improve readability and prevent empty queries.

The conversion from inline boolean guards to explicit if-blocks for Queries 4-7 (lines 263-329) improves code clarity and ensures queries are only added to bookingQueries when relevant data exists. This prevents constructing queries with empty filter arrays and aligns with the refactor pattern applied to Query 1.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 65ea4ba and 30948b6.

📒 Files selected for processing (1)
  • packages/trpc/server/routers/viewer/bookings/get.handler.ts (6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

**/*.ts: For Prisma queries, only select data you need; never use include, always use select
Ensure the credential.key field is never returned from tRPC endpoints or APIs

Files:

  • packages/trpc/server/routers/viewer/bookings/get.handler.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js .utc() in hot paths like loops

Files:

  • packages/trpc/server/routers/viewer/bookings/get.handler.ts
**/*.{ts,tsx,js,jsx}

⚙️ CodeRabbit configuration file

Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.

Files:

  • packages/trpc/server/routers/viewer/bookings/get.handler.ts
🧠 Learnings (2)
📚 Learning: 2025-08-26T08:08:23.395Z
Learnt from: SinghaAnirban005
PR: calcom/cal.com#23343
File: packages/features/insights/server/trpc-router.ts:1080-1101
Timestamp: 2025-08-26T08:08:23.395Z
Learning: In packages/features/insights/server/trpc-router.ts, when filtering personal event types (userId provided, no teamId, not isAll), the query correctly uses user.id (authenticated user) instead of the input userId parameter for security reasons. This prevents users from accessing other users' personal event types by passing arbitrary user IDs.

Applied to files:

  • packages/trpc/server/routers/viewer/bookings/get.handler.ts
📚 Learning: 2025-09-03T11:54:05.409Z
Learnt from: supalarry
PR: calcom/cal.com#23514
File: apps/api/v2/src/ee/bookings/2024-08-13/services/bookings.service.ts:579-582
Timestamp: 2025-09-03T11:54:05.409Z
Learning: In calcom/cal.com bookings repository methods, when Prisma select uses `eventType: true`, all eventType fields including seatsShowAttendees are automatically included in the selection. Explicit field selection is not required when using `true` for nested relations.

Applied to files:

  • packages/trpc/server/routers/viewer/bookings/get.handler.ts
🔇 Additional comments (1)
packages/trpc/server/routers/viewer/bookings/get.handler.ts (1)

174-186: LGTM: Query 1 now correctly guards on narrowed userIds length.

The explicit if-block ensures Query 1 only executes when userIds is non-empty after permission narrowing. This prevents querying with an empty userIds array and aligns with the PR objective of ignoring unauthorized userIds.

Note: This depends on fixing the logic error in lines 161-171 to ensure userIds is correctly set to empty when the current user is not included in an unauthorized filter.

Copy link
Contributor

@kart1ka kart1ka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhyd1997 could you please attach a loom video showing this fixes the issue?

@zhyd1997 zhyd1997 marked this pull request as draft September 30, 2025 13:18
@zhyd1997
Copy link
Contributor Author

Hi @kart1ka

How to verify the patch for the team plan users?

@zhyd1997 zhyd1997 marked this pull request as ready for review September 30, 2025 13:51
@zhyd1997 zhyd1997 requested a review from kart1ka September 30, 2025 13:51
@dosubot dosubot bot added the platform Anything related to our platform plan label Sep 30, 2025
@graphite-app graphite-app bot requested a review from a team September 30, 2025 13:53
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 30948b6 and a2df820.

📒 Files selected for processing (1)
  • packages/trpc/server/routers/viewer/bookings/get.handler.ts (7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

**/*.ts: For Prisma queries, only select data you need; never use include, always use select
Ensure the credential.key field is never returned from tRPC endpoints or APIs

Files:

  • packages/trpc/server/routers/viewer/bookings/get.handler.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js .utc() in hot paths like loops

Files:

  • packages/trpc/server/routers/viewer/bookings/get.handler.ts
**/*.{ts,tsx,js,jsx}

⚙️ CodeRabbit configuration file

Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.

Files:

  • packages/trpc/server/routers/viewer/bookings/get.handler.ts
🧠 Learnings (2)
📚 Learning: 2025-08-26T08:08:23.395Z
Learnt from: SinghaAnirban005
PR: calcom/cal.com#23343
File: packages/features/insights/server/trpc-router.ts:1080-1101
Timestamp: 2025-08-26T08:08:23.395Z
Learning: In packages/features/insights/server/trpc-router.ts, when filtering personal event types (userId provided, no teamId, not isAll), the query correctly uses user.id (authenticated user) instead of the input userId parameter for security reasons. This prevents users from accessing other users' personal event types by passing arbitrary user IDs.

Applied to files:

  • packages/trpc/server/routers/viewer/bookings/get.handler.ts
📚 Learning: 2025-09-03T11:54:05.409Z
Learnt from: supalarry
PR: calcom/cal.com#23514
File: apps/api/v2/src/ee/bookings/2024-08-13/services/bookings.service.ts:579-582
Timestamp: 2025-09-03T11:54:05.409Z
Learning: In calcom/cal.com bookings repository methods, when Prisma select uses `eventType: true`, all eventType fields including seatsShowAttendees are automatically included in the selection. Explicit field selection is not required when using `true` for nested relations.

Applied to files:

  • packages/trpc/server/routers/viewer/bookings/get.handler.ts
🔇 Additional comments (3)
packages/trpc/server/routers/viewer/bookings/get.handler.ts (3)

188-194: LGTM: Correctly derives attendee emails from filtered userIds.

The refactoring properly uses getFilteredUserIds to compute permitted userIds and derives attendeeEmailsFromUserIdsFilter from the result instead of the raw filters.userIds. This ensures only authorized users' emails are included in the query scope.


197-209: LGTM: Guards prevent empty queries and align with filtering logic.

The explicit guards around booking query pushes ensure:

  • Queries are only added when data exists (e.g., userIds.length > 0).
  • Empty in clauses are avoided, improving query efficiency.
  • Consistent pattern across multiple branches (lines 197, 286, 303, 322, 340).

This approach aligns well with the PR objective to silently ignore unauthorized userIds.

Also applies to: 286-299, 303-317, 322-335, 340-352


63-73: PR objectives met: Unauthorized userIds are now ignored instead of throwing.

The refactored logic successfully implements the PR objective:

  • Before: Threw FORBIDDEN error when filter included unauthorized userIds.
  • After: Logs FORBIDDEN (line 66-69) but silently filters to either [] (no current user) or [user.id] (current user included), preventing the error state.

This makes the bookings listing resilient to filter segments referencing users the viewer no longer has permission to see.

@anikdhabal anikdhabal changed the title fix(packages/trpc): ignore userIds form filter segment if no permission fix: ignore userIds form filter segment if no permission Oct 1, 2025
@github-actions
Copy link
Contributor

This PR is being marked as stale due to inactivity.

@github-actions github-actions bot added the Stale label Oct 16, 2025
@zhyd1997 zhyd1997 closed this Oct 16, 2025
@zhyd1997 zhyd1997 deleted the fix/issue-24172-1759237004 branch October 16, 2025 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bookings area: bookings, availability, timezones, double booking 🐛 bug Something isn't working community Created by Linear-GitHub Sync consumer High priority Created by Linear-GitHub Sync platform Anything related to our platform plan size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ignore userIds form filter segment if no permission

3 participants