Utf8 params key #11795

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
Contributor

nanaya commented Aug 7, 2013

Related to #3789 which has been annoying me since forever (error 500 triggered by user error).

@edogawaconan Can you squash 2 commits into one?

Contributor

nanaya commented Aug 14, 2013

Contributor

grosser commented Sep 20, 2013

👍

@robin850 robin850 and 1 other commented on an outdated diff Sep 21, 2013

actionpack/test/dispatch/request_test.rb
@@ -571,6 +571,17 @@ def url_for(options = {})
assert_equal({}, request.parameters)
end
+ test "invalid utf8 sequence in query params key raises bad request error" do
+ mock_rack_env = { "QUERY_STRING" => "foo%81E=1", "rack.input" => "foo" }
+ request = nil
@robin850

robin850 Sep 21, 2013

Member

I think that this assignment is useless since you are defining the value just below — Also, could you add a changelog entry please ?

@nanaya

nanaya Sep 21, 2013

Contributor

done.

Owner

spastorino commented Sep 21, 2013

@edogawaconan can you rebase?. @tenderlove @josevalim do you have any objection to merge this one?.
I think it makes sense.

Owner

tenderlove commented Sep 21, 2013

Where is the exception coming from? Why not rescue in normalize_encode_params? Also, why is Rack raising an ArgumentError? Rescuing ArgumentError seems dangerous IMO.

Contributor

nanaya commented Sep 22, 2013

as mentioned in #3789, it is caused by Rack trying to regexp a query string's key without checking whether or not it is a valid utf8. I tried to fix it on Rack until I notice it likes raising ArgumentError. At least I think that's what I thought when writing this pull request.

Member

arthurnn commented Dec 20, 2013

LGTM

Contributor

greysteil commented May 18, 2014

👍 - there's a blog post on this here and details from the guys at rack here. Would be great to see this merged.

Owner

pixeltrix commented May 18, 2014

Hmm, the comment on the Rack ticket seems to be saying we should use encoding_valid? in normalize_encode_params where we force the encoding rather than rescuing ArgumentError which does make sense.

Contributor

nanaya commented May 18, 2014

His example didn't count invalid encoding at params key though, which I mentioned in rack/rack#610 (and what's being worked around in this issue).

Contributor

nanaya commented May 18, 2014

also, this error happens in super call, before normalize_encode_params is even called.

Owner

pixeltrix commented May 19, 2014

@edogawaconan hmm, parse_nested_query is sort of there for Rails so I'm not sure whether it needs to be fixed there or in Rails. I'll have to inquire further.

Contributor

teohm commented Jul 10, 2014

@edogawaconan any plan to back port this fix to Rails 3.2.x ?

Owner

pixeltrix commented Jul 10, 2014

@teohm the BadRequest exception doesn't exist in Rails 3.2 so it can't be backported

Owner

pixeltrix commented Jul 10, 2014

@edogawaconan are there any other ways that ArgumentError can be raised from within that call - my concern is that we'll mask some important error as a 4xx and it won't get reported.

Contributor

nanaya commented Jul 10, 2014

@pixeltrix I can't really tell as I haven't spent enough time checking for other error paths. Though after looking again, it really shouldn't be handled this way but unfortunately there's no response on what might be the better solution at rack/rack#640.

Contributor

teohm commented Jul 10, 2014

@pixeltrix I'm interested to help patching Rails 3.2.x, since I'm facing this issue in production as well. Do you have some insights on how we can fix this in Rails 3.2?

Contributor

nanaya commented Jul 10, 2014

@teohm I believe the monkeypatch I used "fixed" this issue: https://bitbucket.org/edogawaconan/moebooru/src/a90c305fa3d18c181e5ee368b548ee6a25b56f70/lib/core_ext/fix-utf8_params-2-query_key_parser.rb?at=3.2.1-release

(either that or one of the other fix-utf8_params monkeypatches in same directory)

Contributor

teohm commented Jul 10, 2014

@edogawaconan thanks!! I will try them out ❤️ ❤️ ❤️

hcatlin commented Jul 15, 2014

My Rails 4.1.2 server gets this error due to Chinese search engines crawling the site... it's a damn annoying production bug to get...

Contributor

grosser commented Jul 15, 2014

Maybe the simplest fix is to just make it a :bad_request instead of trying
to fix a broken request.

On Tue, Jul 15, 2014 at 5:35 AM, Hampton Catlin notifications@github.com
wrote:

My Rails 4.1.2 server gets this error due to Chinese search engines
crawling the site... it's a damn annoying production bug to get...


Reply to this email directly or view it on GitHub
#11795 (comment).

Contributor

nanaya commented Jul 15, 2014

I also realized the rack level fix (encode parameter key to binary) doesn't work for rails < 4.2 because actiondispatch's parameter utility force-encoded the keys back to utf-8, causing http parameter filter to explode during filtered key check.

Thankfully(?) the code is removed in master but the side effect is passing unicode parameter key will stop working because the keys will be passed as is as binary-encoded string. I hope no sane programmer use such kind of schema (non-ascii parameter key) though.

Contributor

nanaya commented Jul 15, 2014

this beautiful ugly hack seems to work for rails 4.1.4 (which, as per earlier warning, will break handling of non-ascii parameter key):

updated monkeypatch after this comment

Contributor

nanaya commented Jul 18, 2014

I didn't expect the case for recently closed issue #16207 (i.e. my monkeypatch doesn't work).

