-
Notifications
You must be signed in to change notification settings - Fork 0
OAuth credential sync and app integration enhancements #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: oauth-security-base
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| import type { NextApiRequest, NextApiResponse } from "next"; | ||
| import z from "zod"; | ||
|
|
||
| import { appStoreMetadata } from "@calcom/app-store/appStoreMetaData"; | ||
| import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants"; | ||
| import { symmetricDecrypt } from "@calcom/lib/crypto"; | ||
| import prisma from "@calcom/prisma"; | ||
|
|
||
| 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(), | ||
| }); | ||
| /** */ | ||
| 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" }); | ||
| } | ||
|
|
||
| const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body); | ||
|
|
||
| // Check that the user exists | ||
| const user = await prisma.user.findUnique({ where: { id: reqBody.userId } }); | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ message: "User not found" }); | ||
| } | ||
|
|
||
| const app = await prisma.app.findUnique({ | ||
| where: { slug: reqBody.appSlug }, | ||
| select: { slug: true }, | ||
| }); | ||
|
|
||
| if (!app) { | ||
| return res.status(404).json({ message: "App not found" }); | ||
| } | ||
|
|
||
| // Search for the app's slug and type | ||
| const appMetadata = appStoreMetadata[app.slug as keyof typeof appStoreMetadata]; | ||
|
|
||
| if (!appMetadata) { | ||
| return res.status(404).json({ message: "App not found. Ensure that you have the correct app slug" }); | ||
| } | ||
|
|
||
| // Decrypt the keys | ||
| const keys = JSON.parse( | ||
| symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "") | ||
| ); | ||
|
|
||
| // Can't use prisma upsert as we don't know the id of the credential | ||
| const appCredential = await prisma.credential.findFirst({ | ||
| where: { | ||
| userId: reqBody.userId, | ||
| appId: appMetadata.slug, | ||
| }, | ||
| select: { | ||
| id: true, | ||
| }, | ||
| }); | ||
|
|
||
| if (appCredential) { | ||
| await prisma.credential.update({ | ||
| where: { | ||
| id: appCredential.id, | ||
| }, | ||
| data: { | ||
| key: keys, | ||
| }, | ||
| }); | ||
| return res.status(200).json({ message: `Credentials updated for userId: ${reqBody.userId}` }); | ||
| } else { | ||
| await prisma.credential.create({ | ||
| data: { | ||
| key: keys, | ||
| userId: reqBody.userId, | ||
| appId: appMetadata.slug, | ||
| type: appMetadata.type, | ||
| }, | ||
| }); | ||
| return res.status(200).json({ message: `Credentials created for userId: ${reqBody.userId}` }); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,32 @@ | ||||||||||||||||||||||||||
| 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(), | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
Comment on lines
+5
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The 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
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. |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| 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; | ||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants"; | ||
|
|
||
| const refreshOAuthTokens = async (refreshFunction: () => any, appSlug: string, userId: number | null) => { | ||
| // Check that app syncing is enabled and that the credential belongs to a user | ||
| if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId) { | ||
| // Customize the payload based on what your endpoint requires | ||
| // The response should only contain the access token and expiry date | ||
| const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, { | ||
| method: "POST", | ||
| body: new URLSearchParams({ | ||
| calcomUserId: userId.toString(), | ||
| appSlug, | ||
| }), | ||
| }); | ||
| return response; | ||
| } else { | ||
| const response = await refreshFunction(); | ||
| return response; | ||
| } | ||
| }; | ||
|
|
||
| export default refreshOAuthTokens; |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,6 +18,8 @@ import type { | |||||||||||||||||||||||||||||||||||||
| } from "@calcom/types/Calendar"; | ||||||||||||||||||||||||||||||||||||||
| import type { CredentialPayload } from "@calcom/types/Credential"; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| import parseRefreshTokenResponse from "../../_utils/oauth/parseRefreshTokenResponse"; | ||||||||||||||||||||||||||||||||||||||
| import refreshOAuthTokens from "../../_utils/oauth/refreshOAuthTokens"; | ||||||||||||||||||||||||||||||||||||||
| import { getGoogleAppKeys } from "./getGoogleAppKeys"; | ||||||||||||||||||||||||||||||||||||||
| import { googleCredentialSchema } from "./googleCredentialSchema"; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
@@ -81,11 +83,18 @@ export default class GoogleCalendarService implements Calendar { | |||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const refreshAccessToken = async (myGoogleAuth: Awaited<ReturnType<typeof getGoogleAuth>>) => { | ||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||
| const { res } = await myGoogleAuth.refreshToken(googleCredentials.refresh_token); | ||||||||||||||||||||||||||||||||||||||
| const res = await refreshOAuthTokens( | ||||||||||||||||||||||||||||||||||||||
| async () => { | ||||||||||||||||||||||||||||||||||||||
| const fetchTokens = await myGoogleAuth.refreshToken(googleCredentials.refresh_token); | ||||||||||||||||||||||||||||||||||||||
| return fetchTokens.res; | ||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||
| "google-calendar", | ||||||||||||||||||||||||||||||||||||||
| credential.userId | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
| 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); | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
94
to
+97
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The refreshed token handling assumes that 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
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. |
||||||||||||||||||||||||||||||||||||||
| await prisma.credential.update({ | ||||||||||||||||||||||||||||||||||||||
| where: { id: credential.id }, | ||||||||||||||||||||||||||||||||||||||
| data: { key }, | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,6 +23,7 @@ import type { | |||||||||||||||||||||||||||||||||||||||
| import type { CredentialPayload } from "@calcom/types/Credential"; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| import getAppKeysFromSlug from "../../_utils/getAppKeysFromSlug"; | ||||||||||||||||||||||||||||||||||||||||
| import refreshOAuthTokens from "../../_utils/oauth/refreshOAuthTokens"; | ||||||||||||||||||||||||||||||||||||||||
| import type { HubspotToken } from "../api/callback"; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| const hubspotClient = new hubspot.Client(); | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -173,13 +174,18 @@ export default class HubspotCalendarService implements Calendar { | |||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| const refreshAccessToken = async (refreshToken: string) => { | ||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||
| const hubspotRefreshToken: HubspotToken = await hubspotClient.oauth.tokensApi.createToken( | ||||||||||||||||||||||||||||||||||||||||
| "refresh_token", | ||||||||||||||||||||||||||||||||||||||||
| undefined, | ||||||||||||||||||||||||||||||||||||||||
| WEBAPP_URL + "/api/integrations/hubspot/callback", | ||||||||||||||||||||||||||||||||||||||||
| this.client_id, | ||||||||||||||||||||||||||||||||||||||||
| this.client_secret, | ||||||||||||||||||||||||||||||||||||||||
| refreshToken | ||||||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+177
to
+188
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The 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
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. |
||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // set expiry date as offset from current time. | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,7 @@ import type { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } from "@calcom/types/Calendar"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import type { CredentialPayload } from "@calcom/types/Credential"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import refreshOAuthTokens from "../../_utils/oauth/refreshOAuthTokens"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { handleLarkError, isExpired, LARK_HOST } from "../common"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import type { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CreateAttendeesResp, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -63,17 +64,22 @@ export default class LarkCalendarService implements Calendar { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const appAccessToken = await getAppAccessToken(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+67
to
+82
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: In 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
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const data = await handleLarkError<RefreshTokenResp>(resp, this.log); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.log.debug( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_NAMEis 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⚠️
Steps of Reproduction ✅
Prompt for AI Agent 🤖