From 0f6ef47683c4dbcabc94007391e3900f214c8e7d Mon Sep 17 00:00:00 2001 From: Brad Lindsay Date: Tue, 6 Feb 2018 09:33:02 -0600 Subject: [PATCH 1/5] Add a seperate cache-store proxy for the redis gem While a cache-store proxy exists for the redis-store gem, no such proxy existed for using the redis gem itself. This fills that gap by adding such a proxy. Resolves kickstarter/rack-attack#190 --- lib/rack/attack.rb | 1 + lib/rack/attack/store_proxy.rb | 4 +- lib/rack/attack/store_proxy/redis_proxy.rb | 44 ++++++++++++++++++++++ spec/integration/rack_attack_cache_spec.rb | 3 +- 4 files changed, 49 insertions(+), 3 deletions(-) create mode 100644 lib/rack/attack/store_proxy/redis_proxy.rb diff --git a/lib/rack/attack.rb b/lib/rack/attack.rb index 5619c20b..346fbe58 100644 --- a/lib/rack/attack.rb +++ b/lib/rack/attack.rb @@ -13,6 +13,7 @@ class Rack::Attack 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 :RedisProxy, 'rack/attack/store_proxy/redis_proxy' autoload :Fail2Ban, 'rack/attack/fail2ban' autoload :Allow2Ban, 'rack/attack/allow2ban' autoload :Request, 'rack/attack/request' diff --git a/lib/rack/attack/store_proxy.rb b/lib/rack/attack/store_proxy.rb index 5ce0f52f..d41ddac9 100644 --- a/lib/rack/attack/store_proxy.rb +++ b/lib/rack/attack/store_proxy.rb @@ -1,10 +1,10 @@ module Rack class Attack module StoreProxy - PROXIES = [DalliProxy, MemCacheProxy, RedisStoreProxy].freeze + PROXIES = [DalliProxy, MemCacheProxy, RedisStoreProxy, RedisProxy].freeze ACTIVE_SUPPORT_WRAPPER_CLASSES = Set.new(['ActiveSupport::Cache::MemCacheStore', 'ActiveSupport::Cache::RedisStore']).freeze - ACTIVE_SUPPORT_CLIENTS = Set.new(['Redis::Store', 'Dalli::Client', 'MemCache']).freeze + ACTIVE_SUPPORT_CLIENTS = Set.new(['Redis::Store', 'Redis', 'Dalli::Client', 'MemCache']).freeze def self.build(store) client = unwrap_active_support_stores(store) diff --git a/lib/rack/attack/store_proxy/redis_proxy.rb b/lib/rack/attack/store_proxy/redis_proxy.rb new file mode 100644 index 00000000..ca2786c1 --- /dev/null +++ b/lib/rack/attack/store_proxy/redis_proxy.rb @@ -0,0 +1,44 @@ +require 'delegate' + +module Rack + class Attack + module StoreProxy + class RedisProxy < SimpleDelegator + def self.handle?(store) + defined?(::Redis) && store.is_a?(::Redis) + end + + def initialize(store) + super(store) + end + + def read(key) + get(key) + end + + def write(key, value, options={}) + if (expires_in = options[:expires_in]) + setex(key, expires_in, value) + else + set(key, value) + end + end + + def increment(key, amount, options={}) + count = nil + + pipelined do + count = incrby(key, amount) + expire(key, options[:expires_in]) if options[:expires_in] + end + + count.value if count + end + + def delete(key, options={}) + del(key) + end + end + end + end +end diff --git a/spec/integration/rack_attack_cache_spec.rb b/spec/integration/rack_attack_cache_spec.rb index 86e40237..354e4ba4 100644 --- a/spec/integration/rack_attack_cache_spec.rb +++ b/spec/integration/rack_attack_cache_spec.rb @@ -27,7 +27,8 @@ def sleep_until_expired ActiveSupport::Cache::MemCacheStore.new("127.0.0.1"), Dalli::Client.new, ConnectionPool.new { Dalli::Client.new }, - Redis::Store.new + Redis::Store.new, + Redis.new ] cache_stores.each do |store| From e50bfbebaa1a108026557a745220d63b3cbd21a2 Mon Sep 17 00:00:00 2001 From: Gonzalo Rodriguez Date: Tue, 26 Jun 2018 13:52:32 -0300 Subject: [PATCH 2/5] Acceptance test plain redis as a cache store backend --- .travis.yml | 1 + Appraisals | 4 +++ gemfiles/redis_4.gemfile | 7 +++++ spec/acceptance/stores/redis_spec.rb | 40 ++++++++++++++++++++++++++++ 4 files changed, 52 insertions(+) create mode 100644 gemfiles/redis_4.gemfile create mode 100644 spec/acceptance/stores/redis_spec.rb diff --git a/.travis.yml b/.travis.yml index 46bb12fd..6b99df2c 100644 --- a/.travis.yml +++ b/.travis.yml @@ -26,6 +26,7 @@ gemfile: - gemfiles/rails_5_1.gemfile - gemfiles/rails_4_2.gemfile - gemfiles/dalli2.gemfile + - gemfiles/redis_4.gemfile - gemfiles/connection_pool_dalli.gemfile - gemfiles/active_support_redis_cache_store.gemfile - gemfiles/active_support_redis_cache_store_pooled.gemfile diff --git a/Appraisals b/Appraisals index 2cabba73..1328254c 100644 --- a/Appraisals +++ b/Appraisals @@ -40,6 +40,10 @@ appraise 'dalli2' do gem 'dalli', '~> 2.0' end +appraise 'redis_4' do + gem 'redis', '~> 4.0' +end + appraise "connection_pool_dalli" do gem "connection_pool", "~> 2.2" gem "dalli", "~> 2.7" diff --git a/gemfiles/redis_4.gemfile b/gemfiles/redis_4.gemfile new file mode 100644 index 00000000..701e936c --- /dev/null +++ b/gemfiles/redis_4.gemfile @@ -0,0 +1,7 @@ +# This file was generated by Appraisal + +source "https://rubygems.org" + +gem "redis", "~> 4.0" + +gemspec path: "../" diff --git a/spec/acceptance/stores/redis_spec.rb b/spec/acceptance/stores/redis_spec.rb new file mode 100644 index 00000000..8524db65 --- /dev/null +++ b/spec/acceptance/stores/redis_spec.rb @@ -0,0 +1,40 @@ +require_relative "../../spec_helper" + +if defined?(::Redis) + require_relative "../../support/cache_store_helper" + require "timecop" + + describe "Plain redis as a cache backend" do + before do + Rack::Attack.cache.store = Redis.new + end + + after do + Rack::Attack.cache.store.flushdb + 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.get(key) + + sleep 2.1 + + assert_nil Rack::Attack.cache.store.get(key) + end + end +end From b40b5718dcf33fa14a1127b4b2a274fa2014292b Mon Sep 17 00:00:00 2001 From: Gonzalo Rodriguez Date: Fri, 29 Jun 2018 15:41:36 -0300 Subject: [PATCH 3/5] rubocop --auto-correct --- gemfiles/redis_4.gemfile | 2 ++ lib/rack/attack/store_proxy/redis_proxy.rb | 8 +++++--- spec/acceptance/stores/redis_spec.rb | 2 ++ 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/gemfiles/redis_4.gemfile b/gemfiles/redis_4.gemfile index 701e936c..4372f718 100644 --- a/gemfiles/redis_4.gemfile +++ b/gemfiles/redis_4.gemfile @@ -1,3 +1,5 @@ +# frozen_string_literal: true + # This file was generated by Appraisal source "https://rubygems.org" diff --git a/lib/rack/attack/store_proxy/redis_proxy.rb b/lib/rack/attack/store_proxy/redis_proxy.rb index ca2786c1..756c4c6d 100644 --- a/lib/rack/attack/store_proxy/redis_proxy.rb +++ b/lib/rack/attack/store_proxy/redis_proxy.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'delegate' module Rack @@ -16,7 +18,7 @@ def read(key) get(key) end - def write(key, value, options={}) + def write(key, value, options = {}) if (expires_in = options[:expires_in]) setex(key, expires_in, value) else @@ -24,7 +26,7 @@ def write(key, value, options={}) end end - def increment(key, amount, options={}) + def increment(key, amount, options = {}) count = nil pipelined do @@ -35,7 +37,7 @@ def increment(key, amount, options={}) count.value if count end - def delete(key, options={}) + def delete(key, _options = {}) del(key) end end diff --git a/spec/acceptance/stores/redis_spec.rb b/spec/acceptance/stores/redis_spec.rb index 8524db65..26745141 100644 --- a/spec/acceptance/stores/redis_spec.rb +++ b/spec/acceptance/stores/redis_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require_relative "../../spec_helper" if defined?(::Redis) From 673cf98157554d62c7c041ca0197b6da3cb85b4c Mon Sep 17 00:00:00 2001 From: Gonzalo Rodriguez Date: Tue, 26 Jun 2018 14:24:17 -0300 Subject: [PATCH 4/5] Avoid as much repetition as possible between RedisProxy and RedisStoreProxy --- lib/rack/attack/store_proxy/redis_proxy.rb | 16 ++++++++--- .../attack/store_proxy/redis_store_proxy.rb | 27 +------------------ 2 files changed, 13 insertions(+), 30 deletions(-) diff --git a/lib/rack/attack/store_proxy/redis_proxy.rb b/lib/rack/attack/store_proxy/redis_proxy.rb index 756c4c6d..69fa19d7 100644 --- a/lib/rack/attack/store_proxy/redis_proxy.rb +++ b/lib/rack/attack/store_proxy/redis_proxy.rb @@ -6,16 +6,21 @@ module Rack class Attack module StoreProxy class RedisProxy < SimpleDelegator - def self.handle?(store) - defined?(::Redis) && store.is_a?(::Redis) + def initialize(*args) + if Gem::Version.new(Redis::VERSION) < Gem::Version.new("3") + warn 'RackAttack requires Redis gem >= 3.0.0.' + end + + super(*args) end - def initialize(store) - super(store) + def self.handle?(store) + defined?(::Redis) && store.is_a?(::Redis) end def read(key) get(key) + rescue Redis::BaseError end def write(key, value, options = {}) @@ -24,6 +29,7 @@ def write(key, value, options = {}) else set(key, value) end + rescue Redis::BaseError end def increment(key, amount, options = {}) @@ -35,10 +41,12 @@ def increment(key, amount, options = {}) end count.value if count + rescue Redis::BaseError end def delete(key, _options = {}) del(key) + rescue Redis::BaseError end end end diff --git a/lib/rack/attack/store_proxy/redis_store_proxy.rb b/lib/rack/attack/store_proxy/redis_store_proxy.rb index a7ac81ae..359b542f 100644 --- a/lib/rack/attack/store_proxy/redis_store_proxy.rb +++ b/lib/rack/attack/store_proxy/redis_store_proxy.rb @@ -5,15 +5,7 @@ module Rack class Attack module StoreProxy - class RedisStoreProxy < SimpleDelegator - def initialize(*args) - if Gem::Version.new(Redis::VERSION) < Gem::Version.new("3") - warn 'RackAttack requires Redis gem >= 3.0.0.' - end - - super(*args) - end - + class RedisStoreProxy < RedisProxy def self.handle?(store) defined?(::Redis::Store) && store.is_a?(::Redis::Store) end @@ -31,23 +23,6 @@ def write(key, value, options = {}) end rescue Redis::BaseError end - - def increment(key, amount, options = {}) - count = nil - - pipelined do - count = incrby(key, amount) - expire(key, options[:expires_in]) if options[:expires_in] - end - - count.value if count - rescue Redis::BaseError - end - - def delete(key, _options = {}) - del(key) - rescue Redis::BaseError - end end end end From e295ede874d5247cd86a35972a671048c0960f3e Mon Sep 17 00:00:00 2001 From: Gonzalo Rodriguez Date: Fri, 29 Jun 2018 16:48:40 -0300 Subject: [PATCH 5/5] Use RedisStoreProxy (not RedisProxy) for Redis::Store --- lib/rack/attack/store_proxy.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rack/attack/store_proxy.rb b/lib/rack/attack/store_proxy.rb index 27544679..29bef492 100644 --- a/lib/rack/attack/store_proxy.rb +++ b/lib/rack/attack/store_proxy.rb @@ -3,7 +3,7 @@ module Rack class Attack module StoreProxy - PROXIES = [DalliProxy, MemCacheProxy, RedisProxy, RedisStoreProxy, RedisCacheStoreProxy].freeze + PROXIES = [DalliProxy, MemCacheProxy, RedisStoreProxy, RedisProxy, RedisCacheStoreProxy].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