-
Notifications
You must be signed in to change notification settings - Fork 12
OAuth credential sync and app integration enhancements #8
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?
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 <[email protected]> Co-authored-by: Omar López <[email protected]>
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.
Greptile Summary
This PR introduces a comprehensive OAuth credential sync and app integration enhancement system across the Cal.com codebase. The changes implement two main improvements:
1. OAuth Utility Reorganization: The PR systematically moves OAuth-related utilities (encodeOAuthState, decodeOAuthState, createOAuthAppCredential, etc.) from the general _utils directory into a dedicated _utils/oauth subdirectory across all app store integrations. This affects 20+ app integrations including Google Calendar, Office 365, HubSpot, Salesforce, Zoom, and others, creating better code organization and separation of concerns.
2. Centralized OAuth Token Management: The core enhancement introduces a new credential sharing system that allows OAuth token refresh requests to be optionally routed through a centralized endpoint when APP_CREDENTIAL_SHARING_ENABLED is configured. Key components include:
- A new
refreshOAuthTokensutility that wraps existing token refresh logic and conditionally routes to an external credential sync endpoint - A
parseRefreshTokenResponseutility for standardized token response validation - A webhook handler (
/api/webhook/app-credential) for receiving encrypted credential updates - Environment variables for configuring webhook authentication, encryption keys, and sync endpoints
- A feature flag (
APP_CREDENTIAL_SHARING_ENABLED) that requires both webhook secret and encryption key to be present
This system enables self-hosted Cal.com instances embedded within larger applications to sync OAuth credentials rather than requiring users to re-authenticate with services they've already connected to in the parent application. The implementation uses AES256 encryption for secure credential transmission and maintains backward compatibility by falling back to standard OAuth flows when credential sharing is disabled.
The changes integrate across 15+ calendar and app services (Google Calendar, Office 365, Salesforce, HubSpot, Zoom, Webex, Lark, Zoho, etc.), applying the centralized token refresh pattern consistently while preserving existing functionality.
Confidence score: 2/5
- This PR introduces significant security-sensitive changes with multiple critical implementation issues that could cause authentication failures and security vulnerabilities
- Several files contain logic errors, missing imports, unsafe JSON parsing, hardcoded fallback values, and improper error handling that will likely break OAuth flows
- Files requiring immediate attention:
packages/app-store/salesforce/lib/CalendarService.ts(missing prisma import),packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts(invalid Zod schema syntax),apps/web/pages/api/webhook/app-credential.ts(security vulnerabilities),packages/app-store/_utils/oauth/createOAuthAppCredential.ts(incorrect team validation logic), andpackages/app-store/googlecalendar/api/callback.ts(unsafe credential creation)
40 files reviewed, 11 comments
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider using explicit boolean conversion !!() for clarity, as this currently relies on JavaScript's truthy evaluation of strings
| const hubspotRefreshToken: HubspotToken = await refreshOAuthTokens( | ||
| async () => | ||
| await hubspotClient.oauth.tokensApi.createToken( | ||
| "refresh_token", | ||
| undefined, | ||
| WEBAPP_URL + "/api/integrations/hubspot/callback", | ||
| this.client_id, | ||
| this.client_secret, | ||
| refreshToken | ||
| ), | ||
| "hubspot", | ||
| credential.userId | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: The refreshOAuthTokens wrapper now handles credential sync routing, but the return type may not match the expected HubspotToken. The utility returns a raw fetch Response when using the sync endpoint, which would break the subsequent property access on line 192.
| return Promise.reject(new Error("Invalid refreshed tokens were returned")); | ||
| } | ||
|
|
||
| await prisma.credential.update({ |
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.
syntax: Missing import for prisma. Add import prisma from "@calcom/prisma"; to the imports section.
| await prisma.credential.update({ | |
| import type { TokenResponse } from "jsforce"; | |
| import jsforce from "jsforce"; | |
| import { RRule } from "rrule"; | |
| import { z } from "zod"; | |
| import { getLocation } from "@calcom/lib/CalEventParser"; | |
| import { WEBAPP_URL } from "@calcom/lib/constants"; | |
| import { HttpError } from "@calcom/lib/http-error"; | |
| import logger from "@calcom/lib/logger"; | |
| import prisma from "@calcom/prisma"; | |
| import type { | |
| Calendar, | |
| CalendarEvent, | |
| IntegrationCalendar, | |
| NewCalendarEventType, | |
| Person, | |
| } from "@calcom/types/Calendar"; | |
| import type { CredentialPayload } from "@calcom/types/Credential"; | |
| import getAppKeysFromSlug from "../../_utils/getAppKeysFromSlug"; | |
| import parseRefreshTokenResponse from "../../_utils/oauth/parseRefreshTokenResponse"; |
| }, | ||
| }), | ||
| "zoho-bigin", | ||
| credentialId |
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.
logic: Parameter mismatch: refreshOAuthTokens expects userId but receives credentialId. Check other integrations - Google Calendar passes credential.userId.
| [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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: Invalid Zod schema syntax. Computed property keys like [z.string().toString()] are not valid in Zod object schemas and will cause runtime errors.
| [z.string().toString()]: z.number(), | |
| // Allow other properties in the token response | |
| [z.string().optional().toString()]: z.unknown().optional(), | |
| // Assume that any property with a number is the expiry | |
| // Allow other properties in the token response |
| CALCOM_CREDENTIAL_SYNC_ENDPOINT="" | ||
| # Key should match on Cal.com and your application | ||
| # must be 32 bytes for AES256 encryption algorithm | ||
| # You can use: `openssl rand -base64 24` to generate one |
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.
logic: The encryption key generation command suggests 24 bytes (openssl rand -base64 24) but the comment states 32 bytes are required for AES256. Base64 encoding of 24 bytes produces 32 characters, but AES256 needs 32 actual bytes (256 bits). Should be openssl rand -base64 32 or clarify the requirement.
| keys: z.string(), | ||
| }); | ||
| /** */ | ||
| export default async function handler(req: NextApiRequest, res: NextApiResponse) { |
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.
logic: Missing HTTP method validation - any HTTP method is accepted including GET, PUT, DELETE which could expose this endpoint to CSRF attacks
| export default async function handler(req: NextApiRequest, res: NextApiResponse) { | |
| export default async function handler(req: NextApiRequest, res: NextApiResponse) { | |
| if (req.method !== 'POST') { | |
| return res.status(405).json({ message: 'Method not allowed' }); | |
| } |
| if ( | ||
| req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] !== | ||
| process.env.CALCOM_WEBHOOK_SECRET | ||
| ) { |
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.
logic: Direct string comparison is vulnerable to timing attacks - use crypto.timingSafeEqual() for webhook secret validation
| const keys = JSON.parse( | ||
| symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "") | ||
| ); |
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.
logic: Missing error handling for JSON.parse() and symmetricDecrypt() - malformed data will cause runtime errors
|
|
||
| // Decrypt the keys | ||
| const keys = JSON.parse( | ||
| symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "") |
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.
logic: Using empty string fallback for encryption key defeats the security check on line 19 - should throw error if key is missing
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
|
This PR is being marked as stale due to inactivity. |
Test 8