Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Drop invalid cookies #272

Closed
wants to merge 3 commits into from

7 participants

@infertux

Here's a quite common scenario that throws an exception in Rack then crashes Rails:

  1. Set up a Google Analytics campaign containing the % character, e.g. Example (Not 100% Escaped).

  2. FeedBurner doesn't escape correctly the % character (it should replace it with %25) so we end up with an URL like:
    https://example.com/?utm_source=feedburner&utm_medium=twitter&utm_campaign=Feed%3A+Example+%28Not+100%+Escaped%29

  3. Whenever someone clicks this URL, it sets the GA's __utmz cookie to ...utmcsr=feedburner|utmccn=Feed:%20Example%20(Not%20100%%20Escaped)|utmcmd=twitter.

  4. This incorrectly escaped poison cookie is passed to Rack through Rails.

  5. Rack can't decode it and raises an exception which is not caught by Rails then crashes the whole application rendering a nice HTTP 500 :/.

So we can patch either Rails or Rack. I don't see any point in throwing an exception in this case since the upper application (e.g. Rails) won't be able to use this cookie value anyway. So this patch simply drops incorrectly escaped cookie pairs (see added specs in test/spec_utils.rb).

Maybe a better compromise would be to use Rack's logger and display some kind of warning when Rack is unable to decode.

Related discussions: #225 & rails/rails#2622.

