From ae06498a96eeee4b61db492a2b80f66e5913ae04 Mon Sep 17 00:00:00 2001 From: hamed <15238-genius.ha@users.noreply.drupalcode.org> Date: Sun, 28 Jun 2026 18:49:33 +0330 Subject: [PATCH] fix(security): verify gateway-confirmed amount in payment callback (C1) The callback marked an order success on any verify-ok result without comparing the gateway-settled amount to the amount charged. SEP returns AffectiveAmount; an underpayment or a replayed RefNum from a cheaper order would confirm the expensive order. Now reject (status=failed, no activation) when the gateway reports an amount that mismatches the stored amount_rials. Gateways that don't report a settled amount (Mellat binds it server-side) skip the check. MockGateway now echoes mock_amount so the guard is exercisable in tests. Regression: tests/Payment/PaymentCallbackAmountTest (underpayment rejected, matching amount succeeds). Co-Authored-By: Claude Opus 4.8 --- docs/api/payment.md | 3 +- docs/audit-backlog.md | 5 +- src/Payment/Controller/PaymentController.php | 14 +++- src/Payment/Gateway/MockGateway.php | 7 +- tests/Payment/PaymentCallbackAmountTest.php | 74 ++++++++++++++++++++ 5 files changed, 97 insertions(+), 6 deletions(-) create mode 100644 tests/Payment/PaymentCallbackAmountTest.php diff --git a/docs/api/payment.md b/docs/api/payment.md index 08b72222..09d39e32 100644 --- a/docs/api/payment.md +++ b/docs/api/payment.md @@ -152,7 +152,8 @@ After verifying the gateway result, the backend redirects the user **back to the ``` {frontend_address}?payment_uuid={uuid}&status={status} ``` -- **Success** (`verify` ok): payment → `success`, then the type-specific action runs (appointment → `confirmed`, subscription → activated, sms_wallet → credited). +- **Success** (`verify` ok **and** amount matches): payment → `success`, then the type-specific action runs (appointment → `confirmed`, subscription → activated, sms_wallet → credited). +- **Amount mismatch**: when the gateway reports the settled amount (SEP `AffectiveAmount`) and it does **not** equal the order's `amount_rials`, the callback is treated as failed — payment → `failed`, the type-specific action does **not** run. Guards against underpayment and replaying another (cheaper) order's reference. Gateways that don't report a settled amount (Mellat binds it server-side to the original request) skip this check. - **User canceled** (e.g. Mellat `ResCode=17`, SEP `State=CanceledByUser`, mock `cancel=1`): payment → `canceled`. The gateway circuit-breaker is **not** marked as failed (it's a user choice, not a gateway fault). - **Failed** (any other unsuccessful verify): payment → `failed`, circuit-breaker records a failure. diff --git a/docs/audit-backlog.md b/docs/audit-backlog.md index 953c4eb0..8899e49c 100644 --- a/docs/audit-backlog.md +++ b/docs/audit-backlog.md @@ -23,14 +23,13 @@ Last full scan: 2026-06-28 (5 parallel investigators: idor/massassign, auth-surf | D8 | missing indexes (appointment expiry, session dates, user status) | perf | 8b96753 | | D9 | repair phpstan config | devops | e456809 | | D10 | priv-esc — commission_percent/active admin-only on PATCH representation | security | 6bd49c2 | +| D11 | **C1** payment callback verifies gateway-confirmed amount vs stored amount (anti underpayment / RefNum-replay) | security | (this commit) | --- ## ☐ CRITICAL -| # | Task | File:line | Cat | How to test | -|---|------|-----------|-----|-------------| -| C1 | **Payment callback never verifies gateway-confirmed amount vs stored amount** — pay less / replay another order's RefNum still confirms order at requested amount | src/Payment/Controller/PaymentController.php:265-279 · src/Payment/Gateway/SepGateway.php:54-89 · MellatGateway.php:53-84 | security-callback | Verify SEP `AffectiveAmount` compared to `$payment->getAmountRials()`; functional test asserting mismatch → reject | +_None outstanding._ --- diff --git a/src/Payment/Controller/PaymentController.php b/src/Payment/Controller/PaymentController.php index ec2c6ac1..653e9488 100644 --- a/src/Payment/Controller/PaymentController.php +++ b/src/Payment/Controller/PaymentController.php @@ -263,7 +263,7 @@ class PaymentController extends BaseController $result = $gw?->verify($callbackData) ?? null; if ($result === null || !$result->success) { - $canceled = $result?->canceled ?? false; + $canceled = $result !== null && $result->canceled; $payment->setStatus($canceled ? Payment::STATUS_CANCELED : Payment::STATUS_FAILED); $this->paymentRepo->save($payment); if (!$canceled) { @@ -274,6 +274,18 @@ class PaymentController extends BaseController } $this->circuitBreaker->recordSuccess($gateway); + + // Gateway-confirmed amount must match the amount we charged. Gateways that + // report the settled amount (SEP: AffectiveAmount) let us catch an + // underpayment / RefNum-replay; gateways that don't report it bind the + // amount server-side to the original request, so amountRials is 0 here. + if ($result->amountRials > 0 && $result->amountRials !== $payment->getAmountRials()) { + $payment->setStatus(Payment::STATUS_FAILED); + $this->paymentRepo->save($payment); + + return $this->redirectToFrontend($payment, false); + } + $payment->setStatus(Payment::STATUS_SUCCESS); $payment->setReferenceId($result->referenceId); $this->paymentRepo->save($payment); diff --git a/src/Payment/Gateway/MockGateway.php b/src/Payment/Gateway/MockGateway.php index 25318008..f35a3b04 100644 --- a/src/Payment/Gateway/MockGateway.php +++ b/src/Payment/Gateway/MockGateway.php @@ -26,6 +26,11 @@ class MockGateway implements PaymentGatewayInterface } $refId = $callbackData['RefId'] ?? $callbackData['order_id'] ?? 'MOCK-REF'; - return new PaymentVerifyResult(true, referenceId: $refId); + + // Simulate a gateway that reports the settled amount (like SEP's + // AffectiveAmount) so the controller's amount-mismatch guard is exercisable. + $amount = (int) ($callbackData['mock_amount'] ?? 0); + + return new PaymentVerifyResult(true, referenceId: $refId, amountRials: $amount); } } diff --git a/tests/Payment/PaymentCallbackAmountTest.php b/tests/Payment/PaymentCallbackAmountTest.php new file mode 100644 index 00000000..1b5b509a --- /dev/null +++ b/tests/Payment/PaymentCallbackAmountTest.php @@ -0,0 +1,74 @@ +em->getRepository(SiteConfig::class)->findOneBy(['configKey' => 'payment_test_mode']); + if ($cfg === null) { + $cfg = new SiteConfig('payment_test_mode', '1'); + $this->em->persist($cfg); + } else { + $cfg->setValue('1'); + } + $this->em->flush(); + } + + private function makePayment(int $amountRials): Payment + { + $user = $this->createUser(); + $payment = new Payment($user, $amountRials, 'mock', Payment::TYPE_SMS_WALLET); + $this->em->persist($payment); + $this->em->flush(); + + return $payment; + } + + private function fireCallback(Payment $payment, int $reportedAmount): void + { + $this->client->request('POST', '/api/v1/payment/callback/mock?' . http_build_query([ + 'order_id' => $payment->getOrderId(), + 'mock' => '1', + 'ResCode' => '0', + 'mock_amount' => (string) $reportedAmount, + ])); + } + + private function reload(Payment $payment): Payment + { + $this->em->clear(); + + return $this->em->getRepository(Payment::class)->find($payment->getId()); + } + + public function testUnderpaymentIsRejected(): void + { + $this->enableTestMode(); + $payment = $this->makePayment(50000); + + $this->fireCallback($payment, 10000); + + $this->assertSame(Payment::STATUS_FAILED, $this->reload($payment)->getStatus()); + } + + public function testMatchingAmountSucceeds(): void + { + $this->enableTestMode(); + $payment = $this->makePayment(50000); + + $this->fireCallback($payment, 50000); + + $this->assertSame(Payment::STATUS_SUCCESS, $this->reload($payment)->getStatus()); + } +}