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

Potential Bug in Rack-Attack Gem with Ruby on Rails #668

Closed
dadiaz18 opened this issue Aug 8, 2024 · 4 comments
Closed

Potential Bug in Rack-Attack Gem with Ruby on Rails #668

dadiaz18 opened this issue Aug 8, 2024 · 4 comments

Comments

@dadiaz18
Copy link

dadiaz18 commented Aug 8, 2024

Gem Versions:

  • rack-attack: 6.7.0
  • ruby: 3.3.4
  • rails: 7.0.8.4
  • redis 4.8.1

Context

I encountered an issue with the rack-attack gem when applied to a Rails application. I noticed that the TTL (Time-to-Live) values for Redis keys are inconsistently applied, causing problems with request rate limiting.

Problem Description

When blocking requests based on actions taken on an endpoint, the filters apply a TTL in Redis that does not align with the expected duration. This inconsistency happens randomly. For instance, if a 3-minute window is supposed to validate more than 20 hits, the Redis key should have a TTL of approximately 180 seconds. However, in some cases, it only applies a TTL of 80 seconds, making it impossible to validate the conditions because the data persists incorrectly.

This issue is not noticeable with smaller numbers of hits in a shorter time frame (e.g., 10 hits in one minute) because the TTL mismatch is less obvious. For example, the expected TTL might be 60 seconds, but it sometimes applies 30 seconds, thus obscuring the error.

Example code for implementation

RA_REDIS_CONF = {
  url: ENV.fetch("REDIS_URL"),
  ssl_params: { verify_mode: OpenSSL::SSL::VERIFY_NONE },
}

rack_attack = Rack::Attack
rack_attack.cache.store = ActiveSupport::Cache::RedisCacheStore.new(**RA_REDIS_CONF)

API_ENDPOINT = "/api/endpoint"

rack_attack.blocklist("filter 1") do |req|
  rack_attack::Allow2Ban.filter("filter_1:#{req.ip}", maxretry: 20, findtime: 180, bantime: 1.hour) do
    req.path == API_ENDPOINT && req.post?
  end
end

rack_attack.blocklist("filter 2") do |req|
  rack_attack::Allow2Ban.filter("filter_2:#{req.ip}", maxretry: 10, findtime: 60, bantime: 3.minutes) do
    req.path == API_ENDPOINT && req.post?
  end
end

rack_attack.blocklisted_responder = lambda do |request_env|
  [403, { "Content-Type" => "application/json" }, [{ error: "Too many requests. Please try again later." }.to_json]]
end

Logs of TTLs wrongly applied when I start the counters

TTL for rack::attack:95485:allow2ban:count:filter_1:14.0.4.4: --> 68  # (should be approx 180)
TTL for rack::attack:25841:allow2ban:count:filter_2:14.0.4.4: --> 33  # (should be approx 60)

Please, any possible solution related to this?

Thanks.

@santib
Copy link
Collaborator

santib commented Aug 12, 2024

Hi @dadiaz18, is it possible that this report is related to #578 ?

That the throttling/blocking happens in time windows of the specified duration but that not necessarily blocks one requester for the exact specified time?

related code

def key_and_expiry(unprefixed_key, period)
@last_epoch_time = Time.now.to_i
# Add 1 to expires_in to avoid timing error: https://github.com/rack/rack-attack/pull/85
expires_in = (period - (@last_epoch_time % period) + 1).to_i
["#{prefix}:#{(@last_epoch_time / period).to_i}:#{unprefixed_key}", expires_in]
end

@dadiaz18
Copy link
Author

The truth is that I didn’t analyze the behavior of that function, but it could be closely related. All I did was run tests on a code implemented with the example above and measured the TTL times of Redis that were assigned to track behaviors starting from the first action. I concluded that the problem wasn’t Redis or the versions I had installed; it was a custom implementation of Rack::Attack that I didn’t examine closely.

@santib
Copy link
Collaborator

santib commented Aug 12, 2024

Ok, makes sense.

I was also confused the first time I used rack-attack TBH 😅

I'll close this issue, given that it's how it has been working since the creation of the gem. That said, I'm open to change its behavior, but it should be done under a non-default configuration since it'd change a core aspect of the gem.

@santib santib closed this as completed Aug 12, 2024
@dadiaz18
Copy link
Author

I get that the current approach just does not cut it for me. Thank you!

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