Rails Ruby on Rails → 2.1.1

Comments and changes to this ticket

• 

  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.

• You flagged this item as spam.
  

  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 May 11th, 2008 @ 11:23 PM

  • Assigned user set to “Jeremy Kemper”
• 

  Rich Cavanaugh May 12th, 2008 @ 01:31 AM

  • no changes were found...

  • backwards_compat_cookie_store.patch 5 KB

• You flagged this item as spam.
  

  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 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']

• You flagged this item as spam.
  

  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 May 12th, 2008 @ 10:03 PM

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

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

  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 May 12th, 2008 @ 11:14 PM

  I buy that. Thanks for clarifying, Rich.

• 

  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 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?

• You flagged this item as spam.
  

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

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

  • handle_empty_cookie.patch 1.7 KB
• 

  Jeremy Kemper May 13th, 2008 @ 06:54 PM

  • Milestone set to “2.1.1”

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