peterkhayes/rolling-rate-limiter

Add ability to check remaining requests without incrementing counter

jkirkwood opened this issue · 5 comments

I'm working to update the rate limiting on a login endpoint to look something like this:

  1. Check if rate limit exceeded (without incrementing counter). If rate limit has been exceeded block the request.
  2. Process login request
  3. If login request failed, increment the rate limit counter.

Essentially I just want to rate limit the route based on how many failed login attempts occur. In order to do this I need to be able to check the current rate limit at the beginning of the request without adding an entry to the counter.

At the end of a failed request I will then call the normal limiter function to increment the rate limit counter.

For the redis implementation a new function could be added that uses the current multi without the zadd command.

Is this something you're open to adding to this lib? If so, I can put together a PR.

Yeah, this seems fine. I could also imagine this could be useful for metrics purposes (e.g. viewing the current state of the rate limiter).

However, I do want to add a small warning that this does have a race condition, as it's possible that between the first request and the second request, a user might go from being blocked to not being blocked. As an example:

  • Imagine the rate limit is 3 requests in 10 units of time
  • Time 0: Check if blocked and do action
  • Time 1: Check if blocked and do action
  • Time 2: Check if blocked and do action (further actions from user are now blocked until time 5)
  • Time 4: Check if blocked (user appears to be blocked)
  • Time 5: Attempt to do action (contrary to expectations, action is not blocked)

Yeah makes sense, although I don't think this will be a problem for my use case.

I'm wondering how this new functionality should be exposed. Creating a new RateLimiter returns a single function. Would it make sense to add an optional 3rd options argument to this function, where one of the options is readOnly?

Alternatively we could expose a RateLimiter class with multiple methods, but this would obviously be a breaking change.

You could also add a property to the function? So the two operations would be rateLimiter() and rateLimiter.check()?

I can get behind that. I will update my PR to use that API.

As mentioned in your PR, there's a new version out that includes this functionality. You can see the release notes here.