Robustness: guard process_checkout() against missing cart and invalid gateway#1377
Robustness: guard process_checkout() against missing cart and invalid gateway#1377vuckro wants to merge 3 commits into
Conversation
…lid gateway
Two reachable fatals in Checkout::process_checkout() on the synchronous
finalize path:
- `$payment->get_meta('wu_original_cart')` returns its `false` default for any
payment not created by the normal checkout flow (webhook/admin/register-API
payments), after which `$this->order->set_membership(...)` calls a method on a
bool. Sibling consumers (Discount_Code_Manager, Base_Stripe_Gateway) already
validate this meta with `is_a(..., Cart::class)`; process_checkout did not.
Now bails with a clean WP_Error.
- The `elseif ($gateway->get_id() === 'free')` branch dereferenced `$gateway`
which is `false` when an unknown `gateway` request value is supplied; guarded
so it falls through to the existing "gateway not registered" error.
Both are robustness/DoS fixes; no behavior change for valid checkouts.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughprocess_checkout() now assigns payment meta 'wu_original_cart' to the order and aborts with WP_Error('no-cart') if it's not a Cart instance. The free-gateway branch now verifies $gateway is truthy before calling get_id(). ChangesCheckout Process Validation
Possibly Related PRs
Suggested Labels
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
inc/checkout/class-checkout.php (1)
3050-3056: Negatedinstanceofcheck is already correct in PHP 7.4; no precedence bug
In PHP 7.4,! $this->order instanceof \WP_Ultimo\Checkout\Cartis parsed as!($this->order instanceof \WP_Ultimo\Checkout\Cart), so the guard should fire and prevent the later$this->order->set_membership($membership)call when$this->orderisn’t a Cart. Adding parentheses is optional for readability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@inc/checkout/class-checkout.php` around lines 3050 - 3056, The instanceof guard is functionally correct in PHP 7.4 but ambiguous to readers; make the intent explicit by adding parentheses around the instanceof check so the guard reads as if (! ($this->order instanceof \WP_Ultimo\Checkout\Cart)) to ensure the early return fires before calling $this->order->set_membership($membership) and avoid any accidental dereference of a non-Cart $this->order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@inc/checkout/class-checkout.php`:
- Around line 3050-3056: The instanceof guard is functionally correct in PHP 7.4
but ambiguous to readers; make the intent explicit by adding parentheses around
the instanceof check so the guard reads as if (! ($this->order instanceof
\WP_Ultimo\Checkout\Cart)) to ensure the early return fires before calling
$this->order->set_membership($membership) and avoid any accidental dereference
of a non-Cart $this->order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ff819ef-f9be-4b3d-814b-5bbb8b8a32f4
📒 Files selected for processing (1)
inc/checkout/class-checkout.php
…abbit nit) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary
Two reachable fatals on the synchronous final-checkout path in
Checkout::process_checkout():$payment->get_meta('wu_original_cart')returns itsfalsedefault for anypayment not created by the normal checkout flow (webhook / admin /
register-API payments). The next line called
$this->order->set_membership()on that boolean →
Error: Call to a member function … on bool. Siblingconsumers (
Discount_Code_Manager,Base_Stripe_Gateway) already validatethis meta with
is_a(..., Cart::class); this path did not.elseif ($gateway->get_id() === 'free')branch dereferenced$gateway,which is
falsewhen an unknowngatewayrequest value is supplied.Changes
WP_Errorwhen the stored cart isn't aCart.falsegateway falls through to theexisting "gateway not registered" error.
Both are robustness/DoS fixes; valid checkouts are unaffected.
Summary by CodeRabbit