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 <...> 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): "" ) 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 " + "" + 'JS_LINK' + 'VB_LINK' + ) + 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 = ( + "

VISIBLE_TEXT

" + 'local>SAFE_LINK' + ) + 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 " + "
VISIBLE_MAIN_TEXT
" + 'LINK' + "

SECOND_VISIBLE

" + "" + ) + 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 " + 'SVG_VISIBLE_TEXT' + "" + ) + job = _upload(client, svg, "xss.svg") assert job["status"] == "done", job \ No newline at end of file