diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 9220d54c6fd67..7215ae1831eec 100644 --- a/actionpack/CHANGELOG.md +++ b/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. diff --git a/actionpack/lib/action_dispatch/journey/route.rb b/actionpack/lib/action_dispatch/journey/route.rb index c8eb0f6f2d550..2b399d3ee3177 100644 --- a/actionpack/lib/action_dispatch/journey/route.rb +++ b/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 diff --git a/actionpack/lib/action_dispatch/journey/router/utils.rb b/actionpack/lib/action_dispatch/journey/router/utils.rb index d1a004af50ac8..ac4ecb1e65454 100644 --- a/actionpack/lib/action_dispatch/journey/router/utils.rb +++ b/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 diff --git a/actionpack/lib/action_dispatch/journey/visitors.rb b/actionpack/lib/action_dispatch/journey/visitors.rb index daade5bb7429f..d9f634623d09c 100644 --- a/actionpack/lib/action_dispatch/journey/visitors.rb +++ b/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 diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index a03fb4cee7101..1ec6fa674b0c1 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/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) diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index f74a0ef945168..b22a56bb276b8 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/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 diff --git a/actionpack/test/journey/router/utils_test.rb b/actionpack/test/journey/router/utils_test.rb index 93348f46472a5..584fd56a5c3f0 100644 --- a/actionpack/test/journey/router/utils_test.rb +++ b/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 diff --git a/actionpack/test/journey/router_test.rb b/actionpack/test/journey/router_test.rb index a286f77633d10..e54b64e0f385b 100644 --- a/actionpack/test/journey/router_test.rb +++ b/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