From d05bda45a5df703a7c308a3c63612dc2a111a724 Mon Sep 17 00:00:00 2001 From: Joseph Silber Date: Thu, 4 Sep 2025 13:22:49 -0400 Subject: [PATCH 1/2] [12.x] Allow rate limit hits to only be recorded after success --- src/Illuminate/Cache/RateLimiting/Limit.php | 20 ++++++++++ .../Routing/Middleware/ThrottleRequests.php | 40 ++++++++++++++++++- 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/Illuminate/Cache/RateLimiting/Limit.php b/src/Illuminate/Cache/RateLimiting/Limit.php index 1a14009640e8..349765fee570 100644 --- a/src/Illuminate/Cache/RateLimiting/Limit.php +++ b/src/Illuminate/Cache/RateLimiting/Limit.php @@ -25,6 +25,13 @@ class Limit */ public $decaySeconds; + /** + * Whether to only record a hit after a successful response. + * + * @var bool + */ + public $onSuccess = false; + /** * The response generator callback. * @@ -129,6 +136,19 @@ public function by($key) return $this; } + /** + * Set whether to only record a hit after a successful response. + * + * @param bool $onSuccess + * @return $this + */ + public function onSuccess(bool $onSuccess = true) + { + $this->onSuccess = $onSuccess; + + return $this; + } + /** * Set the callback that should generate the response when the limit is exceeded. * diff --git a/src/Illuminate/Routing/Middleware/ThrottleRequests.php b/src/Illuminate/Routing/Middleware/ThrottleRequests.php index f6a21dd4098b..131c732a8879 100644 --- a/src/Illuminate/Routing/Middleware/ThrottleRequests.php +++ b/src/Illuminate/Routing/Middleware/ThrottleRequests.php @@ -96,6 +96,7 @@ public function handle($request, Closure $next, $maxAttempts = 60, $decayMinutes 'key' => $prefix.$this->resolveRequestSignature($request), 'maxAttempts' => $this->resolveMaxAttempts($request, $maxAttempts), 'decaySeconds' => 60 * $decayMinutes, + 'onSuccess' => false, 'responseCallback' => null, ], ] @@ -131,6 +132,7 @@ protected function handleRequestUsingNamedLimiter($request, Closure $next, $limi 'key' => self::$shouldHashKeys ? md5($limiterName.$limit->key) : $limiterName.':'.$limit->key, 'maxAttempts' => $limit->maxAttempts, 'decaySeconds' => $limit->decaySeconds, + 'onSuccess' => $limit->onSuccess, 'responseCallback' => $limit->responseCallback, ]; })->all() @@ -154,12 +156,20 @@ protected function handleRequest($request, Closure $next, array $limits) throw $this->buildException($request, $limit->key, $limit->maxAttempts, $limit->responseCallback); } - $this->limiter->hit($limit->key, $limit->decaySeconds); + if (! $limit->onSuccess) { + $this->limiter->hit($limit->key, $limit->decaySeconds); + } } $response = $next($request); + $isSuccess = $this->isSuccessfulResponse($request, $response); + foreach ($limits as $limit) { + if ($limit->onSuccess && $isSuccess) { + $this->limiter->hit($limit->key, $limit->decaySeconds); + } + $response = $this->addHeaders( $response, $limit->maxAttempts, @@ -170,6 +180,34 @@ protected function handleRequest($request, Closure $next, array $limits) return $response; } + /** + * Determine whether we should consider the response successful. + * + * @param \Illuminate\Http\Request $request + * @param \Symfony\Component\HttpFoundation\Response $response + * @return bool + */ + protected function isSuccessfulResponse($request, Response $response) + { + if ($request->isPrecognitive()) { + return false; + } + + if ($response->isSuccessful()) { + return true; + } + + if ($response->getStatusCode() >= 400) { + return false; + } + + if (data_get($response, 'exception.status') >= 400) { + return false; + } + + return $response->getStatusCode() == 302; + } + /** * Resolve the number of attempts if the user is authenticated or not. * From d1d1f77d63a295165ef82c7a8bc8f2986be35d37 Mon Sep 17 00:00:00 2001 From: Tim MacDonald Date: Sat, 20 Sep 2025 18:26:52 +1000 Subject: [PATCH 2/2] Make after callback generic --- src/Illuminate/Cache/RateLimiting/Limit.php | 14 ++-- .../Routing/Middleware/ThrottleRequests.php | 38 +-------- .../Integration/Http/ThrottleRequestsTest.php | 80 +++++++++++++++++++ 3 files changed, 91 insertions(+), 41 deletions(-) diff --git a/src/Illuminate/Cache/RateLimiting/Limit.php b/src/Illuminate/Cache/RateLimiting/Limit.php index 349765fee570..351bbf11fb8f 100644 --- a/src/Illuminate/Cache/RateLimiting/Limit.php +++ b/src/Illuminate/Cache/RateLimiting/Limit.php @@ -26,11 +26,11 @@ class Limit public $decaySeconds; /** - * Whether to only record a hit after a successful response. + * The after callback used to determine if the limiter should be hit. * - * @var bool + * @var ?callable */ - public $onSuccess = false; + public $afterCallback = null; /** * The response generator callback. @@ -137,14 +137,14 @@ public function by($key) } /** - * Set whether to only record a hit after a successful response. + * Set the callback to determine if the limiter should be hit. * - * @param bool $onSuccess + * @param callable $callback * @return $this */ - public function onSuccess(bool $onSuccess = true) + public function after($callback) { - $this->onSuccess = $onSuccess; + $this->afterCallback = $callback; return $this; } diff --git a/src/Illuminate/Routing/Middleware/ThrottleRequests.php b/src/Illuminate/Routing/Middleware/ThrottleRequests.php index 131c732a8879..35e09fd25c0f 100644 --- a/src/Illuminate/Routing/Middleware/ThrottleRequests.php +++ b/src/Illuminate/Routing/Middleware/ThrottleRequests.php @@ -96,7 +96,7 @@ public function handle($request, Closure $next, $maxAttempts = 60, $decayMinutes 'key' => $prefix.$this->resolveRequestSignature($request), 'maxAttempts' => $this->resolveMaxAttempts($request, $maxAttempts), 'decaySeconds' => 60 * $decayMinutes, - 'onSuccess' => false, + 'afterCallback' => null, 'responseCallback' => null, ], ] @@ -132,7 +132,7 @@ protected function handleRequestUsingNamedLimiter($request, Closure $next, $limi 'key' => self::$shouldHashKeys ? md5($limiterName.$limit->key) : $limiterName.':'.$limit->key, 'maxAttempts' => $limit->maxAttempts, 'decaySeconds' => $limit->decaySeconds, - 'onSuccess' => $limit->onSuccess, + 'afterCallback' => $limit->afterCallback, 'responseCallback' => $limit->responseCallback, ]; })->all() @@ -156,17 +156,15 @@ protected function handleRequest($request, Closure $next, array $limits) throw $this->buildException($request, $limit->key, $limit->maxAttempts, $limit->responseCallback); } - if (! $limit->onSuccess) { + if (! $limit->afterCallback) { $this->limiter->hit($limit->key, $limit->decaySeconds); } } $response = $next($request); - $isSuccess = $this->isSuccessfulResponse($request, $response); - foreach ($limits as $limit) { - if ($limit->onSuccess && $isSuccess) { + if ($limit->afterCallback && ($limit->afterCallback)($response)) { $this->limiter->hit($limit->key, $limit->decaySeconds); } @@ -180,34 +178,6 @@ protected function handleRequest($request, Closure $next, array $limits) return $response; } - /** - * Determine whether we should consider the response successful. - * - * @param \Illuminate\Http\Request $request - * @param \Symfony\Component\HttpFoundation\Response $response - * @return bool - */ - protected function isSuccessfulResponse($request, Response $response) - { - if ($request->isPrecognitive()) { - return false; - } - - if ($response->isSuccessful()) { - return true; - } - - if ($response->getStatusCode() >= 400) { - return false; - } - - if (data_get($response, 'exception.status') >= 400) { - return false; - } - - return $response->getStatusCode() == 302; - } - /** * Resolve the number of attempts if the user is authenticated or not. * diff --git a/tests/Integration/Http/ThrottleRequestsTest.php b/tests/Integration/Http/ThrottleRequestsTest.php index e465c16e7395..073f56a7df87 100644 --- a/tests/Integration/Http/ThrottleRequestsTest.php +++ b/tests/Integration/Http/ThrottleRequestsTest.php @@ -10,14 +10,19 @@ use Illuminate\Foundation\Auth\User; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Http\Exceptions\ThrottleRequestsException; +use Illuminate\Http\Request; use Illuminate\Routing\Exceptions\MissingRateLimiterException; use Illuminate\Routing\Middleware\ThrottleRequests; use Illuminate\Support\Carbon; +use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\Date; +use Illuminate\Support\Facades\RateLimiter as RateLimiterFacade; use Illuminate\Support\Facades\Route; use Orchestra\Testbench\Attributes\WithConfig; use Orchestra\Testbench\Attributes\WithMigration; use Orchestra\Testbench\TestCase; use PHPUnit\Framework\Attributes\DataProvider; +use Symfony\Component\HttpFoundation\Response; use Throwable; #[WithConfig('hashing.driver', 'bcrypt')] @@ -410,6 +415,81 @@ public function testItFallbacksToUserAccessorWhenThereIsNoNamedLimiterWhenAuthen $this->assertEquals(Carbon::now()->addSeconds(2)->getTimestamp(), $e->getHeaders()['X-RateLimit-Reset']); } } + + public function testItCanThrottleBasedOnResponse() + { + RateLimiterFacade::for('throttle-not-found', function (Request $request) { + return Limit::perMinute(1)->after(fn ($response) => $response->status() === 404); + }); + Route::get('/', fn () => match(request('status')) { + '404' => abort(404), + default => 'ok', + })->middleware(ThrottleRequests::using('throttle-not-found')); + + $this->travelTo('2000-01-01 00:00:00'); + $this->get('?status=404')->assertNotFound(); + $this->get('?status=404')->assertTooManyRequests(); + $this->get('?status=404')->assertTooManyRequests(); + + $this->travelTo('2000-01-01 00:00:59'); + $this->get('?status=404')->assertTooManyRequests(); + $this->get('?status=404')->assertTooManyRequests(); + + $this->travelTo('2000-01-01 00:01:00'); + $this->get('?status=404')->assertNotFound(); + $this->get('?status=404')->assertTooManyRequests(); + $this->get('?status=404')->assertTooManyRequests(); + + $this->travelTo('2000-01-01 00:01:59'); + $this->get('?status=404')->assertTooManyRequests(); + $this->get('?status=404')->assertTooManyRequests(); + + $this->travelTo('2000-01-01 00:02:00'); + $this->get('?status=404')->assertNotFound(); + $this->get('?status=404')->assertTooManyRequests(); + $this->get('?status=404')->assertTooManyRequests(); + } + + public function testItDoesNotHitLimiterUntilResponseHasBeenGenerated() + { + ThrottleRequests::shouldHashKeys(false); + RateLimiterFacade::for('throttle-not-found', function (Request $request) { + return Limit::perMinute(1)->after(fn ($response) => $response->status() === 404); + }); + $duringRequest = null; + Route::get('/', function () use (&$duringRequest) { + $duringRequest = [ + Cache::get('throttle-not-found:'), + Cache::get('throttle-not-found::timer'), + ]; + + abort(404); + })->middleware(ThrottleRequests::using('throttle-not-found')); + + $this->travelTo('2000-01-01 00:00:00'); + $this->get('?status=404')->assertNotFound(); + + $this->assertSame([null, null], $duringRequest); + $this->assertSame([1, 946684860], [ + Cache::get('throttle-not-found:'), + Cache::get('throttle-not-found::timer'), + ]); + } + + public function testItReturnsConfiguredResponseWhenUsingAfterLimit(): void + { + ThrottleRequests::shouldHashKeys(false); + RateLimiterFacade::for('throttle-not-found', function (Request $request) { + return Limit::perMinute(1) + ->after(fn ($response) => $response->status() === 404) + ->response(fn () => response('ah ah ah', status: 429)); + }); + Route::get('/', fn () => abort(404))->middleware(ThrottleRequests::using('throttle-not-found')); + + $this->travelTo('2000-01-01 00:00:00'); + $this->get('?status=404')->assertNotFound(); + $this->get('?status=404')->assertTooManyRequests()->assertContent('ah ah ah'); + } } class UserWithAccessor extends User