Skip to content

Commit c25a1c9

Browse files
committed
fix(tui): prevent path traversal in MCP storage via server name sanitization
The server_path() function was vulnerable to path traversal attacks where a malicious server name like '../../../etc/passwd' could escape the mcps directory and access arbitrary files. Changes: - 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 - Path separators and other special chars are replaced with underscores - Add comprehensive unit tests for path traversal prevention Security Impact: Prevents directory traversal attacks that could lead to unauthorized file access or manipulation outside the MCP storage directory. Fixes: issue #5403
1 parent c398212 commit c25a1c9

File tree

1 file changed

+75
-1
lines changed

1 file changed

+75
-1
lines changed

src/cortex-tui/src/mcp_storage.rs

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,37 @@ use anyhow::{Context, Result};
1515
use cortex_common::AppDirs;
1616
use serde::{Deserialize, Serialize};
1717

18+
// ============================================================
19+
// SECURITY HELPERS
20+
// ============================================================
21+
22+
/// Sanitize a server name to prevent path traversal attacks.
23+
///
24+
/// Only allows alphanumeric characters, hyphens, and underscores.
25+
/// Any other characters (including path separators) are replaced with underscores.
26+
fn sanitize_server_name(name: &str) -> String {
27+
name.chars()
28+
.map(|c| {
29+
if c.is_ascii_alphanumeric() || c == '-' || c == '_' {
30+
c
31+
} else {
32+
'_'
33+
}
34+
})
35+
.collect()
36+
}
37+
38+
/// Validate a server name for safe filesystem use.
39+
///
40+
/// Returns true if the name contains only safe characters.
41+
#[allow(dead_code)]
42+
pub fn validate_server_name(name: &str) -> bool {
43+
!name.is_empty()
44+
&& name
45+
.chars()
46+
.all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_')
47+
}
48+
1849
/// MCP transport type
1950
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
2051
#[serde(rename_all = "lowercase")]
@@ -158,8 +189,11 @@ impl McpStorage {
158189
}
159190

160191
/// Get the path to a server's config file
192+
///
193+
/// The server name is sanitized to prevent path traversal attacks.
161194
fn server_path(&self, name: &str) -> PathBuf {
162-
self.mcps_dir.join(format!("{}.json", name))
195+
let sanitized_name = sanitize_server_name(name);
196+
self.mcps_dir.join(format!("{}.json", sanitized_name))
163197
}
164198

165199
/// Save an MCP server configuration
@@ -372,4 +406,44 @@ mod tests {
372406
let result = storage.load_server("nonexistent").unwrap();
373407
assert!(result.is_none());
374408
}
409+
410+
#[test]
411+
fn test_sanitize_server_name() {
412+
// Normal names stay the same
413+
assert_eq!(sanitize_server_name("my-server"), "my-server");
414+
assert_eq!(sanitize_server_name("server_123"), "server_123");
415+
416+
// Path traversal attempts get sanitized
417+
assert_eq!(sanitize_server_name("../../../etc"), "________etc");
418+
assert_eq!(sanitize_server_name("test/subdir"), "test_subdir");
419+
assert_eq!(sanitize_server_name("test\\windows"), "test_windows");
420+
}
421+
422+
#[test]
423+
fn test_validate_server_name() {
424+
// Valid names
425+
assert!(validate_server_name("my-server"));
426+
assert!(validate_server_name("server_123"));
427+
assert!(validate_server_name("ABC"));
428+
429+
// Invalid names
430+
assert!(!validate_server_name("../../../etc"));
431+
assert!(!validate_server_name("test/subdir"));
432+
assert!(!validate_server_name(""));
433+
assert!(!validate_server_name("name with spaces"));
434+
}
435+
436+
#[test]
437+
fn test_server_path_traversal() {
438+
let (storage, tmp) = test_storage();
439+
let base_dir = tmp.path().to_path_buf();
440+
441+
// Attempt path traversal
442+
let malicious_name = "../../../etc/passwd";
443+
let result_path = storage.server_path(malicious_name);
444+
445+
// The result should still be under mcps_dir
446+
assert!(result_path.starts_with(base_dir.join("mcps")));
447+
assert!(!result_path.to_string_lossy().contains(".."));
448+
}
375449
}

0 commit comments

Comments
 (0)