Skip to content

Commit

Permalink
fix: avoid unintended effects on load_config_initializers and other g…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
grzuy committed Oct 29, 2019
1 parent 9bfec1a commit e3056e7
Show file tree
Hide file tree
Showing 4 changed files with 6 additions and 17 deletions.
3 changes: 2 additions & 1 deletion lib/rack/attack.rb
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,9 @@ def initialize(app)
end

def call(env)
return @app.call(env) unless self.class.enabled
return @app.call(env) 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)

Expand Down
12 changes: 2 additions & 10 deletions lib/rack/attack/railtie.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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")
middlewares = app.config.middleware
operations = middlewares.send(:operations) + middlewares.send(:delete_operations)

use_middleware = operations.none? do |operation|
middleware = operation[1]
middleware.include?(Rack::Attack)
end

middlewares.use(Rack::Attack) if use_middleware
app.middleware.use(Rack::Attack)
end
end
end
Expand Down
6 changes: 0 additions & 6 deletions spec/acceptance/rails_middleware_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,6 @@
assert_equal 1, @app.middleware.count(Rack::Attack)
end

it "is not added when it was added explicitly" do
@app.config.middleware.use(Rack::Attack)
@app.initialize!
assert_equal 1, @app.middleware.count(Rack::Attack)
end

it "is not added when it was explicitly deleted" do
@app.config.middleware.delete(Rack::Attack)
@app.initialize!
Expand Down
2 changes: 2 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
use Rack::Attack
use Rack::Lint

Expand Down

0 comments on commit e3056e7

Please sign in to comment.