Cookie.py does not parse httponly or secure cookie flags #60815

jdennis · 2012-12-04T20:50:24Z

BPO	16611
Nosy	@ezio-melotti, @merwok, @bitdancer, @florentx
Files	test_cookie.py cookies-httponly-secure.diff cookies-httponly-secure-2.diff cookies-httponly-secure-3.diff cookie_flags.patch

^{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:

assignee = None
closed_at = <Date 2013-08-25.15:15:13.226>
created_at = <Date 2012-12-04.20:50:24.451>
labels = ['type-bug', 'library']
title = 'Cookie.py does not parse httponly or secure cookie flags'
updated_at = <Date 2016-07-10.18:01:11.447>
user = 'https://bugs.python.org/jdennis'

bugs.python.org fields:

activity = <Date 2016-07-10.18:01:11.447>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2013-08-25.15:15:13.226>
closer = 'r.david.murray'
components = ['Library (Lib)']
creation = <Date 2012-12-04.20:50:24.451>
creator = 'jdennis'
dependencies = []
files = ['28208', '30962', '30969', '31307', '31405']
hgrepos = []
issue_num = 16611
keywords = ['patch']
message_count = 19.0
messages = ['176957', '184112', '184114', '189252', '189642', '193274', '193298', '193317', '195297', '195811', '195812', '195815', '196106', '196107', '196108', '196140', '196142', '222085', '270112']
nosy_count = 7.0
nosy_names = ['ezio.melotti', 'eric.araujo', 'r.david.murray', 'flox', 'jdennis', 'python-dev', 'julien.phalip']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue16611'
versions = ['Python 3.3', 'Python 3.4']

jdennis · 2012-12-04T20:50:23Z

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 bpo-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)

ezio-melotti · 2013-03-13T20:56:50Z

Even though bpo-3073 has been fixed, I still see the same failures when I run the attached test_cookie.py.

jdennis · 2013-03-13T21:10:30Z

That's because bpo-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.

merwok · 2013-05-14T22:10:00Z

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.

florentx · 2013-05-20T08:47:50Z

Eric, this last one is not a bug in Cookie. This is a limitation with Python 2. See bpo-18012.

julienphalip · 2013-07-18T09:47:12Z

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!

jdennis · 2013-07-18T14:12:31Z

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?

julienphalip · 2013-07-18T19:59:55Z

Thank you for the feedback. I've updated the patch to compile the regular expression at load time.

julienphalip · 2013-08-15T22:33:55Z

Updated the patch to use a tuple instead of a list for the cookie flags.

bitdancer · 2013-08-21T18:08:52Z

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.

bitdancer · 2013-08-21T18:11:21Z

Woops, corrupted patch, and I see a bug...

bitdancer · 2013-08-21T18:19:48Z

Corrected patch.

julienphalip · 2013-08-25T00:02:50Z

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?

bitdancer · 2013-08-25T00:16:56Z

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.

julienphalip · 2013-08-25T01:32:45Z

Thanks for the clarification, that makes perfect sense!

python-dev · 2013-08-25T15:10:09Z

New changeset 0dbe45697efc by R David Murray in branch '3.3':
bpo-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 bpo-16611: BaseCookie now parses 'secure' and 'httponly' flags.
http://hg.python.org/cpython/rev/f81846c2b746

bitdancer · 2013-08-25T15:15:13Z

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.

python-dev · 2014-07-02T07:48:41Z

New changeset 0ba6ebd90b9d by Berker Peksag in branch '2.7':
Issue bpo-19870: BaseCookie now parses 'secure' and 'httponly' flags.
http://hg.python.org/cpython/rev/0ba6ebd90b9d

python-dev · 2016-07-10T18:01:11Z

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

jdennis mannequin added the stdlib Python modules in the Lib dir label Dec 4, 2012

ezio-melotti added the type-bug An unexpected behavior, bug, or error label Mar 13, 2013

bitdancer changed the title ~~multiple problems with Cookie.py~~ Cookie.py does not parse httponly or secure cookie flags Aug 21, 2013

bitdancer closed this as completed Aug 25, 2013

ezio-melotti transferred this issue from another repository Apr 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cookie.py does not parse httponly or secure cookie flags #60815

Cookie.py does not parse httponly or secure cookie flags #60815

jdennis mannequin commented Dec 4, 2012

jdennis mannequin commented Dec 4, 2012

ezio-melotti commented Mar 13, 2013

jdennis mannequin commented Mar 13, 2013

merwok commented May 14, 2013

florentx mannequin commented May 20, 2013

julienphalip mannequin commented Jul 18, 2013

jdennis mannequin commented Jul 18, 2013

julienphalip mannequin commented Jul 18, 2013

julienphalip mannequin commented Aug 15, 2013

bitdancer commented Aug 21, 2013

bitdancer commented Aug 21, 2013

bitdancer commented Aug 21, 2013

julienphalip mannequin commented Aug 25, 2013

bitdancer commented Aug 25, 2013

julienphalip mannequin commented Aug 25, 2013

python-dev mannequin commented Aug 25, 2013

bitdancer commented Aug 25, 2013

python-dev mannequin commented Jul 2, 2014

python-dev mannequin commented Jul 10, 2016

Cookie.py does not parse httponly or secure cookie flags #60815

Cookie.py does not parse httponly or secure cookie flags #60815

Comments

jdennis mannequin commented Dec 4, 2012

jdennis mannequin commented Dec 4, 2012

ezio-melotti commented Mar 13, 2013

jdennis mannequin commented Mar 13, 2013

merwok commented May 14, 2013

florentx mannequin commented May 20, 2013

julienphalip mannequin commented Jul 18, 2013

jdennis mannequin commented Jul 18, 2013

julienphalip mannequin commented Jul 18, 2013

julienphalip mannequin commented Aug 15, 2013

bitdancer commented Aug 21, 2013

bitdancer commented Aug 21, 2013

bitdancer commented Aug 21, 2013

julienphalip mannequin commented Aug 25, 2013

bitdancer commented Aug 25, 2013

julienphalip mannequin commented Aug 25, 2013

python-dev mannequin commented Aug 25, 2013

bitdancer commented Aug 25, 2013

python-dev mannequin commented Jul 2, 2014

python-dev mannequin commented Jul 10, 2016