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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle failure to upcase invalid UTF8 strings for _method values #1249

Merged

Conversation

mclark
Copy link

@mclark mclark commented Apr 12, 2018

馃憢 Hi folks, I recently encountered a case where a client was submitting invalid UTF8 characters in the "_method" parameter which was subsequently causing method.to_s.upcase to raise an ArgumentError. This pull request proposes instead logging the failure to "rack.errors" in the environment and just not override REQUEST_METHOD with anything when this occurs.

I originally planned on raising Rack::Utils::InvalidParameterError but when I saw method_override_param uses "rack.errors" I switched to that:

https://github.com/mclark/rack/blob/a795740a6e071333ecc7b498fdeed24d58750da4/lib/rack/methodoverride.rb#L42-L48

def method_override_param(req)
  req.POST[METHOD_OVERRIDE_PARAM_KEY]
rescue Utils::InvalidParameterError, Utils::ParameterTypeError
  req.env["rack.errors"].puts "Invalid or incomplete POST params"
rescue EOFError
  req.env["rack.errors"].puts "Bad request content body"
end

I'm happy to go either way of course, but if it raises we should probably also update the docs for InvalidParameterError to be a little less specific to parse_nested_query.

I can open a PR on 2.0 (or master, whatever is preferred) as well if this change is desired, I was just thinking I'd let the discussion happen here first (based on 1.6-stable) if that's cool with everyone. I guess I could have checked the process on that one first 馃槃

@mclark
Copy link
Author

mclark commented Apr 13, 2018

cc @eileencodes I'm happy to do whatever to move this along, I don't want to have it be forgotten 馃檪

Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what is up with the CircleCI build as it's pretty new but the Travis build seems affected by your change.

app.call env

errors.rewind
errors.read.should =~ /Invalid string for method/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come we're using a match here instead of should.equal? Can it be something else other than "invalid string for method"? I think this is also making the Ruby 1.8.7 build cranky https://travis-ci.org/rack/rack/jobs/365845664#L616

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was copying the existing style from a little further down the test:
https://github.com/mclark/rack/blob/a795740a6e071333ecc7b498fdeed24d58750da4/test/spec_methodoverride.rb#L100

I can update both of them if you like?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the 1.8.7 build is probably failing because I don't think that version of ruby even supports UTF-8, so the error can't be thrown. Does rack have a standard pattern for skipping tests based on the ruby version? I suspect that might be the best solution in that case, but I'm far from an encoding expert (nor do I desire to become one! 馃槃 )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example https://github.com/rack/rack/blob/1-6-stable/test/spec_chunked.rb#L45

But I prefer wrapping the entire test in the conditional rather than the conditional after the end like:

if RUBY_VERSION >= "1.9"
 test blah blah
end

@mclark mclark force-pushed the handle-invalid-method-parameters branch 2 times, most recently from 38e2efd to 1c945e0 Compare April 13, 2018 20:25
@eileencodes
Copy link
Member

cc/ @tenderlove can you take a look at this when you get a chance? This was an issue we hit where someone was sending an invalid _method param like \xBF.

In older versions of Ruby this raises an invalid utf-8 error (which we were rescuing in our app), but in newer versions it raises a "input string invalid" (which we are raising).

I suggested we fix this upstream in Rack and @mclark took on that work. I want to double check if you think this is the right place to fix the issue or if there's somewhere in Rack I didn't think of that's more appropriate.

@tenderlove
Copy link
Member

@eileencodes This looks like the right place. I'm concerned about encoding for the "mock request". Post data will come in over the wire with binary encoding, so I feel like this line should have a .b at the end.

Also, looks like this PR is targeting 1-6-stable. Is this already fixed on master? If not, we should target master and backport.

@eileencodes
Copy link
Member

Yup I was planning on porting to master / 2.0 once we had everything settled in our branch.

One thing I'm not sure about is we're at 1.6.9 so I guess we need to make a 1-7-stable branch to do a new release or do we go to 1.6.9.1 or something like that?

@tenderlove
Copy link
Member

@eileencodes this is a bugfix, shouldn't we just release 1.6.10? Regardless, I'm OK with this fix as long as we make sure it lands on master too. 馃榾

@eileencodes
Copy link
Member

Yea I forgot that after .9 is not .1 but .10. 馃槼

@mclark mclark force-pushed the handle-invalid-method-parameters branch from 1c945e0 to 23d55d0 Compare April 15, 2018 15:22
@mclark
Copy link
Author

mclark commented Apr 15, 2018

so I feel like this line should have a .b at the end.

馃挴 agree. I've fixed that and force pushed. Thanks for taking a look @tenderlove.

@eileencodes does this need PRs for master and 2.0-stable? I don't mind cherry picking and opening PRs if that's the case. By the look of things nothing has changed in the affected files (other than the addition of frozen string literals 馃帀) so it should be conflict-free. Or you can cherry-pick directly onto those branches I suppose, if you're comfortable with that.

@eileencodes
Copy link
Member

If you can backport to both in new PR's that'd be awesome - otherwise I'll do it after merging this one. Some versions of Ruby don't support .b so you can use if "<3".respond_to? :b in your conditional instead of checking the Ruby version. I think that will cover all the ones that this won't be supported for.

Some strings cannot be upper cased and raise an ArgumentError instead.
Let's capture this error and log its occurence to the environment via
rack.errors
@mclark mclark force-pushed the handle-invalid-method-parameters branch from 23d55d0 to b27dd86 Compare April 16, 2018 17:36
@mclark
Copy link
Author

mclark commented Apr 16, 2018

@eileencodes, @tenderlove I opted to change it to .force_encoding("ASCII-8BIT") instead of .b so that the 1.9x tests would still run but our encoding would be the expected "8bit ascii" or raw. LMK if this makes sense or not, from my research it seemed to be the best path to emulating .b in 1.9x.

@eileencodes eileencodes merged commit 2293c6a into rack:1-6-stable Apr 23, 2018
eileencodes added a commit to eileencodes/rack that referenced this pull request Apr 23, 2018
鈥ers

handle failure to upcase invalid UTF8 strings for `_method` values
eileencodes added a commit to eileencodes/rack that referenced this pull request Apr 23, 2018
鈥ers

handle failure to upcase invalid UTF8 strings for `_method` values
eileencodes added a commit to eileencodes/rack that referenced this pull request Apr 23, 2018
鈥ers

handle failure to upcase invalid UTF8 strings for `_method` values
eileencodes added a commit that referenced this pull request Apr 23, 2018
Merge pull request #1249 from mclark/handle-invalid-method-parameters
eileencodes added a commit to eileencodes/rack that referenced this pull request Apr 23, 2018
鈥ers

handle failure to upcase invalid UTF8 strings for `_method` values
eileencodes added a commit to eileencodes/rack that referenced this pull request Apr 23, 2018
鈥ers

handle failure to upcase invalid UTF8 strings for `_method` values
eileencodes added a commit that referenced this pull request Apr 23, 2018
鈥table

Merge pull request #1249 from mclark/handle-invalid-method-parameters
@eileencodes
Copy link
Member

Thanks for working on this @mclark!! I merged and forwardported this to master and 2-0-stable and then released 2.0.5 and 1.6.10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants