fix: Filter terminal escape sequence responses in PTY sessions

[inureyes]

Summary

When running terminal applications like Neovim via SSH, raw terminal capability query responses (XTGETTCAP, DA1/DA2/DA3, OSC) were appearing as visible text on screen. This PR fixes the issue by:

• Adding an escape sequence filter module that uses a state machine to identify and filter terminal query responses
• Setting TERM and COLORTERM environment variables before PTY allocation using russh's set_env API
• Filtering XTGETTCAP, DA responses, OSC responses, and DCS sequences while preserving valid escape sequences for colors, cursor movement, etc.

Changes

• New: src/pty/session/escape_filter.rs - State machine-based filter for terminal response sequences
• Modified: src/pty/session/session_manager.rs - Added TERM env var propagation and escape filtering
• Modified: src/pty/session/mod.rs - Added escape_filter module
• Modified: ARCHITECTURE.md - Documented the terminal escape sequence filtering design

Filtered Sequences

The filter handles the following types of terminal responses:

• XTGETTCAP responses (\x1bP+r...): Terminal capability query responses
• DA1/DA2/DA3 responses (\x1b[?...c): Device Attributes responses
• OSC responses (\x1b]...): Operating System Command responses (colors, clipboard)
• DCS responses (\x1bP...): Device Control String responses

Test Plan

• All 11 new escape filter unit tests pass
• All existing PTY tests pass (5 integration + 8 utils)
• Cargo check passes without errors
• Clippy passes without warnings for new code
• Manual testing: Run nvim after SSH connection via bssh
• Manual testing: Verify vim, htop, and other TUI apps work without artifacts
• Manual testing: Verify colors and cursor movements work correctly

Fixes #76

[inureyes]

Security & Performance Review: PR #77

Analysis Summary

• Scope: changed-files
• Languages: Rust
• Total issues: 6
• Critical: 0 | High: 2 | Medium: 3 | Low: 1

---

Prioritized Issue Roadmap

HIGH - State Machine Can Buffer Indefinitely in CSI State

File: /src/pty/session/escape_filter.rs
Lines: 134-145

Issue: In the FilterState::Csi state, the state machine only transitions out when it receives an alphabetic byte or ~. If malicious or malformed input sends a continuous stream of non-terminating bytes (digits, semicolons, etc.), the pending_buffer will grow until hitting the 4KB limit, potentially delaying legitimate output.

Example Attack Vector:

\x1b[1;2;3;4;5;6;7;8;9;0;1;2;3... (continues with non-terminating chars)

Impact:

• Denial of service through output delay
• Memory consumption up to 4KB per malformed sequence
• Legitimate terminal output could be significantly delayed

Recommendation: Add a reasonable limit check earlier in CSI/CsiQuestion states (e.g., CSI parameters rarely exceed 100 bytes):

// In Csi and CsiQuestion states, add:
if self.pending_buffer.len() > 256 {
    // Malformed CSI - flush and reset
    output.extend_from_slice(&self.pending_buffer);
    self.pending_buffer.clear();
    self.state = FilterState::Normal;
}

---

HIGH - PtyMessage::RemoteOutput Path Not Filtered

File: /src/pty/session/session_manager.rs
Lines: 324-330

Issue: The PtyMessage::RemoteOutput message handler writes data directly to stdout without applying the escape sequence filter. While this path appears to be used differently than the main ChannelMsg::Data path, if any code path sends terminal data through RemoteOutput, the escape sequences would leak through unfiltered.

Current Code:

Some(PtyMessage::RemoteOutput(data)) => {
    // Write directly to stdout for better performance
    if let Err(e) = io::stdout().write_all(&data) {

Recommendation: Either:

1. Apply the escape filter to this path as well, or
2. Add a comment explaining why this path is intentionally unfiltered and ensure no terminal response data can flow through it

---

MEDIUM - DCS Sequences Over-filtered

File: /src/pty/session/escape_filter.rs
Lines: 167-185

Issue: All DCS sequences (ESC P ...) are filtered by default, including those that are not responses. While XTGETTCAP responses should be filtered, some DCS sequences are legitimate terminal commands (like DECRQSS responses, sixel graphics setup, etc.).

Impact: Potential loss of legitimate terminal functionality that uses DCS sequences.

Recommendation: Be more selective about which DCS sequences to filter:

FilterState::Dcs => {
    self.pending_buffer.push(byte);
    if byte == b'+' {
        self.state = FilterState::DcsPlus;
    } else if byte == b'$' {
        // DECRQSS response - filter
        self.state = FilterState::DcsResponse;
    } else if byte == 0x1b {
        self.state = FilterState::St;
    } else if byte == 0x07 {
        // Only filter known response patterns
        if self.is_dcs_response() {
            // filter
        } else {
            output.extend_from_slice(&self.pending_buffer);
        }
        self.pending_buffer.clear();
        self.state = FilterState::Normal;
    }
}

---

MEDIUM - State Persistence After Filter Call

File: /src/pty/session/escape_filter.rs
Lines: 91-311

Issue: The filter maintains state (pending_buffer and state) across multiple calls to handle sequences split across buffer boundaries. However, there's no timeout mechanism. If a partial escape sequence is received and no more data arrives, the state will persist indefinitely, and the next data chunk may be incorrectly interpreted.

Scenario:

1. Receive: \x1b[?64 (partial DA response)
2. Long pause (user idle)
3. Receive: c hello - The 'c' completes the DA response, but "hello" comes after
4. Expected: " hello" displayed
5. Actual: Works correctly, but "hello" would appear

Actually, looking more carefully, this is handled correctly. The issue is more subtle - if we receive \x1b[ and then much later receive unrelated data like hello, the hello would be incorrectly buffered as CSI parameters until hitting a terminator.

Recommendation: Consider adding a timestamp-based flush mechanism for very old pending sequences, or document this limitation.

---

MEDIUM - OSC Parameter Parsing Integer Overflow Risk

File: /src/pty/session/escape_filter.rs
Lines: 335-341

Issue: The is_osc_response() method parses the OSC parameter number using parse::<u32>(). While Rust's parse is safe and won't overflow, a very long string of digits in the buffer could cause unnecessary processing.

Current Code:

let param_str = String::from_utf8_lossy(&self.pending_buffer[start..end]);
if let Ok(param) = param_str.parse::<u32>() {
    matches!(param, 4 | 10..=19 | 52)
}

Impact: Minor - the 4KB buffer limit prevents truly pathological cases, but the allocation of String via from_utf8_lossy is unnecessary.

Recommendation: Parse directly from bytes to avoid allocation:

fn parse_osc_param(&self) -> Option<u32> {
    let start = 2;
    let mut end = start;
    let mut value: u32 = 0;
    
    while end < self.pending_buffer.len() && end - start < 10 {
        let b = self.pending_buffer[end];
        if b.is_ascii_digit() {
            value = value.checked_mul(10)?.checked_add((b - b'0') as u32)?;
            end += 1;
        } else {
            break;
        }
    }
    
    if end > start { Some(value) } else { None }
}

---

LOW - Missing Test for Buffer Overflow Protection

File: /src/pty/session/escape_filter.rs
Lines: 297-308

Issue: The MAX_PENDING_SIZE overflow protection logic (line 299-308) is not tested. This is a critical safety mechanism that should have explicit test coverage.

Recommendation: Add test:

#[test]
fn test_buffer_overflow_protection() {
    let mut filter = EscapeSequenceFilter::new();
    // Start a DCS sequence that never terminates
    let mut input = vec![0x1b, b'P'];
    // Add enough data to exceed MAX_PENDING_SIZE (4096)
    input.extend(vec![b'x'; 4100]);
    
    let output = filter.filter(&input);
    // After overflow, buffer should be flushed to output
    assert!(!output.is_empty(), "Overflow protection should flush buffer");
}

---

Additional Observations (Informational)

1. Good Practice: The implementation correctly uses Vec::with_capacity for the output buffer, pre-allocating based on input size.

2. Good Practice: The use of a state machine is appropriate for parsing escape sequences that can span buffer boundaries.

3. Good Practice: The buffer overflow protection at 4KB is a reasonable safety measure.

4. Documentation: The module documentation is comprehensive and explains the filtering philosophy well.

5. Test Coverage: The existing 11 tests cover the main happy paths and some edge cases, but could benefit from more adversarial input testing.

---

Manual Review Required

• Verify that PtyMessage::RemoteOutput path cannot receive terminal response data
• Confirm that filtering all DCS sequences (except specific XTGETTCAP) is acceptable for the intended use case
• Manual test with Neovim, vim, htop to verify no legitimate sequences are incorrectly filtered

---

Summary

The implementation is generally well-designed with good safety measures. The two HIGH priority issues should be addressed before merging:

1. Add early termination for malformed CSI sequences to prevent output delay attacks
2. Either filter the RemoteOutput path or document why it's safe to leave unfiltered

The MEDIUM issues are worth addressing for robustness but are not blockers.

[inureyes]

Additional Fixes from Code Review

Added the following improvements based on pr-reviewer feedback:

HIGH Priority Fixes

1. CSI Buffer Limit (escape_filter.rs):

   • Added MAX_CSI_SEQUENCE_SIZE (256 bytes) to prevent malformed CSI sequences from buffering indefinitely
   • If a CSI sequence exceeds this limit without a proper terminator, it's flushed to output instead of potentially blocking legitimate output

2. RemoteOutput Filtering (session_manager.rs):

   • Applied escape sequence filter to PtyMessage::RemoteOutput path for consistent filtering across all output channels
   • Previously this path bypassed the filter, which could have allowed escape sequences to leak through

Additional Improvements

• Added 3 new tests for buffer overflow protection and malformed CSI sequence handling
• Fixed clippy warning by using derive(Default) for StrictHostKeyChecking enum

All 14 escape filter tests pass (up from 11).