hamed/clinicpro
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(security): unique payment reference_id, reject replayed callbacks (H3)

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
hamed
2026-06-28 19:08:26 +03:30
co-authored by Claude Opus 4.8
parent aa87b4a9cb
commit 7fa4b55d3f
7 changed files with 80 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/api/payment.md
+1
View File
@@ -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.
docs/audit-backlog.md
+1 -1
View File
@@ -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 |