Code

Opened 19 months ago

Closed 18 months ago

Last modified 18 months ago

#19039 closed Bug (fixed)

Python 3.3 fails unit test for duplicate bad cookies

Reported by: clelland Owned by: nobody
Component: HTTP handling Version: master
Severity: Release blocker Keywords: cookie, python3.3
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In #15852, a user was encountering problems with a cookie that contained multiple copies of a morsel with a ":" character in it, and the Django-patched SimpleCookie class was throwing an error.

The solution to that problem was to further patch SimpleCookie to keep track of the bad morsel properly and not throw an exception. A unit test was put in place, which used "," as a bad-morsel-triggering character.

Unfortunately, two things have happened with Python 3.3 (Both of these should be good things, but they end up with an awkward regression):

First, the Python standard library version of http.cookie.SimpleCookie passes our tests, so Django doesn't try to patch it. This means that the patch put in place does not run, and the unit test fails.

Second, ':' is no longer considered an invalid character in cookies (see http://bugs.python.org/issue2193 for the four-year-long discussion). While this is good for the original problem -- those cookies can now be handled properly -- the patch actually changes the behaviour for other invalid characters, and people may be relying on that behaviour. (Is this covered by the backwards-incompatibility guarantee?)

I see a couple ways out of this -- we could:

  1. Check in django.http whether SimpleCookie skips bad morsels, and patch SimpleCookie if it doesn't. Practically, I think this means that we'll be patching SimpleCookie forever, unless we can convince the Python developers that it's a bug.
  1. Change the unit test to only make sure that it handles duplicate keys with ":" properly, like the original user needed, possibly leaving other the handling of other invalid cookies in an undetermined state (https://code.djangoproject.com/ticket/15852#comment:1 seems to claim that the Python handling of invalid cookies is acceptable)

or 3. Delete the unit test -- it advertises that we handle bad cookie morsels in a graceful way, which we definitely do not do if we use the built-in SimpleCookie. We call this a bug in Python, and just declare that "it's just how Python SimpleCookie works"

Attachments (0)

Change History (6)

comment:1 Changed 19 months ago by clelland

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Option 4: Mark the unit test as an expected failure in Python 3.3+. Add a new unit test for the original problem (with colons), which should pass everywhere.

comment:2 Changed 18 months ago by claudep

  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 18 months ago by lukeplant

First, we should fix the test test_repeated_nonstandard_keys to use the values in #15852 i.e. a colon not a comma, so that it will pass everywhere. We regard this as a bug in the test - it should never have used a comma. If people were relying on that (unlikely), they were relying on a bug.

Second, we should file a bug against Python 3.3. I think this is a bug, since it renders SimpleCookie useless for parsing cookies from untrusted sources, and all cookies come from untrusted sources (the client, and often untrusted subdomains).

Then, if and only if we get a bug report about failing to handle commas or other characters in cookie names, we patch our SimpleCookie for this case. There is no point us fixing bugs that no-one is encountering in real life. I suspect you'll find that browsers do not accept comma in the cookie name, so they won't be sending cookies like that. People could send them manually to be perverse, but they don't harm anyone but themselves.

Last edited 18 months ago by lukeplant (previous) (diff)

comment:4 Changed 18 months ago by Luke Plant <L.Plant.98@…>

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

In 71734dfa72140f90c71e199c3a63f47024636df9:

Fixed #19039 - Python 3.3 fails unit test for duplicate bad cookies

Thanks to clelland for the report.

comment:5 Changed 18 months ago by Luke Plant <L.Plant.98@…>

In c426d5665095e7b91df69e54d4773180b12ff548:

[1.5.x] Fixed #19039 - Python 3.3 fails unit test for duplicate bad cookies

Thanks to clelland for the report.

Backport of 71734dfa72140f90c71e199c3a63f47024636df9 from master.

comment:6 Changed 18 months ago by lukeplant

Further to last comments:

  • If SimpleCookie can't parse cookies with commas in names, it is arguably behaving correctly, and isn't going to cause any problems in the real world since browsers don't send cookies with commas in names.
  • If it throws a CookieError exception, that doesn't hurt us since our parse_cookie catches that and returns an empty dictionary.

So with the current solution, we shouldn't have further problems, and we don't need to file a bug against Python.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.