classification
Title: Fixing fractional expiry time bug in cookiejar
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.4, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, demian.brecht, orsenthil, python-dev, r.david.murray, rbcollins, serhiy.storchaka, ssh
Priority: normal Keywords: patch

Created on 2015-04-08 14:17 by ssh, last changed 2015-08-03 22:11 by rbcollins. This issue is now closed.

Files
File name Uploaded Description Edit
mywork.patch ssh, 2015-04-08 14:17 Removes the fractional part of the expiry time review
mywork.patch ssh, 2015-05-07 02:12 review
mywork.patch ssh, 2015-05-07 21:23 review
Messages (11)
msg240265 - (view) Author: (ssh) * Date: 2015-04-08 14:17
If the FileCookieJar reads a cookie whose expiry time is a decimal fraction, it crashes.

Chrome extensions "cookies.txt" and "EdiThisCookie" export the expiry time as decimal fractions. This is accepted by wget and curl, but not by the FileCookieJar which ends up crashing.

I made a StackOverflow question checking if fractional decimal expiry times were even allowed (if it was a bug in the extensions), but didn't get a response: https://stackoverflow.com/questions/29502672/can-the-cookie-expires-field-be-a-decimal-value

At any rate, this patch should make the library more robust.
msg240907 - (view) Author: Demian Brecht (demian.brecht) * (Python triager) Date: 2015-04-14 15:28
Although it's likely not going to be a high frequency issue (but would be painful to track down when it does become an issue), by doing an int(float(expires)), you're losing the sub-second portion of the expiry. Why not something like this:

try:
    expires = int(expires)
except ValueError:
    expires = int(float(expires) * 1e6)

Looking at the values in the Chrome sqlite3 database, it seems like the above is what they're storing (INTEGER datatype).

How are wget and curl treating the fractional part? I'd be really surprised if they were ignoring it as in the proposed patch.
msg240909 - (view) Author: Demian Brecht (demian.brecht) * (Python triager) Date: 2015-04-14 15:30
Changing to "behavior" type as crash is generally used to indicate an interpreter issue.
msg242674 - (view) Author: (ssh) * Date: 2015-05-06 14:22
Wouldn't int(float(expires) * 1e6) set the date much further in the future? I'm not sure why you'd do that unless the plan is to change the internal time unit to microseconds (which seems like a much bigger change, and overkill for handling this special case). Cookie strings operate at the second granularity, so I'm not sure if the sub-second precision is required.

I took a quick look at curl's code and test cases, and they use a time_t structure which doesn't have subsecond precision. Fractional time is not a part of their test cases.

https://github.com/bagder/curl/blob/6f8046f7a4bd3d6edcc53c2eec936105ec424d54/tests/libtest/lib517.c

https://github.com/bagder/curl/blob/664b9baf67c2c22ebaf3606298ca9c4ce0b382d2/lib/parsedate.c#L331

Wget also appears to do something similar:

http://bzr.savannah.gnu.org/lh/wget/trunk/annotate/head:/src/cookies.c#L387
msg242694 - (view) Author: Demian Brecht (demian.brecht) * (Python triager) Date: 2015-05-06 17:55
Thanks for the follow up on that. In light of your findings, I agree with the path you've taken in your patch (and yeah, you'd have to deal with conversions in the output if you were to take my suggestion). I've left a couple minor comments in Reitveld. Other than those, the patch LGTM.
msg242708 - (view) Author: (ssh) * Date: 2015-05-07 02:12
Attaching patch after addressing comments in code review.
msg242720 - (view) Author: Demian Brecht (demian.brecht) * (Python triager) Date: 2015-05-07 16:48
I left a small comment around indentation in Rietveld.

Also, for the sake of completeness (and for others taking part in this review/commit), I dug through the relevant RFCs and none of them seem to define time as having sub-section resolution.
msg242735 - (view) Author: (ssh) * Date: 2015-05-07 21:23
Thanks for checking in the RFC. I had done that before I posted my StackOverflow question, but should have mentioned it here for completeness. I've addressed the comments.
msg242777 - (view) Author: Demian Brecht (demian.brecht) * (Python triager) Date: 2015-05-08 17:27
LGTM, thanks for the patch!
msg247956 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-08-03 22:07
New changeset 1c1c508d1b98 by Robert Collins in branch '3.4':
Issue #23888: Handle fractional time in cookie expiry. Patch by ssh.
https://hg.python.org/cpython/rev/1c1c508d1b98

New changeset 06406205ae99 by Robert Collins in branch '3.5':
Issue #23888: Handle fractional time in cookie expiry. Patch by ssh.
https://hg.python.org/cpython/rev/06406205ae99

New changeset 73902903d397 by Robert Collins in branch 'default':
Issue #23888: Handle fractional time in cookie expiry. Patch by ssh.
https://hg.python.org/cpython/rev/73902903d397
msg247957 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-08-03 22:11
Thanks for the patch.
History
Date User Action Args
2015-08-03 22:11:50rbcollinssetstatus: open -> closed

nosy: + rbcollins
messages: + msg247957

resolution: fixed
stage: commit review -> resolved
2015-08-03 22:07:58python-devsetnosy: + python-dev
messages: + msg247956
2015-08-03 22:02:08rbcollinssetversions: + Python 3.4, Python 3.6
2015-05-08 17:27:42demian.brechtsetnosy: + r.david.murray

messages: + msg242777
stage: patch review -> commit review
2015-05-08 04:45:51berker.peksagsetnosy: + berker.peksag
2015-05-07 21:23:59sshsetfiles: + mywork.patch

messages: + msg242735
2015-05-07 16:48:29demian.brechtsetmessages: + msg242720
2015-05-07 02:12:16sshsetfiles: + mywork.patch

messages: + msg242708
2015-05-06 17:55:12demian.brechtsetmessages: + msg242694
2015-05-06 14:22:27sshsetmessages: + msg242674
2015-04-14 15:30:09demian.brechtsetnosy: + orsenthil
messages: + msg240909

type: crash -> behavior
stage: patch review
2015-04-14 15:28:55demian.brechtsetmessages: + msg240907
2015-04-08 14:57:06demian.brechtsetnosy: + demian.brecht
2015-04-08 14:17:39sshcreate