From 7f72d4f34aebb3db22328bdc0073d050fd0e9926 Mon Sep 17 00:00:00 2001
From: johnnyshields <27655+johnnyshields@users.noreply.github.com>
Date: Thu, 16 Nov 2023 19:34:38 +0900
Subject: [PATCH] This PR removes the `rescuing` blocks from Dalli/RedisProxy
 classes and instead catches errors at the top level.

---
 README.md                                     | 17 +++-
 lib/rack/attack.rb                            | 24 ++++-
 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/error_handling_spec.rb        | 93 +++++++++++++++++++
 spec/spec_helper.rb                           |  3 +-
 8 files changed, 156 insertions(+), 56 deletions(-)
 create mode 100644 spec/acceptance/error_handling_spec.rb

diff --git a/README.md b/README.md
index 545003cc..b38a1bc0 100644
--- a/README.md
+++ b/README.md
@@ -37,6 +37,7 @@ 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)
+- [Custom Error Handling](#custom-error-handling)
 - [Testing](#testing)
 - [How it works](#how-it-works)
   - [About Tracks](#about-tracks)
@@ -400,11 +401,21 @@ ActiveSupport::Notifications.subscribe(/rack_attack/) do |name, start, finish, r
 end
 ```
 
+## Custom Error Handling
+
+You may specify the list of internal errors to allow (i.e. not raise an error)
+as either an array of Class and/or String values.
+
+```ruby
+# in initializers/rack_attack.rb
+Rack::Attack.allowed_errors += [MyErrorClass, 'MyOtherErrorClass']
+```
+
 ## 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 c9094b21..fda47984 100644
--- a/lib/rack/attack.rb
+++ b/lib/rack/attack.rb
@@ -32,8 +32,14 @@ class IncompatibleStoreError < Error; end
     autoload :Fail2Ban,             'rack/attack/fail2ban'
     autoload :Allow2Ban,            'rack/attack/allow2ban'
 
+    DEFAULT_ALLOWED_ERRORS = %w[Dalli::DalliError Redis::BaseError].freeze
+
     class << self
-      attr_accessor :enabled, :notifier, :throttle_discriminator_normalizer
+      attr_accessor :enabled,
+                    :notifier,
+                    :throttle_discriminator_normalizer,
+                    :allowed_errors
+
       attr_reader :configuration
 
       def instrument(request)
@@ -59,6 +65,15 @@ def reset!
         cache.reset!
       end
 
+      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)
+          end
+        end
+      end
+
       extend Forwardable
       def_delegators(
         :@configuration,
@@ -86,7 +101,10 @@ def reset!
       )
     end
 
-    # Set defaults
+    # Set class defaults
+    self.allowed_errors = DEFAULT_ALLOWED_ERRORS.dup
+
+    # Set instance defaults
     @enabled = true
     @notifier = ActiveSupport::Notifications if defined?(ActiveSupport::Notifications)
     @throttle_discriminator_normalizer = lambda do |discriminator|
@@ -128,6 +146,8 @@ def call(env)
         configuration.tracked?(request)
         @app.call(env)
       end
+    rescue StandardError => error
+      self.class.allow_error?(error) ? @app.call(request.env) : raise(error)
     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 41cc7a8f..2d5c72f6 100644
--- a/rack-attack.gemspec
+++ b/rack-attack.gemspec
@@ -32,6 +32,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', "~> 2.0"
   s.add_development_dependency 'rake', "~> 13.0"
   s.add_development_dependency "rubocop", "1.12.1"
diff --git a/spec/acceptance/error_handling_spec.rb b/spec/acceptance/error_handling_spec.rb
new file mode 100644
index 00000000..8ae2121b
--- /dev/null
+++ b/spec/acceptance/error_handling_spec.rb
@@ -0,0 +1,93 @@
+# 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 '.allowed_errors' do
+    before do
+      allow(store).to receive(:read).and_raise(RuntimeError)
+    end
+
+    it 'has default value' do
+      assert_equal Rack::Attack.allowed_errors, %w[Dalli::DalliError Redis::BaseError]
+    end
+
+    it 'can get and set value' do
+      Rack::Attack.allowed_errors = %w[Foobar]
+      assert_equal Rack::Attack.allowed_errors, %w[Foobar]
+    end
+
+    it 'can ignore error as Class' do
+      Rack::Attack.allowed_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.allowed_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.allowed_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.allowed_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 '.allowed_errors?' do
+
+    it 'can match String or Class' do
+      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.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.allowed_errors = ['StandardError']
+      assert Rack::Attack.allow_error?(ArgumentError.new)
+      refute Rack::Attack.allow_error?(Exception.new)
+    end
+  end
+end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index f529e6a1..4de9e85a 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -3,7 +3,7 @@
 require "bundler/setup"
 
 require "minitest/autorun"
-require "minitest/pride"
+require "rspec/mocks/minitest_integration"
 require "rack/test"
 require "active_support"
 require "rack/attack"
@@ -35,6 +35,7 @@ class Minitest::Spec
   after do
     Rack::Attack.clear_configuration
     Rack::Attack.instance_variable_set(:@cache, nil)
+    Rack::Attack.allowed_errors = Rack::Attack::DEFAULT_ALLOWED_ERRORS.dup
   end
 
   def app