handle failure to upcase invalid UTF8 strings for _method values

[mclark]

👋 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]

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

[eileencodes]

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.

[eileencodes]

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

[mclark]

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?

[mclark]

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! 😄 )

[eileencodes]

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

[eileencodes]

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]

@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]

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]

@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]

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

[mclark]

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]

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.

[mclark]

@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]

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.