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.

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

cookielib uses time.time(), making incorrect checks of expiration times in cookies #66492

Closed
regu0004 mannequin opened this issue Aug 29, 2014 · 8 comments
Closed

cookielib uses time.time(), making incorrect checks of expiration times in cookies #66492

regu0004 mannequin opened this issue Aug 29, 2014 · 8 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@regu0004
Copy link
Mannequin

regu0004 mannequin commented Aug 29, 2014

BPO 22296
Nosy @terryjreedy, @pitrou, @4kir4
Files
  • cookie_timestamp_test.py: Example of incorrect expiration check in the cookielib module
  • cookie_timestamp_test_3.4.py: Example of incorrect expiration check in the http.cookiejar module in 3.4
  • 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 2014-09-02.06:15:27.268>
    created_at = <Date 2014-08-29.10:00:50.174>
    labels = ['invalid', 'type-bug', 'library']
    title = 'cookielib uses time.time(), making incorrect checks of expiration times in cookies'
    updated_at = <Date 2014-09-02.07:22:16.390>
    user = 'https://bugs.python.org/regu0004'

    bugs.python.org fields:

    activity = <Date 2014-09-02.07:22:16.390>
    actor = 'berker.peksag'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-09-02.06:15:27.268>
    closer = 'regu0004'
    components = ['Library (Lib)']
    creation = <Date 2014-08-29.10:00:50.174>
    creator = 'regu0004'
    dependencies = []
    files = ['36502', '36519']
    hgrepos = []
    issue_num = 22296
    keywords = []
    message_count = 8.0
    messages = ['226056', '226097', '226213', '226222', '226225', '226226', '226227', '226261']
    nosy_count = 4.0
    nosy_names = ['terry.reedy', 'pitrou', 'akira', 'regu0004']
    pr_nums = []
    priority = 'normal'
    resolution = 'not a bug'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue22296'
    versions = ['Python 2.7']

    @regu0004
    Copy link
    Mannequin Author

    regu0004 mannequin commented Aug 29, 2014

    The cookielib module uses time.time(), which produces a timestamp in the local timezone (as read from the system time?), as the timestamp against which expiration dates in cookies are compared.

    However, typical usage of HTTP cookies would be specifying the expiration date in UTC. This assumption seems to be supported for example by the inclusion of cookielib.http2time, which (only) supports UTC timestamps.

    This behaviour is also included in e.g. MozillaCookieJar, which (erroneously) excludes cookies from being saved/loaded based on the local timestamp from time.time().

    See the attached file for a small example where the check if a cookie is expired against a UTC time is correct but the check against local time fails (simulating the behaviour of the cookielib module).

    @regu0004 regu0004 mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Aug 29, 2014
    @terryjreedy
    Copy link
    Member

    If you have not, please check if this issue applies to 3.4, and post a 3.4 version of the test file.

    In the absence of a standard, I am not sure if this is a bug, and even if we call it such, whether 2.7 should be changed.

    @regu0004
    Copy link
    Mannequin Author

    regu0004 mannequin commented Sep 1, 2014

    I've checked and an updated test file for 3.4 shows the same behaviour in the renamed module http.cookiejar.

    Even though no standard exists I hope 3.4+ would be changed to simplify the cookie handling, since there is a lot of hassle converting UTC times to local times (which in general should be avoided to avoid introducing further problems with e.g. daylight savings).

    @4kir4
    Copy link
    Mannequin

    4kir4 mannequin commented Sep 1, 2014

    time.time() returns the current time in seconds since Epoch
    it is neither local nor UTC time. It can be converted to both.

    You can get local time using datetime.fromtimestamp(ts).
    You can get UTC time using datetime.utcfromtimestamp(ts) or
    to get an aware datetime object: datetime.fromtimestamp(ts, timezone.utc), where ts is the timestamp in seconds since Epoch.

    I don't know whether there is an issue with cookie.is_expired() but it
    is incorrect to use time.mktime() with UTC time tuple unless the local
    time is UTC. To get the timestamp from a datetime object, you could
    use .timestamp() method instead:

      from datetime import datetime, timezone
    
      now = datetime.now(timezone.utc) # the current time
      seconds_since_epoch = now.timestamp()
      seconds_since_epoch = time.time() # might be less precise

    @pitrou
    Copy link
    Member

    pitrou commented Sep 1, 2014

    Note the .timestamp() method will work correctly if the datetime object is expressed in *local time*, which is not what Rebecka's code uses. Otherwise the incantation is a bit more complex:

    https://docs.python.org/3/library/datetime.html#datetime.datetime.timestamp

    (also .timestamp() doesn't exist in 2.7, in which case the manual computation must also be used)

    @4kir4
    Copy link
    Mannequin

    4kir4 mannequin commented Sep 1, 2014

    timestamp() method works correctly for an aware datetime objects
    as in my example (notice: timezone.utc in the code).

    The issue is not that it is a manual computation,
    the issue is that it is incorrect:

    #XXX WRONG, DO NOT DO IT
    time.mktime(datetime.datetime.utcnow().timetuple())

    On older Python versions, given a utc time as a naive datetime
    object, POSIX timestamp is:

      ts = (utc_dt - datetime(1970, 1, 1)).total_seconds()
      utc_dt = datetime(1970, 1, 1) + timedelta(seconds=ts) # in reverse

    @4kir4
    Copy link
    Mannequin

    4kir4 mannequin commented Sep 1, 2014

    The last example assumes that time.gmtime(0) is 1970-01-01 00:00:00Z
    (otherwise time.time() may return different timestamp)

    @regu0004
    Copy link
    Mannequin Author

    regu0004 mannequin commented Sep 2, 2014

    Akira is correct: using time.mktime to produce the expiration date for the cookie is wrong (thank you very much for the pointers!).

    Using e.g. http.cookiejar.http2time with a HTTP formatted date string gives a correct time stamp (with which cookie.is_expired succeeds), so this was not a bug (just user error...).

    @regu0004 regu0004 mannequin closed this as completed Sep 2, 2014
    @regu0004 regu0004 mannequin added the invalid label Sep 2, 2014
    @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
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants