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.

9 files changed, 101 insertions, 35 deletions

diff --git a/actionpack/test/dispatch/request/json_params_parsing_test.rb b/actionpack/test/dispatch/request/json_params_parsing_test.rb
index 3655c7f570..a07138b55e 100644
--- a/actionpack/test/dispatch/request/json_params_parsing_test.rb
+++ b/actionpack/test/dispatch/request/json_params_parsing_test.rb
@@ -103,7 +103,9 @@ class JsonParamsParsingTest < ActionDispatch::IntegrationTest
     def with_test_routing
       with_routing do |set|
         set.draw do
-          post ':action', :to => ::JsonParamsParsingTest::TestController
+          ActiveSupport::Deprecation.silence do
+            post ':action', :to => ::JsonParamsParsingTest::TestController
+          end
         end
         yield
       end
@@ -191,7 +193,9 @@ class RootLessJSONParamsParsingTest < ActionDispatch::IntegrationTest
     def with_test_routing(controller)
       with_routing do |set|
         set.draw do
-          post ':action', :to => controller
+          ActiveSupport::Deprecation.silence do
+            post ':action', :to => controller
+          end
         end
         yield
       end
diff --git a/actionpack/test/dispatch/request/multipart_params_parsing_test.rb b/actionpack/test/dispatch/request/multipart_params_parsing_test.rb
index b36fbd3c76..bab4413b2a 100644
--- a/actionpack/test/dispatch/request/multipart_params_parsing_test.rb
+++ b/actionpack/test/dispatch/request/multipart_params_parsing_test.rb
@@ -159,7 +159,9 @@ class MultipartParamsParsingTest < ActionDispatch::IntegrationTest
   test "does not raise EOFError on GET request with multipart content-type" do
     with_routing do |set|
       set.draw do
-        get ':action', controller: 'multipart_params_parsing_test/test'
+        ActiveSupport::Deprecation.silence do
+          get ':action', controller: 'multipart_params_parsing_test/test'
+        end
       end
       headers = { "CONTENT_TYPE" => "multipart/form-data; boundary=AaB03x" }
       get "/parse", headers: headers
@@ -188,7 +190,9 @@ class MultipartParamsParsingTest < ActionDispatch::IntegrationTest
     def with_test_routing
       with_routing do |set|
         set.draw do
-          post ':action', :controller => 'multipart_params_parsing_test/test'
+          ActiveSupport::Deprecation.silence do
+            post ':action', :controller => 'multipart_params_parsing_test/test'
+          end
         end
         yield
       end
diff --git a/actionpack/test/dispatch/request/query_string_parsing_test.rb b/actionpack/test/dispatch/request/query_string_parsing_test.rb
index bc6716525e..f04022a544 100644
--- a/actionpack/test/dispatch/request/query_string_parsing_test.rb
+++ b/actionpack/test/dispatch/request/query_string_parsing_test.rb
@@ -144,7 +144,9 @@ class QueryStringParsingTest < ActionDispatch::IntegrationTest
   test "ambiguous query string returns a bad request" do
     with_routing do |set|
       set.draw do
-        get ':action', :to => ::QueryStringParsingTest::TestController
+        ActiveSupport::Deprecation.silence do
+          get ':action', :to => ::QueryStringParsingTest::TestController
+        end
       end
 
       get "/parse", headers: { "QUERY_STRING" => "foo[]=bar&foo[4]=bar" }
@@ -156,7 +158,9 @@ class QueryStringParsingTest < ActionDispatch::IntegrationTest
     def assert_parses(expected, actual)
       with_routing do |set|
         set.draw do
-          get ':action', :to => ::QueryStringParsingTest::TestController
+          ActiveSupport::Deprecation.silence do
+            get ':action', :to => ::QueryStringParsingTest::TestController
+          end
         end
         @app = self.class.build_app(set) do |middleware|
           middleware.use(EarlyParse)
diff --git a/actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb b/actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb
index 365edf849a..b9f8c52378 100644
--- a/actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb
+++ b/actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb
@@ -140,7 +140,9 @@ class UrlEncodedParamsParsingTest < ActionDispatch::IntegrationTest
     def with_test_routing
       with_routing do |set|
         set.draw do
-          post ':action', to: ::UrlEncodedParamsParsingTest::TestController
+          ActiveSupport::Deprecation.silence do
+            post ':action', to: ::UrlEncodedParamsParsingTest::TestController
+          end
         end
         yield
       end
diff --git a/actionpack/test/dispatch/routing/inspector_test.rb b/actionpack/test/dispatch/routing/inspector_test.rb
index fd85cc6e9f..d8cffa425f 100644
--- a/actionpack/test/dispatch/routing/inspector_test.rb
+++ b/actionpack/test/dispatch/routing/inspector_test.rb
@@ -133,7 +133,9 @@ module ActionDispatch
 
       def test_inspect_routes_shows_dynamic_action_route
         output = draw do
