Download binaries served as text/html instead of rendering them

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"<!--", b"<?")):
         close = b"-->" if start.startswith(b"<!--") else b"?>"
         end = start.find(close)
         if end == -1:
-            return False
+            return b""
         start = start[end + len(close) :].lstrip(_WHITESPACE)
-    return start.startswith((b"<!doctype html", b"<html", b"<head", b"<body"))
+    return start
+
+
+def _is_html(head):
+    return _first_markup(head).startswith(
+        (b"<!doctype html", b"<html", b"<head", b"<body")
+    )
+
+
+def _is_svg(head):
+    start = _first_markup(head)
+    if start.startswith(b"<!doctype svg"):
+        return True
+    return start.startswith(b"<svg") and start[4:5] in (
+        b">",
+        b"/",
+        *(bytes([c]) for c in _WHITESPACE),
+    )
 
 
 class WebDownloadContextMetadata(GenericFileContextMetadata):
@@ -301,6 +387,8 @@ def get_cache_key(self, path, asset_ref=False, language=None, **kwargs) -> str:
                 key += ":lang"
             if asset_ref:
                 key += ":asset"
+            if name_ext in _SERVER_SCRIPT_EXTENSIONS:
+                key += ":script"
         elif asset_ref and name_ext in STATIC_ASSET_EXTENSIONS:
             key += ":asset:v2"
         return key
@@ -326,11 +414,7 @@ def handle_file(
             r.close()
             raise
         chunks = r.iter_content(chunk_size=8192)
-        head = b""
-        for chunk in chunks:
-            head += chunk
-            if len(head) >= _SNIFF_BYTES:
-                break
+        head = _read_sniff_window(chunks)
         if ext:
             ext = resolve_path_ext(original_filename, declared_ext=ext)
         else:
@@ -365,6 +449,8 @@ def _resolve_ext(
                 and name_ext not in exts_from_mimetype(sniffed.mime)
             ):
                 return sniffed.extension
+        if mimetype in self.HTML_CONTENT_TYPES and _is_svg(head):
+            return "svg"
         page = (
             name_ext in NAME_KEPT_EXTENSIONS
             and mimetype in self.HTML_CONTENT_TYPES
@@ -382,10 +468,7 @@ def _resolve_ext(
         if name_ext is None and default_ext and not html:
             return default_ext
         ext = ext_from_content_type(content_type) or getattr(sniffed, "extension", None)
-        # Many formats (.ggb, .sb3, .jar) are containers under their own extension.
-        if name_ext and ext in _CONTAINER_EXTENSIONS:
-            ext = default_ext if default_ext in _ARCHIVE_EXTENSIONS else name_ext
-        ext = ext or name_ext
+        ext = _container_ext(ext, name_ext, default_ext) or name_ext
         if not ext:
             raise InvalidFileException(f"Could not determine the file type of {path}")
         if not language and ext in SubtitleConversionHandler.EXTENSIONS:
@@ -936,7 +1019,9 @@ def should_handle(self, url: str) -> bool:
             return False
         if parsed.scheme not in ("http", "https") or not parsed.netloc:
             return False
-        return self._content_type(url) in self.HTML_CONTENT_TYPES
+        if self._content_type(url) not in self.HTML_CONTENT_TYPES:
+            return False
+        return not self._is_non_page(url)
 
     def get_cache_key(self, path, rejected_default_ext=None, **kwargs) -> str:
         # Include the crawl settings so two depths/scopes of the same URL do
@@ -982,10 +1067,10 @@ class DownloadStageHandler(StageHandler):
 
     def __init__(self, children=None):
         super().__init__(children=children)
-        content_type_cache = {}
+        probe_cache = {}
         for child in self._children:
             if isinstance(child, WebResourceHandler):
-                child._content_type_cache = content_type_cache
+                child._probe_cache = probe_cache
 
     @staticmethod
     def _claims(handler, path):

tests/cassettes/test_cached_render_survives_failing_source.yaml

@@ -13,6 +13,20 @@ interactions:
     status:
       code: 200
       message: OK
+- request:
+    body: null
+    headers: {}
+    method: GET
+    uri: https://site.example/lesson
+  response:
+    body:
+      string: <html><body>Lesson</body></html>
+    headers:
+      Content-Type:
+      - text/html
+    status:
+      code: 200
+      message: OK
 - request:
     body: null
     headers: {}

tests/pipeline/helpers.py

@@ -10,6 +10,9 @@
     b"\r\n-\xb4\x00\x00\x00\x00IEND\xaeB`\x82"
 )
 
+_SVG = b'<svg xmlns="http://www.w3.org/2000/svg" width="1" height="1"></svg>'
+_XML_SVG = b'<?xml version="1.0"?>\n' + _SVG
+
 
 def fake_render_page(**expected_kwargs):
     """Return a render_page stand-in that emits an index.html with a data: img.

tests/pipeline/test_convert.py

@@ -91,6 +91,8 @@
 from ricecooker.utils.zip import directory_member_names
 
 from .helpers import _PNG_1x1
+from .helpers import _SVG
+from .helpers import _XML_SVG
 from .helpers import fake_render_page
 
 
@@ -1726,6 +1728,47 @@ def render_page(url, output_dir, **kwargs):
             with open(os.path.join(out_dir, "index.html")) as f:
                 assert f.read() == page
 
+    @pytest.mark.parametrize(
+        "url,body,ext",
+        [
+            ("https://site.example/img.php?id=3", _PNG_1x1, ".png"),
+            ("https://site.example/chart.php?id=3", _XML_SVG, ".svg"),
+            ("https://site.example/chart.php?id=3", b"<!-- chart -->\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'<html><body><img src="{url}"></body></html>'
+        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'<html><body><img src="{url}"></body></html>'
+        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"<!doctype html><html></html>"},
+                content_types={url: "text/html"},
+            ),
+        ):
+            assert render.call_count == 1
+
     @pytest.mark.parametrize(
         "url, content_type",
         [