Possible race condition between counting requests and reporting additional data

[doliveirakn]

We are using rack-attack and we want to report additional information for well-behaved clients.

In particular we want to inform the client of what the limit is, when it gets reset, and how many requests are remaining.

Consider some middleware that is doing something like this:
(Note: This is very similar to the sample code provided in the X-RateLimit headers for well-behaved clients section of the README, it just provides the data on every request, not just throttled ones)

class FooMiddleware
  def initialize(app)
    @app = app
  end

  def call(env)
    status, headers, body = @app.call(env)
    now = Time.now
    match_data = env['rack.attack.match_data']
    if match_data
      headers.merge!({
        'X-RateLimit-Limit' => match_data[:limit].to_s,
        'X-RateLimit-Remaining' => match_data[:limit] - match_data[:count],
        'X-RateLimit-Reset' => (now + (match_data[:period] - now.to_i % match_data[:period])).to_s
      })
    end
    [status, headers, body]
  end
end

This does the trick, but there is a race condition here.

Rack::Attack::Throttle#[] uses Rack::Attack::Cache#count to count the number of requests it has seen so far. The cache uses the following code to generate which key it is using

      def key_and_expiry(unprefixed_key, period)
        epoch_time = Time.now.to_i
        # Add 1 to expires_in to avoid timing error: http://git.io/i1PHXA
        expires_in = (period - (epoch_time % period) + 1).to_i
        ["#{prefix}:#{(epoch_time / period).to_i}:#{unprefixed_key}", expires_in]
      end

So here is the race condition.

1. the request enters the cache method at T1 (at the very end of the second)
2. that increments the counter (as expected)
3. the middleware that reports the headers uses Time.now but it is now T1+1 second
4. The X-RateLimit-Remaining has a value of the previous time frame, but the X-RateLimit-Reset has rolled over

To better illustrate this here is a series of headers that could happen

REQUEST 1 - This request processes as I would expect

HTTP/1.1 201 Created
Status: 201 Created
X-RateLimit-Limit: 300
X-RateLimit-Reset: 1518397920
X-RateLimit-Remaining: 133

REQUEST #2
This request processes as I would expect. Notice that the Reset date is the same and the RateLimit-Remaining has decreased by one.

HTTP/1.1 201 Created
Status: 201 Created
X-RateLimit-Limit: 300
X-RateLimit-Reset: 1518397920
X-RateLimit-Remaining: 132

REQUEST #3
This is where things get weird. You’ll notice that the Reset date has changed which should result in the Remaining value being reset, however, the remaining value did not reset.

HTTP/1.1 200 OK
Status: 200 OK
X-RateLimit-Limit: 300
X-RateLimit-Reset: 1518397980
X-RateLimit-Remaining: 131

REQUEST #4
Now things are as they should be. The reset and remaining are now in sync.

HTTP/1.1 201 Created
Status: 201 Created
X-RateLimit-Limit: 300
X-RateLimit-Reset: 1518397980
X-RateLimit-Remaining: 299

A solution here would be to also expose the time that was used in the match data so that anything that has access to the data can operating in the same timeframe that rack attack believes.

I've written up a possible PR that would provide this behaviour here:
#282

[grzuy]

Thank you for reporting this.

[grzuy]

Closed by #282

[grzuy]

For the record, rack-attack 5.4.0 has been released with this included.