-          get 'api/:action' => 'api'
+          ActiveSupport::Deprecation.silence do
+            get 'api/:action' => 'api'
+          end
         end
 
         assert_equal [
@@ -144,7 +146,9 @@ module ActionDispatch
 
       def test_inspect_routes_shows_controller_and_action_only_route
         output = draw do
-          get ':controller/:action'
+          ActiveSupport::Deprecation.silence do
+            get ':controller/:action'
+          end
         end
 
         assert_equal [
@@ -155,7 +159,9 @@ module ActionDispatch
 
       def test_inspect_routes_shows_controller_and_action_route_with_constraints
         output = draw do
-          get ':controller(/:action(/:id))', :id => /\d+/
+          ActiveSupport::Deprecation.silence do
+            get ':controller(/:action(/:id))', :id => /\d+/
+          end
         end
 
         assert_equal [
@@ -335,7 +341,9 @@ module ActionDispatch
 
       def test_regression_route_with_controller_regexp
         output = draw do
-          get ':controller(/:action)', controller: /api\/[^\/]+/, format: false
+          ActiveSupport::Deprecation.silence do
+            get ':controller(/:action)', controller: /api\/[^\/]+/, format: false
+          end
         end
 
         assert_equal ["Prefix Verb URI Pattern            Controller#Action",
diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
index 5ead9357ae..09830c0c46 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -116,7 +116,9 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
     assert_raise(ArgumentError) do
       draw do
         namespace :admin do
-          get '/:controller(/:action(/:id(.:format)))'
+          ActiveSupport::Deprecation.silence do
+            get '/:controller(/:action(/:id(.:format)))'
+          end
         end
       end
     end
@@ -125,7 +127,9 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
   def test_namespace_without_controller_segment
     draw do
       namespace :admin do
-        get 'hello/:controllers/:action'
+        ActiveSupport::Deprecation.silence do
+          get 'hello/:controllers/:action'
+        end
       end
     end
     get '/admin/hello/foo/new'
@@ -427,7 +431,10 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
         get 'global/hide_notice'
         get 'global/export',      :action => :export, :as => :export_request
         get '/export/:id/:file',  :action => :export, :as => :export_download, :constraints => { :file => /.*/ }
-        get 'global/:action'
+
+        ActiveSupport::Deprecation.silence do
+          get 'global/:action'
+        end
       end
     end
 
@@ -450,7 +457,9 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
 
   def test_local
     draw do
-      get "/local/:action", :controller => "local"
+      ActiveSupport::Deprecation.silence do
+        get "/local/:action", :controller => "local"
+      end
     end
 
     get '/local/dashboard'
@@ -1506,7 +1515,9 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
 
   def test_not_matching_shorthand_with_dynamic_parameters
     draw do
-      get ':controller/:action/admin'
+      ActiveSupport::Deprecation.silence do
+        get ':controller/:action/admin'
+      end
     end
 
     get '/finances/overview/admin'
@@ -1542,7 +1553,9 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
   def test_scoped_controller_with_namespace_and_action
     draw do
       namespace :account do
-        get ':action/callback', :action => /twitter|github/, :controller => "callbacks", :as => :callback
+        ActiveSupport::Deprecation.silence do
+          get ':action/callback', :action => /twitter|github/, :controller => "callbacks", :as => :callback
+        end
       end
     end
 
@@ -1837,7 +1850,9 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
 
   def test_url_generator_for_generic_route
     draw do
-      get "whatever/:controller(/:action(/:id))"
+      ActiveSupport::Deprecation.silence do
+        get "whatever/:controller(/:action(/:id))"
+      end
     end
 
     get '/whatever/foo/bar'
@@ -1849,7 +1864,9 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
 
   def test_url_generator_for_namespaced_generic_route
     draw do
-      get "whatever/:controller(/:action(/:id))", :id => /\d+/
+      ActiveSupport::Deprecation.silence do
+        get "whatever/:controller(/:action(/:id))", :id => /\d+/
+      end
     end
 
     get '/whatever/foo/bar/show'
@@ -3125,12 +3142,6 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
     end
 
     assert_raise(ArgumentError) do
-      assert_deprecated do
-        draw { controller("/feeds") { get '/feeds/:service', :to => :show } }
-      end
-    end
-
-    assert_raise(ArgumentError) do
       draw { resources :feeds, :controller => '/feeds' }
     end
   end
@@ -3599,6 +3610,22 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
     assert_equal '/?id=1', root_path(params)
   end
 
+  def test_dynamic_controller_segments_are_deprecated
+    assert_deprecated do
+      draw do
+        get '/:controller', action: 'index'
+      end
+    end
+  end
+
+  def test_dynamic_action_segments_are_deprecated
+    assert_deprecated do
+      draw do
+        get '/pages/:action', controller: 'pages'
+      end
+    end
+  end
+
 private
 
   def draw(&block)
@@ -4122,7 +4149,11 @@ class TestOptimizedNamedRoutes < ActionDispatch::IntegrationTest
     app.draw do
       ok = lambda { |env| [200, { 'Content-Type' => 'text/plain' }, []] }
       get '/foo' => ok, as: :foo
-      get '/post(/:action(/:id))' => ok, as: :posts
+
+      ActiveSupport::Deprecation.silence do
+        get '/post(/:action(/:id))' => ok, as: :posts
+      end
+
       get '/:foo/:foo_type/bars/:id' => ok, as: :bar
       get '/projects/:id.:format' => ok, as: :project
       get '/pages/:id' => ok, as: :page
@@ -4292,11 +4323,16 @@ class TestInvalidUrls < ActionDispatch::IntegrationTest
 
   test "invalid UTF-8 encoding returns a 400 Bad Request" do
     with_routing do |set|
-      set.draw do
-        get "/bar/:id", :to => redirect("/foo/show/%{id}")
-        get "/foo/show(/:id)", :to => "test_invalid_urls/foo#show"
-        get "/foo(/:action(/:id))", :controller => "test_invalid_urls/foo"
-        get "/:controller(/:action(/:id))"
+      ActiveSupport::Deprecation.silence do
+        set.draw do
+          get "/bar/:id", :to => redirect("/foo/show/%{id}")
+          get "/foo/show(/:id)", :to => "test_invalid_urls/foo#show"
+
+          ActiveSupport::Deprecation.silence do
+            get "/foo(/:action(/:id))", :controller => "test_invalid_urls/foo"
+            get "/:controller(/:action(/:id))"
+          end
+        end
       end
 
       get "/%E2%EF%BF%BD%A6"
@@ -4627,7 +4663,9 @@ class TestErrorsInController < ActionDispatch::IntegrationTest
 
   Routes = ActionDispatch::Routing::RouteSet.new
   Routes.draw do
-    get '/:controller(/:action)'
+    ActiveSupport::Deprecation.silence do
+      get '/:controller(/:action)'
+    end
   end
 
   APP = build_app Routes
diff --git a/actionpack/test/dispatch/session/cache_store_test.rb b/actionpack/test/dispatch/session/cache_store_test.rb
index dbb996973d..769de1a1e0 100644
--- a/actionpack/test/dispatch/session/cache_store_test.rb
+++ b/actionpack/test/dispatch/session/cache_store_test.rb
@@ -164,7 +164,9 @@ class CacheStoreTest < ActionDispatch::IntegrationTest
     def with_test_route_set
       with_routing do |set|
         set.draw do
-          get ':action', :to => ::CacheStoreTest::TestController
+          ActiveSupport::Deprecation.silence do
+            get ':action', :to => ::CacheStoreTest::TestController
+          end
         end
 
         @app = self.class.build_app(set) do |middleware|
diff --git a/actionpack/test/dispatch/session/cookie_store_test.rb b/actionpack/test/dispatch/session/cookie_store_test.rb
index f07e215e3a..09cb1d925f 100644
--- a/actionpack/test/dispatch/session/cookie_store_test.rb
+++ b/actionpack/test/dispatch/session/cookie_store_test.rb
@@ -345,7 +345,9 @@ class CookieStoreTest < ActionDispatch::IntegrationTest
     def with_test_route_set(options = {})
       with_routing do |set|
         set.draw do
-          get ':action', :to => ::CookieStoreTest::TestController
+          ActiveSupport::Deprecation.silence do
+            get ':action', :to => ::CookieStoreTest::TestController
+          end
         end
 
         options = { :key => SessionKey }.merge!(options)
diff --git a/actionpack/test/dispatch/session/mem_cache_store_test.rb b/actionpack/test/dispatch/session/mem_cache_store_test.rb
index 3fed9bad4f..18cb227dad 100644
--- a/actionpack/test/dispatch/session/mem_cache_store_test.rb
+++ b/actionpack/test/dispatch/session/mem_cache_store_test.rb
@@ -187,7 +187,9 @@ class MemCacheStoreTest < ActionDispatch::IntegrationTest
     def with_test_route_set
       with_routing do |set|
         set.draw do
-          get ':action', :to => ::MemCacheStoreTest::TestController
+          ActiveSupport::Deprecation.silence do
+            get ':action', :to => ::MemCacheStoreTest::TestController
+          end
         end
 
         @app = self.class.build_app(set) do |middleware|