Permalink
Browse files

Remove default match without specified method

In the current router DSL, using the +match+ DSL
method will match all verbs for the path to the
specified endpoint.

In the vast majority of cases, people are
currently using +match+ when they actually mean
+get+. This introduces security implications.

This commit disallows calling +match+ without
an HTTP verb constraint by default. To explicitly
match all verbs, this commit also adds a
:via => :all option to +match+.

Closes #5964
  • Loading branch information...
1 parent 0cc32c5 commit 56cdc81c08b1847c5c1f699810a8c3b9ac3715a6 @wycats wycats committed Apr 25, 2012
Showing with 463 additions and 455 deletions.
  1. +12 −2 actionpack/lib/action_dispatch/routing/mapper.rb
  2. +2 −2 actionpack/test/abstract_unit.rb
  3. +1 −1 actionpack/test/activerecord/active_record_store_test.rb
  4. +10 −10 actionpack/test/controller/action_pack_assertions_test.rb
  5. +7 −7 actionpack/test/controller/base_test.rb
  6. +3 −3 actionpack/test/controller/caching_test.rb
  7. +1 −1 actionpack/test/controller/flash_test.rb
  8. +4 −4 actionpack/test/controller/integration_test.rb
  9. +1 −1 actionpack/test/controller/mime_responds_test.rb
  10. +1 −1 actionpack/test/controller/new_base/content_type_test.rb
  11. +1 −1 actionpack/test/controller/new_base/render_template_test.rb
  12. +2 −2 actionpack/test/controller/new_base/render_test.rb
  13. +2 −2 actionpack/test/controller/new_base/render_text_test.rb
  14. +2 −2 actionpack/test/controller/redirect_test.rb
  15. +1 −1 actionpack/test/controller/render_test.rb
  16. +1 −1 actionpack/test/controller/render_xml_test.rb
  17. +3 −3 actionpack/test/controller/rescue_test.rb
  18. +2 −2 actionpack/test/controller/resources_test.rb
  19. +168 −168 actionpack/test/controller/routing_test.rb
  20. +5 −5 actionpack/test/controller/test_case_test.rb
  21. +16 −16 actionpack/test/controller/url_for_integration_test.rb
  22. +8 −8 actionpack/test/controller/url_for_test.rb
  23. +1 −1 actionpack/test/controller/url_rewriter_test.rb
  24. +1 −1 actionpack/test/controller/webservice_test.rb
  25. +8 −8 actionpack/test/dispatch/mapper_test.rb
  26. +13 −13 actionpack/test/dispatch/prefix_generation_test.rb
  27. +2 −2 actionpack/test/dispatch/request/json_params_parsing_test.rb
  28. +1 −1 actionpack/test/dispatch/request/multipart_params_parsing_test.rb
  29. +1 −1 actionpack/test/dispatch/request/query_string_parsing_test.rb
  30. +1 −1 actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb
  31. +2 −2 actionpack/test/dispatch/request/xml_params_parsing_test.rb
  32. +2 −2 actionpack/test/dispatch/request_id_test.rb
  33. +53 −53 actionpack/test/dispatch/routing_test.rb
  34. +1 −1 actionpack/test/dispatch/session/cache_store_test.rb
  35. +1 −1 actionpack/test/dispatch/session/cookie_store_test.rb
  36. +1 −1 actionpack/test/dispatch/session/mem_cache_store_test.rb
  37. +1 −1 actionpack/test/dispatch/url_generation_test.rb
  38. +1 −1 actionpack/test/template/form_helper_test.rb
  39. +1 −1 actionpack/test/template/test_test.rb
  40. +9 −9 actionpack/test/template/url_helper_test.rb
  41. +37 −39 guides/source/routing.textile
  42. +1 −1 railties/lib/rails/application/finisher.rb
  43. +4 −4 railties/lib/rails/engine.rb
  44. +1 −1 railties/test/application/asset_debugging_test.rb
  45. +5 −5 railties/test/application/assets_test.rb
  46. +1 −1 railties/test/application/initializers/frameworks_test.rb
  47. +2 −2 railties/test/application/initializers/i18n_test.rb
  48. +8 −8 railties/test/application/loading_test.rb
  49. +1 −1 railties/test/application/middleware/cache_test.rb
  50. +1 −1 railties/test/application/middleware/exceptions_test.rb
  51. +7 −7 railties/test/application/route_inspect_test.rb
  52. +10 −10 railties/test/application/routing_test.rb
  53. +1 −1 railties/test/application/url_generation_test.rb
  54. +3 −3 railties/test/isolation/abstract_unit.rb
  55. +1 −1 railties/test/rails_info_controller_test.rb
  56. +16 −16 railties/test/railties/engine_test.rb
  57. +11 −11 railties/test/railties/mounted_engine_test.rb
