Check cookie overflow #6796

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@iGEL
Contributor

iGEL commented Jun 20, 2012

Rails does not check the length of the value of normal and permanent cookies, just signed cookies. This pull request changes this, so all cookies are checked.

@@ -86,7 +86,9 @@ class CookieOverflow < StandardError; end
class CookieJar #:nodoc:
include Enumerable
-
+
+ MAX_COOKIE_SIZE = 4096 # Cookies can typically store 4096 bytes.

This comment has been minimized.

Show comment Hide comment
@gmile

gmile Jun 20, 2012

Contributor

I might be missing something, buy RFC2109 states the following:

Furthermore, general-use user agents should provide each of the following minimum capabilities individually, although not necessarily simultaneously:
...

  • at least 4096 bytes per cookie (as measured by the size of the characters that comprise the cookie non-terminal in the syntax description of the Set-Cookie header)

...

So why considering 4096 as a max size then?

@gmile

gmile Jun 20, 2012

Contributor

I might be missing something, buy RFC2109 states the following:

Furthermore, general-use user agents should provide each of the following minimum capabilities individually, although not necessarily simultaneously:
...

  • at least 4096 bytes per cookie (as measured by the size of the characters that comprise the cookie non-terminal in the syntax description of the Set-Cookie header)

...

So why considering 4096 as a max size then?

This comment has been minimized.

Show comment Hide comment
@stevenh512

stevenh512 Jun 20, 2012

Contributor

Because the RFC states that browsers have to allow at least 4096 bytes per cookie. The browser may allow more than that, but there's no guarantee that more than 4096 bytes is allowed. Going with the RFC is easier and probably more reliable than sniffing user-agent strings and trying to accommodate each individual browser's actual max. cookie size.

edit: Actually, since the word should is used in the RFC, there's also no guarantee that 4096 bytes is allowed.. but that shouldn't be a problem with any modern browser.

@stevenh512

stevenh512 Jun 20, 2012

Contributor

Because the RFC states that browsers have to allow at least 4096 bytes per cookie. The browser may allow more than that, but there's no guarantee that more than 4096 bytes is allowed. Going with the RFC is easier and probably more reliable than sniffing user-agent strings and trying to accommodate each individual browser's actual max. cookie size.

edit: Actually, since the word should is used in the RFC, there's also no guarantee that 4096 bytes is allowed.. but that shouldn't be a problem with any modern browser.

@iGEL

This comment has been minimized.

Show comment Hide comment
@iGEL

iGEL Jun 20, 2012

Contributor

Well, after thinking about this, I believe my implementation is not correct. As far as I understand the 4K limit, the escaped value should not exceed the 4K, but I check the raw value. Also, it's allowed to store an Array in the cookie. In that case, I don't calculate the correct value length either. What do you think, should I adjust the behavior to reflect this or isn't it worth the hassle? Maybe Rack should check the length of cookies, not Rails.

Contributor

iGEL commented Jun 20, 2012

Well, after thinking about this, I believe my implementation is not correct. As far as I understand the 4K limit, the escaped value should not exceed the 4K, but I check the raw value. Also, it's allowed to store an Array in the cookie. In that case, I don't calculate the correct value length either. What do you think, should I adjust the behavior to reflect this or isn't it worth the hassle? Maybe Rack should check the length of cookies, not Rails.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jun 20, 2012

Member

@iGEL please open the pull request pointing to master branch. If we think that it should be backported to 3-2-stable we will ask.

Thanks.

Member

rafaelfranca commented Jun 20, 2012

@iGEL please open the pull request pointing to master branch. If we think that it should be backported to 3-2-stable we will ask.

Thanks.

@iGEL

This comment has been minimized.

Show comment Hide comment
@iGEL

iGEL Jun 24, 2012

Contributor

Going to create a new pull request pointing to master and with correct behavior.

Contributor

iGEL commented Jun 24, 2012

Going to create a new pull request pointing to master and with correct behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment