Python 3.3 fails unit test for duplicate bad cookies

[Ian Clelland]

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.

2. 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"

[Ian Clelland]

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.

[Luke Plant]

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.

[Luke Plant <L.Plant.98@…>]

In 71734dfa72140f90c71e199c3a63f47024636df9:

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

Thanks to clelland for the report.

[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.

[Luke Plant]

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.