Opened 14 years ago

Closed 13 years ago

Last modified 12 years ago

#15863 closed Bug (fixed)

SimpleCookies are not correctly serialized with the file or database cache backends

Reported by: Raphael Kubo da Costa Owned by: Paul McMillan
Component: Core (Cache system) Version: 1.2
Severity: Release blocker Keywords:
Cc: Raphael Kubo da Costa, teolicy Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

As discussed in the django-developers mailing list, it appears that SimpleCookies in HttpResponses are not being correctly serialized when one uses either the file or the database cache backends.

The following steps are enough to trigger the incorrect behaviour:

  1. Enable UpdateCacheMiddleware and FetchFromCacheMiddleware in settings.py, and set CACHE_BACKEND accordingly
  2. Enable SessionMiddleware and CsrfViewMiddleware
  3. Have a view with a simple form and no specific cache decorators. Since the session application is being used, the Vary: Cookie header will be added anyway.
  4. In the template used by the view, include the csrf_token tag, as usual.
  5. Access the view, either via curl or a web browser.
  6. The first time the view is accessed, the csrf token is both set in the header as a cookie and displayed as a hidden form element, as expected. The header has the format Set-Cookie: csrftoken=XX; Max-Age: YY; Path=/.
  7. The next times the view is accessed, the cookie header has the format Set-Cookie: csrftoken="Set-Cookie: csrftoken=XX Max-Age: YY; Path=/", and so has the csrf form element, which causes the submitted form to be invalid when the csrf checks are made.

It turns out that UpdateCacheMiddleware serializes the returned HttpResponse in process_response, and both the file and the database cache backends use pickle.dumps with protocol=pickle.HIGHEST_PROTOCOL. It is known that SimpleCookies are incompatible with pickle.HIGHEST_PROTOCOL. FetchFromCacheMiddleware later retrieves this same HttpResponse and returns it, however the cookies have invalid values.

The attached testcase triggers the problem in the unit tests.

Attachments (3)

django_cache_simplecookie_test_failures.diff (954 bytes ) - added by Raphael Kubo da Costa 14 years ago.
Testcase referenced in the bug description.
test-cache-middleware-unpickable-objects.diff (2.3 KB ) - added by Raphael Kubo da Costa 13 years ago.
Proposed addition to Django's test suite
ticket_15863.diff (3.0 KB ) - added by Luke Plant 13 years ago.
Patch (incorporating tests)

Download all attachments as: .zip

Change History (14)

by Raphael Kubo da Costa, 14 years ago

Testcase referenced in the bug description.

comment:1 by Paul McMillan, 14 years ago

milestone: 1.4
Needs tests: set
Status: newassigned
Triage Stage: UnreviewedAccepted

Marking this as accepted, per the mailing list discussion.

The attached testcase triggers a similar error, and is the reason that this is a problem, but it is not a valid test for this bug (since properly fixing this bug will not cause the test to pass).

I'll also note here that the local memory cache backend should use the highest pickle protocol so that it is similar to the other backends.

by Raphael Kubo da Costa, 13 years ago

Proposed addition to Django's test suite

comment:2 by Raphael Kubo da Costa, 13 years ago

Easy pickings: unset

Hey there. I've finally found some time to go back to this problem. The patch I've just attached is suits better in Django's unit tests -- it uses middlewares in the base test class, but by being in the base class it is easier to test all existing backends.

As for changing the locmem cache backend to use the highest pickle protocol, would you like me to create a new ticket with a patch for that?

comment:3 by Raphael Kubo da Costa, 13 years ago

Cc: Raphael Kubo da Costa added

comment:4 by teolicy, 13 years ago

Cc: teolicy added
UI/UX: unset

I ran into a similar snag (caching breaks cookies, because pylibmc uses the highest protocol version).

Now, perhaps I misunderstood something along the way, but as far as I can see the solution should be to make SimpleCookie a properly serializable object.

I mean, what's so special about SimpleCookie that makes it unreasonable to ask for this code to simply work?

>>> from django.http import HttpResponse
>>> resp = HttpResponse('')
>>> resp.set_cookie('xx', 'yy')
>>> import cPickle
>>> cPickle.loads(cPickle.dumps(resp)).cookies # this is fine
<SimpleCookie: xx='yy'>
>>> cPickle.loads(cPickle.dumps(resp, 2)).cookies # this is broken
<SimpleCookie: xx='Set-Cookie: xx=yy; Path=/'>
>>>

Also, I humbly think that the ticket's Component should be HTTP Handling, not Cache, and that this is something that should be slated for 1.3.1, not 1.4.

comment:5 by Luke Plant, 13 years ago

@teolicy:

From the Python bug report, it looks like they are going to WONTFIX it. Ideally, our SimpleCookie is exactly the same as the stdlib SimpleCookie (currently it isn't with any released version of Python, but as fixes get in to Pythno stdlib it should become that). Since we don't to diverge from stdlib if we can avoid it, we'd prefer to put the fix elsewhere.

Regarding Django 1.3.1, our current policy is that we do not backport bugfixes like this to the stable branch.

comment:6 by Paul McMillan, 13 years ago

I split off the issue of locmem pickle protocol into #16378.

comment:7 by Jacob, 13 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:8 by Paul McMillan, 13 years ago

Severity: NormalRelease blocker

I'm marking this as a release blocker. It shouldn't be possible to mix and match the built in functionality we provide to silently return incorrect results.

I'm leaving it assigned to myself since I've looked at the issue already, but if anyone else wants to fix it before I get to it, please feel free to take it over.

The fix here is to serialize the SimpleCookie to a string before pickling the request for the cache.

by Luke Plant, 13 years ago

Attachment: ticket_15863.diff added

Patch (incorporating tests)

comment:9 by Luke Plant, 13 years ago

Resolution: fixed
Status: assignedclosed

In [17200]:

Fixed #15863 - SimpleCookies are not correctly serialized with the file or database cache backends

Thanks to rakuco for the report and for the tests.

comment:10 by Claude Paroz <claude@…>, 12 years ago

In 4d817b38875c900d70793acd528afc9e954bbcb7:

Fixed #19262 -- Support cookie pickling in SimpleTemplateResponse

Refs #15863.

comment:11 by Claude Paroz <claude@…>, 12 years ago

In 6554137eebe4bd10bdf3f1be21f63f0a9cffd7ff:

[1.5.x] Fixed #19262 -- Support cookie pickling in SimpleTemplateResponse

Refs #15863.
Backport of 4d817b3887 from master.

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