OAuth credential sync and app integration enhancements#7
OAuth credential sync and app integration enhancements#7ShashankFC wants to merge 1 commit intooauth-security-basefrom
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>
|
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: |
|
Claude finished @ShashankFC's task in 2m 35s —— View job Code Review CompleteTasks
OverviewThis PR implements OAuth credential synchronization between Cal.com and self-hosted platforms. While the implementation adds useful functionality for self-hosters, there are critical security vulnerabilities and several code quality issues that must be addressed before merging. Critical Security Issues1. Missing HMAC Validation in Webhook Endpoint (Critical)File: The webhook endpoint uses simple string comparison for authentication: if (
req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] !==
process.env.CALCOM_WEBHOOK_SECRET
) {Issues:
Fix timing attack vulnerability → Add HMAC signature validation → 2. Missing Encryption Key Validation (Critical)File: symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "")Issue: If Recommendation: Add validation at application startup: if (APP_CREDENTIAL_SHARING_ENABLED && !process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY) {
throw new Error("CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY must be set when credential sharing is enabled");
}Fix encryption key validation → 3. Missing HTTP Method Validation (High)File: The webhook endpoint doesn't validate the HTTP method. It should only accept POST requests. 4. Unvalidated External Endpoint Calls (High)File: const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
method: "POST",
body: new URLSearchParams({
calcomUserId: userId.toString(),
appSlug,
}),
});Issues:
Recommendation: const controller = new AbortController();
const timeout = setTimeout(() => controller.abort(), 10000);
try {
const response = await fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({ calcomUserId: userId.toString(), appSlug }),
signal: controller.signal,
});
if (!response.ok) {
throw new Error(`Credential sync failed: ${response.status}`);
}
return response;
} finally {
clearTimeout(timeout);
}Fix external endpoint call issues → High Priority Issues5. Inconsistent Error Handling in parseRefreshTokenResponse (High)File: if (!refreshTokenResponse.data.refresh_token) {
refreshTokenResponse.data.refresh_token = "refresh_token";
}Issue: Setting a literal string "refresh_token" when none is provided is dangerous. This will cause token refresh to fail silently later. Recommendation: Either throw an error or use the existing refresh token from the credential. 6. Missing Import in Salesforce CalendarService (High)File: The code uses await prisma.credential.update({Missing import: 7. Invalid Zod Schema in parseRefreshTokenResponse (High)File: const minimumTokenResponseSchema = z.object({
access_token: z.string(),
[z.string().toString()]: z.number(),
[z.string().optional().toString()]: z.unknown().optional(),
});Issue: This is not valid Zod syntax. You cannot use dynamic keys with Correct approach: const minimumTokenResponseSchema = z.object({
access_token: z.string(),
}).catchall(z.unknown());Or use Medium Priority Issues8. PR Title Convention (Medium)The PR title "OAuth credential sync and app integration enhancements" doesn't follow Conventional Commits. It should start with a type prefix like Recommendation: Change to: 9. Incomplete createOAuthAppCredential Function (Medium)File: await throwIfNotHaveAdminAccessToTeam({ teamId: state?.teamId ?? null, userId });This call happens AFTER the team check on line 30-46, and it's checking when Likely intended logic:
Remove line 59 or clarify the intended logic. Fix inverted logic in createOAuthAppCredential → 10. Missing Error Handling (Medium)The 11. Documentation (Medium)The new environment variables in
Consider adding a Low Priority / Style Issues12. Inconsistent Return StatementsFiles like 13. Type SafetyConsider using stricter types instead of 14. Missing Rate LimitingThe webhook endpoint has no rate limiting, making it vulnerable to DoS attacks. Testing Recommendations
SummaryMust Fix Before Merge:
Should Fix:
Nice to Have:
|
|
This PR is being marked as stale due to inactivity. |
2 participants
Test 8nn---nReplicated from ai-code-review-evaluation/cal.com-greptile#8