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.
This commit is contained in:
parent
a61792574d
commit
5460591f02
@ -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*
|
||||
|
||||
* Add alias `ActionDispatch::Http::UploadedFile#to_io` to
|
||||
`ActionDispatch::Http::UploadedFile#tempfile`.
|
||||
|
||||
|
@ -101,6 +101,10 @@ def required_defaults
|
||||
end
|
||||
end
|
||||
|
||||
def glob?
|
||||
!path.spec.grep(Nodes::Star).empty?
|
||||
end
|
||||
|
||||
def dispatcher?
|
||||
@dispatcher
|
||||
end
|
||||
|
@ -37,6 +37,7 @@ class UriEncoder # :nodoc:
|
||||
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)
|
||||
@ -47,6 +48,10 @@ 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
|
||||
@ -69,6 +74,10 @@ def self.escape_path(path)
|
||||
ENCODER.escape_path(path.to_s)
|
||||
end
|
||||
|
||||
def self.escape_segment(segment)
|
||||
ENCODER.escape_segment(segment.to_s)
|
||||
end
|
||||
|
||||
def self.escape_fragment(fragment)
|
||||
ENCODER.escape_fragment(fragment.to_s)
|
||||
end
|
||||
|
@ -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
|
||||
|
@ -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)
|
||||
|
@ -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
|
||||
|
@ -8,6 +8,10 @@ def test_path_escape
|
||||
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%25?e", Utils.escape_fragment("a/b c+d%?e")
|
||||
end
|
||||
|
@ -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
|
||||
|
Loading…
Reference in New Issue
Block a user