Fixed optimized route segment escaping. Closes #9562.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@7487 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

4 files changed, 38 insertions, 30 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index 6b2f30f1b1..f8ac1237ea 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Fixed optimized route segment escaping.  #9562 [wildchild, Jeremy Kemper]
+
 * root_path returns '/' not ''.  #9563 [lifofifo]
 
 * Fixed that setting request.format should also affect respond_to blocks [DHH]
diff --git a/actionpack/lib/action_controller/routing.rb b/actionpack/lib/action_controller/routing.rb
index bbf6eefd8f..adeb70235c 100644
--- a/actionpack/lib/action_controller/routing.rb
+++ b/actionpack/lib/action_controller/routing.rb
@@ -718,8 +718,8 @@ module ActionController
         s << "\n#{expiry_statement}"
       end
   
-      def interpolation_chunk
-        "\#{URI.escape(#{local_name}.to_s, ActionController::Routing::Segment::UNSAFE_PCHAR)}"
+      def interpolation_chunk(value_code = "#{local_name}.to_s")
+        "\#{URI.escape(#{value_code}, ActionController::Routing::Segment::UNSAFE_PCHAR)}"
       end
   
       def string_structure(prior_segments)
@@ -776,8 +776,8 @@ module ActionController
       end
 
       # Don't URI.escape the controller name since it may contain slashes.
-      def interpolation_chunk
-        "\#{#{local_name}.to_s}"
+      def interpolation_chunk(value_code = "#{local_name}.to_s")
+        "\#{#{value_code}}"
       end
 
       # Make sure controller names like Admin/Content are correctly normalized to
@@ -799,8 +799,8 @@ module ActionController
       RESERVED_PCHAR = "#{Segment::RESERVED_PCHAR}/"
       UNSAFE_PCHAR = Regexp.new("[^#{URI::REGEXP::PATTERN::UNRESERVED}#{RESERVED_PCHAR}]", false, 'N').freeze
 
-      def interpolation_chunk
-        "\#{URI.escape(#{local_name}.to_s, ActionController::Routing::PathSegment::UNSAFE_PCHAR)}"
+      def interpolation_chunk(value_code = "#{local_name}.to_s")
+        "\#{URI.escape(#{value_code}, ActionController::Routing::PathSegment::UNSAFE_PCHAR)}"
       end
 
       def default
@@ -1150,34 +1150,33 @@ module ActionController
             @module.send(:protected, selector)
             helpers << selector
           end
-          
+
           def define_url_helper(route, name, kind, options)
             selector = url_helper_name(name, kind)
             # The segment keys used for positional paramters
-            
+
             hash_access_method = hash_access_name(name, kind)
-            
+
+            # allow ordered parameters to be associated with corresponding
+            # dynamic segments, so you can do
+            #
+            #   foo_url(bar, baz, bang)
+            #
+            # instead of
+            #
+            #   foo_url(:bar => bar, :baz => baz, :bang => bang)
+            #
+            # Also allow options hash, so you can do
+            #
+            #   foo_url(bar, baz, bang, :sort_by => 'baz')
+            #
             @module.send :module_eval, <<-end_eval # We use module_eval to avoid leaks
               def #{selector}(*args)
-
                 #{generate_optimisation_block(route, kind)}
-                
+
                 opts = if args.empty? || Hash === args.first
                   args.first || {}
                 else
-                  # allow ordered parameters to be associated with corresponding
-                  # dynamic segments, so you can do
-                  #
-                  #   foo_url(bar, baz, bang)
-                  #
-                  # instead of
-                  #
-                  #   foo_url(:bar => bar, :baz => baz, :bang => bang)
-                  #
-                  # Also allow options hash, so you can do
-                  #
-                  #   foo_url(bar, baz, bang, :sort_by => 'baz')
-                  #
                   options = args.last.is_a?(Hash) ? args.pop : {}
                   args = args.zip(#{route.segment_keys.inspect}).inject({}) do |h, (v, k)|
                     h[k] = v
@@ -1185,7 +1184,7 @@ module ActionController
                   end
                   options.merge(args)
                 end
-                
+
                 url_for(#{hash_access_method}(opts))
               end
             end_eval
@@ -1193,7 +1192,7 @@ module ActionController
             helpers << selector
           end
       end
-  
+
       attr_accessor :routes, :named_routes
   
       def initialize
@@ -1445,4 +1444,4 @@ module ActionController
 
     Routes = RouteSet.new
   end
-end
\ No newline at end of file
+end
diff --git a/actionpack/lib/action_controller/routing_optimisation.rb b/actionpack/lib/action_controller/routing_optimisation.rb
index 1b447b17e7..eaff1869dd 100644
--- a/actionpack/lib/action_controller/routing_optimisation.rb
+++ b/actionpack/lib/action_controller/routing_optimisation.rb
@@ -56,7 +56,6 @@ module ActionController
           elements = []
           idx = 0
 
-
           if kind == :url
             elements << '#{request.protocol}'
             elements << '#{request.host_with_port}'
@@ -67,10 +66,10 @@ module ActionController
           # we don't include the trailing slashes, so skip them.
           ((route.segments.size == 1 && kind == :path) ? route.segments : route.segments[0..-2]).each do |segment|
             if segment.is_a?(DynamicSegment)
-              elements << "\#{URI.escape(args[#{idx}].to_param, ActionController::Routing::Segment::UNSAFE_PCHAR)}"
+              elements << segment.interpolation_chunk("args[#{idx}].to_param")
               idx += 1
             else
-              elements << segment.to_s
+              elements << segment.interpolation_chunk
             end
           end
           %("#{elements * ''}")
diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb
index fee1ad3ccf..793e1d8cdf 100644
--- a/actionpack/test/controller/routing_test.rb
+++ b/actionpack/test/controller/routing_test.rb
@@ -273,6 +273,14 @@ class LegacyRouteSetTests < Test::Unit::TestCase
     assert_equal [], results[:path]
   end
   
+  def test_paths_slashes_unescaped_with_ordered_parameters
+    rs.add_named_route :path, '/file/*path', :controller => 'content' 
+
+    # No / to %2F in URI, only for query params. 
+    x = setup_for_named_route 
+    assert_equal("/file/hello/world", x.send(:path_path, 'hello/world'))
+  end
+  
   def test_non_controllers_cannot_be_matched
     rs.draw do |map|
       map.connect ':controller/:action/:id'