classification
Title: urllib.parse.urlparse() accepts wrong URLs
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.5
process
Status: open Resolution:
Dependencies: 20270 Superseder:
Assigned To: Nosy List: demian.brecht, gvanrossum, martin.panter, orsenthil, serhiy.storchaka, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2014-01-15 12:32 by serhiy.storchaka, last changed 2015-03-23 05:26 by martin.panter.

Files
File name Uploaded Description Edit
urlparse.patch vstinner, 2014-03-07 16:19 review
urlparse_2.patch serhiy.storchaka, 2015-03-03 18:40 review
Messages (12)
msg208156 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-15 12:32
>>> import urllib.parse
>>> p = urllib.parse.urlparse('http://[::1]spam:80')
>>> p
ParseResult(scheme='http', netloc='[::1]spam:80', path='', params='', query='', fragment='')
>>> p.hostname
'::1'
>>> p.port
80

'http://[::1]spam:80' is invalid URL, but urllib.parse.urlparse() accepts it and just ignore the spam part.
msg208675 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-21 17:14
What should be correct behavior? Raise an exception, return '[::1]spam' as hostname, or left all as is?
msg209852 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-01 00:23
I think we should start raising an exception in 3.5 (backwards incompatible change to back-port it)
msg212888 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-03-07 16:19
Here is a patch for Python 3.5 which breaks backward compatibility: urlparse functions now raise a ValueError if the IPv6 address, port or host is invalid.

Examples of invalid URLs:

- "HTTP://WWW.PYTHON.ORG:65536/doc/#frag": 65536 is invalid
- "http://www.example.net:foo"
- "http://::1/"
- "http://[127.0.0.1]/"
- "http://[host]/"

According to unit tests, Python 3.4 is more tolerant: it only raises an error when the port number is read (obj.port) from an URL with an invalid port. There error is not raised when the whole URL is parsed.

Is it ok to break backward compatibility?
msg212889 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-03-07 16:19
Oh, by the way my patch fixes also #18191.
msg212890 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-03-07 16:21
My patch "urlparse.patch" may be modified to fix also #18191 in Python 2.7, 3.3 and 3.4: splitport() should handle IPv6 ("[::1]:80") and auth ("user:passowrd@host") but not raises an exception.
msg236990 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-01 22:09
splitport() shouldn't handle auth, it should be called after auth is dropped with splituser().

The patch makes URL parsing slower, especially URLs with IPv6 addresses.

$ ./python -m timeit -s "from urllib.parse import urlparse, clear_cache" -- "urlparse('http://python.org:80'); clear_cache()"
Unpatched: 10000 loops, best of 3: 70.3 usec per loop
Patched:   10000 loops, best of 3: 105 usec per loop

$ ./python -m timeit -s "from urllib.parse import urlparse, clear_cache" -- "urlparse('http://[2001:4802:7901:0:e60a:1375:0:5]:80'); clear_cache()"
Unpatched: 10000 loops, best of 3: 71.1 usec per loop
Patched:   1000 loops, best of 3: 239 usec per loop
msg237001 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-02 01:10
I suspect the proposed patch here would make some of the special error handling for the “port” property redundant, and would compete with my patch for Issue 20059, which raises ValueError in more cases when “port” is accessed, rather than returning None.

There should also be test cases (and maybe documentation or fixes?) to illustrate the behaviour of parsing:

* http://[fe80::a%25en1] (RFC 6874-style scoped IPv6 address with zone identifier)
* http://[v1.fe80::a+en1] (RFC 3986 IPvFuture format, using <https://tools.ietf.org/html/draft-sweet-uri-zoneid-01> style address)
msg237157 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-03 18:40
With optimizations in issue23563 and weaken IPv6 check the implementation of urlsplit() can be faster.

$ ./python -m timeit -s "from urllib.parse import urlparse, clear_cache" -- "urlparse('http://python.org:80'); clear_cache()"
10000 loops, best of 3: 86.3 usec per loop
$ ./python -m timeit -s "from urllib.parse import urlparse, clear_cache" -- "urlparse('http://[2001:4802:7901:0:e60a:1375:0:5]:80'); clear_cache()"
10000 loops, best of 3: 88.6 usec per loop
msg237369 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-06 18:29
I think some further consideration around this change is worthwhile:

