This project is archived and is in readonly mode.
Fix broken Rack params parser in Rails >= 2.3.6
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.04 KB
-
-
Stephen Blackstone
+1 This is impacting my production application...
-
Michael KoziarskiFor 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
- Assigned user set to Michael Koziarski
-
Neeraj SinghI 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.
-
Rob AndertonAs 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
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.
-
Roel van der Hoorn
+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.
-
Rob Anderton
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?
-
-
Trung Lêwhere's the love? +1
-
Andrew WhiteDoesn't the rack 1.1.1.pre release fix this - specifically:
https://github.com/rack/rack/commit/6fa19e3a268c05e24907ffe1b344b23... -
Rob Anderton
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.
-
Rob Anderton
Ah maybe Rack is finally going to sort itself out :)
http://groups.google.com/group/rack-devel/browse_thread/thread/b1c3... -
jrochkind
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
If you use the Rack 1.1.1.pre gem with 2.3.11 it should be fixed.
-
Rob Anderton
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.