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

invalid %-encoding error in application for malformed uri #337

Closed
StefanH opened this Issue Feb 1, 2012 · 59 comments

Comments

Projects
None yet
@StefanH

StefanH commented Feb 1, 2012

When you go to, for example, /foo?%9g in my (rails) app, rack raises an ArgumentError: invalid %-encoding. I would expect to get a 400 Bad request back.

The offending line is:

  URI.decode_www_form_component(s, encoding)

in rack/utils.rb line 39, wich tries to decode this invalid url component.

I'm using Nginx with passenger as a webserver. I've tried Rack versions 1.4.1 and 1.3.6.

Interestingly, when I go to /%9g (i.e. use %9g as part of the URI instead of passing it as a parameter), I immediately get a 400 Bad request from nginx. Webrick returns a 400 either way. I'm not sure if nginx is correct in ignoring the invalid URI component and passing this request off to the app, but it would be nice if rack could handle it either way.

@matiaskorhonen

This comment has been minimized.

Contributor

matiaskorhonen commented Feb 3, 2012

This same exception happens if a user attempts to upload a file which has a percentage sign in the filename.

Backtrace: https://gist.github.com/4f67425a065fd12b43b8

@vanderhoorn

This comment has been minimized.

vanderhoorn commented Feb 19, 2012

I'm having the same issues as StefanH reports.

@cmeiklejohn

This comment has been minimized.

cmeiklejohn commented Feb 26, 2012

I'm also experiencing this. Any update?

@PatrickLef

This comment has been minimized.

PatrickLef commented Feb 28, 2012

+1

@elcuervo

This comment has been minimized.

Contributor

elcuervo commented Feb 28, 2012

@k33l0r I think that's fixed at least in 1.4.1 https://github.com/rack/rack/blob/master/lib/rack/multipart/parser.rb#L128-134 (https://github.com/rack/rack/blob/master/test/spec_multipart.rb#L229-242)

@StefanH Thin and Webrick raise an error even before rack, what's the use case you are having?

@vanderhoorn

This comment has been minimized.

vanderhoorn commented Mar 9, 2012

@elcuervo StefanH wrote: "I'm using Nginx with passenger as a webserver. I've tried Rack versions 1.4.1 and 1.3.6."

@bwbuchanan

This comment has been minimized.

bwbuchanan commented Mar 15, 2012

I'm seeing this same error regularly on a site that is using unicorn behind nginx. Some bot keeps sending invalid requests that have the unescaped query string: ?iframe=true&width=80%&height=80%

I suppose I could write some middleware to intercept these requests, but it would be very nice if Rack would do the right thing and send back a 400 Bad Request instead of throwing an error.

@gtd

This comment has been minimized.

gtd commented Mar 26, 2012

Same issue as bwbuchanan. The bot actually appears to be googlebot, though obviously that could be spoofed. According to the latest information at http://stackoverflow.com/questions/9208999/whats-up-with-those-requests-having-iframe-truewidth-80height-80-query-pa/9251195#9251195 these are being generated by spidername.com somehow.

Anyway, I actually started down this road of writing a middleware, but it's tricky to test because neither thin nor webrick let these errors through. I'm not 100% sure whether I agree that failure should be silent, after all if such requests were due to my own coding error rather than a misbehaving spider I would be glad to receive the usual exception notification, but on balance 400 makes more sense.

@juniovitorino

This comment has been minimized.

juniovitorino commented Apr 17, 2012

For me a simple acess to 127.0.0.1:9292 raises the error.

screenshot

@piemuffin

This comment has been minimized.

piemuffin commented May 9, 2012

Apparently these wrongly encoded URLs come from "ECMAScript"
http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf
Search the document for "%u"

This can easily be verified by opening a JavaScript console in Safari or Chrome and entering
escape('あ')
The result will be "%u3042"

Someone is probably crafting URLs in JavaScript, and this is the result.

Please fix it, either way, by returning 400 Bad Request or by unescaping it.

@psychocandy

This comment has been minimized.

psychocandy commented Jul 17, 2012

For now I'm using the following monkey patch https://gist.github.com/3130349 (Taken from http://stackoverflow.com/a/11162317/1075006 and changed to fit ruby 1.9.x). It does smell a bit odd to me, I'm thinking of attempting to create an apache rule for these URLs.
Any idea if this issue will be fixed in Rack at all?

@montebrown

This comment has been minimized.

montebrown commented Aug 7, 2012

We're also seeing this issue - whenever we have a percentage sign in a cookie value.

https://gist.github.com/3130349 resolves the exception.

@houen

This comment has been minimized.

houen commented Sep 7, 2012

