Merge branch 'better_handling_of_nested_classes'

lib/brakeman/processors/controller_alias_processor.rb

@@ -64,16 +64,9 @@ class Brakeman::ControllerAliasProcessor < Brakeman::AliasProcessor
     end
   end
 
-  #Processes a class which is probably a controller.
-  #(This method should be retired - only classes should ever be processed
-  # and @current_module will never be set, leading to inaccurate class names)
+  #Skip it, must be an inner class
   def process_class exp
-    @current_class = class_name(exp.class_name)
-    if @current_module
-      @current_class = ("#@current_module::#@current_class").to_sym
-    end
-
-    process_default exp
+    exp
   end
 
   #Processes a method definition, which may include
@@ -226,7 +219,8 @@ class Brakeman::ControllerAliasProcessor < Brakeman::AliasProcessor
     while controller
       filters = get_before_filters(method, controller) + filters
 
-      controller = @tracker.controllers[controller[:parent]]
+      controller = @tracker.controllers[controller[:parent]] ||
+                   @tracker.libs[controller[:parent]]
     end
 
     filters

lib/brakeman/processors/controller_processor.rb

@@ -24,28 +24,66 @@ class Brakeman::ControllerProcessor < Brakeman::BaseProcessor
   def process_class exp
     name = class_name(exp.class_name)
 
-    if not name.to_s.match(/Controller$/)
-      #Skip classes that are not controllers, but treat as a module because
-      #a class that is not a controller might contain a controller
-      return process_module exp
+    begin
+      parent = class_name exp.parent_name
+    rescue StandardError => e
+      Brakeman.debug e
+      parent = nil
     end
 
-    if @controller
-      Brakeman.debug "[Notice] Skipping inner class: #{name}"
-      return ignore
+    #If inside a real controller, treat any other classes as libraries.
+    #But if not inside a controller already, then the class may include
+    #a real controller, so we can't take this shortcut.
+    if @controller and @controller[:name].to_s.end_with? "Controller"
+      Brakeman.debug "[Notice] Treating inner class as library: #{name}"
+      Brakeman::LibraryProcessor.new(@tracker).process_library exp, @file_name
+      return exp
+    end
+
+    if not name.to_s.end_with? "Controller"
+      Brakeman.debug "[Notice] Adding noncontroller as library: #{name}"
+
+      current_controller = @controller
+
+      #Set the class to be a module in order to get the right namespacing.
+      #Add class to libraries, in case it is needed later (e.g. it's used
+      #as a parent class for a controller.)
+      #However, still want to process it in this class, so have to set
+      #@controller to this not-really-a-controller thing.
+      process_module exp do
+        name = @current_module
+
+        if @tracker.libs[name.to_sym]
+          @controller = @tracker.libs[name]
+        else
+          set_controller name, parent, exp
+          @tracker.libs[name.to_sym] = @controller
+        end
+
+        process_all exp.body
+      end
+
+      @controller = current_controller
+
+      return exp
     end
 
     if @current_module
       name = (@current_module.to_s + "::" + name.to_s).to_sym
     end
 
-    begin
-      parent = class_name exp.parent_name
-    rescue StandardError => e
-      Brakeman.debug e
-      parent = nil
-    end
+    set_controller name, parent, exp
+
+    @tracker.controllers[@controller[:name]] = @controller
+
+    exp.body = process_all! exp.body
+    set_layout_name
 
+    @controller = nil
+    exp
+  end
+
+  def set_controller name, parent, exp
     @controller = { :name => name,
                     :parent => parent,
                     :includes => [],
@@ -55,11 +93,6 @@ class Brakeman::ControllerProcessor < Brakeman::BaseProcessor
                     :options => {:before_filters => []},
                     :src => exp,
                     :file => @file_name }
-    @tracker.controllers[@controller[:name]] = @controller
-    exp.body = process_all! exp.body
-    set_layout_name
-    @controller = nil
-    exp
   end
 
   #Look for specific calls inside the controller

lib/brakeman/processors/lib/processor_helper.rb

@@ -40,7 +40,11 @@ module Brakeman::ProcessorHelper
       @current_module = module_name
     end
 
-    process_all exp.body
+    if block_given?
+      yield
+    else
+      process_all exp.body
+    end
 
     @current_module = prev_module
 

test/apps/rails3/app/controllers/application_controller.rb

 class ApplicationController < ActionController::Base
 #  protect_from_forgery
+  before_filter :action_in_parent, :only => :action_in_child
 end

test/apps/rails3/app/controllers/base_thing.rb

+class BaseThing < ApplicationController
+  def action_in_parent
+    @from_parent = params[:horrible_thing]
+  end
+end

test/apps/rails3/app/controllers/child_controller.rb

+class ChildController < BaseThing
+  def action_in_child
+    #Should get @from_parent here
+  end
+end

test/apps/rails3/app/views/child/action_in_child.html.erb

+<%= raw @from_parent %>

test/tests/test_rails3.rb

@@ -14,7 +14,7 @@ class Rails3Tests < Test::Unit::TestCase
     @expected ||= {
       :controller => 1,
       :model => 8,
-      :template => 37,
+      :template => 38,
       :warning => 54
     }
 
@@ -848,6 +848,17 @@ class Rails3Tests < Test::Unit::TestCase
       :file => /so_nested\.html\.erb/
   end
 
+  def test_cross_site_scripting_from_parent
+    assert_warning :type => :template,
+      :warning_code => 2,
+      :fingerprint => "1e860da2c9a0cac3d898f3c4327877b3bdfa391048a19bfd6f55d6e283cc5b33",
+      :warning_type => "Cross Site Scripting",
+      :line => 1,
+      :message => /^Unescaped\ parameter\ value/,
+      :confidence => 0,
+      :relative_path => "app/views/child/action_in_child.html.erb"
+  end
+
   def test_cross_site_scripting_select_tag_CVE_2012_3463
     assert_warning :type => :template,
       :warning_type => "Cross Site Scripting",