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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
Date: 2013-08-21 18:11 |
Woops, corrupted patch, and I see a bug...
|
msg195815 - (view) |
Author: R. David Murray (r.david.murray) *  |
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) *  |
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) *  |
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
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:39 | admin | set | github: 60815 |
2016-07-10 18:01:11 | python-dev | set | messages:
+ msg270112 |
2014-07-02 07:48:41 | python-dev | set | messages:
+ msg222085 |
2013-08-25 15:15:13 | r.david.murray | set | status: open -> closed resolution: fixed messages:
+ msg196142
stage: patch review -> resolved |
2013-08-25 15:10:09 | python-dev | set | nosy:
+ python-dev messages:
+ msg196140
|
2013-08-25 01:32:45 | julien.phalip | set | messages:
+ msg196108 |
2013-08-25 00:16:56 | r.david.murray | set | messages:
+ msg196107 |
2013-08-25 00:02:50 | julien.phalip | set | messages:
+ msg196106 |
2013-08-21 18:19:48 | r.david.murray | set | files:
+ cookie_flags.patch
messages:
+ msg195815 |
2013-08-21 18:11:41 | r.david.murray | set | files:
- cookie_flags.patch |
2013-08-21 18:11:20 | r.david.murray | set | messages:
+ msg195812 |
2013-08-21 18:08:53 | r.david.murray | set | files:
+ 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:56 | julien.phalip | set | files:
+ cookies-httponly-secure-3.diff
messages:
+ msg195297 |
2013-07-18 19:59:55 | julien.phalip | set | files:
+ cookies-httponly-secure-2.diff
messages:
+ msg193317 |
2013-07-18 14:12:31 | jdennis | set | messages:
+ msg193298 |
2013-07-18 09:47:13 | julien.phalip | set | files:
+ cookies-httponly-secure.diff versions:
- Python 2.7 nosy:
+ julien.phalip
messages:
+ msg193274
keywords:
+ patch |
2013-05-20 08:47:50 | flox | set | nosy:
+ flox messages:
+ msg189642
|
2013-05-14 22:10:00 | eric.araujo | set | nosy:
+ eric.araujo messages:
+ msg189252
|
2013-03-13 21:10:30 | jdennis | set | messages:
+ msg184114 |
2013-03-13 20:56:50 | ezio.melotti | set | nosy:
+ ezio.melotti, r.david.murray messages:
+ msg184112
type: behavior stage: needs patch |
2012-12-04 20:50:24 | jdennis | create | |