fix: add missing error handling in OAuth token exchange and credential decryption#148
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves resilience and debuggability of OAuth flows and credential resolution by adding explicit handling for failed token exchanges and corrupted encrypted credential blobs.
Changes:
- Add
Response.okguards before parsing OAuth token exchange responses for Google and GitHub. - Wrap decrypted-credentials
JSON.parsein a try/catch to avoid crashing credential resolution on corruption, falling back to settings-only.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/web/src/lib/services/app-config-service.ts | Prevents crashes when decrypted credentials can’t be parsed/decrypted by returning settings-only. |
| apps/web/src/lib/apps/google-oauth.ts | Adds non-2xx guard for Google token exchange to avoid JSON parse errors on non-JSON bodies. |
| apps/web/src/lib/apps/github.ts | Adds non-2xx guard for GitHub token exchange to avoid JSON parse errors on non-JSON bodies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| throw new Error( | ||
| `Google token exchange failed: ${tokenRes.status} ${tokenRes.statusText}`, |
There was a problem hiding this comment.
The early !tokenRes.ok throw prevents reading Google’s JSON error payload (e.g., error_description), and makes the later tokenData.error handling ineffective for non-2xx responses. Consider on non-OK: read Content-Type and attempt tokenRes.json() when it’s JSON (falling back to tokenRes.text() otherwise) so the thrown error includes the provider’s error details while still avoiding Unexpected token '<' on HTML bodies.
| throw new Error( | |
| `Google token exchange failed: ${tokenRes.status} ${tokenRes.statusText}`, | |
| const contentType = tokenRes.headers.get("content-type") ?? ""; | |
| let errorDetail: string | undefined; | |
| try { | |
| if (contentType.includes("application/json")) { | |
| const errorBody = (await tokenRes.json()) as { | |
| error?: string; | |
| error_description?: string; | |
| }; | |
| errorDetail = | |
| errorBody.error_description || | |
| errorBody.error || | |
| undefined; | |
| } else { | |
| const text = await tokenRes.text(); | |
| errorDetail = text || undefined; | |
| } | |
| } catch { | |
| // Ignore body parsing errors; fall back to generic message below. | |
| } | |
| const baseMessage = `Google token exchange failed: ${tokenRes.status} ${tokenRes.statusText}`; | |
| throw new Error( | |
| errorDetail ? `${baseMessage} - ${errorDetail}` : baseMessage, |
| throw new Error( | ||
| `GitHub token exchange failed: ${tokenRes.status} ${tokenRes.statusText}`, |
There was a problem hiding this comment.
On non-OK responses, this throws using only status/statusText and skips reading the response body. GitHub’s token endpoint often returns a JSON body with useful error/error_description even when the status isn’t 2xx; consider including the parsed JSON (when available) or at least await tokenRes.text() in the thrown error to improve diagnostics.
| throw new Error( | |
| `GitHub token exchange failed: ${tokenRes.status} ${tokenRes.statusText}`, | |
| let errorDetail = ""; | |
| try { | |
| const bodyText = await tokenRes.text(); | |
| if (bodyText) { | |
| errorDetail = ` - Response body: ${bodyText}`; | |
| } | |
| } catch { | |
| // Ignore body read errors; fall back to status information only. | |
| } | |
| throw new Error( | |
| `GitHub token exchange failed: ${tokenRes.status} ${tokenRes.statusText}${errorDetail}`, |
| } catch { | ||
| // Corrupted or invalid encrypted credentials — return settings only |
There was a problem hiding this comment.
This catch swallows all decrypt/parse errors and returns settings without any logging, which can make key misconfiguration (e.g., missing/rotated SECRET_ENCRYPTION_KEY) or data corruption very hard to diagnose in production. Consider catching (err) and logging a warning/error with { accountId, provider } (without secrets) before returning settings.
| } catch { | |
| // Corrupted or invalid encrypted credentials — return settings only | |
| } catch (err) { | |
| // Corrupted or invalid encrypted credentials — log and return settings only | |
| console.error("Failed to decrypt app config credentials", { | |
| accountId, | |
| provider, | |
| error: err instanceof Error ? err.message : err, | |
| }); |
…l decryption Check response.ok before parsing token exchange responses, and wrap JSON.parse in credential decryption with try-catch for corrupted data.
24f41c0 to
b61eb8f
Compare
Summary
Was debugging a self-hosted deployment where Google OAuth kept failing with cryptic JSON parse errors. Traced it to the token exchange endpoint returning a 503 during a brief outage —
tokenRes.json()was called without checkingtokenRes.okfirst, so instead of a clear error, users got an unhelpfulSyntaxError: Unexpected token '<'from trying to parse an HTML error page as JSON.Noticed the same pattern in
github.ts— the token exchange fetch also skips the.okcheck. Interestingly, both files already do it correctly for the user info fetch right below (if (userRes.ok)ingoogle-oauth.ts:87andgithub.ts:116), so this just brings the token exchange in line with the existing pattern.Also found that
getAppConfigCredentials()inapp-config-service.tscallsJSON.parse()on decrypted credentials without a try-catch. If the encrypted blob gets corrupted (wrong key rotation, truncated write, etc.), this crashes the whole credential resolution flow instead of falling back gracefully. Other files likeoauth-state.tsalready wrapJSON.parsein try-catch for exactly this reason.Changes
google-oauth.ts: Addif (!tokenRes.ok)guard before parsing token exchange responsegithub.ts: Same — addif (!tokenRes.ok)guard before parsing token exchange responseapp-config-service.ts: WrapJSON.parseof decrypted credentials in try-catch, return settings-only on corruptionTest plan