Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
papapumpnz opened this issue Jan 12, 2021 · 8 comments
Open

Comments

@papapumpnz
Copy link

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

anyone else experiencing this issue?

@laurentS
Copy link
Owner

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
Copy link
Author

@laurentS still happening for me

@papapumpnz
Copy link
Author

@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
Copy link
Owner

@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
Copy link
Author

papapumpnz commented Mar 16, 2021

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants