Improve Rails 3 route processing

lib/brakeman/processors/lib/rails3_route_processor.rb

@@ -14,6 +14,7 @@ def initialize tracker
     @prefix = [] #Controller name prefix (a module name, usually)
     @current_controller = nil
     @with_options = nil #For use inside map.with_options
+    @controller_block = false
   end
 
   def process_routes exp
@@ -49,6 +50,8 @@ def process_iter exp
       process_resources_block exp
     when :scope
       process_scope_block exp
+    when :controller
+      process_controller_block exp
     else
       super
     end
@@ -71,10 +74,8 @@ def process_root exp
     args = exp[3][1..-1]
 
     if value = hash_access(args[0], :to)
-      if string? value[1]
-        controller, action = extract_action v[1]
-
-        add_route action, controller
+      if string? value
+        add_route_from_string value
       end
     end
 
@@ -96,18 +97,36 @@ def process_match exp
         return exp
       elsif matcher.include? ':action'
         action_variable = true
+      elsif args[1].nil? and in_controller_block? and not matcher.include? ":"
+        add_route matcher
       end
     end
 
     if hash? args[-1]
       hash_iterate args[-1] do |k, v|
-        if string? k and string? v
-          controller, action = extract_action v[1]
+        if string? k
+          if string? v
+            add_route_from_string v[1]
+          elsif in_controller_block? and symbol? v
+            add_route v
+          end
+        elsif symbol? k
+         case k[1]
+         when :action
+          if string? v
+            add_route_from_string v
+          else
+            add_route v
+          end
 
-          add_route action if action
-        elsif symbol? k and k[1] == :action
-          add_route action
           action_variable = false
+         when :to
+           if string? v
+             add_route_from_string v[1]
+           elsif in_controller_block? and symbol? v
+             add_route v
+           end
+         end
         end
       end
     end
@@ -116,20 +135,35 @@ def process_match exp
       @tracker.routes[@current_controller] = :allow_all_actions
     end
 
+    @current_controller = nil unless in_controller_block?
     exp
   end
 
+  def add_route_from_string value
+    value = value[1] if string? value
+
+    controller, action = extract_action value
+
+    if action
+      add_route action, controller
+    elsif in_controller_block?
+      add_route value
+    end
+  end
+
   def process_verb exp
     args = exp[3][1..-1]
 
     if symbol? args[0] and not hash? args[1]
       add_route args[0]
     elsif hash? args[1]
       hash_iterate args[1] do |k, v|
-        if symbol? k and k[1] == :to and string? v
-          controller, action = extract_action v[1]
-
-          add_route action, controller
+        if symbol? k and k[1] == :to
+          if string? v
+            add_route_from_string v[1]
+          elsif in_controller_block? and symbol? v
+            add_route v
+          end
         end
       end
     elsif string? args[0]
@@ -138,19 +172,22 @@ def process_verb exp
         add_route route[0]
       else
         add_route route[1], route[0]
-        @current_controller = nil
       end
+    elsif in_controller_block? and symbol? args[0]
+      add_route args[0]
     else hash? args[0]
       hash_iterate args[0] do |k, v|
-        if string? v
-          controller, action = extract_action v[1]
-
-          add_route action, controller
-          break
+        if string? k
+          if string? v
+            add_route_from_string v
+          elsif in_controller_block?
+            add_route v
+          end
         end
       end
     end
 
+    @current_controller = nil unless in_controller_block?
     exp
   end
 
@@ -166,6 +203,7 @@ def process_resources exp
       end
     end
 
+    @current_controller = nil unless in_controller_block?
     exp
   end
 
@@ -181,18 +219,27 @@ def process_resource exp
       end
     end
 
+    @current_controller = nil unless in_controller_block?
     exp
   end
 
   def process_resources_block exp
-    process_resources exp[1]
-    process exp[3]
+    in_controller_block do
+      process_resources exp[1]
+      process exp[3]
+    end
+
+    @current_controller = nil
     exp
   end
 
   def process_resource_block exp
-    process_resource exp[1]
-    process exp[3]
+    in_controller_block do
+      process_resource exp[1]
+      process exp[3]
+    end
+
+    @current_controller = nil
     exp
   end
 
@@ -202,7 +249,30 @@ def process_scope_block exp
     exp
   end
 
+  def process_controller_block exp
+    args = exp[1][3]
+    self.current_controller = args[1][1]
+
+    in_controller_block do
+      process exp[-1] if exp[-1]
+    end
+
+    @current_controller = nil
+    exp
+  end
+
   def extract_action str
     str.split "#"
   end
