Cal.com PR #29 — Live Benchmark

Golden Comments (5/5 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	Case sensitivity bypass in email blacklist	HIGH	✓ Found	"Email blacklist and existing-attendee checks are case-sensitive: `blacklistedGuestEmails` is lowercased but the submitted `guest` is compared raw, allowing the blacklist to be bypassed by case changes; case-variant duplicates of existing attendees also create duplicate attendee rows. Normalize emails to lowercase before comparison and before persisting" (addGuests.handler.ts L75)
#2	The logic for checking team admin/owner permissions is incorrect. This condition uses AND (`&&`) which requires both `isTeamAdmin` AND `isTeamOwner` to be true, but it should use OR (`\|\|`) since a user needs to be either an admin OR an owner to have permission	HIGH	✓ Found	"Team admin/owner permission check uses `&&` instead of `\|\|`, effectively locking out team ADMINs (only OWNERs pass). `isTeamAdmin` already returns truthy for ADMIN or OWNER, so chaining with `&&` degrades to `isTeamOwner`. This diverges from the project convention (`role === OWNER \|\| role === ADMIN`) used in `confirm.handler.ts` and `get.handler.ts`; use `\|\|`" (addGuests.handler.ts L47)
#3	This calls the email sender with the original guests, so existing attendees included in the input will be treated as new when sending notifications, leading to incorrect emails	MEDIUM	✓ Found	"The attendee discriminator in `sendAddGuestsEmails` matches `newGuests.includes(attendee.email)` against raw, un-normalized input: the handler passes the unfiltered `guests` (existing attendees are filtered before persisting but not before this call), so re-typing the email of someone already on the booking makes that existing attendee receive a full `AttendeeScheduledEmail` confirmation instead of the 'new guests added' `AttendeeAddGuestsEmail`; case-variant emails also get the wrong template" (email-manager.ts L540)
#4	`uniqueGuests` filters out existing attendees and blacklisted emails but does not deduplicate duplicates within the input; `createMany` can insert duplicate attendee rows if the client submits repeated emails	MEDIUM	✓ Found	"No de-duplication of duplicates within the submitted `guests` array at the API boundary: the Zod schema is just `z.array(z.string().email())` with no `.refine` uniqueness (only the frontend enforces it). A direct API caller can send `["a@b.com", "a@b.com"]` and `createMany` inserts identical attendees" (addGuests.handler.ts L73)
#5	Starting with an array containing an empty string may cause validation issues. Consider starting with an empty array `[]` and handling the empty state in the `MultiEmail` component instead	LOW	✓ Found	"`multiEmailValue` is initialized to `['']`, so the `if (multiEmailValue.length === 0) return;` guard only fires after the user manually removes every field. Clicking 'Add' without typing falls through to `ZAddGuestsInputSchema.safeParse`, which rejects `['']` and shows a misleading 'Emails must be unique and valid' error" (AddGuestsDialog.tsx L41)

Supplementary Findings (12 findings)

Finding	File	Severity	Legitimate?
Attendees are persisted to the DB (`prisma.booking.update`/`createMany`) before the calendar sync, which is not wrapped in try/catch (unlike the email step): if the sync throws, the mutation rejects while the DB is already mutated, and a retry then hits the `emails_must_be_unique_valid` BAD_REQUEST because those emails are now existing attendees	addGuests.handler.ts:157	HIGH	✓ Real — partial-success, non-idempotent state on sync failure; arguably the PR's most damaging defect
Calendar sync uses the caller's credentials (`getUsersCredentials(ctx.user)`) rather than the booking organizer's: since any attendee or a team admin (who may not be the organizer) can call this mutation, `updateCalendarAttendees` can write events to the wrong calendar	addGuests.handler.ts:150	MEDIUM	✓ Real — non-organizer callers can trigger calendar writes with their own credentials
The translation key `you_do_not_have_permission` is referenced by `addGuests.handler.ts` (line 55) in a TRPCError message rendered via `t(err.message)`, but no such key exists in `en/common.json` nor the repo's locale files — an unauthorized caller sees the raw literal instead of a localized message	common.json:1136	MEDIUM	✓ Real — raw key rendered to users; sibling keys (`booking_not_found`, `emails_must_be_unique_valid`) already exist
`prisma.user.findFirstOrThrow({ where: { id: booking.userId \|\| 0 } })` throws an uncaught Prisma P2025 (500) for bookings without a `userId` (e.g. collective/managed bookings or a removed organizer account) instead of a handled TRPCError	addGuests.handler.ts:60	MEDIUM	✓ Real — 500 on a legit booking shape; guard with a TRPCError is justified
`onSuccess` resets `multiEmailValue` and closes the dialog but never resets `isInvalidEmail`: after an invalid submission, fixing and resubmitting, then reopening 'Add guests', the stale 'Emails must be unique and valid' error renders immediately over a clean input	AddGuestsDialog.tsx:31	MEDIUM	✓ Real — stale error state leaks across dialog reopenings
The `from` of the organizer-add-guests email uses `APP_NAME` instead of `EMAIL_FROM_NAME`, unlike every other organizer template: an instance customizing `EMAIL_FROM_NAME` is silently ignored by this new email	organizer-add-guests-email.ts:23	LOW	✓ Real — breaks the org-wide from-name convention
const message = `${err.data?.code}: ${t(err.message)}`; is always truthy (even `'undefined: <key>'`), so the `message \|\| t('unable_to_add_guests')` fallback never fires; non-TRPC/network errors render `undefined: ...`	AddGuestsDialog.tsx:36	LOW	✓ Real — dead fallback with a raw `undefined:` prefix on network errors
`onOpenChange={setIsOpenDialog}` only flips the flag, so closing via the overlay, Escape or the dialog's close button leaves `multiEmailValue` and `isInvalidEmail` untouched — the next open shows previously typed emails and/or a stale validation error	AddGuestsDialog.tsx:25	LOW	✓ Real — only the explicit Cancel button resets state
Reassigning the destructured `value` parameter (`value = value \|\| []`) is a no-param-reassign anti-pattern and masks callers passing `undefined`	MultiEmail.tsx:19	LOW	✓ Borderline: style nit; `value ?? []` is cleaner but behavior is identical
The remove `<button>` inside `addOnSuffix` has no `onClick`; removal only works because the `Addon` wrapper attaches `onClickAddon` to the whole addon — functional but brittle	MultiEmail.tsx:45	LOW	✓ Borderline: works by accident of the wrapper; fragile to refactors
`<label htmlFor="emails">` doesn't associate with any input (each `EmailField` generates its own `useId` id) and repeats for every row	MultiEmail.tsx:27	LOW	✓ Borderline: accessibility nit, non-functional
Array index used as React key — acceptable here because each `EmailField` is fully controlled from `value`, but row removal re-indexes and any internal state would be lost	MultiEmail.tsx:26	LOW	✓ Borderline: safe with the controlled-input design, latent otherwise

Honest Analysis

FriendlyReviewer found all five golden comments of this PR — recall 100%. Golden #2 (Critical in the dataset: isTeamAdmin && isTeamOwner instead of ||) is found as a red item with the correct degradation analysis — isTeamAdmin already returns truthy for ADMIN or OWNER, so the AND chain reduces to isTeamOwner and locks team ADMINS out — and it cites the project convention (role === OWNER || role === ADMIN) used in confirm.handler.ts and get.handler.ts. Golden #1 (case-sensitivity bypass of the blacklist) is found with the exact mechanism: blacklistedGuestEmails is lowercased while the submitted guest is compared raw, so case changes bypass the block and create case-variant duplicates.

A calibration gap deserves attention: golden #1 is a security bypass, yet the review files it under "Worth checking" (yellow) rather than the close-look section, while the review's executive summary asserts "no security risk was identified in the client-side files" — accurate for the frontend, but a reader could easily take it as a blanket statement. Golden #3 (existing attendees treated as new) is found with the correct mechanism (newGuests.includes against raw input at email-manager.ts L540), golden #4 (no intra-input dedup) is found at the Zod-schema level with the ["a@b.com", "a@b.com"] repro, and golden #5 ([''] initial value) is found with the misleading validation error it produces.

The 12 supplementary findings are the real value of this review. Its top red item — attendees persisted before the calendar sync without try/catch, leaving partial-success non-idempotent state on sync failure — is arguably the most damaging defect of the PR and is absent from the goldens. The other yellows (caller credentials instead of the organizer's, the missing you_do_not_have_permission translation key, the findFirstOrThrow 500 on bookings without userId, the stale isInvalidEmail state) are all legitimate. The seven blue items, however, are mostly style and accessibility nits (no-param-reassign, an unassociated htmlFor, array-index keys) — real but low-value, and they inflate the "2 high, 8 medium, 7 low" tally. Overall: perfect recall, strong extras on the backend, a severity-calibration question on golden #1, and a noisy low-severity tail.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech	🌤️ Feat
2 high, 8 medium, 7 low	1 mitigated

This MR adds the ability to add guests to a booking from the app.cal.com/bookings UI. The frontend flow (AddGuestsDialog, MultiEmail) is functionally wired and consistent with existing patterns, the new i18n keys are present and uniquely defined, and the new email templates closely follow the established location-change pattern, including correct iCal invite generation for newly added guests. Overall the feature works on the primary happy path (an owner adding a guest), but the backend handler contains several blocking defects that should be addressed before merge: the team admin/owner permission check uses && instead of || (locking out team ADMINS), attendees are persisted before the calendar sync without try/catch (causing partial-success, non-idempotent retries), email normalization is case-sensitive (enabling blacklist bypass and duplicate attendees), and calendar sync runs with the caller's credentials rather than the organizer's. There is also a missing translation key that will render raw to users. No security risk was identified in the client-side files, and the email/translation scopes are largely consistent, with only minor robustness/consistency nits.

These might need a close look

🔴 packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (L47)
Team admin/owner permission check uses && instead of ||, effectively locking out team ADMINs (only OWNERs pass). isTeamAdmin already returns truthy for ADMIN or OWNER, so chaining with && degrades to isTeamOwner. This diverges from the project convention (role === OWNER || role === ADMIN) used in confirm.handler.ts and get.handler.ts. Use ||.
🔴 packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (L157)
Attendees are persisted to the DB (prisma.booking.update/createMany) before the calendar sync, which is not wrapped in try/catch (unlike the email step). If the calendar/event-management sync throws, the mutation rejects while the DB is already mutated; a retry then hits the emails_must_be_unique_valid BAD_REQUEST because those emails are now existing attendees, leaving the user with a partial success they cannot complete. Wrap the sync in try/catch or reorder so failures don't leave inconsistent state, and make duplicate detection idempotent.

Worth checking

🟡 packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (L75)
Email blacklist and existing-attendee checks are case-sensitive: blacklistedGuestEmails is lowercased but the submitted guest is compared raw, allowing the blacklist to be bypassed by case changes; case-variant duplicates of existing attendees also create duplicate attendee rows. Normalize emails to lowercase before comparison and before persisting.
🟡 packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (L73)
No de-duplication of duplicates within the submitted guests array at the API boundary — the Zod schema is just z.array(z.string().email()) with no .refine uniqueness (only the frontend enforces it). A direct API caller can send ["a@b.com", "a@b.com"] and createMany inserts identical attendees. Enforce uniqueness in the backend schema or de-dupe in the handler.
🟡 packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (L150)
Calendar sync uses the caller's credentials (getUsersCredentials(ctx.user)) rather than the booking organizer's. Since the permission model lets any attendee or a team admin (who may not be the organizer) call this mutation, updateCalendarAttendees can write events to the wrong calendar, and non-organizers can trigger calendar writes/emails to all attendees. Use the organizer's credentials (e.g. booking.user.credentials) and reconsider whether non-organizer attendees should trigger these side effects.
🟡 apps/web/public/static/locales/en/common.json (L1136)
The translation key you_do_not_have_permission is referenced by addGuests.handler.ts (line 55) in a TRPCError message that the dialog renders via t(err.message), but no such key exists in en/common.json (nor the repo's locale files). An unauthorized caller sees the raw literal instead of a localized message. Sibling keys booking_not_found and emails_must_be_unique_valid already exist; add this one.
🟡 packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (L60)
prisma.user.findFirstOrThrow({ where: { id: booking.userId || 0 } }) throws an uncaught Prisma P2025 (500) for bookings without a userId (e.g. collective/managed bookings or an organizer whose account was removed) instead of a handled TRPCError. Guard this case (e.g. if (!booking.userId) throw new TRPCError(...)).
🟡 packages/emails/email-manager.ts (L540)
The attendee discriminator in sendAddGuestsEmails matches newGuests.includes(attendee.email) against raw, un-normalized input. Because the handler passes the unfiltered guests (existing attendees are filtered before persisting but not before this call), re-typing the email of someone already on the booking makes that existing attendee receive a full AttendeeScheduledEmail confirmation instead of the 'new guests added' AttendeeAddGuestsEmail; case-variant emails also get the wrong template. Normalize both sides (.toLowerCase()) and/or pass the actually-persisted new-guest list.
🟡 apps/web/components/dialog/AddGuestsDialog.tsx (L31)
onSuccess resets multiEmailValue and closes the dialog but never resets isInvalidEmail. After (1) submitting an invalid email → error shown, (2) fixing and resubmitting → success and close, (3) reopening 'Add guests', the stale 'Emails must be unique and valid' error renders immediately over a clean/empty input. Reset isInvalidEmail in onSuccess (and ideally when the dialog opens).
🟡 apps/web/components/dialog/AddGuestsDialog.tsx (L41)
multiEmailValue is initialized to [''], so the if (multiEmailValue.length === 0) return; guard only fires after the user manually removes every field. Clicking 'Add' without typing falls through to ZAddGuestsInputSchema.safeParse, which rejects [''] and shows a misleading 'Emails must be unique and valid' error. Trim/filter empty entries before validating (and decide whether an entirely-empty input should no-op instead of error).

Small things (take or leave)

🔵 packages/emails/templates/organizer-add-guests-email.ts (L23)
from uses APP_NAME instead of EMAIL_FROM_NAME, unlike every other organizer template (organizer-scheduled-email.ts, organizer-location-change-email.ts, etc.). When an instance customizes EMAIL_FROM_NAME, this new email ignores it and uses the raw APP_NAME, breaking the org-wide from-name convention. Import and use EMAIL_FROM_NAME from @calcom/lib/constants.
🔵 apps/web/components/dialog/AddGuestsDialog.tsx (L36)
const message = `${err.data?.code}: ${t(err.message)}`; is always truthy (even 'undefined: <key>'), so the message || t('unable_to_add_guests') fallback never fires. Non-TRPC/network errors render undefined: ..., and TRPC errors expose the machine code prefix (e.g. BAD_REQUEST:). Derive the message from err.data?.message ?? err.message and only prepend the code conditionally so the fallback works.
🔵 apps/web/components/dialog/AddGuestsDialog.tsx (L25)
onOpenChange={setIsOpenDialog} only flips the flag, so closing via the overlay, Escape, or the dialog's close button leaves multiEmailValue and isInvalidEmail untouched — the next open shows previously typed emails and/or a stale validation error. Only the explicit Cancel button resets state. Reset form state when the dialog transitions to closed (or key the inner content off isOpenDialog).
🔵 packages/ui/form/MultiEmail.tsx (L19)
Reassigning the destructured value parameter (value = value || []) is an anti-pattern (no-param-reassign) and masks callers passing undefined. Prefer const normalizedValue = value ?? [] and use it throughout.
🔵 packages/ui/form/MultiEmail.tsx (L45)
The remove <button> inside addOnSuffix has no onClick; removal only works because the Addon wrapper in InputField attaches onClickAddon to the whole addon. Functional but brittle/fragmented — give the button its own onClick that calls the removal logic (keeping onClickAddon off the field) or drop the inner button and rely on the wrapper.
🔵 packages/ui/form/MultiEmail.tsx (L27)
<label htmlFor="emails"> doesn't associate with any input (each EmailField generates its own useId id) and repeats for every row. Accessibility nit; consider omitting the standalone label or wiring htmlFor to a real input id.
🔵 packages/ui/form/MultiEmail.tsx (L26)
Array index used as React key. Acceptable here because each EmailField is fully controlled from value, but when a row is removed the inputs re-index and any internal state would be lost. Low priority; a stable key isn't necessary given the controlled-input design.

✅ Feature-level checklist

Partially covered

⚠️ Ability to add guests to a booking via app.cal.com/bookings (fixes #14691, #15663).
The guest-add flow is functional on the primary happy path (owner adds a guest): frontend wiring, i18n keys, email templates, and iCal invites for new guests are in place and consistent with existing patterns. However, the requirement is only partially met due to caveats: the team admin/owner permission check excludes team ADMINs (only OWNERs pass), attendees are persisted before the calendar sync leading to partial-success/non-idempotent retries, email blacklist/duplicate checks are case-sensitive (security bypass possible), and calendar sync uses the caller's credentials rather than the organizer's. These should be fixed before the feature is considered fully compliant.

Add guest management functionality to existing bookings