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();