* 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.
'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.
