Merge pull request presidentbeef#702 from presidentbeef/port_render_p…

…ath_improvements

Port render path improvements from Brakeman Pro

lib/brakeman/processor.rb

@@ -47,8 +47,8 @@ def process_controller src, file_name
 
     #Process variable aliasing in controller source and save it in the
     #tracker.
-    def process_controller_alias name, src, only_method = nil
-      ControllerAliasProcessor.new(@app_tree, @tracker, only_method).process_controller name, src
+    def process_controller_alias name, src, only_method = nil, file = nil
+      ControllerAliasProcessor.new(@app_tree, @tracker, only_method).process_controller name, src, file
     end
 
     #Process a model source

lib/brakeman/processors/controller_alias_processor.rb

@@ -20,12 +20,13 @@ def initialize app_tree, tracker, only_method = nil
     @method_cache = {} #Cache method lookups
   end
 
-  def process_controller name, src
+  def process_controller name, src, file
     if not node_type? src, :class
       Brakeman.debug "#{name} is not a class, it's a #{src.node_type}"
       return
     else
       @current_class = name
+      @file = file
 
       process_default src
 
@@ -58,6 +59,7 @@ def process_mixins
           method = processor.process method
         end
 
+        @file = mixin.file
         #Then process it like any other method in the controller
         process method
       end
@@ -166,13 +168,22 @@ def process_before_filter name
   #Processes the default template for the current action
   def process_default_render exp
     process_layout
-    process_template template_name, nil
+    process_template template_name, nil, nil, nil
   end
 
   #Process template and add the current class and method name as called_from info
-  def process_template name, args
-    render_path = Brakeman::RenderPath.new.add_controller_render(@current_class, @current_method)
-    super name, args, render_path
+  def process_template name, args, _, line
+    # If line is null, assume implicit render and set the end of the action
+    # method as the line number
+    if line.nil? and controller = @tracker.controllers[@current_class]
+      if meth = controller.get_method(@current_method)
+        line = meth[:src] && meth[:src].last && meth[:src].last.line
+        line += 1
+      end
+    end
+
+    render_path = Brakeman::RenderPath.new.add_controller_render(@current_class, @current_method, line, relative_path(@file))
+    super name, args, render_path, line
   end
 
   #Turns a method name into a template name

lib/brakeman/processors/lib/render_helper.rb

@@ -9,15 +9,15 @@ def process_render exp
     @rendered = true
     case exp.render_type
     when :action, :template
-      process_action exp[2][1], exp[3]
+      process_action exp[2][1], exp[3], exp.line
     when :default
       begin
-        process_template template_name, exp[3]
+        process_template template_name, exp[3], nil, exp.line
       rescue ArgumentError
         Brakeman.debug "Problem processing render: #{exp}"
       end
     when :partial, :layout
-      process_partial exp[2], exp[3]
+      process_partial exp[2], exp[3], exp.line
     when :nothing
     end
     exp
@@ -31,30 +31,31 @@ def process_layout name = nil
 
     return unless name
 
-    process_template name, nil
+    process_template name, nil, nil, nil
   end
 
   #Determines file name for partial and then processes it
-  def process_partial name, args
+  def process_partial name, args, line
     if name == "" or !(string? name or symbol? name)
       return
     end
 
     names = name.value.to_s.split("/")
     names[-1] = "_" + names[-1]
-    process_template template_name(names.join("/")), args
+    process_template template_name(names.join("/")), args, nil, line
   end
 
   #Processes a given action
-  def process_action name, args
+  def process_action name, args, line
     if name.is_a? String or name.is_a? Symbol
-      process_template template_name(name), args
+      process_template template_name(name), args, nil, line
     end
   end
 
   #Processes a template, adding any instance variables
   #to its environment.
-  def process_template name, args, called_from = nil
+  def process_template name, args, called_from = nil, *_
+
     Brakeman.debug "Rendering #{name} (#{called_from})"
     #Get scanned source for this template
     name = name.to_s.gsub(/^\//, "")
@@ -81,7 +82,7 @@ def process_template name, args, called_from = nil
 
       #Process layout
       if string? options[:layout]
-        process_template "layouts/#{options[:layout][1]}", nil
+        process_template "layouts/#{options[:layout][1]}", nil, nil, nil
       elsif node_type? options[:layout], :false
         #nothing
       elsif not template.name.to_s.match(/[^\/_][^\/]+$/)

lib/brakeman/processors/lib/render_path.rb

@@ -6,19 +6,25 @@ def initialize
       @path = []
     end
 
-    def add_controller_render controller_name, method_name
+    def add_controller_render controller_name, method_name, line, file
       method_name ||= ""
 
       @path << { :type => :controller,
                  :class => controller_name.to_sym,
-                 :method => method_name.to_sym }
+                 :method => method_name.to_sym,
+                 :line => line,
+                 :file => file
+                }
 
       self
     end
 
-    def add_template_render template_name
+    def add_template_render template_name, line, file
       @path << { :type => :template,
-                 :name => template_name.to_sym }
+                 :name => template_name.to_sym,
+                 :line => line,
+                 :file => file
+               }
 
       self
     end
@@ -89,7 +95,7 @@ def to_sym
     end
 
     def to_json *args
-      MultiJson.dump(self.to_a)
+      MultiJson.dump(@path)
     end
 
     def initialize_copy original

lib/brakeman/processors/template_alias_processor.rb

@@ -18,16 +18,18 @@ def initialize tracker, template, called_from = nil
   end
 
   #Process template
-  def process_template name, args
+  def process_template name, args, _, line = nil
+    file = relative_path(@template.file || @tracker.templates[@template.name])
+
     if @called_from
       if @called_from.include_template? name
         Brakeman.debug "Skipping circular render from #{@template.name} to #{name}"
         return
       end
 
-      super name, args, @called_from.dup.add_template_render(@template.name)
+      super name, args, @called_from.dup.add_template_render(@template.name, line, file)
     else
-      super name, args, Brakeman::RenderPath.new.add_template_render(@template.name)
+      super name, args, Brakeman::RenderPath.new.add_template_render(@template.name, line, file)
     end
   end
 

lib/brakeman/rescanner.rb

@@ -118,8 +118,8 @@ def rescan_controller path
           end
         end
 
-        controller.src.each_value do |src|
-          @processor.process_controller_alias controller.name, src
+        controller.src.each do |file, src|
+          @processor.process_controller_alias controller.name, src, nil, file
         end
       end
     end
@@ -165,7 +165,7 @@ def rescan_template path
 
         controller.src.each do |file, src|
           unless @paths.include? file
-            @processor.process_controller_alias controller.name, src, r[2]
+            @processor.process_controller_alias controller.name, src, r[2], file
           end
         end
       elsif r[0] == :template

lib/brakeman/scanner.rb

@@ -221,8 +221,8 @@ def process_controller_data_flows
 
     track_progress controllers, "controllers" do |name, controller|
       Brakeman.debug "Processing #{name}"
-      controller.src.each_value do |src|
-        @processor.process_controller_alias name, src
+      controller.src.each do |file, src|
+        @processor.process_controller_alias name, src, nil, file
       end
     end
 

lib/brakeman/warning.rb

@@ -89,9 +89,8 @@ def eql? other_warning
   end
 
   #Returns name of a view, including where it was rendered from
-  def view_name
-    return @view_name if @view_name
-    if called_from
+  def view_name(include_renderer = true)
+    if called_from and include_renderer
       @view_name = "#{template.name} (#{called_from.last})"
     else
       @view_name = template.name
@@ -183,10 +182,10 @@ def fingerprint
     Digest::SHA2.new(256).update("#{warning_code_string}#{code_string}#{location_string}#{@relative_path}#{self.confidence}").to_s
   end
 
-  def location
+  def location include_renderer = true
     case @warning_set
     when :template
-      location = { :type => :template, :template => self.view_name }
+      location = { :type => :template, :template => self.view_name(include_renderer) }
     when :model
       location = { :type => :model, :model => self.model }
     when :controller
@@ -210,7 +209,7 @@ def to_hash
       :link => self.link,
       :code => (@code && self.format_code(false)),
       :render_path => self.called_from,
-      :location => self.location,
+      :location => self.location(false),
       :user_input => (@user_input && self.format_user_input(false)),
       :confidence => TEXT_CONFIDENCE[self.confidence]
     }

test/tests/json_output.rb

@@ -39,4 +39,8 @@ def test_for_errors
   def test_paths
     assert @@json["warnings"].all? { |w| not w["file"].start_with? "/" }
   end
+
+  def test_template_names_dont_have_renderer
+    assert @@json["warnings"].none? { |warning| warning["render_path"] and warning["location"]["template"].include? "(" }
+  end
 end

test/tests/render_path.rb

@@ -4,34 +4,34 @@ def setup
   end
 
   def test_include_controller
-    @r.add_controller_render :TestController, :test
+    @r.add_controller_render :TestController, :test, 1, 'app/controllers/test_controller.rb'
 
     assert @r.include_controller? :TestController
   end
 
   def test_rendered_from_controller
-    @r.add_controller_render :TestController, :test
+    @r.add_controller_render :TestController, :test, 1, 'app/controllers/test_controller.rb'
 
     assert @r.rendered_from_controller?
   end
 
   def test_include_template
-    @r.add_template_render 'some/template'
+    @r.add_template_render 'some/template', 1, 'app/views/some/template.html.erb'
 
     assert @r.include_template? :'some/template'
   end
 
   def test_include_any_method
-    @r.add_controller_render :TestController, :test
-    @r.add_controller_render :TestController, :test2
-    @r.add_controller_render :TestController, :test3
+    @r.add_controller_render :TestController, :test, 10, 'app/controllers/test_controller.rb'
+    @r.add_controller_render :TestController, :test2, 20, 'app/controllers/test_controller.rb'
+    @r.add_controller_render :TestController, :test3, 30, 'app/controllers/test_controller.rb'
 
     assert @r.include_any_method? ['test']
   end
 
   def test_each
-    @r.add_controller_render :TestController, :test
-    @r.add_template_render 'some/template'
+    @r.add_controller_render :TestController, :test, 1, 'app/controllers/test_controller.rb'
+    @r.add_template_render 'some/template', 2, 'app/views/some/template.html.erb'
 
     @r.each do |loc|
       case loc[:type]
@@ -45,10 +45,10 @@ def test_each
   end
 
   def test_dup
-    @r.add_controller_render :TestController, :test
+    @r.add_controller_render :TestController, :test, 1, 'app/controllers/test_controller.rb'
 
     s = @r.dup
-    s.add_template_render 'some/template'
+    s.add_template_render 'some/template', 2, 'app/views/some/template.html.erb'
 
     assert_equal 1, @r.length
     assert_equal 2, s.length