Excess leading path separators causes ConnectionPool.urlopen to parse URL as host & port

[sigmavirus24]

Subject

Originally reported in Requests (psf/requests#6643) but also applicable here, : is a valid character in a path per RFC3986. However, if a path is used in urllib3 where the path starts with // then urllib3 interprets this as a scheme-less host and attempts to parse the hostname and port but the part after the : may not be an integer.

Environment

Describe your environment.
At least, paste here the output of:

import platform
import ssl
import urllib3

print("OS", platform.platform())
print("Python", platform.python_version())
print(ssl.OPENSSL_VERSION)
print("urllib3", urllib3.__version__)

OS Linux-6.5.10-200.fc38.x86_64-x86_64-with-glibc2.38
Python 3.12.1
OpenSSL 3.1.1 30 May 2023
urllib3 2.2.1

Steps to Reproduce

import urllib3

urllib3.PoolManager().urlopen(method="GET", url="http://127.0.0.1:10000//v:h")

Expected Behavior

If you have a server running on that port, a 200 OK, otherwise a connection error.

Actual Behavior

  File "/home/sigmavirus24/sandbox/requests/.tox/py312/lib/python3.12/site-packages/urllib3/util/url.py", line 425, in parse_url
    host, port = _HOST_PORT_RE.match(host_port).groups()  # type: ignore[union-attr]
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'groups'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/sigmavirus24/sandbox/requests/.tox/py312/lib/python3.12/site-packages/urllib3/poolmanager.py", line 444, in urlopen
    response = conn.urlopen(method, u.request_uri, **kw)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/sigmavirus24/sandbox/requests/.tox/py312/lib/python3.12/site-packages/urllib3/connectionpool.py", line 711, in urlopen
    parsed_url = parse_url(url)
                 ^^^^^^^^^^^^^^
  File "/home/sigmavirus24/sandbox/requests/.tox/py312/lib/python3.12/site-packages/urllib3/util/url.py", line 451, in parse_url
    raise LocationParseError(source_url) from e
urllib3.exceptions.LocationParseError: Failed to parse: //v:h

Part of this is that u.request_uri is properly parsed and returned as //v:h but ConnectionPool.urlopen seems to be doing the same thing that PoolManager.urlopen does and the thing that PoolManager.urlopen is trying to delegate to it - i.e., it:

• reparses the URI (

  urllib3/src/urllib3/connectionpool.py

  Line 711 in d4ffa29

  parsed_url = parse_url(url)

  )
• and in reparsing it tries to normalize the host (

  urllib3/src/urllib3/util/url.py

  Line 425 in d4ffa29

  host, port = _HOST_PORT_RE.match(host_port).groups() # type: ignore[union-attr]

  )

If, however, we can (as early as possible) normalize the URL then we'll be in a better place. For example, this doesn't happen if you do this on the ConnectionPool level directly:

>>> connpool = urllib3.HTTPConnectionPool(host='localhost', port=10000)
>>> connpool.urlopen(method='GET', url='http://localhost:10000//v:h')
...
urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=10000): Max retries exceeded with url: http://localhost:10000//v:h (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7fcb11ad7020>: Failed to establish a new connection: [Errno 111] Connection refused'))

And so I think that in PoolManager specifically, we need to trim those leading /. There are a few ways to do this:

s = '//v:h'
print(f'/{s.lstrip("/")}')
# => '/v:h'
print(re.sub('^/+', '/', s))
# => '/v:h'

[sigmavirus24]

I'm also worried about how this is handled in when handling redirects in the retry logic (e.g., server returns a Location: //v:h or Location: //host:port//v:h but I haven't investigated that

[crabhi]

