Automated Test: oauth-security-enhanced #379
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 pull request reorganizes OAuth utility modules into a dedicated Changes
Sequence DiagramsequenceDiagram
participant External as External System
participant Webhook as POST /webhook/<br/>app-credential
participant Validator as Secret &<br/>Schema Validator
participant Crypto as AES256<br/>Decryption
participant DB as Database
External->>Webhook: POST with encrypted credentials
Webhook->>Validator: Check feature flag & webhook secret
alt Feature disabled or invalid secret
Validator-->>External: 403 Forbidden
else Valid
Validator->>Validator: Validate request body schema
alt Invalid schema
Validator-->>External: 400 Bad Request
else Valid
Webhook->>Crypto: Decrypt keys with app credential key
Crypto->>Webhook: Decrypted JSON credentials
Webhook->>DB: Check user exists
alt User not found
DB-->>External: 404 Not Found
else User exists
Webhook->>DB: Check app exists by slug
alt App not found
DB-->>External: 404 Not Found
else App exists
Webhook->>DB: Lookup existing credential<br/>by userId + appId
alt Credential found
DB->>DB: Update credential key
DB-->>Webhook: Updated
Webhook-->>External: 200 OK (Updated)
else Credential not found
DB->>DB: Create new credential
DB-->>Webhook: Created
Webhook-->>External: 200 OK (Created)
end
end
end
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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/app-store/googlecalendar/api/callback.ts (1)
53-80:⚠️ Potential issue | 🟠 MajorPre-existing: missing
returnbeforeres.redirectcauses double-redirect.When
state?.installGoogleVideois true and no existing Google Meet credential is found,res.redirect(...)is called on line 71, but execution falls through to the secondres.redirect(...)on line 77. This will attempt to set headers after they've already been sent, resulting in an error at runtime. This is pre-existing and not introduced by this PR, but worth fixing.Proposed fix
await prisma.credential.create({ data: { type: "google_video", key: {}, userId: req.session.user.id, appId: "google-meet", }, }); - res.redirect( + return res.redirect( getSafeRedirectUrl(CAL_URL + "/apps/installed/conferencing?hl=google-meet") ?? getInstalledAppPath({ variant: "conferencing", slug: "google-meet" }) ); } }packages/app-store/salesforce/api/callback.ts (1)
18-21:⚠️ Potential issue | 🟡 MinorPre-existing bug: condition uses
&&instead of||, so non-stringcodevalues (e.g., arrays) slip through.
code === undefined && typeof code !== "string"is onlytruewhencodeisundefined(where the second clause is always redundant). Ifcodeis an array, both clauses arefalseso the guard is bypassed. The intent likely wascode === undefined || typeof code !== "string".Not introduced by this PR, but worth fixing while you're here.
Suggested fix
- if (code === undefined && typeof code !== "string") { + if (code === undefined || typeof code !== "string") {packages/app-store/webex/api/callback.ts (1)
19-27:⚠️ Potential issue | 🟠 MajorPre-existing bug: missing
=afterclient_idin the token URL.Line 20-21 concatenates
...&client_iddirectly with theclient_idvalue, producing a malformed query parameter (e.g.,client_idABC123instead ofclient_id=ABC123). This would likely cause the Webex token exchange to fail.Not introduced by this PR, but this is a correctness issue worth fixing.
Suggested fix
"https://webexapis.com/v1/access_token?grant_type=authorization_code&client_id" + - client_id + + "=" + client_id +Or better, consider using
URLSearchParamsto construct the query string safely.packages/app-store/googlecalendar/lib/CalendarService.ts (1)
97-101:⚠️ Potential issue | 🔴 CriticalBug: storing the
SafeParseReturnTypeobject instead of parsed credentials.
parseRefreshTokenResponsereturns a safeParse-like result ({ success, data, error }), as seen in Zoom (line 104, checks.success) and Office365 usage. Here, the entire result object is assigned tokeyand written to the DB. Previously,googleCredentialSchema.parse(...)returned the parsed data directly.This will persist
{ success: true, data: { access_token, ... } }instead of{ access_token, ... }, corrupting the stored credential and causing token retrieval to fail on subsequent reads (line 75 parsescredential.keywithgoogleCredentialSchema).🐛 Proposed fix
- const key = parseRefreshTokenResponse(googleCredentials, googleCredentialSchema); + const parsedKey = parseRefreshTokenResponse(googleCredentials, googleCredentialSchema); + if (!parsedKey.success) { + this.log.error("Invalid refreshed tokens were returned for Google Calendar"); + return myGoogleAuth; + } + const key = parsedKey.data; await prisma.credential.update({ where: { id: credential.id }, data: { key }, });packages/app-store/salesforce/lib/CalendarService.ts (1)
96-108:⚠️ Potential issue | 🔴 CriticalBug:
prismais not imported and refreshed tokens are not used for the connection.Two issues:
prismais used on line 96 but never imported in this file. This will fail at compile/runtime with aReferenceError.The
jsforce.Connectionon lines 101-108 still uses the oldcredentialKeyvalues (credentialKey.instance_url,credentialKey.access_token) instead of the freshly refreshedaccessTokenParsed.data. The refreshed tokens are saved to the DB but the current request will use the expired token, likely causing API failures.🐛 Proposed fix
Add the missing import at the top of the file:
import prisma from "@calcom/prisma";Then use the refreshed credentials for the connection:
await prisma.credential.update({ where: { id: credential.id }, data: { key: { ...accessTokenParsed.data, refresh_token: credentialKey.refresh_token } }, }); return new jsforce.Connection({ clientId: consumer_key, clientSecret: consumer_secret, redirectUri: WEBAPP_URL + "/api/integrations/salesforce/callback", - instanceUrl: credentialKey.instance_url, - accessToken: credentialKey.access_token, + 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: Replace the unconditional call to
appCredentialWebhookRequestBodySchema.parse with a safe parse flow: call
appCredentialWebhookRequestBodySchema.safeParse(req.body), check the
result.success flag, and if false respond with a 400 (Bad Request) including the
validation errors; otherwise assign the parsed data to reqBody and continue.
Update references to reqBody and preserve existing handler logic so only invalid
bodies return early with a 400 and validation details.
- Around line 57-59: The decryption and JSON parsing of reqBody.keys using
symmetricDecrypt(...) and JSON.parse(...) can throw and are currently unhandled;
wrap the call that assigns keys in a try-catch around
symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY
|| "") and JSON.parse(...) (the code that assigns the keys variable), catch
errors, log a non-sensitive error message, and return a safe client error
response (e.g., 400 with a generic "invalid credentials" message) instead of
letting the exception propagate; ensure no sensitive details or stack traces are
returned to the client.
- Around line 17-29: The handler must reject non-POSTs and must not allow a
missing secret to bypass validation: in the exported handler function, first
enforce req.method === "POST" and return 405 for others; then ensure
process.env.CALCOM_WEBHOOK_SECRET is defined (if not, return 403) before
comparing it to the incoming header (use process.env.CALCOM_WEBHOOK_HEADER_NAME
|| "calcom-webhook-secret" to read the header); finally perform the strict
compare between the known secret and the header value and return 403 on
mismatch. Reference: handler, APP_CREDENTIAL_SHARING_ENABLED,
CALCOM_WEBHOOK_SECRET, CALCOM_WEBHOOK_HEADER_NAME.
In `@packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts`:
- Around line 25-27: The code sets refreshTokenResponse.data.refresh_token to
the literal "refresh_token", which masks the absence of a real token; instead,
stop injecting that sentinel: leave refreshTokenResponse.data.refresh_token as
undefined or null when the provider omits it so downstream logic can detect "no
refresh token". Update the conditional in parseRefreshTokenResponse to remove
the assignment and ensure downstream consumers (any code reading
refreshTokenResponse.data.refresh_token) handle undefined/null distinctly from a
real token.
- Around line 5-11: minimumTokenResponseSchema is incorrectly using computed-key
syntax which creates literal strange keys and also drops unknown properties;
replace the object schema with a schema that accepts access_token plus arbitrary
provider fields by defining z.object({ access_token: z.string() }).passthrough()
(or, if you need to enforce numeric expiry values, validate those separately
with z.record(z.number()) or a refinement/transform), and remove the
computed-key entries so expiry/refresh/provider-specific fields are preserved by
parseRefreshTokenResponse.
In `@packages/app-store/_utils/oauth/refreshOAuthTokens.ts`:
- Around line 3-20: refreshOAuthTokens currently returns a raw fetch Response
when APP_CREDENTIAL_SHARING_ENABLED is true but returns the provider-specific
value from refreshFunction() otherwise, causing downstream callers (expecting
AxiosResponse, HubSpot token object, etc.) to break; update refreshOAuthTokens
to always return a normalized contract: call
fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, ...) then await
response.json(), validate/transform that JSON into the same shape(s) your
callers expect (or into a simple common wrapper { provider: string, data: any,
expiryDate?: string, error?: any }), and return that normalized object so code
using refreshFunction(), refreshOAuthTokens, and callers (which inspect
tokenInfo.data, .expiryDate, or .data.error) receive a consistent structure;
keep the refreshFunction() fallback behavior but adapt/normalize its result to
the same contract as well.
- Around line 8-15: Wrap the fetch to
process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT in a try/catch, check response.ok
after the await fetch, and throw a descriptive error (including response.status
and response.statusText or response body) when not ok; specifically update the
code around the fetch call (the use of fetch, URLSearchParams, and the returned
response) so network errors are caught and a clear error is thrown on non-2xx
responses before returning the successful response or parsed body.
In `@packages/app-store/googlecalendar/lib/CalendarService.ts`:
- Around line 86-93: The Google token refresh call in CalendarService is
assuming a GaxiosResponse shape (res?.data) but refreshOAuthTokens can return a
standard fetch Response when APP_CREDENTIAL_SHARING_ENABLED, so update the
handling around the result of refreshOAuthTokens (the call wrapping
myGoogleAuth.refreshToken and the subsequent use of token.access_token) to
detect and normalize both shapes: if res has a .data property use that,
otherwise parse the fetch Response body (use the existing helper
handleErrorsJson or similar) to extract the JSON and assign it to token before
accessing token.access_token; adjust the code paths around refreshOAuthTokens,
myGoogleAuth.refreshToken, googleCredentials and token to ensure a unified token
object is used.
In `@packages/app-store/hubspot/lib/CalendarService.ts`:
- Around line 177-189: refreshOAuthTokens can return either a Token-like object
or a raw fetch Response when credential sharing is enabled, but
CalendarService.ts immediately reads hubspotRefreshToken.expiresIn (and other
Token fields), causing NaN/undefined behavior; update the code around the call
to refreshOAuthTokens (the invocation that passes
hubspotClient.oauth.tokensApi.createToken) to detect if the result is a fetch
Response and, if so, await response.json() and map/normalize its fields to the
HubspotToken shape (accessToken, expiresIn, refreshToken, etc.) before using
expiresIn to compute expiryDate and before updating credentials; alternatively
add a type guard/assertion to ensure hubspotRefreshToken matches the expected
Token shape and throw/log a clear error if not.
In `@packages/app-store/salesforce/lib/CalendarService.ts`:
- Line 86: In CalendarService, the current check uses response.statusText which
is unreliable; change the condition to use response.ok (or check
response.status) when validating the fetch result: replace the `if
(response.statusText !== "OK") throw new HttpError({ statusCode: 400, message:
response.statusText });` with a check like `if (!response.ok) throw new
HttpError({ statusCode: response.status, message: response.statusText ||
String(response.status) });` so the code throws with the real HTTP status and a
fallback message; update the `response` usage and HttpError construction
accordingly.
In `@packages/app-store/zoho-bigin/lib/CalendarService.ts`:
- Around line 85-94: The call to refreshOAuthTokens is passing credentialId (the
DB PK) but the function expects the user's ID (calcomUserId) — use
credential.userId instead. Modify the code that defines credentialId (and/or the
refreshAccessToken / biginAuth flow) to also carry credential.userId and update
the call to refreshOAuthTokens(..., "zoho-bigin", credential.userId); if
refreshAccessToken or biginAuth currently only accept credentialId, add a
parameter (or attach userId to the credential object) so you can pass
credential.userId through to refreshOAuthTokens.
In `@packages/app-store/zohocrm/lib/CalendarService.ts`:
- Line 219: The token expiry calculation sets zohoCrmTokenInfo.data.expiryDate
incorrectly by adding 60*60 (milliseconds) to Date.now(), producing an expiry
~3.6 seconds away; update the assignment in CalendarService where
zohoCrmTokenInfo.data.expiryDate is set to add milliseconds for one hour
(multiply by 1000) — e.g., replace the current expression with Date.now() + 60 *
60 * 1000 (optionally wrapped in Math.round or Math.floor) so the expiry is one
hour in the future.
In `@packages/app-store/zoomvideo/lib/VideoApiAdapter.ts`:
- Line 104: The call to parseRefreshTokenResponse(…, zoomRefreshedTokenSchema)
in VideoApiAdapter.ts now throws on failure, so the existing if
(!parsedToken.success) block is dead; wrap the parseRefreshTokenResponse call in
a try-catch instead: call parseRefreshTokenResponse and assign to parsedToken
inside try, and in catch log the error (include context like "failed to parse
refresh token") and rethrow or convert to the adapter's expected error/return
form so callers get a clear failure; update any references to parsedToken
afterward to assume a successful parse when no exception was thrown.
In `@packages/lib/constants.ts`:
- Around line 103-104: APP_CREDENTIAL_SHARING_ENABLED currently evaluates to the
raw CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY string when both env vars exist; coerce
the expression to a boolean so the key value is never exposed. Change the
initializer for APP_CREDENTIAL_SHARING_ENABLED to return a strict boolean based
on the presence of CALCOM_WEBHOOK_SECRET and
CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY (e.g., wrap the combined check with
Boolean(...) or use double-negation) so imports of
APP_CREDENTIAL_SHARING_ENABLED cannot leak secret values.
🧹 Nitpick comments (6)
packages/app-store/zoho-bigin/api/add.ts (1)
17-17: Hard-coded slug replaces dynamicappConfig.slugin redirect URI.The redirect URI now uses a hard-coded
"zoho-bigin"instead of interpolatingappConfig.slug. Note thatappConfig.slugis still used on line 12 forgetAppKeysFromSlug. If the slug inconfig.jsonever drifts from"zoho-bigin", this would create a mismatch between the key lookup and the redirect URI.Consider using
appConfig.slugconsistently:Suggested change
- const redirectUri = WEBAPP_URL + `/api/integrations/zoho-bigin/callback`; + const redirectUri = WEBAPP_URL + `/api/integrations/${appConfig.slug}/callback`;turbo.json (1)
205-207:CALCOM_WEBHOOK_SECRETis out of alphabetical order.It's placed after
CALENDSO_ENCRYPTION_KEY, butCALCOM_W...sorts beforeCALEND.... Swap lines 207 and 206 to maintain the alphabetical convention used throughout this array.Proposed fix
"CALCOM_WEBHOOK_HEADER_NAME", - "CALENDSO_ENCRYPTION_KEY", "CALCOM_WEBHOOK_SECRET", + "CALENDSO_ENCRYPTION_KEY", "CI",packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts (1)
13-30:safeParseresult type is not narrowed —refreshTokenResponse.datamay be typed asunknown.When using
schema.safeParse()whereschemaisz.ZodTypeAny, the inferreddatatype isany. This is acceptable at runtime but loses type safety. Additionally, the function mutates.data.refresh_tokenon the safe-parse result object — modifying the parse result in-place is fragile. Consider constructing a new object with the defaults spread in.packages/app-store/webex/lib/VideoApiAdapter.ts (1)
151-151: Debugconsole.logstatements should be removed before merging.Lines 151, 184–186, 194, and 223 contain
console.logcalls that appear to be debug artifacts (e.g.,"result of accessToken in fetchWebexApi","meting body"). These leak implementation details at runtime and should be removed or converted to structured logger calls.packages/app-store/office365calendar/lib/CalendarService.ts (1)
263-264: Silent failure when token parsing fails.If
parseRefreshTokenResponsereturns{ success: false }, the spread on line 264 (...(false)) is a no-op, so stale credentials are silently re-saved to the DB (lines 265-272) without any log or error signal. The AI summary notes that inline error logging for token parsing failure was removed here. Consider adding a warning log to aid debugging when token refresh silently fails.💡 Proposed improvement
const tokenResponse = parseRefreshTokenResponse(responseJson, refreshTokenResponseSchema); + if (!tokenResponse.success) { + this.log.warn("Office365 token refresh: failed to parse refreshed token response"); + } o365AuthCredentials = { ...o365AuthCredentials, ...(tokenResponse.success && tokenResponse.data) };apps/web/pages/api/webhook/app-credential.ts (1)
16-16: Empty JSDoc comment.The
/** */comment on line 16 appears to be an incomplete placeholder. Either add meaningful documentation for the handler or remove it.
| 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 and brittle webhook secret validation.
-
No method check: This handler accepts any HTTP method. It should restrict to
POSTonly. -
Webhook secret bypass when unset: If
CALCOM_WEBHOOK_SECRETis undefined and the request omits the header, both sides areundefined, soundefined !== undefinedevaluates tofalseand the check passes. TheAPP_CREDENTIAL_SHARING_ENABLEDguard on line 19 mitigates this (since it requires the env var to be set), but defense-in-depth is warranted.
🛡️ Proposed fix
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 webhookSecret = process.env.CALCOM_WEBHOOK_SECRET;
+ if (!webhookSecret) {
+ return res.status(500).json({ message: "Webhook secret is not configured" });
+ }
if (
req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] !==
- process.env.CALCOM_WEBHOOK_SECRET
+ webhookSecret
) {
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, The
handler must reject non-POSTs and must not allow a missing secret to bypass
validation: in the exported handler function, first enforce req.method ===
"POST" and return 405 for others; then ensure process.env.CALCOM_WEBHOOK_SECRET
is defined (if not, return 403) before comparing it to the incoming header (use
process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret" to read the
header); finally perform the strict compare between the known secret and the
header value and return 403 on mismatch. Reference: handler,
APP_CREDENTIAL_SHARING_ENABLED, CALCOM_WEBHOOK_SECRET,
CALCOM_WEBHOOK_HEADER_NAME.
| return res.status(403).json({ message: "Invalid webhook secret" }); | ||
| } | ||
|
|
||
| const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body); |
There was a problem hiding this comment.
Unhandled ZodError on invalid request body.
z.parse() throws a ZodError if the body doesn't match the schema, resulting in an unhandled 500 with a stack trace. Use .safeParse() and return a 400 for invalid input.
🛡️ Proposed fix
- const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body);
+ const parsed = appCredentialWebhookRequestBodySchema.safeParse(req.body);
+ if (!parsed.success) {
+ return res.status(400).json({ message: "Invalid request body" });
+ }
+ 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" }); | |
| } | |
| const reqBody = parsed.data; |
🤖 Prompt for AI Agents
In `@apps/web/pages/api/webhook/app-credential.ts` at line 31, Replace the
unconditional call to appCredentialWebhookRequestBodySchema.parse with a safe
parse flow: call appCredentialWebhookRequestBodySchema.safeParse(req.body),
check the result.success flag, and if false respond with a 400 (Bad Request)
including the validation errors; otherwise assign the parsed data to reqBody and
continue. Update references to reqBody and preserve existing handler logic so
only invalid bodies return early with a 400 and validation details.
| const keys = JSON.parse( | ||
| symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "") | ||
| ); |
There was a problem hiding this comment.
Unhandled errors from decryption and JSON parsing.
Both symmetricDecrypt (on invalid ciphertext) and JSON.parse (on malformed JSON) can throw. Without a try-catch, these surface as 500 errors with potentially sensitive stack traces.
🛡️ Proposed fix
- 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 (error) {
+ 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.
| 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 (error) { | |
| 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 57 - 59, The
decryption and JSON parsing of reqBody.keys using symmetricDecrypt(...) and
JSON.parse(...) can throw and are currently unhandled; wrap the call that
assigns keys in a try-catch around symmetricDecrypt(reqBody.keys,
process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "") and JSON.parse(...) (the
code that assigns the keys variable), catch errors, log a non-sensitive error
message, and return a safe client error response (e.g., 400 with a generic
"invalid credentials" message) instead of letting the exception propagate;
ensure no sensitive details or stack traces are returned to the client.
| 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
# First, verify the file exists and check its content
find . -name "parseRefreshTokenResponse.ts" -type fRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Test the Zod schema behavior directly
cd /tmp
cat > test_schema.mjs << 'EOF'
import { z } from "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 current schema ===");
const currentSchema = z.object({
access_token: z.string(),
[z.string().toString()]: z.number(),
[z.string().optional().toString()]: z.unknown().optional(),
});
const testResponse = {
access_token: "test_token",
refresh_token: "test_refresh",
expires_in: 3600,
expiry_date: 1700000000,
};
const result = currentSchema.safeParse(testResponse);
console.log("Parse success:", result.success);
console.log("Input keys:", Object.keys(testResponse));
console.log("Output keys:", result.success ? Object.keys(result.data) : "N/A");
console.log("Full output:", JSON.stringify(result.data || result.error.issues, null, 2));
console.log("\n=== Testing with passthrough ===");
const passthroughSchema = z.object({
access_token: z.string(),
}).passthrough();
const result2 = passthroughSchema.safeParse(testResponse);
console.log("Parse success:", result2.success);
console.log("Output keys:", result2.success ? Object.keys(result2.data) : "N/A");
console.log("Full output:", JSON.stringify(result2.data, null, 2));
EOF
# Try to run with node
node test_schema.mjs 2>&1 || echo "Node execution failed or not available"Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1039
🏁 Script executed:
#!/bin/bash
# Read the file to verify the schema definition
cat -n ./packages/app-store/_utils/oauth/parseRefreshTokenResponse.tsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1344
minimumTokenResponseSchema does not work as intended — computed-key syntax is not a Zod catch-all.
[z.string().toString()] evaluates to a literal string key (the toString() representation of the Zod schema object), not a dynamic record key matcher. The schema ends up defining two oddly-named literal keys rather than "any string key → number" and "any optional key → unknown". Additionally, z.object() strips unrecognized keys by default, so expiry fields, refresh tokens, and other provider-specific properties will be silently dropped from the parsed output.
To accept access_token plus arbitrary additional fields, 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();📝 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 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 is incorrectly using computed-key syntax which
creates literal strange keys and also drops unknown properties; replace the
object schema with a schema that accepts access_token plus arbitrary provider
fields by defining z.object({ access_token: z.string() }).passthrough() (or, if
you need to enforce numeric expiry values, validate those separately with
z.record(z.number()) or a refinement/transform), and remove the computed-key
entries so expiry/refresh/provider-specific fields are preserved by
parseRefreshTokenResponse.
| if (!refreshTokenResponse.data.refresh_token) { | ||
| refreshTokenResponse.data.refresh_token = "refresh_token"; | ||
| } |
There was a problem hiding this comment.
Setting refresh_token to the literal string "refresh_token" is a risky sentinel value.
If a provider legitimately omits refresh_token (e.g., because it doesn't rotate refresh tokens), injecting a dummy string will cause a hard failure on the next token refresh attempt — but with a confusing error from the provider rather than a clear local error. Consider leaving it undefined or null so downstream code can distinguish "no refresh token" from "has a refresh token."
🤖 Prompt for AI Agents
In `@packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts` around lines 25
- 27, The code sets refreshTokenResponse.data.refresh_token to the literal
"refresh_token", which masks the absence of a real token; instead, stop
injecting that sentinel: leave refreshTokenResponse.data.refresh_token as
undefined or null when the provider omits it so downstream logic can detect "no
refresh token". Update the conditional in parseRefreshTokenResponse to remove
the assignment and ensure downstream consumers (any code reading
refreshTokenResponse.data.refresh_token) handle undefined/null distinctly from a
real token.
| }), | ||
| }); | ||
|
|
||
| if (response.statusText !== "OK") throw new HttpError({ statusCode: 400, message: response.statusText }); |
There was a problem hiding this comment.
Fragile status check: response.statusText is unreliable.
response.statusText is not guaranteed to be "OK" even for 200 responses — the Fetch spec allows servers to send any status text, and HTTP/2 doesn't include it at all. Use response.ok instead.
🐛 Proposed fix
- if (response.statusText !== "OK") throw new HttpError({ statusCode: 400, message: response.statusText });
+ if (!response.ok) throw new HttpError({ statusCode: response.status, message: response.statusText });🤖 Prompt for AI Agents
In `@packages/app-store/salesforce/lib/CalendarService.ts` at line 86, In
CalendarService, the current check uses response.statusText which is unreliable;
change the condition to use response.ok (or check response.status) when
validating the fetch result: replace the `if (response.statusText !== "OK")
throw new HttpError({ statusCode: 400, message: response.statusText });` with a
check like `if (!response.ok) throw new HttpError({ statusCode: response.status,
message: response.statusText || String(response.status) });` so the code throws
with the real HTTP status and a fallback message; update the `response` usage
and HttpError construction accordingly.
| const tokenInfo = await refreshOAuthTokens( | ||
| async () => | ||
| await axios.post(accountsUrl, qs.stringify(formData), { | ||
| headers: { | ||
| "Content-Type": "application/x-www-form-urlencoded;charset=utf-8", | ||
| }, | ||
| }), | ||
| "zoho-bigin", | ||
| credentialId | ||
| ); |
There was a problem hiding this comment.
Wrong identifier passed as userId — credentialId is the credential's PK, not the user's ID.
credentialId (from line 53: const credentialId = credential.id) is the credential row's database primary key, not the user ID. The refreshOAuthTokens function expects userId (used as calcomUserId in the sync endpoint payload). This will send an incorrect identifier to the credential sync endpoint when sharing is enabled.
All other callers in this PR pass credential.userId. This file should do the same.
Proposed fix
The refreshAccessToken method needs access to credential.userId. Either store credential (or credential.userId) alongside credentialId, or pass it through:
- private async refreshAccessToken(credentialId: number, credentialKey: BiginToken) {
+ private async refreshAccessToken(credential: CredentialPayload, credentialKey: BiginToken) {
+ const credentialId = credential.id;
...
const tokenInfo = await refreshOAuthTokens(
async () =>
await axios.post(accountsUrl, qs.stringify(formData), {
headers: {
"Content-Type": "application/x-www-form-urlencoded;charset=utf-8",
},
}),
"zoho-bigin",
- credentialId
+ credential.userId
);And update the call site in biginAuth accordingly.
🤖 Prompt for AI Agents
In `@packages/app-store/zoho-bigin/lib/CalendarService.ts` around lines 85 - 94,
The call to refreshOAuthTokens is passing credentialId (the DB PK) but the
function expects the user's ID (calcomUserId) — use credential.userId instead.
Modify the code that defines credentialId (and/or the refreshAccessToken /
biginAuth flow) to also carry credential.userId and update the call to
refreshOAuthTokens(..., "zoho-bigin", credential.userId); if refreshAccessToken
or biginAuth currently only accept credentialId, add a parameter (or attach
userId to the credential object) so you can pass credential.userId through to
refreshOAuthTokens.
| }), | ||
| "zohocrm", | ||
| credential.userId | ||
| ); | ||
| if (!zohoCrmTokenInfo.data.error) { | ||
| // set expiry date as offset from current time. | ||
| zohoCrmTokenInfo.data.expiryDate = Math.round(Date.now() + 60 * 60); |
There was a problem hiding this comment.
Pre-existing bug: expiry offset is ~3.6 seconds, not 1 hour.
Math.round(Date.now() + 60 * 60) adds 3600 milliseconds to Date.now() (which is in ms). This sets the token expiry to ~3.6 seconds from now, causing near-immediate re-refresh on every call. Should likely be Date.now() + 60 * 60 * 1000.
This is pre-existing and not introduced by this PR, but worth flagging.
Proposed fix
- zohoCrmTokenInfo.data.expiryDate = Math.round(Date.now() + 60 * 60);
+ zohoCrmTokenInfo.data.expiryDate = Math.round(Date.now() + 60 * 60 * 1000);📝 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.
| zohoCrmTokenInfo.data.expiryDate = Math.round(Date.now() + 60 * 60); | |
| zohoCrmTokenInfo.data.expiryDate = Math.round(Date.now() + 60 * 60 * 1000); |
🤖 Prompt for AI Agents
In `@packages/app-store/zohocrm/lib/CalendarService.ts` at line 219, The token
expiry calculation sets zohoCrmTokenInfo.data.expiryDate incorrectly by adding
60*60 (milliseconds) to Date.now(), producing an expiry ~3.6 seconds away;
update the assignment in CalendarService where zohoCrmTokenInfo.data.expiryDate
is set to add milliseconds for one hour (multiply by 1000) — e.g., replace the
current expression with Date.now() + 60 * 60 * 1000 (optionally wrapped in
Math.round or Math.floor) so the expiry is one hour in the future.
| @@ -94,7 +101,7 @@ const zoomAuth = (credential: CredentialPayload) => { | |||
| } | |||
| } | |||
| // We check the if the new credentials matches the expected response structure | |||
| const parsedToken = zoomRefreshedTokenSchema.safeParse(responseBody); | |||
| const parsedToken = parseRefreshTokenResponse(responseBody, zoomRefreshedTokenSchema); | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd parseRefreshTokenResponse --type fRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 150
🏁 Script executed:
cat -n packages/app-store/zoomvideo/lib/VideoApiAdapter.ts | head -150 | tail -50Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 2338
🏁 Script executed:
cat -n packages/app-store/_utils/oauth/parseRefreshTokenResponse.tsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1344
Behavioral difference confirmed: parseRefreshTokenResponse throws instead of returning falsy result.
The check on line 108 (if (!parsedToken.success)) is now dead code. parseRefreshTokenResponse throws an error on parse failure (line 21-22 in parseRefreshTokenResponse.ts) rather than returning { success: false, ... }, so the success flag will always be true if execution reaches line 111. The error message on line 109 is unreachable.
Update the code to either:
- Remove the dead check (lines 108-110) since errors are thrown, or
- Wrap the call in try-catch to handle the thrown error
🤖 Prompt for AI Agents
In `@packages/app-store/zoomvideo/lib/VideoApiAdapter.ts` at line 104, The call to
parseRefreshTokenResponse(…, zoomRefreshedTokenSchema) in VideoApiAdapter.ts now
throws on failure, so the existing if (!parsedToken.success) block is dead; wrap
the parseRefreshTokenResponse call in a try-catch instead: call
parseRefreshTokenResponse and assign to parsedToken inside try, and in catch log
the error (include context like "failed to parse refresh token") and rethrow or
convert to the adapter's expected error/return form so callers get a clear
failure; update any references to parsedToken afterward to assume a successful
parse when no exception was thrown.
| export const APP_CREDENTIAL_SHARING_ENABLED = | ||
| process.env.CALCOM_WEBHOOK_SECRET && process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY; |
There was a problem hiding this comment.
APP_CREDENTIAL_SHARING_ENABLED leaks the encryption key value — coerce to boolean.
Because && returns the second operand's value when both are truthy, this constant holds the raw CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY string. Any code importing APP_CREDENTIAL_SHARING_ENABLED (e.g., for logging or error context) could inadvertently expose the encryption key.
🔒 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);📝 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 const APP_CREDENTIAL_SHARING_ENABLED = | |
| process.env.CALCOM_WEBHOOK_SECRET && process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY; | |
| export const APP_CREDENTIAL_SHARING_ENABLED = | |
| !!(process.env.CALCOM_WEBHOOK_SECRET && process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY); |
🤖 Prompt for AI Agents
In `@packages/lib/constants.ts` around lines 103 - 104,
APP_CREDENTIAL_SHARING_ENABLED currently evaluates to the raw
CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY string when both env vars exist; coerce the
expression to a boolean so the key value is never exposed. Change the
initializer for APP_CREDENTIAL_SHARING_ENABLED to return a strict boolean based
on the presence of CALCOM_WEBHOOK_SECRET and
CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY (e.g., wrap the combined check with
Boolean(...) or use double-negation) so imports of
APP_CREDENTIAL_SHARING_ENABLED cannot leak secret values.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Refactor