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

cookiejar parses cookie value as int with empty name-value pair and Expires #67327

Closed
chfoo mannequin opened this issue Dec 31, 2014 · 11 comments
Closed

cookiejar parses cookie value as int with empty name-value pair and Expires #67327

chfoo mannequin opened this issue Dec 31, 2014 · 11 comments
Assignees
Labels
easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@chfoo
Copy link
Mannequin

chfoo mannequin commented Dec 31, 2014

BPO 23138
Nosy @berkerpeksag, @serhiy-storchaka, @demianbrecht
Files
  • issue23138_tip.patch
  • issue23138_1.patch
  • issue23138_2.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 = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2015-03-13.07:17:20.141>
    created_at = <Date 2014-12-31.05:06:29.200>
    labels = ['easy', 'type-bug', 'library']
    title = 'cookiejar parses cookie value as int with empty name-value pair and Expires'
    updated_at = <Date 2015-03-13.10:34:11.206>
    user = 'https://bugs.python.org/chfoo'

    bugs.python.org fields:

    activity = <Date 2015-03-13.10:34:11.206>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2015-03-13.07:17:20.141>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2014-12-31.05:06:29.200>
    creator = 'chfoo'
    dependencies = []
    files = ['38260', '38416', '38464']
    hgrepos = []
    issue_num = 23138
    keywords = ['patch', 'easy']
    message_count = 11.0
    messages = ['233227', '236497', '236782', '237433', '237652', '237653', '237728', '237733', '238010', '238011', '238022']
    nosy_count = 6.0
    nosy_names = ['BreamoreBoy', 'python-dev', 'berker.peksag', 'serhiy.storchaka', 'demian.brecht', 'chfoo']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue23138'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @chfoo
    Copy link
    Mannequin Author

    chfoo mannequin commented Dec 31, 2014

    Something like "Set-Cookie: ; Expires=Thu, 01 Jan 1970 00:00:10 GMT" causes the resulting cookie.value to be parsed as an int.

    I expected either str or None as described in the documentation.

    Example evil server:

    try:
        import http.server as http_server
    except ImportError:
        import BaseHTTPServer as http_server
    
        class MyHandler(http_server.BaseHTTPRequestHandler):
            def do_GET(self):
                self.send_response(200)
                self.send_header('Set-Cookie', '; Expires=Thu, 01 Jan 1970 00:00:10 GMT')
                self.send_header('Set-Cookie', 'good=123.45600')
                self.end_headers()
    
    
        def main():
            server = http_server.HTTPServer(('127.0.0.1', 8000), MyHandler)
            server.serve_forever()
    
    
        if __name__ == '__main__':
            main()
            

    Example innocent client:

    try:
        import http.cookiejar as http_cookiejar
    except ImportError:
        import cookielib as http_cookiejar
    
    try:
        import urllib.request as urllib_request
    except ImportError:
        import urllib2 as urllib_request
    
        def main():
            cj = http_cookiejar.CookieJar()
            opener = urllib_request.build_opener(urllib_request.HTTPCookieProcessor(cj))
            r = opener.open("http://127.0.0.1:8000/")
            
            print(cj._cookies)
    
        if __name__ == '__main__':
            main()

    The resulting output is:

    {'127.0.0.1': {'/': {'expires': Cookie(version=0, name='expires', value=10.0, port=None, port_specified=False, domain='127.0.0.1', domain_specified=False, domain_initial_dot=False, path='/', path_specified=False, secure=False, expires=None, discard=True, comment=None, comment_url=None, rest={}, rfc2109=False), 'good': Cookie(version=0, name='good', value='123.45600', port=None, port_specified=False, domain='127.0.0.1', domain_specified=False, domain_initial_dot=False, path='/', path_specified=False, secure=False, expires=None, discard=True, comment=None, comment_url=None, rest={}, rfc2109=False)}}}

    It gives two cookies where the first one contains name='expires', value=10.0 which is unexpected. I expected that either the bad cookie is discarded or it is accepted but the value is always a str (even if it is garbage) or None.

    This bug was found in my custom cookie policy where I do len(cookie.value or ''). There is also a reference on StackOverflow but I believe no Python library bug report was filed: http://stackoverflow.com/q/20325571/1524507 .

    This was tested on Python 2.7.8, 3.2.6, 3.3.6, and 3.4.2.

    @chfoo chfoo mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Dec 31, 2014
    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Feb 24, 2015

    @demian I believe this may be of interest to you.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Feb 27, 2015

    Attached is a fix that ignores the entire invalid cookie as defined in RFC 6265, Section 5.2. I'm also attaching patches for maintenance branches as it's a valid bug (NAME=VALUE pairs are required across all RFCs), although it would break backwards compatibility if the user was expecting invalid behaviour.

    @demianbrecht demianbrecht mannequin added the easy label Feb 27, 2015
    @serhiy-storchaka serhiy-storchaka self-assigned this Mar 7, 2015
    @serhiy-storchaka
    Copy link
    Member

    According to RFC 6265, Section 5.2:

    1. If the name-value-pair string lacks a %x3D ("=") character,
      ignore the set-cookie-string entirely.

    But "Set-Cookie: spam; Expires=Thu, 01 Jan 1970 00:00:10 GMT" is accepted. key="spam", value=None.

    1. If the name string is empty, ignore the set-cookie-string
      entirely.

    But "Set-Cookie: =spam; Expires=Thu, 01 Jan 1970 00:00:10 GMT" is accepted. key="", value="spam".

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Mar 9, 2015

    I agree that the current implementation doesn't conform to standards, but do you think those cases are worth fixing as they can potentially break backwards compatibility? I think that the reported case makes sense to fix as the name/value pair are entirely unexpected. However, the current behaviour is logical for the cases that you've pointed out.

    @serhiy-storchaka
    Copy link
    Member

    I think that for consistency either parse empty name-value pair as key="", value=None, or ignore all non-conformed cases. For backward compatibility I prefer first way.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Mar 10, 2015

    I think that for consistency either parse empty name-value pair as key="", value=None

    There is already a test present (https://hg.python.org/cpython/file/0469af231d22/Lib/test/test_http_cookiejar.py#l1084) that ensures an unset name/value pair is ignored altogether, so I don't think that makes sense from a backwards compatibility standpoint. For consistency, I kept the functionality where nameless cookies are ignored (i.e. "=foo"). I think that while it may be breaking backwards compatibility for buggy edge cases, it's more consistent with existing functionality and actually conforms to the RFC. That said, I'm not going to argue over it heatedly, so if you'd still rather see those cases permitted, let me know and I'll change it.

    Valueless cookies are still permitted to keep backwards compatible as there are existing tests for that.

    @serhiy-storchaka
    Copy link
    Member

    This looks reasonable.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 13, 2015

    New changeset 44c1c0cbdc06 by Serhiy Storchaka in branch '2.7':
    Issue bpo-23138: Fixed parsing cookies with absent keys or values in cookiejar.
    https://hg.python.org/cpython/rev/44c1c0cbdc06

    New changeset c1abcbcfefab by Serhiy Storchaka in branch '3.4':
    Issue bpo-23138: Fixed parsing cookies with absent keys or values in cookiejar.
    https://hg.python.org/cpython/rev/c1abcbcfefab

    New changeset 7cc7c794d1cb by Serhiy Storchaka in branch 'default':
    Issue bpo-23138: Fixed parsing cookies with absent keys or values in cookiejar.
    https://hg.python.org/cpython/rev/7cc7c794d1cb

    @serhiy-storchaka
    Copy link
    Member

    As side effect the parsing is now twice faster.

    $ ./python -m timeit -s "from http.cookiejar import parse_ns_headers" -- "parse_ns_headers('foo=bar; Expires=Thu, 01 Jan 1970 00:00:10 GMT')"
    Before: 1000 loops, best of 3: 976 usec per loop
    After:  1000 loops, best of 3: 537 usec per loop

    @serhiy-storchaka
    Copy link
    Member

    Oh, this was incorrect example. The correct one is:

    $ ./python -m timeit -s "from http.cookiejar import parse_ns_headers" -- "parse_ns_headers(['foo=bar; path=/; version=1; Expires=Thu, 01 Jan 1970 00:00:10 GMT'])"
    Before: 10000 loops, best of 3: 177 usec per loop
    After:  10000 loops, best of 3: 104 usec per loop

    @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
    easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant