Ensure empty body response for 1xx/204/304 per RFC 9112 sec 6.3 (#7756)…

… (#7778)

<!-- Thank you for your contribution! -->

`Transfer-Encoding` and `Content-Length` will be removed from the server
response when sending a 1xx (Informational), 204 (No Content), or 304
(Not Modified) status since no body is expected for these status codes

- `Content-Length` forbidden on 1xx/204 per
https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.2 and
discouraged on 304 per
https://datatracker.ietf.org/doc/html/rfc7232#section-4.1

We need to ensure `Content-Length` is kept for `HEAD` to ensure
keep-alive works as expected and is allowed per
https://datatracker.ietf.org/doc/html/rfc2616#section-14.13

- `Transfer-Encoding` removed per
https://datatracker.ietf.org/doc/html/rfc9112#section-6.1
> any recipient on the response chain (including the origin server) can
remove transfer codings when they are not needed.

`aiohttp` would incorrectly send an body of `0\r\n\r\n` when `chunked`
was set for `Transfer-Encoding` with the above status code.

This change ensures `aiohttp` does not send these invalid bodies.

The `Transfer-Encoding` and `Content-Length` headers will be removed
when sending a 1xx (Informational), 204 (No Content), or 304 (Not
Modified) except for `HEAD` responses since these status since no body
is expected for these status codes.

An invalid body of `0\r\n\r\n` will no longer be sent with these status
codes.

<!-- Are there any issues opened that will be resolved by merging this
change? -->

- [x] I think the code is well written
- [x] Unit tests for the changes exist
- [ ] Documentation reflects the changes
- [ ] If you provide code modification, please add yourself to
`CONTRIBUTORS.txt`
  * The format is &lt;Name&gt; &lt;Surname&gt;.
  * Please keep alphabetical order, the file is sorted by names.
- [x] Add a new news fragment into the `CHANGES` folder
  * name it `<issue_id>.<type>` for example (588.bugfix)
* if you don't have an `issue_id` change it to the pr id after creating
the pr
* ensure type is one of the following: * `.feature`: Signifying a new
feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a
documentation improvement. * `.removal`: Signifying a deprecation or
removal of public API.
* `.misc`: A ticket has been closed, but it is not of interest to users.
* Make sure to use full sentences with correct case and punctuation, for
example: "Fix issue with non-ascii contents in doctest text files."

TODO:

- [x] figure out why client closes connection on HEAD requests with
content length
- [x] figure out why client functional tests fail with
AIOHTTP_NO_EXTENSIONS=1 `test_keepalive_after_empty_body_status` and
`test_keepalive_after_empty_body_status_stream_response` -- They do not
fail anymore after the fixed in
#7755
- [x] timeline
- [x] more functional testing

---------

Co-authored-by: Sam Bull <git@sambull.org>
(cherry picked from commit f63cf18)

<!-- Thank you for your contribution! -->

## What do these changes do?

<!-- Please give a short brief about these changes. -->

## Are there changes in behavior for the user?

<!-- Outline any notable behaviour for the end users. -->

## Related issue number

<!-- Are there any issues opened that will be resolved by merging this
change? -->

## Checklist

- [ ] I think the code is well written
- [ ] Unit tests for the changes exist
- [ ] Documentation reflects the changes
- [ ] If you provide code modification, please add yourself to
`CONTRIBUTORS.txt`
  * The format is &lt;Name&gt; &lt;Surname&gt;.
  * Please keep alphabetical order, the file is sorted by names.
- [ ] Add a new news fragment into the `CHANGES` folder
  * name it `<issue_id>.<type>` for example (588.bugfix)
* if you don't have an `issue_id` change it to the pr id after creating
the pr
  * ensure type is one of the following:
    * `.feature`: Signifying a new feature.
    * `.bugfix`: Signifying a bug fix.
    * `.doc`: Signifying a documentation improvement.
    * `.removal`: Signifying a deprecation or removal of public API.
* `.misc`: A ticket has been closed, but it is not of interest to users.
* Make sure to use full sentences with correct case and punctuation, for
example: "Fix issue with non-ascii contents in doctest text files."

CHANGES/7756.bugfix

@@ -0,0 +1 @@
+Ensure empty body response for 1xx/204/304 per RFC 9112 sec 6.3

aiohttp/helpers.py

@@ -970,6 +970,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
@@ -981,3 +990,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)
+    )

aiohttp/web_fileresponse.py

@@ -19,7 +19,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,
@@ -270,7 +270,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")

aiohttp/web_response.py

@@ -30,9 +30,11 @@
     QUOTED_ETAG_RE,
     ETag,
     HeadersMixin,
+    must_be_empty_body,
     parse_http_date,
     rfc822_formatted_time,
     sentinel,
+    should_remove_content_length,
     validate_etag_value,
 )
 from .http import SERVER_SOFTWARE, HttpVersion10, HttpVersion11
@@ -86,6 +88,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] = {}
 
@@ -408,7 +411,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:
@@ -447,26 +450,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)
@@ -722,7 +732,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)
@@ -734,14 +744,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)
 

tests/test_client_functional.py

@@ -8,7 +8,7 @@
 import pathlib
 import socket
 import ssl
-from typing import AsyncIterator
+from typing import Any, AsyncIterator
 from unittest import mock
 
 import pytest
@@ -69,6 +69,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(
@@ -83,7 +84,75 @@ async def on_reuseconn(session, ctx, params):
     assert 1 == cnt_conn_reuse
 
 
-async def test_keepalive_response_released(aiohttp_client) -> None:
+@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()
         assert b"" == body
@@ -1841,7 +1910,82 @@ async def handler(request):
     resp.close()
 
 
-async def test_bad_payload_content_length(aiohttp_client) -> None:
+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")
         resp.headers["Content-Length"] = "10000"

tests/test_helpers.py

@@ -16,7 +16,12 @@
 from yarl import URL
 
 from aiohttp import helpers
-from aiohttp.helpers import method_must_be_empty_body, parse_http_date
+from aiohttp.helpers import (
+    method_must_be_empty_body,
+    must_be_empty_body,
+    parse_http_date,
+    should_remove_content_length,
+)
 
 IS_PYPY = platform.python_implementation() == "PyPy"
 
@@ -916,3 +921,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