aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--actionpack/CHANGELOG.md16
-rw-r--r--actionpack/lib/action_dispatch/journey/route.rb4
-rw-r--r--actionpack/lib/action_dispatch/journey/router/utils.rb9
-rw-r--r--actionpack/lib/action_dispatch/journey/visitors.rb21
-rw-r--r--actionpack/lib/action_dispatch/routing/route_set.rb4
-rw-r--r--actionpack/test/dispatch/routing_test.rb28
-rw-r--r--actionpack/test/journey/router/utils_test.rb4
-rw-r--r--actionpack/test/journey/router_test.rb13
8 files changed, 89 insertions, 10 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 7e3a426eb2..221aaa338c 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*
+
* Add alias `ActionDispatch::Http::UploadedFile#to_io` to
`ActionDispatch::Http::UploadedFile#tempfile`.
diff --git a/actionpack/lib/action_dispatch/journey/route.rb b/actionpack/lib/action_dispatch/journey/route.rb
index c8eb0f6f2d..2b399d3ee3 100644
--- a/actionpack/lib/action_dispatch/journey/route.rb
+++ b/actionpack/lib/action_dispatch/journey/route.rb
@@ -101,6 +101,10 @@ module ActionDispatch
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 246d91da01..ac4ecb1e65 100644
--- a/actionpack/lib/action_dispatch/journey/router/utils.rb
+++ b/actionpack/lib/action_dispatch/journey/router/utils.rb
@@ -37,6 +37,7 @@ module ActionDispatch
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 @@ module ActionDispatch
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 @@ module ActionDispatch
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
diff --git a/actionpack/lib/action_dispatch/journey/visitors.rb b/actionpack/lib/action_dispatch/journey/visitors.rb
index daade5bb74..d9f634623d 100644
--- a/actionpack/lib/action_dispatch/journey/visitors.rb
+++ b/actionpack/lib/action_dispatch/journey/visitors.rb
@@ -114,19 +114,26 @@ module ActionDispatch
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 @@ module ActionDispatch
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 a03fb4cee7..1ec6fa674b 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -155,7 +155,7 @@ module ActionDispatch
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 @@ module ActionDispatch
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 f74a0ef945..b22a56bb27 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 @@ class TestUriPathEscaping < ActionDispatch::IntegrationTest
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 @@ class TestOptimizedNamedRoutes < ActionDispatch::IntegrationTest
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 8b3a4e340a..584fd56a5c 100644
--- a/actionpack/test/journey/router/utils_test.rb
+++ b/actionpack/test/journey/router/utils_test.rb
@@ -8,6 +8,10 @@ module ActionDispatch
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
diff --git a/actionpack/test/journey/router_test.rb b/actionpack/test/journey/router_test.rb
index a286f77633..e54b64e0f3 100644
--- a/actionpack/test/journey/router_test.rb
+++ b/actionpack/test/journey/router_test.rb
@@ -367,7 +367,18 @@ module ActionDispatch
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
generated by cgit v1.2.3 (git 2.25.1) at 2024-06-29 21:38:01 +0000