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")