From fe6383314e0fbfdfc6e8bc519606272910655f76 Mon Sep 17 00:00:00 2001 From: hamed <15238-genius.ha@users.noreply.drupalcode.org> Date: Sun, 28 Jun 2026 20:17:52 +0330 Subject: [PATCH] fix(security): enforce ownership on 4 IDOR read/bind endpoints (M2-M5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - M2 GET /insurance/{id}: was unguarded; now owner-or-admin (403 otherwise) — stops reading another doctor's negotiated price by id enumeration. - M3 GET /clinic-pro/doctor-address/{id}: add the same owner/admin check the sibling PATCH/DELETE already had. - M4 POST/PATCH /service-item: staff_uuid must belong to the caller's tenant (entity_type/entity_id) → 422; stops binding another tenant's staff. - M5 appointment-settings list endpoints (date-override/holidays/ available-locations): add the per-doctor ownership check the sibling single-record endpoints already enforce. Regressions (6 negative cases fail without the fixes): DoctorInsuranceOwnershipTest, DoctorAddressOwnershipTest, ServiceItemStaffOwnershipTest, AppointmentSettingsListOwnershipTest. Co-Authored-By: Claude Opus 4.8 --- docs/api/appointment-settings.md | 6 +-- docs/api/clinic-services.md | 2 + docs/api/insurance.md | 2 +- docs/audit-backlog.md | 8 +-- .../AppointmentSettingsController.php | 18 +++++-- .../Controller/ClinicServiceController.php | 13 +++-- src/Doctor/Controller/DoctorController.php | 6 ++- .../Controller/InsuranceController.php | 7 ++- .../AppointmentSettingsListOwnershipTest.php | 52 +++++++++++++++++++ .../ServiceItemStaffOwnershipTest.php | 44 ++++++++++++++++ tests/Doctor/DoctorAddressOwnershipTest.php | 45 ++++++++++++++++ .../DoctorInsuranceOwnershipTest.php | 49 +++++++++++++++++ 12 files changed, 236 insertions(+), 16 deletions(-) create mode 100644 tests/Appointment/AppointmentSettingsListOwnershipTest.php create mode 100644 tests/ClinicService/ServiceItemStaffOwnershipTest.php create mode 100644 tests/Doctor/DoctorAddressOwnershipTest.php create mode 100644 tests/Insurance/DoctorInsuranceOwnershipTest.php diff --git a/docs/api/appointment-settings.md b/docs/api/appointment-settings.md index 7d58d896..b2e01bd8 100644 --- a/docs/api/appointment-settings.md +++ b/docs/api/appointment-settings.md @@ -248,7 +248,7 @@ Override a specific date — mark it inactive (day off) or give it custom sessio Get all date overrides for a doctor. -**Permission:** `AUTH` (class-level) +**Permission:** `AUTH` — must be the owning doctor or `ROLE_ADMIN` (`403 ERR_AUTH_006` otherwise). ### Response `200` ```json @@ -425,7 +425,7 @@ Mark a date range as holiday — all slots blocked, no overrides apply. Get all holidays for a doctor. -**Permission:** `AUTH` (class-level) +**Permission:** `AUTH` — must be the owning doctor or `ROLE_ADMIN` (`403 ERR_AUTH_006` otherwise). ### Response `200` ```json @@ -573,7 +573,7 @@ The `SlotCalculatorService` calculates available slots in this priority order: ### `GET /api/v1/appointment-settings/available-locations/{doctorUuid}` -**Permission:** Public +**Permission:** `AUTH` — must be the owning doctor or `ROLE_ADMIN` (`403 ERR_AUTH_006` otherwise). Returns all locations a doctor can assign as `location_id` in their schedule sessions. Includes both the doctor's personal addresses and the addresses of all clinics they belong to. diff --git a/docs/api/clinic-services.md b/docs/api/clinic-services.md index c724acf8..2b646983 100644 --- a/docs/api/clinic-services.md +++ b/docs/api/clinic-services.md @@ -147,6 +147,8 @@ } ``` +> `staff_uuid` باید به پرسنل متعلق به همان tenant (`entity_type`/`entity_id` کاربر) اشاره کند؛ ربط‌دادن پرسنل tenant دیگر → `422 ERR_VALIDATION_001` (`field: staff_uuid`). همین قید روی `POST /service-item` نیز اعمال می‌شود. + --- ## DELETE /api/v1/service-item/{uuid} diff --git a/docs/api/insurance.md b/docs/api/insurance.md index c8ab9992..c11ae445 100644 --- a/docs/api/insurance.md +++ b/docs/api/insurance.md @@ -225,7 +225,7 @@ Add an insurance to a doctor's accepted list. Get a doctor-insurance link. -**Permission:** `PUBLIC` +**Permission:** `AUTH` — must be the owning doctor or `ROLE_ADMIN` (otherwise `403 ERR_AUTH_006`). Prevents reading another doctor's negotiated price by id enumeration. ### Response `200` DoctorInsurance object. diff --git a/docs/audit-backlog.md b/docs/audit-backlog.md index 9edd9dd9..a07dabe0 100644 --- a/docs/audit-backlog.md +++ b/docs/audit-backlog.md @@ -55,10 +55,10 @@ _None outstanding._ | # | 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 | 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 | +| ✅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 | 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 | diff --git a/src/Appointment/Controller/AppointmentSettingsController.php b/src/Appointment/Controller/AppointmentSettingsController.php index 0a0e0d8a..bcb22539 100644 --- a/src/Appointment/Controller/AppointmentSettingsController.php +++ b/src/Appointment/Controller/AppointmentSettingsController.php @@ -148,13 +148,17 @@ class AppointmentSettingsController extends BaseController // ── Date Overrides ──────────────────────────────────────────────────────── #[Route('/api/v1/appointment-settings/date-override/list/{doctorUuid}', methods: ['GET'])] - public function listOverrides(string $doctorUuid): JsonResponse + public function listOverrides(string $doctorUuid, #[CurrentUser] User $user): JsonResponse { $doctor = $this->doctorRepo->findByUuid($doctorUuid); if ($doctor === null) { return $this->error(ErrorCodes::ERR_VALIDATION_002, 'دکتر یافت نشد', 404); } + if ($doctor->getUser()->getId() !== $user->getId() && !$user->hasRole('ROLE_ADMIN')) { + return $this->error(ErrorCodes::ERR_AUTH_006, 'دسترسی ممنوع', 403); + } + $overrides = array_map( fn(DateOverride $o) => $o->toArray(), $this->overrideRepo->findByDoctor($doctor) @@ -254,13 +258,17 @@ class AppointmentSettingsController extends BaseController // ── Holidays ────────────────────────────────────────────────────────────── #[Route('/api/v1/appointment-settings/holidays/list/{doctorUuid}', methods: ['GET'])] - public function listHolidays(string $doctorUuid): JsonResponse + public function listHolidays(string $doctorUuid, #[CurrentUser] User $user): JsonResponse { $doctor = $this->doctorRepo->findByUuid($doctorUuid); if ($doctor === null) { return $this->error(ErrorCodes::ERR_VALIDATION_002, 'دکتر یافت نشد', 404); } + if ($doctor->getUser()->getId() !== $user->getId() && !$user->hasRole('ROLE_ADMIN')) { + return $this->error(ErrorCodes::ERR_AUTH_006, 'دسترسی ممنوع', 403); + } + $items = array_map(fn(Holiday $h) => $h->toArray(), $this->holidayRepo->findAllByDoctor($doctor)); return $this->success(['data' => $items]); @@ -347,13 +355,17 @@ class AppointmentSettingsController extends BaseController // ── Available Locations ─────────────────────────────────────────────────── #[Route('/api/v1/appointment-settings/available-locations/{doctorUuid}', methods: ['GET'])] - public function availableLocations(string $doctorUuid): JsonResponse + public function availableLocations(string $doctorUuid, #[CurrentUser] User $user): JsonResponse { $doctor = $this->doctorRepo->findByUuid($doctorUuid); if ($doctor === null) { return $this->error(ErrorCodes::ERR_VALIDATION_002, 'دکتر یافت نشد', 404); } + if ($doctor->getUser()->getId() !== $user->getId() && !$user->hasRole('ROLE_ADMIN')) { + return $this->error(ErrorCodes::ERR_AUTH_006, 'دسترسی ممنوع', 403); + } + $clinics = $this->clinicRepo->findByDoctor($doctor); $clinicIds = array_map(fn(Clinic $c) => $c->getId(), $clinics); $clinicMap = []; diff --git a/src/ClinicService/Controller/ClinicServiceController.php b/src/ClinicService/Controller/ClinicServiceController.php index 29205382..f6f8dce6 100644 --- a/src/ClinicService/Controller/ClinicServiceController.php +++ b/src/ClinicService/Controller/ClinicServiceController.php @@ -156,9 +156,10 @@ class ClinicServiceController extends BaseController if (!empty($data['staff_uuid'])) { $staff = $this->staffRepo->findByUuid($data['staff_uuid']); - if ($staff !== null) { - $item->setStaff($staff); + if ($staff === null || $staff->getEntityType() !== $entityType || $staff->getEntityId() !== $entityId) { + return $this->error(ErrorCodes::ERR_VALIDATION_001, 'پرسنل انتخاب‌شده متعلق به شما نیست', 422, 'staff_uuid'); } + $item->setStaff($staff); } if (isset($data['insurance_covered'])) { @@ -193,7 +194,13 @@ class ClinicServiceController extends BaseController if (isset($data['price_rials'])) { $item->setPriceRials((int) $data['price_rials']); $priceChanged = true; } if (isset($data['active'])) { $item->setActive((bool) $data['active']); } if (array_key_exists('staff_uuid', $data)) { - $staff = $data['staff_uuid'] ? $this->staffRepo->findByUuid($data['staff_uuid']) : null; + $staff = null; + if ($data['staff_uuid']) { + $staff = $this->staffRepo->findByUuid($data['staff_uuid']); + if ($staff === null || $staff->getEntityType() !== $entityType || $staff->getEntityId() !== $entityId) { + return $this->error(ErrorCodes::ERR_VALIDATION_001, 'پرسنل انتخاب‌شده متعلق به شما نیست', 422, 'staff_uuid'); + } + } $item->setStaff($staff); } if (isset($data['insurance_covered'])) { diff --git a/src/Doctor/Controller/DoctorController.php b/src/Doctor/Controller/DoctorController.php index da818025..1201e4ae 100644 --- a/src/Doctor/Controller/DoctorController.php +++ b/src/Doctor/Controller/DoctorController.php @@ -560,13 +560,17 @@ class DoctorController extends BaseController )] #[Route('/api/v1/clinic-pro/doctor-address/{id}', methods: ['GET'])] #[IsGranted('IS_AUTHENTICATED_FULLY')] - public function showAddress(int $id): JsonResponse + public function showAddress(int $id, #[CurrentUser] User $user): JsonResponse { $address = $this->addressRepo->find($id); if ($address === null) { return $this->error(ErrorCodes::ERR_VALIDATION_002, 'آدرس یافت نشد', 404); } + if ($address->getDoctor()?->getUser()->getId() !== $user->getId() && !$user->hasRole('ROLE_ADMIN')) { + return $this->error(ErrorCodes::ERR_AUTH_006, 'دسترسی ممنوع', 403); + } + return $this->success(['data' => $address->toArray()]); } diff --git a/src/Insurance/Controller/InsuranceController.php b/src/Insurance/Controller/InsuranceController.php index 20099230..34578846 100644 --- a/src/Insurance/Controller/InsuranceController.php +++ b/src/Insurance/Controller/InsuranceController.php @@ -506,13 +506,18 @@ class InsuranceController extends BaseController } #[Route('/api/v1/insurance/{id}', methods: ['GET'])] - public function showDoctorInsurance(int $id): JsonResponse + #[IsGranted('IS_AUTHENTICATED_FULLY')] + public function showDoctorInsurance(int $id, #[CurrentUser] User $user): JsonResponse { $doctorInsurance = $this->doctorInsuranceRepo->find($id); if ($doctorInsurance === null) { return $this->error(ErrorCodes::ERR_VALIDATION_002, 'بیمه پزشک یافت نشد', 404); } + if ($doctorInsurance->getDoctor()->getUser()->getId() !== $user->getId() && !$user->hasRole('ROLE_ADMIN')) { + return $this->error(ErrorCodes::ERR_AUTH_006, 'دسترسی ممنوع', 403); + } + return $this->success(['data' => $doctorInsurance->toArray()]); } diff --git a/tests/Appointment/AppointmentSettingsListOwnershipTest.php b/tests/Appointment/AppointmentSettingsListOwnershipTest.php new file mode 100644 index 00000000..c305e940 --- /dev/null +++ b/tests/Appointment/AppointmentSettingsListOwnershipTest.php @@ -0,0 +1,52 @@ +createUser(['ROLE_DOCTOR']), 'دکتر'); + $this->em->persist($doctor); + $this->em->flush(); + + return $doctor; + } + + /** @return iterable */ + public static function endpoints(): iterable + { + yield 'date-override list' => ['/api/v1/appointment-settings/date-override/list/']; + yield 'holidays list' => ['/api/v1/appointment-settings/holidays/list/']; + yield 'available-locations' => ['/api/v1/appointment-settings/available-locations/']; + } + + #[DataProvider('endpoints')] + public function testStrangerForbidden(string $prefix): void + { + $doctor = $this->makeDoctor(); + $stranger = $this->createUser(['ROLE_DOCTOR']); + + $this->authJson('GET', $prefix . $doctor->getUuid(), $stranger); + + $this->assertSame(403, $this->responseCode()); + } + + #[DataProvider('endpoints')] + public function testOwnerAllowed(string $prefix): void + { + $doctor = $this->makeDoctor(); + + $this->authJson('GET', $prefix . $doctor->getUuid(), $doctor->getUser()); + + $this->assertSame(200, $this->responseCode()); + } +} diff --git a/tests/ClinicService/ServiceItemStaffOwnershipTest.php b/tests/ClinicService/ServiceItemStaffOwnershipTest.php new file mode 100644 index 00000000..b3f97874 --- /dev/null +++ b/tests/ClinicService/ServiceItemStaffOwnershipTest.php @@ -0,0 +1,44 @@ +createUser(['ROLE_DOCTOR']); + $doctorA = new Doctor($ownerA, 'دکتر A'); + $this->em->persist($doctorA); + $this->em->flush(); + + $section = new ServiceSection('doctor', $doctorA->getId(), 'بخش A'); + $item = new ServiceItem($section, 'خدمت'); + $this->em->persist($section); + $this->em->persist($item); + + // unrelated tenant B's staff + $doctorB = new Doctor($this->createUser(['ROLE_DOCTOR']), 'دکتر B'); + $this->em->persist($doctorB); + $this->em->flush(); + $foreignStaff = new ClinicStaff('doctor', $doctorB->getId(), 'پرسنل B'); + $this->em->persist($foreignStaff); + $this->em->flush(); + + $this->authJson('PATCH', '/api/v1/service-item/' . $item->getUuid(), $ownerA, [ + 'staff_uuid' => $foreignStaff->getUuid(), + ]); + + $this->assertSame(422, $this->responseCode()); + } +} diff --git a/tests/Doctor/DoctorAddressOwnershipTest.php b/tests/Doctor/DoctorAddressOwnershipTest.php new file mode 100644 index 00000000..2813f227 --- /dev/null +++ b/tests/Doctor/DoctorAddressOwnershipTest.php @@ -0,0 +1,45 @@ +createUser(['ROLE_DOCTOR']), 'دکتر مالک'); + $this->em->persist($doctor); + $address = DoctorAddress::forDoctor($doctor)->setName('مطب'); + $this->em->persist($address); + $this->em->flush(); + + return $address; + } + + public function testOtherDoctorCannotRead(): void + { + $address = $this->makeAddress(); + $stranger = $this->createUser(['ROLE_DOCTOR']); + + $this->authJson('GET', '/api/v1/clinic-pro/doctor-address/' . $address->getId(), $stranger); + + $this->assertSame(403, $this->responseCode()); + } + + public function testOwnerCanRead(): void + { + $address = $this->makeAddress(); + $owner = $address->getDoctor()->getUser(); + + $this->authJson('GET', '/api/v1/clinic-pro/doctor-address/' . $address->getId(), $owner); + + $this->assertSame(200, $this->responseCode()); + } +} diff --git a/tests/Insurance/DoctorInsuranceOwnershipTest.php b/tests/Insurance/DoctorInsuranceOwnershipTest.php new file mode 100644 index 00000000..8b11fe14 --- /dev/null +++ b/tests/Insurance/DoctorInsuranceOwnershipTest.php @@ -0,0 +1,49 @@ +createUser(['ROLE_DOCTOR']), 'دکتر مالک'); + $this->em->persist($doctor); + $insurance = new Insurance('بیمه تست', InsuranceType::Basic); + $this->em->persist($insurance); + $di = new DoctorInsurance($doctor, $insurance); + $this->em->persist($di); + $this->em->flush(); + + return $di; + } + + public function testOtherDoctorCannotRead(): void + { + $di = $this->makeDoctorInsurance(); + $stranger = $this->createUser(['ROLE_DOCTOR']); + + $this->authJson('GET', '/api/v1/insurance/' . $di->getId(), $stranger); + + $this->assertSame(403, $this->responseCode()); + } + + public function testOwnerCanRead(): void + { + $di = $this->makeDoctorInsurance(); + $owner = $di->getDoctor()->getUser(); + + $this->authJson('GET', '/api/v1/insurance/' . $di->getId(), $owner); + + $this->assertSame(200, $this->responseCode()); + } +}