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.

Author: pixeltrix

actionpack/CHANGELOG.md

@@ -1,3 +1,19 @@
+*   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.
+
+    *Andrew White*, *Edho Arief*
+
 *   Add alias `ActionDispatch::Http::UploadedFile#to_io` to
     `ActionDispatch::Http::UploadedFile#tempfile`.
 

actionpack/lib/action_dispatch/journey/route.rb

@@ -101,6 +101,10 @@ def required_defaults
         end
       end
 
+      def glob?
+        !path.spec.grep(Nodes::Star).empty?
+      end
+
       def dispatcher?
         @dispatcher
       end

actionpack/lib/action_dispatch/journey/router/utils.rb

@@ -37,6 +37,7 @@ class UriEncoder # :nodoc:
           ESCAPED  = /%[a-zA-Z0-9]{2}/.freeze
 
           FRAGMENT = /[^#{UNRESERVED}#{SUB_DELIMS}:@\/\?]/.freeze
+          SEGMENT  = /[^#{UNRESERVED}#{SUB_DELIMS}:@]/.freeze
           PATH     = /[^#{UNRESERVED}#{SUB_DELIMS}:@\/]/.freeze
 
           def escape_fragment(fragment)
@@ -47,6 +48,10 @@ def escape_path(path)
             escape(path, PATH)
           end
 
+          def escape_segment(segment)
+            escape(segment, SEGMENT)
+          end
+
           def unescape_uri(uri)
             uri.gsub(ESCAPED) { [$&[1, 2].hex].pack('C') }.force_encoding(uri.encoding)
           end
@@ -69,6 +74,10 @@ def self.escape_path(path)
           ENCODER.escape_path(path.to_s)
         end
 
+        def self.escape_segment(segment)
+          ENCODER.escape_segment(segment.to_s)
+        end
+
         def self.escape_fragment(fragment)
           ENCODER.escape_fragment(fragment.to_s)
         end

actionpack/lib/action_dispatch/journey/visitors.rb

@@ -114,19 +114,26 @@ def initialize(options)
         end
 
         private
+          def escape_path(value)
+            Router::Utils.escape_path(value)
+          end
+
+          def escape_segment(value)
+            Router::Utils.escape_segment(value)
+          end
 
           def visit(node, optional = false)
             case node.type
             when :LITERAL, :SLASH, :DOT
               node.left
             when :STAR
-              visit(node.left)
+              visit_STAR(node.left)
             when :GROUP
               visit(node.left, true)
             when :CAT
               visit_CAT(node, optional)
             when :SYMBOL
-              visit_SYMBOL(node)
+              visit_SYMBOL(node, node.to_sym)
             end
           end
 
@@ -141,9 +148,15 @@ def visit_CAT(node, optional)
             end
           end
 
-          def visit_SYMBOL(node)
+          def visit_STAR(node)
             if value = options[node.to_sym]
-              Router::Utils.escape_path(value)
+              escape_path(value)
+            end
+          end
+
+          def visit_SYMBOL(node, name)
+            if value = options[name]
+              name == :controller ? escape_path(value) : escape_segment(value)
             end
           end
       end

actionpack/lib/action_dispatch/routing/route_set.rb

@@ -155,7 +155,7 @@ def self.create(route, options)
           end
 
           def self.optimize_helper?(route)
-            route.requirements.except(:controller, :action).empty?
+            !route.glob? && route.requirements.except(:controller, :action).empty?
           end
 
           class OptimizedUrlHelper < UrlHelper # :nodoc:
@@ -194,7 +194,7 @@ def optimized_helper(args)
             end
 
             def replace_segment(params, segment)
-              Symbol === segment ? @klass.escape_fragment(params[segment]) : segment
+              Symbol === segment ? @klass.escape_segment(params[segment]) : segment
             end
 
             def optimize_routes_generation?(t)

actionpack/test/dispatch/routing_test.rb

@@ -3596,16 +3596,16 @@ 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
     get '/a%20b%2Fc+d'
     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 @@ def app; Routes end
     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

actionpack/test/journey/router/utils_test.rb

@@ -8,6 +8,10 @@ def test_path_escape
           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

actionpack/test/journey/router_test.rb

@@ -367,7 +367,18 @@ def test_generate_escapes
           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