Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check cookie overflow #6796

Closed
wants to merge 1 commit into from
Closed

Check cookie overflow #6796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions actionpack/lib/action_dispatch/middleware/cookies.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,9 @@ class CookieOverflow < StandardError; end

class CookieJar #:nodoc:
include Enumerable


MAX_COOKIE_SIZE = 4096 # Cookies can typically store 4096 bytes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# This regular expression is used to split the levels of a domain.
# The top level domain can be any string without a period or
# **.**, ***.** style TLDs like co.uk or com.au
Expand Down Expand Up @@ -168,6 +170,8 @@ def []=(key, options)
value = options
options = { :value => value }
end

raise CookieOverflow if options[:value].to_s.size > MAX_COOKIE_SIZE

@cookies[key.to_s] = value

Expand Down Expand Up @@ -274,7 +278,6 @@ def method_missing(method, *arguments, &block)
end

class SignedCookieJar < CookieJar #:nodoc:
MAX_COOKIE_SIZE = 4096 # Cookies can typically store 4096 bytes.
SECRET_MIN_LENGTH = 30 # Characters

def initialize(parent_jar, secret)
Expand All @@ -299,7 +302,6 @@ def []=(key, options)
options = { :value => @verifier.generate(options) }
end

raise CookieOverflow if options[:value].size > MAX_COOKIE_SIZE
@parent_jar[key] = options
end

Expand Down
24 changes: 23 additions & 1 deletion actionpack/test/dispatch/cookies_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,17 @@ def set_signed_cookie
end

def raise_data_overflow
cookies.signed[:foo] = 'bye!' * 1024
cookies[:foo] = 'bye!' * 1024 + "a"
head :ok
end

def raise_data_overflow_permanent
cookies.permanent[:foo] = 'bye!' * 1024 + "a"
head :ok
end

def raise_data_overflow_signed
cookies.signed[:foo] = 'bye!' * 1024 + "a"
head :ok
end

Expand Down Expand Up @@ -284,6 +294,18 @@ def test_raise_data_overflow
end
end

def test_raise_data_overflow_permanent
assert_raise(ActionDispatch::Cookies::CookieOverflow) do
get :raise_data_overflow_permanent
end
end

def test_raise_data_overflow_signed
assert_raise(ActionDispatch::Cookies::CookieOverflow) do
get :raise_data_overflow_signed
end
end

def test_tampered_cookies
assert_nothing_raised do
get :tampered_cookies
Expand Down