OAuth credential sync and app integration enhancements

[akshayutture-augment]

Test 8

---

Replicated from ai-code-review-evaluation/cal.com-greptile#8

[akshayutture-augment]

@claude review this PR

[akshayutture-augment]

@claude review this PR

[claude]

Claude finished @akshayutture-augment's task —— View job

---

Code Review Complete

I've reviewed the OAuth credential sync and app integration enhancements PR. Here's my comprehensive feedback:

---

🔐 Security Issues

Critical: Webhook Authentication Vulnerability

File: apps/web/pages/api/webhook/app-credential.ts:24-29

The webhook secret validation has a significant vulnerability:

if (
  req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] !==
  process.env.CALCOM_WEBHOOK_SECRET
) {
  return res.status(403).json({ message: "Invalid webhook secret" });
}

Issues:

1. Missing HTTP Method Validation: The endpoint doesn't validate that the request is a POST. GET requests could potentially trigger this endpoint.
2. No Request Body Validation Before Auth: The Zod schema validation (appCredentialWebhookRequestBodySchema.parse(req.body)) happens AFTER the authentication check, but the request body is never validated for existence before access.
3. Header Name Security: Using a custom, configurable header name for secrets is less secure than standard approaches (like X-Webhook-Signature with HMAC).

Recommendations:

• Add HTTP method check: if (req.method !== 'POST') return res.status(405)
• Consider HMAC-based signature validation instead of plain secret comparison
• Move all request validation before any business logic

Critical: Missing Input Validation

File: apps/web/pages/api/webhook/app-credential.ts:57-59

The decryption happens without proper error handling:

const keys = JSON.parse(
  symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "")
);

Issues:

1. If CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY is not set (empty string fallback), decryption will fail silently or produce incorrect results
2. No try-catch around JSON.parse - malformed JSON after decryption will crash the endpoint
3. No validation of the decrypted keys structure

Recommendations:

if (!process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY) {
  return res.status(500).json({ message: "Server configuration error" });
}

let keys;
try {
  const decrypted = symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY);
  keys = JSON.parse(decrypted);
} catch (error) {
  return res.status(400).json({ message: "Invalid encrypted payload" });
}

Medium: Timing Attack Vulnerability

File: apps/web/pages/api/webhook/app-credential.ts:24-29

The direct string comparison for webhook secret is vulnerable to timing attacks.

Recommendation: Use a constant-time comparison function like crypto.timingSafeEqual().

---

🐛 Logic Issues

Bug: Incorrect Error Handling in parseRefreshTokenResponse

File: packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts:25-27

if (!refreshTokenResponse.data.refresh_token) {
  refreshTokenResponse.data.refresh_token = "refresh_token";
}

Issue: Setting a placeholder string "refresh_token" when the refresh token is missing is dangerous. This will cause authentication failures later when this placeholder is used. The code should either:

1. Use the existing refresh token from credentials
2. Throw an error if refresh token is missing
3. Document why this is acceptable for credential sync scenarios

Bug: Missing Import in Salesforce CalendarService

File: packages/app-store/salesforce/lib/CalendarService.ts:96

