-
Notifications
You must be signed in to change notification settings - Fork 0
OAuth credential sync and app integration enhancements #4
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix minimum token schema for credential sync
When credential sharing is enabled the new helper runs minimumTokenResponseSchema.safeParse(response) (lines 6‑10). The computed property names [z.string().toString()] and [z.string().optional().toString()] evaluate to the literal strings "[object ZodString]", so the schema now requires the response payload to contain two impossible keys with those names. As a result, every valid token response fails validation and parseRefreshTokenResponse always throws “Invalid refreshed tokens” whenever CALCOM_CREDENTIAL_SYNC_ENDPOINT is configured, making the newly added credential‑sync flow unusable. The schema needs to accept arbitrary numeric/extra properties (e.g. by using .passthrough() or .catchall) instead of hard‑coded bogus keys.
Useful? React with 👍 / 👎.
| const token = res?.data; | ||
| googleCredentials.access_token = token.access_token; | ||
| googleCredentials.expiry_date = token.expiry_date; | ||
| const key = googleCredentialSchema.parse(googleCredentials); | ||
| const key = parseRefreshTokenResponse(googleCredentials, googleCredentialSchema); | ||
| 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.
Save parsed Google credentials instead of SafeParse wrapper
In the Google refresh flow (lines 94‑98) the return value of parseRefreshTokenResponse is written directly to Prisma as the credential key. That helper returns the whole SafeParseReturn object ({ success, data, error }), not just the parsed token. After the first refresh the DB stores this wrapper object, so the next time googleCredentialSchema.parse(credential.key) runs it receives an object with success/data fields instead of the expected OAuth token shape and throws, breaking Google Calendar for that user. Persist only the parsed token (parseRefreshTokenResponse(...).data) rather than the SafeParse metadata.
Useful? React with 👍 / 👎.
| 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", |
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.
refreshOAuthTokens must return token data when sync is enabled
When APP_CREDENTIAL_SHARING_ENABLED is true, refreshOAuthTokens returns the raw fetch Response without parsing it (lines 5‑9). Callers such as HubspotCalendarService (lines 175‑203) immediately treat the return value as a HubspotToken and access .expiryDate, .accessToken, etc. As soon as credential sharing is enabled, hubspotRefreshToken becomes a Response object and these property accesses throw, preventing refresh for every synced credential (Google has the same issue with res?.data). The helper needs to parse the JSON (or otherwise transform it to match the original token shape) before returning so that callers keep receiving the expected token object.
Useful? React with 👍 / 👎.
Test 8
Replicated from ai-code-review-evaluation/cal.com-cursor#8