diff --git a/CHANGELOG.md b/CHANGELOG.md index 65298cc..851945c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The test suite runs on PHPUnit 11/12 (`^11.5 || ^12.5`, was the EOL `^10.5`): PHP 8.3+ resolves PHPUnit 12, PHP 8.2 stays on PHPUnit 11. Dev-only - nothing changes for consumers of the plugin. +- `doctrine/orm` 3.6.8 is excluded from the dev dependencies. It calls the + DBAL `Schema::edit()` API, which needs `doctrine/dbal` ^4.5, while Sylius 2.x + requires `doctrine/dbal` ^3.9 - so schema generation throws and every + functional test errors in `prepareDatabase()`. Dev-only, and a `require-dev` + entry rather than a `conflict` so consumers keep resolving their own graph; + drop it once a fixed 3.6.x is out. - Billing/shipping address fields in the one-off payment request are truncated to the character limits EveryPay enforces from 2026-10-01 (city/street 50 characters, postcode 16), so an over-long address keeps @@ -45,6 +51,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 payment completes its refund transition with the fresh snapshot. Any other rejection still rolls back and keeps the payment `completed`. +### Security + +- EveryPay API error messages no longer carry the `api_username` - half of the + HTTP Basic credential pair, which rides in the query string of every GET call. + Messages now quote the endpoint without its query string, and credential values + echoed back in a gateway error body or quoted in a transport error are redacted. +- A failed capture or status check stores a generic indicator in the payment + request `responseData` (`gateway_unavailable`, `invalid_gateway_response`) + rather than the raw exception message. Sylius serializes `responseData` to the + shopper, so gateway error bodies no longer reach it; the full detail stays in + the `everypay` log channel. An API consumer reading the `error` key now gets a + stable code instead of prose. +- A transport failure is no longer chained to the `EveryPayApiException` it + raises. Log handlers and error trackers render a previous exception's message + verbatim, and the transport one quotes the full request URL - so the credential + the message itself had dropped came straight back through the chain. The + failing exception's class and reason are carried into the message instead. + ## [0.5.0] - 2026-08-04 ### Added diff --git a/composer.json b/composer.json index c108d13..2add8b2 100644 --- a/composer.json +++ b/composer.json @@ -44,6 +44,7 @@ "require-dev": { "api-platform/json-schema": "^4.2", "behat/behat": "^3.14", + "doctrine/orm": "!=3.6.8", "friends-of-behat/symfony-extension": "^2.6", "knplabs/knp-menu-bundle": "^3.5", "phpstan/extension-installer": "^1.4", diff --git a/src/Client/EveryPayApiClient.php b/src/Client/EveryPayApiClient.php index aef4317..95bc3ee 100644 --- a/src/Client/EveryPayApiClient.php +++ b/src/Client/EveryPayApiClient.php @@ -108,12 +108,27 @@ private function request(EveryPayCredentials $credentials, string $method, strin $options['json'] = $body; } + // Exception messages reach logs and responseData, so they quote the + // endpoint rather than $path, whose query string carries api_username. + $endpoint = $this->endpoint($path); + try { $response = $this->httpClient->request($method, $credentials->baseUrl . $path, $options); $statusCode = $response->getStatusCode(); $content = $response->getContent(false); } catch (HttpClientExceptionInterface $e) { - throw new EveryPayApiException(sprintf('EveryPay request %s %s failed: %s', $method, $path, $e->getMessage()), previous: $e); + // Transport messages quote the full request URL back at us. + $reason = str_replace($credentials->baseUrl . $path, $credentials->baseUrl . $endpoint, $e->getMessage()); + + // Not chained on purpose: log handlers render a previous exception's + // message verbatim, which would put the unredacted URL straight back. + throw new EveryPayApiException(sprintf( + 'EveryPay request %s %s failed (%s): %s', + $method, + $endpoint, + $e::class, + $this->redactCredentials($reason, $credentials), + )); } $data = json_decode($content, true); @@ -123,13 +138,18 @@ private function request(EveryPayCredentials $credentials, string $method, strin 'EveryPay responded HTTP %d to %s %s: %s', $statusCode, $method, - $path, - $this->extractErrorMessage($data, $content), + $endpoint, + $this->redactCredentials($this->extractErrorMessage($data, $content), $credentials), ), $statusCode); } if (!is_array($data)) { - throw new EveryPayApiException(sprintf('EveryPay returned a non-JSON body to %s %s: %s', $method, $path, $content), $statusCode); + throw new EveryPayApiException(sprintf( + 'EveryPay returned a non-JSON body to %s %s: %s', + $method, + $endpoint, + $this->redactCredentials($content, $credentials), + ), $statusCode); } /** @var array $decoded */ @@ -138,6 +158,30 @@ private function request(EveryPayCredentials $credentials, string $method, strin return $decoded; } + /** The path without its query string, which carries api_username on every GET. */ + private function endpoint(string $path): string + { + $queryPosition = strpos($path, '?'); + + return false === $queryPosition ? $path : substr($path, 0, $queryPosition); + } + + /** Scrubs credentials from text we do not control: transport messages and gateway error bodies. */ + private function redactCredentials(string $message, EveryPayCredentials $credentials): string + { + $secrets = array_values(array_unique(array_filter([ + $credentials->apiUsername, + rawurlencode($credentials->apiUsername), + $credentials->apiSecret, + ], static fn (string $secret): bool => '' !== $secret))); + + if ([] === $secrets) { + return $message; + } + + return str_replace($secrets, '[redacted]', $message); + } + private function extractErrorMessage(mixed $data, string $fallback): string { if (is_array($data)) { diff --git a/src/CommandHandler/CaptureEveryPayPaymentHandler.php b/src/CommandHandler/CaptureEveryPayPaymentHandler.php index d2c4bc5..4b74ee2 100644 --- a/src/CommandHandler/CaptureEveryPayPaymentHandler.php +++ b/src/CommandHandler/CaptureEveryPayPaymentHandler.php @@ -94,7 +94,7 @@ public function __invoke(CaptureEveryPayPayment $command): void 'payment_id' => $payment->getId(), 'exception' => $e, ]); - $this->fail($paymentRequest, $payment, $e->getMessage()); + $this->fail($paymentRequest, $payment, EveryPayGateway::ERROR_GATEWAY_UNAVAILABLE); return; } @@ -106,7 +106,7 @@ public function __invoke(CaptureEveryPayPayment $command): void 'payment_id' => $payment->getId(), 'response' => $response, ]); - $this->fail($paymentRequest, $payment, 'EveryPay response is missing payment_reference or payment_link.'); + $this->fail($paymentRequest, $payment, EveryPayGateway::ERROR_INVALID_GATEWAY_RESPONSE); return; } @@ -142,9 +142,10 @@ public function __invoke(CaptureEveryPayPayment $command): void ); } - private function fail(PaymentRequestInterface $paymentRequest, PaymentInterface $payment, string $reason): void + /** @param string $errorCode an EveryPayGateway::ERROR_* indicator, never raw exception or gateway text */ + private function fail(PaymentRequestInterface $paymentRequest, PaymentInterface $payment, string $errorCode): void { - $paymentRequest->setResponseData(['error' => $reason]); + $paymentRequest->setResponseData(['error' => $errorCode]); $this->stateMachine->apply( $paymentRequest, diff --git a/src/CommandHandler/StatusEveryPayPaymentHandler.php b/src/CommandHandler/StatusEveryPayPaymentHandler.php index 6be77fb..944bc05 100644 --- a/src/CommandHandler/StatusEveryPayPaymentHandler.php +++ b/src/CommandHandler/StatusEveryPayPaymentHandler.php @@ -46,7 +46,7 @@ public function __invoke(StatusEveryPayPayment $command): void 'payment_id' => $payment->getId(), 'exception' => $e, ]); - $paymentRequest->setResponseData(['error' => $e->getMessage()]); + $paymentRequest->setResponseData(['error' => EveryPayGateway::ERROR_GATEWAY_UNAVAILABLE]); $this->stateMachine->apply( $paymentRequest, PaymentRequestTransitions::GRAPH, diff --git a/src/EveryPayGateway.php b/src/EveryPayGateway.php index ecd8f56..432d5e4 100644 --- a/src/EveryPayGateway.php +++ b/src/EveryPayGateway.php @@ -54,6 +54,16 @@ final class EveryPayGateway /** Key inside Payment::getDetails() holding the EveryPay payment snapshot. */ public const DETAILS_KEY = 'everypay'; + /** + * Indicators for responseData['error']. Sylius serializes responseData to the + * shopper, so raw exception text must never go there - it carries the + * api_username and the gateway body. Detail belongs in the everypay log only. + */ + public const ERROR_GATEWAY_UNAVAILABLE = 'gateway_unavailable'; + + /** EveryPay answered 2xx, but without the fields the flow needs. */ + public const ERROR_INVALID_GATEWAY_RESPONSE = 'invalid_gateway_response'; + /** Sent as integration_details.integration (EveryPay merchant telemetry). */ public const INTEGRATION_NAME = 'pkglt/sylius-everypay-plugin'; diff --git a/tests/Functional/CapturePaymentRequestTest.php b/tests/Functional/CapturePaymentRequestTest.php index 6a1836e..c2f9583 100644 --- a/tests/Functional/CapturePaymentRequestTest.php +++ b/tests/Functional/CapturePaymentRequestTest.php @@ -93,7 +93,12 @@ public function testApiFailureFailsPaymentRequestAndPaymentWithoutThrowing(): vo $payment = EveryPayGateway::corePaymentFrom($paymentRequest); self::assertSame(PaymentRequestInterface::STATE_FAILED, $paymentRequest->getState()); self::assertSame(PaymentInterface::STATE_FAILED, $payment->getState()); - self::assertArrayHasKey('error', $paymentRequest->getResponseData()); + // Shopper-readable responseData carries a generic code, never the + // gateway text (which embeds the api_username). + self::assertSame( + ['error' => EveryPayGateway::ERROR_GATEWAY_UNAVAILABLE], + $paymentRequest->getResponseData(), + ); } private function createCapturePaymentRequest(): PaymentRequestInterface diff --git a/tests/Unit/Client/EveryPayApiClientTest.php b/tests/Unit/Client/EveryPayApiClientTest.php index 866db67..9d91556 100644 --- a/tests/Unit/Client/EveryPayApiClientTest.php +++ b/tests/Unit/Client/EveryPayApiClientTest.php @@ -4,6 +4,7 @@ namespace Tests\Pkg\SyliusEveryPayPlugin\Unit\Client; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Pkg\SyliusEveryPayPlugin\Client\EveryPayApiClient; use Pkg\SyliusEveryPayPlugin\Client\EveryPayApiException; @@ -148,4 +149,59 @@ public function testTransportErrorIsWrappedInApiException(): void $client->getPayment($this->credentials(), 'abc123def456abc1'); } + + /** + * Failure messages are persisted into payment request responseData, which + * the shop API serializes for the customer - the api_username half of the + * Basic-auth pair must not ride along, neither from the request URL nor + * from a gateway response echoing it back. + * + * @param list $expectedFragments + */ + #[DataProvider('credentialLeakingFailures')] + public function testFailureMessagesKeepTheEndpointButNotTheApiUsername(MockResponse $response, array $expectedFragments): void + { + $client = new EveryPayApiClient(new MockHttpClient($response)); + + try { + $client->getPayment($this->credentials(), 'abc123def456abc1'); + self::fail('The failing request did not throw.'); + } catch (EveryPayApiException $exception) { + $message = $exception->getMessage(); + + self::assertStringNotContainsString('a04e7ce1060e7024', $message); + self::assertStringNotContainsString('api_username=', $message); + + // Log handlers walk the chain, so a linked exception must stay clean too. + for ($linked = $exception->getPrevious(); $linked !== null; $linked = $linked->getPrevious()) { + self::assertStringNotContainsString('a04e7ce1060e7024', $linked->getMessage()); + } + + foreach ($expectedFragments as $fragment) { + self::assertStringContainsString($fragment, $message); + } + } + } + + /** @return iterable}> */ + public static function credentialLeakingFailures(): iterable + { + yield 'transport error quoting the request URL' => [ + new MockResponse('', ['error' => 'Could not resolve host for "https://igw-demo.every-pay.com/api/v4/payments/abc123def456abc1?api_username=a04e7ce1060e7024"']), + ['GET /v4/payments/abc123def456abc1 failed', 'Could not resolve host'], + ]; + + yield 'error body echoing the api_username' => [ + new MockResponse( + json_encode(['error' => ['code' => 4013, 'message' => 'Unknown api_username a04e7ce1060e7024']], \JSON_THROW_ON_ERROR), + ['http_code' => 401], + ), + ['HTTP 401 to GET /v4/payments/abc123def456abc1', 'Unknown api_username [redacted]'], + ]; + + yield 'non-JSON body echoing the api_username' => [ + new MockResponse('api_username a04e7ce1060e7024 blocked'), + ['non-JSON body to GET /v4/payments/abc123def456abc1', 'api_username [redacted] blocked'], + ]; + } } diff --git a/tests/Unit/CommandHandler/CaptureEveryPayPaymentHandlerTest.php b/tests/Unit/CommandHandler/CaptureEveryPayPaymentHandlerTest.php index 402f54e..5f8637f 100644 --- a/tests/Unit/CommandHandler/CaptureEveryPayPaymentHandlerTest.php +++ b/tests/Unit/CommandHandler/CaptureEveryPayPaymentHandlerTest.php @@ -73,7 +73,12 @@ public function testApiFailureFailsPaymentRequestAndPaymentWithoutThrowing(): vo $handler(new CaptureEveryPayPayment('hash')); - self::assertArrayHasKey('error', $paymentRequest->getResponseData()); + // A generic code only - the exception message carries the api_username + // and the gateway body, and responseData reaches the shopper. + self::assertSame( + ['error' => EveryPayGateway::ERROR_GATEWAY_UNAVAILABLE], + $paymentRequest->getResponseData(), + ); self::assertSame( [ [$paymentRequest, PaymentRequestTransitions::GRAPH, PaymentRequestTransitions::TRANSITION_FAIL], @@ -107,7 +112,10 @@ public function testResponseWithoutReferenceOrLinkFailsTheAttempt(): void // A 2xx response without the hosted page link is as unusable as an // API error: fail the attempt so the customer gets a fresh payment. - self::assertArrayHasKey('error', $paymentRequest->getResponseData()); + self::assertSame( + ['error' => EveryPayGateway::ERROR_INVALID_GATEWAY_RESPONSE], + $paymentRequest->getResponseData(), + ); self::assertSame( [ [$paymentRequest, PaymentRequestTransitions::GRAPH, PaymentRequestTransitions::TRANSITION_FAIL], diff --git a/tests/Unit/CommandHandler/StatusEveryPayPaymentHandlerTest.php b/tests/Unit/CommandHandler/StatusEveryPayPaymentHandlerTest.php index 1405b10..9c2bcd0 100644 --- a/tests/Unit/CommandHandler/StatusEveryPayPaymentHandlerTest.php +++ b/tests/Unit/CommandHandler/StatusEveryPayPaymentHandlerTest.php @@ -11,6 +11,7 @@ use Pkg\SyliusEveryPayPlugin\EveryPayGateway; use Pkg\SyliusEveryPayPlugin\Processor\EveryPayPaymentSynchronizer; use Pkg\SyliusEveryPayPlugin\Processor\EveryPayStateMapper; +use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; use Sylius\Abstraction\StateMachine\StateMachineInterface; use Sylius\Bundle\PaymentBundle\Provider\PaymentRequestProviderInterface; @@ -23,11 +24,15 @@ use Sylius\Component\Payment\PaymentTransitions; use Symfony\Component\HttpClient\MockHttpClient; use Symfony\Component\HttpClient\Response\MockResponse; +use Tests\Pkg\SyliusEveryPayPlugin\Support\RecordingLogger; final class StatusEveryPayPaymentHandlerTest extends TestCase { private const PAYMENT_REFERENCE = 'abc123def456abc123def456abc123def456abc123def456abc123def456abcd'; + /** Half of the HTTP Basic credential pair - it must never reach the shopper. */ + private const API_USERNAME = 'a04e7ce1060e7024'; + /** @var array */ private array $appliedTransitions = []; @@ -61,7 +66,10 @@ public function testApiFailureFailsOnlyTheRequestSoCallbacksSettleThePaymentLate // The documented invariant: a temporary API failure on customer return // is swallowed - the payment stays processing and the server callback // redeliveries settle it later. - self::assertArrayHasKey('error', $paymentRequest->getResponseData()); + self::assertSame( + ['error' => EveryPayGateway::ERROR_GATEWAY_UNAVAILABLE], + $paymentRequest->getResponseData(), + ); self::assertSame( [[$paymentRequest, PaymentRequestTransitions::GRAPH, PaymentRequestTransitions::TRANSITION_FAIL]], $this->appliedTransitions, @@ -72,11 +80,38 @@ public function testApiFailureFailsOnlyTheRequestSoCallbacksSettleThePaymentLate self::assertSame(PaymentInterface::STATE_PROCESSING, $payment->getState()); } + public function testApiFailureKeepsTheCredentialAndGatewayTextOutOfTheShopperVisibleResponse(): void + { + $paymentRequest = $this->paymentRequest(); + $logger = new RecordingLogger(); + // The client folds the request path (which carries api_username) and the + // raw gateway body into the exception message. responseData is served to + // the shopper by the Sylius shop API, so neither may be persisted there. + $handler = $this->handler( + $paymentRequest, + new MockResponse('gateway maintenance in progress', ['http_code' => 500]), + $logger, + ); + + $handler(new StatusEveryPayPayment('hash')); + + $responseData = $paymentRequest->getResponseData(); + self::assertSame(['error' => EveryPayGateway::ERROR_GATEWAY_UNAVAILABLE], $responseData); + + $serialized = json_encode($responseData, \JSON_THROW_ON_ERROR); + self::assertStringNotContainsString(self::API_USERNAME, $serialized); + self::assertStringNotContainsString('maintenance', $serialized); + + // The detail stays operator-facing: it is logged (with the exception in + // the context) on the everypay channel. + self::assertSame(['EveryPay status check on customer return failed.'], $logger->messages('error')); + } + private function paymentRequest(): PaymentRequest { $gatewayConfig = $this->createStub(GatewayConfigInterface::class); $gatewayConfig->method('getConfig')->willReturn([ - EveryPayGateway::CONFIG_API_USERNAME => 'a04e7ce1060e7024', + EveryPayGateway::CONFIG_API_USERNAME => self::API_USERNAME, EveryPayGateway::CONFIG_API_SECRET => 'secret', EveryPayGateway::CONFIG_ACCOUNT_NAME => 'EUR3D1', EveryPayGateway::CONFIG_ENVIRONMENT => EveryPayGateway::ENVIRONMENT_DEMO, @@ -95,7 +130,7 @@ private function paymentRequest(): PaymentRequest return new PaymentRequest($payment, $method); } - private function handler(PaymentRequest $paymentRequest, MockResponse $apiResponse): StatusEveryPayPaymentHandler + private function handler(PaymentRequest $paymentRequest, MockResponse $apiResponse, ?LoggerInterface $logger = null): StatusEveryPayPaymentHandler { $paymentRequestProvider = $this->createStub(PaymentRequestProviderInterface::class); $paymentRequestProvider->method('provide')->willReturn($paymentRequest); @@ -120,7 +155,7 @@ function (object $subject, string $graph, string $transition): void { $paymentRequestProvider, $synchronizer, $stateMachine, - new NullLogger(), + $logger ?? new NullLogger(), ); } }