Fix Phase 17 code review issues#244
Merged
Merged
Conversation
- Add CreditAccount::lockForUpdate() before idempotency check in CreditPackWebhookHandler to prevent double-grant on concurrent Stripe webhook retries (same pattern as SubscriptionWebhookHandler) - Exclude stripe/webhook route from CSRF protection so Stripe can deliver webhooks without a session token in production - Point Buy Credits CTA to /billing instead of dashboard Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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. Comment |
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
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.
Summary
Fixes three issues identified in Phase 17 code review.
P0 — Missing
CreditAccount::lockForUpdate()inCreditPackWebhookHandlerRoot cause: Concurrent Stripe webhook retries for the same checkout session can both pass the
alreadyGrantedForCheckoutSession()check before either transaction commits, resulting in double-grant.Fix: Acquired a row-level lock on the
credit_accountsrow insideDB::transaction()before the idempotency check — identical to the pattern inSubscriptionWebhookHandler::handleInvoicePaid().P0 — Stripe webhook not excluded from CSRF protection
Root cause: Laravel's CSRF middleware guards all POST routes. Stripe delivers webhooks without session cookies or CSRF tokens, so every production webhook would be rejected with HTTP 419.
Fix: Added
stripe/webhookto the CSRF exception list inbootstrap/app.phpusing Laravel 11'svalidateCsrfTokens(except: [...]). Added a test that sends a POST without cookies/session and asserts status ≠ 419.P2 — Buy Credits CTA linked to dashboard
Root cause:
/billing(Phase 18) didn't exist at the time, so the route fell back todashboard.Fix: Changed href to
/billing— the correct semantic destination that Phase 18 will implement. Removedroute('dashboard')hardcoding.Test plan
CashierWebhookRouteTest— new test verifies webhook accepts sessionless POST (no 419)composer testpasses (487 tests)composer lintpasses🤖 Generated with Claude Code
Summary by cubic
Fixes three Phase 17 review issues: prevents double credit grants on concurrent Stripe webhooks, allows Stripe webhooks through CSRF, and updates the Buy Credits CTA to point to
/billing.CreditPackWebhookHandlerby locking thecredit_accountsrow withlockForUpdate()before the idempotency check.stripe/webhookfrom CSRF inbootstrap/app.phpso Stripe requests aren’t rejected; added a test to assert the endpoint doesn’t return 419./billing(instead of dashboard) for the correct destination.Written for commit 11c6945. Summary will update on new commits.