v6.2 auto-load prevents specific middleware placement

[zarqman]

Upon upgrading to 6.2.1 (from 6.1.0), we end up with Rack::Attack twice in our middleware stack.

Actual behavior:

$ rails middleware
...
use Rack::Head
use Rack::ConditionalGet
use Authenticator::PartOne
use Rack::Attack     # <--- our intended Rack::Attack instance
use Authenticator::PartTwo
use Rack::Attack     # <--- the duplicate now added in 6.2 that we don't want
run OurApp::Application.routes

Expected behavior:

$ rails middleware
...
use Authenticator::PartOne
use Rack::Attack     # <--- our intended Rack::Attack instance
use Authenticator::PartTwo
run OurApp::Application.routes

As you can see, our specific case involves a two-phase authentication middleware stack that is specifically designed to surround Rack::Attack, so we cannot move Rack::Attack to the end of the stack, where it installs itself by default.

I see that 6.2.1 was intended to turn the second instance into a no-op, but we'd much prefer avoiding the double load to begin with.

Given the need to configure Rack::Attack anyway, one could argue against auto-loading the middleware. However, I'd like to instead propose a couple of ways to allow disabling the auto-load for advanced configurations like ours, while preserving it as the new default behavior.

1. Add a config option Rack::Attack.auto_load = true. Defaults to true. When false, the initializer would skip the auto-load.

2. Rework the gem's default require by moving most of lib/rack/attack.rb into a new file. Replace the original file with nothing but:

require 'rack/attack/<newfile>'
require 'rack/attack/railtie' if defined?(::Rails)

This would allow advanced configurations to set the Gemfile as

gem 'rack-attack', require: 'rack/attack/<newfile>'

which would implicitly disable the auto-load railtie.

Either way preserves your new default while adding a path for more specific configuration for those of us with apps with advanced middleware configurations.

Are you open to either of the above solutions? If at least one is acceptable, I'm happy to prepare a PR accordingly if that'd be helpful.

FWIW, either of the above would also allow the removal of the already-called condition (return ... if ... env["rack.attack.called"]) added in #457, which I think may break/prevent PR #442.

[grzuy]

Hi @zarqman,

Thank you for the detailed issue report!

How are you inserting Authenticator::PartTwo middleware?

[zarqman]

@grzuy, appreciate the quick response.

As added background, part of our auth process dynamically affects Rack::Attack's rate limits. Because of that tight relationship, we use the following to assure everything remains adjacent. From application.rb:

config.middleware.use Rack::Attack
config.middleware.insert_before Rack::Attack, Authenticator::PartOne
config.middleware.insert_after Rack::Attack, Authenticator::PartTwo

[fatkodima]

use Rack::Attack # <--- the duplicate now added in 6.2 that we don't want

I see that 6.2.1 was intended to turn the second instance into a no-op, but we'd much prefer avoiding the double load to begin with.

From reading the issue description, I didn't get what the problem with this:

$ rails middleware
...
use Rack::Head
use Rack::ConditionalGet
use Authenticator::PartOne
use Rack::Attack     # <--- our intended Rack::Attack instance
use Authenticator::PartTwo
use Rack::Attack     # <--- the duplicate now added in 6.2 that we don't want
run OurApp::Application.routes

Can you elaborate more what is the problem with second no-op Rack::Attack middleware?

[zarqman]

@fatkodima, there are a few reasons we'd rather not have duplicate copies of Rack::Attack in the middleware stack:

• It adds confusion to any developer who looks at the stack in the future, raising questions like, "Is it actually executing twice?" That Rack::Attack will self no-op on the second execution is non-obvious.
• It adds an extra few lines to stack traces. Minor, but one more bit of stuff to sift through.
• We're using this in an API (probably not uncommon for Rack::Attack) and every bit of performance counts, especially in the middleware chain. We've carefully deleted several unneeded default middlewares for the same reason.
• The self no-op also breaks PR Add ability to use multiple instances of middleware #442 or any other path of intentionally running multiple instances of Rack::Attack. This isn't our need, but I did see that it was something being explored.

Are you open to adding something like the Rack::Attack.auto_load parameter proposed above?

[fatkodima]

1. yes, valid reason, but not critical
2. agree, but minor
3. avoiding calling basically empty method won't save anything, unless in app with tens of thousands requests per second, I guess
4. yes, agree, after merging of that PR, auto-including middleware will not make sense if somebody will start using multiple instances of rack-attack or a new proposed syntax for configuring. But will still work for current users usecases (monkey-patching Rack::Attack class and defining configuration there).

I would rather prefer to revert to original explicit need to include rack-attack middleware, than introducing any more flags or parameters. But let's wait for @grzuy thoughts.

[mt-kelvintaywl]

moving forward, is there any way we can configure Rack::Attack to not auto-load?
I have a use case in where I'd like to specify the exact position of this middleware in the chain of my middleware as above, and i'd prefer not having a duplicated (2nd) Rack::Attack middleware if so.

Thank you!

[grzuy]

@fatkodima, there are a few reasons we'd rather not have duplicate copies of Rack::Attack in the middleware stack:

* It adds confusion to any developer who looks at the stack in the future, raising questions like, "Is it actually executing twice?" That Rack::Attack will self no-op on the second execution is non-obvious.

* It adds an extra few lines to stack traces. Minor, but one more bit of stuff to sift through.

* We're using this in an API (probably not uncommon for Rack::Attack) and every bit of performance counts, especially in the middleware chain. We've carefully deleted several unneeded default middlewares for the same reason.

* The self no-op also breaks PR #442 or any other path of intentionally running multiple instances of Rack::Attack. This isn't our need, but I did see that it was something being explored.

@zarqman Aside from these 4 listed concerns, is your custom "2 step authentication" still working with rack-attack v6.2 then?

Are you open to adding something like the Rack::Attack.auto_load parameter proposed above?

[zarqman]

@grzuy Correct, no other changes in 6.2 have broken anything else.

[grzuy]

I would rather prefer to revert to original explicit need to include rack-attack middleware, than introducing any more flags or parameters. But let's wait for @grzuy thoughts.

Agree with @fatkodima I would prefer not to add a new setting/flag for this. Not sure how we would eventually let users set a config option before even loading the gem... Feels like we could end up with something too "hacky" and brittle.

I don't think "Revert to original explicit need to include rack-attack middleware" is the way to go either. To put this issue into perspective, rack-attack has been downloaded ~1,4 million times from RubyGems.org since we released this "auto-load" feature and we heard only 2 users asking for disabling so far. Not that we don't want everyone to be happy :-), but just pointing this out to understand the real magnitude of this. I prefer to keep this feature enabled by default for the benefit of the majority of the user base.

I think the most feasible option so far, if we end up addressing this, is the gem 'rack-attack', require: 'rack/attack/<newfile>' proposed by @zarqman.

[mt-kelvintaywl]

for what it's worth,

Rack::Timeout is doing something similar with the require: to prevent auto-loading of the middleware.
https://github.com/sharpstone/rack-timeout#rails-apps-manually