#143 Treat models and arrays of models as safe input to redirect_to

This is an enhancement and and a bug fix.  Models use polymorphic routes and should be considered safe as do arrays.  Treat arrays containing things other than models as unsafe as well.

lib/brakeman/checks/base_check.rb

@@ -305,6 +305,10 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
     end
   end
 
+  def is_immediate_model? exp, out = nil
+    exp == has_immediate_model?(exp, out)
+  end
+
   #Checks for a model attribute at the top level of the
   #expression.
   def has_immediate_model? exp, out = nil

lib/brakeman/checks/check_redirect.rb

@@ -66,9 +66,30 @@ class Brakeman::CheckRedirect < Brakeman::BaseCheck
       return false
     end
 
+    # if the first argument is an array, rails assumes you are building a polymorphic route.
+    # therefore, if each value is a model, we're safe.  You can guess a url if there are user
+    # supplied values in the array so long as something resposnds to <parameter_value>_path,
+    # so we still need to consider anything other than a model as dangerous.
+    if array? first_arg
+      args.first.each do |arg|
+        next if arg == :array #wtf bugfix?
+
+        unless is_immediate_model? arg
+          return Match.new(:immediate, arg)
+        end
+      end
+
+      return false
+    end
+
     args.each do |arg|
       if res = has_immediate_model?(arg)
-        return Match.new(:immediate, res)
+        # polymorphic routes are assumed to be safe
+        if is_immediate_model? arg
+          return false
+        else
+          return Match.new(:immediate, res)
+        end
       elsif call? arg
         if request_value? arg
           return Match.new(:immediate, arg)

test/apps/rails2/app/controllers/home_controller.rb

@@ -160,4 +160,24 @@ class HomeController < ApplicationController
   def or_equals
     params[:still_bad] ||= {}
   end
+
+  def test_safe_model_redirect
+    redirect_to User.find(1)
+  end
+
+  def test_safe_mode_array_redirect
+    redirect_to [User.find(1), User.find(2)]
+  end
+
+  def test_array_with_badness
+    redirect_to [params[:badness]]
+  end
+
+  def test_model_attr_badness
+    redirect_to [User.new.donkey]
+  end
+
+  def test_model_attributes
+    redirect_to User.new.donkey
+  end
 end

test/tests/test_rails2.rb

@@ -12,13 +12,13 @@ class Rails2Tests < Test::Unit::TestCase
         :controller => 1,
         :model => 2,
         :template => 41,
-        :warning => 31}
+        :warning => 34}
     else
       @expected ||= {
         :controller => 1,
         :model => 2,
         :template => 41,
-        :warning => 32 }
+        :warning => 35 }
     end
   end
 
@@ -111,6 +111,27 @@ class Rails2Tests < Test::Unit::TestCase
       :message => /^Possible unprotected redirect/,
       :confidence => 0,
       :file => /home_controller\.rb/
+
+    assert_warning :type => :warning,
+      :warning_type => "Redirect",
+      :line => 173,
+      :message => /^Possible unprotected redirect/,
+      :confidence => 0,
+      :file => /home_controller\.rb/
+
+    assert_warning :type => :warning,
+      :warning_type => "Redirect",
+      :line => 177,
+      :message => /^Possible unprotected redirect/,
+      :confidence => 0,
+      :file => /home_controller\.rb/
+
+    assert_warning :type => :warning,
+      :warning_type => "Redirect",
+      :line => 181,
+      :message => /^Possible unprotected redirect/,
+      :confidence => 0,
+      :file => /home_controller\.rb/
   end
 
   def test_dynamic_render_path