classification
Title: Cookie.py does not parse httponly or secure cookie flags
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eric.araujo, ezio.melotti, flox, jdennis, julien.phalip, python-dev, r.david.murray
Priority: normal Keywords: patch

Created on 2012-12-04 20:50 by jdennis, last changed 2016-07-10 18:01 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
test_cookie.py jdennis, 2012-12-04 20:50
cookies-httponly-secure.diff julien.phalip, 2013-07-18 09:47 review
cookies-httponly-secure-2.diff julien.phalip, 2013-07-18 19:59 review
cookies-httponly-secure-3.diff julien.phalip, 2013-08-15 22:33 review
cookie_flags.patch r.david.murray, 2013-08-21 18:19 review
Messages (19)
msg176957 - (view) Author: John Dennis (jdennis) Date: 2012-12-04 20:50
There are multiple problems with Cookie.py. Some of the issues are covered in http://bugs.python.org/issue3073 which is still open (after 4.5 years).

In all honesty the API and the implementation are not great perhaps the best thing would be to remove it from the core libraries, however you can't remove a core library. There is cookielib.py is which is pretty good however cookielib.py is tightly coupled to urllib2 and if you're not using urllib2 you can't use cookielib.py so you're stuck using Cookie.py which means the best thing is to get the bugs in Cookie.py fixed.

Of the problems illustrated in the attached unittest (test_cookie.py) the absolute must fix issues are the inability to parse an Expires attribute and the impossibility of testing the HttpOnly & Secure flags for a truth value after parsing. Those are critical because it makes using Cookie.py impossible. The other errors would be nice to get fixed, but not as critical. Next in importance would be respecting the truth value when setting the HttpOnly & Secure flags. Failing to detect an improperly formatted cookie when parsing is the least important because hopefully you won't have improperly formatted cookies (unfortunately a weak assumption)

Note: the HttpOnly and Secure issues are symmetrical, they both suffer the same problems because they're both boolean flags whose True value is asserted by the flag's presence and it's False value by it's absence.
 
Cookie parsing problems:

* Cannot read a properly formatted Expires attribute (see also issue 3073)

* Impossible to determine state of HttpOnly boolean flag after parsing

* Impossible to determine state of Secure boolean flag after parsing

* Fails to raise any errors when parsing invalid cookie strings

Cookie creation/initialization problems:

* Setting HttpOnly flag to a value which evaluates to False results in the flag being set to True (there is no check whatsoever on the value).

* Setting Secure flag to a value which evaluates to False results in the flag being set to True (there is no check whatsoever on the value).

Attached is a unittest illustrating the problems (more details are in the unittest).

python test_cookie.py
FF.FFFFFF...F
----------------------------------------------------------------------
Ran 13 tests in 0.003s

FAILED (failures=9)
msg184112 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2013-03-13 20:56
Even though #3073 has been fixed, I still see the same failures when I run the attached test_cookie.py.
msg184114 - (view) Author: John Dennis (jdennis) Date: 2013-03-13 21:10
That's because #3073 never addressed the core problems, so yes I would expect you would see failures. The point of the attached test is to illustrate the deficiencies in Cookie.py, so apparently it's doing it's job :-)

FWIW, we wrote a new cookie library to get around the inherent problems in Cookie.py and cookielib. We could contribute it if there is interest.
msg189252 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2013-05-14 22:10
I have just been bitten by a bug (haven’t checked if it’s covered by the added tests) where Cookies uses string.translate incorrectly:

  File "/usr/lib/python2.7/Cookie.py", line 323, in _quote
    if "" == translate(str, idmap, LegalChars):
  File "/usr/lib/python2.7/string.py", line 493, in translate
    return s.translate(table, deletions)
TypeError: translate() takes exactly one argument (2 given)

The state of the Cookie module is quite embarrassing.
msg189642 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2013-05-20 08:47
Eric, this last one is not a bug in Cookie. This is a limitation with Python 2. See #18012.
msg193274 - (view) Author: Julien Phalip (julien.phalip) Date: 2013-07-18 09:47
I'm attaching a suggested patch to fix the issues relating to serializing/deserializing the httponly and secure flags. The main idea is that for a flag to be active, it needs to both be set and have the True value.

I think this is a much more correct and saner approach than the current implementation. As it's been discussed previously, currently the httponly and secure flag are systematically given the empty string as default value when instantiating a Morsel object. So one would infer that the empty string means that the flags are inactive. However, when deserializing a Morsel object, the empty string is used to indicate that a flag is active. Both behaviors contradict each other.

While the suggested change is backwards-incompatible, it would break the code of developers relying on an inconsistent behavior. So perhaps this might be compelling enough to allow breaking backwards compatibility in this case.

Let me know what you think. Thanks!
msg193298 - (view) Author: John Dennis (jdennis) Date: 2013-07-18 14:12
I think your basic approach is fine and it's O.K. to break backwards compatibility. I'm not sure anyone was using the httponly and secure flags in the past because it was so broken, the logic was completely contradictory, so backwards compatibility should not trump fixing obviously broken logic.

I didn't review your patch carefully, just a really quick glance. The one thing that did pop out was compiling the static regexp every time the function is called. Why not make it a module or class object and compile once at load time?
msg193317 - (view) Author: Julien Phalip (julien.phalip) Date: 2013-07-18 19:59
Thank you for the feedback. I've updated the patch to compile the regular expression at load time.
msg195297 - (view) Author: Julien Phalip (julien.phalip) Date: 2013-08-15 22:33
Updated the patch to use a tuple instead of a list for the cookie flags.
msg195811 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-08-21 18:08
Backward compatibility is a concern.  However, having the flags test true when present, which they don't currently, should be considered a bug fix, I think.

So, I added a some tests to confirm the current behavior, and wrote a new patch that *just* fixes the 'test true if present' bug, leaving the other behaviors unchanged.  Reviews appreciated.  And if anyone can think of tests I missed, please let me know; I'm a little nervous about the regex change (I hate dealing with regexes :)

This issue was originally about other bugs as well, but I'm changing the title to reflect just this fix.  If there are still other remaining bugs, please open a new issues for them.

Thanks for the patch, Julien.  Although I didn't use your code, it did inform my work.
msg195812 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-08-21 18:11
Woops, corrupted patch, and I see a bug...
msg195815 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-08-21 18:19
Corrected patch.
msg196106 - (view) Author: Julien Phalip (julien.phalip) Date: 2013-08-25 00:02
Thanks for the review and new patch, David! Your approach makes sense and the patch looks good to me.

However, regarding backwards-compatibility, is that really a concern?

Currently the deserialization process systematically 1) Adds the 'httponly' and 'secure' dict keys to the cookie object and 2) Puts an empty string value for those keys, regardless of whether those flags are present or not in the loaded string. So currently nobody's code could possibly rely on any particular state or behavior in the cookie object to determine if those flags were originally present in the loaded string.

I guess I'm wondering what could possibly break in people's code if we now implemented a fully logical fix for this. What do you think?
msg196107 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-08-25 00:16
I think a fully logical fix could be implemented for 3.4 (after I commit this), because as you say it is *unlikely* that anyone is relying on the behaviors mentioned as "backward compatibility issues" in the added tests.  However, it is *possible* someone is dealing with a quirky situation where some other broken software is using these flags in key/value form, and depending on it.  Not likely, no (especially since the value would be lost on re-serialization), but since we *can* fix the issue without risking breaking such code, we should, for the maintenance release.
msg196108 - (view) Author: Julien Phalip (julien.phalip) Date: 2013-08-25 01:32
Thanks for the clarification, that makes perfect sense!
msg196140 - (view) Author: Roundup Robot (python-dev) Date: 2013-08-25 15:10
New changeset 0dbe45697efc by R David Murray in branch '3.3':
#16611: BaseCookie now parses 'secure' and 'httponly' flags.
http://hg.python.org/cpython/rev/0dbe45697efc

New changeset f81846c2b746 by R David Murray in branch 'default':
Merge #16611: BaseCookie now parses 'secure' and 'httponly' flags.
http://hg.python.org/cpython/rev/f81846c2b746
msg196142 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-08-25 15:15
Fix committed.  Thanks, Julien.

If you want to propose a new patch the makes the behavior more consistent/useful with respect to what browsers and servers actually do, feel free to propose one.  I'm going to close this issue, though.
msg222085 - (view) Author: Roundup Robot (python-dev) Date: 2014-07-02 07:48
New changeset 0ba6ebd90b9d by Berker Peksag in branch '2.7':
Issue #19870: BaseCookie now parses 'secure' and 'httponly' flags.
http://hg.python.org/cpython/rev/0ba6ebd90b9d
msg270112 - (view) Author: Roundup Robot (python-dev) 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
History
Date User Action Args
2016-07-10 18:01:11python-devsetmessages: + msg270112
2014-07-02 07:48:41python-devsetmessages: + msg222085
2013-08-25 15:15:13r.david.murraysetstatus: open -> closed
resolution: fixed
messages: + msg196142

stage: patch review -> resolved
2013-08-25 15:10:09python-devsetnosy: + python-dev
messages: + msg196140
2013-08-25 01:32:45julien.phalipsetmessages: + msg196108
2013-08-25 00:16:56r.david.murraysetmessages: + msg196107
2013-08-25 00:02:50julien.phalipsetmessages: + msg196106
2013-08-21 18:19:48r.david.murraysetfiles: + cookie_flags.patch

messages: + msg195815
2013-08-21 18:11:41r.david.murraysetfiles: - cookie_flags.patch
2013-08-21 18:11:20r.david.murraysetmessages: + msg195812
2013-08-21 18:08:53r.david.murraysetfiles: + cookie_flags.patch
versions: + Python 3.3, Python 3.4
title: multiple problems with Cookie.py -> Cookie.py does not parse httponly or secure cookie flags
messages: + msg195811

stage: needs patch -> patch review
2013-08-15 22:33:56julien.phalipsetfiles: + cookies-httponly-secure-3.diff

messages: + msg195297
2013-07-18 19:59:55julien.phalipsetfiles: + cookies-httponly-secure-2.diff

messages: + msg193317
2013-07-18 14:12:31jdennissetmessages: + msg193298
2013-07-18 09:47:13julien.phalipsetfiles: + cookies-httponly-secure.diff
versions: - Python 2.7
nosy: + julien.phalip

messages: + msg193274

keywords: + patch
2013-05-20 08:47:50floxsetnosy: + flox
messages: + msg189642
2013-05-14 22:10:00eric.araujosetnosy: + eric.araujo
messages: + msg189252
2013-03-13 21:10:30jdennissetmessages: + msg184114
2013-03-13 20:56:50ezio.melottisetnosy: + ezio.melotti, r.david.murray
messages: + msg184112

type: behavior
stage: needs patch
2012-12-04 20:50:24jdenniscreate