From 72b5f2f0ddfd268382ed12b7c617c6562be8f40e Mon Sep 17 00:00:00 2001
From: sirdave73 <sirdave73@epub2pdf.local>
Date: Fri, 4 Sep 2026 02:25:07 -0400
Subject: [PATCH] C1+C3 review findings: streaming decompression-bomb guard +
 stored-XSS stripping

C1 (decompression bomb): wire the shared common._copy_stream_with_cap into both
zip-extraction paths. app.py._safe_extract_zip now streams each EPUB member with
a WRITTEN-byte counter (app.py:456) instead of an unbounded copyfileobj, so a
member that inflates a small header into a huge stream is stopped mid-write.
pandoc_converter._validate_office_zip now streams every member's decompressed
bytes through the same guard (pandoc_converter.py:134) before pandoc reads them.

C3 (stored XSS): common.html_to_pdf_bytes now strips stored-XSS vectors via
common._strip_scripts (common.py:320) BEFORE WeasyPrint renders, so every
non-pandoc render path (html, svg, epub, md, csv, xml) neutralizes <script>/
<iframe> content, on* handlers and javascript:/vbscript: URLs. Regression tests
added to tests/test_review_findings_20260903.py (C1 unit+e2e, C3 unit+e2e).
---
 app.py                                 |  11 +-
 converters/common.py                   |  45 ++++---
 converters/pandoc_converter.py         |  11 ++
 tests/test_review_findings_20260903.py | 180 ++++++++++++++++++++++++-
 4 files changed, 227 insertions(+), 20 deletions(-)

diff --git a/app.py b/app.py
index 1b4a745..6ca7c05 100644
--- a/app.py
+++ b/app.py
@@ -407,6 +407,10 @@ def _safe_extract_zip(epub_path: Path, extract_dir: Path) -> None:
     Also enforces the zip-bomb cap: total uncompressed size must stay within
     max(8x upload size, ZIP_BOMB_MIN_CAP) or the job fails with a clear error.
     """
+    # Lazy import: app imports the converters package at module load (app.py:37),
+    # but the common submodule is pulled lazily here to avoid any import-cycle
+    # concern. Provides the C1 streaming decompression-bomb guard.
+    from converters import common
     upload_size = epub_path.stat().st_size
     cap = max(8 * upload_size, ZIP_BOMB_MIN_CAP)
     try:
@@ -444,7 +448,12 @@ def _safe_extract_zip(epub_path: Path, extract_dir: Path) -> None:
                 continue
             target.parent.mkdir(parents=True, exist_ok=True)
             with zf.open(info) as src, open(target, "wb") as dst:
-                shutil.copyfileobj(src, dst)
+                # C1 (real enforcement): stream-copy counting WRITTEN bytes so a
+                # member that inflates a small header into a huge stream is stopped
+                # mid-write. A header-only check (lines 418-427) cannot see a member
+                # whose declared size is understated; this counts actual bytes and
+                # mirrors the shared guard wired into pandoc._validate_office_zip.
+                common._copy_stream_with_cap(src, dst, cap)
 
 
 # --------------------------------------------------------------------------
diff --git a/converters/common.py b/converters/common.py
index 50226a8..f1a1cff 100644
--- a/converters/common.py
+++ b/converters/common.py
@@ -177,24 +177,23 @@ def _strip_scripts(html: str) -> str:
         r"\s+on\w+\s*=\s*(\"[^\"]*\"|'[^']*'|[^\s>]+)", "", html,
         flags=re.IGNORECASE,
     )
+
     # javascript:/vbscript: URLs in href/src -> blank the attribute.
+    # group(4) captures the value INCLUDING its surrounding quotes, so the
+    # scheme check strips the quotes first; otherwise the leading quote makes
+    # "javascript:" never match and the handler silently survives (the C3 bug).
     def _blank(match):
-        attr, quote, val = (
-            match.group(1), match.group(2), match.group(3)
+        quote, scheme = (
+            match.group(4)[0],
+            match.group(4).strip().strip('"').strip("'").strip(),
         )
-        stripped = val.strip()
-        if re.match(r"(?:javascript|vbscript):", stripped, re.IGNORECASE):
-            return f"{attr}={quote}{quote}"
+        if re.match(r"(?:javascript|vbscript):", scheme, re.IGNORECASE):
+            return f"{match.group(1)}{match.group(2)}{match.group(3)}{quote}{quote}"
         return match.group(0)
 
     html = re.sub(
         r"(\s)(href|src)(\s*=\s*)(\"[^\"]*\"|'[^']*'|[^\s>]+)",
-        lambda m: (
-            f'{m.group(1)}{m.group(2)}{m.group(3)}{m.group(4).strip()}'
-            if re.match(r"(?:javascript|vbscript):",
-                        m.group(4).strip(), re.IGNORECASE)
-            else m.group(0)
-        ),
+        _blank,
         html, flags=re.IGNORECASE,
     )
     return html
@@ -222,12 +221,17 @@ def zip_header_guard(infos, cap) -> None:
             )
 
 
-def _copy_stream_with_cap(src, dst, cap) -> None:
+def _copy_stream_with_cap(src, dst, cap) -> int:
     """Stream-copy src\u2192dst counting WRITTEN bytes; abort mid-stream if the
     decompressed total exceeds `cap` (C1 real enforcement).
 
-    Unlike a header-only check, this counts actual bytes delivered to disk,
-    so a zip that inflates a 5 MB header to 50 GB is stopped mid-write.
+    Unlike a header-only check, this counts actual bytes delivered to disk, so a
+    zip that inflates a 5 MB header to 50 GB is stopped mid-write. `dst` may be
+    None: when None the bytes are counted but not written (a pure pre-check that
+    must not touch disk), e.g. streaming the decompressed size of an office zip
+    before pandoc reads it. Always returns the total WRITTEN bytes; callers that
+    write to a file ignore the return.
+
     Satisfies: C1
     """
     chunk_size = 64 * 1024
@@ -235,7 +239,7 @@ def _copy_stream_with_cap(src, dst, cap) -> None:
     while True:
         chunk = src.read(chunk_size)
         if not chunk:
-            return
+            return written
         written += len(chunk)
         if written > cap:
             raise ValueError(
@@ -243,7 +247,9 @@ def _copy_stream_with_cap(src, dst, cap) -> None:
                 f"{written} bytes (cap {cap}); aborting extraction to "
                 "prevent disk-fill"
             )
-        dst.write(chunk)
+        if dst is not None:
+            dst.write(chunk)
+    return written
 
 
 # Resource-attribute names whose string value WeasyPrint may fetch from the
@@ -305,6 +311,13 @@ def html_to_pdf_bytes(html: str,
     ever runs, so every converter rendering user HTML/SVG is SSRF-safe by default.
     Satisfies: C2
     """
+    # C3: strip stored-XSS vectors BEFORE WeasyPrint renders the document.
+    # This is the single choke point every HTML/SVG/MD/CSV/XML/text converter
+    # flows through (svg renders here directly; the rest reach it via
+    # common.convert_single -> html_to_pdf_bytes), so one strip here covers the
+    # whole non-pandoc render surface. Harmless no-op for json/text, which escape
+    # their content into &lt;...&gt; so the script regex never matches.
+    html = _strip_scripts(html)
     scan_for_remote(html)
     from weasyprint import CSS, HTML
 
diff --git a/converters/pandoc_converter.py b/converters/pandoc_converter.py
index bb7f893..c77a132 100644
--- a/converters/pandoc_converter.py
+++ b/converters/pandoc_converter.py
@@ -121,6 +121,17 @@ def _validate_office_zip(path: Path) -> None:
         raise ValueError(
             f"zip-bomb guard: uncompressed size {total} exceeds cap {cap}"
         )
+    # C1 (real enforcement): stream every member's DECOMPRESSED bytes through the
+    # shared counter (dst=None -> count-only) so a container that inflates a small
+    # header into a huge stream is stopped mid-read, before pandoc ever opens it.
+    # The header check above is defense-in-depth; this counts actual bytes and
+    # mirrors _safe_extract_zip's streaming guard.
+    with zipfile.ZipFile(path) as zf:
+        for info in zf.infolist():
+            if info.is_dir():
+                continue
+            with zf.open(info) as member:
+                common._copy_stream_with_cap(member, None, cap)
     root = path.parent.resolve()
     for info in infos:
         name = info.filename
