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.

4 files changed, 32 insertions, 6 deletions

diff --git a/actionpack/lib/action_dispatch/journey/route.rb b/actionpack/lib/action_dispatch/journey/route.rb
index c8eb0f6f2d..2b399d3ee3 100644
--- a/actionpack/lib/action_dispatch/journey/route.rb
+++ b/actionpack/lib/action_dispatch/journey/route.rb
@@ -101,6 +101,10 @@ module ActionDispatch
         end
       end
 
+      def glob?
+        !path.spec.grep(Nodes::Star).empty?
+      end
+
       def dispatcher?
         @dispatcher
       end
diff --git a/actionpack/lib/action_dispatch/journey/router/utils.rb b/actionpack/lib/action_dispatch/journey/router/utils.rb
index 246d91da01..ac4ecb1e65 100644
--- a/actionpack/lib/action_dispatch/journey/router/utils.rb
+++ b/actionpack/lib/action_dispatch/journey/router/utils.rb
@@ -37,6 +37,7 @@ module ActionDispatch
           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 @@ module ActionDispatch
             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 @@ module ActionDispatch
           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
diff --git a/actionpack/lib/action_dispatch/journey/visitors.rb b/actionpack/lib/action_dispatch/journey/visitors.rb
index daade5bb74..d9f634623d 100644
--- a/actionpack/lib/action_dispatch/journey/visitors.rb
+++ b/actionpack/lib/action_dispatch/journey/visitors.rb
@@ -114,19 +114,26 @@ module ActionDispatch
         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 @@ module ActionDispatch
             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
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index a03fb4cee7..1ec6fa674b 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -155,7 +155,7 @@ module ActionDispatch
           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 @@ module ActionDispatch
             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)