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

Inconsistent urlparse/urllib.parse handling of invalid port values? #64258

Closed
chadbirch mannequin opened this issue Dec 23, 2013 · 15 comments
Closed

Inconsistent urlparse/urllib.parse handling of invalid port values? #64258

chadbirch mannequin opened this issue Dec 23, 2013 · 15 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@chadbirch
Copy link
Mannequin

chadbirch mannequin commented Dec 23, 2013

BPO 20059
Nosy @orsenthil, @rbtcollins, @bitdancer, @berkerpeksag, @vadmium, @serhiy-storchaka, @demianbrecht
Files
  • port-ValueError.patch
  • port-ValueError.v2.patch
  • port-ValueError.v3.patch: For 3.6
  • 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-09.21:54:02.895>
    created_at = <Date 2013-12-23.23:51:50.551>
    labels = ['type-bug', 'library']
    title = 'Inconsistent urlparse/urllib.parse handling of invalid port values?'
    updated_at = <Date 2015-08-09.21:54:02.894>
    user = 'https://bugs.python.org/chadbirch'

    bugs.python.org fields:

    activity = <Date 2015-08-09.21:54:02.894>
    actor = 'rbcollins'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-08-09.21:54:02.895>
    closer = 'rbcollins'
    components = ['Library (Lib)']
    creation = <Date 2013-12-23.23:51:50.551>
    creator = 'chad.birch'
    dependencies = []
    files = ['38073', '38642', '40061']
    hgrepos = []
    issue_num = 20059
    keywords = ['patch']
    message_count = 15.0
    messages = ['206881', '232797', '235660', '237002', '237038', '237173', '237399', '238979', '247621', '247625', '247628', '247638', '247642', '247643', '248340']
    nosy_count = 10.0
    nosy_names = ['orsenthil', 'rbcollins', 'r.david.murray', 'cvrebert', 'python-dev', 'berker.peksag', 'martin.panter', 'serhiy.storchaka', 'demian.brecht', 'chad.birch']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue20059'
    versions = ['Python 3.6']

    @chadbirch
    Copy link
    Mannequin Author

    chadbirch mannequin commented Dec 23, 2013

    I'm not sure if this is something that needs adjustment, but it seems somewhat inconsistent to me.

    After using urlparse() on various urls with invalid port values, trying to access .port on the result will raise a ValueError. This case includes urls such as:

    "http://www.example.com:asdf"

    "http://www.example.com:1.5"

    "http://www.example.com:"

    However, as of May 24 2012 (http://hg.python.org/cpython/diff/d769e64aed79/Lib/urllib/parse.py), if the invalid port value is an integer, accessing .port will result in None. So this includes urls such as:

    "http://www.example.com:66000"

    "http://www.example.com:-1"

    Should these two cases be made consistent, so that either .port is always None or always results in a ValueError if the port section of the url is invalid? I'd be happy to write a patch for it if it's wanted, but I thought I'd check first (and see which of the two options would be correct, if so).

    @chadbirch chadbirch mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Dec 23, 2013
    @vadmium
    Copy link
    Member

    vadmium commented Dec 17, 2014

    I would go for raising ValueError for port numbers out of range. The value of None was already defined to mean that no port is included in the URL.

    Also, the ValueError exception should be documented. It is surprising that urlsplit() does not raise any exception, but then simply getting the “port” attribute does raise the exception.

    BTW your third example is wrong:

    >>> urlparse("http://www.example.com:").port is None
    True

    It’s probably safest to keep this one as it is, but maybe it also needs documenting :)

    @vadmium
    Copy link
    Member

    vadmium commented Feb 10, 2015

    Mapping out-of-range ports to None was added in bpo-14036, though I don’t understand why that approach was taken instead of raising ValueError. Here is a patch to raise ValueError for out-of-range integer values instead.

    @vadmium
    Copy link
    Member

    vadmium commented Mar 2, 2015

    See also bpo-20271, which has a proposed patch with more strict urlsplit() etc behaviour before even returning a SplitResult object.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Mar 2, 2015

    LGTM and I think it is worth to be applied to maintained releases.

    @berkerpeksag
    Copy link
    Member

    berkerpeksag commented Mar 4, 2015

    I think it is worth to be applied to maintained releases.

    I'd commit this only to the default branch. Changing the return value from None to an exception after three 3.4 bugfix releases(3.4.1, 3.4.2 and 3.4.3 -- also since 3.4.3 was released in Feb 2015, 3.4.4 will probably be the last bugfix release of 3.4) would not be the best action. I understand that the current situation is inconsistent, but I'm not sure it's worth it to commit to the 2.7 and 3.4 branches.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Mar 6, 2015

    It is surprising that urlsplit() does not raise any exception

    I have a bit of a TL;DR in bpo-20271, trying to capture what the responsibilities of split and parse methods in urllib are and what they should be if consistency is something that we're after.

    Around the patch though:

    It seems quite odd to me to be raising exceptions on accessors rather than on instantiation or when the parsing occurs (in the body of urlparse). Wouldn't it better to raise the exception there?

    @vadmium
    Copy link
    Member

    vadmium commented Mar 23, 2015

    Patch v2 just changes a test to use “with self.assertRaises()”.

    The behaviour of urlparse() succeeding and then result.port failing is indeed odd and surprising. Hopefully documenting this behaviour will help with the “surprising” aspect. But changing it would surely break compatibility. For example, the following is still potentially useful:

    >>> urlsplit("http://localhost:ipp/").netloc
    'localhost:ipp'

    On Unix, some programs look up port names in /etc/services, even though this is not valid for URLs according to the RFCs, and urlsplit().port does not support it. In this case “ipp” would be whatever port the Internet Printing Protocol server is configured to run on.

    @rbtcollins
    Copy link
    Member

    rbtcollins commented Jul 29, 2015

    So, I think this is worth applying. The discussion around :ipp etc is irrelevant here: this patch changes large or negative ints to be a valueerror, as non-ints are.

    The only question is where. I think this is in the category of 'will only break buggy applications' - applications that already handle ValueError to deal with bad inputs, will not be broken. Applications that depend on ports outside the valid range for ports will be broken, but thats the definition of the bug.

    So I propose to apply to 2.7/3.4/3.5/3.6, but I'm going to seek a second opinion.

    @bitdancer
    Copy link
    Member

    bitdancer commented Jul 29, 2015

    Because it raises an error where none was raised before, I'd only apply this to 3.6. This is especially true since this issue is not a *bug* report, but a "shouldn't this be more consistent" report. That is, there's no great weight (the OP wasn't even sure it should be changed) in favor of the backport, so even a small possibility of breaking working code argues against the backport.

    (To be clear: by working code I'm envisioning code that is getting that None value and using it as an error signal or treating it the same as a missing port. Such code is technically "broken" but could be working if the default port happens to work, or the only out of range values it encounters are integers, or it is happy with the exception bubbling up in the non-integer cases.)

    @rbtcollins
    Copy link
    Member

    rbtcollins commented Jul 29, 2015

    ok, 3.6 only.

    @vadmium
    Copy link
    Member

    vadmium commented Jul 29, 2015

    If we take the 3.6-only path, does that warrant adding “Version changed” notices, and/or a What’s New entry?

    @bitdancer
    Copy link
    Member

    bitdancer commented Jul 29, 2015

    Hmm. Good question. I think it probably does, because it means getting an exception where one did not previously happen.

    @vadmium
    Copy link
    Member

    vadmium commented Jul 30, 2015

    Added versioning notices in port-ValueError.v3.patch.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 9, 2015

    New changeset 7c78279afc30 by Robert Collins in branch 'default':
    Issue bpo-20059: urllib.parse raises ValueError on all invalid ports.
    https://hg.python.org/cpython/rev/7c78279afc30

    @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

    5 participants