Refuse a callback whose transaction is not successful (D5)

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);

Test/Unit/Controller/Payment/CallbackTest.php

@@ -43,13 +43,17 @@ class CallbackTest extends TestCase
     /** @var MockObject|MessageManager */
     private $messageManager;
 
+    /** @var MockObject|LoggerInterface */
+    private $logger;
+
     private function createController(): Callback
     {
         $this->paystackClient = $this->createMock(PaystackApiClient::class);
         $this->eventManager = $this->createMock(EventManager::class);
         $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();