From ae9d7fd4b29468430d7682c2ddc88b8a670904de Mon Sep 17 00:00:00 2001
From: shields <shields@tablecheck.com>
Date: Sun, 20 Mar 2022 18:12:37 +0900
Subject: [PATCH 1/6] This PR adds several features and changes to error
 handling:

- Catch and handle all errors once per request.
- Remove the `rescuing` blocks from the store proxies; rescuing per-method (read, write, increment) is bad because (a) it may result in undefined behavior, and (b) it will trigger repeated connection timeouts if your cache is down, e.g. N * M * timeout latency where N is the number of Rack::Attack metrics and M is the cache requests per metric.
- Add `Rack::Attack.ignored_errors` config. This defaults to Dalli::DalliError and Redis::BaseError.
- Add `Rack::Attack.failure_cooldown` config. This temporarily disables Rack::Attack after an error occurs (including ignored errors), to prevent cache connection latency. The default is 60 seconds.
- Add `Rack::Attack.error_handler` which takes a Proc for custom error handling. It's probably not needed but there may be esoteric use cases for it. You can also use the shortcut symbols :block, :throttle, and :allow to respond to errors using those.
- Add `Rack::Attack.calling?` method which uses Thread.current (or RequestStore, if available) to indicate that Rack::Attack code is executing. The reason for this is to add custom error handlers in the Rails Cache, i.e. "raise the error if it occurred while Rack::Attack was executing, so that Rack::Attack and handle it." Refer to readme.
- Add "Fault Tolerance & Error Handling" section to Readme which includes all of the above.
---
 README.md                                     | 103 ++++++++
 lib/rack/attack.rb                            | 138 +++++++++--
 lib/rack/attack/store_proxy/dalli_proxy.rb    |  30 +--
 lib/rack/attack/store_proxy/redis_proxy.rb    |  38 ++-
 .../attack/store_proxy/redis_store_proxy.rb   |   6 +-
 rack-attack.gemspec                           |   1 +
 spec/acceptance/calling_spec.rb               |  58 +++++
 spec/acceptance/error_handling_spec.rb        | 219 ++++++++++++++++++
 spec/acceptance/failure_cooldown_spec.rb      | 129 +++++++++++
 spec/spec_helper.rb                           |   5 +
 10 files changed, 659 insertions(+), 68 deletions(-)
 create mode 100644 spec/acceptance/calling_spec.rb
 create mode 100644 spec/acceptance/error_handling_spec.rb
 create mode 100644 spec/acceptance/failure_cooldown_spec.rb

