fix: use Proxy-Authorization header for proxy auth - #152
Conversation
LeyckerS
left a comment
There was a problem hiding this comment.
@slegarraga — first: you did what the issue asked, you did it quickly, you avoided the Authorization/Proxy-Authorization trap the issue warned about, you kept the >=3.9 floor with a clean feature-detect instead of moving it, and you verified with -W error::DeprecationWarning. That is a better-executed first pull request than most. The problem is upstream of all of it: the issue told you to do the wrong thing. That is my mistake, and I have corrected it publicly on #151.
Here is what happens with this change on the path the program actually uses.
aiohttp/connector.py, _update_proxy_auth_header_and_build_proxy_req:
if not req.is_ssl():
req.headers[hdrs.PROXY_AUTHORIZATION] = auth # http → on the request
else:
proxy_req.headers[hdrs.PROXY_AUTHORIZATION] = auth # https → on the CONNECTFor an https target, aiohttp opens a CONNECT tunnel and the proxy authenticates that. moon_download.py:502 now puts the credentials in hdrs, which becomes headers= on the tunnelled request — sent inside TLS, encrypted end to end with the file host. The proxy cannot read it and answers 407.
Both providers hand out https links, so this affects every real download through an authenticated proxy while looking correct everywhere else.
Two things worth taking away from this, neither of which is a criticism of the patch:
Your three new tests are good tests and they pass — but they assert what parse_proxy_line returns, and this bug lives in what aiohttp does with it. No unit test at that boundary could have caught it. That is not a gap you should have filled; it is why the issue needed to be right.
And the deprecation turns out not to be fixable at all right now. proxy_auth rejects anything that is not a BasicAuth (raise ValueError("proxy_auth must be None or BasicAuth() tuple")), and BasicAuth is the thing that warns. aiohttp deprecated the only type its own parameter accepts without shipping a replacement.
So I am not asking you to revise this into something else — there is nothing correct to revise it into until aiohttp 4 provides a proxy-auth API. I have rewritten #151 to be what it should have been: keep BasicAuth, and silence the warning as noise if it is worth silencing.
If you would like that one instead, it is yours — it needs a pytest.ini (or a [tool.pytest.ini_options] block; this repo has neither yet) with a filterwarnings entry and a comment pointing at the thread so nobody re-opens this in six months. Small, and it would put your name on the changelog for finding the real answer here rather than the one I guessed at. Say the word and I will assign it.
If you would rather not spend another evening on my mistake, that is completely fair too, and thank you for this one regardless.
Closes #151.
Builds proxy credentials as a
Proxy-Authorizationheader value instead ofaiohttp.BasicAuth. Usesaiohttp.encode_basic_authwhen available and falls back toBasicAuth(...).encode()otherwise, so theaiohttp>=3.9floor does not change.Verified:
pytest tests/ -q-> 32 passedpython -W error::DeprecationWarning -m pytest tests/test_proxy_pool.py -q-> 8 passed