The issue
---
When using rack-attack with a rails app, developers expect the request
path to be normalized. In particular, trailing slashes are stripped so
a request path "/login/" becomes "/login" by the time you're in
ActionController.
Since Rack::Attack runs before ActionDispatch, the request path is not
yet normalized. This can cause throttles and blacklists to not work as
expected.
E.g., a throttle:
throttle('logins', ...) {|req| req.path == "/login" }
would not match a request to '/login/', though Rails would route
'/login/' to the same '/login' action.
The solution
---
This patch looks if ActionDispatch's request normalization is loaded,
and if so, uses it to normalize the path before processing throttles,
blacklists, etc.
If it's not loaded, the request path is not modified.
Credit
---
Thanks to Andres Riancho at Include Security for reporting this issue.
I have a response which is a class. While I can still have my class
implement `#[]`, it does look a bit off. On the other side, having
objects, responding to #call, that are not procs is pretty common.
So I propose to invoke the responses with `#call` to let users override
it with response objects, that respond to `#call` instead of `#[]`.
It has a couple of cons:
1. If we slip a typo in the whole line, we won't easily catch it. Can
you guys spot the problem problem in the following line? Chasing such
issues is quite tricky.
```ruby
retry_after = evn['rack.attack.match_data'][:period] rescue nil
```
2. Throwing and catching an exception is quite slower than a new hash
allocation, so there is a speed benefit too.
We are guaranteed from Rack that env is a `Hash`, so we can even use
`Hash#fetch`.
```ruby
retry_after = env.fetch('rack.attack.match_data', {})[:period]
```
This reads better, but always allocates the default value hash, when the
other version allocates it only when needed. If you prefer `Hash#fetch`,
I'm fine with that, as long as we avoid `rescue nil`.
I need to filter requests on a period I need to get dynamically out of
information I have in the requests. Currently, I can work out the limit,
as it can be a `Proc`, however I can't do that with the period.
This PR adds support for that. Tried to do it in a way that doesn't
brake backwards compatibility, as periods are coerced to numbers during
`Rack::Throttle` initialization.
Fixes#69.
There was a race condition when `Time.now.to_i` changes between when
`epoch_time` is computed in line 18, and the cache request is made (and
the `key` is expired).
I.e., a throttle check starts at t0, but doesn’t reach the cache until
t1, the cache will have expired the throttle count. The request will
likely be allowed, even if the request exceeded the limit.
This has the effect of keeping keys in cache about 1 second longer than
strictly necessary. But the extra cache space seems like a good
trade-off for correct throttling.
For throttling, when the redis client throws an exception, the request
ends up getting rate limited. Modify this to be similar to how
ActiveSupport.MemCacheStore functions (the read, write and increment
methods do not raise exceptions)
