diff --git a/docs/topics/download-handlers.rst b/docs/topics/download-handlers.rst index 94e75ab6f..433a6d139 100644 --- a/docs/topics/download-handlers.rst +++ b/docs/topics/download-handlers.rst @@ -130,17 +130,23 @@ using different handlers. Here is a comparison of some features of the built-in HTTP handlers, see the individual handler docs for more differences: -================== ================= ===================== ==================== -Feature H2DownloadHandler HTTP11DownloadHandler HttpxDownloadHandler -================== ================= ===================== ==================== -Requires asyncio No No Yes -Requires a reactor Yes Yes No -HTTP/1.1 No Yes Yes -HTTP/2 Yes No Yes -TLS implementation ``cryptography`` ``cryptography`` Stdlib ``ssl`` -HTTP proxies No Yes Yes -SOCKS proxies No No Yes -================== ================= ===================== ==================== +=================== ================= ===================== ==================== +Feature H2DownloadHandler HTTP11DownloadHandler HttpxDownloadHandler +=================== ================= ===================== ==================== +Requires asyncio No No Yes +Requires a reactor Yes Yes No +HTTP/1.1 No Yes Yes +HTTP/2 Yes No Yes +TLS implementation ``cryptography`` ``cryptography`` Stdlib ``ssl`` +HTTP proxies No Yes Yes +SOCKS proxies No No Yes +Bad header handling Not applicable Skip bad Fail +=================== ================= ===================== ==================== + +Bad header handling is what a handler does when a response has a bad header +line, e.g. one with no colon in it, which some servers send. Handlers that skip +bad header lines, like web browsers do, still parse the header lines that follow +them; other handlers also lose those, or cannot download such responses at all. You can find additional HTTP download handlers in the scrapy-download-handlers-incubator_ package. This package is made by the Scrapy @@ -191,6 +197,7 @@ Features and limitations HTTP proxies No (not implemented) SOCKS proxies No (not supported by the library) HTTP/2 Yes +Bad header handling Not applicable (HTTP/2 only) ``response.certificate`` :class:`twisted.internet.ssl.Certificate` object Per-request ``bindaddress`` Yes TLS implementation ``pyOpenSSL``/``cryptography`` @@ -239,11 +246,16 @@ Features and limitations HTTP proxies Yes SOCKS proxies No (not supported by the library) HTTP/2 No (implemented as a separate handler) +Bad header handling Skip bad, like web browsers do ``response.certificate`` :class:`twisted.internet.ssl.Certificate` object Per-request ``bindaddress`` Yes TLS implementation ``pyOpenSSL``/``cryptography`` =========================== ================================================ +.. versionchanged:: VERSION + Bad header lines with no colon in them are now skipped, instead of making + the whole response impossible to download. + Other limitations: - IPv6 support requires setting :setting:`TWISTED_DNS_RESOLVER` @@ -297,6 +309,7 @@ Features and limitations HTTP proxies Yes SOCKS proxies Yes (SOCKS5) HTTP/2 Yes +Bad header handling Fail (not supported by the library) ``response.certificate`` DER bytes Per-request ``bindaddress`` No (not supported by the library) TLS implementation Standard library ``ssl`` diff --git a/docs/topics/extensions.rst b/docs/topics/extensions.rst index 78b38cc3f..6d61cc342 100644 --- a/docs/topics/extensions.rst +++ b/docs/topics/extensions.rst @@ -374,8 +374,8 @@ This extension periodically logs rich stat data as a JSON object:: "elapsed": 360.008903, "log_interval": 60.0, "log_interval_real": 60.006694, - "start_time": "2023-08-03 23:24:57", - "utcnow": "2023-08-03 23:30:57" + "start_time": "2023-08-03T23:24:57.148903+00:00", + "utcnow": "2023-08-03T23:30:57.157806+00:00" } } diff --git a/docs/topics/feed-exports.rst b/docs/topics/feed-exports.rst index 2f686fd0f..467abc989 100644 --- a/docs/topics/feed-exports.rst +++ b/docs/topics/feed-exports.rst @@ -104,7 +104,8 @@ storage backend types which are defined by the URI scheme. The storages backends supported out of the box are: - :ref:`topics-feed-storage-fs` -- :ref:`topics-feed-storage-ftp` +- :ref:`feed-storage-ftp` +- :ref:`feed-storage-ftps` - :ref:`topics-feed-storage-s3` (requires the :ref:`s3 ` extra) - :ref:`topics-feed-storage-gcs` (requires the :ref:`gcs ` extra) - :ref:`topics-feed-storage-stdout` @@ -168,6 +169,7 @@ you specify a path (e.g. ``/tmp/export.csv``). Alternatively you can also use a :class:`pathlib.Path` object. .. _topics-feed-storage-ftp: +.. _feed-storage-ftp: FTP --- @@ -178,6 +180,9 @@ The feeds are stored in a FTP server. - Example URI: ``ftp://user:pass@ftp.example.com/path/to/export.csv`` - Required external libraries: none +FTP sends credentials and data in cleartext. Use :ref:`feed-storage-ftps` +instead where possible. + FTP supports two different connection modes: `active or passive `_. Scrapy uses the passive connection mode by default. To use the active connection mode instead, set the @@ -192,6 +197,28 @@ storage backend is: ``True``. This storage backend uses :ref:`delayed file delivery `. +.. _feed-storage-ftps: + +FTPS +---- + +The feeds are stored in a FTP server, over a TLS connection, with the +certificate of the server verified. + +.. versionadded:: VERSION + +- URI scheme: ``ftps`` +- Example URI: ``ftps://user:pass@ftp.example.com/path/to/export.csv`` +- Required external libraries: none + +See :ref:`feed-storage-ftp` for connection modes, the ``overwrite`` default and +file delivery. + +.. note:: For SFTP, an unrelated protocol built on SSH, use + `scrapy-feedexporter-sftp + `_. + + .. _topics-feed-storage-s3: S3 @@ -502,7 +529,7 @@ as a fallback value if that key is not provided for a specific feed definition: - :ref:`topics-feed-storage-fs`: ``False`` - - :ref:`topics-feed-storage-ftp`: ``True`` + - :ref:`feed-storage-ftp` and :ref:`feed-storage-ftps`: ``True`` .. note:: Some FTP servers may not support appending to files (the ``APPE`` FTP command). @@ -624,6 +651,7 @@ Default: "s3": "scrapy.extensions.feedexport.S3FeedStorage", "gs": "scrapy.extensions.feedexport.GCSFeedStorage", "ftp": "scrapy.extensions.feedexport.FTPFeedStorage", + "ftps": "scrapy.extensions.feedexport.FTPFeedStorage", } A dict containing the built-in feed storage backends supported by Scrapy. You diff --git a/docs/topics/settings.rst b/docs/topics/settings.rst index fccc391e7..cb803abb6 100644 --- a/docs/topics/settings.rst +++ b/docs/topics/settings.rst @@ -1446,7 +1446,7 @@ FEED_TEMPDIR Default: ``None`` The Feed Temp dir allows you to set a custom folder to save crawler -temporary files before uploading with :ref:`FTP feed storage ` and +temporary files before uploading with :ref:`FTP feed storage ` and :ref:`Amazon S3 `. .. setting:: FEED_STORAGE_GCS_ACL diff --git a/scrapy/core/downloader/handlers/http11.py b/scrapy/core/downloader/handlers/http11.py index f10c9cbc8..285c47c81 100644 --- a/scrapy/core/downloader/handlers/http11.py +++ b/scrapy/core/downloader/handlers/http11.py @@ -17,7 +17,12 @@ from twisted.internet.defer import Deferred, succeed from twisted.internet.endpoints import TCP4ClientEndpoint from twisted.internet.protocol import Factory, Protocol, connectionDone from twisted.python.failure import Failure -from twisted.web._newclient import HTTP11ClientProtocol +from twisted.web._newclient import ( + HEADER, + STATUS, + HTTP11ClientProtocol, + HTTPClientParser, +) from twisted.web.client import ( URI, Agent, @@ -65,6 +70,7 @@ if TYPE_CHECKING: from twisted.internet.base import ReactorBase from twisted.internet.interfaces import IAddress, IConsumer + from twisted.web._newclient import Request as TxRequest # typing.NotRequired requires Python 3.11 from typing_extensions import NotRequired @@ -86,7 +92,72 @@ class _ResultT(TypedDict): stop_download: NotRequired[StopDownload | None] -class _TrackedHTTP11ClientProtocol(HTTP11ClientProtocol): +class _LenientHTTPClientParser(HTTPClientParser): + """Response parser that skips bad response header lines, those with no + colon in them, instead of failing to parse the whole response. + + Some servers send such lines, and web browsers skip them and keep parsing + the header lines that follow. See + https://github.com/scrapy/scrapy/issues/210. + """ + + def lineReceived(self, line: bytes) -> None: + # A copy of twisted.web._newclient.HTTPParser.lineReceived() where the + # header name and value are only extracted from header lines that have + # a colon. + + # Handle the normal CR LF case. + if line[-1:] == b"\r": + line = line[:-1] + + if self.state == STATUS: + self.statusReceived(line) # type: ignore[no-untyped-call] + self.state = HEADER + return + + # HEADER is the only other state in which lines are received, as the + # parser switches to raw mode for the response body. + if not line or line[0] not in b" \t": + if self._partialHeader is not None: + header = b"".join(self._partialHeader) + if b":" in header: + name, value = header.split(b":", 1) + self.headerReceived(name, value.strip()) # type: ignore[no-untyped-call] + else: + logger.debug( + f"Skipping the bad response header line {header!r}, as " + f"it has no colon." + ) + if not line: + # Empty line means the header section is over. + self.allHeadersReceived() # type: ignore[no-untyped-call] + else: + # Line not beginning with LWS is another header. + self._partialHeader = [line] + else: + # A line beginning with LWS is a continuation of a header begun on + # a previous line. + self._partialHeader.append(line) # type: ignore[union-attr] + + +class _LenientHTTP11ClientProtocol(HTTP11ClientProtocol): + """Protocol that parses responses with :class:`_LenientHTTPClientParser`.""" + + def request(self, request: TxRequest) -> Deferred[IResponse]: + d: Deferred[IResponse] = super().request(request) + # HTTP11ClientProtocol.request() hardcodes the parser class, so the + # only way to use a different one is to replace the class of the parser + # object that it creates. This is safe because + # _LenientHTTPClientParser defines no additional state. The parser is + # always there because HTTPConnectionPool only reuses connections whose + # protocol is in the QUIESCENT state, for which request() always + # creates a parser. + assert self._parser is not None + self._parser.__class__ = _LenientHTTPClientParser + return d + + +class _TrackedHTTP11ClientProtocol(_LenientHTTP11ClientProtocol): """Connection that tells the pool that opened it when it closes, whether it was cached at the time or in use.""" diff --git a/scrapy/extensions/feedexport.py b/scrapy/extensions/feedexport.py index 32abc82d3..874023e25 100644 --- a/scrapy/extensions/feedexport.py +++ b/scrapy/extensions/feedexport.py @@ -363,6 +363,7 @@ class FTPFeedStorage(BlockingFeedStorage): self.username: str = u.username or "" self.password: str = unquote(u.password or "") self.path: str = u.path + self.tls: bool = u.scheme == "ftps" self.use_active_mode: bool = use_active_mode self.overwrite: bool = not feed_options or feed_options.get("overwrite", True) @@ -390,6 +391,7 @@ class FTPFeedStorage(BlockingFeedStorage): password=self.password, use_active_mode=self.use_active_mode, overwrite=self.overwrite, + tls=self.tls, ) diff --git a/scrapy/settings/default_settings.py b/scrapy/settings/default_settings.py index bdb9d2968..fb2a58959 100644 --- a/scrapy/settings/default_settings.py +++ b/scrapy/settings/default_settings.py @@ -382,6 +382,7 @@ FEED_STORAGES_BASE = { "": "scrapy.extensions.feedexport.FileFeedStorage", "file": "scrapy.extensions.feedexport.FileFeedStorage", "ftp": "scrapy.extensions.feedexport.FTPFeedStorage", + "ftps": "scrapy.extensions.feedexport.FTPFeedStorage", "gs": "scrapy.extensions.feedexport.GCSFeedStorage", "s3": "scrapy.extensions.feedexport.S3FeedStorage", "stdout": "scrapy.extensions.feedexport.StdoutFeedStorage", diff --git a/scrapy/utils/ftp.py b/scrapy/utils/ftp.py index a3e7a4306..f08e5303d 100644 --- a/scrapy/utils/ftp.py +++ b/scrapy/utils/ftp.py @@ -1,7 +1,8 @@ import posixpath from contextlib import closing -from ftplib import FTP, error_perm +from ftplib import FTP, FTP_TLS, error_perm from posixpath import dirname +from ssl import create_default_context from typing import IO @@ -29,13 +30,20 @@ def ftp_store_file( password: str, use_active_mode: bool = False, overwrite: bool = True, + tls: bool = False, ) -> None: - """Opens a FTP connection with passed credentials,sets current directory - to the directory extracted from given path, then uploads the file to server + """Opens a FTP connection with passed credentials, sets current directory + to the directory extracted from given path, then uploads the file to server. + + If *tls* is ``True``, the connection is secured with TLS (FTPS), and the + certificate of the server is verified. """ - with FTP() as ftp, closing(file): + ftp = FTP_TLS(context=create_default_context()) if tls else FTP() + with ftp, closing(file): ftp.connect(host, port) ftp.login(username, password) + if isinstance(ftp, FTP_TLS): + ftp.prot_p() if use_active_mode: ftp.set_pasv(False) file.seek(0) diff --git a/scrapy/utils/serialize.py b/scrapy/utils/serialize.py index 5d06bbe30..803b63bef 100644 --- a/scrapy/utils/serialize.py +++ b/scrapy/utils/serialize.py @@ -10,18 +10,11 @@ from scrapy.http import Request, Response class ScrapyJSONEncoder(json.JSONEncoder): - DATE_FORMAT = "%Y-%m-%d" - TIME_FORMAT = "%H:%M:%S" - def default(self, o: Any) -> Any: if isinstance(o, set): return list(o) - if isinstance(o, datetime.datetime): - return o.strftime(f"{self.DATE_FORMAT} {self.TIME_FORMAT}") - if isinstance(o, datetime.date): - return o.strftime(self.DATE_FORMAT) - if isinstance(o, datetime.time): - return o.strftime(self.TIME_FORMAT) + if isinstance(o, (datetime.datetime, datetime.date, datetime.time)): + return o.isoformat() if isinstance(o, decimal.Decimal): return str(o) if isinstance(o, defer.Deferred): diff --git a/tests/keys/__init__.py b/tests/keys/__init__.py index 9b73ca4f0..804c9b6a8 100644 --- a/tests/keys/__init__.py +++ b/tests/keys/__init__.py @@ -1,4 +1,5 @@ from datetime import datetime, timedelta, timezone +from ipaddress import IPv4Address from pathlib import Path from cryptography.hazmat.backends import default_backend @@ -12,6 +13,7 @@ from cryptography.hazmat.primitives.serialization import ( from cryptography.x509 import ( CertificateBuilder, DNSName, + IPAddress, Name, NameAttribute, SubjectAlternativeName, @@ -53,7 +55,9 @@ def generate_keys(): .not_valid_before(datetime.now(tz=timezone.utc)) .not_valid_after(datetime.now(tz=timezone.utc) + timedelta(days=10)) .add_extension( - SubjectAlternativeName([DNSName("localhost")]), + SubjectAlternativeName( + [DNSName("localhost"), IPAddress(IPv4Address("127.0.0.1"))] + ), critical=False, ) .sign(key, SHA256(), default_backend()) diff --git a/tests/mockserver/ftp.py b/tests/mockserver/ftp.py index 72760b5ac..d88e953a7 100644 --- a/tests/mockserver/ftp.py +++ b/tests/mockserver/ftp.py @@ -10,7 +10,7 @@ from tempfile import mkdtemp from typing import TYPE_CHECKING from pyftpdlib.authorizers import DummyAuthorizer -from pyftpdlib.handlers import FTPHandler +from pyftpdlib.handlers import FTPHandler, TLS_FTPHandler from pyftpdlib.servers import FTPServer from tests.utils import get_script_run_env @@ -25,28 +25,32 @@ if TYPE_CHECKING: class MockFTPServer: """Creates an FTP server on a random port with a default passwordless user (anonymous) and a temporary root path that you can read from the - :attr:`path` attribute.""" + :attr:`path` attribute. + + If *tls* is ``True``, the server requires FTPS, using the test certificate + from :file:`tests/keys`. + """ proc: Popen[str] port: int path: Path - def __init__(self) -> None: + def __init__(self, tls: bool = False) -> None: self.host: str = "127.0.0.1" + self.tls: bool = tls def __enter__(self) -> Self: self.path = Path(mkdtemp()) self.proc = Popen( - [sys.executable, "-u", "-m", "tests.mockserver.ftp", "-d", str(self.path)], + [sys.executable, "-u", "-m", "tests.mockserver.ftp", "-d", str(self.path)] + + (["--tls"] if self.tls else []), stderr=PIPE, env=get_script_run_env(), text=True, ) assert self.proc.stderr is not None for line in self.proc.stderr: - if "starting FTP server" in line and ( - m := re.search(r"starting FTP server on ([^ :]+):(\d+),", line) - ): + if m := re.search(r"starting FTPS? .*on ([^ :]+):(\d+),", line): self.port = int(m.group(2)) break else: @@ -68,18 +72,28 @@ class MockFTPServer: self.proc.communicate() def url(self, path: str) -> str: - return f"ftp://{self.host}:{self.port}/{path}" + scheme = "ftps" if self.tls else "ftp" + return f"{scheme}://{self.host}:{self.port}/{path}" def main() -> None: parser = ArgumentParser() parser.add_argument("-d", "--directory", required=True) + parser.add_argument("--tls", action="store_true") args = parser.parse_args() authorizer = DummyAuthorizer() full_permissions = "elradfmwMT" authorizer.add_anonymous(args.directory, perm=full_permissions) - handler = FTPHandler + if args.tls: + keys = Path(__file__).parent.parent / "keys" + handler = TLS_FTPHandler + handler.certfile = str(keys / "localhost.crt") + handler.keyfile = str(keys / "localhost.key") + handler.tls_control_required = True + handler.tls_data_required = True + else: + handler = FTPHandler handler.authorizer = authorizer address = ("127.0.0.1", 0) server = FTPServer(address, handler) diff --git a/tests/mockserver/http.py b/tests/mockserver/http.py index 81e74dce1..350153393 100644 --- a/tests/mockserver/http.py +++ b/tests/mockserver/http.py @@ -11,6 +11,7 @@ from tests import tests_datadir from .http_base import BaseMockServer, main_factory from .http_resources import ( ArbitraryLengthPayloadResource, + BadHeader, BaseResource, BrokenChunkedResource, BrokenDownloadResource, @@ -53,6 +54,7 @@ class Root(BaseResource): put_child(self, b"partial", Partial()) put_child(self, b"drop", Drop()) put_child(self, b"raw", Raw()) + put_child(self, b"bad-header", BadHeader()) put_child(self, b"echo", Echo()) put_child(self, b"payload", PayloadResource()) put_child(self, b"alpayload", ArbitraryLengthPayloadResource()) diff --git a/tests/mockserver/http_resources.py b/tests/mockserver/http_resources.py index a1c3928d0..68c14af83 100644 --- a/tests/mockserver/http_resources.py +++ b/tests/mockserver/http_resources.py @@ -211,6 +211,39 @@ class Raw(LeafResource): request.finish() +class BadHeader(LeafResource): + """Sends a response with a bad header line, one with no colon in it, like + some servers do, between two good ones. + + One of the good header lines is split into two lines, so that handling of + such headers is also covered. + """ + + response = ( + b"HTTP/1.1 200 OK\r\n" + b"Content-Length: 5\r\n" + b"Content-Type: text/html\r\n" + b"X-Folded-Header: one\r\n" + b"\ttwo\r\n" + b'\r\n' + b"X-After-Bad-Header: works\r\n" + b"\r\n" + b"Works" + ) + + def render_GET(self, request: Request) -> int: + request.startedWriting = 1 + self.deferRequest(request, 0, self._delayedRender, request) + return NOT_DONE_YET + + def _delayedRender(self, request: Request) -> None: + request.write(self.response) + # Clients that stop parsing headers at the bad one don't get + # Content-Length, so they need the connection to be closed to know that + # the response body is over. + close_connection(request) + + class Echo(LeafResource): def render_GET(self, request: Request) -> bytes: assert request.content diff --git a/tests/test_downloader_handler_httpx.py b/tests/test_downloader_handler_httpx.py index 25d63281d..21326a4b0 100644 --- a/tests/test_downloader_handler_httpx.py +++ b/tests/test_downloader_handler_httpx.py @@ -61,6 +61,7 @@ class HttpxDownloadHandlerMixin: class TestHttp(HttpxDownloadHandlerMixin, TestHttpBase): handler_supports_bindaddress_meta = False + handler_bad_header_handling = "fail" @pytest.mark.skipif( sys.platform == "darwin", @@ -82,6 +83,7 @@ class TestHttp(HttpxDownloadHandlerMixin, TestHttpBase): class TestHttps(HttpxDownloadHandlerMixin, TestHttpsBase): handler_supports_bindaddress_meta = False + handler_bad_header_handling = "fail" tls_log_message = "SSL connection to 127.0.0.1 using protocol TLSv1.3, cipher" @pytest.mark.skip(reason="The check is Twisted-specific") diff --git a/tests/test_exporters.py b/tests/test_exporters.py index 956697a04..4359f4ff4 100644 --- a/tests/test_exporters.py +++ b/tests/test_exporters.py @@ -578,7 +578,7 @@ class TestJsonLinesItemExporter(TestBaseItemExporter): self.ie.finish_exporting() del self.ie # See the first “del self.ie” in this file for context. exported = json.loads(to_unicode(self.output.getvalue())) - item["time"] = str(item["time"]) + item["time"] = item["time"].isoformat() assert exported == item @@ -661,7 +661,7 @@ class TestJsonItemExporter(TestJsonLinesItemExporter): self.ie.finish_exporting() del self.ie # See the first “del self.ie” in this file for context. exported = json.loads(to_unicode(self.output.getvalue())) - item["time"] = str(item["time"]) + item["time"] = item["time"].isoformat() assert exported == [item] diff --git a/tests/test_feedexport_storages.py b/tests/test_feedexport_storages.py index d7fb5c9c2..23b39431e 100644 --- a/tests/test_feedexport_storages.py +++ b/tests/test_feedexport_storages.py @@ -7,6 +7,7 @@ import sys import tempfile from io import BytesIO from pathlib import Path +from ssl import SSLCertVerificationError from typing import IO, Any from unittest import mock from urllib.parse import quote @@ -169,6 +170,24 @@ class TestFTPFeedStorage: await self._store(url, b"bar", settings=settings) self._assert_stored(ftp_server.path / filename, b"bar") + @coroutine_test + async def test_tls(self, monkeypatch): + monkeypatch.setenv( + "SSL_CERT_FILE", str(Path(__file__).parent / "keys" / "localhost.crt") + ) + with MockFTPServer(tls=True) as ftp_server: + filename = "file" + await self._store(ftp_server.url(filename), b"foo") + self._assert_stored(ftp_server.path / filename, b"foo") + + @coroutine_test + async def test_tls_untrusted_certificate(self): + with ( + MockFTPServer(tls=True) as ftp_server, + pytest.raises(SSLCertVerificationError), + ): + await self._store(ftp_server.url("file"), b"foo") + def test_uri_auth_quote(self): # RFC3986: 3.2.1. User Information pw_quoted = quote(string.punctuation, safe="") diff --git a/tests/test_utils_serialize.py b/tests/test_utils_serialize.py index 2702c2cce..6becad13e 100644 --- a/tests/test_utils_serialize.py +++ b/tests/test_utils_serialize.py @@ -20,11 +20,17 @@ class TestJsonEncoder: def test_encode_decode(self, encoder: ScrapyJSONEncoder) -> None: dt = datetime.datetime(2010, 1, 2, 10, 11, 12) - dts = "2010-01-02 10:11:12" + dts = "2010-01-02T10:11:12" + dt_aware = datetime.datetime( + 2010, 1, 2, 10, 11, 12, 133700, tzinfo=datetime.timezone.utc + ) + dt_awares = "2010-01-02T10:11:12.133700+00:00" d = datetime.date(2010, 1, 2) ds = "2010-01-02" t = datetime.time(10, 11, 12) ts = "10:11:12" + t_us = datetime.time(10, 11, 12, 133700) + t_uss = "10:11:12.133700" dec = Decimal("1000.12") decs = "1000.12" s = {"foo"} @@ -36,7 +42,9 @@ class TestJsonEncoder: ("foo", "foo"), (d, ds), (t, ts), + (t_us, t_uss), (dt, dts), + (dt_aware, dt_awares), (dec, decs), (["foo", d], ["foo", ds]), (s, ss), diff --git a/tests/utils/bases/download_handlers_http.py b/tests/utils/bases/download_handlers_http.py index 9f72a1594..8cd0458cf 100644 --- a/tests/utils/bases/download_handlers_http.py +++ b/tests/utils/bases/download_handlers_http.py @@ -11,7 +11,7 @@ from contextlib import asynccontextmanager from http import HTTPStatus from ipaddress import IPv4Address from socket import gethostbyname -from typing import TYPE_CHECKING, Any, ClassVar +from typing import TYPE_CHECKING, Any, ClassVar, Literal from urllib.parse import urlparse import pytest @@ -61,6 +61,9 @@ if TYPE_CHECKING: from tests.mockserver.http import MockServer +BadHeaderHandling = Literal["skip-bad", "skip-rest", "fail"] + + class TestHttpBase(ABC): is_secure: bool = False http2: bool = False @@ -73,6 +76,14 @@ 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 + # 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 + # are still parsed, which is what web browsers do; + # "skip-rest": the bad line is skipped along with the header lines that + # follow it; + # "fail": the response cannot be downloaded at all. + handler_bad_header_handling: BadHeaderHandling = "skip-bad" # default headers added by the underlying library that cannot be suppressed always_present_req_headers: ClassVar[frozenset[str]] = frozenset() default_handler_settings: ClassVar[dict[str, Any]] = {} @@ -709,6 +720,32 @@ class TestHttpBase(ABC): in caplog.text ) + @coroutine_test + async def test_download_bad_header(self, mockserver: MockServer) -> None: + if self.http2: + pytest.skip("Header lines are specific to HTTP/1.x") + request = Request(mockserver.url("/bad-header", is_secure=self.is_secure)) + async with self.get_dh() as download_handler: + if self.handler_bad_header_handling == "fail": + with pytest.raises(DownloadFailedError): + await download_handler.download_request(request) + return + response = await download_handler.download_request(request) + assert response.status == 200 + assert response.body == b"Works" + # the header line that precedes the bad one + assert response.headers.get(b"Content-Type") == b"text/html" + # the header split into two lines, also before the bad one + folded_header = response.headers.get(b"X-Folded-Header") + assert folded_header is not None + # the separator between both parts depends on the handler + assert folded_header.split() == [b"one", b"two"] + # the header line that follows the bad one + expected_value = ( + b"works" if self.handler_bad_header_handling == "skip-bad" else None + ) + assert response.headers.get(b"X-After-Bad-Header") == expected_value + @coroutine_test async def test_download_chunked_content(self, mockserver: MockServer) -> None: request = Request(mockserver.url("/chunked", is_secure=self.is_secure))