From 37a4c3d31d5c451372a808e8ab9184889ff3c3b0 Mon Sep 17 00:00:00 2001 From: Jules Nsenda Date: Mon, 17 Aug 2026 19:49:28 +0200 Subject: [PATCH] fix: refuse a callback whose transaction is not successful (D5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /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`, so a single unauthenticated GET with a guessed increment ID advanced any order to Processing with nothing paid. Measured on dev-repro: with this change reverted, an order backed by a real `abandoned` transaction still advances to Processing and the caller lands on the success page; with it in place the order stays new/pending, and a genuinely successful test charge on the same route still advances it. 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 D6/D7, closed by R2.1's amount/currency window and R2.3's payment registration. Alongside the gate, 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 any more. 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, rather than 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` has been reassigned to, and pass the exception for its trace. Tests: 112 total green. Of the 14 cases in CallbackTest, 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 remaining two are characterization tests, passing either way: 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. Co-Authored-By: Claude Opus 5 (1M context) --- Controller/Payment/Callback.php | 91 ++++++- Test/Unit/Controller/Payment/CallbackTest.php | 229 +++++++++++++++++- 2 files changed, 310 insertions(+), 10 deletions(-) diff --git a/Controller/Payment/Callback.php b/Controller/Payment/Callback.php index 5ae7169..03fdb51 100644 --- a/Controller/Payment/Callback.php +++ b/Controller/Payment/Callback.php @@ -33,14 +33,53 @@ public function execute() { $reference = $this->request->get('reference'); $message = ""; - + + // Kept separate: `$reference` is reassigned below to the order's increment ID, + // and it is the value the caller actually sent that a rejection needs to record. + $requestedReference = $reference; + + // A rejection must not tell the customer to try again once money may have + // moved. This is the one generic surface used by every failure branch. + $unconfirmed = "We could not confirm your payment. If you believe you were " + . "charged, please contact support before trying again."; + if (!$reference) { return $this->redirectToFinal(false, "No reference supplied"); } - + + $dispatched = false; + try { $transactionDetails = $this->paystackClient->verifyTransaction($reference); - + + // The verify response is the only trustworthy statement of what happened + // to the money. Anyone can hit this route with an arbitrary reference and + // no session, so nothing may advance an order until Paystack itself says + // the charge succeeded. + $status = $transactionDetails->data->status ?? null; + + if ('success' !== $status) { + $this->logger->warning( + 'Paystack callback rejected: transaction is not successful', + ["reference" => $requestedReference, "status" => $status] + ); + + // `pending`, `ongoing` and `queued` are in-flight, not failed: bank + // transfer and USSD sit there at callback time and settle minutes + // later via the webhook. Telling that customer to try again invites a + // second payment for a charge already on its way. + $inFlight = in_array($status, ["pending", "ongoing", "queued"], true); + + return $this->redirectToFinal( + false, + $inFlight + ? "Your payment is still being confirmed. Please do not pay " + . "again — we will email you once it completes." + : "Your payment was not completed. If you believe you were " + . "charged, please contact support before trying again." + ); + } + $reference = explode('_', $transactionDetails->data->reference, 2); $reference = ($reference[0])?: 0; @@ -49,6 +88,8 @@ public function execute() { if ($order && $reference === $order->getIncrementId()) { // dispatch the `payment_verify_after` event to update the order status + $dispatched = true; + $this->eventManager->dispatch('paystack_payment_verify_after', [ "paystack_order" => $order, ]); @@ -57,13 +98,45 @@ public function execute() { } $message = "Invalid reference or order number"; - + } catch (\Pstk\Paystack\Gateway\Exception\ApiException $e) { - $message = $e->getMessage(); - - } catch (Exception $e) { - $message = $e->getMessage(); - + // The message is not reflected to the caller: it is built from curl_error() + // and Paystack's raw response, so it both leaks internal detail and turns + // this anonymous route into an oracle that distinguishes "no such + // reference" from "reference exists". + $this->logger->error( + 'Paystack callback API error: ' . $e->getMessage(), + ["reference" => $requestedReference, "exception" => $e] + ); + + $message = $unconfirmed; + + } catch (\Throwable $e) { + // Was `catch (Exception $e)` — unqualified in a namespaced file with no + // `use`, so it resolved to a class that does not exist and never caught + // anything: every non-ApiException escaped as a 500. The message is not + // shown to the customer because it can carry internal detail. + $this->logger->error( + 'Paystack callback failed: ' . $e->getMessage(), + ["reference" => $requestedReference, "exception" => $e] + ); + + $message = $unconfirmed; + } + + if ($dispatched) { + // Verification succeeded and the advance was already under way when something + // downstream threw — the observer saves the order before it sends the email. + // Showing the failure page here would present a paid order as failed and + // invite a second payment; the warning covers the narrower case where the + // throw came before the save, so the order is paid but not yet advanced. + $this->messageManager->addWarningMessage( + __("Your payment was received, but we could not finish updating your " + . "order. Please do not pay again — contact support if you do not " + . "receive a confirmation email shortly.") + ); + + return $this->redirectToFinal(true); } return $this->redirectToFinal(false, $message); diff --git a/Test/Unit/Controller/Payment/CallbackTest.php b/Test/Unit/Controller/Payment/CallbackTest.php index 0e41620..0b04515 100644 --- a/Test/Unit/Controller/Payment/CallbackTest.php +++ b/Test/Unit/Controller/Payment/CallbackTest.php @@ -43,6 +43,9 @@ class CallbackTest extends TestCase /** @var MockObject|MessageManager */ private $messageManager; + /** @var MockObject|LoggerInterface */ + private $logger; + private function createController(): Callback { $this->paystackClient = $this->createMock(PaystackApiClient::class); @@ -50,6 +53,7 @@ private function createController(): Callback $this->request = $this->createMock(HttpRequest::class); $this->orderInterface = $this->createMock(\Magento\Sales\Model\Order::class); $this->messageManager = $this->createMock(MessageManager::class); + $this->logger = $this->createMock(LoggerInterface::class); $redirect = $this->createMock(Redirect::class); $redirect->method('setUrl')->willReturnSelf(); @@ -84,7 +88,7 @@ private function createController(): Callback $this->createMock(StoreManagerInterface::class), $this->eventManager, $this->request, - $this->createMock(LoggerInterface::class), + $this->logger, $this->paystackClient ); } @@ -120,6 +124,37 @@ public function testSuccessfulCallbackDispatchesEvent(): void $controller->execute(); } + /** + * The load-bearing half of the gate: which order is settled comes from Paystack's + * reply, never from the caller's query string. With both set to the same value a + * refactor could swap them silently, so here they differ. + */ + public function testOrderIsLoadedFromTheVerifyResponseNotTheRequest(): void + { + $controller = $this->createController(); + + $this->request->method('get')->willReturn('000000099_attacker'); + + $this->paystackClient->method('verifyTransaction') + ->with('000000099_attacker') + ->willReturn((object) ['data' => (object) [ + 'reference' => '000000001_suffix', + 'status' => 'success', + ]]); + + $order = $this->createMock(\Magento\Sales\Model\Order::class); + $order->method('getIncrementId')->willReturn('000000001'); + + $this->orderInterface->expects($this->once()) + ->method('loadByIncrementId') + ->with('000000001') + ->willReturn($order); + + $this->eventManager->expects($this->once())->method('dispatch'); + + $controller->execute(); + } + public function testMissingReferenceRedirectsToFailure(): void { $controller = $this->createController(); @@ -153,6 +188,198 @@ public function testApiExceptionRedirectsToFailure(): void $controller->execute(); } + /** + * D5: the callback route is anonymous, so an unverified transaction must never + * advance an order. Before this gate existed, any reference at all — including + * one whose transaction was abandoned and never paid for — dispatched the event + * and flipped the order to Processing. + * + * @dataProvider nonSuccessStatusProvider + */ + public function testNonSuccessfulTransactionDoesNotDispatchEvent($hasStatus, $status): void + { + $controller = $this->createController(); + + $this->request->method('get')->willReturn('000000001'); + + $data = ['reference' => '000000001']; + if ($hasStatus) { + $data['status'] = $status; + } + $this->paystackClient->method('verifyTransaction') + ->willReturn((object) ['data' => (object) $data]); + + // The order exists and its increment ID matches, so the only thing standing + // between this request and an advanced order is the status gate. + $order = $this->createMock(\Magento\Sales\Model\Order::class); + $order->method('getIncrementId')->willReturn('000000001'); + $this->orderInterface->method('loadByIncrementId')->willReturn($order); + + $this->eventManager->expects($this->never())->method('dispatch'); + $this->messageManager->expects($this->once())->method('addErrorMessage'); + // The other half of the observable: the customer must not be told it worked. + $this->messageManager->expects($this->never())->method('addSuccessMessage'); + + $controller->execute(); + } + + public static function nonSuccessStatusProvider(): array + { + return [ + 'abandoned' => [true, 'abandoned'], + 'failed' => [true, 'failed'], + 'pending' => [true, 'pending'], + 'reversed' => [true, 'reversed'], + 'no status field at all' => [false, null], + ]; + } + + /** + * A verify response with no `data` object at all must fail closed too — this is what + * stops a later refactor from reading the status through anything but `??`. + */ + public function testResponseWithoutDataIsRejected(): void + { + $controller = $this->createController(); + + $this->request->method('get')->willReturn('000000001'); + $this->paystackClient->method('verifyTransaction') + ->willReturn((object) ['status' => true, 'message' => 'Verification successful']); + + $this->eventManager->expects($this->never())->method('dispatch'); + $this->messageManager->expects($this->never())->method('addSuccessMessage'); + + $controller->execute(); + } + + /** + * The generic catch was `catch (Exception $e)` — unqualified in a namespaced file + * with no `use`, so it caught nothing and anything but an ApiException escaped as + * a 500. It is `\Throwable` now. + */ + public function testUnexpectedThrowableIsCaughtAndDoesNotDispatch(): void + { + $controller = $this->createController(); + + $this->request->method('get')->willReturn('000000001'); + + $this->paystackClient->method('verifyTransaction') + ->willThrowException(new \RuntimeException('connection reset')); + + $this->eventManager->expects($this->never())->method('dispatch'); + $this->messageManager->expects($this->once())->method('addErrorMessage'); + $this->messageManager->expects($this->never())->method('addSuccessMessage'); + + $controller->execute(); + } + + /** + * The customer-facing message must not carry internal exception detail. + */ + public function testThrowableMessageIsNotShownToTheCustomer(): void + { + $controller = $this->createController(); + + $this->request->method('get')->willReturn('000000001'); + + $this->paystackClient->method('verifyTransaction') + ->willThrowException(new \RuntimeException('SQLSTATE[HY000] internal detail')); + + $this->messageManager->expects($this->once()) + ->method('addErrorMessage') + ->with($this->callback(function ($message) { + return strpos((string) $message, 'internal detail') === false; + })); + + $controller->execute(); + } + + /** + * `pending`/`ongoing`/`queued` are in-flight (bank transfer, USSD), not failed. The + * customer must not be told to try again while a charge is on its way. + * + * @dataProvider inFlightStatusProvider + */ + public function testInFlightTransactionDoesNotInviteASecondPayment($status): void + { + $controller = $this->createController(); + + $this->request->method('get')->willReturn('000000001'); + $this->paystackClient->method('verifyTransaction') + ->willReturn((object) ['data' => (object) [ + 'reference' => '000000001', + 'status' => $status, + ]]); + + $this->messageManager->expects($this->once()) + ->method('addErrorMessage') + ->with($this->callback(function ($message) { + return stripos((string) $message, 'do not pay again') !== false; + })); + + $controller->execute(); + } + + public static function inFlightStatusProvider(): array + { + return [['pending'], ['ongoing'], ['queued']]; + } + + /** + * ApiException messages are built from curl_error() and Paystack's raw response, so + * reflecting them leaks internal detail and turns this anonymous route into an + * oracle for which references and increment IDs exist. + */ + public function testApiExceptionMessageIsNotShownToTheCustomer(): void + { + $controller = $this->createController(); + + $this->request->method('get')->willReturn('000000001'); + $this->paystackClient->method('verifyTransaction') + ->willThrowException(new ApiException('Transaction reference not found')); + + $this->messageManager->expects($this->once()) + ->method('addErrorMessage') + ->with($this->callback(function ($message) { + return stripos((string) $message, 'reference not found') === false; + })); + + $controller->execute(); + } + + /** + * The observer saves the order before it can throw, so a throwable raised after the + * dispatch leaves a paid, advanced order — reporting failure there would invite a + * second payment for it. + */ + public function testThrowableAfterDispatchDoesNotReportFailure(): void + { + $controller = $this->createController(); + + $this->request->method('get')->willReturn('000000001'); + $this->paystackClient->method('verifyTransaction') + ->willReturn((object) ['data' => (object) [ + 'reference' => '000000001', + 'status' => 'success', + ]]); + + $order = $this->createMock(\Magento\Sales\Model\Order::class); + $order->method('getIncrementId')->willReturn('000000001'); + $this->orderInterface->method('loadByIncrementId')->willReturn($order); + + $this->eventManager->method('dispatch') + ->willThrowException(new \RuntimeException('observer blew up after saving')); + + $this->messageManager->expects($this->never())->method('addErrorMessage'); + $this->messageManager->expects($this->once()) + ->method('addWarningMessage') + ->with($this->callback(function ($message) { + return stripos((string) $message, 'do not pay again') !== false; + })); + + $controller->execute(); + } + public function testOrderNotFoundRedirectsToFailure(): void { $controller = $this->createController();