Rationalize route path escaping according to RFC 2396 section 3.3. Closes #7544, #8307.

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

3 files changed, 53 insertions, 42 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index b6907d831e..25210bb34f 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Rationalize route path escaping according to RFC 2396 section 3.3.  #7544, #8307. [Jeremy Kemper, chrisroos, begemot, jugend]
+
 * Added record identification with polymorphic routes for ActionController::Base#url_for and ActionView::Base#url_for [DHH]. Examples:
 
     redirect_to(post)         # => redirect_to(posts_url(post))         => Location: http://example.com/posts/1
diff --git a/actionpack/lib/action_controller/routing.rb b/actionpack/lib/action_controller/routing.rb
index 49661850ea..be5eb164a1 100644
--- a/actionpack/lib/action_controller/routing.rb
+++ b/actionpack/lib/action_controller/routing.rb
@@ -248,6 +248,7 @@ module ActionController
   #  end
   #
   module Routing
+    # TODO: , (comma) should be an allowed path character.
     SEPARATORS = %w( / ; . , ? )
 
     # The root paths which may contain controller files
@@ -547,6 +548,10 @@ module ActionController
     end
 
     class Segment #:nodoc:
+      # TODO: , (comma) should be an allowed path character.
+      RESERVED_PCHAR = ':@&=+$'
+      UNSAFE_PCHAR = Regexp.new("[^#{URI::REGEXP::PATTERN::UNRESERVED}#{RESERVED_PCHAR}]", false, 'N').freeze
+
       attr_accessor :is_optional
       alias_method :optional?, :is_optional
 
@@ -567,7 +572,11 @@ module ActionController
           prior_segments.last.string_structure(new_priors)
         end
       end
-  
+
+      def interpolation_chunk
+        URI.escape(value, UNSAFE_PCHAR)
+      end
+
       # Return a string interpolation statement for this segment and those before it.
       def interpolation_statement(prior_segments)
         chunks = prior_segments.collect { |s| s.interpolation_chunk }
@@ -611,11 +620,11 @@ module ActionController
       end
   
       def interpolation_chunk
-        raw? ? value : URI.escape(value)
+        raw? ? value : super
       end
   
       def regexp_chunk
-        chunk = Regexp.escape value
+        chunk = Regexp.escape(value)
         optional? ? Regexp.optionalize(chunk) : chunk
       end
   
@@ -692,7 +701,7 @@ module ActionController
       end
   
       def interpolation_chunk
-        "\#{CGI.escape(#{local_name}.to_s)}"
+        "\#{URI.escape(#{local_name}.to_s, ActionController::Routing::Segment::UNSAFE_PCHAR)}"
       end
   
       def string_structure(prior_segments)
@@ -723,12 +732,12 @@ module ActionController
         optional? ? Regexp.optionalize(pattern) : pattern
       end
       def match_extraction(next_capture)
