Repository navigation
Conversation
| # A direct origin is not a proxy. req.host has no scheme, so a | ||
| # password stored for https://HOST/ would match and be sent in | ||
| # cleartext on the retry (gh-158907). HTTPS tunnels set | ||
| # _tunnel_host instead of making has_proxy() true. |
There was a problem hiding this comment.
What do you think about making this comment a bit less verbose?
| # A direct origin is not a proxy. req.host has no scheme, so a | |
| # password stored for https://HOST/ would match and be sent in | |
| # cleartext on the retry (gh-158907). HTTPS tunnels set | |
| # _tunnel_host instead of making has_proxy() true. | |
| # gh-158907: a 407 from a direct origin is not a proxy challenge |
| http_handler.requests[0].has_header("Proxy-authorization")) | ||
|
|
||
| def test_proxy_basic_auth_https_tunnel_still_authenticates(self): | ||
| # 407 from the proxy during an HTTPS tunnel must still be answered. |
There was a problem hiding this comment.
One more thing. It's possible I'm confused here!
I'm not sure if 407 from the proxy during an HTTPS tunnel even reaches http_error_407().
HTTPConnection._tunnel() raises OSError on a non-200 CONNECT reply:
Lines 1039 to 1041 in 2639fd6
I think this is #51540
Perhaps then:
| # 407 from the proxy during an HTTPS tunnel must still be answered. | |
| # gh-51540: a 407 to CONNECT never reaches this handler today, since | |
| # http.client raises OSError on a non-200 CONNECT reply. If it ever | |
| # did, the credentials would go to the proxy in CONNECT, not to the | |
| # origin. |
Another approach is removing this test. I believe it passes even without the PR.
There was a problem hiding this comment.
You're right on both counts. HTTPConnection._tunnel() raises OSError for a non-200 CONNECT, and do_open turns that into URLError, so that 407 never reaches http_error_407. That's the existing gap in gh-51540.
The tunnel test also passes on main, so I removed it. _tunnel_host stays in the guard because an HTTPS proxy request does not set has_proxy(), and a 407 that does reach this handler should still be answered. On the retry, Proxy-Authorization is sent on CONNECT, not to the origin.
ProxyBasicAuthHandler looked up passwords with the scheme-less origin host, so an HTTP 407 could match credentials stored for HTTPS and retry in cleartext.
A CONNECT 407 never reaches http_error_407, and the tunnel test passed without this change.
dea14f3 to
b9bdbcc
Compare
|
@rajorin Thank you for a great PR! One very small note: there’s no need to sync with the |
ProxyBasicAuthHandler.http_error_407()looked up passwords withreq.host, which has no scheme.HTTPPasswordMgrtreats a scheme-less lookup as a match for any scheme, so a direct HTTP origin that returns 407 could receive credentials stored forhttps://HOST/and the handler would retry in cleartext withProxy-Authorization.A 407 is now ignored unless the request is actually going through a proxy (
Request.has_proxy(), or an HTTPS tunnel via_tunnel_host). Genuine proxy authentication, including HTTPS tunnels, still sendsProxy-Authorization.