Refuse a callback whose transaction is not successful (D5)

[jules-paystack]

Stacked on #73 (base is fix/money-path-verification-gate) — review that one first; the diff here is one commit.

What was wrong

/paystack/payment/callback is an anonymous route. It loaded the order named by the caller's reference and dispatched paystack_payment_verify_after without ever reading the verify response's own data.status. One unauthenticated GET with a guessed increment ID advanced any order to Processing with nothing paid.

Measured on the dev-repro harness (Magento 2.4.9 / PHP 8.5 / CSP on), against real Paystack test-mode transactions and the real route with no session and no cookies:

CONTROL (this change reverted)   order 081799607  paystack status: abandoned
  → 302 …/checkout/onepage/success/     after: processing/processing     exploit reproduced

ARM A (with the gate)            order 081799609  paystack status: abandoned
  → 302 …/checkout/onepage/failure/     after: new/pending              refused

ARM B (with the gate)            order 081799610  paystack status: success, amount 500500
  → 302 …/checkout/onepage/success/     after: processing/processing     still settles

The control matters: arm A's first green was for the wrong reason (a bad amount meant no Paystack transaction existed, so it failed on not-found rather than on status), and only the reverted-code comparison caught that.

Scope, stated precisely

A transaction that is not successful can no longer advance an order. This is not full closure of the callback surface: nothing here compares the amount or the currency, and nothing binds a reference to one order, so a genuine minimum-amount charge carrying a victim's increment ID still passes. That residual is the amount/currency window and the payment registration in the next items — so please don't write a release note that says the callback is secured.

Also in the commit, all from its diff review

• catch (Exception $e) was unqualified in a namespaced file with no use, so it resolved to a non-existent class and caught nothing — every non-ApiException escaped as a 500. It is \Throwable now.
• Neither catch reflects the exception message to the caller. Those messages are built from curl_error() and Paystack's raw response, so they leaked internal detail and let an anonymous caller tell "no such reference" apart from "reference exists".
• In-flight statuses (pending, ongoing, queued — bank transfer and USSD sit there at callback time) get their own message telling the customer not to pay again, instead of the failed-payment retry wording.
• A throwable raised after the dispatch no longer shows the failure page: the observer saves the order before it can throw, so that presented a paid, advanced order as failed and invited a second payment.
• Rejections log the reference the caller actually sent (not the value $reference is reassigned to) and pass the exception for its trace.

Tests

112 green. Of CallbackTest's 14 cases, 12 fail on the pre-fix code and so regression-test this change: every non-success status shape including a missing status field (5), the in-flight ones (3), both throwable paths (3), and the ApiException message no longer being reflected (1). The other two pass either way and are characterization tests — that the order is loaded from Paystack's reply rather than the caller's query string, and that a response with no data at all is refused. Both pin properties a later refactor could quietly drop.

One decision for you

There is no release vehicle for this yet: the plan defines 3.0.11 (send-side) and 3.1.0 (accept-side), so as it stands this protects nobody until 3.1.0. Either a 3.0.12 security patch carrying D5 alone, or it rides 3.1.0.

Also worth knowing before the next item: registering paystack_payment_verify_after outside etc/frontend/ (the D12 fix) must not land on its own. PaymentManagement::verifyPayment() has no status check at all, so that registration would turn the inline REST leg into a second unauthenticated advance-an-unpaid-order path. Both reviewers flagged it independently at high severity; the plan is updated.

🤖 Generated with Claude Code