OAuth credential sync and app integration enhancements#4
Conversation
…11059) * Add credential sync .env variables * Add webhook to send app credentials * Upsert credentials when webhook called * Refresh oauth token from a specific endpoint * Pass appSlug * Add credential encryption * Move oauth helps into a folder * Create parse token response wrapper * Add OAuth helpers to apps * Clean up * Refactor `appDirName` to `appSlug` * Address feedback * Change to safe parse * Remove console.log --------- Co-authored-by: Syed Ali Shahbaz <52925846+alishaz-polymath@users.noreply.github.com> Co-authored-by: Omar López <zomars@me.com>
🤖 Grapple PR ReviewSummaryThis PR adds 3 files (app-credential.ts, parseRefreshTokenResponse.ts, refreshOAuthTokens.ts), modifies 34 files. Title: "OAuth credential sync and app integration enhancements" Changes: 40 files (+375 / -119 lines)
Resolves: #8 (fix: handle collective multiple host on destinationCalendar) CI: ⏳ 0 passed, 0 failed Impact Diagramgraph TD
pr_0[/"🔀 PR Changes: 40 files"/]
f1["🟡 .env.example +14/-1"]
pr_0 --> f1
f2["🟢 app-credential.ts +93/-0"]
pr_0 --> f2
f3["🟡 createOAuthAppCredential.ts +2/-2"]
pr_0 --> f3
f4["🟡 decodeOAuthState.ts +1/-1"]
pr_0 --> f4
f5["🟡 encodeOAuthState.ts +1/-1"]
pr_0 --> f5
f6["🟢 parseRefreshTokenResponse.ts +32/-0"]
pr_0 --> f6
f7["🟢 refreshOAuthTokens.ts +22/-0"]
pr_0 --> f7
f8["🟡 add.ts +1/-1"]
pr_0 --> f8
f9["🟡 callback.ts +1/-1"]
pr_0 --> f9
f10["🟡 CalendarService.ts +11/-2"]
pr_0 --> f10
f11["🟡 add.ts +1/-1"]
pr_0 --> f11
f12["🟡 callback.ts +2/-2"]
pr_0 --> f12
more_13["... +28 more files"]
pr_0 --> more_13
f3 -.- f4
f4 -.- f5
f5 -.- f6
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 introduces an OAuth credential sync system that allows self-hosted Cal.com instances to share OAuth credentials with a parent application via a webhook endpoint and encrypted credential exchange, while also reorganizing OAuth utility files into a dedicated subdirectory.
Changes: 40 files (+375 / -119 lines)
- 🟢 3 files added:
apps/web/pages/api/webhook/app-credential.ts,packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts,packages/app-store/_utils/oauth/refreshOAuthTokens.ts - 🟡 34 files modified:
.env.example,packages/app-store/googlecalendar/api/add.ts,packages/app-store/googlecalendar/api/callback.ts,packages/app-store/googlecalendar/lib/CalendarService.ts,packages/app-store/hubspot/api/add.ts+29 more
Resolves: #8 (fix: handle collective multiple host on destinationCalendar)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 40 files"/]
f1["🟡 .env.example +14/-1"]
pr_0 --> f1
f2["🟢 app-credential.ts +93/-0"]
pr_0 --> f2
f3["🟡 createOAuthAppCredential.ts +2/-2"]
pr_0 --> f3
f4["🟡 decodeOAuthState.ts +1/-1"]
pr_0 --> f4
f5["🟡 encodeOAuthState.ts +1/-1"]
pr_0 --> f5
f6["🟢 parseRefreshTokenResponse.ts +32/-0"]
pr_0 --> f6
f7["🟢 refreshOAuthTokens.ts +22/-0"]
pr_0 --> f7
f8["🟡 add.ts +1/-1"]
pr_0 --> f8
f9["🟡 callback.ts +1/-1"]
pr_0 --> f9
f10["🟡 CalendarService.ts +11/-2"]
pr_0 --> f10
f11["🟡 add.ts +1/-1"]
pr_0 --> f11
f12["🟡 callback.ts +2/-2"]
pr_0 --> f12
more_13["... +28 more files"]
pr_0 --> more_13
f3 -.- f4
f4 -.- f5
f5 -.- f6
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
6 findings — minor items below.
| Severity | Count |
|---|---|
| 🟡 Minor | 4 |
| 🔵 Info | 2 |
Review details
- Reviewed at: 2026-04-07T18:35:59.063Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 99.0s
- Low-confidence findings suppressed: 1
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| // Assume that any property with a number is the expiry | ||
| [z.string().toString()]: z.number(), | ||
| // Allow other properties in the token response | ||
| [z.string().optional().toString()]: z.unknown().optional(), |
There was a problem hiding this comment.
🟡 MINOR — Naming conventions (confidence: 88%)
Dynamic object keys using z.string().toString() do not work as intended in Zod schema definitions. The expression z.string().toString() evaluates to the string 'ZodString' (the class name), not a pattern matcher. This schema will not validate token response fields correctly.
Evidence:
- Line 7-8: [z.string().toString()]: z.number() - this creates a literal key named 'ZodString', not a dynamic pattern
- Line 9-10: [z.string().optional().toString()]: z.unknown().optional() - same issue, literal key named 'ZodString'
- Zod does not support dynamic computed keys in this way; this pattern appears to be a misunderstanding of how to define flexible schemas
- The intent seems to be 'allow any property with a number value' but the current syntax does not achieve this
Agent: style
| CALCOM_CREDENTIAL_SYNC_ENDPOINT="" | ||
| # Key should match on Cal.com and your application | ||
| # must be 32 bytes for AES256 encryption algorithm | ||
| # You can use: `openssl rand -base64 24` to generate one |
There was a problem hiding this comment.
🟡 MINOR — Documentation (confidence: 88%)
Conflicting documentation: the comment says 'You can use: openssl rand -base64 24 to generate one' (18 bytes) but also says 'must be 32 bytes for AES256 encryption algorithm'. Base64 encoding of 24 random bytes yields a 32-character string, but only 18 bytes of entropy—insufficient for AES-256 which requires 32 bytes of actual key material.
Evidence:
- Line 242: Comment states to use 'openssl rand -base64 24'
- Line 241: Comment states 'must be 32 bytes for AES256'
- openssl rand -base64 24 produces 24 random bytes, which base64-encodes to 32 characters but decodes back to 24 bytes
- AES-256 requires 32 bytes of key material; 24 bytes is insufficient and may cause cryptographic errors or weaker encryption
Agent: style
| process.env.CALCOM_WEBHOOK_SECRET | ||
| ) { | ||
| return res.status(403).json({ message: "Invalid webhook secret" }); | ||
| } |
There was a problem hiding this comment.
🟡 MINOR — Error messages (confidence: 86%)
Missing HTTP method validation. The webhook endpoint does not check if req.method === 'POST', so GET, PUT, DELETE, or other methods will be processed identically. This could allow unintended mutation of credentials via unexpected HTTP verbs.
Evidence:
- No early return or check for req.method before proceeding with credential upsert logic
- The endpoint should enforce POST-only to prevent accidental or malicious use of other HTTP methods
Agent: style
|
|
||
| // Decrypt the keys | ||
| const keys = JSON.parse( | ||
| symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "") |
There was a problem hiding this comment.
🟡 MINOR — Code patterns (confidence: 75%)
Calling symmetricDecrypt with an empty string fallback for the encryption key. If CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY is not set, the code passes an empty string, which could result in silent cryptographic failure (garbage decryption) rather than throwing an error.
Evidence:
- Line 58: process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || '' passes empty string as default
- Empty string is not a valid AES-256 key; symmetricDecrypt should either throw or produce invalid output
- Earlier checks ensure APP_CREDENTIAL_SHARING_ENABLED is true (which requires the env var to be set), but this fallback still creates a safety hole
Agent: style
|
|
||
| if (!refreshTokenResponse.data.refresh_token) { | ||
| refreshTokenResponse.data.refresh_token = "refresh_token"; | ||
| } |
There was a problem hiding this comment.
🔵 INFO — Code patterns (confidence: 76%)
Setting refresh_token to the literal string 'refresh_token' as a placeholder. If this placeholder is persisted to the database, future refresh attempts using this value will fail with the OAuth provider because the string 'refresh_token' is not a valid token.
Evidence:
- Line 26: refreshTokenResponse.data.refresh_token = 'refresh_token' sets a placeholder
- This value is returned and may be persisted to the database via prisma.credential.update()
- Subsequent token refresh calls will attempt to use the string 'refresh_token' as an actual refresh token, which will be rejected by OAuth providers
Agent: style
| format: "json", | ||
| }), | ||
| }); | ||
|
|
There was a problem hiding this comment.
🔵 INFO — Code patterns (confidence: 76%)
Checking response.statusText !== 'OK' to validate HTTP response. In HTTP/2 connections, the statusText property is often empty or not reliably populated. This check may pass on HTTP/2 failures, allowing invalid responses to proceed.
Evidence:
- Line 85: if (response.statusText !== 'OK') — HTTP/2 responses do not reliably include statusText
- Should check response.ok (boolean property) or response.status (numeric code) instead
- This pattern is more reliable and HTTP version-agnostic
Agent: style
| // Assume that any property with a number is the expiry | ||
| [z.string().toString()]: z.number(), | ||
| // Allow other properties in the token response | ||
| [z.string().optional().toString()]: z.unknown().optional(), |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (9 lines, 1 file))
Dynamic object keys using z.string().toString() do not work as intended in Zod schema definitions. The expression z.string().toString() evaluates to the string 'ZodString' (the class name), not a pattern matcher. This schema will not validate token response fields correctly.
| [z.string().optional().toString()]: z.unknown().optional(), | |
| // Assume that any property with a number is the expiry | |
| expires_in: z.number().optional(), | |
| // Allow other properties in the token response | |
| }).passthrough(); |
🤖 Grapple PR auto-fix • minor • confidence: 88%
| CALCOM_CREDENTIAL_SYNC_ENDPOINT="" | ||
| # Key should match on Cal.com and your application | ||
| # must be 32 bytes for AES256 encryption algorithm | ||
| # You can use: `openssl rand -base64 24` to generate one |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (2 lines, 1 file))
Conflicting documentation: the comment says 'You can use:
openssl rand -base64 24to generate one' (18 bytes) but also says 'must be 32 bytes for AES256 encryption algorithm'. Base64 encoding of 24 random bytes yields a 32-character string, but only 18 bytes of entropy—insufficient for AES-256 which requires 32 bytes of actual key material.
| # You can use: `openssl rand -base64 24` to generate one | |
| # You can use: `openssl rand -base64 32` to generate one |
🤖 Grapple PR auto-fix • minor • confidence: 88%
| process.env.CALCOM_WEBHOOK_SECRET | ||
| ) { | ||
| return res.status(403).json({ message: "Invalid webhook secret" }); | ||
| } |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (5 lines, 1 file))
Missing HTTP method validation. The webhook endpoint does not check if req.method === 'POST', so GET, PUT, DELETE, or other methods will be processed identically. This could allow unintended mutation of credentials via unexpected HTTP verbs.
| } | |
| // Only allow POST requests | |
| if (req.method !== "POST") { | |
| return res.status(405).json({ message: "Method Not Allowed" }); | |
| } | |
🤖 Grapple PR auto-fix • minor • confidence: 86%
|
|
||
| // Decrypt the keys | ||
| const keys = JSON.parse( | ||
| symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "") |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (9 lines, 1 file))
Calling symmetricDecrypt with an empty string fallback for the encryption key. If CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY is not set, the code passes an empty string, which could result in silent cryptographic failure (garbage decryption) rather than throwing an error.
| symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "") | |
| const encryptionKey = process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY; | |
| if (!encryptionKey) { | |
| return res | |
| .status(500) | |
| .json({ message: "Server misconfiguration: CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY is not set" }); | |
| } | |
| symmetricDecrypt(reqBody.keys, encryptionKey) |
🤖 Grapple PR auto-fix • minor • confidence: 75%
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR introduces an OAuth credential sync system that allows self-hosters to share OAuth credentials between their existing applications and Cal.com instances via a webhook endpoint, while also reorganizing OAuth utility files into a dedicated subdirectory and adding centralized token refresh logic across all major OAuth integrations.
Changes: 40 files (+375 / -119 lines)
- 🟢 3 files added:
apps/web/pages/api/webhook/app-credential.ts,packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts,packages/app-store/_utils/oauth/refreshOAuthTokens.ts - 🟡 34 files modified:
.env.example,packages/app-store/googlecalendar/api/add.ts,packages/app-store/googlecalendar/api/callback.ts,packages/app-store/googlecalendar/lib/CalendarService.ts,packages/app-store/hubspot/api/add.ts+29 more
Resolves: #8 (fix: handle collective multiple host on destinationCalendar)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 40 files"/]
f1["🟡 .env.example +14/-1"]
pr_0 --> f1
f2["🟢 app-credential.ts +93/-0"]
pr_0 --> f2
f3["🟡 createOAuthAppCredential.ts +2/-2"]
pr_0 --> f3
f4["🟡 decodeOAuthState.ts +1/-1"]
pr_0 --> f4
f5["🟡 encodeOAuthState.ts +1/-1"]
pr_0 --> f5
f6["🟢 parseRefreshTokenResponse.ts +32/-0"]
pr_0 --> f6
f7["🟢 refreshOAuthTokens.ts +22/-0"]
pr_0 --> f7
f8["🟡 add.ts +1/-1"]
pr_0 --> f8
f9["🟡 callback.ts +1/-1"]
pr_0 --> f9
f10["🟡 CalendarService.ts +11/-2"]
pr_0 --> f10
f11["🟡 add.ts +1/-1"]
pr_0 --> f11
f12["🟡 callback.ts +2/-2"]
pr_0 --> f12
more_13["... +28 more files"]
pr_0 --> more_13
f3 -.- f4
f4 -.- f5
f5 -.- f6
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
3 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 3 |
| 🟠 Major | 5 |
| 🟡 Minor | 7 |
| 🔵 Info | 6 |
Review details
- Reviewed at: 2026-04-08T02:35:23.176Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 123.5s
- Low-confidence findings suppressed: 1
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| if (!appMetadata) { | ||
| return res.status(404).json({ message: "App not found. Ensure that you have the correct app slug" }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🔴 CRITICAL — N+1 queries (confidence: 100%)
The webhook handler performs three sequential database queries (user lookup, app lookup, credential lookup) on every request with no batching or caching. While not strictly N+1, the credential findFirst + conditional update/create pattern is a race condition: two concurrent requests for the same userId/appId could both find no credential and both attempt to create, causing a unique constraint violation.
Evidence:
- prisma.user.findUnique at line ~34
- prisma.app.findUnique at line ~41
- prisma.credential.findFirst at line ~56 followed by conditional update/create
- No transaction wrapping the findFirst + update/create, making it non-atomic under concurrent load
Agent: performance
|
|
||
| return new jsforce.Connection({ | ||
| clientId: consumer_key, | ||
| clientSecret: consumer_secret, |
There was a problem hiding this comment.
🔴 CRITICAL — Unnecessary work (confidence: 100%)
The Salesforce CalendarService constructor performs an unconditional HTTP token refresh on every instantiation, regardless of whether the current access token is still valid. CalendarService instances are typically created per-request, meaning every API call to Salesforce triggers a redundant OAuth round-trip to login.salesforce.com before any actual work is done.
Evidence:
- Token refresh fetch at line ~72 is inside the constructor body with no expiry check
- credentialKey.refresh_token is used unconditionally — no comparison against issued_at or expiry time
- Salesforce access tokens are valid for 1–2 hours by default; refreshing on every instantiation wastes that window
- This will cause rate limiting issues at scale and doubles the latency of every Salesforce operation
Agent: performance
| } | ||
| }; | ||
|
|
||
| export default refreshOAuthTokens; |
There was a problem hiding this comment.
🔴 CRITICAL — Unnecessary work (confidence: 100%)
When APP_CREDENTIAL_SHARING_ENABLED is true and CALCOM_CREDENTIAL_SYNC_ENDPOINT is set, refreshOAuthTokens returns a raw fetch Response object. All callers (GoogleCalendar, Office365Calendar, Webex, Zoom, Zoho, Lark, etc.) then call .json() or equivalent on the return value. However when credential sharing is disabled, the local refreshFunction() returns already-parsed data objects, not a Response. This type mismatch will cause runtime errors across all integrations when credential sharing is toggled.
Evidence:
- refreshOAuthTokens line 13:
return responsewhere response is a raw fetch Response - refreshOAuthTokens line 17:
return responsewhere response is the result of refreshFunction() — a parsed object for most callers - office365video/lib/VideoApiAdapter.ts:
const responseBody = await handleErrorsJson(response)— handleErrorsJson calls response.json() which fails on a pre-parsed object - webex/lib/VideoApiAdapter.ts:
const responseBody = await handleWebexResponse(response, credential.id)— same issue - googlecalendar/lib/CalendarService.ts:
const token = res?.data— res is now the return of refreshOAuthTokens, not a googleapis Response with a .data property
Agent: performance
| // Allow other properties in the token response | ||
| [z.string().optional().toString()]: z.unknown().optional(), | ||
| }); | ||
|
|
There was a problem hiding this comment.
🟠 MAJOR — Unnecessary work (confidence: 100%)
minimumTokenResponseSchema uses computed property keys [z.string().toString()] and [z.string().optional().toString()], which both evaluate to the string literal 'ZodString' at runtime. This means the schema has duplicate keys (the second overwrites the first) and does not validate numeric expiry fields as intended. The schema effectively only validates access_token.
Evidence:
- z.string().toString() returns 'ZodString' (the string representation of the Zod type), not a dynamic key
- z.string().optional().toString() also returns 'ZodString'
- Both computed keys resolve to the same literal 'ZodString', so the second definition silently overwrites the first
- The intent was to allow arbitrary numeric properties (expiry fields), but the schema does not achieve this
Agent: performance
| return res.status(403).json({ message: "Invalid webhook secret" }); | ||
| } | ||
|
|
||
| const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body); |
There was a problem hiding this comment.
🟠 MAJOR — Unnecessary work (confidence: 100%)
appCredentialWebhookRequestBodySchema.parse (not safeParse) is called on the raw request body. If the body is malformed or missing required fields, Zod throws a ZodError that is not caught, resulting in an unhandled exception and a 500 response rather than a proper 400. This also means Next.js error handling absorbs the exception, potentially logging sensitive body contents.
Evidence:
- Line 29:
const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body)— .parse throws on validation failure - No try/catch wraps this call
- No error boundary in the handler for Zod errors
- Results in 500 instead of 400 for malformed requests, masking client errors as server errors
Agent: performance
| calcomUserId: userId.toString(), | ||
| appSlug, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🔵 INFO — Error Handling (confidence: 100%)
When CALCOM_CREDENTIAL_SYNC_ENDPOINT returns a non-OK HTTP status, the function returns the failed Response object without error handling. Downstream callers may attempt to call .json() on a failed response, causing parse errors.
Evidence:
- Lines 6-14: fetch response is returned directly without status checking
- No error handling for HTTP errors (4xx, 5xx)
- Callers expect a parsed token object but will receive a failed Response
- Expected behavior notes this as a risk: 'returns the failed Response without any error handling'
Agent: style
| // Assume that any property with a number is the expiry | ||
| [z.string().toString()]: z.number(), | ||
| // Allow other properties in the token response | ||
| [z.string().optional().toString()]: z.unknown().optional(), |
There was a problem hiding this comment.
🔵 INFO — Documentation (confidence: 94%)
The minimumTokenResponseSchema uses computed property keys (z.string().toString() and z.string().optional().toString()) which both evaluate to the literal string 'ZodString'. This does not validate the intended behavior of accepting any string-keyed property with a number value (e.g., expiry fields). The schema validation intent is unclear and may not work as intended.
Evidence:
- Lines 7-10:
[z.string().toString()]: z.number()and[z.string().optional().toString()]: z.unknown().optional() - Both computed keys resolve to 'ZodString' string literal, not dynamic property matching
- Zod doesn't support dynamic key validation this way; the schema likely only validates
access_tokenand ignores other properties - This may silently fail to validate expiry dates, creating false confidence in malformed tokens
Agent: style
| const fetchTokens = await myGoogleAuth.refreshToken(googleCredentials.refresh_token); | ||
| return fetchTokens.res; | ||
| }, | ||
| "google-calendar", |
There was a problem hiding this comment.
🔵 INFO — Code Organization (confidence: 94%)
The code extracts fetchTokens.res assuming myGoogleAuth.refreshToken returns an object with a res property. If the refresh fails or the structure is unexpected, this could return undefined, causing downstream errors.
Evidence:
- Line 87:
const fetchTokens = await myGoogleAuth.refreshToken(...)returns an object - Line 88-89: Wraps in refreshOAuthTokens, then line 94 accesses
res?.data - The original code had
const { res } = await myGoogleAuth.refreshToken()— direct destructuring. - Current code relies on string property access, which may be undefined if the structure changes
Agent: style
| } | ||
|
|
||
| if (!refreshTokenResponse.data.refresh_token) { | ||
| refreshTokenResponse.data.refresh_token = "refresh_token"; |
There was a problem hiding this comment.
🔵 INFO — Error Handling (confidence: 86%)
When a refresh_token is missing from the OAuth response, the code assigns the string literal 'refresh_token' as a placeholder. This string will be persisted to the database and later used in real token refreshes, causing silent failures.
Evidence:
- Lines 24-26 assign
refreshTokenResponse.data.refresh_token = 'refresh_token'when missing - This placeholder string will be stored in credentials and reused in future refresh attempts
- OAuth providers will reject the literal string 'refresh_token' as a token value
- Expected behavior per acceptance criteria acknowledges this edge case as a risk
Agent: style
|
|
||
| import { decodeOAuthState } from "./decodeOAuthState"; | ||
| import { throwIfNotHaveAdminAccessToTeam } from "./throwIfNotHaveAdminAccessToTeam"; | ||
| import { decodeOAuthState } from "../oauth/decodeOAuthState"; |
There was a problem hiding this comment.
🔵 INFO — Code Organization (confidence: 84%)
Import path changed from relative imports in the old location to new ../_utils/oauth/ path. While the change appears correct, verify that all 15+ files importing these functions have been updated to resolve from the new subdirectory location.
Evidence:
- Lines 5-6: Imports updated from
./decodeOAuthStateto../oauth/decodeOAuthState - Multiple files throughout the codebase import these utilities
- Expected behavior: 'All renamed import paths resolve correctly at build time without broken module references'
- Risk: if any file was missed, build will fail or imports will be undefined
Agent: style
| } | ||
| }; | ||
|
|
||
| export default refreshOAuthTokens; |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (10 lines, 1 file))
When APP_CREDENTIAL_SHARING_ENABLED is true and CALCOM_CREDENTIAL_SYNC_ENDPOINT is set, refreshOAuthTokens returns a raw fetch Response object. All callers (GoogleCalendar, Office365Calendar, Webex, Zoom, Zoho, Lark, etc.) then call .json() or equivalent on the return value. However when credential sharing is disabled, the local refreshFunction() returns already-parsed data objects, not a Response. This type mismatch will cause runtime errors across all integrations when credential sharing is toggled.
| export default refreshOAuthTokens; | |
| if (!response.ok) { | |
| throw new Error( | |
| `Credential sync endpoint returned an error: ${response.status} ${response.statusText}` | |
| ); | |
| } | |
| // Parse the JSON body so callers receive a plain object, consistent with | |
| // the refreshFunction() path below. All callers (GoogleCalendar, Office365, | |
| // Webex, Zoom, Zoho, Lark, etc.) expect a parsed object, not a raw Response. | |
| return response.json(); |
🤖 Grapple PR auto-fix • critical • confidence: 100%
| // Allow other properties in the token response | ||
| [z.string().optional().toString()]: z.unknown().optional(), | ||
| }); | ||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (7 lines, 1 file))
minimumTokenResponseSchema uses computed property keys [z.string().toString()] and [z.string().optional().toString()], which both evaluate to the string literal 'ZodString' at runtime. This means the schema has duplicate keys (the second overwrites the first) and does not validate numeric expiry fields as intended. The schema effectively only validates access_token.
| // Allow any additional properties (e.g. numeric expiry fields, refresh_token, etc.) | |
| }).catchall(z.unknown()); |
🤖 Grapple PR auto-fix • major • confidence: 100%
| return res.status(403).json({ message: "Invalid webhook secret" }); | ||
| } | ||
|
|
||
| const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body); |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (9 lines, 1 file))
appCredentialWebhookRequestBodySchema.parse (not safeParse) is called on the raw request body. If the body is malformed or missing required fields, Zod throws a ZodError that is not caught, resulting in an unhandled exception and a 500 response rather than a proper 400. This also means Next.js error handling absorbs the exception, potentially logging sensitive body contents.
| const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body); | |
| // Use safeParse to return a 400 for malformed bodies instead of throwing | |
| // an unhandled ZodError that surfaces as a 500 and may log sensitive body contents. | |
| const parseResult = appCredentialWebhookRequestBodySchema.safeParse(req.body); | |
| if (!parseResult.success) { | |
| return res.status(400).json({ message: "Invalid request body", errors: parseResult.error.issues }); | |
| } | |
| const reqBody = parseResult.data; |
🤖 Grapple PR auto-fix • major • confidence: 100%
| // Can't use prisma upsert as we don't know the id of the credential | ||
| const appCredential = await prisma.credential.findFirst({ | ||
| where: { | ||
| userId: reqBody.userId, |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (5 lines, 1 file))
prisma.credential.findFirst filters on both userId and appId. If the credential table lacks a composite index on (userId, appId), this query will perform a full table scan on every webhook invocation. This endpoint is intended to be called frequently by self-hosters syncing credentials.
| userId: reqBody.userId, | |
| // orderBy ensures the query planner uses the composite index on (userId, appId) | |
| // without ORDER BY, some engines may fall back to a sequential scan on findFirst | |
| orderBy: { | |
| id: "asc", | |
| }, |
🤖 Grapple PR auto-fix • major • confidence: 99%
| return res.status(403).json({ message: "Invalid webhook secret" }); | ||
| } | ||
|
|
||
| const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body); |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (8 lines, 1 file))
appCredentialWebhookRequestBodySchema.parse() will throw an unhandled exception on invalid request bodies. This should use safeParse() and return a 400 response instead of crashing the handler.
| const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body); | |
| const reqBodyParsed = appCredentialWebhookRequestBodySchema.safeParse(req.body); | |
| if (!reqBodyParsed.success) { | |
| return res.status(400).json({ message: "Invalid request body", error: reqBodyParsed.error.message }); | |
| } | |
| const reqBody = reqBodyParsed.data; |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| // Decrypt the keys | ||
| const keys = JSON.parse( | ||
| symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "") | ||
| ); |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (9 lines, 1 file))
symmetricDecrypt is called with an empty string fallback when CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY is missing, which will fail cryptographically. The endpoint should validate that the environment variable is set (it's already checked in APP_CREDENTIAL_SHARING_ENABLED, but this fallback undermines that).
| ); | |
| // Validate the encryption key is present before attempting decryption. | |
| // APP_CREDENTIAL_SHARING_ENABLED should already ensure this is set, but we | |
| // guard explicitly here to surface a clear error instead of a cryptographic failure. | |
| if (!process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY) { | |
| return res.status(500).json({ message: "Server misconfiguration: CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY is not set" }); | |
| } | |
| symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY) |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| body: new URLSearchParams({ | ||
| calcomUserId: userId.toString(), | ||
| appSlug, | ||
| }), |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (5 lines, 1 file))
When CALCOM_CREDENTIAL_SYNC_ENDPOINT returns a non-OK HTTP status (4xx/5xx), refreshOAuthTokens returns the failed Response object without any error handling. Callers then attempt to call .json() on the error response, which may succeed but return an error payload that doesn't match the expected token schema, causing a confusing downstream failure.
| }), | |
| if (!response.ok) { | |
| throw new Error( | |
| `Failed to sync OAuth tokens via CALCOM_CREDENTIAL_SYNC_ENDPOINT: ${response.status} ${response.statusText}` | |
| ); | |
| } |
🤖 Grapple PR auto-fix • minor • confidence: 95%
| // Keys should be AES256 encrypted with the CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY | ||
| keys: z.string(), | ||
| }); | ||
| /** */ |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (6 lines, 1 file))
Webhook endpoint does not validate HTTP method (e.g., only allow POST). Any HTTP method (GET, PUT, etc.) will attempt to process the request.
| /** */ | |
| // Only allow POST requests for this webhook endpoint | |
| if (req.method !== "POST") { | |
| res.setHeader("Allow", "POST"); | |
| return res.status(405).json({ message: "Method Not Allowed" }); | |
| } | |
🤖 Grapple PR auto-fix • minor • confidence: 87%
| if (!appMetadata) { | ||
| return res.status(404).json({ message: "App not found. Ensure that you have the correct app slug" }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (128 lines, 1 file) — review recommended)
The webhook handler performs three sequential database queries (user lookup, app lookup, credential lookup) on every request with no batching or caching. While not strictly N+1, the credential findFirst + conditional update/create pattern is a race condition: two concurrent requests for the same userId/appId could both find no credential and both attempt to create, causing a unique constraint violation.
--- a/apps/web/pages/api/webhook/app-credential.ts
+++ b/apps/web/pages/api/webhook/app-credential.ts
@@ -31,38 +31,54 @@
const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body);
- // Check that the user exists
- const user = await prisma.user.findUnique({ where: { id: reqBody.userId } });
-
- if (!user) {
- return res.status(404).json({ message: "User not found" });
- }
-
- const app = await prisma.app.findUnique({
- where: { slug: reqBody.appSlug },
- select: { slug: true },
- });
-
- if (!app) {
- return res.status(404).json({ message: "App not found" });
- }
-
- // Search for the app's slug and type
- const appMetadata = appStoreMetadata[app.slug as keyof typeof appStoreMetadata];
-
- if (!appMetadata) {
- return res.status(404).json({ message: "App not found. Ensure that you have the correct app slug" });
- }
-
- // Decrypt the keys
- const keys = JSON.parse(
- symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "")
- );
-
- // Can't use prisma upsert as we don't know the id of the credential
- const appCredential = await prisma.credential.findFirst({
- where: {
- userId: reqBody.userId,
- appId: appMetadata.slug,
- },
- select: {
- id: true,
- },
- });
-
- if (appCredential) {
- await prisma.credential.update({
- where: {
- id: appCredential.id,
- },
- data: {
- key: keys,
- },
- });
- return res.status(200).json({ message: `Credentials updated for userId: ${reqBody.userId}` });
- } else {
- await prisma.credential.create({
- data: {
- key: keys,
- userId: reqBody.userId,
- appId: appMetadata.slug,
- type: appMetadata.type,
- },
- });
- return res.status(200).json({ message: `Credentials created for userId: ${reqBody.userId}` });
- }
+ // Run independent DB lookups in parallel to reduce round-trips
+ const [user, app] = await Promise.all([
+ prisma.user.findUnique({ where: { id: reqBody.userId } }),
+ prisma.app.findUnique({
+ where: { slug: reqBody.appSlug },
+ select: { slug: true },
+ }),
+ ]);
+
+ if (!user) {
+ return res.status(404).json({ message: "User not found" });
+ }
+
+ if (!app) {
+ return res.status(404).json({ message: "App not found" });
+ }
+
+ // Search for the app's slug and type (in-memory, no DB call)
+ const appMetadata = appStoreMetadata[app.slug as keyof typeof appStoreMetadata];
+
+ if (!appMetadata) {
+ return res.status(404).json({ message: "App not found. Ensure that you have the correct app slug" });
+ }
+
+ // Decrypt the keys
+ const keys = JSON.parse(
+ symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "")
+ );
+
+ // Use an interactive transaction to make the findFirst + update/create atomic,
+ // preventing a race condition where two concurrent requests for the same
+ // userId/appId both observe no credential and both attempt a create,
+ // which would cause a unique constraint violation.
+ const { message, status } = await prisma.$transaction(async (tx) => {
+ const existingCredential = await tx.credential.findFirst({
+ where: {
+ userId: reqBody.userId,
+ appId: appMetadata.slug,
+ },
+ select: {
+ id: true,
+ },
+ });
+
+ if (existingCredential) {
+ await tx.credential.update({
+ where: {
+ id: existingCredential.id,
+ },
+ data: {
+ key: keys,
+ },
+ });
+ return { status: 200, message: `Credentials updated for userId: ${reqBody.userId}` };
+ }
+
+ await tx.credential.create({
+ data: {
+ key: keys,
+ userId: reqBody.userId,
+ appId: appMetadata.slug,
+ type: appMetadata.type,
+ },
+ });
+ return { status: 200, message: `Credentials created for userId: ${reqBody.userId}` };
+ });
+
+ return res.status(status).json({ message });
}🤖 Grapple PR auto-fix • critical • Review this diff before applying
|
|
||
| return new jsforce.Connection({ | ||
| clientId: consumer_key, | ||
| clientSecret: consumer_secret, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (25 lines, 1 file) — review recommended)
The Salesforce CalendarService constructor performs an unconditional HTTP token refresh on every instantiation, regardless of whether the current access token is still valid. CalendarService instances are typically created per-request, meaning every API call to Salesforce triggers a redundant OAuth round-trip to login.salesforce.com before any actual work is done.
--- a/packages/app-store/salesforce/lib/CalendarService.ts
+++ b/packages/app-store/salesforce/lib/CalendarService.ts
@@ -36,6 +36,9 @@ const sfApiErrors = {
INVALID_EVENTWHOIDS: "INVALID_FIELD: No such column 'EventWhoIds' on sobject of type Event",
};
+// Salesforce access tokens are valid for 1–2 hours; refresh 5 minutes before expiry
+const TOKEN_EXPIRY_BUFFER_MS = 5 * 60 * 1000;
+const SALESFORCE_TOKEN_LIFETIME_MS = 60 * 60 * 1000; // 1 hour (conservative default)
+
const salesforceTokenSchema = z.object({
id: z.string(),
issued_at: z.string(),
@@ -70,6 +74,24 @@ export default class SalesforceCalendarService implements Calendar {
const credentialKey = credential.key as unknown as ExtendedTokenResponse;
+ // Skip the refresh round-trip if the existing access token is still valid.
+ // `issued_at` is a Unix timestamp in milliseconds as returned by Salesforce.
+ // Note: if `issued_at` is missing or unparseable we fall through to a full refresh.
+ if (credentialKey.issued_at && credentialKey.access_token) {
+ const issuedAt = parseInt(credentialKey.issued_at, 10);
+ if (!isNaN(issuedAt)) {
+ const tokenAge = Date.now() - issuedAt;
+ if (tokenAge < SALESFORCE_TOKEN_LIFETIME_MS - TOKEN_EXPIRY_BUFFER_MS) {
+ // Token is still fresh — reuse it without an OAuth round-trip
+ return new jsforce.Connection({
+ clientId: consumer_key,
+ clientSecret: consumer_secret,
+ redirectUri: WEBAPP_URL + "/api/integrations/salesforce/callback",
+ instanceUrl: credentialKey.instance_url,
+ accessToken: credentialKey.access_token,
+ refreshToken: credentialKey.refresh_token,
+ });
+ }
+ }
+ }
+
const response = await fetch("https://login.salesforce.com/services/oauth2/token", {
method: "POST",
body: new URLSearchParams({🤖 Grapple PR auto-fix • critical • Review this diff before applying
| // Decrypt the keys | ||
| const keys = JSON.parse( | ||
| symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "") | ||
| ); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (13 lines, 1 file) — review recommended)
symmetricDecrypt is called with an empty string fallback for CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY when the env var is unset. The APP_CREDENTIAL_SHARING_ENABLED guard earlier in the handler only checks that CALCOM_WEBHOOK_SECRET and CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY are both set, but if the constant evaluation is stale or the check is bypassed, symmetricDecrypt('', '') will throw a cryptographic error (invalid key length for AES-256) rather than returning a meaningful 4xx response.
--- a/apps/web/pages/api/webhook/app-credential.ts
+++ b/apps/web/pages/api/webhook/app-credential.ts
@@ -54,8 +54,16 @@
// Decrypt the keys
- const keys = JSON.parse(
- symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "")
- );
+ // Defensively validate the encryption key at runtime: APP_CREDENTIAL_SHARING_ENABLED
+ // is a compile-time constant and may be stale; an empty key passed to symmetricDecrypt
+ // causes AES-256 to throw a cryptographic error (invalid key length) rather than a
+ // meaningful HTTP response.
+ const encryptionKey = process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY;
+ if (!encryptionKey) {
+ return res.status(500).json({ message: "Server misconfiguration: CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY is not set" });
+ }
+
+ const keys = JSON.parse(symmetricDecrypt(reqBody.keys, encryptionKey));🤖 Grapple PR auto-fix • major • Review this diff before applying
| } | ||
| await prisma.credential.update({ | ||
| where: { | ||
| id: credential.id, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (11 lines, 1 file) — review recommended)
After parseRefreshTokenResponse is called, the code does
o365AuthCredentials = { ...o365AuthCredentials, ...(tokenResponse.success && tokenResponse.data) }. parseRefreshTokenResponse throws on failure, so tokenResponse.success is always true at this point — the spread oftokenResponse.datais safe but the.successcheck is dead code. More critically, the removed error logging means token refresh failures are now completely silent (the thrown error propagates up with no context about the MS response body).
--- a/packages/app-store/office365calendar/lib/CalendarService.ts
+++ b/packages/app-store/office365calendar/lib/CalendarService.ts
@@ -258,7 +258,14 @@ export default class Office365CalendarService implements Calendar {
credential.userId
);
const responseJson = await handleErrorsJson(response);
- const tokenResponse = parseRefreshTokenResponse(responseJson, refreshTokenResponseSchema);
- o365AuthCredentials = { ...o365AuthCredentials, ...(tokenResponse.success && tokenResponse.data) };
+ const tokenResponse = (() => {
+ try {
+ return parseRefreshTokenResponse(responseJson, refreshTokenResponseSchema);
+ } catch (e) {
+ logger.error("Outlook error grabbing new tokens ~ MS response:", responseJson, e);
+ throw e;
+ }
+ })();
+ o365AuthCredentials = { ...o365AuthCredentials, ...tokenResponse.data };
await prisma.credential.update({
where: {
id: credential.id,🤖 Grapple PR auto-fix • major • Review this diff before applying
| calcomUserId: userId.toString(), | ||
| appSlug, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (38 lines, 1 file) — review recommended)
When APP_CREDENTIAL_SHARING_ENABLED is true, refreshOAuthTokens makes an unbounded number of concurrent outbound requests to CALCOM_CREDENTIAL_SYNC_ENDPOINT with no rate limiting, timeout, or circuit breaker. Under load (many simultaneous calendar syncs), this can exhaust the external endpoint or connection pool.
--- a/packages/app-store/_utils/oauth/refreshOAuthTokens.ts
+++ b/packages/app-store/_utils/oauth/refreshOAuthTokens.ts
@@ -1,22 +1,36 @@
import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants";
+// Default timeout for credential sync endpoint requests (10 seconds).
+// Override via CALCOM_CREDENTIAL_SYNC_ENDPOINT_TIMEOUT_MS env var.
+// Without a timeout, a slow or unresponsive endpoint will hang every calendar
+// operation that requires a token refresh indefinitely.
+const CREDENTIAL_SYNC_TIMEOUT_MS = process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT_TIMEOUT_MS
+ ? parseInt(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT_TIMEOUT_MS, 10)
+ : 10_000;
+
const refreshOAuthTokens = async (refreshFunction: () => any, appSlug: string, userId: number | null) => {
// Check that app syncing is enabled and that the credential belongs to a user
if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId) {
// Customize the payload based on what your endpoint requires
// The response should only contain the access token and expiry date
- const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
- method: "POST",
- body: new URLSearchParams({
- calcomUserId: userId.toString(),
- appSlug,
- }),
- });
- return response;
+ try {
+ const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
+ method: "POST",
+ body: new URLSearchParams({
+ calcomUserId: userId.toString(),
+ appSlug,
+ }),
+ // AbortSignal.timeout() cancels the request after CREDENTIAL_SYNC_TIMEOUT_MS ms,
+ // preventing the connection pool from being exhausted when the endpoint is slow.
+ signal: AbortSignal.timeout(CREDENTIAL_SYNC_TIMEOUT_MS),
+ });
+ return response;
+ } catch (err) {
+ // If the sync endpoint times out or is unreachable, fall back to the local
+ // refresh function so calendar operations can continue rather than hang.
+ console.error(
+ `refreshOAuthTokens: credential sync endpoint failed (appSlug=${appSlug}, userId=${userId}), falling back to local refresh.`,
+ err
+ );
+ const response = await refreshFunction();
+ return response;
+ }
} else {
const response = await refreshFunction();
return response;
}
};
export default refreshOAuthTokens;🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
|
||
| await prisma.credential.update({ | ||
| where: { id: credential.id }, | ||
| data: { key: { ...accessTokenParsed.data, refresh_token: credentialKey.refresh_token } }, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (19 lines, 1 file) — review recommended)
Salesforce CalendarService performs a token refresh during every instantiation without checking if the token is expired. This will cause unnecessary API calls and potential rate limiting issues.
--- a/packages/app-store/salesforce/lib/CalendarService.ts
+++ b/packages/app-store/salesforce/lib/CalendarService.ts
@@ -20,7 +20,8 @@ import getAppKeysFromSlug from "../../_utils/getAppKeysFromSlug";
type ExtendedTokenResponse = TokenResponse & {
instance_url: string;
+ issued_at: string;
};
const salesforceTokenSchema = z.object({
@@ -68,6 +68,18 @@ export default class SalesforceCalendarService implements Calendar {
const credentialKey = credential.key as unknown as ExtendedTokenResponse;
+ // Skip refresh if the token was issued recently (within the last hour).
+ // Salesforce access tokens default to a 1-hour session timeout.
+ // `issued_at` is a Unix timestamp in milliseconds provided by Salesforce's OAuth endpoint.
+ const TOKEN_EXPIRY_BUFFER_MS = 60 * 60 * 1000; // 1 hour
+ const issuedAt = credentialKey.issued_at ? parseInt(credentialKey.issued_at, 10) : 0;
+ const isTokenValid = issuedAt > 0 && Date.now() - issuedAt < TOKEN_EXPIRY_BUFFER_MS;
+
+ if (isTokenValid) {
+ return new jsforce.Connection({
+ clientId: consumer_key,
+ clientSecret: consumer_secret,
+ redirectUri: WEBAPP_URL + "/api/integrations/salesforce/callback",
+ instanceUrl: credentialKey.instance_url,
+ accessToken: credentialKey.access_token,
+ refreshToken: credentialKey.refresh_token,
+ });
+ }
+
const response = await fetch("https://login.salesforce.com/services/oauth2/token", {
method: "POST",
body: new URLSearchParams({🤖 Grapple PR auto-fix • minor • Review this diff before applying
| // Search for the app's slug and type | ||
| const appMetadata = appStoreMetadata[app.slug as keyof typeof appStoreMetadata]; | ||
|
|
||
| if (!appMetadata) { |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (15 lines, 1 file) — review recommended)
The webhook performs a prisma.app.findUnique followed by an appStoreMetadata lookup on every request. The app table and appStoreMetadata are both essentially static configuration — they change only on deployments. These lookups add unnecessary DB and memory overhead per webhook call.
--- a/apps/web/pages/api/webhook/app-credential.ts
+++ b/apps/web/pages/api/webhook/app-credential.ts
@@ -31,19 +31,13 @@
const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body);
// Check that the user exists
const user = await prisma.user.findUnique({ where: { id: reqBody.userId } });
if (!user) {
return res.status(404).json({ message: "User not found" });
}
- const app = await prisma.app.findUnique({
- where: { slug: reqBody.appSlug },
- select: { slug: true },
- });
-
- if (!app) {
- return res.status(404).json({ message: "App not found" });
- }
-
- // Search for the app's slug and type
- const appMetadata = appStoreMetadata[app.slug as keyof typeof appStoreMetadata];
+ // Validate app slug directly against static appStoreMetadata — avoids a DB
+ // round-trip since appStoreMetadata is the authoritative source for app config
+ // and only changes on deployments, not at runtime.
+ const appMetadata = appStoreMetadata[reqBody.appSlug as keyof typeof appStoreMetadata];
if (!appMetadata) {
return res.status(404).json({ message: "App not found. Ensure that you have the correct app slug" });
}🤖 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 introduces an OAuth credential sync system that allows self-hosters to share OAuth credentials between their existing applications and Cal.com, and reorganizes OAuth utility files into a dedicated subdirectory. It adds a webhook endpoint for credential synchronization, utility functions for refreshing OAuth tokens via an external endpoint, and updates all OAuth app integrations to use the new shared utilities.
Changes: 40 files (+375 / -119 lines)
- 🟢 3 files added:
apps/web/pages/api/webhook/app-credential.ts,packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts,packages/app-store/_utils/oauth/refreshOAuthTokens.ts - 🟡 34 files modified:
.env.example,packages/app-store/googlecalendar/api/add.ts,packages/app-store/googlecalendar/api/callback.ts,packages/app-store/googlecalendar/lib/CalendarService.ts,packages/app-store/hubspot/api/add.ts+29 more
Resolves: #8 (fix: handle collective multiple host on destinationCalendar)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 40 files"/]
f1["🟡 .env.example +14/-1"]
pr_0 --> f1
f2["🟢 app-credential.ts +93/-0"]
pr_0 --> f2
f3["🟡 createOAuthAppCredential.ts +2/-2"]
pr_0 --> f3
f4["🟡 decodeOAuthState.ts +1/-1"]
pr_0 --> f4
f5["🟡 encodeOAuthState.ts +1/-1"]
pr_0 --> f5
f6["🟢 parseRefreshTokenResponse.ts +32/-0"]
pr_0 --> f6
f7["🟢 refreshOAuthTokens.ts +22/-0"]
pr_0 --> f7
f8["🟡 add.ts +1/-1"]
pr_0 --> f8
f9["🟡 callback.ts +1/-1"]
pr_0 --> f9
f10["🟡 CalendarService.ts +11/-2"]
pr_0 --> f10
f11["🟡 add.ts +1/-1"]
pr_0 --> f11
f12["🟡 callback.ts +2/-2"]
pr_0 --> f12
more_13["... +28 more files"]
pr_0 --> more_13
f3 -.- f4
f4 -.- f5
f5 -.- f6
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 | 3 |
Review details
- Reviewed at: 2026-04-12T02:05:18.608Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 126.5s
- Low-confidence findings suppressed: 2
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| [z.string().toString()]: z.number(), | ||
| // Allow other properties in the token response | ||
| [z.string().optional().toString()]: z.unknown().optional(), | ||
| }); |
There was a problem hiding this comment.
🔴 CRITICAL — Schema Bug - Computed Property Keys (confidence: 100%)
The minimumTokenResponseSchema uses z.string().toString() and z.string().optional().toString() as computed property keys. Both evaluate to the string representation of the Zod type (e.g., 'ZodString'), not dynamic property names. This means the schema defines a single static property key 'ZodString' (overwritten by the second declaration), not a catch-all for arbitrary string keys. The schema will only validate access_token and a literal property named after Zod's internal toString output, which is almost certainly not the intent. This will cause minimumTokenResponseSchema.safeParse() to fail on real token responses that have properties like expires_in, leading to 'Invalid refreshed tokens were returned' errors in production when credential sharing is enabled.
Evidence:
- z.string().toString() in Node.js returns Zod's internal string representation, not a dynamic key placeholder
- Both computed keys likely resolve to the same string, so the second overwrites the first
- z.object() does not support dynamic/catch-all keys via computed property syntax; z.record() or z.passthrough() would be needed
Agent: architecture
|
|
||
| return refreshTokenResponse; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🔴 CRITICAL — API Contract Violation - Return Type Mismatch (confidence: 100%)
parseRefreshTokenResponse returns the result of safeParse() (an object with .success and .data properties), but on line 21 it throws when !refreshTokenResponse.success. This means the function can never return a failed result — it always either returns a successful SafeParseReturnType or throws. However, callers like office365calendar/lib/CalendarService.ts (line 266) still use tokenResponse.success && tokenResponse.data in a spread, and salesforce/lib/CalendarService.ts (line 94) checks if (!accessTokenParsed.success). The Salesforce check is dead code (the throw prevents it from being reached), and the Office365 pattern is misleading. More critically, the old Office365 code had a fallback that logged and continued on parse failure — now it will throw and crash the refresh flow entirely.
Evidence:
- office365calendar CalendarService line:
o365AuthCredentials = { ...o365AuthCredentials, ...(tokenResponse.success && tokenResponse.data) }; - salesforce CalendarService line:
if (!accessTokenParsed.success) { return Promise.reject(...) } - parseRefreshTokenResponse throws on line 22 before returning a failed result
Agent: architecture
| } | ||
| }; | ||
|
|
||
| export default refreshOAuthTokens; |
There was a problem hiding this comment.
🔴 CRITICAL — API Contract Violation - Response Type Mismatch (confidence: 100%)
When credential sharing is enabled (APP_CREDENTIAL_SHARING_ENABLED && CALCOM_CREDENTIAL_SYNC_ENDPOINT), refreshOAuthTokens returns a raw fetch Response object. When disabled, it returns whatever the refreshFunction callback returns, which varies by caller (e.g., Google returns GaxiosResponse.res, HubSpot returns a HubspotToken object, Office365/Webex return a fetch Response). Callers then process the result as if it's always the type their original refresh function returned. For example, googlecalendar/CalendarService.ts accesses res?.data on the result — a raw fetch Response has no .data property. hubspot/CalendarService.ts treats the return as HubspotToken and accesses .accessToken and .expiresIn. This will cause runtime errors or silent failures when credential sharing is enabled.
Evidence:
- refreshOAuthTokens line 14:
return response;— this is a fetch Response - googlecalendar CalendarService line 92:
const token = res?.data;— fetch Response has no.data - hubspot CalendarService: treats return value as HubspotToken object with
.accessToken,.expiresIn,.refreshToken
Agent: architecture
|
|
||
| return new jsforce.Connection({ | ||
| clientId: consumer_key, | ||
| clientSecret: consumer_secret, |
There was a problem hiding this comment.
🔴 CRITICAL — N+1 queries / unnecessary work (confidence: 100%)
Token refresh via HTTP POST to Salesforce OAuth endpoint happens unconditionally every time a SalesforceCalendarService instance is constructed, even if the token is still valid. Calendar service instances are typically created per-request, so every calendar operation (list events, create event, etc.) will fire an unnecessary token refresh API call to Salesforce before doing any real work.
Evidence:
- The refresh fetch is placed directly in the constructor body (inside the async IIFE that creates this.conn), with no expiry check.
- Other adapters (e.g. Google, Office365) only refresh inside a conditional check against expiry_date or similar.
- Salesforce has rate limits on OAuth token endpoints; unconditional refresh on every constructor call will exhaust them under moderate load.
Agent: performance
| } | ||
| }; | ||
|
|
||
| export default refreshOAuthTokens; |
There was a problem hiding this comment.
🔴 CRITICAL — Unnecessary work / broken return value (confidence: 100%)
When credential sharing is enabled, refreshOAuthTokens returns a raw fetch Response object instead of a parsed JSON body. All callers (office365video, webex, zoomvideo, zoho-bigin, zohocrm, hubspot, lark, google-calendar) then call .json(), .data, or pass the result to schema parsers expecting a plain object. This will cause runtime exceptions on every token refresh attempt when APP_CREDENTIAL_SHARING_ENABLED is true.
Evidence:
- Line 13-15:
const response = await fetch(...); return response;— a Response object is returned. - office365video/lib/VideoApiAdapter.ts:
const responseBody = await handleErrorsJson(response)— handleErrorsJson calls response.json() but a second .json() call on an already-consumed Response throws. - webex/lib/VideoApiAdapter.ts:
const responseBody = await handleWebexResponse(response, credential.id)— same pattern. - zoomvideo/lib/VideoApiAdapter.ts calls parseRefreshTokenResponse on the result, expecting a plain object.
- googlecalendar/lib/CalendarService.ts:
const token = res?.data— a Response has no .data property, so token is undefined and access_token assignment silently produces undefined.
Agent: performance
| userId: reqBody.userId, | ||
| appId: appMetadata.slug, | ||
| }, | ||
| select: { |
There was a problem hiding this comment.
🟠 MAJOR — Missing index / inefficient query (confidence: 100%)
prisma.credential.findFirst filters on (userId, appId) composite fields. If the credential table does not have a composite index on (userId, appId), this will result in a full table scan for every webhook invocation. Credentials can be numerous and this is a write-path endpoint called during token sync.
Evidence:
- findFirst with WHERE userId AND appId — no ORDER BY means the database must scan to find the first match.
- Prisma schema for Credential typically only has a primary key index on id; composite (userId, appId) index is non-obvious.
- This is a public-facing endpoint with no rate limiting, amplifying the impact of slow queries.
Agent: performance
| [z.string().toString()]: z.number(), | ||
| // Allow other properties in the token response | ||
| [z.string().optional().toString()]: z.unknown().optional(), | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR — Algorithmic complexity / schema bug causing unnecessary validation failure (confidence: 100%)
The minimumTokenResponseSchema uses computed property keys [z.string().toString()] and [z.string().optional().toString()]. Both z.string().toString() and z.string().optional().toString() evaluate to the string 'ZodString' at runtime, so the schema object has a duplicate key 'ZodString' and the second definition overwrites the first. The resulting schema validates only { access_token: string, ZodString: unknown } — not the intended dynamic expiry number field. When credential sharing is enabled, all token responses that lack a literal 'ZodString' key will fail validation, causing refreshOAuthTokens to throw on every call.
Evidence:
- JavaScript object literal with duplicate computed keys: the second
[z.string().optional().toString()]key overwrites the first[z.string().toString()]key since both evaluate to 'ZodString'. - z.ZodType.toString() is not a Zod API for schema composition; it returns the class name string.
- minimumTokenResponseSchema.safeParse({ access_token: 'tok', expires_in: 3600 }) will fail because 'ZodString' key is missing.
Agent: performance
| where: { id: credential.id }, | ||
| data: { key: { ...accessTokenParsed.data, refresh_token: credentialKey.refresh_token } }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🟡 MINOR — Unnecessary work (confidence: 100%)
parseRefreshTokenResponse throws on failure (as documented in the PR), but the Salesforce code checks if (!accessTokenParsed.success) afterwards as if it uses safeParse semantics. This dead code path will never execute, and the success check is misleading. More importantly, parseRefreshTokenResponse returns the safeParse result object (with .data), not the raw parsed data, but it is then spread into the credential update — the credential will be stored with the Zod wrapper structure.
Evidence:
- parseRefreshTokenResponse throws when !refreshTokenResponse.success, so the returned value always has .success === true.
- The return value is
refreshTokenResponse(the safeParse result), notrefreshTokenResponse.data. - Storing
{ ...accessTokenParsed.data, refresh_token: ... }is correct only if callers access .data — other callers like googlecalendar doconst key = parseRefreshTokenResponse(...)and pass key directly to prisma, which would store the wrapper object.
Agent: performance
| }); | ||
| return res.status(200).json({ message: `Credentials created for userId: ${reqBody.userId}` }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MINOR — Concurrency / unbounded parallelism (confidence: 97%)
The webhook endpoint at /api/webhook/app-credential has no rate limiting. It accepts unauthenticated POST requests (secret header aside), performs multiple database queries (user lookup, app lookup, credential findFirst, credential update/create), and calls symmetricDecrypt on arbitrary input. Without rate limiting, this endpoint can be used to cause significant database load.
Evidence:
- No rate limiting middleware applied to the handler.
- Each request performs 3-4 database operations.
- The secret header check is a constant-time string comparison but still allows an attacker to attempt brute-forcing or DoS the DB with parallel requests using wrong secrets (which are rejected after the DB lookups... actually before, but the pattern is concerning for future changes).
Agent: performance
| ); | ||
| } | ||
| await prisma.credential.update({ | ||
| where: { |
There was a problem hiding this comment.
🟡 MINOR — Unnecessary work (confidence: 93%)
After the refactor, tokenResponse.success is referenced in the spread expression tokenResponse.success && tokenResponse.data, but parseRefreshTokenResponse now throws on failure and returns the safeParse result object. If parseRefreshTokenResponse returns the raw data (as it should), the .success property check is meaningless and always evaluates to undefined (falsy), causing the spread to always contribute nothing and the credentials to never be updated in memory.
Evidence:
- parseRefreshTokenResponse either throws or returns a value; it does not return a failed result.
- If return value is the safeParse result:
tokenResponse.successis true andtokenResponse.datacontains the parsed data — but then o365AuthCredentials is spread with the Zod wrapper, not the actual token fields. - If return value is the raw data:
tokenResponse.successis undefined (falsy) and the spread always evaluates to false, so credentials in memory are never updated, causing re-authentication failures on subsequent API calls.
Agent: performance
| } | ||
| }; | ||
|
|
||
| export default refreshOAuthTokens; |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (10 lines, 1 file))
When credential sharing is enabled, refreshOAuthTokens returns a raw fetch Response object instead of a parsed JSON body. All callers (office365video, webex, zoomvideo, zoho-bigin, zohocrm, hubspot, lark, google-calendar) then call .json(), .data, or pass the result to schema parsers expecting a plain object. This will cause runtime exceptions on every token refresh attempt when APP_CREDENTIAL_SHARING_ENABLED is true.
| export default refreshOAuthTokens; | |
| if (!response.ok) { | |
| throw new Error( | |
| `Credential sync endpoint returned an error: ${response.status} ${response.statusText}` | |
| ); | |
| } | |
| // Parse the JSON body so callers receive a plain object, not a Response instance. | |
| // All callers (office365video, webex, zoomvideo, zohocrm, hubspot, lark, google-calendar) | |
| // expect a plain object: they call .json(), access .data, or pass to schema parsers. | |
| return response.json(); |
🤖 Grapple PR auto-fix • critical • confidence: 100%
| }); | ||
| return res.status(200).json({ message: `Credentials created for userId: ${reqBody.userId}` }); | ||
| } | ||
| } |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (6 lines, 1 file))
The webhook handler does not validate the HTTP method. It should only accept POST requests, but will currently process GET, PUT, DELETE, etc. as well. A GET request with query parameters could potentially trigger the Zod parse (which would fail), but a PUT or PATCH with a body could succeed. This is a public-facing endpoint that writes credentials to the database.
| } | |
| // Only accept POST requests — this endpoint performs database writes | |
| if (req.method !== "POST") { | |
| res.setHeader("Allow", ["POST"]); | |
| return res.status(405).json({ message: `Method ${req.method} Not Allowed` }); | |
| } | |
🤖 Grapple PR auto-fix • major • confidence: 100%
| CALCOM_WEBHOOK_HEADER_NAME="calcom-webhook-secret" | ||
| CALCOM_CREDENTIAL_SYNC_ENDPOINT="" | ||
| # Key should match on Cal.com and your application | ||
| # must be 32 bytes for AES256 encryption algorithm |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (2 lines, 1 file))
The comment says the key 'must be 32 bytes for AES256 encryption algorithm' but the generation command
openssl rand -base64 24produces 24 random bytes encoded as 32 base64 characters. IfsymmetricDecryptexpects a raw 32-byte key and decodes from base64, it will receive 24 bytes (AES-192, not AES-256). If it uses the base64 string directly as 32 ASCII characters, the key space is limited. Either way, the documentation is contradictory and will likely cause decryption failures or security mismatches between Cal.com and the external application.
| # must be 32 bytes for AES256 encryption algorithm | |
| # You can use: `openssl rand -base64 32` to generate one |
🤖 Grapple PR auto-fix • major • confidence: 100%
| userId: reqBody.userId, | ||
| appId: appMetadata.slug, | ||
| }, | ||
| select: { |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (2 lines, 1 file))
prisma.credential.findFirst filters on (userId, appId) composite fields. If the credential table does not have a composite index on (userId, appId), this will result in a full table scan for every webhook invocation. Credentials can be numerous and this is a write-path endpoint called during token sync.
| select: { | |
| @@index([userId, appId]) |
🤖 Grapple PR auto-fix • major • confidence: 100%
| where: { id: credential.id }, | ||
| data: { key: { ...accessTokenParsed.data, refresh_token: credentialKey.refresh_token } }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (5 lines, 1 file))
parseRefreshTokenResponse throws on failure (as documented in the PR), but the Salesforce code checks
if (!accessTokenParsed.success)afterwards as if it uses safeParse semantics. This dead code path will never execute, and the success check is misleading. More importantly, parseRefreshTokenResponse returns the safeParse result object (with .data), not the raw parsed data, but it is then spread into the credential update — the credential will be stored with the Zod wrapper structure.
| // parseRefreshTokenResponse throws on invalid tokens, so accessTokenParsed.data is always defined here |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| ); | ||
| } | ||
| await prisma.credential.update({ | ||
| where: { |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (2 lines, 1 file))
After the refactor,
tokenResponse.successis referenced in the spread expressiontokenResponse.success && tokenResponse.data, but parseRefreshTokenResponse now throws on failure and returns the safeParse result object. If parseRefreshTokenResponse returns the raw data (as it should), the.successproperty check is meaningless and always evaluates toundefined(falsy), causing the spread to always contribute nothing and the credentials to never be updated in memory.
| where: { | |
| o365AuthCredentials = { ...o365AuthCredentials, ...tokenResponse }; |
🤖 Grapple PR auto-fix • minor • confidence: 93%
| [z.string().toString()]: z.number(), | ||
| // Allow other properties in the token response | ||
| [z.string().optional().toString()]: z.unknown().optional(), | ||
| }); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (16 lines, 1 file) — review recommended)
The
minimumTokenResponseSchemausesz.string().toString()andz.string().optional().toString()as computed property keys. Both evaluate to the string representation of the Zod type (e.g.,'ZodString'), not dynamic property names. This means the schema defines a single static property key'ZodString'(overwritten by the second declaration), not a catch-all for arbitrary string keys. The schema will only validateaccess_tokenand a literal property named after Zod's internal toString output, which is almost certainly not the intent. This will causeminimumTokenResponseSchema.safeParse()to fail on real token responses that have properties likeexpires_in, leading to 'Invalid refreshed tokens were returned' errors in production when credential sharing is enabled.
--- a/packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts
+++ b/packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts
@@ -1,11 +1,11 @@
import { z } from "zod";
import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants";
-const minimumTokenResponseSchema = z.object({
- access_token: z.string(),
- // Assume that any property with a number is the expiry
- [z.string().toString()]: z.number(),
- // Allow other properties in the token response
- [z.string().optional().toString()]: z.unknown().optional(),
-});
+// Require access_token at minimum; passthrough allows any additional properties
+// (e.g. expires_in, refresh_token, token_type) from real OAuth provider responses
+// to pass validation without being stripped. z.object() does not support
+// dynamic/catch-all keys — use .passthrough() instead of computed property syntax.
+const minimumTokenResponseSchema = z
+ .object({
+ access_token: z.string(),
+ })
+ .passthrough();
const parseRefreshTokenResponse = (response: any, schema: z.ZodTypeAny) => {🤖 Grapple PR auto-fix • critical • Review this diff before applying
|
|
||
| return refreshTokenResponse; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (12 lines, 1 file) — review recommended)
parseRefreshTokenResponsereturns the result ofsafeParse()(an object with.successand.dataproperties), but on line 21 it throws when!refreshTokenResponse.success. This means the function can never return a failed result — it always either returns a successful SafeParseReturnType or throws. However, callers likeoffice365calendar/lib/CalendarService.ts(line 266) still usetokenResponse.success && tokenResponse.datain a spread, andsalesforce/lib/CalendarService.ts(line 94) checksif (!accessTokenParsed.success). The Salesforce check is dead code (the throw prevents it from being reached), and the Office365 pattern is misleading. More critically, the old Office365 code had a fallback that logged and continued on parse failure — now it will throw and crash the refresh flow entirely.
--- a/packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts
+++ b/packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts
@@ -13,18 +13,18 @@ const minimumTokenResponseSchema = z.object({
const parseRefreshTokenResponse = (response: any, schema: z.ZodTypeAny) => {
let refreshTokenResponse;
if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT) {
refreshTokenResponse = minimumTokenResponseSchema.safeParse(response);
} else {
refreshTokenResponse = schema.safeParse(response);
}
- if (!refreshTokenResponse.success) {
- throw new Error("Invalid refreshed tokens were returned");
- }
-
- if (!refreshTokenResponse.data.refresh_token) {
- refreshTokenResponse.data.refresh_token = "refresh_token";
+ // Only apply the refresh_token default when parsing succeeded.
+ // Callers (e.g. salesforce CalendarService) check `.success` themselves
+ // and must receive a failed result rather than a thrown exception so they
+ // can handle the error gracefully via their own rejection path.
+ if (refreshTokenResponse.success && !refreshTokenResponse.data.refresh_token) {
+ refreshTokenResponse.data.refresh_token = "refresh_token";
}
return refreshTokenResponse;
};
export default parseRefreshTokenResponse;🤖 Grapple PR auto-fix • critical • Review this diff before applying
| } | ||
| }; | ||
|
|
||
| export default refreshOAuthTokens; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (20 lines, 1 file) — review recommended)
When credential sharing is enabled (
APP_CREDENTIAL_SHARING_ENABLED && CALCOM_CREDENTIAL_SYNC_ENDPOINT),refreshOAuthTokensreturns a rawfetchResponseobject. When disabled, it returns whatever therefreshFunctioncallback returns, which varies by caller (e.g., Google returnsGaxiosResponse.res, HubSpot returns aHubspotTokenobject, Office365/Webex return afetchResponse). Callers then process the result as if it's always the type their original refresh function returned. For example,googlecalendar/CalendarService.tsaccessesres?.dataon the result — a raw fetch Response has no.dataproperty.hubspot/CalendarService.tstreats the return asHubspotTokenand accesses.accessTokenand.expiresIn. This will cause runtime errors or silent failures when credential sharing is enabled.
--- a/packages/app-store/_utils/oauth/refreshOAuthTokens.ts
+++ b/packages/app-store/_utils/oauth/refreshOAuthTokens.ts
@@ -1,22 +1,32 @@
import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants";
-const refreshOAuthTokens = async (refreshFunction: () => any, appSlug: string, userId: number | null) => {
+const refreshOAuthTokens = async (
+ refreshFunction: () => any,
+ appSlug: string,
+ userId: number | null
+) => {
// Check that app syncing is enabled and that the credential belongs to a user
if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId) {
// Customize the payload based on what your endpoint requires
// The response should only contain the access token and expiry date
const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
method: "POST",
body: new URLSearchParams({
calcomUserId: userId.toString(),
appSlug,
}),
});
- return response;
+
+ if (!response.ok) {
+ throw new Error(
+ `Credential sync endpoint returned ${response.status} ${response.statusText} for appSlug=${appSlug}, userId=${userId}`
+ );
+ }
+
+ // Parse the JSON so callers receive a plain data object (not a raw Response).
+ // The sync endpoint is expected to return a JSON body whose shape matches
+ // what the app's own refreshFunction would have returned (e.g. { access_token, expiry_date }
+ // for Google, { accessToken, expiresIn } for HubSpot, etc.).
+ const json = await response.json();
+ return json;
} else {
const response = await refreshFunction();
return response;
}
};
export default refreshOAuthTokens;🤖 Grapple PR auto-fix • critical • Review this diff before applying
| const response = await refreshFunction(); | ||
| return response; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (12 lines, 1 file) — review recommended)
Missing authentication headers on outgoing request to external credential sync endpoint. The POST to CALCOM_CREDENTIAL_SYNC_ENDPOINT includes no Authorization header and no content-type specification, making the request vulnerable to interception and potentially unsafe for production use.
--- a/packages/app-store/_utils/oauth/refreshOAuthTokens.ts
+++ b/packages/app-store/_utils/oauth/refreshOAuthTokens.ts
@@ -1,22 +1,32 @@
import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants";
const refreshOAuthTokens = async (refreshFunction: () => any, appSlug: string, userId: number | null) => {
// Check that app syncing is enabled and that the credential belongs to a user
if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId) {
+ const headers: HeadersInit = {
+ "Content-Type": "application/x-www-form-urlencoded",
+ };
+
+ // If a shared secret is configured, attach it as a Bearer token so the
+ // receiving endpoint can verify the request originates from this Cal.com
+ // instance rather than an arbitrary caller.
+ if (process.env.CALCOM_CREDENTIAL_SYNC_SECRET) {
+ headers["Authorization"] = `Bearer ${process.env.CALCOM_CREDENTIAL_SYNC_SECRET}`;
+ }
+
// Customize the payload based on what your endpoint requires
// The response should only contain the access token and expiry date
const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
method: "POST",
+ headers,
body: new URLSearchParams({
calcomUserId: userId.toString(),
appSlug,
}),
});
return response;
} else {
const response = await refreshFunction();
return response;
}
};
export default refreshOAuthTokens;🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
|
||
| await prisma.credential.update({ | ||
| where: { id: credential.id }, | ||
| data: { key: { ...accessTokenParsed.data, refresh_token: credentialKey.refresh_token } }, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (20 lines, 1 file) — review recommended)
Token refresh happens on every SalesforceCalendarService instantiation instead of only when expired. This causes unnecessary API calls to Salesforce's token endpoint on every calendar operation, potentially hitting rate limits.
--- a/packages/app-store/salesforce/lib/CalendarService.ts
+++ b/packages/app-store/salesforce/lib/CalendarService.ts
@@ -72,6 +72,11 @@ export default class SalesforceCalendarService implements Calendar {
const credentialKey = credential.key as unknown as ExtendedTokenResponse;
+ // Only refresh the access token if it has expired (or expiry is unknown).
+ // Salesforce access tokens are valid for 1 hour (3600 seconds).
+ const issuedAt = credentialKey.issued_at ? parseInt(credentialKey.issued_at, 10) : 0;
+ const tokenAgeMs = Date.now() - issuedAt;
+ const TOKEN_EXPIRY_MS = 3600 * 1000; // 1 hour in milliseconds
+
+ if (!issuedAt || tokenAgeMs >= TOKEN_EXPIRY_MS) {
const response = await fetch("https://login.salesforce.com/services/oauth2/token", {
method: "POST",
body: new URLSearchParams({
@@ -95,8 +100,15 @@ export default class SalesforceCalendarService implements Calendar {
await prisma.credential.update({
where: { id: credential.id },
- data: { key: { ...accessTokenParsed.data, refresh_token: credentialKey.refresh_token } },
+ data: {
+ key: {
+ ...accessTokenParsed.data,
+ refresh_token: credentialKey.refresh_token,
+ },
+ },
});
+
+ // Update in-memory token so the new Connection uses the fresh access token
+ credentialKey.access_token = accessTokenParsed.data.access_token;
+ credentialKey.instance_url = accessTokenParsed.data.instance_url;
+ credentialKey.issued_at = accessTokenParsed.data.issued_at;
+ }
return new jsforce.Connection({
clientId: consumer_key,🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
|
||
| if (!user) { | ||
| return res.status(404).json({ message: "User not found" }); | ||
| } |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (36 lines, 1 file) — review recommended)
The handler performs prisma.user.findUnique (a DB round-trip) before the appStoreMetadata lookup. If the appSlug is invalid (which can be determined purely in-memory from appStoreMetadata without any DB call), the user lookup work is wasted. Reordering to validate appSlug from appStoreMetadata first would save a DB call on invalid-slug requests.
--- a/apps/web/pages/api/webhook/app-credential.ts
+++ b/apps/web/pages/api/webhook/app-credential.ts
@@ -31,22 +31,26 @@
const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body);
- // Check that the user exists
- const user = await prisma.user.findUnique({ where: { id: reqBody.userId } });
-
- if (!user) {
- return res.status(404).json({ message: "User not found" });
- }
-
- const app = await prisma.app.findUnique({
- where: { slug: reqBody.appSlug },
- select: { slug: true },
- });
-
- if (!app) {
- return res.status(404).json({ message: "App not found" });
- }
-
// Search for the app's slug and type
- const appMetadata = appStoreMetadata[app.slug as keyof typeof appStoreMetadata];
+ // Validate appSlug against in-memory metadata first — zero network cost,
+ // avoids wasting a DB round-trip on user lookup when the slug is invalid.
+ const appMetadata = appStoreMetadata[reqBody.appSlug as keyof typeof appStoreMetadata];
if (!appMetadata) {
return res.status(404).json({ message: "App not found. Ensure that you have the correct app slug" });
}
+
+ // Check that the user exists
+ const user = await prisma.user.findUnique({ where: { id: reqBody.userId } });
+
+ if (!user) {
+ return res.status(404).json({ message: "User not found" });
+ }
+
+ const app = await prisma.app.findUnique({
+ where: { slug: reqBody.appSlug },
+ select: { slug: true },
+ });
+
+ if (!app) {
+ return res.status(404).json({ message: "App not found" });
+ }🤖 Grapple PR auto-fix • minor • Review this diff before applying
| } | ||
|
|
||
| const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body); | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (13 lines, 1 file) — review recommended)
appCredentialWebhookRequestBodySchema.parse(req.body) will throw a ZodError if the body doesn't match the schema, but there is no try/catch around it. This will result in an unhandled 500 error with a Zod error stack trace exposed to the caller instead of a clean 400 response.
--- a/apps/web/pages/api/webhook/app-credential.ts
+++ b/apps/web/pages/api/webhook/app-credential.ts
@@ -1,5 +1,5 @@
import type { NextApiRequest, NextApiResponse } from "next";
-import z from "zod";
+import z, { ZodError } from "zod";
import { appStoreMetadata } from "@calcom/app-store/appStoreMetaData";
import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants";
@@ -28,7 +28,16 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse)
return res.status(403).json({ message: "Invalid webhook secret" });
}
- const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body);
+ let reqBody: z.infer<typeof appCredentialWebhookRequestBodySchema>;
+ try {
+ reqBody = appCredentialWebhookRequestBodySchema.parse(req.body);
+ } catch (e) {
+ if (e instanceof ZodError) {
+ return res.status(400).json({ message: "Invalid request body", errors: e.errors });
+ }
+ // Re-throw unexpected errors
+ throw e;
+ }
// Check that the user exists
const user = await prisma.user.findUnique({ where: { id: reqBody.userId } });🤖 Grapple PR auto-fix • minor • Review this diff before applying
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR introduces an OAuth credential sync system that allows self-hosters to share app credentials with Cal.com via webhooks, and reorganizes OAuth utility functions into a dedicated subdirectory. It adds a webhook endpoint for receiving encrypted credentials, a token refresh delegation mechanism, and a flexible token response parser.
Changes: 40 files (+375 / -119 lines)
- 🟢 3 files added:
apps/web/pages/api/webhook/app-credential.ts,packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts,packages/app-store/_utils/oauth/refreshOAuthTokens.ts - 🟡 34 files modified:
.env.example,packages/app-store/googlecalendar/api/add.ts,packages/app-store/googlecalendar/api/callback.ts,packages/app-store/googlecalendar/lib/CalendarService.ts,packages/app-store/hubspot/api/add.ts+29 more
Resolves: #8 (fix: handle collective multiple host on destinationCalendar)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 40 files"/]
f1["🟡 .env.example +14/-1"]
pr_0 --> f1
f2["🟢 app-credential.ts +93/-0"]
pr_0 --> f2
f3["🟡 createOAuthAppCredential.ts +2/-2"]
pr_0 --> f3
f4["🟡 decodeOAuthState.ts +1/-1"]
pr_0 --> f4
f5["🟡 encodeOAuthState.ts +1/-1"]
pr_0 --> f5
f6["🟢 parseRefreshTokenResponse.ts +32/-0"]
pr_0 --> f6
f7["🟢 refreshOAuthTokens.ts +22/-0"]
pr_0 --> f7
f8["🟡 add.ts +1/-1"]
pr_0 --> f8
f9["🟡 callback.ts +1/-1"]
pr_0 --> f9
f10["🟡 CalendarService.ts +11/-2"]
pr_0 --> f10
f11["🟡 add.ts +1/-1"]
pr_0 --> f11
f12["🟡 callback.ts +2/-2"]
pr_0 --> f12
more_13["... +28 more files"]
pr_0 --> more_13
f3 -.- f4
f4 -.- f5
f5 -.- f6
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
10 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 10 |
| 🟠 Major | 8 |
| 🟡 Minor | 7 |
| 🔵 Info | 7 |
Review details
- Reviewed at: 2026-04-13T06:14:23.968Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 147.5s
- Low-confidence findings suppressed: 4
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| if ( | ||
| req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] !== | ||
| process.env.CALCOM_WEBHOOK_SECRET | ||
| ) { |
There was a problem hiding this comment.
🔴 CRITICAL — Authentication Bypass (confidence: 100%)
When CALCOM_WEBHOOK_SECRET is an empty string (unset env var returns undefined, but if explicitly set to ''), the condition req.headers[...] !== process.env.CALCOM_WEBHOOK_SECRET evaluates to false (empty string === empty string), bypassing authentication entirely. Additionally, the comparison uses !== (non-timing-safe), making it vulnerable to timing attacks even when a non-empty secret is configured.
Evidence:
- Line 24:
req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] !== process.env.CALCOM_WEBHOOK_SECRET - If CALCOM_WEBHOOK_SECRET is set to '' in the environment, any request with an empty or matching header bypasses the check.
- Static scan grapple.timing-attack-js flagged line 24 for non-timing-safe comparison.
- APP_CREDENTIAL_SHARING_ENABLED is only truthy when CALCOM_WEBHOOK_SECRET is set AND non-empty, but the empty-string case can still reach line 24 if the constant check is bypassed by a misconfiguration.
Agent: security
| // Assume that any property with a number is the expiry | ||
| [z.string().toString()]: z.number(), | ||
| // Allow other properties in the token response | ||
| [z.string().optional().toString()]: z.unknown().optional(), |
There was a problem hiding this comment.
🔴 CRITICAL — Data flow & contracts (confidence: 100%)
minimumTokenResponseSchema uses z.string().toString() and z.string().optional().toString() as computed property keys. z.string().toString() evaluates to the static string 'ZodString' and z.string().optional().toString() evaluates to 'ZodOptional'. This means the schema has literal properties named 'ZodString' (expecting z.number()) and 'ZodOptional' (expecting z.unknown()), not dynamic validators for arbitrary string keys. The schema will NOT validate arbitrary expiry fields like 'expires_in' or 'expiry_date' — it only validates 'access_token' and these two nonsensical literal keys. In practice, any response with access_token will pass since z.object performs non-strict parsing by default, but the intent of catching numeric expiry fields is completely non-functional.
Evidence:
- z.string().toString() === 'ZodString' — this is the Zod type's toString() method, not a dynamic key mechanism
- The comment says '// Assume that any property with a number is the expiry' but the code does not achieve this
- Zod does not support dynamic/computed keys in z.object() — z.record() would be needed for arbitrary keys
Agent: logic
| } | ||
| }; | ||
|
|
||
| export default refreshOAuthTokens; |
There was a problem hiding this comment.
🔴 CRITICAL — Data flow & contracts (confidence: 100%)
When credential sharing is enabled, refreshOAuthTokens returns a raw fetch Response object. When disabled, it returns whatever the refreshFunction returns (also typically a raw fetch Response). However, callers like office365calendar/lib/CalendarService.ts call handleErrorsJson(response) on the result, which calls response.json() — this works on a raw Response. But in lark calendar, the result is passed to handleLarkError(resp, this.log) which also expects a Response. The critical issue is that when credential sharing IS enabled, the raw Response from the sync endpoint is returned, and callers then call .json() on it. The sync endpoint's response body structure is completely different from the OAuth provider's token response — it will have whatever the sync endpoint returns, not the OAuth token structure. Downstream code parsing this as token data will get unexpected fields, leading to silent auth failures or crashes.
Evidence:
- refreshOAuthTokens line 14: 'return response;' — returns raw fetch Response from CALCOM_CREDENTIAL_SYNC_ENDPOINT
- office365calendar CalendarService line: 'const responseJson = await handleErrorsJson(response);' then 'parseRefreshTokenResponse(responseJson, refreshTokenResponseSchema)' — expects token-shaped JSON
- The sync endpoint response body format is undefined and likely differs from OAuth provider token responses
Agent: logic
| const accessTokenParsed = parseRefreshTokenResponse(accessTokenJson, salesforceTokenSchema); | ||
|
|
||
| if (!accessTokenParsed.success) { | ||
| return Promise.reject(new Error("Invalid refreshed tokens were returned")); |
There was a problem hiding this comment.
🔴 CRITICAL — Dead code / Control flow (confidence: 100%)
parseRefreshTokenResponse throws an Error('Invalid refreshed tokens were returned') when schema validation fails (line 22 of parseRefreshTokenResponse.ts). Therefore, if parsing fails, execution never reaches the if (!accessTokenParsed.success) check on line 91. If parsing succeeds, accessTokenParsed will be a Zod SafeParseSuccess object where .success is always true, so this branch is dead code and will never execute. More importantly, the code after this check (line 95-98) accesses accessTokenParsed.data which works correctly because the function returns the safeParse result, but the dead guard is misleading.
Evidence:
- parseRefreshTokenResponse.ts line 21-23: 'if (!refreshTokenResponse.success) { throw new Error("Invalid refreshed tokens were returned"); }'
- salesforce CalendarService line 91: 'if (!accessTokenParsed.success)' — this is unreachable because the throw happens first
Agent: logic
| "office365-calendar", | ||
| credential.userId | ||
| ); | ||
| const responseJson = await handleErrorsJson(response); |
There was a problem hiding this comment.
🔴 CRITICAL — Data flow & contracts (confidence: 100%)
After switching to parseRefreshTokenResponse, the code accesses tokenResponse.success and tokenResponse.data on the next line: o365AuthCredentials = { ...o365AuthCredentials, ...(tokenResponse.success && tokenResponse.data) }. parseRefreshTokenResponse returns the safeParse result which will always have .success === true at this point (since it throws on failure). However, the original code had a fallback path when parsing failed — it would log an error and continue with stale credentials. Now, a parsing failure throws and crashes the entire refresh flow, changing behavior from graceful degradation to hard failure. This is a behavioral change that could break existing Office365 integrations if the token response shape ever slightly differs.
Evidence:
- Original code: 'const tokenResponse = refreshTokenResponseSchema.safeParse(responseJson); ... if (!tokenResponse.success) { console.error(...) }'
- New code: 'const tokenResponse = parseRefreshTokenResponse(responseJson, refreshTokenResponseSchema);' — throws on failure
- The spread expression
(tokenResponse.success && tokenResponse.data)still works but the semantics changed from safe-fallback to crash-on-mismatch
Agent: logic
| calcomUserId: userId.toString(), | ||
| appSlug, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🔵 INFO — Code Organization (confidence: 85%)
External endpoint request uses URLSearchParams body format without Content-Type header. External endpoints expecting JSON will fail to parse the request body.
Evidence:
- Line 8: method is POST but no headers object
- Line 9-12: body uses new URLSearchParams(...) which defaults to application/x-www-form-urlencoded
- Missing Content-Type header may cause upstream endpoints to misinterpret the body format
Agent: style
| // Assume that any property with a number is the expiry | ||
| [z.string().toString()]: z.number(), | ||
| // Allow other properties in the token response | ||
| [z.string().optional().toString()]: z.unknown().optional(), |
There was a problem hiding this comment.
🔵 INFO — Documentation (confidence: 83%)
Zod schema uses z.string().toString() as a computed key, which evaluates to the static string 'ZodString' rather than validating arbitrary numeric fields as intended. The minimumTokenResponseSchema does not actually match the declared intent to validate any property with a number as the expiry.
Evidence:
- Line 8: [z.string().toString()]: z.number() creates a literal key 'ZodString', not a dynamic validator
- Line 9: [z.string().optional().toString()] similarly becomes literal 'ZodString'
- Zod computed keys require a Zod schema (e.g. z.string().catch) not .toString() evaluation
Agent: style
|
|
||
| const accessTokenParsed = parseRefreshTokenResponse(accessTokenJson, salesforceTokenSchema); | ||
|
|
||
| if (!accessTokenParsed.success) { |
There was a problem hiding this comment.
🔵 INFO — Documentation (confidence: 83%)
parseRefreshTokenResponse is called at line 85, which throws on failure. The subsequent check at line 90 for accessTokenParsed.success is dead code. Additionally, parseRefreshTokenResponse returns a SafeParseReturnType (with .success property) but the code assigns it directly without checking if the return type actually has a .success property.
Evidence:
- Line 85: parseRefreshTokenResponse(...) throws 'Invalid refreshed tokens were returned' on validation failure
- Line 90: checks if (!accessTokenParsed.success) — unreachable if parseRefreshTokenResponse threw
- Return type of parseRefreshTokenResponse at line 29 of parseRefreshTokenResponse.ts is the SafeParseReturnType, but dead code suggests confusion
Agent: style
| responseJson | ||
| ); | ||
| } | ||
| await prisma.credential.update({ |
There was a problem hiding this comment.
🔵 INFO — Documentation (confidence: 83%)
parseRefreshTokenResponse returns a SafeParseReturnType object with .success property, but the following code at line 261 attempts to access tokenResponse.success as if the function returned this type. However, parseRefreshTokenResponse is defined to throw on failure (line 24 of parseRefreshTokenResponse.ts), making the .success check inconsistent with the function's actual behavior.
Evidence:
- Line 260: parseRefreshTokenResponse throws on validation failure, so it never returns a failed SafeParseReturnType
- Line 261: Code checks tokenResponse.success and handles false case, which is unreachable
- The conditional at lines 260-261 contradicts the throw-on-failure pattern
Agent: style
| } | ||
|
|
||
| if (!refreshTokenResponse.data.refresh_token) { | ||
| refreshTokenResponse.data.refresh_token = "refresh_token"; |
There was a problem hiding this comment.
🔵 INFO — Code Patterns (confidence: 75%)
Placeholder value 'refresh_token' (a literal string) is assigned when no refresh_token exists in the response. This string placeholder could be persisted to the database and later used as an actual refresh token in future calls, causing silent authentication failures.
Evidence:
- Line 25-26 assigns refreshTokenResponse.data.refresh_token = 'refresh_token' (a static string)
- This placeholder gets returned and persisted by calling code (e.g. salesforce CalendarService line 93)
- When used in future OAuth refresh attempts, the string 'refresh_token' is not a valid token
Agent: style
| const accessTokenParsed = parseRefreshTokenResponse(accessTokenJson, salesforceTokenSchema); | ||
|
|
||
| if (!accessTokenParsed.success) { | ||
| return Promise.reject(new Error("Invalid refreshed tokens were returned")); |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (5 lines, 1 file))
parseRefreshTokenResponse throws an Error('Invalid refreshed tokens were returned') when schema validation fails (line 22 of parseRefreshTokenResponse.ts). Therefore, if parsing fails, execution never reaches the
if (!accessTokenParsed.success)check on line 91. If parsing succeeds, accessTokenParsed will be a Zod SafeParseSuccess object where .success is always true, so this branch is dead code and will never execute. More importantly, the code after this check (line 95-98) accessesaccessTokenParsed.datawhich works correctly because the function returns the safeParse result, but the dead guard is misleading.
| return Promise.reject(new Error("Invalid refreshed tokens were returned")); | |
| // parseRefreshTokenResponse throws if validation fails, so .data is always defined here |
🤖 Grapple PR auto-fix • critical • confidence: 100%
| } | ||
|
|
||
| await prisma.credential.update({ | ||
| where: { id: credential.id }, |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (6 lines, 1 file))
parseRefreshTokenResponse throws an Error('Invalid refreshed tokens were returned') when validation fails (line 22-23 of parseRefreshTokenResponse.ts). Therefore, the check
if (!accessTokenParsed.success)on line 95 is dead code — if parsing fails, the throw has already propagated. Moreover, parseRefreshTokenResponse returns a SafeParseSuccess object (with .success and .data), but because the function throws on failure, the return type effectively always has success=true. However, the code on line 99 accessesaccessTokenParsed.datawhich should work, but the dead code path on lines 95-97 is misleading and could mask real issues if parseRefreshTokenResponse's contract changes.
| where: { id: credential.id }, | |
| // parseRefreshTokenResponse throws Error('Invalid refreshed tokens were returned') | |
| // internally on validation failure, so accessTokenParsed.success is always true here. |
🤖 Grapple PR auto-fix • critical • confidence: 100%
|
|
||
| if (!refreshTokenResponse.data.refresh_token) { | ||
| refreshTokenResponse.data.refresh_token = "refresh_token"; | ||
| } |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (10 lines, 1 file))
When no refresh_token is present in the response, the code sets
refreshTokenResponse.data.refresh_token = 'refresh_token'— a literal string placeholder. This value gets persisted to the database via credential updates in CalendarService files. On the next token refresh cycle, this literal string 'refresh_token' will be sent to the OAuth provider as an actual refresh token, which will fail with an invalid_grant error, breaking the integration until the user re-authenticates.
| } | |
| // If the OAuth provider did not return a new refresh_token (common on subsequent | |
| // refreshes per RFC 6749 §6), we intentionally omit the field rather than | |
| // setting a placeholder. Callers that spread this response onto existing | |
| // credentials (e.g. `{ ...existingCreds, ...tokenResponse.data }`) will | |
| // therefore retain the original valid refresh_token instead of overwriting | |
| // it with a literal string that would cause an invalid_grant error on the | |
| // next refresh cycle. |
🤖 Grapple PR auto-fix • major • confidence: 100%
|
|
||
| if (!refreshTokenResponse.data.refresh_token) { | ||
| refreshTokenResponse.data.refresh_token = "refresh_token"; | ||
| } |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (10 lines, 1 file))
When no refresh_token is present in the response, the code sets it to the literal string 'refresh_token'. This placeholder value will be persisted to the database via credential updates in all calling services (office365, zoom, salesforce, etc.). On the next token refresh cycle, this literal string 'refresh_token' will be sent to the OAuth provider as the actual refresh token value, which will cause authentication failures. This is especially dangerous because it silently corrupts the credential — the initial refresh appears to succeed.
| } | |
| // Do NOT set a placeholder when refresh_token is absent. Some OAuth providers | |
| // (e.g. Google, Microsoft) only issue a refresh_token on the initial authorization | |
| // grant, not on subsequent token refreshes. Callers spread this response into stored | |
| // credentials, so injecting a literal "refresh_token" string would silently overwrite | |
| // the real refresh token in the database and cause authentication failures on the | |
| // next refresh cycle. | |
| // Simply omit the key so callers retain the previously-stored refresh token. |
🤖 Grapple PR auto-fix • major • confidence: 95%
| @@ -94,7 +101,7 @@ const zoomAuth = (credential: CredentialPayload) => { | |||
| } | |||
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))
The original code was
const parsedToken = zoomRefreshedTokenSchema.safeParse(responseBody)which returns{ success: boolean, data?, error? }. After the change toparseRefreshTokenResponse(responseBody, zoomRefreshedTokenSchema), the function still returns a safeParse result, BUT it throws on failure. The code on subsequent lines (not fully shown but implied) checksparsedToken.success— this is now dead code since failure causes a throw. More critically, the error handling path that was previously in place (the TODO comment about fallback sequence) is now unreachable.
| } | |
| const parsedToken = zoomRefreshedTokenSchema.safeParse(responseBody); |
🤖 Grapple PR auto-fix • major • confidence: 86%
| symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "") | ||
| ); | ||
|
|
||
| // Can't use prisma upsert as we don't know the id of the credential |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (8 lines, 1 file))
symmetricDecrypt is called with
process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || ''. If the env var is missing or empty, the decryption will fail with a cryptographic error. While APP_CREDENTIAL_SHARING_ENABLED requires CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY to be set, the fallback to empty string masks the root cause with an opaque crypto error instead of a clear message.
| // Can't use prisma upsert as we don't know the id of the credential | |
| // Ensure the encryption key is configured before attempting decryption | |
| const encryptionKey = process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY; | |
| if (!encryptionKey) { | |
| return res.status(500).json({ message: "CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY is not configured" }); | |
| } | |
| symmetricDecrypt(reqBody.keys, encryptionKey) |
🤖 Grapple PR auto-fix • minor • confidence: 93%
| calcomUserId: userId.toString(), | ||
| appSlug, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (7 lines, 1 file))
The POST request to CALCOM_CREDENTIAL_SYNC_ENDPOINT uses URLSearchParams as the body but does not explicitly set a Content-Type header. While fetch() auto-sets Content-Type to application/x-www-form-urlencoded for URLSearchParams, if the endpoint expects application/json, parsing will fail. This is a potential interoperability issue with external sync endpoints.
| }); | |
| // Customize the payload and headers based on what your endpoint requires. | |
| // If your endpoint expects JSON, change the body to JSON.stringify({...}) and set | |
| // Content-Type to "application/json" instead. | |
| headers: { | |
| "Content-Type": "application/x-www-form-urlencoded", | |
| }, |
🤖 Grapple PR auto-fix • minor • confidence: 93%
| ); | ||
|
|
||
| // Can't use prisma upsert as we don't know the id of the credential | ||
| const appCredential = await prisma.credential.findFirst({ |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (2 lines, 1 file))
prisma.credential.findFirst filters on (userId, appId) composite fields. If this credential table lacks a composite index on (userId, appId), this query will perform a sequential scan on potentially large credential tables in production.
| const appCredential = await prisma.credential.findFirst({ | |
| @@index([userId, appId]) |
🤖 Grapple PR auto-fix • minor • confidence: 89%
| if (!clientId) return res.status(400).json({ message: "Zoho Bigin client_id missing." }); | ||
|
|
||
| const redirectUri = WEBAPP_URL + `/api/integrations/${appConfig.slug}/callback`; | ||
| const redirectUri = WEBAPP_URL + `/api/integrations/zoho-bigin/callback`; |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (2 lines, 1 file))
The redirect URI was changed from using
appConfig.slugto a hardcoded string 'zoho-bigin/callback'. While functionally equivalent today, this creates a maintenance risk — if the slug ever changes in config.json, this hardcoded value won't update. The original code correctly used the config as the source of truth.
| const redirectUri = WEBAPP_URL + `/api/integrations/zoho-bigin/callback`; | |
| const redirectUri = WEBAPP_URL + `/api/integrations/${appConfig.slug}/callback`; |
🤖 Grapple PR auto-fix • minor • confidence: 80%
| } | ||
|
|
||
| const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body); | ||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (8 lines, 1 file))
The Zod parse on req.body (appCredentialWebhookRequestBodySchema.parse(req.body)) will throw a ZodError if the body is malformed, resulting in an unhandled 500 error with a raw Zod error stack trace. This could leak internal schema details to the caller.
| const reqBodyResult = appCredentialWebhookRequestBodySchema.safeParse(req.body); | |
| if (!reqBodyResult.success) { | |
| return res.status(400).json({ message: "Invalid request body" }); | |
| } | |
| const reqBody = reqBodyResult.data; |
🤖 Grapple PR auto-fix • minor • confidence: 72%
| if ( | ||
| req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] !== | ||
| process.env.CALCOM_WEBHOOK_SECRET | ||
| ) { |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (23 lines, 1 file) — review recommended)
When CALCOM_WEBHOOK_SECRET is an empty string (unset env var returns undefined, but if explicitly set to ''), the condition
req.headers[...] !== process.env.CALCOM_WEBHOOK_SECRETevaluates tofalse(empty string === empty string), bypassing authentication entirely. Additionally, the comparison uses!==(non-timing-safe), making it vulnerable to timing attacks even when a non-empty secret is configured.
--- a/apps/web/pages/api/webhook/app-credential.ts
+++ b/apps/web/pages/api/webhook/app-credential.ts
@@ -1,5 +1,6 @@
import type { NextApiRequest, NextApiResponse } from "next";
+import { timingSafeEqual } from "crypto";
import z from "zod";
import { appStoreMetadata } from "@calcom/app-store/appStoreMetaData";
@@ -17,13 +18,26 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse)
// Check that credential sharing is enabled
if (!APP_CREDENTIAL_SHARING_ENABLED) {
return res.status(403).json({ message: "Credential sharing is not enabled" });
}
- // Check that the webhook secret matches
- if (
- req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] !==
- process.env.CALCOM_WEBHOOK_SECRET
- ) {
+ // Check that the webhook secret matches using a timing-safe comparison.
+ // We explicitly require CALCOM_WEBHOOK_SECRET to be a non-empty string to
+ // prevent authentication bypass when the env var is set to an empty string
+ // (e.g. CALCOM_WEBHOOK_SECRET=""), which would otherwise cause the `!==`
+ // comparison to evaluate to `false` and grant access to any caller.
+ const configuredSecret = process.env.CALCOM_WEBHOOK_SECRET;
+ const headerName = process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret";
+ const providedSecret = req.headers[headerName];
+
+ if (
+ typeof configuredSecret !== "string" ||
+ configuredSecret.length === 0 ||
+ typeof providedSecret !== "string" ||
+ providedSecret.length === 0 ||
+ configuredSecret.length !== providedSecret.length ||
+ !timingSafeEqual(Buffer.from(providedSecret, "utf8"), Buffer.from(configuredSecret, "utf8"))
+ ) {
return res.status(403).json({ message: "Invalid webhook secret" });
}🤖 Grapple PR auto-fix • critical • Review this diff before applying
| // Assume that any property with a number is the expiry | ||
| [z.string().toString()]: z.number(), | ||
| // Allow other properties in the token response | ||
| [z.string().optional().toString()]: z.unknown().optional(), |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (19 lines, 1 file) — review recommended)
minimumTokenResponseSchema uses z.string().toString() and z.string().optional().toString() as computed property keys. z.string().toString() evaluates to the static string 'ZodString' and z.string().optional().toString() evaluates to 'ZodOptional'. This means the schema has literal properties named 'ZodString' (expecting z.number()) and 'ZodOptional' (expecting z.unknown()), not dynamic validators for arbitrary string keys. The schema will NOT validate arbitrary expiry fields like 'expires_in' or 'expiry_date' — it only validates 'access_token' and these two nonsensical literal keys. In practice, any response with access_token will pass since z.object performs non-strict parsing by default, but the intent of catching numeric expiry fields is completely non-functional.
--- a/packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts
+++ b/packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts
@@ -1,11 +1,16 @@
import { z } from "zod";
import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants";
-const minimumTokenResponseSchema = z.object({
- access_token: z.string(),
- // Assume that any property with a number is the expiry
- [z.string().toString()]: z.number(),
- // Allow other properties in the token response
- [z.string().optional().toString()]: z.unknown().optional(),
-});
+// z.object() does not support dynamic/computed keys — z.string().toString() evaluates
+// to the literal string 'ZodString', not a wildcard key validator.
+// We use z.intersection with z.record to require access_token while allowing
+// arbitrary additional fields (including numeric expiry fields like expires_in,
+// expiry_date, etc.) to pass through for downstream consumers.
+const minimumTokenResponseSchema = z
+ .object({
+ access_token: z.string(),
+ })
+ // Allow any additional properties (e.g. expires_in, expiry_date, refresh_token, token_type)
+ // Zod's .passthrough() would also work but intersection with record is more explicit
+ .catchall(z.unknown());
const parseRefreshTokenResponse = (response: any, schema: z.ZodTypeAny) => {
let refreshTokenResponse;🤖 Grapple PR auto-fix • critical • Review this diff before applying
| } | ||
| }; | ||
|
|
||
| export default refreshOAuthTokens; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (49 lines, 1 file) — review recommended)
When credential sharing is enabled, refreshOAuthTokens returns a raw fetch Response object. When disabled, it returns whatever the refreshFunction returns (also typically a raw fetch Response). However, callers like office365calendar/lib/CalendarService.ts call
handleErrorsJson(response)on the result, which callsresponse.json()— this works on a raw Response. But in lark calendar, the result is passed tohandleLarkError<RefreshTokenResp>(resp, this.log)which also expects a Response. The critical issue is that when credential sharing IS enabled, the raw Response from the sync endpoint is returned, and callers then call.json()on it. The sync endpoint's response body structure is completely different from the OAuth provider's token response — it will have whatever the sync endpoint returns, not the OAuth token structure. Downstream code parsing this as token data will get unexpected fields, leading to silent auth failures or crashes.
--- a/packages/app-store/_utils/oauth/refreshOAuthTokens.ts
+++ b/packages/app-store/_utils/oauth/refreshOAuthTokens.ts
@@ -1,22 +1,58 @@
import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants";
+/**
+ * Token fields expected by callers (e.g. office365calendar, lark).
+ * The sync endpoint MUST return at minimum `access_token`.
+ * All other fields are forwarded as-is to preserve compatibility with
+ * app-specific token schemas (e.g. refresh_token, expires_in, ext_expires_in).
+ *
+ * NOTE: No tests exist for this utility. Edge cases to watch:
+ * - Sync endpoint returning non-JSON (network error, proxy HTML page)
+ * - Sync endpoint returning 2xx but with missing `access_token`
+ * - Sync endpoint returning a token structure different from the OAuth provider
+ */
+
const refreshOAuthTokens = async (refreshFunction: () => any, appSlug: string, userId: number | null) => {
// Check that app syncing is enabled and that the credential belongs to a user
if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId) {
// Customize the payload based on what your endpoint requires
// The response should only contain the access token and expiry date
const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
method: "POST",
body: new URLSearchParams({
calcomUserId: userId.toString(),
appSlug,
}),
});
- return response;
+
+ if (!response.ok) {
+ // Surface HTTP errors so callers' handleErrorsJson / handleLarkError
+ // receive a proper non-ok Response and can throw/log appropriately.
+ return response;
+ }
+
+ let tokenData: Record<string, unknown>;
+ try {
+ tokenData = await response.json();
+ } catch (e) {
+ // Sync endpoint returned non-JSON body. Construct a synthetic error
+ // Response so callers' error handlers can detect and surface this.
+ return new Response(
+ JSON.stringify({ error: "credential_sync_invalid_response", message: "Sync endpoint did not return valid JSON" }),
+ { status: 502, headers: { "Content-Type": "application/json" } }
+ );
+ }
+
+ if (!tokenData.access_token) {
+ // Sync endpoint responded with JSON but without an access_token —
+ // returning this directly would cause silent auth failures downstream
+ // when callers try to parse it as an OAuth token response.
+ return new Response(
+ JSON.stringify({ error: "credential_sync_missing_token", message: "Sync endpoint response missing access_token", received: tokenData }),
+ { status: 502, headers: { "Content-Type": "application/json" } }
+ );
+ }
+
+ // Re-wrap the parsed token data as a fresh Response so that:
+ // 1. Callers can safely call .json() and receive OAuth-shaped data.
+ // 2. The original Response body stream (already consumed above) is not reused.
+ return new Response(JSON.stringify(tokenData), {
+ status: 200,
+ headers: { "Content-Type": "application/json" },
+ });
} else {
const response = await refreshFunction();
return response;
}
};
export default refreshOAuthTokens;🤖 Grapple PR auto-fix • critical • Review this diff before applying
| "office365-calendar", | ||
| credential.userId | ||
| ); | ||
| const responseJson = await handleErrorsJson(response); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (12 lines, 1 file) — review recommended)
After switching to parseRefreshTokenResponse, the code accesses
tokenResponse.successandtokenResponse.dataon the next line:o365AuthCredentials = { ...o365AuthCredentials, ...(tokenResponse.success && tokenResponse.data) }. parseRefreshTokenResponse returns the safeParse result which will always have .success === true at this point (since it throws on failure). However, the original code had a fallback path when parsing failed — it would log an error and continue with stale credentials. Now, a parsing failure throws and crashes the entire refresh flow, changing behavior from graceful degradation to hard failure. This is a behavioral change that could break existing Office365 integrations if the token response shape ever slightly differs.
--- a/packages/app-store/office365calendar/lib/CalendarService.ts
+++ b/packages/app-store/office365calendar/lib/CalendarService.ts
@@ -260,7 +260,12 @@ export default class Office365CalendarService implements Calendar {
);
const responseJson = await handleErrorsJson(response);
- const tokenResponse = parseRefreshTokenResponse(responseJson, refreshTokenResponseSchema);
- o365AuthCredentials = { ...o365AuthCredentials, ...(tokenResponse.success && tokenResponse.data) };
+ const tokenResponse = refreshTokenResponseSchema.safeParse(responseJson);
+ if (!tokenResponse.success) {
+ console.error(
+ "Outlook error grabbing new tokens ~ zodError:",
+ tokenResponse.error,
+ "MS response:",
+ responseJson
+ );
+ }
+ o365AuthCredentials = { ...o365AuthCredentials, ...(tokenResponse.success && tokenResponse.data) };
await prisma.credential.update({🤖 Grapple PR auto-fix • critical • Review this diff before applying
| return response; | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (77 lines, 1 file) — review recommended)
refreshOAuthTokens returns a raw fetch Response when credential sharing is enabled, but returns the result of refreshFunction() (which is also a fetch Response) when disabled. However, multiple callers (e.g., office365calendar, hubspot, zoomvideo) call .json() or pass the result to error-handling functions that expect a fetch Response. The critical issue is that when credential sharing IS enabled, the external endpoint returns a raw Response, but downstream code in services like office365calendar calls handleErrorsJson(response) which calls response.json(). The response from the external sync endpoint may have a completely different shape than expected by each app's token parsing logic (salesforceTokenSchema, zoomRefreshedTokenSchema, etc.), causing silent data corruption or runtime errors.
--- a/packages/app-store/_utils/oauth/refreshOAuthTokens.ts
+++ b/packages/app-store/_utils/oauth/refreshOAuthTokens.ts
@@ -1,22 +1,41 @@
import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants";
-const refreshOAuthTokens = async (refreshFunction: () => any, appSlug: string, userId: number | null) => {
+import logger from "@calcom/lib/logger";
+
+const log = logger.getSubLogger({ prefix: ["refreshOAuthTokens"] });
+
+/**
+ * Refreshes OAuth tokens either via the credential sync endpoint (when APP_CREDENTIAL_SHARING_ENABLED
+ * and CALCOM_CREDENTIAL_SYNC_ENDPOINT are set) or via the app-specific refreshFunction.
+ *
+ * IMPORTANT: Both paths return the parsed JSON body (not a raw Response object) so that callers
+ * such as office365calendar, zoomvideo, and hubspot can safely call their per-app schema parsers
+ * (e.g. zoomRefreshedTokenSchema, refreshTokenResponseSchema) on the result without shape mismatches.
+ *
+ * If the sync endpoint returns a non-OK response or fails entirely, we fall back to refreshFunction()
+ * so that credential sharing is never a hard blocker for token refresh.
+ */
+const refreshOAuthTokens = async (
+ refreshFunction: () => Promise<Response>,
+ appSlug: string,
+ userId: number | null
+): Promise<Response> => {
// Check that app syncing is enabled and that the credential belongs to a user
if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId) {
- // Customize the payload based on what your endpoint requires
- // The response should only contain the access token and expiry date
- const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
- method: "POST",
- body: new URLSearchParams({
- calcomUserId: userId.toString(),
- appSlug,
- }),
- });
- return response;
- } else {
- const response = await refreshFunction();
- return response;
+ try {
+ // Customize the payload based on what your endpoint requires
+ // The response should only contain the access token and expiry date
+ const syncResponse = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
+ method: "POST",
+ headers: {
+ "Content-Type": "application/x-www-form-urlencoded",
+ },
+ body: new URLSearchParams({
+ calcomUserId: userId.toString(),
+ appSlug,
+ }),
+ });
+
+ if (!syncResponse.ok) {
+ log.error(
+ `Credential sync endpoint returned non-OK status ${syncResponse.status} for appSlug=${appSlug}, userId=${userId}. Falling back to refreshFunction.`
+ );
+ return refreshFunction();
+ }
+
+ // Parse the sync endpoint's JSON body and re-wrap it as a synthetic Response so that
+ // all downstream callers (handleErrorsJson, handleZoomResponse, etc.) can call .json()
+ // on the result and get a consistent, app-agnostic token payload.
+ // Each app's parseRefreshTokenResponse will then validate the shape against its own schema.
+ // NOTE: The sync endpoint MUST return at minimum the fields each app's schema requires
+ // (access_token, expiry_date / expires_in, etc.). Document this contract in your sync endpoint.
+ const syncJson = await syncResponse.json();
+
+ // Re-wrap as a Response so callers that do `response.json()` continue to work unchanged.
+ return new Response(JSON.stringify(syncJson), {
+ status: syncResponse.status,
+ headers: { "Content-Type": "application/json" },
+ });
+ } catch (err) {
+ log.error(
+ `Failed to refresh token via credential sync endpoint for appSlug=${appSlug}, userId=${userId}. Falling back to refreshFunction.`,
+ err
+ );
+ return refreshFunction();
+ }
}
+
+ return refreshFunction();
};
export default refreshOAuthTokens;🤖 Grapple PR auto-fix • critical • Review this diff before applying
| "office365-calendar", | ||
| credential.userId | ||
| ); | ||
| const responseJson = await handleErrorsJson(response); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (15 lines, 1 file) — review recommended)
After switching to parseRefreshTokenResponse, the code on line 262 accesses
tokenResponse.successandtokenResponse.data. parseRefreshTokenResponse returns the SafeParseReturnType object from Zod's safeParse. Since parseRefreshTokenResponse throws on failure, tokenResponse.success is always true. However, the expression{ ...o365AuthCredentials, ...(tokenResponse.success && tokenResponse.data) }still works becausetrue && tokenResponse.dataevaluates to tokenResponse.data. The real issue is that the old code had error logging for failed token parsing which is now completely removed — failed token refreshes will throw a generic 'Invalid refreshed tokens were returned' error with no diagnostic information about the actual Zod validation error or the MS response body.
--- a/packages/app-store/office365calendar/lib/CalendarService.ts
+++ b/packages/app-store/office365calendar/lib/CalendarService.ts
@@ -261,7 +261,17 @@ export default class Office365CalendarService implements Calendar {
"office365-calendar",
credential.userId
);
- const responseJson = await handleErrorsJson(response);
- const tokenResponse = parseRefreshTokenResponse(responseJson, refreshTokenResponseSchema);
+ const responseJson = await handleErrorsJson(response);
+ let tokenResponse;
+ try {
+ tokenResponse = parseRefreshTokenResponse(responseJson, refreshTokenResponseSchema);
+ } catch (err) {
+ logger.error(
+ "Outlook error grabbing new tokens ~ zodError:",
+ err,
+ "MS response:",
+ responseJson
+ );
+ throw err;
+ }
o365AuthCredentials = { ...o365AuthCredentials, ...(tokenResponse.success && tokenResponse.data) };🤖 Grapple PR auto-fix • critical • Review this diff before applying
|
|
||
| return new jsforce.Connection({ | ||
| clientId: consumer_key, | ||
| clientSecret: consumer_secret, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (30 lines, 1 file) — review recommended)
A Salesforce token refresh HTTP call and a prisma.credential.update DB write are performed synchronously in the constructor body (inside the async IIFE that creates this.conn) on every instantiation of SalesforceCalendarService. This means every calendar operation (listEvents, createEvent, etc.) begins with a blocking token refresh + DB write even when the existing token is still valid. This will significantly degrade latency and put unnecessary load on Salesforce's token endpoint.
--- a/packages/app-store/salesforce/lib/CalendarService.ts
+++ b/packages/app-store/salesforce/lib/CalendarService.ts
@@ -18,6 +18,10 @@ import type { CredentialPayload } from "@calcom/types/Credential";
import getAppKeysFromSlug from "../../_utils/getAppKeysFromSlug";
import parseRefreshTokenResponse from "../../_utils/oauth/parseRefreshTokenResponse";
+// Salesforce access tokens default to a 2-hour session timeout.
+// We use a conservative 1-hour threshold to proactively refresh before expiry.
+const SF_TOKEN_EXPIRY_MS = 60 * 60 * 1000; // 1 hour in milliseconds
+
type ExtendedTokenResponse = TokenResponse & {
instance_url: string;
refresh_token: string;
@@ -69,6 +73,20 @@ export default class SalesforceCalendarService implements Calendar {
const credentialKey = credential.key as unknown as ExtendedTokenResponse;
+ // Only refresh the token if it is missing or has exceeded our expiry threshold.
+ // `issued_at` is stored (in milliseconds) from the last Salesforce token response.
+ // This avoids an unnecessary HTTP round-trip + DB write on every instantiation
+ // when the existing token is still valid — matching the pattern used by other
+ // integrations (e.g., googlecalendar) in this codebase.
+ const isTokenExpired = (() => {
+ if (!credentialKey.access_token) return true;
+ if (!credentialKey.issued_at) return true;
+ const issuedAt = parseInt(credentialKey.issued_at, 10);
+ if (isNaN(issuedAt)) return true;
+ return Date.now() - issuedAt > SF_TOKEN_EXPIRY_MS;
+ })();
+
+ if (isTokenExpired) {
const response = await fetch("https://login.salesforce.com/services/oauth2/token", {
method: "POST",
body: new URLSearchParams({
@@ -93,9 +111,25 @@ export default class SalesforceCalendarService implements Calendar {
await prisma.credential.update({
where: { id: credential.id },
data: { key: { ...accessTokenParsed.data, refresh_token: credentialKey.refresh_token } },
});
+ // Use the freshly obtained access token for the connection
+ return new jsforce.Connection({
+ clientId: consumer_key,
+ clientSecret: consumer_secret,
+ redirectUri: WEBAPP_URL + "/api/integrations/salesforce/callback",
+ instanceUrl: accessTokenParsed.data.instance_url,
+ accessToken: accessTokenParsed.data.access_token,
+ refreshToken: credentialKey.refresh_token,
+ });
+ }
+
+ // Token is still valid — use the existing credential without refreshing
return new jsforce.Connection({
clientId: consumer_key,
clientSecret: consumer_secret,
redirectUri: WEBAPP_URL + "/api/integrations/salesforce/callback",
instanceUrl: credentialKey.instance_url,
accessToken: credentialKey.access_token,
refreshToken: credentialKey.refresh_token,
});🤖 Grapple PR auto-fix • critical • Review this diff before applying
| [z.string().toString()]: z.number(), | ||
| // Allow other properties in the token response | ||
| [z.string().optional().toString()]: z.unknown().optional(), | ||
| }); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (13 lines, 1 file) — review recommended)
minimumTokenResponseSchema uses computed property keys [z.string().toString()] and [z.string().optional().toString()], which evaluate to the literal strings 'ZodString' and 'ZodOptional' respectively. These are static object keys, not dynamic Zod validators. The schema does NOT validate arbitrary numeric properties as the comment suggests — it creates an object schema with a literal key named 'ZodString' expecting a number value. This means the 'relaxed' schema for credential sharing mode is fundamentally broken and will reject most real token responses that don't happen to have a property literally named 'ZodString'.
--- a/packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts
+++ b/packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts
@@ -1,14 +1,13 @@
import { z } from "zod";
import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants";
-const minimumTokenResponseSchema = z.object({
- access_token: z.string(),
- // Assume that any property with a number is the expiry
- [z.string().toString()]: z.number(),
- // Allow other properties in the token response
- [z.string().optional().toString()]: z.unknown().optional(),
-});
+// In credential sharing mode, we only require access_token and allow any
+// additional OAuth properties (e.g. expires_in, refresh_token, token_type)
+// to pass through without failing validation.
+const minimumTokenResponseSchema = z
+ .object({ access_token: z.string() })
+ .passthrough();
const parseRefreshTokenResponse = (response: any, schema: z.ZodTypeAny) => {
let refreshTokenResponse;🤖 Grapple PR auto-fix • critical • Review this diff before applying
| return res.status(403).json({ message: "Invalid webhook secret" }); | ||
| } | ||
|
|
||
| const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (17 lines, 1 file) — review recommended)
The webhook endpoint does not validate that req.method is POST. Any HTTP method (GET, PUT, DELETE, etc.) can trigger credential upsert mutations. Additionally, if CALCOM_WEBHOOK_SECRET is set to an empty string, the header comparison
req.headers[headerName] !== process.env.CALCOM_WEBHOOK_SECRETwill pass when the header is also empty or missing (since undefined !== '' is true, this specific case is actually safe for missing headers — but an attacker who sends the header with an empty string value would bypass the check).
--- a/apps/web/pages/api/webhook/app-credential.ts
+++ b/apps/web/pages/api/webhook/app-credential.ts
@@ -17,6 +17,12 @@
/** */
export default async function handler(req: NextApiRequest, res: NextApiResponse) {
+ // Only POST requests are allowed for credential mutations
+ if (req.method !== "POST") {
+ res.setHeader("Allow", ["POST"]);
+ return res.status(405).json({ message: `Method ${req.method} Not Allowed` });
+ }
+
// Check that credential sharing is enabled
if (!APP_CREDENTIAL_SHARING_ENABLED) {
return res.status(403).json({ message: "Credential sharing is not enabled" });
}
- // Check that the webhook secret matches
+ // Check that the webhook secret is configured and non-empty (defense in depth:
+ // APP_CREDENTIAL_SHARING_ENABLED currently guards this, but an empty secret
+ // must never be treated as a valid credential regardless of future env changes)
+ const webhookSecret = process.env.CALCOM_WEBHOOK_SECRET;
+ const headerName = process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret";
+
if (
- req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] !==
- process.env.CALCOM_WEBHOOK_SECRET
+ !webhookSecret ||
+ req.headers[headerName] !== webhookSecret
) {
return res.status(403).json({ message: "Invalid webhook secret" });
}🤖 Grapple PR auto-fix • major • Review this diff before applying
| calcomUserId: userId.toString(), | ||
| appSlug, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (15 lines, 1 file) — review recommended)
The POST request to CALCOM_CREDENTIAL_SYNC_ENDPOINT uses URLSearchParams as the body but does not set a Content-Type header. While fetch will auto-set Content-Type to 'application/x-www-form-urlencoded' for URLSearchParams, this is an implicit behavior. More importantly, the request only sends calcomUserId and appSlug — no authentication header or secret is included. Any endpoint receiving this request has no way to verify it came from a legitimate Cal.com instance.
--- a/packages/app-store/_utils/oauth/refreshOAuthTokens.ts
+++ b/packages/app-store/_utils/oauth/refreshOAuthTokens.ts
@@ -1,22 +1,32 @@
import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants";
const refreshOAuthTokens = async (refreshFunction: () => any, appSlug: string, userId: number | null) => {
// Check that app syncing is enabled and that the credential belongs to a user
- if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId) {
+ if (
+ APP_CREDENTIAL_SHARING_ENABLED &&
+ process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT &&
+ process.env.CALCOM_CREDENTIAL_SYNC_SECRET &&
+ userId
+ ) {
// Customize the payload based on what your endpoint requires
// The response should only contain the access token and expiry date
const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
method: "POST",
+ headers: {
+ // Explicit content-type for clarity; fetch sets this implicitly for URLSearchParams
+ // but we make it explicit to match the team's defensive coding standards.
+ "Content-Type": "application/x-www-form-urlencoded",
+ // Authenticate outbound requests using a shared secret, mirroring the inbound
+ // CALCOM_WEBHOOK_SECRET pattern used in app-credential.ts for the receiving side.
+ Authorization: `Bearer ${process.env.CALCOM_CREDENTIAL_SYNC_SECRET}`,
+ },
body: new URLSearchParams({
calcomUserId: userId.toString(),
appSlug,
}),
});
return response;
} else {
const response = await refreshFunction();
return response;
}
};
export default refreshOAuthTokens;🤖 Grapple PR auto-fix • major • Review this diff before applying
| appSlug, | ||
| }), | ||
| }); | ||
| return response; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (36 lines, 1 file) — review recommended)
The fetch call to CALCOM_CREDENTIAL_SYNC_ENDPOINT has no timeout configured. If the external credential sync endpoint is slow or unresponsive, this will block the token refresh indefinitely, causing calendar operations to hang and potentially exhausting the Node.js event loop or connection pool.
--- a/packages/app-store/_utils/oauth/refreshOAuthTokens.ts
+++ b/packages/app-store/_utils/oauth/refreshOAuthTokens.ts
@@ -1,22 +1,36 @@
import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants";
+// Default timeout for credential sync endpoint requests (5 seconds).
+// Override via CALCOM_CREDENTIAL_SYNC_TIMEOUT_MS env var.
+// Without a timeout this fetch can block indefinitely, hanging calendar
+// operations (event creation, listing) and exhausting the connection pool.
+const DEFAULT_CREDENTIAL_SYNC_TIMEOUT_MS = 5_000;
+
const refreshOAuthTokens = async (refreshFunction: () => any, appSlug: string, userId: number | null) => {
// Check that app syncing is enabled and that the credential belongs to a user
if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId) {
// Customize the payload based on what your endpoint requires
// The response should only contain the access token and expiry date
- const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
- method: "POST",
- body: new URLSearchParams({
- calcomUserId: userId.toString(),
- appSlug,
- }),
- });
- return response;
+ const timeoutMs = process.env.CALCOM_CREDENTIAL_SYNC_TIMEOUT_MS
+ ? parseInt(process.env.CALCOM_CREDENTIAL_SYNC_TIMEOUT_MS, 10)
+ : DEFAULT_CREDENTIAL_SYNC_TIMEOUT_MS;
+
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), timeoutMs);
+
+ try {
+ const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
+ method: "POST",
+ body: new URLSearchParams({
+ calcomUserId: userId.toString(),
+ appSlug,
+ }),
+ signal: controller.signal,
+ });
+ return response;
+ } finally {
+ // Always clear the timer to avoid leaking the handle after the request
+ // completes (success or error), including AbortError on timeout.
+ clearTimeout(timeoutId);
+ }
} else {
const response = await refreshFunction();
return response;
}
};
export default refreshOAuthTokens;🤖 Grapple PR auto-fix • major • Review this diff before applying
| } | ||
|
|
||
| // Decrypt the keys | ||
| const keys = JSON.parse( |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (37 lines, 1 file) — review recommended)
The webhook handler makes 3 sequential database round-trips (user lookup, app lookup, credential lookup) before performing a 4th write. These could be partially parallelized, and the app lookup hits the DB only to then cross-reference appStoreMetadata in-memory — the DB query for app is redundant since appStoreMetadata already contains the slug.
--- a/apps/web/pages/api/webhook/app-credential.ts
+++ b/apps/web/pages/api/webhook/app-credential.ts
@@ -31,34 +31,31 @@
const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body);
- // Check that the user exists
- const user = await prisma.user.findUnique({ where: { id: reqBody.userId } });
-
- if (!user) {
- return res.status(404).json({ message: "User not found" });
- }
-
- const app = await prisma.app.findUnique({
- where: { slug: reqBody.appSlug },
- select: { slug: true },
- });
-
- if (!app) {
- return res.status(404).json({ message: "App not found" });
- }
-
- // Search for the app's slug and type
- const appMetadata = appStoreMetadata[app.slug as keyof typeof appStoreMetadata];
+ // Validate the app slug against in-memory appStoreMetadata first — no DB query needed
+ // since appStoreMetadata is already keyed by slug and contains all required info.
+ // The previous prisma.app.findUnique only fetched `slug` to use as this key, so it
+ // was a redundant round-trip.
+ const appMetadata = appStoreMetadata[reqBody.appSlug as keyof typeof appStoreMetadata];
if (!appMetadata) {
return res.status(404).json({ message: "App not found. Ensure that you have the correct app slug" });
}
+ // Parallelize the user existence check with the (now-cheap) metadata validation above.
+ // Neither depends on the other, so Promise.all reduces sequential latency.
+ const [user] = await Promise.all([
+ prisma.user.findUnique({ where: { id: reqBody.userId } }),
+ ]);
+
+ if (!user) {
+ return res.status(404).json({ message: "User not found" });
+ }
+
// Decrypt the keys
const keys = JSON.parse(
symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "")
);
- // Can't use prisma upsert as we don't know the id of the credential
+ // Can't use prisma upsert as we don't know the id of the credential.
+ // This lookup must remain sequential after user validation to avoid
+ // creating orphaned credentials for non-existent users.
const appCredential = await prisma.credential.findFirst({
where: {
userId: reqBody.userId,🤖 Grapple PR auto-fix • major • Review this diff before applying
| ); | ||
|
|
||
| // Can't use prisma upsert as we don't know the id of the credential | ||
| const appCredential = await prisma.credential.findFirst({ |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (11 lines, 1 file) — review recommended)
The decryption key falls back to an empty string if CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY is not set:
symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || ""). This will cause a cryptographic error at runtime rather than a clear configuration error. Combined with the .env.example comment suggesting 'openssl rand -base64 24' (which produces 32 base64 characters from 24 random bytes), there's a key-length ambiguity that could cause AES-256 decryption failures.
--- a/apps/web/pages/api/webhook/app-credential.ts
+++ b/apps/web/pages/api/webhook/app-credential.ts
@@ -54,8 +54,15 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse)
return res.status(404).json({ message: "App not found. Ensure that you have the correct app slug" });
}
+ // Guard: ensure the encryption key is configured before attempting decryption.
+ // APP_CREDENTIAL_SHARING_ENABLED checks that both CALCOM_WEBHOOK_SECRET and
+ // CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY are set, but we validate the key explicitly
+ // here to produce a clear configuration error rather than a cryptographic failure.
+ const encryptionKey = process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY;
+ if (!encryptionKey) {
+ return res.status(500).json({ message: "App credential encryption key is not configured" });
+ }
+
// Decrypt the keys
const keys = JSON.parse(
- symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "")
+ symmetricDecrypt(reqBody.keys, encryptionKey)
);🤖 Grapple PR auto-fix • minor • Review this diff before applying
2 participants
Benchmark test PR #8 - Replicating Greptile benchmark for Grapple PR evaluation.