fix: resolve MCP protocol and file validation issues

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."""
@@ -41,15 +47,15 @@
 def with_timeout(timeout_seconds: int = 30) -> Any:
     """Decorator to add timeout protection to functions using threading."""
 
     def decorator(func: Any) -> Any:
         @functools.wraps(func)
         def wrapper(*args: Any, **kwargs: Any) -> Any:
             import threading
 
             result: list[Any] = [None]
             exception: list[Exception | None] = [None]
 
             def target() -> None:
                 try:
                     result[0] = func(*args, **kwargs)
                 except Exception as e:
@@ -124,7 +130,7 @@
         SecurityError: If XML contains dangerous constructs
     """
     try:
         with Path(file_path).open(encoding="utf-8", errors="ignore") as f:
             content = f.read()
 
         # Check for dangerous XML patterns
@@ -175,7 +181,7 @@
         SecurityError: If JSON is too deeply nested or complex
     """
     try:
         with Path(file_path).open(encoding="utf-8", errors="ignore") as f:
             content = f.read()
 
         # Check file size first
@@ -188,9 +194,11 @@
         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:
             if current_depth > max_depth:
                 raise SecurityError("Security violation: JSON recursion depth limit exceeded")
 
@@ -207,6 +215,8 @@
     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
 
@@ -230,7 +240,7 @@
             raise SecurityError("Security violation: CSV file too large")
 
         # Analyze CSV structure
         with Path(file_path).open(encoding="utf-8", errors="ignore") as f:
             # Read first few lines to check structure
             sample = f.read(1024 * 1024)  # 1MB sample
 
@@ -282,11 +292,11 @@
         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
@@ -327,7 +337,7 @@
     """
 
     @functools.wraps(func)
     def wrapper(*args: Any, **kwargs: Any) -> Any:
         start_time = time.time()
         try:
             result = func(*args, **kwargs)
@@ -429,7 +439,7 @@
 
 
 @with_timeout(30)  # type: ignore[misc]
 def safe_convert_with_limits(markitdown_instance: MarkItDown, file_path: str) -> Any:
     """Safely convert a file with timeout and recursion protection.
 
     Args:
@@ -441,13 +451,9 @@
 
     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:
@@ -461,7 +467,7 @@
         # Check if file might contain binary data in text format
         file_path_obj = Path(validated_file_path)
         if file_path_obj.exists():
             with Path(validated_file_path).open("rb") as f:
                 data = f.read(1024)  # Read first 1KB to check
 
             # If it's a text file but contains significant binary content
@@ -531,9 +537,6 @@
             with contextlib.suppress(OSError, PermissionError):
                 Path(sanitized_file_path).unlink(missing_ok=True)
 
-        # Restore original recursion limit
-        sys.setrecursionlimit(original_limit)
-
 
 @normalize_timing
 def validate_and_sanitize_path(
@@ -590,10 +593,11 @@
             if allowed_dirs:
                 # Resolve allowed directories for proper comparison
                 resolved_allowed_dirs = [
-                    str(Path(allowed_dir).resolve()) for allowed_dir in 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 +642,45 @@
     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 +689,41 @@
 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 +804,11 @@
         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 +838,8 @@
     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 +851,11 @@
                 )
 
             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 +886,9 @@
                 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 +960,7 @@
                     ) 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 +1000,7 @@
                 )
 
         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 +1024,7 @@
 
             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"],
@@ -1085,8 +1057,8 @@
             },
         )
 
     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:
@@ -1187,10 +1159,10 @@
                         markdown_content = result.text_content
 
                         # Write file asynchronously
                         def write_file(
                             path: str = output_path, content: str = markdown_content
                         ) -> None:
                             with Path(path).open("w", encoding="utf-8") as f:
                                 f.write(content)
 
                         await asyncio.get_event_loop().run_in_executor(None, write_file)
@@ -1259,14 +1231,18 @@
 
                 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:
@@ -1274,7 +1250,7 @@
                     if response.error is not None:
                         response_dict["error"] = response.error
 
                     print(json.dumps(response_dict), flush=True)
 
                 except json.JSONDecodeError as e:
                     logger.error(f"Invalid JSON received: {e}")
@@ -1289,6 +1265,7 @@
 
 def main() -> None:
     """Main entry point for console script."""
+    configure_stdio()
 
     async def run_server() -> None:
         """Run the MCP server asynchronously."""
@@ -1298,5 +1275,16 @@
     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()