fix: handle collective multiple host on destinationCalendar
calcom/cal.com#10967
Golden recall: 80.0%
Extra findings: 13
Golden Comments (4/5 found, 1 partial)
| # | Comment | Severity | Status | FriendlyReviewer Detail |
|---|---|---|---|---|
| #1 | Null reference if mainHostDestinationCalendar is undefined when destinationCalendar is null or empty array |
HIGH | ✓ Found | FriendlyReviewer: "Critical null-safety bug: mainHostDestinationCalendar.integration can throw TypeError if destinationCalendar array is empty/null" (EventManager.ts:137). Match perfect. |
| #2 | Redundant optional chaining: mainHostDestinationCalendar?.integration — already checked in the ternary |
LOW | ✗ Missed | FriendlyReviewer did not flag this specific code quality redundancy |
| #3 | Logic error in externalCalendarId: searching for externalId === externalCalendarId looks for a calendar matching itself — always fails |
HIGH | ◈ Partial | FriendlyReviewer found the bug in updateEvent/deleteEvent of Google Calendar (L255): "broken fallback search when externalCalendarId is null/undefined." Explains it differently (null/undefined case) vs the golden's self-matching logic, but flags the same broken path. |
| #4 | Logic inversion in organization creation: slug conditionally set when IS_TEAM_BILLING_ENABLED is true instead of false — inverted from intended logic |
MEDIUM | ✓ Found | FriendlyReviewer: "Critical logic inversion bug: Slug is not persisted when billing is disabled, making organizations unreachable" (create.handler.ts:148). Match perfect, severity upgraded to HIGH. |
| #5 | Interface non-compliance: Calendar implementations (Lark, Office365) still declare createEvent(event) without the new credentialId parameter |
LOW | ✓ Found | FriendlyReviewer: "Interface non-compliance: createEvent missing required credentialId parameter (Lark, Office365, BaseCalDAV)" (L125). Match perfect. FR also discovered BaseCalDAV in addition to the golden's two. |
Supplementary Findings (16 findings)
Additional issues found by FriendlyReviewer that are not part of the golden (reference) set.
| Finding | File | Severity | Legitimate? |
|---|---|---|---|
| Inverted guard condition in buildUIDCalendarEvent — throws when users is populated instead of empty | CalendarEvent/builder.ts:148 | HIGH | ✓ Logic bug |
| Idempotency bug: webhooks PayPal/Stripe without idempotency — duplicate calendar events | paypal-webhook.ts:175 | HIGH | ✓ Idempotency |
| Missing output fields: updateEvent does not return externalId/credentialId — tracking loss | CalendarManager.ts:291 | HIGH | ✓ Missing data |
| Missing cleanup: booking rejected — orphaned calendar events on external calendars | confirm.handler.ts:215 | HIGH | ✓ Data integrity |
| Security: MD5 used for password generation + plaintext password in API response | organizations/create.handler.ts:89 | HIGH | ✓ Vulnerability |
Bug in cron: gte: interval prevents short-interval reminders after a long interval |
cron/bookingReminder.ts:90 | HIGH | ✓ Scheduling bug |
null destinationCalendar causes silent data loss for team member calendars |
handleNewBooking.ts:1065 | HIGH | ✓ Data integrity |
| Missing DB credential fallback for video reference deletion | handleCancelBooking.ts:496 | MEDIUM | ✓ Missing fallback |
| Missing webhook dispatch for location changes | editLocation.handler.ts:125 | MEDIUM | ✓ Missing event |
Unhandled async: calendars.map(async ...) not awaited — fire-and-forget |
deleteCredential.handler.ts:335 | MEDIUM | ✓ Unhandled promise |
| Race condition: slug collision check and creation not in a transaction | organizations/create.handler.ts:76 | MEDIUM | ✓ Race condition |
| Incomplete error handling for Vercel domain creation — only forbidden/domain_taken handled | organizations/create.handler.ts:55 | MEDIUM | ✓ Error handling |
| Builder missing setters for new CalendarEvent fields (schedulingType, seatsPerTimeSlot, etc.) | CalendarEvent/builder.ts:163 | MEDIUM | ✓ Missing fields |
| Over-cleanup risk: destination calendar cleanup matches by appId instead of credential.id | deleteCredential.handler.ts:122 | MEDIUM | ✓ Data integrity |
Variable shadowing: uid of booking reference shadows uid from params |
handleCancelBooking.ts:423 | LOW | ✓ Code quality |
Mutation of cached eventType data: users.push() mutates the original array |
CalendarEvent/builder.ts:78 | LOW | ✓ Code quality |
Honest Analysis
FriendlyReviewer's full review
As posted on the PR — the complete output from FriendlyReviewer.
Hi there 👋
| ☁️ Overview |
|---|
| 10 high, 6 medium, 4 low |
This PR handles collective multiple host scenarios for destinationCalendar. The review uncovered several critical issues spanning null-safety bugs, security vulnerabilities, data integrity problems, and concurrency concerns across 22 modified files.
Must fix
- 🔴 CalendarEvent/builder.ts (L148)
Inverted guard condition in buildUIDCalendarEvent — throws when users array is populated instead of empty. Change to throw when!users?.length. - 🔴 EventManager.ts (L137)
Critical null-safety bug: mainHostDestinationCalendar.integration can throw TypeError if destinationCalendar array is empty/null. Add guard before accessing .integration. - 🔴 googlecalendar/CalendarService.ts (L255)
Logic bug in updateEvent/deleteEvent: broken fallback search when externalCalendarId is null/undefined. Should skip fallback or use a proper search. - 🔴 paypal-webhook.ts (L175)
Idempotency bug: PayPal/Stripe webhook handlers without idempotency keys — duplicate events possible. Add idempotency check before processing. - 🔴 CalendarManager.ts (L291)
Missing output fields: updateEvent does not return externalId/credentialId — downstream tracking breaks. - 🔴 confirm.handler.ts (L215)
Missing cleanup: when a booking is rejected, existing calendar events on external calendars are orphaned. - 🔴 organizations/create.handler.ts (L89)
Security: MD5 used for password generation (crypto.createHash('md5')) — use a stronger hash. Plaintext password also exposed in API response. - 🔴 organizations/create.handler.ts (L148)
Critical logic inversion: slug not persisted when billing is disabled — organizations become unreachable. - 🔴 cron/bookingReminder.ts (L90)
Bug:gte: intervalin Prisma query prevents shorter-interval reminders from firing after a long-interval reminder. - 🔴 handleNewBooking.ts (L1065)
Null destinationCalendar causes silent data loss for team members' calendar assignments.
Should address
- 🟡 handleCancelBooking.ts (L496)
Missing DB credential fallback for video reference deletion — falls back to nothing when credential is missing. - 🟡 editLocation.handler.ts (L125)
Missing webhook dispatch for location changes — attendees not notified via webhook. - 🟡 deleteCredential.handler.ts (L335)
Unhandled async:calendars.map(async ...)not awaited — fire-and-forget behavior, errors swallowed. - 🟡 organizations/create.handler.ts (L76)
Race condition: slug collision check and creation are not in a database transaction. - 🟡 organizations/create.handler.ts (L55)
Incomplete error handling for Vercel domain creation — only forbidden and domain_taken errors handled. - 🟡 CalendarEvent/builder.ts (L163)
Builder missing setters for new CalendarEvent fields (schedulingType, seatsPerTimeSlot, etc.) — patterns inconsistent. - 🟡 deleteCredential.handler.ts (L122)
Over-cleanup risk: destination calendar cleanup matches by appId instead of credential.id — may delete wrong records.
Minor
- 🔵 handleCancelBooking.ts (L423)
Variable shadowing:uidof booking reference shadows uid from function params — confusing but not breaking. - 🔵 CalendarEvent/builder.ts (L78)
Mutation of cached eventType data:users.push()mutates the original cached array — unexpected side effects. - 🔵 larkcalendar/CalendarService.ts (L125)
Interface non-compliance: createEvent missing credentialId parameter in Lark, Office365, BaseCalDAV.
FriendlyReviewer found 3 of 5 golden bugs with one partial match on this 22-file PR. The partial match on Golden #3 (externalCalendarId logic) is worth noting: FR correctly identified a broken code path but attributed it to a null/undefined scenario rather than the self-matching identity comparison the golden describes. The result is the same (the fallback search breaks), but the diagnosis differs.
The supplementary findings are a mixed bag. The MD5 password generation (create.handler.ts) is a genuine security finding that should have been caught by human reviewers. The inverted guard condition in the CalendarEvent builder is also a real logic bug. But several findings are architectural observations rather than actionable bugs — "missing setters for new fields" and "builder doesn't expose field X" describe incomplete refactoring, not defects. These inflate the count without representing actual bug discoveries.
What stood out in this review is the speed: 7m 56s for 22 files with 20 findings. That's 24 seconds per file. The trade-off shows — findings are shallower and occasionally misdiagnosed (partial match #3), but the breadth is undeniable. A human reviewer taking that speed would miss far more.
One clear miss: Golden #2 (redundant optional chaining) is a trivial code quality issue that should have been caught by any linter-aware review. Its absence suggests FR doesn't perform dead-code or redundancy analysis effectively.