From e83080458635c5ad3b0ff10b72be2e5328fdaa7f Mon Sep 17 00:00:00 2001 From: Gonzalo Rodriguez Date: Wed, 20 Jun 2018 17:47:21 -0300 Subject: [PATCH 1/5] Acceptance test pooled RedisCacheStore as a backend store --- .../stores/redis_cache_store_pooled_spec.rb | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 spec/acceptance/stores/redis_cache_store_pooled_spec.rb diff --git a/spec/acceptance/stores/redis_cache_store_pooled_spec.rb b/spec/acceptance/stores/redis_cache_store_pooled_spec.rb new file mode 100644 index 0000000..cd54b30 --- /dev/null +++ b/spec/acceptance/stores/redis_cache_store_pooled_spec.rb @@ -0,0 +1,40 @@ +require_relative "../../spec_helper" +require_relative "../../support/cache_store_helper" + +require "timecop" + +if ActiveSupport.version >= Gem::Version.new("5.2.0") + describe "RedisCacheStore (pooled) as a cache backend" do + before do + Rack::Attack.cache.store = ActiveSupport::Cache::RedisCacheStore.new(pool_size: 2) + end + + after do + Rack::Attack.cache.store.clear + end + + it_works_for_cache_backed_features + + it "doesn't leak keys" do + Rack::Attack.throttle("by ip", limit: 1, period: 1) do |request| + request.ip + end + + key = nil + + # Freeze time during these statement to be sure that the key used by rack attack is the same + # we pre-calculate in local variable `key` + Timecop.freeze do + key = "rack::attack:#{Time.now.to_i}:by ip:1.2.3.4" + + get "/", {}, "REMOTE_ADDR" => "1.2.3.4" + end + + assert Rack::Attack.cache.store.fetch(key) + + sleep 2.1 + + assert_nil Rack::Attack.cache.store.fetch(key) + end + end +end From 3caee5c3ca6e91ff3670f7a62f858229a3c36772 Mon Sep 17 00:00:00 2001 From: Alexey Vasiliev Date: Wed, 20 Jun 2018 22:11:46 +0300 Subject: [PATCH 2/5] Fix usage of RedisCacheStore for rails 5.2.0 --- lib/rack/attack/store_proxy/redis_cache_store_proxy.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb b/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb index 8bb3f46..dcf085f 100644 --- a/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb +++ b/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb @@ -10,11 +10,13 @@ module Rack def increment(name, amount, options = {}) # Redis doesn't check expiration on the INCRBY command. See https://redis.io/commands/expire - count = redis.pipelined do - redis.incrby(name, amount) - redis.expire(name, options[:expires_in]) if options[:expires_in] + redis.with do |r| + count = r.pipelined do + r.incrby(name, amount) + r.expire(name, options[:expires_in]) if options[:expires_in] + end + count.first end - count.first end def read(name, options = {}) From 3af7394b6ac6ae97e2cd63eda18daa92a29d0e6b Mon Sep 17 00:00:00 2001 From: Gonzalo Rodriguez Date: Wed, 20 Jun 2018 18:32:55 -0300 Subject: [PATCH 3/5] Refactor RedisCacheStoreProxy to unlearn everything about redis client details to make it less prone to bugs in the future 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. --- .../store_proxy/redis_cache_store_proxy.rb | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb b/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb index dcf085f..2b506f3 100644 --- a/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb +++ b/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb @@ -9,13 +9,16 @@ module Rack end def increment(name, amount, options = {}) - # Redis doesn't check expiration on the INCRBY command. See https://redis.io/commands/expire - redis.with do |r| - count = r.pipelined do - r.incrby(name, amount) - r.expire(name, options[:expires_in]) if options[:expires_in] - end - count.first + # RedisCacheStore#increment ignores options[:expires_in]. + # + # So in order to workaround this we use RedisCacheStore#write (which sets expiration) to initialize + # the counter. After that we continue using the original RedisCacheStore#increment. + if options[:expires_in] && !read(name) + write(name, 1, options) + + 1 + else + super end end From ca2e75293780d150f9bd6ab79a6c381b2640f20d Mon Sep 17 00:00:00 2001 From: Gonzalo Rodriguez Date: Wed, 20 Jun 2018 19:14:02 -0300 Subject: [PATCH 4/5] Honor amount argument instead of hard coding counter --- lib/rack/attack/store_proxy/redis_cache_store_proxy.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb b/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb index 2b506f3..f14e678 100644 --- a/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb +++ b/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb @@ -14,7 +14,7 @@ module Rack # So in order to workaround this we use RedisCacheStore#write (which sets expiration) to initialize # the counter. After that we continue using the original RedisCacheStore#increment. if options[:expires_in] && !read(name) - write(name, 1, options) + write(name, amount, options) 1 else From 2c1cbc323ee8dfca4d43e02d43af023ae2b0da3f Mon Sep 17 00:00:00 2001 From: Gonzalo Rodriguez Date: Wed, 20 Jun 2018 19:14:37 -0300 Subject: [PATCH 5/5] Default increment amount to 1 as RedisCacheStore --- lib/rack/attack/store_proxy/redis_cache_store_proxy.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb b/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb index f14e678..810994c 100644 --- a/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb +++ b/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb @@ -8,7 +8,7 @@ module Rack defined?(::ActiveSupport::Cache::RedisCacheStore) && store.is_a?(::ActiveSupport::Cache::RedisCacheStore) end - def increment(name, amount, options = {}) + def increment(name, amount = 1, options = {}) # RedisCacheStore#increment ignores options[:expires_in]. # # So in order to workaround this we use RedisCacheStore#write (which sets expiration) to initialize