Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Rationalize route path escaping according to RFC 2396 section 3.3. Cl…

…oses #7544, #8307.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@6730 5ecf4fe2-1ee6-0310-87b1-e25e094e27de
  • Loading branch information...
commit 50253edec97429ea92621bce6578cdedfcc19b62 1 parent c769ad8
Jeremy Kemper jeremy authored
2  actionpack/CHANGELOG
View
@@ -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
32 actionpack/lib/action_controller/routing.rb
View
@@ -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 @@ def requirement_for(key)
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 @@ def continue_string_structure(prior_segments)
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 @@ def initialize(value = nil)
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 @@ def extraction_code
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 @@ def build_pattern(pattern)
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 @@ def regexp_chunk
"(?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 @@ def match_extraction(next_capture)
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
61 actionpack/test/controller/routing_test.rb
View
@@ -13,44 +13,36 @@ def warn(msg)
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 @@ def test_default_route_should_work_with_action_but_no_id
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
Please sign in to comment.
Something went wrong with that request. Please try again.