Contributor

nanaya commented Jul 18, 2014

FWIW, the error with form POST happens before ActionDispatch::Request handles it and thus completely not caught.

Contributor

nanaya commented Jul 18, 2014

Hopefully this eases up understanding the problem sources.

Orders:

  1. Web server
  2. Rack::MethodOverride (call to Rack::Request which does not catch TypeError)
  3. ActionDispatch::ParamsParser (call to ActionDispatch::Request which catches TypeError)

Queries:

  • Lone percent: a%=1
  • Invalid utf8: a%81A=1

Result

Request Type Invalid string location Invalid string type Handler Result
GET Query Lone percent Web server 400 (correct)
POST Query Lone percent Web server 400 (correct)
POST Query Invalid utf8 Rack::MethodOverride: params_nested_query ArgumentError
POST Form Invalid utf8 Rack::MethodOverride: params_nested_query ArgumentError
POST Form Lone percent Rack::MethodOverride: unescape ArgumentError
GET Query Invalid utf8 ActionDispatch::ParamsParser: params_nested_query ArgumentError
GET Form Invalid utf8 ActionDispatch::ParamsParser: params_nested_query ArgumentError
GET Form Lone percent ActionDispatch::ParamsParser: unescape ArgumentError
  • unescape: error when calling URI.decode_...
  • params_nested_query: error when doing =~ comparison of string with invalid encoding

Summary

  1. Lone percent in query string immediately halted by web server
  2. POST request incurs additional call to Rack::Request at Rack::MethodOverride which then parses query string (and explodes)
  3. GET request makes call to ActionDispatch::Request at ActionDispatch::ParamsParser which then parses query string (and explodes; at least this one catches TypeError, though)

Suggested fix (no)

# rack/lib/utils.rb
# line 45
def unescape(s, encoding = Encoding::UTF_8)
  decoded_string = URI.decode_www_form_component(s, encoding)
  decoded_string.valid_encoding? ? decoded_string : nil
rescue ArgumentError
  nil
end

correct fix probably involves raising correct error on unescape block (instead of returning nil) and wrapping Rack::MethodOverride with rails' own middleware, which probably look like this:

module ActionDispatch
  class MethodOverride < Rack::MethodOverride
    def call(*args)
      super
    rescue TypeError => e
      raise ActionController::BadRequest.new(:request, e)
    end
  end
end

Monkey patch™

# config/initializers/rack_params_key_unescape_fix.rb
if defined?(::Encoding)
  require 'rack/utils'

  module Rack::Utils
    def unescape(s, encoding = Encoding::UTF_8)
      decoded_string = URI.decode_www_form_component(s, encoding)
      decoded_string.valid_encoding? ? decoded_string : nil
    rescue ArgumentError
      nil
    end
    module_function :unescape
  end
end
Owner

rafaelfranca commented Jul 18, 2014

@edogawaconan yes, your current fix will not fix that issue. But we are looking for a fix for both cases.

Owner

rafaelfranca commented Jul 18, 2014

Closed by rack/rack#713

Contributor

teohm commented Jul 22, 2014

@rafaelfranca, can we bump rack version for Rails 3.2.x branch? So that this fix can benefits all Rails 3.2 apps?

Currently Rails 3.2.x requires rack (~> 1.4.5)
https://github.com/rails/rails/blob/v3.2.19/actionpack/actionpack.gemspec#L26

Owner

rafaelfranca commented Jul 22, 2014

No, sorry, Rails 3 is not supported anymore. Since using rack 1.6 require some changes on the code it will be included only on 4.2

Contributor

nanaya commented Jul 22, 2014

last I checked the change in rack doesn't fix the problem, just making it easier to catch somewhere.

Owner

rafaelfranca commented Jul 22, 2014

@edogawaconan it is catch by Rails that return 400 instead of 500. So it does fix the problem since we should not trying to fix bad user inputs.

Contributor

nanaya commented Jul 22, 2014

the error that happens in Rack::MethodOverride (during POST) is uncaught.

Owner

rafaelfranca commented Jul 22, 2014

Sorry, but could you create an application reproducing this error?

Contributor

nanaya commented Jul 22, 2014

any app (even fresh one), with curl: curl --data 'a%=' 'http://localhost:3000/'

Owner

rafaelfranca commented Jul 22, 2014

Thanks. I'll take a look ❤️

Contributor

nanaya commented Jul 22, 2014

actually, the later change ( rack/rack@975ccac ) changed the error type into ArgumentError, making it uncatchable by ActionDispatch::Request.

Owner

rafaelfranca commented Jul 22, 2014

😰. Ok. I'll fix it on Rails side.

Contributor

ignatiusreza commented Jul 23, 2014

Probably worth considering.. Reproducible in fresh rails 4.1.4 and changing consider_all_requests_local to true..

when the invalid utf8 params comes to an invalid path, rails will raise ActionController::RoutingError as expected, which then got caught by the exceptions_app (ActionDispatch::PublicExceptions)..

exceptions_app will throw another error when it run the following:

request      = ActionDispatch::Request.new(env)
content_type = request.formats.first

resulting in ActionDispatch::ShowExceptions responding with the FAILSAFE_RESPONSE

Owner

rafaelfranca commented Aug 19, 2014

Fixed on master.

Contributor

grosser commented Aug 19, 2014

❤️

Contributor

greysteil commented Aug 19, 2014

❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment