rack-attack-4.3.1 RedisStoreProxy does not accept plain redis

[ryandv]

We are experiencing breakage after upgrading from rack-attack-4.2.0 to rack-attack-4.3.1, due to what appears to be an API incompatibility with redis:

app error: wrong number of arguments (4 for 3) (ArgumentError)
[...]/bundled_gems/ruby/2.1.0/gems/redis-3.2.0/lib/redis.rb:689:in `setex'
[...]/bundled_gems/ruby/2.1.0/gems/rack-attack-4.3.1/lib/rack/attack/store_proxy/redis_store_proxy.rb:22:in `write'
[...]/bundled_gems/ruby/2.1.0/gems/rack-attack-4.3.1/lib/rack/attack/cache.rb:53:in `do_count'
[...]/bundled_gems/ruby/2.1.0/gems/rack-attack-4.3.1/lib/rack/attack/cache.rb:19:in `count'
[...]/bundled_gems/ruby/2.1.0/gems/rack-attack-4.3.1/lib/rack/attack/throttle.rb:27:in `[]'
[...]/bundled_gems/ruby/2.1.0/gems/rack-attack-4.3.1/lib/rack/attack.rb:58:in `block in throttled?'
[...]/bundled_gems/ruby/2.1.0/gems/rack-attack-4.3.1/lib/rack/attack.rb:57:in `each'
[...]/bundled_gems/ruby/2.1.0/gems/rack-attack-4.3.1/lib/rack/attack.rb:57:in `any?'
[...]/bundled_gems/ruby/2.1.0/gems/rack-attack-4.3.1/lib/rack/attack.rb:57:in `throttled?'
[...]/bundled_gems/ruby/2.1.0/gems/rack-attack-4.3.1/lib/rack/attack.rb:102:in `call'

Previously, we were able to pass an instance of Redis from version 3.2.0 of the redis gem
to Rack::Attack, which satisfies the version constraint provided in the README:

Rack::Attack.cache.store = Rack::Attack::StoreProxy::RedisStoreProxy.new(
  Redis.new(some_redis_config)
)

The above configuration appears to have been recommended by others in a previous issue.

However, after this commit, it appears that the contract of RedisStoreProxy was changed to now depend on an instance of Redis::Store and not Redis, as only Redis::Store from the redis-store-1.1.5 gem
supports the fourth raw: true parameter introduced by the aforementioned commit.

This issue has been reported previously (see #136 and #190).

It appears that rack-attack now depends on redis-store extensions to the interface, which is breaking older configurations that pass a plain redis instance. Can redis be preserved as a valid store for the RedisStoreProxy, to support these pre-existing configurations?

Thanks!

[rietta]

Ran into the same issue. And rack-attack-4.2 has a CVE.

[fabianoarruda]

I'm getting this error using rack-attack 4.4.1 and redis gem v3.3.5. I'm not sure but per @ryandv comment, I would have to update my config file to fix the error? This is how it is currently set up in my initializer:

Rack::Attack.cache.store = Rack::Attack::StoreProxy::RedisStoreProxy.new(Redis.current)

Here is the backtrace:

ArgumentError: wrong number of arguments (given 4, expected 3)
/gems/redis-3.3.5/lib/redis.rb:767 in setex
/opt/rbenv/versions/2.3.1/lib/ruby/2.3.0/delegate.rb:83 in method_missing
/gems/rack-attack-4.4.1/lib/rack/attack/store_proxy/redis_store_proxy.rb:22 in write
/gems/rack-attack-4.4.1/lib/rack/attack/cache.rb:53 in do_count
/gems/rack-attack-4.4.1/lib/rack/attack/cache.rb:19 in count
/gems/rack-attack-4.4.1/lib/rack/attack/throttle.rb:27 in []
/gems/rack-attack-4.4.1/lib/rack/attack.rb:59 in block in throttled?
/gems/rack-attack-4.4.1/lib/rack/attack.rb:58 in any?
/gems/rack-attack-4.4.1/lib/rack/attack.rb:58 in throttled?
/gems/rack-attack-4.4.1/lib/rack/attack.rb:103 in call

[asheynkman]

anyone ever figured out the fix for this issue?

[allam-matsubara]

I'm still facing this issue with rack-attack version 5.0.1 and redis-server 2.8.4.

[lmansur]

This app reproduces this issue.

As @ryandv pointed out, #118 introduced this bug in order to fix some issues discussed in #113.

I'm only seeing two options right now:

1. Fix this issue by reverting Pass raw: true to prevent Ruby de/serialization. This is to make it po... #118 in version 4.5.
2. Don't fix this bug to preserve fix Pass raw: true to prevent Ruby de/serialization. This is to make it po... #118.

Option 1

Fixes this issue by reverting the contract back to what it was.

Users of Redis 3.2 will be able to keep upgrading rack-attack 4.

Users that relied on fix #118 won't be able to keep upgrading rack-attack 4. However, #118 was meant to allow this kind of code: store.write(key, 0, :expires_in => expires_in). This kind of code looks like an attempt to reset the cache, which is already supported by Cache#delete(key)

Option 2

Does not fix this issue.

Users of Redis 3.2 won't be able to keep upgrading rack-attack 4.

Users that relied on fix #118 won't be affected.

---

What do you think, @grzuy ?

[ryandv]

There is a third option, which I've submitted as a pull request.

[grzuy]

Hi @ryandv, thank you very much for detailed issue report.

From what I understand RedisStoreProxy was always meant to make just redis-store work for rack-attack, not explicitly plain redis. If plain redis worked occasionally in conjunction with RedisStoreProxy sounds like it was just a coincidence.

That said, I'll tag this feature request instead of error report.

[grzuy]

closed by #280

[grzuy]

For the record, plain Redis support added in rack-attack 5.4.0.

[strouptl]

@grzuy , I am using a config similar to the above, and am getting the same error message. I can reproduce the last four lines of the traceback in both version 4.3.0 and 5.4.0 from a rails console, as well, with the following:

s=Rack::Attack::StoreProxy::RedisStoreProxy.new(Redis.current)
s.write("test_key", "test_value", :expires_in => 1)

Is this the expected behavior?

[strouptl]

OK, I get it: the correct way to interact with a "Redis" instance (vs. Redis::Store) is to use the new Rack::Attack::StoreProxy::RedisProxy class, as this class drops the fourth argument (i.e. "raw: true" that caused @ryandv's configuration above to throw the "wrong number of arguments" (see this commit)

@grzuy, thanks for merging this in! Hopefully this will help any others who stumble across this issue.

[grzuy]

@laneystroup That's right 🎯

Kudos to @bfad and @ryandv for making it happen 👏