+1

@goro

This comment has been minimized.

goro commented Sep 9, 2012

It looks like the problem is in URI, not Rack, in URI.decode_www_form_component(s, encoding):
http://www.ruby-doc.org/stdlib-1.9.3/libdoc/uri/rdoc/URI.html#method-c-decode_www_form_component

This line is raising an error for any % that's not like %\h\h, but should probably just be escaping them:
raise ArgumentError, "invalid %-encoding (#{str})" unless /\A[^%](?:%\h\h[^%])*\z/ =~ str

@raggi

This comment has been minimized.

@etozzato

This comment has been minimized.

etozzato commented Nov 7, 2012

Hi, is it reasonable to take a similar approach as https://github.com/rack/rack/blob/master/lib/rack/multipart/parser.rb#L130 and apply it to https://github.com/rack/rack/blob/master/lib/rack/request.rb#L379 ?

The hot-fix seems to be working, but I have no idea what can or worms I may be opening..

def parse_query(qs)
  if qs.scan(/%.?.?/).all? { |s| s =~ /%[0-9a-fA-F]{2}/ }
    Utils.parse_nested_query(qs)
  else
    Utils.parse_nested_query(qs.gsub(/%(?![0-9a-fA-F]{2})/, '%25'))
  end
end
@etozzato

This comment has been minimized.

etozzato commented Nov 7, 2012

I already see it as a performance issue :)

👎

@jarl-dk

This comment has been minimized.

jarl-dk commented Nov 26, 2012

I think the web server/app container (nginx/apache or passenger) should handle this:

https://groups.google.com/d/msg/phusion-passenger/Wy06BoVZIX8/kDUyyi83ZksJ

The request should never reach Rack IMHO.

@raggi

This comment has been minimized.

Member

raggi commented Dec 29, 2012

Puma, Thin, Webrick, Unicorn all return a 400 Bad Request before this hits Rack.

The SPEC says that PATH_INFO may be percent encoded, but it is expected to be valid. This may not be 100% clear in the spec at this time.

If an invalid PATH_INFO hits rack, then 500 is the correct response, as this is a server error, that has been caused by an invalid request.

@raggi raggi closed this Dec 29, 2012

@tiegz

This comment has been minimized.

tiegz commented Jan 7, 2013

@raggi just wanted to mention that I can reproduce this locally on Thin/Unicorn/Pow with a fresh Rails 3.2.10 app. Passing "%9g" as querystring returns a Bad Request, but as a form body it does pass it on to Rack where it raises this exception.

@maletor

This comment has been minimized.

maletor commented Jan 16, 2013

The behavior should be consistent, don't you think @tiegz?

@tiegz

This comment has been minimized.

tiegz commented Jan 16, 2013

@raggi

This comment has been minimized.

Member

raggi commented Jan 17, 2013

At the rack level, it's up to apps to handle how they deal with corrupt params. Rack doesn't itself enforce responses in any way. The app is in full control of how it deals with internal errors. If we didn't follow this policy, then you'd either need an API from rack to customize response pages, or you wouldn't be able to.

Put another way, Rails should handle the exception raised from invalid params, and execute the appropriate error handlers, or the default ones. I'm not sure (as I haven't looked), but I'm guessing this actually occurs before it hits the router/controller layer, due to the fact that they use a parameter middleware. In this case, my recommendation would be that the middleware catch the error, insert a value in env to indicate that it happened, and then the controller dispatcher will dispatch the appropriate error handler in response to the presence of that value in env.

/cc @spastorino @tenderlove

@matt-glover

This comment has been minimized.

matt-glover commented Jan 17, 2013

For more of the runaround on corrupt parameter issues see rails/rails#2622.

@jarl-dk

This comment has been minimized.

jarl-dk commented Jan 17, 2013

Looking at the application stack.

  • Http server (e.g. apache/nginx/webrick/puma/thin)
  • Ruby adapter/bridge (e.g. passenger)
  • Rack
  • Rails
  • Rails application

Since this is a not even a valid http request it should be handled by the http server as 400 bad request. If the http server does not do that, then yes that is an "Internal Server Error" (of the http server), because the http server leaks this request down the stack, which may eventually result in http response 500 (or something else). However, IMHO the problem relies in the http server, not rack or rails.

@bf4

This comment has been minimized.

bf4 commented Jul 10, 2014

fwiw, here's a middleware I wrote that I am using https://gist.github.com/bf4/d26259acfa29f3b9882b#file-exception_app-rb

fofr added a commit to alphagov/support that referenced this issue May 22, 2015

