fix(tui): prevent path traversal in session storage via session_id sanitization

[echobt]

Summary

Prevent path traversal attacks in session storage by sanitizing session IDs.

Problem

The session_dir() function was vulnerable to path traversal attacks where a malicious session_id like '../../../etc/passwd' could escape the sessions directory.

Solution

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

Testing

Added unit tests:

• test_validate_session_id
• test_sanitize_session_id
• test_session_dir_path_traversal

Related Issue

Fixes issue #5404

[greptile-apps]

Greptile Overview

Greptile Summary

This PR introduces security hardening to prevent path traversal attacks in the session storage system by sanitizing session IDs before they're used in filesystem paths.

Key Changes:

• Added sanitize_session_id() function that replaces any non-alphanumeric characters (except hyphens and underscores) with underscores
• Added validate_session_id() public function for pre-validation of untrusted input
• Modified session_dir() to automatically sanitize session IDs before creating paths
• Added comprehensive unit tests covering validation, sanitization, and path traversal prevention

Security Impact:
The fix prevents attackers from using malicious session IDs like ../../../etc/passwd to escape the sessions directory. All session IDs are now sanitized to contain only safe characters (alphanumeric, hyphen, underscore).

Issues Found:

• The list_sessions() function reads directory names without validation before passing them to load_meta(). While the sanitization in session_dir() provides defense-in-depth, manually created malicious directories could still be processed. Adding validation would provide an additional security layer.
• Documentation for session_dir() mentions "invalid path" but the function always returns a valid, sanitized path under base_dir

Confidence Score: 4/5

• This PR is safe to merge with one minor security improvement recommended
• The core security fix is solid and well-tested. The sanitization approach is correct and comprehensive. However, there's a minor gap in list_sessions() where directory names aren't validated before use, and the documentation could be clearer. These are non-critical issues that don't block the merge.
• Pay attention to the list_sessions() function (line 276) for the validation gap mentioned in the review comments

Sequence Diagram

sequenceDiagram
    participant CLI as CLI/User Input
    participant Manager as SessionManager
    participant Storage as SessionStorage
    participant Sanitize as sanitize_session_id()
    participant FS as File System

    CLI->>Manager: load(session_id)
    Manager->>Storage: load_meta(session_id)
    Storage->>Storage: session_dir(session_id)
    Storage->>Sanitize: sanitize_session_id(session_id)
    Note over Sanitize: Replace non-alphanumeric<br/>chars (except - and _)<br/>with underscores
    Sanitize-->>Storage: sanitized_id
    Storage->>Storage: base_dir.join(sanitized_id)
    Note over Storage: Path is now safe,<br/>stays under base_dir
    Storage->>FS: read meta.json from safe path
    FS-->>Storage: metadata content
    Storage-->>Manager: SessionMeta
    Manager-->>CLI: Session loaded

Loading

[greptile-apps]

_{1 file reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

documentation states function "does not panic" and mentions returning "an invalid path", but this is misleading. the function always sanitizes and returns a valid path under base_dir. consider updating the documentation to be clearer

Suggested change

	/// # 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.
	/// # Security
	///
	/// The session_id is sanitized to prevent path traversal attacks.
	/// Only alphanumeric characters, hyphens, and underscores are allowed.
	/// Any other characters are automatically replaced with underscores.
	///
	/// Returns a path under `base_dir` with the sanitized session_id.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/cortex-tui/src/session/storage.rs
Line: 89:93

Comment:
documentation states function "does not panic" and mentions returning "an invalid path", but this is misleading. the function always sanitizes and returns a valid path under `base_dir`. consider updating the documentation to be clearer

```suggestion
    /// # Security
    ///
    /// The session_id is sanitized to prevent path traversal attacks.
    /// Only alphanumeric characters, hyphens, and underscores are allowed.
    /// Any other characters are automatically replaced with underscores.
    ///
    /// Returns a path under `base_dir` with the sanitized session_id.
```

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

[greptile-apps]

Additional Comments (1)

src/cortex-tui/src/session/storage.rs
potential issue: directory names are not validated before being passed to load_meta(). if an attacker manually creates directories with malicious names like ../../../etc, they could be processed here. consider validating with validate_session_id() before calling load_meta()

                    && let Some(session_id) = path.file_name().and_then(|n| n.to_str())
                    && validate_session_id(session_id)
                    && let Ok(meta) = self.load_meta(session_id)

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/cortex-tui/src/session/storage.rs
Line: 276:277

Comment:
potential issue: directory names are not validated before being passed to `load_meta()`. if an attacker manually creates directories with malicious names like `../../../etc`, they could be processed here. consider validating with `validate_session_id()` before calling `load_meta()`

```suggestion
                    && let Some(session_id) = path.file_name().and_then(|n| n.to_str())
                    && validate_session_id(session_id)
                    && let Ok(meta) = self.load_meta(session_id)
```

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 #83 (MCP storage path traversal) into a consolidated security PR for path traversal prevention.