diff --git a/tests/test_review_findings_20260903.py b/tests/test_review_findings_20260903.py
index 4be662f..92b8681 100644
--- a/tests/test_review_findings_20260903.py
+++ b/tests/test_review_findings_20260903.py
@@ -17,14 +17,26 @@ Satisfies: C2
 """
 import base64
 import io
+import os
+import tempfile
+import zipfile
+
+import pytest
 
 import app
-from tests.helpers import wait_for_job
+import converters.common as C
+from tests.helpers import wait_for_job, download_bytes, pdf_text
 
 
 def _upload(client, content, filename="doc.html"):
-    """POST an upload, wait for the terminal job state, return the snapshot."""
-    files = {"file": (io.BytesIO(content.encode("utf-8")), filename)}
+    """POST an upload, wait for the terminal job state, return the snapshot.
+
+    Accepts either a str (encoded to utf-8) or bytes so the HTML/SVG string
+    tests and the binary-epub C1 zip-bomb test can both share it.
+    """
+    if isinstance(content, str):
+        content = content.encode("utf-8")
+    files = {"file": (io.BytesIO(content), filename)}
     resp = client.post("/api/convert", data=files, content_type="multipart/form-data")
     assert resp.status_code == 202, resp.get_json()
     return wait_for_job(client, resp.get_json()["job_id"])
@@ -84,4 +96,166 @@ def test_local_data_uri_image_succeeds(client):
         "</body></html>"
     )
     job = _upload(client, html)
+    assert job["status"] == "done", job
+
+
+# ---------------------------------------------------------------------------
+# Finding C1 (regression): decompression-bomb guard on zip extraction
+# ---------------------------------------------------------------------------
+# Two independent mechanisms are exercised:
+#   * the SHARED streaming guard common._copy_stream_with_cap (proven by the
+#     unit tests: it counts WRITTEN decompressed bytes and aborts mid-stream
+#     past the cap), and
+#   * app.py's zip-bomb guard end-to-end (proven by the e2e: a genuinely large
+#     member trips the per-member size cap and the job fails with 'zip-bomb').
+
+
+def _append_member(epub_bytes, name, data=b"x", external_attr=None):
+    """Append a member to an existing epub byte string (mirrors test_review_fixes.py)."""
+    src = io.BytesIO(epub_bytes)
+    out = io.BytesIO()
+    with zipfile.ZipFile(src, "r") as zin, zipfile.ZipFile(out, "w", zipfile.ZIP_DEFLATED) as zout:
+        for item in zin.namelist():
+            zout.writestr(item, zin.read(item))
+        zi = zipfile.ZipInfo(name)
+        if external_attr is not None:
+            zi.external_attr = external_attr
+        zout.writestr(zi, data)
+    return out.getvalue()
+
+
+def test_copy_stream_with_cap_counts_written_bytes():
+    """_copy_stream_with_cap returns the exact byte count it streamed to dst.
+    Satisfies: C1
+    """
+    data = b"x" * (10 * 1024)  # 10 KiB
+    buf = io.BytesIO()
+    written = C._copy_stream_with_cap(io.BytesIO(data), buf, cap=10 * 1024 * 1024)
+    assert written == len(data)
+    assert buf.getvalue() == data
+
+
+def test_copy_stream_with_cap_count_only_when_dst_none():
+    """When dst is None the bytes are counted but NOT written (used by the pandoc
+    pre-check, which must not touch disk). Satisfies: C1
+    """
+    data = b"x" * (10 * 1024)
+    written = C._copy_stream_with_cap(io.BytesIO(data), None, cap=10 * 1024 * 1024)
+    assert written == len(data)
+
+
+def test_copy_stream_with_cap_aborts_on_overflow():
+    """_copy_stream_with_cap raises a zip-bomb guard error mid-stream when the
+    decompressed total exceeds the cap - the real decompression-bomb protection.
+    Satisfies: C1
+    """
+    data = b"x" * (10 * 1024)  # 10 KiB
+    with pytest.raises(ValueError) as excinfo:
+        C._copy_stream_with_cap(io.BytesIO(data), None, cap=1024)  # 1 KiB cap
+    assert "zip-bomb" in str(excinfo.value).lower()
+
+
+def test_c1_zip_bomb_guard_rejects_large_member(client, fixture_epub_bytes, monkeypatch):
+    """A genuinely large member (ZIP_BOMB_MIN_CAP) fails the job with a 'zip-bomb'
+    error through the public HTTP API. Production floor is 2 GiB (impractical in a
+    unit test), so the floor is shrunk so the guard logic is exercised with a 4 MiB
+    member in a ~8 KiB upload. Satisfies: C1
+    """
+    monkeypatch.setattr(app, "ZIP_BOMB_MIN_CAP", 1024 * 1024)  # 1 MiB floor
+    large = b"\x00" * (4 * 1024 * 1024)  # 4 MiB of NUL bytes, exceeds the 1 MiB cap
+    content = _append_member(fixture_epub_bytes, "OEBPS/bomb.bin", large)
+    job = _upload(client, content, "bomb.epub")
+    assert job["status"] == "error", job
+    assert "zip-bomb" in job["error"].lower()
+
+
+# ---------------------------------------------------------------------------
+# Finding C3 (regression): stored-XSS stripping in rendered HTML/SVG
+# ---------------------------------------------------------------------------
+# WeasyPrint already drops <script> content and event-handler attribute VALUES from
+# rendered PDF *text* (confirmed empirically), so 'script absent from the PDF' is NOT
+# by itself a proof the guard works. The AUTHORITATIVE proof is the C3 unit test on
+# common._strip_scripts (it exercises the guard directly); the e2e below proves the
+# guard is wired into the render path without breaking the document (job succeeds and
+# visible content still renders). Satisfies: C3
+
+
+def test_strip_scripts_removes_all_vectors():
+    """_strip_scripts removes <script>/<iframe> content, on* handlers and
+    javascript:/vbscript: URLs. This is the authoritative C3 guard test.
+    Satisfies: C3
+    """
+    html = (
+        '<div onmouseover="EVIL_ATTR()">BODY</div>'
+        "<script>SCRIPT_BODY</script>"
+        "<iframe>IFRAME_BODY</iframe>"
+        '<a href="javascript:EVIL_JS()">JS_LINK</a>'
+        '<a href="vbscript:EVIL_VB()">VB_LINK</a>'
+    )
+    out = C._strip_scripts(html)
+    assert "SCRIPT_BODY" not in out
+    assert "IFRAME_BODY" not in out
+    assert "EVIL_ATTR()" not in out
+    assert "EVIL_JS()" not in out
+    assert "EVIL_VB()" not in out
+    assert "onmouseover" not in out.lower()
+    assert "javascript:" not in out.lower()
+    assert "vbscript:" not in out.lower()
+
+
+def test_strip_scripts_preserves_visible_and_safe_urls():
+    """_strip_scripts leaves visible text and safe data:/https: references intact.
+    Satisfies: C3
+    """
+    html = (
+        "<p>VISIBLE_TEXT</p>"
+        '<img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAIAAACQd1Pe'
+        'AAAADElEQVR4nGP4z8AAAAMBAQDJ/pLvAAAAAElFTkSuQmCC" alt="local>"'
+        '<a href="https://example.com/ok">SAFE_LINK</a>'
+    )
+    out = C._strip_scripts(html)
+    assert "VISIBLE_TEXT" in out
+    assert "SAFE_LINK" in out
+    assert "https://example.com/ok" in out
+    assert "data:image/png;base64," in out
+
+
+def test_c3_html_guard_wired_job_done_and_content_preserved(client):
+    """An .html upload with <script>, an on* handler and a javascript: link converts
+    successfully (guard is wired into the render path) and the visible content is
+    preserved. Satisfies: C3
+    """
+    html = (
+        "<!DOCTYPE html><html><head><title>C3</title></head>"
+        "<body>"
+        "<script>SCRIPT_MARKER_TEXT</script>"
+        "<div onmouseover='EVIL_MARKER'>VISIBLE_MAIN_TEXT</div>"
+        '<a href="javascript:EVIL_LINK()">LINK</a>'
+        "<p>SECOND_VISIBLE</p>"
+        "</body></html>"
+    )
+    job = _upload(client, html, "xss.html")
+    assert job["status"] == "done", job
+    full = pdf_text(download_bytes(client, job["filename"]))
+    # Guard must not blank the document: visible content still renders.
+    assert "VISIBLE_MAIN_TEXT" in full
+    assert "SECOND_VISIBLE" in full
+    # Guarded vectors must not be reachable in the rendered document.
+    assert "SCRIPT_MARKER_TEXT" not in full
+    assert "EVIL_MARKER" not in full
+    assert "EVIL_LINK" not in full
+
+
+def test_c3_svg_guard_wired_job_done(client):
+    """An .svg upload with a <script> converts successfully (guard is wired into the
+    SVG render path, which also flows through common._strip_scripts).
+    Satisfies: C3
+    """
+    svg = (
+        '<svg xmlns="http://www.w3.org/2000/svg" width="100" height="100">'
+        "<script>SVG_SCRIPT_MARKER</script>"
+        '<text x="10" y="50">SVG_VISIBLE_TEXT</text>'
+        "</svg>"
+    )
+    job = _upload(client, svg, "xss.svg")
     assert job["status"] == "done", job
\ No newline at end of file