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

docs(audit): durable backlog — 5-dimension scan, 70 findings prioritized

Browse Source 
Persists the audit plan so a session restart no longer loses it (TodoWrite
is volatile). 10 done, 1 critical, 10 high, 21 medium, 12 low, 4 epics.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
hamed
2026-06-28 18:44:25 +03:30
co-authored by Claude Opus 4.8
parent 6bd49c2d3e
commit 8e5cd51873
1 changed files with 116 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/audit-backlog.md
+116
View File
@@ -0,0 +1,116 @@
# 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 |
---
## ☐ 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 |
---
## ☐ 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 | POST `/api/v1/my/appointment` w/ unrelated doctor_uuid → expect 403 |
| H2 | No UNIQUE `(doctor_id, slot_start)` on Appointment → double-booking race (index is non-unique) | src/Appointment/Entity/Appointment.php:13 | db-unique | Concurrent POST same doctor+slot → only one persists. ⚠️ check existing dup data before adding constraint |
| 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 |
---
## 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>