OAuth credential sync and app integration enhancements#5
Open
ShashankFC wants to merge 1 commit into
Open
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>
Author
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. |
codeant-ai
Bot
added
the
size:L
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+24
to
+27
| if ( | ||
| req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] !== | ||
| process.env.CALCOM_WEBHOOK_SECRET | ||
| ) { |
There was a problem hiding this comment.
Suggestion: The header name lookup for the webhook secret uses the environment variable value directly, but Node/Next.js normalizes header keys to lowercase, so if CALCOM_WEBHOOK_HEADER_NAME is set with any uppercase characters the lookup will fail and valid requests will be rejected; lowercasing the configured header name and normalizing potential array values avoids this logic error. [logic error]
Severity Level: Major ⚠️
- ❌ App credential webhook rejects valid requests with non-lowercase header configuration.
- ⚠️ Credential synchronization between apps silently fails under misconfigured header name.
Suggested change
| if ( | |
| req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] !== | |
| process.env.CALCOM_WEBHOOK_SECRET | |
| ) { | |
| const webhookHeaderName = (process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret").toLowerCase(); | |
| const webhookHeaderValue = req.headers[webhookHeaderName]; | |
| const receivedSecret = Array.isArray(webhookHeaderValue) ? webhookHeaderValue[0] : webhookHeaderValue; | |
| if (receivedSecret !== process.env.CALCOM_WEBHOOK_SECRET) { |
Steps of Reproduction ✅
1. Configure the environment variable `CALCOM_WEBHOOK_HEADER_NAME` to a mixed-case header
name, for example `X-Calcom-Webhook-Secret`, instead of the lowercase default shown in
`.env.example:239`.
2. Start the Next.js app so that the API route
`apps/web/pages/api/webhook/app-credential.ts:17-93` is available at `POST
/api/webhook/app-credential`.
3. Send a POST request to `/api/webhook/app-credential` with a valid JSON body matching
the schema at `apps/web/pages/api/webhook/app-credential.ts:9-15` (`userId`, `appSlug`,
`keys`) and include the HTTP header `X-Calcom-Webhook-Secret` set to the same value as
`CALCOM_WEBHOOK_SECRET` (see `.env.example:237`).
4. In the handler at `apps/web/pages/api/webhook/app-credential.ts:23-29`, Next.js/Node
exposes headers in `req.headers` only under lowercase keys (e.g.
`x-calcom-webhook-secret`), but the code indexes
`req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME]` using the mixed-case string
`X-Calcom-Webhook-Secret`, so the lookup returns `undefined`, making the comparison
`undefined !== process.env.CALCOM_WEBHOOK_SECRET` true and returning `403 { "message":
"Invalid webhook secret" }` even though the correct secret was sent.
5. Additionally, if an upstream proxy or client sends the configured secret header twice,
Node.js will populate `req.headers[headerName]` as a `string[]` (per `NextApiRequest`
header typing), so the existing code compares an array to
`process.env.CALCOM_WEBHOOK_SECRET`, which is always unequal and again returns `403
"Invalid webhook secret"`; the improved code normalizes this by unwrapping array values.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** apps/web/pages/api/webhook/app-credential.ts
**Line:** 24:27
**Comment:**
*Logic Error: The header name lookup for the webhook secret uses the environment variable value directly, but Node/Next.js normalizes header keys to lowercase, so if `CALCOM_WEBHOOK_HEADER_NAME` is set with any uppercase characters the lookup will fail and valid requests will be rejected; lowercasing the configured header name and normalizing potential array values avoids this logic error.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+5
to
+11
| 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(), | ||
| }); |
There was a problem hiding this comment.
Suggestion: The minimumTokenResponseSchema is defined using computed property names based on z.string().toString(), which evaluates to the literal key "[object Object]", so any real token response without such a key will fail safeParse when credential sharing is enabled, causing valid refresh responses to be rejected; defining a minimal schema that requires only access_token and allows arbitrary additional fields fixes this logic error. [logic error]
Severity Level: Critical 🚨
- ❌ Zoom token refresh fails when credential sharing enabled.
- ❌ Zoom meeting creation fails once token needs refresh.
- ❌ Office365 calendar token refresh fails under sharing config.
- ❌ Salesforce calendar token refresh fails under sharing config.
- ❌ Google calendar token refresh fails under sharing config.
Suggested change
| 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 minimumTokenResponseSchema = z | |
| .object({ | |
| access_token: z.string(), | |
| }) | |
| .catchall(z.unknown()); |
Steps of Reproduction ✅
1. Configure the environment so that credential sharing is enabled:
- Set `CALCOM_WEBHOOK_SECRET` and `CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY` so that
`APP_CREDENTIAL_SHARING_ENABLED` becomes truthy (`packages/lib/constants.ts:103`).
- Set `CALCOM_CREDENTIAL_SYNC_ENDPOINT` to a valid HTTP URL so the condition in
`parseRefreshTokenResponse` and `refreshOAuthTokens` can be true
(`packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts:15`,
`packages/app-store/_utils/oauth/refreshOAuthTokens.ts:5`).
2. Use a Zoom credential whose access token is expired so that a refresh is required:
- Zoom token validity is checked in `isTokenValid`
(`packages/app-store/zoomvideo/lib/VideoApiAdapter.ts:63`), and `zoomAuth.getToken()`
calls `refreshAccessToken` when a refresh is needed
(`packages/app-store/zoomvideo/lib/VideoApiAdapter.ts:128-139`).
3. Trigger any code path that creates or updates a Zoom meeting, which uses
`ZoomVideoApiAdapter`:
- `ZoomVideoApiAdapter` constructs `fetchZoomApi` which calls
`zoomAuth(credential).getToken()` to obtain an access token before API calls
(`packages/app-store/zoomvideo/lib/VideoApiAdapter.ts:235-243`).
- `createMeeting` uses `fetchZoomApi("users/me/meetings", ...)`
(`packages/app-store/zoomvideo/lib/VideoApiAdapter.ts:267-275`), so calling
`createMeeting` on an adapter instance with the expired credential will invoke
`zoomAuth.getToken()` and then `refreshAccessToken`.
4. Observe the failing token parse when credential sharing is enabled:
- Inside `refreshAccessToken` for Zoom, tokens are fetched via `refreshOAuthTokens`
(`packages/app-store/zoomvideo/lib/VideoApiAdapter.ts:74-95`), which, under the same
`APP_CREDENTIAL_SHARING_ENABLED && CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId`
condition, calls the credential sync endpoint instead of the provider
(`packages/app-store/_utils/oauth/refreshOAuthTokens.ts:3-15`).
- The HTTP `Response` from that endpoint is converted to JSON by `handleZoomResponse`
(`packages/app-store/zoomvideo/lib/VideoApiAdapter.ts:333-352`), producing
`responseBody`.
- `parseRefreshTokenResponse(responseBody, zoomRefreshedTokenSchema)` is then called
(`packages/app-store/zoomvideo/lib/VideoApiAdapter.ts:104`).
- Because `APP_CREDENTIAL_SHARING_ENABLED` and `CALCOM_CREDENTIAL_SYNC_ENDPOINT` are
set, `parseRefreshTokenResponse` uses `minimumTokenResponseSchema.safeParse(response)`
instead of the integration-specific schema
(`packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts:13-19`).
- `minimumTokenResponseSchema` currently includes a computed key
`[z.string().toString()]: z.number()`
(`packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts:5-10`), which evaluates
to a literal property name such as `"[object Object]"`. Real token JSON from a
credential sync endpoint (or any OAuth provider) will not include such a key, so
`safeParse` returns `{ success: false, ... }`.
- `parseRefreshTokenResponse` checks `if (!refreshTokenResponse.success)` and throws
`new Error("Invalid refreshed tokens were returned")`
(`packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts:21-22`), causing the
refresh to fail and bubbling an error back to `refreshAccessToken`,
`zoomAuth.getToken`, and ultimately `ZoomVideoApiAdapter.createMeeting`.
5. The same failure pattern applies to other integrations whenever credential sharing is
enabled:
- Salesforce: `CalendarService.getClient` parses the Salesforce OAuth response via
`parseRefreshTokenResponse(accessTokenJson, salesforceTokenSchema)`
(`packages/app-store/salesforce/lib/CalendarService.ts:75-90`), which will also
incorrectly use `minimumTokenResponseSchema` under the same environment conditions,
rejecting valid Salesforce responses.
- Office 365: `Office365CalendarService.o365Auth.refreshAccessToken` calls
`parseRefreshTokenResponse(responseJson, refreshTokenResponseSchema)`
(`packages/app-store/office365calendar/lib/CalendarService.ts:244-264`), which again
uses `minimumTokenResponseSchema` and rejects valid token responses when sharing is
enabled.
- Google Calendar: `GoogleCalendarService.googleAuth.refreshAccessToken` calls
`parseRefreshTokenResponse(googleCredentials, googleCredentialSchema)`
(`packages/app-store/googlecalendar/lib/CalendarService.ts:84-101`); with sharing
enabled, this also runs through `minimumTokenResponseSchema` and throws, breaking
Google token refresh.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts
**Line:** 5:11
**Comment:**
*Logic Error: The `minimumTokenResponseSchema` is defined using computed property names based on `z.string().toString()`, which evaluates to the literal key `"[object Object]"`, so any real token response without such a key will fail `safeParse` when credential sharing is enabled, causing valid refresh responses to be rejected; defining a minimal schema that requires only `access_token` and allows arbitrary additional fields fixes this logic error.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
94
to
+97
| const token = res?.data; | ||
| googleCredentials.access_token = token.access_token; | ||
| googleCredentials.expiry_date = token.expiry_date; | ||
| const key = googleCredentialSchema.parse(googleCredentials); | ||
| const key = parseRefreshTokenResponse(googleCredentials, googleCredentialSchema); |
There was a problem hiding this comment.
Suggestion: The refreshed token handling assumes that refreshOAuthTokens always returns an object with a .data property and also writes the raw parseRefreshTokenResponse result into key, which is incorrect: when credential sharing is enabled refreshOAuthTokens returns a Response without .data, causing token.access_token to throw, and parseRefreshTokenResponse returns a Zod parse result (with { success, data, ... }), so storing it directly corrupts the credential shape and will break future googleCredentialSchema.parse calls. Instead, normalize the response by checking for .data vs .json(), pass the actual token payload into parseRefreshTokenResponse, and persist only its .data as the credential key. [logic error]
Severity Level: Critical 🚨
- ❌ Google Calendar token refresh crashes when sharing feature enabled.
- ❌ Persisted Google credentials become invalid after successful refresh.
- ❌ Subsequent GoogleCalendarService construction fails Zod parsing.
- ❌ Google event creation fails once credentials are corrupted.
- ❌ Google availability and calendar listing fail after first refresh.
Suggested change
| const token = res?.data; | |
| googleCredentials.access_token = token.access_token; | |
| googleCredentials.expiry_date = token.expiry_date; | |
| const key = googleCredentialSchema.parse(googleCredentials); | |
| const key = parseRefreshTokenResponse(googleCredentials, googleCredentialSchema); | |
| let tokenResponse: any; | |
| if (res && typeof (res as any).data !== "undefined") { | |
| tokenResponse = (res as any).data; | |
| } else if (res && typeof (res as any).json === "function") { | |
| tokenResponse = await (res as any).json(); | |
| } else { | |
| tokenResponse = res; | |
| } | |
| const parsedToken = parseRefreshTokenResponse(tokenResponse, googleCredentialSchema); | |
| const tokenData = parsedToken.data as any; | |
| googleCredentials.access_token = tokenData.access_token; | |
| googleCredentials.expiry_date = tokenData.expiry_date; | |
| const key = parsedToken.data; |
Steps of Reproduction ✅
1. Deploy the backend with this PR and configure a Google Calendar integration so that a
`Credential` row exists and is loaded into `GoogleCalendarService` (constructor at
`packages/app-store/googlecalendar/lib/CalendarService.ts:66-72`), where `this.auth` is
initialized via `this.googleAuth(credential)` (line 69).
2. Trigger any Google Calendar operation that uses an authenticated client, for example
event creation via `GoogleCalendarService.createEvent` at `CalendarService.ts:181-252`,
which calls `this.authedCalendar()` at line 207, or availability fetching via
`getAvailability` at lines 414-451; both call `authedCalendar` at lines 130-137.
3. Inside `authedCalendar` (lines 130-137), `this.auth.getToken()` (lines 122-125)
constructs a `MyGoogleAuth` instance via `getGoogleAuth` (lines 77-81) and checks
`myGoogleAuth.isTokenExpiring()`. Once Google's OAuth2 token is considered expiring,
`getToken` calls `refreshAccessToken(myGoogleAuth)` at line 125.
4. In `refreshAccessToken` (lines 84-120), the code calls `refreshOAuthTokens` at lines
86-93. When app credential sharing is enabled (`APP_CREDENTIAL_SHARING_ENABLED` true,
`CALCOM_CREDENTIAL_SYNC_ENDPOINT` set, and `credential.userId` truthy),
`refreshOAuthTokens` (implementation at
`packages/app-store/_utils/oauth/refreshOAuthTokens.ts:3-19`) enters its first branch
(lines 5-15) and returns a `fetch` `Response` object without a `.data` property.
5. Back in `CalendarService.refreshAccessToken`, the returned `Response` is assigned to
`res`, and the code executes `const token = res?.data;` at line 94. Because `Response` has
no `.data` property, `token` is `undefined`, and `googleCredentials.access_token =
token.access_token;` at line 95 throws a runtime `TypeError` (`Cannot read properties of
undefined`), causing the Google Calendar operation
(createEvent/getAvailability/listCalendars/deleteEvent) to fail at the point of token
refresh.
6. In environments where credential sharing is disabled (the `else` branch in
`refreshOAuthTokens.ts:16-19`), `refreshOAuthTokens` returns the underlying Google OAuth
client response (`fetchTokens.res` from `MyGoogleAuth.refreshToken` at
`CalendarService.ts:88-89`), which *does* have a `.data` property, so lines 94-96 succeed.
However, the code then calls `parseRefreshTokenResponse(googleCredentials,
googleCredentialSchema)` at line 97, passing the already-parsed credential object instead
of the token response JSON.
7. `parseRefreshTokenResponse` (implementation at
`packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts:13-29`) uses
`schema.safeParse(response)` in this non-sharing case (line 18), returning a Zod
safe-parse result `{ success, data, ... }`. That wrapper object is assigned directly to
`key` at line 97 and persisted to `prisma.credential` as `data: { key }` at lines 98-101.
8. On any subsequent use of this credential, `GoogleCalendarService.googleAuth` is invoked
again and immediately calls `googleCredentialSchema.parse(credential.key)` at line 75.
Since `credential.key` now holds the *safe-parse result object* instead of the expected
`googleCredentialSchema` shape, Zod validation fails and throws, preventing any further
use of this Google Calendar credential across all operations that instantiate
`GoogleCalendarService`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/app-store/googlecalendar/lib/CalendarService.ts
**Line:** 94:97
**Comment:**
*Logic Error: The refreshed token handling assumes that `refreshOAuthTokens` always returns an object with a `.data` property and also writes the raw `parseRefreshTokenResponse` result into `key`, which is incorrect: when credential sharing is enabled `refreshOAuthTokens` returns a `Response` without `.data`, causing `token.access_token` to throw, and `parseRefreshTokenResponse` returns a Zod parse result (with `{ success, data, ... }`), so storing it directly corrupts the credential shape and will break future `googleCredentialSchema.parse` calls. Instead, normalize the response by checking for `.data` vs `.json()`, pass the actual token payload into `parseRefreshTokenResponse`, and persist only its `.data` as the credential key.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+177
to
+188
| const hubspotRefreshToken: HubspotToken = await refreshOAuthTokens( | ||
| async () => | ||
| await hubspotClient.oauth.tokensApi.createToken( | ||
| "refresh_token", | ||
| undefined, | ||
| WEBAPP_URL + "/api/integrations/hubspot/callback", | ||
| this.client_id, | ||
| this.client_secret, | ||
| refreshToken | ||
| ), | ||
| "hubspot", | ||
| credential.userId |
There was a problem hiding this comment.
Suggestion: The refreshAccessToken helper now wraps the HubSpot SDK call in refreshOAuthTokens, which returns either a generic Response from the credential sync endpoint or the SDK token object; this code always treats the result as a HubspotToken (accessing expiresIn, accessToken, and persisting it), so when credential sharing is enabled it will store an invalid object in the database and fail to set a usable access token on the HubSpot client. [logic error]
Severity Level: Critical 🚨
- ❌ HubSpot calendar event creation fails when token refresh needed.
- ❌ HubSpot credential row stores invalid Response-shaped JSON key.
- ⚠️ HubSpot contact search/association break due to missing access token.
Suggested change
| const hubspotRefreshToken: HubspotToken = await refreshOAuthTokens( | |
| async () => | |
| await hubspotClient.oauth.tokensApi.createToken( | |
| "refresh_token", | |
| undefined, | |
| WEBAPP_URL + "/api/integrations/hubspot/callback", | |
| this.client_id, | |
| this.client_secret, | |
| refreshToken | |
| ), | |
| "hubspot", | |
| credential.userId | |
| const hubspotRefreshToken: HubspotToken = await hubspotClient.oauth.tokensApi.createToken( | |
| "refresh_token", | |
| undefined, | |
| WEBAPP_URL + "/api/integrations/hubspot/callback", | |
| this.client_id, | |
| this.client_secret, | |
| refreshToken |
Steps of Reproduction ✅
1. Ensure environment is configured to use credential sync: set
`APP_CREDENTIAL_SHARING_ENABLED` so that it is truthy and set
`CALCOM_CREDENTIAL_SYNC_ENDPOINT` (both read by
`packages/app-store/_utils/oauth/refreshOAuthTokens.ts:1-15`). Use a HubSpot credential
whose `userId` is non-null and whose stored key matches `HubspotToken` (used at
`packages/app-store/hubspot/lib/CalendarService.ts:167`).
2. Let the stored HubSpot OAuth token expire so that `(token.expiresIn ||
token.expiryDate) < Date.now()` evaluates to `true` in `isTokenValid`
(`CalendarService.ts:168-173`). This is the condition under which `hubspotAuth` will try
to refresh the token instead of returning early from `getToken`
(`CalendarService.ts:208-210`).
3. Trigger any code path that creates or updates a HubSpot calendar event so that
`HubspotCalendarService.createEvent` or `updateEvent` is called
(`CalendarService.ts:237-280` and `283-287`). Both methods await `this.auth` and call
`auth.getToken()`, which in turn invokes `refreshAccessToken(credentialKey.refreshToken)`
when the token is expired (`CalendarService.ts:175-189`).
4. Inside `refreshAccessToken`, `refreshOAuthTokens` is called with a refresh function
that returns `hubspotClient.oauth.tokensApi.createToken(...)`
(`CalendarService.ts:177-186`). Because `APP_CREDENTIAL_SHARING_ENABLED` and
`CALCOM_CREDENTIAL_SYNC_ENDPOINT` are set and `userId` is non-null, `refreshOAuthTokens`
takes its sync-branch and returns a `fetch` `Response` object instead of a `HubspotToken`
(`refreshOAuthTokens.ts:3-18). The calling code still treats this as `HubspotToken`, so
`hubspotRefreshToken.expiresIn` and `hubspotRefreshToken.accessToken` are `undefined` when
used at `CalendarService.ts:191-192` and `202`. This results in `expiryDate` becoming
`NaN` and an invalid object being persisted to `prisma.credential`
(`CalendarService.ts:193-200`), and `hubspotClient.setAccessToken` receiving `undefined`
(`CalendarService.ts:202`). Subsequent calls to HubSpot APIs from `hubspotContactSearch`,
`hubspotCreateMeeting`, or `hubspotAssociate` (`CalendarService.ts:79-138`, `109-123`,
`125-138`) fail due to the missing/invalid access token, breaking HubSpot calendar event
creation/update flows under credential sharing.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/app-store/hubspot/lib/CalendarService.ts
**Line:** 177:188
**Comment:**
*Logic Error: The `refreshAccessToken` helper now wraps the HubSpot SDK call in `refreshOAuthTokens`, which returns either a generic `Response` from the credential sync endpoint or the SDK token object; this code always treats the result as a `HubspotToken` (accessing `expiresIn`, `accessToken`, and persisting it), so when credential sharing is enabled it will store an invalid object in the database and fail to set a usable access token on the HubSpot client.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+67
to
+82
| const resp = await refreshOAuthTokens( | ||
| async () => | ||
| await fetch(`${this.url}/authen/v1/refresh_access_token`, { | ||
| method: "POST", | ||
| headers: { | ||
| Authorization: `Bearer ${appAccessToken}`, | ||
| "Content-Type": "application/json; charset=utf-8", | ||
| }, | ||
| body: JSON.stringify({ | ||
| grant_type: "refresh_token", | ||
| refresh_token: refreshToken, | ||
| }), | ||
| }), | ||
| "lark-calendar", | ||
| credential.userId | ||
| ); |
There was a problem hiding this comment.
Suggestion: In refreshAccessToken, the new use of refreshOAuthTokens passes the Lark refresh fetch into a wrapper that may return a Response from the internal credential sync endpoint, but the result is then passed directly to handleLarkError which expects a Lark API response with code/msg; when credential sharing is enabled this mismatch will cause refresh failures because the sync endpoint's response will not match the expected Lark schema and will always be treated as an error. [logic error]
Severity Level: Critical 🚨
- ❌ Lark access-token refresh fails when credential sharing enabled.
- ❌ Lark calendar operations fail once initial token expires.
- ⚠️ Affects createEvent, updateEvent, deleteEvent, listCalendars, availability.
- ⚠️ Breakage tied to APP_CREDENTIAL_SHARING_ENABLED deployments only.
Suggested change
| const resp = await refreshOAuthTokens( | |
| async () => | |
| await fetch(`${this.url}/authen/v1/refresh_access_token`, { | |
| method: "POST", | |
| headers: { | |
| Authorization: `Bearer ${appAccessToken}`, | |
| "Content-Type": "application/json; charset=utf-8", | |
| }, | |
| body: JSON.stringify({ | |
| grant_type: "refresh_token", | |
| refresh_token: refreshToken, | |
| }), | |
| }), | |
| "lark-calendar", | |
| credential.userId | |
| ); | |
| const resp = await fetch(`${this.url}/authen/v1/refresh_access_token`, { | |
| method: "POST", | |
| headers: { | |
| Authorization: `Bearer ${appAccessToken}`, | |
| "Content-Type": "application/json; charset=utf-8", | |
| }, | |
| body: JSON.stringify({ | |
| grant_type: "refresh_token", | |
| refresh_token: refreshToken, | |
| }), | |
| }); |
Steps of Reproduction ✅
1. Ensure the environment enables credential sharing: set
`APP_CREDENTIAL_SHARING_ENABLED=true` and `CALCOM_CREDENTIAL_SYNC_ENDPOINT` to a reachable
URL, as required by `refreshOAuthTokens` in
`packages/app-store/_utils/oauth/refreshOAuthTokens.ts:3-19`. Use a Lark calendar
credential whose `userId` is non-null so the `if` condition at line 5 evaluates true.
2. Create or load a `CredentialPayload` for the Lark app such that `credential.key`
matches `LarkAuthCredentials` with an expired `expiry_date` but a non-expired
`refresh_expires_date` and a non-empty `refresh_token` (checked in
`LarkCalendarService.refreshAccessToken` at
`packages/app-store/larkcalendar/lib/CalendarService.ts:57-63`).
3. Instantiate `LarkCalendarService` with this credential (constructor at
`CalendarService.ts:40-45`), then trigger any method that performs a network call, e.g.
`listCalendars()` at `CalendarService.ts:321-373` or `createEvent()` at
`CalendarService.ts:133-172`. These methods call the private `fetcher` at
`CalendarService.ts:114-131`, which obtains an access token via `this.auth.getToken()`
configured in `larkAuth` at `CalendarService.ts:47-54`. Since `expiry_date` is expired,
`getToken` calls `this.refreshAccessToken(credential)` at line 53.
4. Inside `refreshAccessToken` (`CalendarService.ts:57-82`), because credential sharing is
enabled and `userId` is set, `refreshOAuthTokens` executes its feature-flag branch and
POSTs to `process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT`, returning that endpoint's
`Response` (see branch at `refreshOAuthTokens.ts:5-15`). The returned `Response` is
assumed by `handleLarkError` at `larkcalendar/common.ts:13-25` to be a Lark API response
with JSON shape `{ code: number; msg: string; ... }`. Since the sync endpoint is
documented in `refreshOAuthTokens.ts:6-8` as returning "only the access token and expiry
date", its JSON body lacks `code` and `msg`, so `data.code !== 0` at line 18 evaluates
true, causing `handleLarkError` to throw. This propagates back through
`refreshAccessToken` (which logs and rethrows at `CalendarService.ts:108-110`) and
ultimately causes the original `listCalendars` / `createEvent` / etc. call to fail with an
error instead of successfully refreshing the token.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/app-store/larkcalendar/lib/CalendarService.ts
**Line:** 67:82
**Comment:**
*Logic Error: In `refreshAccessToken`, the new use of `refreshOAuthTokens` passes the Lark refresh fetch into a wrapper that may return a `Response` from the internal credential sync endpoint, but the result is then passed directly to `handleLarkError` which expects a Lark API response with `code`/`msg`; when credential sharing is enabled this mismatch will cause refresh failures because the sync endpoint's response will not match the expected Lark schema and will always be treated as an error.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| }, | ||
| }), | ||
| "zoho-bigin", | ||
| credentialId |
There was a problem hiding this comment.
Suggestion: The refreshAccessToken method passes the credential ID as the userId argument to refreshOAuthTokens and then assumes the returned value is an Axios response with a .data property; when credential sharing is enabled, refreshOAuthTokens instead returns a fetch Response (with no .data field) based on that userId, so this mismatch will cause runtime errors (accessing tokenInfo.data) and also sends the wrong identifier to the sync endpoint. Using null for the userId here avoids the sync branch and preserves the original, working axios-based refresh behavior. [logic error]
Severity Level: Critical 🚨
- ❌ Zoho Bigin bookings fail when credential sharing is enabled.
- ❌ Zoho Bigin token refresh crashes once access token expires.
- ⚠️ Credential sync endpoint receives credential IDs, not user IDs.
Suggested change
| credentialId | |
| null |
Steps of Reproduction ✅
1. Enable credential sharing for the deployment by setting
`APP_CREDENTIAL_SHARING_ENABLED=true` and `CALCOM_CREDENTIAL_SYNC_ENDPOINT` to a valid URL
(these flags are read in `packages/app-store/_utils/oauth/refreshOAuthTokens.ts:1-5`).
2. Connect a Zoho Bigin account so that the `zoho-bigin` app is used (it is registered in
`packages/app-store/index.ts:35` and `packages/app-store/apps.server.generated.ts:72`) and
ensure the stored `BiginToken.expiryDate` is in the past so the token is considered
invalid by `isTokenValid` in `packages/app-store/zoho-bigin/lib/CalendarService.ts:55-56`.
3. Trigger any flow that uses the Zoho Bigin calendar integration (for example, creating a
booking that reaches `BiginCalendarService.createEvent` at
`packages/app-store/zoho-bigin/lib/CalendarService.ts:218-238`), which calls
`this.auth.getToken()` from `biginAuth` at lines 51-63; because the token is invalid,
`getToken` calls `this.refreshAccessToken(credentialId, credentialKey)` at line 62.
4. Inside `refreshAccessToken`
(`packages/app-store/zoho-bigin/lib/CalendarService.ts:69-115`), the code at lines 85-93
calls `refreshOAuthTokens` with an Axios `post` refresh function and passes `credentialId`
as the third argument; in `refreshOAuthTokens`
(`packages/app-store/_utils/oauth/refreshOAuthTokens.ts:3-15`), the condition
`APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId`
is true (because `credentialId` is a non-zero number), so it returns a `fetch` `Response`
object from the sync endpoint instead of the Axios response. Back in `refreshAccessToken`,
`tokenInfo` is this `Response`, so `tokenInfo.data` is `undefined` and `if
(!tokenInfo.data.error)` at line 96 throws `TypeError: Cannot read properties of undefined
(reading 'error')`, breaking Zoho Bigin token refresh and the calling booking flow;
additionally, the sync endpoint receives `calcomUserId=<credentialId>` instead of a real
user ID.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/app-store/zoho-bigin/lib/CalendarService.ts
**Line:** 93:93
**Comment:**
*Logic Error: The `refreshAccessToken` method passes the credential ID as the `userId` argument to `refreshOAuthTokens` and then assumes the returned value is an Axios response with a `.data` property; when credential sharing is enabled, `refreshOAuthTokens` instead returns a `fetch` `Response` (with no `.data` field) based on that `userId`, so this mismatch will cause runtime errors (accessing `tokenInfo.data`) and also sends the wrong identifier to the sync endpoint. Using `null` for the `userId` here avoids the sync branch and preserves the original, working axios-based refresh behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+204
to
+216
| const zohoCrmTokenInfo = await refreshOAuthTokens( | ||
| async () => | ||
| await axios({ | ||
| method: "post", | ||
| url: url, | ||
| data: qs.stringify(formData), | ||
| headers: { | ||
| "Content-Type": "application/x-www-form-urlencoded;charset=utf-8", | ||
| }, | ||
| }), | ||
| "zohocrm", | ||
| credential.userId | ||
| ); |
There was a problem hiding this comment.
Suggestion: The new token refresh integration assumes that the helper always returns an axios-style response with a data property, but when credential sharing is enabled refreshOAuthTokens actually returns a fetch Response object without data, so subsequent access to zohoCrmTokenInfo.data will throw at runtime; normalize the return value here so both code paths produce an object with a data property. [type error]
Severity Level: Critical 🚨
- ❌ ZohoCrmCalendarService.createEvent fails when credential sharing enabled.
- ❌ ZohoCrmCalendarService.updateEvent/deleteEvent fail on token refresh.
- ⚠️ Zoho CRM OAuth token sync endpoint unusable for this integration.
- ⚠️ Similar axios-based integrations assume axios-style data property.
Suggested change
| const zohoCrmTokenInfo = await refreshOAuthTokens( | |
| async () => | |
| await axios({ | |
| method: "post", | |
| url: url, | |
| data: qs.stringify(formData), | |
| headers: { | |
| "Content-Type": "application/x-www-form-urlencoded;charset=utf-8", | |
| }, | |
| }), | |
| "zohocrm", | |
| credential.userId | |
| ); | |
| const zohoCrmTokenInfo = await (async () => { | |
| const response = await refreshOAuthTokens( | |
| async () => | |
| await axios({ | |
| method: "post", | |
| url: url, | |
| data: qs.stringify(formData), | |
| headers: { | |
| "Content-Type": "application/x-www-form-urlencoded;charset=utf-8", | |
| }, | |
| }), | |
| "zohocrm", | |
| credential.userId | |
| ); | |
| if ((response as any).data) { | |
| return response as any; | |
| } | |
| const data = await (response as any).json(); | |
| return { data }; | |
| })(); |
Steps of Reproduction ✅
1. Enable credential sharing so `refreshOAuthTokens` returns a `fetch` Response instead of
the raw provider response: set `APP_CREDENTIAL_SHARING_ENABLED` and
`CALCOM_CREDENTIAL_SYNC_ENDPOINT` as required in
`packages/app-store/_utils/oauth/refreshOAuthTokens.ts:1-15`, and use a Zoho CRM
credential with a non-null `userId`.
2. Ensure the Zoho CRM integration is instantiated so `ZohoCrmCalendarService` is
constructed with that credential, triggering `this.zohoCrmAuth(credential)` in
`packages/app-store/zohocrm/lib/CalendarService.ts:73-76`.
3. Invoke any operation that requires an access token refresh, e.g. event creation via
`ZohoCrmCalendarService.createEvent` at
`packages/app-store/zohocrm/lib/CalendarService.ts:265-310`, which awaits `this.auth` and
then `auth.getToken()` (returned from `zohoCrmAuth` at lines 179-245).
4. During `getToken`, `refreshAccessToken` is called (defined at lines 195-241). Inside
it, `refreshOAuthTokens` is awaited into `zohoCrmTokenInfo` at lines 204-216, which will
now be a `Response` object without a `data` property. The next line `if
(!zohoCrmTokenInfo.data.error)` at line 217 attempts to read
`zohoCrmTokenInfo.data.error`, causing a runtime `TypeError: Cannot read properties of
undefined (reading 'error')`, and the Zoho CRM calendar operation fails.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/app-store/zohocrm/lib/CalendarService.ts
**Line:** 204:216
**Comment:**
*Type Error: The new token refresh integration assumes that the helper always returns an axios-style response with a `data` property, but when credential sharing is enabled `refreshOAuthTokens` actually returns a `fetch` `Response` object without `data`, so subsequent access to `zohoCrmTokenInfo.data` will throw at runtime; normalize the return value here so both code paths produce an object with a `data` property.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
| const responseJson = await handleErrorsJson(response); | ||
| const tokenResponse = refreshTokenResponseSchema.safeParse(responseJson); | ||
| const tokenResponse = parseRefreshTokenResponse(responseJson, refreshTokenResponseSchema); | ||
| o365AuthCredentials = { ...o365AuthCredentials, ...(tokenResponse.success && tokenResponse.data) }; |
There was a problem hiding this comment.
Suggestion: When using parseRefreshTokenResponse, the current merge logic always overwrites the stored refresh token with whatever is in the parsed response; because parseRefreshTokenResponse injects a placeholder "refresh_token" string when the provider does not return a refresh_token, this code will replace a valid refresh token with that placeholder and subsequent refresh attempts will fail. To fix this, preserve the existing refresh_token from o365AuthCredentials and only use the parsed response for the other token fields, mirroring the previous behavior where an absent refresh_token in the response did not clobber the stored one. [logic error]
Severity Level: Major ⚠️
- ❌ Office 365 token refresh can fail after config toggles.
- ❌ Office 365 calendar operations fail once access token expires.
- ⚠️ Stored refresh tokens corrupted for affected Office 365 credentials.
| return Promise.reject(new Error("Invalid refreshed tokens were returned")); | ||
| } | ||
|
|
||
| await prisma.credential.update({ |
There was a problem hiding this comment.
Suggestion: After successfully refreshing the Salesforce OAuth tokens and persisting them, the code still initializes jsforce.Connection using the old access_token and instance_url from credentialKey, so if the original access token is expired this will keep using invalid credentials and cause subsequent Salesforce API calls to fail even though a fresh token is available. [logic error]
Severity Level: Major ⚠️
- ❌ Salesforce API calls may use expired access tokens.
- ⚠️ Salesforce calendar sync can intermittently fail after token refresh.
|
CodeAnt AI finished running the review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Test 8nn
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/cal.com-coderabbit#8](https://github.com/ai-code-review-evaluation/cal.com-coderabbit/pull/8)*CodeAnt-AI Description
Add app credential sync webhook and optional centralized OAuth token refresh
What Changed
Impact
✅ Easier credential provisioning for self-hosted integrations✅ Fewer manual reconnects for integrations when centralized refresh is used✅ Clearer webhook rejection when credential sharing is not enabled or secret mismatches💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.