Hi! I investigated a breakage introduced after requests upgrade which led me here (see psf/requests#6711).

I think the root cause of the issue is here in urrlib3 and it can't be effectively fixed in the requests package. If I read RFC3986 correctly, empty path segments are allowed and double slash at the beginning of Path is forbidden only if the authority (host, port) is missing. Also, there are some servers that vary their behaviour depending on the count of leading slashes in the URL, most notably AWS S3.

Therefore, the correct fix IMO is not to strip the URL but rather to parse the double slash into the "Path".

[sigmavirus24]

@crabhi please let's keep discussion to one place for now

[crabhi]

Sure. I've posted the comment above before your first reply. I agree to keep the current discussion in the other issue.

[crabhi]

I took a look into urllib3 and it seems a bit complicated. The API of HTTPConnectionPool is slightly confusing. It uses the url parameter to mean two things. It can be either the full URL in case of a proxy request or it may be just the path part. Consider

>>> pool = urllib3.PoolManager().connection_from_host('127.0.0.1', port=8000, scheme='http')
>>> pool.urlopen('GET', '/a')
<urllib3.response.HTTPResponse object at 0x700dd1f3d570>

vs.

>>> pool = urllib3.PoolManager().connection_from_host('127.0.0.1', port=8000, scheme='http')
>>> pool.urlopen('GET', 'http://127.1.2.3/bb')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/krab/workspace/urllib3/src/urllib3/connectionpool.py", line 721, in urlopen
    raise HostChangedError(self, url, retries)
urllib3.exceptions.HostChangedError: HTTPConnectionPool(host='127.0.0.1', port=8000): Tried to open a foreign host with url: http://127.1.2.3/bb

In the example above (urllib3.PoolManager().urlopen(method="GET", url="http://127.0.0.1:10000//v:h")), the PoolManager.urlopen parses the URL. ℹ️ The first parse succeeds and correctly parses //v:h into path. Then, HTTPConnectionPool.urlopen is called with the URL of //v:h and it tries to parse the (path part of) URL again and fails. Probably correctly, because h is not a number of port.

Therefore, the fix should take into account the interplay between PoolManager and HTTPConnectionPool. A quick fix would be to add another parameter that would tell HTTPConnectionPool that the URL should not be parsed again because it's in fact only a path. Such change would be backwards compatible but there's already a ton of parameters.

I'm tagging also @shazow and @jalopezsilva because @sigmavirus24 has "Taking a break from FOSS" in his status line. It seems @shazow is the original author from long time ago and @jalopezsilva committed 691679f that added the unconditional parse_url call to HTTPConnectionPool.urlopen.

Do you see an option to avoid re-parsing the path?

[shazow]

The API of HTTPConnectionPool is slightly confusing. It uses the url parameter to mean two things.

The way it was designed originally is through a layered approach: Each of the lower level layers is unaware of the higher level layers (HTTPConnection -> HTTPConnectionPool -> PoolManager etc).

Traditionally we prefer to do whatever input mangling in the highest layer possible, to allow for falling back to lower layers when necessary.

But I'm not very up to date on recent changes, cc @SethMichaelLarson @pquentin who are the current lead maintainers. :)

[crabhi]

Yeah, the problem is in the double parsing of the URL using the same code. The first pass parses http://127.0.0.1:10000//v:h correctly. Then, //v:h is sent again to parse_url and it fails.

[nateprewitt]

We got another drive-by PR in Requests about this so I took a look again. I think the solution is actually a lot simpler than we were thinking the last time I dug into this. The second parse_url is exclusively for scheme parsing for proxy support. We don't even use the parsed result when the path starts with a /, so we can completely sidestep it.

I have full details in #4952.

[nateprewitt]

I'm also worried about how this is handled in when handling redirects in the retry logic (e.g., server returns a Location: //v:h or Location: //host:port//v:h but I haven't investigated that

Also this case is broken. This PR doesn't change anything about those semantics though. I don't know how common this is in practice since it's never been raised in either repo.

[nateprewitt]

This should be resolved in the next release with #3352. Then we can cleanup the patch in Requests. I'm going to close this out since the underlying issue is resolved. If we want to dig into the redirect case more, I think that's probably its own issue.