Routing: better support for escaped values in route segments. Closes #7544.

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

3 files changed, 55 insertions, 6 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index 4c4136a105..e131ec0f9e 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,8 @@
 *SVN*
 
+* Routing: better support for escaped values in route segments.  #7544 [Chris
+Roos]
+
 * Introduce a cookie-based session store as the Rails default. Sessions typically contain at most a user_id and flash message; both fit within the 4K cookie size limit. A secure hash is included with the cookie to ensure data integrity (a user cannot alter his user_id without knowing the secret key included in the hash). If you have more than 4K of session data or don't want your data to be visible to the user, pick another session store. Cookie-based sessions are dramatically faster than the alternatives.  [Jeremy Kemper]
 
 * Added .erb and .builder as preferred aliases to the now deprecated .rhtml and .rxml extensions [Chad Fowler]. This is done to separate the renderer from the mime type. .erb templates are often used to render emails, atom, csv, whatever. So labeling them .rhtml doesn't make too much sense. The same goes for .rxml, which can be used to build everything from HTML to Atom to whatever. .rhtml and .rxml will continue to work until Rails 3.0, though. So this is a slow phasing out. All generators and examples will start using the new aliases, though.
diff --git a/actionpack/lib/action_controller/routing.rb b/actionpack/lib/action_controller/routing.rb
index 58207eed74..723aaada83 100644
--- a/actionpack/lib/action_controller/routing.rb
+++ b/actionpack/lib/action_controller/routing.rb
@@ -412,7 +412,7 @@ module ActionController
       def recognition_extraction
         next_capture = 1
         extraction = segments.collect do |segment|
-          x = segment.match_extraction next_capture
+          x = segment.match_extraction(next_capture)
           next_capture += Regexp.new(segment.regexp_chunk).number_of_captures
           x
         end
@@ -694,7 +694,7 @@ module ActionController
       end
   
       def interpolation_chunk
-        "\#{URI.escape(#{local_name}.to_s)}"
+        "\#{CGI.escape(#{local_name}.to_s)}"
       end
   
       def string_structure(prior_segments)
@@ -725,10 +725,17 @@ module ActionController
         optional? ? Regexp.optionalize(pattern) : pattern
       end
       def match_extraction(next_capture)
-        hangon = (default ? "|| #{default.inspect}" : "if match[#{next_capture}]")
-        
         # All non code-related keys (such as :id, :slug) have to be unescaped as other CGI params
-        "params[:#{key}] = match[#{next_capture}] #{hangon}"
+        default_value = default ? default.inspect : nil
+        %[
+          value = if (m = match[#{next_capture}])
+            m = m.gsub('+', '%2B')
+            CGI.unescape(m)
+          else
+            #{default_value}
+          end
+          params[:#{key}] = value if value
+        ]
       end
   
       def optionality_implied?
@@ -1292,7 +1299,6 @@ module ActionController
       end
   
       def recognize_path(path, environment={})
-        path = URI.unescape(path)
         routes.each do |route|
           result = route.recognize(path, environment) and return result
         end
diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb
index f81d0393f0..5f75bf3876 100644
--- a/actionpack/test/controller/routing_test.rb
+++ b/actionpack/test/controller/routing_test.rb
@@ -13,6 +13,46 @@ class ROUTING::RouteBuilder
   end
 end
 
+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'
+    end
+  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')
+  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')
+  end
+  
+end
+
 class LegacyRouteSetTests < Test::Unit::TestCase
   attr_reader :rs
   def setup