Skip to content

Conversation

@echobt
Copy link
Contributor

@echobt echobt commented Feb 4, 2026

Summary

This PR consolidates 4 security and robustness fixes into a single, cohesive change.

Included PRs:

Key Changes:

  • Added shell escaping for paths in shell-snapshot restore scripts to prevent injection
  • Replaced unwrap() with expect() for better error context in exec runner
  • Use secure random temp files instead of predictable names to prevent symlink attacks
  • Added streaming chunk timeout to prevent indefinite hangs during LLM responses

Files Modified:

  • src/cortex-shell-snapshot/src/snapshot.rs
  • src/cortex-exec/src/runner.rs
  • src/cortex-tui/Cargo.toml
  • src/cortex-tui/src/external_editor.rs

Closes #74, #76, #78, #79

This PR consolidates the following fixes:
- #74: Prevent shell injection in restore_script via path escaping
- #76: Replace unsafe unwrap() with expect() in init_client
- #78: Use secure random temp files in external editor to prevent symlink attacks
- #79: Add per-chunk streaming timeout to prevent indefinite hangs

Key changes:
- Added shell escaping for paths in shell-snapshot restore scripts
- Replaced unwrap() with expect() for better error context in exec runner
- Use secure random temp files instead of predictable names
- Added streaming chunk timeout to prevent hangs during LLM responses
@greptile-apps
Copy link

greptile-apps bot commented Feb 4, 2026

Greptile Overview

Greptile Summary

This PR consolidates four well-implemented security and robustness improvements into a single cohesive change.

Key improvements:

  • Shell injection prevention: Added proper path escaping in snapshot restore scripts using the '\"'\"' technique for single quotes, with comprehensive test coverage
  • Better error context: Replaced unwrap() with expect() providing clear error message
  • Symlink attack prevention: Migrated from predictable temp filenames to cryptographically secure random names using the tempfile crate with O_EXCL and restricted permissions
  • Streaming timeout: Added 30-second per-chunk timeout to prevent indefinite hangs when LLM connections stall mid-stream

All changes are well-documented, include appropriate tests, and follow Rust security best practices.

Confidence Score: 5/5

  • Safe to merge - all changes are security improvements with proper test coverage
  • All four security fixes are correctly implemented, well-tested, and use industry-standard approaches (shell escaping, expect over unwrap, tempfile crate, streaming timeouts)
  • No files require special attention

Important Files Changed

Filename Overview
src/cortex-shell-snapshot/src/snapshot.rs added shell_escape_path function to prevent shell injection in restore scripts, with comprehensive test coverage
src/cortex-exec/src/runner.rs replaced unwrap with expect for better error context, added per-chunk streaming timeout to prevent indefinite hangs
src/cortex-tui/src/external_editor.rs replaced predictable temp filenames with secure random temp files using tempfile crate to prevent symlink attacks
src/cortex-tui/Cargo.toml added tempfile dependency for secure temp file creation

Sequence Diagram

sequenceDiagram
    participant User
    participant TUI as Cortex TUI
    participant ExecRunner
    participant ShellSnapshot
    participant LLMProvider
    participant TempFile as Temp File System

    Note over User,TempFile: Security Improvements in Action

    User->>TUI: Press Ctrl+G for external editor
    TUI->>TempFile: Create secure temp file<br/>(random 16-byte suffix)
    TempFile-->>TUI: Secure file handle with O_EXCL
    Note over TUI,TempFile: Prevents symlink attacks
    TUI->>TUI: Open editor with temp file
    User->>TUI: Edit and save content
    TUI->>TempFile: Read edited content
    TUI->>TempFile: Delete temp file securely

    User->>ExecRunner: Execute LLM request
    ExecRunner->>ExecRunner: init_client()
    Note over ExecRunner: Uses expect() for<br/>better error context
    ExecRunner->>LLMProvider: Start streaming request
    
    loop For each chunk (30s timeout)
        LLMProvider->>ExecRunner: Stream response chunk
        Note over ExecRunner,LLMProvider: Per-chunk timeout<br/>prevents indefinite hangs
        ExecRunner->>ExecRunner: Process chunk
    end
    
    LLMProvider->>ExecRunner: Complete response
    ExecRunner->>ShellSnapshot: Generate restore script
    ShellSnapshot->>ShellSnapshot: shell_escape_path()
    Note over ShellSnapshot: Escapes single quotes<br/>using '"'"' technique
    ShellSnapshot-->>ExecRunner: Safe shell script
    ExecRunner-->>User: Return result
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@echobt
Copy link
Contributor Author

echobt commented Feb 4, 2026

Closing this PR to consolidate into a single mega-PR combining all bug fixes. The changes will be included in a new consolidated PR.

@echobt echobt closed this Feb 4, 2026
echobt added a commit that referenced this pull request Feb 4, 2026
This PR consolidates all bug fixes and security improvements from PRs #69-88 into a single cohesive change.

## Categories

### Security Fixes
- Path traversal prevention in MCP and session storage
- Shell injection prevention in restore scripts
- Secure random temp files for external editor
- TOCTOU race condition fixes

### TUI Improvements
- Overflow prevention for u16 conversions
- Cursor positioning fixes in selection lists
- Unicode width handling for popups
- Empty section handling in help browser

### Error Handling
- Graceful semaphore and init failure handling
- Improved error propagation in middleware
- Better client access error handling
- SystemTime operation safety

### Memory and Storage
- Cache size limits to prevent unbounded growth
- File lock cleanup for memory leak prevention
- fsync after critical writes for durability
- Bounded ToolResponseStore with automatic cleanup

### Protocol Robustness
- Buffer size limits for StreamProcessor
- ToolState transition validation
- State machine documentation

### Numeric Safety
- Saturating operations to prevent overflow/underflow
- Safe UTF-8 string slicing throughout codebase

### Tools
- Parameter alias support for backward compatibility
- Handler name consistency fixes

## Files Modified
Multiple files across cortex-tui, cortex-engine, cortex-exec, cortex-common,
cortex-protocol, cortex-storage, cortex-mcp-server, and other crates.

Closes #69, #70, #71, #73, #75, #80, #82, #87, #88
echobt added a commit that referenced this pull request Feb 4, 2026
This PR consolidates all bug fixes and security improvements from PRs #69-88 into a single cohesive change.

## Categories

### Security Fixes
- Path traversal prevention in MCP and session storage
- Shell injection prevention in restore scripts
- Secure random temp files for external editor
- TOCTOU race condition fixes

### TUI Improvements
- Overflow prevention for u16 conversions
- Cursor positioning fixes in selection lists
- Unicode width handling for popups
- Empty section handling in help browser

### Error Handling
- Graceful semaphore and init failure handling
- Improved error propagation in middleware
- Better client access error handling
- SystemTime operation safety

### Memory and Storage
- Cache size limits to prevent unbounded growth
- File lock cleanup for memory leak prevention
- fsync after critical writes for durability
- Bounded ToolResponseStore with automatic cleanup

### Protocol Robustness
- Buffer size limits for StreamProcessor
- ToolState transition validation
- State machine documentation

### Numeric Safety
- Saturating operations to prevent overflow/underflow
- Safe UTF-8 string slicing throughout codebase

### Tools
- Parameter alias support for backward compatibility
- Handler name consistency fixes

## Files Modified
Multiple files across cortex-tui, cortex-engine, cortex-exec, cortex-common,
cortex-protocol, cortex-storage, cortex-mcp-server, and other crates.

Closes #69, #70, #71, #73, #75, #80, #82, #87, #88
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant