classification
Title: Regression in Python 3.2 cookie parsing
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.2
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, Tim.Graham, berker.peksag, floppymaster, georg.brandl, pitrou, python-dev, r.david.murray
Priority: normal Keywords: patch

Created on 2014-10-29 10:14 by Tim.Graham, last changed 2016-07-14 03:37 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
failing-django-tests-22758.txt Tim.Graham, 2014-10-29 10:40
cookie-pickling-fix.diff georg.brandl, 2014-10-29 11:30 review
secure-httponly-3.2-backport.diff Tim.Graham, 2014-11-04 13:26 review
secure-httponly-3.2-backport.diff Tim.Graham, 2015-03-20 19:18 review
secure-httponly-3.2-backport.diff Tim.Graham, 2015-05-27 01:02 review
Messages (22)
msg230200 - (view) Author: Tim Graham (Tim.Graham) * Date: 2014-10-29 10:14
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]
msg230201 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-29 10:20
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).
msg230202 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-29 10:21
Can you give a pointer to the failing Django test, by the way?
msg230203 - (view) Author: Tim Graham (Tim.Graham) * Date: 2014-10-29 10:40
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.
msg230204 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-29 10:44
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.
msg230205 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-10-29 11:30
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).
msg230241 - (view) Author: Tim Graham (Tim.Graham) * Date: 2014-10-29 20:19
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).
msg230243 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-10-29 20:47
Thanks, this is indeed a regression.
msg230363 - (view) Author: Tim Graham (Tim.Graham) * Date: 2014-10-31 18:10
FYI, I created #22775 and submitted a patch for the issue that SimpleCookie doesn't pickle properly with HIGHEST_PROTOCOL.
msg230572 - (view) Author: Tim Graham (Tim.Graham) * Date: 2014-11-04 02:14
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?
msg230586 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-11-04 07:47
That seems like the best course of action.
msg230619 - (view) Author: Tim Graham (Tim.Graham) * Date: 2014-11-04 13:26
The patch from #16611 applies cleanly to 3.2. I added a mention in Misc/NEWS and confirmed that all tests pass.
msg230638 - (view) Author: Tim Graham (Tim.Graham) * Date: 2014-11-04 16:48
I also created #22796 for the lax parsing issue.
msg238714 - (view) Author: Tim Graham (Tim.Graham) * Date: 2015-03-20 19:18
Patch updated to fix conflict in NEWS. Could we have it committed to ensure it gets fixed in the next 3.2 released?
msg244141 - (view) Author: Tim Graham (Tim.Graham) * Date: 2015-05-27 01:03
Patch rebased again after cookie fix from #22931.
msg256062 - (view) Author: Tim Graham (Tim.Graham) * Date: 2015-12-07 13:44
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.
msg261882 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-03-17 03:03
I will commit this to the 3.2 branch today.
msg270109 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-07-10 17:48
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)?
msg270113 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-10 18:01
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
msg270116 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-07-10 18:03
Oops.  I guess there's no commit hook after all.
msg270119 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-10 18:13
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
msg270358 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-14 03:37
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
History
Date User Action Args
2016-07-14 03:37:23python-devsetmessages: + msg270358
2016-07-10 18:13:24python-devsetmessages: + msg270119
2016-07-10 18:03:25r.david.murraysetstatus: open -> closed
resolution: fixed
messages: + msg270116

stage: commit review -> resolved
2016-07-10 18:01:11python-devsetnosy: + python-dev
messages: + msg270113
2016-07-10 17:48:24r.david.murraysetresolution: wont fix -> (no value)
messages: + msg270109
2016-03-17 03:03:19berker.peksagsetstatus: closed -> open

messages: + msg261882
2015-12-07 13:44:29Tim.Grahamsetstatus: open -> closed
resolution: wont fix
messages: + msg256062
2015-09-02 07:48:32berker.peksagsetstage: commit review
2015-05-27 01:03:39Tim.Grahamsetmessages: + msg244141
2015-05-27 01:02:41Tim.Grahamsetfiles: + secure-httponly-3.2-backport.diff
2015-03-20 19:18:40Tim.Grahamsetfiles: + secure-httponly-3.2-backport.diff

messages: + msg238714
2014-12-26 19:56:34floppymastersetnosy: + floppymaster
2014-11-04 16:48:35Tim.Grahamsetmessages: + msg230638
2014-11-04 13:26:24Tim.Grahamsetfiles: + secure-httponly-3.2-backport.diff

messages: + msg230619
2014-11-04 07:47:08georg.brandlsetmessages: + msg230586
2014-11-04 02:14:45Tim.Grahamsetmessages: + msg230572
2014-10-31 18:21:14Arfreversetnosy: + Arfrever
2014-10-31 18:10:59Tim.Grahamsetmessages: + msg230363
2014-10-29 20:47:06georg.brandlsetmessages: + msg230243
2014-10-29 20:19:28Tim.Grahamsetmessages: + msg230241
2014-10-29 11:30:16georg.brandlsetfiles: + cookie-pickling-fix.diff
keywords: + patch
messages: + msg230205
2014-10-29 10:44:59pitrousetmessages: + msg230204
2014-10-29 10:40:57Tim.Grahamsetfiles: + failing-django-tests-22758.txt

messages: + msg230203
2014-10-29 10:21:20pitrousetmessages: + msg230202
2014-10-29 10:20:34pitrousetmessages: + msg230201
2014-10-29 10:14:37Tim.Grahamcreate