fix: handle Stripe API errors in OAuth account connection flow#29247
Closed
calebcgates wants to merge 3 commits intocalcom:mainfrom
Closed
fix: handle Stripe API errors in OAuth account connection flow#29247calebcgates wants to merge 3 commits intocalcom:mainfrom
calebcgates wants to merge 3 commits intocalcom:mainfrom
Conversation
The saveStripeAccount method makes two Stripe API calls without try-catch blocks. If the OAuth authorization code is invalid/expired or the Stripe account has been deleted, the unhandled StripeError propagates as a 500 Internal Server Error instead of returning a meaningful error to the user. This adds targeted error handling: - oauth.token(): catches StripeInvalidGrantError for expired/invalid authorization codes and returns 400 with actionable message - accounts.retrieve(): catches StripeInvalidRequestError for missing/deleted accounts and returns 400 with clear explanation - All other Stripe errors surface as 500 with a generic message rather than leaking raw Stripe error details
Contributor
|
Welcome to Cal.diy, @calebcgates! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
…ount Covers all error paths introduced in the Stripe Connect OAuth callback: - StripeInvalidGrantError → BadRequestException (expired/invalid code) - StripeInvalidRequestError → BadRequestException (deleted account) - Unexpected errors → InternalServerErrorException - Happy path with and without stripe_user_id - Credential cleanup before creating new ones
Contributor
📝 WalkthroughWalkthroughA test suite for the 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/v2/src/modules/stripe/stripe.service.spec.ts (1)
135-176: ⚡ Quick winAvoid calling
saveStripeAccounttwice in each rejection test.These tests currently execute the method twice to assert type and message. Use a single rejection instance per test to reduce brittleness and accidental side effects in future changes.
Suggested pattern
- await expect(service.saveStripeAccount(mockState, "expired_code", 1)).rejects.toThrow(BadRequestException); - await expect(service.saveStripeAccount(mockState, "expired_code", 1)).rejects.toThrow( - "Invalid or expired Stripe authorization code" - ); + const promise = service.saveStripeAccount(mockState, "expired_code", 1); + await expect(promise).rejects.toThrow(BadRequestException); + await expect(promise).rejects.toThrow("Invalid or expired Stripe authorization code");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/v2/src/modules/stripe/stripe.service.spec.ts` around lines 135 - 176, Tests call service.saveStripeAccount twice per rejection case which can cause side effects; capture the single rejected promise returned by saveStripeAccount (e.g., const promise = service.saveStripeAccount(...)) and assert both the error type and message against that promise instead of invoking saveStripeAccount twice. Apply this change to each failing test that currently calls saveStripeAccount twice (those using mockOAuthToken and mockAccountsRetrieve setups) so assertions check the same rejection instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/v2/src/modules/stripe/stripe.service.ts`:
- Around line 105-110: Add an early guard that validates the OAuth `code` before
calling stripeInstance.oauth.token: check the `code` input (used where
code?.toString() is passed to stripeInstance.oauth.token) and if it's
null/undefined/empty, return or throw a client error (HTTP 400) with a clear
message instead of proceeding to call stripeInstance.oauth.token; only call
stripeInstance.oauth.token with the confirmed non-empty code (e.g., use
code.toString() after the guard).
---
Nitpick comments:
In `@apps/api/v2/src/modules/stripe/stripe.service.spec.ts`:
- Around line 135-176: Tests call service.saveStripeAccount twice per rejection
case which can cause side effects; capture the single rejected promise returned
by saveStripeAccount (e.g., const promise = service.saveStripeAccount(...)) and
assert both the error type and message against that promise instead of invoking
saveStripeAccount twice. Apply this change to each failing test that currently
calls saveStripeAccount twice (those using mockOAuthToken and
mockAccountsRetrieve setups) so assertions check the same rejection instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e24084e-d4a2-440d-9c14-25e52dc73889
📒 Files selected for processing (2)
apps/api/v2/src/modules/stripe/stripe.service.spec.tsapps/api/v2/src/modules/stripe/stripe.service.ts
Comment on lines
+105
to
+110
| let response; | ||
| try { | ||
| response = await stripeInstance.oauth.token({ | ||
| grant_type: "authorization_code", | ||
| code: code?.toString(), | ||
| }); |
Contributor
There was a problem hiding this comment.
Add an early guard for missing OAuth code before calling Stripe.
Line [109] allows an empty/undefined code to reach oauth.token, which then falls into the 500 path. This should fail fast as a client error (400) before the external call.
Suggested fix
async saveStripeAccount(state: OAuthCallbackState, code: string, userId: number): Promise<{ url: string }> {
if (!userId) {
throw new UnauthorizedException("Invalid Access token.");
}
+ if (!code?.trim()) {
+ throw new BadRequestException("Missing Stripe authorization code.");
+ }
let response;
try {
response = await stripeInstance.oauth.token({
grant_type: "authorization_code",
- code: code?.toString(),
+ code,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/v2/src/modules/stripe/stripe.service.ts` around lines 105 - 110, Add
an early guard that validates the OAuth `code` before calling
stripeInstance.oauth.token: check the `code` input (used where code?.toString()
is passed to stripeInstance.oauth.token) and if it's null/undefined/empty,
return or throw a client error (HTTP 400) with a clear message instead of
proceeding to call stripeInstance.oauth.token; only call
stripeInstance.oauth.token with the confirmed non-empty code (e.g., use
code.toString() after the guard).
Member
|
please reopen with video demo attached |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds error handling around two unprotected Stripe API calls in saveStripeAccount() in the Stripe Connect OAuth callback flow. Without try-catch, expired authorization codes or deleted Stripe accounts crash the request with a raw 500 error instead of returning an actionable 400.
Visual Demo (For contributors especially)
N/A — This is a backend-only error handling fix with no UI changes.
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
- Expired/invalid OAuth code → 400 BadRequestException with message: "Invalid or expired Stripe authorization code"
- Deleted/deactivated Stripe account → 400 BadRequestException with message: "Could not retrieve Stripe account"
- Unexpected Stripe errors → 500 InternalServerErrorException without leaking raw Stripe error details
References: