Merge pull request #493 from jjarmoc

Improve route checking for HTTP verb routes.

lib/brakeman/checks/check_default_routes.rb

@@ -22,11 +22,16 @@ class Brakeman::CheckDefaultRoutes < Brakeman::BaseCheck
 
       tracker.routes.each do |name, actions|
         if actions.is_a? Array and actions[0] == :allow_all_actions
+          if actions[1].is_a? Hash and actions[1][:allow_verb]
+            verb = actions[1][:allow_verb]
+          else
+            verb = "any"
+          end
           warn :controller => name,
             :warning_type => "Default Routes", 
             :warning_code => :controller_default_routes,
-            :message => "Any public method in #{name} can be used as an action.",
-            :line => actions[1],
+            :message => "Any public method in #{name} can be used as an action for #{verb} requests.",
+            :line => actions[2],
             :confidence => CONFIDENCE[:med],
             :file => "#{tracker.options[:app_path]}/config/routes.rb"
         end

lib/brakeman/checks/check_filter_skipping.rb

@@ -21,7 +21,7 @@ class Brakeman::CheckFilterSkipping < Brakeman::BaseCheck
 
   def uses_arbitrary_actions?
     tracker.routes.each do |name, actions|
-      if actions == :allow_all_actions
+      if actions.include? :allow_all_actions
         return true
       end
     end

lib/brakeman/processors/controller_alias_processor.rb

@@ -206,7 +206,7 @@ class Brakeman::ControllerAliasProcessor < Brakeman::AliasProcessor
       true
     else
       routes = @tracker.routes[@current_class]
-      routes and (routes == :allow_all_actions or routes.include? method)
+      routes and (routes.include? :allow_all_actions or routes.include? method)
     end
   end
 

lib/brakeman/processors/lib/rails3_route_processor.rb

@@ -89,18 +89,17 @@ class Brakeman::Rails3RoutesProcessor < Brakeman::BaseProcessor
     second_arg = exp.second_arg
     last_arg = exp.last_arg
 
-    #Check if there is an unrestricted action parameter
-    action_variable = false
-
     if string? first_arg
-      matcher = first_arg.value
 
+      matcher = first_arg.value
       if matcher == ':controller(/:action(/:id(.:format)))' or
-        matcher.include? ':controller' and matcher.include? ':action' #Default routes
+        matcher.include? ':controller' and action_route?(matcher)  #Default routes
         @tracker.routes[:allow_all_actions] = first_arg
         return exp
-      elsif matcher.include? ':action'
-        action_variable = true
+      elsif action_route?(first_arg)
+          if hash? second_arg and controller_name = hash_access(second_arg, :controller)
+            loose_action(controller_name, "matched") #TODO: Parse verbs
+          end
       elsif second_arg.nil? and in_controller_block? and not matcher.include? ":"
         add_route matcher
       end
@@ -123,7 +122,6 @@ class Brakeman::Rails3RoutesProcessor < Brakeman::BaseProcessor
             add_route v
           end
 
-          action_variable = false
          when :to
            if string? v
              add_route_from_string v[1]
@@ -135,10 +133,6 @@ class Brakeman::Rails3RoutesProcessor < Brakeman::BaseProcessor
       end
     end
 
-    if action_variable
-      @tracker.routes[@current_controller] = :allow_all_actions
-    end
-
     @current_controller = nil unless in_controller_block?
     exp
   end
@@ -169,9 +163,17 @@ class Brakeman::Rails3RoutesProcessor < Brakeman::BaseProcessor
           elsif in_controller_block? and symbol? v
             add_route v
           end
+        elsif action_route?(first_arg)
+          if hash? second_arg and controller_name = hash_access(second_arg, :controller)
+            loose_action(controller_name, exp.method)
+          end
         end
       end
     elsif string? first_arg
+      if first_arg.value.include? ':controller' and action_route?(first_arg) #Default routes
+        @tracker.routes[:allow_all_actions] = first_arg
+      end
+
       route = first_arg.value.split "/"
       if route.length != 2
         add_route route[0]
@@ -287,4 +289,17 @@ class Brakeman::Rails3RoutesProcessor < Brakeman::BaseProcessor
     yield
     @controller_block = prev_block
   end
+
+  def action_route? arg
+    if string? arg
+      arg = arg.value
+    end
+
+    arg.is_a? String and (arg.include? ":action" or arg.include? "*action")
+  end
+
+  def loose_action controller_name, verb = "any"
+    self.current_controller = controller_name.value
+    @tracker.routes[@current_controller] = [:allow_all_actions, {:allow_verb => verb}]
+  end
 end

lib/brakeman/processors/lib/route_helper.rb

@@ -39,7 +39,7 @@ module Brakeman::RouteHelper
 
     routes = @tracker.routes[@current_controller]
     
