Fixing fractional expiry time bug in cookiejar

[ssh]

BPO	23888
Nosy	@orsenthil, @rbtcollins, @bitdancer, @berkerpeksag, @serhiy-storchaka, @demianbrecht
Files	mywork.patch: Removes the fractional part of the expiry time mywork.patch mywork.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 2015-08-03.22:11:50.312>
created_at = <Date 2015-04-08.14:17:39.300>
labels = ['type-bug', 'library']
title = 'Fixing fractional expiry time bug in cookiejar'
updated_at = <Date 2015-08-03.22:11:50.310>
user = 'https://bugs.python.org/ssh'

bugs.python.org fields:

activity = <Date 2015-08-03.22:11:50.310>
actor = 'rbcollins'
assignee = 'none'
closed = True
closed_date = <Date 2015-08-03.22:11:50.312>
closer = 'rbcollins'
components = ['Library (Lib)']
creation = <Date 2015-04-08.14:17:39.300>
creator = 'ssh'
dependencies = []
files = ['38864', '39311', '39315']
hgrepos = []
issue_num = 23888
keywords = ['patch']
message_count = 11.0
messages = ['240265', '240907', '240909', '242674', '242694', '242708', '242720', '242735', '242777', '247956', '247957']
nosy_count = 8.0
nosy_names = ['orsenthil', 'rbcollins', 'r.david.murray', 'python-dev', 'berker.peksag', 'serhiy.storchaka', 'demian.brecht', 'ssh']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue23888'
versions = ['Python 3.4', 'Python 3.5', 'Python 3.6']

[ssh]

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.

[demianbrecht]

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.

[demianbrecht]

Changing to "behavior" type as crash is generally used to indicate an interpreter issue.

[ssh]

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

[demianbrecht]

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.

[ssh]

Attaching patch after addressing comments in code review.

[demianbrecht]

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.

[ssh]

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.

[demianbrecht]

LGTM, thanks for the patch!

[python-dev]

New changeset 1c1c508d1b98 by Robert Collins in branch '3.4':
Issue bpo-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 bpo-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 bpo-23888: Handle fractional time in cookie expiry. Patch by ssh.
https://hg.python.org/cpython/rev/73902903d397

[rbtcollins]

Thanks for the patch.