-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,93 @@ | ||||||||||||
| import type { NextApiRequest, NextApiResponse } from "next"; | ||||||||||||
| import z from "zod"; | ||||||||||||
|
|
||||||||||||
| import { appStoreMetadata } from "@calcom/app-store/appStoreMetaData"; | ||||||||||||
| import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants"; | ||||||||||||
| import { symmetricDecrypt } from "@calcom/lib/crypto"; | ||||||||||||
| import prisma from "@calcom/prisma"; | ||||||||||||
|
|
||||||||||||
| const appCredentialWebhookRequestBodySchema = z.object({ | ||||||||||||
| // UserId of the cal.com user | ||||||||||||
| userId: z.number().int(), | ||||||||||||
| appSlug: z.string(), | ||||||||||||
| // Keys should be AES256 encrypted with the CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY | ||||||||||||
| 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 commentThe 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
Suggested change
|
||||||||||||
| // 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 | ||||||||||||
| ) { | ||||||||||||
|
Comment on lines
+24
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Direct string comparison is vulnerable to timing attacks - use |
||||||||||||
| return res.status(403).json({ message: "Invalid webhook secret" }); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body); | ||||||||||||
|
|
||||||||||||
| // Check that the user exists | ||||||||||||
| const user = await prisma.user.findUnique({ where: { id: reqBody.userId } }); | ||||||||||||
|
|
||||||||||||
| if (!user) { | ||||||||||||
| return res.status(404).json({ message: "User not found" }); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const app = await prisma.app.findUnique({ | ||||||||||||
| where: { slug: reqBody.appSlug }, | ||||||||||||
| select: { slug: true }, | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| if (!app) { | ||||||||||||
| return res.status(404).json({ message: "App not found" }); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // Search for the app's slug and type | ||||||||||||
| const appMetadata = appStoreMetadata[app.slug as keyof typeof appStoreMetadata]; | ||||||||||||
|
|
||||||||||||
| if (!appMetadata) { | ||||||||||||
| return res.status(404).json({ message: "App not found. Ensure that you have the correct app slug" }); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // 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 commentThe 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 |
||||||||||||
| ); | ||||||||||||
|
Comment on lines
+57
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Missing error handling for |
||||||||||||
|
|
||||||||||||
| // Can't use prisma upsert as we don't know the id of the credential | ||||||||||||
| const appCredential = await prisma.credential.findFirst({ | ||||||||||||
| where: { | ||||||||||||
| userId: reqBody.userId, | ||||||||||||
| appId: appMetadata.slug, | ||||||||||||
| }, | ||||||||||||
| select: { | ||||||||||||
| id: true, | ||||||||||||
| }, | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| if (appCredential) { | ||||||||||||
| await prisma.credential.update({ | ||||||||||||
| where: { | ||||||||||||
| id: appCredential.id, | ||||||||||||
| }, | ||||||||||||
| data: { | ||||||||||||
| key: keys, | ||||||||||||
| }, | ||||||||||||
| }); | ||||||||||||
| return res.status(200).json({ message: `Credentials updated for userId: ${reqBody.userId}` }); | ||||||||||||
| } else { | ||||||||||||
| await prisma.credential.create({ | ||||||||||||
| data: { | ||||||||||||
| key: keys, | ||||||||||||
| userId: reqBody.userId, | ||||||||||||
| appId: appMetadata.slug, | ||||||||||||
| type: appMetadata.type, | ||||||||||||
| }, | ||||||||||||
| }); | ||||||||||||
| return res.status(200).json({ message: `Credentials created for userId: ${reqBody.userId}` }); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,32 @@ | ||||||||||||
| import { z } from "zod"; | ||||||||||||
|
|
||||||||||||
| import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants"; | ||||||||||||
|
|
||||||||||||
| 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(), | ||||||||||||
|
Comment on lines
+8
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. syntax: Invalid Zod schema syntax. Computed property keys like
Suggested change
|
||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| const parseRefreshTokenResponse = (response: any, schema: z.ZodTypeAny) => { | ||||||||||||
| let refreshTokenResponse; | ||||||||||||
| if (APP_CREDENTIAL_SHARING_ENABLED && process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT) { | ||||||||||||
| refreshTokenResponse = minimumTokenResponseSchema.safeParse(response); | ||||||||||||
| } else { | ||||||||||||
| refreshTokenResponse = schema.safeParse(response); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (!refreshTokenResponse.success) { | ||||||||||||
| throw new Error("Invalid refreshed tokens were returned"); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (!refreshTokenResponse.data.refresh_token) { | ||||||||||||
| refreshTokenResponse.data.refresh_token = "refresh_token"; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| return refreshTokenResponse; | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| export default parseRefreshTokenResponse; | ||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import { APP_CREDENTIAL_SHARING_ENABLED } from "@calcom/lib/constants"; | ||
|
|
||
| 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, | ||
| }), | ||
|
Comment on lines
+10
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider adding Content-Type header when sending URLSearchParams to ensure proper parsing by the endpoint |
||
| }); | ||
| return response; | ||
| } else { | ||
| const response = await refreshFunction(); | ||
| return response; | ||
| } | ||
| }; | ||
|
|
||
| export default refreshOAuthTokens; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ import type { | |
| import type { CredentialPayload } from "@calcom/types/Credential"; | ||
|
|
||
| import getAppKeysFromSlug from "../../_utils/getAppKeysFromSlug"; | ||
| import refreshOAuthTokens from "../../_utils/oauth/refreshOAuthTokens"; | ||
| import type { HubspotToken } from "../api/callback"; | ||
|
|
||
| const hubspotClient = new hubspot.Client(); | ||
|
|
@@ -173,13 +174,18 @@ export default class HubspotCalendarService implements Calendar { | |
|
|
||
| const refreshAccessToken = async (refreshToken: string) => { | ||
| try { | ||
| const hubspotRefreshToken: HubspotToken = await hubspotClient.oauth.tokensApi.createToken( | ||
| "refresh_token", | ||
| undefined, | ||
| WEBAPP_URL + "/api/integrations/hubspot/callback", | ||
| this.client_id, | ||
| this.client_secret, | ||
| refreshToken | ||
| 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 | ||
| ); | ||
|
Comment on lines
+177
to
189
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: The |
||
|
|
||
| // set expiry date as offset from current time. | ||
|
|
||
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 beopenssl rand -base64 32or clarify the requirement.