Routing uses URI escaping for path components and CGI escaping for query parameters.

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

3 files changed, 19 insertions, 9 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index 40b4f5b746..f303154f31 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Routing uses URI escaping for path components and CGI escaping for query parameters.  [darix, Jeremy Kemper]
+
 * Fix assert_redirected_to bug where redirecting from a nested to to a top-level controller incorrectly added the current controller's nesting.  Closes #6128.  [Rick Olson]
 
 * Singleton resources: POST /singleton => create, GET /singleton/new => new.  [Jeremy Kemper]
diff --git a/actionpack/lib/action_controller/routing.rb b/actionpack/lib/action_controller/routing.rb
index 8ae1cb06d9..88b60396c6 100644
--- a/actionpack/lib/action_controller/routing.rb
+++ b/actionpack/lib/action_controller/routing.rb
@@ -1,4 +1,5 @@
 require 'cgi'
+require 'uri'
 
 class Object
   def to_param
@@ -601,7 +602,7 @@ module ActionController
       end
   
       def interpolation_chunk
-        raw? ? value : CGI.escape(value)
+        raw? ? value : URI.escape(value)
       end
   
       def regexp_chunk
@@ -682,7 +683,7 @@ module ActionController
       end
   
       def interpolation_chunk
-        "\#{CGI.escape(#{local_name}.to_s)}"
+        "\#{URI.escape(#{local_name}.to_s)}"
       end
   
       def string_structure(prior_segments)
@@ -731,7 +732,7 @@ module ActionController
         "(?i-:(#{(regexp || Regexp.union(*possible_names)).source}))"
       end
 
-      # Don't CGI.escape the controller name, since it may have slashes in it,
+      # Don't URI.escape the controller name, since it may have slashes in it,
       # like admin/foo.
       def interpolation_chunk
         "\#{#{local_name}.to_s}"
@@ -753,9 +754,9 @@ module ActionController
     end
 
     class PathSegment < DynamicSegment
-      EscapedSlash = CGI.escape("/")
+      EscapedSlash = URI.escape("/")
       def interpolation_chunk
-        "\#{CGI.escape(#{local_name}.to_s).gsub(#{EscapedSlash.inspect}, '/')}"
+        "\#{URI.escape(#{local_name}.to_s).gsub(#{EscapedSlash.inspect}, '/')}"
       end
 
       def default
@@ -777,7 +778,7 @@ module ActionController
       class Result < ::Array #:nodoc:
         def to_s() join '/' end 
         def self.new_escaped(strings)
-          new strings.collect {|str| CGI.unescape str}
+          new strings.collect {|str| URI.unescape str}
         end     
       end     
     end
@@ -1256,7 +1257,7 @@ module ActionController
       end
   
       def recognize_path(path, environment={})
-        path = CGI.unescape(path)
+        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 43397dee35..ced8ca2dc6 100644
--- a/actionpack/test/controller/routing_test.rb
+++ b/actionpack/test/controller/routing_test.rb
@@ -207,8 +207,15 @@ class LegacyRouteSetTests < Test::Unit::TestCase
       map.path 'file/*path', :controller => 'content', :action => 'show_file'
       map.connect ':controller/:action/:id'
     end
+
+    # No + to space in URI escaping, only for query params.
     results = rs.recognize_path "/file/hello+world/how+are+you%3F"
     assert results, "Recognition should have succeeded"
+    assert_equal ['hello+world', 'how+are+you?'], results[:path]
+
+    # Use %20 for space instead.
+    results = rs.recognize_path "/file/hello%20world/how%20are%20you%3F"
+    assert results, "Recognition should have succeeded"
     assert_equal ['hello world', 'how are you?'], results[:path]
 
     results = rs.recognize_path "/file"
@@ -1457,11 +1464,11 @@ class RouteSetTest < Test::Unit::TestCase
   
   def test_recognize_with_encoded_id_and_regex
     set.draw do |map|
-      map.connect 'page/:id', :controller => 'pages', :action => 'show', :id => /[a-zA-Z0-9 ]+/
+      map.connect 'page/:id', :controller => 'pages', :action => 'show', :id => /[a-zA-Z0-9\+]+/
     end
 
     assert_equal({:controller => 'pages', :action => 'show', :id => '10'}, set.recognize_path('/page/10'))
-    assert_equal({:controller => 'pages', :action => 'show', :id => 'hello world'}, set.recognize_path('/page/hello+world'))
+    assert_equal({:controller => 'pages', :action => 'show', :id => 'hello+world'}, set.recognize_path('/page/hello+world'))
   end
 
   def test_recognize_with_conditions