diff --git a/lib/brakeman/checks/check_redirect.rb b/lib/brakeman/checks/check_redirect.rb index be664b44f77781303e50e966dfa12468f6ef405c..2619f8f3712d86881eb75731eba20840bfa8bfd4 100644 --- a/lib/brakeman/checks/check_redirect.rb +++ b/lib/brakeman/checks/check_redirect.rb @@ -95,14 +95,14 @@ class Brakeman::CheckRedirect < Brakeman::BaseCheck #Checks +redirect_to+ arguments for +only_path => true+ which essentially #nullifies the danger posed by redirecting with user input def only_path? call - call.args.each do |arg| - if hash? arg - if value = hash_access(arg, :only_path) - return true if true?(value) - end - elsif call? arg and arg.method == :url_for - return check_url_for(arg) + arg = call.first_arg + + if hash? arg + if value = hash_access(arg, :only_path) + return true if true?(value) end + elsif call? arg and arg.method == :url_for + return check_url_for(arg) end false @@ -111,11 +111,11 @@ class Brakeman::CheckRedirect < Brakeman::BaseCheck #+url_for+ is only_path => true by default. This checks to see if it is #set to false for some reason. def check_url_for call - call.args.each do |arg| - if hash? arg - if value = hash_access(arg, :only_path) - return false if false?(value) - end + arg = call.first_arg + + if hash? arg + if value = hash_access(arg, :only_path) + return false if false?(value) end end diff --git a/test/apps/rails3/app/controllers/home_controller.rb b/test/apps/rails3/app/controllers/home_controller.rb index ee34bfdd8bf37ef64432882764c8305f027c6267..74d171d00e2b3f8486e9de6399d9053a3d958e9d 100644 --- a/test/apps/rails3/app/controllers/home_controller.rb +++ b/test/apps/rails3/app/controllers/home_controller.rb @@ -73,8 +73,8 @@ class HomeController < ApplicationController current_user.something.something.build(params[:awesome_user]) end - def test_only_path - redirect_to params[:user], :only_path => true + def test_only_path_wrong + redirect_to params[:user], :only_path => true #This should still warn end def test_url_for_only_path @@ -92,6 +92,12 @@ class HomeController < ApplicationController y + 1 + 2 end + def test_only_path_correct + params.merge! :only_path => true + redirect_to params + end + + private def filter_it diff --git a/test/tests/test_rails3.rb b/test/tests/test_rails3.rb index f4cb221c2f81d76702d59b1c2b24299904d368fb..671e23962b61cdf5f4b66ee90e3cab3f9dc1c34f 100644 --- a/test/tests/test_rails3.rb +++ b/test/tests/test_rails3.rb @@ -15,7 +15,7 @@ class Rails3Tests < Test::Unit::TestCase :controller => 1, :model => 5, :template => 23, - :warning => 29 + :warning => 30 } end @@ -135,11 +135,11 @@ class Rails3Tests < Test::Unit::TestCase :file => /products_controller\.rb/ end - def test_redirect_only_path - assert_no_warning :type => :warning, + def test_redirect_only_path_in_wrong_argument + assert_warning :type => :warning, :warning_type => "Redirect", - :line => 78, - :message => /^Possible unprotected redirect near line 78: redirect_to\(params\[/, + :line => 77, + :message => /^Possible unprotected redirect near line 77: redirect_to\(params\[/, :confidence => 0, :file => /home_controller\.rb/ end