From 2bdcc44d1e163fb5cc48a8662425e35e15adfe6a Mon Sep 17 00:00:00 2001 From: Illia Volochii Date: Thu, 7 May 2026 18:39:03 +0300 Subject: [PATCH] Merge commit from fork * Avoid any decoding in `HTTPResponse.drain_conn` * Add a comment * Simplify `drain_conn` * Add tests * Add additional checks to the test * Fix full decompression on the 2nd small read from response using Brotli * Add a changelog entry * Inverse the order in the changelog entry * Mention `stream` call --- changelog/GHSA-mf9v-mfxr-j63j.bugfix.rst | 7 +++++++ src/urllib3/response.py | 17 +++++++++++------ test/test_response.py | 24 +++++++++++++++++++++--- test/with_dummyserver/test_connection.py | 19 +++++++++++++++++++ 4 files changed, 58 insertions(+), 9 deletions(-) create mode 100644 changelog/GHSA-mf9v-mfxr-j63j.bugfix.rst Index: urllib3-2.5.0/changelog/GHSA-mf9v-mfxr-j63j.bugfix.rst =================================================================== --- /dev/null +++ urllib3-2.5.0/changelog/GHSA-mf9v-mfxr-j63j.bugfix.rst @@ -0,0 +1,7 @@ +Fixed two high-severity security issues where decompression-bomb safeguards of the streaming API were bypassed: + + +1. When ``HTTPResponse.drain_conn()`` was called after the response had been read and decompressed partially. +2. During the second ``HTTPResponse.read(amt=N)`` or ``HTTPResponse.stream(amt=N)`` call when the response was decompressed using the official `Brotli `__ library. + +See `GHSA-mf9v-mfxr-j63j `__ for details. Index: urllib3-2.5.0/src/urllib3/response.py =================================================================== --- urllib3-2.5.0.orig/src/urllib3/response.py +++ urllib3-2.5.0/src/urllib3/response.py @@ -842,13 +842,14 @@ class HTTPResponse(BaseHTTPResponse): Unread data in the HTTPResponse connection blocks the connection from being released back to the pool. """ try: - self.read( - # Do not spend resources decoding the content unless - # decoding has already been initiated. - decode_content=self._has_decoded_content, - ) + self._raw_read() except (HTTPError, OSError, BaseSSLError, HTTPException): pass + if self._has_decoded_content: + # `_raw_read` skips decompression, so we should clean up the + # decoder to avoid keeping unnecessary data in memory. + self._decoded_buffer = BytesQueueBuffer() + self._decoder = None @property def data(self) -> bytes: @@ -1143,7 +1144,11 @@ class HTTPResponse(BaseHTTPResponse): elif amt is not None: cache_content = False - if self._decoder and self._decoder.has_unconsumed_tail: + if ( + self._decoder + and self._decoder.has_unconsumed_tail + and len(self._decoded_buffer) < amt + ): decoded_data = self._decode( b"", decode_content, Index: urllib3-2.5.0/test/test_response.py =================================================================== --- urllib3-2.5.0.orig/test/test_response.py +++ urllib3-2.5.0/test/test_response.py @@ -769,22 +769,33 @@ class TestResponse: pytest.skip(f"Proper {request.node.callspec.id} decoder is not available") name, compressed_data = data - limit = 1024 * 1024 # 1 MiB + limit1 = 1024 * 1024 # 1 MiB + # We test with two read calls because the second call may be + # able to use the internal buffer filled by the first call, and + # we want to ensure that full decompression is never triggered + # by the second call. The limit for the second call is lowered + # to make sure that the internal buffer is used for the Brotli + # case specifically https://github.com/google/brotli/issues/1396 + limit2 = 1024 # 1 KiB if read_method in ("read_chunked", "stream"): httplib_r = httplib.HTTPResponse(MockSock) # type: ignore[arg-type] + httplib_r.chunked = True + httplib_r.chunk_left = 1 httplib_r.fp = MockChunkedEncodingResponse([compressed_data]) # type: ignore[assignment] r = HTTPResponse( httplib_r, preload_content=False, headers={"transfer-encoding": "chunked", "content-encoding": name}, ) - next(getattr(r, read_method)(amt=limit, decode_content=True)) + for limit in (limit1, limit2): + next(getattr(r, read_method)(amt=limit, decode_content=True)) else: fp = BytesIO(compressed_data) r = HTTPResponse( fp, headers={"content-encoding": name}, preload_content=False ) - getattr(r, read_method)(amt=limit, decode_content=True) + for limit in (limit1, limit2): + getattr(r, read_method)(amt=limit, decode_content=True) # Check that the internal decoded buffer is empty unless brotli # is used. @@ -794,6 +805,13 @@ class TestResponse: if name != "br" or brotli.__name__ == "brotlicffi": assert len(r._decoded_buffer) == 0 + # Check that memory usage is still within the limit while the + # connection is being drained, meaning that the call does not + # decompress the whole content. + r.drain_conn() + assert r._decoder is None + assert len(r._decoded_buffer) == 0 + def test_multi_decoding_deflate_deflate(self) -> None: data = zlib.compress(zlib.compress(b"foo")) Index: urllib3-2.5.0/test/with_dummyserver/test_connection.py =================================================================== --- urllib3-2.5.0.orig/test/with_dummyserver/test_connection.py +++ urllib3-2.5.0/test/with_dummyserver/test_connection.py @@ -138,3 +138,22 @@ def test_invalid_tunnel_scheme(pool: HTT str(e.value) == "Invalid proxy scheme for tunneling: 'socks', must be either 'http' or 'https'" ) + + +def test_response_after_drain_conn(pool: HTTPConnectionPool) -> None: + """ + Test that a connection can be reused after calling `drain_conn` on + an unread response. + """ + conn = pool._get_conn() + + conn.request("GET", "/", preload_content=False) + response = conn.getresponse() + assert response.status == 200 + response.drain_conn() + + conn.request("GET", "/", preload_content=False) + response = conn.getresponse() + assert response.status == 200 + + conn.close()