Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scrub the invalid paramter value before using it in the error #29793

Merged

Conversation

@arthurnn
Copy link
Member

@arthurnn arthurnn commented Jul 14, 2017

You should be able to safely use the String error message. So when
finding the parameter has an invalid encoding we need to remove the
invalid bytes before using it in the error. Otherwise, the caller might
get another Encoding error if they use the message.

I found this issue because due the way we parallelize CI runs, we end-up having to reply some error messages from one process to another, which we end-up having to do some string manipulation with error messages. However, in a broken test that was failing because of this error, we would get another error on top saying the Encoding of the String was invalid.

@arthurnn arthurnn requested review from tenderlove and eileencodes Jul 14, 2017
actionpack/lib/action_dispatch/request/utils.rb Outdated
@@ -32,7 +32,7 @@ def self.check_param_encoding(params)
unless params.valid_encoding?
# Raise Rack::Utils::InvalidParameterError for consistency with Rack.
# ActionDispatch::Request#GET will re-raise as a BadRequest error.
raise Rack::Utils::InvalidParameterError, "Non UTF-8 value: #{params}"
raise Rack::Utils::InvalidParameterError, "Enconding is invalid for paramter value: #{params.scrub}"

This comment has been minimized.

@arthurnn

arthurnn Jul 14, 2017
Author Member

I also changed the error message.
Because It doesn't mean that the parameter having an invalid encoding is the same as not been UTF8

This comment has been minimized.

@kaspth

kaspth Jul 15, 2017
Member

👋 @arthurnn! There's some typos in the message. How about: "Invalid encoding for parameter: #{params.scrub}" 😊

@kaspth
kaspth approved these changes Jul 15, 2017
Copy link
Member

@kaspth kaspth left a comment

2 things, otherwise 👍

actionpack/lib/action_dispatch/request/utils.rb Outdated
@@ -32,7 +32,7 @@ def self.check_param_encoding(params)
unless params.valid_encoding?
# Raise Rack::Utils::InvalidParameterError for consistency with Rack.
# ActionDispatch::Request#GET will re-raise as a BadRequest error.
raise Rack::Utils::InvalidParameterError, "Non UTF-8 value: #{params}"
raise Rack::Utils::InvalidParameterError, "Enconding is invalid for paramter value: #{params.scrub}"

This comment has been minimized.

@kaspth

kaspth Jul 15, 2017
Member

👋 @arthurnn! There's some typos in the message. How about: "Invalid encoding for parameter: #{params.scrub}" 😊

actionpack/test/dispatch/request_test.rb Outdated
@@ -1024,7 +1024,8 @@ class RequestParameters < BaseRequestTest
request.path_parameters = { foo: "\xBE" }
end

assert_equal "Invalid path parameters: Non UTF-8 value: \xBE", err.message
assert err.message.valid_encoding?

This comment has been minimized.

@kaspth

kaspth Jul 15, 2017
Member

assert_predicate?

You should be able to safely use the String error message. So when
finding the paramter has an invalid encoding we need to remove the
invalid bytes before using it in the error. Otherwise the caller might
get another Encoding error if they use the message.
@arthurnn arthurnn force-pushed the arthurnn:arthurnn/param_encoding_error_msg branch to 2513a41 Jul 17, 2017
@arthurnn
Copy link
Member Author

@arthurnn arthurnn commented Jul 17, 2017

Thanks for your review and suggestions @kaspth. I updated the PR accordingly.

Thanks

@rafaelfranca rafaelfranca merged commit 5a8b430 into rails:master Jul 17, 2017
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
rafaelfranca added a commit that referenced this pull request Jul 17, 2017
…_msg

Scrub the invalid paramter value before using it in the error
@arthurnn arthurnn deleted the arthurnn:arthurnn/param_encoding_error_msg branch Jul 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants