Repository navigation
URLPattern never matches default ports in proxy keys (NO_PROXY=example.com:80) #3827
Replies: 2 comments 1 reply
|
Confirmed, and your diagnosis is right — but there's a wrinkle in the fix surface that's worth deciding before you write the PR: The mechanism, agreed. The trap. port_priority = 0 if self.port is not None else 1So a pattern that spells out On "read the port from the raw pattern": be careful here.
The second shape handles a case your description doesn't cover: Concrete suggestion for if self.port is not None:
other_port = other.port
if other_port is None:
other_port = {"http": 80, "https": 443}.get(other.scheme)
if self.port != other_port:
return Falseand leave On your test plan: 7 cases in
Also worth checking: Net: yes, take it as a PR — but scope the description to "default ports in patterns now match, and explicit ports gain the specificity they already claimed in |
|
I have the branch ready at The repository has an interaction limit active that restricts opening pull requests to collaborators, so GitHub blocks opening a PR directly. The branch and tests are pushed to https://github.com/davrens101/httpx/tree/fix/urlpattern-default-ports if you want to pull from there or open the gate. |
Uh oh!
There was an error while loading. Please reload this page.
Proxy keys and NO_PROXY entries that spell out a default port never match. NO_PROXY=example.com:80 becomes the key all://example.com:80, and it doesn't match http://example.com.
On httpx 0.28.1 and current master (b5addb6), Python 3.12.10, Windows 11.
The cause is that URL drops a port equal to its scheme's default, but only when it knows the scheme. httpx.URL("http://example.com").port is None, while httpx.URL("all://example.com:80").port is 80 because "all" has no default. So the pattern keeps 80, the request URL has None, and the comparison fails. https://example.com:443 works today since the https scheme is known.
I have a small fix in URLPattern: read the port from the raw pattern, and compare it against the request URL after normalizing it for the request's scheme. It comes with 7 added cases in test_url_matches. Is this something you'd take as a PR?
All reactions