Skip to content

Commit

Permalink
Distinguish between truncated and excess content in response (#3273)
Browse files Browse the repository at this point in the history
`HTTPResponse._raw_read` raises `IncompleteRead` if the content length does
not match the expected content length. For malformed responses (e.g. 204
response with content) the re-raised `ProtocolError` was a bit too unclear
about the unexpected excess content being the reason for the exception.

With this change, the exception points out, that the client is not dealing
with a connection error but a protocol violation.
  • Loading branch information
crazyscientist committed Feb 12, 2024
1 parent cfe52f9 commit fa54179
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 4 deletions.
1 change: 1 addition & 0 deletions changelog/3261.misc.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Made raised ``urllib3.exceptions.ProtocolError`` more verbose when a response contains content unexpectedly.
7 changes: 5 additions & 2 deletions src/urllib3/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -252,13 +252,16 @@ class IncompleteRead(HTTPError, httplib_IncompleteRead):
for ``partial`` to avoid creating large objects on streamed reads.
"""

partial: int # type: ignore[assignment]
expected: int

def __init__(self, partial: int, expected: int) -> None:
self.partial = partial # type: ignore[assignment]
self.partial = partial
self.expected = expected

def __repr__(self) -> str:
return "IncompleteRead(%i bytes read, %i more expected)" % (
self.partial, # type: ignore[str-format]
self.partial,
self.expected,
)

Expand Down
12 changes: 11 additions & 1 deletion src/urllib3/response.py
Original file line number Diff line number Diff line change
Expand Up @@ -749,8 +749,18 @@ def _error_catcher(self) -> typing.Generator[None, None, None]:

raise ReadTimeoutError(self._pool, None, "Read timed out.") from e # type: ignore[arg-type]

except IncompleteRead as e:
if (
e.expected is not None
and e.partial is not None
and e.expected == -e.partial
):
arg = "Response may not contain content."
else:
arg = f"Connection broken: {e!r}"
raise ProtocolError(arg, e) from e

except (HTTPException, OSError) as e:
# This includes IncompleteRead.
raise ProtocolError(f"Connection broken: {e!r}", e) from e

# If no exception is thrown, we should avoid cleaning up
Expand Down
23 changes: 22 additions & 1 deletion test/test_response.py
Original file line number Diff line number Diff line change
Expand Up @@ -1293,7 +1293,7 @@ def test_buggy_incomplete_read(self) -> None:

orig_ex = ctx.value.args[1]
assert isinstance(orig_ex, IncompleteRead)
assert orig_ex.partial == 0 # type: ignore[comparison-overlap]
assert orig_ex.partial == 0
assert orig_ex.expected == content_length

def test_incomplete_chunk(self) -> None:
Expand Down Expand Up @@ -1504,6 +1504,27 @@ def make_bad_mac_fp() -> typing.Generator[BytesIO, None, None]:
resp.read()
assert e.value.args[0] == mac_error

def test_unexpected_body(self) -> None:
with pytest.raises(ProtocolError) as excinfo:
fp = BytesIO(b"12345")
headers = {"content-length": "5"}
resp = HTTPResponse(fp, status=204, headers=headers)
resp.read(16)
assert "Response may not contain content" in str(excinfo.value)

with pytest.raises(ProtocolError):
fp = BytesIO(b"12345")
headers = {"content-length": "0"}
resp = HTTPResponse(fp, status=204, headers=headers)
resp.read(16)
assert "Response may not contain content" in str(excinfo.value)

with pytest.raises(ProtocolError):
fp = BytesIO(b"12345")
resp = HTTPResponse(fp, status=204)
resp.read(16)
assert "Response may not contain content" in str(excinfo.value)


class MockChunkedEncodingResponse:
def __init__(self, content: list[bytes]) -> None:
Expand Down

0 comments on commit fa54179

Please sign in to comment.