+
+  def in_controller_block?
+    @controller_block
+  end
+
+  def in_controller_block
+    prev_block = @controller_block
+    @controller_block = true
+    yield
+    @controller_block = prev_block
+  end
 end

lib/brakeman/processors/lib/route_helper.rb

@@ -35,7 +35,11 @@ def add_route route, controller = nil
       self.current_controller = controller
     end
 
-    @tracker.routes[@current_controller] << route
+    routes = @tracker.routes[@current_controller]
+
+    if routes and routes != :allow_all_actions
+      routes << route
+    end
   end
 
   #Add default routes

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

@@ -0,0 +1,29 @@
+class OtherController < ApplicationController
+  def a
+    @a = params[:bad]
+  end
+
+  def b
+    @b = params[:bad]
+  end
+
+  def c
+    @c = params[:bad]
+  end
+
+  def d
+    @d = params[:bad]
+  end
+
+  def e
+    @e = params[:bad]
+  end
+
+  def f
+    @f = params[:bad]
+  end
+
+  def g
+    @g = params[:bad]
+  end
+end

test/apps/rails3.1/app/views/other/a.html.erb

@@ -0,0 +1 @@
+<%= raw @a %>

test/apps/rails3.1/app/views/other/b.html.erb

@@ -0,0 +1 @@
+<%= raw @b %>

test/apps/rails3.1/app/views/other/c.html.erb

@@ -0,0 +1 @@
+<%= raw @c %>

test/apps/rails3.1/app/views/other/d.html.erb

@@ -0,0 +1 @@
+<%= raw @d %>

test/apps/rails3.1/app/views/other/e.html.erb

@@ -0,0 +1 @@
+<%= raw @e %>

test/apps/rails3.1/app/views/other/f.html.erb

@@ -0,0 +1 @@
+<%= raw @f %>

test/apps/rails3.1/app/views/other/g.html.erb

@@ -0,0 +1 @@
+<%= raw @g %>

test/apps/rails3.1/config/routes.rb

@@ -4,6 +4,23 @@
     get 'mixin_default'
   end
 
+  resources :other do
+    get :a
+    delete 'f'
+  end
+
+  controller :other do
+    get 'b'
+    post 'something' => 'c'
+    put 'dee', :to => :d
+  end
+
+  match 'e', :to => 'other#e', :as => 'eeeee'
+
+  get 'g' => 'other#g'
+
+  match 'blah/:id', :action => 'blarg' 
+
   # The priority is based upon order of creation:
   # first created -> highest priority.
 

test/tests/test_rails31.rb

@@ -13,7 +13,7 @@ def report
   def expected
     @expected ||= {
       :model => 0,
-      :template => 4,
+      :template => 11,
       :controller => 1,
       :warning => 44 }
   end
@@ -455,6 +455,68 @@ def test_controller_mixin_default_render
       :file => /users\/mixin_default\.html\.erb/
   end
 
+  def test_get_in_resources_block
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 1,
+      :message => /^Unescaped\ parameter\ value/,
+      :confidence => 0,
+      :file => /\/a\.html\.erb/
+  end
+
+  def test_get_in_controller_block
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 1,
+      :message => /^Unescaped\ parameter\ value/,
+      :confidence => 0,
+      :file => /\/b\.html\.erb/
+  end
+
+  def test_post_with_just_hash_in_controller_block
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 1,
+      :message => /^Unescaped\ parameter\ value/,
+      :confidence => 0,
+      :file => /\/c\.html\.erb/
+  end
+
+  def test_put_to_in_controller_block
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 1,
+      :message => /^Unescaped\ parameter\ value/,
+      :confidence => 0,
+      :file => /\/d\.html\.erb/
+  end
+
+  def test_match_to_route
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 1,
+      :message => /^Unescaped\ parameter\ value/,
+      :confidence => 0,
+      :file => /\/e\.html\.erb/
+  end
+
+  def test_delete_in_resources_block
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 1,
+      :message => /^Unescaped\ parameter\ value/,
+      :confidence => 0,
+      :file => /\/f\.html\.erb/
+  end
+
+  def test_route_hash_shorthand
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 1,
+      :message => /^Unescaped\ parameter\ value/,
+      :confidence => 0,
+      :file => /\/g\.html\.erb/
+  end
 
   def test_file_access_indirect_user_input
     assert_warning :type => :warning,