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 <noreply@anthropic.com>
This commit is contained in:
+2
-1
@@ -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.
|
||||
|
||||
|
||||
@@ -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._
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,74 @@
|
||||
<?php
|
||||
|
||||
namespace App\Tests\Payment;
|
||||
|
||||
use App\Config\Entity\SiteConfig;
|
||||
use App\Payment\Entity\Payment;
|
||||
use App\Tests\ApiTestCase;
|
||||
|
||||
/**
|
||||
* The payment callback must confirm an order only when the gateway-reported
|
||||
* settled amount matches what we charged. Guards against underpayment / a
|
||||
* replayed RefNum from another (cheaper) order marking an expensive order paid.
|
||||
*/
|
||||
class PaymentCallbackAmountTest extends ApiTestCase
|
||||
{
|
||||
private function enableTestMode(): void
|
||||
{
|
||||
$cfg = $this->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());
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user