Prevent invalid query strings from erroring
For certain referrer URLs  eg `Taxed%20to%2`,
`Rack::Utils.parse_query(uri.query)` throws an ArgumentError: `invalid
%-encoding`.

Because this is mostly an enhancement to the table, rather than attempt
to decode the query correctly, return a nil search term.

A discussion on the nuances of this issue can be found here:
rack/rack#337
@md5

This comment has been minimized.

md5 commented Mar 4, 2016

Would there be any chance of getting a patch like the following into Rack?

diff --git a/lib/rack/utils.rb b/lib/rack/utils.rb
index d541608..e0a528c 100644
--- a/lib/rack/utils.rb
+++ b/lib/rack/utils.rb
@@ -10,6 +10,8 @@ module Rack
   # Rack::Utils contains a grab-bag of useful methods for writing web
   # applications adopted from all kinds of Ruby libraries.

+  class InvalidPercentEncodingError < ArgumentError; end
+
   module Utils
     ParameterTypeError = QueryParser::ParameterTypeError
     InvalidParameterError = QueryParser::InvalidParameterError
@@ -48,7 +50,11 @@ module Rack
     # Unescapes a URI escaped string with +encoding+. +encoding+ will be the
     # target encoding of the string returned, and it defaults to UTF-8
     def unescape(s, encoding = Encoding::UTF_8)
-      URI.decode_www_form_component(s, encoding)
+      begin
+        URI.decode_www_form_component(s, encoding)
+      rescue ArgumentError
+        raise InvalidPercentEncodingError, $!.message
+      end
     end
     module_function :unescape

That would make possible to do a rescue_from "Rack::InvalidPercentEncodingError" in Rails cleanly instead of having to poke around to see if it's the right kind of ArgumentError.

@raggi

This comment has been minimized.

Member

raggi commented Mar 6, 2016

Why not propose this to MRI?

@md5

This comment has been minimized.

md5 commented Mar 6, 2016

@raggi I did consider that.

All the custom errors in URI::* are child classes of URI::Error which itself descends from StandardError. If I were to propose changing decode_www_form_component, I would either have to introduce an error class in that module that doesn't descend from URI::Error and instead descends from ArgumentError or possibly break everyone's code that was used to rescuing ArgumentError.

@AlexWayfer

This comment has been minimized.

AlexWayfer commented Jan 18, 2018

Puma, Thin, Webrick, Unicorn all return a 400 Bad Request before this hits Rack.

No.

This is a rack / rails bug, not puma.

puma/puma#500 (comment)

😐

@array42

This comment has been minimized.

array42 commented Apr 9, 2018

Post XML Body with Comment also breaks for Rails 5.

@raggi

This comment has been minimized.

Member

raggi commented Apr 9, 2018

@AlexWayfer

This comment has been minimized.

AlexWayfer commented Apr 9, 2018

but frameworks seem to have taken this on just fine

Puma still no. We're getting low-level errors.

In production:

An unhandled lowlevel error occurred. The application logs may have details.

In development:

Puma caught this error: invalid %-encoding (%9g) (Rack::QueryParser::InvalidParameterError)
/home/alex/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/rack-2.0.4/lib/rack/query_parser.rb:72:in `rescue in parse_nested_query'
/home/alex/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/rack-2.0.4/lib/rack/query_parser.rb:60:in `parse_nested_query'
/home/alex/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/rack-2.0.4/lib/rack/request.rb:468:in `parse_query'
/home/alex/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/rack-2.0.4/lib/rack/request.rb:319:in `GET'
/home/alex/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/rack-2.0.4/lib/rack/request.rb:358:in `params'
/home/alex/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/rack-2.0.4/lib/rack/request.rb:20:in `params'
@raggi

This comment has been minimized.

Member

raggi commented Apr 9, 2018

@AlexWayfer

This comment has been minimized.

AlexWayfer commented Apr 9, 2018

You will want to validate request parameters in your application or catch this error from rack request.

But how? I even can't take Rack::Request#params for validating. Or where should I catch this error?

@bf4

This comment has been minimized.

bf4 commented Apr 9, 2018

Easy middleware from this thread years ago #337 (comment)

@dentarg

This comment has been minimized.

dentarg commented Apr 10, 2018

The link in the above comment didn't work for me (https://github.com/rack/rack/issues/337#comment-48555831), I think @bf4 meant to link to https://github.com/rack/rack/issues/337#issuecomment-48555831

So here it is: #337 (comment)

AlexWayfer added a commit to AlexWayfer/flame that referenced this issue Apr 10, 2018

@AlexWayfer

This comment has been minimized.

AlexWayfer commented Apr 10, 2018

@bf4, @dentarg: thank you.

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