Automated Test: oauth-security-enhanced #363
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>
📝 WalkthroughWalkthroughThis PR introduces a credential sharing system for OAuth apps by adding a webhook endpoint for syncing credentials, creating centralized OAuth token refresh utilities, reorganizing OAuth imports into a dedicated directory, and updating multiple integrations to use the new token refresh flow. Changes
Sequence DiagramsequenceDiagram
actor ExtSystem as External<br/>System
participant Webhook as Webhook<br/>Endpoint
participant Auth as Auth<br/>Service
participant DB as Database
ExtSystem->>Webhook: POST /api/webhook/app-credential<br/>(encrypted keys + userId + appSlug)
Webhook->>Webhook: Validate webhook secret<br/>from header
alt Secret Invalid
Webhook-->>ExtSystem: 403 Forbidden
else Secret Valid
Webhook->>Auth: Decrypt credential keys<br/>using encryption key
Webhook->>Auth: Verify user & app exist
Webhook->>Auth: Lookup app metadata
alt Credential exists
Webhook->>DB: Update existing credential
Webhook-->>ExtSystem: 200 Updated
else Credential not found
Webhook->>DB: Create new credential
Webhook-->>ExtSystem: 200 Created
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
packages/app-store/webex/api/callback.ts (1)
19-27:⚠️ Potential issue | 🟠 MajorPre-existing bug: missing
=afterclient_idquery parameter.Not introduced in this PR, but Line 20 concatenates
&client_iddirectly with the value instead of&client_id=. This produces a malformed URL (...&client_idACTUAL_VALUE...). Same issue applies to the rest of the string-concatenated parameters—consider switching toURLSearchParamsto avoid this class of bug entirely.- "https://webexapis.com/v1/access_token?grant_type=authorization_code&client_id" + + "https://webexapis.com/v1/access_token?grant_type=authorization_code&client_id=" +packages/app-store/salesforce/api/callback.ts (1)
18-21:⚠️ Potential issue | 🟡 MinorPre-existing bug:
&&should be||— array values forcodebypass validation.
req.query.codecan bestring | string[] | undefined. The current conditioncode === undefined && typeof code !== "string"only matchesundefined; astring[]value passes through and gets unsafely cast on line 39 ascode as string. Compare with hubspot's callback which usescode && typeof code !== "string".Proposed fix
- if (code === undefined && typeof code !== "string") { + if (code === undefined || typeof code !== "string") {With this fix, the cast on line 39 is also no longer needed since TypeScript will narrow
codetostring.packages/app-store/zohocrm/api/callback.ts (1)
50-51:⚠️ Potential issue | 🟠 MajorPre-existing bug: token expiry is set to ~3.6 seconds from now.
Date.now()returns milliseconds, but60 * 60is 3600 (seconds). This effectively sets the expiry date to ~3.6s in the future, causing the token to appear expired almost immediately on next use. This isn't introduced by this PR but is worth fixing while you're in the file.Proposed fix
- zohoCrmTokenInfo.data.expiryDate = Math.round(Date.now() + 60 * 60); + zohoCrmTokenInfo.data.expiryDate = Math.round(Date.now() + 60 * 60 * 1000);packages/app-store/webex/lib/VideoApiAdapter.ts (1)
151-152:⚠️ Potential issue | 🟡 MinorPre-existing: debug
console.logstatements leak access tokens and request bodies.Lines 151–152, 184–186, 194, and 223 contain
console.logcalls that output access tokens and full request/response bodies. These are likely leftover debug artifacts and should be removed or converted to a debug-level logger to avoid leaking sensitive data in production logs.packages/app-store/zoomvideo/lib/VideoApiAdapter.ts (1)
104-111:⚠️ Potential issue | 🟠 MajorDead code and coupling to
SafeParseReturnTypereturn shape.
parseRefreshTokenResponsethrows on parse failure (line 22 of that file), so the!parsedToken.successcheck on line 108 is unreachable. Additionally, lines 108 and 111 access.successand.datawhich assumes theSafeParseReturnTypeshape.If
parseRefreshTokenResponseis fixed to return.datadirectly, update accordingly:Proposed fix (after parseRefreshTokenResponse returns .data)
const parsedToken = parseRefreshTokenResponse(responseBody, zoomRefreshedTokenSchema); - // TODO: If the new token is invalid, initiate the fallback sequence instead of throwing - // Expanding on this we can use server-to-server app and create meeting from admin calcom account - if (!parsedToken.success) { - return Promise.reject(new Error("Invalid refreshed tokens were returned")); - } - const newTokens = parsedToken.data; + const newTokens = parsedToken;packages/app-store/googlecalendar/lib/CalendarService.ts (1)
97-101:⚠️ Potential issue | 🔴 CriticalStoring
SafeParseReturnTypeas credential key corrupts the database record.
parseRefreshTokenResponsereturns{ success: true, data: { access_token, ... } }. This entire wrapper object gets stored as the credentialkey, so subsequent reads viagoogleCredentialSchema.parse(credential.key)will fail becauseaccess_tokenis nested under.datarather than at the top level.This will cause Google Calendar authentication to break after the first token refresh.
Proposed fix (after parseRefreshTokenResponse returns .data)
If
parseRefreshTokenResponseis fixed to return.datadirectly, this line works as-is. Otherwise:- const key = parseRefreshTokenResponse(googleCredentials, googleCredentialSchema); + const key = parseRefreshTokenResponse(googleCredentials, googleCredentialSchema).data;packages/app-store/salesforce/lib/CalendarService.ts (1)
101-108:⚠️ Potential issue | 🔴 CriticalConnection is initialised with stale (pre-refresh) tokens.
After refreshing the token and persisting the new
access_tokenandinstance_url(line 98), thejsforce.Connectionon lines 101-108 still reads fromcredentialKey(the original, possibly-expired values). This means the connection will use the old access token, defeating the purpose of the refresh.🐛 Proposed fix — use the refreshed values
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, + instanceUrl: accessTokenParsed.data.instance_url, + accessToken: accessTokenParsed.data.access_token, + refreshToken: credentialKey.refresh_token, });
🤖 Fix all issues with AI agents
In `@apps/web/pages/api/webhook/app-credential.ts`:
- Line 31: The code calls appCredentialWebhookRequestBodySchema.parse(req.body)
which throws a ZodError and yields a 500; change this to use
appCredentialWebhookRequestBodySchema.safeParse(req.body) (or wrap parse in
try/catch) in the API handler so validation failures are detected, and when
validation fails return a 400 with the validation errors instead of letting the
exception bubble; update the variable assignment (reqBody) to only be set when
parsing succeeds and use res.status(400).json(...) to send the structured error
response.
- Around line 56-59: Wrap the decryption/parse of reqBody.keys (the call to
symmetricDecrypt(...) and JSON.parse(...)) in a try/catch inside the API handler
so failures return a 400 with a clear message instead of throwing a 500;
specifically detect if process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY is empty
or if symmetricDecrypt/JSON.parse throws, catch the error, log or include the
error message for debugging, and respond with res.status(400).json({ error:
"Invalid encrypted keys" /* plus brief detail */ }) rather than allowing the
exception to bubble.
- Around line 17-29: Add an explicit HTTP method guard at the top of the handler
(export default async function handler) to only allow POST and return 405 for
other methods; keep the existing APP_CREDENTIAL_SHARING_ENABLED check. Replace
the direct !== comparison of the incoming header
(req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"])
against process.env.CALCOM_WEBHOOK_SECRET with a timing-safe comparison using
Node's crypto.timingSafeEqual: coerce both values to Buffers, ensure equal
length before calling timingSafeEqual (treat unequal lengths as a failed match),
and handle missing header or missing secret by returning 403.
In `@packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts`:
- Around line 21-29: parseRefreshTokenResponse currently throws on parse failure
but returns the full SafeParseReturnType object, which leads callers (e.g.,
Google Calendar code that passes the return value as the `key` to
prisma.credential.update and Zoom code that checks `parsedToken.success`) to
misuse the shape; change parseRefreshTokenResponse to throw on failure and
return only the parsed `.data` (the credential token object) instead of the
entire SafeParseReturnType, then update callers that expect the full object
(notably the Google Calendar flow that writes to prisma.credential.update and
the Zoom flow that redundantly checks `parsedToken.success`) to treat the
function as returning the credential data directly (remove the dead
`!parsedToken.success` check and pass the returned data as the `key`/credential
payload).
- Around line 5-11: minimumTokenResponseSchema currently uses computed keys like
[z.string().toString()] which evaluate to literal "[object Object]" and don't
create wildcard matchers; replace the object definition with a schema that only
asserts required fields and allows all other properties to pass through — e.g.
change minimumTokenResponseSchema to z.object({ access_token: z.string()
}).passthrough() (remove the computed-key lines) so properties like
refresh_token, expires_in, expiry_date, scope, etc. are preserved.
In `@packages/app-store/_utils/oauth/refreshOAuthTokens.ts`:
- Around line 3-19: refreshOAuthTokens currently returns different types
depending on APP_CREDENTIAL_SHARING_ENABLED (raw fetch Response vs
provider-specific refreshFunction result), causing caller breakage; change the
function to have a consistent return shape (make it generic or return a
normalized RefreshTokenResp-like object) and update the signature to accept
refreshFunction: () => Promise<Response> (or a typed Promise<T>), call the sync
endpoint inside a try/catch to handle network errors, parse and validate the
sync endpoint JSON and map it into the provider-expected schema (or reject with
a clear error) so callers like handleWebexResponse and
handleLarkError<RefreshTokenResp> always receive the same, typed object; ensure
errors from fetch or validation are thrown as meaningful exceptions.
- Around line 8-14: The POST to CALCOM_CREDENTIAL_SYNC_ENDPOINT in
refreshOAuthTokens.ts must include the webhook auth header used by the receiver;
modify the fetch call that builds the request (the const response = await
fetch(...) block) to add a headers object with the header name taken from
process.env.CALCOM_WEBHOOK_HEADER_NAME and the secret from
process.env.CALCOM_WEBHOOK_SECRET so the outbound request matches the validation
in app-credential.ts; ensure you read both env vars and include the header when
making the POST.
In `@packages/app-store/office365calendar/lib/CalendarService.ts`:
- Around line 263-264: The code is assuming parseRefreshTokenResponse returns a
SafeParseReturnType (checking tokenResponse.success and tokenResponse.data);
update CalendarService.ts to handle the refactor where parseRefreshTokenResponse
returns the parsed data directly: call parseRefreshTokenResponse(...) into a
variable like tokenResponseData and then merge only if tokenResponseData is
truthy (e.g., o365AuthCredentials = { ...o365AuthCredentials,
...(tokenResponseData || {}) }) or otherwise adjust to check the new shape (no
.success) so tokenResponse/data access no longer occurs; update references to
tokenResponse.success and tokenResponse.data accordingly.
In `@packages/app-store/salesforce/lib/CalendarService.ts`:
- Around line 96-99: The file uses prisma in the call prisma.credential.update
(updating credential.id with key data) but never imports it; add the missing
import for the Prisma client (e.g., import prisma from "@calcom/prisma") at the
top of CalendarService.ts so prisma is defined before the
prisma.credential.update call that uses credential, credential.id,
credentialKey, and accessTokenParsed.
- Line 86: Replace the brittle statusText check with the Fetch API idiomatic
check: in the CalendarService.ts where the code currently does `if
(response.statusText !== "OK") throw new HttpError(...)`, change it to check
`!response.ok` (or `response.status` range) before throwing the HttpError so the
error path uses a reliable indicator of non-2xx responses; keep the thrown
HttpError({ statusCode: 400, message: response.statusText }) but consider using
response.status or response.statusText for the message if available.
🧹 Nitpick comments (4)
packages/app-store/_utils/oauth/createOAuthAppCredential.ts (1)
6-6: Redundant parent traversal in same-directory import.Since
createOAuthAppCredential.tsanddecodeOAuthState.tsare both in_utils/oauth/, the import../oauth/decodeOAuthStateunnecessarily goes up to the parent and back down. Use a sibling import instead.🔧 Suggested simplification
-import { decodeOAuthState } from "../oauth/decodeOAuthState"; +import { decodeOAuthState } from "./decodeOAuthState";turbo.json (1)
205-207:CALCOM_WEBHOOK_SECRETis out of alphabetical order.It should come before
CALENDSO_ENCRYPTION_KEYto maintain the sorted convention of this list.Proposed fix
"CALCOM_WEBHOOK_HEADER_NAME", - "CALENDSO_ENCRYPTION_KEY", "CALCOM_WEBHOOK_SECRET", + "CALENDSO_ENCRYPTION_KEY",packages/lib/constants.ts (1)
102-104: Consider coercing to an explicit boolean for type consistency.Other feature-flag constants in this file (e.g.,
IS_STRIPE_ENABLED) use!!to produce aboolean. Without it,APP_CREDENTIAL_SHARING_ENABLEDis typed asstring | undefined, which works in truthy checks but may surprise consumers expecting a boolean.Proposed fix
export const APP_CREDENTIAL_SHARING_ENABLED = - process.env.CALCOM_WEBHOOK_SECRET && process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY; + !!(process.env.CALCOM_WEBHOOK_SECRET && process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY);packages/app-store/zoho-bigin/api/add.ts (1)
17-17: Hardcoded slug reduces maintainability vs. the dynamicappConfig.slugused elsewhere.Line 17 hardcodes
"zoho-bigin"while the callback handler (line 36 incallback.ts) still usesappConfig.slug. This inconsistency could cause a redirect_uri mismatch if the slug is ever updated inconfig.json. Consider using the dynamic value for consistency:- const redirectUri = WEBAPP_URL + `/api/integrations/zoho-bigin/callback`; + const redirectUri = WEBAPP_URL + `/api/integrations/${appConfig.slug}/callback`;
| 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.
Missing HTTP method restriction — endpoint accepts any method.
This webhook handler should only accept POST. Without a method guard, GET/DELETE/etc. requests fall through to the body-parsing logic, resulting in confusing 500 errors instead of a clean 405 Method Not Allowed.
Additionally, the webhook-secret comparison on line 25 uses !==, which is susceptible to timing attacks. Use timingSafeEqual from Node's crypto module instead.
🛡️ Proposed fix
+import { timingSafeEqual } from "crypto";
+
export default async function handler(req: NextApiRequest, res: NextApiResponse) {
+ if (req.method !== "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" });
}
// Check that the webhook secret matches
- if (
- req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] !==
- process.env.CALCOM_WEBHOOK_SECRET
- ) {
+ const expectedSecret = process.env.CALCOM_WEBHOOK_SECRET || "";
+ const receivedSecret = String(
+ req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] || ""
+ );
+ if (
+ expectedSecret.length !== receivedSecret.length ||
+ !timingSafeEqual(Buffer.from(expectedSecret), Buffer.from(receivedSecret))
+ ) {
return res.status(403).json({ message: "Invalid webhook secret" });
}📝 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.
| 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" }); | |
| } | |
| import { timingSafeEqual } from "crypto"; | |
| export default async function handler(req: NextApiRequest, res: NextApiResponse) { | |
| if (req.method !== "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" }); | |
| } | |
| // Check that the webhook secret matches | |
| const expectedSecret = process.env.CALCOM_WEBHOOK_SECRET || ""; | |
| const receivedSecret = String( | |
| req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] || "" | |
| ); | |
| if ( | |
| expectedSecret.length !== receivedSecret.length || | |
| !timingSafeEqual(Buffer.from(expectedSecret), Buffer.from(receivedSecret)) | |
| ) { | |
| return res.status(403).json({ message: "Invalid webhook secret" }); | |
| } |
🤖 Prompt for AI Agents
In `@apps/web/pages/api/webhook/app-credential.ts` around lines 17 - 29, Add an
explicit HTTP method guard at the top of the handler (export default async
function handler) to only allow POST and return 405 for other methods; keep the
existing APP_CREDENTIAL_SHARING_ENABLED check. Replace the direct !== comparison
of the incoming header (req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME ||
"calcom-webhook-secret"]) against process.env.CALCOM_WEBHOOK_SECRET with a
timing-safe comparison using Node's crypto.timingSafeEqual: coerce both values
to Buffers, ensure equal length before calling timingSafeEqual (treat unequal
lengths as a failed match), and handle missing header or missing secret by
returning 403.
| return res.status(403).json({ message: "Invalid webhook secret" }); | ||
| } | ||
|
|
||
| const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body); |
There was a problem hiding this comment.
Unhandled ZodError from .parse() will surface as a 500.
appCredentialWebhookRequestBodySchema.parse(req.body) throws a ZodError on invalid input. Without a try/catch (or using .safeParse()), the caller receives a raw 500 instead of a structured 400 response.
♻️ Proposed fix using safeParse
- const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body);
+ const parsed = appCredentialWebhookRequestBodySchema.safeParse(req.body);
+ if (!parsed.success) {
+ return res.status(400).json({ message: "Invalid request body", error: parsed.error.flatten() });
+ }
+ const reqBody = parsed.data;📝 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 reqBody = appCredentialWebhookRequestBodySchema.parse(req.body); | |
| const parsed = appCredentialWebhookRequestBodySchema.safeParse(req.body); | |
| if (!parsed.success) { | |
| return res.status(400).json({ message: "Invalid request body", error: parsed.error.flatten() }); | |
| } | |
| const reqBody = parsed.data; |
🤖 Prompt for AI Agents
In `@apps/web/pages/api/webhook/app-credential.ts` at line 31, The code calls
appCredentialWebhookRequestBodySchema.parse(req.body) which throws a ZodError
and yields a 500; change this to use
appCredentialWebhookRequestBodySchema.safeParse(req.body) (or wrap parse in
try/catch) in the API handler so validation failures are detected, and when
validation fails return a 400 with the validation errors instead of letting the
exception bubble; update the variable assignment (reqBody) to only be set when
parsing succeeds and use res.status(400).json(...) to send the structured error
response.
| // Decrypt the keys | ||
| const keys = JSON.parse( | ||
| symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "") | ||
| ); |
There was a problem hiding this comment.
Decryption and JSON parse are unguarded — any failure becomes a 500.
If CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY is unset (falls back to ""), or the payload is tampered with, symmetricDecrypt or JSON.parse will throw. Wrap this in a try/catch and return a 400 with a meaningful message.
♻️ Proposed fix
// Decrypt the keys
- const keys = JSON.parse(
- symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "")
- );
+ let keys;
+ try {
+ keys = JSON.parse(
+ symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "")
+ );
+ } catch (e) {
+ return res.status(400).json({ message: "Failed to decrypt or parse credential keys" });
+ }📝 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.
| // Decrypt the keys | |
| const keys = JSON.parse( | |
| symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "") | |
| ); | |
| // Decrypt the keys | |
| let keys; | |
| try { | |
| keys = JSON.parse( | |
| symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "") | |
| ); | |
| } catch (e) { | |
| return res.status(400).json({ message: "Failed to decrypt or parse credential keys" }); | |
| } |
🤖 Prompt for AI Agents
In `@apps/web/pages/api/webhook/app-credential.ts` around lines 56 - 59, Wrap the
decryption/parse of reqBody.keys (the call to symmetricDecrypt(...) and
JSON.parse(...)) in a try/catch inside the API handler so failures return a 400
with a clear message instead of throwing a 500; specifically detect if
process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY is empty or if
symmetricDecrypt/JSON.parse throws, catch the error, log or include the error
message for debugging, and respond with res.status(400).json({ error: "Invalid
encrypted keys" /* plus brief detail */ }) rather than allowing the exception to
bubble.
| 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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Test Zod behavior with computed property keys and key stripping
cat <<'EOF' > /tmp/test_zod.js
const { z } = require("zod");
console.log("=== Testing z.string().toString() ===");
console.log("z.string().toString() =>", z.string().toString());
console.log("z.string().optional().toString() =>", z.string().optional().toString());
console.log("\n=== Testing z.object with computed keys ===");
const schema = z.object({
access_token: z.string(),
[z.string().toString()]: z.number(),
[z.string().optional().toString()]: z.unknown().optional(),
});
console.log("Schema keys:", Object.keys(schema.shape));
console.log("\n=== Testing safeParse with unknown keys ===");
const input = { access_token: "abc", refresh_token: "def", expires_in: 3600, scope: "read" };
const result = schema.safeParse(input);
console.log("Parse success:", result.success);
if (result.success) {
console.log("Parsed data keys:", Object.keys(result.data));
console.log("Parsed data:", JSON.stringify(result.data, null, 2));
} else {
console.log("Error:", result.error.message);
}
console.log("\n=== Testing with .passthrough() ===");
const passthroughSchema = z.object({
access_token: z.string(),
}).passthrough();
const result2 = passthroughSchema.safeParse(input);
console.log("Parse success:", result2.success);
if (result2.success) {
console.log("Parsed data keys:", Object.keys(result2.data));
console.log("Parsed data:", JSON.stringify(result2.data, null, 2));
}
EOF
cd /tmp && npm init -y --silent 2>/dev/null && npm install zod --silent 2>/dev/null && node test_zod.jsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 975
🏁 Script executed:
# Check the parseRefreshTokenResponse.ts file for the Google Calendar issue
grep -n "parseRefreshTokenResponse" packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts | head -5
cat -n packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts | head -35Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1469
🏁 Script executed:
# Search for usage of parseRefreshTokenResponse in Google Calendar and other apps
rg "parseRefreshTokenResponse" --type typescript -B 2 -A 5 packages/app-store/ | grep -E "(googlecalendar|zoomvideo|office365calendar)" -A 8 | head -60Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 124
🏁 Script executed:
# Search for parseRefreshTokenResponse usage in the codebase
rg "parseRefreshTokenResponse" -B 2 -A 8 packages/app-store/ | head -150Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 9924
🏁 Script executed:
# Double-check what the literal computed key string is
cat <<'EOF' > /tmp/check_key.js
const key = "[object Object]";
console.log("Literal key string:", key);
console.log("Key from z.string().toString():", JSON.stringify(key));
// Verify the schema shape
const { z } = require("zod");
const schema = z.object({
access_token: z.string(),
[z.string().toString()]: z.number(),
});
console.log("Schema shape keys:", Object.keys(schema.shape));
EOF
cd /tmp && npm install zod --silent 2>/dev/null && node check_key.jsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 232
minimumTokenResponseSchema computed keys do not work as intended.
z.string().toString() evaluates to the literal string "[object Object]" (JavaScript's default toString() representation), creating fixed property names rather than wildcard matchers. This means the schema defines literal keys like "[object Object]" instead of "any string key → number" patterns. Since z.object() strips unrecognized keys by default, when this schema is used (i.e., when APP_CREDENTIAL_SHARING_ENABLED is true), the parsed output will lose properties like refresh_token, expires_in, expiry_date, scope, etc.
To retain all properties while only requiring access_token, use .passthrough():
Proposed fix
-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(),
+ })
+ .passthrough();🤖 Prompt for AI Agents
In `@packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts` around lines 5
- 11, minimumTokenResponseSchema currently uses computed keys like
[z.string().toString()] which evaluate to literal "[object Object]" and don't
create wildcard matchers; replace the object definition with a schema that only
asserts required fields and allows all other properties to pass through — e.g.
change minimumTokenResponseSchema to z.object({ access_token: z.string()
}).passthrough() (remove the computed-key lines) so properties like
refresh_token, expires_in, expiry_date, scope, etc. are preserved.
| if (!refreshTokenResponse.success) { | ||
| throw new Error("Invalid refreshed tokens were returned"); | ||
| } | ||
|
|
||
| if (!refreshTokenResponse.data.refresh_token) { | ||
| refreshTokenResponse.data.refresh_token = "refresh_token"; | ||
| } | ||
|
|
||
| return refreshTokenResponse; |
There was a problem hiding this comment.
Return type is SafeParseReturnType, but callers expect inconsistent things.
The function throws on parse failure (Line 22) but returns the full SafeParseReturnType (with .success, .data, .error properties). This creates two issues across callers:
- Google Calendar (line 97) passes the return value directly as
keytoprisma.credential.update, storing{ success: true, data: {...} }instead of the actual credential data — this corrupts the stored credential. - Zoom (line 108) checks
!parsedToken.successafter this function already threw on failure — dead code that can never be reached.
Since the function already throws on failure, it should return just .data:
Proposed fix
if (!refreshTokenResponse.success) {
throw new Error("Invalid refreshed tokens were returned");
}
if (!refreshTokenResponse.data.refresh_token) {
refreshTokenResponse.data.refresh_token = "refresh_token";
}
- return refreshTokenResponse;
+ return refreshTokenResponse.data;
};📝 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.
| if (!refreshTokenResponse.success) { | |
| throw new Error("Invalid refreshed tokens were returned"); | |
| } | |
| if (!refreshTokenResponse.data.refresh_token) { | |
| refreshTokenResponse.data.refresh_token = "refresh_token"; | |
| } | |
| return refreshTokenResponse; | |
| if (!refreshTokenResponse.success) { | |
| throw new Error("Invalid refreshed tokens were returned"); | |
| } | |
| if (!refreshTokenResponse.data.refresh_token) { | |
| refreshTokenResponse.data.refresh_token = "refresh_token"; | |
| } | |
| return refreshTokenResponse.data; |
🤖 Prompt for AI Agents
In `@packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts` around lines 21
- 29, parseRefreshTokenResponse currently throws on parse failure but returns
the full SafeParseReturnType object, which leads callers (e.g., Google Calendar
code that passes the return value as the `key` to prisma.credential.update and
Zoom code that checks `parsedToken.success`) to misuse the shape; change
parseRefreshTokenResponse to throw on failure and return only the parsed `.data`
(the credential token object) instead of the entire SafeParseReturnType, then
update callers that expect the full object (notably the Google Calendar flow
that writes to prisma.credential.update and the Zoom flow that redundantly
checks `parsedToken.success`) to treat the function as returning the credential
data directly (remove the dead `!parsedToken.success` check and pass the
returned data as the `key`/credential payload).
| 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; | ||
| } |
There was a problem hiding this comment.
Return type divergence between the two code paths may break callers.
When APP_CREDENTIAL_SHARING_ENABLED is true, this returns a Response from CALCOM_CREDENTIAL_SYNC_ENDPOINT. The callers (e.g., Webex's handleWebexResponse, Lark's handleLarkError<RefreshTokenResp>) then parse the response body using provider-specific schemas. The external sync endpoint would need to return a body conforming to each provider's expected schema, which is fragile and undocumented.
Additionally:
- No error handling on the
fetchto the sync endpoint — network errors will throw unhandled. - The
() => anyparameter type loses all type safety. Consider() => Promise<Response>since all callers pass fetch calls.
Suggested improvements
-const refreshOAuthTokens = async (refreshFunction: () => any, appSlug: string, userId: number | null) => {
+const refreshOAuthTokens = async (
+ refreshFunction: () => Promise<Response>,
+ appSlug: string,
+ userId: number | null
+): Promise<Response> => {
// Check that app syncing is enabled and that the credential belongs to a user
if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId) {
- // Customize the payload based on what your endpoint requires
- // The response should only contain the access token and expiry date
const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
method: "POST",
body: new URLSearchParams({
calcomUserId: userId.toString(),
appSlug,
}),
});
+ if (!response.ok) {
+ throw new Error(`Credential sync endpoint returned ${response.status}`);
+ }
return response;
} else {
- const response = await refreshFunction();
- return response;
+ return refreshFunction();
}
};📝 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 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; | |
| } | |
| const refreshOAuthTokens = async ( | |
| refreshFunction: () => Promise<Response>, | |
| appSlug: string, | |
| userId: number | null | |
| ): Promise<Response> => { | |
| // Check that app syncing is enabled and that the credential belongs to a user | |
| if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId) { | |
| const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, { | |
| method: "POST", | |
| body: new URLSearchParams({ | |
| calcomUserId: userId.toString(), | |
| appSlug, | |
| }), | |
| }); | |
| if (!response.ok) { | |
| throw new Error(`Credential sync endpoint returned ${response.status}`); | |
| } | |
| return response; | |
| } else { | |
| return refreshFunction(); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@packages/app-store/_utils/oauth/refreshOAuthTokens.ts` around lines 3 - 19,
refreshOAuthTokens currently returns different types depending on
APP_CREDENTIAL_SHARING_ENABLED (raw fetch Response vs provider-specific
refreshFunction result), causing caller breakage; change the function to have a
consistent return shape (make it generic or return a normalized
RefreshTokenResp-like object) and update the signature to accept
refreshFunction: () => Promise<Response> (or a typed Promise<T>), call the sync
endpoint inside a try/catch to handle network errors, parse and validate the
sync endpoint JSON and map it into the provider-expected schema (or reject with
a clear error) so callers like handleWebexResponse and
handleLarkError<RefreshTokenResp> always receive the same, typed object; ensure
errors from fetch or validation are thrown as meaningful exceptions.
| const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, { | ||
| method: "POST", | ||
| body: new URLSearchParams({ | ||
| calcomUserId: userId.toString(), | ||
| appSlug, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if CALCOM_WEBHOOK_SECRET is used anywhere for outbound requests
echo "=== Usage of CALCOM_WEBHOOK_SECRET ==="
rg -n "CALCOM_WEBHOOK_SECRET" --type=ts -C3
echo ""
echo "=== Usage of CALCOM_WEBHOOK_HEADER_NAME ==="
rg -n "CALCOM_WEBHOOK_HEADER_NAME" --type=ts -C3Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1695
🏁 Script executed:
# Check .env.example for these variables
echo "=== .env.example definitions ==="
if [ -f .env.example ]; then
grep -n "CALCOM_WEBHOOK_SECRET\|CALCOM_WEBHOOK_HEADER_NAME" .env.example
else
echo ".env.example not found"
fi
echo ""
# Find and check the actual refreshOAuthTokens.ts file
echo "=== Full context of refreshOAuthTokens.ts ==="
find . -name "refreshOAuthTokens.ts" -type fRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 309
🏁 Script executed:
# Check how CALCOM_CREDENTIAL_SYNC_ENDPOINT is used
echo "=== Usage of CALCOM_CREDENTIAL_SYNC_ENDPOINT ==="
rg -n "CALCOM_CREDENTIAL_SYNC_ENDPOINT" --type=ts -C5Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 2683
🏁 Script executed:
cat -n packages/app-store/_utils/oauth/refreshOAuthTokens.tsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1094
🏁 Script executed:
# Check for any documentation or comments about CALCOM_CREDENTIAL_SYNC_ENDPOINT
rg -n "CALCOM_CREDENTIAL_SYNC_ENDPOINT" --type=ts --type=md -B2 -A2Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1699
🏁 Script executed:
# Check how the webhook endpoint at app-credential.ts is structured (see if it's meant to receive this request)
cat -n apps/web/pages/api/webhook/app-credential.ts | head -50Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 2123
Add authentication header to credential sync request.
The POST to CALCOM_CREDENTIAL_SYNC_ENDPOINT lacks the webhook secret header. The codebase establishes a clear authentication pattern in apps/web/pages/api/webhook/app-credential.ts that validates requests using CALCOM_WEBHOOK_HEADER_NAME and CALCOM_WEBHOOK_SECRET (defined in .env.example lines 237–239). The outbound credential sync request should include this same header to authenticate the call.
Proposed fix — include the webhook secret header
const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
method: "POST",
+ headers: {
+ [process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"]:
+ process.env.CALCOM_WEBHOOK_SECRET || "",
+ },
body: new URLSearchParams({
calcomUserId: userId.toString(),
appSlug,
}),
});🤖 Prompt for AI Agents
In `@packages/app-store/_utils/oauth/refreshOAuthTokens.ts` around lines 8 - 14,
The POST to CALCOM_CREDENTIAL_SYNC_ENDPOINT in refreshOAuthTokens.ts must
include the webhook auth header used by the receiver; modify the fetch call that
builds the request (the const response = await fetch(...) block) to add a
headers object with the header name taken from
process.env.CALCOM_WEBHOOK_HEADER_NAME and the secret from
process.env.CALCOM_WEBHOOK_SECRET so the outbound request matches the validation
in app-credential.ts; ensure you read both env vars and include the header when
making the POST.
| const tokenResponse = parseRefreshTokenResponse(responseJson, refreshTokenResponseSchema); | ||
| o365AuthCredentials = { ...o365AuthCredentials, ...(tokenResponse.success && tokenResponse.data) }; |
There was a problem hiding this comment.
Tightly coupled to parseRefreshTokenResponse returning SafeParseReturnType.
Line 264 accesses tokenResponse.success and tokenResponse.data, which only works because parseRefreshTokenResponse currently returns the full SafeParseReturnType object. If the return value is fixed to return .data directly (as flagged in the parseRefreshTokenResponse.ts review), this line must be updated:
Proposed fix (after parseRefreshTokenResponse returns .data)
const tokenResponse = parseRefreshTokenResponse(responseJson, refreshTokenResponseSchema);
- o365AuthCredentials = { ...o365AuthCredentials, ...(tokenResponse.success && tokenResponse.data) };
+ o365AuthCredentials = { ...o365AuthCredentials, ...tokenResponse };🤖 Prompt for AI Agents
In `@packages/app-store/office365calendar/lib/CalendarService.ts` around lines 263
- 264, The code is assuming parseRefreshTokenResponse returns a
SafeParseReturnType (checking tokenResponse.success and tokenResponse.data);
update CalendarService.ts to handle the refactor where parseRefreshTokenResponse
returns the parsed data directly: call parseRefreshTokenResponse(...) into a
variable like tokenResponseData and then merge only if tokenResponseData is
truthy (e.g., o365AuthCredentials = { ...o365AuthCredentials,
...(tokenResponseData || {}) }) or otherwise adjust to check the new shape (no
.success) so tokenResponse/data access no longer occurs; update references to
tokenResponse.success and tokenResponse.data accordingly.
| }), | ||
| }); | ||
|
|
||
| if (response.statusText !== "OK") throw new HttpError({ statusCode: 400, message: response.statusText }); |
There was a problem hiding this comment.
Prefer !response.ok over comparing statusText.
response.statusText is not guaranteed to be "OK" across all runtimes and HTTP versions (HTTP/2 does not include a reason phrase). Using !response.ok (or checking response.status) is the idiomatic and reliable approach with the Fetch API.
♻️ Suggested fix
- if (response.statusText !== "OK") throw new HttpError({ statusCode: 400, message: response.statusText });
+ if (!response.ok) throw new HttpError({ statusCode: 400, message: response.statusText });📝 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.
| if (response.statusText !== "OK") throw new HttpError({ statusCode: 400, message: response.statusText }); | |
| if (!response.ok) throw new HttpError({ statusCode: 400, message: response.statusText }); |
🤖 Prompt for AI Agents
In `@packages/app-store/salesforce/lib/CalendarService.ts` at line 86, Replace the
brittle statusText check with the Fetch API idiomatic check: in the
CalendarService.ts where the code currently does `if (response.statusText !==
"OK") throw new HttpError(...)`, change it to check `!response.ok` (or
`response.status` range) before throwing the HttpError so the error path uses a
reliable indicator of non-2xx responses; keep the thrown HttpError({ statusCode:
400, message: response.statusText }) but consider using response.status or
response.statusText for the message if available.
| await prisma.credential.update({ | ||
| where: { id: credential.id }, | ||
| data: { key: { ...accessTokenParsed.data, refresh_token: credentialKey.refresh_token } }, | ||
| }); |
There was a problem hiding this comment.
prisma is used but never imported — this will throw a ReferenceError at runtime.
There is no import prisma from "@calcom/prisma" in this file. The other CalendarService files in the PR (e.g., zoho-bigin) do import it.
🐛 Proposed fix
Add the import near the top of the file (e.g., after line 9):
import logger from "@calcom/lib/logger";
+import prisma from "@calcom/prisma";
import type {#!/bin/bash
# Verify that prisma is not imported anywhere in this file
rg -n "import.*prisma" packages/app-store/salesforce/lib/CalendarService.ts🤖 Prompt for AI Agents
In `@packages/app-store/salesforce/lib/CalendarService.ts` around lines 96 - 99,
The file uses prisma in the call prisma.credential.update (updating
credential.id with key data) but never imports it; add the missing import for
the Prisma client (e.g., import prisma from "@calcom/prisma") at the top of
CalendarService.ts so prisma is defined before the prisma.credential.update call
that uses credential, credential.id, credentialKey, and accessTokenParsed.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit