Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
65 commits
Select commit Hold shift + click to select a range
62586b2
Remove Transfer-Encoding for empty body response per RFC 9112 sec 6.3
bdraco Oct 27, 2023
dce3e25
review
bdraco Oct 27, 2023
e3db2b8
make enable_chunking conditional on status_code_indicates_empty_body
bdraco Oct 27, 2023
3fb3f79
Handle HEAD response as well
bdraco Oct 27, 2023
8f9f752
move asserts out of side effect (keep mock since it needs to be async)
bdraco Oct 27, 2023
65d711f
fix regression
bdraco Oct 27, 2023
efcbcaf
remove unreachable
bdraco Oct 27, 2023
9c8c8fa
add functional tests
bdraco Oct 27, 2023
8ed3c5e
add a head test as well
bdraco Oct 27, 2023
028e8a2
reduce comment
bdraco Oct 27, 2023
9d54c05
reduce comment
bdraco Oct 27, 2023
d60e402
add test for empty body status
bdraco Oct 28, 2023
ce8e12e
fixes for empty body with client
bdraco Oct 28, 2023
591875d
must_be_empty_body
bdraco Oct 28, 2023
5b557b9
move must_be_empty_body block out one level
bdraco Oct 28, 2023
ae2f8d1
dry
bdraco Oct 28, 2023
66443af
one more place
bdraco Oct 28, 2023
1f1b241
http/1.0 does allow 0
bdraco Oct 28, 2023
04e6cfa
Add connect to _must_be_empty_body
bdraco Oct 28, 2023
7975313
adjust test
bdraco Oct 28, 2023
b8d5991
simplify mocking
bdraco Oct 28, 2023
d94f959
dry
bdraco Oct 28, 2023
66befbd
fix http/1.0 keep alive with empty body
bdraco Oct 28, 2023
fbd91c6
stale debug
bdraco Oct 28, 2023
9ef28a0
assert order
bdraco Oct 28, 2023
772b33b
missed one
bdraco Oct 28, 2023
f3853f0
typo
bdraco Oct 28, 2023
5ff372a
adjust test for zero length
bdraco Oct 28, 2023
a71eaf2
dry
bdraco Oct 28, 2023
50f534f
refactor for rfc7230 section-3.3.2
bdraco Oct 28, 2023
68f3d12
revert
bdraco Oct 28, 2023
1cf5b38
more reverts
bdraco Oct 28, 2023
45e303c
document
bdraco Oct 29, 2023
eab7c2b
fixes
bdraco Oct 29, 2023
856b388
adjust test
bdraco Oct 29, 2023
0080a6a
adjust test
bdraco Oct 29, 2023
f413083
explict test for 204/304
bdraco Oct 29, 2023
233bced
explict test for 204/304
bdraco Oct 29, 2023
c76368f
explict test for 204/304
bdraco Oct 29, 2023
1329334
explict test for 204/304
bdraco Oct 29, 2023
a2e4976
explict test for 101/204/304
bdraco Oct 29, 2023
78c8742
explict test for 1xx
bdraco Oct 29, 2023
7e1712e
it was still wrong since one is not always a subset of
bdraco Oct 29, 2023
9d32134
fix order
bdraco Oct 29, 2023
253db8a
remove comment since its moved to helper and better explained
bdraco Oct 29, 2023
ff7c712
remove comment since its moved to helper and better explained
bdraco Oct 29, 2023
f9e831a
make sure no body
bdraco Oct 29, 2023
8d47fef
sync with client side pr
bdraco Oct 30, 2023
2f6cade
Merge branch 'master' into ignore_transfer_encoding_304_rfc_9112_sq
bdraco Oct 30, 2023
90fa474
Merge branch 'master' into ignore_transfer_encoding_304_rfc_9112_sq
bdraco Oct 30, 2023
3041a31
timeline
bdraco Oct 30, 2023
7a09009
Update spec links
Dreamsorcerer Oct 31, 2023
bea21de
Update aiohttp/web_response.py
bdraco Oct 31, 2023
8f7c0e2
simplify check
bdraco Oct 31, 2023
a7dc41c
remove superfluous close
bdraco Oct 31, 2023
5630ca5
adjust logic for empty body on 200 with connect
bdraco Oct 31, 2023
79d55e2
sync and adjust test
bdraco Oct 31, 2023
286a108
adjust CONNECT check for 2xx
bdraco Oct 31, 2023
e851e0a
handle head with body unset
bdraco Oct 31, 2023
02f22fa
adjust test_head_returns_empty_body
bdraco Oct 31, 2023
15c9814
fix 304 test
bdraco Oct 31, 2023
ae1c29a
Merge branch 'master' into ignore_transfer_encoding_304_rfc_9112_sq
bdraco Oct 31, 2023
3e5a6ee
ruff
bdraco Oct 31, 2023
cccbdb3
make it just a bit safer
bdraco Oct 31, 2023
8eebfe1
Update links
Dreamsorcerer Nov 2, 2023
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
1 change: 1 addition & 0 deletions CHANGES/7756.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Ensure empty body response for 1xx/204/304 per RFC 9112 sec 6.3
23 changes: 23 additions & 0 deletions aiohttp/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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}
Dreamsorcerer marked this conversation as resolved.
Show resolved Hide resolved
or 100 <= code < 200
or (200 <= code < 300 and method.upper() == hdrs.METH_CONNECT)
)
4 changes: 2 additions & 2 deletions aiohttp/web_fileresponse.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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")
Expand Down
52 changes: 35 additions & 17 deletions aiohttp/web_response.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -77,6 +79,7 @@
"_req",
"_payload_writer",
"_eof_sent",
"_must_be_empty_body",
"_body_length",
"_state",
"_headers",
Expand Down Expand Up @@ -104,6 +107,7 @@
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] = {}

Expand Down Expand Up @@ -333,7 +337,7 @@
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:
Expand Down Expand Up @@ -370,26 +374,33 @@
"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)
Expand Down Expand Up @@ -650,7 +661,7 @@
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)
Expand All @@ -662,14 +673,21 @@
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]

Check warning on line 678 in aiohttp/web_response.py

View check run for this annotation

Codecov / codecov/patch

aiohttp/web_response.py#L678

Added line #L678 was not covered by tests
elif not self._chunked and hdrs.CONTENT_LENGTH not in self._headers:
Dreamsorcerer marked this conversation as resolved.
Show resolved Hide resolved
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)

Expand Down
144 changes: 144 additions & 0 deletions tests/test_client_functional.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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()
Expand Down Expand Up @@ -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")
Expand Down
33 changes: 33 additions & 0 deletions tests/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Loading
Loading