Skip to content

Add guest management functionality to existing bookings#2

Open
zaibkhan wants to merge 1 commit into
guest-management-basefrom
guest-management-enhanced
Open

Add guest management functionality to existing bookings#2
zaibkhan wants to merge 1 commit into
guest-management-basefrom
guest-management-enhanced

Conversation

@zaibkhan

Copy link
Copy Markdown

No description provided.

* feat: ability to add guests via app.cal.com/bookings

* fix: some update

* fix: minor issue

* fix: final update

* update

* update

* add requested changes

* fix type error

* small update

* final update

* fix type error

* fix location

* update calender event

---------

Co-authored-by: Somay Chauhan <somaychauhan98@gmail.com>
@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 10, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Tighten guests schema validation, prevent duplicates
What’s good: Good modularization: added lazy-loaded MultiEmail and i18n keys; email template exports are clean and consistent.
Review Status: ❌ Requires changes
Overall Priority: High
Review Update:
• Coverage: Reviewed all 15 files across 2 batches

Issues (Critical & High only)

Severity Issue Why it matters
High Correctness — Admin/owner authorization uses && instead of || …/bookings/addGuests.handler.ts
Authorization combines admin and owner with &&, which requires a user to be both; this likely blocks legitimate admin/owner access. Use logical OR so either role suffices, matching the intent implied by the variable name.
High Correctness — Uses caller credentials instead of organizer for calendar... …/bookings/addGuests.handler.ts
Calendar updates should run under the organizer’s account; using the caller’s credentials (attendee/admin) can fail or update the wrong calendar. Use the booking organizer’s user object.
High Correctness — Mailer uses unfiltered guest list (blacklist/duplicates l... …/bookings/addGuests.handler.ts
You’re emailing the original input, which may include blacklisted, existing, or duplicate addresses. Send only the filtered set, and consider normalizing to lowercase/trim when filtering to avoid case-based blacklist bypass.

Showing top 3 issues. Critical: 0, High: 3. See inline suggestions for more.

Key Feedback (click to expand)
  • Needs improvement: Strengthen server-side schema: ensure bookingId is a positive integer, guests array is non-empty, emails are normalized (trim/lowercased), and duplicates are rejected.
  • Testing: Add schema tests for: (1) duplicate emails (e.g., ['a@x.com','A@x.com ']), (2) empty guests array, (3) invalid bookingId (0, -1, 1.5), and (4) mixed whitespace in emails to confirm normalization.
  • Documentation: Document that guest emails are trimmed, lowercased, and deduplicated on the server, and clarify any hard limits (max guests per request) if applicable.
  • Compatibility: Ensure other consumers using addGuests respect the new validation (positive int bookingId, non-empty unique guests). Frontend should surface the 'emails_must_be_unique_valid' message.
  • Performance: No significant concerns; the Set-based uniqueness check is O(n) and appropriate for typical guest list sizes.
  • Security: No direct security issues surfaced in the provided diff; recommend adding a sane cap on guest list length server-side to avoid abuse.
  • Open questions: Should bookingId strictly be an integer? Do we have a maximum allowed number of guests to guard against abuse (e.g., 100+)?

Confidence: 2/5 — Not ready to merge (3 high · status: Requires changes)

React with 👍 or 👎 if you found this review useful.


if (!booking) throw new TRPCError({ code: "NOT_FOUND", message: "booking_not_found" });

const isTeamAdminOrOwner =

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ High: Authorization combines admin and owner with &&, which requires a user to be both; this likely blocks legitimate admin/owner access. Use logical OR so either role suffices, matching the intent implied by the variable name.

Suggested change
const isTeamAdminOrOwner =
```suggestion
const isTeamAdminOrOwner =
(await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) ||
(await isTeamOwner(user.id, booking.eventType?.teamId ?? 0));

};
}

const credentials = await getUsersCredentials(ctx.user);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ High: Calendar updates should run under the organizer’s account; using the caller’s credentials (attendee/admin) can fail or update the wrong calendar. Use the booking organizer’s user object.

Suggested change
const credentials = await getUsersCredentials(ctx.user);
```suggestion
const credentials = await getUsersCredentials(booking.user);
const eventManager = new EventManager({
...booking.user,
credentials: [...credentials],
});

await eventManager.updateCalendarAttendees(evt, booking);

try {
await sendAddGuestsEmails(evt, guests);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ High: You’re emailing the original input, which may include blacklisted, existing, or duplicate addresses. Send only the filtered set, and consider normalizing to lowercase/trim when filtering to avoid case-based blacklist bypass.

Suggested change
await sendAddGuestsEmails(evt, guests);
```suggestion
await sendAddGuestsEmails(evt, uniqueGuests);

@@ -0,0 +1,8 @@
import { z } from "zod";

export const ZAddGuestsInputSchema = z.object({

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔷 Medium: The schema allows any number (including non-integers) for bookingId and accepts an empty guests array and duplicate emails with differing cases/whitespace. This can cause duplicate invitations and ambiguous server behavior. Normalize, enforce uniqueness, require at least one guest, and constrain bookingId to a positive integer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants