From 46d1dba97609f8a4930c55a0be56d21dc6537c3f Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Sun, 18 Oct 2026 17:34:33 -0700 Subject: [PATCH] Download binaries served as text/html instead of rendering them SingleFileRenderHandler sniffs the response body; a filetype or SVG signature downloads the file under its sniffed extension. Fixes #867 Co-Authored-By: Claude Opus 5.5 --- ricecooker/utils/pipeline/transfer.py | 143 +++++++++++++---- ...cached_render_survives_failing_source.yaml | 14 ++ tests/pipeline/helpers.py | 3 + tests/pipeline/test_convert.py | 43 ++++++ tests/pipeline/test_transfer.py | 144 +++++++++++++----- tests/test_files.py | 82 +++++++++- tests/test_tree.py | 60 ++++++++ 7 files changed, 418 insertions(+), 71 deletions(-) diff --git a/ricecooker/utils/pipeline/transfer.py b/ricecooker/utils/pipeline/transfer.py index a76ca065..7c8b7d95 100644 --- a/ricecooker/utils/pipeline/transfer.py +++ b/ricecooker/utils/pipeline/transfer.py @@ -7,7 +7,9 @@ import re import subprocess import tempfile +from contextlib import closing from contextlib import contextmanager +from dataclasses import dataclass from dataclasses import field from sys import platform from typing import Dict @@ -116,6 +118,12 @@ def handle_file(self, path, default_ext=None, ext=None, rejected_default_ext=Non return FileMetadata(original_filename=os.path.basename(path)) +@dataclass +class _Probe: + content_type: str + non_page: Optional[bool] + + class WebResourceHandler(FileHandler): """Base class for handling web URLs""" @@ -125,30 +133,36 @@ class WebResourceHandler(FileHandler): def __init__(self, **context): super().__init__(**context) - self._content_type_cache = {} + self._probe_cache = {} - def _content_type(self, url: str) -> str: - if url not in self._content_type_cache: + def _probe(self, url: str) -> _Probe: + if url not in self._probe_cache: try: response = config.DOWNLOAD_SESSION.head( url, allow_redirects=True, timeout=(30, 30) ) except RequestException: response = None - if response is None or not response.ok: + if response is not None and response.ok: + self._probe_cache[url] = _Probe(_media_type(response), None) + else: try: - response = config.DOWNLOAD_SESSION.get( - url, stream=True, timeout=(30, 60) - ) - response.close() - response.raise_for_status() + self._probe_cache[url] = _get_sniffed(url) except RequestException as e: raise ProbeError(e) from e - content_type = response.headers.get("content-type", "") - self._content_type_cache[url] = ( - content_type.split(";", 1)[0].strip().lower() - ) - return self._content_type_cache[url] + return self._probe_cache[url] + + def _content_type(self, url: str) -> str: + return self._probe(url).content_type + + def _is_non_page(self, url: str) -> bool: + probe = self._probe(url) + if probe.non_page is None: + try: + probe.non_page = _get_sniffed(url).non_page + except RequestException: + probe.non_page = False + return probe.non_page def should_handle(self, url): """Check if this handler should handle the given URL""" @@ -242,6 +256,23 @@ def path_ext_or_none(path): _CONTAINER_EXTENSIONS = frozenset({"7z", "bz2", "gz", "rar", "tar", "xz", "zip"}) +_SERVER_SCRIPT_EXTENSIONS = frozenset( + { + "action", + "ashx", + "asp", + "aspx", + "cfm", + "cgi", + "do", + "jsp", + "php", + "phtml", + "pl", + "py", + } +) + _ARCHIVE_EXTENSIONS = frozenset( ext for handler in ConversionStageHandler.DEFAULT_CHILDREN @@ -269,15 +300,70 @@ def path_ext_or_none(path): _SNIFF_BYTES = 8192 -def _is_html(head): +def _read_sniff_window(chunks): + head = b"" + for chunk in chunks: + head += chunk + if len(head) >= _SNIFF_BYTES: + break + return head + + +def _get_sniffed(url): + response = config.DOWNLOAD_SESSION.get(url, stream=True, timeout=(30, 60)) + with closing(response): + response.raise_for_status() + try: + head = _read_sniff_window(response.iter_content(chunk_size=8192)) + except RequestException: + head = b"" + return _Probe( + _media_type(response), filetype.guess(head) is not None or _is_svg(head) + ) + + +def _media_type(response): + content_type = response.headers.get("content-type", "") + return content_type.split(";", 1)[0].strip().lower() + + +def _container_ext(ext, name_ext, default_ext): + # Many formats (.ggb, .sb3, .jar) are containers under their own extension. + if not name_ext or ext not in _CONTAINER_EXTENSIONS: + return ext + if default_ext in _ARCHIVE_EXTENSIONS: + return default_ext + if name_ext in _SERVER_SCRIPT_EXTENSIONS: + return ext + return name_ext + + +def _first_markup(head): start = head.lstrip(b"\xef\xbb\xbf" + _WHITESPACE).lower() while start.startswith((b"" if start.startswith(b"\n" + _SVG, ".svg"), + ], + ids=["png", "xml-svg", "comment-svg"], + ) + def test_file_ref_answered_as_html_is_stored_by_type(self, url, body, ext): + page = f'' + with ( + patch.object(config, "UPDATE", True), + patch.object( + transfer, "render_page", side_effect=AssertionError("rendered") + ), + _run_external_refs( + {"index.html": page}, {url: body}, content_types={url: "text/html"} + ) as (out_dir, _fetched), + ): + [asset] = [n for n in os.listdir(out_dir) if n != "index.html"] + assert asset.endswith(ext) + with open(os.path.join(out_dir, "index.html")) as f: + assert f'src="{asset}"' in f.read() + + def test_html_ref_answered_as_html_is_rendered(self): + url = "https://site.example/img.php?id=3" + page = f'' + with ( + patch.object(config, "UPDATE", True), + patch.object( + transfer, "render_page", side_effect=fake_render_page() + ) as render, + _run_external_refs( + {"index.html": page}, + {url: b""}, + content_types={url: "text/html"}, + ), + ): + assert render.call_count == 1 + @pytest.mark.parametrize( "url, content_type", [ diff --git a/tests/pipeline/test_transfer.py b/tests/pipeline/test_transfer.py index 87ecdf8d..8763477f 100644 --- a/tests/pipeline/test_transfer.py +++ b/tests/pipeline/test_transfer.py @@ -62,6 +62,8 @@ from ricecooker.utils.youtube import YouTubeResource from .helpers import _PNG_1x1 +from .helpers import _SVG +from .helpers import _XML_SVG from .helpers import fake_render_page _OGG_THEORA = b"OggS\0\x02" + b"\0" * 22 + b"\x80theora" + b"\0" * 30 @@ -478,11 +480,7 @@ def fake_extractors(results): @contextmanager def html_page(url, render_page=None): with ( - patch.object( - config.DOWNLOAD_SESSION, - "head", - side_effect=_fake_head({url: "text/html"}), - ) as head, + _fake_probe({url: "text/html"}) as head, patch( "ricecooker.utils.pipeline.transfer.render_page", side_effect=render_page or AssertionError("rendered the page"), @@ -1469,6 +1467,27 @@ def raise_for_status(self): def iter_content(self, chunk_size=8192): yield self.content + def close(self): + pass + + +@contextmanager +def _fake_probe(content_types_by_url, get=None): + with ( + patch.object( + config.DOWNLOAD_SESSION, + "head", + side_effect=_fake_head(content_types_by_url), + ) as head, + patch.object( + config.DOWNLOAD_SESSION, + "get", + get + or MagicMock(return_value=_FakeGetResponse(b"", "text/html")), + ), + ): + yield head + @pytest.mark.parametrize( "url,content_type,content,default_ext,suffix", @@ -1600,12 +1619,19 @@ def iter_content(self, chunk_size=8192): "zip", ".zip", ), + ( + "https://s.example/scene.ggb?id=5", + "application/zip", + b"PK\x03\x04", + "vtt", + ".ggb", + ), ( "https://s.example/getsub.php?id=5", "application/zip", b"PK\x03\x04", "vtt", - ".php", + ".zip", ), ("https://s.example/stream.php?id=1", "audio/ogg", b"OggS", None, ".ogg"), ("https://s.example/feed.xml", "text/html", b"1", None, ".xml"), @@ -1638,6 +1664,14 @@ def iter_content(self, chunk_size=8192): "png", ".png", ), + ("https://site.example/chart.php?id=3", "text/html", _XML_SVG, None, ".svg"), + ( + "https://site.example/chart?id=3", + "text/html", + b"" + _SVG, + None, + ".svg", + ), ("https://site.example/clip.mp4", "text/html", _PNG_1x1, None, ".png"), ("https://site.example/clip.mp3", "text/html", _PNG_1x1, None, ".png"), ("https://site.example/clip", "text/html", _PNG_1x1, "mp4", ".png"), @@ -1652,6 +1686,14 @@ def iter_content(self, chunk_size=8192): ("https://site.example/pkg.h5p", "text/html", b"PK\x03\x04", None, ".h5p"), ("https://s.example/get.php?id=2", "text/html", b"PK\x03\x04", "h5p", ".h5p"), ("https://s.example/get?id=1", "text/html", b"PK\x03\x04", None, ".zip"), + ("https://s.example/app.php?id=5", "text/html", b"PK\x03\x04", None, ".zip"), + ( + "https://s.example/download.ashx?id=5", + "text/html", + b"PK\x03\x04", + None, + ".zip", + ), ("https://x.example/clip.ogv", "text/html", _OGG_THEORA, None, ".ogv"), ("https://x.example/song.m4a", "text/html", _MP4_ISOM, None, ".m4a"), ("https://x.example/app.js", "text/html", b"BMap.init();\n", None, ".js"), @@ -1678,6 +1720,7 @@ def iter_content(self, chunk_size=8192): "svg-served-as-html", "container-type-takes-default-ext", "container-type-ignores-non-archive-default-ext", + "container-type-at-script-ext-takes-container-ext", "ogg-not-oga", "xml-served-as-html", "comment-first-svg-served-as-html", @@ -1685,6 +1728,8 @@ def iter_content(self, chunk_size=8192): "comment-only-css-served-as-html", "xhtml-page-at-asset-ext", "png-served-as-html-at-php", + "xml-svg-served-as-html-at-php", + "comment-svg-served-as-html-without-ext", "png-served-as-html-at-mp4", "png-served-as-html-at-mp3", "png-served-as-html-without-ext", @@ -1693,6 +1738,8 @@ def iter_content(self, chunk_size=8192): "zip-served-as-html-keeps-name-ext", "zip-served-as-html-at-php-takes-default-ext", "zip-served-as-html-without-ext", + "zip-served-as-html-at-php", + "zip-served-as-html-at-ashx", "ogg-served-as-html-keeps-ogv", "mp4-served-as-html-keeps-m4a", "bmp-like-js-served-as-html-keeps-js", @@ -1750,17 +1797,20 @@ def test_every_pil_image_mimetype_maps_to_a_converted_ext(): @pytest.mark.parametrize( - "url,cached,refetched", + "url,cached_key,cached,refetched", [ - ("https://site.example/stale-cache.php", "stale.php", True), - ("https://site.example/kept-cache.png", "kept.png", False), + ("https://site.example/stale-cache.php", "", "stale.php", True), + ("https://site.example/kept-cache.png", "", "kept.png", False), + ("https://site.example/stale-v2-app.php?id=5", ":v2", "stale-v2.php", True), ], - ids=["renamed-ext-refetched", "kept-ext-reused"], + ids=["renamed-ext-refetched", "kept-ext-reused", "v2-script-ext-refetched"], ) -def test_catchall_download_cached_by_earlier_release(url, cached, refetched): +def test_catchall_download_cached_by_earlier_release( + url, cached_key, cached, refetched +): with open(config.get_storage_path(cached), "wb") as fh: fh.write(_PNG_1x1) - set_cache_data(f"DOWNLOAD:{url}", {"filename": cached}) + set_cache_data(f"DOWNLOAD:{url}{cached_key}", {"filename": cached}) with patch.object( config.DOWNLOAD_SESSION, "get", @@ -1865,9 +1915,7 @@ def test_singlefile_render_handler_should_handle_detects_html(): "https://spa.example/": "text/html; charset=utf-8", "https://spa.example/report.pdf": "application/pdf", } - with patch.object( - config.DOWNLOAD_SESSION, "head", side_effect=_fake_head(content_types) - ): + with _fake_probe(content_types): assert handler.should_handle("https://spa.example/") is True # A non-HTML resource falls through to the catch-all download handler. assert handler.should_handle("https://spa.example/report.pdf") is False @@ -1876,15 +1924,51 @@ def test_singlefile_render_handler_should_handle_detects_html(): assert handler.should_handle("data:image/png;base64,AA") is False -def test_singlefile_render_handler_head_is_cached(): - # should_handle is called repeatedly (composite probe + dispatch); the HEAD - # request must fire at most once per URL. +@pytest.mark.parametrize( + "body,claims", + [ + (b"", True), + (_PNG_1x1, False), + (_XML_SVG, False), + (b"" + _SVG, False), + (b"" + _SVG, False), + ( + b'', + True, + ), + ], + ids=["html", "png", "xml-svg", "comment-svg", "doctype-svg", "xhtml"], +) +def test_singlefile_render_handler_probe_is_cached(body, claims): + handler = SingleFileRenderHandler() + get = MagicMock(return_value=_FakeGetResponse(body, "text/html")) + with _fake_probe({"https://spa.example/": "text/html"}, get) as head: + assert handler.should_handle("https://spa.example/") is claims + assert handler.should_handle("https://spa.example/") is claims + assert head.call_count == 1 + assert get.call_count == 1 + + +def test_singlefile_render_handler_claims_when_sniff_fails(): handler = SingleFileRenderHandler() - head = MagicMock(return_value=_FakeHeadResponse("text/html")) - with patch.object(config.DOWNLOAD_SESSION, "head", head): + get = MagicMock(side_effect=requests.ConnectionError("refused")) + with _fake_probe({"https://spa.example/": "text/html"}, get): assert handler.should_handle("https://spa.example/") is True assert handler.should_handle("https://spa.example/") is True - assert head.call_count == 1 + assert get.call_count == 1 + + +def test_rejected_head_binary_probe_body_is_not_rendered(monkeypatch): + methods = _fake_http( + monkeypatch, + lambda pool, method, url, headers: ( + _response(405) + if method == "HEAD" + else _response(200, "text/html", _PNG_1x1) + ), + ) + assert SingleFileRenderHandler().should_handle("https://site.example/img") is False + assert methods == ["HEAD", "GET"] def test_singlefile_render_handler_produces_zip(): @@ -1976,11 +2060,7 @@ def test_singlefile_render_end_to_end_explosion(): "ricecooker.utils.pipeline.transfer.render_page", side_effect=fake_render_page(), ), - patch.object( - config.DOWNLOAD_SESSION, - "head", - side_effect=_fake_head({"https://spa.example/": "text/html"}), - ), + _fake_probe({"https://spa.example/": "text/html"}), ): result = pipeline.execute("https://spa.example/") @@ -2010,9 +2090,7 @@ def test_page_at_asset_extension_url_renders(url): "ricecooker.utils.pipeline.transfer.render_page", side_effect=fake_render_page(), ), - patch.object( - config.DOWNLOAD_SESSION, "head", side_effect=_fake_head({url: "text/html"}) - ), + _fake_probe({url: "text/html"}), ): result = FilePipeline().execute(url) assert result[0].preset == format_presets.KPUB_ZIP @@ -2026,9 +2104,7 @@ def test_default_pipeline_renders_html_and_downloads_other_sources(): "https://spa.example/": "text/html", "https://example.com/x.pdf": "application/pdf", } - with patch.object( - config.DOWNLOAD_SESSION, "head", side_effect=_fake_head(content_types) - ): + with _fake_probe(content_types): # An HTML page is claimed by the render handler. assert pipeline.should_handle("https://spa.example/") is True # A non-HTML resource still routes to a default download handler. @@ -2173,7 +2249,7 @@ def test_cached_render_survives_failing_source(monkeypatch): with pytest.raises(ExpectedFileException, match="503"): DownloadStageHandler().execute(url, skip_cache=True) assert second[0].path == first[0].path - assert cassette.play_count == 4 + assert cassette.play_count == 5 @pytest.mark.parametrize( @@ -2216,7 +2292,7 @@ def test_cached_download_of_html_page_is_rendered(monkeypatch): ): result = DownloadStageHandler().execute(url) assert result[0].filename.endswith(".zip") - assert methods == ["GET", "HEAD"] + assert methods == ["GET", "HEAD", "GET"] def _failing_source(status): diff --git a/tests/test_files.py b/tests/test_files.py index a306568e..3e49ee17 100644 --- a/tests/test_files.py +++ b/tests/test_files.py @@ -33,6 +33,8 @@ from le_utils.constants.exercises import GRAPHIE_DELIMITER from pdf2image.exceptions import PDFInfoNotInstalledError from PIL import Image +from pipeline.helpers import _SVG +from pipeline.helpers import _XML_SVG from pipeline.helpers import fake_render_page from PyPDF2 import PdfFileWriter from requests import ConnectionError @@ -880,6 +882,14 @@ def test_rejected_default_ext_keyword_fails_cached_download(tmp_path): assert "Incompatible default_ext" in file.error +def _zip_bytes(files): + buffer = BytesIO() + with zipfile.ZipFile(buffer, "w") as zf: + for name, content in files.items(): + zf.writestr(name, content) + return buffer.getvalue() + + @contextmanager def _fake_html_session( url, @@ -1026,6 +1036,70 @@ def test_png_served_as_html_fails_media_file_as_incompatible(file_class, url, bo assert render.call_count == 0 +@pytest.fixture +def fresh_pipeline(monkeypatch): + monkeypatch.setattr(config, "FILE_PIPELINE", FilePipeline()) + + +@pytest.mark.parametrize( + "file_class,url,body,ext", + [ + (DownloadFile, "https://site.example/img.php?id=3", _PNG_1x1, ".png"), + ( + DownloadFile, + "https://site.example/img.php?id=3", + [_PNG_1x1[:1], _PNG_1x1[1:]], + ".png", + ), + ( + HTMLZipFile, + "https://site.example/app.php?id=5", + _zip_bytes({"index.html": "App"}), + ".zip", + ), + (DownloadFile, "https://site.example/chart.php?id=3", _XML_SVG, ".svg"), + ( + DownloadFile, + "https://site.example/chart.php?id=3", + b"\n" + _SVG, + ".svg", + ), + ], + ids=["png", "split-png", "zip", "xml-svg", "comment-svg"], +) +@pytest.mark.usefixtures("fresh_pipeline") +def test_binary_served_as_html_downloads(file_class, url, body, ext): + with ( + patch("ricecooker.config.UPDATE", True), + _fake_html_session(url, body=body) as render, + ): + file = file_class(url) + filename = file.process_file() + assert file.error is None + assert filename.endswith(ext) + assert render.call_count == 0 + + +@pytest.mark.parametrize( + "file_class,url", + [ + (DownloadFile, "https://site.example/img.php?id=3"), + (HTMLZipFile, "https://site.example/app.php?id=5"), + ], +) +@pytest.mark.usefixtures("fresh_pipeline") +def test_html_page_at_binary_url_renders(file_class, url): + with ( + patch("ricecooker.config.UPDATE", True), + _fake_html_session(url) as render, + ): + file = file_class(url) + filename = file.process_file() + assert file.error is None + assert filename.endswith(".zip") + assert render.call_count == 1 + + @pytest.mark.parametrize( "file_class,sample,default_ext,url,disposition,content_type", [ @@ -1314,14 +1388,6 @@ def test_truncated_utf16_member_left_unchanged(tmp_path): assert zf.read("other.html") == other -def _zip_bytes(files): - buffer = BytesIO() - with zipfile.ZipFile(buffer, "w") as zf: - for name, content in files.items(): - zf.writestr(name, content) - return buffer.getvalue() - - def _epub_container(encoding="utf-8"): return ( f'' diff --git a/tests/test_tree.py b/tests/test_tree.py index c60558e0..29c9eddc 100644 --- a/tests/test_tree.py +++ b/tests/test_tree.py @@ -1,5 +1,6 @@ """Tests for tree construction""" +import io import json import logging import os @@ -8,6 +9,7 @@ import time import uuid import zipfile +from contextlib import nullcontext from unittest.mock import MagicMock from unittest.mock import mock_open from unittest.mock import patch @@ -31,6 +33,8 @@ from le_utils.constants.labels import subjects from le_utils.constants.languages import getlang from PIL import Image +from pipeline.helpers import _PNG_1x1 +from pipeline.helpers import fake_render_page from pipeline.test_transfer import _http_error from pipeline.test_transfer import _video_info from pipeline.test_transfer import fake_extractors @@ -803,6 +807,62 @@ def test_automatic_resource_node_document(document_file): assert node.learning_activities == [learning_activities.READ] +def _html5_zip_bytes(): + buf = io.BytesIO() + with zipfile.ZipFile(buf, "w") as zf: + zf.writestr("index.html", "

Hi

") + return buf.getvalue() + + +@pytest.mark.parametrize( + "url,body,render_count,exts,kind", + [ + ("https://site.example/img.php?id=3", _PNG_1x1, 0, [], None), + ( + "https://site.example/app.php?id=5", + _html5_zip_bytes(), + 0, + [".kpub"], + content_kinds.DOCUMENT, + ), + ( + "https://site.example/img.php?id=3", + b"", + 1, + [".kpub"], + content_kinds.DOCUMENT, + ), + ], + ids=["png", "zip", "html"], +) +def test_automatic_resource_node_html_labelled_uri(url, body, render_count, exts, kind): + node = ContentNode( + "test", + "test", + licenses.CC_BY, + uri=url, + pipeline=FilePipeline(), + copyright_holder="Demo Holdings", + ) + with ( + patch.object(config, "UPDATE", True), + fake_download_session({url: body}, {url: "text/html"}), + patch( + "ricecooker.utils.pipeline.transfer.render_page", + side_effect=fake_render_page(), + ) as render, + ( + pytest.raises(InvalidNodeException, match="No format preset inferred") + if kind is None + else nullcontext() + ), + ): + node.process_files() + assert render.call_count == render_count + assert [os.path.splitext(f.filename)[1] for f in node.files] == exts + assert node.kind == kind + + @pytest.mark.parametrize("node_class", [ContentNode, DocumentNode]) def test_uri_node_failure_carries_pipeline_error(node_class): node = node_class(