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

fix(security): per-mobile OTP cap + refresh-token rotation (M6, M7)

Browse Source 
M6: send-code rate-limited only per IP, so a victim's number could be
SMS-flooded from rotating IPs. Add a per-mobile bucket (same 5/hour policy)
keyed by the validated mobile.

M7: /oauth/token/refresh reused the presented refresh token verbatim (no
rotation) and never re-checked the user. The rotation infra already existed
(issueTokens mints a fresh refresh token) — the controller just discarded it.
Now revoke the presented token (single-use), issue a fresh pair, and reject a
suspended user (status != 1).

Regressions: tests/Auth/SendCodeMobileRateLimitTest,
tests/Auth/RefreshTokenRotationTest (both fail without the fix).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
hamed
2026-06-28 20:25:22 +03:30
co-authored by Claude Opus 4.8
parent fe6383314e
commit 670cef24f4
5 changed files with 122 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/audit-backlog.md
+2 -2
View File
@@ -59,8 +59,8 @@ _None outstanding._
| ✅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 |
| ✅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 | 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 |