Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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
107 changes: 107 additions & 0 deletions tests/test_proxy.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from yarl import URL

import aiohttp
from aiohttp.client_proto import ResponseHandler
from aiohttp.client_reqrep import (
ClientRequest,
ClientRequestArgs,
Expand Down Expand Up @@ -1167,3 +1168,109 @@ 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)?


This test uses a real ResponseHandler instance so we can assert that
``_skip_payload`` is actually set to ``True`` on the protocol object
after the CONNECT 200 handshake. ``_skip_payload=True`` causes the
underlying ``HttpResponseParser`` to be configured with
``response_with_body=False``, which means any body bytes advertised
by Content-Length / Transfer-Encoding in the tunnel response are
silently discarded rather than blocking the TLS upgrade.
"""
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]
):
# Use a real ResponseHandler so we can assert its internal state
# rather than only checking that a mock method was called.
tr = mock.Mock()
proto = ResponseHandler(loop=event_loop)
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()
)

# Verify that the connector configured the real protocol
# to skip the CONNECT response body. This flag causes
# HttpResponseParser to use response_with_body=False, so
# any Content-Length / Transfer-Encoding bytes in the
# tunnel handshake are not consumed before TLS is started.
assert proto._skip_payload is True, (
"ResponseHandler._skip_payload must be True after a "
"CONNECT 200 so that proxy body bytes are not read "
"before the TLS upgrade (RFC-9110 §9.3.6)"
)

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