laurentS / slowapi

A rate limiter for Starlette and FastAPI
https://pypi.org/project/slowapi/
MIT License
1.23k stars 79 forks source link

Global limit and routes setting a limit cause double response headers #33

Open papapumpnz opened 3 years ago

papapumpnz commented 3 years ago

A default limit set as :

limiter = Limiter(key_func=default_identifier,
                  default_limits="10/minute",
                  headers_enabled=True,
                  storage_uri=settings.RateLimit.redis_uri,
                  in_memory_fallback_enabled=True,
                  swallow_errors=True)
app.state.limiter = limiter
app.add_middleware(SlowAPIMiddleware)
app.add_exception_handler(RateLimitExceeded, _rate_limit_exceeded_handler)

With routes that use the global limit, eg do not use the @limiter.limit decorator they respond with headers as expected with:

 x-ratelimit-limit: 10 
 x-ratelimit-remaining: 9 
 x-ratelimit-reset: 1610412027 

But routes that use the @limitier.limit decorator now get double headers as such:

 x-ratelimit-limit: 1000,1000 
 x-ratelimit-remaining: 950,950 
 x-ratelimit-reset: 1612495738,1612495738 

and an error is thrown:

2021-01-12 14:09:37,215 [17636] ERROR: Failed to update rate limit headers. Swallowing error
Traceback (most recent call last):
  File "D:\programming\financefeast_api\venv\lib\site-packages\slowapi\extension.py", line 381, in _inject_headers
    retry_after = parsedate_to_datetime(existing_retry_after_header)
  File "C:\Python38\lib\email\utils.py", line 198, in parsedate_to_datetime
    *dtuple, tz = _parsedate_tz(data)
TypeError: cannot unpack non-iterable NoneType object

If the parameter 'swallow_errors' is set to False, SlowAPI throws an stack-trace with:

2021-01-12 13:44:28,823 [16576] ERROR: Exception in ASGI application
Traceback (most recent call last):
  File "D:\programming\financefeast_api\venv\lib\site-packages\uvicorn\protocols\http\h11_impl.py", line 394, in run_asgi
    result = await app(self.scope, self.receive, self.send)
  File "D:\programming\financefeast_api\venv\lib\site-packages\uvicorn\middleware\proxy_headers.py", line 45, in __call__
    return await self.app(scope, receive, send)
  File "D:\programming\financefeast_api\venv\lib\site-packages\uvicorn\middleware\debug.py", line 81, in __call__
    raise exc from None
  File "D:\programming\financefeast_api\venv\lib\site-packages\uvicorn\middleware\debug.py", line 78, in __call__
    await self.app(scope, receive, inner_send)
  File "D:\programming\financefeast_api\venv\lib\site-packages\fastapi\applications.py", line 199, in __call__
    await super().__call__(scope, receive, send)
  File "D:\programming\financefeast_api\venv\lib\site-packages\starlette\applications.py", line 111, in __call__
    await self.middleware_stack(scope, receive, send)
  File "D:\programming\financefeast_api\venv\lib\site-packages\starlette\middleware\errors.py", line 181, in __call__
    raise exc from None
  File "D:\programming\financefeast_api\venv\lib\site-packages\starlette\middleware\errors.py", line 159, in __call__
    await self.app(scope, receive, _send)
  File "D:\programming\financefeast_api\venv\lib\site-packages\starlette\middleware\base.py", line 25, in __call__
    response = await self.dispatch_func(request, self.call_next)
  File ".\app\middleware\request_context.py", line 42, in dispatch
    raise e
  File ".\app\middleware\request_context.py", line 38, in dispatch
    response = await call_next(request)
  File "D:\programming\financefeast_api\venv\lib\site-packages\starlette\middleware\base.py", line 45, in call_next
    task.result()
  File "D:\programming\financefeast_api\venv\lib\site-packages\starlette\middleware\base.py", line 38, in coro
    await self.app(scope, receive, send)
  File "D:\programming\financefeast_api\venv\lib\site-packages\starlette\middleware\base.py", line 25, in __call__
    response = await self.dispatch_func(request, self.call_next)
  File "D:\programming\financefeast_api\venv\lib\site-packages\slowapi\middleware.py", line 54, in dispatch
    response = limiter._inject_headers(response, request.state.view_rate_limit)
  File "D:\programming\financefeast_api\venv\lib\site-packages\slowapi\extension.py", line 381, in _inject_headers
    retry_after = parsedate_to_datetime(existing_retry_after_header)
  File "C:\Python38\lib\email\utils.py", line 198, in parsedate_to_datetime
    *dtuple, tz = _parsedate_tz(data)
TypeError: cannot unpack non-iterable NoneType object

It seems since there is an existing header on the response, a string to datetime conversion is attempted on the response header '"Retry-After", but being less that 5 characters parsedate_tz returns None and we get the error above.

Question: why are routes that define a limit return double response headers?

papapumpnz commented 3 years ago

Further investigation it seems the issue only appears when using a custom method to return the limit, but does not happen when using a string.

papapumpnz commented 3 years ago

The custom method to return a rate limit is simply:

def get_user_rate_limit() -> str:
    return "100/1 day"

So nothing fancy there. If I simply use a string on the limit decorator everything works as I would expect.

papapumpnz commented 3 years ago

anyone else experiencing this issue?

laurentS commented 3 years ago

I've been able to reproduce the issue in a test, but haven't had time to put together a complete fix. I'm not far off, I'll try to finish it over the weekend.

papapumpnz commented 3 years ago

@laurentS still happening for me

papapumpnz commented 3 years ago

@laurentS Did you manage to get a fix for this?

Its causing some weird behaviour around the slowapi/extension.__limit_decorator:async_wrapper where the second request comes through as None throwing:

  File "D:\programming\financefeast_api\venv\lib\site-packages\slowapi\extension.py", line 622, in async_wrapper
    raise Exception(
Exception: parameter `request` must be an instance of starlette.requests.Request
laurentS commented 3 years ago

@papapumpnz no, sorry. I haven't found time to work on this any further. It would probably help if you could provide a couple of test cases that reproduce the error(s) you see (and the cases where you don't see an error).

papapumpnz commented 3 years ago

Ok @laurentS forked and have a new test suite called test_double_headers that produces double headers, or single headers. Difference is the limiter decorator uses either a callable (produces double headers) or a string (produces single headers).