diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php index 3ea8731cd..db336859d 100644 --- a/app/Exceptions/Handler.php +++ b/app/Exceptions/Handler.php @@ -14,6 +14,7 @@ use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\HttpKernel\Exception\HttpException; +use Symfony\Component\HttpKernel\Exception\TooManyRequestsHttpException; use Throwable; use Illuminate\Validation\ValidationException; use Illuminate\Auth\Access\AuthorizationException; @@ -68,6 +69,10 @@ public function render($request, Throwable $e) } if ($request->expectsJson() || $request->is('api/*')) { + if ($e instanceof TooManyRequestsHttpException) { + // keep Retry-After / X-RateLimit-* so clients can back off instead of seeing a fake server fault + return response()->json(['message' => 'Too Many Requests'], 429, $e->getHeaders()); + } $status = $this->isHttpException($e) ? $e->getStatusCode() : 500; return response()->json(['message' => 'server error'], $status); } diff --git a/app/Http/Middleware/RateLimitMiddleware.php b/app/Http/Middleware/RateLimitMiddleware.php index 6890a8deb..1fb6f8984 100644 --- a/app/Http/Middleware/RateLimitMiddleware.php +++ b/app/Http/Middleware/RateLimitMiddleware.php @@ -113,4 +113,22 @@ public function handle($request, Closure $next, $maxAttempts = 60, $decayMinutes $this->calculateRemainingAttempts($key, $maxAttempts) ); } + + /** + * Scopes the counter per HTTP method + matched route template + client, so the budget (and the + * window, which the first writer of a key fixes) of one rate limited route never leaks into another. + * The route TEMPLATE is used, not the concrete URL, so path values do not create a bucket each. + * @param \Illuminate\Http\Request $request + * @return string + */ + protected function resolveRequestSignature($request) + { + return sha1(sprintf + ( + '%s|%s|%s', + $request->getMethod(), + $request->route()?->uri() ?? $request->path(), + parent::resolveRequestSignature($request) + )); + } } \ No newline at end of file diff --git a/tests/Unit/RateLimitMiddlewareTest.php b/tests/Unit/RateLimitMiddlewareTest.php new file mode 100644 index 000000000..c5a28c5ba --- /dev/null +++ b/tests/Unit/RateLimitMiddlewareTest.php @@ -0,0 +1,99 @@ +createMock(IApiEndpointRepository::class), + $this->createMock(IEndpointRateLimitByIPRepository::class), + new RateLimiter(new Repository(new ArrayStore())) + ); + } + + private function buildRequest(string $method, string $uri, string $ip): Request + { + $request = Request::create($uri, $method, [], [], [], ['REMOTE_ADDR' => $ip]); + $request->setRouteResolver(fn() => Route::getRoutes()->match($request)); + return $request; + } + + private function next(): \Closure + { + return fn($request) => new JsonResponse(['ok' => true], 200); + } + + public function test_handle_counter_of_one_route_does_not_block_another_route_for_same_ip(): void + { + $middleware = $this->buildMiddleware(); + + // exhaust the notify budget (30 per 3600 seconds) from one IP + for ($i = 0; $i < 30; $i++) { + $middleware->handle($this->buildRequest('PUT', self::NOTIFY_URI, '10.0.0.1'), $this->next(), 30, 3600); + } + + // a different route from the same IP has its own budget + $response = $middleware->handle($this->buildRequest('GET', self::APPLY_URI, '10.0.0.1'), $this->next(), 25, 1); + + $this->assertSame(200, $response->getStatusCode()); + } + + public function test_handle_same_route_still_enforces_its_own_limit(): void + { + $middleware = $this->buildMiddleware(); + + for ($i = 0; $i < 25; $i++) { + $middleware->handle($this->buildRequest('GET', self::APPLY_URI, '10.0.0.1'), $this->next(), 25, 60); + } + + $this->expectException(ThrottleRequestsException::class); + $middleware->handle($this->buildRequest('GET', self::APPLY_URI, '10.0.0.1'), $this->next(), 25, 60); + } + + public function test_handle_429_response_is_not_masked_as_server_error(): void + { + config(['app.debug' => false]); + + $exception = new ThrottleRequestsException('Too Many Attempts.', null, ['Retry-After' => 60]); + $response = (new Handler($this->app))->render( + Request::create(self::APPLY_URI, 'GET'), + $exception + ); + + $this->assertSame(429, $response->getStatusCode()); + $this->assertSame('Too Many Requests', json_decode($response->getContent(), true)['message']); + $this->assertSame('60', $response->headers->get('Retry-After')); + } +}