Skip to content

Loading…

Fix broken Rack params parser in Rails >= 2.3.6 #739

Closed
lighthouse-import opened this Issue · 17 comments

1 participant

@lighthouse-import

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/5873
Created by Rob Anderton - 2011-01-18 08:52:34 UTC

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:

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

@lighthouse-import

Imported from Lighthouse.
Comment by Jeff Kreeftmeijer - 2010-11-01 17:04:55 UTC

Automatic cleanup of spam.

@lighthouse-import

Imported from Lighthouse.
Comment by Stephen Blackstone - 2010-11-03 19:51:51 UTC

+1 This is impacting my production application...

@lighthouse-import

Imported from Lighthouse.
Comment by Michael Koziarski - 2010-11-09 22:38:01 UTC

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

@lighthouse-import

Imported from Lighthouse.
Comment by Neeraj Singh - 2010-11-10 03:49:19 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Rob Anderton - 2010-11-10 09:45:39 UTC

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 :)

@lighthouse-import

Imported from Lighthouse.
Comment by Neeraj Singh - 2010-11-10 09:51:33 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Roel van der Hoorn - 2010-12-16 09:04:56 UTC

+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.

@lighthouse-import

Imported from Lighthouse.
Comment by Rob Anderton - 2010-12-16 09:32:51 UTC

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?

@lighthouse-import

Imported from Lighthouse.
Comment by Jonathan Wallace - 2011-01-14 14:24:59 UTC

+1

@lighthouse-import

Imported from Lighthouse.
Comment by Trung Lê - 2011-02-15 18:48:35 UTC

where's the love? +1

@lighthouse-import

Imported from Lighthouse.
Comment by Andrew White - 2011-02-15 20:30:52 UTC

Doesn't the rack 1.1.1.pre release fix this - specifically:
rack/rack@6fa19e3

@lighthouse-import

Imported from Lighthouse.
Comment by Rob Anderton - 2011-02-16 11:01:06 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Rob Anderton - 2011-02-16 11:07:44 UTC

Ah maybe Rack is finally going to sort itself out :)
http://groups.google.com/group/rack-devel/browse_thread/thread/b1c3bb2adda31e3c

@lighthouse-import

Imported from Lighthouse.
Comment by jrochkind - 2011-02-16 14:37:17 UTC

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?

@lighthouse-import

Imported from Lighthouse.
Comment by Andrew White - 2011-02-16 14:48:11 UTC

If you use the Rack 1.1.1.pre gem with 2.3.11 it should be fixed.

@lighthouse-import

Imported from Lighthouse.
Comment by Rob Anderton - 2011-02-16 16:01:56 UTC

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.