New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Regression in Python 3.2 cookie parsing #66947
Comments
I noticed some failing Django tests on Python 3.2.6 the other day. The regression is caused by this change: 572d9c5 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 b92e104 (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:
c = SimpleCookie("Set-Cookie: foo=bar; Path=/")
print(c) Here are the results: Python 2.7.8: Python 3.5.0: Python 3.4.2: Python 3.3.6: 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 572d9c5 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: 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:
(The change for bpo-16611 reintroduces "lax" parsing behavior that the security fix was supposed to prevent.)
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 bpo-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 bpo-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 bpo-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. |
I also created bpo-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 bpo-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': |
Oops. I guess there's no commit hook after all. |
New changeset 1c07bd735282 by R David Murray in branch '3.3': New changeset 5b712993dce5 by R David Murray in branch '3.4': New changeset 26342c9e8c1d by R David Murray in branch '3.5': New changeset ce140ed0a56c by R David Murray in branch 'default': |
New changeset a0bf31e50da5 by Martin Panter in branch '3.2': |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: