python / cpython

The Python programming language
https://www.python.org
Other
62.39k stars 29.96k forks source link

http.cookiejar handle cookie.version to be None #83009

Open 835f6b8c-37e9-4cfb-bb5c-c81164a22ded opened 4 years ago

835f6b8c-37e9-4cfb-bb5c-c81164a22ded commented 4 years ago
BPO 38828
Nosy @tirkarthi, @alclarks

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: ```python assignee = None closed_at = None created_at = labels = ['3.8', 'type-bug', 'library'] title = 'http.cookiejar handle cookie.version to be None' updated_at = user = 'https://bugs.python.org/kovid' ``` bugs.python.org fields: ```python activity = actor = 'alclarks' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'kovid' dependencies = [] files = [] hgrepos = [] issue_num = 38828 keywords = ['3.8regression'] message_count = 7.0 messages = ['356797', '356798', '356802', '356804', '356805', '356807', '356960'] nosy_count = 3.0 nosy_names = ['kovid', 'xtreak', 'alclarks'] pr_nums = [] priority = 'normal' resolution = None stage = 'test needed' status = 'open' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue38828' versions = ['Python 3.8'] ```

835f6b8c-37e9-4cfb-bb5c-c81164a22ded commented 4 years ago

In python 3.8 cookiejar.py is full of code that compares cookie.version to integers, which raises as exception when cookie.version is None. For example, in set_ok_version() and set_ok_path(). Both the Cookie constructor and _cookie_from_cookie_tuple() explicitly assume version can be None and setting version to None worked fine in previous pythonreleases.

tirkarthi commented 4 years ago

Can you please add a sample script to reproduce this that worked fine before Python 3.8?

835f6b8c-37e9-4cfb-bb5c-c81164a22ded commented 4 years ago

The issue is obvious with a simple glance at the code. Either the Cookie constructor needs to change version = None to zero or some other integer or the various methods in that module need to handle a None version. I dont personally care about this issue any more since I have worked around it in my code, feel free to fix it or not, as you wish.

tirkarthi commented 4 years ago

You specifically mentioned Python 3.8 and I just wanted to understand the issue to see if it's a regression. I don't see any version related code changes in recent times. Hence this is not necessarily a regression in 3.8 unless there is a reproducible script. The code in set_ok_version for cookie.version to be None is not tested hence I assume there are no tests for the reported scenario that would be good to have. I have also changed the issue title accordingly.

835f6b8c-37e9-4cfb-bb5c-c81164a22ded commented 4 years ago

It's trivially True that it is a regression from python 2 since in python 2 comparison to None is fine. Whether it ever worked in any python 3 version before 3.8 I'm not sure about.

835f6b8c-37e9-4cfb-bb5c-c81164a22ded commented 4 years ago

Here's a trivial script to reproduce:

from urllib.request import Request
from http.cookiejar import Cookie, CookieJar

jar = CookieJar()
jar.set_cookie(Cookie(
            None, 'test', 'test',
            None, False,
            '.test.com', True, False,
            '/', True,
            False, None, False, None, None, None
        ))
r = Request('http://www.test.com')
jar.add_cookie_header(r)
310cbb8d-de10-43c7-9726-d4010211fa47 commented 4 years ago

Hi, it looks like this needs a fix - I'll write a patch to fix up the handling and add some more testing to cover this scenario.