Merge pull request #441 from presidentbeef/fix_haml_template_processing

Fix HAML template processing

lib/brakeman/processors/haml_template_processor.rb

@@ -3,6 +3,7 @@ require 'brakeman/processors/template_processor'
 #Processes HAML templates.
 class Brakeman::HamlTemplateProcessor < Brakeman::TemplateProcessor
   HAML_FORMAT_METHOD = /format_script_(true|false)_(true|false)_(true|false)_(true|false)_(true|false)_(true|false)_(true|false)/
+  HAML_HELPERS = s(:colon2, s(:const, :Haml), :Helpers)
   
   #Processes call, looking for template output
   def process_call exp
@@ -37,9 +38,7 @@ class Brakeman::HamlTemplateProcessor < Brakeman::TemplateProcessor
               else
                 case method.to_s
                 when "push_text"
-                  s = Sexp.new(:output, out)
-                  @current_template[:outputs] << s
-                  s
+                  build_output_from_push_text(out)
                 when HAML_FORMAT_METHOD
                   if $4 == "true"
                     Sexp.new :format_escaped, out
@@ -117,4 +116,52 @@ class Brakeman::HamlTemplateProcessor < Brakeman::TemplateProcessor
     exp.target.value == :_hamlout and
     exp.method == :buffer
   end
+
+  #HAML likes to put interpolated values into _hamlout.push_text
+  #but we want to handle those individually
+  def build_output_from_push_text exp
+    if node_type? exp, :string_interp, :dstr
+      exp.map! do |e|
+        if sexp? e
+          if node_type? e, :string_eval, :evstr
+            e = e.value
+          end
+
+          get_pushed_value e
+        else
+          e
+        end
+      end
+    end
+  end
+
+  #Gets outputs from values interpolated into _hamlout.push_text
+  def get_pushed_value exp
+    return exp unless sexp? exp
+    
+    case exp.node_type
+    when :format
+      exp.node_type = :output
+      @current_template[:outputs] << exp
+      exp
+    when :format_escaped
+      exp.node_type = :escaped_output
+      @current_template[:outputs] << exp
+      exp
+    when :str, :ignore, :output, :escaped_output
+      exp
+    when :block, :rlist, :string_interp, :dstr
+      exp.map! { |e| get_pushed_value e }
+    else
+      if call? exp and exp.target == HAML_HELPERS and exp.method == :html_escape
+        s = Sexp.new(:escaped_output, exp.first_arg)
+      else
+        s = Sexp.new(:output, exp)
+      end
+
+      s.line(exp.line)
+      @current_template[:outputs] << s
+      s
+    end
+  end
 end

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

@@ -65,4 +65,8 @@ class OtherController < ApplicationController
 
     render :xss_dupes, :layout => 'thing'
   end
+
+  def test_haml_stuff
+    render :locals => { :user => User.first }
+  end
 end

test/apps/rails2/app/views/other/test_haml_stuff.html.haml

+%tr
+  %td= user.age.to_i
+  %td= user.stuff
+  %td= user.status

test/tests/rails2.rb

@@ -11,13 +11,13 @@ class Rails2Tests < Test::Unit::TestCase
       @expected ||= {
         :controller => 1,
         :model => 3,
-        :template => 45,
+        :template => 47,
         :generic => 49 }
     else
       @expected ||= {
         :controller => 1,
         :model => 3,
-        :template => 45,
+        :template => 47,
         :generic => 50 }
     end
   end
@@ -1119,6 +1119,30 @@ class Rails2Tests < Test::Unit::TestCase
       :relative_path => "app/views/layouts/thing.html.erb"
   end
 
+  def test_cross_site_scripting_in_haml
+    assert_warning :type => :template,
+      :warning_code => 2,
+      :fingerprint => "702f9bae476402bb2614794276083849342540bd8b5e8f2fc35b15b40e9f34fc",
+      :warning_type => "Cross Site Scripting",
+      :line => 3,
+      :message => /^Unescaped\ model\ attribute/,
+      :confidence => 0,
+      :relative_path => "app/views/other/test_haml_stuff.html.haml",
+      :user_input => nil
+  end
+
+  def test_cross_site_scripting_in_haml2
+    assert_warning :type => :template,
+      :warning_code => 2,
+      :fingerprint => "79cbc87a06ad9247362be97ba4b6cc12b9619fd0f68d468b81cbed376bfbcc5c",
+      :warning_type => "Cross Site Scripting",
+      :line => 4,
+      :message => /^Unescaped\ model\ attribute/,
+      :confidence => 0,
+      :relative_path => "app/views/other/test_haml_stuff.html.haml",
+      :user_input => nil
+  end
+
   def test_dangerous_send_try
     assert_warning :type => :warning,
       :warning_type => "Dangerous Send",
@@ -1287,13 +1311,13 @@ class Rails2WithOptionsTests < Test::Unit::TestCase
       @expected ||= {
         :controller => 1,
         :model => 4,
-        :template => 45,
+        :template => 47,
         :generic => 49 }
     else
       @expected ||= {
         :controller => 1,
         :model => 4,
-        :template => 45,
+        :template => 47,
         :generic => 50 }
     end
   end

test/tests/tabs_output.rb

@@ -3,9 +3,9 @@ class TestTabsOutput < Test::Unit::TestCase
 
   def test_reported_warnings
     if Brakeman::Scanner::RUBY_1_9
-      assert_equal 99, Report.lines.to_a.count
+      assert_equal 101, Report.lines.to_a.count
     else
-      assert_equal 100, Report.lines.to_a.count
+      assert_equal 102, Report.lines.to_a.count
     end
   end
 end