From 58121e90069828e003bc1b71c3be40d95d094e32 Mon Sep 17 00:00:00 2001 From: Jules Nsenda Date: Tue, 29 Sep 2026 13:23:50 +0200 Subject: [PATCH] fix: stop Setup.php leaking raw gateway errors to the customer Controller/Payment/Setup.php passed ApiException::getMessage() -- built from curl_error() and Paystack's raw API response body -- straight to the customer-facing checkout failure page. Same leak class D5 already closed on Callback.php, missed here. The abandoned fix/r2.1-settlement-gate branch (reconciled in PR #77) had already fixed this but it wasn't part of that reconciliation's scope; found by diffing that branch against master after the fact. Detail now goes to the logger (context array, not string concatenation, so an embedded newline can't forge log lines) and order history (admin-only) instead; the customer gets a fixed, safe string. Also added a bare \Throwable catch matching Callback.php's D5 fix -- the ApiException-only catch left every other exception processAuthorization() can throw (a missing store URL, a malformed response, a save failure) uncaught. Co-Authored-By: Claude Sonnet 5 --- Controller/Payment/Setup.php | 27 +++++++++- Test/Unit/Controller/Payment/SetupTest.php | 61 ++++++++++++++++++++++ 2 files changed, 86 insertions(+), 2 deletions(-) diff --git a/Controller/Payment/Setup.php b/Controller/Payment/Setup.php index 65be86a..4d3ee87 100644 --- a/Controller/Payment/Setup.php +++ b/Controller/Payment/Setup.php @@ -38,9 +38,32 @@ public function execute() { try { return $this->processAuthorization($order); } catch (\Pstk\Paystack\Gateway\Exception\ApiException $e) { - $message = $e->getMessage(); - $order->addStatusToHistory($order->getStatus(), $message); + // The exception message is built from curl_error() and Paystack's raw + // response body (see Gateway/PaystackApiClient::request()), so it can + // carry internal hostnames, TLS/proxy detail, or gateway-side state — + // the same leak D5 closed on Callback.php. Detail stays in the + // (admin-only) order history; the customer gets a fixed, safe string. + // Context array, not string concatenation, so an embedded newline in + // the gateway/curl text can't forge additional log lines. + $this->logger->error('Paystack setup API error', [ + 'error' => $e->getMessage(), + 'exception' => $e, + ]); + $order->addStatusToHistory($order->getStatus(), $e->getMessage()); $this->orderRepository->save($order); + $message = "We could not start your Paystack payment. Please try again " + . "or contact support if the problem continues."; + } catch (\Throwable $e) { + // Same rationale as the ApiException branch above, for anything else + // processAuthorization() can throw (a missing store URL, a malformed + // Paystack response, a save failure) — D5's Callback.php fix added + // this same bare-Throwable catch for the identical reason. + $this->logger->error('Paystack setup failed', [ + 'error' => $e->getMessage(), + 'exception' => $e, + ]); + $message = "We could not start your Paystack payment. Please try again " + . "or contact support if the problem continues."; } } diff --git a/Test/Unit/Controller/Payment/SetupTest.php b/Test/Unit/Controller/Payment/SetupTest.php index 16b15c2..3f60b37 100644 --- a/Test/Unit/Controller/Payment/SetupTest.php +++ b/Test/Unit/Controller/Payment/SetupTest.php @@ -457,6 +457,67 @@ public function testApiExceptionSavesStatusHistory(): void ->method('save') ->with($order); + // The raw ApiException message ("Invalid key" — built from curl_error() + // and Paystack's raw response body in real use) must never reach the + // customer-facing failure page; only the fixed, safe string may. + $this->messageManager->expects($this->once()) + ->method('addErrorMessage') + ->with($this->callback(function ($message) { + $text = (string) $message; + return !str_contains($text, 'Invalid key') + && str_contains($text, 'We could not start your Paystack payment'); + })); + + $controller->execute(); + } + + public function testGenericThrowableShowsSafeMessageAndDoesNotLeakDetail(): void + { + $controller = $this->createController(); + + $lastOrder = $this->createMock(Order::class); + $lastOrder->method('getIncrementId')->willReturn('000000001'); + $this->checkoutSession->method('getLastRealOrder')->willReturn($lastOrder); + + $payment = $this->createMock(Payment::class); + $payment->method('getMethod')->willReturn(Paystack::CODE); + + $order = $this->createMock(Order::class); + $order->method('getPayment')->willReturn($payment); + $order->method('getStatus')->willReturn('pending'); + $order->method('getCustomerFirstname')->willReturn('John'); + $order->method('getCustomerLastname')->willReturn('Doe'); + $order->method('getGrandTotal')->willReturn(100.00); + $order->method('getCustomerEmail')->willReturn('john@test.com'); + $order->method('getIncrementId')->willReturn('000000001'); + $order->method('getOrderCurrencyCode')->willReturn('NGN'); + + $this->orderInterface->method('loadByIncrementId')->willReturn($order); + + $methodInstance = $this->createMock(MethodInterface::class); + $methodInstance->method('getCode')->willReturn(Paystack::CODE); + $this->paymentHelper->method('getMethodInstance')->willReturn($methodInstance); + + $store = $this->createMock(Store::class); + $store->method('getBaseUrl')->willReturn('https://example.com/'); + $this->storeManager->method('getStore')->willReturn($store); + + $this->transactionValidator->method('expectedSubunits')->willReturn(10000); + + // A non-ApiException throwable (e.g. a malformed Paystack response, a + // missing store URL) must be caught too — not just ApiException — and + // must never surface its own message to the customer. + $this->paystackClient->method('initializeTransaction') + ->willThrowException(new \RuntimeException('unexpected internal detail')); + + $this->messageManager->expects($this->once()) + ->method('addErrorMessage') + ->with($this->callback(function ($message) { + $text = (string) $message; + return !str_contains($text, 'unexpected internal detail') + && str_contains($text, 'We could not start your Paystack payment'); + })); + $controller->execute(); } }