infertux added some commits
@infertux infertux Ignore incorrectly formatted cookies.
This will prevent the application software (like Rails) to crash when given
unescaped cookie value (e.g. "100%" instead of "100%25").
078a218
@infertux infertux Use another test case to assert error raising since the old one shoul…
…d not raise anymore.
df5d554
@richmeyers richmeyers commented on the diff
lib/rack/utils.rb
@@ -56,7 +56,11 @@ module Rack
params = {}
(qs || '').split(d ? /[#{d}] */n : DEFAULT_SEP).each do |p|
- k, v = p.split('=', 2).map { |x| unescape(x) }
+ begin
+ k, v = p.split('=', 2).map { |x| unescape(x) }
+ rescue

Should at a minimum reraise Interrupt and SystemExit here, and ideally only rescue whatever exception(s) unescape raises when input is invalid.

Good point, b443d22 fixes that.

@raggi Owner
raggi added a note

Thank you for improving this.

For the record, Interrupt and SystemExit are not children of StandardError and are not caught by argument-less rescue. That particular problem only exists for the common rescue Exception anti-pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@raggi raggi commented on the diff
test/spec_utils.rb
@@ -105,6 +105,20 @@ describe Rack::Utils do
Rack::Utils.parse_query("foo%3Dbaz=bar").should.equal "foo=baz" => "bar"
end
+ should "raise ArgumentError, 'invalid %-encoding (...)'" do
+ lambda {
+ URI.decode_www_form_component "%"
+ }.should.raise(ArgumentError).
+ message.should.match /^invalid %-encoding \(.*\)$/
+ end
@raggi Owner
raggi added a note

Thank you for putting this check in place. I agree with this approach in general.

Either way, I'm feeling a little uneasy about coupling this code to the content of an error message from the stdlib.

@tenderlove - dude, this really is coming back to that stdlib discussion again - I'd rather we didn't risk old versions not implementing this feature because the stdlib changes in future. Do you have any good suggestions on how to deal with this?

@dkubb
dkubb added a note

I was going to mention the coupling above. It seems like it could be a bit brittle.

Is there a way (in future releases) where instead of raising an ArgumentError, the system raises a subclass of ArgumentError for this specific case? Code that expects an kind-of ArgumentError will still work, and the Rack code can start to match on the subclass in addition to ArgumentError as it does above. At some point in the future Rack can remove the coupling to the old approach.

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

I'm not sure I agree with this. People could depend on the fact that improperly formatted query strings raise an exception. Imagine a user tries to parse the incorrectly formatted cookie that @infertux is presenting. They may not know that the cookie is incorrectly formatted, yet cookie parsing succeeds. Now they have the parsed cookie, but some value is missing. The user is now left asking questions like:

  • Where did the value go?
  • Why wasn't I informed that the cookie was malformed?
  • How am I able to detect malformed cookies?
  • I want to reject clients that send malformed cookies, but how?

I don't think these are decisions we should be making for rack users. I think it would make more sense to provide the method with a block that gets called on error. The default block can raise an exception in order to maintain backwards compatibility, but users can provide a block if they wish to recover.

@richmeyers

The method that parses cookies should raise exceptions on bogus cookie values, or any other problems. The code in rack that drives parsing of headers and sets up the environment for the application should handle these exceptions and do something sensible in those cases. Ignoring malformed cookies would be an improvement over refusing to process the request altogether if you cannot communicate to the application that some input cookies were dropped.

The key part is that application should be able to process the request even if parts of it are not usable. Since rack cannot know which less-than-100%-correct requests the application will find perfectly acceptable, and which of those are important enough to need to be processed, rack should let the application process all requests.

@tenderlove
Owner

@richmeyers yes, I agree. One problem is how to communicate these failures back to the application. Normally, I think exceptions would be a fine way to do this, but rack provides no way for an application to reprocess the current request. I know this might sound extreme, but if the handling of malformed headers is an application concern, then maybe it isn't rack's responsibility to be parsing these things in the first place. Maybe header parsing should happen in a middleware that people can remove, or it's done lazily via accessors on an object (that way people can get access to the raw headers without blowing up the request).

Back to the pull request at hand, it definitely feels wrong swallowing problems and I'm -1 on this request.

@infertux

To be honest, it was somehow a quick hot fix and I'm not a big fan of this solution either because of the coupling with stdlib string literal and especially because it doesn't communicate to the application that some cookies were dropped. After all, it's probably not Rack's job to deal with malformed cookies.

As @dkubb suggested, I guess the best move here could be to raise a subclass of ArgumentError to make the application able to cope with that specific parsing error.

Anyway, I'm closing this pull request as it's definitively not the best way to fix the issue.

Apart from that, +1 for a way to access raw headers although it sounds more like a new feature not related to the aforementioned issue.

@infertux infertux closed this
@morenocarullo

I was working on something that is strongly related to this.

In particular in RFC 2109 http://www.ietf.org/rfc/rfc2109.txt I noticed that the contents of the cookie should not be encoded/decoded:

     The VALUE is opaque to the user agent and may be anything the
      origin server chooses to send, possibly in a server-selected
      printable ASCII encoding.  "Opaque" implies that the content is of
      interest and relevance only to the origin server.  The content
      may, in fact, be readable by anyone that examines the Set-Cookie
      header.

in other words, we can't simply use the same method for cookies and query string.

I noticed the thing by comparing the way Rack and ASP.NET encodes/decodes cookies coming from an HTTP request.

Is it correct or I am missing something?

@eirc

Here's a hotfix for rails: https://gist.github.com/2049542

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 18, 2011
  1. @infertux

    Ignore incorrectly formatted cookies.

    infertux authored
    This will prevent the application software (like Rails) to crash when given
    unescaped cookie value (e.g. "100%" instead of "100%25").
  2. @infertux
Commits on Nov 22, 2011
  1. @infertux
This page is out of date. Refresh to see the latest.
View
2  lib/rack/request.rb
@@ -261,8 +261,6 @@ def cookies
Utils.parse_query(string, ';,').each { |k,v| hash[k] = Array === v ? v.first : v }
@env["rack.request.cookie_string"] = string
hash
- rescue => error
- raise error.class, "cannot parse Cookie header: #{error.message}"
end
def xhr?
View
9 lib/rack/utils.rb
@@ -56,7 +56,14 @@ def parse_query(qs, d = nil)
params = {}
(qs || '').split(d ? /[#{d}] */n : DEFAULT_SEP).each do |p|
- k, v = p.split('=', 2).map { |x| unescape(x) }
+ begin
+ k, v = p.split('=', 2).map { |x| unescape(x) }
+ rescue ArgumentError => e

Should at a minimum reraise Interrupt and SystemExit here, and ideally only rescue whatever exception(s) unescape raises when input is invalid.

Good point, b443d22 fixes that.

@raggi Owner
raggi added a note

Thank you for improving this.

For the record, Interrupt and SystemExit are not children of StandardError and are not caught by argument-less rescue. That particular problem only exists for the common rescue Exception anti-pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ # Ignore invalid (key,value) pairs
+ next if /^invalid %-encoding \(.*\)$/.match e.message
+ # Reraise other errors
+ raise
+ end
if cur = params[k]
if cur.class == Array
params[k] << v
View
4 test/spec_request.rb
@@ -369,8 +369,8 @@
end
should "raise any errors on every request" do
- req = Rack::Request.new Rack::MockRequest.env_for("", "HTTP_COOKIE" => "foo=%")
- 2.times { proc { req.cookies }.should.raise(ArgumentError) }
+ req = Rack::Request.new Rack::MockRequest.env_for("", "rack.input" => nil)
+ 2.times { proc { req.POST }.should.raise(RuntimeError) }
end
should "parse cookies according to RFC 2109" do
View
16 test/spec_utils.rb
@@ -68,7 +68,7 @@ def kcodeu
Rack::Utils.escape("ø".encode("ISO-8859-1")).should.equal "%F8"
end
end
-
+
should "not hang on escaping long strings that end in % (http://redmine.ruby-lang.org/issues/5149)" do
lambda {
timeout(1) do
@@ -105,6 +105,20 @@ def kcodeu
Rack::Utils.parse_query("foo%3Dbaz=bar").should.equal "foo=baz" => "bar"
end
+ should "raise ArgumentError, 'invalid %-encoding (...)'" do
+ lambda {
+ URI.decode_www_form_component "%"
+ }.should.raise(ArgumentError).
+ message.should.match /^invalid %-encoding \(.*\)$/
+ end
@raggi Owner
raggi added a note

Thank you for putting this check in place. I agree with this approach in general.

Either way, I'm feeling a little uneasy about coupling this code to the content of an error message from the stdlib.

@tenderlove - dude, this really is coming back to that stdlib discussion again - I'd rather we didn't risk old versions not implementing this feature because the stdlib changes in future. Do you have any good suggestions on how to deal with this?

@dkubb
dkubb added a note

I was going to mention the coupling above. It seems like it could be a bit brittle.

Is there a way (in future releases) where instead of raising an ArgumentError, the system raises a subclass of ArgumentError for this specific case? Code that expects an kind-of ArgumentError will still work, and the Rack code can start to match on the subclass in addition to ArgumentError as it does above. At some point in the future Rack can remove the coupling to the old approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ should "ignore incorrectly escaped query strings" do
+ Rack::Utils.parse_query("foo=100%wrong").
+ should.be.empty
+ Rack::Utils.parse_query("foo=bar&nasty=100%wrong&baz=okay").
+ should.equal "foo" => "bar", "baz" => "okay"
+ end
+
should "parse nested query strings correctly" do
Rack::Utils.parse_nested_query("foo").
should.equal "foo" => nil
Something went wrong with that request. Please try again.