* attempt to improve semantics for legibility
* Attempt to improve legibility by simplifying
* Make it more clear that we're calling procs/blocks here
* Enable rubocop Style/BlockDelimiters cop
* Prefer 'request' over 'req' abbreviation for legibility/clarity
* Instances of Track named 'track' not 'tracker'
Let RedisCacheStoreProxy only know and assume things about
RedisCacheStore API. Don't let it know anything about the specific redis
client behind the scenes, that's the job of RedisCacheStore only, not
ours.
Rack::Attack::PathNormalizer and Rack::Attack::Request are both
used in #call method, which is going to be used by every rack-attack
user as long as they insert the middleware in their app.
This is allows access to the same time that the cache uses for the count. This can be important for clients that want to provide rate limit information for well-behaved clients
While a cache-store proxy exists for the redis-store gem, no such proxy
existed for using the redis gem itself. This fills that gap by adding
such a proxy.
Resolveskickstarter/rack-attack#190
'defined?' is buggy in ruby 2.5.0, which under certain circumstances
users using rack-attack can hit. See issue #253.
I reported (https://bugs.ruby-lang.org/issues/14407) and
fixed (https://github.com/ruby/ruby/pull/1800) the issue in
ruby already, but i guess i would take some time before there's
a new ruby release including that fix.
So for now we would need to circumvent this bug by using
'const_defined?' instead of 'defined?' for this particular case.
More details:
Anyone using:
* ruby 2.5.0
* redis
* rack-attack without redis-store and using at least one throttle
* having a toplevel class named Store
will hit this ruby 2.5.0 bug https://bugs.ruby-lang.org/issues/14407
That's because of the following buggy behavior of 'defined?' under ruby
2.5:
```
$ ruby -v
ruby 2.5.0p0 (2017-12-25 revision 61468) [x86_64-linux]
$ irb
> class Redis
> end
=> nil
> class Store
> end
=> nil
> defined?(::Redis::Store)
=> "constant"
> ::Redis::Store
NameError (uninitialized constant Redis::Store
Did you mean? Store)
```
In v4.4.0, checking `defined?(ActiveSupport::Cache::MemCacheStore)`
could trigger an error loading dalli, which isn’t needed.
This fixes that bug, and prevents similar bugs by checking
`store.class.to_s` rather than `defined?(klass) && store.is_a?(klass)`.
Writing an automated test to ensure that dalli is truly optional is
difficult, but I was able to recreate the dalli load error in v4.4.0 by
running:
gem uninstall dalli
ruby -Ilib -ractive_support/all -ractive_support/cache/redis_store
-rrack/attack -e 'p Rack::Attack::StoreProxy.build(Redis::Store.new)'
Fixes#163
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)
401 Unauthorized suggests that the requests can be
retried with appropriate credentials. 403 explicitly
states that the request should not be repeated.
See #41
An alternate to fail2ban that allows clients until they hit the
thresholds, then blocks them. Think of it like a throttle where you can
block for more than one period.
based on gist from @ktheory https://gist.github.com/ktheory/5723534
Modified slightly to use fail2ban `filter` terminology to simplify
Rack::Attack initializer configuration (only one block is requred for
this approach instead of 2)
Throttles use a cache key with a timestamp (Time.now.to_i/period), so a
new cache key is used for each period.
No longer set an explicit expiry on each cache key (though it may
inherit a default expiry from the cache store).
Also, set env['rack.attack.throttle_data'] with info about incremented
(but not necessarily exceeded) throttles.
