Skipping empty params inside query what leads to parsing error. #378

Merged
merged 1 commit into from May 4, 2012

Projects

None yet
@josin
Contributor
josin commented Apr 12, 2012

I found this problem when webpage on domain www.example.com was saving these strange cookies ("foo=bar,;bar=foo" or ",foo=bar;,") and subdomain was failing because of that.

@josin josin Skipping empty params inside query what lead to parsing error.
Example of these cookies would be: "foo=bar,;bar=foo" or ",foo=bar;,"
cb05a10
@skiz
skiz commented May 2, 2012

Confirmed this works for a very simliar issue we were having after upgrading from rack 1.2.5 to 1.4.1

curl --head --cookie ";=foo;x=999" http://localhost:3000/

Using this branch fixes the issue for us. Not sure how those cookies got there, but the parsing shouldn't 500 rack.

👍

@raggi raggi merged commit 73cd273 into rack:master May 4, 2012
@sgrunberger

I don't yet see the point of this change. If p is an empty string, then on the next line p.split.map will result in an empty array, and we'll skip to the next iteration on the line after that. Is this just for code clarity?

@josin
Contributor
josin commented Jul 30, 2012

@sgrunberger well the point was to fix issues described with this pull request and the same described by @skiz... on the other side your comment make sense, but without this it leads to 500 on rack...

@mattetti
mattetti commented Sep 4, 2012

Just a quick note to let you know that I also experienced the problem reported here because of some Google Analytics' cookies.

@mattetti mattetti referenced this pull request in mattetti/wd-sinatra Oct 11, 2012
Closed

In production env: can't convert nil into Integer #13

@mperham
Contributor
mperham commented Oct 19, 2012

It sure would be nice to see a 1.4.2 gem release with this fix. We see it regularly too.

@mperham
Contributor
mperham commented Oct 19, 2012

Note if I use master, I get a big scary SECURITY WARNING due to Rails' session store usage. So I stick with the bug or I upgrade and we get to look at a scary message for the foreseeable future. Neither is a fun choice.

@jimherz
jimherz commented Dec 4, 2012

+1 for 1.4.2 release with this fix.

@andreas12345

It's a mystery to me why this is not yet released as 1.4.2. I see this issue daily in production, and having to monkey-patch rack is not a nice solution.

@jumph4x
jumph4x commented Dec 14, 2012

+1 :(

@mikelewis

+1

@findchris

👍 Waiting for 1.4.2. Hit this in production toda: Google Analytics set a cookie (containing a URL that contained commas) that caused 500s. Thanks!

@JoshuaOSHickman

+1

I'd like to see this too, was a tricky one to even find, by-passed my error reporting.

@jumph4x
jumph4x commented Dec 21, 2012

@JoshuaOSHickman ditto. I used to use Airbrake and it couldn't find it. Switched to Ratchet and it came right up.

@mattetti

Does anyone know what's going on with Rack releases? Who's in charge, what's blocking, can we help?

@rkh
Member
rkh commented Dec 21, 2012

@raggi is in charge of releases. I think only he and @chneukirchen can actually push to rubygems.org.

@raggi
Member
raggi commented Dec 25, 2012

@mattetti what's blocking is pending security fixes that I've yet to get any response back from the core team on the -core ML for many months. I've been slowly working through prepping the release, but have yet to find the time to finalize it. Some of the issues simply won't be fixed.

You might be able to help by taking the time to properly review incoming PRs for regressions and/or other issues, as most PRs that we get contain problems (sadly).

I realize that this bug affects a bunch of people, and that many of those people just want a release with this patch, however, people also don't want a CVE out without a patch, nor do these folks in #360.

I get it, you want a release, but please stop filling my inbox with 👍, I'm well aware that folks are affected, but I'm also on the first holiday I've had in a year, which I intend to take at least a few more days before I do more charity work.

I know there are also plenty of people who will happily step forward and say "I'll help", but when it comes down to it, and I reveal the harder compatibility problems we have, they all of a sudden disappear. I also have no more time for folks who just want commit access for "prestige", as I'm sick of this arrival and then complete lack of assistance as soon as it gets tough. I'd love to hand this over to someone, and honestly if someone asked to take responsibility, and put in writing they'd fix their own mistakes in the first few releases, I'd hand the torch over PDQ.

In the meantime, I'll try and keep the releases at least stable for most of our users most of the time, with what time I can scrape together.

Merry christmas all, hopefully I'll get something out around or shortly after new year.

@jumph4x
jumph4x commented Dec 25, 2012

Deployed it via the :git directive by pointing to this repo, took care of my problems.

Take it easy @raggi :) Enjoy the time off and thanks for the hard work 👏

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