Skip to content
Browse files

Make URL escaping more consistent

1. Escape '%' characters in URLs - only unescaped data
   should be passed to URL helpers

2. Add an `escape_segment` helper to `Router::Utils`
   that escapes '/' characters

3. Use `escape_segment` rather than `escape_fragment`
   in optimized URL generation

4. Use `escape_segment` rather than `escape_path`
   in URL generation

For point 4 there are two exceptions. Firstly, when a route uses wildcard
segments (e.g. *foo) then we use `escape_path` as the value may contain '/'
characters. This means that wildcard routes can't be optimized. Secondly,
if a `:controller` segment is used in the path then this uses `escape_path`
as the controller may be namespaced.

Fixes #14629, #14636 and #14070.

Cherry picked from:
  e2ef83f
  a617925
  5460591

Conflicts:
	actionpack/CHANGELOG.md
  • Loading branch information...
1 parent c1d9c11 commit 8a067640e6fe222022dc77bb63d5da37ef75a189 @nanaya nanaya committed with pixeltrix
View
16 actionpack/CHANGELOG.md
@@ -1,3 +1,19 @@
+* Make URL escaping more consistent:
+
+ 1. Escape '%' characters in URLs - only unescaped data should be passed to URL helpers
+ 2. Add an `escape_segment` helper to `Router::Utils` that escapes '/' characters
+ 3. Use `escape_segment` rather than `escape_fragment` in optimized URL generation
+ 4. Use `escape_segment` rather than `escape_path` in URL generation
+
+ For point 4 there are two exceptions. Firstly, when a route uses wildcard segments
+ (e.g. *foo) then we use `escape_path` as the value may contain '/' characters. This
+ means that wildcard routes can't be optimized. Secondly, if a `:controller` segment
+ is used in the path then this uses `escape_path` as the controller may be namespaced.
+
+ Fixes #14629, #14636 and #14070.
+
+ *Andrew White*, *Edho Arief*
+
* Returns null type format when format is not know and controller is using `any`
format block.
View
4 actionpack/lib/action_dispatch/journey/route.rb
@@ -101,6 +101,10 @@ def required_defaults
end
end
+ def glob?
+ !path.spec.grep(Nodes::Star).empty?
+ end
+
def dispatcher?
@dispatcher
end
View
68 actionpack/lib/action_dispatch/journey/router/utils.rb
@@ -1,5 +1,3 @@
-require 'uri'
-
module ActionDispatch
module Journey # :nodoc:
class Router # :nodoc:
@@ -25,31 +23,67 @@ def self.normalize_path(path)
# URI path and fragment escaping
# http://tools.ietf.org/html/rfc3986
- module UriEscape # :nodoc:
- # Symbol captures can generate multiple path segments, so include /.
- reserved_segment = '/'
- reserved_fragment = '/?'
- reserved_pchar = ':@&=+$,;%'
-
- safe_pchar = "#{URI::REGEXP::PATTERN::UNRESERVED}#{reserved_pchar}"
- safe_segment = "#{safe_pchar}#{reserved_segment}"
- safe_fragment = "#{safe_pchar}#{reserved_fragment}"
- UNSAFE_SEGMENT = Regexp.new("[^#{safe_segment}]", false).freeze
- UNSAFE_FRAGMENT = Regexp.new("[^#{safe_fragment}]", false).freeze
+ class UriEncoder # :nodoc:
+ ENCODE = "%%%02X".freeze
+ ENCODING = Encoding::US_ASCII
+ EMPTY = "".force_encoding(ENCODING).freeze
+ DEC2HEX = (0..255).to_a.map{ |i| ENCODE % i }.map{ |s| s.force_encoding(ENCODING) }
+
+ ALPHA = "a-zA-Z".freeze
+ DIGIT = "0-9".freeze
+ UNRESERVED = "#{ALPHA}#{DIGIT}\\-\\._~".freeze
+ SUB_DELIMS = "!\\$&'\\(\\)\\*\\+,;=".freeze
+
+ ESCAPED = /%[a-zA-Z0-9]{2}/.freeze
+
+ FRAGMENT = /[^#{UNRESERVED}#{SUB_DELIMS}:@\/\?]/.freeze
+ SEGMENT = /[^#{UNRESERVED}#{SUB_DELIMS}:@]/.freeze
+ PATH = /[^#{UNRESERVED}#{SUB_DELIMS}:@\/]/.freeze
+
+ def escape_fragment(fragment)
+ escape(fragment, FRAGMENT)
+ end
+
+ def escape_path(path)
+ escape(path, PATH)
+ end
+
+ def escape_segment(segment)
+ escape(segment, SEGMENT)
+ end
+
+ def unescape_uri(uri)
+ uri.gsub(ESCAPED) { [$&[1, 2].hex].pack('C') }.force_encoding(uri.encoding)
+ end
+
+ protected
+ def escape(component, pattern)
+ component.gsub(pattern){ |unsafe| percent_encode(unsafe) }.force_encoding(ENCODING)
+ end
+
+ def percent_encode(unsafe)
+ safe = EMPTY.dup
+ unsafe.each_byte { |b| safe << DEC2HEX[b] }
+ safe
+ end
end
- Parser = URI::Parser.new
+ ENCODER = UriEncoder.new
def self.escape_path(path)
- Parser.escape(path.to_s, UriEscape::UNSAFE_SEGMENT)
+ ENCODER.escape_path(path.to_s)
+ end
+
+ def self.escape_segment(segment)
+ ENCODER.escape_segment(segment.to_s)
end
def self.escape_fragment(fragment)
- Parser.escape(fragment.to_s, UriEscape::UNSAFE_FRAGMENT)
+ ENCODER.escape_fragment(fragment.to_s)
end
def self.unescape_uri(uri)
- Parser.unescape(uri)
+ ENCODER.unescape_uri(uri)
end
end
end
View
21 actionpack/lib/action_dispatch/journey/visitors.rb
@@ -114,19 +114,26 @@ def initialize(options)
end
private
+ def escape_path(value)
+ Router::Utils.escape_path(value)
+ end
+
+ def escape_segment(value)
+ Router::Utils.escape_segment(value)
+ end
def visit(node, optional = false)
case node.type
when :LITERAL, :SLASH, :DOT
node.left
when :STAR
- visit(node.left)
+ visit_STAR(node.left)
when :GROUP
visit(node.left, true)
when :CAT
visit_CAT(node, optional)
when :SYMBOL
- visit_SYMBOL(node)
+ visit_SYMBOL(node, node.to_sym)
end
end
@@ -141,9 +148,15 @@ def visit_CAT(node, optional)
end
end
- def visit_SYMBOL(node)
+ def visit_STAR(node)
if value = options[node.to_sym]
- Router::Utils.escape_path(value)
+ escape_path(value)
+ end
+ end
+
+ def visit_SYMBOL(node, name)
+ if value = options[name]
+ name == :controller ? escape_path(value) : escape_segment(value)
end
end
end
View
4 actionpack/lib/action_dispatch/routing/route_set.rb
@@ -155,7 +155,7 @@ def self.create(route, options)
end
def self.optimize_helper?(route)
- route.requirements.except(:controller, :action).empty?
+ !route.glob? && route.requirements.except(:controller, :action).empty?
end
class OptimizedUrlHelper < UrlHelper # :nodoc:
@@ -194,7 +194,7 @@ def optimized_helper(args)
end
def replace_segment(params, segment)
- Symbol === segment ? @klass.escape_fragment(params[segment]) : segment
+ Symbol === segment ? @klass.escape_segment(params[segment]) : segment
end
def optimize_routes_generation?(t)
View
28 actionpack/test/dispatch/routing_test.rb
@@ -3596,8 +3596,8 @@ class TestUriPathEscaping < ActionDispatch::IntegrationTest
include Routes.url_helpers
def app; Routes end
- test 'escapes generated path segment' do
- assert_equal '/a%20b/c+d', segment_path(:segment => 'a b/c+d')
+ test 'escapes slash in generated path segment' do
+ assert_equal '/a%20b%2Fc+d', segment_path(:segment => 'a b/c+d')
end
test 'unescapes recognized path segment' do
@@ -3605,7 +3605,7 @@ def app; Routes end
assert_equal 'a b/c+d', @response.body
end
- test 'escapes generated path splat' do
+ test 'does not escape slash in generated path splat' do
assert_equal '/a%20b/c+d', splat_path(:splat => 'a b/c+d')
end
@@ -3790,6 +3790,8 @@ class TestOptimizedNamedRoutes < ActionDispatch::IntegrationTest
get '/post(/:action(/:id))' => ok, as: :posts
get '/:foo/:foo_type/bars/:id' => ok, as: :bar
get '/projects/:id.:format' => ok, as: :project
+ get '/pages/:id' => ok, as: :page
+ get '/wiki/*page' => ok, as: :wiki
end
end
@@ -3822,6 +3824,26 @@ def app; Routes end
assert_equal '/projects/1.json', Routes.url_helpers.project_path(1, :json)
assert_equal '/projects/1.json', project_path(1, :json)
end
+
+ test 'segments with question marks are escaped' do
+ assert_equal '/pages/foo%3Fbar', Routes.url_helpers.page_path('foo?bar')
+ assert_equal '/pages/foo%3Fbar', page_path('foo?bar')
+ end
+
+ test 'segments with slashes are escaped' do
+ assert_equal '/pages/foo%2Fbar', Routes.url_helpers.page_path('foo/bar')
+ assert_equal '/pages/foo%2Fbar', page_path('foo/bar')
+ end
+
+ test 'glob segments with question marks are escaped' do
+ assert_equal '/wiki/foo%3Fbar', Routes.url_helpers.wiki_path('foo?bar')
+ assert_equal '/wiki/foo%3Fbar', wiki_path('foo?bar')
+ end
+
+ test 'glob segments with slashes are not escaped' do
+ assert_equal '/wiki/foo/bar', Routes.url_helpers.wiki_path('foo/bar')
+ assert_equal '/wiki/foo/bar', wiki_path('foo/bar')
+ end
end
class TestNamedRouteUrlHelpers < ActionDispatch::IntegrationTest
View
8 actionpack/test/journey/router/utils_test.rb
@@ -5,11 +5,15 @@ module Journey
class Router
class TestUtils < ActiveSupport::TestCase
def test_path_escape
- assert_equal "a/b%20c+d", Utils.escape_path("a/b c+d")
+ assert_equal "a/b%20c+d%25", Utils.escape_path("a/b c+d%")
+ end
+
+ def test_segment_escape
+ assert_equal "a%2Fb%20c+d%25", Utils.escape_segment("a/b c+d%")
end
def test_fragment_escape
- assert_equal "a/b%20c+d?e", Utils.escape_fragment("a/b c+d?e")
+ assert_equal "a/b%20c+d%25?e", Utils.escape_fragment("a/b c+d%?e")
end
def test_uri_unescape
View
13 actionpack/test/journey/router_test.rb
@@ -367,7 +367,18 @@ def test_generate_escapes
nil, { :controller => "tasks",
:action => "a/b c+d",
}, {})
- assert_equal '/tasks/a/b%20c+d', path
+ assert_equal '/tasks/a%2Fb%20c+d', path
+ end
+
+ def test_generate_escapes_with_namespaced_controller
+ path = Path::Pattern.new '/:controller(/:action)'
+ @router.routes.add_route @app, path, {}, {}, {}
+
+ path, _ = @formatter.generate(:path_info,
+ nil, { :controller => "admin/tasks",
+ :action => "a/b c+d",
+ }, {})
+ assert_equal '/admin/tasks/a%2Fb%20c+d', path
end
def test_generate_extra_params

0 comments on commit 8a06764

Please sign in to comment.
Something went wrong with that request. Please try again.