Opened 18 months ago

Closed 18 months ago

Last modified 18 months ago

#22242 closed Bug (fixed)

Setting cookie that is too large fails silently

Reported by: Euphorbium <compositum@…> Owned by: pirosb3
Component: Documentation Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Recently I've ran into a situation, that was extremely difficult to debug. Sometimes the cookie was set correctly, but sometimes it was not set at all. Turns out, that sometimes I was trying to set a cookie that was larger than 4096 bytes, and that is the cookie size limit in chrome and many other browsers. Trying to set a cookie that is too large should throw an exception. I could fix this myself, but I would like to hear other's opinions on this, because cookie size limit varies between the browsers, and I don't think that there is a way to find this out from django without a trial and error. This affects all versions of django.

Change History (14)

comment:1 Changed 18 months ago by erikr

  • Component changed from Uncategorized to HTTP handling
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Both RFC 2109 and the more recent, but possibly not adopted everywhere yet, RFC 6265 state a minimum size allowance of 4096 bytes for cookies, as a SHOULD. Note that this includes the name, value and attributes as well.

If modern browsers follow the RFC, (it seems that Internet Explorer does, for example), and all allow at least 4096 bytes, I think we should throw an exception for larger cookies. This does not catch every potential issue: there are also limits on the total amount or size of cookies. However, testing for the maximum size of a single cookie seems to be so trivial compared to the benefit, I think we should add this check.

comment:2 Changed 18 months ago by Euphorbium <compositum@…>

  • Owner changed from nobody to anonymous
  • Status changed from new to assigned

comment:3 follow-up: Changed 18 months ago by Euphorbium <compositum@…>

Ok, I've noticed that I am unable to set any cookie bigger than 4087 bytes on Chrome from django.

#views.py
from django.http import HttpResponse

def cookie(request):
    response.set_cookie('a', value="a"*4086)
    return response

Chrome probably displays the size of the cookie incorrectly, because if I try to set any cookie parameter, the cookie is not set at all, but if I try to set a parameter for a smaller cookie the cookie sets correctly, but the size displayed is not increased. It is the same is for firefox with firebug, but firefox tolerates larger cookies, so that is not a problem.

So django seems to set some cookie parameters by default, and it is hard form me to find them, as the size of the parameters is not calculated in the size of the cookie displayed by chrome.
I was not able to set the cookie that was 4096 bytes from javascript either, max that it accepted was 4095 bytes

document.cookie="a=any_4094_character_string"

comment:4 in reply to: ↑ 3 Changed 18 months ago by erikr

Replying to Euphorbium <compositum@…>:

So django seems to set some cookie parameters by default, and it is hard form me to find them, as the size of the parameters is not calculated in the size of the cookie displayed by chrome.

Well, one of the defaults is to set the Path to /, for example. But we shouldn't make assumptions about the cookie parameters in any case. It looks like Django cookie handling code can't determine the final length: in the end the cookie is generated by the Python Cookie library. Encoding of special characters also influences the size. Therefore, I'm not sure whether it's actually possible to determine the final length of the cookie when set_cookie is being called.

comment:5 Changed 18 months ago by PirosB3

hey guys,

In core/handlers/wsgi.py

        for c in response.cookies.values():
            response_headers.append((str('Set-Cookie'), str(c.output(header=''))))

Couldn't we override django.http.cookie.SimpleCookie.output, and make it throw an exception for larger cookies?

Regards,
Daniel Pyrathon

comment:6 Changed 18 months ago by PirosB3

I had a bigger look at this.

in Cookie.py, the OutputString is what builds our output. http://hg.python.org/cpython/file/2.7/Lib/Cookie.py
I'll paste it for convenience:

  def OutputString(self, attrs=None):
        # Build up our result
        #
        result = []
        RA = result.append

        # First, the key=value pair
        RA("%s=%s" % (self.key, self.coded_value))

        # Now add any defined attributes
        if attrs is None:
            attrs = self._reserved
        items = self.items()
        items.sort()
        for K,V in items:
            if V == "": continue
            if K not in attrs: continue
            if K == "expires" and type(V) == type(1):
                RA("%s=%s" % (self._reserved[K], _getdate(V)))
            elif K == "max-age" and type(V) == type(1):
                RA("%s=%d" % (self._reserved[K], V))
            elif K == "secure":
                RA(str(self._reserved[K]))
            elif K == "httponly":
                RA(str(self._reserved[K]))
            else:
                RA("%s=%s" % (self._reserved[K], V))

        # Return the result
        return _semispacejoin(result)

RCF 6265 says that Cookies are measured by the sum of the length of the cookie's name, value, and attributes. So we are interested in the result array, which would make it very easy to calculcate the size of each cookie. Unfortunately the function returns a string. Does anyone see a good way of interacting with this function without having to repeat this code in another function?

comment:7 Changed 18 months ago by erikr

So, a number of things have come up:

  • In set_cookie(), there is insufficient information to decide whether or not the cookie will be too large.
  • One way to resolve that is to generate the full cookie in set_cookie(), or something like that, just to check the length.
  • Alternatively, this could be caught when generating the Set-Cookie headers in the WSGIHandler.
  • Regardless of this change, there are other ways to send broken cookies, like having a path or domain that does not match the current one, or having many cookies which are individually small enough, but together too large.

Raising an error from WSGIHandler does not seem like the appropriate place, and regardless would not even work as expected: as far as I can see, the 500 handling, which will display the debug page or render the configured 500 page, etc., is not called if an exception is raised at this stage - that exception handling happens earlier.

Calculating the full length of the cookie on every set_cookie() call would work. However, this adds overhead to every cookie ever set, while setting cookies that are too large seems very exceptional to me. I don't think the rarity of this issue warrants adding such a performance impact on every cookie generated by every Django instance. And even in that case, we would only catch some of the errors made.

All things considered, it seems this is much harder than initially thought. Considering that, in combination with the rareness of this issue, and the fact that we can only catch some of these problems, I think it is not sensible to enforce cookie size in set_cookie(). Amending the set_cookie() documentation to mention the typical maximum length would definitely be useful and reasonable.

comment:8 Changed 18 months ago by Euphorbium <compositum@…>

  • Easy pickings unset
  • Owner anonymous deleted
  • Status changed from assigned to new

comment:9 Changed 18 months ago by pirosb3

Hi erikr,

I understand your reasoning perfectly. The compromise of having to check the cookie size on every set is too big.
Should I update the documentation?

Regards,
Dan

comment:10 Changed 18 months ago by erikr

  • Component changed from HTTP handling to Documentation
  • Easy pickings set
  • Version changed from 1.6 to master

Yes, please make a documentation patch.

comment:11 Changed 18 months ago by pirosb3

  • Has patch set

I have made a documentation patch. Please let me know if anything else should be added.

https://github.com/django/django/pull/2434

comment:12 Changed 18 months ago by pirosb3

  • Owner set to pirosb3
  • Status changed from new to assigned

comment:13 Changed 18 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 885e7adf568037b59b5642ab061133eaa00e5d7d:

Fixed #22242 -- Documented common cookie size limit.

comment:14 Changed 18 months ago by Tim Graham <timograham@…>

In 9f7bd831846a921e233c1f95d725235db9550438:

[1.6.x] Fixed #22242 -- Documented common cookie size limit.

Backport of 885e7adf56 from master

Note: See TracTickets for help on using tickets.
Back to Top