Trim excess leading path separators #6644

sigmavirus24 · 2024-02-22T01:11:15Z

A URL with excess leading / (path-separator)s would cause urllib3 to attempt to reparse the request-uri as a full URI with a host and port. This bypasses that logic in ConnectionPool.urlopen by replacing these leading /s with just a single /.

Closes #6643

tests/test_adapters.py

sigmavirus24 · 2024-02-22T12:39:14Z

src/requests/adapters.py

@@ -389,7 +390,7 @@ def request_url(self, request, proxies):
            proxy_scheme = urlparse(proxy).scheme.lower()
            using_socks_proxy = proxy_scheme.startswith("socks")

-        url = request.path_url
+        url = re.sub("^/+", "/", request.path_url)


As mentioned on urllib3/urllib3#3352 this could also be

url = f"/{request.path_url.lstrip('/')}"

I could benchmark these but I don't particularly care what the implementation is. I just threw this together to show that it can be fixed

It looks like the f-string (Python 3.9-3.12 tested) is ~4x faster but we're talking on the scale of nanoseconds so it's basically moot. I'd vote the f-string for readability, but don't have a strong opinion.

Yeah, I'm also happy to shove this into a branch too like

if path.startswith('//'):

To make it clearer that we only care about the separator being repeated. What I want is clarity in the reader as to why we're doing this. My old school brain things the regexp is clearer and the f-string looks sus but that's just my opinion and I'm not holding it closely

Yeah, branching seems fine to me too.

Did both (f string and branch)

A URL with excess leading / (path-separator)s would cause urllib3 to attempt to reparse the request-uri as a full URI with a host and port. This bypasses that logic in ConnectionPool.urlopen by replacing these leading /s with just a single /. Closes psf#6643

sigmavirus24 assigned nateprewitt and sethmlarson Feb 22, 2024