Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions aiohttp/connector.py
Original file line number Diff line number Diff line change
Expand Up @@ -1532,9 +1532,13 @@ async def _create_proxy_connection(
# read_until_eof=True will ensure the connection isn't closed
# once the response is received and processed allowing
# START_TLS to work on the connection below.
# skip_payload=True per RFC-9110 §9.3.6: a client MUST ignore
# any Content-Length or Transfer-Encoding header fields received
# in a successful response to CONNECT.
protocol.set_response_params(
read_until_eof=True,
timeout_ceil_threshold=self._timeout_ceil_threshold,
skip_payload=True,
)
resp = await proxy_resp.start(conn)
except BaseException:
Expand Down
95 changes: 95 additions & 0 deletions tests/test_proxy.py
Original file line number Diff line number Diff line change
Expand Up @@ -1167,3 +1167,98 @@ async def test_https_auth( # type: ignore[misc]
proxy_resp.close()
await req._close()
await connector.close()


@mock.patch("aiohttp.connector.ClientRequestBase")
@mock.patch(
"aiohttp.connector.aiohappyeyeballs.start_connection",
autospec=True,
spec_set=True,
)
async def test_https_connect_skip_payload_on_200( # type: ignore[misc]
start_connection: mock.Mock,
ClientRequestMock: mock.Mock,
make_client_request: _RequestMaker,
) -> None:
"""Regression test for https://github.com/aio-libs/aiohttp/issues/8472.

Per RFC-9110 §9.3.6 a client MUST ignore any Content-Length or
Transfer-Encoding header fields in a successful response to CONNECT.
Comment on lines +1186 to +1187
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test doesn't seem to validate any of this. It just verifies the function is called with the given arguments.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Verify that set_response_params is called with skip_payload=True so the
HTTP parser does not try to read a body from the 200 tunnel response.
"""
event_loop = asyncio.get_running_loop()
proxy_req = ClientRequestBase(
"GET",
URL("http://proxy.example.com"),
auth=None,
loop=event_loop,
ssl=True,
headers=CIMultiDict({}),
)
ClientRequestMock.return_value = proxy_req

url = URL("http://proxy.example.com")
proxy_resp = ClientResponse(
"get",
url,
writer=None,
continue100=None,
timer=TimerNoop(),
traces=[],
loop=event_loop,
session=mock.Mock(),
request_headers=CIMultiDict[str](),
original_url=url,
)
with mock.patch.object(proxy_req, "_send", autospec=True, return_value=proxy_resp):
with mock.patch.object(proxy_resp, "start", autospec=True) as m:
m.return_value.status = 200

connector = aiohttp.TCPConnector()
r = {
"hostname": "hostname",
"host": "127.0.0.1",
"port": 80,
"family": socket.AF_INET,
"proto": 0,
"flags": 0,
}
with mock.patch.object(
connector, "_resolve_host", autospec=True, return_value=[r]
):
tr, proto = mock.Mock(), mock.Mock()
with mock.patch.object(
event_loop,
"create_connection",
autospec=True,
return_value=(tr, proto),
):
with mock.patch.object(
event_loop,
"start_tls",
autospec=True,
return_value=mock.Mock(),
):
req = make_client_request(
"GET",
URL("https://www.python.org"),
proxy=URL("http://proxy.example.com"),
loop=event_loop,
)
await connector._create_connection(
req, [], aiohttp.ClientTimeout()
)

# proto is the mock protocol returned by create_connection.
# The connector calls protocol.set_response_params(...) on it
# directly, so we assert on proto's auto-mock attribute.
proto.set_response_params.assert_called_once_with(
read_until_eof=True,
timeout_ceil_threshold=mock.ANY,
skip_payload=True,
)

proxy_resp.close()
await req._close()
await connector.close()
Loading