Skip to content

OAuth credential sync and app integration enhancements#4

Open
zaibkhan wants to merge 1 commit into
oauth-security-basefrom
oauth-security-enhanced
Open

OAuth credential sync and app integration enhancements#4
zaibkhan wants to merge 1 commit into
oauth-security-basefrom
oauth-security-enhanced

Conversation

@zaibkhan

Copy link
Copy Markdown

No description provided.

…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>
@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 10, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Consolidate OAuth helpers, standardize refresh handling
What’s good: Centralizing encode/decode and credential creation under _utils/oauth reduces duplication and promotes consistent OAuth behavior across providers; introducing refreshOAuthTokens for HubSpot simplifies token refresh flows.
Review Status: ❌ Requires changes
Overall Priority: Critical
Review Update:
• Scope: Large PR detected (40 files)
• Coverage: Reviewed 30 highest-risk files across 3 batches
• Note: For full confidence, consider splitting this PR; current review covers top 30/40 files

This review covered the top 30 of 40 files (risk-ranked). For complete coverage and faster feedback, consider splitting into ~1 smaller PR(s).

Issues (Critical & High only)

Severity Issue Why it matters
Critical Correctness — Refresh token default corrupts credentials when omitted b... …/oauth/parseRefreshTokenResponse.ts
Overwriting a missing refresh_token with the literal string "refresh_token" corrupts stored credentials; when an endpoint omits refresh_token (e.g., Office365), subsequent refreshes will fail after this default is saved. Instead, do not set a placeholder here—let callers preserve the existing refresh_token when absent.
Critical Correctness — Missing prisma import and stale token used for jsforce.Co... …/lib/CalendarService.ts
Two issues: (1) prisma is used but not imported in this file, causing a runtime ReferenceError; (2) after refreshing tokens, the jsforce.Connection is constructed using credentialKey.instance_url and credentialKey.access_token (old values), not the freshly parsed tokens, so the connection may be initialized with an expired token. Import prisma from "@calcom/prisma" and use accessTokenParsed.data.instance_url and accessTokenParsed.data.access_token when creating the connection. Also prefer checking response.ok instead of response.statusText.
High Security — Webhook decrypt/parse lacks error handling and validation …/webhook/app-credential.ts
Decrypting and JSON parsing of untrusted payload can throw and currently leads to a 500; additionally, there's no validation of the decrypted structure. Wrap in try/catch and return 400 on failure, and ensure the encryption key is present.
High Correctness — Unsafe dereference of refreshed token fields can crash flow …/lib/CalendarService.ts
res?.data is not validated, but the next lines dereference token.access_token and token.expiry_date, which will throw if the refresh response is malformed or empty. This can crash event flows and leave credentials in a partially-updated state.
High Security — JSON.parse on untrusted OAuth state can crash request …/oauth/decodeOAuthState.ts
Untrusted query parameter is parsed without a try/catch; malformed JSON in state will throw and cause a 500 on OAuth callbacks. This is a reliability and security hardening issue.

Showing top 5 issues. Critical: 2, High: 3. See inline suggestions for more.

Key Feedback (click to expand)
  • Needs improvement: Ensure the new refreshOAuthTokens helper consistently handles provider-specific edge cases (e.g., refresh token rotation, error mapping), and align all providers to use it to avoid regression drift.
  • Testing: Add unit tests for refreshOAuthTokens covering: successful refresh, invalid/expired refresh token, provider-specific error payloads, and refresh token rotation (verifying the new token is returned and persisted). Add an integration test in HubSpot flow to confirm redirectUri consistency and that the refresh path is invoked on expiry.
  • Documentation: Document refreshOAuthTokens’ contract (inputs, expected return type, error behavior/retry policy) and note any provider-specific nuances. Briefly note the new _utils/oauth module structure to guide future contributors.
  • Compatibility: The path refactors appear internal (relative imports), but double-check any barrels/re-exports to prevent downstream import breakage in packages referencing the old utilities.
  • Performance: If refreshOAuthTokens performs retries, ensure exponential backoff + jitter and short-circuit on non-retryable errors to avoid unnecessary latency/pressure on provider APIs.
  • Security: Verify refreshOAuthTokens never logs raw tokens or sensitive fields and that any telemetry/redaction is applied uniformly; confirm state parameters still undergo strict validation after the refactor.
  • Open questions: Does refreshOAuthTokens sanitize logs and avoid writing access/refresh tokens to logs/metrics? If HubSpot rotates refresh tokens, where is the updated token persisted to avoid subsequent failures?

Confidence: 1/5 — Unsafe to merge (2 critical · 3 high · status: Requires changes · scope: top 30/40 files reviewed)

React with 👍 or 👎 if you found this review useful.

}

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 Critical: Overwriting a missing refresh_token with the literal string "refresh_token" corrupts stored credentials; when an endpoint omits refresh_token (e.g., Office365), subsequent refreshes will fail after this default is saved. Instead, do not set a placeholder here—let callers preserve the existing refresh_token when absent.

Suggested change
refreshTokenResponse.data.refresh_token = "refresh_token";
// preserve existing refresh_token at the callsite; do not override missing values here

const accessTokenParsed = parseRefreshTokenResponse(accessTokenJson, salesforceTokenSchema);

if (!accessTokenParsed.success) {
return Promise.reject(new Error("Invalid refreshed tokens were returned"));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 Critical: Two issues: (1) prisma is used but not imported in this file, causing a runtime ReferenceError; (2) after refreshing tokens, the jsforce.Connection is constructed using credentialKey.instance_url and credentialKey.access_token (old values), not the freshly parsed tokens, so the connection may be initialized with an expired token. Import prisma from "@calcom/prisma" and use accessTokenParsed.data.instance_url and accessTokenParsed.data.access_token when creating the connection. Also prefer checking response.ok instead of response.statusText.

}

// Decrypt the keys
const keys = JSON.parse(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High: Decrypting and JSON parsing of untrusted payload can throw and currently leads to a 500; additionally, there's no validation of the decrypted structure. Wrap in try/catch and return 400 on failure, and ensure the encryption key is present.

Suggested change
const keys = JSON.parse(
let keys;
try {
const decrypted = symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "");
keys = JSON.parse(decrypted);
if (!keys || typeof keys !== "object") return res.status(400).json({ message: "Invalid keys payload" });
} catch (e) {
return res.status(400).json({ message: "Invalid encrypted keys" });
}

"google-calendar",
credential.userId
);
const token = res?.data;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High: res?.data is not validated, but the next lines dereference token.access_token and token.expiry_date, which will throw if the refresh response is malformed or empty. This can crash event flows and leave credentials in a partially-updated state.

Suggested change
const token = res?.data;
const token = res?.data;
if (!token || !token.access_token || typeof token.expiry_date !== "number") {
throw new Error("Failed to refresh Google access token: missing access_token/expiry_date");
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants