From bbe3fee3618f1a5c856ea682454bada26a905012 Mon Sep 17 00:00:00 2001 From: johnnyshields <27655+johnnyshields@users.noreply.github.com> Date: Tue, 21 Nov 2023 02:50:38 +0900 Subject: [PATCH 1/5] This PR removes the `rescuing` blocks from Dalli/RedisProxy classes and instead catches errors at the top level. --- lib/rack/attack.rb | 11 ++++ 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 | 54 +++++++++++++++++++ spec/spec_helper.rb | 2 +- 7 files changed, 91 insertions(+), 51 deletions(-) create mode 100644 spec/acceptance/error_handling_spec.rb diff --git a/lib/rack/attack.rb b/lib/rack/attack.rb index c9094b21..cd5baeeb 100644 --- a/lib/rack/attack.rb +++ b/lib/rack/attack.rb @@ -128,6 +128,17 @@ def call(env) configuration.tracked?(request) @app.call(env) end + rescue *allowed_errors + @app.call(request.env) + end + + private + + def allowed_errors + errors = [] + errors << Dalli::DalliError if defined?(Dalli) + errors << Redis::BaseError if defined?(Redis) + errors 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..ae27993b --- /dev/null +++ b/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::BaseError', 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 diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index f529e6a1..bda61a84 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" From 4c59ce537408c772e5fd111f445b88d925d02f97 Mon Sep 17 00:00:00 2001 From: Johnny Shields <27655+johnnyshields@users.noreply.github.com> Date: Tue, 21 Nov 2023 12:34:25 +0900 Subject: [PATCH 2/5] Update attack.rb --- lib/rack/attack.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rack/attack.rb b/lib/rack/attack.rb index cd5baeeb..7e4b3ce6 100644 --- a/lib/rack/attack.rb +++ b/lib/rack/attack.rb @@ -137,7 +137,7 @@ def call(env) def allowed_errors errors = [] errors << Dalli::DalliError if defined?(Dalli) - errors << Redis::BaseError if defined?(Redis) + errors << Redis::BaseConnectionError if defined?(Redis) errors end end From d32166f952cfd8f0e7841462dc9501d3dba2eca5 Mon Sep 17 00:00:00 2001 From: Johnny Shields <27655+johnnyshields@users.noreply.github.com> Date: Tue, 21 Nov 2023 12:35:17 +0900 Subject: [PATCH 3/5] Update error_handling_spec.rb --- spec/acceptance/error_handling_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/acceptance/error_handling_spec.rb b/spec/acceptance/error_handling_spec.rb index ae27993b..07c2eda9 100644 --- a/spec/acceptance/error_handling_spec.rb +++ b/spec/acceptance/error_handling_spec.rb @@ -32,7 +32,7 @@ end describe 'when raising Redis::BaseError' do - let(:raised_error) { stub_const('Redis::BaseError', Class.new(StandardError)) } + let(:raised_error) { stub_const('Redis::BaseConnectionError', Class.new(StandardError)) } it 'allows the response' do get "/", {}, "REMOTE_ADDR" => "1.2.3.4" From dec27d2d90f4c70cf3314f780142146055caf91c Mon Sep 17 00:00:00 2001 From: Johnny Shields <27655+johnnyshields@users.noreply.github.com> Date: Wed, 22 Nov 2023 15:57:31 +0900 Subject: [PATCH 4/5] Update attack.rb --- lib/rack/attack.rb | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/rack/attack.rb b/lib/rack/attack.rb index 7e4b3ce6..435c5070 100644 --- a/lib/rack/attack.rb +++ b/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"] + return @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,14 +133,14 @@ def call(env) end else configuration.tracked?(request) - @app.call(env) + return true end + + false rescue *allowed_errors - @app.call(request.env) + true end - private - def allowed_errors errors = [] errors << Dalli::DalliError if defined?(Dalli) From 1b600a35f6935df80f8527eb1cd24ea836fc1346 Mon Sep 17 00:00:00 2001 From: Johnny Shields <27655+johnnyshields@users.noreply.github.com> Date: Fri, 24 Nov 2023 02:49:44 +0900 Subject: [PATCH 5/5] Update attack.rb --- lib/rack/attack.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rack/attack.rb b/lib/rack/attack.rb index 435c5070..bb3d5efc 100644 --- a/lib/rack/attack.rb +++ b/lib/rack/attack.rb @@ -102,7 +102,7 @@ def initialize(app) end def call(env) - return @app.call(env) if handle_call(env) + @app.call(env) if handle_call(env) end private