diff --git a/docs/topics/download-handlers.rst b/docs/topics/download-handlers.rst index 2d04b621c..019c75334 100644 --- a/docs/topics/download-handlers.rst +++ b/docs/topics/download-handlers.rst @@ -253,6 +253,9 @@ Other limitations: - HTTPS proxies to HTTPS destinations are not supported. +- A ``Content-Length`` request header is sent in addition to the one built + from the request body, and most servers reject requests with two. + .. _httpx-handler: HttpxDownloadHandler diff --git a/scrapy/core/_http2/stream.py b/scrapy/core/_http2/stream.py index 4a07d198b..f2bc2358c 100644 --- a/scrapy/core/_http2/stream.py +++ b/scrapy/core/_http2/stream.py @@ -247,24 +247,12 @@ class Stream: (":path", path), ] - content_length = str(len(self._request.body)) - headers.append(("Content-Length", content_length)) + if b"Content-Length" not in self._request.headers: + headers.append(("Content-Length", str(len(self._request.body)))) - content_length_name = self._request.headers.normkey(b"Content-Length") for name, values in self._request.headers.items(): for value_bytes in values: - value = str(value_bytes, "utf-8") - if name == content_length_name: - if value != content_length: - logger.warning( - "Ignoring bad Content-Length header %r of request %r, " - "sending %r instead", - value, - self._request, - content_length, - ) - continue - headers.append((str(name, "utf-8"), value)) + headers.append((str(name, "utf-8"), str(value_bytes, "utf-8"))) return headers diff --git a/tests/test_downloader_handler_twisted_http11.py b/tests/test_downloader_handler_twisted_http11.py index db353750d..6a3297b1b 100644 --- a/tests/test_downloader_handler_twisted_http11.py +++ b/tests/test_downloader_handler_twisted_http11.py @@ -48,6 +48,8 @@ class HTTP11DownloadHandlerMixin: } } + handler_supports_custom_content_length = False + def test_not_configured_without_reactor() -> None: crawler = Crawler(Spider, {"TWISTED_REACTOR_ENABLED": False}) diff --git a/tests/test_downloader_handler_twisted_http2.py b/tests/test_downloader_handler_twisted_http2.py index 95e1ff633..428298574 100644 --- a/tests/test_downloader_handler_twisted_http2.py +++ b/tests/test_downloader_handler_twisted_http2.py @@ -2,7 +2,6 @@ from __future__ import annotations -import logging import sys from typing import TYPE_CHECKING, Any @@ -122,33 +121,12 @@ class TestHttp2(H2DownloadHandlerMixin, TestHttpsBase): assert response.body == b"" @coroutine_test - async def test_custom_content_length_good(self, mockserver: MockServer) -> None: + async def test_custom_content_length_bad(self, mockserver: MockServer) -> None: request = Request(mockserver.url("/contentlength", is_secure=self.is_secure)) - custom_content_length = str(len(request.body)) - request.headers["Content-Length"] = custom_content_length + request.headers["Content-Length"] = str(len(request.body) + 1) async with self.get_dh() as download_handler: - response = await download_handler.download_request(request) - assert response.text == custom_content_length - - @coroutine_test - async def test_custom_content_length_bad( - self, caplog: pytest.LogCaptureFixture, mockserver: MockServer - ) -> None: - request = Request(mockserver.url("/contentlength", is_secure=self.is_secure)) - actual_content_length = str(len(request.body)) - bad_content_length = str(len(request.body) + 1) - request.headers["Content-Length"] = bad_content_length - async with self.get_dh() as download_handler: - with caplog.at_level(logging.DEBUG): - response = await download_handler.download_request(request) - assert response.text == actual_content_length - assert ( - "scrapy.core._http2.stream", - logging.WARNING, - f"Ignoring bad Content-Length header " - f"{bad_content_length!r} of request {request}, sending " - f"{actual_content_length!r} instead", - ) in caplog.record_tuples + with pytest.raises(DownloadFailedError): + await download_handler.download_request(request) @coroutine_test async def test_data_loss_handling(self, mockserver: MockServer) -> None: diff --git a/tests/utils/bases/download_handlers_http.py b/tests/utils/bases/download_handlers_http.py index abf153817..8a72fa494 100644 --- a/tests/utils/bases/download_handlers_http.py +++ b/tests/utils/bases/download_handlers_http.py @@ -75,6 +75,9 @@ class TestHttpBase(ABC): # h2.connection.H2Connection.receive_data()), thus closing all streams that # were using it, and we handle this as a normal exception. handler_supports_http2_dataloss: bool = True + # whether the handler sends a request Content-Length header as is, instead + # of building its own (https://github.com/scrapy/scrapy/issues/4919) + handler_supports_custom_content_length: bool = True # What the handler does with a bad response header line, e.g. one with no # colon in it: # "skip-bad": the bad line is skipped and the header lines that follow it @@ -424,6 +427,40 @@ class TestHttpBase(ABC): assert len(contentlengths) == 1 assert contentlengths == [b"0"] + @coroutine_test + async def test_custom_content_length(self, mockserver: MockServer) -> None: + body = b"1" * 100 + request = Request( + mockserver.url("/echo", is_secure=self.is_secure), + method="POST", + body=body, + headers={"Content-Length": str(len(body))}, + ) + async with self.get_dh() as download_handler: + response = await download_handler.download_request(request) + if not self.handler_supports_custom_content_length: + # The server gets two Content-Length headers and rejects the + # request. + assert response.status == 400 + return + echo = json.loads(response.text) + assert Headers(echo["headers"]).getlist("Content-Length") == [b"100"] + assert echo["body"] == body.decode() + + @coroutine_test + async def test_custom_content_length_bodyless(self, mockserver: MockServer) -> None: + request = Request( + mockserver.url("/contentlength", is_secure=self.is_secure), + method="POST", + headers={"Content-Length": "0"}, + ) + async with self.get_dh() as download_handler: + response = await download_handler.download_request(request) + if not self.handler_supports_custom_content_length: + assert response.status == 400 + return + assert response.body == b"0" + @coroutine_test async def test_payload(self, mockserver: MockServer) -> None: body = b"1" * 100 # PayloadResource requires body length to be 100