OAuth credential sync and app integration enhancements#4
OAuth credential sync and app integration enhancements#4akshayutture-augment wants to merge 1 commit into
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>
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request introduces app credential sharing infrastructure through new OAuth utilities, environment configuration, and a webhook endpoint. OAuth helper functions are reorganized into a dedicated subdirectory and two new utilities ( Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Integration
participant RefreshHelper as refreshOAuthTokens
participant ShareEndpoint as CALCOM Endpoint<br/>(if enabled)
participant LocalRefresh as Local Refresh<br/>Function
participant TokenParser as parseRefreshTokenResponse
App->>RefreshHelper: Call refreshOAuthTokens(refreshFn, appSlug, userId)
alt Credential Sharing Enabled
RefreshHelper->>ShareEndpoint: POST with userId & appSlug
ShareEndpoint-->>RefreshHelper: Token Response
else Credential Sharing Disabled
RefreshHelper->>LocalRefresh: Execute refresh function
LocalRefresh-->>RefreshHelper: Token Response
end
RefreshHelper->>TokenParser: Parse response with schema
TokenParser-->>RefreshHelper: Validated token data
RefreshHelper-->>App: Return parsed tokens
sequenceDiagram
participant Client as OAuth Client
participant WebhookAPI as Webhook Endpoint<br/>app-credential.ts
participant DB as Database<br/>(Prisma)
participant Crypto as Decryption
Client->>WebhookAPI: POST /webhook/app-credential<br/>{userId, appSlug, keys}
WebhookAPI->>WebhookAPI: Validate webhook secret<br/>from header
alt Secret Invalid
WebhookAPI-->>Client: 403 Forbidden
end
WebhookAPI->>WebhookAPI: Parse & validate<br/>request body
alt Validation Fails
WebhookAPI-->>Client: 400 Bad Request
end
WebhookAPI->>DB: Verify user exists
alt User Not Found
WebhookAPI-->>Client: 404 Not Found
end
WebhookAPI->>Crypto: Decrypt keys using<br/>encryption key
WebhookAPI->>DB: Check for existing<br/>credential
alt Credential Exists
WebhookAPI->>DB: Update credential key
WebhookAPI-->>Client: 200 Updated
else No Existing Credential
WebhookAPI->>DB: Create new credential
WebhookAPI-->>Client: 200 Created
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/app-store/office365video/lib/VideoApiAdapter.ts (1)
61-99: Import and useparseRefreshTokenResponseto handle sync endpoint response format compatibility.The code currently bypasses the built-in sync endpoint validation. Unlike other adapters (Zoom, Office365Calendar, GoogleCalendar, Salesforce),
VideoApiAdapter.tsdoes not useparseRefreshTokenResponse(), which contains logic to accept the minimal response format (minimumTokenResponseSchema:access_token+ numeric expiry) fromCALCOM_CREDENTIAL_SYNC_ENDPOINTwhile falling back to strictITokenResponsevalidation for direct OAuth.When the sync endpoint is enabled,
refreshOAuthTokens()returns a response lacking requiredITokenResponsefields (token_type,scope), causinghandleErrorsJson<ITokenResponse>()to fail. Import and useparseRefreshTokenResponse()at line 77 (similar to the pattern inoffice365calendar/lib/CalendarService.ts:263) to ensure compatibility with both sync endpoint and direct OAuth token responses.packages/app-store/salesforce/api/callback.ts (1)
18-21: Fixcodevalidation condition to reject non-string valuesThe guard
if (code === undefined && typeof code !== "string") {will not catch the
string[]case (req.query.codecan bestring | string[] | undefined), so an array can slip through. Using an OR here better enforces the contract:- if (code === undefined && typeof code !== "string") { + if (code === undefined || typeof code !== "string") { res.status(400).json({ message: "`code` must be a string" }); return; }packages/app-store/zoomvideo/lib/VideoApiAdapter.ts (1)
333-353: Fix gzip handling and unreachable condition inhandleZoomResponseTwo issues here can cause runtime errors or hide failures:
- In the gzip branch,
_responseis reassigned to the result ofJSON.parse(responseString), which is a plain object; later calling_response.json()will throw at runtime.- The condition
response.status < 200 && response.status >= 300is impossible; this part of the check is effectively dead code.Since
handleZoomResponseis now used for both token refresh and API calls, tightening this is important. A safer implementation drops the broken gzip special case and simplifies error handling:-const handleZoomResponse = async (response: Response, credentialId: Credential["id"]) => { - let _response = response.clone(); - const responseClone = response.clone(); - if (_response.headers.get("content-encoding") === "gzip") { - const responseString = await response.text(); - _response = JSON.parse(responseString); - } - if (!response.ok || (response.status < 200 && response.status >= 300)) { - const responseBody = await _response.json(); +const handleZoomResponse = async (response: Response, credentialId: Credential["id"]) => { + if (!response.ok || response.status < 200 || response.status >= 300) { + let responseBody: any = {}; + try { + responseBody = await response.json(); + } catch { + // ignore JSON parsing errors; responseBody stays empty + } @@ - } - // handle 204 response code with empty response (causes crash otherwise as "" is invalid JSON) - if (response.status === 204) { - return; - } - return responseClone.json(); + } + // handle 204 response code with empty response (causes crash otherwise as "" is invalid JSON) + if (response.status === 204) { + return; + } + return response.json(); };packages/app-store/googlecalendar/lib/CalendarService.ts (1)
84-101: Fix return value handling in Google Calendar token refreshThe code assumes
refreshOAuthTokensalways returns an object with a.dataproperty, but the helper returns different types depending on the credential-sharing configuration:
- When credential sharing is enabled: returns a raw fetch
Responseobject (no.dataproperty)- When disabled: returns the result of
myGoogleAuth.refreshToken()which does have the expected structureThis causes
const token = res?.datato beundefinedwhen credential sharing is active, leading to a runtime error attoken.access_token.Other integrations (office365calendar, webex, zoomvideo) avoid this by normalizing the response with handler functions before accessing token properties. Add similar normalization here or ensure the return shape is consistent across both code paths in
refreshOAuthTokens.packages/app-store/salesforce/lib/CalendarService.ts (2)
8-9:prismais used but never imported
prisma.credential.update(...)will fail at compile/runtime becauseprismais not in this module’s imports.Suggested fix:
import { getLocation } from "@calcom/lib/CalEventParser"; import { WEBAPP_URL } from "@calcom/lib/constants"; import { HttpError } from "@calcom/lib/http-error"; import logger from "@calcom/lib/logger"; +import prisma from "@calcom/prisma";Also applies to: 96-99
75-108: Refresh flow updates DB but still uses stale tokens and has brittle status checkThe refresh logic has a few issues:
- It checks
response.statusText !== "OK"instead of usingresponse.ok/status, which is brittle.- After a successful refresh, the updated token payload is written to the DB, but
jsforce.Connectionis still constructed from the oldcredentialKey.instance_url/credentialKey.access_token, so the current request keeps using stale tokens.- In an
asyncfunction,return Promise.reject(...)is redundant vs throwing.You can address all three with something like:
- const response = await fetch("https://login.salesforce.com/services/oauth2/token", { - method: "POST", - body: new URLSearchParams({ - grant_type: "refresh_token", - client_id: consumer_key, - client_secret: consumer_secret, - refresh_token: credentialKey.refresh_token, - format: "json", - }), - }); - - if (response.statusText !== "OK") throw new HttpError({ statusCode: 400, message: response.statusText }); - - const accessTokenJson = await response.json(); - - const accessTokenParsed = parseRefreshTokenResponse(accessTokenJson, salesforceTokenSchema); - - if (!accessTokenParsed.success) { - return Promise.reject(new Error("Invalid refreshed tokens were returned")); - } - - await prisma.credential.update({ - where: { id: credential.id }, - data: { key: { ...accessTokenParsed.data, refresh_token: credentialKey.refresh_token } }, - }); - - 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({ + grant_type: "refresh_token", + client_id: consumer_key, + client_secret: consumer_secret, + refresh_token: credentialKey.refresh_token, + format: "json", + }), + }); + + if (!response.ok) { + throw new HttpError({ statusCode: response.status, message: response.statusText }); + } + + const accessTokenJson = await response.json(); + const accessTokenParsed = parseRefreshTokenResponse(accessTokenJson, salesforceTokenSchema); + + if (!accessTokenParsed.success) { + throw new Error("Invalid refreshed tokens were returned"); + } + + const refreshedTokens = { + ...accessTokenParsed.data, + refresh_token: credentialKey.refresh_token, + }; + + await prisma.credential.update({ + where: { id: credential.id }, + data: { key: refreshedTokens }, + }); + + return new jsforce.Connection({ + clientId: consumer_key, + clientSecret: consumer_secret, + redirectUri: WEBAPP_URL + "/api/integrations/salesforce/callback", + instanceUrl: refreshedTokens.instance_url, + accessToken: refreshedTokens.access_token, + refreshToken: refreshedTokens.refresh_token, + });
🧹 Nitpick comments (5)
packages/app-store/_utils/oauth/refreshOAuthTokens.ts (1)
5-5: Validate the CALCOM_CREDENTIAL_SYNC_ENDPOINT URL format.The environment variable is used directly without validation. If it contains an invalid URL, the fetch will throw at runtime.
Consider adding URL validation:
- if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId) { + if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId) { + try { + new URL(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT); + } catch { + console.error('Invalid CALCOM_CREDENTIAL_SYNC_ENDPOINT URL'); + return await refreshFunction(); + }packages/app-store/office365calendar/lib/CalendarService.ts (1)
20-21: Office 365 token refresh wiring matches the new helpers, with minor robustness tweaks possible
Importing
parseRefreshTokenResponse/refreshOAuthTokensand routing the refresh POST through:const response = await refreshOAuthTokens( async () => await fetch("https://login.microsoftonline.com/common/oauth2/v2.0/token", { ... }), "office365-calendar", credential.userId );is consistent with other integrations and preserves the original token-refresh request.
refreshTokenResponseSchemacorrectly normalizesexpires_ininto an absolute expiry:expires_in: z .number() .transform((currentTimeOffsetInSeconds) => Math.round(+new Date() / 1000 + currentTimeOffsetInSeconds)),and the
isExpiredcheck uses this as an epoch‑seconds value, which is coherent.In
refreshAccessToken, you merge parsed token data intoo365AuthCredentials:const tokenResponse = parseRefreshTokenResponse(responseJson, refreshTokenResponseSchema); o365AuthCredentials = { ...o365AuthCredentials, ...(tokenResponse.success && tokenResponse.data) };If
parseRefreshTokenResponseever returnssuccess: false, this silently leaves credentials unchanged but still returnso365AuthCredentials.access_token. Consider at least logging or throwing whensuccessis false so that malformed responses don’t cause repeated failed refresh attempts without visibility.Also applies to: 51-57, 244-265, 276-282
packages/lib/constants.ts (1)
103-104: CoerceAPP_CREDENTIAL_SHARING_ENABLEDto a boolean to avoid leaking secretsRight now this flag evaluates to the encryption key string when enabled, not a
boolean. That works in boolean conditions but increases the risk of accidentally logging or exposing the raw key if the flag is ever inspected directly.Recommend coercing to a
boolean:-export const APP_CREDENTIAL_SHARING_ENABLED = - process.env.CALCOM_WEBHOOK_SECRET && process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY; +export const APP_CREDENTIAL_SHARING_ENABLED = !!( + process.env.CALCOM_WEBHOOK_SECRET && process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY +);.env.example (1)
233-245: Env docs for app credential sync look clear; linter warnings are cosmeticThe new APP CREDENTIAL SYNC block is well documented and matches the feature flag wiring. The dotenv‑linter warnings about quotes and key ordering here are purely stylistic; you can safely ignore them or tidy later if you want a clean lint run (e.g., by dropping quotes on empty values and reordering keys).
Also applies to: 259-259
apps/web/pages/api/webhook/app-credential.ts (1)
61-92: Consider avoiding race conditions when creating/updating credentialsThe comment notes you “can’t use prisma upsert as we don’t know the id of the credential”, so you
findFirstthenupdate/create. If(userId, appId)is intended to be unique, concurrent webhooks for the same pair could both see no existing credential and both callcreate, leading to duplicates.If schema changes are acceptable, consider:
- Adding a unique constraint on
(userId, appId)in the Prisma model.- Switching this block to
prisma.credential.upsert({ where: { userId_appId: { userId, appId } }, ... }).If schema changes aren’t feasible right now, it’s at least worth documenting the potential for duplicates here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
.env.example(2 hunks)apps/web/pages/api/webhook/app-credential.ts(1 hunks)packages/app-store/_utils/oauth/createOAuthAppCredential.ts(1 hunks)packages/app-store/_utils/oauth/decodeOAuthState.ts(1 hunks)packages/app-store/_utils/oauth/encodeOAuthState.ts(1 hunks)packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts(1 hunks)packages/app-store/_utils/oauth/refreshOAuthTokens.ts(1 hunks)packages/app-store/googlecalendar/api/add.ts(1 hunks)packages/app-store/googlecalendar/api/callback.ts(1 hunks)packages/app-store/googlecalendar/lib/CalendarService.ts(2 hunks)packages/app-store/hubspot/api/add.ts(1 hunks)packages/app-store/hubspot/api/callback.ts(1 hunks)packages/app-store/hubspot/lib/CalendarService.ts(2 hunks)packages/app-store/larkcalendar/api/add.ts(1 hunks)packages/app-store/larkcalendar/api/callback.ts(1 hunks)packages/app-store/larkcalendar/lib/CalendarService.ts(2 hunks)packages/app-store/office365calendar/api/add.ts(1 hunks)packages/app-store/office365calendar/api/callback.ts(1 hunks)packages/app-store/office365calendar/lib/CalendarService.ts(2 hunks)packages/app-store/office365video/api/add.ts(1 hunks)packages/app-store/office365video/api/callback.ts(1 hunks)packages/app-store/office365video/lib/VideoApiAdapter.ts(2 hunks)packages/app-store/salesforce/api/add.ts(1 hunks)packages/app-store/salesforce/api/callback.ts(1 hunks)packages/app-store/salesforce/lib/CalendarService.ts(4 hunks)packages/app-store/stripepayment/api/callback.ts(1 hunks)packages/app-store/tandemvideo/api/callback.ts(1 hunks)packages/app-store/webex/api/callback.ts(1 hunks)packages/app-store/webex/lib/VideoApiAdapter.ts(2 hunks)packages/app-store/zoho-bigin/api/add.ts(2 hunks)packages/app-store/zoho-bigin/api/callback.ts(1 hunks)packages/app-store/zoho-bigin/lib/CalendarService.ts(2 hunks)packages/app-store/zohocrm/api/_getAdd.ts(1 hunks)packages/app-store/zohocrm/api/callback.ts(1 hunks)packages/app-store/zohocrm/lib/CalendarService.ts(2 hunks)packages/app-store/zoomvideo/api/add.ts(1 hunks)packages/app-store/zoomvideo/api/callback.ts(1 hunks)packages/app-store/zoomvideo/lib/VideoApiAdapter.ts(3 hunks)packages/lib/constants.ts(1 hunks)turbo.json(1 hunks)
🧰 Additional context used
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 237-237: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 239-239: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 239-239: [UnorderedKey] The CALCOM_WEBHOOK_HEADER_NAME key should go before the CALCOM_WEBHOOK_SECRET key
(UnorderedKey)
[warning] 240-240: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 240-240: [UnorderedKey] The CALCOM_CREDENTIAL_SYNC_ENDPOINT key should go before the CALCOM_WEBHOOK_HEADER_NAME key
(UnorderedKey)
[warning] 244-244: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 244-244: [UnorderedKey] The CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY key should go before the CALCOM_CREDENTIAL_SYNC_ENDPOINT key
(UnorderedKey)
🔇 Additional comments (32)
packages/app-store/office365video/api/callback.ts (1)
9-10: LGTM: Import path refactor aligns with OAuth utility reorganization.The updated import paths correctly reference the relocated OAuth utilities in the
oauthsubdirectory.packages/app-store/zohocrm/api/callback.ts (1)
10-11: LGTM: Consistent import path refactor.The OAuth utility imports are correctly updated to reference the new subdirectory location.
packages/app-store/zoho-bigin/api/callback.ts (1)
10-11: LGTM: Import paths updated correctly.The changes align with the OAuth utility reorganization across the app store modules.
packages/app-store/zoho-bigin/api/add.ts (1)
7-7: LGTM: Import path updated correctly.packages/app-store/office365video/lib/VideoApiAdapter.ts (1)
12-12: LGTM: Import added for token refresh utility.packages/app-store/_utils/oauth/encodeOAuthState.ts (1)
3-3: LGTM: Import path adjusted for file relocation.The relative import path is correctly updated after moving the file into the
oauthsubdirectory.packages/app-store/salesforce/api/add.ts (1)
7-7: LGTM: Import path updated for OAuth utility reorganization.The change correctly references the relocated
encodeOAuthStateutility in theoauthsubdirectory.packages/app-store/office365video/api/add.ts (1)
7-7: Import path refactor looks correctThe
encodeOAuthStateimport path update to the oauth utils subdirectory is consistent with the rest of the PR, with unchanged usage at Line 18.packages/app-store/office365calendar/api/add.ts (1)
7-7: Consistent OAuth state helper importThe
encodeOAuthStateimport now points to the sharedoauthutils path, matching other integrations; behavior of the handler remains the same.packages/app-store/office365calendar/api/callback.ts (1)
9-9: Shared decodeOAuthState import is aligned with refactorSwitching
decodeOAuthStateto theoauthutils path keeps the callback logic intact and aligns with the new OAuth helper layout.packages/app-store/googlecalendar/api/add.ts (1)
7-7: Google Calendar add handler import update is soundThe
encodeOAuthStateimport relocation to../../_utils/oauth/encodeOAuthStateis consistent with other integrations; no behavior changes in the auth URL generation.packages/app-store/larkcalendar/api/add.ts (1)
9-9: Lark OAuth state import aligned with shared utilsThe updated
encodeOAuthStateimport path correctly targets the centralized oauth utility, with the rest of the handler unchanged.packages/app-store/googlecalendar/api/callback.ts (1)
10-10: decodeOAuthState import refactor looks goodImporting
decodeOAuthStatefrom theoauthsubdirectory is consistent with the wider refactor; callback behavior remains identical.packages/app-store/salesforce/api/callback.ts (1)
9-10: Centralized OAuth helper imports are consistentUsing
createOAuthAppCredentialanddecodeOAuthStatefrom the sharedoauthutils directory matches the rest of the PR and keeps the callback flow the same.packages/app-store/zoomvideo/api/add.ts (1)
8-8: Zoom OAuth state helper import update is correctThe
encodeOAuthStateimport moved to the shared oauth utils path while its usage in the Zoom auth URL generation remains unchanged.packages/app-store/zohocrm/api/_getAdd.ts (1)
7-7: LGTM! Clean refactor.The import path update aligns with the OAuth utilities reorganization into the
oauthsubdirectory. The function usage remains unchanged and this is a straightforward refactor.packages/app-store/larkcalendar/api/callback.ts (1)
10-10: LGTM! Consistent refactor.The import path change correctly reflects the OAuth utilities reorganization. No functional changes.
packages/app-store/_utils/oauth/decodeOAuthState.ts (1)
3-3: LGTM! Correct path adjustment.The import path correctly adjusts for the file's new location in the
oauthsubdirectory. Going from"../types"to"../../types"is the expected change.packages/app-store/tandemvideo/api/callback.ts (1)
7-7: LGTM! Consistent refactor.The import path update aligns with the broader reorganization of OAuth utilities. No functional changes.
packages/app-store/stripepayment/api/callback.ts (1)
6-6: LGTM! Consistent refactor.The import path correctly reflects the OAuth utilities reorganization into the
oauthsubdirectory.packages/app-store/hubspot/api/add.ts (1)
7-7: LGTM! Consistent refactor.The import path update aligns with the OAuth utilities reorganization. Function usage remains unchanged.
packages/app-store/zoomvideo/api/callback.ts (1)
7-7: LGTM! Consistent refactor.The import path correctly reflects the OAuth utilities reorganization. No functional changes.
packages/app-store/larkcalendar/lib/CalendarService.ts (1)
14-14: Implementation follows established patterns; no issues found.The
refreshOAuthTokenswrapper is already widely adopted across the codebase in 8+ services (googlecalendar, zohocrm, zoho-bigin, office365calendar, hubspot, webex, zoomvideo, office365video). The larkcalendar integration uses the identical pattern: passing the async fetch operation, provider identifier ("lark-calendar"), and userId. Error handling viahandleLarkErroris preserved, and the change is consistent with all existing implementations.packages/app-store/_utils/oauth/createOAuthAppCredential.ts (1)
6-7: Import path refactor for OAuth helpers looks correctThe updated relative imports for
decodeOAuthStateandthrowIfNotHaveAdminAccessToTeammatch the new_utils/oauthlayout and keep the existing credential creation logic intact.packages/app-store/hubspot/api/callback.ts (1)
10-11: HubSpot callback now correctly targets relocated OAuth utilitiesSwitching
createOAuthAppCredentialanddecodeOAuthStateto the_utils/oauthsubfolder is consistent with the new structure; no behavioral changes introduced.packages/app-store/webex/api/callback.ts (1)
7-7: Webex callback import path updated consistentlyImporting
createOAuthAppCredentialfrom_utils/oauth/createOAuthAppCredentialaligns this file with the centralized OAuth helper layout; existing callback behavior is preserved.packages/app-store/zoomvideo/lib/VideoApiAdapter.ts (1)
12-13: Centralized Zoom token refresh integration looks consistent
- Using
refreshOAuthTokenswith the existing Zoom refresh POST preserves the previous request shape while routing through the new credential-sharing-aware helper ("zoomvideo",credential.userId), which matches patterns in other providers.parseRefreshTokenResponse(responseBody, zoomRefreshedTokenSchema)is correctly used to validate the refreshed token payload before mutating and persisting the credential key.- The subsequent update of
access_token,refresh_token, andexpiry_dateon the parsedzoomTokenSchemakey remains aligned with the earlier behavior, just with stronger validation.Also applies to: 79-95, 104-125
packages/app-store/googlecalendar/lib/CalendarService.ts (1)
21-22: Centralizing Google OAuth refresh via shared helpers looks consistentThe new imports align this service with the shared OAuth utilities and are fine as-is.
packages/app-store/webex/lib/VideoApiAdapter.ts (1)
11-12: Review comment is incorrect; no Response interface issue exists.Both branches of
refreshOAuthTokensreturn a nativefetch()Response object:
- Credential-sharing path (enabled): Returns
fetch(CALCOM_CREDENTIAL_SYNC_ENDPOINT, ...)- Non-sharing path (disabled): Returns the wrapped
refreshFunction()which isfetch("https://webexapis.com/v1/access_token", ...)The
handleWebexResponsefunction correctly uses all standard Response methods (clone(),headers,ok,status,text(),json()), which are fully available on the native Response object returned byrefreshOAuthTokens.Likely an incorrect or invalid review comment.
packages/app-store/zohocrm/lib/CalendarService.ts (1)
19-20: Fix three critical token validation bugs in Zoho CRM authTwo issues identified in the token expiry and validation logic:
isTokenValiduses inverted comparison (line 188)
ChecksexpiryDate < Date.now()(true when token IS expired) and returns true, butgetTokentreats true as "don't refresh." This means expired tokens are never refreshed.
Fix:- const isValid = token && token.access_token && token.expiryDate && token.expiryDate < Date.now(); + const isValid = token && token.access_token && token.expiryDate && token.expiryDate > Date.now();
expiryDatecalculation missing* 1000and ignores server value (line 219)
HardcodedDate.now() + 60 * 60adds only 3,600 milliseconds (~3.6 seconds) instead of the correct one hour. Per Zoho returnsexpires_inin seconds, use the response value:- zohoCrmTokenInfo.data.expiryDate = Math.round(Date.now() + 60 * 60); + zohoCrmTokenInfo.data.expiryDate = Math.round(Date.now() + (zohoCrmTokenInfo.data.expires_in * 1000));Same
expiryDatebug exists inapi/callback.tsat the same line.Likely an incorrect or invalid review comment.
turbo.json (1)
190-207: Env additions for credential sharing look consistentThe new
CALCOM_*entries inglobalEnvalign with the credential‑sharing feature and existing patterns for other secrets/feature flags; no issues from the Turborepo config side.apps/web/pages/api/webhook/app-credential.ts (1)
1-3: Review comment is incorrect; file follows established codebase conventionThe target file's Zod import (
import z from "zod";) is actually consistent with the codebase pattern, not inconsistent. A search across the codebase reveals 41 files using the default import style, with no evidence of the named import pattern (import { z } from "zod";) being prevalent. The suggested change would move the file away from the established convention, not toward it.Likely an incorrect or invalid review comment.
| const appCredentialWebhookRequestBodySchema = z.object({ | ||
| // UserId of the cal.com user | ||
| userId: z.number().int(), | ||
| appSlug: z.string(), | ||
| // Keys should be AES256 encrypted with the CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY | ||
| keys: z.string(), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Use safeParse for body validation and harden key decryption/parsing
Two related robustness issues:
appCredentialWebhookRequestBodySchema.parse(req.body)will throw on invalid input, returning a 500 instead of a clear 4xx.JSON.parse(symmetricDecrypt(...))is unguarded and passes""as a fallback encryption key, which can also throw and end up as 500s.
A more defensive pattern:
-const appCredentialWebhookRequestBodySchema = z.object({
- // UserId of the cal.com user
- userId: z.number().int(),
- appSlug: z.string(),
- // Keys should be AES256 encrypted with the CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY
- keys: z.string(),
-});
+const appCredentialWebhookRequestBodySchema = z.object({
+ // UserId of the cal.com user
+ userId: z.number().int(),
+ appSlug: z.string(),
+ // Keys should be AES256 encrypted with the CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY
+ keys: z.string(),
+});
@@
- const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body);
+ const parsedBody = appCredentialWebhookRequestBodySchema.safeParse(req.body);
+ if (!parsedBody.success) {
+ return res.status(400).json({ message: "Invalid request body" });
+ }
+ const reqBody = parsedBody.data;
@@
- // Decrypt the keys
- const keys = JSON.parse(
- symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "")
- );
+ // Decrypt the keys
+ const encryptionKey = process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY;
+ if (!encryptionKey) {
+ return res.status(500).json({ message: "Encryption key misconfigured" });
+ }
+
+ let keys;
+ try {
+ const decrypted = symmetricDecrypt(reqBody.keys, encryptionKey);
+ keys = JSON.parse(decrypted);
+ } catch {
+ return res.status(400).json({ message: "Invalid keys payload" });
+ }This keeps malformed bodies and invalid/mis‑encrypted keys from bubbling up as unhandled exceptions and gives clients clear 4xx/5xx feedback.
Also applies to: 31-32, 56-59, 72-92
I'll verify the review comment by examining the actual file and code patterns.
---
🏁 Script executed:
```shell
#!/bin/bash
# Fetch and examine the webhook file at the specified lines
cat apps/web/pages/api/webhook/app-credential.ts | head -100
Length of output: 2782
Use safeParse for body validation and harden key decryption/parsing
Two related robustness issues are confirmed in the code:
- Line 31:
appCredentialWebhookRequestBodySchema.parse(req.body)will throw on invalid input, returning a 500 instead of a clear 4xx. - Lines 56–59:
JSON.parse(symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || ""))is unguarded and passes""as a fallback encryption key, which can also throw and result in 500s instead of clear error responses.
A more defensive pattern:
- const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body);
+ const parsedBody = appCredentialWebhookRequestBodySchema.safeParse(req.body);
+ if (!parsedBody.success) {
+ return res.status(400).json({ message: "Invalid request body" });
+ }
+ const reqBody = parsedBody.data;- // Decrypt the keys
- const keys = JSON.parse(
- symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "")
- );
+ // Decrypt the keys
+ const encryptionKey = process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY;
+ if (!encryptionKey) {
+ return res.status(500).json({ message: "Encryption key misconfigured" });
+ }
+
+ let keys;
+ try {
+ const decrypted = symmetricDecrypt(reqBody.keys, encryptionKey);
+ keys = JSON.parse(decrypted);
+ } catch {
+ return res.status(400).json({ message: "Invalid keys payload" });
+ }This keeps malformed bodies and invalid/mis-encrypted keys from bubbling up as unhandled exceptions and gives clients clear 4xx/5xx feedback.
Committable suggestion skipped: line range outside the PR's diff.
| 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 | ||
| ) { | ||
| return res.status(403).json({ message: "Invalid webhook secret" }); | ||
| } |
There was a problem hiding this comment.
❓ Verification inconclusive
Add HTTP method guard and make webhook secret check header‑case safe
Right now the handler:
- Accepts any HTTP method (no 405 guard).
- Reads
req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"]directly. Node/Next lowercase header keys, so if the env header name contains uppercase characters, the lookup will fail and every request will be rejected.
You can make this stricter and more robust as follows:
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
- ) {
- return res.status(403).json({ message: "Invalid webhook secret" });
- }
+ if (req.method !== "POST") {
+ res.setHeader("Allow", "POST");
+ return res.status(405).json({ message: "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" });
+ }
+
+ const webhookHeaderName = (process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret").toLowerCase();
+ const providedSecret = req.headers[webhookHeaderName];
+ const expectedSecret = process.env.CALCOM_WEBHOOK_SECRET;
+
+ // Check that the webhook secret matches
+ if (
+ !expectedSecret ||
+ (Array.isArray(providedSecret)
+ ? !providedSecret.includes(expectedSecret)
+ : providedSecret !== expectedSecret)
+ ) {
+ return res.status(403).json({ message: "Invalid webhook secret" });
+ }This keeps the check case‑insensitive, deals with string | string[], and returns 405 for non‑POST methods.
I'll verify the key claims in this review by searching for documentation on how Node.js and Next.js handle HTTP headers.
Based on my verification, the review comment identifies valid and important issues. Here is the rewritten review:
Add HTTP method guard and make webhook secret check header-case safe
The handler currently:
- Accepts any HTTP method (no 405 guard)
- Reads
req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"]directly. Node has taken the pragmatic approach of lowercasing all incoming headers, so if the env header name contains uppercase characters, the lookup will fail and every request will be rejected.
Suggested improvements:
export default async function handler(req: NextApiRequest, res: NextApiResponse) {
+ if (req.method !== "POST") {
+ res.setHeader("Allow", "POST");
+ return res.status(405).json({ message: "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" });
}
+ const webhookHeaderName = (process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret").toLowerCase();
+ const providedSecret = req.headers[webhookHeaderName];
+ const expectedSecret = process.env.CALCOM_WEBHOOK_SECRET;
+
// Check that the webhook secret matches
if (
- req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] !==
- process.env.CALCOM_WEBHOOK_SECRET
+ !expectedSecret ||
+ (Array.isArray(providedSecret)
+ ? !providedSecret.includes(expectedSecret)
+ : providedSecret !== expectedSecret)
) {
return res.status(403).json({ message: "Invalid webhook secret" });
}This makes the check case-insensitive, handles the string | string[] case for headers, and returns 405 with an Allow header as required for method not allowed responses.
🤖 Prompt for AI Agents
In apps/web/pages/api/webhook/app-credential.ts around lines 17 to 29, add an
HTTP method guard and make the webhook-secret header check case- and type-safe:
only allow the intended method (e.g., POST) and if other methods are received
return 405 with an "Allow" header; derive the header key from
process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret" and normalize
it to lower-case before accessing req.headers (Node lowercases incoming
headers), then handle the header value possibly being a string[] by selecting
the first element (or treating undefined appropriately) and compare that string
to process.env.CALCOM_WEBHOOK_SECRET.
| 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(), | ||
| }); | ||
|
|
||
| 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"; | ||
| } | ||
|
|
||
| return refreshTokenResponse; | ||
| }; | ||
|
|
||
| export default parseRefreshTokenResponse; |
There was a problem hiding this comment.
🧩 Analysis chain
Fix parseRefreshTokenResponse schema and return type; current version will mis-parse and corrupt stored keys
There are a few serious issues here:
minimumTokenResponseSchemais effectively unusable
Using computed keys like:
[z.string().toString()]: z.number(),
[z.string().optional().toString()]: z.unknown().optional(),produces literal property names like "[object Object]", not an index signature. No real token response will have such keys, so minimumTokenResponseSchema.safeParse(response) will always fail when APP_CREDENTIAL_SHARING_ENABLED is true, causing "Invalid refreshed tokens were returned" even for valid responses.
- The helper returns the entire
safeParseresult, not the parsed token
Callers such as the Google calendar service pass the returned value directly into prisma.credential.update({ data: { key } }). With the current implementation, key becomes { success: true, data: { ... } } instead of the credential object expected by the corresponding Zod schema, so subsequent schema.parse(credential.key) calls will throw.
- Hard‑coding
refresh_token = "refresh_token"is dangerous
If a response genuinely omits refresh_token and the caller persists the returned object, you will overwrite any existing real refresh token with the literal string "refresh_token", breaking future refreshes.
At minimum, this helper should:
- Use a realistic “minimal token” schema (access token + optional numeric expiry fields), allowing passthrough for unknown properties.
- Return the parsed
dataobject, not thesafeParsewrapper. - Leave
refresh_tokenhandling to the caller (or at least not fabricate a placeholder string).
A safer implementation could look like this:
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(),
-});
-
-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";
- }
-
- return refreshTokenResponse;
-};
+const minimumTokenResponseSchema = z
+ .object({
+ access_token: z.string(),
+ // Common expiry fields used by providers; both optional.
+ expires_in: z.number().optional(),
+ expiry_date: z.number().optional(),
+ refresh_token: z.string().optional(),
+ })
+ // Allow provider‑specific extras without failing validation.
+ .passthrough();
+
+const parseRefreshTokenResponse = (response: unknown, schema: z.ZodTypeAny) => {
+ const tokenSchema =
+ APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT
+ ? minimumTokenResponseSchema
+ : schema;
+
+ const parsed = tokenSchema.safeParse(response);
+
+ if (!parsed.success) {
+ throw new Error("Invalid refreshed tokens were returned");
+ }
+
+ const data = parsed.data as any;
+
+ // Do not fabricate a refresh_token here; callers can decide how to handle
+ // missing refresh tokens (e.g., by reusing the previous one).
+
+ return data;
+};With this change, callers will receive a plain token object (matching either the provider schema or the minimal schema), and existing usages like the Google calendar service can safely persist it as credential.key without corrupting the stored structure.
🏁 Script executed:
#!/bin/bash
# After applying the change, run TypeScript build/tests focusing on OAuth flows.
pnpm test --filter "app-store..." -- --runInBand || pnpm test -- --runInBandLength of output: 385
Let me verify the codebase and search for how this function is being used.
🏁 Script executed:
#!/bin/bash
# Verify current implementation and find all call sites
echo "=== Current implementation ==="
cat packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts 2>/dev/null || echo "File not found"
echo -e "\n=== Call sites of parseRefreshTokenResponse ==="
rg -n "parseRefreshTokenResponse" --type ts --type tsx -g "!node_modules" -C 2Length of output: 1138
🏁 Script executed:
#!/bin/bash
# Try running tests with yarn (which the project uses)
cd /repo && yarn test --help 2>&1 | head -20Length of output: 55
Let me search for call sites with corrected syntax:
🏁 Script executed:
#!/bin/bash
# Find all call sites of parseRefreshTokenResponse
echo "=== Searching for parseRefreshTokenResponse imports and usage ==="
rg -n "parseRefreshTokenResponse" -g "!node_modules" -C 3
echo -e "\n=== Checking if APP_CREDENTIAL_SHARING_ENABLED is used anywhere ==="
rg -n "APP_CREDENTIAL_SHARING_ENABLED" -g "!node_modules"
echo -e "\n=== Checking CALCOM_CREDENTIAL_SYNC_ENDPOINT usage ==="
rg -n "CALCOM_CREDENTIAL_SYNC_ENDPOINT" -g "!node_modules"Length of output: 8800
🏁 Script executed:
#!/bin/bash
# Get full context around Google Calendar usage
echo "=== Google Calendar usage context ==="
rg -n "parseRefreshTokenResponse" packages/app-store/googlecalendar/lib/CalendarService.ts -A 10 -B 5
echo -e "\n=== Salesforce usage context ==="
rg -n "parseRefreshTokenResponse" packages/app-store/salesforce/lib/CalendarService.ts -A 10 -B 5
echo -e "\n=== Office365 usage context ==="
rg -n "parseRefreshTokenResponse" packages/app-store/office365calendar/lib/CalendarService.ts -A 10 -B 5Length of output: 3713
🏁 Script executed:
#!/bin/bash
# Get Zoom usage context
echo "=== Zoom usage context ==="
rg -n "parseRefreshTokenResponse" packages/app-store/zoomvideo/lib/VideoApiAdapter.ts -A 15 -B 5
# Check what happens when credentials are read from database
echo -e "\n=== Check how credentials are used after stored ==="
rg -n "googleCredentialSchema" packages/app-store/googlecalendar/lib/CalendarService.ts -B 2 -A 2Length of output: 2987
🏁 Script executed:
#!/bin/bash
# Verify what happens when Google credentials are parsed after being stored with wrapper
echo "=== Google credential schema definition ==="
cat packages/app-store/googlecalendar/lib/googleCredentialSchema.ts 2>/dev/null || rg -n "googleCredentialSchema" packages/app-store/googlecalendar/lib/ -A 5 -B 2
# Check if there's any defensive parsing or error handling
echo -e "\n=== Check error handling around line 75 ==="
rg -n "googleCredentialSchema.parse" packages/app-store/googlecalendar/lib/CalendarService.ts -A 3 -B 3Length of output: 591
Fix parseRefreshTokenResponse implementation and update all callers; current code causes Google Calendar failures and proposed fix is incomplete
The review correctly identifies three critical issues:
-
minimumTokenResponseSchemacomputed keys are broken:[z.string().toString()]: z.number()produces literal property names like"[object Object]", not a pattern. The schema will never validate real token responses, and any call will throw "Invalid refreshed tokens were returned". -
Google Calendar stores corrupted data: At line 97,
parseRefreshTokenResponsereturns a Zod SafeParseReturnType object{ success: true, data: {...} }instead of just the data. This gets stored directly at line 98-100 viaprisma.credential.update({data: {key}}). When credentials are later parsed at line 75 withgoogleCredentialSchema.parse(credential.key), it fails because the stored object hassuccessanddataproperties instead of the expected{scope, token_type, expiry_date, access_token, refresh_token}. -
Hard-coding
refresh_token = "refresh_token"is dangerous: If a response omitsrefresh_tokenand gets persisted, you overwrite the stored token with a literal placeholder string.
Critical note on the proposed fix: The suggestion to return parsed.data directly will fix Google Calendar but will break Zoom (line 104-111), Salesforce (line 90-98), and Office365 (line 263-264), all of which check the .success property and access .data. The proposed diff does not update these three call sites, so following it would introduce new failures.
A proper fix requires either:
- Returning
.datadirectly AND updating Zoom/Salesforce/Office365 to not check.success, or - Keeping SafeParseReturnType return and fixing only the schema + Google Calendar call site
At minimum, minimumTokenResponseSchema must use .passthrough() or explicit optional fields like expires_in, expiry_date, and refresh_token, not computed keys.
| @@ -0,0 +1,22 @@ | |||
| import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants"; | |||
|
|
|||
| const refreshOAuthTokens = async (refreshFunction: () => any, appSlug: string, userId: number | null) => { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace any return type with a proper type definition.
The function signature uses any for both the refreshFunction parameter and return type, which eliminates type safety. This makes it difficult to catch type errors at compile time and reduces code maintainability.
Consider defining an interface for the token response:
+interface TokenRefreshResponse {
+ access_token: string;
+ expiry_date: number;
+ refresh_token?: string;
+ [key: string]: any;
+}
+
-const refreshOAuthTokens = async (refreshFunction: () => any, appSlug: string, userId: number | null) => {
+const refreshOAuthTokens = async (
+ refreshFunction: () => Promise<Response>,
+ appSlug: string,
+ userId: number | null
+): Promise<Response> => {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/app-store/_utils/oauth/refreshOAuthTokens.ts around line 3, the
function uses `any` for the refreshFunction parameter and its return type;
replace `any` with a concrete type by defining an interface (e.g. TokenResponse
with accessToken, refreshToken, expiresIn, scope, etc.) or a generic like <T
extends TokenResponse> and update the refreshFunction signature to return
Promise<T> (or T) and the refreshOAuthTokens return type to Promise<T> (or T).
Update callers to pass functions that match the new type or specify the generic,
and import/export the interface so the token shape is consistently enforced
across the codebase.
| const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, { | ||
| method: "POST", | ||
| body: new URLSearchParams({ | ||
| calcomUserId: userId.toString(), | ||
| appSlug, | ||
| }), | ||
| }); | ||
| return response; |
There was a problem hiding this comment.
Add error handling and timeout for the credential sync endpoint call.
The fetch request to CALCOM_CREDENTIAL_SYNC_ENDPOINT lacks error handling and timeout configuration, which could lead to:
- Unhandled network failures or HTTP errors
- Indefinite hanging if the endpoint is unresponsive
- Poor user experience during OAuth flows
Apply this diff to add proper error handling and timeout:
- const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
- method: "POST",
- body: new URLSearchParams({
- calcomUserId: userId.toString(),
- appSlug,
- }),
- });
- return response;
+ try {
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 10000); // 10s timeout
+
+ const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
+ method: "POST",
+ body: new URLSearchParams({
+ calcomUserId: userId.toString(),
+ appSlug,
+ }),
+ signal: controller.signal,
+ });
+
+ clearTimeout(timeoutId);
+
+ if (!response.ok) {
+ console.error(`Credential sync failed: ${response.status} ${response.statusText}`);
+ // Fall back to local refresh on sync endpoint failure
+ return await refreshFunction();
+ }
+
+ return response;
+ } catch (error) {
+ console.error('Credential sync endpoint error:', error);
+ // Fall back to local refresh on network error
+ return await refreshFunction();
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/app-store/_utils/oauth/refreshOAuthTokens.ts around lines 8 to 15,
the POST to CALCOM_CREDENTIAL_SYNC_ENDPOINT lacks timeout and error handling;
wrap the fetch in a try/catch, validate the environment variable first, create
an AbortController with a sensible timeout (e.g. 5s) and pass its signal to
fetch, check response.ok and throw or return a clear error if the status is not
2xx, ensure you clear the timeout on success, and surface or log the caught
error so callers can handle failures instead of hanging indefinitely.
| import refreshOAuthTokens from "../../_utils/oauth/refreshOAuthTokens"; | ||
| import type { HubspotToken } from "../api/callback"; |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm refreshOAuthTokens returns a HubspotToken object in this path
Here refreshOAuthTokens is expected to return a plain HubspotToken (used directly for expiresIn, accessToken, etc.), whereas other integrations (e.g., Zoho, Webex) appear to expect an HTTP response–like shape with .data.
Please verify that, for the "hubspot" provider, refreshOAuthTokens normalizes its result to the HubspotToken structure rather than an Axios-style { data } wrapper; otherwise this code will break at runtime.
Also applies to: 175-205
🏁 Script executed:
#!/bin/bash
# Quickly inspect the implementation of refreshOAuthTokens and confirm branching per provider.
fd 'refreshOAuthTokens.ts' -a
if [ -f "packages/app-store/_utils/oauth/refreshOAuthTokens.ts" ]; then
sed -n '1,260p' packages/app-store/_utils/oauth/refreshOAuthTokens.ts
fiLength of output: 923
I need to examine how the return value from refreshOAuthTokens is being used in HubSpot's CalendarService to understand the impact.
I need to inspect the actual HubSpot CalendarService implementation to see how the return value from refreshOAuthTokens is being used.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Inspect HubSpot CalendarService refreshAccessToken method (lines 175-205)
if [ -f "packages/app-store/hubspot/lib/CalendarService.ts" ]; then
sed -n '175,205p' packages/app-store/hubspot/lib/CalendarService.ts
fiLength of output: 1011
I need to understand what tokensApi.createToken() returns and compare with other providers to confirm the issue.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Search for tokensApi.createToken usage and check other provider implementations
rg -n "tokensApi\.createToken" --type=ts -A2 -B2 packages/app-store/hubspot/
# Also check other providers for pattern comparison
echo "=== Zoho pattern ==="
rg -n "refreshOAuthTokens" packages/app-store/zoho/ --type=ts -A5 -B2 | head -30
echo "=== Webex pattern ==="
rg -n "refreshOAuthTokens" packages/app-store/webex/ --type=ts -A5 -B2 | head -30Length of output: 2499
I need to see how Webex actually handles the response to understand the expected pattern.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# See how Webex uses the response from refreshOAuthTokens
sed -n '60,85p' packages/app-store/webex/lib/VideoApiAdapter.ts
# Also check if there's any .json() or .data extraction in Webex
rg -n "response\." packages/app-store/webex/lib/VideoApiAdapter.ts -A2 -B1 | head -40Length of output: 2576
The verification confirms the issue raised in the review comment. Let me check the HubSpot token type definition to be thorough.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Verify HubspotToken type definition
cat packages/app-store/hubspot/api/callback.ts | grep -A10 "type HubspotToken"
# Also verify tokensApi.createToken return type by checking the import
rg -n "hubspotClient" packages/app-store/hubspot/lib/CalendarService.ts -B5 | head -20Length of output: 615
Confirm refreshOAuthTokens returns inconsistent types that will cause runtime failure in HubSpot
The review comment is correct. refreshOAuthTokens has two return paths:
- When
APP_CREDENTIAL_SHARING_ENABLEDis true: returns aResponseobject fromfetch() - When disabled: returns the result from the callback function (
tokensApi.createToken())
The HubSpot code at lines 191–192 directly accesses .expiresIn and .accessToken without extracting from a Response object. This means:
- If the sync endpoint is enabled, the code will crash with "Cannot read properties of undefined" because
Responseobjects don't have these fields - If the sync endpoint is disabled, it works because
tokensApi.createToken()returns the token object directly
Unlike Webex (which calls handleWebexResponse() to extract .json() from the Response), HubSpot has no such handler. The wrapper function needs to return a consistent type or HubSpot's code needs to handle the Response case.
| 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.
🛠️ Refactor suggestion | 🟠 Major
Hardcoded redirect URI creates inconsistency with callback handler.
The redirect URI is now hardcoded as "zoho-bigin", while the callback handler at packages/app-store/zoho-bigin/api/callback.ts line 36 uses the dynamic ${appConfig.slug} pattern. Although they currently resolve to the same value, this inconsistency could cause OAuth flow failures if appConfig.slug changes or differs in any environment.
Apply this diff to maintain consistency:
- const redirectUri = WEBAPP_URL + `/api/integrations/zoho-bigin/callback`;
+ const redirectUri = WEBAPP_URL + `/api/integrations/${appConfig.slug}/callback`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const redirectUri = WEBAPP_URL + `/api/integrations/zoho-bigin/callback`; | |
| const redirectUri = WEBAPP_URL + `/api/integrations/${appConfig.slug}/callback`; |
🤖 Prompt for AI Agents
In packages/app-store/zoho-bigin/api/add.ts around line 17, the redirectUri is
hardcoded to use "zoho-bigin" which is inconsistent with the callback handler
that uses the dynamic appConfig.slug; replace the literal segment with the same
dynamic value (use appConfig.slug) when building WEBAPP_URL +
`/api/integrations/${appConfig.slug}/callback`, ensuring appConfig is
imported/available in this file and that the resulting URI matches the callback
handler pattern.
| import type { CredentialPayload } from "@calcom/types/Credential"; | ||
|
|
||
| import getAppKeysFromSlug from "../../_utils/getAppKeysFromSlug"; | ||
| import refreshOAuthTokens from "../../_utils/oauth/refreshOAuthTokens"; |
There was a problem hiding this comment.
🧩 Analysis chain
Token refresh centralization is good; double‑check expiry units and context ID
The switch to refreshOAuthTokens keeps the Zoho Bigin refresh POST intact while plugging into the shared OAuth refresh infrastructure, which is a solid direction.
Two things to verify here:
-
expiryDatecalculation likely needs unit conversionconst isTokenValid = (token: BiginToken) => token.access_token && token.expiryDate && token.expiryDate > Date.now(); ... tokenInfo.data.expiryDate = Math.round(Date.now() + tokenInfo.data.expires_in);
Date.now()is milliseconds since epoch, whereas OAuth-styleexpires_infields are typically expressed in seconds. If Zoho Bigin follows that convention, you should multiply by1000:
- tokenInfo.data.expiryDate = Math.round(Date.now() + tokenInfo.data.expires_in);
- tokenInfo.data.expiryDate = Math.round(Date.now() + tokenInfo.data.expires_in * 1000);
Otherwise tokens will be treated as expiring ~1000× earlier than intended.
-
Consistency of the third
refreshOAuthTokensargumentHere the third argument is
credentialId:const tokenInfo = await refreshOAuthTokens( async () => await axios.post(...), "zoho-bigin", credentialId );
In other integrations (e.g., Zoom and Office 365) this parameter is
credential.userId. IfrefreshOAuthTokensexpects a user identifier (for centralized credential sharing), passing the credential’s DB id may cause mismatches. Either align this tocredential.userIdor explicitly document that this argument is meant to be a credential id and make other call sites consistent.
Also applies to: 55-57, 85-99
🏁 Script executed:
#!/bin/bash
# Inspect all usages and the implementation of refreshOAuthTokens
rg -n "refreshOAuthTokens" -C4Length of output: 17148
🏁 Script executed:
#!/bin/bash
# Extract the exact third argument for each refreshOAuthTokens call site
echo "=== Zoom VideoApiAdapter ===" && sed -n '75,95p' packages/app-store/zoomvideo/lib/VideoApiAdapter.ts
echo -e "\n=== Zoho Bigin CalendarService ===" && sed -n '80,100p' packages/app-store/zoho-bigin/lib/CalendarService.ts
echo -e "\n=== Office 365 Calendar ===" && sed -n '240,260p' packages/app-store/office365calendar/lib/CalendarService.ts
echo -e "\n=== Office 365 Video ===" && sed -n '55,75p' packages/app-store/office365video/lib/VideoApiAdapter.ts
echo -e "\n=== Webex ===" && sed -n '58,78p' packages/app-store/webex/lib/VideoApiAdapter.tsLength of output: 3855
Token refresh centralization is good; fix expiry units and align context ID with other integrations
The switch to refreshOAuthTokens properly centralizes OAuth refresh, but two issues need correction:
-
expiryDatecalculation requires unit conversion (line 98)tokenInfo.data.expiryDate = Math.round(Date.now() + tokenInfo.data.expires_in);
Date.now()returns milliseconds, whereas OAuth-compliantexpires_infields are in seconds. Without multiplying by1000, tokens expire ~1000× earlier than intended:
- tokenInfo.data.expiryDate = Math.round(Date.now() + tokenInfo.data.expires_in);
- tokenInfo.data.expiryDate = Math.round(Date.now() + tokenInfo.data.expires_in * 1000);
-
Align the third argument from
credentialIdtocredential.userId(line 85)const tokenInfo = await refreshOAuthTokens( async () => await axios.post(...), "zoho-bigin", credentialId // ← should be credential.userId );
All other integrations (Zoom, Office 365, Webex, etc.) pass
credential.userId. The function signature expects a user ID (userId: number | null) for credential sharing. UsingcredentialIdinstead will cause mismatches in centralized credential sync.
🤖 Prompt for AI Agents
In packages/app-store/zoho-bigin/lib/CalendarService.ts around lines 85 and 98,
two fixes are needed: call refreshOAuthTokens with credential.userId (not
credentialId) as the third argument to match other integrations and the
function's expected userId, and adjust expiryDate calculation to use
milliseconds by multiplying the OAuth expires_in (seconds) by 1000 when adding
to Date.now() (e.g., Date.now() + expires_in * 1000), keeping the rounding logic
intact.
2 participants
Test 8
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/cal.com-coderabbit#8