hamedandClaude Opus 4.8
2764e68e60
E5: generate phpstan-baseline.neon (the 41 pre-existing errors) and include it, so `phpstan analyse` returns OK and the gate now fails only on NEW errors. The baseline is meant to be burned down over time. E4: add .github/workflows/ci.yml — MariaDB 11.8 + redis services, composer install, JWT keygen, phpstan (baseline-clean), migrate-on-empty-db smoke, and phpunit. Locally verified the substantive checks: a fresh empty DB migrates cleanly to 64 tables (guards the "migrations break on empty DB" bug class), phpstan is green, and the 70-test suite passes. The GitHub Actions service wiring itself is first-run-pending (can't be exercised offline). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
126 lines
18 KiB
Markdown
126 lines
18 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 | **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 | **DONE** — onDelete RESTRICT + migration. `tests/Settlement/FinancialBreakdownIntegrityTest` |
|
||
| ✅H5 | Insurance pricing/coverage modeled as raw int FKs (no FK/onDelete) → orphan rows on delete | src/Insurance/Entity/EntityInsurancePricing.php · TenantInsurance.php · TenantServiceCoverage.php | db-ondelete | **DONE (entity-owner path)** — `entity_id` is polymorphic (doctor\|clinic) so no DB FK is possible; added `TenantInsuranceCleanupService::purgeForEntity()` wired into doctor + clinic DELETE (purges tenant_insurances + pricing + coverage). `tests/Insurance/TenantInsuranceCleanupTest`. **Residual (→ M20-adjacent):** orphans when an *insurance category* itself is deleted (`insurance_id` ref) or a *service_item* is deleted (`service_item_id` ref) — different deletion paths, lower freq. |
|
||
| ✅H6 | N+1: clinic doctors list lazy-loads `specialties` per doctor | src/Doctor/Repository/DoctorRepository.php (findByClinicWithFilters) | perf-nplus1 | **DONE** — `addSelect('s')` + `Paginator(fetchJoinCollection:true)`. Added `ApiTestCase::countQueries()` helper. `tests/Doctor/ClinicDoctorListNPlusOneTest` (query count constant vs doctor count; verified 4→10 without fix). |
|
||
| ✅H7 | N+1: comments list lazy-loads `likes`→`user`, `replies` (recursive), author realName | src/Rating/Repository/CommentRepository.php (findApprovedRootsByDoctor) | perf-nplus1 | **DONE** — two fetch-join passes (roots+user+likes; replies+their user+likes+one more reply level) hydrate everything `toArray()` touches → bounded queries for a 2-level thread. Functional correctness test `tests/Rating/CommentListNPlusOneTest` (like counts, approved-only replies). Query-count assertion unreliable through HTTP here, so correctness-tested. |
|
||
| ✅H8 | N+1: insurance service-coverage `serviceItemRepo->find()` per row in array_map | src/Insurance/Controller/InsuranceController.php:406-420 | perf-nplus1 | **DONE** — batch `findBy(['id' => $ids])` + uuid map. Verified by functional correctness test (`tests/Insurance/ServiceCoverageNPlusOneTest`); query-count assertion was unreliable for this endpoint (identity-map), so correctness-tested instead. |
|
||
| ✅H9 | Unbounded list: `listClaims` loads ALL tenant claims, no LIMIT/pagination | src/Billing/Controller/BillingController.php · ClaimRepository.php | perf-pagination | **DONE** — `findByTenant(page,limit)` + `countByTenant` (shared QB), default limit 50/max 100. Additive `data.meta` (envelope unchanged → backward compatible). `tests/Billing/ClaimsListPaginationTest`. |
|
||
| ✅H10 | Unbounded list: `wallet/transactions` ~~loads ALL~~ user transactions | src/Settlement/Controller/SettlementController.php · WalletTransactionRepository.php | perf-pagination | **DONE** — **finding overstated**: `findByUser` already defaulted to `limit=50` (bounded, just page-less). Added `page`/`offset` + `countByUser` + `data.meta`. `tests/Settlement/WalletTransactionsPaginationTest`. |
|
||
|
||
---
|
||
|
||
## ☐ 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~~ | src/Billing/Controller/BillingController.php | security-massassign | **DONE** — status-jump part was already guarded by `canTransitionTo` (false alarm). Added bounds: approved ∈ [0, claimed], paid ∈ [0, approved] → 422. `tests/Billing/ClaimAmountBoundsTest`. |
|
||
| ✅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 | **DONE** — auth+ownership (owner/admin). `tests/Insurance/DoctorInsuranceOwnershipTest` |
|
||
| ✅M3 | IDOR read: `showAddress` loads any DoctorAddress by id, no owner check | src/Doctor/Controller/DoctorController.php:559 | security-idor | **DONE** — ownership mirror of PATCH/DELETE. `tests/Doctor/DoctorAddressOwnershipTest` |
|
||
| ✅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 | **DONE** — staff must match tenant (entity_type/id) → 422. `tests/ClinicService/ServiceItemStaffOwnershipTest` |
|
||
| ✅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 | **DONE** — ownership added to listOverrides/listHolidays/availableLocations. `tests/Appointment/AppointmentSettingsListOwnershipTest` |
|
||
| ✅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 | **DONE** — per-mobile limiter (5/hr) added alongside per-IP. `tests/Auth/SendCodeMobileRateLimitTest` (6 reqs / 6 IPs → 6th 429). |
|
||
| ✅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 | **DONE** — single-use rotation (revoke old + issue new) + suspended-user (status!=1) rejected. `tests/Auth/RefreshTokenRotationTest`. |
|
||
| ✅M8 | N+1: secretary list lazy-loads secretary/doctor ManyToOne per row | src/Secretary/Controller/SecretaryController.php:192,213 | perf-nplus1 | **DONE** — fetch-join secretary/doctor/clinic. `tests/Secretary/SecretaryListNPlusOneTest` |
|
||
| ✅M9 | N+1: billing claims lazy `items` + `insuranceRepo->find()` per claim in enrichClaims | src/Billing/Controller/BillingController.php:~49,68 | perf-nplus1 | **DONE** — fetch-join items (Paginator) + batch insurance names. `tests/Billing/ClaimsListNPlusOneTest` |
|
||
| ✅M10 | Unbounded list: `listMine` settlements `findByUser` no limit | src/Settlement/Controller/SettlementController.php:193 | perf-pagination | **DONE** — page/limit + countByUser + data.meta. `tests/Settlement/SettlementListPaginationTest` |
|
||
| ✅M11 | Unbounded list: admin `pendingComments` `findPending` no limit | src/Rating/Controller/RatingController.php:350 | perf-pagination | **DONE** — findPending(limit,offset)+countPending+meta. `tests/Rating/CommentPaginationTest::testAdminPendingListPaginates` |
|
||
| ✅M12 | Unbounded list: public `listComments` per-doctor no limit | src/Rating/Controller/RatingController.php:270 | perf-pagination | **DONE** — Paginator(fetchJoinCollection) page/limit + countApprovedRootsByDoctor + meta. `tests/Rating/CommentPaginationTest::testPublicListPaginates` |
|
||
| ⚠️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 | **DONE** — added composite `idx_wallet_user_type` + migration. `InfraSmokeTest::testWalletUserTypeIndexExists`. |
|
||
| ⚠️M15 | Missing index: `ClinicDoctorInvitation.doctor_id` FK unindexed | src/ClinicInvitation/Entity/ClinicDoctorInvitation.php:41-42 | perf-index | **FALSE POSITIVE** — `doctor_id` has a FK → auto-indexed (IDX_26DCCFEB87F4FB17 exists). No change. |
|
||
| ✅M16 | `User.email` and `User.nationalCode` not unique → duplicate identities | src/Auth/Entity/User.php:31-32,37-38 | db-unique | **DONE** — unique on email + national_code (NULLs ok; no dup data). `tests/Database/UniqueConstraintsTest` |
|
||
| ✅M17 | `Payment.gatewayToken` not unique | src/Payment/Entity/Payment.php:58-59 | db-unique | **DONE** — unique gateway_token. UniqueConstraintsTest |
|
||
| ✅M18 | `DateOverride` no UNIQUE `(doctor_id, date)` → ambiguous schedule | src/Appointment/Entity/DateOverride.php:12 | db-unique | **DONE** — UNIQUE(doctor_id,date). UniqueConstraintsTest |
|
||
| ✅M19 | `FinancialBreakdown` no unique `(payment_id, source)` → double-accounting | src/Settlement/Entity/FinancialBreakdown.php:28-33 | db-unique | **DONE** — UNIQUE(payment_id,source). UniqueConstraintsTest |
|
||
| ✅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 | **DONE (config path)** — ServiceItem delete now purges its Tariff + TenantServiceCoverage rows (raw-int refs). `tests/ClinicService/ServiceItemDeleteCleanupTest`. **Residual (accepted):** ClinicStaff/SmsWallet/DoctorAddress.clinicId/Claim.insurance_id orphans on rare owner deletions are harmless unreferenced rows (no integrity/security impact); SmsWallet deliberately NOT auto-deleted (financial record). |
|
||
| ✅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 | **DONE (wire-safe)** — registered 14 legacy codes as constants in ErrorCodes.php (values unchanged so clients unaffected) + messages; replaced raw strings across Admin/MyAppointments/Category/PreRegistration/ClinicInvitation. `tests/Shared/ErrorCodesTest`. |
|
||
|
||
---
|
||
|
||
## ☐ LOW
|
||
|
||
> **Low-tier triage (2026-06-28):** ✅ **L1** (session patient-gate) + **L11** (201 on pre-registration/IBAN) fixed — `tests/Audit/LowTierFixesTest`. Verified against the live schema/code and **accepted without change** (proportionate): **L8** false positive (`doctor_secretaries.clinic_id` already FK-auto-indexed); **L6/L7/L9** marginal composite/sort indexes on small/rare result sets (same call as M13/M15); **L4/L5** small bounded N+1 (per-session services / per-clinic addresses, typically <10); **L10** admin plans count in PHP (tiny table); **L2** OTP per-uuid lockout (now partly mitigated by the M6 per-mobile cap); **L3** `payment_test_mode` is an ops/config concern (confirm never enabled in prod, not a code bug); **L12** assorted nullable gaps. None carry security/integrity impact.
|
||
|
||
| # | 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 | **DONE (scaffold, first-run-pending)** — `.github/workflows/ci.yml`: MariaDB+redis services, phpstan (baseline-clean), migrate-on-empty-db smoke, phpunit. Verified locally: empty-DB migrate → 64 tables, phpstan OK, 70 tests green. GH-Actions service wiring needs first-run validation. |
|
||
| ✅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 | **DONE** — generated `phpstan-baseline.neon` (41 errors) + included it; `phpstan analyse` now returns OK, so the gate catches only NEW errors. Burn down over time. |
|
||
| E6 | **No test DB isolation** — `ApiTestCase` doesn't reset/rollback `db_test` between tests/runs, so rows accumulate; count/time-based assertions are fragile (hit twice this session). Add per-test transaction rollback or a DB reset. | test | tests rely on random keys + relaxed assertions as a workaround |
|
||
|
||
---
|
||
|
||
## 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 (pre-audit-session): 10.
|
||
|
||
## Progress (this audit session)
|
||
- ✅ **CRITICAL: 1/1** (C1)
|
||
- ✅ **HIGH: 10/10** (H1–H10) — the prompt's hard completion gate (no Critical/High remaining) is **MET**.
|
||
- ☐ MEDIUM: 0/21 · ☐ LOW: 0/12 · ☐ Epics E1–E6.
|
||
- 11 task-commits, each with a regression test + docs; full suite **36 tests / 82 assertions green**; phpstan adds **0 new** errors over the 41-error baseline (E5).
|
||
</content>
|
||
</invoke>
|