From 11b526a85e4697b7b4164261783698887c8b1a34 Mon Sep 17 00:00:00 2001 From: trsdn Date: Wed, 10 Jun 2026 11:44:03 +0200 Subject: [PATCH 1/2] fix(server): resolve MCP protocol and file validation issues Fix OOXML file validation so DOCX/XLSX/PPTX ZIP containers are not routed through XML sanitization. Add MARKITDOWN_SAFE_DIRS support, handle missing home directories safely, make tools/list schema Claude-compatible, and avoid responses for JSON-RPC notifications. Stabilize performance tests with warmup and realistic memory-efficiency checks. Refs #36 Refs #38 Closes #40 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- markitdown_mcp/server.py | 223 +++++++++---------- tests/integration/test_claude_integration.py | 6 +- tests/performance/test_large_files.py | 8 + tests/performance/test_memory_usage.py | 5 +- tests/unit/test_convert_file_tool.py | 61 +++-- tests/unit/test_mcp_protocol.py | 42 ++++ 6 files changed, 208 insertions(+), 137 deletions(-) diff --git a/markitdown_mcp/server.py b/markitdown_mcp/server.py index 1bdfa42..b29163e 100644 --- a/markitdown_mcp/server.py +++ b/markitdown_mcp/server.py @@ -29,6 +29,12 @@ logging.basicConfig(level=logging.INFO) logger = logging.getLogger("markitdown-mcp") +JSONRPCId = str | int | None + +XML_MIME_TYPES = {"application/xml", "text/xml"} +JSON_MIME_TYPES = {"application/json", "text/json"} +CSV_MIME_TYPES = {"application/csv", "text/csv"} + class SecurityError(Exception): """Raised when a security violation is detected.""" @@ -188,6 +194,8 @@ def validate_json_security(file_path: str) -> str: except json.JSONDecodeError: # If it's not valid JSON, let MarkItDown handle it normally return file_path + except RecursionError as e: + raise SecurityError("Security violation: JSON recursion depth limit exceeded") from e # Check nesting depth def check_depth(obj: Any, current_depth: int = 0, max_depth: int = 30) -> None: @@ -207,6 +215,8 @@ def check_depth(obj: Any, current_depth: int = 0, max_depth: int = 30) -> None: except Exception as e: if isinstance(e, SecurityError): raise + if isinstance(e, RecursionError): + raise SecurityError("Security violation: JSON recursion depth limit exceeded") from e # If validation fails, let MarkItDown handle it return file_path @@ -282,11 +292,11 @@ 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"]: + if mime_type in XML_MIME_TYPES or file_ext in [".xml", ".xhtml"]: return validate_xml_security(file_path) - if (mime_type and "json" in mime_type) or file_ext == ".json": + if mime_type in JSON_MIME_TYPES or file_ext == ".json": return validate_json_security(file_path) - if (mime_type and "csv" in mime_type) or file_ext == ".csv": + if mime_type in CSV_MIME_TYPES or file_ext == ".csv": return validate_csv_security(file_path) # General file size check @@ -441,13 +451,9 @@ def safe_convert_with_limits(markitdown_instance: MarkItDown, file_path: str) -> Raises: TimeoutError: If conversion times out - RecursionError: If recursion limit is exceeded + RecursionError: If recursion depth is exceeded SecurityError: For security violations """ - # Set recursion limit - original_limit = sys.getrecursionlimit() - sys.setrecursionlimit(100) # Conservative limit - sanitized_file_path = None try: @@ -531,8 +537,6 @@ def safe_convert_with_limits(markitdown_instance: MarkItDown, file_path: str) -> with contextlib.suppress(OSError, PermissionError): Path(sanitized_file_path).unlink(missing_ok=True) - # Restore original recursion limit - sys.setrecursionlimit(original_limit) @normalize_timing @@ -589,11 +593,10 @@ def validate_and_sanitize_path( if file_path.startswith("/") or (len(file_path) > 2 and file_path[1] == ":"): if allowed_dirs: # Resolve allowed directories for proper comparison - resolved_allowed_dirs = [ - str(Path(allowed_dir).resolve()) for allowed_dir in allowed_dirs - ] + resolved_allowed_dirs = [Path(allowed_dir).resolve() for allowed_dir in allowed_dirs] is_allowed = any( - str(path).startswith(allowed_dir) for allowed_dir in resolved_allowed_dirs + path == allowed_dir or allowed_dir in path.parents + for allowed_dir in resolved_allowed_dirs ) if not is_allowed: raise SecurityError("Security violation: invalid path") @@ -638,24 +641,45 @@ def get_safe_working_directories() -> list[str]: safe_dirs = [] # Add current working directory - safe_dirs.append(str(Path.cwd())) + safe_dirs.append(str(Path.cwd().resolve())) # Add home directory subdirectories (but not root directories) - home = Path.home() - safe_subdirs = ["Documents", "Downloads", "Desktop", "tmp"] - for subdir in safe_subdirs: - potential_dir = home / subdir - if potential_dir.exists(): - safe_dirs.append(str(potential_dir)) + try: + home = Path.home() + except RuntimeError: + logger.warning("Could not determine user home; skipping home subdirectories") + else: + safe_subdirs = ["Documents", "Downloads", "Desktop", "tmp"] + for subdir in safe_subdirs: + potential_dir = home / subdir + if potential_dir.exists(): + safe_dirs.append(str(potential_dir.resolve())) # Add temp directories - temp_dir = Path(tempfile.gettempdir()) + temp_dir = Path(tempfile.gettempdir()).resolve() safe_dirs.append(str(temp_dir)) # Add test fixtures directory if it exists fixtures_dir = Path.cwd() / "tests" / "fixtures" if fixtures_dir.exists(): - safe_dirs.append(str(fixtures_dir)) + safe_dirs.append(str(fixtures_dir.resolve())) + + for raw_dir in os.environ.get("MARKITDOWN_SAFE_DIRS", "").split(os.pathsep): + if not raw_dir: + continue + + configured_dir = Path(raw_dir).expanduser() + if not configured_dir.is_absolute(): + logger.warning("Ignoring non-absolute MARKITDOWN_SAFE_DIRS entry: %s", raw_dir) + continue + if not configured_dir.exists(): + logger.warning("Ignoring missing MARKITDOWN_SAFE_DIRS entry: %s", raw_dir) + continue + if not configured_dir.is_dir(): + logger.warning("Ignoring non-directory MARKITDOWN_SAFE_DIRS entry: %s", raw_dir) + continue + + safe_dirs.append(str(configured_dir.resolve())) return safe_dirs @@ -664,20 +688,41 @@ def get_safe_working_directories() -> list[str]: class MCPRequest: """Represents an incoming MCP protocol request.""" - id: str + id: JSONRPCId method: str - params: dict[str, Any] + params: dict[str, Any] | None @dataclass class MCPResponse: """Represents an MCP protocol response.""" - id: str + id: JSONRPCId result: dict[str, Any] | None = None error: dict[str, Any] | None = None +def get_convert_file_tool_schema() -> dict[str, Any]: + """Return the convert_file tool schema without top-level composition keywords.""" + return { + "type": "object", + "properties": { + "file_path": { + "type": "string", + "description": "Path to the file to convert", + }, + "file_content": { + "type": "string", + "description": "Base64 encoded file content (alternative to file_path)", + }, + "filename": { + "type": "string", + "description": "Original filename when using file_content", + }, + }, + } + + class MarkItDownMCPServer: """Model Context Protocol (MCP) server for document to Markdown conversion. @@ -758,30 +803,11 @@ def get_tools(self) -> list[dict[str, Any]]: return [ { "name": "convert_file", - "description": "Convert a file to Markdown using MarkItDown", - "inputSchema": { - "type": "object", - "properties": { - "file_path": { - "type": "string", - "description": "Path to the file to convert", - }, - "file_content": { - "type": "string", - "description": ( - "Base64 encoded file content (alternative to file_path)" - ), - }, - "filename": { - "type": "string", - "description": "Original filename when using file_content", - }, - }, - "anyOf": [ - {"required": ["file_path"]}, - {"required": ["file_content", "filename"]}, - ], - }, + "description": ( + "Convert a file to Markdown using MarkItDown. Provide either " + "'file_path' OR both 'file_content' (base64) and 'filename'." + ), + "inputSchema": get_convert_file_tool_schema(), }, { "name": "list_supported_formats", @@ -811,6 +837,8 @@ def get_tools(self) -> list[dict[str, Any]]: async def handle_request(self, request: MCPRequest) -> MCPResponse: """Handle incoming MCP requests.""" try: + params = request.params or {} + if request.method == "initialize": return MCPResponse( id=request.id, @@ -822,71 +850,11 @@ async def handle_request(self, request: MCPRequest) -> MCPResponse: ) if request.method == "tools/list": - return MCPResponse( - id=request.id, - result={ - "tools": [ - { - "name": "convert_file", - "description": "Convert a file to Markdown using MarkItDown", - "inputSchema": { - "type": "object", - "properties": { - "file_path": { - "type": "string", - "description": "Path to the file to convert", - }, - "file_content": { - "type": "string", - "description": ( - "Base64 encoded file content " - "(alternative to file_path)" - ), - }, - "filename": { - "type": "string", - "description": "Original filename when using " - "file_content", - }, - }, - "anyOf": [ - {"required": ["file_path"]}, - {"required": ["file_content", "filename"]}, - ], - }, - }, - { - "name": "list_supported_formats", - "description": "List all supported file formats for conversion", - "inputSchema": {"type": "object", "properties": {}}, - }, - { - "name": "convert_directory", - "description": "Convert all supported files in a " - "directory to Markdown", - "inputSchema": { - "type": "object", - "properties": { - "input_directory": { - "type": "string", - "description": "Path to the input directory", - }, - "output_directory": { - "type": "string", - "description": "Path to the output directory " - "(optional)", - }, - }, - "required": ["input_directory"], - }, - }, - ] - }, - ) + return MCPResponse(id=request.id, result={"tools": self.get_tools()}) if request.method == "tools/call": - tool_name = request.params.get("name") - arguments = request.params.get("arguments", {}) + tool_name = params.get("name") + arguments = params.get("arguments", {}) # Validate required parameters if not tool_name: @@ -917,7 +885,9 @@ async def handle_request(self, request: MCPRequest) -> MCPResponse: id=request.id, error={"code": -32603, "message": f"Internal error: {e!s}"} ) - async def convert_file_tool(self, request_id: str, arguments: dict[str, Any]) -> MCPResponse: + async def convert_file_tool( + self, request_id: JSONRPCId, arguments: dict[str, Any] + ) -> MCPResponse: """Convert a single file to Markdown.""" try: file_path = arguments.get("file_path") @@ -989,6 +959,7 @@ async def convert_file_tool(self, request_id: str, arguments: dict[str, Any]) -> ) as temp_file: temp_file.write(decoded_content) temp_path = temp_file.name + del decoded_content try: result = safe_convert_with_limits(self.markitdown, temp_path) @@ -1028,7 +999,7 @@ 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}") + logger.exception("Error in convert_file_tool") # Sanitize error message to prevent information disclosure error_str = str(e).lower() if ( @@ -1052,7 +1023,7 @@ async def convert_file_tool(self, request_id: str, arguments: dict[str, Any]) -> return MCPResponse(id=request_id, error={"code": -32603, "message": safe_message}) - async def list_supported_formats_tool(self, request_id: str) -> MCPResponse: + async def list_supported_formats_tool(self, request_id: JSONRPCId) -> MCPResponse: """List all supported file formats.""" format_categories = { "Office Documents": [".pdf", ".docx", ".pptx", ".xlsx", ".xls"], @@ -1086,7 +1057,7 @@ async def list_supported_formats_tool(self, request_id: str) -> MCPResponse: ) async def convert_directory_tool( - self, request_id: str, arguments: dict[str, Any] + self, request_id: JSONRPCId, arguments: dict[str, Any] ) -> MCPResponse: """Convert all supported files in a directory.""" try: @@ -1259,14 +1230,18 @@ async def run(self) -> None: try: message = json.loads(line.strip()) + is_notification = "id" not in message request = MCPRequest( - id=message.get("id", "unknown"), + id=message.get("id"), method=message["method"], params=message.get("params", {}), ) response = await self.handle_request(request) + if is_notification: + continue + # Send response response_dict: dict[str, Any] = {"jsonrpc": "2.0", "id": response.id} if response.result is not None: @@ -1289,6 +1264,7 @@ async def run(self) -> None: def main() -> None: """Main entry point for console script.""" + configure_stdio() async def run_server() -> None: """Run the MCP server asynchronously.""" @@ -1298,5 +1274,16 @@ async def run_server() -> None: asyncio.run(run_server()) +def configure_stdio() -> None: + """Use UTF-8 text I/O and LF-delimited JSON-RPC messages when supported.""" + stdin_reconfigure = getattr(sys.stdin, "reconfigure", None) + if stdin_reconfigure is not None: + stdin_reconfigure(encoding="utf-8") + + stdout_reconfigure = getattr(sys.stdout, "reconfigure", None) + if stdout_reconfigure is not None: + stdout_reconfigure(encoding="utf-8", newline="\n") + + if __name__ == "__main__": main() diff --git a/tests/integration/test_claude_integration.py b/tests/integration/test_claude_integration.py index 22a9c3a..5b21d95 100644 --- a/tests/integration/test_claude_integration.py +++ b/tests/integration/test_claude_integration.py @@ -358,8 +358,10 @@ async def test_tool_schema_claude_compatibility(self): assert "file_content" in properties assert "filename" in properties - # Should have anyOf for alternative argument sets - assert "anyOf" in schema + # Claude/Anthropic clients reject top-level anyOf/oneOf/allOf. + assert "anyOf" not in schema + assert "oneOf" not in schema + assert "allOf" not in schema elif tool["name"] == "list_supported_formats": # Should accept empty arguments diff --git a/tests/performance/test_large_files.py b/tests/performance/test_large_files.py index 5333b4f..36d3e40 100644 --- a/tests/performance/test_large_files.py +++ b/tests/performance/test_large_files.py @@ -478,6 +478,14 @@ async def test_small_files_throughput(self, temp_dir): file_path.write_text(content) files.append(str(file_path)) + warmup_request = MCPRequest( + id="throughput-warmup", + method="tools/call", + params={"name": "convert_file", "arguments": {"file_path": files[0]}}, + ) + warmup_response = await server.handle_request(warmup_request) + assert_mcp_success_response(warmup_response, "throughput-warmup") + monitor.start_monitoring() # Process all files diff --git a/tests/performance/test_memory_usage.py b/tests/performance/test_memory_usage.py index 1694508..88f5349 100644 --- a/tests/performance/test_memory_usage.py +++ b/tests/performance/test_memory_usage.py @@ -389,8 +389,9 @@ async def test_directory_processing_memory_scaling(self, temp_dir): summary["peak_delta_mb"] < 200 ), f"Directory processing used excessive memory: {summary['peak_delta_mb']:.2f}MB" - # Memory efficiency should be reasonable compared to total content - if total_content_size > 0: + # Memory efficiency ratios are only meaningful once input size is large enough + # to dominate fixed interpreter/library overhead. + if total_content_size >= 1.0: efficiency_ratio = summary["peak_delta_mb"] / total_content_size assert ( efficiency_ratio < 100.0 diff --git a/tests/unit/test_convert_file_tool.py b/tests/unit/test_convert_file_tool.py index 4c39d64..3ca2608 100644 --- a/tests/unit/test_convert_file_tool.py +++ b/tests/unit/test_convert_file_tool.py @@ -3,12 +3,19 @@ """ import base64 +import os +import zipfile from pathlib import Path from unittest.mock import Mock, patch import pytest -from markitdown_mcp.server import MarkItDownMCPServer, MCPRequest +from markitdown_mcp.server import ( + MarkItDownMCPServer, + MCPRequest, + get_safe_working_directories, + validate_file_content_security, +) from tests.helpers.assertions import ( assert_convert_file_response, assert_mcp_error_response, @@ -17,7 +24,9 @@ from tests.helpers.file_utils import ( create_corrupted_file, create_large_file, + create_minimal_docx, create_minimal_pdf, + create_minimal_xlsx, create_test_file, ) @@ -78,6 +87,37 @@ async def test_convert_csv_file_by_path(self, mcp_server, sample_csv_file): content_text = response.result["content"][0]["text"] assert "Name,Age,City" in content_text or "John Doe" in content_text + @pytest.mark.unit + def test_ooxml_files_are_not_routed_through_xml_sanitizer(self, temp_dir): + """DOCX/XLSX files are ZIP containers even though their MIME type contains xml.""" + docx_path = create_minimal_docx(temp_dir) + xlsx_path = create_minimal_xlsx(temp_dir) + + for file_path in [docx_path, xlsx_path]: + assert zipfile.is_zipfile(file_path) + validated_path = validate_file_content_security(file_path) + assert validated_path == file_path + assert zipfile.is_zipfile(validated_path) + + @pytest.mark.unit + def test_markitdown_safe_dirs_env_adds_absolute_existing_directories( + self, monkeypatch, temp_dir + ): + """MARKITDOWN_SAFE_DIRS extends the operator-controlled safe directory list.""" + extra_dir = Path(temp_dir) / "extra-safe-dir" + extra_dir.mkdir() + missing_dir = Path(temp_dir) / "missing" + monkeypatch.setenv( + "MARKITDOWN_SAFE_DIRS", + os.pathsep.join([str(extra_dir), str(missing_dir), "relative-dir"]), + ) + + safe_dirs = get_safe_working_directories() + + assert str(extra_dir.resolve()) in safe_dirs + assert str(missing_dir.resolve()) not in safe_dirs + assert "relative-dir" not in safe_dirs + class TestConvertFileBase64Content: """Test convert_file tool with base64 encoded content.""" @@ -311,18 +351,11 @@ class TestConvertFileFormats: @pytest.mark.unit @pytest.mark.parametrize( - "file_extension,content_type", - [ - (".txt", "text/plain"), - (".json", "application/json"), - (".csv", "text/csv"), - (".html", "text/html"), - (".xml", "application/xml"), - (".md", "text/markdown"), - ], + ("file_extension"), + [".txt", ".json", ".csv", ".html", ".xml", ".md"], ) @pytest.mark.asyncio - async def test_supported_text_formats(self, mcp_server, temp_dir, file_extension, content_type): + async def test_supported_text_formats(self, mcp_server, temp_dir, file_extension): """Test conversion of various supported text formats.""" content_map = { ".txt": "Plain text content", @@ -479,9 +512,7 @@ class TestConvertFileWithMocking: @pytest.mark.unit @patch("markitdown_mcp.server.MarkItDown") @pytest.mark.asyncio - async def test_markitdown_success_mock( - self, mock_markitdown_class, mcp_server, sample_text_file - ): + async def test_markitdown_success_mock(self, mock_markitdown_class, sample_text_file): """Test successful conversion with mocked MarkItDown.""" # Setup mock mock_instance = mock_markitdown_class.return_value @@ -506,7 +537,7 @@ async def test_markitdown_success_mock( @pytest.mark.unit @patch("markitdown_mcp.server.MarkItDown") @pytest.mark.asyncio - async def test_markitdown_error_mock(self, mock_markitdown_class, mcp_server, sample_text_file): + async def test_markitdown_error_mock(self, mock_markitdown_class, sample_text_file): """Test error handling with mocked MarkItDown exception.""" # Setup mock to raise exception mock_instance = mock_markitdown_class.return_value diff --git a/tests/unit/test_mcp_protocol.py b/tests/unit/test_mcp_protocol.py index 68973cb..56c0325 100644 --- a/tests/unit/test_mcp_protocol.py +++ b/tests/unit/test_mcp_protocol.py @@ -2,6 +2,7 @@ Unit tests for MCP protocol handling in MarkItDown MCP Server """ +import io import json from unittest.mock import patch @@ -107,6 +108,20 @@ async def test_tools_list_method(self, mcp_server, mcp_tools_list_request): assert "type" in tool["inputSchema"] assert tool["inputSchema"]["type"] == "object" + @pytest.mark.unit + @pytest.mark.asyncio + async def test_convert_file_schema_avoids_top_level_composition(self, mcp_server): + """Anthropic MCP clients reject anyOf/oneOf/allOf at the inputSchema top level.""" + response = await mcp_server.handle_request(MCPRequest(id="tools-test", method="tools/list", params={})) + + convert_file_tool = next( + tool for tool in response.result["tools"] if tool["name"] == "convert_file" + ) + + assert "anyOf" not in convert_file_tool["inputSchema"] + assert "oneOf" not in convert_file_tool["inputSchema"] + assert "allOf" not in convert_file_tool["inputSchema"] + class TestUnknownMethods: """Test handling of unknown MCP methods.""" @@ -232,6 +247,33 @@ async def test_response_structure_compliance(self, mcp_server): assert parsed["id"] == "compliance-test" assert "result" in parsed + @pytest.mark.unit + @pytest.mark.asyncio + async def test_server_does_not_reply_to_notifications(self, monkeypatch): + """JSON-RPC notifications do not have an id and must not receive responses.""" + server = MarkItDownMCPServer() + stdin = io.StringIO( + json.dumps( + { + "jsonrpc": "2.0", + "method": "notifications/initialized", + "params": {}, + } + ) + + "\n" + + json.dumps({"jsonrpc": "2.0", "id": 1, "method": "initialize", "params": {}}) + + "\n" + ) + stdout = io.StringIO() + monkeypatch.setattr("sys.stdin", stdin) + monkeypatch.setattr("sys.stdout", stdout) + + await server.run() + + responses = [json.loads(line) for line in stdout.getvalue().splitlines()] + assert len(responses) == 1 + assert responses[0]["id"] == 1 + @pytest.mark.unit @pytest.mark.asyncio async def test_error_code_compliance(self, mcp_server): From 59c7181993b90f596c02e2fc4c0ef7f411ec8986 Mon Sep 17 00:00:00 2001 From: trsdn Date: Wed, 10 Jun 2026 11:59:50 +0200 Subject: [PATCH 2/2] fix: format server path validation changes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- markitdown_mcp/server.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/markitdown_mcp/server.py b/markitdown_mcp/server.py index b29163e..416c726 100644 --- a/markitdown_mcp/server.py +++ b/markitdown_mcp/server.py @@ -538,7 +538,6 @@ def safe_convert_with_limits(markitdown_instance: MarkItDown, file_path: str) -> Path(sanitized_file_path).unlink(missing_ok=True) - @normalize_timing def validate_and_sanitize_path( file_path: str, allowed_dirs: list[str] | None = None @@ -593,7 +592,9 @@ def validate_and_sanitize_path( if file_path.startswith("/") or (len(file_path) > 2 and file_path[1] == ":"): if allowed_dirs: # Resolve allowed directories for proper comparison - resolved_allowed_dirs = [Path(allowed_dir).resolve() for allowed_dir in allowed_dirs] + resolved_allowed_dirs = [ + Path(allowed_dir).resolve() for allowed_dir in allowed_dirs + ] is_allowed = any( path == allowed_dir or allowed_dir in path.parents for allowed_dir in resolved_allowed_dirs