OAuth credential sync and app integration enhancements#8
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>
WalkthroughThis update introduces a centralized app credential sharing and synchronization mechanism, primarily by adding new environment variables and feature flags, a webhook API endpoint, and utility functions for OAuth token refresh and response parsing. Several integration modules are refactored to use these new utilities, and import paths are reorganized for consistency. No existing logic is removed. Changes
Sequence Diagram(s)sequenceDiagram
participant ExternalApp as External App
participant WebhookAPI as Webhook API Endpoint
participant DB as Database
ExternalApp->>WebhookAPI: POST /api/webhook/app-credential (with encrypted keys)
WebhookAPI->>WebhookAPI: Validate headers & body, decrypt keys
WebhookAPI->>DB: Find or create credential for user & app
WebhookAPI-->>ExternalApp: Respond with status (created/updated/error)
sequenceDiagram
participant Integration as Integration Service
participant RefreshUtil as refreshOAuthTokens Utility
participant SyncEndpoint as Credential Sync Endpoint
Integration->>RefreshUtil: Request token refresh (with appSlug, userId)
alt Sharing enabled & userId present
RefreshUtil->>SyncEndpoint: POST /sync-endpoint (userId, appSlug)
SyncEndpoint-->>RefreshUtil: Token response
else
RefreshUtil->>Integration: Call original refresh function
end
RefreshUtil-->>Integration: Return token data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (5)
packages/app-store/salesforce/api/callback.ts (1)
18-21: Guard condition letscodeslip through when it is an array
req.query.codecan bestring | string[] | undefined.
Using logical AND (&&) only rejects the case wherecodeis simultaneously undefined and not a string – it silently accepts an array (e.g.["a","b"]). That breaks the OAuth flow and can be abused.- if (code === undefined && typeof code !== "string") { + if (code === undefined || typeof code !== "string") {packages/app-store/hubspot/api/callback.ts (1)
24-28: Missing check for absentcodeparameterWith the current condition the handler accepts a request that omits
codeentirely, which causes HubSpot to throw later. Reject early:- if (code && typeof code !== "string") { + if (code === undefined || typeof code !== "string") {The same validation pattern appears in several callback handlers – consider extracting a small helper to enforce it consistently.
packages/app-store/zohocrm/api/callback.ts (1)
18-22: Samecodevalidation bug as in Salesforce handler- if (code === undefined && typeof code !== "string") { + if (code === undefined || typeof code !== "string") {Please patch across all similar handlers to avoid inconsistent behaviour.
packages/app-store/zoho-bigin/api/callback.ts (1)
17-20:codemay be undefined yet no error is returned- if (code && typeof code !== "string") { + if (code === undefined || typeof code !== "string") {This aligns the behaviour with other OAuth callbacks.
packages/app-store/salesforce/lib/CalendarService.ts (1)
101-108: Use refreshed tokens for connection.The jsforce connection is created with the original credential tokens instead of the refreshed ones:
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, });
♻️ Duplicate comments (4)
packages/app-store/webex/api/callback.ts (1)
7-7: Same alias recommendation as for TandemVideo callbackSee previous comment about replacing deep relative imports with a path alias for OAuth utilities.
packages/app-store/zoomvideo/api/callback.ts (1)
7-7: Same alias recommendation as for TandemVideo callbackSee earlier note about using a path alias for
createOAuthAppCredential.packages/app-store/larkcalendar/api/callback.ts (1)
10-10: Same alias recommendation as for TandemVideo callbackUsing an alias (e.g.,
@utils/oauth/decodeOAuthState) will make this import more resilient.packages/app-store/office365video/api/callback.ts (1)
9-10: Same alias recommendation as for TandemVideo callbackApply a path alias for both
createOAuthAppCredentialanddecodeOAuthStateto avoid long relative paths.
🧹 Nitpick comments (8)
.env.example (1)
233-245: Well-documented credential sync configuration with minor formatting improvements needed.The new APP CREDENTIAL SYNC section is comprehensive and well-documented. However, there are some formatting inconsistencies to address:
-CALCOM_WEBHOOK_SECRET="" +CALCOM_WEBHOOK_SECRET= -CALCOM_WEBHOOK_HEADER_NAME="calcom-webhook-secret" +CALCOM_WEBHOOK_HEADER_NAME=calcom-webhook-secret -CALCOM_CREDENTIAL_SYNC_ENDPOINT="" +CALCOM_CREDENTIAL_SYNC_ENDPOINT= -CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY="" +CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY=The documentation clearly explains the 32-byte requirement for AES256 encryption and provides helpful
opensslcommands for key generation.packages/app-store/zoomvideo/api/add.ts (1)
8-8: Consider introducing a path alias to avoid deep relative importsWith OAuth helpers now living under
_utils/oauth, a growing number of files use long relative paths like"../../_utils/oauth/encodeOAuthState". Adding apathsalias intsconfig.json, e.g."paths": { "@utils/oauth/*": ["packages/app-store/_utils/oauth/*"] }would shorten these imports to
@utils/oauth/encodeOAuthState, improving readability and easing future moves.packages/app-store/tandemvideo/api/callback.ts (1)
7-7: Prefer a path alias over deep relative importThe new path works, but
../../_utils/oauth/createOAuthAppCredentialis brittle; folder moves will break multiple files.
Consider adding a@utils/oauth(or similar) alias intsconfig.jsonand switching to:-import createOAuthAppCredential from "../../_utils/oauth/createOAuthAppCredential"; +import createOAuthAppCredential from "@utils/oauth/createOAuthAppCredential";This keeps imports stable and improves readability across the app-store packages.
packages/app-store/_utils/oauth/createOAuthAppCredential.ts (1)
6-7: Redundant directory hop in relative import
createOAuthAppCredential.tsalready lives inside…/_utils/oauth/.
Using../oauth/decodeOAuthStateclimbs up one level only to descend back into the same folder. This works but is brittle if the folder is renamed.-import { decodeOAuthState } from "../oauth/decodeOAuthState"; +import { decodeOAuthState } from "./decodeOAuthState";[note: keep
throwIfNotHaveAdminAccessToTeamas-is]packages/app-store/_utils/oauth/refreshOAuthTokens.ts (2)
3-3: Improve type safety for better maintainability.The function signature uses
anyfor both therefreshFunctionparameter and return type, which reduces type safety.Consider improving the typing:
-const refreshOAuthTokens = async (refreshFunction: () => any, appSlug: string, userId: number | null) => { +const refreshOAuthTokens = async <T>(refreshFunction: () => Promise<T>, appSlug: string, userId: number | null): Promise<T> => {
5-5: Consider validating the credential sync endpoint URL.While the current checks are adequate, validating the endpoint URL format could prevent runtime errors.
- if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId) { + if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId) { + try { + new URL(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT); + } catch { + console.warn('Invalid CALCOM_CREDENTIAL_SYNC_ENDPOINT URL, falling back to original refresh'); + return await refreshFunction(); + }apps/web/pages/api/webhook/app-credential.ts (1)
56-92: Solid credential management logic with one improvement opportunity.The encryption/decryption and database operations are well implemented. The manual upsert logic is appropriate given the constraint.
Consider adding error handling around the JSON.parse operation to gracefully handle malformed encrypted data:
// 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 (error) { + return res.status(400).json({ message: "Invalid encrypted keys format" }); + }packages/app-store/salesforce/lib/CalendarService.ts (1)
75-99: Consider consistency with other integrations.This integration implements token refresh directly while others use the
refreshOAuthTokensutility. Consider whether this should follow the same pattern for consistency, especially if the credential sharing feature should apply to Salesforce as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
.env.example(2 hunks)apps/web/pages/api/webhook/app-credential.ts(1 hunks)packages/app-store/_utils/oauth/createOAuthAppCredential.ts(1 hunks)packages/app-store/_utils/oauth/decodeOAuthState.ts(1 hunks)packages/app-store/_utils/oauth/encodeOAuthState.ts(1 hunks)packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts(1 hunks)packages/app-store/_utils/oauth/refreshOAuthTokens.ts(1 hunks)packages/app-store/googlecalendar/api/add.ts(1 hunks)packages/app-store/googlecalendar/api/callback.ts(1 hunks)packages/app-store/googlecalendar/lib/CalendarService.ts(2 hunks)packages/app-store/hubspot/api/add.ts(1 hunks)packages/app-store/hubspot/api/callback.ts(1 hunks)packages/app-store/hubspot/lib/CalendarService.ts(2 hunks)packages/app-store/larkcalendar/api/add.ts(1 hunks)packages/app-store/larkcalendar/api/callback.ts(1 hunks)packages/app-store/larkcalendar/lib/CalendarService.ts(2 hunks)packages/app-store/office365calendar/api/add.ts(1 hunks)packages/app-store/office365calendar/api/callback.ts(1 hunks)packages/app-store/office365calendar/lib/CalendarService.ts(2 hunks)packages/app-store/office365video/api/add.ts(1 hunks)packages/app-store/office365video/api/callback.ts(1 hunks)packages/app-store/office365video/lib/VideoApiAdapter.ts(2 hunks)packages/app-store/salesforce/api/add.ts(1 hunks)packages/app-store/salesforce/api/callback.ts(1 hunks)packages/app-store/salesforce/lib/CalendarService.ts(4 hunks)packages/app-store/stripepayment/api/callback.ts(1 hunks)packages/app-store/tandemvideo/api/callback.ts(1 hunks)packages/app-store/webex/api/callback.ts(1 hunks)packages/app-store/webex/lib/VideoApiAdapter.ts(2 hunks)packages/app-store/zoho-bigin/api/add.ts(2 hunks)packages/app-store/zoho-bigin/api/callback.ts(1 hunks)packages/app-store/zoho-bigin/lib/CalendarService.ts(2 hunks)packages/app-store/zohocrm/api/_getAdd.ts(1 hunks)packages/app-store/zohocrm/api/callback.ts(1 hunks)packages/app-store/zohocrm/lib/CalendarService.ts(2 hunks)packages/app-store/zoomvideo/api/add.ts(1 hunks)packages/app-store/zoomvideo/api/callback.ts(1 hunks)packages/app-store/zoomvideo/lib/VideoApiAdapter.ts(3 hunks)packages/lib/constants.ts(1 hunks)turbo.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/app-store/zoho-bigin/api/add.ts (1)
packages/lib/constants.ts (1)
WEBAPP_URL(9-15)
packages/app-store/salesforce/lib/CalendarService.ts (1)
packages/prisma/index.ts (1)
prisma(56-56)
packages/app-store/hubspot/lib/CalendarService.ts (2)
packages/app-store/hubspot/api/callback.ts (1)
HubspotToken(17-19)packages/lib/constants.ts (1)
WEBAPP_URL(9-15)
packages/app-store/_utils/oauth/refreshOAuthTokens.ts (1)
packages/lib/constants.ts (1)
APP_CREDENTIAL_SHARING_ENABLED(103-104)
packages/app-store/office365calendar/lib/CalendarService.ts (1)
packages/lib/errors.ts (1)
handleErrorsJson(14-30)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 237-237: [QuoteCharacter] The value has quote characters (', ")
[warning] 239-239: [QuoteCharacter] The value has quote characters (', ")
[warning] 239-239: [UnorderedKey] The CALCOM_WEBHOOK_HEADER_NAME key should go before the CALCOM_WEBHOOK_SECRET key
[warning] 240-240: [QuoteCharacter] The value has quote characters (', ")
[warning] 240-240: [UnorderedKey] The CALCOM_CREDENTIAL_SYNC_ENDPOINT key should go before the CALCOM_WEBHOOK_HEADER_NAME key
[warning] 244-244: [QuoteCharacter] The value has quote characters (', ")
[warning] 244-244: [UnorderedKey] The CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY key should go before the CALCOM_CREDENTIAL_SYNC_ENDPOINT key
🔇 Additional comments (48)
packages/lib/constants.ts (1)
103-104: LGTM! Well-implemented feature flag.The
APP_CREDENTIAL_SHARING_ENABLEDconstant properly checks for both required environment variables, ensuring the credential sharing feature is only enabled when fully configured. This prevents runtime errors from partial configurations.packages/app-store/_utils/oauth/encodeOAuthState.ts (1)
3-3: LGTM! Correct import path adjustment.The import path update correctly reflects the OAuth utilities reorganization into the dedicated
oauthsubdirectory.packages/app-store/_utils/oauth/decodeOAuthState.ts (1)
3-3: LGTM! Consistent import path update.The import path correctly reflects the OAuth utilities reorganization, maintaining consistency with the other OAuth utility files.
packages/app-store/larkcalendar/api/add.ts (1)
9-9: LGTM! Import path updated for OAuth utility reorganization.The import path correctly reflects the new location of OAuth utilities in the dedicated
oauthsubdirectory, maintaining consistency with the broader refactoring effort.packages/app-store/hubspot/api/add.ts (1)
7-7: Import path realigned to new OAuth utils folder – looks correctNothing else to flag for this change.
packages/app-store/zohocrm/api/_getAdd.ts (1)
7-7: Import path update looks correctNo issues spotted; the handler still calls
encodeOAuthStateright below.packages/app-store/office365calendar/api/callback.ts (1)
9-9: decodeOAuthState Module VerifiedThe file
packages/app-store/_utils/oauth/decodeOAuthState.tsexists and exportsdecodeOAuthStateas a named function (line 5). No further changes are needed.packages/app-store/salesforce/api/add.ts (1)
7-7: Import path realigned – looks goodNo further action needed.
packages/app-store/stripepayment/api/callback.ts (1)
5-7: Import paths verified – no changes neededI confirmed that both imports resolve correctly:
createOAuthAppCredentialexists atpackages/app-store/_utils/oauth/createOAuthAppCredential.tsgetInstalledAppPathexists atpackages/app-store/_utils/getInstalledAppPath.tsAll relative paths are valid; the code can be merged as-is.
packages/app-store/googlecalendar/api/add.ts (1)
6-8: Import path confirmed for encodeOAuthStateThe helper file exists at
packages/app-store/_utils/oauth/encodeOAuthState.ts, so the import inpackages/app-store/googlecalendar/api/add.tsis correct. No changes needed.packages/app-store/office365calendar/api/add.ts (1)
6-8: encodeOAuthState import verified
TheencodeOAuthStateutility exists atpackages/app-store/_utils/oauth/encodeOAuthState.ts. No changes are required here.packages/app-store/googlecalendar/api/callback.ts (1)
8-11: decodeOAuthState import path is correctVerified that
packages/app-store/_utils/oauth/decodeOAuthState.tsexists and the relative import inpackages/app-store/googlecalendar/api/callback.ts("../../_utils/oauth/decodeOAuthState") correctly resolves. No changes needed.packages/app-store/office365video/api/add.ts (1)
6-8: encodeOAuthState Import Path ConfirmedThe
encodeOAuthStatehelper exists atpackages/app-store/_utils/oauth/encodeOAuthState.ts, so the import inpackages/app-store/office365video/api/add.tsis correct and OAuth initiation won’t fail. No further changes needed.packages/app-store/salesforce/api/callback.ts (1)
9-11: Import-path update looks correctMoving OAuth helpers under
_utils/oauthis consistent with the new structure and the relative path resolves correctly from this location. No action needed.packages/app-store/hubspot/api/callback.ts (1)
10-12: Import-path update LGTMpackages/app-store/zohocrm/api/callback.ts (1)
10-12: Import-path update LGTMpackages/app-store/zoho-bigin/api/callback.ts (1)
10-12: Import-path update LGTMpackages/app-store/zoho-bigin/api/add.ts (2)
7-7: LGTM! Import path correctly updated.The import path change aligns with the OAuth utilities reorganization into the
oauthsubdirectory.
17-17: No breaking change: hardcoded slug matches config.jsonThe
config.jsonfor this package defines"slug": "zoho-bigin", so the literal/api/integrations/zoho-bigin/callbackURI is identical to using/api/integrations/${appConfig.slug}/callbackhere. No other references toappConfig.sluginadd.tsneed updating.turbo.json (1)
200-201: LGTM! Environment variables properly added for credential sharing feature.The new environment variables for app credential sharing (
CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY,CALCOM_CREDENTIAL_SYNC_ENDPOINT,CALCOM_WEBHOOK_HEADER_NAME,CALCOM_WEBHOOK_SECRET) are correctly added to the globalEnv array and maintain alphabetical ordering.Also applies to: 205-205, 207-207
packages/app-store/webex/lib/VideoApiAdapter.ts (2)
11-11: LGTM! Import correctly added for OAuth utility.The import for
refreshOAuthTokensfollows the consistent OAuth utilities reorganization.
62-78: LGTM! OAuth token refresh properly refactored.The token refresh logic has been correctly refactored to use the
refreshOAuthTokensutility while preserving the original fetch implementation. The service identifier "webex" and credential.userId are properly passed as parameters.packages/app-store/zohocrm/lib/CalendarService.ts (2)
19-19: LGTM! Import correctly added for OAuth utility.The import for
refreshOAuthTokensaligns with the OAuth utilities reorganization.
204-216: LGTM! OAuth token refresh properly refactored.The token refresh logic has been correctly refactored to use the
refreshOAuthTokensutility while preserving the original axios implementation. The service identifier "zohocrm" and credential.userId are properly passed.packages/app-store/office365video/lib/VideoApiAdapter.ts (2)
12-12: LGTM! Import correctly added for OAuth utility.The import for
refreshOAuthTokensfollows the consistent OAuth utilities reorganization pattern.
61-75: LGTM! OAuth token refresh properly refactored.The token refresh logic has been correctly refactored to use the
refreshOAuthTokensutility while preserving the original fetch implementation. The service identifier "msteams" appropriately represents the Microsoft Teams service, and credential.userId is properly passed.packages/app-store/larkcalendar/lib/CalendarService.ts (2)
14-14: LGTM: Import addition is correct.The import for
refreshOAuthTokensis properly structured and follows the project's import patterns.
67-82: LGTM: OAuth token refresh refactor is well-implemented.The refactor correctly wraps the original Lark API token refresh call in the
refreshOAuthTokensutility while preserving all existing logic. The integration name and user ID are properly passed for credential sync functionality.packages/app-store/zoho-bigin/lib/CalendarService.ts (2)
18-18: LGTM: Import addition is correct.The import for
refreshOAuthTokensfollows the established pattern and is properly positioned.
85-94: LGTM: OAuth token refresh refactor is correctly implemented.The refactor properly wraps the axios POST request in the
refreshOAuthTokensutility while maintaining all existing token handling logic. Note that this implementation passescredentialIdas the third parameter, which is consistent with this method's signature that receives the credential ID rather than the full credential object.packages/app-store/hubspot/lib/CalendarService.ts (2)
26-26: LGTM: Import addition is correct.The import for
refreshOAuthTokensis properly structured and consistent with other integrations.
177-189: LGTM: OAuth token refresh refactor is well-implemented.The refactor correctly wraps the HubSpot SDK token refresh call in the
refreshOAuthTokensutility while preserving all existing logic. The integration name and user ID are properly passed for credential sync functionality.packages/app-store/googlecalendar/lib/CalendarService.ts (2)
21-22: LGTM: Import additions are correct.Both imports for
parseRefreshTokenResponseandrefreshOAuthTokensfollow the established pattern and are properly positioned.
86-93: LGTM: OAuth token refresh refactor is well-implemented.The refactor correctly wraps the Google OAuth token refresh call in the
refreshOAuthTokensutility and introduces theparseRefreshTokenResponsehelper for standardized token parsing. This enables both local and centralized credential handling while preserving all existing logic.Also applies to: 97-97
packages/app-store/zoomvideo/lib/VideoApiAdapter.ts (3)
12-13: LGTM! Clean import organization.The new utility imports for OAuth token refresh and response parsing are properly structured and follow consistent naming conventions.
79-94: Well-structured refactoring of token refresh logic.The delegation to
refreshOAuthTokensutility centralizes the OAuth flow while preserving the Zoom-specific request details through the callback pattern. The additional parameters (app identifier and user ID) properly support the new credential sharing feature.
104-104: Good standardization of token response parsing.The use of
parseRefreshTokenResponseutility centralizes response validation logic while maintaining type safety through the schema parameter.apps/web/pages/api/webhook/app-credential.ts (3)
17-29: Excellent security implementation.The webhook properly validates both the feature flag and webhook secret before processing requests. The configurable header name via environment variable adds good flexibility while maintaining security.
31-54: Comprehensive validation logic.The request validation covers all necessary checks: schema validation, user existence, app existence, and metadata validation. Error handling provides appropriate HTTP status codes and informative messages.
9-15: Well-defined request schema.The Zod schema is appropriately typed with clear documentation about the encryption requirement for the keys field.
packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts (3)
15-19: Smart conditional validation approach.The logic appropriately uses minimal validation when credential sharing is enabled with a sync endpoint, while maintaining strict validation otherwise. This provides good flexibility for different deployment scenarios.
21-23: Appropriate error handling.The error handling correctly throws on validation failure with a generic message that doesn't expose implementation details.
25-27: Clarify the purpose of default refresh_token assignment.Setting
"refresh_token"as a literal default value seems unusual. Could you clarify:
- Is this intentional for credential sharing scenarios where refresh tokens aren't needed?
- Would it be better to document this behavior or use a more descriptive placeholder?
- Could this mask legitimate cases where refresh tokens are unexpectedly missing?
packages/app-store/office365calendar/lib/CalendarService.ts (3)
20-21: Consistent utility imports.The OAuth utility imports follow the same pattern as other integrations and are properly structured.
246-261: Consistent token refresh refactoring.The refactoring properly delegates to the
refreshOAuthTokensutility while preserving Office365-specific OAuth request details. The callback approach maintains the existing logic structure.
263-263: Standardized response parsing.The use of
parseRefreshTokenResponseutility maintains consistency across OAuth integrations and supports the credential sharing feature.packages/app-store/salesforce/lib/CalendarService.ts (2)
4-4: Appropriate imports for Salesforce integration.The zod and parseRefreshTokenResponse imports are correctly added for the token validation functionality.
Also applies to: 20-20
39-47: Well-defined Salesforce token schema.The schema comprehensively covers all expected Salesforce OAuth token response fields with appropriate types.
| 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.
Fix the schema definition syntax.
The dynamic key syntax appears incorrect and may not work as intended:
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(),
+}).catchall(z.unknown());Alternatively, if you need to capture numeric expiry fields specifically, consider using z.record() or defining expected fields explicitly.
📝 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(), | |
| }).catchall(z.unknown()); |
🤖 Prompt for AI Agents
In packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts around lines 5
to 11, the schema definition uses invalid dynamic key syntax with
z.string().toString() which is incorrect. Replace the dynamic keys with a proper
z.record() to capture keys with number values for expiry fields, and use
z.record(z.unknown()) or explicitly define expected fields for other properties.
This will fix the schema validation and correctly allow additional properties.
| 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; |
There was a problem hiding this comment.
Address security and reliability concerns in credential sync.
The credential sync request has several critical issues:
- Missing error handling: The fetch request can throw exceptions that aren't caught
- No authentication: The endpoint request lacks authentication headers or signing
- No response validation: The response status and content aren't validated
- Missing timeout: No request timeout configured for external API calls
- Sensitive data exposure: User ID sent in URLSearchParams without encryption
Consider these improvements:
if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT && userId) {
+ try {
const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
method: "POST",
+ headers: {
+ 'Authorization': `Bearer ${process.env.CALCOM_WEBHOOK_SECRET}`,
+ 'Content-Type': 'application/x-www-form-urlencoded',
+ },
body: new URLSearchParams({
calcomUserId: userId.toString(),
appSlug,
}),
+ signal: AbortSignal.timeout(10000), // 10 second timeout
});
+
+ if (!response.ok) {
+ throw new Error(`Credential sync failed: ${response.status}`);
+ }
+
return response;
+ } catch (error) {
+ // Log error and fallback to original refresh function
+ console.error('Credential sync failed, falling back to original refresh:', error);
+ return await refreshFunction();
+ }
} else {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/app-store/_utils/oauth/refreshOAuthTokens.ts between lines 5 and 15,
the fetch request for credential sync lacks error handling, authentication,
response validation, timeout, and sends sensitive data unencrypted. To fix this,
wrap the fetch call in a try-catch block to handle exceptions, add appropriate
authentication headers or signing to the request, check the response status and
validate the response content before returning, implement a timeout mechanism
for the fetch request, and avoid sending sensitive user ID data in plain
URLSearchParams by encrypting or securely encoding it.
| }), | ||
| }); | ||
|
|
||
| if (response.statusText !== "OK") throw new HttpError({ statusCode: 400, message: response.statusText }); |
There was a problem hiding this comment.
Fix HTTP status validation.
The status text check is incorrect. HTTP responses typically have "OK" as status text, not "OK":
- if (response.statusText !== "OK") throw new HttpError({ statusCode: 400, message: response.statusText });
+ if (!response.ok) throw new HttpError({ statusCode: response.status, 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: response.status, message: response.statusText }); |
🤖 Prompt for AI Agents
In packages/app-store/salesforce/lib/CalendarService.ts at line 86, the code
incorrectly checks response.statusText against "OK" with wrong casing or format.
Fix this by validating the HTTP status code instead of statusText, for example,
check if response.status is 200 to confirm success, and throw the HttpError if
it is not 200.
|
This PR is being marked as stale due to inactivity. |
2 participants
Test 8
Summary by CodeRabbit
New Features
Refactor
Chores