System Hardening Phase 1: Catch errors at top level, take two

lib/rack/attack.rb

@@ -102,14 +102,21 @@ def initialize(app)
     end
 
     def call(env)
-      return @app.call(env) if !self.class.enabled || env["rack.attack.called"]
+      @app.call(env) if handle_call(env)
+    end
+
+    private
+
+    # Returns true if call should be forwarded to middleware.
+    def handle_call(env)
+      return true if !self.class.enabled || env["rack.attack.called"]
 
       env["rack.attack.called"] = true
       env['PATH_INFO'] = PathNormalizer.normalize_path(env['PATH_INFO'])
       request = Rack::Attack::Request.new(env)
 
       if configuration.safelisted?(request)
-        @app.call(env)
+        return true
       elsif configuration.blocklisted?(request)
         # Deprecated: Keeping blocklisted_response for backwards compatibility
         if configuration.blocklisted_response
@@ -126,8 +133,19 @@ def call(env)
         end
       else
         configuration.tracked?(request)
-        @app.call(env)
+        return true
       end
+
+      false
+    rescue *allowed_errors
+      true
+    end
+
+    def allowed_errors
+      errors = []
+      errors << Dalli::DalliError if defined?(Dalli)
+      errors << Redis::BaseConnectionError if defined?(Redis)
+      errors
     end
   end
 end

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

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

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

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"

spec/acceptance/error_handling_spec.rb

@@ -0,0 +1,54 @@
+# 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 '.call' do
+    before do
+      allow(store).to receive(:read).and_raise(raised_error)
+    end
+
+    describe 'when raising Dalli::DalliError' do
+      let(:raised_error) { stub_const('Dalli::DalliError', Class.new(StandardError)) }
+
+      it 'allows the response' do
+        get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+        assert_equal 200, last_response.status
+      end
+    end
+
+    describe 'when raising Redis::BaseError' do
+      let(:raised_error) { stub_const('Redis::BaseConnectionError', Class.new(StandardError)) }
+
+      it 'allows the response' do
+        get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+
+        assert_equal 200, last_response.status
+      end
+    end
+
+    describe 'when raising other error' do
+      let(:raised_error) { RuntimeError }
+
+      it 'raises error if not ignored' do
+        assert_raises(RuntimeError) do
+          get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+        end
+      end
+    end
+  end
+end

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"