homepage

I noticed some failing Django tests on Python 3.2.6 the other day. The regression is caused by this change: https://github.com/python/cpython/commit/572d9c59a1441c6f8ffb9308824c804856020e31
Behavior before that commit (and on other version of Python even after that commit):
>>> from http.cookies import SimpleCookie
>>> SimpleCookie("Set-Cookie: foo=bar; Path=/")
<SimpleCookie: foo='bar'>
New broken behavior on Python 3.2.6:
>>> from http.cookies import SimpleCookie
>>> SimpleCookie("Set-Cookie: foo=bar; Path=/")
<SimpleCookie: >
Python 3.2.6 no longer accepts the "Set-Cookie: " prefix to BaseCookie.load:
>>> SimpleCookie("Set-Cookie: foo=bar; Path=/")
<SimpleCookie: >
>>> SimpleCookie("foo=bar; Path=/")
<SimpleCookie: foo='bar'>
This issue doesn't affect 2.7, 3.3, or 3.4 because of https://github.com/python/cpython/commit/b92e104dc57c37ddf22ada1c6e5380e09268ee93 (this commit wasn't backported to 3.2 because that branch is in security-fix-only mode).
I asked Berker about this and he suggested to create this issue and said, "If Georg is OK to backout the commit I can write a patch with additional test cases and commit it."
He also confirmed the regression as follows:
I've tested your example on Python 2.7.8, 3.2.6, 3.3.6, 3.4.2, 3.5.0 (all unreleased development versions - they will be X.Y.Z+1) and looks like it's a regression.
My test script is:
 try:
 from http.cookies import SimpleCookie
 except ImportError:
 from Cookie import SimpleCookie
 c = SimpleCookie("Set-Cookie: foo=bar; Path=/")
 print(c)
Here are the results:
Python 2.7.8:
Set-Cookie: foo=bar; Path=/
Python 3.5.0:
Set-Cookie: foo=bar; Path=/
Python 3.4.2:
Set-Cookie: foo=bar; Path=/
Python 3.3.6:
Set-Cookie: foo=bar; Path=/
[45602 refs]
Python 3.2.6:
[38937 refs]

Is it a normal use of SimpleCookie? The docs don't seem to imply it:
"""
>>> C = cookies.SimpleCookie()
>>> C.load("chips=ahoy; vienna=finger") # load from a string (HTTP header)
"""
In any case, it's up to Georg to decide. But changeset 572d9c59a1441c6f8ffb9308824c804856020e31 fixes a security issue reported to security@python.org (the report included a concrete example of how to exploit it under certain conditions).

Can you give a pointer to the failing Django test, by the way?

I wasn't sure if it was expected behavior or not. I'm attaching a file with the list of failing tests on Django's master.
Perhaps more useful is a reference to the problematic usage in Django:
https://github.com/django/django/blob/349471eeb9a4db2f5d8e95cb6555e7b3f2c94e3f/django/http/response.py#L208-L217
That logic was added to fix https://code.djangoproject.com/ticket/15863.

Ah, so it's about round-tripping between SimpleCookie.__str__() and SimpleCookie.__init__(). That sounds like a reasonable behaviour to preserve (and easier than parsing arbitrary Set-Cookie headers). IMO we should also add for tests for it in other versions.

OK, so there are two root issues here:
* Django uses __init__(str()) roundtripping, which is not explicitly supported by the library, and worked by accident with previous versions. That it works again with 3.3+ is another accident, and a bug.
(The change for #16611 reintroduces "lax" parsing behavior that the security fix was supposed to prevent.)
* BaseCookie doesn't roundtrip correctly when pickled with protocol >= 2. This should be fixed in upcoming bugfix releases.
I would advise Django to subclass SimpleCookie and fix the pickling issue, which is not hard (see attached diff).

Thank-you Georg; I believe I was able to fix some of the failures by patching Django as you suggested.
However, I think I found another issue due to #16611 (support for httponly/secure cookies) not being backported to Python 3.2. The issue is that any cookies that appear after one that uses httponly or secure are dropped:
>>> from http.cookies import SimpleCookie
>>> c = SimpleCookie()
>>> c['a'] = 'b'
>>> c['a']['httponly'] = True
>>> c['d'] = 'e'
>>> out = c.output(header='', sep='; ')
>>> SimpleCookie(out)
<SimpleCookie: a='b'>
Here's another example using the 'domain' option to show the same flow from above working as expected:
>>> c = SimpleCookie()
>>> c['a'] = 'b'
>>> c['a']['domain'] = 'foo.com'
>>> c['d'] = 'e'
>>> out = c.output(header='', sep='; ')
>>> SimpleCookie(out)
<SimpleCookie: a='b' d='e'>
It seems to me this may warrant backporting httponly/secure support to Python 3.2 now that cookie parsing is more strict (unless Django is again relying on incorrect behavior).

Thanks, this is indeed a regression.

FYI, I created #22775 and submitted a patch for the issue that SimpleCookie doesn't pickle properly with HIGHEST_PROTOCOL.

Georg, how do want to proceed with this issue? Should we backport #16611 (support for parsing secure/httponly flag) to 3.2 to fix this regression and then create a separate issue to fix the lax parsing issue on all versions?

That seems like the best course of action.

The patch from #16611 applies cleanly to 3.2. I added a mention in Misc/NEWS and confirmed that all tests pass.

I also created #22796 for the lax parsing issue.

Patch updated to fix conflict in NEWS. Could we have it committed to ensure it gets fixed in the next 3.2 released?

Patch rebased again after cookie fix from #22931.

Given the inactivity here, I guess the patch won't be applied before Python 3.2 is end-of-life so I'm going to close the ticket.

I will commit this to the 3.2 branch today.

My understanding is that there is a commit hook that prevents pushing to the 3.2 branch, so that Georg needs to do this. I've applied the patch and run the tests myself, and agree that it passes (as in, none of the test failures I see are related to cookies). This isn't set to release blocker...should it be (ie: since this is the last release do we want to make sure it gets in)?

New changeset d22fadc18d01 by R David Murray in branch '3.2':
#22758: fix regression in handling of secure cookies.
https://hg.python.org/cpython/rev/d22fadc18d01 

Oops. I guess there's no commit hook after all.

New changeset 1c07bd735282 by R David Murray in branch '3.3':
#22758 null merge
https://hg.python.org/cpython/rev/1c07bd735282
New changeset 5b712993dce5 by R David Murray in branch '3.4':
#22758 null merge
https://hg.python.org/cpython/rev/5b712993dce5
New changeset 26342c9e8c1d by R David Murray in branch '3.5':
#22758 null merge
https://hg.python.org/cpython/rev/26342c9e8c1d
New changeset ce140ed0a56c by R David Murray in branch 'default':
#22758 null merge
https://hg.python.org/cpython/rev/ce140ed0a56c 

New changeset a0bf31e50da5 by Martin Panter in branch '3.2':
Issue #22758: Move NEWS entry to Library section
https://hg.python.org/cpython/rev/a0bf31e50da5