Refactor framed I/O and harden server paths #8
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements a comprehensive refactor of the Unity MCP (Model Context Protocol) Bridge focusing on protocol framing improvements, security hardening, and code consolidation. The changes span both the C# Unity Bridge and Python MCP server components.
Core Protocol Improvements:
The main enhancement centralizes framed socket I/O logic in UnityMcpBridge.cs by extracting duplicate read/write operations into reusable methods ReadFrameAsUtf8Async and WriteFrameAsync. This eliminates code duplication that existed across the HandleClientAsync method while adding robust frame size validation, including overflow checks for int.MaxValue. The framing protocol maintains backward compatibility while enforcing consistent length validation.
Security and Path Management:
Significant security improvements were made to path handling in the Python server. The new _resolve_safe_path_from_uri() function prevents directory traversal attacks by validating that all resolved paths remain within the project root using pathlib's relative_to() method. Environment variable support for UNITY_PROJECT_ROOT provides deployment flexibility while maintaining security boundaries.
Protocol Communication Fixes:
A critical fix addresses stdout interference in the MCP protocol communication. Tool registration now uses proper logging instead of print statements, preventing corruption of JSON message exchange between the server and AI clients. This is essential for MCP protocol compliance.
Development Workflow Improvements:
Script management tools were streamlined by deprecating the 'update' operation in favor of more granular apply_text_edits, aligning with modern LSP-style editing patterns. The edit bounds checking logic was enhanced to properly handle end-of-file append operations while maintaining safety.
Connection Robustness:
Handshake error handling was improved in the Unity connection layer, now sending properly framed error messages when FRAMING=1 requirements aren't met, along with better socket cleanup to prevent resource leaks.
Important Files Changed
File Changes Summary
| Filename | Score | Overview |
|---|---|---|
| UnityMcpBridge/Editor/UnityMcpBridge.cs | 4/5 | Centralizes framed I/O logic into reusable methods and adds robust frame size validation |
| UnityMcpBridge/UnityMcpServer~/src/server.py | 4/5 | Implements secure path resolution with directory traversal protection and environment variable support |
| UnityMcpBridge/UnityMcpServer~/src/tools/init.py | 5/5 | Replaces print statements with proper logging to prevent stdout interference in MCP protocol |
| UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py | 4/5 | Deprecates 'update' operation and simplifies content handling logic |
| UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py | 4/5 | Improves bounds checking logic for end-of-file append operations |
| UnityMcpBridge/UnityMcpServer~/src/unity_connection.py | 4/5 | Enhances handshake error handling with framed error responses and socket cleanup |
Confidence score: 4/5
- This PR is generally safe to merge with good architectural improvements and security enhancements
- Score reflects solid refactoring with proper error handling, though some changes modify core protocol behavior
- Pay close attention to the path resolution changes in server.py and truthiness logic changes in manage_script.py
Sequence Diagram
sequenceDiagram
participant User
participant Client as "MCP Client"
participant Server as "Python Server"
participant Unity as "Unity Bridge"
User->>Client: "Send MCP command"
Client->>Server: "Tool call with parameters"
Server->>Server: "Validate parameters"
Server->>Unity: "Connect via TCP socket"
Unity->>Server: "WELCOME UNITY-MCP 1 FRAMING=1"
Server->>Unity: "Send framed command with length header"
Unity->>Unity: "Parse JSON command"
Unity->>Unity: "Execute command via tool handlers"
Unity->>Server: "Send framed response with results"
Server->>Client: "Return structured response"
Client->>User: "Display results"
6 files reviewed, 2 comments
| "scriptType": script_type, | ||
| } | ||
| if contents is not None: | ||
| if contents: |
There was a problem hiding this comment.
logic: Changing from if contents is not None: to if contents: means empty strings will now be treated as falsy. This could break existing functionality if empty script creation was previously supported.
| # Base64 encode the contents if they exist to avoid JSON escaping issues | ||
| if contents is not None: | ||
| if action in ['create', 'update']: | ||
| if contents: |
There was a problem hiding this comment.
logic: Same truthiness change here - verify that empty string contents for non-create actions should be ignored rather than processed.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bugbot free trial expires on August 31, 2025
Learn more in the Cursor dashboard.
| if contents is not None: | ||
| if action in ['create', 'update']: | ||
| if contents: | ||
| if action == 'create': |
There was a problem hiding this comment.
Bug: Empty Scripts Fail Base64 Encoding
The if contents: condition now treats empty strings as falsy, preventing base64 encoding and sending of explicitly empty script content. This breaks creating scripts with intentionally empty content, as Unity may expect the encodedContents field even for empty strings. This impacts both create_script and manage_script.
Additional Locations (1)
| header = self._read_exact(sock, 8) | ||
| payload_len = struct.unpack('>Q', header)[0] | ||
| if payload_len == 0 or payload_len > (64 * 1024 * 1024): | ||
| if payload_len > (64 * 1024 * 1024): |
There was a problem hiding this comment.
Bug: Protocol Mismatch: Zero-Length Messages
The Python side now permits zero-length framed messages, but the Unity C# side still rejects them. This creates a protocol mismatch, leading to communication failures when empty frames are sent.
Additional Locations (1)
1 participant
Summary
Testing
pytest -qhttps://chatgpt.com/codex/tasks/task_e_68a0e9a504208327bdbeb2a5e75223fc