From 33e7e34ff8affa2db3658ecdbed873c4b58e0a3e Mon Sep 17 00:00:00 2001 From: echobt Date: Wed, 4 Feb 2026 16:51:07 +0000 Subject: [PATCH] fix(security): consolidated path traversal prevention for MCP and session storage ## Summary This PR consolidates **2 security fixes** for path traversal vulnerabilities. ### Included PRs: - #81: Prevent path traversal in session storage via session_id sanitization - #83: Prevent path traversal in MCP storage via server name sanitization ### Key Changes: - Add sanitize_session_id() function that replaces dangerous characters - Add validate_session_id() for pre-validation of untrusted input - Add sanitize_server_name() function for MCP server names - Add validate_server_name() for pre-validation of MCP server names - Only alphanumeric, hyphen, and underscore characters are allowed ### Files Modified: - src/cortex-tui/src/session/storage.rs - src/cortex-tui/src/mcp_storage.rs Closes #81, #83 --- src/cortex-tui/src/mcp_storage.rs | 76 ++++++++++++++++++++++- src/cortex-tui/src/session/storage.rs | 87 ++++++++++++++++++++++++++- 2 files changed, 161 insertions(+), 2 deletions(-) diff --git a/src/cortex-tui/src/mcp_storage.rs b/src/cortex-tui/src/mcp_storage.rs index 19b247b..cc4f845 100644 --- a/src/cortex-tui/src/mcp_storage.rs +++ b/src/cortex-tui/src/mcp_storage.rs @@ -15,6 +15,37 @@ use anyhow::{Context, Result}; use cortex_common::AppDirs; use serde::{Deserialize, Serialize}; +// ============================================================ +// SECURITY HELPERS +// ============================================================ + +/// Sanitize a server name to prevent path traversal attacks. +/// +/// Only allows alphanumeric characters, hyphens, and underscores. +/// Any other characters (including path separators) are replaced with underscores. +fn sanitize_server_name(name: &str) -> String { + name.chars() + .map(|c| { + if c.is_ascii_alphanumeric() || c == '-' || c == '_' { + c + } else { + '_' + } + }) + .collect() +} + +/// Validate a server name for safe filesystem use. +/// +/// Returns true if the name contains only safe characters. +#[allow(dead_code)] +pub fn validate_server_name(name: &str) -> bool { + !name.is_empty() + && name + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') +} + /// MCP transport type #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] #[serde(rename_all = "lowercase")] @@ -158,8 +189,11 @@ impl McpStorage { } /// Get the path to a server's config file + /// + /// The server name is sanitized to prevent path traversal attacks. fn server_path(&self, name: &str) -> PathBuf { - self.mcps_dir.join(format!("{}.json", name)) + let sanitized_name = sanitize_server_name(name); + self.mcps_dir.join(format!("{}.json", sanitized_name)) } /// Save an MCP server configuration @@ -372,4 +406,44 @@ mod tests { let result = storage.load_server("nonexistent").unwrap(); assert!(result.is_none()); } + + #[test] + fn test_sanitize_server_name() { + // Normal names stay the same + assert_eq!(sanitize_server_name("my-server"), "my-server"); + assert_eq!(sanitize_server_name("server_123"), "server_123"); + + // Path traversal attempts get sanitized + assert_eq!(sanitize_server_name("../../../etc"), "________etc"); + assert_eq!(sanitize_server_name("test/subdir"), "test_subdir"); + assert_eq!(sanitize_server_name("test\\windows"), "test_windows"); + } + + #[test] + fn test_validate_server_name() { + // Valid names + assert!(validate_server_name("my-server")); + assert!(validate_server_name("server_123")); + assert!(validate_server_name("ABC")); + + // Invalid names + assert!(!validate_server_name("../../../etc")); + assert!(!validate_server_name("test/subdir")); + assert!(!validate_server_name("")); + assert!(!validate_server_name("name with spaces")); + } + + #[test] + fn test_server_path_traversal() { + let (storage, tmp) = test_storage(); + let base_dir = tmp.path().to_path_buf(); + + // Attempt path traversal + let malicious_name = "../../../etc/passwd"; + let result_path = storage.server_path(malicious_name); + + // The result should still be under mcps_dir + assert!(result_path.starts_with(base_dir.join("mcps"))); + assert!(!result_path.to_string_lossy().contains("..")); + } } diff --git a/src/cortex-tui/src/session/storage.rs b/src/cortex-tui/src/session/storage.rs index 7e1621e..6d1b657 100644 --- a/src/cortex-tui/src/session/storage.rs +++ b/src/cortex-tui/src/session/storage.rs @@ -21,6 +21,37 @@ const META_FILE: &str = "meta.json"; /// History file name. const HISTORY_FILE: &str = "history.jsonl"; +// ============================================================ +// SECURITY HELPERS +// ============================================================ + +/// Sanitize a session ID to prevent path traversal attacks. +/// +/// Only allows alphanumeric characters, hyphens, and underscores. +/// Any other characters (including path separators) are replaced with underscores. +fn sanitize_session_id(session_id: &str) -> String { + session_id + .chars() + .map(|c| { + if c.is_ascii_alphanumeric() || c == '-' || c == '_' { + c + } else { + '_' + } + }) + .collect() +} + +/// Validate a session ID for safe filesystem use. +/// +/// Returns true if the session_id contains only safe characters. +pub fn validate_session_id(session_id: &str) -> bool { + !session_id.is_empty() + && session_id + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') +} + // ============================================================ // SESSION STORAGE // ============================================================ @@ -49,8 +80,21 @@ impl SessionStorage { } /// Gets the directory for a specific session. + /// + /// # Security + /// + /// The session_id is validated to prevent path traversal attacks. + /// Only alphanumeric characters, hyphens, and underscores are allowed. + /// + /// # Panics + /// + /// This function does not panic but will return an invalid path if + /// the session_id contains disallowed characters. Use `validate_session_id` + /// before calling this function for untrusted input. pub fn session_dir(&self, session_id: &str) -> PathBuf { - self.base_dir.join(session_id) + // Sanitize session_id to prevent path traversal + let sanitized_id = sanitize_session_id(session_id); + self.base_dir.join(&sanitized_id) } /// Gets the metadata file path for a session. @@ -429,4 +473,45 @@ mod tests { let loaded = storage.load_meta(&session_id).unwrap(); assert!(loaded.archived); } + + #[test] + fn test_validate_session_id() { + // Valid IDs + assert!(validate_session_id("abc-123")); + assert!(validate_session_id("test_session")); + assert!(validate_session_id("ABC123")); + + // Invalid IDs - path traversal attempts + assert!(!validate_session_id("../../../etc")); + assert!(!validate_session_id("..")); + assert!(!validate_session_id("test/../passwd")); + assert!(!validate_session_id("test/subdir")); + assert!(!validate_session_id("")); + } + + #[test] + fn test_sanitize_session_id() { + // Normal ID stays the same + assert_eq!(sanitize_session_id("abc-123"), "abc-123"); + assert_eq!(sanitize_session_id("test_session"), "test_session"); + + // Path traversal gets sanitized + assert_eq!(sanitize_session_id("../../../etc"), "________etc"); + assert_eq!(sanitize_session_id("test/subdir"), "test_subdir"); + assert_eq!(sanitize_session_id("test\x00evil"), "test_evil"); + } + + #[test] + fn test_session_dir_path_traversal() { + let (storage, temp) = create_test_storage(); + let base_dir = temp.path().to_path_buf(); + + // Attempt path traversal - should be sanitized + let malicious_id = "../../../etc/passwd"; + let result_path = storage.session_dir(malicious_id); + + // The result should still be under base_dir, not escaping it + assert!(result_path.starts_with(&base_dir)); + assert!(!result_path.to_string_lossy().contains("..")); + } }