Merge pull request #350 from grzuy/cache_key_leak

[Fixes #349] Don't leak cache keys

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

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

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)

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

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 Rack::Attack.cache.store.get(key)
+
+    sleep 2.1
+
+    assert_nil Rack::Attack.cache.store.get(key)
+  end
 end

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 Rack::Attack.cache.store.fetch(key)
+
+      sleep 2.1
+
+      assert_nil Rack::Attack.cache.store.fetch(key)
+    end
   end
 end

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)