Repository navigation
gh-158907: Do not send proxy credentials to a direct origin on 407 - #158956
rajorin wants to merge 3 commits into
Conversation
Sorry, something went wrong.
|
All commit authors signed the Contributor License Agreement. |
Sorry, something went wrong.
| # 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 |
Sorry, something went wrong.
There was a problem hiding this comment.
Shortened it to that line.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
|
@rajorin Thank you for a great PR! One very small note: there’s no need to sync with the main unless there's a conflict or it's particularly stale. |
Sorry, something went wrong.
ProxyBasicAuthHandler.http_error_407() looked up passwords with req.host, which has no scheme. HTTPPasswordMgr treats a scheme-less lookup as a match for any scheme, so a direct HTTP origin that returns 407 could receive credentials stored for https://HOST/ and the handler would retry in cleartext with Proxy-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 sends Proxy-Authorization.