From bd3b8d885123c874b28b6de4a37190c16066699a Mon Sep 17 00:00:00 2001
From: oreoshake <neil.matatall@gmail.com>
Date: Fri, 21 Sep 2012 08:56:41 -0700
Subject: [PATCH] Always treat an array as safe from off-host redirects

---
 lib/brakeman/checks/check_redirect.rb         | 20 ++++---------------
 .../rails2/app/controllers/home_controller.rb |  4 ----
 test/tests/test_rails2.rb                     | 11 ++--------
 3 files changed, 6 insertions(+), 29 deletions(-)

diff --git a/lib/brakeman/checks/check_redirect.rb b/lib/brakeman/checks/check_redirect.rb
index 8fc8616d..db801ab7 100644
--- a/lib/brakeman/checks/check_redirect.rb
+++ b/lib/brakeman/checks/check_redirect.rb
@@ -59,6 +59,10 @@ class Brakeman::CheckRedirect < Brakeman::BaseCheck
     args = call.args
     first_arg = call.first_arg
 
+    # if the first argument is an array, rails assumes you are building a
+    # polymorphic route, which will never jump off-host
+    return false if array? first_arg
+
     if tracker.options[:ignore_redirect_to_model] and call? first_arg and
       (@model_find_calls.include? first_arg.method or first_arg.method.to_s.match(/^find_by_/)) and
       model_name? first_arg.target
@@ -66,22 +70,6 @@ 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)
         # polymorphic routes are assumed to be safe
diff --git a/test/apps/rails2/app/controllers/home_controller.rb b/test/apps/rails2/app/controllers/home_controller.rb
index 63988cdd..dd5e0c1d 100644
--- a/test/apps/rails2/app/controllers/home_controller.rb
+++ b/test/apps/rails2/app/controllers/home_controller.rb
@@ -169,10 +169,6 @@ class HomeController < ApplicationController
     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
diff --git a/test/tests/test_rails2.rb b/test/tests/test_rails2.rb
index dd5cc85b..69c7c19b 100644
--- a/test/tests/test_rails2.rb
+++ b/test/tests/test_rails2.rb
@@ -12,13 +12,13 @@ class Rails2Tests < Test::Unit::TestCase
         :controller => 1,
         :model => 2,
         :template => 41,
-        :warning => 34}
+        :warning => 33}
     else
       @expected ||= {
         :controller => 1,
         :model => 2,
         :template => 41,
-        :warning => 35 }
+        :warning => 34 }
     end
   end
 
@@ -125,13 +125,6 @@ class Rails2Tests < Test::Unit::TestCase
       :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
-- 
GitLab