From 705f4220f3c2343ca62a519d26fa7c232f9a84b2 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Wed, 10 Dec 2025 15:15:52 +0900 Subject: [PATCH 01/10] fix: Filter terminal escape sequence responses in PTY sessions 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 instead of being properly handled. Changes: - Add escape_filter module with state machine to filter terminal responses - Set TERM and COLORTERM environment variables before PTY allocation - Filter XTGETTCAP, DA responses, OSC responses, and DCS sequences - Preserve valid escape sequences for colors, cursor movement, etc. - Add comprehensive test suite for escape sequence filtering This ensures interactive applications work cleanly without visual artifacts. Fixes #76 --- ARCHITECTURE.md | 32 +- src/pty/session/escape_filter.rs | 470 +++++++++++++++++++++++++++++ src/pty/session/mod.rs | 1 + src/pty/session/session_manager.rs | 70 ++++- 4 files changed, 557 insertions(+), 16 deletions(-) create mode 100644 src/pty/session/escape_filter.rs diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index cc43b3f6..9a7932be 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1538,16 +1538,19 @@ The PTY implementation provides true terminal emulation for interactive SSH sess ### Core Components -1. **PTY Session (`pty/session/*`, Refactored 2025-10-17)** +1. **PTY Session (`pty/session/*`, Refactored 2025-10-17, Enhanced 2025-12-10)** - **Module Structure:** - - `session/session_manager.rs` - Core session management (381 lines) + - `session/session_manager.rs` - Core session management (~400 lines) - `session/input.rs` - Input event handling (193 lines) - `session/constants.rs` - Terminal key sequences and buffers (105 lines) - `session/terminal_modes.rs` - Terminal mode configuration (91 lines) - - `session/mod.rs` - Module exports (22 lines) + - `session/escape_filter.rs` - Terminal escape sequence filtering (~350 lines) + - `session/mod.rs` - Module exports (23 lines) - Manages bidirectional terminal communication - Handles terminal resize events - Processes key sequences and ANSI escape codes + - **Filters terminal query responses** (XTGETTCAP, DA1/DA2/DA3, OSC responses) + - **Sets TERM environment variable** for proper terminal capability detection - Provides graceful shutdown with proper cleanup 2. **PTY Manager (`pty/mod.rs`)** @@ -1606,6 +1609,29 @@ All timeouts and buffer sizes have been carefully chosen based on empirical test - **PTY Shutdown**: 5 seconds - Time for multiple sessions to cleanup - **SSH Exit Delay**: 100ms - Ensures remote shell processes exit command +### Terminal Escape Sequence Filtering (Added 2025-12-10) + +**Problem:** When running terminal applications like Neovim via SSH, raw terminal capability query responses (XTGETTCAP, DA1/DA2/DA3, OSC responses) were appearing as visible text instead of being properly processed. + +**Solution:** The `escape_filter` module implements a state machine to identify and filter terminal query responses: + +**Filtered Sequences:** +- **XTGETTCAP responses** (`\x1bP+r...`): Terminal capability query responses from the remote shell +- **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 + +**Filter Design:** +- State machine tracks incomplete sequences across buffer boundaries +- Conservative filtering: only removes known terminal response sequences +- Preserves all valid escape sequences for colors, cursor movement, etc. +- Buffer overflow protection (4KB limit) prevents memory exhaustion from malformed sequences + +**TERM Environment Variable:** +- Set before PTY allocation using russh's `set_env` API +- Also sets `COLORTERM=truecolor` for better color support +- Gracefully handles server rejection (AcceptEnv not configured) + ### Memory Management Strategy **Stack-Allocated Optimizations:** diff --git a/src/pty/session/escape_filter.rs b/src/pty/session/escape_filter.rs new file mode 100644 index 00000000..e6f4c836 --- /dev/null +++ b/src/pty/session/escape_filter.rs @@ -0,0 +1,470 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Terminal escape sequence filter for PTY sessions. +//! +//! This module filters out terminal escape sequence responses that should not +//! be displayed on screen. When applications like Neovim query terminal +//! capabilities, the responses can sometimes appear as raw text if not +//! properly handled. +//! +//! ## 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 +//! - **DCS responses** (`\x1bP...`): Device Control String responses +//! +//! ## Design Philosophy +//! +//! The filter uses a conservative approach: +//! - Only filters known terminal response sequences +//! - Preserves all other output including valid escape sequences for colors, etc. +//! - Uses a state machine to track incomplete sequences across buffer boundaries + +/// Filter for terminal escape sequence responses. +/// +/// This filter removes terminal query responses that applications send back +/// to the terminal but should not be visible to the user. +#[derive(Debug)] +pub struct EscapeSequenceFilter { + /// Buffer for incomplete escape sequences spanning multiple data chunks + pending_buffer: Vec, + /// Current filter state + state: FilterState, +} + +/// State machine for tracking escape sequence parsing +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum FilterState { + /// Normal text processing + Normal, + /// Saw ESC (0x1b), waiting for next character + Escape, + /// In CSI sequence (ESC [) + Csi, + /// In CSI with question mark (ESC [ ?) + CsiQuestion, + /// In DCS sequence (ESC P) + Dcs, + /// In DCS with + (ESC P +) + DcsPlus, + /// In XTGETTCAP response (ESC P + r) + Xtgettcap, + /// In OSC sequence (ESC ]) + Osc, + /// In ST (String Terminator) - saw ESC, waiting for backslash + St, +} + +impl Default for EscapeSequenceFilter { + fn default() -> Self { + Self::new() + } +} + +impl EscapeSequenceFilter { + /// Create a new escape sequence filter. + pub fn new() -> Self { + Self { + pending_buffer: Vec::with_capacity(64), + state: FilterState::Normal, + } + } + + /// Filter terminal data, removing terminal query responses. + /// + /// Returns the filtered data that should be displayed. + pub fn filter(&mut self, data: &[u8]) -> Vec { + let mut output = Vec::with_capacity(data.len()); + let mut i = 0; + + while i < data.len() { + let byte = data[i]; + + match self.state { + FilterState::Normal => { + if byte == 0x1b { + // Start of escape sequence + self.state = FilterState::Escape; + self.pending_buffer.clear(); + self.pending_buffer.push(byte); + } else { + output.push(byte); + } + } + + FilterState::Escape => { + self.pending_buffer.push(byte); + match byte { + b'[' => { + // CSI sequence + self.state = FilterState::Csi; + } + b'P' => { + // DCS sequence + self.state = FilterState::Dcs; + } + b']' => { + // OSC sequence + self.state = FilterState::Osc; + } + _ => { + // Other escape sequence - pass through + output.extend_from_slice(&self.pending_buffer); + self.pending_buffer.clear(); + self.state = FilterState::Normal; + } + } + } + + FilterState::Csi => { + self.pending_buffer.push(byte); + if byte == b'?' { + self.state = FilterState::CsiQuestion; + } else if byte.is_ascii_alphabetic() || byte == b'~' { + // End of CSI sequence - pass through (colors, cursor, etc.) + output.extend_from_slice(&self.pending_buffer); + self.pending_buffer.clear(); + self.state = FilterState::Normal; + } + // Continue collecting CSI sequence parameters + } + + FilterState::CsiQuestion => { + self.pending_buffer.push(byte); + if byte == b'c' { + // This is a DA (Device Attributes) response: ESC [ ? ... c + // Filter it out - don't add to output + tracing::trace!( + "Filtered DA response: {:?}", + String::from_utf8_lossy(&self.pending_buffer) + ); + self.pending_buffer.clear(); + self.state = FilterState::Normal; + } else if byte.is_ascii_alphabetic() || byte == b'~' { + // End of CSI ? sequence - pass through (DEC private modes) + output.extend_from_slice(&self.pending_buffer); + self.pending_buffer.clear(); + self.state = FilterState::Normal; + } + // Continue collecting CSI ? sequence parameters + } + + FilterState::Dcs => { + self.pending_buffer.push(byte); + if byte == b'+' { + self.state = FilterState::DcsPlus; + } else if byte == 0x1b { + // Start of string terminator + self.state = FilterState::St; + } else if byte == 0x07 { + // BEL as string terminator - end of DCS + // Filter out DCS sequences (most are responses) + tracing::trace!( + "Filtered DCS sequence: {:?}", + String::from_utf8_lossy(&self.pending_buffer) + ); + self.pending_buffer.clear(); + self.state = FilterState::Normal; + } + // Continue collecting DCS content + } + + FilterState::DcsPlus => { + self.pending_buffer.push(byte); + if byte == b'r' { + // XTGETTCAP response: ESC P + r ... + self.state = FilterState::Xtgettcap; + } else if byte == 0x1b { + self.state = FilterState::St; + } else if byte == 0x07 { + // BEL as string terminator + tracing::trace!( + "Filtered DCS+ sequence: {:?}", + String::from_utf8_lossy(&self.pending_buffer) + ); + self.pending_buffer.clear(); + self.state = FilterState::Normal; + } + } + + FilterState::Xtgettcap => { + self.pending_buffer.push(byte); + if byte == 0x1b { + // Start of string terminator + self.state = FilterState::St; + } else if byte == 0x07 { + // BEL as string terminator - end of XTGETTCAP response + tracing::trace!( + "Filtered XTGETTCAP response: {:?}", + String::from_utf8_lossy(&self.pending_buffer) + ); + self.pending_buffer.clear(); + self.state = FilterState::Normal; + } + // Continue collecting XTGETTCAP content + } + + FilterState::Osc => { + self.pending_buffer.push(byte); + if byte == 0x1b { + // Start of string terminator (ESC \) + self.state = FilterState::St; + } else if byte == 0x07 { + // BEL as string terminator - end of OSC + // Some OSC sequences should be passed through (like title setting) + // But OSC responses (like OSC 52 clipboard responses) should be filtered + if self.is_osc_response() { + tracing::trace!( + "Filtered OSC response: {:?}", + String::from_utf8_lossy(&self.pending_buffer) + ); + self.pending_buffer.clear(); + } else { + output.extend_from_slice(&self.pending_buffer); + self.pending_buffer.clear(); + } + self.state = FilterState::Normal; + } + // Continue collecting OSC content + } + + FilterState::St => { + self.pending_buffer.push(byte); + if byte == b'\\' { + // String terminator complete (ESC \) + // Check what type of sequence we were in + match self.pending_buffer.get(1) { + Some(b'P') => { + // DCS sequence complete - filter it + tracing::trace!( + "Filtered DCS sequence with ST: {:?}", + String::from_utf8_lossy(&self.pending_buffer) + ); + } + Some(b']') => { + // OSC sequence complete + if self.is_osc_response() { + tracing::trace!( + "Filtered OSC response with ST: {:?}", + String::from_utf8_lossy(&self.pending_buffer) + ); + } else { + output.extend_from_slice(&self.pending_buffer); + } + } + _ => { + // Unknown - pass through to be safe + output.extend_from_slice(&self.pending_buffer); + } + } + self.pending_buffer.clear(); + self.state = FilterState::Normal; + } else { + // False ST start - return to appropriate state + // This handles cases where ESC appears in the middle of a sequence + match self.pending_buffer.get(1) { + Some(b'P') => self.state = FilterState::Dcs, + Some(b']') => self.state = FilterState::Osc, + _ => { + // Malformed - pass through + output.extend_from_slice(&self.pending_buffer); + self.pending_buffer.clear(); + self.state = FilterState::Normal; + } + } + } + } + } + + i += 1; + } + + // Handle buffer timeout for incomplete sequences + // If pending buffer gets too large, it's likely garbage - flush it + const MAX_PENDING_SIZE: usize = 4096; + if self.pending_buffer.len() > MAX_PENDING_SIZE { + tracing::warn!( + "Escape sequence buffer overflow, flushing {} bytes", + self.pending_buffer.len() + ); + output.extend_from_slice(&self.pending_buffer); + self.pending_buffer.clear(); + self.state = FilterState::Normal; + } + + output + } + + /// Check if the current OSC sequence is a response that should be filtered. + fn is_osc_response(&self) -> bool { + // OSC sequences that are responses (not commands): + // - OSC 52 ; ... (clipboard response) + // - OSC 10-19 (color query responses) + // - OSC 4 ; ... (color palette response) + if self.pending_buffer.len() < 4 { + return false; + } + + // Check for OSC parameter number + // Format: ESC ] NUMBER ; ... + let start = 2; // Skip ESC ] + let mut end = start; + while end < self.pending_buffer.len() && self.pending_buffer[end].is_ascii_digit() { + end += 1; + } + + if end == start { + return false; + } + + let param_str = String::from_utf8_lossy(&self.pending_buffer[start..end]); + if let Ok(param) = param_str.parse::() { + // Filter known response types + matches!(param, 4 | 10..=19 | 52) + } else { + false + } + } + + /// Reset the filter state. + /// + /// Call this when starting a new session or after an error. + #[allow(dead_code)] + pub fn reset(&mut self) { + self.pending_buffer.clear(); + self.state = FilterState::Normal; + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_normal_text_passthrough() { + let mut filter = EscapeSequenceFilter::new(); + let input = b"Hello, World!"; + let output = filter.filter(input); + assert_eq!(output, input.to_vec()); + } + + #[test] + fn test_normal_escape_sequences_passthrough() { + let mut filter = EscapeSequenceFilter::new(); + // Color escape sequence: ESC [ 31 m (red foreground) + let input = b"\x1b[31mRed Text\x1b[0m"; + let output = filter.filter(input); + assert_eq!(output, input.to_vec()); + } + + #[test] + fn test_da_response_filtered() { + let mut filter = EscapeSequenceFilter::new(); + // DA1 response: ESC [ ? 6 4 ; 4 c + let input = b"\x1b[?64;4c"; + let output = filter.filter(input); + assert!(output.is_empty(), "DA response should be filtered"); + } + + #[test] + fn test_xtgettcap_response_filtered() { + let mut filter = EscapeSequenceFilter::new(); + // XTGETTCAP response: ESC P + r ... ST + let input = b"\x1bP+r736574726762\x1b\\"; + let output = filter.filter(input); + assert!(output.is_empty(), "XTGETTCAP response should be filtered"); + } + + #[test] + fn test_xtgettcap_with_bel_filtered() { + let mut filter = EscapeSequenceFilter::new(); + // XTGETTCAP response with BEL terminator + let input = b"\x1bP+r736574726762\x07"; + let output = filter.filter(input); + assert!( + output.is_empty(), + "XTGETTCAP response with BEL should be filtered" + ); + } + + #[test] + fn test_mixed_content() { + let mut filter = EscapeSequenceFilter::new(); + // Mix of normal text, color codes, and filtered responses + let input = b"Hello\x1b[31m\x1b[?64;4cWorld\x1b[0m"; + let output = filter.filter(input); + // Should keep: Hello, color start, World, color reset + // Should filter: DA response + assert_eq!(output, b"Hello\x1b[31mWorld\x1b[0m"); + } + + #[test] + fn test_osc_title_passthrough() { + let mut filter = EscapeSequenceFilter::new(); + // OSC 0 ; title BEL (set window title) + let input = b"\x1b]0;My Title\x07"; + let output = filter.filter(input); + // Title setting should pass through (not a response) + assert_eq!(output, input.to_vec()); + } + + #[test] + fn test_osc_color_response_filtered() { + let mut filter = EscapeSequenceFilter::new(); + // OSC 10 ; ... BEL (foreground color query response) + let input = b"\x1b]10;rgb:ffff/ffff/ffff\x07"; + let output = filter.filter(input); + assert!(output.is_empty(), "OSC color response should be filtered"); + } + + #[test] + fn test_partial_sequence_buffering() { + let mut filter = EscapeSequenceFilter::new(); + + // Send escape sequence in parts + let part1 = b"\x1b[?64"; + let part2 = b";4c"; + + let output1 = filter.filter(part1); + assert!(output1.is_empty(), "Partial sequence should be buffered"); + + let output2 = filter.filter(part2); + assert!( + output2.is_empty(), + "Complete DA response should be filtered" + ); + } + + #[test] + fn test_cursor_movement_passthrough() { + let mut filter = EscapeSequenceFilter::new(); + // Cursor movement: ESC [ H (home), ESC [ 10 ; 20 H (move to 10,20) + let input = b"\x1b[H\x1b[10;20H"; + let output = filter.filter(input); + assert_eq!(output, input.to_vec()); + } + + #[test] + fn test_dec_private_mode_passthrough() { + let mut filter = EscapeSequenceFilter::new(); + // Enable alternate screen: ESC [ ? 1049 h + let input = b"\x1b[?1049h"; + let output = filter.filter(input); + assert_eq!(output, input.to_vec()); + } +} diff --git a/src/pty/session/mod.rs b/src/pty/session/mod.rs index 2eeb54ac..02b047df 100644 --- a/src/pty/session/mod.rs +++ b/src/pty/session/mod.rs @@ -15,6 +15,7 @@ //! PTY session management for interactive SSH connections. mod constants; +mod escape_filter; mod input; mod session_manager; mod terminal_modes; diff --git a/src/pty/session/session_manager.rs b/src/pty/session/session_manager.rs index cbeb8215..115f978e 100644 --- a/src/pty/session/session_manager.rs +++ b/src/pty/session/session_manager.rs @@ -15,6 +15,7 @@ //! Core PTY session management implementation use super::constants::*; +use super::escape_filter::EscapeSequenceFilter; use super::input::handle_input_event; use super::terminal_modes::configure_terminal_modes; use crate::pty::{ @@ -46,6 +47,8 @@ pub struct PtySession { /// Message channels for internal communication (bounded to prevent memory exhaustion) msg_tx: Option>, msg_rx: Option>, + /// Filter for terminal escape sequence responses + escape_filter: EscapeSequenceFilter, } impl PtySession { @@ -67,6 +70,7 @@ impl PtySession { cancel_rx, msg_tx: Some(msg_tx), msg_rx: Some(msg_rx), + escape_filter: EscapeSequenceFilter::new(), }) } @@ -82,6 +86,34 @@ impl PtySession { // Get terminal size let (width, height) = crate::pty::utils::get_terminal_size()?; + // Set TERM environment variable before requesting PTY + // This ensures the remote shell knows the terminal capabilities + // Note: The server may not accept this (AcceptEnv in sshd_config), + // but the PTY request will also include the term type + if let Err(e) = self + .channel + .set_env(false, "TERM", &self.config.term_type) + .await + { + // Log but don't fail - server may reject env requests + tracing::debug!( + "Server did not accept TERM environment variable: {}. \ + This is expected if AcceptEnv is not configured for TERM in sshd_config. \ + The terminal type will still be set via the PTY request.", + e + ); + } + + // Also set COLORTERM for better color support detection + // Many modern terminals set this to indicate truecolor support + if let Err(e) = self + .channel + .set_env(false, "COLORTERM", "truecolor") + .await + { + tracing::trace!("Server did not accept COLORTERM environment variable: {}", e); + } + // Request PTY on the SSH channel with properly configured terminal modes // Configure terminal modes for proper sudo/passwd password input support let terminal_modes = configure_terminal_modes(); @@ -105,7 +137,11 @@ impl PtySession { .with_context(|| "Failed to request shell on SSH channel")?; self.state = PtyState::Active; - tracing::debug!("PTY session {} initialized", self.session_id); + tracing::debug!( + "PTY session {} initialized with TERM={}", + self.session_id, + self.config.term_type + ); Ok(()) } @@ -233,22 +269,30 @@ impl PtySession { msg = self.channel.wait() => { match msg { Some(ChannelMsg::Data { ref data }) => { - // Write directly to stdout - if let Err(e) = io::stdout().write_all(data) { - tracing::error!("Failed to write to stdout: {e}"); - should_terminate = true; - } else { - let _ = io::stdout().flush(); + // Filter terminal escape sequence responses before display + // This prevents raw XTGETTCAP, DA1/DA2/DA3 responses from appearing + // on screen when running applications like Neovim + let filtered_data = self.escape_filter.filter(data); + if !filtered_data.is_empty() { + if let Err(e) = io::stdout().write_all(&filtered_data) { + tracing::error!("Failed to write to stdout: {e}"); + should_terminate = true; + } else { + let _ = io::stdout().flush(); + } } } Some(ChannelMsg::ExtendedData { ref data, ext }) => { if ext == 1 { - // stderr - write to stdout as well for PTY mode - if let Err(e) = io::stdout().write_all(data) { - tracing::error!("Failed to write stderr to stdout: {e}"); - should_terminate = true; - } else { - let _ = io::stdout().flush(); + // stderr - also filter escape sequences + let filtered_data = self.escape_filter.filter(data); + if !filtered_data.is_empty() { + if let Err(e) = io::stdout().write_all(&filtered_data) { + tracing::error!("Failed to write stderr to stdout: {e}"); + should_terminate = true; + } else { + let _ = io::stdout().flush(); + } } } } From e0ad6bf0e562bd76a7c1e5a7f0b4d61e79fda130 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Wed, 10 Dec 2025 15:24:03 +0900 Subject: [PATCH 02/10] fix: Add buffer limits and improve escape filter robustness - Add MAX_CSI_SEQUENCE_SIZE limit (256 bytes) to prevent malformed CSI sequences from buffering indefinitely in Csi and CsiQuestion states - Apply escape sequence filter to PtyMessage::RemoteOutput path for consistent filtering across all output channels - Add comprehensive tests for buffer overflow protection and malformed CSI sequence handling - Fix clippy warning by using derive(Default) for StrictHostKeyChecking These changes address HIGH priority issues from code review: 1. State machine could buffer indefinitely with malformed CSI input 2. RemoteOutput path was bypassing escape sequence filter --- src/pty/session/escape_filter.rs | 85 +++++++++++++++++++++++++++++- src/pty/session/session_manager.rs | 17 +++--- src/ssh/known_hosts.rs | 9 +--- 3 files changed, 97 insertions(+), 14 deletions(-) diff --git a/src/pty/session/escape_filter.rs b/src/pty/session/escape_filter.rs index e6f4c836..5ac73837 100644 --- a/src/pty/session/escape_filter.rs +++ b/src/pty/session/escape_filter.rs @@ -35,6 +35,14 @@ //! - Preserves all other output including valid escape sequences for colors, etc. //! - Uses a state machine to track incomplete sequences across buffer boundaries +/// Maximum size for CSI sequences before forcing buffer flush. +/// Standard CSI sequences are typically under 32 bytes (e.g., "\x1b[38;2;255;255;255m" is 22 bytes). +/// Using 256 bytes provides generous headroom while preventing DoS through malformed input. +const MAX_CSI_SEQUENCE_SIZE: usize = 256; + +/// Maximum size for pending buffer before overflow protection triggers. +const MAX_PENDING_SIZE: usize = 4096; + /// Filter for terminal escape sequence responses. /// /// This filter removes terminal query responses that applications send back @@ -140,6 +148,16 @@ impl EscapeSequenceFilter { output.extend_from_slice(&self.pending_buffer); self.pending_buffer.clear(); self.state = FilterState::Normal; + } else if self.pending_buffer.len() > MAX_CSI_SEQUENCE_SIZE { + // Malformed CSI sequence - too long without terminator + // Flush buffer to prevent memory issues and output delay + tracing::trace!( + "Flushing malformed CSI sequence (size {})", + self.pending_buffer.len() + ); + output.extend_from_slice(&self.pending_buffer); + self.pending_buffer.clear(); + self.state = FilterState::Normal; } // Continue collecting CSI sequence parameters } @@ -160,6 +178,16 @@ impl EscapeSequenceFilter { output.extend_from_slice(&self.pending_buffer); self.pending_buffer.clear(); self.state = FilterState::Normal; + } else if self.pending_buffer.len() > MAX_CSI_SEQUENCE_SIZE { + // Malformed CSI ? sequence - too long without terminator + // Flush buffer to prevent memory issues and output delay + tracing::trace!( + "Flushing malformed CSI? sequence (size {})", + self.pending_buffer.len() + ); + output.extend_from_slice(&self.pending_buffer); + self.pending_buffer.clear(); + self.state = FilterState::Normal; } // Continue collecting CSI ? sequence parameters } @@ -296,7 +324,6 @@ impl EscapeSequenceFilter { // Handle buffer timeout for incomplete sequences // If pending buffer gets too large, it's likely garbage - flush it - const MAX_PENDING_SIZE: usize = 4096; if self.pending_buffer.len() > MAX_PENDING_SIZE { tracing::warn!( "Escape sequence buffer overflow, flushing {} bytes", @@ -467,4 +494,60 @@ mod tests { let output = filter.filter(input); assert_eq!(output, input.to_vec()); } + + #[test] + fn test_malformed_csi_sequence_flushed() { + let mut filter = EscapeSequenceFilter::new(); + // Create a malformed CSI sequence that exceeds MAX_CSI_SEQUENCE_SIZE (256 bytes) + // without a proper terminator (no alphabetic character or ~) + let mut malformed = vec![0x1b, b'[']; // ESC [ + // Add enough non-terminating bytes to exceed the limit + for _ in 0..300 { + malformed.push(b';'); // Keep adding parameter separators + } + malformed.push(b'X'); // Finally add a terminator + + let output = filter.filter(&malformed); + + // The malformed sequence should be flushed (not filtered) + // because it exceeded the size limit before getting a terminator + assert!(!output.is_empty(), "Malformed CSI sequence should be flushed to output"); + } + + #[test] + fn test_buffer_overflow_protection() { + let mut filter = EscapeSequenceFilter::new(); + // Create a DCS sequence that exceeds MAX_PENDING_SIZE (4096 bytes) + // DCS sequences don't have the early termination, only the global limit applies + let mut large_dcs = vec![0x1b, b'P']; // ESC P (DCS start) + // Add enough bytes to exceed the 4096 byte limit + for i in 0..5000 { + large_dcs.push(b'A' + (i % 26) as u8); + } + + let output = filter.filter(&large_dcs); + + // The buffer should be flushed when it exceeds MAX_PENDING_SIZE + assert!(!output.is_empty(), "Buffer overflow should flush content to output"); + // State should be reset to Normal + assert_eq!(filter.state, FilterState::Normal); + assert!(filter.pending_buffer.is_empty(), "Pending buffer should be cleared"); + } + + #[test] + fn test_malformed_csi_question_sequence_flushed() { + let mut filter = EscapeSequenceFilter::new(); + // Create a malformed CSI ? sequence that exceeds MAX_CSI_SEQUENCE_SIZE + let mut malformed = vec![0x1b, b'[', b'?']; // ESC [ ? + // Add enough non-terminating bytes + for _ in 0..300 { + malformed.push(b'0'); // Keep adding digits + } + malformed.push(b'h'); // Finally add a terminator + + let output = filter.filter(&malformed); + + // The malformed sequence should be flushed (not filtered) + assert!(!output.is_empty(), "Malformed CSI? sequence should be flushed to output"); + } } diff --git a/src/pty/session/session_manager.rs b/src/pty/session/session_manager.rs index 115f978e..3996cfbe 100644 --- a/src/pty/session/session_manager.rs +++ b/src/pty/session/session_manager.rs @@ -322,12 +322,17 @@ impl PtySession { } } Some(PtyMessage::RemoteOutput(data)) => { - // Write directly to stdout for better performance - if let Err(e) = io::stdout().write_all(&data) { - tracing::error!("Failed to write to stdout: {e}"); - should_terminate = true; - } else { - let _ = io::stdout().flush(); + // Apply escape filter for consistency with SSH channel data + // This path may receive data from other sources that could + // contain terminal responses that shouldn't be displayed + let filtered_data = self.escape_filter.filter(&data); + if !filtered_data.is_empty() { + if let Err(e) = io::stdout().write_all(&filtered_data) { + tracing::error!("Failed to write to stdout: {e}"); + should_terminate = true; + } else { + let _ = io::stdout().flush(); + } } } Some(PtyMessage::Resize { width, height }) => { diff --git a/src/ssh/known_hosts.rs b/src/ssh/known_hosts.rs index f6a83e40..613a7614 100644 --- a/src/ssh/known_hosts.rs +++ b/src/ssh/known_hosts.rs @@ -89,13 +89,14 @@ pub fn get_check_method(strict_mode: StrictHostKeyChecking) -> ServerCheckMethod } /// Mode for host key checking -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] pub enum StrictHostKeyChecking { /// Always verify host keys (fail on unknown/changed) Yes, /// Never verify host keys (accept all) No, /// Verify known hosts, add new ones automatically (TOFU) + #[default] AcceptNew, } @@ -105,12 +106,6 @@ impl StrictHostKeyChecking { } } -impl Default for StrictHostKeyChecking { - fn default() -> Self { - Self::AcceptNew - } -} - impl FromStr for StrictHostKeyChecking { type Err = (); From 3e75cf261f852c1b2e0776368db2eb9447916481 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Wed, 10 Dec 2025 16:01:12 +0900 Subject: [PATCH 03/10] fix: Selective DCS filtering to preserve terminal functionality - Only filter known DCS response sequences (XTGETTCAP: +r, DECRQSS: $) - Pass through non-response DCS sequences (sixel graphics, DECUDK, etc.) - Add DcsDecrqss and DcsPassthrough states for proper state tracking - Add is_dcs_response() helper method - Add 4 new tests for DCS passthrough and filtering behavior --- src/pty/session/escape_filter.rs | 144 +++++++++++++++++++++++++++---- 1 file changed, 129 insertions(+), 15 deletions(-) diff --git a/src/pty/session/escape_filter.rs b/src/pty/session/escape_filter.rs index 5ac73837..896c72d9 100644 --- a/src/pty/session/escape_filter.rs +++ b/src/pty/session/escape_filter.rs @@ -66,12 +66,16 @@ enum FilterState { Csi, /// In CSI with question mark (ESC [ ?) CsiQuestion, - /// In DCS sequence (ESC P) + /// In DCS sequence (ESC P) - not yet determined if it's a response Dcs, /// In DCS with + (ESC P +) DcsPlus, - /// In XTGETTCAP response (ESC P + r) + /// In XTGETTCAP response (ESC P + r) - should be filtered Xtgettcap, + /// In DCS $ sequence (ESC P $) - DECRQSS response, should be filtered + DcsDecrqss, + /// In DCS passthrough (non-response DCS sequence) - should pass through + DcsPassthrough, /// In OSC sequence (ESC ]) Osc, /// In ST (String Terminator) - saw ESC, waiting for backslash @@ -196,20 +200,24 @@ impl EscapeSequenceFilter { self.pending_buffer.push(byte); if byte == b'+' { self.state = FilterState::DcsPlus; + } else if byte == b'$' { + // DECRQSS response (ESC P $ ...) + self.state = FilterState::DcsDecrqss; } else if byte == 0x1b { - // Start of string terminator + // Start of string terminator - treat as passthrough + // since we didn't identify it as a known response self.state = FilterState::St; } else if byte == 0x07 { // BEL as string terminator - end of DCS - // Filter out DCS sequences (most are responses) - tracing::trace!( - "Filtered DCS sequence: {:?}", - String::from_utf8_lossy(&self.pending_buffer) - ); + // Unknown DCS type - pass through to preserve functionality + // (e.g., sixel graphics, DECUDK, application-specific sequences) + output.extend_from_slice(&self.pending_buffer); self.pending_buffer.clear(); self.state = FilterState::Normal; + } else { + // Other DCS content - transition to passthrough mode + self.state = FilterState::DcsPassthrough; } - // Continue collecting DCS content } FilterState::DcsPlus => { @@ -247,6 +255,38 @@ impl EscapeSequenceFilter { // Continue collecting XTGETTCAP content } + FilterState::DcsDecrqss => { + self.pending_buffer.push(byte); + if byte == 0x1b { + // Start of string terminator + self.state = FilterState::St; + } else if byte == 0x07 { + // BEL as string terminator - end of DECRQSS response + tracing::trace!( + "Filtered DECRQSS response: {:?}", + String::from_utf8_lossy(&self.pending_buffer) + ); + self.pending_buffer.clear(); + self.state = FilterState::Normal; + } + // Continue collecting DECRQSS content + } + + FilterState::DcsPassthrough => { + self.pending_buffer.push(byte); + if byte == 0x1b { + // Start of string terminator + self.state = FilterState::St; + } else if byte == 0x07 { + // BEL as string terminator - end of DCS passthrough + // Pass through non-response DCS sequences + output.extend_from_slice(&self.pending_buffer); + self.pending_buffer.clear(); + self.state = FilterState::Normal; + } + // Continue collecting DCS passthrough content + } + FilterState::Osc => { self.pending_buffer.push(byte); if byte == 0x1b { @@ -278,11 +318,16 @@ impl EscapeSequenceFilter { // Check what type of sequence we were in match self.pending_buffer.get(1) { Some(b'P') => { - // DCS sequence complete - filter it - tracing::trace!( - "Filtered DCS sequence with ST: {:?}", - String::from_utf8_lossy(&self.pending_buffer) - ); + // DCS sequence complete - check if it's a response + if self.is_dcs_response() { + tracing::trace!( + "Filtered DCS response with ST: {:?}", + String::from_utf8_lossy(&self.pending_buffer) + ); + } else { + // Non-response DCS - pass through + output.extend_from_slice(&self.pending_buffer); + } } Some(b']') => { // OSC sequence complete @@ -306,7 +351,21 @@ impl EscapeSequenceFilter { // False ST start - return to appropriate state // This handles cases where ESC appears in the middle of a sequence match self.pending_buffer.get(1) { - Some(b'P') => self.state = FilterState::Dcs, + Some(b'P') => { + // Return to appropriate DCS state based on content + if self.is_dcs_response() { + // Stay in a response state (Xtgettcap or DcsDecrqss) + if self.pending_buffer.len() > 3 && self.pending_buffer[2] == b'+' { + self.state = FilterState::Xtgettcap; + } else if self.pending_buffer.len() > 2 && self.pending_buffer[2] == b'$' { + self.state = FilterState::DcsDecrqss; + } else { + self.state = FilterState::DcsPassthrough; + } + } else { + self.state = FilterState::DcsPassthrough; + } + } Some(b']') => self.state = FilterState::Osc, _ => { // Malformed - pass through @@ -337,6 +396,23 @@ impl EscapeSequenceFilter { output } + /// Check if the current DCS sequence is a response that should be filtered. + fn is_dcs_response(&self) -> bool { + // DCS sequences that are responses (not commands): + // - ESC P + r ... (XTGETTCAP response) + // - ESC P $ ... (DECRQSS response) + if self.pending_buffer.len() < 3 { + return false; + } + + // Check the third byte to determine DCS type + match self.pending_buffer.get(2) { + Some(b'+') => true, // XTGETTCAP response + Some(b'$') => true, // DECRQSS response + _ => false, // Other DCS sequences (sixel, DECUDK, etc.) - pass through + } + } + /// Check if the current OSC sequence is a response that should be filtered. fn is_osc_response(&self) -> bool { // OSC sequences that are responses (not commands): @@ -550,4 +626,42 @@ mod tests { // The malformed sequence should be flushed (not filtered) assert!(!output.is_empty(), "Malformed CSI? sequence should be flushed to output"); } + + #[test] + fn test_dcs_sixel_passthrough() { + let mut filter = EscapeSequenceFilter::new(); + // Sixel graphics DCS sequence: ESC P q ... ST + // This should pass through, not be filtered + let input = b"\x1bPq#0;2;0;0;0#1;2;100;100;100\x1b\\"; + let output = filter.filter(input); + assert_eq!(output, input.to_vec(), "Sixel DCS should pass through"); + } + + #[test] + fn test_dcs_xtgettcap_filtered() { + let mut filter = EscapeSequenceFilter::new(); + // XTGETTCAP response: ESC P + r ... ST (should be filtered) + let input = b"\x1bP+r736574726762\x1b\\"; + let output = filter.filter(input); + assert!(output.is_empty(), "XTGETTCAP response should be filtered"); + } + + #[test] + fn test_dcs_decrqss_filtered() { + let mut filter = EscapeSequenceFilter::new(); + // DECRQSS response: ESC P $ r ... ST (should be filtered) + let input = b"\x1bP$r0m\x1b\\"; + let output = filter.filter(input); + assert!(output.is_empty(), "DECRQSS response should be filtered"); + } + + #[test] + fn test_dcs_generic_passthrough() { + let mut filter = EscapeSequenceFilter::new(); + // Generic DCS sequence (not + or $): ESC P 1 2 3 ST + // This should pass through + let input = b"\x1bP123\x1b\\"; + let output = filter.filter(input); + assert_eq!(output, input.to_vec(), "Generic DCS should pass through"); + } } From 5eb4a2eadd9f0e81165978b3ca9360609dd826f3 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Wed, 10 Dec 2025 16:05:53 +0900 Subject: [PATCH 04/10] fix: Add timeout mechanism for incomplete escape sequences - Add sequence_start timestamp to track when sequences begin - Flush incomplete sequences after 500ms timeout on next filter call - Add reset_to_normal() helper for consistent state cleanup - Add helper methods: has_timed_out_sequence(), flush_pending() - Add 4 new tests for timeout and timestamp functionality --- src/pty/session/escape_filter.rs | 166 +++++++++++++++++++++++++------ 1 file changed, 133 insertions(+), 33 deletions(-) diff --git a/src/pty/session/escape_filter.rs b/src/pty/session/escape_filter.rs index 896c72d9..5b10de2e 100644 --- a/src/pty/session/escape_filter.rs +++ b/src/pty/session/escape_filter.rs @@ -35,6 +35,8 @@ //! - Preserves all other output including valid escape sequences for colors, etc. //! - Uses a state machine to track incomplete sequences across buffer boundaries +use std::time::{Duration, Instant}; + /// Maximum size for CSI sequences before forcing buffer flush. /// Standard CSI sequences are typically under 32 bytes (e.g., "\x1b[38;2;255;255;255m" is 22 bytes). /// Using 256 bytes provides generous headroom while preventing DoS through malformed input. @@ -43,6 +45,10 @@ const MAX_CSI_SEQUENCE_SIZE: usize = 256; /// Maximum size for pending buffer before overflow protection triggers. const MAX_PENDING_SIZE: usize = 4096; +/// Maximum time to wait for incomplete escape sequences before flushing. +/// Terminal responses typically arrive within milliseconds, so 500ms is generous. +const SEQUENCE_TIMEOUT: Duration = Duration::from_millis(500); + /// Filter for terminal escape sequence responses. /// /// This filter removes terminal query responses that applications send back @@ -53,6 +59,8 @@ pub struct EscapeSequenceFilter { pending_buffer: Vec, /// Current filter state state: FilterState, + /// Timestamp when the current pending sequence started + sequence_start: Option, } /// State machine for tracking escape sequence parsing @@ -94,14 +102,41 @@ impl EscapeSequenceFilter { Self { pending_buffer: Vec::with_capacity(64), state: FilterState::Normal, + sequence_start: None, } } + /// Reset to normal state and clear timing. + #[inline] + fn reset_to_normal(&mut self) { + self.pending_buffer.clear(); + self.state = FilterState::Normal; + self.sequence_start = None; + } + /// Filter terminal data, removing terminal query responses. /// /// Returns the filtered data that should be displayed. pub fn filter(&mut self, data: &[u8]) -> Vec { let mut output = Vec::with_capacity(data.len()); + + // Check for timed-out incomplete sequences at the start of each filter call + if self.state != FilterState::Normal { + if let Some(start) = self.sequence_start { + if start.elapsed() > SEQUENCE_TIMEOUT { + tracing::trace!( + "Flushing timed-out escape sequence ({:?}): {:?}", + start.elapsed(), + String::from_utf8_lossy(&self.pending_buffer) + ); + output.extend_from_slice(&self.pending_buffer); + self.pending_buffer.clear(); + self.state = FilterState::Normal; + self.sequence_start = None; + } + } + } + let mut i = 0; while i < data.len() { @@ -114,6 +149,7 @@ impl EscapeSequenceFilter { self.state = FilterState::Escape; self.pending_buffer.clear(); self.pending_buffer.push(byte); + self.sequence_start = Some(Instant::now()); } else { output.push(byte); } @@ -137,8 +173,7 @@ impl EscapeSequenceFilter { _ => { // Other escape sequence - pass through output.extend_from_slice(&self.pending_buffer); - self.pending_buffer.clear(); - self.state = FilterState::Normal; + self.reset_to_normal(); } } } @@ -150,8 +185,7 @@ impl EscapeSequenceFilter { } else if byte.is_ascii_alphabetic() || byte == b'~' { // End of CSI sequence - pass through (colors, cursor, etc.) output.extend_from_slice(&self.pending_buffer); - self.pending_buffer.clear(); - self.state = FilterState::Normal; + self.reset_to_normal(); } else if self.pending_buffer.len() > MAX_CSI_SEQUENCE_SIZE { // Malformed CSI sequence - too long without terminator // Flush buffer to prevent memory issues and output delay @@ -160,8 +194,7 @@ impl EscapeSequenceFilter { self.pending_buffer.len() ); output.extend_from_slice(&self.pending_buffer); - self.pending_buffer.clear(); - self.state = FilterState::Normal; + self.reset_to_normal(); } // Continue collecting CSI sequence parameters } @@ -175,13 +208,11 @@ impl EscapeSequenceFilter { "Filtered DA response: {:?}", String::from_utf8_lossy(&self.pending_buffer) ); - self.pending_buffer.clear(); - self.state = FilterState::Normal; + self.reset_to_normal(); } else if byte.is_ascii_alphabetic() || byte == b'~' { // End of CSI ? sequence - pass through (DEC private modes) output.extend_from_slice(&self.pending_buffer); - self.pending_buffer.clear(); - self.state = FilterState::Normal; + self.reset_to_normal(); } else if self.pending_buffer.len() > MAX_CSI_SEQUENCE_SIZE { // Malformed CSI ? sequence - too long without terminator // Flush buffer to prevent memory issues and output delay @@ -190,8 +221,7 @@ impl EscapeSequenceFilter { self.pending_buffer.len() ); output.extend_from_slice(&self.pending_buffer); - self.pending_buffer.clear(); - self.state = FilterState::Normal; + self.reset_to_normal(); } // Continue collecting CSI ? sequence parameters } @@ -212,8 +242,7 @@ impl EscapeSequenceFilter { // Unknown DCS type - pass through to preserve functionality // (e.g., sixel graphics, DECUDK, application-specific sequences) output.extend_from_slice(&self.pending_buffer); - self.pending_buffer.clear(); - self.state = FilterState::Normal; + self.reset_to_normal(); } else { // Other DCS content - transition to passthrough mode self.state = FilterState::DcsPassthrough; @@ -233,8 +262,7 @@ impl EscapeSequenceFilter { "Filtered DCS+ sequence: {:?}", String::from_utf8_lossy(&self.pending_buffer) ); - self.pending_buffer.clear(); - self.state = FilterState::Normal; + self.reset_to_normal(); } } @@ -249,8 +277,7 @@ impl EscapeSequenceFilter { "Filtered XTGETTCAP response: {:?}", String::from_utf8_lossy(&self.pending_buffer) ); - self.pending_buffer.clear(); - self.state = FilterState::Normal; + self.reset_to_normal(); } // Continue collecting XTGETTCAP content } @@ -266,8 +293,7 @@ impl EscapeSequenceFilter { "Filtered DECRQSS response: {:?}", String::from_utf8_lossy(&self.pending_buffer) ); - self.pending_buffer.clear(); - self.state = FilterState::Normal; + self.reset_to_normal(); } // Continue collecting DECRQSS content } @@ -281,8 +307,7 @@ impl EscapeSequenceFilter { // BEL as string terminator - end of DCS passthrough // Pass through non-response DCS sequences output.extend_from_slice(&self.pending_buffer); - self.pending_buffer.clear(); - self.state = FilterState::Normal; + self.reset_to_normal(); } // Continue collecting DCS passthrough content } @@ -301,12 +326,10 @@ impl EscapeSequenceFilter { "Filtered OSC response: {:?}", String::from_utf8_lossy(&self.pending_buffer) ); - self.pending_buffer.clear(); } else { output.extend_from_slice(&self.pending_buffer); - self.pending_buffer.clear(); } - self.state = FilterState::Normal; + self.reset_to_normal(); } // Continue collecting OSC content } @@ -345,8 +368,7 @@ impl EscapeSequenceFilter { output.extend_from_slice(&self.pending_buffer); } } - self.pending_buffer.clear(); - self.state = FilterState::Normal; + self.reset_to_normal(); } else { // False ST start - return to appropriate state // This handles cases where ESC appears in the middle of a sequence @@ -370,8 +392,7 @@ impl EscapeSequenceFilter { _ => { // Malformed - pass through output.extend_from_slice(&self.pending_buffer); - self.pending_buffer.clear(); - self.state = FilterState::Normal; + self.reset_to_normal(); } } } @@ -389,8 +410,7 @@ impl EscapeSequenceFilter { self.pending_buffer.len() ); output.extend_from_slice(&self.pending_buffer); - self.pending_buffer.clear(); - self.state = FilterState::Normal; + self.reset_to_normal(); } output @@ -449,8 +469,28 @@ impl EscapeSequenceFilter { /// Call this when starting a new session or after an error. #[allow(dead_code)] pub fn reset(&mut self) { - self.pending_buffer.clear(); - self.state = FilterState::Normal; + self.reset_to_normal(); + } + + /// Check if there's a pending incomplete sequence that might need flushing. + /// Returns true if there's an incomplete sequence that has timed out. + #[allow(dead_code)] + pub fn has_timed_out_sequence(&self) -> bool { + if self.state != FilterState::Normal { + if let Some(start) = self.sequence_start { + return start.elapsed() > SEQUENCE_TIMEOUT; + } + } + false + } + + /// Flush any pending incomplete sequence regardless of state. + /// Returns the pending data if any. + #[allow(dead_code)] + pub fn flush_pending(&mut self) -> Vec { + let data = std::mem::take(&mut self.pending_buffer); + self.reset_to_normal(); + data } } @@ -664,4 +704,64 @@ mod tests { let output = filter.filter(input); assert_eq!(output, input.to_vec(), "Generic DCS should pass through"); } + + #[test] + fn test_sequence_start_timestamp_set() { + let mut filter = EscapeSequenceFilter::new(); + // Initially no timestamp + assert!(filter.sequence_start.is_none()); + + // Start an incomplete sequence + let _ = filter.filter(b"\x1b[?64"); + + // Timestamp should be set + assert!(filter.sequence_start.is_some()); + assert_eq!(filter.state, FilterState::CsiQuestion); + } + + #[test] + fn test_sequence_timestamp_cleared_on_completion() { + let mut filter = EscapeSequenceFilter::new(); + + // Complete a sequence + let _ = filter.filter(b"\x1b[31m"); + + // Timestamp should be cleared + assert!(filter.sequence_start.is_none()); + assert_eq!(filter.state, FilterState::Normal); + } + + #[test] + fn test_has_timed_out_sequence() { + let mut filter = EscapeSequenceFilter::new(); + + // No pending sequence - should not be timed out + assert!(!filter.has_timed_out_sequence()); + + // Start an incomplete sequence + let _ = filter.filter(b"\x1b[?64"); + + // Should not be timed out immediately + assert!(!filter.has_timed_out_sequence()); + + // State should be CsiQuestion + assert_eq!(filter.state, FilterState::CsiQuestion); + } + + #[test] + fn test_flush_pending() { + let mut filter = EscapeSequenceFilter::new(); + + // Start an incomplete sequence + let _ = filter.filter(b"\x1b[?64"); + + // Flush should return the pending data + let flushed = filter.flush_pending(); + assert_eq!(flushed, b"\x1b[?64"); + + // State should be reset + assert_eq!(filter.state, FilterState::Normal); + assert!(filter.sequence_start.is_none()); + assert!(filter.pending_buffer.is_empty()); + } } From dc0fc5a1cbf619199790c0a5ebb352c721b48eb7 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Wed, 10 Dec 2025 16:07:50 +0900 Subject: [PATCH 05/10] fix: Zero-allocation OSC parameter parsing - Replace String::from_utf8_lossy with direct byte-to-u32 parsing - Add parse_osc_param() helper method for efficient parameter extraction - Use checked_mul/checked_add for overflow protection (max 10 digits) - Add 3 new tests for OSC parameter parsing --- src/pty/session/escape_filter.rs | 83 +++++++++++++++++++++++++++----- 1 file changed, 71 insertions(+), 12 deletions(-) diff --git a/src/pty/session/escape_filter.rs b/src/pty/session/escape_filter.rs index 5b10de2e..6463f431 100644 --- a/src/pty/session/escape_filter.rs +++ b/src/pty/session/escape_filter.rs @@ -443,24 +443,40 @@ impl EscapeSequenceFilter { return false; } - // Check for OSC parameter number + // Parse OSC parameter number directly from bytes to avoid String allocation // Format: ESC ] NUMBER ; ... - let start = 2; // Skip ESC ] - let mut end = start; - while end < self.pending_buffer.len() && self.pending_buffer[end].is_ascii_digit() { - end += 1; + if let Some(param) = self.parse_osc_param() { + // Filter known response types + matches!(param, 4 | 10..=19 | 52) + } else { + false } + } - if end == start { - return false; + /// Parse OSC parameter number directly from bytes without allocation. + /// Returns None if parsing fails. + fn parse_osc_param(&self) -> Option { + let start = 2; // Skip ESC ] + let mut idx = start; + let mut value: u32 = 0; + + // Limit to 10 digits to prevent overflow (max u32 is 10 digits) + while idx < self.pending_buffer.len() && idx - start < 10 { + let byte = self.pending_buffer[idx]; + if byte.is_ascii_digit() { + // Safe: checked_mul and checked_add prevent overflow + value = value.checked_mul(10)?.checked_add((byte - b'0') as u32)?; + idx += 1; + } else { + break; + } } - let param_str = String::from_utf8_lossy(&self.pending_buffer[start..end]); - if let Ok(param) = param_str.parse::() { - // Filter known response types - matches!(param, 4 | 10..=19 | 52) + // Return Some only if we parsed at least one digit + if idx > start { + Some(value) } else { - false + None } } @@ -764,4 +780,47 @@ mod tests { assert!(filter.sequence_start.is_none()); assert!(filter.pending_buffer.is_empty()); } + + #[test] + fn test_parse_osc_param_valid() { + let mut filter = EscapeSequenceFilter::new(); + + // OSC 10 - should parse to 10 + filter.pending_buffer = b"\x1b]10;test\x07".to_vec(); + assert_eq!(filter.parse_osc_param(), Some(10)); + + // OSC 52 - should parse to 52 + filter.pending_buffer = b"\x1b]52;data\x07".to_vec(); + assert_eq!(filter.parse_osc_param(), Some(52)); + + // OSC 0 - should parse to 0 + filter.pending_buffer = b"\x1b]0;title\x07".to_vec(); + assert_eq!(filter.parse_osc_param(), Some(0)); + + // OSC 4 - should parse to 4 + filter.pending_buffer = b"\x1b]4;color\x07".to_vec(); + assert_eq!(filter.parse_osc_param(), Some(4)); + } + + #[test] + fn test_parse_osc_param_invalid() { + let mut filter = EscapeSequenceFilter::new(); + + // No digits after ESC ] + filter.pending_buffer = b"\x1b];text\x07".to_vec(); + assert_eq!(filter.parse_osc_param(), None); + + // Buffer too short + filter.pending_buffer = b"\x1b]".to_vec(); + assert_eq!(filter.parse_osc_param(), None); + } + + #[test] + fn test_parse_osc_param_overflow_protection() { + let mut filter = EscapeSequenceFilter::new(); + + // Very large number that would overflow u32 - should return None + filter.pending_buffer = b"\x1b]99999999999;test\x07".to_vec(); + assert_eq!(filter.parse_osc_param(), None); + } } From dbfd54b370b7f878bcdc9a340c92d17790897bac Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Wed, 10 Dec 2025 16:15:59 +0900 Subject: [PATCH 06/10] style: Apply rustfmt formatting --- Cargo.lock | 216 ++++++++++++++++++----------- Cargo.toml | 6 +- src/pty/session/escape_filter.rs | 34 +++-- src/pty/session/session_manager.rs | 11 +- 4 files changed, 167 insertions(+), 100 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 57ce1815..a8cf974c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -24,7 +24,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d122413f284cf2d62fb1b7db97e02edb8cda96d769b16e443a4f6195e35662b0" dependencies = [ "crypto-common", - "generic-array", + "generic-array 0.14.9", ] [[package]] @@ -266,7 +266,7 @@ dependencies = [ "miniz_oxide", "object", "rustc-demangle", - "windows-link 0.2.1", + "windows-link", ] [[package]] @@ -336,7 +336,7 @@ version = "0.10.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3078c7629b62d3f0439517fa394996acacc5cbc91c5a20d8c658e77abd503a71" dependencies = [ - "generic-array", + "generic-array 0.14.9", ] [[package]] @@ -345,7 +345,7 @@ version = "0.3.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a8894febbff9f758034a5b8e12d87918f56dfc64a8e1fe757d65e29041538d93" dependencies = [ - "generic-array", + "generic-array 0.14.9", ] [[package]] @@ -505,7 +505,7 @@ dependencies = [ "js-sys", "num-traits", "wasm-bindgen", - "windows-link 0.2.1", + "windows-link", ] [[package]] @@ -682,9 +682,9 @@ checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" [[package]] name = "core-models" -version = "0.0.3" +version = "0.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "94950e87ea550d6d68f1993f3e7bebc8cb7235157bff84337d46195c3aa0b3f0" +checksum = "0940496e5c83c54f3b753d5317daec82e8edac71c33aaa1f666d76f518de2444" dependencies = [ "hax-lib", "pastey", @@ -764,7 +764,7 @@ version = "0.5.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0dc92fb57ca44df6db8059111ab3af99a63d5d0f8375d9972e319a379c6bab76" dependencies = [ - "generic-array", + "generic-array 0.14.9", "rand_core 0.6.4", "subtle", "zeroize", @@ -776,7 +776,7 @@ version = "0.1.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1bfb12502f3fc46cca1bb51ac28df9d618d813cdc3d2f25b9fe775a34af26bb3" dependencies = [ - "generic-array", + "generic-array 0.14.9", "typenum", ] @@ -1029,7 +1029,7 @@ dependencies = [ "crypto-bigint", "digest", "ff", - "generic-array", + "generic-array 0.14.9", "group", "hkdf", "pem-rfc7468", @@ -1274,6 +1274,17 @@ dependencies = [ "zeroize", ] +[[package]] +name = "generic-array" +version = "1.3.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eaf57c49a95fd1fe24b90b3033bee6dc7e8f1288d51494cb44e627c295e38542" +dependencies = [ + "generic-array 0.14.9", + "rustversion", + "typenum", +] + [[package]] name = "getrandom" version = "0.2.16" @@ -1560,7 +1571,7 @@ dependencies = [ "js-sys", "log", "wasm-bindgen", - "windows-core 0.62.2", + "windows-core", ] [[package]] @@ -1617,7 +1628,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "879f10e63c20629ecabbb64a8010319738c66a5cd0c29b02d63d272b03751d01" dependencies = [ "block-padding", - "generic-array", + "generic-array 0.14.9", ] [[package]] @@ -1730,9 +1741,9 @@ checksum = "2874a2af47a2325c2001a6e6fad9b16a53b802102b528163885171cf92b15976" [[package]] name = "libcrux-intrinsics" -version = "0.0.3" +version = "0.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5d3b41dcbc21a5fb7efbbb5af7405b2e79c4bfe443924e90b13afc0080318d31" +checksum = "bc9ee7ef66569dd7516454fe26de4e401c0c62073929803486b96744594b9632" dependencies = [ "core-models", "hax-lib", @@ -1740,16 +1751,18 @@ dependencies = [ [[package]] name = "libcrux-ml-kem" -version = "0.0.3" +version = "0.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5d368d3e8d6a74e277178d54921eca112a1e6b7837d7d8bc555091acb5d817f5" +checksum = "4bb6a88086bf11bd2ec90926c749c4a427f2e59841437dbdede8cde8a96334ab" dependencies = [ "hax-lib", "libcrux-intrinsics", "libcrux-platform", "libcrux-secrets", "libcrux-sha3", + "libcrux-traits", "rand 0.9.2", + "tls_codec", ] [[package]] @@ -1763,22 +1776,33 @@ dependencies = [ [[package]] name = "libcrux-secrets" -version = "0.0.3" +version = "0.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "332737e629fe6ba7547f5c0f90559eac865d5dbecf98138ffae8f16ab8cbe33f" +checksum = "6e4dbbf6bc9f2bc0f20dc3bea3e5c99adff3bdccf6d2a40488963da69e2ec307" dependencies = [ "hax-lib", ] [[package]] name = "libcrux-sha3" -version = "0.0.3" +version = "0.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "29d95de4257eafdfaf3bffecadb615219b0ca920c553722b3646d32dde76c797" +checksum = "2400bec764d1c75b8a496d5747cffe32f1fb864a12577f0aca2f55a92021c962" dependencies = [ "hax-lib", "libcrux-intrinsics", "libcrux-platform", + "libcrux-traits", +] + +[[package]] +name = "libcrux-traits" +version = "0.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9adfd58e79d860f6b9e40e35127bfae9e5bd3ade33201d1347459011a2add034" +dependencies = [ + "libcrux-secrets", + "rand 0.9.2", ] [[package]] @@ -2131,9 +2155,9 @@ dependencies = [ [[package]] name = "pageant" -version = "0.1.0" +version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bb28bd89a207e5cad59072ac4b364b08459d05f90ccfbcdaa920a95857d94430" +checksum = "1b537f975f6d8dcf48db368d7ec209d583b015713b5df0f5d92d2631e4ff5595" dependencies = [ "byteorder", "bytes", @@ -2141,9 +2165,11 @@ dependencies = [ "futures", "log", "rand 0.8.5", + "sha2", "thiserror 1.0.69", "tokio", "windows", + "windows-strings", ] [[package]] @@ -2166,7 +2192,7 @@ dependencies = [ "libc", "redox_syscall", "smallvec", - "windows-link 0.2.1", + "windows-link", ] [[package]] @@ -2563,13 +2589,12 @@ dependencies = [ [[package]] name = "russh" -version = "0.54.6" +version = "0.55.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "23ca8e9091b72afdc9979bddcd1988ad898fa7bc0b85c8da8c154c29d44319eb" +checksum = "82b4d036bb45d7bbe99dbfef4ec60eaeb614708d22ff107124272f8ef6b54548" dependencies = [ "aes", "aws-lc-rs", - "base64ct", "bitflags", "block-padding", "byteorder", @@ -2587,7 +2612,7 @@ dependencies = [ "enum_dispatch", "flate2", "futures", - "generic-array", + "generic-array 1.3.5", "getrandom 0.2.16", "hex-literal", "hmac", @@ -2598,7 +2623,6 @@ dependencies = [ "log", "md5", "num-bigint", - "once_cell", "p256", "p384", "p521", @@ -2797,7 +2821,7 @@ checksum = "d3e97a565f76233a6003f9f5c54be1d9c5bdfa3eccfb189469f11ec4901c47dc" dependencies = [ "base16ct", "der", - "generic-array", + "generic-array 0.14.9", "pkcs8", "subtle", "zeroize", @@ -3225,6 +3249,27 @@ dependencies = [ "crunchy", ] +[[package]] +name = "tls_codec" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0de2e01245e2bb89d6f05801c564fa27624dbd7b1846859876c7dad82e90bf6b" +dependencies = [ + "tls_codec_derive", + "zeroize", +] + +[[package]] +name = "tls_codec_derive" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2d2e76690929402faae40aebdda620a2c0e25dd6d3b9afe48867dfd95991f4bd" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "tokio" version = "1.47.1" @@ -3295,9 +3340,9 @@ dependencies = [ [[package]] name = "tracing" -version = "0.1.41" +version = "0.1.43" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "784e0ac535deb450455cbfa28a6f0df145ea1bb7ae51b821cf5e7927fdcfbdd0" +checksum = "2d15d90a0b5c19378952d479dc858407149d7bb45a14de0142f6c534b16fc647" dependencies = [ "pin-project-lite", "tracing-attributes", @@ -3306,9 +3351,9 @@ dependencies = [ [[package]] name = "tracing-attributes" -version = "0.1.30" +version = "0.1.31" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "81383ab64e72a7a8b8e13130c49e3dab29def6d0c7d76a03087b3cf71c5c6903" +checksum = "7490cfa5ec963746568740651ac6781f701c9c5ea257c58e057f3ba8cf69e8da" dependencies = [ "proc-macro2", "quote", @@ -3317,9 +3362,9 @@ dependencies = [ [[package]] name = "tracing-core" -version = "0.1.34" +version = "0.1.35" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b9d12581f227e93f094d3af2ae690a574abb8a2b9b7a96e7cfe9647b2b617678" +checksum = "7a04e24fab5c89c6a36eb8558c9656f30d81de51dfa4d3b45f26b21d61fa0a6c" dependencies = [ "once_cell", "valuable", @@ -3338,9 +3383,9 @@ dependencies = [ [[package]] name = "tracing-subscriber" -version = "0.3.20" +version = "0.3.22" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2054a14f5307d601f88daf0553e1cbf472acc4f2c51afab632431cdcd72124d5" +checksum = "2f30143827ddab0d256fd843b7a66d164e9f271cfa0dde49142c5ca0ca291f1e" dependencies = [ "matchers", "nu-ansi-term", @@ -3609,25 +3654,23 @@ checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f" [[package]] name = "windows" -version = "0.59.0" +version = "0.62.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7f919aee0a93304be7f62e8e5027811bbba96bcb1de84d6618be56e43f8a32a1" +checksum = "527fadee13e0c05939a6a05d5bd6eec6cd2e3dbd648b9f8e447c6518133d8580" dependencies = [ - "windows-core 0.59.0", - "windows-targets 0.53.5", + "windows-collections", + "windows-core", + "windows-future", + "windows-numerics", ] [[package]] -name = "windows-core" -version = "0.59.0" +name = "windows-collections" +version = "0.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "810ce18ed2112484b0d4e15d022e5f598113e220c53e373fb31e67e21670c1ce" +checksum = "23b2d95af1a8a14a3c7367e1ed4fc9c20e0a26e79551b1454d72583c97cc6610" dependencies = [ - "windows-implement 0.59.0", - "windows-interface", - "windows-result 0.3.4", - "windows-strings 0.3.1", - "windows-targets 0.53.5", + "windows-core", ] [[package]] @@ -3636,22 +3679,22 @@ version = "0.62.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b8e83a14d34d0623b51dce9581199302a221863196a1dde71a7663a4c2be9deb" dependencies = [ - "windows-implement 0.60.2", + "windows-implement", "windows-interface", - "windows-link 0.2.1", - "windows-result 0.4.1", - "windows-strings 0.5.1", + "windows-link", + "windows-result", + "windows-strings", ] [[package]] -name = "windows-implement" -version = "0.59.0" +name = "windows-future" +version = "0.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "83577b051e2f49a058c308f17f273b570a6a758386fc291b5f6a934dd84e48c1" +checksum = "e1d6f90251fe18a279739e78025bd6ddc52a7e22f921070ccdc67dde84c605cb" dependencies = [ - "proc-macro2", - "quote", - "syn", + "windows-core", + "windows-link", + "windows-threading", ] [[package]] @@ -3676,12 +3719,6 @@ dependencies = [ "syn", ] -[[package]] -name = "windows-link" -version = "0.1.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5e6ad25900d524eaabdbbb96d20b4311e1e7ae1699af4fb28c17ae66c80d798a" - [[package]] name = "windows-link" version = "0.2.1" @@ -3689,12 +3726,13 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5" [[package]] -name = "windows-result" -version = "0.3.4" +name = "windows-numerics" +version = "0.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "56f42bd332cc6c8eac5af113fc0c1fd6a8fd2aa08a0119358686e5160d0586c6" +checksum = "6e2e40844ac143cdb44aead537bbf727de9b044e107a0f1220392177d15b0f26" dependencies = [ - "windows-link 0.1.3", + "windows-core", + "windows-link", ] [[package]] @@ -3703,16 +3741,7 @@ version = "0.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7781fa89eaf60850ac3d2da7af8e5242a5ea78d1a11c49bf2910bb5a73853eb5" dependencies = [ - "windows-link 0.2.1", -] - -[[package]] -name = "windows-strings" -version = "0.3.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "87fa48cc5d406560701792be122a10132491cff9d0aeb23583cc2dcafc847319" -dependencies = [ - "windows-link 0.1.3", + "windows-link", ] [[package]] @@ -3721,7 +3750,7 @@ version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7837d08f69c77cf6b07689544538e017c1bfcf57e34b4c0ff58e6c2cd3b37091" dependencies = [ - "windows-link 0.2.1", + "windows-link", ] [[package]] @@ -3757,7 +3786,7 @@ version = "0.61.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ae137229bcbd6cdf0f7b80a31df61766145077ddf49416a728b02cb3921ff3fc" dependencies = [ - "windows-link 0.2.1", + "windows-link", ] [[package]] @@ -3782,7 +3811,7 @@ version = "0.53.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4945f9f551b88e0d65f3db0bc25c33b8acea4d9e41163edf90dcd0b19f9069f3" dependencies = [ - "windows-link 0.2.1", + "windows-link", "windows_aarch64_gnullvm 0.53.1", "windows_aarch64_msvc 0.53.1", "windows_i686_gnu 0.53.1", @@ -3793,6 +3822,15 @@ dependencies = [ "windows_x86_64_msvc 0.53.1", ] +[[package]] +name = "windows-threading" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3949bd5b99cafdf1c7ca86b43ca564028dfe27d66958f2470940f73d86d75b37" +dependencies = [ + "windows-link", +] + [[package]] name = "windows_aarch64_gnullvm" version = "0.52.6" @@ -3920,3 +3958,17 @@ name = "zeroize" version = "1.8.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b97154e67e32c85465826e8bcc1c59429aaaf107c1e4a9e53c8d8ccd5eff88d0" +dependencies = [ + "zeroize_derive", +] + +[[package]] +name = "zeroize_derive" +version = "1.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ce36e65b0d2999d2aafac989fb249189a141aee1f53c612c1f37d72631959f69" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] diff --git a/Cargo.toml b/Cargo.toml index 9cf1e278..102d1ef6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,13 +12,13 @@ edition = "2021" [dependencies] tokio = { version = "1.47.1", features = ["full"] } -russh = "0.54.6" +russh = "0.55.0" russh-sftp = "2.1.1" clap = { version = "4", features = ["derive", "env"] } anyhow = "1.0.99" thiserror = "2.0.16" tracing = "0.1.41" -tracing-subscriber = { version = "0.3", features = ["env-filter"] } +tracing-subscriber = { version = "0.3.22", features = ["env-filter"] } serde = { version = "1.0.219", features = ["derive"] } serde_yaml = "0.9" futures = "0.3.31" @@ -38,7 +38,7 @@ zeroize = "1.8" rustyline = "17.0.1" crossterm = "0.29" ratatui = "0.29" -regex = "1" +regex = "1.12.2" lazy_static = "1.5" ctrlc = "3.4" signal-hook = "0.3.18" diff --git a/src/pty/session/escape_filter.rs b/src/pty/session/escape_filter.rs index 6463f431..ed8c6ce5 100644 --- a/src/pty/session/escape_filter.rs +++ b/src/pty/session/escape_filter.rs @@ -377,9 +377,13 @@ impl EscapeSequenceFilter { // Return to appropriate DCS state based on content if self.is_dcs_response() { // Stay in a response state (Xtgettcap or DcsDecrqss) - if self.pending_buffer.len() > 3 && self.pending_buffer[2] == b'+' { + if self.pending_buffer.len() > 3 + && self.pending_buffer[2] == b'+' + { self.state = FilterState::Xtgettcap; - } else if self.pending_buffer.len() > 2 && self.pending_buffer[2] == b'$' { + } else if self.pending_buffer.len() > 2 + && self.pending_buffer[2] == b'$' + { self.state = FilterState::DcsDecrqss; } else { self.state = FilterState::DcsPassthrough; @@ -633,7 +637,7 @@ mod tests { // Create a malformed CSI sequence that exceeds MAX_CSI_SEQUENCE_SIZE (256 bytes) // without a proper terminator (no alphabetic character or ~) let mut malformed = vec![0x1b, b'[']; // ESC [ - // Add enough non-terminating bytes to exceed the limit + // Add enough non-terminating bytes to exceed the limit for _ in 0..300 { malformed.push(b';'); // Keep adding parameter separators } @@ -643,7 +647,10 @@ mod tests { // The malformed sequence should be flushed (not filtered) // because it exceeded the size limit before getting a terminator - assert!(!output.is_empty(), "Malformed CSI sequence should be flushed to output"); + assert!( + !output.is_empty(), + "Malformed CSI sequence should be flushed to output" + ); } #[test] @@ -652,7 +659,7 @@ mod tests { // Create a DCS sequence that exceeds MAX_PENDING_SIZE (4096 bytes) // DCS sequences don't have the early termination, only the global limit applies let mut large_dcs = vec![0x1b, b'P']; // ESC P (DCS start) - // Add enough bytes to exceed the 4096 byte limit + // Add enough bytes to exceed the 4096 byte limit for i in 0..5000 { large_dcs.push(b'A' + (i % 26) as u8); } @@ -660,10 +667,16 @@ mod tests { let output = filter.filter(&large_dcs); // The buffer should be flushed when it exceeds MAX_PENDING_SIZE - assert!(!output.is_empty(), "Buffer overflow should flush content to output"); + assert!( + !output.is_empty(), + "Buffer overflow should flush content to output" + ); // State should be reset to Normal assert_eq!(filter.state, FilterState::Normal); - assert!(filter.pending_buffer.is_empty(), "Pending buffer should be cleared"); + assert!( + filter.pending_buffer.is_empty(), + "Pending buffer should be cleared" + ); } #[test] @@ -671,7 +684,7 @@ mod tests { let mut filter = EscapeSequenceFilter::new(); // Create a malformed CSI ? sequence that exceeds MAX_CSI_SEQUENCE_SIZE let mut malformed = vec![0x1b, b'[', b'?']; // ESC [ ? - // Add enough non-terminating bytes + // Add enough non-terminating bytes for _ in 0..300 { malformed.push(b'0'); // Keep adding digits } @@ -680,7 +693,10 @@ mod tests { let output = filter.filter(&malformed); // The malformed sequence should be flushed (not filtered) - assert!(!output.is_empty(), "Malformed CSI? sequence should be flushed to output"); + assert!( + !output.is_empty(), + "Malformed CSI? sequence should be flushed to output" + ); } #[test] diff --git a/src/pty/session/session_manager.rs b/src/pty/session/session_manager.rs index 3996cfbe..93413fe2 100644 --- a/src/pty/session/session_manager.rs +++ b/src/pty/session/session_manager.rs @@ -106,12 +106,11 @@ impl PtySession { // Also set COLORTERM for better color support detection // Many modern terminals set this to indicate truecolor support - if let Err(e) = self - .channel - .set_env(false, "COLORTERM", "truecolor") - .await - { - tracing::trace!("Server did not accept COLORTERM environment variable: {}", e); + if let Err(e) = self.channel.set_env(false, "COLORTERM", "truecolor").await { + tracing::trace!( + "Server did not accept COLORTERM environment variable: {}", + e + ); } // Request PTY on the SSH channel with properly configured terminal modes From 2418cd3d7a9f651240d95950b0b67ba71d79a92d Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Wed, 10 Dec 2025 17:42:38 +0900 Subject: [PATCH 07/10] fix: Pass through DA responses for terminal capability detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove unconditional filtering of Device Attributes (DA) responses - DA responses (\x1b[?...c) are needed by ncurses/htop for terminal capability detection and cursor key mode (DECCKM) configuration - Filtering DA responses prevented htop from detecting terminal type, causing arrow keys to be misinterpreted - The original Neovim issue (DA responses showing on screen) was likely caused by specific PTY configurations and should be handled there - Update tests: test_da_response_filtered → test_da_response_passthrough --- src/pty/session/escape_filter.rs | 45 ++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/src/pty/session/escape_filter.rs b/src/pty/session/escape_filter.rs index ed8c6ce5..301c760e 100644 --- a/src/pty/session/escape_filter.rs +++ b/src/pty/session/escape_filter.rs @@ -201,16 +201,15 @@ impl EscapeSequenceFilter { FilterState::CsiQuestion => { self.pending_buffer.push(byte); - if byte == b'c' { - // This is a DA (Device Attributes) response: ESC [ ? ... c - // Filter it out - don't add to output - tracing::trace!( - "Filtered DA response: {:?}", - String::from_utf8_lossy(&self.pending_buffer) - ); - self.reset_to_normal(); - } else if byte.is_ascii_alphabetic() || byte == b'~' { - // End of CSI ? sequence - pass through (DEC private modes) + if byte.is_ascii_alphabetic() || byte == b'~' { + // End of CSI ? sequence + // DA responses (\x1b[?...c) should pass through to applications + // as they need this for terminal capability detection. + // Filtering DA responses breaks applications like htop that + // rely on ncurses terminal detection. + // The original issue (DA responses showing on screen) was likely + // caused by specific terminal/PTY configurations and should be + // handled at that level, not by filtering. output.extend_from_slice(&self.pending_buffer); self.reset_to_normal(); } else if self.pending_buffer.len() > MAX_CSI_SEQUENCE_SIZE { @@ -536,12 +535,18 @@ mod tests { } #[test] - fn test_da_response_filtered() { + fn test_da_response_passthrough() { let mut filter = EscapeSequenceFilter::new(); // DA1 response: ESC [ ? 6 4 ; 4 c + // DA responses should pass through as applications like htop need them + // for terminal capability detection let input = b"\x1b[?64;4c"; let output = filter.filter(input); - assert!(output.is_empty(), "DA response should be filtered"); + assert_eq!( + output, + input.to_vec(), + "DA response should pass through for terminal detection" + ); } #[test] @@ -568,12 +573,12 @@ mod tests { #[test] fn test_mixed_content() { let mut filter = EscapeSequenceFilter::new(); - // Mix of normal text, color codes, and filtered responses + // Mix of normal text, color codes, and DA responses + // DA responses now pass through for terminal detection let input = b"Hello\x1b[31m\x1b[?64;4cWorld\x1b[0m"; let output = filter.filter(input); - // Should keep: Hello, color start, World, color reset - // Should filter: DA response - assert_eq!(output, b"Hello\x1b[31mWorld\x1b[0m"); + // Should keep: Hello, color start, DA response, World, color reset + assert_eq!(output, b"Hello\x1b[31m\x1b[?64;4cWorld\x1b[0m"); } #[test] @@ -607,9 +612,11 @@ mod tests { assert!(output1.is_empty(), "Partial sequence should be buffered"); let output2 = filter.filter(part2); - assert!( - output2.is_empty(), - "Complete DA response should be filtered" + // DA responses now pass through for terminal detection + assert_eq!( + output2, + b"\x1b[?64;4c".to_vec(), + "Complete DA response should pass through" ); } From 25115da1334cb309ffc5b3297ed91839dda7053d Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Wed, 10 Dec 2025 17:55:11 +0900 Subject: [PATCH 08/10] fix: Use Application Cursor Keys (SS3) format for arrow keys - Change arrow key sequences from CSI format (\x1b[A) to SS3 format (\x1bOA) - ncurses applications (htop) expect SS3 format when DECCKM is enabled - vim/neovim and most terminals accept both formats, but ncurses strictly follows terminfo which typically specifies SS3 format for cursor keys - This fixes arrow key navigation in htop while maintaining compatibility with other applications --- src/pty/session/constants.rs | 14 +++++++++----- src/pty/session/escape_filter.rs | 13 +++++++++++++ 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/pty/session/constants.rs b/src/pty/session/constants.rs index e166ef98..e1a7e6d1 100644 --- a/src/pty/session/constants.rs +++ b/src/pty/session/constants.rs @@ -76,11 +76,15 @@ pub const TAB_SEQUENCE: &[u8] = &[0x09]; // Tab pub const BACKSPACE_SEQUENCE: &[u8] = &[0x7f]; // DEL pub const ESC_SEQUENCE: &[u8] = &[0x1b]; // ESC -/// Arrow keys - ANSI escape sequences -pub const UP_ARROW_SEQUENCE: &[u8] = &[0x1b, 0x5b, 0x41]; // ESC[A -pub const DOWN_ARROW_SEQUENCE: &[u8] = &[0x1b, 0x5b, 0x42]; // ESC[B -pub const RIGHT_ARROW_SEQUENCE: &[u8] = &[0x1b, 0x5b, 0x43]; // ESC[C -pub const LEFT_ARROW_SEQUENCE: &[u8] = &[0x1b, 0x5b, 0x44]; // ESC[D +/// Arrow keys - Application Cursor Keys mode (SS3 sequences) +/// ncurses applications (htop, etc.) typically expect this format when +/// DECCKM (DEC Cursor Key Mode) is enabled via \x1b[?1h +/// Most terminal emulators and applications (vim, neovim) accept both formats, +/// but ncurses strictly follows terminfo which often specifies SS3 format. +pub const UP_ARROW_SEQUENCE: &[u8] = &[0x1b, 0x4f, 0x41]; // ESC O A +pub const DOWN_ARROW_SEQUENCE: &[u8] = &[0x1b, 0x4f, 0x42]; // ESC O B +pub const RIGHT_ARROW_SEQUENCE: &[u8] = &[0x1b, 0x4f, 0x43]; // ESC O C +pub const LEFT_ARROW_SEQUENCE: &[u8] = &[0x1b, 0x4f, 0x44]; // ESC O D /// Function keys - commonly used pub const F1_SEQUENCE: &[u8] = &[0x1b, 0x4f, 0x50]; // F1: ESC OP diff --git a/src/pty/session/escape_filter.rs b/src/pty/session/escape_filter.rs index 301c760e..831b8dd6 100644 --- a/src/pty/session/escape_filter.rs +++ b/src/pty/session/escape_filter.rs @@ -117,6 +117,19 @@ impl EscapeSequenceFilter { /// Filter terminal data, removing terminal query responses. /// /// Returns the filtered data that should be displayed. + /// + /// ## Filtering Strategy (Conservative) + /// + /// The filter only removes sequences that are **known terminal responses** + /// that should never appear on screen: + /// - XTGETTCAP responses (DCS +r) + /// - DECRQSS responses (DCS $) + /// - OSC color query responses (OSC 4, 10-19, 52) + /// + /// All other sequences pass through, including: + /// - DA responses (needed for terminal capability detection) + /// - All CSI sequences (colors, cursor movement, DEC private modes) + /// - All SS3 sequences (application mode cursor keys, function keys) pub fn filter(&mut self, data: &[u8]) -> Vec { let mut output = Vec::with_capacity(data.len()); From fac88f3886d384d431aebabf961c6d3764e0fc44 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Wed, 10 Dec 2025 18:02:02 +0900 Subject: [PATCH 09/10] fix: Add serial test attribute to env-dependent auth tests - Add #[serial] to tests that modify SSH_AUTH_SOCK and HOME env vars - Properly save/restore original env vars in test_determine_auth_method_with_agent - Prevents race conditions when tests run in parallel in CI --- src/ssh/client/connection.rs | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/src/ssh/client/connection.rs b/src/ssh/client/connection.rs index 81ff0891..3d00d0a0 100644 --- a/src/ssh/client/connection.rs +++ b/src/ssh/client/connection.rs @@ -230,6 +230,7 @@ impl SshClient { #[cfg(test)] mod tests { use super::*; + use serial_test::serial; use tempfile::TempDir; #[tokio::test] @@ -261,9 +262,13 @@ mod tests { #[cfg(target_os = "macos")] #[tokio::test] + #[serial] async fn test_determine_auth_method_with_agent() { use std::os::unix::net::UnixListener; + // Save original SSH_AUTH_SOCK + let original_ssh_auth_sock = std::env::var("SSH_AUTH_SOCK").ok(); + // Create a temporary directory for the socket let temp_dir = TempDir::new().unwrap(); let socket_path = temp_dir.path().join("ssh-agent.sock"); @@ -285,19 +290,28 @@ mod tests { .await .unwrap(); + // Restore original SSH_AUTH_SOCK + if let Some(sock) = original_ssh_auth_sock { + std::env::set_var("SSH_AUTH_SOCK", sock); + } else { + std::env::remove_var("SSH_AUTH_SOCK"); + } + match auth { AuthMethod::Agent => {} _ => panic!("Expected Agent auth method"), } - - std::env::remove_var("SSH_AUTH_SOCK"); } #[cfg(target_os = "linux")] #[tokio::test] + #[serial] async fn test_determine_auth_method_with_agent() { use std::os::unix::net::UnixListener; + // Save original SSH_AUTH_SOCK + let original_ssh_auth_sock = std::env::var("SSH_AUTH_SOCK").ok(); + // Create a temporary directory for the socket let temp_dir = TempDir::new().unwrap(); let socket_path = temp_dir.path().join("ssh-agent.sock"); @@ -313,12 +327,17 @@ mod tests { .await .unwrap(); + // Restore original SSH_AUTH_SOCK + if let Some(sock) = original_ssh_auth_sock { + std::env::set_var("SSH_AUTH_SOCK", sock); + } else { + std::env::remove_var("SSH_AUTH_SOCK"); + } + match auth { AuthMethod::Agent => {} _ => panic!("Expected Agent auth method"), } - - std::env::remove_var("SSH_AUTH_SOCK"); } #[test] @@ -331,6 +350,7 @@ mod tests { } #[tokio::test] + #[serial] async fn test_determine_auth_method_fallback_to_default() { // Save original environment variables let original_home = std::env::var("HOME").ok(); From b4348bb6dd9a731414137b2089b980ab03fe9582 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Wed, 10 Dec 2025 18:09:56 +0900 Subject: [PATCH 10/10] chore: Update num-bigint-dig to 0.8.6 to fix future incompatibility warning - Update num-bigint-dig from 0.8.4 to 0.8.6 via cargo update - Fixes Rust future incompatibility warning about private `vec` macro - See: https://github.com/rust-lang/rust/issues/120192 --- Cargo.lock | 5 ++--- Cargo.toml | 1 + 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a8cf974c..4dcde934 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2021,11 +2021,10 @@ dependencies = [ [[package]] name = "num-bigint-dig" -version = "0.8.4" +version = "0.8.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dc84195820f291c7697304f3cbdadd1cb7199c0efc917ff5eafd71225c136151" +checksum = "e661dda6640fad38e827a6d4a310ff4763082116fe217f279885c97f511bb0b7" dependencies = [ - "byteorder", "lazy_static", "libm", "num-integer", diff --git a/Cargo.toml b/Cargo.toml index 102d1ef6..9302edb6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -61,3 +61,4 @@ mockito = "1" once_cell = "1.21.3" tokio-test = "0.4" serial_test = "3.2" +