Merge pull request #139 from presidentbeef/fix_redirect_false_negative

Fix `redirect_to` false negative with only_path

lib/brakeman/checks/check_redirect.rb

@@ -95,7 +95,8 @@ 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|
+    arg = call.first_arg
+
     if hash? arg
       if value = hash_access(arg, :only_path)
         return true if true?(value)
@@ -103,7 +104,6 @@ class Brakeman::CheckRedirect < Brakeman::BaseCheck
     elsif call? arg and arg.method == :url_for
       return check_url_for(arg)
     end
-    end
 
     false
   end
@@ -111,13 +111,13 @@ 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|
+    arg = call.first_arg
+
     if hash? arg
       if value = hash_access(arg, :only_path)
         return false if false?(value)
       end
     end
-    end
 
     true
   end

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

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