Skip to content

Loading…

Improve Rails 3 route processing #116

Merged
merged 11 commits into from

2 participants

@presidentbeef

This patch fixes some issues with Rails 3 route processing.

Improvements include:

  • Handling "controller" blocks
controller :whatever do
  get :something => :else
end
  • Handling routes that don't specify a controller
resource :book do
  get :reserve
end
  • Handling :to options in match
match 'path', :to => 'controller#action'
  • Handling :to options in blocks
resource :book do
  get 'reserve', :to => :checkout
end
@oreoshake oreoshake commented on the diff
lib/brakeman/processors/lib/rails3_route_processor.rb
((5 lines not shown))
exp
end
def process_resources_block exp
- process_resources exp[1]

alias :process_resources_block, :process_resource_block?

@presidentbeef Owner

They are slightly different...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@oreoshake

code climate is making me overthink this, but yeah, :+1:

@presidentbeef presidentbeef merged commit 57c2672 into master
@presidentbeef presidentbeef deleted the improve_rails3_route_processing branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
View
116 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,9 +135,22 @@ 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]
@@ -126,10 +158,12 @@ def process_verb exp
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]

alias :process_resources_block, :process_resource_block?

@presidentbeef Owner

They are slightly different...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- 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
View
6 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
View
29 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
View
1 test/apps/rails3.1/app/views/other/a.html.erb
@@ -0,0 +1 @@
+<%= raw @a %>
View
1 test/apps/rails3.1/app/views/other/b.html.erb
@@ -0,0 +1 @@
+<%= raw @b %>
View
1 test/apps/rails3.1/app/views/other/c.html.erb
@@ -0,0 +1 @@
+<%= raw @c %>
View
1 test/apps/rails3.1/app/views/other/d.html.erb
@@ -0,0 +1 @@
+<%= raw @d %>
View
1 test/apps/rails3.1/app/views/other/e.html.erb
@@ -0,0 +1 @@
+<%= raw @e %>
View
1 test/apps/rails3.1/app/views/other/f.html.erb
@@ -0,0 +1 @@
+<%= raw @f %>
View
1 test/apps/rails3.1/app/views/other/g.html.erb
@@ -0,0 +1 @@
+<%= raw @g %>
View
17 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.
View
64 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,
Something went wrong with that request. Please try again.