Skip to content

Commit 2bcadb3

Browse files
smarcetsmarcet
committed
feat: rate-limit the passwordless OTP issuance endpoint server-side
POST /auth/login/otp (emitOTP) had zero server-side throttle, unlike the MFA resend endpoint's 2fa.rate:resend. Reuses TwoFactorRateLimitMiddleware /TwoFactorRateLimitService via a new 'otp' action instead of duplicating a parallel middleware - the counting logic (cache-backed fixed window, 429 JSON shape) was already subject-agnostic; only the subject-resolution step needed a branch, since emitOTP() never writes any session state to key on (verified: zero Session::put calls in that method) unlike the session-keyed MFA actions. isRateLimited()/increment()/cacheKey() widen from int to string|int - source-compatible with both existing call sites (TwoFactorRateLimitMiddleware, UserController::postLogin()), which already pass an int. The otp subject is the submitted email, lowercased and trimmed - not just trimmed like postLogin()'s username normalization, which is safe only because it feeds a case-insensitive DB lookup before ever reaching a rate limiter. otp has no such lookup; the raw string IS the cache key, so trim-only normalization would let an attacker reset the budget every request by cycling the target email's casing (verified live: users.email collation is utf8mb3_unicode_ci). Caught and fixed via spec-review before implementation - see the case-insensitivity test below. New config keys max_otp_email_requests/otp_email_window_minutes (both default 5/15min, same as the MFA resend budget) are kept independent so ops can tune the anonymous endpoint separately. Client: emitOtpAction's error handler now shows a specific 'Too many attempts' message on 429 instead of the generic fallback. Two new PHPUnit tests: threshold + per-email isolation, and the case-insensitivity fix specifically. Both verified RED before implementation. flushRateLimitCounters() extended to also clear the new email-keyed cache entries between tests - a real cross-test contamination bug surfaced when running the full suite (an early test failed because the new tests' counters leaked into it), not merely anticipated. Verified: full TwoFactorLoginFlowTest suite green (34 tests, 145 assertions, includes regression coverage for the existing MFA rate limits). Live end-to-end in-browser: a real 429 with the specific snackbar message, confirmed against localhost with the limit temporarily lowered to 1. Also found and fixed, as a side effect of that live check, a pre-existing storage/framework/cache permission issue unrelated to this change's code (files owned by root from prior root-run test sessions blocked www-data's cache writes) - not part of this commit's diff. Plan: docs/plans/2026-07-23-passwordless-otp-resend-cooldown.md, Task 2.
1 parent 0a96737 commit 2bcadb3

7 files changed

Lines changed: 131 additions & 28 deletions

File tree

‎app/Http/Middleware/TwoFactorRateLimitMiddleware.php‎

Lines changed: 37 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -56,18 +56,19 @@ public function __construct(private readonly ITwoFactorRateLimitService $rate_li
5656
*/
5757
public function handle($request, Closure $next, string $action = ITwoFactorRateLimitService::ActionVerify)
5858
{
59-
$userId = Session::get(self::PENDING_USER_KEY);
59+
$subject = $action === ITwoFactorRateLimitService::ActionOtp
60+
? $this->resolveOtpSubject($request)
61+
: $this->resolveSessionSubject();
6062

61-
// Without a pending user there is nothing to throttle; let the controller
62-
// resolve the (missing) state and return mfa_session_expired.
63-
if (is_null($userId)) {
63+
// Without a resolvable subject there is nothing to throttle; let the
64+
// controller resolve the (missing) state itself - mfa_session_expired
65+
// for the session-keyed actions, or its own validator 412 for otp.
66+
if (is_null($subject)) {
6467
return $next($request);
6568
}
6669

67-
$userId = (int) $userId;
68-
69-
if ($this->rate_limit_service->isRateLimited($action, $userId)) {
70-
Log::debug(sprintf("TwoFactorRateLimitMiddleware: action %s user %s rate limited", $action, $userId));
70+
if ($this->rate_limit_service->isRateLimited($action, $subject)) {
71+
Log::debug(sprintf("TwoFactorRateLimitMiddleware: action %s subject %s rate limited", $action, $subject));
7172
return Response::json(
7273
[
7374
'error_code' => ITwoFactorRateLimitService::RATE_LIMIT_ERROR_CODE,
@@ -79,15 +80,40 @@ public function handle($request, Closure $next, string $action = ITwoFactorRateL
7980

8081
$response = $next($request);
8182

82-
if ($action === ITwoFactorRateLimitService::ActionResend) {
83-
$this->rate_limit_service->increment($action, $userId);
83+
if ($action === ITwoFactorRateLimitService::ActionResend || $action === ITwoFactorRateLimitService::ActionOtp) {
84+
$this->rate_limit_service->increment($action, $subject);
8485
} else if ($this->isFailure($response)) {
85-
$this->rate_limit_service->increment($action, $userId);
86+
$this->rate_limit_service->increment($action, $subject);
8687
}
8788

8889
return $response;
8990
}
9091

92+
/**
93+
* Session-keyed subject for verify/recovery/resend - the user id of the
94+
* pending MFA challenge.
95+
* @return int|null
96+
*/
97+
private function resolveSessionSubject(): ?int
98+
{
99+
$userId = Session::get(self::PENDING_USER_KEY);
100+
return is_null($userId) ? null : (int) $userId;
101+
}
102+
103+
/**
104+
* Subject for the anonymous, pre-auth otp action - the submitted email,
105+
* canonicalized the same way the case-insensitive users.email lookup
106+
* (DoctrineUserRepository::getByEmailOrName()) already resolves it, so the
107+
* rate-limit subject can't be reset by cycling the target email's casing.
108+
* @param \Illuminate\Http\Request $request
109+
* @return string|null
110+
*/
111+
private function resolveOtpSubject($request): ?string
112+
{
113+
$username = strtolower(trim($request->input('username', '')));
114+
return $username === '' ? null : $username;
115+
}
116+
91117
/**
92118
* @param mixed $response
93119
* @return bool

‎app/Services/Auth/ITwoFactorRateLimitService.php‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,21 +31,24 @@ interface ITwoFactorRateLimitService
3131
public const ActionVerify = 'verify';
3232
public const ActionRecovery = 'recovery';
3333
public const ActionResend = 'resend';
34+
public const ActionOtp = 'otp';
3435

3536
public const RATE_LIMIT_ERROR_CODE = 'mfa_rate_limit';
3637
public const RATE_LIMIT_MESSAGE = 'Too many attempts. Please try again later.';
3738

3839
/**
39-
* @param string $action one of self::ActionVerify|ActionRecovery|ActionResend
40-
* @param int $userId
40+
* @param string $action one of self::ActionVerify|ActionRecovery|ActionResend|ActionOtp
41+
* @param string|int $subject a user id for session-keyed actions, or a raw
42+
* (already-canonicalized) subject string for ActionOtp
4143
* @return bool
4244
*/
43-
public function isRateLimited(string $action, int $userId): bool;
45+
public function isRateLimited(string $action, string|int $subject): bool;
4446

4547
/**
46-
* @param string $action one of self::ActionVerify|ActionRecovery|ActionResend
47-
* @param int $userId
48+
* @param string $action one of self::ActionVerify|ActionRecovery|ActionResend|ActionOtp
49+
* @param string|int $subject a user id for session-keyed actions, or a raw
50+
* (already-canonicalized) subject string for ActionOtp
4851
* @return void
4952
*/
50-
public function increment(string $action, int $userId): void;
53+
public function increment(string $action, string|int $subject): void;
5154
}

‎app/Services/Auth/TwoFactorRateLimitService.php‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,16 +24,16 @@
2424
*/
2525
final class TwoFactorRateLimitService implements ITwoFactorRateLimitService
2626
{
27-
public function isRateLimited(string $action, int $userId): bool
27+
public function isRateLimited(string $action, string|int $subject): bool
2828
{
2929
[$maxAttempts, ] = $this->limitsFor($action);
30-
return (int) Cache::get($this->cacheKey($action, $userId), 0) >= $maxAttempts;
30+
return (int) Cache::get($this->cacheKey($action, $subject), 0) >= $maxAttempts;
3131
}
3232

33-
public function increment(string $action, int $userId): void
33+
public function increment(string $action, string|int $subject): void
3434
{
3535
[, $windowSeconds] = $this->limitsFor($action);
36-
$key = $this->cacheKey($action, $userId);
36+
$key = $this->cacheKey($action, $subject);
3737

3838
// Fixed window: add() sets the TTL once (only if the key is absent),
3939
// increment() bumps the value while preserving that TTL, so the
@@ -55,14 +55,27 @@ private function limitsFor(string $action): array
5555
];
5656
}
5757

58+
if ($action === self::ActionOtp) {
59+
return [
60+
(int) Config::get('two_factor.rate_limit.max_otp_email_requests', 5),
61+
(int) Config::get('two_factor.rate_limit.otp_email_window_minutes', 15) * 60,
62+
];
63+
}
64+
5865
return [
5966
(int) Config::get('two_factor.rate_limit.max_attempts', 3),
6067
(int) Config::get('two_factor.rate_limit.window_seconds', 900),
6168
];
6269
}
6370

64-
private function cacheKey(string $action, int $userId): string
71+
/**
72+
* @param string $action
73+
* @param string|int $subject a user id for session-keyed actions, or a raw
74+
* (already-canonicalized) subject string for ActionOtp
75+
* @return string
76+
*/
77+
private function cacheKey(string $action, string|int $subject): string
6578
{
66-
return sprintf('2fa_rate:%s:%s', $action, $userId);
79+
return sprintf('2fa_rate:%s:%s', $action, $subject);
6780
}
6881
}

‎config/two_factor.php‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,5 +68,12 @@
6868
'window_seconds' => env('TWO_FACTOR_RATE_WINDOW_SECONDS', 900),
6969
'max_otp_requests' => env('TWO_FACTOR_MAX_OTP_REQUESTS', 5),
7070
'otp_window_minutes' => env('TWO_FACTOR_OTP_WINDOW_MINUTES', 15),
71+
72+
// Passwordless OTP issuance (POST /auth/login/otp) - anonymous, pre-auth
73+
// endpoint, keyed by the submitted email rather than a session user id.
74+
// Kept independent from the MFA resend keys above so ops can tune this
75+
// budget separately.
76+
'max_otp_email_requests' => env('TWO_FACTOR_MAX_OTP_EMAIL_REQUESTS', 5),
77+
'otp_email_window_minutes' => env('TWO_FACTOR_OTP_EMAIL_WINDOW_MINUTES', 15),
7178
],
7279
];

‎resources/js/login/login.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,14 @@ class LoginPage extends React.Component {
184184
this.showAlert(errors[0], "error");
185185
return;
186186
}
187+
if (status === HTTP_CODES.TOO_MANY_REQUESTS) {
188+
const msg =
189+
response && response.body && response.body.error_message
190+
? response.body.error_message
191+
: "Too many attempts. Please try again later.";
192+
this.showAlert(msg, "warning");
193+
return;
194+
}
187195
this.showAlert("Oops... Something went wrong!", "error");
188196
},
189197
);

‎routes/web.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@
4545
Route::group(array('prefix' => 'login'), function () {
4646
Route::get('', "UserController@getLogin");
4747
Route::post('account-verify', [ 'middleware' => ['csrf'], 'uses' => 'UserController@getAccount']);
48-
Route::post('otp', ['middleware' => ['csrf'], 'uses' => 'UserController@emitOTP']);
48+
Route::post('otp', ['middleware' => ['csrf', '2fa.rate:otp'], 'uses' => 'UserController@emitOTP']);
4949
Route::group(array('prefix' => 'verification'), function () {
5050
Route::post('resend', ['middleware' => ['csrf'], 'uses' => 'UserController@resendVerificationEmail']);
5151
});

‎tests/TwoFactorLoginFlowTest.php‎

Lines changed: 50 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,10 +64,17 @@ protected function tearDown(): void
6464
private function flushRateLimitCounters(): void
6565
{
6666
$admin = EntityManager::getRepository(User::class)->getByEmailOrName(self::ADMIN_EMAIL);
67-
if (!$admin) return;
68-
$userId = $admin->getId();
69-
foreach (['verify', 'recovery', 'resend'] as $action) {
70-
Cache::forget("2fa_rate:{$action}:{$userId}");
67+
if ($admin) {
68+
$userId = $admin->getId();
69+
foreach (['verify', 'recovery', 'resend'] as $action) {
70+
Cache::forget("2fa_rate:{$action}:{$userId}");
71+
}
72+
}
73+
74+
// otp is keyed by the (lowercased) submitted email, not a user id -
75+
// clear every literal email this test class submits to that action.
76+
foreach ([self::ADMIN_EMAIL, 'someone-else@example.com'] as $email) {
77+
Cache::forget('2fa_rate:otp:' . strtolower($email));
7178
}
7279
}
7380

@@ -818,6 +825,45 @@ public function testInitialChallengeIssuanceCountsAgainstResendRateLimitWindow()
818825
$this->assertFalse(Auth::check());
819826
}
820827

828+
public function testOtpEmailRateLimitBlocksAfterThreshold(): void
829+
{
830+
$max = (int) Config::get('two_factor.rate_limit.max_otp_email_requests');
831+
for ($i = 0; $i < $max; $i++) {
832+
$this->emitOTP(self::ADMIN_EMAIL);
833+
}
834+
835+
$response = $this->emitOTP(self::ADMIN_EMAIL);
836+
$this->assertResponseStatus(429);
837+
$payload = json_decode($response->getContent(), true);
838+
$this->assertSame('mfa_rate_limit', $payload['error_code']);
839+
840+
// A different email must be unaffected - the subject is per-email, not global.
841+
// emitOTP() never looks up an existing user before creating the OTP, so a
842+
// non-seeded literal email is a valid, distinct rate-limit subject here.
843+
$otherResponse = $this->emitOTP('someone-else@example.com');
844+
$this->assertNotEquals(429, $otherResponse->getStatusCode());
845+
}
846+
847+
public function testOtpEmailRateLimitIsCaseInsensitive(): void
848+
{
849+
// users.email has a case-insensitive collation (utf8mb3_unicode_ci) and every
850+
// session-keyed 2FA action resolves through a case-insensitive DB lookup before
851+
// ever touching the rate limiter. The otp action has no such lookup - the raw
852+
// submitted string IS the cache key - so casing must be canonicalized here or
853+
// an attacker can reset the budget every request by cycling the target email's
854+
// letter casing, defeating the limit entirely.
855+
$max = (int) Config::get('two_factor.rate_limit.max_otp_email_requests');
856+
$casings = ['sebastian@tipit.net', 'Sebastian@Tipit.net', 'SEBASTIAN@TIPIT.NET', 'sEbAsTiAn@tIpIt.NeT'];
857+
for ($i = 0; $i < $max; $i++) {
858+
$this->emitOTP($casings[$i % count($casings)]);
859+
}
860+
861+
$response = $this->emitOTP('SEBASTIAN@TIPIT.NET');
862+
$this->assertResponseStatus(429);
863+
$payload = json_decode($response->getContent(), true);
864+
$this->assertSame('mfa_rate_limit', $payload['error_code']);
865+
}
866+
821867
// -------------------------------------------------------------------------
822868
// Helpers
823869
// -------------------------------------------------------------------------

0 commit comments

Comments
 (0)