From 7fa4b55d3f4a3206520bec1ffa201ba1cf7c31b4 Mon Sep 17 00:00:00 2001 From: hamed <15238-genius.ha@users.noreply.drupalcode.org> Date: Sun, 28 Jun 2026 19:08:26 +0330 Subject: [PATCH] fix(security): unique payment reference_id, reject replayed callbacks (H3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reference_id (the gateway's settled-transaction ref) was not unique, so the same successful callback — or a RefNum replayed onto another order — could credit twice. Add a unique index (NULL until success, so pending/failed rows don't collide) and an application-level pre-check in the callback that fails the payment if the reference already belongs to another order. The unique index is the hard backstop behind the check. Regression: PaymentCallbackAmountTest::testReplayedGatewayReferenceIsRejected. Co-Authored-By: Claude Opus 4.8 --- docs/api/payment.md | 1 + docs/audit-backlog.md | 2 +- migrations/Version20260628153625.php | 31 ++++++++++++++++++++ src/Payment/Controller/PaymentController.php | 13 ++++++++ src/Payment/Entity/Payment.php | 2 +- src/Payment/Repository/PaymentRepository.php | 5 ++++ tests/Payment/PaymentCallbackAmountTest.php | 28 ++++++++++++++++++ 7 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 migrations/Version20260628153625.php diff --git a/docs/api/payment.md b/docs/api/payment.md index 09d39e32..bd697e30 100644 --- a/docs/api/payment.md +++ b/docs/api/payment.md @@ -154,6 +154,7 @@ After verifying the gateway result, the backend redirects the user **back to the ``` - **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. +- **Replayed reference**: a gateway `reference_id` identifies exactly one settled transaction. If the callback's reference already belongs to another payment, it is rejected (payment → `failed`). Enforced by a unique index on `payments.reference_id` with an application-level pre-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 2b119782..bb7670db 100644 --- a/docs/audit-backlog.md +++ b/docs/audit-backlog.md @@ -39,7 +39,7 @@ _None outstanding._ |---|------|-----------|-----|-------------| | ✅H1 | IDOR write: `createAppointment` trusts request `doctor_uuid`, no scope check — any staff books onto any doctor's calendar | src/Appointment/Controller/MyAppointmentsController.php:59 | security-idor | **DONE** — `canBookForDoctor()` scope gate + `tests/Appointment/BookingScopeTest` | | ✅H2 | No UNIQUE `(doctor_id, slot_start)` on Appointment → double-booking race (index is non-unique) | src/Appointment/Entity/Appointment.php | db-unique | **DONE** — nullable unique `active_slot_key` (occupying = pending/confirmed, mirrors `isSlotTaken`); `bookAtomically` catches the unique violation + expires lapsed pendings in-txn; all 3 booking paths (online/my/admin) routed through it. Migration backfills one row per slot (non-destructive). `tests/Appointment/SlotUniquenessTest`. **NB:** backfill surfaced a real pre-existing double-booked slot in dev data (doctor 1764, two `expired` rows) — harmless (both expired = key NULL). | -| H3 | `Payment.referenceId` not unique → same gateway callback credited twice | src/Payment/Entity/Payment.php:61-62 | db-unique | Persist two Payments same reference_id → 2nd rejected. (pairs w/ C1) | +| ✅H3 | `Payment.referenceId` not unique → same gateway callback credited twice | src/Payment/Entity/Payment.php:61-62 | db-unique | **DONE** — unique index on `reference_id` (NULL until success → no collision) + callback pre-check rejects replays. `tests/Payment/PaymentCallbackAmountTest::testReplayedGatewayReferenceIsRejected` | | H4 | `FinancialBreakdown.payment` onDelete CASCADE on non-nullable FK → deleting a Payment destroys ledger rows; should be RESTRICT | src/Settlement/Entity/FinancialBreakdown.php:28-30 | db-ondelete | Delete a Payment w/ breakdown → expect FK restrict error, ledger preserved | | H5 | Insurance pricing/coverage modeled as raw int FKs (no FK/onDelete) → orphan rows on delete: `EntityInsurancePricing.entity_id/insurance_id`, `TenantInsurance.entity_id/insurance_id`, `TenantServiceCoverage.tenant_insurance_id` | src/Insurance/Entity/EntityInsurancePricing.php:25-29 · TenantInsurance.php:29,32 · TenantServiceCoverage.php:23-24 | db-ondelete | Delete insurance/tenant → children removed or restricted, no dangling rows | | H6 | N+1: clinic doctors list lazy-loads `specialties` per doctor | src/Clinic/Controller/ClinicController.php:326 · src/Doctor/Repository/DoctorRepository.php:49 | perf-nplus1 | SQL profiler on clinic doctors list → 1 query/doctor for specialties | diff --git a/migrations/Version20260628153625.php b/migrations/Version20260628153625.php new file mode 100644 index 00000000..5eb05adb --- /dev/null +++ b/migrations/Version20260628153625.php @@ -0,0 +1,31 @@ +addSql('CREATE UNIQUE INDEX UNIQ_65D29B321645DEA9 ON payments (reference_id)'); + } + + public function down(Schema $schema): void + { + // this down() migration is auto-generated, please modify it to your needs + $this->addSql('DROP INDEX UNIQ_65D29B321645DEA9 ON payments'); + } +} diff --git a/src/Payment/Controller/PaymentController.php b/src/Payment/Controller/PaymentController.php index 653e9488..b060210f 100644 --- a/src/Payment/Controller/PaymentController.php +++ b/src/Payment/Controller/PaymentController.php @@ -286,6 +286,19 @@ class PaymentController extends BaseController return $this->redirectToFrontend($payment, false); } + // A gateway reference identifies exactly one settled transaction. If it + // already belongs to another payment, this is a replay — reject it. The + // unique DB index on reference_id is the hard backstop behind this check. + if ($result->referenceId !== '') { + $owner = $this->paymentRepo->findByReferenceId($result->referenceId); + if ($owner !== null && $owner->getId() !== $payment->getId()) { + $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/Entity/Payment.php b/src/Payment/Entity/Payment.php index 12ef37c9..22f0f13a 100644 --- a/src/Payment/Entity/Payment.php +++ b/src/Payment/Entity/Payment.php @@ -58,7 +58,7 @@ class Payment #[ORM\Column(name: 'gateway_token', type: 'string', length: 255, nullable: true)] private ?string $gatewayToken = null; - #[ORM\Column(name: 'reference_id', type: 'string', length: 255, nullable: true)] + #[ORM\Column(name: 'reference_id', type: 'string', length: 255, nullable: true, unique: true)] private ?string $referenceId = null; #[ORM\Column(name: 'frontend_address', type: 'string', length: 500, nullable: true)] diff --git a/src/Payment/Repository/PaymentRepository.php b/src/Payment/Repository/PaymentRepository.php index 73dedf4d..8fc6121b 100644 --- a/src/Payment/Repository/PaymentRepository.php +++ b/src/Payment/Repository/PaymentRepository.php @@ -54,6 +54,11 @@ class PaymentRepository extends ServiceEntityRepository return $this->findOneBy(['orderId' => $orderId]); } + public function findByReferenceId(string $referenceId): ?Payment + { + return $this->findOneBy(['referenceId' => $referenceId]); + } + public function findPendingByAppointment(Appointment $appointment): ?Payment { return $this->findOneBy([ diff --git a/tests/Payment/PaymentCallbackAmountTest.php b/tests/Payment/PaymentCallbackAmountTest.php index 1b5b509a..56bffe7d 100644 --- a/tests/Payment/PaymentCallbackAmountTest.php +++ b/tests/Payment/PaymentCallbackAmountTest.php @@ -71,4 +71,32 @@ class PaymentCallbackAmountTest extends ApiTestCase $this->assertSame(Payment::STATUS_SUCCESS, $this->reload($payment)->getStatus()); } + + public function testReplayedGatewayReferenceIsRejected(): void + { + $this->enableTestMode(); + + // Unique per run — db_test is shared and not reset between runs. + $ref = 'REF-' . bin2hex(random_bytes(8)); + + $first = $this->makePayment(50000); + $this->fireCallbackWithRef($first, $ref, 50000); + $this->assertSame(Payment::STATUS_SUCCESS, $this->reload($first)->getStatus()); + + // Same gateway reference replayed onto a different (same-amount) order. + $second = $this->makePayment(50000); + $this->fireCallbackWithRef($second, $ref, 50000); + $this->assertSame(Payment::STATUS_FAILED, $this->reload($second)->getStatus()); + } + + private function fireCallbackWithRef(Payment $payment, string $refId, int $reportedAmount): void + { + $this->client->request('POST', '/api/v1/payment/callback/mock?' . http_build_query([ + 'order_id' => $payment->getOrderId(), + 'mock' => '1', + 'ResCode' => '0', + 'RefId' => $refId, + 'mock_amount' => (string) $reportedAmount, + ])); + } }