-    if routes and routes != :allow_all_actions
+    if routes and not routes.include? :allow_all_actions
       routes << route
     end
   end

lib/brakeman/report/report_base.rb

@@ -39,7 +39,7 @@ class Brakeman::Report::Base
       name = name.to_sym
       c = tracker.controllers[name]
 
-      if tracker.routes[:allow_all_actions] or tracker.routes[name] == :allow_all_actions
+      if tracker.routes.include? :allow_all_actions or (tracker.routes[name] and tracker.routes[name].include? :allow_all_actions)
         routes = c[:public].keys.map{|e| e.to_s}.sort.join(", ")
       elsif tracker.routes[name].nil?
         #No routes defined for this controller.

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

@@ -12,6 +12,16 @@ Rails32::Application.routes.draw do
 
   match 'exec' => 'exec#exec_this'
 
+  # Routes for 'Default Routes' tests
+  get "/glob/*action", controller: 'glob_get'
+  post '/glob_post/*action', controller: 'glob_post'
+  put 'glob_put/*action', controller: 'glob_put'
+  match "/glob/*action", controller: 'glob_match'
+  get "/foo_get/:action", controller: 'foo_get'
+  post "/foo_post/:action", controller: 'foo_post'
+  put 'foo_put/:action', controller: 'foo_put'
+  match '/bar/:action', controller: 'bar_match'
+
   # The priority is based upon order of creation:
   # first created -> highest priority.
 

test/tests/only_files_option.rb

@@ -8,7 +8,7 @@ class OnlyFilesOptionTests < Test::Unit::TestCase
 
   def expected
     @expected ||= {
-      :controller => 0,
+      :controller => 8,
       :model => 0,
       :template => 1,
       :generic => 8 }

test/tests/rails32.rb

@@ -13,7 +13,7 @@ class Rails32Tests < Test::Unit::TestCase
 
   def expected
     @expected ||= {
-      :controller => 0,
+      :controller => 8,
       :model => 5,
       :template => 11,
       :generic => 13 }
@@ -335,4 +335,55 @@ class Rails32Tests < Test::Unit::TestCase
       :relative_path => "app/controllers/exec_controller/command_dependency.rb",
       :format_code => /params\[:user_input\]/
   end
+
+   def test_controller_default_routes
+    # Test to ensure warnings are generated for loose routes
+     assert_warning :type => :controller,
+      :warning_type => "Default Routes",
+      :message => /GlobGetController.*get requests/,
+      :fingerprint => "6550aaf3da845a600a9c8fb767d08489679a9e3d89554db3c920ddb4eafcfb8e",
+      :file => /routes\.rb/
+
+    assert_warning :type => :controller,
+      :warning_type => "Default Routes",
+      :message => /GlobMatchController.*matched requests/,
+      :fingerprint => "fb878909d1635de22ddc819e33e6a75e7f2cce0ff1efd2b7e76b361be88bb73e",
+      :file => /routes\.rb/
+
+    assert_warning :type => :controller,
+      :warning_type => "Default Routes",
+      :message => /GlobPostController.*post requests/,
+      :fingerprint => "e5364369e3c89e5632aac3645e183037cc18de49f1b67547dca0c7febb6c849f",
+      :file => /routes\.rb/
+
+    assert_warning :type => :controller,
+      :warning_type => "Default Routes",
+      :message => /GlobPutController.*put requests/,
+      :fingerprint => "b85eeac90866fc04b4bea19c971aed4f2458afe53c908aa7162eb1e46b84f9b6",
+      :file => /routes\.rb/
+
+    assert_warning :type => :controller,
+      :warning_type => "Default Routes",
+      :message => /FooPostController.*post requests/,
+      :fingerprint => "880355a0a87704aa0d615dea6a175ba78711d1593843a596935f95cac3abc8a5",
+      :file => /routes\.rb/
+
+    assert_warning :type => :controller,
+      :warning_type => "Default Routes",
+      :message => /FooGetController.*get requests/,
+      :fingerprint => "e4f29dc75741f74327ce95678173d3d5fe296335275f062c06d1348678f6a339",
+      :file => /routes\.rb/
+
+    assert_warning :type => :controller,
+      :warning_type => "Default Routes",
+      :message => /FooPutController.*put requests/,
+      :fingerprint => "05a92f06689436b7b8189c358baab371de5f0fb7936ab206a11b251b0e5f7570",
+      :file => /routes\.rb/
+
+    assert_warning :type => :controller,
+      :warning_type => "Default Routes",
+      :message => /BarMatchController.*matched requests/,
+      :fingerprint => "857efc249dfd1b5086dcf79c35e31ef19a7782d03b3beaa12f55f8634b543d2d",
+      :file => /routes\.rb/
+   end
 end