This project is archived and is in readonly mode.

#130 ✓resolved
Rich Cavanaugh

CGI::Cookie and CookieStore performing unnecessary work causing interoperability bug with Merb

Reported by Rich Cavanaugh | May 7th, 2008 @ 05:58 AM | in 2.1.1

CookieStore is currently escaping and unescaping while this is handled in other places in ActionController. This means double the work which does, in some cases result in different output.

CGI::Cookie had a bug in it's handling of a Hash parameter. It handled a string value with newlines in it differently depending on whether it was handed a Hash or not. Given a Hash with a string value containing newlines, the value would be split into multiple values. This resulted in '&' being inserted into the CookieStore values due to how multiple cookie values get concatenated.

Both of these issues caused interoperability bugs with my Merb application that shared the CookieStore with the Rails app.

All existing Rails tests pass with these patches. I've added tests where appropriate. These minor fixes did correct all related issues with my applications.

Comments and changes to this ticket

  • Pratik

    Pratik May 11th, 2008 @ 10:33 PM

    • State changed from “new” to “invalid”

    This is not a backward compatible change and will cause all existing cookie sessions to raise CGI::Session::CookieStore::TamperedWithCookie exception.

    Thanks.

  • Rich Cavanaugh

    Rich Cavanaugh May 11th, 2008 @ 11:19 PM

    It would be simple enough to make this work with existing cookies. Would a patch implementing this be likely to be accepted?

  • Pratik

    Pratik May 11th, 2008 @ 11:23 PM

    • Assigned user set to “Jeremy Kemper”
  • Rich Cavanaugh
  • Rich Cavanaugh

    Rich Cavanaugh May 12th, 2008 @ 07:27 PM

    Here's a patch that passes all of rails' tests, my application specific tests and is backwards compatible (with tests for that too).

  • Jeremy Kemper

    Jeremy Kemper May 12th, 2008 @ 09:28 PM

    • State changed from “invalid” to “open”
    -        @value = Array(name['value'])
    +        @value = name['value'].kind_of?(String) ? [name['value']] : Array(name['value'])
    

    Why? Array('foo') == ['foo']

  • Rich Cavanaugh

    Rich Cavanaugh May 12th, 2008 @ 09:55 PM

    @value is expected to be an array but if you pass "one\ntwo" to Array you get:

    Array("one\ntwo") # => ["one\n", "two"]

    Which gets written out in the cookie header to "one%0A&two" (notice the & used for joining the separate elements). This issue, when used with other systems without this behavior (in my case merb), causes digest mis-matches.

    Array("one\ntwo") != ["one\ntwo"] # => true

  • Jeremy Kemper

    Jeremy Kemper May 12th, 2008 @ 10:03 PM

    Ah, I see. Could you include a test for that case? The patch looks good, otherwise.

  • Rich Cavanaugh

    Rich Cavanaugh May 12th, 2008 @ 10:05 PM

    I actually did, it's called test_cookies_should_not_be_split_on_values_with_newlines

  • Jeremy Kemper

    Jeremy Kemper May 12th, 2008 @ 10:39 PM

    Sorry, I missed that. I have mixed feelings about changing this behavior for all cookie values. Backward compatibility applies to the cookie store only.

  • Rich Cavanaugh

    Rich Cavanaugh May 12th, 2008 @ 10:52 PM

    Yeah, I agree but, taken in context CGI::Cookie is plainly broken and acting differently depending on how you initialize.

    When name is a string the value is already an array so:

    CGI::Cookie.new("avar", "one\ntwo").value # => ["one\ntwo"]

    While:

    CGI::Cookie.new(:name => "avar", :value => "one\ntwo").value # => ["one\n", "two"]

    That's why I didn't make the same change to the String path in CGI::Cookie#initialize because the variable argument *value is always an array and Array doesn't mangle an array's values the same way it does strings.

    So the behavior is already inconsistent and this patch simply makes the initializer act consistently.

  • Jeremy Kemper

    Jeremy Kemper May 12th, 2008 @ 11:14 PM

    I buy that. Thanks for clarifying, Rich.

  • Repository

    Repository May 12th, 2008 @ 11:27 PM

    • State changed from “open” to “resolved”

    (from [a425cd147363a0e8d7e17177ef252dd760197f15]) Don't double-escape cookie store data. Don't split cookie values with newlines into an array. [#130 state:resolved]

    Signed-off-by: Jeremy Kemper

    http://github.com/rails/rails/co...

  • Tarmo Tänav

    Tarmo Tänav May 13th, 2008 @ 05:04 PM

    This change seems to change the behavior of deleting a cookie. When a cookie is deleted it's value is set to an empty string. Previously that would have meant:

    Array('') => []

    but now it is turned into: ['']

    I'm not really familiar with how cookies are supposed to behave so I'm not sure where the bug is, but for example with the restful_authentication plugin generated code what happens is:

    1. action does 'cookies.delete :auth_token'

    2. test fails on 'assert_equal @response.cookies["auth_token"], []' with [""] not being equal to []

    Is this expected?

  • Rich Cavanaugh

    Rich Cavanaugh May 13th, 2008 @ 05:21 PM

    I've attached a new patch addressing this issue along with a test for it.

  • Jeremy Kemper

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages