From 670cef24f47357f51fb731eb0efc43171daf1251 Mon Sep 17 00:00:00 2001 From: hamed <15238-genius.ha@users.noreply.drupalcode.org> Date: Sun, 28 Jun 2026 20:25:22 +0330 Subject: [PATCH] fix(security): per-mobile OTP cap + refresh-token rotation (M6, M7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/api/auth.md | 12 +++- docs/audit-backlog.md | 4 +- src/Auth/Controller/AuthController.php | 16 ++++-- tests/Auth/RefreshTokenRotationTest.php | 65 ++++++++++++++++++++++ tests/Auth/SendCodeMobileRateLimitTest.php | 34 +++++++++++ 5 files changed, 122 insertions(+), 9 deletions(-) create mode 100644 tests/Auth/RefreshTokenRotationTest.php create mode 100644 tests/Auth/SendCodeMobileRateLimitTest.php diff --git a/docs/api/auth.md b/docs/api/auth.md index a86c9d9a..20195d9e 100644 --- a/docs/api/auth.md +++ b/docs/api/auth.md @@ -38,6 +38,7 @@ Send OTP code to mobile number. |------|------|-------------| | `ERR_VALIDATION_001` | 422 | Invalid mobile format | | `ERR_AUTH_004` | 429 | OTP rate limit exceeded | +| `ERR_RATE_LIMIT_001` | 429 | بیش از حد مجاز — هم per-IP (۵ در ۶۰ دقیقه) و هم **per-mobile** (۵ در ۶۰ دقیقه؛ ضد flood از IPهای چرخشی) | --- @@ -219,18 +220,23 @@ Refresh expired JWT using refresh token. } ``` +> **چرخش (rotation):** هر refresh token **یک‌بارمصرف** است. با هر فراخوانی، توکن ارائه‌شده باطل می‌شود و یک جفت `access_token` + `refresh_token` تازه صادر می‌گردد. توکن قبلی دیگر کار نمی‌کند (`401`). کاربر **معلق** (`status != 1`) نمی‌تواند refresh کند. + ### Response `200` ```json { - "token": "eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiJ9...", - "refresh_token": "def50200..." + "access_token": "eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiJ9...", + "refresh_token": "<توکن جدید — با قبلی فرق دارد>", + "token_type": "Bearer", + "expires_in": 900, + "refresh_token_expires_in": 2592000 } ``` ### Errors | Code | HTTP | Description | |------|------|-------------| -| `ERR_AUTH_001` | 401 | Invalid or expired refresh token | +| `ERR_AUTH_001` | 401 | Invalid/expired/already-rotated refresh token, or suspended user | --- diff --git a/docs/audit-backlog.md b/docs/audit-backlog.md index a07dabe0..4463eece 100644 --- a/docs/audit-backlog.md +++ b/docs/audit-backlog.md @@ -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 | diff --git a/src/Auth/Controller/AuthController.php b/src/Auth/Controller/AuthController.php index 318c6565..51705d32 100644 --- a/src/Auth/Controller/AuthController.php +++ b/src/Auth/Controller/AuthController.php @@ -147,6 +147,13 @@ class AuthController extends BaseController return $this->error(ErrorCodes::ERR_VALIDATION_001, 'فرمت شماره موبایل نادرست است', 422, 'mobile'); } + // Per-mobile cap (in addition to per-IP) so a victim's number can't be + // SMS-flooded from rotating IPs. + $mobileLimiter = $this->sendCodeLimiter->create('mobile:' . $mobile); + if (!$mobileLimiter->consume(1)->isAccepted()) { + return $this->error(ErrorCodes::ERR_RATE_LIMIT_001, ErrorCodes::message(ErrorCodes::ERR_RATE_LIMIT_001), 429); + } + $uuid = $this->otpService->sendCode($mobile); return new JsonResponse(['uuid' => $uuid, 'message' => 'کد تایید با موفقیت ارسال شد.']); @@ -470,14 +477,15 @@ class AuthController extends BaseController $result = $this->tokenService->refreshToken($refreshToken); $user = $this->userRepo->find($result['userId']); - if ($user === null) { + if ($user === null || $user->getStatus() !== 1) { return $this->error(ErrorCodes::ERR_AUTH_001, ErrorCodes::message(ErrorCodes::ERR_AUTH_001), 401); } - $tokens = $this->tokenService->issueTokens($user); - $tokens['refresh_token'] = $result['rawToken']; + // Rotate: the presented refresh token is single-use. Revoke it and issue a + // fresh access + refresh pair, so a stolen token can't be reused. + $this->tokenService->revokeRefreshToken($refreshToken); - return new JsonResponse($tokens); + return new JsonResponse($this->tokenService->issueTokens($user)); } #[OA\Get( diff --git a/tests/Auth/RefreshTokenRotationTest.php b/tests/Auth/RefreshTokenRotationTest.php new file mode 100644 index 00000000..d6e428fd --- /dev/null +++ b/tests/Auth/RefreshTokenRotationTest.php @@ -0,0 +1,65 @@ +get(TokenService::class)->issueTokens($user)['refresh_token']; + } + + private function refresh(string $token): array + { + $this->client->request( + 'POST', + '/oauth/token/refresh', + server: ['CONTENT_TYPE' => 'application/json'], + content: json_encode(['refresh_token' => $token]), + ); + + return json_decode($this->client->getResponse()->getContent(), true) ?? []; + } + + public function testTokenIsRotatedAndOldOneRevoked(): void + { + $this->client->disableReboot(); + $user = $this->createUser(); + + $old = $this->issueRefresh($user); + $body = $this->refresh($old); + $this->assertSame(200, $this->responseCode()); + + $new = $body['refresh_token']; + $this->assertNotSame($old, $new, 'refresh token was not rotated'); + + // the old token is now single-use-spent → rejected + $this->refresh($old); + $this->assertSame(401, $this->responseCode()); + + // the new token still works + $this->refresh($new); + $this->assertSame(200, $this->responseCode()); + } + + public function testSuspendedUserCannotRefresh(): void + { + $this->client->disableReboot(); + $user = $this->createUser(); + $token = $this->issueRefresh($user); + + $user->setStatus(0); + $this->em->flush(); + + $this->refresh($token); + $this->assertSame(401, $this->responseCode()); + } +} diff --git a/tests/Auth/SendCodeMobileRateLimitTest.php b/tests/Auth/SendCodeMobileRateLimitTest.php new file mode 100644 index 00000000..fae9877f --- /dev/null +++ b/tests/Auth/SendCodeMobileRateLimitTest.php @@ -0,0 +1,34 @@ +client->disableReboot(); + $mobile = '0912' . str_pad((string) random_int(0, 9_999_999), 7, '0', STR_PAD_LEFT); + + $statuses = []; + for ($i = 0; $i < 6; $i++) { + // each request from a different IP → the per-IP limiter never fires + $this->client->request( + 'POST', + '/api/v1/user/send-code', + server: ['REMOTE_ADDR' => "10.20.30.$i", 'CONTENT_TYPE' => 'application/json'], + content: json_encode(['mobile' => $mobile]), + ); + $statuses[] = $this->client->getResponse()->getStatusCode(); + } + + // send_code limit is 5/hour → the 6th for the same mobile is rejected + $this->assertSame(429, $statuses[5], 'per-mobile cap not enforced: ' . implode(',', $statuses)); + $this->assertNotContains(429, array_slice($statuses, 0, 5)); + } +}