Check `request.path_parameters` encoding at the point they're set
Check for any non-UTF8 characters in path parameters at the point they're set in `env`. Previously they were checked for when used to get a controller class, but this meant routes that went directly to a Rack app, or skipped controller instantiation for some other reason, had to defend against non-UTF8 characters themselves.
This commit is contained in:
parent
ea31bdd7c8
commit
9f38a3fb0c
|
@ -1,3 +1,13 @@
|
|||
* Check `request.path_parameters` encoding at the point they're set
|
||||
|
||||
Check for any non-UTF8 characters in path parameters at the point they're
|
||||
set in `env`. Previously they were checked for when used to get a controller
|
||||
class, but this meant routes that went directly to a Rack app, or skipped
|
||||
controller instantiation for some other reason, had to defend against
|
||||
non-UTF8 characters themselves.
|
||||
|
||||
*Grey Baker*
|
||||
|
||||
* Don't raise ActionController::UnknownHttpMethod from ActionDispatch::Static
|
||||
|
||||
Pass `Rack::Request` objects to `ActionDispatch::FileHandler` to avoid it
|
||||
|
|
|
@ -44,7 +44,14 @@ module ActionDispatch
|
|||
|
||||
def path_parameters=(parameters) #:nodoc:
|
||||
delete_header('action_dispatch.request.parameters')
|
||||
|
||||
# If any of the path parameters has an invalid encoding then
|
||||
# raise since it's likely to trigger errors further on.
|
||||
Request::Utils.check_param_encoding(parameters)
|
||||
|
||||
set_header PARAMETERS_KEY, parameters
|
||||
rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e
|
||||
raise ActionController::BadRequest.new("Invalid path parameters: #{e.message}")
|
||||
end
|
||||
|
||||
# Returns a hash with the \parameters used to form the \path of the request.
|
||||
|
|
|
@ -66,24 +66,12 @@ module ActionDispatch
|
|||
def commit_cookie_jar! # :nodoc:
|
||||
end
|
||||
|
||||
def check_path_parameters!
|
||||
# If any of the path parameters has an invalid encoding then
|
||||
# raise since it's likely to trigger errors further on.
|
||||
path_parameters.each do |key, value|
|
||||
next unless value.respond_to?(:valid_encoding?)
|
||||
unless value.valid_encoding?
|
||||
raise ActionController::BadRequest, "Invalid parameter encoding: #{key} => #{value.inspect}"
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
PASS_NOT_FOUND = Class.new { # :nodoc:
|
||||
def self.action(_); self; end
|
||||
def self.call(_); [404, {'X-Cascade' => 'pass'}, []]; end
|
||||
}
|
||||
|
||||
def controller_class
|
||||
check_path_parameters!
|
||||
params = path_parameters
|
||||
|
||||
if params.key?(:controller)
|
||||
|
|
|
@ -22,7 +22,6 @@ module ActionDispatch
|
|||
end
|
||||
|
||||
def serve(req)
|
||||
req.check_path_parameters!
|
||||
uri = URI.parse(path(req.path_parameters, req))
|
||||
|
||||
unless uri.host
|
||||
|
|
|
@ -1018,17 +1018,13 @@ class RequestParameters < BaseRequestTest
|
|||
end
|
||||
|
||||
test "path parameters with invalid UTF8 encoding" do
|
||||
request = stub_request(
|
||||
"action_dispatch.request.path_parameters" => { foo: "\xBE" }
|
||||
)
|
||||
request = stub_request
|
||||
|
||||
err = assert_raises(ActionController::BadRequest) do
|
||||
request.check_path_parameters!
|
||||
request.path_parameters = { foo: "\xBE" }
|
||||
end
|
||||
|
||||
assert_match "Invalid parameter encoding", err.message
|
||||
assert_match "foo", err.message
|
||||
assert_match "\\xBE", err.message
|
||||
assert_equal "Invalid path parameters: Non UTF-8 value: \xBE", err.message
|
||||
end
|
||||
|
||||
test "parameters not accessible after rack parse error of invalid UTF8 character" do
|
||||
|
|
|
@ -4331,15 +4331,16 @@ class TestInvalidUrls < ActionDispatch::IntegrationTest
|
|||
|
||||
test "invalid UTF-8 encoding returns a 400 Bad Request" do
|
||||
with_routing do |set|
|
||||
ActiveSupport::Deprecation.silence do
|
||||
set.draw do
|
||||
get "/bar/:id", :to => redirect("/foo/show/%{id}")
|
||||
get "/foo/show(/:id)", :to => "test_invalid_urls/foo#show"
|
||||
set.draw do
|
||||
get "/bar/:id", :to => redirect("/foo/show/%{id}")
|
||||
get "/foo/show(/:id)", :to => "test_invalid_urls/foo#show"
|
||||
|
||||
ActiveSupport::Deprecation.silence do
|
||||
get "/foo(/:action(/:id))", :controller => "test_invalid_urls/foo"
|
||||
get "/:controller(/:action(/:id))"
|
||||
end
|
||||
ok = lambda { |env| [200, { 'Content-Type' => 'text/plain' }, []] }
|
||||
get '/foobar/:id', to: ok
|
||||
|
||||
ActiveSupport::Deprecation.silence do
|
||||
get "/foo(/:action(/:id))", :controller => "test_invalid_urls/foo"
|
||||
get "/:controller(/:action(/:id))"
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -4354,6 +4355,9 @@ class TestInvalidUrls < ActionDispatch::IntegrationTest
|
|||
|
||||
get "/bar/%E2%EF%BF%BD%A6"
|
||||
assert_response :bad_request
|
||||
|
||||
get "/foobar/%E2%EF%BF%BD%A6"
|
||||
assert_response :bad_request
|
||||
end
|
||||
end
|
||||
end
|
||||
|
@ -4774,7 +4778,9 @@ class TestPathParameters < ActionDispatch::IntegrationTest
|
|||
end
|
||||
end
|
||||
|
||||
get ':controller(/:action/(:id))'
|
||||
ActiveSupport::Deprecation.silence do
|
||||
get ':controller(/:action/(:id))'
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
|
|
Loading…
Reference in New Issue