diff --git a/README.md b/README.md
index d6ae610c..a026a908 100644
--- a/README.md
+++ b/README.md
@@ -37,6 +37,11 @@ See the [Backing & Hacking blog post](https://www.kickstarter.com/backing-and-ha
 - [Customizing responses](#customizing-responses)
   - [RateLimit headers for well-behaved clients](#ratelimit-headers-for-well-behaved-clients)
 - [Logging & Instrumentation](#logging--instrumentation)
+- [Fault Tolerance & Error Handling](#fault-tolerance--error-handling)
+  - [Expose Rails cache errors to Rack::Attack](#expose-rails-cache-errors-to-rackattack)
+  - [Configure cache timeout](#configure-cache-timeout)
+  - [Failure cooldown](#failure-cooldown)
+  - [Custom error handling](#custom-error-handling)
 - [Testing](#testing)
 - [How it works](#how-it-works)
   - [About Tracks](#about-tracks)
@@ -395,6 +400,104 @@ ActiveSupport::Notifications.subscribe(/rack_attack/) do |name, start, finish, r
 end
 ```
 
+## Fault Tolerance & Error Handling
+
+Rack::Attack has a mission-critical dependency on your [cache store](#cache-store-configuration).
+If the cache system experiences an outage, it may cause severe latency within Rack::Attack
+and lead to an overall application outage.
+
+This section explains how to configure your application and handle errors in order to mitigate issues.
+
+### Expose Rails cache errors to Rack::Attack
+
+If using Rails cache, by default, Rails cache will suppress any errors raised by the underlying cache store.
+You'll need to expose these errors to Rack::Attack with a custom error handler follows:
+
+```ruby
+# in your Rails config
+config.cache_store = :redis_cache_store,
+                     { # ...
+                       error_handler: -> (method:, returning:, exception:) do
+                         raise exception if Rack::Attack.calling?
+                       end }
+```
+
+By default, if a Redis or Dalli cache error occurs, Rack::Attack will ignore the error and allow the request.
+
+### Configure cache timeout
+
+In your application config, it is recommended to set your cache timeout to 0.1 seconds or lower.
+Please refer to the [Rails Guide](https://guides.rubyonrails.org/caching_with_rails.html).
+
+```ruby
+# Set 100 millisecond timeout on Redis
+config.cache_store = :redis_cache_store,
+                     { # ...
+                       connect_timeout: 0.1,
+                       read_timeout: 0.1,
+                       write_timeout: 0.1 }
+```
+
+To use different timeout values specific to Rack::Attack, you may set a
+[Rack::Attack-specific cache configuration](#cache-store-configuration).
+
+### Failure cooldown
+
+When any error occurs, Rack::Attack becomes disabled for a 60 seconds "cooldown" period.
+This prevents a cache outage from adding timeout latency on each Rack::Attack request.
+You can configure the cooldown period as follows:
+
+```ruby
+# in initializers/rack_attack.rb
+
+# Disable Rack::Attack for 5 minutes if any cache failure occurs
+Rack::Attack.failure_cooldown = 300
+
+# Do not use failure cooldown
+Rack::Attack.failure_cooldown = nil
+```
+
+### Custom error handling
+
+By default, Rack::Attack will ignore any Redis or Dalli cache errors, and raise any other errors it receives.
+Note that ignored errors will still trigger the failure cooldown. Ignored errors may be specified as Class
+or String values.
+
+```ruby
+# in initializers/rack_attack.rb
+Rack::Attack.ignored_errors += [MyErrorClass, 'MyOtherErrorClass']
+```
+
+Alternatively, you may define a custom error handler as a Proc. The error handler will receive all errors,
+regardless of whether they are on the ignore list. Your handler should return either `:allow`, `:block`,
+or `:throttle`, or else re-raise the error; other returned values will allow the request.
+
+```ruby
+# Set a custom error handler which blocks ignored errors
+# and raises all others
+Rack::Attack.error_handler = -> (error) do
+  if Rack::Attack.ignored_error?(error)
+    Rails.logger.warn("Blocking error: #{error}")
+    :block
+  else
+    raise(error)
+  end
+end
+```
+
+Lastly, you can define the error handlers as a Symbol shortcut:
+
+```ruby
+# Handle all errors with block response
+Rack::Attack.error_handler = :block
+
+# Handle all errors with throttle response
+Rack::Attack.error_handler = :throttle
+
+# Handle all errors by allowing the request
+Rack::Attack.error_handler = :allow
+```
+
 ## Testing
 
 A note on developing and testing apps using Rack::Attack - if you are using throttling in particular, you will
diff --git a/lib/rack/attack.rb b/lib/rack/attack.rb
index 9b134165..3e96e971 100644
--- a/lib/rack/attack.rb
+++ b/lib/rack/attack.rb
@@ -30,8 +30,18 @@ class IncompatibleStoreError < Error; end
     autoload :Fail2Ban,             'rack/attack/fail2ban'
     autoload :Allow2Ban,            'rack/attack/allow2ban'
 
+    THREAD_CALLING_KEY = 'rack.attack.calling'
+    DEFAULT_FAILURE_COOLDOWN = 60
+    DEFAULT_IGNORED_ERRORS = %w[Dalli::DalliError Redis::BaseError].freeze
+
     class << self
-      attr_accessor :enabled, :notifier, :throttle_discriminator_normalizer
+      attr_accessor :enabled,
+                    :notifier,
+                    :throttle_discriminator_normalizer,
+                    :error_handler,
+                    :ignored_errors,
+                    :failure_cooldown
+
       attr_reader :configuration
 
       def instrument(request)
@@ -57,6 +67,39 @@ def reset!
         cache.reset!
       end
 
+      def failed!
+        @last_failure_at = Time.now
+      end
+
+      def failure_cooldown?
+        return unless @last_failure_at && failure_cooldown
+        Time.now < @last_failure_at + failure_cooldown
+      end
+
+      def ignored_error?(error)
+        ignored_errors&.any? do |ignored_error|
+          case ignored_error
+          when String then error.class.ancestors.any? {|a| a.name == ignored_error }
+          else error.is_a?(ignored_error)
+          end
+        end
+      end
+
+      def calling?
+        !!thread_store[THREAD_CALLING_KEY]
+      end
+
+      def with_calling
+        thread_store[THREAD_CALLING_KEY] = true
+        yield
+      ensure
+        thread_store[THREAD_CALLING_KEY] = nil
+      end
+
+      def thread_store
+        defined?(RequestStore) ? RequestStore.store : Thread.current
+      end
+
       extend Forwardable
       def_delegators(
         :@configuration,
@@ -84,7 +127,11 @@ def reset!
       )
     end
 
-    # Set defaults
+    # Set class defaults
+    self.failure_cooldown = DEFAULT_FAILURE_COOLDOWN
+    self.ignored_errors = DEFAULT_IGNORED_ERRORS.dup
+
+    # Set instance defaults
     @enabled = true
     @notifier = ActiveSupport::Notifications if defined?(ActiveSupport::Notifications)
     @throttle_discriminator_normalizer = lambda do |discriminator|
@@ -100,32 +147,87 @@ def initialize(app)
     end
 
     def call(env)
-      return @app.call(env) if !self.class.enabled || env["rack.attack.called"]
+      return @app.call(env) if !self.class.enabled || env["rack.attack.called"] || self.class.failure_cooldown?
 
-      env["rack.attack.called"] = true
+      env['rack.attack.called'] = true
       env['PATH_INFO'] = PathNormalizer.normalize_path(env['PATH_INFO'])
       request = Rack::Attack::Request.new(env)
+      result = :allow
 
+      self.class.with_calling do
+        result = get_result(request)
+      rescue StandardError => error
+        return do_error_response(error, request, env)
+      end
+
+      do_response(result, request, env)
+    end
+
+    private
+
+    def get_result(request)
       if configuration.safelisted?(request)
-        @app.call(env)
+        :allow
       elsif configuration.blocklisted?(request)
-        # Deprecated: Keeping blocklisted_response for backwards compatibility
-        if configuration.blocklisted_response
-          configuration.blocklisted_response.call(env)
-        else
-          configuration.blocklisted_responder.call(request)
-        end
+        :block
       elsif configuration.throttled?(request)
-        # Deprecated: Keeping throttled_response for backwards compatibility
-        if configuration.throttled_response
-          configuration.throttled_response.call(env)
-        else
-          configuration.throttled_responder.call(request)
-        end
+        :throttle
       else
         configuration.tracked?(request)
-        @app.call(env)
+        :allow
+      end
+    end
+
+    def do_response(result, request, env)
+      case result
+      when :block then do_block_response(request, env)
+      when :throttle then do_throttle_response(request, env)
+      else @app.call(env)
+      end
+    end
+
+    def do_block_response(request, env)
+      # Deprecated: Keeping blocklisted_response for backwards compatibility
+      if configuration.blocklisted_response
+        configuration.blocklisted_response.call(env)
+      else
+        configuration.blocklisted_responder.call(request)
+      end
+    end
+
+    def do_throttle_response(request, env)
+      # Deprecated: Keeping throttled_response for backwards compatibility
+      if configuration.throttled_response
+        configuration.throttled_response.call(env)
+      else
+        configuration.throttled_responder.call(request)
+      end
+    end
+
+    def do_error_response(error, request, env)
+      self.class.failed!
+      result = error_result(error, request, env)
+      result ? do_response(result, request, env) : raise(error)
+    end
+
+    def error_result(error, request, env)
+      handler = self.class.error_handler
+      if handler
+        error_handler_result(handler, error, request, env)
+      elsif self.class.ignored_error?(error)
+        :allow
       end
     end
+
+    def error_handler_result(handler, error, request, env)
+      result = handler
+
+      if handler.is_a?(Proc)
+        args = [error, request, env].first(handler.arity)
+        result = handler.call(*args) # may raise error
+      end
+
+      %i[block throttle].include?(result) ? result : :allow
+    end
   end
 end
diff --git a/lib/rack/attack/store_proxy/dalli_proxy.rb b/lib/rack/attack/store_proxy/dalli_proxy.rb
index 48198bb2..b2914ade 100644
--- a/lib/rack/attack/store_proxy/dalli_proxy.rb
+++ b/lib/rack/attack/store_proxy/dalli_proxy.rb
@@ -24,34 +24,26 @@ def initialize(client)
         end
 
         def read(key)
-          rescuing do
-            with do |client|
-              client.get(key)
-            end
+          with do |client|
+            client.get(key)
           end
         end
 
         def write(key, value, options = {})
-          rescuing do
-            with do |client|
-              client.set(key, value, options.fetch(:expires_in, 0), raw: true)
-            end
+          with do |client|
+            client.set(key, value, options.fetch(:expires_in, 0), raw: true)
           end
         end
 
         def increment(key, amount, options = {})
-          rescuing do
-            with do |client|
-              client.incr(key, amount, options.fetch(:expires_in, 0), amount)
-            end
+          with do |client|
+            client.incr(key, amount, options.fetch(:expires_in, 0), amount)
           end
         end
 
         def delete(key)
-          rescuing do
-            with do |client|
-              client.delete(key)
-            end
+          with do |client|
+            client.delete(key)
           end
         end
 
@@ -66,12 +58,6 @@ def with
             end
           end
         end
-
-        def rescuing
-          yield
-        rescue Dalli::DalliError
-          nil
-        end
       end
     end
   end
diff --git a/lib/rack/attack/store_proxy/redis_proxy.rb b/lib/rack/attack/store_proxy/redis_proxy.rb
index 830d39de..e8d833b8 100644
--- a/lib/rack/attack/store_proxy/redis_proxy.rb
+++ b/lib/rack/attack/store_proxy/redis_proxy.rb
@@ -19,50 +19,38 @@ def self.handle?(store)
         end
 
         def read(key)
-          rescuing { get(key) }
+          get(key)
         end
 
         def write(key, value, options = {})
           if (expires_in = options[:expires_in])
-            rescuing { setex(key, expires_in, value) }
+            setex(key, expires_in, value)
           else
-            rescuing { set(key, value) }
+            set(key, value)
           end
         end
 
         def increment(key, amount, options = {})
-          rescuing do
-            pipelined do |redis|
-              redis.incrby(key, amount)
-              redis.expire(key, options[:expires_in]) if options[:expires_in]
-            end.first
-          end
+          pipelined do |redis|
+            redis.incrby(key, amount)
+            redis.expire(key, options[:expires_in]) if options[:expires_in]
+          end.first
         end
 
         def delete(key, _options = {})
-          rescuing { del(key) }
+          del(key)
         end
 
         def delete_matched(matcher, _options = nil)
           cursor = "0"
 
-          rescuing do
-            # Fetch keys in batches using SCAN to avoid blocking the Redis server.
-            loop do
-              cursor, keys = scan(cursor, match: matcher, count: 1000)
-              del(*keys) unless keys.empty?
-              break if cursor == "0"
-            end
+          # Fetch keys in batches using SCAN to avoid blocking the Redis server.
+          loop do
+            cursor, keys = scan(cursor, match: matcher, count: 1000)
+            del(*keys) unless keys.empty?
+            break if cursor == "0"
           end
         end
-
-        private
-
-        def rescuing
-          yield
-        rescue Redis::BaseConnectionError
-          nil
-        end
       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 28557bcb..7249e1d5 100644
--- a/lib/rack/attack/store_proxy/redis_store_proxy.rb
+++ b/lib/rack/attack/store_proxy/redis_store_proxy.rb
@@ -11,14 +11,14 @@ def self.handle?(store)
         end
 
         def read(key)
-          rescuing { get(key, raw: true) }
+          get(key, raw: true)
         end
 
         def write(key, value, options = {})
           if (expires_in = options[:expires_in])
-            rescuing { setex(key, expires_in, value, raw: true) }
+            setex(key, expires_in, value, raw: true)
           else
-            rescuing { set(key, value, raw: true) }
+            set(key, value, raw: true)
           end
         end
       end
diff --git a/rack-attack.gemspec b/rack-attack.gemspec
index cf7db71f..b4008caa 100644
--- a/rack-attack.gemspec
+++ b/rack-attack.gemspec
@@ -35,6 +35,7 @@ Gem::Specification.new do |s|
   s.add_development_dependency "bundler", ">= 1.17", "< 3.0"
   s.add_development_dependency 'minitest', "~> 5.11"
   s.add_development_dependency "minitest-stub-const", "~> 0.6"
+  s.add_development_dependency 'rspec-mocks', '~> 3.11.0'
   s.add_development_dependency 'rack-test', "~> 1.0"
   s.add_development_dependency 'rake', "~> 13.0"
   s.add_development_dependency "rubocop", "0.89.1"
diff --git a/spec/acceptance/calling_spec.rb b/spec/acceptance/calling_spec.rb
new file mode 100644
index 00000000..59d47262
--- /dev/null
+++ b/spec/acceptance/calling_spec.rb
@@ -0,0 +1,58 @@
+# frozen_string_literal: true
+
+require_relative '../spec_helper'
+
+describe '.with_calling' do
+
+  it 'can specify a calling scope' do
+    refute Rack::Attack.calling?
+    assert_nil Thread.current['rack.attack.calling']
+
+    Rack::Attack.with_calling do
+      assert Rack::Attack.calling?
+      assert Thread.current['rack.attack.calling']
+    end
+
+    refute Rack::Attack.calling?
+    assert_nil Thread.current['rack.attack.calling']
+  end
+
+  it 'uses RequestStore if available' do
+    store = double('RequestStore', store: {})
+    stub_const('RequestStore', store)
+
+    refute Rack::Attack.calling?
+    assert_nil Thread.current['rack.attack.calling']
+
+    Rack::Attack.with_calling do
+      assert Rack::Attack.calling?
+      assert store.store['rack.attack.calling']
+      assert_nil Thread.current['rack.attack.calling']
+    end
+
+    refute Rack::Attack.calling?
+    assert_nil store.store['rack.attack.calling']
+    assert_nil Thread.current['rack.attack.calling']
+  end
+
+  it 'is true within error handler scope' do
+    allow(Rack::Attack.cache.store).to receive(:read).and_raise(RuntimeError)
+
+    Rack::Attack.blocklist("fail2ban pentesters") do |request|
+      Rack::Attack::Fail2Ban.filter(request.ip, maxretry: 0, bantime: 600, findtime: 30) { true }
+    end
+
+    error_raised = false
+    Rack::Attack.error_handler = -> (_error) do
+      error_raised = true
+      assert Rack::Attack.calling?
+    end
+
+    refute Rack::Attack.calling?
+
+    get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+    assert error_raised
+
+    refute Rack::Attack.calling?
+  end
+end
diff --git a/spec/acceptance/error_handling_spec.rb b/spec/acceptance/error_handling_spec.rb
new file mode 100644
index 00000000..46f4b103
--- /dev/null
+++ b/spec/acceptance/error_handling_spec.rb
@@ -0,0 +1,219 @@
+# frozen_string_literal: true
+
+require_relative "../spec_helper"
+
+describe "error handling" do
+
+  let(:store) do
+    ActiveSupport::Cache::MemoryStore.new
+  end
+
+  before do
+    Rack::Attack.cache.store = store
+
+    Rack::Attack.blocklist("fail2ban pentesters") do |request|
+      Rack::Attack::Fail2Ban.filter(request.ip, maxretry: 0, bantime: 600, findtime: 30) { true }
+    end
+  end
+
+  describe '.ignored_errors' do
+    before do
+      allow(store).to receive(:read).and_raise(RuntimeError)
+    end
+
+    it 'has default value' do
+      assert_equal Rack::Attack.ignored_errors, %w[Dalli::DalliError Redis::BaseError]
+    end
+
+    it 'can get and set value' do
+      Rack::Attack.ignored_errors = %w[Foobar]
+      assert_equal Rack::Attack.ignored_errors, %w[Foobar]
+    end
+
+    it 'can ignore error as Class' do
+      Rack::Attack.ignored_errors = [RuntimeError]
+
+      get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+      assert_equal 200, last_response.status
+    end
+
+    it 'can ignore error ancestor as Class' do
+      Rack::Attack.ignored_errors = [StandardError]
+
+      get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+      assert_equal 200, last_response.status
+    end
+
+    it 'can ignore error as String' do
+      Rack::Attack.ignored_errors = %w[RuntimeError]
+
+      get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+      assert_equal 200, last_response.status
+    end
+
+    it 'can ignore error error ancestor as String' do
+      Rack::Attack.ignored_errors = %w[StandardError]
+
+      get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+      assert_equal 200, last_response.status
+    end
+
+    it 'raises error if not ignored' do
+      assert_raises(RuntimeError) do
+        get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+      end
+    end
+  end
+
+  describe '.ignored_errors?' do
+
+    it 'can match String or Class' do
+      Rack::Attack.ignored_errors = ['ArgumentError', RuntimeError]
+      assert Rack::Attack.ignored_error?(ArgumentError.new)
+      assert Rack::Attack.ignored_error?(RuntimeError.new)
+      refute Rack::Attack.ignored_error?(StandardError.new)
+    end
+
+    it 'can match Class ancestors' do
+      Rack::Attack.ignored_errors = [StandardError]
+      assert Rack::Attack.ignored_error?(ArgumentError.new)
+      refute Rack::Attack.ignored_error?(Exception.new)
+    end
+
+    it 'can match String ancestors' do
+      Rack::Attack.ignored_errors = ['StandardError']
+      assert Rack::Attack.ignored_error?(ArgumentError.new)
+      refute Rack::Attack.ignored_error?(Exception.new)
+    end
+  end
+
+  describe '.error_handler' do
+    before do
+      Rack::Attack.error_handler = error_handler if defined?(error_handler)
+      allow(store).to receive(:read).and_raise(ArgumentError)
+    end
+
+    it 'can get and set value' do
+      Rack::Attack.error_handler = :test
+      assert_equal Rack::Attack.error_handler, :test
+    end
+
+    describe 'Proc which returns :block' do
+      let(:error_handler) { ->(_error) { :block } }
+
+      it 'blocks the request' do
+        get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+        assert_equal 403, last_response.status
+      end
+    end
+
+    describe 'Proc which returns :throttle' do
+      let(:error_handler) { ->(_error) { :throttle } }
+
+      it 'throttles the request' do
+        get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+        assert_equal 429, last_response.status
+      end
+    end
+
+    describe 'Proc which returns :allow' do
+      let(:error_handler) { ->(_error) { :allow } }
+
+      it 'allows the request' do
+        get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+        assert_equal 200, last_response.status
+      end
+    end
+
+    describe 'Proc which returns nil' do
+      let(:error_handler) { ->(_error) { nil } }
+
+      it 'allows the request' do
+        get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+        assert_equal 200, last_response.status
+      end
+    end
+
+    describe 'Proc which re-raises the error' do
+      let(:error_handler) { ->(error) { raise error } }
+
+      it 'raises the error' do
+        assert_raises(ArgumentError) do
+          get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+        end
+      end
+    end
+
+    describe ':block' do
+      let(:error_handler) { :block }
+
+      it 'blocks the request' do
+        get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+        assert_equal 403, last_response.status
+      end
+    end
+
+    describe ':throttle' do
+      let(:error_handler) { :throttle }
+
+      it 'throttles the request' do
+        get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+        assert_equal 429, last_response.status
+      end
+    end
+
+    describe ':allow' do
+      let(:error_handler) { :allow }
+
+      it 'allows the request' do
+        get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+        assert_equal 200, last_response.status
+      end
+    end
+
+    describe 'non-nil value' do
+      let(:error_handler) { true }
+
+      it 'allows the request' do
+        get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+        assert_equal 200, last_response.status
+      end
+    end
+
+    describe 'nil' do
+      let(:error_handler) { nil }
+
+      it 'raises the error' do
+        assert_raises(ArgumentError) do
+          get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+        end
+      end
+    end
+
+    describe 'when error ignored' do
+      let(:error_handler) { :throttle }
+
+      before do
+        Rack::Attack.ignored_errors = [ArgumentError]
+      end
+
+      it 'calls handler despite ignored error' do
+        get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+        assert_equal 429, last_response.status
+      end
+    end
+  end
+end
diff --git a/spec/acceptance/failure_cooldown_spec.rb b/spec/acceptance/failure_cooldown_spec.rb
new file mode 100644
index 00000000..2cc7207f
--- /dev/null
+++ b/spec/acceptance/failure_cooldown_spec.rb
@@ -0,0 +1,129 @@
+# frozen_string_literal: true
+
+require_relative "../spec_helper"
+require "timecop"
+
+describe ".failure_cooldown" do
+
+  let(:store) do
+    ActiveSupport::Cache::MemoryStore.new
+  end
+  
+  let(:ignored_error) do
+    RuntimeError
+  end
+
+  before do
+    Rack::Attack.cache.store = store
+    Rack::Attack.ignored_errors << ignored_error
+
+    Rack::Attack.blocklist("fail2ban pentesters") do |request|
+      Rack::Attack::Fail2Ban.filter(request.ip, maxretry: 0, bantime: 600, findtime: 30) { true }
+    end
+  end
+
+  it 'has default value' do
+    assert_equal Rack::Attack.failure_cooldown, 60
+  end
+
+  it 'can get and set value' do
+    Rack::Attack.failure_cooldown = 123
+    assert_equal Rack::Attack.failure_cooldown, 123
+  end
+
+  it "allows requests for 60 seconds after an internal error" do
+    get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+    assert_equal 403, last_response.status
+
+    allow(store).to receive(:read).and_raise(ignored_error)
+
+    get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+    assert_equal 200, last_response.status
+
+    allow(store).to receive(:read).and_call_original
+
+    Timecop.travel(30) do
+      get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+      assert_equal 200, last_response.status
+    end
+
+    Timecop.travel(60) do
+      get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+      assert_equal 403, last_response.status
+    end
+  end
+
+  it 'raises non-ignored error' do
+    get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+    assert_equal 403, last_response.status
+
+    allow(store).to receive(:read).and_raise(ArgumentError)
+
+    assert_raises(ArgumentError) do
+      get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+    end
+  end
+
+  describe 'user-defined cooldown value' do
+
+    before do
+      Rack::Attack.failure_cooldown = 100
+    end
+
+    it "allows requests for user-defined period after an internal error" do
+      get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+      assert_equal 403, last_response.status
+
+      allow(store).to receive(:read).and_raise(ignored_error)
+
+      get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+      assert_equal 200, last_response.status
+
+      allow(store).to receive(:read).and_call_original
+
+      Timecop.travel(60) do
+        get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+        assert_equal 200, last_response.status
+      end
+
+      Timecop.travel(100) do
+        get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+        assert_equal 403, last_response.status
+      end
+    end
+  end
+
+  describe 'nil' do
+
+    before do
+      Rack::Attack.failure_cooldown = nil
+    end
+
+    it 'disables failure cooldown feature' do
+      get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+      assert_equal 403, last_response.status
+
+      allow(store).to receive(:read).and_raise(ignored_error)
+
+      get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+      assert_equal 200, last_response.status
+
+      allow(store).to receive(:read).and_call_original
+
+      get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+      assert_equal 403, last_response.status
+    end
+  end
+end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 10f856bf..ca850a0d 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -4,6 +4,7 @@
 
 require "minitest/autorun"
 require "minitest/pride"
+require "rspec/mocks/minitest_integration"
 require "rack/test"
 require "rails"
 
@@ -35,6 +36,10 @@ class MiniTest::Spec
   after do
     Rack::Attack.clear_configuration
     Rack::Attack.instance_variable_set(:@cache, nil)
+    Rack::Attack.instance_variable_set(:@last_failure_at, nil)
+    Rack::Attack.error_handler = nil
+    Rack::Attack.failure_cooldown = Rack::Attack::DEFAULT_FAILURE_COOLDOWN
+    Rack::Attack.ignored_errors = Rack::Attack::DEFAULT_IGNORED_ERRORS.dup
   end
 
   def app

From d3853e8ebf0f36a3b35714b879f2de856d749f26 Mon Sep 17 00:00:00 2001
From: shields <shields@tablecheck.com>
Date: Sun, 20 Mar 2022 18:49:36 +0900
Subject: [PATCH 2/6] Clarify custom error handling in readme

---
 README.md | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/README.md b/README.md
index a026a908..76fbc225 100644
--- a/README.md
+++ b/README.md
@@ -41,6 +41,7 @@ See the [Backing & Hacking blog post](https://www.kickstarter.com/backing-and-ha
   - [Expose Rails cache errors to Rack::Attack](#expose-rails-cache-errors-to-rackattack)
   - [Configure cache timeout](#configure-cache-timeout)
   - [Failure cooldown](#failure-cooldown)
+  - [Built-in error handling](#built-in-error-handling)
   - [Custom error handling](#custom-error-handling)
 - [Testing](#testing)
 - [How it works](#how-it-works)
@@ -457,11 +458,21 @@ Rack::Attack.failure_cooldown = 300
 Rack::Attack.failure_cooldown = nil
 ```
 
+### Built-in error handling
+
+By default, Rack::Attack "does the right thing" when internal errors occur:
+
+- If the error is a Redis or Dalli cache error, ignore the error and allow the request.
+- Otherwise, raise the error. The request will fail.
+
+All errors will trigger the failure cooldown, regardless of whether they are ignored or raised.
+
 ### Custom error handling
 
-By default, Rack::Attack will ignore any Redis or Dalli cache errors, and raise any other errors it receives.
-Note that ignored errors will still trigger the failure cooldown. Ignored errors may be specified as Class
-or String values.
+For most use cases, it is not necessary to re-configure Rack::Attack's default error handling.
+However, there are several ways you may do so.
+
+First, you may specify the list of errors to ignore as an array of Class and/or String values:
 
 ```ruby
 # in initializers/rack_attack.rb

From 78c09395cf5f9ecf1470cd238c14c4c987851c19 Mon Sep 17 00:00:00 2001
From: shields <shields@tablecheck.com>
Date: Sun, 20 Mar 2022 19:30:44 +0900
Subject: [PATCH 3/6] Improve clarity of readme

---
 README.md | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/README.md b/README.md
index 76fbc225..0c219cd6 100644
--- a/README.md
+++ b/README.md
@@ -38,10 +38,10 @@ See the [Backing & Hacking blog post](https://www.kickstarter.com/backing-and-ha
   - [RateLimit headers for well-behaved clients](#ratelimit-headers-for-well-behaved-clients)
 - [Logging & Instrumentation](#logging--instrumentation)
 - [Fault Tolerance & Error Handling](#fault-tolerance--error-handling)
+  - [Built-in error handling](#built-in-error-handling)
   - [Expose Rails cache errors to Rack::Attack](#expose-rails-cache-errors-to-rackattack)
   - [Configure cache timeout](#configure-cache-timeout)
   - [Failure cooldown](#failure-cooldown)
-  - [Built-in error handling](#built-in-error-handling)
   - [Custom error handling](#custom-error-handling)
 - [Testing](#testing)
 - [How it works](#how-it-works)
@@ -409,10 +409,24 @@ and lead to an overall application outage.
 
 This section explains how to configure your application and handle errors in order to mitigate issues.
 
+### Built-in error handling
+
+By default, Rack::Attack "does the right thing" when errors occur:
+
+- If the error is a Redis or Dalli cache error, Rack::Attack ignores the error and allow the request.
+- Otherwise, Rack::Attack re-raises the error. The request will fail.
+
+All errors will trigger a failure cooldown (see below), regardless of whether they are ignored or raised.
+
 ### Expose Rails cache errors to Rack::Attack
 
-If using Rails cache, by default, Rails cache will suppress any errors raised by the underlying cache store.
-You'll need to expose these errors to Rack::Attack with a custom error handler follows:
+If you are using Rack::Attack with Rails cache, by default, Rails cache will **suppress**
+any such errors, and Rack::Attack will not be able to handle them properly as per above.
+This can be dangerous: if your cache is timing out due to high request volume,
+for example, Rack::Attack will continue to blindly send requests to your cache and worsen the problem.
+
+When using Rails cache with `:redis_cache_store`, you'll need to expose errors to Rack::Attack
+with a custom error handler as follows:
 
 ```ruby
 # in your Rails config
@@ -420,10 +434,12 @@ config.cache_store = :redis_cache_store,
                      { # ...
                        error_handler: -> (method:, returning:, exception:) do
                          raise exception if Rack::Attack.calling?
-                       end }
+                       end
+                     }
 ```
 
-By default, if a Redis or Dalli cache error occurs, Rack::Attack will ignore the error and allow the request.
+Rails `:mem_cache_store` and `:dalli_store` suppress all Dalli errors. The recommended
+workaround is to set a [Rack::Attack-specific cache configuration](#cache-store-configuration).
 
 ### Configure cache timeout
 
@@ -436,7 +452,8 @@ config.cache_store = :redis_cache_store,
                      { # ...
                        connect_timeout: 0.1,
                        read_timeout: 0.1,
-                       write_timeout: 0.1 }
+                       write_timeout: 0.1
+                     }
 ```
 
 To use different timeout values specific to Rack::Attack, you may set a
@@ -446,6 +463,7 @@ To use different timeout values specific to Rack::Attack, you may set a
 
 When any error occurs, Rack::Attack becomes disabled for a 60 seconds "cooldown" period.
 This prevents a cache outage from adding timeout latency on each Rack::Attack request.
+The failure cooldown is triggered by all errors, regardless of whether they are ignored or handled.
 You can configure the cooldown period as follows:
 
 ```ruby
@@ -458,21 +476,12 @@ Rack::Attack.failure_cooldown = 300
 Rack::Attack.failure_cooldown = nil
 ```
 
-### Built-in error handling
-
-By default, Rack::Attack "does the right thing" when internal errors occur:
-
-- If the error is a Redis or Dalli cache error, ignore the error and allow the request.
-- Otherwise, raise the error. The request will fail.
-
-All errors will trigger the failure cooldown, regardless of whether they are ignored or raised.
-
 ### Custom error handling
 
 For most use cases, it is not necessary to re-configure Rack::Attack's default error handling.
 However, there are several ways you may do so.
 
-First, you may specify the list of errors to ignore as an array of Class and/or String values:
+First, you may specify the list of errors to ignore as an array of Class and/or String values.
 
 ```ruby
 # in initializers/rack_attack.rb

From 6af71d3f2c8ebae446837414a4e3259445b207bd Mon Sep 17 00:00:00 2001
From: shields <shields@tablecheck.com>
Date: Sun, 20 Mar 2022 19:34:43 +0900
Subject: [PATCH 4/6] Rename ignored_errors to allowed_errors

---
 README.md                                | 16 +++++-----
 lib/rack/attack.rb                       | 12 +++----
 spec/acceptance/error_handling_spec.rb   | 40 ++++++++++++------------
 spec/acceptance/failure_cooldown_spec.rb |  2 +-
 spec/spec_helper.rb                      |  2 +-
 5 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/README.md b/README.md
index 0c219cd6..089ac55a 100644
--- a/README.md
+++ b/README.md
@@ -413,10 +413,10 @@ This section explains how to configure your application and handle errors in ord
 
 By default, Rack::Attack "does the right thing" when errors occur:
 
-- If the error is a Redis or Dalli cache error, Rack::Attack ignores the error and allow the request.
+- If the error is a Redis or Dalli cache error, Rack::Attack allows the error and allow the request.
 - Otherwise, Rack::Attack re-raises the error. The request will fail.
 
-All errors will trigger a failure cooldown (see below), regardless of whether they are ignored or raised.
+All errors will trigger a failure cooldown (see below), regardless of whether they are allowed or raised.
 
 ### Expose Rails cache errors to Rack::Attack
 
@@ -463,7 +463,7 @@ To use different timeout values specific to Rack::Attack, you may set a
 
 When any error occurs, Rack::Attack becomes disabled for a 60 seconds "cooldown" period.
 This prevents a cache outage from adding timeout latency on each Rack::Attack request.
-The failure cooldown is triggered by all errors, regardless of whether they are ignored or handled.
+All errors trigger the failure cooldown, regardless of whether they are allowed or handled.
 You can configure the cooldown period as follows:
 
 ```ruby
@@ -481,22 +481,22 @@ Rack::Attack.failure_cooldown = nil
 For most use cases, it is not necessary to re-configure Rack::Attack's default error handling.
 However, there are several ways you may do so.
 
-First, you may specify the list of errors to ignore as an array of Class and/or String values.
+First, you may specify the list of errors to allow as an array of Class and/or String values.
 
 ```ruby
 # in initializers/rack_attack.rb
-Rack::Attack.ignored_errors += [MyErrorClass, 'MyOtherErrorClass']
+Rack::Attack.allowed_errors += [MyErrorClass, 'MyOtherErrorClass']
 ```
 
 Alternatively, you may define a custom error handler as a Proc. The error handler will receive all errors,
-regardless of whether they are on the ignore list. Your handler should return either `:allow`, `:block`,
+regardless of whether they are on the allow list. Your handler should return either `:allow`, `:block`,
 or `:throttle`, or else re-raise the error; other returned values will allow the request.
 
 ```ruby
-# Set a custom error handler which blocks ignored errors
+# Set a custom error handler which blocks allowed errors
 # and raises all others
 Rack::Attack.error_handler = -> (error) do
-  if Rack::Attack.ignored_error?(error)
+  if Rack::Attack.allow_error?(error)
     Rails.logger.warn("Blocking error: #{error}")
     :block
   else
diff --git a/lib/rack/attack.rb b/lib/rack/attack.rb
index 3e96e971..4561dde1 100644
--- a/lib/rack/attack.rb
+++ b/lib/rack/attack.rb
@@ -32,14 +32,14 @@ class IncompatibleStoreError < Error; end
 
     THREAD_CALLING_KEY = 'rack.attack.calling'
     DEFAULT_FAILURE_COOLDOWN = 60
-    DEFAULT_IGNORED_ERRORS = %w[Dalli::DalliError Redis::BaseError].freeze
+    DEFAULT_ALLOWED_ERRORS = %w[Dalli::DalliError Redis::BaseError].freeze
 
     class << self
       attr_accessor :enabled,
                     :notifier,
                     :throttle_discriminator_normalizer,
                     :error_handler,
-                    :ignored_errors,
+                    :allowed_errors,
                     :failure_cooldown
 
       attr_reader :configuration
@@ -76,8 +76,8 @@ def failure_cooldown?
         Time.now < @last_failure_at + failure_cooldown
       end
 
-      def ignored_error?(error)
-        ignored_errors&.any? do |ignored_error|
+      def allow_error?(error)
+        allowed_errors&.any? do |ignored_error|
           case ignored_error
           when String then error.class.ancestors.any? {|a| a.name == ignored_error }
           else error.is_a?(ignored_error)
@@ -129,7 +129,7 @@ def thread_store
 
     # Set class defaults
     self.failure_cooldown = DEFAULT_FAILURE_COOLDOWN
-    self.ignored_errors = DEFAULT_IGNORED_ERRORS.dup
+    self.allowed_errors = DEFAULT_ALLOWED_ERRORS.dup
 
     # Set instance defaults
     @enabled = true
@@ -214,7 +214,7 @@ def error_result(error, request, env)
       handler = self.class.error_handler
       if handler
         error_handler_result(handler, error, request, env)
-      elsif self.class.ignored_error?(error)
+      elsif self.class.allow_error?(error)
         :allow
       end
     end
diff --git a/spec/acceptance/error_handling_spec.rb b/spec/acceptance/error_handling_spec.rb
index 46f4b103..d624d6e5 100644
--- a/spec/acceptance/error_handling_spec.rb
+++ b/spec/acceptance/error_handling_spec.rb
@@ -16,22 +16,22 @@
     end
   end
 
-  describe '.ignored_errors' do
+  describe '.allowed_errors' do
     before do
       allow(store).to receive(:read).and_raise(RuntimeError)
     end
 
     it 'has default value' do
-      assert_equal Rack::Attack.ignored_errors, %w[Dalli::DalliError Redis::BaseError]
+      assert_equal Rack::Attack.allowed_errors, %w[Dalli::DalliError Redis::BaseError]
     end
 
     it 'can get and set value' do
-      Rack::Attack.ignored_errors = %w[Foobar]
-      assert_equal Rack::Attack.ignored_errors, %w[Foobar]
+      Rack::Attack.allowed_errors = %w[Foobar]
+      assert_equal Rack::Attack.allowed_errors, %w[Foobar]
     end
 
     it 'can ignore error as Class' do
-      Rack::Attack.ignored_errors = [RuntimeError]
+      Rack::Attack.allowed_errors = [RuntimeError]
 
       get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
 
@@ -39,7 +39,7 @@
     end
 
     it 'can ignore error ancestor as Class' do
-      Rack::Attack.ignored_errors = [StandardError]
+      Rack::Attack.allowed_errors = [StandardError]
 
       get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
 
@@ -47,7 +47,7 @@
     end
 
     it 'can ignore error as String' do
-      Rack::Attack.ignored_errors = %w[RuntimeError]
+      Rack::Attack.allowed_errors = %w[RuntimeError]
 
       get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
 
@@ -55,7 +55,7 @@
     end
 
     it 'can ignore error error ancestor as String' do
-      Rack::Attack.ignored_errors = %w[StandardError]
+      Rack::Attack.allowed_errors = %w[StandardError]
 
       get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
 
@@ -69,25 +69,25 @@
     end
   end
 
-  describe '.ignored_errors?' do
+  describe '.allowed_errors?' do
 
     it 'can match String or Class' do
-      Rack::Attack.ignored_errors = ['ArgumentError', RuntimeError]
-      assert Rack::Attack.ignored_error?(ArgumentError.new)
-      assert Rack::Attack.ignored_error?(RuntimeError.new)
-      refute Rack::Attack.ignored_error?(StandardError.new)
+      Rack::Attack.allowed_errors = ['ArgumentError', RuntimeError]
+      assert Rack::Attack.allow_error?(ArgumentError.new)
+      assert Rack::Attack.allow_error?(RuntimeError.new)
+      refute Rack::Attack.allow_error?(StandardError.new)
     end
 
     it 'can match Class ancestors' do
-      Rack::Attack.ignored_errors = [StandardError]
-      assert Rack::Attack.ignored_error?(ArgumentError.new)
-      refute Rack::Attack.ignored_error?(Exception.new)
+      Rack::Attack.allowed_errors = [StandardError]
+      assert Rack::Attack.allow_error?(ArgumentError.new)
+      refute Rack::Attack.allow_error?(Exception.new)
     end
 
     it 'can match String ancestors' do
-      Rack::Attack.ignored_errors = ['StandardError']
-      assert Rack::Attack.ignored_error?(ArgumentError.new)
-      refute Rack::Attack.ignored_error?(Exception.new)
+      Rack::Attack.allowed_errors = ['StandardError']
+      assert Rack::Attack.allow_error?(ArgumentError.new)
+      refute Rack::Attack.allow_error?(Exception.new)
     end
   end
 
@@ -206,7 +206,7 @@
       let(:error_handler) { :throttle }
 
       before do
-        Rack::Attack.ignored_errors = [ArgumentError]
+        Rack::Attack.allowed_errors = [ArgumentError]
       end
 
       it 'calls handler despite ignored error' do
diff --git a/spec/acceptance/failure_cooldown_spec.rb b/spec/acceptance/failure_cooldown_spec.rb
index 2cc7207f..e81b18a9 100644
--- a/spec/acceptance/failure_cooldown_spec.rb
+++ b/spec/acceptance/failure_cooldown_spec.rb
@@ -15,7 +15,7 @@
 
   before do
     Rack::Attack.cache.store = store
-    Rack::Attack.ignored_errors << ignored_error
+    Rack::Attack.allowed_errors << ignored_error
 
     Rack::Attack.blocklist("fail2ban pentesters") do |request|
       Rack::Attack::Fail2Ban.filter(request.ip, maxretry: 0, bantime: 600, findtime: 30) { true }
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index ca850a0d..da6d87c1 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -39,7 +39,7 @@ class MiniTest::Spec
     Rack::Attack.instance_variable_set(:@last_failure_at, nil)
     Rack::Attack.error_handler = nil
     Rack::Attack.failure_cooldown = Rack::Attack::DEFAULT_FAILURE_COOLDOWN
-    Rack::Attack.ignored_errors = Rack::Attack::DEFAULT_IGNORED_ERRORS.dup
+    Rack::Attack.allowed_errors = Rack::Attack::DEFAULT_ALLOWED_ERRORS.dup
   end
 
   def app

From 59d56191c145c0fbe59dc6ec1bd0212fbda272aa Mon Sep 17 00:00:00 2001
From: shields <shields@tablecheck.com>
Date: Mon, 21 Mar 2022 00:20:20 +0900
Subject: [PATCH 5/6] Add specs for failure_cooldown? and failed! methods

---
 lib/rack/attack.rb                       |  2 +-
 spec/acceptance/failure_cooldown_spec.rb | 31 ++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/lib/rack/attack.rb b/lib/rack/attack.rb
index 4561dde1..82c96df6 100644
--- a/lib/rack/attack.rb
+++ b/lib/rack/attack.rb
@@ -72,7 +72,7 @@ def failed!
       end
 
       def failure_cooldown?
-        return unless @last_failure_at && failure_cooldown
+        return false unless @last_failure_at && failure_cooldown
         Time.now < @last_failure_at + failure_cooldown
       end
 
diff --git a/spec/acceptance/failure_cooldown_spec.rb b/spec/acceptance/failure_cooldown_spec.rb
index e81b18a9..437b5388 100644
--- a/spec/acceptance/failure_cooldown_spec.rb
+++ b/spec/acceptance/failure_cooldown_spec.rb
@@ -127,3 +127,34 @@
     end
   end
 end
+
+describe '.failure_cooldown?' do
+
+  it 'returns false if no failure' do
+    refute Rack::Attack.failure_cooldown?
+  end
+
+  it 'returns false if failure_cooldown is nil' do
+    Rack::Attack.failure_cooldown = nil
+    refute Rack::Attack.failure_cooldown?
+  end
+
+  it 'returns true if still within cooldown period' do
+    Rack::Attack.instance_variable_set(:@last_failure_at, Time.now - 30)
+    assert Rack::Attack.failure_cooldown?
+  end
+
+  it 'returns false if cooldown period elapsed' do
+    Rack::Attack.instance_variable_set(:@last_failure_at, Time.now - 61)
+    refute Rack::Attack.failure_cooldown?
+  end
+end
+
+describe '.failed!' do
+
+  it 'sets last failure timestamp' do
+    assert_nil Rack::Attack.instance_variable_get(:@last_failure_at)
+    Rack::Attack.failed!
+    refute_nil Rack::Attack.instance_variable_get(:@last_failure_at)
+  end
+end

From 9cb782279f03ead9ad2113078339f3737ab8ca35 Mon Sep 17 00:00:00 2001
From: shields <shields@tablecheck.com>
Date: Fri, 24 Jun 2022 15:28:07 +0900
Subject: [PATCH 6/6] - Use request.env rather than separate env parameter -
 Improvements to readme

---
 README.md          | 47 +++++++++++++++++++++++++---------------------
 lib/rack/attack.rb | 43 ++++++++++++++++++++++--------------------
 2 files changed, 49 insertions(+), 41 deletions(-)

diff --git a/README.md b/README.md
index 089ac55a..91355a27 100644
--- a/README.md
+++ b/README.md
@@ -407,14 +407,17 @@ Rack::Attack has a mission-critical dependency on your [cache store](#cache-stor
 If the cache system experiences an outage, it may cause severe latency within Rack::Attack
 and lead to an overall application outage.
 
-This section explains how to configure your application and handle errors in order to mitigate issues.
+Although Rack::Attack is designed to be "fault-tolerant by default", depending on your application
+setup, additional configuration may be required. Please **read this section carefully** to understand
+how to best protect your application.
 
 ### Built-in error handling
 
-By default, Rack::Attack "does the right thing" when errors occur:
+As a Rack middleware component, Rack::Attack wraps your application's request handling endpoint.
+When an error occurs within either within Rack::Attack **or** within your application, by default:
 
-- If the error is a Redis or Dalli cache error, Rack::Attack allows the error and allow the request.
-- Otherwise, Rack::Attack re-raises the error. The request will fail.
+- If the error is a Redis or Dalli cache error, Rack::Attack logs the error then allows the request.
+- Otherwise, Rack::Attack raises the error. The request will fail.
 
 All errors will trigger a failure cooldown (see below), regardless of whether they are allowed or raised.
 
@@ -425,20 +428,22 @@ any such errors, and Rack::Attack will not be able to handle them properly as pe
 This can be dangerous: if your cache is timing out due to high request volume,
 for example, Rack::Attack will continue to blindly send requests to your cache and worsen the problem.
 
-When using Rails cache with `:redis_cache_store`, you'll need to expose errors to Rack::Attack
-with a custom error handler as follows:
+To mitigate this:
 
-```ruby
-# in your Rails config
-config.cache_store = :redis_cache_store,
-                     { # ...
-                       error_handler: -> (method:, returning:, exception:) do
-                         raise exception if Rack::Attack.calling?
-                       end
-                     }
-```
+* When using Rails cache with `:redis_cache_store`, you'll need to expose errors to Rack::Attack
+with a custom error handler as follows:
 
-Rails `:mem_cache_store` and `:dalli_store` suppress all Dalli errors. The recommended
+    ```ruby
+    # in your Rails config
+    config.cache_store = :redis_cache_store,
+                         { # ...
+                           error_handler: -> (method:, returning:, exception:) do
+                             raise exception if Rack::Attack.calling?
+                           end
+                         }
+    ```
+
+* Rails `:mem_cache_store` and `:dalli_store` suppress all Dalli errors. The recommended
 workaround is to set a [Rack::Attack-specific cache configuration](#cache-store-configuration).
 
 ### Configure cache timeout
@@ -495,9 +500,9 @@ or `:throttle`, or else re-raise the error; other returned values will allow the
 ```ruby
 # Set a custom error handler which blocks allowed errors
 # and raises all others
-Rack::Attack.error_handler = -> (error) do
+Rack::Attack.error_handler = -> (error, request) do
   if Rack::Attack.allow_error?(error)
-    Rails.logger.warn("Blocking error: #{error}")
+    Rails.logger.warn("Blocking error: #{error.class.name} from IP #{request.ip}")
     :block
   else
     raise(error)
@@ -520,9 +525,9 @@ Rack::Attack.error_handler = :allow
 
 ## Testing
 
-A note on developing and testing apps using Rack::Attack - if you are using throttling in particular, you will
-need to enable the cache in your development environment. See [Caching with Rails](http://guides.rubyonrails.org/caching_with_rails.html)
-for more on how to do this.
+When developing and testing apps using Rack::Attack, if you are using throttling in particular,
+you must enable the cache in your development environment. See
+[Caching with Rails](http://guides.rubyonrails.org/caching_with_rails.html) for how to do this.
 
 ### Disabling
 
diff --git a/lib/rack/attack.rb b/lib/rack/attack.rb
index 82c96df6..4dfe6576 100644
--- a/lib/rack/attack.rb
+++ b/lib/rack/attack.rb
@@ -73,6 +73,7 @@ def failed!
 
       def failure_cooldown?
         return false unless @last_failure_at && failure_cooldown
+
         Time.now < @last_failure_at + failure_cooldown
       end
 
@@ -149,18 +150,20 @@ def initialize(app)
     def call(env)
       return @app.call(env) if !self.class.enabled || env["rack.attack.called"] || self.class.failure_cooldown?
 
-      env['rack.attack.called'] = true
+      env["rack.attack.called"] = true
       env['PATH_INFO'] = PathNormalizer.normalize_path(env['PATH_INFO'])
       request = Rack::Attack::Request.new(env)
       result = :allow
 
       self.class.with_calling do
-        result = get_result(request)
-      rescue StandardError => error
-        return do_error_response(error, request, env)
+        begin
+          result = get_result(request)
+        rescue StandardError => error
+          return do_error_response(error, request)
+        end
       end
 
-      do_response(result, request, env)
+      do_response(result, request)
     end
 
     private
@@ -178,52 +181,52 @@ def get_result(request)
       end
     end
 
-    def do_response(result, request, env)
+    def do_response(result, request)
       case result
-      when :block then do_block_response(request, env)
-      when :throttle then do_throttle_response(request, env)
-      else @app.call(env)
+      when :block then do_block_response(request)
+      when :throttle then do_throttle_response(request)
+      else @app.call(request.env)
       end
     end
 
-    def do_block_response(request, env)
+    def do_block_response(request)
       # Deprecated: Keeping blocklisted_response for backwards compatibility
       if configuration.blocklisted_response
-        configuration.blocklisted_response.call(env)
+        configuration.blocklisted_response.call(request.env)
       else
         configuration.blocklisted_responder.call(request)
       end
     end
 
-    def do_throttle_response(request, env)
+    def do_throttle_response(request)
       # Deprecated: Keeping throttled_response for backwards compatibility
       if configuration.throttled_response
-        configuration.throttled_response.call(env)
+        configuration.throttled_response.call(request.env)
       else
         configuration.throttled_responder.call(request)
       end
     end
 
-    def do_error_response(error, request, env)
+    def do_error_response(error, request)
       self.class.failed!
-      result = error_result(error, request, env)
-      result ? do_response(result, request, env) : raise(error)
+      result = error_result(error, request)
+      result ? do_response(result, request) : raise(error)
     end
 
-    def error_result(error, request, env)
+    def error_result(error, request)
       handler = self.class.error_handler
       if handler
-        error_handler_result(handler, error, request, env)
+        error_handler_result(handler, error, request)
       elsif self.class.allow_error?(error)
         :allow
       end
     end
 
-    def error_handler_result(handler, error, request, env)
+    def error_handler_result(handler, error, request)
       result = handler
 
       if handler.is_a?(Proc)
-        args = [error, request, env].first(handler.arity)
+        args = [error, request].first(handler.arity)
         result = handler.call(*args) # may raise error
       end