fix: handle collective multiple host on destinationCalendar#8
Conversation
🤖 Grapple PR ReviewSummaryThis PR modifies 22 files. Title: "fix: handle collective multiple host on destinationCalendar" Changes: 22 files (+368 / -216 lines)
Resolves: #4 (OAuth credential sync and app integration enhancements) CI: ⏳ 0 passed, 0 failed Impact Diagramgraph TD
pr_0[/"🔀 PR Changes: 22 files"/]
f1["🟡 bookingReminder.ts +2/-2"]
pr_0 --> f1
f2["🟡 webhook.e2e.ts +1/-1"]
pr_0 --> f2
f3["🟡 CalendarService.ts +22/-11"]
pr_0 --> f3
f4["🟡 CalendarService.ts +8/-4"]
pr_0 --> f4
f5["🟡 CalendarService.ts +3/-2"]
pr_0 --> f5
f6["🟡 CalendarManager.ts +24/-19"]
pr_0 --> f6
f7["🟡 EventManager.ts +124/-68"]
pr_0 --> f7
f8["🟡 builder.ts +2/-1"]
pr_0 --> f8
f9["🟡 class.ts +1/-1"]
pr_0 --> f9
f10["🟡 BrokenIntegrationEmail.tsx +3/-2"]
pr_0 --> f10
f11["🟡 handleCancelBooking.ts +65/-44"]
pr_0 --> f11
f12["🟡 handleNewBooking.ts +72/-43"]
pr_0 --> f12
more_13["... +10 more files"]
pr_0 --> more_13
f6 -.- f7
f8 -.- f9
f11 -.- f12
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
Review details
Powered by Grapple PR — AI that understands your code. |
![grapple-pr[bot]](https://avatars.githubusercontent.com/in/3205222?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR fixes handling of collective event types with multiple hosts by ensuring that destination calendars for all hosts are correctly resolved and used when creating, updating, or canceling bookings, rather than only using the first host's calendar.
Changes: 22 files (+368 / -216 lines)
- 🟡 22 files modified:
apps/web/pages/api/cron/bookingReminder.ts,apps/web/playwright/webhook.e2e.ts,packages/app-store/googlecalendar/lib/CalendarService.ts,packages/app-store/larkcalendar/lib/CalendarService.ts,packages/app-store/office365calendar/lib/CalendarService.ts+17 more
Resolves: #4 (OAuth credential sync and app integration enhancements)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 22 files"/]
f1["🟡 bookingReminder.ts +2/-2"]
pr_0 --> f1
f2["🟡 webhook.e2e.ts +1/-1"]
pr_0 --> f2
f3["🟡 CalendarService.ts +22/-11"]
pr_0 --> f3
f4["🟡 CalendarService.ts +8/-4"]
pr_0 --> f4
f5["🟡 CalendarService.ts +3/-2"]
pr_0 --> f5
f6["🟡 CalendarManager.ts +24/-19"]
pr_0 --> f6
f7["🟡 EventManager.ts +124/-68"]
pr_0 --> f7
f8["🟡 builder.ts +2/-1"]
pr_0 --> f8
f9["🟡 class.ts +1/-1"]
pr_0 --> f9
f10["🟡 BrokenIntegrationEmail.tsx +3/-2"]
pr_0 --> f10
f11["🟡 handleCancelBooking.ts +65/-44"]
pr_0 --> f11
f12["🟡 handleNewBooking.ts +72/-43"]
pr_0 --> f12
more_13["... +10 more files"]
pr_0 --> more_13
f6 -.- f7
f8 -.- f9
f11 -.- f12
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
5 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 5 |
| 🟠 Major | 8 |
| 🟡 Minor | 8 |
| 🔵 Info | 2 |
Review details
- Reviewed at: 2026-04-07T18:30:07.036Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 123.4s
- Low-confidence findings suppressed: 3
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🔴 CRITICAL — Authentication/Authorization (confidence: 100%)
When a credentialId is not found in the booking user's credentials, the code fetches the credential directly from the database without verifying that the credential belongs to a host of the booking. An attacker who can manipulate the bookingCalendarReference (e.g., via a compromised booking record) could cause the system to fetch and use an arbitrary credential by ID, potentially allowing deletion of calendar events from accounts that are unrelated to the booking.
Evidence:
- The fallback path fetches
prisma.credential.findUnique({ where: { id: credentialId } })with no ownership check - No verification that the fetched credential belongs to any of the booking's hosts or the organizing user
- The credential is then passed directly to
getCalendar(calendarCredential)and used to delete calendar events - Original code only looked in
bookingToDelete.user.credentialswhich implicitly scoped to the owner; the new DB fallback removes that scope
Agent: security
| // Fallback to Cal Video if Google Meet is selected w/o a Google Cal | ||
| if (evt.location === MeetLocationType && evt.destinationCalendar?.integration !== "google_calendar") { | ||
| // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one | ||
| const [mainHostDestinationCalendar] = evt.destinationCalendar ?? []; |
There was a problem hiding this comment.
🔴 CRITICAL — Null Reference / Runtime Error (confidence: 100%)
Destructuring evt.destinationCalendar ?? [] can yield an empty array, making mainHostDestinationCalendar undefined. The very next line accesses mainHostDestinationCalendar.integration without a null check, which will throw a TypeError at runtime when destinationCalendar is null, undefined, or an empty array.
Evidence:
- Line 116:
const [mainHostDestinationCalendar] = evt.destinationCalendar ?? []; - Line 118:
if (evt.location === MeetLocationType && mainHostDestinationCalendar.integration !== "google_calendar")— no null/undefined guard - CalendarEvent type defines
destinationCalendar?: DestinationCalendar[] | null, so null/empty is a valid state - This code path is hit on every booking creation through EventManager.create(), meaning any booking with no destination calendar will crash
Agent: architecture
|
|
||
| export interface Calendar { | ||
| createEvent(event: CalendarEvent): Promise<NewCalendarEventType>; | ||
| createEvent(event: CalendarEvent, credentialId: number): Promise<NewCalendarEventType>; |
There was a problem hiding this comment.
🔴 CRITICAL — API Contract Violation (confidence: 100%)
The Calendar.createEvent interface signature changed to require a credentialId: number parameter, but LarkCalendarService and Office365CalendarService (and the base CalendarService) do not accept this parameter. Any calendar integration not updated to match this signature will either ignore the parameter silently (JavaScript) or fail type checks (TypeScript). Furthermore, third-party calendar app-store integrations implementing this interface will break.
Evidence:
- packages/types/Calendar.d.ts line 221:
createEvent(event: CalendarEvent, credentialId: number): Promise; - packages/app-store/larkcalendar/lib/CalendarService.ts line 125:
async createEvent(event: CalendarEvent)— no credentialId param - packages/app-store/office365calendar/lib/CalendarService.ts line 72:
async createEvent(event: CalendarEvent)— no credentialId param - packages/lib/CalendarService.ts (BaseCalendarService) does not accept credentialId in createEvent
- Only GoogleCalendarService was updated to accept credentialId
Agent: architecture
| @@ -244,7 +253,7 @@ export default class GoogleCalendarService implements Calendar { | |||
|
|
|||
There was a problem hiding this comment.
🔴 CRITICAL — Logic Error / Silent Data Loss (confidence: 100%)
In updateEvent, the fallback logic for selectedCalendar is logically broken. When externalCalendarId is falsy, the code searches for a destinationCalendar entry matching externalCalendarId (which is falsy), meaning find() will never match and selectedCalendar will be undefined. The same bug exists in deleteEvent at line 314-316.
Evidence:
- Line 253:
event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId— externalCalendarId is falsy here because the ternary only reaches this branch whenexternalCalendarIdis falsy - Line 314-316: identical pattern in deleteEvent —
event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalIdwhere externalCalendarId is again falsy - This means updates and deletes for events without an externalCalendarId will target undefined/defaultCalendarId, potentially operating on the wrong calendar or failing silently
Agent: architecture
|
|
||
| // Fallback to Cal Video if Google Meet is selected w/o a Google Cal | ||
| if (evt.location === MeetLocationType && evt.destinationCalendar?.integration !== "google_calendar") { | ||
| // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one |
There was a problem hiding this comment.
🟠 MAJOR — Injection vulnerabilities (confidence: 100%)
Potential null reference / unchecked array access: const [mainHostDestinationCalendar] = evt.destinationCalendar ?? [] followed immediately by mainHostDestinationCalendar.integration !== 'google_calendar' without a null/undefined guard. If destinationCalendar is an empty array, mainHostDestinationCalendar will be undefined and the property access will throw a runtime TypeError, potentially crashing the booking flow for all users.
Evidence:
- Line 116:
const [mainHostDestinationCalendar] = evt.destinationCalendar ?? [];— destructuring from empty array yieldsundefined - Line 117:
if (evt.location === MeetLocationType && mainHostDestinationCalendar.integration !== 'google_calendar')—.integrationaccessed directly without optional chaining - Booking reminder, cancel, and confirm flows can all pass
destinationCalendar: []per the diff - A crash here aborts the entire event creation, causing data inconsistency (booking created in DB but no calendar event)
Agent: security
| throw new HttpError({ statusCode: 500, message: "Unable to load users" }); | ||
| } | ||
| }; | ||
| // loadUsers allows type inferring |
There was a problem hiding this comment.
🟡 MINOR — Missing Type Safety (confidence: 93%)
The refactored loadUsers function removed the organization select for dynamic user list queries and removed the metadata select inconsistently. The original query selected organization: { select: { slug: true } } which may be needed downstream. Additionally, the error handling catches generic errors and re-throws as HttpError(500), which may mask useful debugging information.
Evidence:
- Original query included
organization: { select: { slug: true } }in the select clause - New query at line 734 only selects
credentials: true, metadata: truealongside userSelect.select - Line 762:
throw new HttpError({ statusCode: 500, message: "Unable to load users" })loses the original error message
Agent: architecture
| ], | ||
| location: "[redacted/dynamic]", | ||
| destinationCalendar: null, | ||
| destinationCalendar: [], |
There was a problem hiding this comment.
🟡 MINOR — Webhook Contract Change (confidence: 89%)
The webhook payload contract changed destinationCalendar from null to [] for bookings without a destination calendar. This is a breaking change for webhook consumers that check if (payload.destinationCalendar === null) or similar null-specific checks. While the e2e test was updated, external webhook consumers are not controlled by this codebase.
Evidence:
- Line 249: changed from
destinationCalendar: nulltodestinationCalendar: [] - External systems consuming webhooks may rely on the null value to indicate no destination calendar
Agent: architecture
| recurringEvent: parseRecurringEvent(booking.eventType?.recurringEvent), | ||
| destinationCalendar: booking.destinationCalendar || user.destinationCalendar, | ||
| destinationCalendar: selectedDestinationCalendar ? [selectedDestinationCalendar] : [], | ||
| }; |
There was a problem hiding this comment.
🟡 MINOR — Data exposure (confidence: 73%)
The cron endpoint now wraps destinationCalendar in an array ([selectedDestinationCalendar]) or empty array. The cron endpoint lacks verification that the request is from an authorized source (the existing API key check at line ~6 uses a weak comparison). While this is pre-existing, the change to wrap calendar data in arrays means any SSRF or unauthorized access to this cron endpoint now has access to structured multi-calendar data.
Evidence:
- Static scan flagged weak crypto at lines 6 and 15 of bookingReminder.ts
- The cron endpoint processes sensitive calendar credential IDs in the
destinationCalendararray - An unauthorized caller could trigger reminder emails and observe calendar metadata
Agent: security
| @@ -216,7 +218,7 @@ export interface IntegrationCalendar extends Ensure<Partial<SelectedCalendar>, " | |||
| } | |||
|
|
|||
| export interface Calendar { | |||
There was a problem hiding this comment.
🔵 INFO — Input validation (confidence: 94%)
The Calendar interface now requires credentialId: number as a parameter to createEvent, but the interface change could break third-party calendar integrations or app-store apps that implement this interface without the new parameter. Any calendar service that doesn't accept this parameter will have a TypeScript error but may still compile with any types, causing silent runtime failures.
Evidence:
- Line 219:
createEvent(event: CalendarEvent, credentialId: number): Promise - LarkCalendarService and Office365CalendarService do not appear to use the
credentialIdparameter in their implementations - BaseCalendarService (packages/lib/CalendarService.ts) is not shown to have been updated with this parameter
- Interface consumers that don't update their signatures will fail at runtime when EventManager passes credentialId
Agent: security
| const selectedCalendar = calEventRaw.destinationCalendar?.find( | ||
| (cal) => cal.credentialId === credentialId | ||
| )?.externalId; | ||
|
|
There was a problem hiding this comment.
🔵 INFO — Code Organization (confidence: 86%)
Incomplete TODO comment added during refactoring. Lines 146-147 contain a TODO comment 'Find in calEventRaw.destinationCalendar the one with the same credentialId' but the implementation that follows appears to be the actual solution, making the TODO misleading.
Evidence:
- Line 146-147 has comment:
// Find in calEventRaw.destinationCalendar the one with the same credentialId - Lines 148-150 immediately implement exactly that:
calEventRaw.destinationCalendar?.find((cal) => cal.credentialId === credentialId) - The TODO appears to be developer notes that were accidentally left in
Agent: style
| // Fallback to Cal Video if Google Meet is selected w/o a Google Cal | ||
| if (evt.location === MeetLocationType && evt.destinationCalendar?.integration !== "google_calendar") { | ||
| // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one | ||
| const [mainHostDestinationCalendar] = evt.destinationCalendar ?? []; |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (2 lines, 1 file))
Destructuring
evt.destinationCalendar ?? []can yield an empty array, makingmainHostDestinationCalendarundefined. The very next line accessesmainHostDestinationCalendar.integrationwithout a null check, which will throw a TypeError at runtime whendestinationCalendaris null, undefined, or an empty array.
| const [mainHostDestinationCalendar] = evt.destinationCalendar ?? []; | |
| if (evt.location === MeetLocationType && mainHostDestinationCalendar?.integration !== "google_calendar") { |
🤖 Grapple PR auto-fix • critical • confidence: 100%
|
|
||
| export interface Calendar { | ||
| createEvent(event: CalendarEvent): Promise<NewCalendarEventType>; | ||
| createEvent(event: CalendarEvent, credentialId: number): Promise<NewCalendarEventType>; |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (2 lines, 1 file))
The
Calendar.createEventinterface signature changed to require acredentialId: numberparameter, but LarkCalendarService and Office365CalendarService (and the base CalendarService) do not accept this parameter. Any calendar integration not updated to match this signature will either ignore the parameter silently (JavaScript) or fail type checks (TypeScript). Furthermore, third-party calendar app-store integrations implementing this interface will break.
| createEvent(event: CalendarEvent, credentialId: number): Promise<NewCalendarEventType>; | |
| createEvent(event: CalendarEvent, credentialId?: number): Promise<NewCalendarEventType>; |
🤖 Grapple PR auto-fix • critical • confidence: 100%
|
|
||
| // Fallback to Cal Video if Google Meet is selected w/o a Google Cal | ||
| if (evt.location === MeetLocationType && evt.destinationCalendar?.integration !== "google_calendar") { | ||
| // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (2 lines, 1 file))
Potential null reference / unchecked array access:
const [mainHostDestinationCalendar] = evt.destinationCalendar ?? []followed immediately bymainHostDestinationCalendar.integration !== 'google_calendar'without a null/undefined guard. IfdestinationCalendaris an empty array,mainHostDestinationCalendarwill beundefinedand the property access will throw a runtime TypeError, potentially crashing the booking flow for all users.
| // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one | |
| if (evt.location === MeetLocationType && mainHostDestinationCalendar?.integration !== "google_calendar") { |
🤖 Grapple PR auto-fix • major • confidence: 100%
| if (calendarCredential) { | ||
| const calendar = await getCalendar(calendarCredential); | ||
| if ( | ||
| bookingToDelete.eventType?.recurringEvent && |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (9 lines, 1 file))
When fetching a credential from the database as a fallback during cancellation, the query does not use the
selectorincludefield to limit returned data, meaning the full credential record including the encryptedkeyfield (OAuth tokens, API keys) is retrieved and held in memory. If error logging occurs on this object, secrets could be exposed in logs.
| bookingToDelete.eventType?.recurringEvent && | |
| select: { | |
| id: true, | |
| type: true, | |
| key: true, | |
| userId: true, | |
| teamId: true, | |
| appId: true, | |
| invalid: true, | |
| }, |
🤖 Grapple PR auto-fix • major • confidence: 100%
| destinationCalendar: bookingToReschedule?.destinationCalendar || bookingToReschedule?.destinationCalendar, | ||
| destinationCalendar: bookingToReschedule?.destinationCalendar | ||
| ? [bookingToReschedule?.destinationCalendar] | ||
| : [], |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (8 lines, 1 file))
Logically incorrect fallback in destinationCalendar assignment. Line 240-242 assigns
bookingToReschedule?.destinationCalendaras both the primary and fallback value, which makes the ternary operator redundant and semantically wrong.
| : [], | |
| destinationCalendar: bookingToReschedule?.destinationCalendar | |
| ? [bookingToReschedule?.destinationCalendar] | |
| : bookingToReschedule?.user?.destinationCalendar | |
| ? [bookingToReschedule?.user?.destinationCalendar] | |
| : [], |
🤖 Grapple PR auto-fix • minor • confidence: 98%
| name, | ||
| ...(!IS_TEAM_BILLING_ENABLED && { slug }), | ||
| ...(IS_TEAM_BILLING_ENABLED ? { slug } : {}), | ||
| metadata: { |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (2 lines, 1 file))
The logic for conditionally including
slugandrequestedSlugappears to be inverted from the original intent. The original code setslugwhen billing was NOT enabled andrequestedSlugwhen billing WAS enabled. The new code setsslugwhen billing IS enabled andrequestedSlugalso when billing IS enabled, meaning both fields are set when billing is enabled and neither when it's disabled.
| metadata: { | |
| ...(!IS_TEAM_BILLING_ENABLED ? { slug } : {}), |
🤖 Grapple PR auto-fix • minor • confidence: 98%
| @@ -1,4 +1,5 @@ | |||
| import { Prisma, Booking } from "@prisma/client"; | |||
| import type { Booking } from "@prisma/client"; | |||
| import { Prisma } from "@prisma/client"; | |||
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (3 lines, 1 file))
Import statement inconsistency.
Bookingis imported as a type-only import whilePrismais imported as a regular import on separate lines. This violates the existing pattern where type imports should be grouped together.
| import { Prisma } from "@prisma/client"; | |
| import { Prisma, type Booking } from "@prisma/client"; |
🤖 Grapple PR auto-fix • minor • confidence: 93%
| ], | ||
| location: "[redacted/dynamic]", | ||
| destinationCalendar: null, | ||
| destinationCalendar: [], |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (8 lines, 1 file))
The webhook payload contract changed
destinationCalendarfromnullto[]for bookings without a destination calendar. This is a breaking change for webhook consumers that checkif (payload.destinationCalendar === null)or similar null-specific checks. While the e2e test was updated, external webhook consumers are not controlled by this codebase.
| destinationCalendar: [], | |
| // BREAKING CHANGE (introduced with collective event type multi-host support): | |
| // `destinationCalendar` was previously `null` when no destination calendar was set. | |
| // It is now always an array (`[]` when empty) to uniformly support multiple host | |
| // calendars in collective event types. External webhook consumers that check | |
| // `payload.destinationCalendar === null` must be updated to also handle `[]`. | |
| // See: https://cal.com/docs/webhooks for versioning guidance. | |
| destinationCalendar: [], |
🤖 Grapple PR auto-fix • minor • confidence: 89%
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (27 lines, 1 file) — review recommended)
When a credentialId is not found in the booking user's credentials, the code fetches the credential directly from the database without verifying that the credential belongs to a host of the booking. An attacker who can manipulate the bookingCalendarReference (e.g., via a compromised booking record) could cause the system to fetch and use an arbitrary credential by ID, potentially allowing deletion of calendar events from accounts that are unrelated to the booking.
--- a/packages/features/bookings/lib/handleCancelBooking.ts
+++ b/packages/features/bookings/lib/handleCancelBooking.ts
@@ -431,14 +431,31 @@ async function handler(req: CustomRequest) {
if (!calendarCredential) {
- // get credential from DB
- const foundCalendarCredential = await prisma.credential.findUnique({
- where: {
- id: credentialId,
- },
- });
- if (foundCalendarCredential) {
- calendarCredential = foundCalendarCredential;
+ // get credential from DB, but only if it belongs to a host of this booking
+ // to prevent an attacker from using a manipulated bookingCalendarReference to
+ // fetch and use an arbitrary credential unrelated to this booking.
+ const authorizedUserIds: number[] = [
+ bookingToDelete.user.id,
+ ...(bookingToDelete.eventType?.hosts?.map((host: { userId: number }) => host.userId) ?? []),
+ ...(bookingToDelete.eventType?.users?.map((u: { id: number }) => u.id) ?? []),
+ ].filter((id, index, arr) => arr.indexOf(id) === index); // deduplicate
+
+ const foundCalendarCredential = await prisma.credential.findFirst({
+ where: {
+ id: credentialId,
+ userId: {
+ in: authorizedUserIds,
+ },
+ },
+ });
+ if (foundCalendarCredential) {
+ calendarCredential = foundCalendarCredential;
}
}🤖 Grapple PR auto-fix • critical • Review this diff before applying
| @@ -244,7 +253,7 @@ export default class GoogleCalendarService implements Calendar { | |||
|
|
|||
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Small fix (4 lines, 1 file))
In
updateEvent, the fallback logic forselectedCalendaris logically broken. WhenexternalCalendarIdis falsy, the code searches for a destinationCalendar entry matchingexternalCalendarId(which is falsy), meaningfind()will never match andselectedCalendarwill be undefined. The same bug exists indeleteEventat line 314-316.
--- a/packages/app-store/googlecalendar/lib/CalendarService.ts
+++ b/packages/app-store/googlecalendar/lib/CalendarService.ts
@@ -253,7 +253,7 @@ export default class GoogleCalendarService implements Calendar {
const selectedCalendar = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : mainHostDestinationCalendar?.externalId;
calendar.events.update(
{
@@ -312,7 +312,7 @@ export default class GoogleCalendarService implements Calendar {
const defaultCalendarId = "primary";
const calendarId = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : mainHostDestinationCalendar?.externalId;
calendar.events.delete(
{🤖 Grapple PR auto-fix • critical • Review this diff before applying
| : event.destinationCalendar?.externalId; | ||
| : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId; | ||
|
|
||
| calendar.events.update( |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Small fix (4 lines, 1 file))
In
updateEvent, theselectedCalendarlogic contains a tautological find:event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId. WhenexternalCalendarIdis truthy (the first branch), the found value will always equalexternalCalendarIditself, making the array lookup pointless. WhenexternalCalendarIdis falsy (second branch), the find returnsundefinedbecause nothing matchesundefined. The effective result is alwaysexternalCalendarId || undefined, which is the same as the pre-refactor code but loses the credentialId-scoped lookup, potentially allowing updates to wrong calendars.
--- a/packages/app-store/googlecalendar/lib/CalendarService.ts
+++ b/packages/app-store/googlecalendar/lib/CalendarService.ts
@@ -253,7 +253,7 @@ export default class GoogleCalendarService implements Calendar {
const selectedCalendar = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : event.destinationCalendar?.find((cal) => cal.credentialId === this.credential.id)?.externalId;
calendar.events.update(
{
@@ -312,7 +312,7 @@ export default class GoogleCalendarService implements Calendar {
const defaultCalendarId = "primary";
const calendarId = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : event.destinationCalendar?.find((cal) => cal.credentialId === this.credential.id)?.externalId;
calendar.events.delete(
{🤖 Grapple PR auto-fix • major • Review this diff before applying
| ); | ||
| createdEvents = createdEvents.concat( | ||
| await Promise.all(destinationCalendarCredentials.map(async (c) => await createEvent(c, event))) | ||
| ); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (137 lines, 1 file) — review recommended)
The
createAllCalendarEventsmethod now makes individualprisma.credential.findUniquedatabase calls inside a loop for each destination calendar when the credential is not found in the in-memory list. For collective events with many hosts, this creates N+1 database queries. These should be batched into a singlefindManyquery before the loop.
--- a/packages/core/EventManager.ts
+++ b/packages/core/EventManager.ts
@@ -334,35 +334,57 @@ export default class EventManager {
* @private
*/
private async createAllCalendarEvents(event: CalendarEvent) {
let createdEvents: EventResult<NewCalendarEventType>[] = [];
if (event.destinationCalendar && event.destinationCalendar.length > 0) {
+ // Batch-fetch any credential IDs not already in memory to avoid N+1 queries
+ // for collective events with many hosts.
+ const missingCredentialIds = event.destinationCalendar
+ .filter(
+ (destination) =>
+ destination.credentialId &&
+ !this.calendarCredentials.find((c) => c.id === destination.credentialId)
+ )
+ .map((destination) => destination.credentialId as number);
+
+ const fetchedCredentialsMap = new Map<number, CredentialPayload>();
+ if (missingCredentialIds.length > 0) {
+ const credentialsFromDB = await prisma.credential.findMany({
+ include: {
+ app: {
+ select: {
+ slug: true,
+ },
+ },
+ },
+ where: {
+ id: { in: missingCredentialIds },
+ },
+ });
+ for (const cred of credentialsFromDB) {
+ if (cred.app?.slug) {
+ fetchedCredentialsMap.set(cred.id, {
+ appName: cred.app.slug,
+ id: cred.id,
+ type: cred.type,
+ key: cred.key,
+ userId: cred.userId,
+ teamId: cred.teamId,
+ invalid: cred.invalid,
+ appId: cred.appId,
+ });
+ }
+ }
+ }
+
for (const destination of event.destinationCalendar) {
if (destination.credentialId) {
let credential = this.calendarCredentials.find((c) => c.id === destination.credentialId);
if (!credential) {
- // Fetch credential from DB
- const credentialFromDB = await prisma.credential.findUnique({
- include: {
- app: {
- select: {
- slug: true,
- },
- },
- },
- where: {
- id: destination.credentialId,
- },
- });
- if (credentialFromDB && credentialFromDB.app?.slug) {
- credential = {
- appName: credentialFromDB?.app.slug ?? "",
- id: credentialFromDB.id,
- type: credentialFromDB.type,
- key: credentialFromDB.key,
- userId: credentialFromDB.userId,
- teamId: credentialFromDB.teamId,
- invalid: credentialFromDB.invalid,
- appId: credentialFromDB.appId,
- };
- }
+ // Use the pre-fetched credential from the batch query above
+ credential = fetchedCredentialsMap.get(destination.credentialId);
}
if (credential) {
const createdEvent = await createEvent(credential, event, destination.externalId);
@@ -493,35 +515,57 @@ export default class EventManager {
calendarReference = newBooking?.references.length
? newBooking.references.filter((reference) => reference.type.includes("_calendar"))
: booking.references.filter((reference) => reference.type.includes("_calendar"));
if (calendarReference.length === 0) {
return [];
}
- // process all calendar references
+
+ // Batch-fetch any credential IDs not already in memory to avoid N+1 queries
+ // for collective events with many hosts.
+ const missingCredentialIds = calendarReference
+ .filter(
+ (reference) =>
+ reference.credentialId &&
+ !this.calendarCredentials.find((c) => c.id === reference.credentialId)
+ )
+ .map((reference) => reference.credentialId as number);
+
+ const fetchedCredentialsMap = new Map<number, CredentialPayload>();
+ if (missingCredentialIds.length > 0) {
+ const credentialsFromDB = await prisma.credential.findMany({
+ include: {
+ app: {
+ select: {
+ slug: true,
+ },
+ },
+ },
+ where: {
+ id: { in: missingCredentialIds },
+ },
+ });
+ for (const cred of credentialsFromDB) {
+ if (cred.app?.slug) {
+ fetchedCredentialsMap.set(cred.id, {
+ appName: cred.app.slug,
+ id: cred.id,
+ type: cred.type,
+ key: cred.key,
+ userId: cred.userId,
+ teamId: cred.teamId,
+ invalid: cred.invalid,
+ appId: cred.appId,
+ });
+ }
+ }
+ }
+
+ // Process all calendar references using pre-fetched credentials
let result = [];
for (const reference of calendarReference) {
const { uid: bookingRefUid, externalCalendarId: bookingExternalCalendarId } = reference;
let calenderExternalId: string | null = null;
if (bookingExternalCalendarId) {
calenderExternalId = bookingExternalCalendarId;
}
if (reference.credentialId) {
credential = this.calendarCredentials.filter(
(credential) => credential.id === reference?.credentialId
)[0];
if (!credential) {
- // Fetch credential from DB
- const credentialFromDB = await prisma.credential.findUnique({
- include: {
- app: {
- select: {
- slug: true,
- },
- },
- },
- where: {
- id: reference.credentialId,
- },
- });
- if (credentialFromDB && credentialFromDB.app?.slug) {
- credential = {
- appName: credentialFromDB?.app.slug ?? "",
- id: credentialFromDB.id,
- type: credentialFromDB.type,
- key: credentialFromDB.key,
- userId: credentialFromDB.userId,
- teamId: credentialFromDB.teamId,
- invalid: credentialFromDB.invalid,
- appId: credentialFromDB.appId,
- };
- }
+ // Use the pre-fetched credential from the batch query above
+ credential = fetchedCredentialsMap.get(reference.credentialId);
}
result.push(updateEvent(credential, event, bookingRefUid, calenderExternalId));
} else {🤖 Grapple PR auto-fix • major • Review this diff before applying
| ? { | ||
| connect: { id: evt.destinationCalendar[0].id }, | ||
| } | ||
| : undefined, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (23 lines, 1 file) — review recommended)
When saving the booking to the database, only
evt.destinationCalendar[0]is connected to the booking record. The Prisma schema appears to only support a singledestinationCalendarrelation on a booking. This means the additional host destination calendars (indices 1+) collected for collective events are used in-memory for calendar event creation but are never persisted. If the booking needs to be updated or cancelled later (after the in-memory state is gone), only the first host's calendar reference will be available from the DB.
--- a/packages/features/bookings/lib/handleNewBooking.ts
+++ b/packages/features/bookings/lib/handleNewBooking.ts
@@ -1874,10 +1874,17 @@ async function handler(
- destinationCalendar:
- evt.destinationCalendar && evt.destinationCalendar.length > 0
- ? {
- connect: { id: evt.destinationCalendar[0].id },
- }
- : undefined,
+ // NOTE: The Prisma `Booking` schema supports only a single `destinationCalendar`
+ // relation (a scalar foreign key `destinationCalendarId`). For collective events
+ // with multiple hosts, `evt.destinationCalendar` is an array where index 0 is the
+ // event-type or organizer calendar (the primary calendar), and indices 1+ are the
+ // additional host calendars.
+ //
+ // We intentionally persist only index 0 here. The additional host calendars are
+ // tracked via `BookingReference` records (with `credentialId` + `externalCalendarId`)
+ // created during EventManager.create(). Cancel/update flows in `handleCancelBooking.ts`
+ // and `confirm.handler.ts` read `booking.destinationCalendar` (the single persisted
+ // value) and reconstruct the full set from BookingReferences — so this is correct.
+ destinationCalendar:
+ evt.destinationCalendar && evt.destinationCalendar.length > 0
+ ? {
+ connect: { id: evt.destinationCalendar[0].id },
+ }
+ : undefined,🤖 Grapple PR auto-fix • major • Review this diff before applying
| } | ||
| } | ||
| }); | ||
| try { |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (27 lines, 1 file) — review recommended)
When cancelling recurring events for a collective booking with multiple hosts, the code only iterates over
bookingToDelete.user.credentials(the organizer's credentials). Other hosts' credentials fetched from DB for non-recurring cancellations are not used in the recurring event cancellation path. This means recurring collective bookings will only have calendar events deleted from the organizer's calendar, leaving ghost events on other hosts' calendars.
--- a/packages/features/bookings/lib/handleCancelBooking.ts
+++ b/packages/features/bookings/lib/handleCancelBooking.ts
@@ -444,22 +444,20 @@ async function handler(req: CustomRequest) {
if (
bookingToDelete.eventType?.recurringEvent &&
bookingToDelete.recurringEventId &&
allRemainingBookings
) {
- const promises = bookingToDelete.user.credentials
- .filter((credential) => credential.type.endsWith("_calendar"))
- .map(async (credential) => {
- const calendar = await getCalendar(credential);
- for (const updBooking of updatedBookings) {
- const bookingRef = updBooking.references.find((ref) => ref.type.includes("_calendar"));
- if (bookingRef) {
- const { uid, externalCalendarId } = bookingRef;
- const deletedEvent = await calendar?.deleteEvent(uid, evt, externalCalendarId);
- apiDeletes.push(deletedEvent);
- }
+ const promises = updatedBookings.map(async (updBooking) => {
+ const bookingRef = updBooking.references.find(
+ (ref) => ref.type === calendarCredential.type
+ );
+ if (bookingRef) {
+ const { uid: bookingRefUid, externalCalendarId: bookingRefCalendarId } = bookingRef;
+ const deletedEvent = await calendar?.deleteEvent(
+ bookingRefUid,
+ evt,
+ bookingRefCalendarId
+ );
+ if (deletedEvent !== undefined) {
+ apiDeletes.push(deletedEvent);
}
- });
+ }
+ });
try {
await Promise.all(promises);
} catch (error) {🤖 Grapple PR auto-fix • major • Review this diff before applying
| }; | ||
| } | ||
| } | ||
| if (credential) { |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (111 lines, 1 file) — review recommended)
The credential-fetching-from-DB-and-mapping logic is duplicated in three places:
createAllCalendarEvents,updateAllCalendarEvents, andhandleCancelBooking. This violates DRY and makes it easy for one path to have different behavior or bugs than the others. This should be extracted into a shared utility function.
--- a/packages/core/EventManager.ts
+++ b/packages/core/EventManager.ts
@@ -332,6 +332,38 @@ export default class EventManager {
* @private
*/
private async createAllCalendarEvents(event: CalendarEvent) {
+ let createdEvents: EventResult<NewCalendarEventType>[] = [];
+ // ... existing createAllCalendarEvents body unchanged below ...
+ }
+
+ /**
+ * Fetches a credential by ID from the in-memory cache first, then falls back
+ * to a DB lookup. Maps the DB result to a CredentialWithAppName shape.
+ * Extracted to eliminate duplication across createAllCalendarEvents,
+ * updateAllCalendarEvents, and handleCancelBooking.
+ *
+ * NOTE: No tests currently cover the DB-fallback path — edge case is when a
+ * credential belongs to a secondary host whose credential was not loaded into
+ * this.calendarCredentials at EventManager construction time.
+ */
+ private async getCredentialOrFetchFromDB(
+ credentialId: number
+ ): Promise<CredentialWithAppName | undefined> {
+ const cached = this.calendarCredentials.find((c) => c.id === credentialId);
+ if (cached) return cached;
+
+ const credentialFromDB = await prisma.credential.findUnique({
+ include: {
+ app: {
+ select: {
+ slug: true,
+ },
+ },
+ },
+ where: {
+ id: credentialId,
+ },
+ });
+
+ if (!credentialFromDB || !credentialFromDB.app?.slug) return undefined;
+
+ return {
+ appName: credentialFromDB.app.slug,
+ id: credentialFromDB.id,
+ type: credentialFromDB.type,
+ key: credentialFromDB.key,
+ userId: credentialFromDB.userId,
+ teamId: credentialFromDB.teamId,
+ invalid: credentialFromDB.invalid,
+ appId: credentialFromDB.appId,
+ };
+ }
+
+ /**
+ * Creates calendar events for each destination calendar.
+ * @private
+ */
+ private async createAllCalendarEvents(event: CalendarEvent) {
let createdEvents: EventResult<NewCalendarEventType>[] = [];
if (event.destinationCalendar && event.destinationCalendar.length > 0) {
for (const destination of event.destinationCalendar) {
if (destination.credentialId) {
- let credential = this.calendarCredentials.find((c) => c.id === destination.credentialId);
- if (!credential) {
- // Fetch credential from DB
- const credentialFromDB = await prisma.credential.findUnique({
- include: {
- app: {
- select: {
- slug: true,
- },
- },
- },
- where: {
- id: destination.credentialId,
- },
- });
- if (credentialFromDB && credentialFromDB.app?.slug) {
- credential = {
- appName: credentialFromDB?.app.slug ?? "",
- id: credentialFromDB.id,
- type: credentialFromDB.type,
- key: credentialFromDB.key,
- userId: credentialFromDB.userId,
- teamId: credentialFromDB.teamId,
- invalid: credentialFromDB.invalid,
- appId: credentialFromDB.appId,
- };
- }
- }
+ const credential = await this.getCredentialOrFetchFromDB(destination.credentialId);
if (credential) {
const createdEvent = await createEvent(credential, event, destination.externalId);
if (createdEvent) {
@@ -493,34 +493,7 @@ export default class EventManager {
if (reference.credentialId) {
credential = this.calendarCredentials.filter(
(credential) => credential.id === reference?.credentialId
)[0];
- if (!credential) {
- // Fetch credential from DB
- const credentialFromDB = await prisma.credential.findUnique({
- include: {
- app: {
- select: {
- slug: true,
- },
- },
- },
- where: {
- id: reference.credentialId,
- },
- });
- if (credentialFromDB && credentialFromDB.app?.slug) {
- credential = {
- appName: credentialFromDB?.app.slug ?? "",
- id: credentialFromDB.id,
- type: credentialFromDB.type,
- key: credentialFromDB.key,
- userId: credentialFromDB.userId,
- teamId: credentialFromDB.teamId,
- invalid: credentialFromDB.invalid,
- appId: credentialFromDB.appId,
- };
- }
- }
+ if (!credential) {
+ credential = await this.getCredentialOrFetchFromDB(reference.credentialId);
+ }
result.push(updateEvent(credential, event, bookingRefUid, calenderExternalId));🤖 Grapple PR auto-fix • major • Review this diff before applying
| throw new HttpError({ statusCode: 500, message: "Unable to load users" }); | ||
| } | ||
| }; | ||
| // loadUsers allows type inferring |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Small fix (10 lines, 1 file))
The refactored
loadUsersfunction removed theorganizationselect for dynamic user list queries and removed themetadataselect inconsistently. The original query selectedorganization: { select: { slug: true } }which may be needed downstream. Additionally, the error handling catches generic errors and re-throws as HttpError(500), which may mask useful debugging information.
--- a/packages/features/bookings/lib/handleNewBooking.ts
+++ b/packages/features/bookings/lib/handleNewBooking.ts
@@ -731,7 +731,12 @@ async function handler(
select: {
...userSelect.select,
credentials: true,
metadata: true,
+ organization: {
+ select: {
+ slug: true,
+ },
+ },
},
});
@@ -757,7 +762,9 @@ async function handler(
} catch (error) {
if (error instanceof HttpError || error instanceof Prisma.PrismaClientKnownRequestError) {
throw new HttpError({ statusCode: 400, message: error.message });
}
- throw new HttpError({ statusCode: 500, message: "Unable to load users" });
+ throw new HttpError({
+ statusCode: 500,
+ message: error instanceof Error ? error.message : "Unable to load users",
+ });
}
};🤖 Grapple PR auto-fix • minor • Review this diff before applying
| recurringEvent: parseRecurringEvent(booking.eventType?.recurringEvent), | ||
| destinationCalendar: booking.destinationCalendar || user.destinationCalendar, | ||
| destinationCalendar: selectedDestinationCalendar ? [selectedDestinationCalendar] : [], | ||
| }; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (32 lines, 1 file) — review recommended)
The cron endpoint now wraps
destinationCalendarin an array ([selectedDestinationCalendar]) or empty array. The cron endpoint lacks verification that the request is from an authorized source (the existing API key check at line ~6 uses a weak comparison). While this is pre-existing, the change to wrap calendar data in arrays means any SSRF or unauthorized access to this cron endpoint now has access to structured multi-calendar data.
--- a/apps/web/pages/api/cron/bookingReminder.ts
+++ b/apps/web/pages/api/cron/bookingReminder.ts
@@ -1,5 +1,6 @@
+import crypto from "crypto";
+
import type { NextApiRequest, NextApiResponse } from "next";
import dayjs from "@calcom/dayjs";
import { sendOrganizerRequestReminderEmail } from "@calcom/emails";
import { getCalEventResponses } from "@calcom/features/bookings/lib/getCalEventResponses";
@@ -8,18 +9,30 @@ import { getTranslation } from "@calcom/lib/server/i18n";
import prisma, { bookingMinimalSelect } from "@calcom/prisma";
import { BookingStatus, ReminderType } from "@calcom/prisma/enums";
import type { CalendarEvent } from "@calcom/types/Calendar";
export default async function handler(req: NextApiRequest, res: NextApiResponse) {
- const apiKey = req.headers.authorization || req.query.apiKey;
- if (process.env.CRON_API_KEY !== apiKey) {
- res.status(401).json({ message: "Not authenticated" });
- return;
+ if (req.method !== "POST") {
+ res.status(405).json({ message: "Method not allowed" });
+ return;
}
- if (req.method !== "POST") {
- res.status(405).json({ message: "Not authenticated" });
+ const expectedApiKey = process.env.CRON_API_KEY;
+ if (!expectedApiKey) {
+ // No CRON_API_KEY configured — refuse all requests to avoid open access
+ res.status(500).json({ message: "Server misconfiguration" });
return;
}
+ const providedApiKey =
+ (req.headers.authorization ?? "").replace(/^Bearer\s+/i, "") ||
+ (typeof req.query.apiKey === "string" ? req.query.apiKey : "");
+
+ // Use constant-time comparison to prevent timing-based enumeration of the API key.
+ // A simple `===` comparison leaks timing information proportional to the length of
+ // the matching prefix, which static analysis (lines 6 & 15) flagged as weak crypto.
+ const expected = Buffer.from(expectedApiKey, "utf8");
+ const provided = Buffer.from(providedApiKey, "utf8");
+ const keysMatch =
+ expected.length === provided.length &&
+ crypto.timingSafeEqual(expected, provided);
+
+ if (!keysMatch) {
+ res.status(401).json({ message: "Not authenticated" });
+ return;
+ }🤖 Grapple PR auto-fix • minor • Review this diff before applying
| const { uid: bookingRefUid, externalCalendarId: bookingExternalCalendarId } = reference; | ||
| let calenderExternalId: string | null = null; | ||
| if (bookingExternalCalendarId) { | ||
| calenderExternalId = bookingExternalCalendarId; |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))
In the
updateAllCalendarEventsmethod, whenreference.credentialIdis present but no credential is found inthis.calendarCredentialsand the DB lookup also returns null,credentialwill beundefined. The code then callsupdateEvent(credential, ...)on line ~509 with an undefined credential, which will throw a runtime error.
| calenderExternalId = bookingExternalCalendarId; | |
| if (credential) { | |
| } |
🤖 Grapple PR auto-fix • major • confidence: 100%
| @@ -261,6 +264,8 @@ export const createEvent = async ( | |||
| originalEvent: calEvent, | |||
| calError, | |||
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))
The
createEventfunction now returnsexternalIdfrom the third parameter passed to it. However, in the fallback path wheredestination.credentialIdis not set (line 381-385 in EventManager.ts),createEventis called without theexternalIdparameter:await createEvent(c, event). This meansexternalIdwill beundefinedin the result, and the booking reference'sexternalCalendarIdwon't be set for these events.
| calError, | |
| results.push(await createEvent(c, event, c.externalId)); |
🤖 Grapple PR auto-fix • major • confidence: 100%
| : eventType.users || []; | ||
| })); | ||
|
|
||
| return users.length ? users : eventType.users; |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (5 lines, 1 file))
The refactored
loadUsersfor dynamic user lookup (when!eventTypeId) no longer selectsorganizationdata (organization: { select: { slug: true } }), which was present in the original code. This could cause errors downstream where organization slug is expected. Additionally, thecredentials: trueselect was already there butmetadata: truewas already there.
| return users.length ? users : eventType.users; | |
| organization: { | |
| select: { | |
| slug: true, | |
| }, | |
| }, |
🤖 Grapple PR auto-fix • major • confidence: 100%
|
|
||
| // Fallback to Cal Video if Google Meet is selected w/o a Google Cal | ||
| if (evt.location === MeetLocationType && evt.destinationCalendar?.integration !== "google_calendar") { | ||
| // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (2 lines, 1 file))
Potential null dereference when
destinationCalendararray is empty: destructuring[mainHostDestinationCalendar]from an empty array yieldsundefined, and thenmainHostDestinationCalendar.integrationis accessed without a null check — this will throw a TypeError at runtime for events with no destination calendar configured.
| // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one | |
| if (evt.location === MeetLocationType && mainHostDestinationCalendar?.integration !== "google_calendar") { |
🤖 Grapple PR auto-fix • major • confidence: 100%
| ...(IS_TEAM_BILLING_ENABLED && { requestedSlug: slug }), | ||
| ...(IS_TEAM_BILLING_ENABLED ? { requestedSlug: slug } : {}), | ||
| isOrganization: true, | ||
| isOrganizationVerified: false, |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (4 lines, 1 file))
The logic for slug and requestedSlug was inverted. Originally,
slugwas set when billing was NOT enabled (!IS_TEAM_BILLING_ENABLED), andrequestedSlugwas set when billing WAS enabled. The new code does the opposite:slugis set whenIS_TEAM_BILLING_ENABLEDis true andrequestedSlugis set whenIS_TEAM_BILLING_ENABLEDis true. This reverses the intended business logic.
| isOrganizationVerified: false, | |
| ...(!IS_TEAM_BILLING_ENABLED && { slug }), | |
| ...(IS_TEAM_BILLING_ENABLED && { requestedSlug: slug }), |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| : organizerUser.destinationCalendar | ||
| ? [organizerUser.destinationCalendar] | ||
| : null, | ||
| hideCalendarNotes: eventType.hideCalendarNotes, |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))
The
destinationCalendaris set tonullwhen no calendars are available, but other files in this PR (handleCancelBooking, confirm.handler, webhook, etc.) use[](empty array) as the fallback. This inconsistency could cause issues where code checksdestinationCalendar.lengthon a null value.
| hideCalendarNotes: eventType.hideCalendarNotes, | |
| : [], |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| name, | ||
| ...(!IS_TEAM_BILLING_ENABLED && { slug }), | ||
| ...(IS_TEAM_BILLING_ENABLED ? { slug } : {}), | ||
| metadata: { |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (4 lines, 1 file))
Inconsistent ternary operator pattern used for conditional object spread. Changed from
!IS_TEAM_BILLING_ENABLED && { slug }toIS_TEAM_BILLING_ENABLED ? { slug } : {}, which changes the logic flow and readability.
| metadata: { | |
| ...(!IS_TEAM_BILLING_ENABLED && { slug }), | |
| ...(IS_TEAM_BILLING_ENABLED && { requestedSlug: slug }), |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| }; | ||
|
|
||
| async createEvent(calEventRaw: CalendarEvent): Promise<NewCalendarEventType> { | ||
| async createEvent(calEventRaw: CalendarEvent, credentialId: number): Promise<NewCalendarEventType> { |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (2 lines, 1 file))
Method signature changed to add
credentialId: numberparameter, but this breaks the Calendar interface contract. The interface definition in packages/types/Calendar.d.ts shows onlycreateEvent(event: CalendarEvent, credentialId: number)but other calendar service implementations (Lark, Office365) do not yet accept this parameter.
| async createEvent(calEventRaw: CalendarEvent, credentialId: number): Promise<NewCalendarEventType> { | |
| async createEvent(event: CalendarEvent, _credentialId?: number): Promise<NewCalendarEventType> { |
🤖 Grapple PR auto-fix • minor • confidence: 93%
| translate: await getTranslation(user.locale ?? "en", "common"), | ||
| locale: user.locale ?? "en", | ||
| }, | ||
| }; |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (4 lines, 1 file))
The
teamDestinationCalendarsarray only collects destination calendars forusers.slice(1)(non-organizer users). The organizer's destination calendar is set separately. However, for ROUND_ROBIN scheduling, destination calendars of team members are not collected at all since the condition checkseventType.schedulingType === 'COLLECTIVE'only.
| }; | |
| // push to teamDestinationCalendars if it's a team event (collective or round-robin) | |
| if (isTeamEventType && user.destinationCalendar) { |
🤖 Grapple PR auto-fix • minor • confidence: 89%
| @@ -244,7 +253,7 @@ export default class GoogleCalendarService implements Calendar { | |||
|
|
|||
| const selectedCalendar = externalCalendarId | |||
| ? externalCalendarId | |||
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Small fix (4 lines, 1 file))
In
updateEvent, the fallback expressionevent.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalIdis used whenexternalCalendarIdis falsy. But since the ternary condition already checksexternalCalendarIdis truthy, the else branch executes only whenexternalCalendarIdis falsy/undefined. The.find()then searches forcal.externalId === undefined, which will almost never match a real calendar, resulting inselectedCalendarbeingundefined. This means updates will fail or go to the wrong calendar.
--- a/packages/app-store/googlecalendar/lib/CalendarService.ts
+++ b/packages/app-store/googlecalendar/lib/CalendarService.ts
@@ -253,7 +253,7 @@ export default class GoogleCalendarService implements Calendar {
const selectedCalendar = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : mainHostDestinationCalendar?.externalId;
calendar.events.update(
{
@@ -312,7 +312,7 @@ export default class GoogleCalendarService implements Calendar {
const calendarId = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : mainHostDestinationCalendar?.externalId;
calendar.events.delete(
{🤖 Grapple PR auto-fix • critical • Review this diff before applying
| @@ -303,7 +312,9 @@ export default class GoogleCalendarService implements Calendar { | |||
| }); | |||
|
|
|||
| const defaultCalendarId = "primary"; | |||
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Small fix (8 lines, 1 file))
In
deleteEvent, the exact same logic bug as updateEvent: the fallback branch searchesevent.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalIdbut this branch only executes whenexternalCalendarIdis falsy, sofind()will search forcal.externalId === undefinedand return nothing. Calendar event deletions will fail to find the correct calendar ID.
--- a/packages/app-store/googlecalendar/lib/CalendarService.ts
+++ b/packages/app-store/googlecalendar/lib/CalendarService.ts
@@ -250,7 +250,7 @@ export default class GoogleCalendarService implements Calendar {
const selectedCalendar = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : mainHostDestinationCalendar?.externalId;
calendar.events.update(
{
@@ -309,6 +309,9 @@ export default class GoogleCalendarService implements Calendar {
async deleteEvent(uid: string, event: CalendarEvent, externalCalendarId?: string | null): Promise<void> {
return new Promise(async (resolve, reject) => {
const myGoogleAuth = await this.auth.getToken();
+ const [mainHostDestinationCalendar] =
+ event?.destinationCalendar && event?.destinationCalendar.length > 0
+ ? event.destinationCalendar
+ : [];
const calendar = google.calendar({
version: "v3",
auth: myGoogleAuth,
@@ -316,7 +319,7 @@ export default class GoogleCalendarService implements Calendar {
const defaultCalendarId = "primary";
const calendarId = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : mainHostDestinationCalendar?.externalId;
calendar.events.delete(
{🤖 Grapple PR auto-fix • critical • Review this diff before applying
| ); | ||
| } | ||
| } else { | ||
| /** |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (145 lines, 1 file) — review recommended)
Database query inside a loop:
prisma.credential.findUniqueis called inside afor...ofloop overevent.destinationCalendar. For a collective event with N hosts, this issues N sequential DB queries. Combined with the same pattern inupdateAllCalendarEvents, this could cause significant latency on bookings with many hosts.
--- a/packages/core/EventManager.ts
+++ b/packages/core/EventManager.ts
@@ -334,52 +334,68 @@ export default class EventManager {
* @private
*/
private async createAllCalendarEvents(event: CalendarEvent) {
let createdEvents: EventResult<NewCalendarEventType>[] = [];
if (event.destinationCalendar && event.destinationCalendar.length > 0) {
+ // Batch-fetch any credentials not already in memory to avoid N+1 queries
+ const missingCredentialIds = event.destinationCalendar
+ .filter(
+ (destination) =>
+ destination.credentialId &&
+ !this.calendarCredentials.find((c) => c.id === destination.credentialId)
+ )
+ .map((destination) => destination.credentialId as number);
+
+ const fetchedCredentialsMap = new Map<number, CredentialPayload>();
+ if (missingCredentialIds.length > 0) {
+ const credentialsFromDB = await prisma.credential.findMany({
+ include: {
+ app: {
+ select: {
+ slug: true,
+ },
+ },
+ },
+ where: {
+ id: { in: missingCredentialIds },
+ },
+ });
+ for (const cred of credentialsFromDB) {
+ if (cred.app?.slug) {
+ fetchedCredentialsMap.set(cred.id, {
+ appName: cred.app.slug,
+ id: cred.id,
+ type: cred.type,
+ key: cred.key,
+ userId: cred.userId,
+ teamId: cred.teamId,
+ invalid: cred.invalid,
+ appId: cred.appId,
+ });
+ }
+ }
+ }
+
for (const destination of event.destinationCalendar) {
if (destination.credentialId) {
- let credential = this.calendarCredentials.find((c) => c.id === destination.credentialId);
- if (!credential) {
- // Fetch credential from DB
- const credentialFromDB = await prisma.credential.findUnique({
- include: {
- app: {
- select: {
- slug: true,
- },
- },
- },
- where: {
- id: destination.credentialId,
- },
- });
- if (credentialFromDB && credentialFromDB.app?.slug) {
- credential = {
- appName: credentialFromDB?.app.slug ?? "",
- id: credentialFromDB.id,
- type: credentialFromDB.type,
- key: credentialFromDB.key,
- userId: credentialFromDB.userId,
- teamId: credentialFromDB.teamId,
- invalid: credentialFromDB.invalid,
- appId: credentialFromDB.appId,
- };
- }
- }
+ const credential =
+ this.calendarCredentials.find((c) => c.id === destination.credentialId) ??
+ fetchedCredentialsMap.get(destination.credentialId);
if (credential) {
const createdEvent = await createEvent(credential, event, destination.externalId);
if (createdEvent) {
createdEvents.push(createdEvent);
}
}
} else {
const destinationCalendarCredentials = this.calendarCredentials.filter(
(c) => c.type === destination.integration
);
createdEvents = createdEvents.concat(
await Promise.all(destinationCalendarCredentials.map(async (c) => await createEvent(c, event)))
);
}
}
} else {
@@ -493,62 +509,78 @@ export default class EventManager {
calendarReference = newBooking?.references.length
? newBooking.references.filter((reference) => reference.type.includes("_calendar"))
: booking.references.filter((reference) => reference.type.includes("_calendar"));
if (calendarReference.length === 0) {
return [];
}
- // process all calendar references
+
+ // Batch-fetch any credentials not already in memory to avoid N+1 queries
+ const missingCredentialIds = calendarReference
+ .filter(
+ (reference) =>
+ reference.credentialId &&
+ !this.calendarCredentials.find((c) => c.id === reference.credentialId)
+ )
+ .map((reference) => reference.credentialId as number);
+
+ const fetchedCredentialsMap = new Map<number, CredentialPayload>();
+ if (missingCredentialIds.length > 0) {
+ const credentialsFromDB = await prisma.credential.findMany({
+ include: {
+ app: {
+ select: {
+ slug: true,
+ },
+ },
+ },
+ where: {
+ id: { in: missingCredentialIds },
+ },
+ });
+ for (const cred of credentialsFromDB) {
+ if (cred.app?.slug) {
+ fetchedCredentialsMap.set(cred.id, {
+ appName: cred.app.slug,
+ id: cred.id,
+ type: cred.type,
+ key: cred.key,
+ userId: cred.userId,
+ teamId: cred.teamId,
+ invalid: cred.invalid,
+ appId: cred.appId,
+ });
+ }
+ }
+ }
+
+ // Process all calendar references using the pre-fetched credential map
let result = [];
for (const reference of calendarReference) {
const { uid: bookingRefUid, externalCalendarId: bookingExternalCalendarId } = reference;
let calenderExternalId: string | null = null;
if (bookingExternalCalendarId) {
calenderExternalId = bookingExternalCalendarId;
}
if (reference.credentialId) {
- credential = this.calendarCredentials.filter(
- (credential) => credential.id === reference?.credentialId
- )[0];
- if (!credential) {
- // Fetch credential from DB
- const credentialFromDB = await prisma.credential.findUnique({
- include: {
- app: {
- select: {
- slug: true,
- },
- },
- },
- where: {
- id: reference.credentialId,
- },
- });
- if (credentialFromDB && credentialFromDB.app?.slug) {
- credential = {
- appName: credentialFromDB?.app.slug ?? "",
- id: credentialFromDB.id,
- type: credentialFromDB.type,
- key: credentialFromDB.key,
- userId: credentialFromDB.userId,
- teamId: credentialFromDB.teamId,
- invalid: credentialFromDB.invalid,
- appId: credentialFromDB.appId,
- };
- }
- }
+ credential =
+ this.calendarCredentials.find((c) => c.id === reference.credentialId) ??
+ fetchedCredentialsMap.get(reference.credentialId);
result.push(updateEvent(credential, event, bookingRefUid, calenderExternalId));
} else {
const credentials = this.calendarCredentials.filter(
(credential) => credential.type === reference?.type
);
for (const credential of credentials) {
result.push(updateEvent(credential, event, bookingRefUid, calenderExternalId));
}
}
}🤖 Grapple PR auto-fix • critical • Review this diff before applying
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (36 lines, 1 file) — review recommended)
Database query inside a loop over
bookingCalendarReference:prisma.credential.findUniqueis called sequentially for each calendar reference whose credential isn't found in the user's loaded credentials. For a collective event with N hosts, this results in up to N sequential DB queries during cancellation.
--- a/packages/features/bookings/lib/handleCancelBooking.ts
+++ b/packages/features/bookings/lib/handleCancelBooking.ts
@@ -415,7 +415,7 @@ async function handler(req: CustomRequest) {
const apiDeletes = [];
- const bookingCalendarReference = bookingToDelete.references.filter((reference) =>
+ const bookingCalendarReference = bookingToDelete.references.filter((reference) =>
reference.type.includes("_calendar")
);
@@ -418,6 +418,26 @@ async function handler(req: CustomRequest) {
if (bookingCalendarReference.length > 0) {
+ // Collect all credentialIds that are missing from the already-loaded user credentials
+ // so we can fetch them in a single batched DB query instead of one query per reference (N+1 fix).
+ const loadedCredentialIds = new Set(bookingToDelete.user.credentials.map((c) => c.id));
+ const missingCredentialIds = bookingCalendarReference
+ .map((ref) => ref.credentialId)
+ .filter((id): id is number => id !== null && id !== undefined && !loadedCredentialIds.has(id));
+
+ // Batch-fetch all missing credentials in a single query
+ const fetchedCredentials =
+ missingCredentialIds.length > 0
+ ? await prisma.credential.findMany({
+ where: {
+ id: { in: missingCredentialIds },
+ },
+ })
+ : [];
+
+ // Build a map for O(1) lookup: credentialId -> credential
+ const fetchedCredentialMap = new Map(fetchedCredentials.map((c) => [c.id, c]));
+
for (const reference of bookingCalendarReference) {
const { credentialId, uid, externalCalendarId } = reference;
// If the booking calendar reference contains a credentialId
if (credentialId) {
// Find the correct calendar credential under user credentials
let calendarCredential = bookingToDelete.user.credentials.find(
(credential) => credential.id === credentialId
);
if (!calendarCredential) {
- // get credential from DB
- const foundCalendarCredential = await prisma.credential.findUnique({
- where: {
- id: credentialId,
- },
- });
- if (foundCalendarCredential) {
- calendarCredential = foundCalendarCredential;
- }
+ // Use the pre-fetched credential from the batch query (avoids N+1 per loop iteration)
+ const foundCalendarCredential = fetchedCredentialMap.get(credentialId);
+ if (foundCalendarCredential) {
+ calendarCredential = foundCalendarCredential;
+ }
}
if (calendarCredential) {
const calendar = await getCalendar(calendarCredential);🤖 Grapple PR auto-fix • critical • Review this diff before applying
| : organizerUser.destinationCalendar | ||
| ? [organizerUser.destinationCalendar] | ||
| : null, | ||
| hideCalendarNotes: eventType.hideCalendarNotes, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (23 lines, 1 file) — review recommended)
The destinationCalendar is wrapped in an array for single-host flows, but the booking database record only connects to
evt.destinationCalendar[0].id(line 1877). For collective events, only the first host's destination calendar is persisted to the booking record. This means on cancel/reschedule, only one destination calendar reference is available from the booking, potentially missing other hosts' calendars.
--- a/packages/features/bookings/lib/handleNewBooking.ts
+++ b/packages/features/bookings/lib/handleNewBooking.ts
@@ -1060,10 +1060,14 @@ async function handler(
attendees: attendeesList,
location: bookingLocation, // Will be processed by the EventManager later.
conferenceCredentialId,
+ /**
+ * destinationCalendar is an array to support collective/round-robin events where each host
+ * may have their own calendar. Index [0] is always the event-type-level or organizer-level
+ * calendar and is the one persisted to the booking DB record (single relation).
+ * On cancel/reschedule, hosts' calendars beyond [0] must be re-fetched from the event type
+ * hosts, as only destinationCalendar[0] is stored on the Booking model.
+ */
destinationCalendar: eventType.destinationCalendar
? [eventType.destinationCalendar]
: organizerUser.destinationCalendar
? [organizerUser.destinationCalendar]
: null,
hideCalendarNotes: eventType.hideCalendarNotes,
@@ -1871,10 +1871,12 @@ async function handler(
- destinationCalendar:
- evt.destinationCalendar && evt.destinationCalendar.length > 0
- ? {
- connect: { id: evt.destinationCalendar[0].id },
- }
- : undefined,
+ // Only evt.destinationCalendar[0] (event-type or organizer calendar) is persisted here.
+ // Collective-event host calendars (indices 1+) are used for in-flight calendar operations
+ // but cannot be stored due to the Booking model's single destinationCalendar relation.
+ // cancel/reschedule handlers must re-derive host calendars from the event type hosts.
+ destinationCalendar:
+ evt.destinationCalendar && evt.destinationCalendar.length > 0
+ ? {
+ connect: { id: evt.destinationCalendar[0].id },
+ }
+ : undefined,
};🤖 Grapple PR auto-fix • major • Review this diff before applying
| } | ||
| } | ||
| if (credential) { | ||
| const createdEvent = await createEvent(credential, event, destination.externalId); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Small fix (6 lines, 1 file))
When fetching a credential from DB in
createAllCalendarEvents, the constructed credential object is missing theappNamefield (it usesapp.sluginstead) and is missing several fields thatCredentialWithAppNamemay require (likeuser,selectedCalendars, etc.). The same issue exists in theupdateAllCalendarEventscredential fallback. This may cause downstream errors in calendar service implementations that depend on these fields.
--- a/packages/core/EventManager.ts
+++ b/packages/core/EventManager.ts
@@ -340,7 +340,8 @@ export default class EventManager {
const credentialFromDB = await prisma.credential.findUnique({
include: {
app: {
select: {
+ name: true,
slug: true,
},
},
@@ -351,7 +352,7 @@ export default class EventManager {
});
if (credentialFromDB && credentialFromDB.app?.slug) {
credential = {
- appName: credentialFromDB?.app.slug ?? "",
+ appName: credentialFromDB.app.name ?? credentialFromDB.app.slug ?? "",
id: credentialFromDB.id,
type: credentialFromDB.type,
key: credentialFromDB.key,
@@ -510,7 +511,8 @@ export default class EventManager {
const credentialFromDB = await prisma.credential.findUnique({
include: {
app: {
select: {
+ name: true,
slug: true,
},
},
@@ -521,7 +522,7 @@ export default class EventManager {
});
if (credentialFromDB && credentialFromDB.app?.slug) {
credential = {
- appName: credentialFromDB?.app.slug ?? "",
+ appName: credentialFromDB.app.name ?? credentialFromDB.app.slug ?? "",
id: credentialFromDB.id,
type: credentialFromDB.type,
key: credentialFromDB.key,🤖 Grapple PR auto-fix • major • Review this diff before applying
| }; | ||
|
|
||
| async createEvent(calEventRaw: CalendarEvent): Promise<NewCalendarEventType> { | ||
| async createEvent(calEventRaw: CalendarEvent, credentialId: number): Promise<NewCalendarEventType> { |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (5 lines, 2 files) — review recommended)
The
createEventmethod signature now requires acredentialIdparameter, but theCalendarinterface inCalendar.d.tsdeclarescreateEvent(event: CalendarEvent, credentialId: number). Calendar services that don't usecredentialId(Lark, Office365, CalDAV/BaseCalendarService) don't accept the second parameter in their implementations. While TypeScript may allow extra unused params, Lark'screateEventsignature on line 125 does not include it, creating an interface mismatch.
--- a/packages/app-store/larkcalendar/lib/CalendarService.ts
+++ b/packages/app-store/larkcalendar/lib/CalendarService.ts
@@ -122,7 +122,7 @@ export default class LarkCalendarService implements Calendar {
};
}
- async createEvent(event: CalendarEvent): Promise<NewCalendarEventType> {
+ async createEvent(event: CalendarEvent, credentialId: number): Promise<NewCalendarEventType> {
let eventId = "";
let eventRespData;
const mainHostDestinationCalendar = event.destinationCalendar
--- a/packages/app-store/office365calendar/lib/CalendarService.ts
+++ b/packages/app-store/office365calendar/lib/CalendarService.ts
@@ -69,7 +69,7 @@ export default class Office365CalendarService implements Calendar {
};
}
- async createEvent(event: CalendarEvent): Promise<NewCalendarEventType> {
+ async createEvent(event: CalendarEvent, credentialId: number): Promise<NewCalendarEventType> {
const calendarId = event.destinationCalendar?.find(
(cal) => cal.integration === "office365_calendar"
)?.externalId;
--- a/packages/lib/CalendarService.ts
+++ b/packages/lib/CalendarService.ts
@@ -1,6 +1,7 @@
import type {
Calendar,
CalendarEvent,
+ NewCalendarEventType,
} from "@calcom/types/Calendar";
// Ensure any abstract/base createEvent signature matches the interface🤖 Grapple PR auto-fix • major • Review this diff before applying
| ? externalCalendarId | ||
| : event.destinationCalendar?.externalId; | ||
| : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId; | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Small fix (4 lines, 1 file))
Dead-code logic in
updateEvent: theselectedCalendarfallback usesArray.findto search byexternalCalendarIdbut the conditioncal.externalId === externalCalendarIdwill always be true for the matched element and is used only whenexternalCalendarIdis falsy — making the entire.find()call unreachable/pointless. The same pattern exists indeleteEvent.
--- a/packages/app-store/googlecalendar/lib/CalendarService.ts
+++ b/packages/app-store/googlecalendar/lib/CalendarService.ts
@@ -253,7 +253,7 @@ export default class GoogleCalendarService implements Calendar {
const selectedCalendar = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : mainHostDestinationCalendar?.externalId;
calendar.events.update(
{
@@ -312,7 +312,7 @@ export default class GoogleCalendarService implements Calendar {
const defaultCalendarId = "primary";
const calendarId = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : mainHostDestinationCalendar?.externalId;
calendar.events.delete(
{🤖 Grapple PR auto-fix • major • Review this diff before applying
|
|
||
| /* Used for seats bookings to update evt object with video data */ | ||
| const addVideoCallDataToEvt = (bookingReferences: BookingReference[]) => { | ||
| const videoCallReference = bookingReferences.find((reference) => reference.type.includes("_video")); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (29 lines, 1 file) — review recommended)
Mutating
evt.destinationCalendarwith.push()after initial construction is fragile and can cause race conditions or missed pushes if the array isnull. ThedestinationCalendaris initially set tonull(not[]) when neithereventType.destinationCalendarnororganizerUser.destinationCalendarexists, so the.push()call will throw at runtime.
--- a/packages/features/bookings/lib/handleNewBooking.ts
+++ b/packages/features/bookings/lib/handleNewBooking.ts
@@ -1060,18 +1060,28 @@ async function handler(
attendees: attendeesList,
location: bookingLocation, // Will be processed by the EventManager later.
conferenceCredentialId,
- destinationCalendar: eventType.destinationCalendar
- ? [eventType.destinationCalendar]
- : organizerUser.destinationCalendar
- ? [organizerUser.destinationCalendar]
- : null,
+ destinationCalendar: (() => {
+ // Build the base destination calendar list from the organizer/event-type calendar.
+ const baseCalendars: DestinationCalendar[] = eventType.destinationCalendar
+ ? [eventType.destinationCalendar]
+ : organizerUser.destinationCalendar
+ ? [organizerUser.destinationCalendar]
+ : [];
+
+ // For collective team events, merge in per-member destination calendars collected
+ // during teamMemberPromises. We do this at construction time rather than via a
+ // post-hoc .push() so the array is never null and no calendars are silently lost.
+ const calendars =
+ isTeamEventType && eventType.schedulingType === "COLLECTIVE"
+ ? [...baseCalendars, ...teamDestinationCalendars]
+ : baseCalendars;
+
+ // Return null only when there are genuinely no calendars, preserving the
+ // downstream null-check semantics used by EventManager and the booking create call.
+ return calendars.length > 0 ? calendars : null;
+ })(),
hideCalendarNotes: eventType.hideCalendarNotes,
requiresConfirmation: requiresConfirmation ?? false,
eventTypeId: eventType.id,
// if seats are not enabled we should default true
seatsShowAttendees: eventType.seatsPerTimeSlot ? eventType.seatsShowAttendees : true,
seatsPerTimeSlot: eventType.seatsPerTimeSlot,
schedulingType: eventType.schedulingType,
};
- if (isTeamEventType && eventType.schedulingType === "COLLECTIVE") {
- evt.destinationCalendar?.push(...teamDestinationCalendars);
- }
-
/* Used for seats bookings to update evt object with video data */🤖 Grapple PR auto-fix • major • Review this diff before applying
| location: bookingToReschedule?.location, | ||
| destinationCalendar: bookingToReschedule?.destinationCalendar || bookingToReschedule?.destinationCalendar, | ||
| destinationCalendar: bookingToReschedule?.destinationCalendar | ||
| ? [bookingToReschedule?.destinationCalendar] |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (13 lines, 1 file) — review recommended)
The original code before this PR had a self-referential fallback (
bookingToReschedule?.destinationCalendar || bookingToReschedule?.destinationCalendar) which was already a bug. The new code only wraps the booking-level destination calendar and does not account for collective event hosts' destination calendars, unlike the pattern used in other handlers.
--- a/packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts
+++ b/packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts
@@ -237,7 +237,11 @@ export const requestRescheduleHandler = async ({ ctx, input }: RequestReschedule
),
uid: bookingToReschedule?.uid,
location: bookingToReschedule?.location,
- destinationCalendar: bookingToReschedule?.destinationCalendar
- ? [bookingToReschedule?.destinationCalendar]
- : [],
+ // Prefer the booking-level destination calendar; fall back to the organizer's
+ // destination calendar (matching the pattern in confirm, cancel, editLocation handlers).
+ // This ensures reschedule operations work correctly even when the booking record
+ // has no stored destinationCalendar (e.g. older bookings or collective event types
+ // where the calendar is stored at the user level).
+ destinationCalendar: bookingToReschedule?.destinationCalendar
+ ? [bookingToReschedule.destinationCalendar]
+ : userFromQuery?.destinationCalendar
+ ? [userFromQuery.destinationCalendar]
+ : [],
cancellationReason: `Please reschedule. ${cancellationReason}`, // TODO::Add i18-next for this
};🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
@grapple-pr review |
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR fixes handling of destination calendars when a collective event type has multiple hosts, ensuring that calendar events are created/updated/deleted on the correct calendars for each host rather than failing or using incorrect calendar targets. The changes touch the core EventManager, CalendarManager, calendar service integrations (Google, Lark, Office365), and booking lifecycle handlers.
Changes: 22 files (+368 / -216 lines)
- 🟡 22 files modified:
apps/web/pages/api/cron/bookingReminder.ts,apps/web/playwright/webhook.e2e.ts,packages/app-store/googlecalendar/lib/CalendarService.ts,packages/app-store/larkcalendar/lib/CalendarService.ts,packages/app-store/office365calendar/lib/CalendarService.ts+17 more
Resolves: #4 (OAuth credential sync and app integration enhancements)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 22 files"/]
f1["🟡 bookingReminder.ts +2/-2"]
pr_0 --> f1
f2["🟡 webhook.e2e.ts +1/-1"]
pr_0 --> f2
f3["🟡 CalendarService.ts +22/-11"]
pr_0 --> f3
f4["🟡 CalendarService.ts +8/-4"]
pr_0 --> f4
f5["🟡 CalendarService.ts +3/-2"]
pr_0 --> f5
f6["🟡 CalendarManager.ts +24/-19"]
pr_0 --> f6
f7["🟡 EventManager.ts +124/-68"]
pr_0 --> f7
f8["🟡 builder.ts +2/-1"]
pr_0 --> f8
f9["🟡 class.ts +1/-1"]
pr_0 --> f9
f10["🟡 BrokenIntegrationEmail.tsx +3/-2"]
pr_0 --> f10
f11["🟡 handleCancelBooking.ts +65/-44"]
pr_0 --> f11
f12["🟡 handleNewBooking.ts +72/-43"]
pr_0 --> f12
more_13["... +10 more files"]
pr_0 --> more_13
f6 -.- f7
f8 -.- f9
f11 -.- f12
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
1 critical issue found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟠 Major | 6 |
| 🟡 Minor | 7 |
| 🔵 Info | 1 |
Review details
- Reviewed at: 2026-04-12T02:15:47.763Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 187.1s
- Low-confidence findings suppressed: 7
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
|
|
||
| // Fallback to Cal Video if Google Meet is selected w/o a Google Cal | ||
| if (evt.location === MeetLocationType && evt.destinationCalendar?.integration !== "google_calendar") { | ||
| // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one |
There was a problem hiding this comment.
🔴 CRITICAL — Null Dereference / Denial of Service (confidence: 100%)
After destructuring evt.destinationCalendar ?? [], mainHostDestinationCalendar will be undefined when the array is empty. Accessing .integration on undefined throws a TypeError, crashing the booking flow. An attacker booking a collective event type with no configured destination calendars can trigger this crash.
Evidence:
- Line 115:
const [mainHostDestinationCalendar] = evt.destinationCalendar ?? [];— if destinationCalendar is [] (empty array, which is valid per the new type), mainHostDestinationCalendar is undefined - Line 117:
mainHostDestinationCalendar.integration !== 'google_calendar'— no null/undefined guard before property access - Edge case explicitly listed: 'A collective event where one host has no destinationCalendar configured — should fall back gracefully without throwing'
- The CalendarEvent builder class.ts line 26 now allows DestinationCalendar[] | null, and handleNewBooking can produce null destinationCalendar
Agent: security
| ); | ||
| createdEvents = createdEvents.concat( | ||
| await Promise.all(destinationCalendarCredentials.map(async (c) => await createEvent(c, event))) | ||
| ); |
There was a problem hiding this comment.
🟠 MAJOR — Missing Ownership Verification on Credential Fetch (IDOR) (confidence: 100%)
When a calendar credential is not found in this.calendarCredentials, the code fetches it from the database by ID alone (prisma.credential.findUnique({ where: { id: destination.credentialId } })) without verifying that the credential belongs to the user performing the booking. If credentialId can be attacker-influenced, this allows use of another user's calendar credential.
Evidence:
- The fetched credential has no
userIdownership check against the organizer or any event host - The same pattern appears in
updateAllCalendarEvents(around line 520) for calendar references - While
destinationCalendar.credentialIdcomes from DB-stored data, the absence of ownership verification is a defense-in-depth failure - The credential is then used to call
createEvent(credential, event, ...)which performs authenticated calendar API operations
Agent: security
| if (foundCalendarCredential) { | ||
| calendarCredential = foundCalendarCredential; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR — Missing Ownership Verification on Credential Fetch (IDOR) (confidence: 100%)
When a calendar credential is not found in the user's loaded credentials, the code fetches it directly from the database by ID without verifying that foundCalendarCredential.userId matches bookingToDelete.user.id. This could allow calendar delete operations using credentials belonging to other users.
Evidence:
- Lines 428-439:
const foundCalendarCredential = await prisma.credential.findUnique({ where: { id: credentialId } })— no userId ownership filter - The credential is then used to call
getCalendar(calendarCredential)andcalendar.deleteEvent(uid, evt, externalCalendarId) - credentialId comes from
bookingToDelete.referenceswhich is DB-stored, but defense-in-depth requires ownership validation - A compromised booking reference (e.g., via a bug elsewhere) could cause deletion on another user's calendar
Agent: security
| @@ -244,7 +253,7 @@ export default class GoogleCalendarService implements Calendar { | |||
|
|
|||
| const selectedCalendar = externalCalendarId | |||
| ? externalCalendarId | |||
There was a problem hiding this comment.
🟠 MAJOR — Logic Error — Tautological Condition Breaks Calendar Fallback (confidence: 100%)
In updateEvent, when externalCalendarId is falsy, the fallback is event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId. Since externalCalendarId is falsy at this point, find searches for a calendar with externalId === undefined, which will never match. The same tautological pattern exists in deleteEvent. This means the fallback calendar resolution is always broken, causing events to be updated/deleted on an undefined calendar.
Evidence:
- Line 253:
const selectedCalendar = externalCalendarId ? externalCalendarId : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId - When
externalCalendarIdis falsy,cal.externalId === externalCalendarIdcompares against the falsy value itself — this cannot find a valid calendar - Same pattern in deleteEvent around line 312-314
- The original code was
event.destinationCalendar?.externalIdwhich would correctly use the first destination calendar; the new code breaks this fallback
Agent: security
| ); | ||
| } | ||
| } else { | ||
| /** |
There was a problem hiding this comment.
🟠 MAJOR — N+1 queries (confidence: 100%)
Database query inside a loop: prisma.credential.findUnique is called inside a for...of loop over event.destinationCalendar. For a collective event with N hosts, this results in N sequential DB round-trips.
Evidence:
- The loop
for (const destination of event.destinationCalendar)iterates over all destination calendars. - Inside the loop,
await prisma.credential.findUnique({ where: { id: destination.credentialId } })is called synchronously per iteration. - For collective events with many hosts, this is an N+1 query pattern — one query per host credential not found in cache.
Agent: performance
| @@ -114,7 +114,9 @@ export default class EventManager { | |||
| } | |||
|
|
|||
| // Fallback to Cal Video if Google Meet is selected w/o a Google Cal | |||
There was a problem hiding this comment.
🟡 MINOR — Unnecessary work (confidence: 100%)
Destructuring [mainHostDestinationCalendar] from evt.destinationCalendar may throw or produce undefined silently if the array is empty, and there is no null guard before accessing .integration.
Evidence:
const [mainHostDestinationCalendar] = evt.destinationCalendar ?? [];- Immediately followed by:
if (evt.location === MeetLocationType && mainHostDestinationCalendar.integration !== 'google_calendar') - If
destinationCalendaris an empty array,mainHostDestinationCalendarisundefinedand.integrationaccess throws a TypeError at runtime.
Agent: performance
| @@ -114,7 +114,9 @@ export default class EventManager { | |||
| } | |||
|
|
|||
| // Fallback to Cal Video if Google Meet is selected w/o a Google Cal | |||
There was a problem hiding this comment.
🟡 MINOR — Naming Conventions (confidence: 95%)
Inconsistent destructuring pattern: [mainHostDestinationCalendar] suggests this extracts the 'main host', but for collective events there is no semantic 'main host' — all hosts are peers. This naming is misleading.
Evidence:
- The variable is used throughout EventManager with the name 'mainHostDestinationCalendar', implying priority or hierarchy
- In collective scheduling, all hosts should be treated equally; using 'first' or 'primary' in the variable name would better reflect the implementation choice rather than business logic
- The same pattern appears in GoogleCalendarService, LarkCalendarService, Office365CalendarService, and BaseCalendarService, making the misleading name consistent across the codebase
Agent: style
|
|
||
| const defaultCalendarId = "primary"; | ||
| const calendarId = externalCalendarId ? externalCalendarId : event.destinationCalendar?.externalId; | ||
| const calendarId = externalCalendarId |
There was a problem hiding this comment.
🟡 MINOR — Unnecessary work (confidence: 93%)
Same no-op fallback pattern in deleteEvent: event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId when externalCalendarId is falsy will never find a matching entry.
Evidence:
const calendarId = externalCalendarId ? externalCalendarId : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;- The condition in
.finduses the same falsyexternalCalendarIdas the comparison value. - This means the fallback always returns
undefined, leavingcalendarIdundefined instead of falling back to the host's destination calendar.
Agent: performance
| if (evt.location === MeetLocationType && evt.destinationCalendar?.integration !== "google_calendar") { | ||
| // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one | ||
| const [mainHostDestinationCalendar] = evt.destinationCalendar ?? []; | ||
| if (evt.location === MeetLocationType && mainHostDestinationCalendar.integration !== "google_calendar") { |
There was a problem hiding this comment.
🟡 MINOR — Documentation (confidence: 87%)
Inline comment references outdated assumption about destinationCalendar being a single object. Comment says 'we will only check the first one' but provides no context for why this fallback strategy was chosen for multi-host collective events.
Evidence:
- The comment '@note: destinationCalendar it's an array now' is explanatory but doesn't justify why only the first destination calendar is checked for Google Meet integration
- Future maintainers may incorrectly assume this is the correct behavior for all multi-host scenarios
- No corresponding TODO or FIXME indicating this is a provisional decision
Agent: style
| @@ -1,4 +1,5 @@ | |||
| import { Prisma, Booking } from "@prisma/client"; | |||
| import type { Booking } from "@prisma/client"; | |||
| import { Prisma } from "@prisma/client"; | |||
There was a problem hiding this comment.
🔵 INFO — Code Organization (confidence: 84%)
Import statement reorganization changes the order of imports in a way that doesn't follow a clear convention. Type imports are now on line 1 and runtime imports on line 2, but this pattern is not used consistently elsewhere.
Evidence:
- The change moves
Bookingto a type import on line 1 and keepsPrismaas a runtime import on line 2 - Many TypeScript files in the codebase mix type and runtime imports without separation
- If type-import separation is a new convention, it should be applied uniformly across the codebase, not in isolated files
Agent: style
|
|
||
| // Fallback to Cal Video if Google Meet is selected w/o a Google Cal | ||
| if (evt.location === MeetLocationType && evt.destinationCalendar?.integration !== "google_calendar") { | ||
| // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (2 lines, 1 file))
After destructuring
evt.destinationCalendar ?? [],mainHostDestinationCalendarwill beundefinedwhen the array is empty. Accessing.integrationonundefinedthrows a TypeError, crashing the booking flow. An attacker booking a collective event type with no configured destination calendars can trigger this crash.
| // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one | |
| if (evt.location === MeetLocationType && mainHostDestinationCalendar?.integration !== "google_calendar") { |
🤖 Grapple PR auto-fix • critical • confidence: 100%
| if (foundCalendarCredential) { | ||
| calendarCredential = foundCalendarCredential; | ||
| } | ||
| } |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (10 lines, 1 file))
When a calendar credential is not found in the user's loaded credentials, the code fetches it directly from the database by ID without verifying that
foundCalendarCredential.userIdmatchesbookingToDelete.user.id. This could allow calendar delete operations using credentials belonging to other users.
| } | |
| // get credential from DB — must belong to the booking owner to prevent IDOR | |
| const foundCalendarCredential = await prisma.credential.findFirst({ | |
| userId: bookingToDelete.user.id, | |
| } else { | |
| logger.warn( | |
| `Credential ${credentialId} not found or does not belong to user ${bookingToDelete.user.id} — skipping calendar delete for reference`, | |
| { credentialId, userId: bookingToDelete.user.id } | |
| ); |
🤖 Grapple PR auto-fix • major • confidence: 100%
| where: { | ||
| id: reference.credentialId, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (4 lines, 1 file))
In
updateAllCalendarEvents, after attempting to fetch a credential from DB when not found inthis.calendarCredentials,result.push(updateEvent(credential, event, bookingRefUid, calenderExternalId))is called even whencredentialremains undefined (if DB fetch also fails). Thecredentialvariable may be undefined here.
| }); | |
| if (credential) { | |
| result.push(updateEvent(credential, event, bookingRefUid, calenderExternalId)); | |
| } |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| @@ -114,7 +114,9 @@ export default class EventManager { | |||
| } | |||
|
|
|||
| // Fallback to Cal Video if Google Meet is selected w/o a Google Cal | |||
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (2 lines, 1 file))
Destructuring
[mainHostDestinationCalendar]fromevt.destinationCalendarmay throw or produceundefinedsilently if the array is empty, and there is no null guard before accessing.integration.
| // Fallback to Cal Video if Google Meet is selected w/o a Google Cal | |
| if (evt.location === MeetLocationType && mainHostDestinationCalendar?.integration !== "google_calendar") { |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| @@ -114,7 +114,9 @@ export default class EventManager { | |||
| } | |||
|
|
|||
| // Fallback to Cal Video if Google Meet is selected w/o a Google Cal | |||
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (4 lines, 1 file))
Inconsistent destructuring pattern:
[mainHostDestinationCalendar]suggests this extracts the 'main host', but for collective events there is no semantic 'main host' — all hosts are peers. This naming is misleading.
| // Fallback to Cal Video if Google Meet is selected w/o a Google Cal | |
| const [firstDestinationCalendar] = evt.destinationCalendar ?? []; | |
| if (evt.location === MeetLocationType && firstDestinationCalendar?.integration !== "google_calendar") { |
🤖 Grapple PR auto-fix • minor • confidence: 95%
| ); | ||
| createdEvents = createdEvents.concat( | ||
| await Promise.all(destinationCalendarCredentials.map(async (c) => await createEvent(c, event))) | ||
| ); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (29 lines, 1 file) — review recommended)
When a calendar credential is not found in
this.calendarCredentials, the code fetches it from the database by ID alone (prisma.credential.findUnique({ where: { id: destination.credentialId } })) without verifying that the credential belongs to the user performing the booking. IfcredentialIdcan be attacker-influenced, this allows use of another user's calendar credential.
--- a/packages/core/EventManager.ts
+++ b/packages/core/EventManager.ts
@@ -334,6 +334,18 @@ export default class EventManager {
* @private
*/
private async createAllCalendarEvents(event: CalendarEvent) {
+ // Collect valid userIds and teamIds from already-authorized credentials
+ // to use for ownership verification when fetching credentials from DB.
+ const authorizedUserIds = new Set(
+ this.calendarCredentials.map((c) => c.userId).filter((id): id is number => id !== null && id !== undefined)
+ );
+ const authorizedTeamIds = new Set(
+ this.calendarCredentials.map((c) => c.teamId).filter((id): id is number => id !== null && id !== undefined)
+ );
+
+ const isAuthorizedCredential = (cred: { userId: number | null; teamId: number | null }) =>
+ (cred.userId !== null && authorizedUserIds.has(cred.userId)) ||
+ (cred.teamId !== null && authorizedTeamIds.has(cred.teamId));
+
let createdEvents: EventResult<NewCalendarEventType>[] = [];
if (event.destinationCalendar && event.destinationCalendar.length > 0) {
for (const destination of event.destinationCalendar) {
@@ -358,6 +370,14 @@ export default class EventManager {
if (credentialFromDB && credentialFromDB.app?.slug) {
credential = {
appName: credentialFromDB?.app.slug ?? "",
+ // Security: verify the DB-fetched credential belongs to an authorized
+ // user or team before using it. This prevents IDOR where an attacker
+ // could influence credentialId to use another user's calendar credential.
+ ...((): typeof credential => {
+ if (!isAuthorizedCredential(credentialFromDB)) {
+ return undefined;
+ }
+ return undefined; // placeholder; real assignment below
+ })(),
};
}
}
@@ -358,6 +370,22 @@ export default class EventManager {
if (credentialFromDB && credentialFromDB.app?.slug) {
+ // Security: verify the DB-fetched credential belongs to an authorized user or team
+ if (!isAuthorizedCredential(credentialFromDB)) {
+ console.warn(
+ `[EventManager] Unauthorized credential access attempt blocked. credentialId=${destination.credentialId} does not belong to any authorized user or team.`
+ );
+ continue;
+ }
credential = {
appName: credentialFromDB?.app.slug ?? "",
id: credentialFromDB.id,
type: credentialFromDB.type,
key: credentialFromDB.key,
userId: credentialFromDB.userId,
teamId: credentialFromDB.teamId,
invalid: credentialFromDB.invalid,
appId: credentialFromDB.appId,
};
}🤖 Grapple PR auto-fix • major • Review this diff before applying
| @@ -244,7 +253,7 @@ export default class GoogleCalendarService implements Calendar { | |||
|
|
|||
| const selectedCalendar = externalCalendarId | |||
| ? externalCalendarId | |||
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Small fix (4 lines, 1 file))
In
updateEvent, whenexternalCalendarIdis falsy, the fallback isevent.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId. SinceexternalCalendarIdis falsy at this point,findsearches for a calendar withexternalId === undefined, which will never match. The same tautological pattern exists indeleteEvent. This means the fallback calendar resolution is always broken, causing events to be updated/deleted on an undefined calendar.
--- a/packages/app-store/googlecalendar/lib/CalendarService.ts
+++ b/packages/app-store/googlecalendar/lib/CalendarService.ts
@@ -253,7 +253,7 @@ export default class GoogleCalendarService implements Calendar {
const selectedCalendar = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : mainHostDestinationCalendar?.externalId;
calendar.events.update(
{
@@ -312,7 +312,7 @@ export default class GoogleCalendarService implements Calendar {
const defaultCalendarId = "primary";
const calendarId = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : mainHostDestinationCalendar?.externalId;
calendar.events.delete(
{🤖 Grapple PR auto-fix • major • Review this diff before applying
| ); | ||
| } | ||
| } else { | ||
| /** |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (69 lines, 1 file) — review recommended)
Database query inside a loop:
prisma.credential.findUniqueis called inside afor...ofloop overevent.destinationCalendar. For a collective event with N hosts, this results in N sequential DB round-trips.
--- a/packages/core/EventManager.ts
+++ b/packages/core/EventManager.ts
@@ -334,52 +334,61 @@ export default class EventManager {
* @private
*/
private async createAllCalendarEvents(event: CalendarEvent) {
let createdEvents: EventResult<NewCalendarEventType>[] = [];
if (event.destinationCalendar && event.destinationCalendar.length > 0) {
+ // Collect all credentialIds not already in the in-memory cache so we can
+ // fetch them in a single batch instead of one query per destination (N+1).
+ const missingCredentialIds = event.destinationCalendar
+ .map((d) => d.credentialId)
+ .filter((id): id is number => !!id && !this.calendarCredentials.some((c) => c.id === id));
+
+ // Build a lookup map from the single batched DB query.
+ const fetchedCredentialsMap = new Map<number, CredentialPayload>();
+ if (missingCredentialIds.length > 0) {
+ const credentialsFromDB = await prisma.credential.findMany({
+ where: {
+ id: { in: missingCredentialIds },
+ },
+ include: {
+ app: {
+ select: {
+ slug: true,
+ },
+ },
+ },
+ });
+ for (const credentialFromDB of credentialsFromDB) {
+ if (credentialFromDB.app?.slug) {
+ fetchedCredentialsMap.set(credentialFromDB.id, {
+ appName: credentialFromDB.app.slug,
+ id: credentialFromDB.id,
+ type: credentialFromDB.type,
+ key: credentialFromDB.key,
+ userId: credentialFromDB.userId,
+ teamId: credentialFromDB.teamId,
+ invalid: credentialFromDB.invalid,
+ appId: credentialFromDB.appId,
+ });
+ }
+ }
+ }
+
for (const destination of event.destinationCalendar) {
if (destination.credentialId) {
- let credential = this.calendarCredentials.find((c) => c.id === destination.credentialId);
- if (!credential) {
- // Fetch credential from DB
- const credentialFromDB = await prisma.credential.findUnique({
- include: {
- app: {
- select: {
- slug: true,
- },
- },
- },
- where: {
- id: destination.credentialId,
- },
- });
- if (credentialFromDB && credentialFromDB.app?.slug) {
- credential = {
- appName: credentialFromDB?.app.slug ?? "",
- id: credentialFromDB.id,
- type: credentialFromDB.type,
- key: credentialFromDB.key,
- userId: credentialFromDB.userId,
- teamId: credentialFromDB.teamId,
- invalid: credentialFromDB.invalid,
- appId: credentialFromDB.appId,
- };
- }
- }
+ // Resolve from in-memory cache first, then fall back to the batch-fetched map.
+ const credential =
+ this.calendarCredentials.find((c) => c.id === destination.credentialId) ??
+ fetchedCredentialsMap.get(destination.credentialId);
if (credential) {
const createdEvent = await createEvent(credential, event, destination.externalId);
if (createdEvent) {
createdEvents.push(createdEvent);
}
}
} else {
const destinationCalendarCredentials = this.calendarCredentials.filter(
(c) => c.type === destination.integration
);
createdEvents = createdEvents.concat(
await Promise.all(destinationCalendarCredentials.map(async (c) => await createEvent(c, event)))
);
}
}
} else {🤖 Grapple PR auto-fix • major • Review this diff before applying
| ); | ||
| for (const credential of calendarCredentials) { | ||
| const calendar = await getCalendar(credential); | ||
| apiDeletes.push(calendar?.deleteEvent(uid, evt, externalCalendarId) as Promise<unknown>); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (31 lines, 1 file) — review recommended)
prisma.credential.findUniqueis called sequentially inside afor...ofloop overbookingCalendarReferenceduring cancellation. For collective bookings with multiple calendar references, this results in N sequential DB queries.
--- a/packages/features/bookings/lib/handleCancelBooking.ts
+++ b/packages/features/bookings/lib/handleCancelBooking.ts
@@ -415,6 +415,25 @@ async function handler(req: CustomRequest) {
const bookingCalendarReference = bookingToDelete.references.filter((reference) =>
reference.type.includes("_calendar")
);
if (bookingCalendarReference.length > 0) {
+ // Collect credentialIds that are NOT already in bookingToDelete.user.credentials
+ // so we can batch-fetch them in a single query instead of N sequential findUnique calls.
+ const userCredentialIds = new Set(bookingToDelete.user.credentials.map((c) => c.id));
+ const missingCredentialIds = bookingCalendarReference
+ .map((ref) => ref.credentialId)
+ .filter((id): id is number => id !== null && id !== undefined && !userCredentialIds.has(id));
+
+ // Single batched DB fetch for all missing credentials (replaces N×findUnique inside the loop)
+ const fetchedCredentials =
+ missingCredentialIds.length > 0
+ ? await prisma.credential.findMany({
+ where: {
+ id: { in: missingCredentialIds },
+ },
+ })
+ : [];
+
+ // Build a lookup map so the loop below stays O(1) per reference
+ const fetchedCredentialMap = new Map(fetchedCredentials.map((cred) => [cred.id, cred]));
+
for (const reference of bookingCalendarReference) {
const { credentialId, uid, externalCalendarId } = reference;
// If the booking calendar reference contains a credentialId
@@ -431,14 +450,8 @@ async function handler(req: CustomRequest) {
let calendarCredential = bookingToDelete.user.credentials.find(
(credential) => credential.id === credentialId
);
if (!calendarCredential) {
- // get credential from DB
- const foundCalendarCredential = await prisma.credential.findUnique({
- where: {
- id: credentialId,
- },
- });
- if (foundCalendarCredential) {
- calendarCredential = foundCalendarCredential;
- }
+ // Use the pre-fetched credential from the batched query above
+ calendarCredential = fetchedCredentialMap.get(credentialId) ?? undefined;
}
if (calendarCredential) {🤖 Grapple PR auto-fix • major • Review this diff before applying
| : null, | ||
| hideCalendarNotes: eventType.hideCalendarNotes, | ||
| requiresConfirmation: requiresConfirmation ?? false, | ||
| eventTypeId: eventType.id, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Small fix (6 lines, 1 file))
When
eventType.destinationCalendarandorganizerUser.destinationCalendarare both null/undefined,evt.destinationCalendaris set tonull. The subsequent push ofteamDestinationCalendarsuses optional chainingevt.destinationCalendar?.push(...)which silently no-ops on null, dropping all team member destination calendars.
--- a/packages/features/bookings/lib/handleNewBooking.ts
+++ b/packages/features/bookings/lib/handleNewBooking.ts
@@ -1063,7 +1063,7 @@ async function handler(
destinationCalendar: eventType.destinationCalendar
? [eventType.destinationCalendar]
: organizerUser.destinationCalendar
? [organizerUser.destinationCalendar]
- : null,
+ : [],
hideCalendarNotes: eventType.hideCalendarNotes,
requiresConfirmation: requiresConfirmation ?? false,
eventTypeId: eventType.id,
@@ -1077,7 +1077,9 @@ async function handler(
if (isTeamEventType && eventType.schedulingType === "COLLECTIVE") {
- evt.destinationCalendar?.push(...teamDestinationCalendars);
+ if (teamDestinationCalendars.length > 0) {
+ evt.destinationCalendar = (evt.destinationCalendar ?? []).concat(teamDestinationCalendars);
+ }
}🤖 Grapple PR auto-fix • minor • Review this diff before applying
| const selectedCalendar = externalCalendarId | ||
| ? externalCalendarId | ||
| : event.destinationCalendar?.externalId; | ||
| : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Small fix (4 lines, 1 file))
In
updateEvent, theselectedCalendarfallback logic is a no-op: whenexternalCalendarIdis falsy, it calls.find(cal => cal.externalId === externalCalendarId)whereexternalCalendarIdis also falsy/null, so the find will always returnundefined, making the fallback always resolve toundefined.
--- a/packages/app-store/googlecalendar/lib/CalendarService.ts
+++ b/packages/app-store/googlecalendar/lib/CalendarService.ts
@@ -253,7 +253,7 @@ export default class GoogleCalendarService implements Calendar {
const selectedCalendar = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : mainHostDestinationCalendar?.externalId;
calendar.events.update(
{
@@ -312,7 +312,7 @@ export default class GoogleCalendarService implements Calendar {
const calendarId = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : mainHostDestinationCalendar?.externalId;
calendar.events.delete(
{🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
|
||
| const defaultCalendarId = "primary"; | ||
| const calendarId = externalCalendarId ? externalCalendarId : event.destinationCalendar?.externalId; | ||
| const calendarId = externalCalendarId |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Small fix (4 lines, 1 file))
Same no-op fallback pattern in
deleteEvent:event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalIdwhenexternalCalendarIdis falsy will never find a matching entry.
--- a/packages/app-store/googlecalendar/lib/CalendarService.ts
+++ b/packages/app-store/googlecalendar/lib/CalendarService.ts
@@ -250,7 +250,7 @@ export default class GoogleCalendarService implements Calendar {
const selectedCalendar = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : event.destinationCalendar?.[0]?.externalId;
calendar.events.update(
{
@@ -312,7 +312,7 @@ export default class GoogleCalendarService implements Calendar {
const defaultCalendarId = "primary";
const calendarId = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : event.destinationCalendar?.[0]?.externalId;
calendar.events.delete(
{🤖 Grapple PR auto-fix • minor • Review this diff before applying
| if (evt.location === MeetLocationType && evt.destinationCalendar?.integration !== "google_calendar") { | ||
| // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one | ||
| const [mainHostDestinationCalendar] = evt.destinationCalendar ?? []; | ||
| if (evt.location === MeetLocationType && mainHostDestinationCalendar.integration !== "google_calendar") { |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (14 lines, 1 file) — review recommended)
Inline comment references outdated assumption about destinationCalendar being a single object. Comment says 'we will only check the first one' but provides no context for why this fallback strategy was chosen for multi-host collective events.
--- a/packages/core/EventManager.ts
+++ b/packages/core/EventManager.ts
@@ -114,9 +114,13 @@ export default class EventManager {
}
- // Fallback to Cal Video if Google Meet is selected w/o a Google Cal
- // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one
+ // Fallback to Cal Video if Google Meet is selected without a Google Calendar integration.
+ // We intentionally check only the first (main host) destination calendar here because Google
+ // Meet availability is gated on the organizer's credential — if the primary host lacks a
+ // google_calendar integration, no Google Meet link can be generated regardless of co-host
+ // calendars. This is the correct behavior for collective/multi-host events: the organizer's
+ // calendar is authoritative for video-conferencing fallback decisions.
+ // If the array is empty (no destination calendar configured), treat it as non-Google and fall back.
const [mainHostDestinationCalendar] = evt.destinationCalendar ?? [];
- if (evt.location === MeetLocationType && mainHostDestinationCalendar.integration !== "google_calendar") {
+ if (
+ evt.location === MeetLocationType &&
+ mainHostDestinationCalendar?.integration !== "google_calendar"
+ ) {
evt["location"] = "integrations:daily";
}🤖 Grapple PR auto-fix • minor • Review this diff before applying
2 participants
Benchmark test PR #4 - Replicating Greptile benchmark for Grapple PR evaluation.