diff options
Diffstat (limited to 'actionpack')
| -rw-r--r-- | actionpack/CHANGELOG | 2 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/routing.rb | 32 | ||||
| -rw-r--r-- | actionpack/test/controller/routing_test.rb | 61 |
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 |