From 76c2e3143099d938883ae5654527b47e9e6a8977 Mon Sep 17 00:00:00 2001 From: Aaron Suggs Date: Fri, 18 Dec 2015 09:13:30 -0500 Subject: [PATCH] Normalize request paths when using Rails' ActionDispatch The issue --- When using rack-attack with a rails app, developers expect the request path to be normalized. In particular, trailing slashes are stripped so a request path "/login/" becomes "/login" by the time you're in ActionController. Since Rack::Attack runs before ActionDispatch, the request path is not yet normalized. This can cause throttles and blacklists to not work as expected. E.g., a throttle: throttle('logins', ...) {|req| req.path == "/login" } would not match a request to '/login/', though Rails would route '/login/' to the same '/login' action. The solution --- This patch looks if ActionDispatch's request normalization is loaded, and if so, uses it to normalize the path before processing throttles, blacklists, etc. If it's not loaded, the request path is not modified. Credit --- Thanks to Andres Riancho at Include Security for reporting this issue. --- gemfiles/activesupport3.2.gemfile | 1 + gemfiles/activesupport4.0.gemfile | 1 + gemfiles/activesupport4.1.gemfile | 1 + gemfiles/activesupport4.2.gemfile | 2 +- lib/rack/attack.rb | 2 ++ lib/rack/attack/path_normalizer.rb | 27 ++++++++++++++++++++++++ rack-attack.gemspec | 1 + spec/rack_attack_path_normalizer_spec.rb | 17 +++++++++++++++ spec/rack_attack_spec.rb | 11 ++++++++++ spec/spec_helper.rb | 5 +++++ 10 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 lib/rack/attack/path_normalizer.rb create mode 100644 spec/rack_attack_path_normalizer_spec.rb diff --git a/gemfiles/activesupport3.2.gemfile b/gemfiles/activesupport3.2.gemfile index 61efc24..a79d34e 100644 --- a/gemfiles/activesupport3.2.gemfile +++ b/gemfiles/activesupport3.2.gemfile @@ -3,5 +3,6 @@ source "https://rubygems.org" gem "activesupport", "~> 3.2.0" +gem "actionpack", "~> 3.2.0" gemspec :path => "../" diff --git a/gemfiles/activesupport4.0.gemfile b/gemfiles/activesupport4.0.gemfile index 697ad67..3e6650c 100644 --- a/gemfiles/activesupport4.0.gemfile +++ b/gemfiles/activesupport4.0.gemfile @@ -3,5 +3,6 @@ source "https://rubygems.org" gem "activesupport", "~> 4.0.0" +gem "actionpack", "~> 4.0.0" gemspec :path => "../" diff --git a/gemfiles/activesupport4.1.gemfile b/gemfiles/activesupport4.1.gemfile index 670c4f8..a64a9dd 100644 --- a/gemfiles/activesupport4.1.gemfile +++ b/gemfiles/activesupport4.1.gemfile @@ -3,5 +3,6 @@ source "https://rubygems.org" gem "activesupport", "~> 4.1.0" +gem "actionpack", "~> 4.1.0" gemspec :path => "../" diff --git a/gemfiles/activesupport4.2.gemfile b/gemfiles/activesupport4.2.gemfile index 67154bc..7e517e7 100644 --- a/gemfiles/activesupport4.2.gemfile +++ b/gemfiles/activesupport4.2.gemfile @@ -2,6 +2,6 @@ source "https://rubygems.org" -gem "activesupport", "~> 4.2.1" +gem "actionpack", "~> 4.2.1" gemspec :path => "../" diff --git a/lib/rack/attack.rb b/lib/rack/attack.rb index d8308a5..1e0ae9a 100644 --- a/lib/rack/attack.rb +++ b/lib/rack/attack.rb @@ -3,6 +3,7 @@ require 'forwardable' class Rack::Attack autoload :Cache, 'rack/attack/cache' + autoload :PathNormalizer, 'rack/attack/path_normalizer' autoload :Check, 'rack/attack/check' autoload :Throttle, 'rack/attack/throttle' autoload :Whitelist, 'rack/attack/whitelist' @@ -91,6 +92,7 @@ class Rack::Attack end def call(env) + env['PATH_INFO'] = PathNormalizer.normalize_path(env['PATH_INFO']) req = Rack::Attack::Request.new(env) if whitelisted?(req) diff --git a/lib/rack/attack/path_normalizer.rb b/lib/rack/attack/path_normalizer.rb new file mode 100644 index 0000000..afeb2f9 --- /dev/null +++ b/lib/rack/attack/path_normalizer.rb @@ -0,0 +1,27 @@ +class Rack::Attack + + # When using Rack::Attack with a Rails app, developers expect the request path + # to be normalized. In particular, trailing slashes are stripped. + # (See http://git.io/v0rrR for implementation.) + # + # Look for an ActionDispatch utility class that Rails folks would expect + # to normalize request paths. If unavailable, use a fallback class that + # doesn't normalize the path (as a non-Rails rack app developer expects). + + module FallbackPathNormalizer + def self.normalize_path(path) + path + end + end + + PathNormalizer = if defined?(::ActionDispatch::Journey::Router::Utils) + # For Rails 4+ apps + ::ActionDispatch::Journey::Router::Utils + elsif defined?(::Journey::Router::Utils) + # for Rails 3.2 + ::Journey::Router::Utils + else + FallbackPathNormalizer + end + +end diff --git a/rack-attack.gemspec b/rack-attack.gemspec index bef0f45..057aa6d 100644 --- a/rack-attack.gemspec +++ b/rack-attack.gemspec @@ -28,6 +28,7 @@ Gem::Specification.new do |s| s.add_development_dependency 'rake' s.add_development_dependency 'appraisal' s.add_development_dependency 'activesupport', '>= 3.0.0' + s.add_development_dependency 'actionpack', '>= 3.0.0' s.add_development_dependency 'redis-activesupport' s.add_development_dependency 'dalli' s.add_development_dependency 'connection_pool' diff --git a/spec/rack_attack_path_normalizer_spec.rb b/spec/rack_attack_path_normalizer_spec.rb new file mode 100644 index 0000000..1c5b66e --- /dev/null +++ b/spec/rack_attack_path_normalizer_spec.rb @@ -0,0 +1,17 @@ +require_relative 'spec_helper' + +describe Rack::Attack::PathNormalizer do + subject { Rack::Attack::PathNormalizer } + + it 'should have a normalize_path method' do + subject.normalize_path('/foo').must_equal '/foo' + end + + describe 'FallbackNormalizer' do + subject { Rack::Attack::FallbackPathNormalizer } + + it '#normalize_path does not change the path' do + subject.normalize_path('').must_equal '' + end + end +end diff --git a/spec/rack_attack_spec.rb b/spec/rack_attack_spec.rb index cd6b599..3084ffb 100644 --- a/spec/rack_attack_spec.rb +++ b/spec/rack_attack_spec.rb @@ -3,6 +3,17 @@ require_relative 'spec_helper' describe 'Rack::Attack' do allow_ok_requests + describe 'normalizing paths' do + before do + Rack::Attack.blacklist("banned_path") {|req| req.path == '/foo' } + end + + it 'blocks requests with trailing slash' do + get '/foo/' + last_response.status.must_equal 403 + end + end + describe 'blacklist' do before do @bad_ip = '1.2.3.4' diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 5382087..118d1fe 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -5,6 +5,11 @@ require "minitest/autorun" require "minitest/pride" require "rack/test" require 'active_support' +require 'action_dispatch' + +# Load Journey for Rails 3.2 +require 'journey' if ActionPack::VERSION::MAJOR == 3 + require "rack/attack" begin