await prisma.credential.update({

The file uses prisma but there's no import statement visible. Need to verify this import exists.

Bug: Promise Not Returned

File: packages/app-store/salesforce/lib/CalendarService.ts:299-302

async deleteEvent(uid: string) {
  const deletedEvent = await this.salesforceDeleteEvent(uid);
  if (deletedEvent.success) {
    Promise.resolve();  // ❌ Not returned!
  } else {
    Promise.reject({ calError: "Something went wrong when deleting the event in Salesforce" });
  }
}

Issue: Promise.resolve() and Promise.reject() are not returned, so they have no effect.

Fix:

async deleteEvent(uid: string) {
  const deletedEvent = await this.salesforceDeleteEvent(uid);
  if (deletedEvent.success) {
    return;
  } else {
    throw new Error("Something went wrong when deleting the event in Salesforce");
  }
}

---

🔄 Consistency Issues

Inconsistent refreshOAuthTokens Implementation

The refreshOAuthTokens function is called differently across services:

Google Calendar (packages/app-store/googlecalendar/lib/CalendarService.ts:86-93): ✅ Correctly awaits the response

const res = await refreshOAuthTokens(
  async () => {
    const fetchTokens = await myGoogleAuth.refreshToken(googleCredentials.refresh_token);
    return fetchTokens.res;
  },
  "google-calendar",
  credential.userId
);

Hubspot (packages/app-store/hubspot/lib/CalendarService.ts:177-189): ⚠️ Casts result without validation

const hubspotRefreshToken: HubspotToken = await refreshOAuthTokens(
  async () => await hubspotClient.oauth.tokensApi.createToken(...),
  "hubspot",
  credential.userId
);

Issue: Hubspot assumes the return type is HubspotToken, but when APP_CREDENTIAL_SHARING_ENABLED is true, the response might have a different structure. This could cause runtime type errors.

Recommendation: Add type guards or use parseRefreshTokenResponse consistently.

---

🎯 Design Concerns

Unclear Feature Flag Logic

File: packages/app-store/_utils/oauth/refreshOAuthTokens.ts:3-20

The logic branches based on APP_CREDENTIAL_SHARING_ENABLED:

• When enabled: Makes a POST to CALCOM_CREDENTIAL_SYNC_ENDPOINT
• When disabled: Executes the provided refreshFunction

Issues:

1. No documentation explaining what the endpoint should return
2. The endpoint payload uses URLSearchParams but there's no documentation about expected response format
3. Silent failure: If the endpoint is unreachable, the function just returns the error response without logging

Recommendations:

• Add JSDoc comments explaining the credential sync flow
• Add error logging for failed credential sync attempts
• Document the expected API contract for CALCOM_CREDENTIAL_SYNC_ENDPOINT

Weak Schema in parseRefreshTokenResponse

File: packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts:5-11

const minimumTokenResponseSchema = z.object({
  access_token: z.string(),
  [z.string().toString()]: z.number(),
  [z.string().optional().toString()]: z.unknown().optional(),
});

Issue: This schema is too permissive. Using dynamic keys like [z.string().toString()] doesn't actually validate anything at runtime because Zod doesn't support computed property names in object schemas this way. This code will not work as intended.

Expected behavior: The schema should be defined with explicit fields or use .passthrough() to allow additional properties.

---

📝 Code Quality Issues

Missing Error Handling

Multiple service files make network requests without comprehensive error handling:

Office365 Calendar (packages/app-store/office365calendar/lib/CalendarService.ts:246-261):

const response = await refreshOAuthTokens(
  async () => await fetch("https://login.microsoftonline.com/common/oauth2/v2.0/token", {
    // ... fetch options
  }),
  "office365-calendar",
  credential.userId
);
const responseJson = await handleErrorsJson(response);

Issue: No validation that response is actually a Response object. If refreshOAuthTokens returns something else when syncing is enabled, await handleErrorsJson(response) could fail.

Inconsistent Import Paths

The OAuth utility files were moved to a new oauth/ subdirectory:

• ✅ packages/app-store/_utils/oauth/createOAuthAppCredential.ts
• ✅ packages/app-store/_utils/oauth/decodeOAuthState.ts
• ✅ packages/app-store/_utils/oauth/encodeOAuthState.ts

All imports were updated, but this is a good opportunity to:

• Add an index.ts file to the oauth/ directory for cleaner imports
• Document the purpose of the oauth/ utilities module

---

✅ Positive Aspects

1. Good separation of concerns: The credential sync logic is abstracted into utility functions
2. Consistent pattern: The same refresh pattern is applied across multiple integrations (Google, Office365, Salesforce, HubSpot, Zoom, Webex, Lark)
3. Proper environment variable management: New variables are documented in .env.example
4. Turbo.json updates: Correctly added the new environment variables to the global environment list

---

🔍 Testing Recommendations

1. Unit tests needed for:

   • parseRefreshTokenResponse with various input scenarios
   • refreshOAuthTokens with and without credential sync enabled
   • Webhook endpoint with various invalid inputs

2. Integration tests needed for:

   • OAuth token refresh flow with credential sync enabled
   • Webhook endpoint with valid/invalid secrets
   • Encrypted credential payload handling

3. Security tests:

   • Timing attack resistance for webhook secret comparison
   • Payload tampering detection
   • Encryption/decryption edge cases

---

📋 Summary

Must Fix Before Merge:

• ❌ Webhook authentication vulnerabilities (method check, error handling)
• ❌ Missing error handling in decryption
• ❌ Promise not returned in Salesforce deleteEvent
• ❌ Incorrect placeholder refresh token in parseRefreshTokenResponse
• ❌ Broken Zod schema with computed keys

Should Fix:

• ⚠️ Add timing-safe comparison for webhook secrets
• ⚠️ Consistent error handling across all service integrations
• ⚠️ Type safety for refreshOAuthTokens return values
• ⚠️ Documentation for credential sync endpoint contract

Nice to Have:

• 📚 Add JSDoc comments to utility functions
• 📚 Create oauth/index.ts for cleaner imports
• ✅ Add comprehensive tests

---

[github-actions]

This PR is being marked as stale due to inactivity.