hamedandClaude Opus 4.8
aa87b4a9cb
A non-unique index on (doctor_id, slot_start) plus a count-then-insert check left a TOCTOU race: two concurrent requests could both pass isSlotTaken and both insert. wrapInTransaction alone doesn't stop the phantom under InnoDB REPEATABLE-READ. Add a nullable, unique active_slot_key on Appointment = "doctorId:slotStart" while the booking occupies the slot (pending/confirmed — in lockstep with isSlotTaken); NULL once expired/completed/no_show/cancelled (NULLs don't collide in a MySQL unique index, so released slots rebook freely). bookAtomically now: catches the unique violation -> SlotTakenException, and expires lapsed pendings in-transaction so the ~1-min window before the expiry cron doesn't wrongly block rebooking. All three booking paths (online / my / admin) routed through it. Migration backfills one row per (doctor, slot) — the latest id — so the index builds even on dirty historical data without destructively cancelling bookings. (Backfill surfaced a real pre-existing double-booked slot in dev data.) Regression: tests/Appointment/SlotUniquenessTest. Adjusted the expiry-service test fixture to use distinct slots (one live booking per slot is now enforced). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
117 lines
14 KiB
Markdown
117 lines
14 KiB
Markdown
# Backend Audit Backlog — ClinicPro
|
|
|
|
> Durable backlog for the `backend-audit` branch (prompt: `.claude/prompt/full-backend-audit.md`).
|
|
> This file is the source of truth — `TodoWrite` is volatile and does NOT survive a session restart.
|
|
> Status legend: ✅ done · ▶ doing · ☐ todo. Order: Critical → High → Medium → Low.
|
|
> Every fix needs a regression test that fails without the fix and passes with it.
|
|
|
|
Last full scan: 2026-06-28 (5 parallel investigators: idor/massassign, auth-surface, api/quality, db-integrity, perf).
|
|
|
|
---
|
|
|
|
## ✅ DONE (committed on backend-audit)
|
|
|
|
| # | Task | Cat | Commit |
|
|
|---|------|-----|--------|
|
|
| D1 | Functional test infra (`ApiTestCase` + `db_test` + JWT helpers) | test | 30c5fbe |
|
|
| D2 | `repositoryClass` on all entities w/ custom repo | orm | 05fde81 |
|
|
| D3 | regression: every custom-repo entity maps its repositoryClass | test | 372bea4 |
|
|
| D4 | IDOR — ownership on GET weekly-schedule | security | 90736c8 |
|
|
| D5 | IDOR — ownership on GET date-override (single) | security | 0932930 |
|
|
| D6 | onDelete on required FKs (Clinic, Doctor, ClinicDoctorInvitation, DoctorInsurance, DoctorSecretary, ClinicSubscription, SubscriptionPeriod) | db | 36b87e9 |
|
|
| D7 | N+1 — batch-fetch pending payments in expiry loop | perf | e174046 |
|
|
| 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
|
|
|
|
_None outstanding._
|
|
|
|
---
|
|
|
|
## ☐ HIGH
|
|
|
|
| # | Task | File:line | Cat | How to test |
|
|
|---|------|-----------|-----|-------------|
|
|
| ✅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) |
|
|
| 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 |
|
|
| H7 | N+1: comments list lazy-loads `likes`→`user`, `replies` (recursive), author realName | src/Rating/Controller/RatingController.php:278,352 | perf-nplus1 | Profiler GET comments → query count scales w/ comments |
|
|
| H8 | N+1: insurance service-coverage `serviceItemRepo->find()` per row in array_map | src/Insurance/Controller/InsuranceController.php:410 | perf-nplus1 | GET service-coverage → 1 find()/row |
|
|
| H9 | Unbounded list: `listClaims` loads ALL tenant claims, no LIMIT/pagination | src/Billing/Controller/BillingController.php:173 · ClaimRepository.php:64 | perf-pagination | GET claims high volume → must paginate |
|
|
| H10 | Unbounded list: `wallet/transactions` loads ALL user transactions, no LIMIT | src/Settlement/Controller/SettlementController.php:89-94 | perf-pagination | GET wallet transactions → must paginate |
|
|
|
|
---
|
|
|
|
## ☐ MEDIUM
|
|
|
|
| # | Task | File:line | Cat | How to test |
|
|
|---|------|-----------|-----|-------------|
|
|
| M1 | Mass-assign: claim approve/pay lets owning tenant set arbitrary `approved_rials`/`paid_rials` + force status PAID/APPROVED | src/Billing/Controller/BillingController.php:192 | security-massassign | POST claim approve w/ huge amounts → server computes, ignores caller figures |
|
|
| M2 | IDOR read: `GET /insurance/{id}` (showDoctorInsurance) leaks any doctor's insurance incl. negotiated price by id enumeration | src/Insurance/Controller/InsuranceController.php:499 | security-idor | GET insurance/{id} as non-owner → 403 |
|
|
| M3 | IDOR read: `showAddress` loads any DoctorAddress by id, no owner check | src/Doctor/Controller/DoctorController.php:559 | security-idor | GET doctor-address/{id} as non-owner → 403 |
|
|
| M4 | IDOR write: ClinicService `createItem`/`updateItem` bind staff via global `staffRepo->findByUuid()`, no entity-scope check (cross-tenant staff binding) | src/ClinicService/Controller/ClinicServiceController.php:157,195 | security-idor | Bind other clinic's staff_uuid → 403/validation |
|
|
| M5 | IDOR read: AppointmentSettings list endpoints leak any doctor's config — `listOverrides`, `listHolidays`, `availableLocations` (no ownership on {doctorUuid}) | src/Appointment/Controller/AppointmentSettingsController.php:150,256,349 | security-idor | GET each list for unowned doctor → 403 |
|
|
| M6 | OTP send-code has no per-mobile/per-uuid cap, only per-IP (5/hr) → SMS flood from rotating IPs | src/Auth/Controller/AuthController.php:136-153 · OtpService.php:59-77 · rate_limiter.yaml:4-7 | security-ratelimit | Request many codes one mobile across IPs → per-number cap enforced |
|
|
| M7 | Refresh token not rotated on use (same raw token 30d), never re-checks user status | src/Auth/Controller/AuthController.php:477-480 · TokenService.php:33-45 | security-auth | Call refresh twice → new token issued each time, suspended user rejected |
|
|
| M8 | N+1: secretary list lazy-loads secretary/doctor ManyToOne per row | src/Secretary/Controller/SecretaryController.php:192,213 | perf-nplus1 | Profiler secretary list → ~2 queries/row |
|
|
| M9 | N+1: billing claims lazy `items` + `insuranceRepo->find()` per claim in enrichClaims | src/Billing/Controller/BillingController.php:~49,68 | perf-nplus1 | GET claims → items+insurance query/claim |
|
|
| M10 | Unbounded list: `listMine` settlements `findByUser` no limit | src/Settlement/Controller/SettlementController.php:193 | perf-pagination | GET settlement list → paginate |
|
|
| M11 | Unbounded list: admin `pendingComments` `findPending` no limit | src/Rating/Controller/RatingController.php:350 | perf-pagination | admin moderation large backlog → paginate |
|
|
| M12 | Unbounded list: public `listComments` per-doctor no limit | src/Rating/Controller/RatingController.php:270 | perf-pagination | popular doctor comments → paginate |
|
|
| M13 | Missing index: `ServiceItem.section_id` FK fully unindexed (entity has zero indexes) | src/ClinicService/Entity/ServiceItem.php:23-24 | perf-index | EXPLAIN findBySection → no full scan |
|
|
| M14 | Missing index: `WalletTransaction(user_id, type)` composite for per-type SUM | src/Settlement/Entity/WalletTransaction.php:38-39 | perf-index | EXPLAIN balance SUM query |
|
|
| M15 | Missing index: `ClinicDoctorInvitation.doctor_id` FK unindexed | src/ClinicInvitation/Entity/ClinicDoctorInvitation.php:41-42 | perf-index | EXPLAIN findPendingByDoctor |
|
|
| M16 | `User.email` and `User.nationalCode` not unique → duplicate identities | src/Auth/Entity/User.php:31-32,37-38 | db-unique | Insert two users same email/national_code → 2nd rejected. ⚠️ check dup data first |
|
|
| M17 | `Payment.gatewayToken` not unique | src/Payment/Entity/Payment.php:58-59 | db-unique | Two payments same gateway_token → rejected |
|
|
| M18 | `DateOverride` no UNIQUE `(doctor_id, date)` → ambiguous schedule | src/Appointment/Entity/DateOverride.php:12 | db-unique | Two overrides same doctor+date → rejected |
|
|
| M19 | `FinancialBreakdown` no unique `(payment_id, source)` → double-accounting | src/Settlement/Entity/FinancialBreakdown.php:28-33 | db-unique | Two breakdowns same payment+source → rejected |
|
|
| M20 | Soft-ref FKs orphan (Billing/ClinicService): `ClaimItem.invoice_item_id`, `Claim.insurance_id`, `Tariff.service_item_id`, `TenantServiceCoverage.service_item_id`, `DoctorAddress.clinicId`, polymorphic `SmsWallet`, `ClinicStaff` | src/Billing/Entity/ClaimItem.php:22 · Claim.php:48 · ClinicService/Entity/Tariff.php:23 · Insurance/Entity/TenantServiceCoverage.php:26 · Doctor/Entity/DoctorAddress.php:32 · Sms/Entity/SmsWallet.php:18-22 · Staff/Entity/ClinicStaff.php:22-26 | db-ondelete | Delete parent → child handled (cascade/restrict/set-null), no orphan |
|
|
| M21 | ~30 ad-hoc raw error codes not in ErrorCodes.php (USER_NOT_FOUND, INVALID_ROLE, SLOT_TAKEN, DUPLICATE_REQUEST, ERR_GONE, ERR_ACCESS_DENIED, …) → `message()` returns "خطای ناشناخته" | src/Admin/Controller/AdminApiController.php (many) · MyAppointmentsController.php:41-80 · PreRegistrationController.php:66 · CategoryController.php:36-40 · ClinicInvitationController.php:203 | quality-errorcodes | Trigger each → code present in ErrorCodes.php |
|
|
|
|
---
|
|
|
|
## ☐ LOW
|
|
|
|
| # | Task | File:line | Cat |
|
|
|---|------|-----------|-----|
|
|
| L1 | IDOR: `updateSession` skips `assertPatientGate` (ownership OK, gate inconsistent) | src/Patient/Controller/PatientController.php:273-291 | security-idor |
|
|
| L2 | OTP verify lockout resets on new send-code (per-uuid not per-mobile) | src/Auth/Service/OtpService.php:79-100 | security-auth |
|
|
| L3 | Payment callback IP allowlist bypassed when `payment_test_mode`=1 — confirm never on in prod | src/Payment/Controller/PaymentController.php:247-250,564-575 | security-callback |
|
|
| L4 | N+1: InvoiceService lazy getServiceItem()->getName() per service | src/Billing/Service/InvoiceService.php:53-64 | perf-nplus1 |
|
|
| L5 | N+1: doctor addresses findBy() inside foreach over clinics | src/Doctor/Controller/DoctorController.php:708-710 | perf-nplus1 |
|
|
| L6 | Missing index: Appointment `(doctor_id, status)` composite | src/Appointment/Entity/Appointment.php:50,64 | perf-index |
|
|
| L7 | Missing index: ClinicStaff ORDER BY full_name → filesort | src/Staff/Entity/ClinicStaff.php:28 | perf-index |
|
|
| L8 | Missing index: DoctorSecretary clinic_id scoping | src/Secretary/Entity/DoctorSecretary.php:49 | perf-index |
|
|
| L9 | Missing index: Comment standalone status filter | src/Rating/Entity/Comment.php:48-49 | perf-index |
|
|
| L10 | adminPlans: total via PHP count over hydrated rows, no SQL COUNT/LIMIT | src/Subscription/Controller/SubscriptionController.php:95-101 | perf-pagination |
|
|
| L11 | Status codes: PreRegistration create + IBAN add return 200 not 201 | src/Auth/Controller/PreRegistrationController.php:73 · Representation/Controller/RepresentationActionController.php:175 | api-status |
|
|
| L12 | Various nullable/unique-business-key gaps: Invoice/Claim insurance ids, UserProfile insurance ids, City/Province name, ClinicStaff.nationalCode, MobileVerificationOtp, SmsSettings, SmsLog.templateUuid | (see db scan) | db-unique/ondelete |
|
|
|
|
---
|
|
|
|
## ☐ EPICS (design debt — cross-repo, defer; do NOT quick-fix)
|
|
|
|
| # | Task | Scope | Note |
|
|
|---|------|-------|------|
|
|
| E1 | **Double-nest response contract** — ~12 controllers return `success(['data'=>X])` → `{data:{data:X}}`, others flat. Inconsistent envelope. | AppointmentSettings, Appointment, Rating, Settlement, Secretary, Sms, Location, Doctor, Blog, DoctorService/Specialty/Tag, Representation, UserProfile, Clinic, Insurance, Billing | Documented pitfall in CLAUDE.md; clients (`nobat724_front`, `clinic-pro-tauri`) already read `data.data`. Unifying breaks all 3 clients → needs coordinated cross-repo change + versioning. NOT a quick bug. |
|
|
| E2 | **No DTO/validator on sensitive writes** — admin create, auth flows, payment verify, booking read raw `json_decode` arrays | Admin, Auth, Payment, Appointment, Settlement controllers | Introduce request DTOs + validator incrementally. Large. |
|
|
| E3 | **Fat controllers** — AdminApiController (1938 LOC), MyAppointmentsController booking, RepresentationActionController (835), DoctorController/ClinicController | extract per-domain Services | SOLID refactor; lower urgency than security/db. |
|
|
| E4 | **CI** — no `.github/workflows`; add phpunit + phpstan + migrate-on-empty-db | devops | prompt var §DevOps |
|
|
| E5 | **phpstan baseline dirty** — 41 pre-existing errors across the codebase (D9 only repaired the config so it *runs*). Audit fixes must not add new ones; cleaning the 41 is its own task. | devops | `ddev exec php vendor/bin/phpstan analyse` → 41 errors (e.g. SlotCalculatorService.php:233, SubscriptionController.php:33) |
|
|
|
|
---
|
|
|
|
## Tallies
|
|
- Security: 16 (1 critical, 1 high IDOR-write, rest med/low) · Performance: 20 · DB-integrity: 33 · API/quality: 28 (mostly E1 nesting) · DevOps: 1 epic.
|
|
- Already fixed: 10.
|
|
</content>
|
|
</invoke>
|