From 69df6f488671aab5b25ef6e66b8749f533d11056 Mon Sep 17 00:00:00 2001 From: "Ronald A. Richardson" Date: Tue, 29 Sep 2026 14:57:36 +0800 Subject: [PATCH] fix(throttle): key the API rate limiter on the consumer, not the proxy IP ThrottleRequests runs before API authentication, so Laravel's default signature always fell back to route domain + client IP. Behind a load balancer that IP is the balancer's, and no route has a domain, so every tenant, API key and console visitor shared one bucket: one busy integration returned 429 to the whole platform. Key the limiter on the hashed credential (API key, Sanctum token, basic auth), falling back to the user and then the IP only when none is sent, and scope it by first path segment so /v1 and /int stay separate. Also keep Retry-After and X-RateLimit-* on the 429 response so throttled clients know when to retry. --- src/Exceptions/Handler.php | 3 +- src/Http/Middleware/ThrottleRequests.php | 33 +++++++++++ .../Unit/Exceptions/ExceptionHandlerTest.php | 16 +++++ tests/Unit/Http/MiddlewareContractsTest.php | 59 +++++++++++++++++++ 4 files changed, 110 insertions(+), 1 deletion(-) diff --git a/src/Exceptions/Handler.php b/src/Exceptions/Handler.php index def3bcdf..4abc3167 100644 --- a/src/Exceptions/Handler.php +++ b/src/Exceptions/Handler.php @@ -205,7 +205,8 @@ private function manuallyHandleException(\Throwable $exception): ?\Illuminate\Ht return response()->error('Invalid XSRF token sent with request.', 419); case 'ThrottleRequestsException': - return response()->error('Too many requests.', 429); + // Keep Retry-After and X-RateLimit-* so the throttled client knows when to retry. + return response()->error('Too many requests.', 429)->withHeaders($exception->getHeaders()); case 'AuthenticationException': return response()->error('Unauthenticated.', 401); diff --git a/src/Http/Middleware/ThrottleRequests.php b/src/Http/Middleware/ThrottleRequests.php index 3f32b83c..bb00885a 100644 --- a/src/Http/Middleware/ThrottleRequests.php +++ b/src/Http/Middleware/ThrottleRequests.php @@ -59,6 +59,39 @@ public function handle($request, \Closure $next, $maxAttempts = null, $decayMinu return parent::handle($request, $next, $maxAttempts, $decayMinutes, $prefix); } + /** + * Resolve the limiter key from the consumer rather than the connection. + * + * This middleware runs ahead of API authentication, so Laravel's default signature + * (the authenticated user, else route domain + client IP) always fell through to the + * IP. Behind a load balancer or reverse proxy that IP is the proxy's, and no route has + * a domain, so every tenant, API key and console visitor shared one bucket: a single + * busy integration returned 429 to the entire platform. + * + * The presented credential identifies the consumer without a database lookup, so the + * key is the hashed credential. Only requests carrying no credential fall back to the + * user, then the IP. The first path segment ("v1", "int", ...) keeps the public API and + * the console's public routes in separate buckets. + * + * @param \Illuminate\Http\Request $request + * + * @return string + */ + protected function resolveRequestSignature($request) + { + $scope = 'fleetbase-throttle|' . ($request->segment(1) ?? ''); + + if ($credential = $this->extractApiKey($request)) { + return sha1($scope . '|credential|' . $credential); + } + + if ($user = $request->user()) { + return sha1($scope . '|user|' . $user->getAuthIdentifier()); + } + + return sha1($scope . '|ip|' . $request->ip()); + } + /** * Extract API key from the request. * diff --git a/tests/Unit/Exceptions/ExceptionHandlerTest.php b/tests/Unit/Exceptions/ExceptionHandlerTest.php index 68dfa51d..08c71d30 100644 --- a/tests/Unit/Exceptions/ExceptionHandlerTest.php +++ b/tests/Unit/Exceptions/ExceptionHandlerTest.php @@ -113,6 +113,22 @@ function exception_handler_subject(): Handler 'http not found' => [new NotFoundHttpException(), ['There is nothing to see here.'], 404], ]); + it('keeps the retry-after and rate limit headers on a throttled response', function () { + $handler = exception_handler_subject(); + $exception = new ThrottleRequestsException('Too Many Attempts.', null, [ + 'Retry-After' => 42, + 'X-RateLimit-Limit' => 120, + 'X-RateLimit-Remaining' => 0, + ]); + + $response = $handler->render(Request::create('/v1/orders', 'POST'), $exception); + + expect($response->getStatusCode())->toBe(429) + ->and($response->headers->get('Retry-After'))->toBe('42') + ->and($response->headers->get('X-RateLimit-Limit'))->toBe('120') + ->and($response->headers->get('X-RateLimit-Remaining'))->toBe('0'); + }); + it('returns a resource-specific model not found json response when the model is known', function () { $handler = exception_handler_subject(); $exception = (new ModelNotFoundException())->setModel(User::class); diff --git a/tests/Unit/Http/MiddlewareContractsTest.php b/tests/Unit/Http/MiddlewareContractsTest.php index 96027c7b..952a0f49 100644 --- a/tests/Unit/Http/MiddlewareContractsTest.php +++ b/tests/Unit/Http/MiddlewareContractsTest.php @@ -1023,6 +1023,65 @@ public function info(string $message, array $context = []): void ->and($isUnlimitedApiKey->invoke($middleware, 'Bearer other-key'))->toBeFalse(); }); + test('throttle requests keys the limiter on the presented credential rather than the proxy ip', function () { + middleware_contracts_fixture([ + 'api.throttle.enabled' => true, + 'api.throttle.max_attempts' => 2, + 'api.throttle.decay_minutes' => 1, + 'api.throttle.unlimited_keys' => [], + ]); + + $middleware = middleware_contracts_throttle(); + // Every request arrives from the same load balancer address, as in production. + $send = function (string $uri, ?string $credential = null) use ($middleware) { + $server = ['REMOTE_ADDR' => '10.0.0.5']; + if ($credential) { + $server['HTTP_AUTHORIZATION'] = 'Bearer ' . $credential; + } + $request = Request::create($uri, 'GET', [], [], [], $server); + $request->setRouteResolver(fn () => new Illuminate\Routing\Route(['GET'], ltrim($uri, '/'), fn () => null)); + + try { + return $middleware->handle($request, fn () => new JsonResponse(['ok' => true]))->getStatusCode(); + } catch (Illuminate\Http\Exceptions\ThrottleRequestsException $exception) { + return $exception->getStatusCode(); + } + }; + + $noisy = [$send('/v1/orders', 'flb_live_noisy'), $send('/v1/orders', 'flb_live_noisy'), $send('/v1/orders', 'flb_live_noisy')]; + + expect($noisy)->toBe([200, 200, 429]) + ->and($send('/v1/orders', 'flb_live_quiet'))->toBe(200) + ->and($send('/int/v1/auth/login'))->toBe(200) + ->and($send('/int/v1/lookup/countries'))->toBe(200) + ->and($send('/int/v1/settings/branding'))->toBe(429) + ->and($send('/v1/orders'))->toBe(200); + }); + + test('throttle requests falls back to the authenticated user before the ip when no credential is sent', function () { + middleware_contracts_fixture([ + 'api.throttle.enabled' => true, + 'api.throttle.max_attempts' => 1, + 'api.throttle.unlimited_keys' => [], + ]); + + $middleware = middleware_contracts_throttle(); + $signature = new ReflectionMethod($middleware, 'resolveRequestSignature'); + $signature->setAccessible(true); + $request = function (?string $userId, string $ip) { + $request = Request::create('/v1/orders', 'GET', [], [], [], ['REMOTE_ADDR' => $ip]); + $request->setUserResolver(fn () => $userId ? new Illuminate\Auth\GenericUser(['id' => $userId]) : null); + + return $request; + }; + + expect($signature->invoke($middleware, $request('user-1', '10.0.0.5'))) + ->toBe($signature->invoke($middleware, $request('user-1', '10.0.0.6'))) + ->not->toBe($signature->invoke($middleware, $request('user-2', '10.0.0.5'))) + ->and($signature->invoke($middleware, $request(null, '10.0.0.5'))) + ->not->toBe($signature->invoke($middleware, $request(null, '10.0.0.6'))); + }); + test('basic auth middleware rejects requests without bearer credentials before continuing', function () { middleware_contracts_fixture();