@@ -59,6 +59,16 @@ def initialize(set, scope, path, options)
@options = (@scope[:options] || {}).merge(options)
@path = normalize_path(path)
normalize_options!
+
+ via_all = @options.delete(:via) if @options[:via] == :all
+
+ if !via_all && request_method_condition.empty?
+ msg = "You should not use the `match` method in your router without specifying an HTTP method.\n" \
+ "If you want to expose your action to GET, use `get` in the router:\n\n" \
+ " Instead of: match \"controller#action\"\n" \
+ " Do: get \"controller#action\""
+ raise msg
@tenderlove

tenderlove Dec 18, 2012

Owner

Unfortunately, I found someone using this in the wild. I'll file a ticket up stream, but should we make this a warning?

@steveklabnik

steveklabnik via email Dec 19, 2012

Member
@tenderlove

tenderlove Dec 19, 2012

Owner

Ya, the only annoying thing is we never deprecated the behavior, just removed it. Anyway, I sent a PR here.

@steveklabnik

steveklabnik Dec 19, 2012

Member

That's fair. It's in that weird gray zone; a 'feature' that's a big security issue...

+ end
end
def to_route
@@ -264,7 +274,7 @@ module Base
# of most Rails applications, this is beneficial.
def root(options = {})
options = { :to => options } if options.is_a?(String)
- match '/', { :as => :root }.merge(options)
+ match '/', { :as => :root, :via => :get }.merge(options)
end
# Matches a url pattern to one or more routes. Any symbols in a pattern
@@ -417,7 +427,7 @@ def mount(app, options = nil)
options[:as] ||= app_name(app)
- match(path, options.merge(:to => app, :anchor => false, :format => false))
+ match(path, options.merge(:to => app, :anchor => false, :format => false, :via => :all))
define_generate_prefix(app, options[:as])
self
@@ -125,11 +125,11 @@ class TestCase
# have been loaded.
setup_once do
SharedTestRoutes.draw do
- match ':controller(/:action)'
+ get ':controller(/:action)'
end
ActionDispatch::IntegrationTest.app.routes.draw do
- match ':controller(/:action)'
+ get ':controller(/:action)'
end
end
end
@@ -259,7 +259,7 @@ def test_incoming_invalid_session_id_via_parameter_should_be_ignored
def with_test_route_set(options = {})
with_routing do |set|
set.draw do
- match ':action', :to => 'active_record_store_test/test'
+ get ':action', :to => 'active_record_store_test/test'
end
@app = self.class.build_app(set) do |middleware|
@@ -162,17 +162,17 @@ def test_get_post_request_switch
def test_string_constraint
with_routing do |set|
set.draw do
- match "photos", :to => 'action_pack_assertions#nothing', :constraints => {:subdomain => "admin"}
+ get "photos", :to => 'action_pack_assertions#nothing', :constraints => {:subdomain => "admin"}
end
end
end
def test_assert_redirect_to_named_route_failure
with_routing do |set|
set.draw do
- match 'route_one', :to => 'action_pack_assertions#nothing', :as => :route_one
- match 'route_two', :to => 'action_pack_assertions#nothing', :id => 'two', :as => :route_two
- match ':controller/:action'
+ get 'route_one', :to => 'action_pack_assertions#nothing', :as => :route_one
+ get 'route_two', :to => 'action_pack_assertions#nothing', :id => 'two', :as => :route_two
+ get ':controller/:action'
end
process :redirect_to_named_route
assert_raise(ActiveSupport::TestCase::Assertion) do
@@ -192,8 +192,8 @@ def test_assert_redirect_to_nested_named_route
with_routing do |set|
set.draw do
- match 'admin/inner_module', :to => 'admin/inner_module#index', :as => :admin_inner_module
- match ':controller/:action'
+ get 'admin/inner_module', :to => 'admin/inner_module#index', :as => :admin_inner_module
+ get ':controller/:action'
end
process :redirect_to_index
# redirection is <{"action"=>"index", "controller"=>"admin/admin/inner_module"}>
@@ -206,8 +206,8 @@ def test_assert_redirected_to_top_level_named_route_from_nested_controller
with_routing do |set|
set.draw do
- match '/action_pack_assertions/:id', :to => 'action_pack_assertions#index', :as => :top_level
- match ':controller/:action'
+ get '/action_pack_assertions/:id', :to => 'action_pack_assertions#index', :as => :top_level
+ get ':controller/:action'
end
process :redirect_to_top_level_named_route
# assert_redirected_to "http://test.host/action_pack_assertions/foo" would pass because of exact match early return
@@ -221,8 +221,8 @@ def test_assert_redirected_to_top_level_named_route_with_same_controller_name_in
with_routing do |set|
set.draw do
# this controller exists in the admin namespace as well which is the only difference from previous test
- match '/user/:id', :to => 'user#index', :as => :top_level
- match ':controller/:action'
+ get '/user/:id', :to => 'user#index', :as => :top_level
+ get ':controller/:action'
end
process :redirect_to_top_level_named_route
# assert_redirected_to top_level_url('foo') would pass because of exact match early return
@@ -158,7 +158,7 @@ def setup
def test_url_for_query_params_included
rs = ActionDispatch::Routing::RouteSet.new
rs.draw do
- match 'home' => 'pages#home'
+ get 'home' => 'pages#home'
end
options = {
@@ -174,8 +174,8 @@ def test_url_for_query_params_included
def test_url_options_override
with_routing do |set|
set.draw do
- match 'from_view', :to => 'url_options#from_view', :as => :from_view
- match ':controller/:action'
+ get 'from_view', :to => 'url_options#from_view', :as => :from_view
+ get ':controller/:action'
end
get :from_view, :route => "from_view_url"
@@ -189,7 +189,7 @@ def test_url_options_override
def test_url_helpers_does_not_become_actions
with_routing do |set|
set.draw do
- match "account/overview"
+ get "account/overview"
end
assert !@controller.class.action_methods.include?("account_overview_path")
@@ -208,8 +208,8 @@ def setup
def test_default_url_options_override
with_routing do |set|
set.draw do
- match 'from_view', :to => 'default_url_options#from_view', :as => :from_view
- match ':controller/:action'
+ get 'from_view', :to => 'default_url_options#from_view', :as => :from_view
+ get ':controller/:action'
end
get :from_view, :route => "from_view_url"
@@ -226,7 +226,7 @@ def test_default_url_options_are_used_in_non_positional_parameters
scope("/:locale") do
resources :descriptions
end
- match ':controller/:action'
+ get ':controller/:action'
end
get :from_view, :route => "description_path(1)"
@@ -102,8 +102,8 @@ def teardown
def test_page_caching_resources_saves_to_correct_path_with_extension_even_if_default_route
with_routing do |set|
set.draw do
- match 'posts.:format', :to => 'posts#index', :as => :formatted_posts
- match '/', :to => 'posts#index', :as => :main
+ get 'posts.:format', :to => 'posts#index', :as => :formatted_posts
+ get '/', :to => 'posts#index', :as => :main
end
@params[:format] = 'rss'
assert_equal '/posts.rss', @routes.url_for(@params)
@@ -560,7 +560,7 @@ def test_forbidden_is_not_cached
def test_xml_version_of_resource_is_treated_as_different_cache
with_routing do |set|
set.draw do
- match ':controller(/:action(.:format))'
+ get ':controller(/:action(.:format))'
end
get :index, :format => 'xml'
@@ -277,7 +277,7 @@ def get(path, parameters = nil, env = {})
def with_test_route_set
with_routing do |set|
set.draw do
- match ':action', :to => FlashIntegrationTest::TestController
+ get ':action', :to => FlashIntegrationTest::TestController
end
@app = self.class.build_app(set) do |middleware|
@@ -466,7 +466,7 @@ def with_test_route_set
end
set.draw do
- match ':action', :to => controller
+ match ':action', :to => controller, :via => [:get, :post]
get 'get/:action', :to => controller
end
@@ -530,10 +530,10 @@ def self.routes
end
routes.draw do
- match '', :to => 'application_integration_test/test#index', :as => :empty_string
+ get '', :to => 'application_integration_test/test#index', :as => :empty_string
- match 'foo', :to => 'application_integration_test/test#index', :as => :foo
- match 'bar', :to => 'application_integration_test/test#index', :as => :bar
+ get 'foo', :to => 'application_integration_test/test#index', :as => :foo
+ get 'bar', :to => 'application_integration_test/test#index', :as => :bar
end
def app
@@ -1118,7 +1118,7 @@ def with_test_route_set
resources :quiz_stores do
resources :customers
end
- match ":controller/:action"
+ get ":controller/:action"
end
yield
end
@@ -43,7 +43,7 @@ class ExplicitContentTypeTest < Rack::TestCase
test "default response is HTML and UTF8" do
with_routing do |set|
set.draw do
- match ':controller', :action => 'index'
+ get ':controller', :action => 'index'
end
get "/content_type/base"
@@ -164,7 +164,7 @@ class TestWithLayout < Rack::TestCase
test "rendering with implicit layout" do
with_routing do |set|
- set.draw { match ':controller', :action => :index }
+ set.draw { get ':controller', :action => :index }
get "/render_template/with_layout"
@@ -57,7 +57,7 @@ class RenderTest < Rack::TestCase
test "render with blank" do
with_routing do |set|
set.draw do
- match ":controller", :action => 'index'
+ get ":controller", :action => 'index'
end
get "/render/blank_render"
@@ -70,7 +70,7 @@ class RenderTest < Rack::TestCase
test "rendering more than once raises an exception" do
with_routing do |set|
set.draw do
- match ":controller", :action => 'index'
+ get ":controller", :action => 'index'
end
assert_raises(AbstractController::DoubleRenderError) do
@@ -67,7 +67,7 @@ class RenderTextTest < Rack::TestCase
test "rendering text from a action with default options renders the text with the layout" do
with_routing do |set|
- set.draw { match ':controller', :action => 'index' }
+ set.draw { get ':controller', :action => 'index' }
get "/render_text/simple"
assert_body "hello david"
@@ -77,7 +77,7 @@ class RenderTextTest < Rack::TestCase
test "rendering text from a action with default options renders the text without the layout" do
with_routing do |set|
- set.draw { match ':controller', :action => 'index' }
+ set.draw { get ':controller', :action => 'index' }
get "/render_text/with_layout"
@@ -262,7 +262,7 @@ def test_redirect_to_record
with_routing do |set|
set.draw do
resources :workshops
- match ':controller/:action'
+ get ':controller/:action'
end
get :redirect_to_existing_record
@@ -296,7 +296,7 @@ def test_redirect_to_with_block_and_assigns
def test_redirect_to_with_block_and_accepted_options
with_routing do |set|
set.draw do
- match ':controller/:action'
+ get ':controller/:action'
end
get :redirect_to_with_block_and_options
@@ -1188,7 +1188,7 @@ def test_head_with_location_object
with_routing do |set|
set.draw do
resources :customers
- match ':controller/:action'
+ get ':controller/:action'
end
get :head_with_location_object
@@ -72,7 +72,7 @@ def test_rendering_with_object_location_should_set_header_with_url_for
with_routing do |set|
set.draw do
resources :customers
- match ':controller/:action'
+ get ':controller/:action'
end
get :render_with_object_location
@@ -338,9 +338,9 @@ def show_errors(exception)
def with_test_routing
with_routing do |set|
set.draw do
- match 'foo', :to => ::RescueTest::TestController.action(:foo)
- match 'invalid', :to => ::RescueTest::TestController.action(:invalid)
- match 'b00m', :to => ::RescueTest::TestController.action(:b00m)
+ get 'foo', :to => ::RescueTest::TestController.action(:foo)
+ get 'invalid', :to => ::RescueTest::TestController.action(:invalid)
+ get 'b00m', :to => ::RescueTest::TestController.action(:b00m)
end
yield
end
@@ -680,7 +680,7 @@ def test_new_style_named_routes_for_resource
scope '/threads/:thread_id' do
resources :messages, :as => 'thread_messages' do
get :search, :on => :collection
- match :preview, :on => :new
+ get :preview, :on => :new
end
end
end
@@ -698,7 +698,7 @@ def test_new_style_named_routes_for_singleton_resource
scope '/admin' do
resource :account, :as => :admin_account do
get :login, :on => :member
- match :preview, :on => :new
+ get :preview, :on => :new
end
end
end
Oops, something went wrong.

2 comments on commit 56cdc81

Contributor

homakov replied Apr 25, 2012

kudos! Very appreciate your help!

Contributor

radar replied May 8, 2012

Thank you so much!

Please sign in to comment.