diff --git a/CHANGES/7756.bugfix b/CHANGES/7756.bugfix new file mode 100644 index 00000000000..b8a137ce92e --- /dev/null +++ b/CHANGES/7756.bugfix @@ -0,0 +1 @@ +Ensure empty body response for 1xx/204/304 per RFC 9112 sec 6.3 diff --git a/aiohttp/helpers.py b/aiohttp/helpers.py index eaf6b3c3457..19710e15edc 100644 --- a/aiohttp/helpers.py +++ b/aiohttp/helpers.py @@ -1065,6 +1065,15 @@ def parse_http_date(date_str: Optional[str]) -> Optional[datetime.datetime]: return None +def must_be_empty_body(method: str, code: int) -> bool: + """Check if a request must return an empty body.""" + return ( + status_code_must_be_empty_body(code) + or method_must_be_empty_body(method) + or (200 <= code < 300 and method.upper() == hdrs.METH_CONNECT) + ) + + def method_must_be_empty_body(method: str) -> bool: """Check if a method must return an empty body.""" # https://datatracker.ietf.org/doc/html/rfc9112#section-6.3-2.1 @@ -1076,3 +1085,17 @@ def status_code_must_be_empty_body(code: int) -> bool: """Check if a status code must return an empty body.""" # https://datatracker.ietf.org/doc/html/rfc9112#section-6.3-2.1 return code in {204, 304} or 100 <= code < 200 + + +def should_remove_content_length(method: str, code: int) -> bool: + """Check if a Content-Length header should be removed. + + This should always be a subset of must_be_empty_body + """ + # https://www.rfc-editor.org/rfc/rfc9110.html#section-8.6-8 + # https://www.rfc-editor.org/rfc/rfc9110.html#section-15.4.5-4 + return ( + code in {204, 304} + or 100 <= code < 200 + or (200 <= code < 300 and method.upper() == hdrs.METH_CONNECT) + ) diff --git a/aiohttp/web_fileresponse.py b/aiohttp/web_fileresponse.py index 0fbdeab0dba..5e24c30bab2 100644 --- a/aiohttp/web_fileresponse.py +++ b/aiohttp/web_fileresponse.py @@ -16,7 +16,7 @@ from . import hdrs from .abc import AbstractStreamWriter -from .helpers import ETAG_ANY, ETag +from .helpers import ETAG_ANY, ETag, must_be_empty_body from .typedefs import LooseHeaders, PathLike from .web_exceptions import ( HTTPNotModified, @@ -266,7 +266,7 @@ async def prepare(self, request: "BaseRequest") -> Optional[AbstractStreamWriter ) # If we are sending 0 bytes calling sendfile() will throw a ValueError - if count == 0 or request.method == hdrs.METH_HEAD or self.status in [204, 304]: + if count == 0 or must_be_empty_body(request.method, self.status): return await super().prepare(request) fobj = await loop.run_in_executor(None, filepath.open, "rb") diff --git a/aiohttp/web_response.py b/aiohttp/web_response.py index 1f2d6239382..901b8363bc3 100644 --- a/aiohttp/web_response.py +++ b/aiohttp/web_response.py @@ -30,10 +30,12 @@ CookieMixin, ETag, HeadersMixin, + must_be_empty_body, parse_http_date, populate_with_cookies, rfc822_formatted_time, sentinel, + should_remove_content_length, validate_etag_value, ) from .http import SERVER_SOFTWARE, HttpVersion10, HttpVersion11 @@ -77,6 +79,7 @@ class StreamResponse(BaseClass, HeadersMixin, CookieMixin): "_req", "_payload_writer", "_eof_sent", + "_must_be_empty_body", "_body_length", "_state", "_headers", @@ -104,6 +107,7 @@ def __init__( self._req: Optional[BaseRequest] = None self._payload_writer: Optional[AbstractStreamWriter] = None self._eof_sent = False + self._must_be_empty_body: Optional[bool] = None self._body_length = 0 self._state: Dict[str, Any] = {} @@ -333,7 +337,7 @@ async def prepare(self, request: "BaseRequest") -> Optional[AbstractStreamWriter return None if self._payload_writer is not None: return self._payload_writer - + self._must_be_empty_body = must_be_empty_body(request.method, self.status) return await self._start(request) async def _start(self, request: "BaseRequest") -> AbstractStreamWriter: @@ -370,26 +374,33 @@ async def _prepare_headers(self) -> None: "Using chunked encoding is forbidden " "for HTTP/{0.major}.{0.minor}".format(request.version) ) - writer.enable_chunking() - headers[hdrs.TRANSFER_ENCODING] = "chunked" + if not self._must_be_empty_body: + writer.enable_chunking() + headers[hdrs.TRANSFER_ENCODING] = "chunked" if hdrs.CONTENT_LENGTH in headers: del headers[hdrs.CONTENT_LENGTH] elif self._length_check: writer.length = self.content_length if writer.length is None: - if version >= HttpVersion11 and self.status != 204: - writer.enable_chunking() - headers[hdrs.TRANSFER_ENCODING] = "chunked" - if hdrs.CONTENT_LENGTH in headers: - del headers[hdrs.CONTENT_LENGTH] - else: + if version >= HttpVersion11: + if not self._must_be_empty_body: + writer.enable_chunking() + headers[hdrs.TRANSFER_ENCODING] = "chunked" + elif not self._must_be_empty_body: keep_alive = False - # HTTP 1.1: https://tools.ietf.org/html/rfc7230#section-3.3.2 - # HTTP 1.0: https://tools.ietf.org/html/rfc1945#section-10.4 - elif version >= HttpVersion11 and self.status in (100, 101, 102, 103, 204): - del headers[hdrs.CONTENT_LENGTH] - if self.status not in (204, 304): + # HTTP 1.1: https://tools.ietf.org/html/rfc7230#section-3.3.2 + # HTTP 1.0: https://tools.ietf.org/html/rfc1945#section-10.4 + if self._must_be_empty_body: + if hdrs.CONTENT_LENGTH in headers and should_remove_content_length( + request.method, self.status + ): + del headers[hdrs.CONTENT_LENGTH] + # https://datatracker.ietf.org/doc/html/rfc9112#section-6.1-10 + # https://datatracker.ietf.org/doc/html/rfc9112#section-6.1-13 + if hdrs.TRANSFER_ENCODING in headers: + del headers[hdrs.TRANSFER_ENCODING] + else: headers.setdefault(hdrs.CONTENT_TYPE, "application/octet-stream") headers.setdefault(hdrs.DATE, rfc822_formatted_time()) headers.setdefault(hdrs.SERVER, SERVER_SOFTWARE) @@ -650,7 +661,7 @@ async def write_eof(self, data: bytes = b"") -> None: assert self._req is not None assert self._payload_writer is not None if body is not None: - if self._req._method == hdrs.METH_HEAD or self._status in [204, 304]: + if self._must_be_empty_body: await super().write_eof() elif self._body_payload: payload = cast(Payload, body) @@ -662,14 +673,21 @@ async def write_eof(self, data: bytes = b"") -> None: await super().write_eof() async def _start(self, request: "BaseRequest") -> AbstractStreamWriter: - if not self._chunked and hdrs.CONTENT_LENGTH not in self._headers: + if should_remove_content_length(request.method, self.status): + if hdrs.CONTENT_LENGTH in self._headers: + del self._headers[hdrs.CONTENT_LENGTH] + elif not self._chunked and hdrs.CONTENT_LENGTH not in self._headers: if self._body_payload: size = cast(Payload, self._body).size if size is not None: self._headers[hdrs.CONTENT_LENGTH] = str(size) else: body_len = len(self._body) if self._body else "0" - self._headers[hdrs.CONTENT_LENGTH] = str(body_len) + # https://www.rfc-editor.org/rfc/rfc9110.html#section-8.6-7 + if body_len != "0" or ( + self.status != 304 and request.method.upper() != hdrs.METH_HEAD + ): + self._headers[hdrs.CONTENT_LENGTH] = str(body_len) return await super()._start(request) diff --git a/tests/test_client_functional.py b/tests/test_client_functional.py index 6ce43d1501e..6b728decbc7 100644 --- a/tests/test_client_functional.py +++ b/tests/test_client_functional.py @@ -70,6 +70,7 @@ async def on_reuseconn(session, ctx, params): app = web.Application() app.router.add_route("GET", "/", handler) + app.router.add_route("HEAD", "/", handler) connector = aiohttp.TCPConnector(limit=1) client = await aiohttp_client( @@ -84,6 +85,74 @@ async def on_reuseconn(session, ctx, params): assert 1 == cnt_conn_reuse +@pytest.mark.parametrize("status", (101, 204, 304)) +async def test_keepalive_after_empty_body_status( + aiohttp_client: Any, status: int +) -> None: + async def handler(request): + body = await request.read() + assert b"" == body + return web.Response(status=status) + + cnt_conn_reuse = 0 + + async def on_reuseconn(session, ctx, params): + nonlocal cnt_conn_reuse + cnt_conn_reuse += 1 + + trace_config = aiohttp.TraceConfig() + trace_config._on_connection_reuseconn.append(on_reuseconn) + + app = web.Application() + app.router.add_route("GET", "/", handler) + + connector = aiohttp.TCPConnector(limit=1) + client = await aiohttp_client( + app, connector=connector, trace_configs=[trace_config] + ) + + resp1 = await client.get("/") + await resp1.read() + resp2 = await client.get("/") + await resp2.read() + + assert cnt_conn_reuse == 1 + + +@pytest.mark.parametrize("status", (101, 204, 304)) +async def test_keepalive_after_empty_body_status_stream_response( + aiohttp_client: Any, status: int +) -> None: + async def handler(request): + stream_response = web.StreamResponse(status=status) + await stream_response.prepare(request) + return stream_response + + cnt_conn_reuse = 0 + + async def on_reuseconn(session, ctx, params): + nonlocal cnt_conn_reuse + cnt_conn_reuse += 1 + + trace_config = aiohttp.TraceConfig() + trace_config._on_connection_reuseconn.append(on_reuseconn) + + app = web.Application() + app.router.add_route("GET", "/", handler) + + connector = aiohttp.TCPConnector(limit=1) + client = await aiohttp_client( + app, connector=connector, trace_configs=[trace_config] + ) + + resp1 = await client.get("/") + await resp1.read() + resp2 = await client.get("/") + await resp2.read() + + assert cnt_conn_reuse == 1 + + async def test_keepalive_response_released(aiohttp_client: Any) -> None: async def handler(request): body = await request.read() @@ -1808,6 +1877,81 @@ async def handler(request): resp.close() +async def test_no_payload_304_with_chunked_encoding(aiohttp_client: Any) -> None: + """Test a 304 response with no payload with chunked set should have it removed.""" + + async def handler(request): + resp = web.StreamResponse(status=304) + resp.enable_chunked_encoding() + resp._length_check = False + resp.headers["Transfer-Encoding"] = "chunked" + writer = await resp.prepare(request) + await writer.write_eof() + return resp + + app = web.Application() + app.router.add_get("/", handler) + client = await aiohttp_client(app) + + resp = await client.get("/") + assert resp.status == 304 + assert hdrs.CONTENT_LENGTH not in resp.headers + assert hdrs.TRANSFER_ENCODING not in resp.headers + await resp.read() + + resp.close() + + +async def test_head_request_with_chunked_encoding(aiohttp_client: Any) -> None: + """Test a head response with chunked set should have it removed.""" + + async def handler(request): + resp = web.StreamResponse(status=200) + resp.enable_chunked_encoding() + resp._length_check = False + resp.headers["Transfer-Encoding"] = "chunked" + writer = await resp.prepare(request) + await writer.write_eof() + return resp + + app = web.Application() + app.router.add_head("/", handler) + client = await aiohttp_client(app) + + resp = await client.head("/") + assert resp.status == 200 + assert hdrs.CONTENT_LENGTH not in resp.headers + assert hdrs.TRANSFER_ENCODING not in resp.headers + await resp.read() + + resp.close() + + +async def test_no_payload_200_with_chunked_encoding(aiohttp_client: Any) -> None: + """Test chunked is preserved on a 200 response with no payload.""" + + async def handler(request): + resp = web.StreamResponse(status=200) + resp.enable_chunked_encoding() + resp._length_check = False + resp.headers["Transfer-Encoding"] = "chunked" + writer = await resp.prepare(request) + await writer.write_eof() + return resp + + app = web.Application() + app.router.add_get("/", handler) + client = await aiohttp_client(app) + + resp = await client.get("/") + assert resp.status == 200 + assert hdrs.CONTENT_LENGTH not in resp.headers + assert hdrs.TRANSFER_ENCODING in resp.headers + await resp.read() + + resp.close() + + async def test_bad_payload_content_length(aiohttp_client: Any) -> None: async def handler(request): resp = web.Response(text="text") diff --git a/tests/test_helpers.py b/tests/test_helpers.py index 65ac54f9f99..536b63de4e7 100644 --- a/tests/test_helpers.py +++ b/tests/test_helpers.py @@ -20,7 +20,9 @@ from aiohttp.helpers import ( is_expected_content_type, method_must_be_empty_body, + must_be_empty_body, parse_http_date, + should_remove_content_length, ) IS_PYPY = platform.python_implementation() == "PyPy" @@ -1082,3 +1084,34 @@ def test_method_must_be_empty_body(): assert method_must_be_empty_body("HEAD") is True # CONNECT is only empty on a successful response assert method_must_be_empty_body("CONNECT") is False + + +def test_should_remove_content_length_is_subset_of_must_be_empty_body(): + """Test should_remove_content_length is always a subset of must_be_empty_body.""" + assert should_remove_content_length("GET", 101) is True + assert must_be_empty_body("GET", 101) is True + + assert should_remove_content_length("GET", 102) is True + assert must_be_empty_body("GET", 102) is True + + assert should_remove_content_length("GET", 204) is True + assert must_be_empty_body("GET", 204) is True + + assert should_remove_content_length("GET", 204) is True + assert must_be_empty_body("GET", 204) is True + + assert should_remove_content_length("GET", 200) is False + assert must_be_empty_body("GET", 200) is False + + assert should_remove_content_length("HEAD", 200) is False + assert must_be_empty_body("HEAD", 200) is True + + # CONNECT is only empty on a successful response + assert should_remove_content_length("CONNECT", 200) is True + assert must_be_empty_body("CONNECT", 200) is True + + assert should_remove_content_length("CONNECT", 201) is True + assert must_be_empty_body("CONNECT", 201) is True + + assert should_remove_content_length("CONNECT", 300) is False + assert must_be_empty_body("CONNECT", 300) is False diff --git a/tests/test_web_functional.py b/tests/test_web_functional.py index 05ca6afb8bf..af8585a57fb 100644 --- a/tests/test_web_functional.py +++ b/tests/test_web_functional.py @@ -13,7 +13,15 @@ from yarl import URL import aiohttp -from aiohttp import FormData, HttpVersion10, HttpVersion11, TraceConfig, multipart, web +from aiohttp import ( + FormData, + HttpVersion, + HttpVersion10, + HttpVersion11, + TraceConfig, + multipart, + web, +) from aiohttp.hdrs import CONTENT_LENGTH, CONTENT_TYPE, TRANSFER_ENCODING from aiohttp.test_utils import make_mocked_coro from aiohttp.typedefs import Handler @@ -136,6 +144,10 @@ async def handler(request): assert 200 == resp.status txt = await resp.text() assert "" == txt + # The Content-Length header should be set to 4 which is + # the length of the response body if it would have been + # returned by a GET request. + assert resp.headers["Content-Length"] == "4" async def test_response_before_complete(aiohttp_client: Any) -> None: @@ -2152,3 +2164,23 @@ async def handler(_): resp = await client.get("/", allow_redirects=False) assert "my-cookie" in resp.cookies await resp.release() + + +@pytest.mark.parametrize("status", (101, 204, 304)) +@pytest.mark.parametrize("version", (HttpVersion10, HttpVersion11)) +async def test_no_body_for_1xx_204_304_responses( + aiohttp_client: Any, status: int, version: HttpVersion +) -> None: + """Test no body is present for for 1xx, 204, and 304 responses.""" + + async def handler(_): + return web.Response(status=status, body=b"should not get to client") + + app = web.Application() + app.router.add_get("/", handler) + client = await aiohttp_client(app, version=version) + resp = await client.get("/") + assert CONTENT_TYPE not in resp.headers + assert TRANSFER_ENCODING not in resp.headers + await resp.read() == b"" + await resp.release() diff --git a/tests/test_web_response.py b/tests/test_web_response.py index 5aeb2a085b5..74c2990cce7 100644 --- a/tests/test_web_response.py +++ b/tests/test_web_response.py @@ -16,7 +16,7 @@ from aiohttp import HttpVersion, HttpVersion10, HttpVersion11, hdrs from aiohttp.helpers import ETag -from aiohttp.http_writer import _serialize_headers +from aiohttp.http_writer import StreamWriter, _serialize_headers from aiohttp.payload import BytesPayload from aiohttp.test_utils import make_mocked_coro, make_mocked_request from aiohttp.web import ContentCoding, Response, StreamResponse, json_response @@ -624,6 +624,74 @@ async def write_headers(status_line, headers): assert resp.content_length is None +@pytest.mark.parametrize("status", (100, 101, 204, 304)) +async def test_rm_transfer_encoding_rfc_9112_6_3_http_11(status: int) -> None: + """Remove transfer encoding for RFC 9112 sec 6.3 with HTTP/1.1.""" + writer = mock.create_autospec(StreamWriter, spec_set=True, instance=True) + req = make_request("GET", "/", version=HttpVersion11, writer=writer) + resp = Response(status=status, headers={hdrs.TRANSFER_ENCODING: "chunked"}) + await resp.prepare(req) + assert resp.content_length == 0 + assert not resp.chunked + assert hdrs.CONTENT_LENGTH not in resp.headers + assert hdrs.TRANSFER_ENCODING not in resp.headers + + +@pytest.mark.parametrize("status", (100, 101, 102, 204, 304)) +async def test_rm_content_length_1xx_204_304_responses(status: int) -> None: + """Remove content length for 1xx, 204, and 304 responses. + + Content-Length is forbidden for 1xx and 204 + https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.2 + + Content-Length is discouraged for 304. + https://datatracker.ietf.org/doc/html/rfc7232#section-4.1 + """ + writer = mock.create_autospec(StreamWriter, spec_set=True, instance=True) + req = make_request("GET", "/", version=HttpVersion11, writer=writer) + resp = Response(status=status, body="answer") + await resp.prepare(req) + assert not resp.chunked + assert hdrs.CONTENT_LENGTH not in resp.headers + assert hdrs.TRANSFER_ENCODING not in resp.headers + + +async def test_head_response_keeps_content_length_of_original_body() -> None: + """Verify HEAD response keeps the content length of the original body HTTP/1.1.""" + writer = mock.create_autospec(StreamWriter, spec_set=True, instance=True) + req = make_request("HEAD", "/", version=HttpVersion11, writer=writer) + resp = Response(status=200, body=b"answer") + await resp.prepare(req) + assert resp.content_length == 6 + assert not resp.chunked + assert resp.headers[hdrs.CONTENT_LENGTH] == "6" + assert hdrs.TRANSFER_ENCODING not in resp.headers + + +async def test_head_response_omits_content_length_when_body_unset() -> None: + """Verify HEAD response omits content-length body when its unset.""" + writer = mock.create_autospec(StreamWriter, spec_set=True, instance=True) + req = make_request("HEAD", "/", version=HttpVersion11, writer=writer) + resp = Response(status=200) + await resp.prepare(req) + assert resp.content_length == 0 + assert not resp.chunked + assert hdrs.CONTENT_LENGTH not in resp.headers + assert hdrs.TRANSFER_ENCODING not in resp.headers + + +async def test_304_response_omits_content_length_when_body_unset() -> None: + """Verify 304 response omits content-length body when its unset.""" + writer = mock.create_autospec(StreamWriter, spec_set=True, instance=True) + req = make_request("GET", "/", version=HttpVersion11, writer=writer) + resp = Response(status=304) + await resp.prepare(req) + assert resp.content_length == 0 + assert not resp.chunked + assert hdrs.CONTENT_LENGTH not in resp.headers + assert hdrs.TRANSFER_ENCODING not in resp.headers + + async def test_content_length_on_chunked() -> None: req = make_request("GET", "/") resp = Response(body=b"answer")