Make URL escaping more consistent

1. Escape '%' characters in URLs - only unescaped data
   should be passed to URL helpers

2. Add an `escape_segment` helper to `Router::Utils`
   that escapes '/' characters

3. Use `escape_segment` rather than `escape_fragment`
   in optimized URL generation

4. Use `escape_segment` rather than `escape_path`
   in URL generation

For point 4 there are two exceptions. Firstly, when a route uses wildcard
segments (e.g. *foo) then we use `escape_path` as the value may contain '/'
characters. This means that wildcard routes can't be optimized. Secondly,
if a `:controller` segment is used in the path then this uses `escape_path`
as the controller may be namespaced.

Fixes #14629, #14636 and #14070.

3 files changed, 41 insertions, 4 deletions

diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
index f74a0ef945..b22a56bb27 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -3596,8 +3596,8 @@ class TestUriPathEscaping < ActionDispatch::IntegrationTest
   include Routes.url_helpers
   def app; Routes end
 
-  test 'escapes generated path segment' do
-    assert_equal '/a%20b/c+d', segment_path(:segment => 'a b/c+d')
+  test 'escapes slash in generated path segment' do
+    assert_equal '/a%20b%2Fc+d', segment_path(:segment => 'a b/c+d')
   end
 
   test 'unescapes recognized path segment' do
@@ -3605,7 +3605,7 @@ class TestUriPathEscaping < ActionDispatch::IntegrationTest
     assert_equal 'a b/c+d', @response.body
   end
 
-  test 'escapes generated path splat' do
+  test 'does not escape slash in generated path splat' do
     assert_equal '/a%20b/c+d', splat_path(:splat => 'a b/c+d')
   end
 
@@ -3790,6 +3790,8 @@ class TestOptimizedNamedRoutes < ActionDispatch::IntegrationTest
       get '/post(/:action(/:id))' => ok, as: :posts
       get '/:foo/:foo_type/bars/:id' => ok, as: :bar
       get '/projects/:id.:format' => ok, as: :project
+      get '/pages/:id' => ok, as: :page
+      get '/wiki/*page' => ok, as: :wiki
     end
   end
 
@@ -3822,6 +3824,26 @@ class TestOptimizedNamedRoutes < ActionDispatch::IntegrationTest
     assert_equal '/projects/1.json', Routes.url_helpers.project_path(1, :json)
     assert_equal '/projects/1.json', project_path(1, :json)
   end
+
+  test 'segments with question marks are escaped' do
+    assert_equal '/pages/foo%3Fbar', Routes.url_helpers.page_path('foo?bar')
+    assert_equal '/pages/foo%3Fbar', page_path('foo?bar')
+  end
+
+  test 'segments with slashes are escaped' do
+    assert_equal '/pages/foo%2Fbar', Routes.url_helpers.page_path('foo/bar')
+    assert_equal '/pages/foo%2Fbar', page_path('foo/bar')
+  end
+
+  test 'glob segments with question marks are escaped' do
+    assert_equal '/wiki/foo%3Fbar', Routes.url_helpers.wiki_path('foo?bar')
+    assert_equal '/wiki/foo%3Fbar', wiki_path('foo?bar')
+  end
+
+  test 'glob segments with slashes are not escaped' do
+    assert_equal '/wiki/foo/bar', Routes.url_helpers.wiki_path('foo/bar')
+    assert_equal '/wiki/foo/bar', wiki_path('foo/bar')
+  end
 end
 
 class TestNamedRouteUrlHelpers < ActionDispatch::IntegrationTest
diff --git a/actionpack/test/journey/router/utils_test.rb b/actionpack/test/journey/router/utils_test.rb
index 8b3a4e340a..584fd56a5c 100644
--- a/actionpack/test/journey/router/utils_test.rb
+++ b/actionpack/test/journey/router/utils_test.rb
@@ -8,6 +8,10 @@ module ActionDispatch
           assert_equal "a/b%20c+d%25", Utils.escape_path("a/b c+d%")
         end
 
+        def test_segment_escape
+          assert_equal "a%2Fb%20c+d%25", Utils.escape_segment("a/b c+d%")
+        end
+
         def test_fragment_escape
           assert_equal "a/b%20c+d%25?e", Utils.escape_fragment("a/b c+d%?e")
         end
diff --git a/actionpack/test/journey/router_test.rb b/actionpack/test/journey/router_test.rb
index a286f77633..e54b64e0f3 100644
--- a/actionpack/test/journey/router_test.rb
+++ b/actionpack/test/journey/router_test.rb
@@ -367,7 +367,18 @@ module ActionDispatch
           nil, { :controller        => "tasks",
                  :action            => "a/b c+d",
         }, {})
-        assert_equal '/tasks/a/b%20c+d', path
+        assert_equal '/tasks/a%2Fb%20c+d', path
+      end
+
+      def test_generate_escapes_with_namespaced_controller
+        path  = Path::Pattern.new '/:controller(/:action)'
+        @router.routes.add_route @app, path, {}, {}, {}
+
+        path, _ = @formatter.generate(:path_info,
+          nil, { :controller        => "admin/tasks",
+                 :action            => "a/b c+d",
+        }, {})
+        assert_equal '/admin/tasks/a%2Fb%20c+d', path
       end
 
       def test_generate_extra_params