urllib.parse.urlparse accepts any falsy value as an url

[ff59cd45-ebe3-4b3e-9696-65dc59a38b8c]

BPO	22234
Nosy	@orsenthil, @bitdancer, @vadmium, @serhiy-storchaka, @ztane, @demianbrecht, @poleto, @jacobtylerwalls
PRs	python/cpython#26687
Files	issue22234_36.patch: Patch with the deprecation warning to be applied on Python 3.6. issue22234_37.patch: Changes to urlparse to handle non-str and non-byte arguments urlparse_empty_bad_arg_deprecation.patch urlparse_empty_bad_arg_disallow.patch urlparse_empty_bad_arg_deprecation2.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: ```python assignee = None closed_at = None created_at = labels = ['type-bug', 'library'] title = 'urllib.parse.urlparse accepts any falsy value as an url' updated_at = user = 'https://github.com/ztane' ``` bugs.python.org fields: ```python activity = actor = 'jacobtylerwalls' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'ztane' dependencies = [] files = ['42591', '42592', '42600', '42602', '42607'] hgrepos = [] issue_num = 22234 keywords = ['patch'] message_count = 19.0 messages = ['225566', '225832', '260671', '264023', '264210', '264211', '264217', '264219', '264261', '264269', '264278', '264344', '264346', '264347', '264357', '264427', '265030', '395765', '395766'] nosy_count = 8.0 nosy_names = ['orsenthil', 'r.david.murray', 'martin.panter', 'serhiy.storchaka', 'ztane', 'demian.brecht', 'luiz.poleto', 'jacobtylerwalls'] pr_nums = ['26687'] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue22234' versions = ['Python 3.6'] ```

[ff59cd45-ebe3-4b3e-9696-65dc59a38b8c]

