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.

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

fix: avoid unintended effects on load_config_initializers and other gems load order #457

Merged
merged 1 commit into from
Oct 30, 2019

Conversation

grzuy
Copy link
Collaborator

@grzuy grzuy commented Oct 29, 2019

Because of the sort algorithm rails uses to satisfy after and before
constraints, gems can have unintended effects on others. See
rails/rails@0a120a8

Prefer making rack-attack middleware idempotent instead of relying on
the load order and the contents of the middleware stack too much.

closes #452
closes #456

…ems load order

Because of the sort algorithm rails uses to satisfy `after` and `before`
constraints, gems can have unintended effects on others. See
rails/rails@0a120a8

Prefer making rack-attack middleware idempotent instead of relying on
the load order and the contents of the middleware stack too much.

closes #452
closes #456
Copy link

@geoffharcourt geoffharcourt left a comment

Choose a reason for hiding this comment

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

Thank you for working on this and fielding the issue!

@@ -46,6 +46,8 @@ def app
Rack::Builder.new do
# Use Rack::Lint to test that rack-attack is complying with the rack spec
use Rack::Lint
# Intentionally added twice to test idempotence property
use Rack::Attack

Choose a reason for hiding this comment

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

👍

@@ -3,17 +3,9 @@
module Rack
class Attack
class Railtie < ::Rails::Railtie
initializer 'rack.attack.middleware', after: :load_config_initializers, before: :build_middleware_stack do |app|
initializer "rack-attack.middleware" do |app|
if Gem::Version.new(::Rails::VERSION::STRING) >= Gem::Version.new("5.1")
Copy link
Contributor

Choose a reason for hiding this comment

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

With new implementation, I think this restriction can be removed.

Copy link
Collaborator Author

@grzuy grzuy Oct 30, 2019

Choose a reason for hiding this comment

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

Probably.

I want this change to be the minimal possible update that fixes the issue, which can be released as a patch-level update (v6.2.1).

If we open this up for more rails version, that can be part of v6.3.0.

Thanks for reviewing.

@fatkodima
Copy link
Contributor

Clean and simple 👏 This should be the initial implementation 😀

@jarthod
Copy link

jarthod commented Nov 30, 2020

Hi, I just upgraded from 4.x to the latest version and noticed all these changes about the middleware being inserted by default (which is a good idea I agree). Though I tried moving it because I need it to be before some other middleware of ours (to avoid some database query). I tried gem 'rack-attack', require: false and:

# config/application.rb
...
require 'rack/attack'
config.middleware.use CompressedRequests
...

But the middleware would still end-up after every others:

> rake middleware
...
use CompressedRequests
use Rack::Attack
...

Because of the initializer hook I guess. So AFAICS I don't have any other option than inserting it twice?
Is this really the preferred/official way to move this middleware now? It does not sound super efficient to have it twice :/ (I understand the re-entrance check is quick but that's still additional code for no value at every request).

I am not requesting a change here (especially if I am the only one who needs this), just asking maybe if there's a workaround to change the placing? or a way to skip the auto-add? (I checked the code to see if I could require other files to skip the railties but it's part of the main file so I can't).

Thanks for your answer.

ps: And thanks for rack-attack! it's nice and clean and very useful 🙇

@grzuy
Copy link
Collaborator Author

grzuy commented Dec 29, 2020

Hi @jarthod , thank you for the thorough explanation of your concern.

So, just doing

config.middleware.insert_after(Rack::Attack, CompressedRequests)

doesn't work, does it?

@jarthod
Copy link

jarthod commented Dec 29, 2020

Indeed it doesn't, because the Rack::Attack middleware is not inserted yet, so this (in application.rb):

    require 'rack/attack'
    config.middleware.insert_after(Rack::Attack, CompressedRequests)

Raises: No such middleware to insert after: Rack::Attack (RuntimeError)

Also even if it did, I wouldn't be able to change the insertion line for every middleware I use, especially the ones in gems.
For the same reason, I can't remove the middleware manually (with config.middleware.delete) before adding it back elsewhere.

Unless maybe I manage to define an initializer which is gonna be executed AFTER the one from rack-attack, but I'm not sure how to guarantee that, it sounds brittle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v6.2.0 incompatible with sequel-rails Unexpected interaction with I18n
4 participants