fix: stop corrupting Office Open XML files in security validation

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

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",
+            '<?xml version="1.0" encoding="UTF-8"?>'
+            '<Types xmlns="http://schemas.openxmlformats.org/package/2006/content-types"/>',
+        )
+        zf.writestr("word/document.xml", "<document>hello</document>")
+
+
+@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("<root><a>hi</a></root>", 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")