Browse files

Prevent error in Utils.parse_query/KeySpaceConstrainedParams when a k…

…ey is nil

Before this when parsing a query string or a cookie string, an error were
raised because of the `key.size` in `KeySpaceConstrainedParams#[]=`.
This caused an error when params contained something parsed as a `nil` key.
  • Loading branch information...
1 parent 9f72924 commit 2dfe94071497678c15cfcd8e63663d92f82958e5 @JonathanTron JonathanTron committed Jan 24, 2012
Showing with 8 additions and 1 deletion.
  1. +2 −1 lib/rack/utils.rb
  2. +6 −0 test/spec_utils.rb
View
3 lib/rack/utils.rb
@@ -65,6 +65,7 @@ def parse_query(qs, d = nil)
(qs || '').split(d ? /[#{d}] */n : DEFAULT_SEP).each do |p|
k, v = p.split('=', 2).map { |x| unescape(x) }
+ next unless k || v
if cur = params[k]
if cur.class == Array
@@ -442,7 +443,7 @@ def [](key)
end
def []=(key, value)
- @size += key.size unless @params.key?(key)
+ @size += key.size if key && !@params.key?(key)
raise RangeError, 'exceeded available parameter key space' if @size > @limit
@params[key] = value
end
View
6 test/spec_utils.rb
@@ -112,6 +112,12 @@ def kcodeu
Rack::Utils.parse_query("my+weird+field=q1%212%22%27w%245%267%2Fz8%29%3F").
should.equal "my weird field" => "q1!2\"'w$5&7/z8)?"
Rack::Utils.parse_query("foo%3Dbaz=bar").should.equal "foo=baz" => "bar"
+ Rack::Utils.parse_query("=").should.equal "" => ""
+ Rack::Utils.parse_query("=value").should.equal "" => "value"
+ Rack::Utils.parse_query("key=").should.equal "key" => ""
+ Rack::Utils.parse_query("&key&").should.equal "key" => nil
+ Rack::Utils.parse_query(";key;", ";,").should.equal "key" => nil
+ Rack::Utils.parse_query(",key,", ";,").should.equal "key" => nil
end
should "parse nested query strings correctly" do

12 comments on commit 2dfe940

@andreas-s

Thanks for fixing this! This broke my website for about a quarter of my users.

@krsmurata

Thanks! Will this be on next release? 1.4.2 maybe? :)

@sgrunberger

This fixes what I'd guess is a pretty widespread problem. We've seen many production failures in rack 1.4.1's cookie parsing code due to malformed Google Analytics cookies (a specific GA cookie value sometimes contain unescaped commas).

Google Analytics bug report detailing the problem:
http://code.google.com/p/analytics-issues/issues/detail?id=152

Equivalent Netty bug report related to GA cookie parsing failures:
https://issues.jboss.org/browse/NETTY-255?_sscc=t

@sgrunberger

This fixes what I'd guess is a pretty widespread problem. We've seen many production failures in rack 1.4.1's cookie parsing code due to malformed Google Analytics cookies (a specific GA cookie value sometimes contain unescaped commas).

Google Analytics bug report detailing the problem:
http://code.google.com/p/analytics-issues/issues/detail?id=152

Equivalent Netty bug report related to GA cookie parsing failures:
https://issues.jboss.org/browse/NETTY-255?_sscc=t

@purcell

+1 on a new release containing this fix (see #386).

@raggi
Official Rack repositories member

yes, this will be in the next release, that'll happen as soon as i have a few more hours to sit down and work on rack

@krsmurata

Thank you @raggi :)

I'm using rack directly from github only because of this fix (locked on this commit hash).

@purcell

For the impatient, here's a monkey-patch file: https://gist.github.com/2877275

@ShiningRay

+1 for this. just encounter this weird problem.

@adriancb

Rack 1.4.1 - Rails 3.2.6

rackup - cookies[nil] = { :value => nil } in any controller method

Exception `NoMethodError' at /Users/adriancb/.rvm/gems/ruby-1.9.3-p125-perf@newco/gems/rack-1.4.1/lib/rack/utils.rb:445 - undefined method `size' for nil:NilClass
Exception `NoMethodError' at /Users/adriancb/.rvm/gems/ruby-1.9.3-p125-perf@newco/gems/rack-1.4.1/lib/rack/request.rb:270 - cannot parse Cookie header: undefined method `size' for nil:NilClass

Interesting thing is - Chrome handles the rogue cookie really badly compared to other browsers (Firefox, IE, Safari)...

@cespare

+1 for cutting a new release. Very annoying bug.

@jimherz

+1 for cutting a new release with this fix.

Please sign in to comment.