Permalink
Browse files

Deprecate :controller and :action path parameters

Allowing :controller and :action values to be specified via the path
in config/routes.rb has been an underlying cause of a number of issues
in Rails that have resulted in security releases. In light of this it's
better that controllers and actions are explicitly whitelisted rather
than trying to blacklist or sanitize 'bad' values.
  • Loading branch information...
pixeltrix committed Mar 1, 2016
1 parent 1d3502c commit 6520ea5f7e2215a763ca74bf6cfa87be2347d5df
Showing with 478 additions and 213 deletions.
  1. +9 −0 actionpack/lib/action_dispatch/routing/route_set.rb
  2. +6 −2 actionpack/test/abstract_unit.rb
  3. +16 −4 actionpack/test/controller/action_pack_assertions_test.rb
  4. +12 −3 actionpack/test/controller/base_test.rb
  5. +3 −1 actionpack/test/controller/flash_test.rb
  6. +20 −5 actionpack/test/controller/integration_test.rb
  7. +3 −1 actionpack/test/controller/new_base/content_type_test.rb
  8. +2 −2 actionpack/test/controller/new_base/render_body_test.rb
  9. +2 −2 actionpack/test/controller/new_base/render_html_test.rb
  10. +2 −2 actionpack/test/controller/new_base/render_plain_test.rb
  11. +1 −1 actionpack/test/controller/new_base/render_template_test.rb
  12. +6 −2 actionpack/test/controller/new_base/render_test.rb
  13. +2 −2 actionpack/test/controller/new_base/render_text_test.rb
  14. +7 −2 actionpack/test/controller/redirect_test.rb
  15. +4 −1 actionpack/test/controller/render_test.rb
  16. +4 −1 actionpack/test/controller/render_xml_test.rb
  17. +171 −71 actionpack/test/controller/routing_test.rb
  18. +13 −4 actionpack/test/controller/test_case_test.rb
  19. +9 −6 actionpack/test/controller/url_for_integration_test.rb
  20. +12 −3 actionpack/test/controller/url_for_test.rb
  21. +3 −1 actionpack/test/controller/url_rewriter_test.rb
  22. +6 −2 actionpack/test/dispatch/request/json_params_parsing_test.rb
  23. +6 −2 actionpack/test/dispatch/request/multipart_params_parsing_test.rb
  24. +6 −2 actionpack/test/dispatch/request/query_string_parsing_test.rb
  25. +3 −1 actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb
  26. +12 −4 actionpack/test/dispatch/routing/inspector_test.rb
  27. +59 −21 actionpack/test/dispatch/routing_test.rb
  28. +3 −1 actionpack/test/dispatch/session/cache_store_test.rb
  29. +3 −1 actionpack/test/dispatch/session/cookie_store_test.rb
  30. +3 −1 actionpack/test/dispatch/session/mem_cache_store_test.rb
  31. +69 −61 actionpack/test/journey/router_test.rb
  32. +1 −1 railties/test/isolation/abstract_unit.rb
