feat: prevent-impersonated-booking #24372
Conversation
|
@dhairyashiil is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
consumer
High priority
github-actions
bot
added
❗️ migrations
keithwillcode
added
the
community-interns
WalkthroughAdds a new per-user boolean setting Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
dosubot
bot
added
bookings
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 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.
📒 Files selected for processing (10)
apps/web/modules/settings/my-account/general-view.tsx(2 hunks)apps/web/public/static/locales/en/common.json(1 hunks)packages/features/bookings/lib/handleNewBooking.ts(4 hunks)packages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.ts(1 hunks)packages/lib/server/repository/user.ts(3 hunks)packages/prisma/migrations/20251008191459_add_prevent_impersonated_booking_in_user/migration.sql(1 hunks)packages/prisma/schema.prisma(1 hunks)packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts(1 hunks)packages/trpc/server/routers/viewer/me/get.handler.ts(1 hunks)packages/trpc/server/routers/viewer/me/updateProfile.schema.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/trpc/server/routers/viewer/me/get.handler.tspackages/trpc/server/routers/viewer/me/updateProfile.schema.tspackages/lib/server/repository/user.tspackages/trpc/server/routers/viewer/bookings/addGuests.handler.tspackages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.tspackages/features/bookings/lib/handleNewBooking.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/me/get.handler.tspackages/trpc/server/routers/viewer/me/updateProfile.schema.tspackages/lib/server/repository/user.tspackages/trpc/server/routers/viewer/bookings/addGuests.handler.tsapps/web/modules/settings/my-account/general-view.tsxpackages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.tspackages/features/bookings/lib/handleNewBooking.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/me/get.handler.tspackages/trpc/server/routers/viewer/me/updateProfile.schema.tspackages/lib/server/repository/user.tspackages/trpc/server/routers/viewer/bookings/addGuests.handler.tsapps/web/modules/settings/my-account/general-view.tsxpackages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.tspackages/features/bookings/lib/handleNewBooking.ts
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/modules/settings/my-account/general-view.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-03T09:52:51.182Z
Learnt from: hariombalhara
PR: calcom/cal.com#23541
File: packages/features/bookings/lib/di/modules/RegularBookingServiceModule.ts:22-28
Timestamp: 2025-09-03T09:52:51.182Z
Learning: The IBookingServiceDependencies interface in packages/features/bookings/lib/handleNewBooking.ts contains 6 properties: cacheService, checkBookingAndDurationLimitsService, prismaClient, bookingRepository, featuresRepository, and checkBookingLimitsService. This interface is used by RegularBookingService and matches the depsMap structure in RegularBookingServiceModule.
Applied to files:
packages/features/bookings/lib/handleNewBooking.ts
🧬 Code graph analysis (3)
apps/web/modules/settings/my-account/general-view.tsx (1)
packages/ui/components/form/switch/SettingsToggle.tsx (1)
SettingsToggle(29-125)
packages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.ts (1)
scripts/prepare-local-for-delegation-credentials-testing.js (1)
user(15-17)
packages/features/bookings/lib/handleNewBooking.ts (1)
packages/features/bookings/lib/handleNewBooking/types.ts (1)
Invitee(14-25)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (7)
packages/prisma/migrations/20251008191459_add_prevent_impersonated_booking_in_user/migration.sql (1)
2-2: LGTM: adds non-null boolean with safe default.Looks correct and migration-safe for Postgres.
packages/prisma/schema.prisma (1)
407-408: User.preventEmailImpersonation added — OK.Schema change aligns with feature; default(false) is appropriate.
packages/trpc/server/routers/viewer/me/updateProfile.schema.ts (1)
24-24: Update profile schema extended — OK.Optional boolean matches the UI toggle pattern.
packages/trpc/server/routers/viewer/me/get.handler.ts (1)
143-143: preventEmailImpersonation is already selected in the user repository
Default select inpackages/lib/server/repository/user.ts(line 914) includes this field—never undefined.apps/web/modules/settings/my-account/general-view.tsx (1)
150-152: State initialization looks correctBootstrapping from user.preventEmailImpersonation is consistent with other toggles.
packages/features/bookings/lib/handleNewBooking.ts (2)
27-29: DI container imports: OKImports align with usage of getCacheService/getLuckyUserService/getCheckBookingAndDurationLimitsService.
1532-1533: Non-functional parentheses change: OKNo behavior change; safe.
packages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/modules/settings/my-account/general-view.tsx (1)
360-372: Consider adding error handling to revert state on mutation failure.The toggle implementation looks good and addresses the data-testid concern from the previous review. However, the local state is updated optimistically without reverting if the mutation fails, which can lead to UI/server state mismatch.
Optional: Add an
onErrorcallback to the mutation to revert the local state:const mutation = trpc.viewer.me.updateProfile.useMutation({ onSuccess: async (res) => { await utils.viewer.me.invalidate(); revalidateSettingsGeneral(); revalidateTravelSchedules(); reset(getValues()); showToast(t("settings_updated_successfully"), "success"); await update(res); if (res.locale) { window.calNewLocale = res.locale; document.cookie = `calNewLocale=${res.locale}; path=/`; } }, onError: () => { showToast(t("error_updating_settings"), "error"); },Then update the toggle's
onCheckedChange:onCheckedChange={(checked) => { + const previousValue = isPreventEmailImpersonationChecked; setIsPreventEmailImpersonationChecked(checked); - mutation.mutate({ preventEmailImpersonation: checked }); + mutation.mutate( + { preventEmailImpersonation: checked }, + { + onError: () => { + setIsPreventEmailImpersonationChecked(previousValue); + }, + } + ); }}Note: This pattern should ideally be applied to the other toggles in this file as well for consistency.
packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (1)
83-135: Optional: Precompute normalized guest emails to reduce repeated work.The
extractBaseEmail(g).toLowerCase()call is repeated multiple times: twice in the Prisma query (lines 89, 100) and once per guest in the filter (lines 133-134). While not a functional issue, precomputing could improve performance, especially with large guest lists.Consider this refactor:
+ const normalizedGuests = guests.map((g) => extractBaseEmail(g).toLowerCase()); + // Get emails that have preventEmailImpersonation enabled const usersWithPreventImpersonation = await prisma.user.findMany({ where: { OR: [ { email: { - in: guests.map((g) => extractBaseEmail(g).toLowerCase()), + in: normalizedGuests, }, emailVerified: { not: null, }, preventEmailImpersonation: true, }, { secondaryEmails: { some: { email: { - in: guests.map((g) => extractBaseEmail(g).toLowerCase()), + in: normalizedGuests, }, emailVerified: { not: null, }, }, }, preventEmailImpersonation: true, }, ], }, select: { email: true, secondaryEmails: { select: { email: true, emailVerified: true, }, }, }, }); const protectedEmails = new Set<string>(); usersWithPreventImpersonation.forEach((user) => { protectedEmails.add(user.email.toLowerCase()); user.secondaryEmails.forEach((se) => { if (se.emailVerified) protectedEmails.add(se.email.toLowerCase()); }); }); const uniqueGuests = guests.filter( (guest, index, self) => self.indexOf(guest) === index && - !blacklistedGuestEmails.includes(extractBaseEmail(guest).toLowerCase()) && - !protectedEmails.has(extractBaseEmail(guest).toLowerCase()) + !blacklistedGuestEmails.includes(normalizedGuests[index]) && + !protectedEmails.has(normalizedGuests[index]) );
📜 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.
📒 Files selected for processing (4)
apps/web/modules/settings/my-account/general-view.tsx(2 hunks)packages/features/bookings/lib/handleNewBooking.ts(4 hunks)packages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.ts(1 hunks)packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.ts
- packages/features/bookings/lib/handleNewBooking.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/modules/settings/my-account/general-view.tsx
**/*.{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:
apps/web/modules/settings/my-account/general-view.tsxpackages/trpc/server/routers/viewer/bookings/addGuests.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:
apps/web/modules/settings/my-account/general-view.tsxpackages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
🧬 Code graph analysis (2)
apps/web/modules/settings/my-account/general-view.tsx (1)
packages/ui/components/form/switch/SettingsToggle.tsx (1)
SettingsToggle(29-125)
packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (1)
scripts/prepare-local-for-delegation-credentials-testing.js (1)
user(15-17)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tests / Unit
- GitHub Check: Type check / check-types
- GitHub Check: Linters / lint
🔇 Additional comments (1)
apps/web/modules/settings/my-account/general-view.tsx (1)
150-152: LGTM!State initialization follows the established pattern for other toggles in this component.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/features/bookings/lib/handleNewBooking.ts (1)
1202-1225: Consider redacting PII in removal logs.The guest filtering logic correctly excludes blacklisted and protected emails. However, line 1228 logs the full list of removed guest email addresses, which could expose PII in production logs.
Apply this diff to log counts instead of full emails:
if (guestsRemoved.length > 0) { - log.info("Removed guests from the booking", guestsRemoved); + log.info(`Removed ${guestsRemoved.length} guest(s) from the booking (blacklisted or impersonation-protected)`); }
📜 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.
📒 Files selected for processing (3)
packages/features/bookings/lib/handleNewBooking.ts(5 hunks)packages/features/bookings/lib/handleNewBooking/types.ts(2 hunks)packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts(2 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 useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/trpc/server/routers/viewer/bookings/addGuests.handler.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/bookings/lib/handleNewBooking/types.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/addGuests.handler.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/bookings/lib/handleNewBooking/types.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/addGuests.handler.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/bookings/lib/handleNewBooking/types.ts
🧠 Learnings (1)
📚 Learning: 2025-09-03T09:52:51.182Z
Learnt from: hariombalhara
PR: calcom/cal.com#23541
File: packages/features/bookings/lib/di/modules/RegularBookingServiceModule.ts:22-28
Timestamp: 2025-09-03T09:52:51.182Z
Learning: The IBookingServiceDependencies interface in packages/features/bookings/lib/handleNewBooking.ts contains 6 properties: cacheService, checkBookingAndDurationLimitsService, prismaClient, bookingRepository, featuresRepository, and checkBookingLimitsService. This interface is used by RegularBookingService and matches the depsMap structure in RegularBookingServiceModule.
Applied to files:
packages/features/bookings/lib/handleNewBooking.ts
🧬 Code graph analysis (1)
packages/features/bookings/lib/handleNewBooking.ts (1)
packages/features/bookings/lib/handleNewBooking/types.ts (1)
InviteeItem(14-25)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (5)
packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (2)
83-129: LGTM! Impersonation protection correctly implemented.The query properly fetches users with
preventEmailImpersonationenabled, checking both verified primary and secondary emails. The protection set correctly excludes unverified addresses by explicitly checkinguser.emailVerifiedbefore adding the primary email. This aligns with the feature requirements and addresses previous review concerns.
131-136: Guest filtering correctly excludes protected emails.The deduplication logic consistently applies
extractBaseEmailnormalization and properly checks both blacklisted and protected email sets, preventing impersonation attempts through base-email variants (e.g., Gmail aliases).packages/features/bookings/lib/handleNewBooking/types.ts (1)
14-27: LGTM! Clean type extraction.The
InviteeItemtype clearly defines the attendee structure, and the addition of the optionalphoneNumberfield properly supports phone-based attendee identification. This refactor improves type reusability across the booking flow.packages/features/bookings/lib/handleNewBooking.ts (2)
1145-1196: LGTM! Impersonation protection correctly guards booking creation.The implementation precomputes base emails, guards the query when the guest list is empty (avoiding unnecessary DB hits), and correctly normalizes protected emails using
extractBaseEmail. This prevents impersonation through email variants while ensuring only verified addresses are protected.
1247-1253: LGTM! Attendee list construction includes phone numbers.The mapping correctly includes the
phoneNumberfield fromInviteeItem, ensuring phone-based attendee details flow through the booking system consistently.
|
@dhairyashiil there is also one more PR #24373 opened for the same issue. |
Actually, I raised it first. At that time, there was no PR from Sargam. Maybe he didn’t know either, since none of us mentioned that we were working on it, communication issue. 
|
3 participants
What does this PR do?
Visual Demo (For contributors especially)
Screen.Recording.2025-10-09.at.1.19.14.AM.mov