#5873 Fix broken Rack params parser in Rails >= 2.3.6 - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#5873 new

Fix broken Rack params parser in Rails >= 2.3.6

Reported by Rob Anderton | October 27th, 2010 @ 11:56 PM

Since this change in May that was part of the 2.3.6 release, Rails has been dependent on the Rack 1.1 gem.

Unfortunately this version of Rack does bad things to query strings containing quotes which at best leads to them being stripped out of the params hash values and at worst leads to values being silently truncated.

I first had a look at this problem a few months back and as it's not been fixed in the meantime have now had chance to work on a patch which I think uses the best approach to fixing it: monkey-patching the broken code in Rack.

I appreciate we'd normally want to steer clear of monkey-patching in favour of using the fixed version of the gem, but this isn't really an option as Rack is broken in more ways than one:

Rack 1.2 fixes the query string parser but breaks multipart uploads
Rack 1.2.1 still has broken multipart uploads but also breaks compatibility with Ruby 1.8.6

Rolling back to Rack 1.0.1 also proves problematic as other parts of Rails 2.3 have been changed to work with the 1.1 release.

So to summarise the attached patch:

it adds a monkey-patch for the broken parsing methods in Rack::Utils using a similar pattern to the Rack::Lint patch that had been added previously
it removes the Rack::Lint patch as it no longer applies to Rack 1.1.0
and on a completely unrelated issue it tweaks a template to prevent annoying parentheses warnings when running the tests

fix_rack_params_parsing.diff 8 KB

Comments and changes to this ticket

Jeff Kreeftmeijer November 1st, 2010 @ 05:04 PM
- Tag cleared.
- Importance changed from “” to “Low”
Automatic cleanup of spam.
You flagged this item as spam.
Stephen Blackstone November 3rd, 2010 @ 07:51 PM
+1 This is impacting my production application...
Michael Koziarski November 9th, 2010 @ 10:38 PM
For this one, could we perhaps simply update 2.3.11 to depend on 1.2? Would anything else misbehave?

The rack team have shown no interest in doing point releases for the 1.1. release
Michael Koziarski November 9th, 2010 @ 10:41 PM
- Assigned user set to “Michael Koziarski”
Neeraj Singh November 10th, 2010 @ 03:49 AM
I bumped up rack to 1.2 in actionpack and all the tests are passing. Not sure if that means all is good since I don't really know how many tests go through the full stack and how many bypasses rack.
You flagged this item as spam.
Rob Anderton November 10th, 2010 @ 09:45 AM
As I said in the original ticket I looked at bumping to 1.2 but as these were also broken decided to stick with 1.1 and just patch this specific problem.

Running with Rack 1.2.0 gives 2 failures for me (this is using Ruby 1.8.6 p383):
- CookieTest#test_multiple_cookies fails with an incorrect cookie string as it doesn't expect a trailing newline
- MultipartParamsParsingTest#test_parses_mixed_files fails 500 error because Rack tries to call #rewind on a string
Rack 1.2.1 breaks immediately (the tests don't get to run at all) because Rack::Utils makes a call to the union method of Regexp which is ok with newer Ruby versions but not with 1.8.6.

So whichever version you choose it's gonna involve monkeys :)
Neeraj Singh November 10th, 2010 @ 09:51 AM
My bad.

I ran the tests again with rack 1.2 and I am getting the same error as reported by Rob.

I am using.

ruby 1.8.7 (2010-04-19 patchlevel 253) [i686-darwin10.4.0], MBARI 0x6770, Ruby Enterprise Edition 2010.02.
You flagged this item as spam.
Roel van der Hoorn December 16th, 2010 @ 09:04 AM
+1 for the attached patch; nasty bug.

If Rack 1.2 introduces other problems, it is clearly no way to go for a patch-release. Also, Rack's lack of willing to do a point release for 1.1 is really crappy.
You flagged this item as spam.
Rob Anderton December 16th, 2010 @ 09:32 AM
I'm a little concerned that something that could cause major headaches for people has been assigned a low priority - it's nearly 7 months since the bug was introduced and 2 months since I submitted a patch.

I can only assume it hasn't created more of a furore because it's a silent killer most likely to be blamed on user error :)

What needs to be done to ensure this gets fixed?
You flagged this item as spam.
Jonathan Wallace January 14th, 2011 @ 02:24 PM
+1
You flagged this item as spam.
Trung Lê February 15th, 2011 @ 06:48 PM
where's the love? +1
Andrew White February 15th, 2011 @ 08:30 PM
Doesn't the rack 1.1.1.pre release fix this - specifically:
https://github.com/rack/rack/commit/6fa19e3a268c05e24907ffe1b344b23...
You flagged this item as spam.
Rob Anderton February 16th, 2011 @ 11:01 AM
afaik 1.1.1 never made it to a 'proper' release. 1.2.x also includes the fix but as discussed above this introduces new problems - patching 1.1 seems like a sensible approach to fixing this problem until Rack sorts itself out.

Still surprised more people haven't complained about this.
You flagged this item as spam.
Rob Anderton February 16th, 2011 @ 11:07 AM
Ah maybe Rack is finally going to sort itself out :)
http://groups.google.com/group/rack-devel/browse_thread/thread/b1c3...
You flagged this item as spam.
jrochkind February 16th, 2011 @ 02:37 PM
This is NOT fixed in 2.3.11, right? Alas. Still keeping me from updating to a later Rails 2.x than 2.3.6. It surprises me that this hasn't received more attention and is still marked priority=low, I guess few apps need to have quotes in http query strings?
Andrew White February 16th, 2011 @ 02:48 PM
If you use the Rack 1.1.1.pre gem with 2.3.11 it should be fixed.
You flagged this item as spam.
Rob Anderton February 16th, 2011 @ 04:01 PM
I've just run through the tests with 1.1.1 installed and the good news is it now works.

I've attached an updated patch: same as last time although obviously no longer any need to monkey-patch rack. I have bumped the version to 1.1.1 so that people don't inadvertently run with 1.1.0 and kept the query string tests in case Rack ever goes off the rails again.
- fix_rack_params_parsing_updated.diff 5.2 KB