@@ -513,6 +513,15 @@ def add_route(mapping, path_ast, name, anchor)
route = @set.add_route(name, mapping)
named_routes[name] = route if name
if route.segment_keys.include?(:controller)
ActiveSupport::Deprecation.warn("Using a dynamic :controller segment in a route is deprecated and will be remove in Rails 5.1")
end
if route.segment_keys.include?(:action)
ActiveSupport::Deprecation.warn("Using a dynamic :action segment in a route is deprecated and will be remove in Rails 5.1")
end
route
end
@@ -69,7 +69,9 @@ def root; end;
SharedTestRoutes = ActionDispatch::Routing::RouteSet.new
SharedTestRoutes.draw do
get ':controller(/:action)'
ActiveSupport::Deprecation.silence do
get ':controller(/:action)'
end
end
module ActionDispatch
@@ -118,7 +120,9 @@ def self.build_app(routes = nil)
self.app = build_app
app.routes.draw do
get ':controller(/:action)'
ActiveSupport::Deprecation.silence do
get ':controller(/:action)'
end
end
class DeadEndRoutes < ActionDispatch::Routing::RouteSet
@@ -177,7 +177,10 @@ def test_assert_redirect_to_named_route_failure
set.draw do
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'
ActiveSupport::Deprecation.silence do
get ':controller/:action'
end
end
process :redirect_to_named_route
assert_raise(ActiveSupport::TestCase::Assertion) do
@@ -201,7 +204,10 @@ def test_assert_redirect_to_nested_named_route
with_routing do |set|
set.draw do
get 'admin/inner_module', :to => 'admin/inner_module#index', :as => :admin_inner_module
get ':controller/:action'
ActiveSupport::Deprecation.silence do
get ':controller/:action'
end
end
process :redirect_to_index
# redirection is <{"action"=>"index", "controller"=>"admin/admin/inner_module"}>
@@ -215,7 +221,10 @@ def test_assert_redirected_to_top_level_named_route_from_nested_controller
with_routing do |set|
set.draw do
get '/action_pack_assertions/:id', :to => 'action_pack_assertions#index', :as => :top_level
get ':controller/:action'
ActiveSupport::Deprecation.silence do
get ':controller/:action'
end
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
@@ -231,7 +240,10 @@ def test_assert_redirected_to_top_level_named_route_with_same_controller_name_in
set.draw do
# this controller exists in the admin namespace as well which is the only difference from previous test
get '/user/:id', :to => 'user#index', :as => :top_level
get ':controller/:action'
ActiveSupport::Deprecation.silence do
get ':controller/:action'
end
end
process :redirect_to_top_level_named_route
# assert_redirected_to top_level_url('foo') would pass because of exact match early return
@@ -175,7 +175,10 @@ def test_url_options_override
with_routing do |set|
set.draw do
get 'from_view', :to => 'url_options#from_view', :as => :from_view
get ':controller/:action'
ActiveSupport::Deprecation.silence do
get ':controller/:action'
end
end
get :from_view, params: { route: "from_view_url" }
@@ -209,7 +212,10 @@ def test_default_url_options_override
with_routing do |set|
set.draw do
get 'from_view', :to => 'default_url_options#from_view', :as => :from_view
get ':controller/:action'
ActiveSupport::Deprecation.silence do
get ':controller/:action'
end
end
get :from_view, params: { route: "from_view_url" }
@@ -226,7 +232,10 @@ def test_default_url_options_are_used_in_non_positional_parameters
scope("/:locale") do
resources :descriptions
end
get ':controller/:action'
ActiveSupport::Deprecation.silence do
get ':controller/:action'
end
end
get :from_view, params: { route: "description_path(1)" }
@@ -323,7 +323,9 @@ def get(path, *args)
def with_test_route_set
with_routing do |set|
set.draw do
get ':action', :to => FlashIntegrationTest::TestController
ActiveSupport::Deprecation.silence do
get ':action', :to => FlashIntegrationTest::TestController
end
end
@app = self.class.build_app(set) do |middleware|
@@ -730,8 +730,10 @@ def with_test_route_set
set.draw do
get 'moved' => redirect('/method')
match ':action', :to => controller, :via => [:get, :post], :as => :action
get 'get/:action', :to => controller, :as => :get_action
ActiveSupport::Deprecation.silence do
match ':action', :to => controller, :via => [:get, :post], :as => :action
get 'get/:action', :to => controller, :as => :get_action
end
end
self.singleton_class.include(set.url_helpers)
@@ -1105,7 +1107,12 @@ def ok
def test_request
with_routing do |routes|
routes.draw { get ':action' => FooController }
routes.draw do
ActiveSupport::Deprecation.silence do
get ':action' => FooController
end
end
get '/ok'
assert_response 200
@@ -1173,7 +1180,11 @@ def test_registering_custom_encoder
def test_parsed_body_without_as_option
with_routing do |routes|
routes.draw { get ':action' => FooController }
routes.draw do
ActiveSupport::Deprecation.silence do
get ':action' => FooController
end
end
get '/foos_json.json', params: { foo: 'heyo' }
@@ -1184,7 +1195,11 @@ def test_parsed_body_without_as_option
private
def post_to_foos(as:)
with_routing do |routes|
routes.draw { post ':action' => FooController }
routes.draw do
ActiveSupport::Deprecation.silence do
post ':action' => FooController
end
end
post "/foos_#{as}", params: { foo: 'fighters' }, as: as
@@ -43,7 +43,9 @@ class ExplicitContentTypeTest < Rack::TestCase
test "default response is text/plain and UTF8" do
with_routing do |set|
set.draw do
get ':controller', :action => 'index'
ActiveSupport::Deprecation.silence do
get ':controller', :action => 'index'
end
end
get "/content_type/base"
@@ -85,7 +85,7 @@ class RenderBodyTest < Rack::TestCase
test "rendering body from an action with default options renders the body with the layout" do
with_routing do |set|
set.draw { get ':controller', action: 'index' }
set.draw { ActiveSupport::Deprecation.silence { get ':controller', action: 'index' } }
get "/render_body/simple"
assert_body "hello david"
@@ -95,7 +95,7 @@ class RenderBodyTest < Rack::TestCase
test "rendering body from an action with default options renders the body without the layout" do
with_routing do |set|
set.draw { get ':controller', action: 'index' }
set.draw { ActiveSupport::Deprecation.silence { get ':controller', action: 'index' } }
get "/render_body/with_layout"
@@ -88,7 +88,7 @@ class RenderHtmlTest < Rack::TestCase
test "rendering text from an action with default options renders the text with the layout" do
with_routing do |set|
set.draw { get ':controller', action: 'index' }
set.draw { ActiveSupport::Deprecation.silence { get ':controller', action: 'index' } }
get "/render_html/simple"
assert_body "hello david"
@@ -98,7 +98,7 @@ class RenderHtmlTest < Rack::TestCase
test "rendering text from an action with default options renders the text without the layout" do
with_routing do |set|
set.draw { get ':controller', action: 'index' }
set.draw { ActiveSupport::Deprecation.silence { get ':controller', action: 'index' } }
get "/render_html/with_layout"
@@ -80,7 +80,7 @@ class RenderPlainTest < Rack::TestCase
test "rendering text from an action with default options renders the text with the layout" do
with_routing do |set|
set.draw { get ':controller', action: 'index' }
set.draw { ActiveSupport::Deprecation.silence { get ':controller', action: 'index' } }
get "/render_plain/simple"
assert_body "hello david"
@@ -90,7 +90,7 @@ class RenderPlainTest < Rack::TestCase
test "rendering text from an action with default options renders the text without the layout" do
with_routing do |set|
set.draw { get ':controller', action: 'index' }
set.draw { ActiveSupport::Deprecation.silence { get ':controller', action: 'index' } }
get "/render_plain/with_layout"
@@ -177,7 +177,7 @@ def with_custom_layout
class TestWithLayout < Rack::TestCase
test "rendering with implicit layout" do
with_routing do |set|
set.draw { get ':controller', :action => :index }
set.draw { ActiveSupport::Deprecation.silence { get ':controller', :action => :index } }
get "/render_template/with_layout"
@@ -57,7 +57,9 @@ class RenderTest < Rack::TestCase
test "render with blank" do
with_routing do |set|
set.draw do
get ":controller", :action => 'index'
ActiveSupport::Deprecation.silence do
get ":controller", :action => 'index'
end
end
get "/render/blank_render"
@@ -70,7 +72,9 @@ class RenderTest < Rack::TestCase
test "rendering more than once raises an exception" do
with_routing do |set|
set.draw do
get ":controller", :action => 'index'
ActiveSupport::Deprecation.silence do
get ":controller", :action => 'index'
end
end
assert_raises(AbstractController::DoubleRenderError) do
@@ -83,7 +83,7 @@ class RenderTextTest < Rack::TestCase
test "rendering text from an action with default options renders the text with the layout" do
with_routing do |set|
set.draw { get ':controller', action: 'index' }
set.draw { ActiveSupport::Deprecation.silence { get ':controller', action: 'index' } }
ActiveSupport::Deprecation.silence do
get "/render_text/simple"
@@ -96,7 +96,7 @@ class RenderTextTest < Rack::TestCase
test "rendering text from an action with default options renders the text without the layout" do
with_routing do |set|
set.draw { get ':controller', action: 'index' }
set.draw { ActiveSupport::Deprecation.silence { get ':controller', action: 'index' } }
ActiveSupport::Deprecation.silence do
get "/render_text/with_layout"
@@ -286,7 +286,10 @@ def test_redirect_to_record
with_routing do |set|
set.draw do
resources :workshops
get ':controller/:action'
ActiveSupport::Deprecation.silence do
get ':controller/:action'
end
end
get :redirect_to_existing_record
@@ -328,7 +331,9 @@ def test_redirect_to_with_block_and_assigns
def test_redirect_to_with_block_and_accepted_options
with_routing do |set|
set.draw do
get ':controller/:action'
ActiveSupport::Deprecation.silence do
get ':controller/:action'
end
end
get :redirect_to_with_block_and_options
@@ -615,7 +615,10 @@ def test_head_with_location_object
with_routing do |set|
set.draw do
resources :customers
get ':controller/:action'
ActiveSupport::Deprecation.silence do
get ':controller/:action'
end
end
get :head_with_location_object
@@ -72,7 +72,10 @@ def test_rendering_with_object_location_should_set_header_with_url_for
with_routing do |set|
set.draw do
resources :customers
get ':controller/:action'
ActiveSupport::Deprecation.silence do
get ':controller/:action'
end
end
get :render_with_object_location
Oops, something went wrong.

0 comments on commit 6520ea5

Please sign in to comment.