fix(tui): prevent path traversal in MCP storage via server name sanitization

[echobt]

Summary

Prevent path traversal attacks in MCP storage by sanitizing server names.

Problem

The server_path() function was vulnerable to path traversal attacks where a malicious server name could escape the mcps directory.

Solution

• Add sanitize_server_name() function that replaces dangerous characters
• Add validate_server_name() for pre-validation of untrusted input
• Only alphanumeric, hyphen, and underscore characters are allowed

Testing

Added unit tests for path traversal prevention.

Related Issue

Fixes issue #5403

[greptile-apps]

Greptile Overview

Greptile Summary

This PR successfully addresses a path traversal vulnerability in MCP server storage by implementing character sanitization for server names.

Key Changes

• Added sanitize_server_name() function that replaces path separators and other dangerous characters with underscores
• Added validate_server_name() public function for input validation (currently unused)
• Applied sanitization in server_path() to prevent malicious server names from escaping the mcps directory
• Comprehensive test coverage with three new tests covering sanitization, validation, and path traversal prevention

Security Analysis

The fix effectively prevents path traversal attacks. All file operations (save, load, remove, exists) go through server_path(), which sanitizes the input. Even if a malicious JSON file is manually placed in the directory with a dangerous name field, the sanitization at the server_path() boundary prevents any path traversal.

Issues Found

• Name collision risk: The validate_server_name() function exists but isn't called before saving servers. This allows servers with invalid names (e.g., "../etc") to be saved, which get sanitized to the same filename as servers with already-sanitized names (e.g., "________etc"), causing unintentional overwrites. Consider validating names in save_server() to prevent this UX issue.
• Minor suggestion issue in comment at line 200: The code suggestion for mutating the server name has implementation issues (attempting to modify a borrowed reference). The better approach is upfront validation as noted in the line 41 comment.

Confidence Score: 4/5

• This PR is safe to merge with the path traversal vulnerability effectively fixed
• The security fix is solid with defense-in-depth through sanitization at the filesystem boundary. Comprehensive test coverage validates the fix. The name collision issue is a minor UX concern, not a security vulnerability. Score is 4/5 due to the unused validation function and potential collision edge case.
• No files require special attention - the security fix is correctly implemented

Important Files Changed

Filename	Overview
src/cortex-tui/src/mcp_storage.rs	Added sanitization functions to prevent path traversal attacks in MCP server storage with comprehensive test coverage

Sequence Diagram

sequenceDiagram
    participant User
    participant UI as UI Form
    participant Storage as McpStorage
    participant Sanitizer as sanitize_server_name()
    participant FS as Filesystem

    User->>UI: Enter server name<br/>"../../../etc/passwd"
    UI->>Storage: save_server(server)
    Storage->>Sanitizer: server_path("../../../etc/passwd")
    Sanitizer->>Sanitizer: Replace '.', '/', '\' with '_'
    Sanitizer-->>Storage: "________etc_passwd"
    Storage->>FS: Write to mcps/________etc_passwd.json
    FS-->>Storage: Success
    Storage-->>UI: Saved successfully

    Note over User,FS: Path traversal attack prevented!

    User->>UI: Load server "../../../etc/passwd"
    UI->>Storage: load_server("../../../etc/passwd")
    Storage->>Sanitizer: server_path("../../../etc/passwd")
    Sanitizer-->>Storage: "________etc_passwd"
    Storage->>FS: Read mcps/________etc_passwd.json
    FS-->>Storage: JSON content
    Storage-->>UI: StoredMcpServer object

Loading

[greptile-apps]

_{1 file reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

validate_server_name is unused but should be called before saving to prevent name collision issues

The current implementation allows saving servers with invalid names (e.g., "../etc"), which get sanitized to "________etc" for the filename. This creates a collision risk where "../etc" and "________etc" would map to the same file. Consider validating names in save_server():

pub fn save_server(&self, server: &StoredMcpServer) -> Result<()> {
    if !validate_server_name(&server.name) {
        anyhow::bail!("Invalid server name: {}", server.name);
    }
    // ... rest of implementation
}

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/cortex-tui/src/mcp_storage.rs
Line: 41:47

Comment:
`validate_server_name` is unused but should be called before saving to prevent name collision issues

The current implementation allows saving servers with invalid names (e.g., `"../etc"`), which get sanitized to `"________etc"` for the filename. This creates a collision risk where `"../etc"` and `"________etc"` would map to the same file. Consider validating names in `save_server()`:

```
pub fn save_server(&self, server: &StoredMcpServer) -> Result<()> {
    if !validate_server_name(&server.name) {
        anyhow::bail!("Invalid server name: {}", server.name);
    }
    // ... rest of implementation
}
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

Additional Comments (1)

src/cortex-tui/src/mcp_storage.rs
name mismatch between stored name field and filename can cause collisions

If a user creates a server with name "../etc", it's saved as ________etc.json, but the JSON contains "name": "../etc". If another server with name "________etc" is created later, it will overwrite the first server's file. Consider sanitizing the name field before saving:

    pub fn save_server(&self, server: &StoredMcpServer) -> Result<()> {
        self.ensure_dir()?;

        let sanitized_name = sanitize_server_name(&server.name);
        let mut server = server.clone();
        server.name = sanitized_name.clone();

        let path = self.mcps_dir.join(format!("{}.json", sanitized_name));

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/cortex-tui/src/mcp_storage.rs
Line: 200:206

Comment:
name mismatch between stored name field and filename can cause collisions

If a user creates a server with name `"../etc"`, it's saved as `________etc.json`, but the JSON contains `"name": "../etc"`. If another server with name `"________etc"` is created later, it will overwrite the first server's file. Consider sanitizing the name field before saving:

```suggestion
    pub fn save_server(&self, server: &StoredMcpServer) -> Result<()> {
        self.ensure_dir()?;

        let sanitized_name = sanitize_server_name(&server.name);
        let mut server = server.clone();
        server.name = sanitized_name.clone();

        let path = self.mcps_dir.join(format!("{}.json", sanitized_name));
```

How can I resolve this? If you propose a fix, please make it concise.

[echobt]

Closing to consolidate: This path traversal fix will be merged with PR #81 (session storage path traversal) into a consolidated security PR for path traversal prevention.