C1+C3 review findings: streaming decompression-bomb guard + stored-XSS stripping
Test / Tests (Python 3.12) (push) Canceled after 0s
Test / Tests (Python 3.12) (push) Canceled after 0s
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).
This commit is contained in:
+29
-16
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user