Because of "if x else ''" in _decode_args (http://hg.python.org/cpython/file/3.4/Lib/urllib/parse.py#l96), urllib.parse.urlparse accepts any falsy value as an url, returning a ParseResultBytes with all members set to empty bytestrings.

Thus you get:

    >>> urllib.parse.urlparse({})
    ParseResultBytes(scheme=b'', netloc=b'', path=b'', params=b'', query=b'', fragment=b'')

which may result in some very confusing exceptions later on: I had a list of URLs that accidentally contained some Nones and got very confusing TypeErrors while processing the results expecting them to be strings.

If the if x else '' part were removed, such invalid falsy values would fail with AttributeError: 'foo' object has no attribute 'decode', as happens with any truthy invalid value.

[ff59cd45-ebe3-4b3e-9696-65dc59a38b8c]

On Python 2.7 urlparse.urlparse, parsing None, () or 0 will throw AttributeError because these classes do not have any 'find' method. [] has the find method, but will fail with TypeError, because the built-in caching requires that the input be hashable.

[ff59cd45-ebe3-4b3e-9696-65dc59a38b8c]

I believe urlparse should throw a TypeError if not isinstance(url, (str, bytes))

[bitdancer]

I just posted about this on the mentors list, where someone brought this issue up with a question about our policy on type checking. The short version is the better (preserves duck-typing) and more backward compatibile fix is to change the test to be

x != ''

That will result in an attribute error on 'decode' for values of the incorrect type.

But even that should go through a deperecation cycle, since there may be working programs depending on the current behavior. It's worth fixing, though, because of the error propogation you report. I also suggested a rewrite of _check_args to get a better error message that would indeed be a type error, and I'm anticipating someone from the mentors list will turn that into a patch here.

[55a6ac4b-dfe7-421c-a85f-9db0c32780ac]

As discussed on the Mentors list, the attached patch (issue22234_36.patch) includes the deprecation warning (and related test) on the urlparse function.

[55a6ac4b-dfe7-421c-a85f-9db0c32780ac]

As discussed on the Mentors list, the attached patch (issue22234_37.patch) changes the urlparse function to handle non-str and non-bytes arguments and adds a new test case for it.

[serhiy-storchaka]

x != '' emits BytesWarning if x is bytes. 'encode' attribute is not needed for URL parsing. any() is slower that a for loop.

I would suggest to look at efficient os.path implementations.

[vadmium]

Luiz: Your _36 patch looks like it adds an unconditional warning whenever urlparse() is called, but I would have expected it to depend on the type of the “url” parameter.

There are related functions that seem to accept false values like None in Python 3, but not in Python 2. Perhaps they should also be considered with any changes:

urlsplit(None)
parse_qs(None)
parse_qsl(None)
urldefrag(None)

Also, I wonder if we should continue to accept bytearray as well as bytes. Bytearray has a decode() method.

[serhiy-storchaka]

Here is a patch that deprecates empty non-str and non-decodable arguments for urlparse, urlsplit, urlunparse, urlunsplit, urldefrag, and parse_qsl.

[serhiy-storchaka]

And here is simpler patch that just disallows bad arguments without deprecation.

[serhiy-storchaka]

Updated patch addresses Martin's comment.

[55a6ac4b-dfe7-421c-a85f-9db0c32780ac]

I am seeing some results when running urlparse with patch urlparse_empty_bad_arg_deprecation2.patch applied:

>>> urllib.parse.urlparse({})
__main__:1: DeprecationWarning: Use of {} is deprecated
__main__:1: DeprecationWarning: Use of '' is deprecated
ParseResultBytes(scheme=b'', netloc=b'', path=b'', params=b'', query=b'', fragment=b'')

>>> urllib.parse.urlparse('', b'')
__main__:1: DeprecationWarning: Use of b'' is deprecated
/home/poleto/SCMws/python/latest/cpython/Lib/urllib/parse.py:378: DeprecationWarning: Use of b'' is deprecated
  splitresult = urlsplit(url, scheme, allow_fragments)
ParseResult(scheme=b'', netloc='', path='', params='', query='', fragment='')
Will bytes be deprecated if used as a default_schema?

>>> urllib.parse.urlparse(b'', '')
ParseResultBytes(scheme=b'', netloc=b'', path=b'', params=b'', query=b'', fragment=b'')
Shouldn't it complain that the types are different? In fact it does, if you don't provide empty strings:

>>> urllib.parse.urlparse(b'www.python.org', 'http')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "(...)/cpython/Lib/urllib/parse.py", line 377, in urlparse
    url, scheme, _coerce_result = _coerce_args(url, scheme)
  File "(...)/cpython/Lib/urllib/parse.py", line 120, in _coerce_args
    raise TypeError("Cannot mix str and non-str arguments")
TypeError: Cannot mix str and non-str arguments

>>> urllib.parse.urlparse({'a' : 1})
__main__:1: DeprecationWarning: Use of '' is deprecated
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "(...)/cpython/Lib/urllib/parse.py", line 377, in urlparse
    url, scheme, _coerce_result = _coerce_args(url, scheme)
  File "(...)/cpython/Lib/urllib/parse.py", line 128, in _coerce_args
    return _decode_args(args) + (_encode_result,)
  File "(...)/cpython/Lib/urllib/parse.py", line 98, in _decode_args
    return tuple(x.decode(encoding, errors) if x else '' for x in args)
  File "(...)/cpython/Lib/urllib/parse.py", line 98, in <genexpr>
    return tuple(x.decode(encoding, errors) if x else '' for x in args)
AttributeError: 'dict' object has no attribute 'decode'

>>> urllib.parse.urlparse(['a', 'b', 'c'])
__main__:1: DeprecationWarning: Use of [] is deprecated
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "(...)/cpython/Lib/urllib/parse.py", line 377, in urlparse
    url, scheme, _coerce_result = _coerce_args(url, scheme)
  File "(...)/cpython/Lib/urllib/parse.py", line 128, in _coerce_args
    return _decode_args(args) + (_encode_result,)
  File "(...)/cpython/Lib/urllib/parse.py", line 98, in _decode_args
    return tuple(x.decode(encoding, errors) if x else '' for x in args)
  File "(...)/cpython/Lib/urllib/parse.py", line 98, in <genexpr>
    return tuple(x.decode(encoding, errors) if x else '' for x in args)
AttributeError: 'list' object has no attribute 'decode'

I thought about writing test cases but I wasn't a 100% sure if the above is working as expected so I thought I should ask first.

[vadmium]

Regarding urlparse(b'', ''). Currently the second parameter is “scheme”, which is documented as being an empty text string by default. If we deprecate this, we should update the documentation.

[55a6ac4b-dfe7-421c-a85f-9db0c32780ac]

As for urlparse_empty_bad_arg_disallow.patch, I didn't go too deep into testing it but I found that calling urlparse with different non-str args are producing different results:

urlparse({})
TypeError: unhashable type: 'slice'

urlparse([])
AttributeError: 'list' object has no attribute 'decode'

urlparse(())
AttributeError: 'tuple' object has no attribute 'decode'

I thought they should all raise a TypeError but again, I am not sure it is working as expected by the patch's author.

[orsenthil]

Serhiy:

I left review comments on the patch too. I agree to "tightening" of the input arg type in these urlparse functions.

Before we for the next version, I think, it will be helpful to enumerate the behavior for wrong arg types for these functions that you would like to see.

1. Invalid formats like {}, [], None could just be TypeError.

2. The mix of bytes and str should be deprecated and we could give the suggestion for the encouraged single type in the deprecation warning.

3. Any thing else w.r.t to special rules for various parts of url.

In general, if we are going with the deprecation cycle, we would as well go with deciding on what to allow and present it in a simple way.

[ff59cd45-ebe3-4b3e-9696-65dc59a38b8c]

I do not believe there is code that would depend on urlparse(urlstring={}) not throwing an error, since {} obviously is neither a URL, nor a string.

Further down the documentation explicitly states that

The URL parsing functions were originally designed to operate on character strings only. In practice, it is useful to be able to manipulate properly quoted and encoded URLs as sequences of ASCII bytes. Accordingly, the URL parsing functions in this module all operate on bytes and bytearray objects in addition to str objects.

As the documentation does not state that it should work on any other objects, there shouldn't be any code that should be deprecated. Furthermore even in 3.5, the bool(datetime.time(0, 0)) == False was removed without any deprecation cycle, despite it having been a documented feature for more than a decade (unlike this one).

And IMHO not giving an object of expected type should result in a TypeError.

[serhiy-storchaka]

Thank you Luiz for your testing.

__main__:1: DeprecationWarning: Use of '' is deprecated

It is bad that a warning is emitted for default value.

Will bytes be deprecated if used as a default_schema?

No, only using empty bytes schema with string url is deprecated (because it works now). Using non-empty bytes schema with string url just causes an error.

Shouldn't it complain that the types are different?

This special case is left for compatibility with wrappers.

__main__:1: DeprecationWarning: Use of [] is deprecated

The warning should not be emitted for the value that the user did not provide.

If go by the way of strong deprecation, the patch needs reworking. But this is a way of overcomplication.

Since as pointed Antti, only using str, bytes and bytearray is documented, I think we can ignore the breakage for other types.

[b982ca5e-273f-4a9d-b6df-60abec2375c9]

Both this ticket and bpo-19094 started from one method in urllib.parse and then generalized a proposal for the rest of the submodule to move away from duck-typing and to instead raise TypeErrors (or at least some error) for invalid types. The attached PR does that. But it broke pip, which was passing None to the fragment argument of urlunsplit().

In a blaze of efficiency, Pip already merged my fix, but since the CPython test suite depends on the current version of pip -- perhaps there's an argument for at least raising a deprecation warning for None and then converting to '' or b''? In any case, green CI makes patches more reviewable, so I'm inclined to add such a warning/conversion for now and then see how feedback goes. Cheers, all.

BTW: perhaps we can close this as dupe of bpo-19094?

[b982ca5e-273f-4a9d-b6df-60abec2375c9]

Well, now I've looked at the CPython test failure more closely, and it's in test.test_venv.EnsurePipTest where we just download latest pip.

Their release cadence suggests a new release in July, about 2-4 weeks from now. So I'll wait on adding the DeprecationWarning for passing None unless folks ask for it during review, and am happy to wait a few weeks for green CI.