Currently, urllib.parse.split* methods seem to do inconsistent validation around the data they're splitting. For example:

(None for an invalid port)
>>> parse.splitnport('example.com:foo')
('example.com', None)

Whereas other split* methods do not:

(Auth part should be URL-encoded)
>>> parse.splituser('u@ser:p@ssword@example.com:80')
('u@ser:p@ssword', 'example.com:80')

And others are just plain incorrect:

(:bad should be the port as defined by the ABNF 'authority = [ userinfo "@" ] host [ ":" port ]')
>>> parse.splitport('example.com:bad')
('example.com:bad', None)

However, none of these cases (currently) raise exceptions.

Looking at urllib.parse, two large motivations behind it are splitting and parsing. In my mind, splitting should solely be responsible for splitting input based on RFC-defined delimiters. Parsing on the other hand, should be responsible for both splitting as necessary as well as input validation. It may also make sense to add simple validation functions to the module to comply with the "batteries included" philosophy, but that's a topic for another issue.

My concern with the proposed patch is that it adds further inconsistency to split* methods:

Before patch:

>>> parse.urlsplit('http://[::1]spam:80')
SplitResult(scheme='http', netloc='[::1]spam:80', path='', query='', fragment='')

After patch:

>>> parse.urlsplit('http://[::1]spam:80')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Volumes/src/p/cpython/Lib/urllib/parse.py", line 350, in urlsplit
    netloc, url = _splitnetloc(url, 2)
  File "/Volumes/src/p/cpython/Lib/urllib/parse.py", line 324, in _splitnetloc
    raise ValueError('Invalid IPv6 URL')

(While the above examples still yield the same results and don't raise exceptions)

I do think that the validation should be done and I agree that an exception should be raised, but it should be done at the urlparse level, not on split (in this case, due to the change to _splitnecloc).
msg237371 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-06 18:59
> splitport() shouldn't handle auth, it should be called after auth is dropped with splituser().

Why shouldn't splitport() handle auth? Are you suggesting that in the eyes of splitport() that "user:password@host:port" should be invalid?

>>> parse.splitport('user:password@host:80')
('user:password@host', '80')

Seems to be reasonable behaviour to me. Why add the artificial constraint of calling splituser() first?
msg238982 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-23 05:26
To me, the only difference between urlsplit() and urlparse() is that urlsplit() does not treat parameters following a semicolon specially. The documentation tends to agree with this view. So whatever exceptions are raised by urlparse() should also be raised by urlsplit() for consistency.

Also, I doubt it is worth worrying about the finer details of splitport() or whatever, because they are meant to be deprecated and not used externally, except for backwards compatibility.
History
Date User Action Args
2015-03-23 05:26:01martin.pantersetmessages: + msg238982
2015-03-06 18:59:51demian.brechtsetstage: needs patch -> patch review
2015-03-06 18:59:25demian.brechtsetmessages: + msg237371
2015-03-06 18:29:59demian.brechtsetnosy: + demian.brecht
messages: + msg237369
2015-03-03 18:40:15serhiy.storchakasetfiles: + urlparse_2.patch

messages: + msg237157
2015-03-02 01:10:53martin.pantersetnosy: + martin.panter
messages: + msg237001
2015-03-01 22:09:09serhiy.storchakasetmessages: + msg236990
2014-03-07 16:21:36vstinnersetmessages: + msg212890
2014-03-07 16:19:40vstinnersetmessages: + msg212889
2014-03-07 16:19:24vstinnersetfiles: + urlparse.patch

nosy: + gvanrossum, vstinner
messages: + msg212888

keywords: + patch
2014-02-01 00:23:38yselivanovsetnosy: + yselivanov

messages: + msg209852
versions: + Python 3.5, - Python 2.7, Python 3.3, Python 3.4
2014-01-21 17:14:26serhiy.storchakasetmessages: + msg208675
2014-01-15 12:37:09serhiy.storchakalinkissue18191 dependencies
2014-01-15 12:32:52serhiy.storchakasetdependencies: + urllib.parse doesn't work with empty port
2014-01-15 12:32:09serhiy.storchakacreate