Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixing fractional expiry time bug in cookiejar #68076

Closed
ssh mannequin opened this issue Apr 8, 2015 · 11 comments
Closed

Fixing fractional expiry time bug in cookiejar #68076

ssh mannequin opened this issue Apr 8, 2015 · 11 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@ssh
Copy link
Mannequin

ssh mannequin commented Apr 8, 2015

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
    Copy link
    Mannequin Author

    ssh mannequin commented Apr 8, 2015

    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.

    @ssh ssh mannequin added type-crash A hard crash of the interpreter, possibly with a core dump stdlib Python modules in the Lib dir labels Apr 8, 2015
    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Apr 14, 2015

    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
    Copy link
    Mannequin

    demianbrecht mannequin commented Apr 14, 2015

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

    @demianbrecht demianbrecht mannequin added type-bug An unexpected behavior, bug, or error and removed type-crash A hard crash of the interpreter, possibly with a core dump labels Apr 14, 2015
    @ssh
    Copy link
    Mannequin Author

    ssh mannequin commented May 6, 2015

    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
    Copy link
    Mannequin

    demianbrecht mannequin commented May 6, 2015

    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
    Copy link
    Mannequin Author

    ssh mannequin commented May 7, 2015

    Attaching patch after addressing comments in code review.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented May 7, 2015

    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
    Copy link
    Mannequin Author

    ssh mannequin commented May 7, 2015

    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
    Copy link
    Mannequin

    demianbrecht mannequin commented May 8, 2015

    LGTM, thanks for the patch!

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 3, 2015

    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
    Copy link
    Member

    Thanks for the patch.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    1 participant
    @rbtcollins