From 763a835e4df9227bdd75a664e8593b3900c25d82 Mon Sep 17 00:00:00 2001 From: pagatino-afk <256631749+pagatino-afk@users.noreply.github.com> Date: Sun, 7 Jun 2026 17:44:28 +0200 Subject: [PATCH] fix: stop corrupting Office Open XML files in security validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DOCX/XLSX/PPTX files are ZIP archives whose MIME type contains the substring "xml" ("application/vnd.openxmlformats-..."). The format dispatch in validate_file_content_security() matched this substring and routed the binary ZIP through validate_xml_security(), which reads the file as UTF-8 text (errors="ignore") and writes a sanitized .xml temp copy. That destroys the ZIP structure, so MarkItDown's DocxConverter then fails with "BadZipFile: File is not a zip file" — surfaced to clients only as a generic "Tool execution failed". Fix: - Gate XML validation on real .xml/.xhtml extensions or exact XML MIME types, and explicitly exclude ZIP-based office formats. - Log the full traceback and surface the real error type/message in convert_file_tool instead of an opaque "Conversion failed", so future conversion errors are diagnosable. Add regression tests asserting OOXML files stay valid ZIPs through validation while genuine .xml files are still sanitized. Co-Authored-By: Claude Opus 4.8 (1M context) --- markitdown_mcp/server.py | 22 ++++++-- .../test_office_openxml_not_corrupted.py | 54 +++++++++++++++++++ 2 files changed, 72 insertions(+), 4 deletions(-) create mode 100644 tests/security/test_office_openxml_not_corrupted.py diff --git a/markitdown_mcp/server.py b/markitdown_mcp/server.py index 1bdfa42..4929c17 100644 --- a/markitdown_mcp/server.py +++ b/markitdown_mcp/server.py @@ -282,7 +282,16 @@ def validate_file_content_security(file_path: str) -> str: file_ext = Path(file_path).suffix.lower() # Apply format-specific validation - if (mime_type and "xml" in mime_type) or file_ext in [".xml", ".xhtml"]: + # NOTE: Office Open XML files (.docx/.xlsx/.pptx) are ZIP archives whose + # MIME type contains the substring "xml" ("openxmlformats..."). They must + # NOT be routed through the text-based XML sanitizer, which would read the + # binary ZIP as UTF-8 and corrupt it (-> BadZipFile). Gate on the real + # extension / exact XML MIME types and exclude ZIP-based office formats. + office_zip_exts = {".docx", ".xlsx", ".pptx", ".xlsm", ".docm", ".pptm", ".epub", ".zip"} + is_xml = file_ext in [".xml", ".xhtml"] or ( + mime_type in ("application/xml", "text/xml", "application/xhtml+xml") + ) + if is_xml and file_ext not in office_zip_exts: return validate_xml_security(file_path) if (mime_type and "json" in mime_type) or file_ext == ".json": return validate_json_security(file_path) @@ -1028,9 +1037,14 @@ async def convert_file_tool(self, request_id: str, arguments: dict[str, Any]) -> ) except Exception as e: - logger.error(f"Error in convert_file_tool: {e}") - # Sanitize error message to prevent information disclosure + # Always log the full traceback server-side so the underlying cause + # (e.g. BadZipFile, FileConversionException) is recoverable from logs + # instead of being swallowed by a generic message. + logger.exception("Error in convert_file_tool") error_str = str(e).lower() + # Keep sanitization for cases that may leak sensitive filesystem + # details; otherwise surface the real error type and message so + # callers can actually diagnose conversion failures. if ( "permission denied" in error_str or "access denied" in error_str @@ -1048,7 +1062,7 @@ async def convert_file_tool(self, request_id: str, arguments: dict[str, Any]) -> "extras (e.g., markitdown[pdf])" ) else: - safe_message = "Conversion failed" + safe_message = f"Conversion failed ({type(e).__name__}): {e!s}" return MCPResponse(id=request_id, error={"code": -32603, "message": safe_message}) diff --git a/tests/security/test_office_openxml_not_corrupted.py b/tests/security/test_office_openxml_not_corrupted.py new file mode 100644 index 0000000..dc036f3 --- /dev/null +++ b/tests/security/test_office_openxml_not_corrupted.py @@ -0,0 +1,54 @@ +"""Regression tests for Office Open XML (.docx/.xlsx/.pptx) handling. + +These formats are ZIP archives whose MIME type contains the substring "xml" +(``application/vnd.openxmlformats-...``). A previous version routed them through +the text-based XML sanitizer, which read the binary ZIP as UTF-8 and wrote a +corrupted copy to a ``.xml`` temp file, causing ``BadZipFile`` downstream. + +See: convert_file failing with a generic "Conversion failed" for valid .docx. +""" + +import zipfile +from pathlib import Path + +import pytest + +from markitdown_mcp.server import validate_file_content_security + + +def _make_minimal_ooxml(path: Path) -> None: + """Write a minimal but structurally valid Office Open XML (ZIP) file.""" + with zipfile.ZipFile(path, "w", zipfile.ZIP_DEFLATED) as zf: + zf.writestr( + "[Content_Types].xml", + '' + '', + ) + zf.writestr("word/document.xml", "hello") + + +@pytest.mark.parametrize("ext", [".docx", ".xlsx", ".pptx"]) +def test_ooxml_not_routed_through_xml_sanitizer(temp_dir, ext): + """A ZIP-based office file must survive security validation as a valid ZIP.""" + src = Path(temp_dir) / f"sample{ext}" + _make_minimal_ooxml(src) + assert zipfile.is_zipfile(src) # sanity: input really is a zip + + validated = validate_file_content_security(str(src)) + + # The validator must not rewrite the binary file into a corrupted .xml temp. + assert validated == str(src), "office file should not be rerouted to a sanitized temp file" + # The core regression: the file the converter receives is still a valid ZIP. + assert zipfile.is_zipfile(validated), "office ZIP was corrupted by the XML sanitizer" + + +def test_real_xml_still_sanitized(temp_dir): + """Genuine .xml files must still be routed through the XML sanitizer.""" + src = Path(temp_dir) / "data.xml" + src.write_text("hi", encoding="utf-8") + + validated = validate_file_content_security(str(src)) + + # Sanitizer returns a *new* temp path for real XML input. + assert validated != str(src) + assert validated.endswith(".xml")