-
Notifications
You must be signed in to change notification settings - Fork 3
fix(common): improve path validation and tilde expansion #52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fixes #5292 and #5293 - ToolResponseStore memory issues. Problem: 1. Store grows without limit 2. Consumed responses not removed Solution: - Added ToolResponseStore with configurable max size (default: 500 entries) - Entries are removed when consumed via take() method (#5293) - Automatic periodic cleanup of expired entries based on TTL - Eviction of oldest entries when capacity is reached (#5292) Features: - MAX_STORE_SIZE constant (500) to prevent unbounded growth - DEFAULT_TTL (5 minutes) for automatic expiration - CLEANUP_INTERVAL (1 minute) for periodic cleanup - get() for peeking without removal - take() for consuming and removing entries - cleanup_expired() and cleanup_read() for manual cleanup - Stats tracking for monitoring store behavior
- server.rs: Hold write lock during entire state check and modification in handle_initialize to prevent concurrent initialization races - registry.rs: Use HashMap entry API to atomically check-and-insert plugin registration to prevent duplicate registration races Fixes #5262, #5260
Fixes bypass attempts where blocked commands like 'rm -rf' could be evaded using: - Extra whitespace: 'rm -rf' → 'rm -rf' - Quoted parts: "'rm' -rf" → 'rm -rf' - Path variants: '/bin/rm -rf' → 'rm -rf' Added normalize_command() function that: 1. Collapses whitespace by splitting/joining 2. Strips surrounding quotes from command parts 3. Extracts basename for command (first part) Also added comprehensive tests for bypass scenarios.
- Fix #5181: validate_path_safety() now checks path components instead of substring match, preventing false positives for filenames like 'file..txt' - Fix #5183: expand_tilde() now handles bare '~' path in addition to '~/' prefix - Updated tests to verify correct behavior
Greptile OverviewGreptile SummaryThis PR delivers multiple critical security and reliability fixes across path handling, command validation, concurrency control, and resource management. Key Changes:
Testing: All changes include comprehensive unit tests covering edge cases and security scenarios. Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| src/cortex-cli/src/utils/paths.rs | Fixed path traversal false positives and tilde expansion for bare ~, with comprehensive tests |
| src/cortex-engine/src/validation.rs | Added command normalization to prevent validation bypass via whitespace/quotes/paths |
| src/cortex-engine/src/tools/response_store.rs | New bounded tool response store with TTL-based cleanup and eviction policies |
| src/cortex-mcp-server/src/server.rs | Fixed TOCTOU race in initialize by holding write lock during check-and-set |
| src/cortex-plugins/src/registry.rs | Fixed TOCTOU race in plugin registration using HashMap entry API |
Sequence Diagram
sequenceDiagram
participant User
participant PathValidator
participant CommandValidator
participant MCPServer
participant PluginRegistry
participant ResponseStore
Note over PathValidator: Path Validation Fix
User->>PathValidator: validate_path("file..txt")
PathValidator->>PathValidator: Check path components (not substring)
PathValidator-->>User: ✓ Valid (not traversal)
User->>PathValidator: expand_tilde("~")
PathValidator->>PathValidator: Handle bare tilde case
PathValidator-->>User: /home/user
Note over CommandValidator: Command Bypass Prevention
User->>CommandValidator: validate("/bin/rm -rf /")
CommandValidator->>CommandValidator: normalize_command()
CommandValidator->>CommandValidator: Extract basename: "rm"
CommandValidator->>CommandValidator: Check against blocked: "rm -rf /"
CommandValidator-->>User: ✗ Blocked
Note over MCPServer,PluginRegistry: TOCTOU Race Fixes
User->>MCPServer: initialize()
MCPServer->>MCPServer: Acquire write lock
MCPServer->>MCPServer: Check state == Uninitialized
MCPServer->>MCPServer: Set state = Initializing
MCPServer->>MCPServer: Release write lock
MCPServer-->>User: ✓ Initialized
User->>PluginRegistry: register(plugin)
PluginRegistry->>PluginRegistry: Acquire write lock
PluginRegistry->>PluginRegistry: Entry API check-and-insert
PluginRegistry-->>User: ✓ Registered
Note over ResponseStore: Bounded Storage
User->>ResponseStore: store(response)
ResponseStore->>ResponseStore: Check capacity (500)
alt At capacity
ResponseStore->>ResponseStore: Evict oldest entry
end
ResponseStore->>ResponseStore: Store new response
ResponseStore-->>User: ✓ Stored
User->>ResponseStore: take(call_id)
ResponseStore->>ResponseStore: Remove entry (cleanup)
ResponseStore-->>User: Response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 files reviewed, no comments
This PR consolidates the following memory and storage fixes: - #44: Add cleanup for stale file locks to prevent memory leak - #45: Add cache size limits to prevent unbounded memory growth - #47: Add fsync after file writes to prevent data loss - #50: Bound ToolResponseStore size and cleanup consumed entries - #51: Eliminate TOCTOU races in MCP server and plugin registry - #52: Improve path validation and tilde expansion Key changes: - Added periodic cleanup of stale file locks - Implemented LRU cache limits for config discovery and tokenizer - Added fsync calls after critical file writes - Created bounded ToolResponseStore with automatic cleanup - Fixed time-of-check-time-of-use races - Improved path validation security
|
Consolidated into #80 - fix: consolidated memory and storage improvements |
Summary
Fixes #5181 and #5183 - Path validation and expansion issues.
Problems
Solution