Try to escape each part of a path redirect route correctly
A path redirect may contain any and all parts of a url which have different escaping rules for each part. This commit tries to escape each part correctly by splitting the string into three chunks - path (which may also include a host), query and fragment; then it applies the correct escape pattern to each part. Whilst using `URI.parse` would be better, unfortunately the possible presence of %{name} parameters in the path redirect string prevents us from using it so we have to use a regular expression instead. Fixes #13110.
This commit is contained in:
parent
8ab24bec2e
commit
d2e1caaab9
@ -1,3 +1,9 @@
|
||||
* Try to escape each part of a url correctly when using a redirect route.
|
||||
|
||||
Fixes #13110.
|
||||
|
||||
*Andrew White*
|
||||
|
||||
* Better error message for typos in assert_response argument.
|
||||
|
||||
When the response type argument to `assert_response` is not a known
|
||||
|
@ -57,11 +57,33 @@ def inspect
|
||||
def relative_path?(path)
|
||||
path && !path.empty? && path[0] != '/'
|
||||
end
|
||||
|
||||
def escape(params)
|
||||
Hash[params.map{ |k,v| [k, Rack::Utils.escape(v)] }]
|
||||
end
|
||||
|
||||
def escape_fragment(params)
|
||||
Hash[params.map{ |k,v| [k, Journey::Router::Utils.escape_fragment(v)] }]
|
||||
end
|
||||
|
||||
def escape_path(params)
|
||||
Hash[params.map{ |k,v| [k, Journey::Router::Utils.escape_path(v)] }]
|
||||
end
|
||||
end
|
||||
|
||||
class PathRedirect < Redirect
|
||||
URL_PARTS = /\A([^?]+)?(\?[^#]+)?(#.+)?\z/
|
||||
|
||||
def path(params, request)
|
||||
(params.empty? || !block.match(/%\{\w*\}/)) ? block : (block % escape(params))
|
||||
if block.match(URL_PARTS)
|
||||
path = interpolation_required?($1, params) ? $1 % escape_path(params) : $1
|
||||
query = interpolation_required?($2, params) ? $2 % escape(params) : $2
|
||||
fragment = interpolation_required?($3, params) ? $3 % escape_fragment(params) : $3
|
||||
|
||||
"#{path}#{query}#{fragment}"
|
||||
else
|
||||
interpolation_required?(block, params) ? block % escape(params) : block
|
||||
end
|
||||
end
|
||||
|
||||
def inspect
|
||||
@ -69,8 +91,8 @@ def inspect
|
||||
end
|
||||
|
||||
private
|
||||
def escape(params)
|
||||
Hash[params.map{ |k,v| [k, Rack::Utils.escape(v)] }]
|
||||
def interpolation_required?(string, params)
|
||||
!params.empty? && string && string.match(/%\{\w*\}/)
|
||||
end
|
||||
end
|
||||
|
||||
@ -101,11 +123,6 @@ def path(params, request)
|
||||
def inspect
|
||||
"redirect(#{status}, #{options.map{ |k,v| "#{k}: #{v}" }.join(', ')})"
|
||||
end
|
||||
|
||||
private
|
||||
def escape_path(params)
|
||||
Hash[params.map{ |k,v| [k, URI.parser.escape(v)] }]
|
||||
end
|
||||
end
|
||||
|
||||
module Redirection
|
||||
|
@ -3235,7 +3235,9 @@ class TestRedirectInterpolation < ActionDispatch::IntegrationTest
|
||||
|
||||
get "/foo/:id" => redirect("/foo/bar/%{id}")
|
||||
get "/bar/:id" => redirect(:path => "/foo/bar/%{id}")
|
||||
get "/baz/:id" => redirect("/baz?id=%{id}&foo=?&bar=1#id-%{id}")
|
||||
get "/foo/bar/:id" => ok
|
||||
get "/baz" => ok
|
||||
end
|
||||
end
|
||||
|
||||
@ -3251,6 +3253,14 @@ def app; Routes end
|
||||
verify_redirect "http://www.example.com/foo/bar/1%3E"
|
||||
end
|
||||
|
||||
test "path redirect escapes interpolated parameters correctly" do
|
||||
get "/foo/1%201"
|
||||
verify_redirect "http://www.example.com/foo/bar/1%201"
|
||||
|
||||
get "/baz/1%201"
|
||||
verify_redirect "http://www.example.com/baz?id=1+1&foo=?&bar=1#id-1%201"
|
||||
end
|
||||
|
||||
private
|
||||
def verify_redirect(url, status=301)
|
||||
assert_equal status, @response.status
|
||||
|
Loading…
Reference in New Issue
Block a user