Skip to content

Bypasseable Errors for Rack Attack Proxy Instances #689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Bypasseable Errors for Rack Attack Proxy Instances #689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da9ab48
feat: Pass bypass variables into store
mpenalozag Aug 17, 2025
438c989
feat: Define methods to handle errors in a generic way in base proxy
mpenalozag Aug 17, 2025
3bb8d87
feat: Actual error handling usage within proxy instances
mpenalozag Aug 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions lib/rack/attack/base_proxy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,43 @@
module Rack
class Attack
class BaseProxy < SimpleDelegator
attr_reader :bypass_all_store_errors, :bypassable_store_errors

def initialize(store, bypass_all_store_errors: false, bypassable_store_errors: [])
Copy link
Collaborator

@santib santib Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering if it's better to have just one variable: bypassable_store_errors which can either be a Symbol (:all or :none), or an Array? Just to avoid the possible combinations of both variable sets:

  • bypass_all_store_errors: false, bypassable_store_errors: [...]
  • bypass_all_store_errors: true, bypassable_store_errors: []
  • bypass_all_store_errors: false, bypassable_store_errors: [...]
  • bypass_all_store_errors: true, bypassable_store_errors: []

Copy link
Author

@mpenalozag mpenalozag Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a good approach. We could also raise a configuration error if both variables are defined. I think the one you mention is cleaner

super(store)
@bypass_all_store_errors = bypass_all_store_errors
@bypassable_store_errors = bypassable_store_errors
end

protected

def handle_store_error(&block)
yield
rescue => error
if should_bypass_error?(error)
nil
else
raise error
end
end

private

def should_bypass_error?(error)
return true if @bypass_all_store_errors

@bypassable_store_errors.any? do |bypassable_error|
case bypassable_error
when Class
error.is_a?(bypassable_error)
when String
error.class.name == bypassable_error
else
false
end
end
end

class << self
def proxies
@@proxies ||= []
Expand Down
4 changes: 2 additions & 2 deletions lib/rack/attack/cache.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ def initialize(store: self.class.default_store)

attr_reader :store

def store=(store)
def store=(store, bypass_all_store_errors: false, bypassable_store_errors: [])
Copy link
Collaborator

@grzuy grzuy Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would this be called from the caller?
Thinking about what the end user API would be.

@store =
if (proxy = BaseProxy.lookup(store))
proxy.new(store)
proxy.new(store, bypass_all_store_errors: bypass_all_store_errors, bypassable_store_errors: bypassable_store_errors)
else
store
end
Expand Down
20 changes: 10 additions & 10 deletions lib/rack/attack/store_proxy/dalli_proxy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,37 +18,37 @@ def self.handle?(store)
end
end

def initialize(client)
super(client)
def initialize(client, **options)
super(client, **options)
stub_with_if_missing
end

def read(key)
rescuing do
handle_store_error do
with do |client|
client.get(key)
end
end
end

def write(key, value, options = {})
rescuing do
handle_store_error do
with do |client|
client.set(key, value, options.fetch(:expires_in, 0), raw: true)
end
end
end

def increment(key, amount, options = {})
rescuing do
handle_store_error do
with do |client|
client.incr(key, amount, options.fetch(:expires_in, 0), amount)
end
end
end

def delete(key)
rescuing do
handle_store_error do
with do |client|
client.delete(key)
end
Expand All @@ -67,10 +67,10 @@ def with
end
end

def rescuing
yield
rescue Dalli::DalliError
nil
def should_bypass_error?(error)
# Dalli-specific default behavior: bypass Dalli errors
return true if defined?(::Dalli::DalliError) && error.is_a?(Dalli::DalliError)
Copy link
Collaborator

@santib santib Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea to keep the existing behavior untouched, but maybe we can do something else to not have to override the should_bypass_error? method here?

maybe something like having this DalliProxy have bypassable_store_errors default to ['Dalli::DalliError'] as a String so that it doesn't fail if the gem is not installed?

Copy link
Author

@mpenalozag mpenalozag Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a good idea. Like you said, we could just define the bypassable_store_errors and let the BaseProxy class handle the rest.

Copy link
Collaborator

@santib santib Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, also we may need to decide if we want to let the rack attack user "remove" the default DalliError being rescued by default but still rescue other errors I guess..

Copy link
Author

@mpenalozag mpenalozag Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I'm open to options. I think we could define the DalliError as a predefined element in the bypassable_store_errors and be explicit that if the user defines the bypassable_store_errors then this would overwrite the default bypassable_errors

super
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions lib/rack/attack/store_proxy/mem_cache_store_proxy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ def self.handle?(store)
end

def read(name, options = {})
super(name, options.merge!(raw: true))
handle_store_error { super(name, options.merge!(raw: true)) }
end

def write(name, value, options = {})
super(name, value, options.merge!(raw: true))
handle_store_error { super(name, value, options.merge!(raw: true)) }
end
end
end
Expand Down
10 changes: 5 additions & 5 deletions lib/rack/attack/store_proxy/redis_cache_store_proxy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,25 @@ def increment(name, amount = 1, **options)
# So in order to workaround this we use RedisCacheStore#write (which sets expiration) to initialize
# the counter. After that we continue using the original RedisCacheStore#increment.
if options[:expires_in] && !read(name)
write(name, amount, options)
handle_store_error { write(name, amount, options) }

amount
else
super
handle_store_error { super }
end
end
end

def read(name, options = {})
super(name, options.merge!(raw: true))
handle_store_error { super(name, options.merge!(raw: true)) }
end

def write(name, value, options = {})
super(name, value, options.merge!(raw: true))
handle_store_error { super(name, value, options.merge!(raw: true)) }
end

def delete_matched(matcher, options = nil)
super(matcher.source, options)
handle_store_error { super(matcher.source, options) }
end
end
end
Expand Down
24 changes: 12 additions & 12 deletions lib/rack/attack/store_proxy/redis_proxy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,32 +6,32 @@ module Rack
class Attack
module StoreProxy
class RedisProxy < BaseProxy
def initialize(*args)
def initialize(store, **options)
if Gem::Version.new(Redis::VERSION) < Gem::Version.new("3")
warn 'RackAttack requires Redis gem >= 3.0.0.'
end

super(*args)
super(store, **options)
end

def self.handle?(store)
defined?(::Redis) && store.class == ::Redis
end

def read(key)
rescuing { get(key) }
handle_store_error { get(key) }
end

def write(key, value, options = {})
if (expires_in = options[:expires_in])
rescuing { setex(key, expires_in, value) }
handle_store_error { setex(key, expires_in, value) }
else
rescuing { set(key, value) }
handle_store_error { set(key, value) }
end
end

def increment(key, amount, options = {})
rescuing do
handle_store_error do
pipelined do |redis|
redis.incrby(key, amount)
redis.expire(key, options[:expires_in]) if options[:expires_in]
Expand All @@ -40,14 +40,14 @@ def increment(key, amount, options = {})
end

def delete(key, _options = {})
rescuing { del(key) }
handle_store_error { del(key) }
end

def delete_matched(matcher, _options = nil)
cursor = "0"
source = matcher.source

rescuing do
handle_store_error do
# Fetch keys in batches using SCAN to avoid blocking the Redis server.
loop do
cursor, keys = scan(cursor, match: source, count: 1000)
Expand All @@ -59,10 +59,10 @@ def delete_matched(matcher, _options = nil)

private

def rescuing
yield
rescue Redis::BaseConnectionError
nil
def should_bypass_error?(error)
# Redis-specific default behavior: bypass Redis connection errors
return true if error.is_a?(Redis::BaseConnectionError)
super
end
end
end
Expand Down
6 changes: 3 additions & 3 deletions lib/rack/attack/store_proxy/redis_store_proxy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ def self.handle?(store)
end

def read(key)
rescuing { get(key, raw: true) }
handle_store_error { get(key, raw: true) }
end

def write(key, value, options = {})
if (expires_in = options[:expires_in])
rescuing { setex(key, expires_in, value, raw: true) }
handle_store_error { setex(key, expires_in, value, raw: true) }
else
rescuing { set(key, value, raw: true) }
handle_store_error { set(key, value, raw: true) }
end
end
end
Expand Down
Loading