-        # All non code-related keys (such as :id, :slug) have to be unescaped as other CGI params
+        # All non code-related keys (such as :id, :slug) are URI-unescaped as
+        # path parameters.
         default_value = default ? default.inspect : nil
         %[
           value = if (m = match[#{next_capture}])
-            m = m.gsub('+', '%2B')
-            CGI.unescape(m)
+            URI.unescape(m)
           else
             #{default_value}
           end
@@ -748,8 +757,7 @@ module ActionController
         "(?i-:(#{(regexp || Regexp.union(*possible_names)).source}))"
       end
 
-      # Don't URI.escape the controller name, since it may have slashes in it,
-      # like admin/foo.
+      # Don't URI.escape the controller name since it may contain slashes.
       def interpolation_chunk
         "\#{#{local_name}.to_s}"
       end
@@ -770,9 +778,11 @@ module ActionController
     end
 
     class PathSegment < DynamicSegment #:nodoc:
-      EscapedSlash = URI.escape("/")
+      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).gsub(#{EscapedSlash.inspect}, '/')}"
+        "\#{URI.escape(#{local_name}.to_s, ActionController::Routing::PathSegment::UNSAFE_PCHAR)}"
       end
 
       def default
diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb
index 9c46f23609..0197bf10e2 100644
--- a/actionpack/test/controller/routing_test.rb
+++ b/actionpack/test/controller/routing_test.rb
@@ -13,44 +13,36 @@ class ROUTING::RouteBuilder
   end
 end
 
+# See RFC 3986, section 3.3 for allowed path characters.
 class UriReservedCharactersRoutingTest < Test::Unit::TestCase
-  # See RFC 3986, section 2.2 Reserved Characters
-  
   def setup
     ActionController::Routing.use_controllers! ['controller']
     @set = ActionController::Routing::RouteSet.new
     @set.draw do |map|
-      map.connect ':controller/:action/:var'
+      map.connect ':controller/:action/:variable'
     end
+
+    # TODO: perhaps , (comma) shouldn't be a route separator.
+    safe, unsafe = %w(: @ & = + $), %w(, ^ / ? # [ ] ;)
+    hex = unsafe.map { |char| '%' + char.unpack('H2').first.upcase }
+
+    @segment = "#{safe}#{unsafe}".freeze
+    @escaped = "#{safe}#{hex}".freeze
   end
-  
-  def test_should_escape_reserved_uri_characters_within_individual_path_components
-    assert_equal '/controller/action/p1%3Ap2', @set.generate(:controller => 'controller', :action => 'action', :var => 'p1:p2')
-    assert_equal '/controller/action/p1%2Fp2', @set.generate(:controller => 'controller', :action => 'action', :var => 'p1/p2')
-    assert_equal '/controller/action/p1%3Fp2', @set.generate(:controller => 'controller', :action => 'action', :var => 'p1?p2')
-    assert_equal '/controller/action/p1%23p2', @set.generate(:controller => 'controller', :action => 'action', :var => 'p1#p2')
-    assert_equal '/controller/action/p1%5Bp2', @set.generate(:controller => 'controller', :action => 'action', :var => 'p1[p2')
-    assert_equal '/controller/action/p1%5Dp2', @set.generate(:controller => 'controller', :action => 'action', :var => 'p1]p2')
-    assert_equal '/controller/action/p1%40p2', @set.generate(:controller => 'controller', :action => 'action', :var => 'p1@p2')
+
+  def test_route_generation_escapes_unsafe_path_characters
+    assert_equal "/contr#{@segment}oller/act#{@escaped}ion/var#{@escaped}iable",
+      @set.generate(:controller => "contr#{@segment}oller",
+                    :action => "act#{@segment}ion",
+                    :variable => "var#{@segment}iable")
   end
-  
-  def test_should_recognize_escaped_path_component_and_unescape
-    expected_options = {:var => "p1:p2", :controller => "controller", :action => "action"}
-    assert_equal expected_options, @set.recognize_path('/controller/action/p1%3Ap2')
-    expected_options = {:var => "p1/p2", :controller => "controller", :action => "action"}
-    assert_equal expected_options, @set.recognize_path('/controller/action/p1%2Fp2')
-    expected_options = {:var => "p1?p2", :controller => "controller", :action => "action"}
-    assert_equal expected_options, @set.recognize_path('/controller/action/p1%3Fp2')
-    expected_options = {:var => "p1#p2", :controller => "controller", :action => "action"}
-    assert_equal expected_options, @set.recognize_path('/controller/action/p1%23p2')
-    expected_options = {:var => "p1[p2", :controller => "controller", :action => "action"}
-    assert_equal expected_options, @set.recognize_path('/controller/action/p1%5Bp2')
-    expected_options = {:var => "p1]p2", :controller => "controller", :action => "action"}
-    assert_equal expected_options, @set.recognize_path('/controller/action/p1%5Dp2')
-    expected_options = {:var => "p1@p2", :controller => "controller", :action => "action"}
-    assert_equal expected_options, @set.recognize_path('/controller/action/p1%40p2')
+
+  def test_route_recognition_unescapes_path_components
+    options = { :controller => "controller",
+                :action => "act#{@segment}ion",
+                :variable => "var#{@segment}iable" }
+    assert_equal options, @set.recognize_path("/controller/act#{@escaped}ion/var#{@escaped}iable")
   end
-  
 end
 
 class LegacyRouteSetTests < Test::Unit::TestCase
@@ -924,9 +916,16 @@ class RouteTest < Test::Unit::TestCase
     assert_equal '/accounts/list_all', default_route.generate(o, o, {})
   end
   
-  def test_default_route_should_escape_pluses_in_id
-    expected = {:controller => 'accounts', :action => 'show', :id => 'hello world'}
+  def test_default_route_should_uri_escape_pluses
+    expected = { :controller => 'accounts', :action => 'show', :id => 'hello world' }
+    assert_equal expected, default_route.recognize('/accounts/show/hello world')
+    assert_equal expected, default_route.recognize('/accounts/show/hello%20world')
+    assert_equal '/accounts/show/hello%20world', default_route.generate(expected, expected, {})
+
+    expected[:id] = 'hello+world'
     assert_equal expected, default_route.recognize('/accounts/show/hello+world')
+    assert_equal expected, default_route.recognize('/accounts/show/hello%2Bworld')
+    assert_equal '/accounts/show/hello+world', default_route.generate(expected, expected, {})
   end
 
   def test_matches_controller_and_action