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

[chadbirch]

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]

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).

[vadmium]

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]

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]

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

[serhiy-storchaka]

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

[berkerpeksag]

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]

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]

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]

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]

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]

ok, 3.6 only.

[vadmium]

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

[bitdancer]

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