Skip to content
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

Jump to bottom

Add ability to use multiple instances of middleware #442

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add ability to use multiple instances of middleware #442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 9 additions & 2 deletions lib/rack/attack.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,16 @@ def reset!

attr_reader :configuration

def initialize(app)
def initialize(app, &block)
@app = app
@configuration = self.class.configuration
@configuration =
if block_given?
configuration = Configuration.new
configuration.instance_exec(&block)
Copy link

Choose a reason for hiding this comment

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

You need to make the final statement in this branch configuration, otherwise @configuration gets set to the return value of the last statement in the block, which in my case was an instance of Throttle.

Otherwise, this seems to work nicely, I've monkey patched this change into a project I'm working on it does the job wonderfully.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @jellybob for the code review.

Ideally this problem would reveal in any new test case covering the new block argument.

configuration
else
self.class.configuration
end
end

def call(env)
Expand Down
36 changes: 36 additions & 0 deletions spec/acceptance/middleware_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# frozen_string_literal: true

require_relative "../spec_helper"

describe "Middleware" do
it "is used in Rails by default" do
skip unless defined?(Rails)

app = Class.new(Rails::Application) do
config.eager_load = false
config.logger = Logger.new(nil) # avoid creating the log/ directory automatically
config.cache_store = :null_store # avoid creating tmp/ directory for cache
end

app.initialize!
assert app.middleware.include?(Rack::Attack)
end

def app
Rack::Builder.new do
use Rack::Attack do
blocklist_ip("1.2.3.4")
end
Comment on lines +21 to +23
Copy link
Collaborator

@santib santib May 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

can we actually test that multiple instances of the middleware work together?

Maybe I'm missing something but I tried adding a second middleware

  def app
    Rack::Builder.new do
      use Rack::Attack do
        blocklist_ip("1.2.3.4")
      end
      use Rack::Attack do
        blocklist_ip("4.3.2.1")
      end

      run lambda { |_env| [200, {}, ['Hello World']] }
    end.to_app
  end

  it "can be configured via a block" do
    get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
    assert_equal 403, last_response.status

    get "/", {}, "REMOTE_ADDR" => "4.3.2.1"
    assert_equal 403, last_response.status
  end

and it didn't work 🤔 thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Multiple instances on the same rack app are not working because of this guard clause

rack-attack/lib/rack/attack.rb

Line 105 in b6ce502

return @app.call(env) if !self.class.enabled || env["rack.attack.called"]

Copy link
Collaborator

@santib santib May 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ahh ok I see.. that was introduced to guarantee idempotency. But with this PR, that's no longer accurate.

What I mean is that in the past it was safe to just have Rack::Attack execute once because all configs were the same, but with this PR we could have different configs of Rack::Attack and only the first one to run will execute, the rest will silently be skipped, right?

At this point I'm starting to doubt if the benefits of the PR are greater than its drawbacks, especially around generating confusion to end users and inflicting pain on them 🤔

Pros I see:

  • Nicer way to config using the block rather than the Rack::Attack class.

Cons I see:

  • There are now 2 ways to config Rack::Attack which can increase confusion, especially since you can't use both at the same time.
  • Idempotency guard no longer works as expected, now only the config of the first middleware to run, will take place.
  • Potential conflicts between the automatically added middleware to Rails apps and the usage of the middleware with the new block method.

WDYT? Please let me know your thoughts, I'm open to discussion and changing my mind 🙌

Copy link
Member

Choose a reason for hiding this comment

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

I think this is still a good direction.

Ideally we retain compatibility with any existing configuration mechanism and redirect it to a per-instance configuration.

I don't think the idempotency fix is correct. It seems like XY problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ioquatix Do you have suggestions how to proceed?

I wish the middleware was not automatically added to rails apps some time ago 😐.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have time to provide detailed feedback until after RubyKaigi, but I can take a look then. Is it part of Rails' default stack?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it part of Rails' default stack?

Yes,

rack-attack/lib/rack/attack/railtie.rb

Lines 11 to 15 in b6ce502

class Railtie < ::Rails::Railtie
initializer "rack-attack.middleware" do |app|
app.middleware.use(Rack::Attack)
end
end

but I can take a look then

👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can introduce some configs so that you either get the old way to config + idempotency + the railtie, OR you get the new way to config rack attack with the block + ability to have multiple middlewares added + no railtie?

What I mean is that I’m ok with both approaches, but I think the key here is not mixing both of them at the same time 🤔

Copy link
Member

Choose a reason for hiding this comment

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

It's fairly straight forward to configure using a Railtie, so we could have a default middleware which gets configured via Rails' own configuration system, but that doesn't mean we can't have non-global instances.


run lambda { |_env| [200, {}, ['Hello World']] }
end.to_app
end

it "can be configured via a block" do
get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
assert_equal 403, last_response.status

get "/", {}, "REMOTE_ADDR" => "4.3.2.1"
assert_equal 200, last_response.status
end
end
20 changes: 0 additions & 20 deletions spec/acceptance/rails_middleware_spec.rb
View file
Open in desktop

This file was deleted.