From 8f3bf216db5a6d0a10438f183b6ba488bc0108c0 Mon Sep 17 00:00:00 2001 From: Gonzalo Rodriguez Date: Tue, 19 Jun 2018 11:36:40 -0300 Subject: [PATCH 1/4] Acceptance test MemCacheStore as a store backend doesn't leak keys --- .../acceptance/stores/mem_cache_store_spec.rb | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/spec/acceptance/stores/mem_cache_store_spec.rb b/spec/acceptance/stores/mem_cache_store_spec.rb index 2ee8832..d8684bd 100644 --- a/spec/acceptance/stores/mem_cache_store_spec.rb +++ b/spec/acceptance/stores/mem_cache_store_spec.rb @@ -1,6 +1,8 @@ require_relative "../../spec_helper" require_relative "../../support/cache_store_helper" +require "timecop" + describe "MemCacheStore as a cache backend" do before do Rack::Attack.cache.store = ActiveSupport::Cache::MemCacheStore.new @@ -11,4 +13,26 @@ describe "MemCacheStore as a cache backend" do 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_equal "1", Rack::Attack.cache.store.get(key) + + sleep 2.1 + + assert_nil Rack::Attack.cache.store.get(key) + end end From d831f2490e2a90c1840f0b1a28512d565b265af6 Mon Sep 17 00:00:00 2001 From: Gonzalo Rodriguez Date: Tue, 19 Jun 2018 12:12:45 -0300 Subject: [PATCH 2/4] Acceptance test RedisCacheStore as a store backend doesn't leak keys --- .../stores/redis_cache_store_spec.rb | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/spec/acceptance/stores/redis_cache_store_spec.rb b/spec/acceptance/stores/redis_cache_store_spec.rb index de36275..b0e84e9 100644 --- a/spec/acceptance/stores/redis_cache_store_spec.rb +++ b/spec/acceptance/stores/redis_cache_store_spec.rb @@ -1,6 +1,8 @@ require_relative "../../spec_helper" require_relative "../../support/cache_store_helper" +require "timecop" + if ActiveSupport.version >= Gem::Version.new("5.2.0") describe "RedisCacheStore as a cache backend" do before do @@ -12,5 +14,28 @@ if ActiveSupport.version >= Gem::Version.new("5.2.0") 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" + + # puts key + get "/", {}, "REMOTE_ADDR" => "1.2.3.4" + end + + assert_equal "1", Rack::Attack.cache.store.fetch(key) + + sleep 2.1 + + assert_nil Rack::Attack.cache.store.fetch(key) + end end end From 4cc8d7d854c61295220ae002bed20e97f6f36e4b Mon Sep 17 00:00:00 2001 From: Brian Kephart Date: Sun, 13 May 2018 18:18:50 -0500 Subject: [PATCH 3/4] Support ActiveSupport::RedisCacheStore --- CHANGELOG.md | 2 ++ lib/rack/attack.rb | 25 ++++++++-------- lib/rack/attack/store_proxy.rb | 4 +-- .../store_proxy/redis_cache_store_proxy.rb | 30 +++++++++++++++++++ spec/integration/rack_attack_cache_spec.rb | 5 +++- 5 files changed, 51 insertions(+), 15 deletions(-) create mode 100644 lib/rack/attack/store_proxy/redis_cache_store_proxy.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b93015..e09d049 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,8 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +- Adds support for ActiveSupport::RedisCacheStore + ## [5.2.0] - 2018-03-29 ### Added diff --git a/lib/rack/attack.rb b/lib/rack/attack.rb index 8334dc6..1140f91 100644 --- a/lib/rack/attack.rb +++ b/lib/rack/attack.rb @@ -8,18 +8,19 @@ class Rack::Attack class MisconfiguredStoreError < StandardError; end class MissingStoreError < StandardError; end - autoload :Cache, 'rack/attack/cache' - autoload :Check, 'rack/attack/check' - autoload :Throttle, 'rack/attack/throttle' - autoload :Safelist, 'rack/attack/safelist' - autoload :Blocklist, 'rack/attack/blocklist' - autoload :Track, 'rack/attack/track' - autoload :StoreProxy, 'rack/attack/store_proxy' - autoload :DalliProxy, 'rack/attack/store_proxy/dalli_proxy' - autoload :MemCacheProxy, 'rack/attack/store_proxy/mem_cache_proxy' - autoload :RedisStoreProxy, 'rack/attack/store_proxy/redis_store_proxy' - autoload :Fail2Ban, 'rack/attack/fail2ban' - autoload :Allow2Ban, 'rack/attack/allow2ban' + autoload :Cache, 'rack/attack/cache' + autoload :Check, 'rack/attack/check' + autoload :Throttle, 'rack/attack/throttle' + autoload :Safelist, 'rack/attack/safelist' + autoload :Blocklist, 'rack/attack/blocklist' + autoload :Track, 'rack/attack/track' + autoload :StoreProxy, 'rack/attack/store_proxy' + autoload :DalliProxy, 'rack/attack/store_proxy/dalli_proxy' + autoload :MemCacheProxy, 'rack/attack/store_proxy/mem_cache_proxy' + autoload :RedisStoreProxy, 'rack/attack/store_proxy/redis_store_proxy' + autoload :RedisCacheStoreProxy, 'rack/attack/store_proxy/redis_cache_store_proxy' + autoload :Fail2Ban, 'rack/attack/fail2ban' + autoload :Allow2Ban, 'rack/attack/allow2ban' class << self attr_accessor :notifier, :blocklisted_response, :throttled_response diff --git a/lib/rack/attack/store_proxy.rb b/lib/rack/attack/store_proxy.rb index d83cab9..62a7e68 100644 --- a/lib/rack/attack/store_proxy.rb +++ b/lib/rack/attack/store_proxy.rb @@ -1,9 +1,9 @@ module Rack class Attack module StoreProxy - PROXIES = [DalliProxy, MemCacheProxy, RedisStoreProxy].freeze + PROXIES = [DalliProxy, MemCacheProxy, RedisStoreProxy, RedisCacheStoreProxy].freeze - ACTIVE_SUPPORT_WRAPPER_CLASSES = Set.new(['ActiveSupport::Cache::MemCacheStore', 'ActiveSupport::Cache::RedisStore']).freeze + ACTIVE_SUPPORT_WRAPPER_CLASSES = Set.new(['ActiveSupport::Cache::MemCacheStore', 'ActiveSupport::Cache::RedisStore', 'ActiveSupport::Cache::RedisCacheStore']).freeze ACTIVE_SUPPORT_CLIENTS = Set.new(['Redis::Store', 'Dalli::Client', 'MemCache']).freeze def self.build(store) diff --git a/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb b/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb new file mode 100644 index 0000000..319445e --- /dev/null +++ b/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb @@ -0,0 +1,30 @@ +require 'delegate' + +module Rack + class Attack + module StoreProxy + class RedisCacheStoreProxy < SimpleDelegator + def self.handle?(store) + 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] + end + count.first + end + + def read(name, options = {}) + super(name, options.merge!({ raw: true })) + end + + def write(name, value, options = {}) + super(name, value, options.merge!({ raw: true })) + end + end + end + end +end diff --git a/spec/integration/rack_attack_cache_spec.rb b/spec/integration/rack_attack_cache_spec.rb index 86e4023..2e9f9e5 100644 --- a/spec/integration/rack_attack_cache_spec.rb +++ b/spec/integration/rack_attack_cache_spec.rb @@ -2,7 +2,7 @@ require_relative '../spec_helper' describe Rack::Attack::Cache do # A convenience method for deleting a key from cache. - # Slightly differnet than @cache.delete, which adds a prefix. + # Slightly different than @cache.delete, which adds a prefix. def delete(key) if @cache.store.respond_to?(:delete) @cache.store.delete(key) @@ -18,6 +18,7 @@ describe Rack::Attack::Cache do require 'active_support/cache/dalli_store' require 'active_support/cache/mem_cache_store' require 'active_support/cache/redis_store' + require 'active_support/cache/redis_cache_store' if ActiveSupport.version.to_s.to_f >= 5.2 require 'connection_pool' cache_stores = [ @@ -30,6 +31,8 @@ describe Rack::Attack::Cache do Redis::Store.new ] + cache_stores << ActiveSupport::Cache::RedisCacheStore.new if defined?(ActiveSupport::Cache::RedisCacheStore) + cache_stores.each do |store| store = Rack::Attack::StoreProxy.build(store) From 7438e5122e868ac3fe2a3455b64b312f65a628d3 Mon Sep 17 00:00:00 2001 From: Gonzalo Rodriguez Date: Tue, 19 Jun 2018 14:00:20 -0300 Subject: [PATCH 4/4] Non-leak acceptance test should only care about presence, not exact value --- spec/acceptance/stores/mem_cache_store_spec.rb | 2 +- spec/acceptance/stores/redis_cache_store_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/acceptance/stores/mem_cache_store_spec.rb b/spec/acceptance/stores/mem_cache_store_spec.rb index d8684bd..6c593f1 100644 --- a/spec/acceptance/stores/mem_cache_store_spec.rb +++ b/spec/acceptance/stores/mem_cache_store_spec.rb @@ -29,7 +29,7 @@ describe "MemCacheStore as a cache backend" do get "/", {}, "REMOTE_ADDR" => "1.2.3.4" end - assert_equal "1", Rack::Attack.cache.store.get(key) + assert Rack::Attack.cache.store.get(key) sleep 2.1 diff --git a/spec/acceptance/stores/redis_cache_store_spec.rb b/spec/acceptance/stores/redis_cache_store_spec.rb index b0e84e9..b819749 100644 --- a/spec/acceptance/stores/redis_cache_store_spec.rb +++ b/spec/acceptance/stores/redis_cache_store_spec.rb @@ -31,7 +31,7 @@ if ActiveSupport.version >= Gem::Version.new("5.2.0") get "/", {}, "REMOTE_ADDR" => "1.2.3.4" end - assert_equal "1", Rack::Attack.cache.store.fetch(key) + assert Rack::Attack.cache.store.fetch(key) sleep 2.1