#19039 closed Bug (fixed)
Python 3.3 fails unit test for duplicate bad cookies
Reported by: | Ian Clelland | Owned by: | nobody |
---|---|---|---|
Component: | HTTP handling | Version: | dev |
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:
- 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.
- 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"
Change History (6)
comment:1 by , 12 years ago
comment:2 by , 12 years ago
Severity: | Normal → Release blocker |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:3 by , 12 years ago
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.
comment:4 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:6 by , 12 years ago
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 ourparse_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.
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.