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..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,13 +8,18 @@ module Rack defined?(::ActiveSupport::Cache::RedisCacheStore) && store.is_a?(::ActiveSupport::Cache::RedisCacheStore) end - 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] + 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 + # the counter. After that we continue using the original RedisCacheStore#increment. + if options[:expires_in] && !read(name) + write(name, amount, options) + + 1 + else + super end - count.first end def read(name, options = {}) 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