fix: ignore Content-Length/Transfer-Encoding in CONNECT 2xx proxy responses (RFC-9110)#12397
fix: ignore Content-Length/Transfer-Encoding in CONNECT 2xx proxy responses (RFC-9110)#12397OfekDanny wants to merge 4 commits intoaio-libs:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #12397 +/- ##
========================================
Coverage 98.92% 98.92%
========================================
Files 134 134
Lines 46616 46766 +150
Branches 2429 2430 +1
========================================
+ Hits 46114 46264 +150
Misses 373 373
Partials 129 129
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Merging this PR will not alter performance
Comparing 
Footnotes
|
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
| Per RFC-9110 §9.3.6 a client MUST ignore any Content-Length or | ||
| Transfer-Encoding header fields in a successful response to CONNECT. |
There was a problem hiding this comment.
The test doesn't seem to validate any of this. It just verifies the function is called with the given arguments.
There was a problem hiding this comment.
Good point, thanks! I've updated the test to use a real ResponseHandler instance instead of mock.Mock(). After the CONNECT 200 handshake, the test now asserts proto._skip_payload is True on the actual protocol object.
_skip_payload is the flag that feeds response_with_body=False into HttpResponseParser, so this assertion directly validates the behaviour: any Content-Length / Transfer-Encoding bytes advertised by the proxy in its tunnel response will not be consumed before the TLS upgrade.
If someone removes skip_payload=True from the connector the flag stays False and the assertion fails — which is exactly what a regression test should do.
There was a problem hiding this comment.
But, it says it must ignore those headers, and then tests that by making a request without those headers and validating that an arbitrary attribute is set?
The original issue shows a ClientConnectorError. Can we not create a test that reproduces that error (before the fix is applied)?
Address review feedback: instead of asserting that set_response_params is called with skip_payload=True on a mock, instantiate a real ResponseHandler and assert proto._skip_payload is True after the CONNECT 200 handshake. This validates the actual HTTP parser configuration (response_with_body=False) that prevents tunnel response body bytes from blocking the TLS upgrade (RFC-9110 §9.3.6). Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
|
As far as I can tell from my testing, this was already fixed. |
3 participants
Summary
Per RFC-9110 §9.3.6:
When a proxy server returns a
200 Connection establishedresponse with aContent-Lengthheader, aiohttp's HTTP parser attempts to read that many bytes as a response body. This corrupts the tunnel and causes the subsequent TLS handshake to fail.The fix adds
skip_payload=Trueto theset_response_paramscall used when reading the CONNECT response, which instructs the HTTP parser to treat the response as having no body regardless of any framing headers.Changes
aiohttp/connector.py: addskip_payload=Trueto CONNECT response parsingtests/test_proxy.py: add regression test (test_https_connect_skip_payload_on_200) that verifiesset_response_paramsis called withskip_payload=TrueFixes #8472