Merge branch 'fix_how_immediate_model_attributes_detected'

Conflicts:
	test/tests/rails31.rb

lib/brakeman/checks/base_check.rb

@@ -364,7 +364,11 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
       if @safe_input_attributes.include? method
         false
       elsif call? target and not method.to_s[-1,1] == "?"
-        has_immediate_model? target, out
+        if res = has_immediate_model?(target, out)
+          exp
+        else
+          false
+        end
       elsif model_name? target
         exp
       else
@@ -429,28 +433,6 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
     end
   end
 
-  #Finds entire method call chain where +target+ is a target in the chain
-  def find_chain exp, target
-    return unless sexp? exp
-
-    case exp.node_type
-    when :output, :format
-      find_chain exp.value, target
-    when :call
-      if exp == target or include_target? exp, target
-        return exp
-      end
-    else
-      exp.each do |e|
-        if sexp? e
-          res = find_chain e, target
-          return res if res
-        end
-      end
-      nil
-    end
-  end
-
   #Returns true if +target+ is in +exp+
   def include_target? exp, target
     return false unless call? exp

lib/brakeman/checks/check_content_tag.rb

@@ -107,12 +107,10 @@ class Brakeman::CheckContentTag < Brakeman::CheckCrossSiteScripting
         :link_path => "content_tag"
 
     elsif not tracker.options[:ignore_model_output] and match = has_immediate_model?(arg)
-      method = match[2]
-
-      unless IGNORE_MODEL_METHODS.include? method
+      unless IGNORE_MODEL_METHODS.include? match.method
         add_result result
 
-        if MODEL_METHODS.include? method or method.to_s =~ /^find_by/
+        if likely_model_attribute? match
           confidence = CONFIDENCE[:high]
         else
           confidence = CONFIDENCE[:med]

lib/brakeman/checks/check_cross_site_scripting.rb

@@ -122,7 +122,7 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
       unless IGNORE_MODEL_METHODS.include? method
         add_result exp
 
-        if MODEL_METHODS.include? method or method.to_s =~ /^find_by/
+        if likely_model_attribute? match
           confidence = CONFIDENCE[:high]
         else
           confidence = CONFIDENCE[:med]
@@ -138,12 +138,17 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
           warning_code = :xss_to_json
         end
 
-        code = find_chain out, match
+        code = if match == out
+                 nil
+               else
+                 match
+               end
+
         warn :template => @current_template,
           :warning_type => "Cross Site Scripting",
           :warning_code => warning_code,
           :message => message,
-          :code => code,
+          :code => match,
           :confidence => confidence,
           :link_path => link_path
       end
@@ -153,6 +158,19 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
     end
   end
 
+  #Call already involves a model, but might not be acting on a record
+  def likely_model_attribute? exp
+    return false unless call? exp
+
+    method = exp.method
+
+    if MODEL_METHODS.include? method or method.to_s.start_with? "find_by_"
+      true
+    else
+      likely_model_attribute? exp.target
+    end
+  end
+
   #Process an output Sexp
   def process_output exp
     process exp.value.dup

lib/brakeman/checks/check_link_to.rb

@@ -82,7 +82,7 @@ class Brakeman::CheckLinkTo < Brakeman::CheckCrossSiteScripting
     return false if IGNORE_MODEL_METHODS.include? method
 
     confidence = CONFIDENCE[:med]
-    confidence = CONFIDENCE[:high] if MODEL_METHODS.include? method or method.to_s =~ /^find_by/
+    confidence = CONFIDENCE[:high] if likely_model_attribute? match
     warn_xss(result, "Unescaped model attribute in link_to", match, confidence)
   end
 

test/apps/rails3.1/app/controllers/other_controller.rb

@@ -72,4 +72,8 @@ class OtherController < ApplicationController
 
     Marshal.restore User.find(1).cool_stored_thing
   end
+
+  def test_model_in_haml
+    @user = User.new
+  end
 end

test/apps/rails3.1/app/views/other/test_model_in_haml.html.haml

+%user
+  %footer
+    = @user.updated_at
+
+  %h1= @user.name
+
+  != @user.bio

test/tests/rails31.rb

@@ -13,7 +13,7 @@ class Rails31Tests < Test::Unit::TestCase
   def expected
     @expected ||= {
       :model => 3,
-      :template => 22,
+      :template => 23,
       :controller => 4,
       :generic => 72 }
   end
@@ -1043,4 +1043,26 @@ class Rails31Tests < Test::Unit::TestCase
       :confidence => 0,
       :relative_path => "app/models/user.rb"
   end
+
+  def test_wrong_model_attributes_in_haml
+    assert_no_warning :type => :template,
+      :warning_code => 2,
+      :fingerprint => "8851713f0af477e60090607b814ba68055e4ac1cf19df0628fddd961ff87e763",
+      :warning_type => "Cross Site Scripting",
+      :line => 3,
+      :message => /^Unescaped\ model\ attribute/,
+      :confidence => 0,
+      :relative_path => "app/views/other/test_model_in_haml.html.haml"
+  end
+
+  def test_right_model_attribute_in_haml
+    assert_warning :type => :template,
+      :warning_code => 2,
+      :fingerprint => "3310ef4a4bde8b120fd5d421565ee416af815404e7c116a8069052e8732589d0",
+      :warning_type => "Cross Site Scripting",
+      :line => 7,
+      :message => /^Unescaped\ model\ attribute/,
+      :confidence => 0,
+      :relative_path => "app/views/other/test_model_in_haml.html.haml"
+  end
 end