From 7c0cbe8b559fba66eb233b4e6717421968424608 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Tue, 14 Oct 2025 23:42:56 +0900 Subject: [PATCH 1/2] update: terminal mode implementation --- src/commands/download.rs | 13 ++- src/commands/ping.rs | 2 +- src/commands/upload.rs | 22 ++--- src/executor.rs | 30 +++---- src/forwarding/dynamic.rs | 18 ++--- src/forwarding/manager.rs | 13 ++- src/forwarding/mod.rs | 3 +- src/forwarding/spec.rs | 9 +-- src/forwarding/tunnel.rs | 6 +- src/jump/chain.rs | 18 ++--- src/jump/parser.rs | 9 +-- src/jump/rate_limiter.rs | 4 +- src/main.rs | 22 ++--- src/pty/session.rs | 64 +++++++++++++-- src/security.rs | 14 ++-- src/ssh/client.rs | 103 ++++++++++-------------- src/ssh/config_cache.rs | 37 ++++----- src/ssh/ssh_config/parser.rs | 137 ++++++++++++-------------------- src/ssh/ssh_config/path.rs | 31 +++----- src/ssh/ssh_config/security.rs | 100 ++++++++--------------- src/ssh/tokio_client/client.rs | 46 +++++------ src/utils/fs.rs | 5 +- src/utils/sanitize.rs | 8 +- tests/glob_pattern_test.rs | 2 +- tests/jump_host_timeout_test.rs | 3 +- 25 files changed, 314 insertions(+), 405 deletions(-) diff --git a/src/commands/download.rs b/src/commands/download.rs index 6546635e..ac78f024 100644 --- a/src/commands/download.rs +++ b/src/commands/download.rs @@ -30,11 +30,11 @@ pub async fn download_file( ) -> Result<()> { // Security: Validate the remote source path let validated_source = crate::security::validate_remote_path(source) - .with_context(|| format!("Invalid source path: {}", source))?; + .with_context(|| format!("Invalid source path: {source}"))?; // Security: Validate the local destination path let validated_destination = crate::security::validate_local_path(destination) - .with_context(|| format!("Invalid destination path: {:?}", destination))?; + .with_context(|| format!("Invalid destination path: {destination:?}"))?; // Create destination directory if it doesn't exist if !validated_destination.exists() { @@ -66,10 +66,7 @@ pub async fn download_file( // Check if source is a directory (for recursive download) let is_directory = if params.recursive && !has_glob { // Use a test command to check if source is a directory - let test_cmd = format!( - "test -d '{}' && echo 'dir' || echo 'file'", - validated_source - ); + let test_cmd = format!("test -d '{validated_source}' && echo 'dir' || echo 'file'"); let test_results = executor.execute(&test_cmd).await?; test_results.iter().any(|r| { r.result @@ -166,7 +163,7 @@ pub async fn download_file( .nodes .first() .ok_or_else(|| anyhow::anyhow!("No nodes available"))?; - let glob_command = format!("ls -1 {} 2>/dev/null || true", validated_source); + let glob_command = format!("ls -1 {validated_source} 2>/dev/null || true"); let mut test_client = SshClient::new( test_node.host.clone(), @@ -192,7 +189,7 @@ pub async fn download_file( .collect(); if remote_files.is_empty() { - anyhow::bail!("No files found matching pattern: {}", validated_source); + anyhow::bail!("No files found matching pattern: {validated_source}"); } println!( diff --git a/src/commands/ping.rs b/src/commands/ping.rs index dd824e7d..277a4749 100644 --- a/src/commands/ping.rs +++ b/src/commands/ping.rs @@ -70,7 +70,7 @@ pub async fn ping_nodes( ); if let Err(e) = &result.result { // Display the full error chain for better debugging - let error_chain = format!("{:#}", e); + let error_chain = format!("{e:#}"); // Split by newlines and indent each line for (i, line) in error_chain.lines().enumerate() { if i == 0 { diff --git a/src/commands/upload.rs b/src/commands/upload.rs index 02777586..77ced743 100644 --- a/src/commands/upload.rs +++ b/src/commands/upload.rs @@ -39,17 +39,17 @@ pub async fn upload_file( ) -> Result<()> { // Security: Validate the local source path let validated_source = crate::security::validate_local_path(source) - .with_context(|| format!("Invalid source path: {:?}", source))?; + .with_context(|| format!("Invalid source path: {source:?}"))?; // Security: Validate the remote destination path let validated_destination = crate::security::validate_remote_path(destination) - .with_context(|| format!("Invalid destination path: {}", destination))?; + .with_context(|| format!("Invalid destination path: {destination}"))?; // Collect all files matching the pattern let files = resolve_source_files(&validated_source, params.recursive)?; if files.is_empty() { - anyhow::bail!("No files found matching pattern: {:?}", source); + anyhow::bail!("No files found matching pattern: {source:?}"); } // Determine destination handling based on file count @@ -123,32 +123,32 @@ pub async fn upload_file( } if validated_destination.ends_with('/') { - format!("{}{remote_relative}", validated_destination) + format!("{validated_destination}{remote_relative}") } else { - format!("{}/{remote_relative}", validated_destination) + format!("{validated_destination}/{remote_relative}") } } else { // No base dir, just use filename let filename = file .file_name() - .ok_or_else(|| anyhow::anyhow!("Failed to get filename from {:?}", file))? + .ok_or_else(|| anyhow::anyhow!("Failed to get filename from {file:?}"))? .to_string_lossy(); if validated_destination.ends_with('/') { - format!("{}{filename}", validated_destination) + format!("{validated_destination}{filename}") } else { - format!("{}/{filename}", validated_destination) + format!("{validated_destination}/{filename}") } } } else { // Non-recursive: just append filename let filename = file .file_name() - .ok_or_else(|| anyhow::anyhow!("Failed to get filename from {:?}", file))? + .ok_or_else(|| anyhow::anyhow!("Failed to get filename from {file:?}"))? .to_string_lossy(); if validated_destination.ends_with('/') { - format!("{}{filename}", validated_destination) + format!("{validated_destination}{filename}") } else { - format!("{}/{filename}", validated_destination) + format!("{validated_destination}/{filename}") } } } else { diff --git a/src/executor.rs b/src/executor.rs index 3c111317..577cf9c7 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -133,7 +133,7 @@ impl ParallelExecutor { let style = ProgressStyle::default_bar() .template("{prefix:.bold} {spinner:.cyan} {msg}") - .map_err(|e| anyhow::anyhow!("Failed to create progress bar template: {}", e))? + .map_err(|e| anyhow::anyhow!("Failed to create progress bar template: {e}"))? .tick_chars("⣾⣽⣻⢿⡿⣟⣯⣷ "); let tasks: Vec<_> = self @@ -176,7 +176,7 @@ impl ParallelExecutor { )); return ExecutionResult { node, - result: Err(anyhow::anyhow!("Semaphore acquisition failed: {}", e)), + result: Err(anyhow::anyhow!("Semaphore acquisition failed: {e}")), }; } }; @@ -213,7 +213,7 @@ impl ParallelExecutor { } Err(e) => { // Get the most specific error message from the chain - let error_msg = format!("{:#}", e); + let error_msg = format!("{e:#}"); // Take the first line which is usually the most specific error let first_line = error_msg.lines().next().unwrap_or("Unknown error"); let short_error = if first_line.len() > 50 { @@ -256,9 +256,7 @@ impl ParallelExecutor { let style = ProgressStyle::default_bar() .template("{prefix:.bold} {spinner:.cyan} {msg}") - .map_err(|e| { - anyhow::anyhow!("Failed to create progress bar template for upload: {}", e) - })? + .map_err(|e| anyhow::anyhow!("Failed to create progress bar template for upload: {e}"))? .tick_chars("⣾⣽⣻⢿⡿⣟⣯⣷ "); let tasks: Vec<_> = self @@ -301,7 +299,7 @@ impl ParallelExecutor { )); return UploadResult { node, - result: Err(anyhow::anyhow!("Semaphore acquisition failed: {}", e)), + result: Err(anyhow::anyhow!("Semaphore acquisition failed: {e}")), }; } }; @@ -330,7 +328,7 @@ impl ParallelExecutor { } Err(e) => { // Get the most specific error message from the chain - let error_msg = format!("{:#}", e); + let error_msg = format!("{e:#}"); // Take the first line which is usually the most specific error let first_line = error_msg.lines().next().unwrap_or("Unknown error"); let short_error = if first_line.len() > 50 { @@ -374,7 +372,7 @@ impl ParallelExecutor { let style = ProgressStyle::default_bar() .template("{prefix:.bold} {spinner:.cyan} {msg}") .map_err(|e| { - anyhow::anyhow!("Failed to create progress bar template for download: {}", e) + anyhow::anyhow!("Failed to create progress bar template for download: {e}") })? .tick_chars("⣾⣽⣻⢿⡿⣟⣯⣷ "); @@ -418,7 +416,7 @@ impl ParallelExecutor { )); return DownloadResult { node, - result: Err(anyhow::anyhow!("Semaphore acquisition failed: {}", e)), + result: Err(anyhow::anyhow!("Semaphore acquisition failed: {e}")), }; } }; @@ -493,10 +491,7 @@ impl ParallelExecutor { let style = ProgressStyle::default_bar() .template("{prefix:.bold} {spinner:.cyan} {msg}") .map_err(|e| { - anyhow::anyhow!( - "Failed to create progress bar template for multi-download: {}", - e - ) + anyhow::anyhow!("Failed to create progress bar template for multi-download: {e}") })? .tick_chars("⣾⣽⣻⢿⡿⣟⣯⣷ "); @@ -541,8 +536,7 @@ impl ParallelExecutor { return DownloadResult { node, result: Err(anyhow::anyhow!( - "Semaphore acquisition failed: {}", - e + "Semaphore acquisition failed: {e}" )), }; } @@ -780,7 +774,7 @@ impl UploadResult { "Failed to upload file".red() ); // Show full error chain - let error_chain = format!("{:#}", e); + let error_chain = format!("{e:#}"); for line in error_chain.lines() { println!(" {}", line.dimmed()); } @@ -819,7 +813,7 @@ impl DownloadResult { "Failed to download file".red() ); // Show full error chain - let error_chain = format!("{:#}", e); + let error_chain = format!("{e:#}"); for line in error_chain.lines() { println!(" {}", line.dimmed()); } diff --git a/src/forwarding/dynamic.rs b/src/forwarding/dynamic.rs index 2f28f205..c26f9f8d 100644 --- a/src/forwarding/dynamic.rs +++ b/src/forwarding/dynamic.rs @@ -455,7 +455,7 @@ impl DynamicForwarder { // Send failure response let response = [0, 0x5B, 0, 0, 0, 0, 0, 0]; // 0x5B = request rejected tcp_stream.write_all(&response).await?; - return Err(anyhow::anyhow!("Invalid SOCKS4 version: {}", version)); + return Err(anyhow::anyhow!("Invalid SOCKS4 version: {version}")); } // Only support CONNECT command (0x01) @@ -463,7 +463,7 @@ impl DynamicForwarder { debug!("Unsupported SOCKS4 command: {} from {}", command, peer_addr); let response = [0, 0x5C, 0, 0, 0, 0, 0, 0]; // 0x5C = request failed tcp_stream.write_all(&response).await?; - return Err(anyhow::anyhow!("Unsupported SOCKS4 command: {}", command)); + return Err(anyhow::anyhow!("Unsupported SOCKS4 command: {command}")); } // Read USERID (until NULL byte) @@ -541,7 +541,7 @@ impl DynamicForwarder { let nmethods = auth_request[1]; if version != 5 { - return Err(anyhow::anyhow!("Invalid SOCKS5 version: {}", version)); + return Err(anyhow::anyhow!("Invalid SOCKS5 version: {version}")); } // Read authentication methods @@ -574,10 +574,7 @@ impl DynamicForwarder { let address_type = request_header[3]; if version != 5 { - return Err(anyhow::anyhow!( - "Invalid SOCKS5 request version: {}", - version - )); + return Err(anyhow::anyhow!("Invalid SOCKS5 request version: {version}")); } // Only support CONNECT command (0x01) @@ -585,7 +582,7 @@ impl DynamicForwarder { // Send error response let response = [5, 0x07, 0, 1, 0, 0, 0, 0, 0, 0]; // Command not supported tcp_stream.write_all(&response).await?; - return Err(anyhow::anyhow!("Unsupported SOCKS5 command: {}", command)); + return Err(anyhow::anyhow!("Unsupported SOCKS5 command: {command}")); } // Parse destination address based on address type @@ -626,10 +623,7 @@ impl DynamicForwarder { _ => { let response = [5, 0x08, 0, 1, 0, 0, 0, 0, 0, 0]; // Address type not supported tcp_stream.write_all(&response).await?; - return Err(anyhow::anyhow!( - "Unsupported address type: {}", - address_type - )); + return Err(anyhow::anyhow!("Unsupported address type: {address_type}")); } }; diff --git a/src/forwarding/manager.rs b/src/forwarding/manager.rs index 0bfcd551..58ce82e6 100644 --- a/src/forwarding/manager.rs +++ b/src/forwarding/manager.rs @@ -270,14 +270,13 @@ impl ForwardingManager { let sessions = self.sessions.read().await; let session_arc = sessions .get(&id) - .ok_or_else(|| anyhow::anyhow!("Forwarding session {} not found", id))?; + .ok_or_else(|| anyhow::anyhow!("Forwarding session {id} not found"))?; let mut session = session_arc.lock().await; if session.task_handle.is_some() { return Err(anyhow::anyhow!( - "Forwarding session {} is already started", - id + "Forwarding session {id} is already started" )); } @@ -353,7 +352,7 @@ impl ForwardingManager { let sessions = self.sessions.read().await; let session_arc = sessions .get(&id) - .ok_or_else(|| anyhow::anyhow!("Forwarding session {} not found", id))?; + .ok_or_else(|| anyhow::anyhow!("Forwarding session {id} not found"))?; let mut session = session_arc.lock().await; @@ -392,7 +391,7 @@ impl ForwardingManager { let sessions = self.sessions.read().await; let session_arc = sessions .get(&id) - .ok_or_else(|| anyhow::anyhow!("Forwarding session {} not found", id))?; + .ok_or_else(|| anyhow::anyhow!("Forwarding session {id} not found"))?; let session = session_arc.lock().await; Ok(session.status.clone()) @@ -403,7 +402,7 @@ impl ForwardingManager { let sessions = self.sessions.read().await; let session_arc = sessions .get(&id) - .ok_or_else(|| anyhow::anyhow!("Forwarding session {} not found", id))?; + .ok_or_else(|| anyhow::anyhow!("Forwarding session {id} not found"))?; let session = session_arc.lock().await; Ok(session.stats.clone()) @@ -431,7 +430,7 @@ impl ForwardingManager { let mut sessions = self.sessions.write().await; sessions .remove(&id) - .ok_or_else(|| anyhow::anyhow!("Forwarding session {} not found", id))?; + .ok_or_else(|| anyhow::anyhow!("Forwarding session {id} not found"))?; tracing::info!("Removed forwarding session {}", id); Ok(()) diff --git a/src/forwarding/mod.rs b/src/forwarding/mod.rs index f96a59f6..785d9def 100644 --- a/src/forwarding/mod.rs +++ b/src/forwarding/mod.rs @@ -183,8 +183,7 @@ impl SocksVersion { "4" | "v4" | "socks4" => Ok(SocksVersion::V4), "5" | "v5" | "socks5" => Ok(SocksVersion::V5), _ => Err(anyhow::anyhow!( - "Invalid SOCKS version: {}. Expected 4 or 5", - s + "Invalid SOCKS version: {s}. Expected 4 or 5" )), } } diff --git a/src/forwarding/spec.rs b/src/forwarding/spec.rs index ffb03bf4..2ded9655 100644 --- a/src/forwarding/spec.rs +++ b/src/forwarding/spec.rs @@ -72,8 +72,7 @@ impl ForwardingSpec { }) } _ => Err(anyhow::anyhow!( - "Invalid local forwarding specification: '{}'. Expected format: [bind_address:]port:host:hostport", - spec + "Invalid local forwarding specification: '{spec}'. Expected format: [bind_address:]port:host:hostport" )), } } @@ -120,8 +119,7 @@ impl ForwardingSpec { }) } _ => Err(anyhow::anyhow!( - "Invalid remote forwarding specification: '{}'. Expected format: [bind_address:]port:host:hostport", - spec + "Invalid remote forwarding specification: '{spec}'. Expected format: [bind_address:]port:host:hostport" )), } } @@ -163,8 +161,7 @@ impl ForwardingSpec { "remote" | "r" | "-r" => Self::parse_remote(spec), "dynamic" | "d" | "-d" => Self::parse_dynamic(spec), _ => Err(anyhow::anyhow!( - "Unknown forwarding type: '{}'. Expected: local, remote, or dynamic", - forward_type + "Unknown forwarding type: '{forward_type}'. Expected: local, remote, or dynamic" )), } } diff --git a/src/forwarding/tunnel.rs b/src/forwarding/tunnel.rs index 6c8dc827..4a519ee3 100644 --- a/src/forwarding/tunnel.rs +++ b/src/forwarding/tunnel.rs @@ -137,7 +137,7 @@ impl Tunnel { Err(e) => { stats.error_count.fetch_add(1, Ordering::Relaxed); error!("Failed to write to SSH channel: {}", e); - return Err(anyhow::anyhow!("SSH channel write error: {}", e)); + return Err(anyhow::anyhow!("SSH channel write error: {e}")); } } } @@ -149,7 +149,7 @@ impl Tunnel { break; } else { error!("TCP read error: {}", e); - return Err(anyhow::anyhow!("TCP read error: {}", e)); + return Err(anyhow::anyhow!("TCP read error: {e}")); } } } @@ -172,7 +172,7 @@ impl Tunnel { break; } else { error!("TCP write error: {}", e); - return Err(anyhow::anyhow!("TCP write error: {}", e)); + return Err(anyhow::anyhow!("TCP write error: {e}")); } } } diff --git a/src/jump/chain.rs b/src/jump/chain.rs index 43e6ace2..9a677db0 100644 --- a/src/jump/chain.rs +++ b/src/jump/chain.rs @@ -899,7 +899,7 @@ impl JumpHostChain { let auth_result = handle .authenticate_password(username, &**password) .await - .map_err(|e| anyhow::anyhow!("Password authentication failed: {}", e))?; + .map_err(|e| anyhow::anyhow!("Password authentication failed: {e}"))?; if !auth_result.success() { anyhow::bail!("Password authentication rejected by jump host"); @@ -909,7 +909,7 @@ impl JumpHostChain { AuthMethod::PrivateKey { key_data, key_pass } => { let private_key = russh::keys::decode_secret_key(&key_data, key_pass.as_ref().map(|p| &***p)) - .map_err(|e| anyhow::anyhow!("Failed to decode private key: {}", e))?; + .map_err(|e| anyhow::anyhow!("Failed to decode private key: {e}"))?; let auth_result = handle .authenticate_publickey( @@ -920,7 +920,7 @@ impl JumpHostChain { ), ) .await - .map_err(|e| anyhow::anyhow!("Private key authentication failed: {}", e))?; + .map_err(|e| anyhow::anyhow!("Private key authentication failed: {e}"))?; if !auth_result.success() { anyhow::bail!("Private key authentication rejected by jump host"); @@ -931,11 +931,9 @@ impl JumpHostChain { key_file_path, key_pass, } => { - let private_key = russh::keys::load_secret_key( - key_file_path, - key_pass.as_ref().map(|p| &***p), - ) - .map_err(|e| anyhow::anyhow!("Failed to load private key from file: {}", e))?; + let private_key = + russh::keys::load_secret_key(key_file_path, key_pass.as_ref().map(|p| &***p)) + .map_err(|e| anyhow::anyhow!("Failed to load private key from file: {e}"))?; let auth_result = handle .authenticate_publickey( @@ -946,9 +944,7 @@ impl JumpHostChain { ), ) .await - .map_err(|e| { - anyhow::anyhow!("Private key file authentication failed: {}", e) - })?; + .map_err(|e| anyhow::anyhow!("Private key file authentication failed: {e}"))?; if !auth_result.success() { anyhow::bail!("Private key file authentication rejected by jump host"); diff --git a/src/jump/parser.rs b/src/jump/parser.rs index bab71223..6869030a 100644 --- a/src/jump/parser.rs +++ b/src/jump/parser.rs @@ -152,10 +152,7 @@ pub fn parse_jump_hosts(jump_spec: &str) -> Result> { } if jump_hosts.is_empty() { - anyhow::bail!( - "No valid jump hosts found in specification: '{}'", - jump_spec - ); + anyhow::bail!("No valid jump hosts found in specification: '{jump_spec}'"); } // SECURITY: Validate jump host count to prevent resource exhaustion @@ -248,7 +245,7 @@ fn parse_host_port(host_port: &str) -> Result<(String, Option)> { } return Ok((ipv6_addr.to_string(), Some(port))); } else { - anyhow::bail!("Invalid characters after IPv6 address: '{}'", remaining); + anyhow::bail!("Invalid characters after IPv6 address: '{remaining}'"); } } else { anyhow::bail!("Unclosed bracket in IPv6 address"); @@ -281,7 +278,7 @@ fn parse_host_port(host_port: &str) -> Result<(String, Option)> { // Check if this looks like a port number (all digits) if port_part.chars().all(|c| c.is_ascii_digit()) { // It's clearly intended to be a port but invalid - anyhow::bail!("Invalid port number: '{}' ({})", port_part, e); + anyhow::bail!("Invalid port number: '{port_part}' ({e})"); } else { // Not a port, treat entire string as hostname (might be IPv6) Ok((host_port.to_string(), None)) diff --git a/src/jump/rate_limiter.rs b/src/jump/rate_limiter.rs index e6089d0b..eea21fbd 100644 --- a/src/jump/rate_limiter.rs +++ b/src/jump/rate_limiter.rs @@ -107,9 +107,7 @@ impl ConnectionRateLimiter { host, wait_time ); bail!( - "Connection rate limit exceeded for {}. Please wait {:.1} seconds before retrying.", - host, - wait_time + "Connection rate limit exceeded for {host}. Please wait {wait_time:.1} seconds before retrying." ) } } diff --git a/src/main.rs b/src/main.rs index 0c989e8e..91b11989 100644 --- a/src/main.rs +++ b/src/main.rs @@ -120,7 +120,7 @@ async fn main() -> Result<()> { }; if !expanded_path.exists() { - anyhow::bail!("Config file not found: {:?}", expanded_path); + anyhow::bail!("Config file not found: {expanded_path:?}"); } } @@ -564,12 +564,12 @@ fn parse_node_with_ssh_config(node_str: &str, ssh_config: &SshConfig) -> Result< // Security: Validate hostname let validated_host = bssh::security::validate_hostname(raw_host) - .with_context(|| format!("Invalid hostname in node: {}", raw_host))?; + .with_context(|| format!("Invalid hostname in node: {raw_host}"))?; // Security: Validate username if provided if let Some(user) = user_part { bssh::security::validate_username(user) - .with_context(|| format!("Invalid username in node: {}", user))?; + .with_context(|| format!("Invalid username in node: {user}"))?; } // Now resolve using SSH config with CLI taking precedence @@ -716,7 +716,7 @@ async fn resolve_nodes( if let Some(filter) = cli.get_host_filter() { nodes = filter_nodes(nodes, filter)?; if nodes.is_empty() { - anyhow::bail!("No hosts matched the filter pattern: {}", filter); + anyhow::bail!("No hosts matched the filter pattern: {filter}"); } } @@ -730,10 +730,7 @@ fn filter_nodes(nodes: Vec, pattern: &str) -> Result> { // Security: Validate pattern length to prevent DoS const MAX_PATTERN_LENGTH: usize = 256; if pattern.len() > MAX_PATTERN_LENGTH { - anyhow::bail!( - "Filter pattern too long (max {} characters)", - MAX_PATTERN_LENGTH - ); + anyhow::bail!("Filter pattern too long (max {MAX_PATTERN_LENGTH} characters)"); } // Security: Validate pattern for dangerous constructs @@ -745,10 +742,7 @@ fn filter_nodes(nodes: Vec, pattern: &str) -> Result> { let wildcard_count = pattern.chars().filter(|c| *c == '*' || *c == '?').count(); const MAX_WILDCARDS: usize = 10; if wildcard_count > MAX_WILDCARDS { - anyhow::bail!( - "Filter pattern contains too many wildcards (max {})", - MAX_WILDCARDS - ); + anyhow::bail!("Filter pattern contains too many wildcards (max {MAX_WILDCARDS})"); } // Security: Check for potential path traversal attempts @@ -778,8 +772,8 @@ fn filter_nodes(nodes: Vec, pattern: &str) -> Result> { // If pattern contains wildcards, use glob matching if pattern.contains('*') || pattern.contains('?') || pattern.contains('[') { // Security: Compile pattern with timeout to prevent ReDoS attacks - let glob_pattern = Pattern::new(pattern) - .with_context(|| format!("Invalid filter pattern: {}", pattern))?; + let glob_pattern = + Pattern::new(pattern).with_context(|| format!("Invalid filter pattern: {pattern}"))?; // Performance: Use HashSet for O(1) lookups if we need to check many nodes let mut matched_nodes = Vec::with_capacity(nodes.len()); diff --git a/src/pty/session.rs b/src/pty/session.rs index 51d1b3bc..2faaa71a 100644 --- a/src/pty/session.rs +++ b/src/pty/session.rs @@ -16,7 +16,7 @@ use anyhow::{Context, Result}; use crossterm::event::{Event, KeyCode, KeyEvent, KeyEventKind, KeyModifiers, MouseEvent}; -use russh::{client::Msg, Channel, ChannelMsg}; +use russh::{client::Msg, Channel, ChannelMsg, Pty}; use smallvec::SmallVec; // use signal_hook::iterator::Signals; // Unused in current implementation use std::io::{self, Write}; @@ -98,6 +98,58 @@ const PAGE_DOWN_SEQUENCE: &[u8] = &[0x1b, 0x5b, 0x36, 0x7e]; // ESC[6~ const INSERT_SEQUENCE: &[u8] = &[0x1b, 0x5b, 0x32, 0x7e]; // ESC[2~ const DELETE_SEQUENCE: &[u8] = &[0x1b, 0x5b, 0x33, 0x7e]; // ESC[3~ +/// Configure terminal modes for proper PTY behavior +/// +/// Returns a vector of (Pty, u32) tuples that configure the remote PTY's terminal behavior. +/// These settings are critical for proper operation of interactive programs like sudo and passwd. +/// +/// # Terminal Mode Configuration +/// +/// The modes are configured to provide a standard Unix terminal environment: +/// - **Control Characters**: Ctrl+C (SIGINT), Ctrl+Z (SIGTSTP), Ctrl+D (EOF), etc. +/// - **Input Modes**: CR to NL mapping for Enter key, flow control disabled +/// - **Local Modes**: Signal generation, canonical mode, echo control (for password prompts) +/// - **Output Modes**: NL to CR-NL mapping for proper line endings +/// - **Control Modes**: 8-bit character size +/// +/// These settings match typical Unix terminal configurations and ensure compatibility +/// with command-line utilities that depend on specific terminal behaviors. +fn configure_terminal_modes() -> Vec<(Pty, u32)> { + vec![ + // Special control characters - standard Unix values + (Pty::VINTR, 0x03), // Ctrl+C (SIGINT) + (Pty::VQUIT, 0x1C), // Ctrl+\ (SIGQUIT) + (Pty::VERASE, 0x7F), // DEL (Backspace) + (Pty::VKILL, 0x15), // Ctrl+U (Kill line) + (Pty::VEOF, 0x04), // Ctrl+D (EOF) + (Pty::VSUSP, 0x1A), // Ctrl+Z (SIGTSTP) + // Input modes - configure for proper character handling + (Pty::ICRNL, 1), // Map CR to NL (Enter key works correctly) + (Pty::IXON, 0), // Disable flow control (Ctrl+S/Ctrl+Q usable) + (Pty::IXANY, 0), // Don't restart output on any character + (Pty::IXOFF, 0), // Disable input flow control + (Pty::IMAXBEL, 1), // Ring bell on input queue full + // Local modes - CRITICAL for sudo/passwd password prompts + (Pty::ISIG, 1), // Enable signal generation (Ctrl+C, Ctrl+Z work) + (Pty::ICANON, 1), // Enable canonical mode (line editing with backspace) + (Pty::ECHO, 1), // Enable echo (programs like sudo can disable for passwords) + (Pty::ECHOE, 1), // Visual erase (backspace removes characters visually) + (Pty::ECHOK, 1), // Echo newline after kill character + (Pty::ECHONL, 0), // Don't echo NL when echo is off + (Pty::IEXTEN, 1), // Enable extended input processing + (Pty::ECHOCTL, 1), // Echo control chars as ^X + (Pty::ECHOKE, 1), // Visual erase for kill character + // Output modes - configure for proper line ending handling + (Pty::OPOST, 1), // Enable output processing + (Pty::ONLCR, 1), // Map NL to CR-NL (proper line endings) + // Control modes - 8-bit character size + (Pty::CS8, 1), // 8-bit character size (standard for modern terminals) + // Baud rate (nominal values for compatibility) + (Pty::TTY_OP_ISPEED, 38400), // Input baud rate + (Pty::TTY_OP_OSPEED, 38400), // Output baud rate + ] +} + /// A PTY session managing the bidirectional communication between /// local terminal and remote SSH session. pub struct PtySession { @@ -159,16 +211,18 @@ impl PtySession { // Get terminal size let (width, height) = super::utils::get_terminal_size()?; - // Request PTY on the SSH channel + // 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(); self.channel .request_pty( false, &self.config.term_type, width, height, - 0, // pixel width (0 means undefined) - 0, // pixel height (0 means undefined) - &[], // terminal modes (empty means use defaults) + 0, // pixel width (0 means undefined) + 0, // pixel height (0 means undefined) + &terminal_modes, // Terminal modes using russh Pty enum ) .await .with_context(|| "Failed to request PTY on SSH channel")?; diff --git a/src/security.rs b/src/security.rs index f4533253..fa5cf717 100644 --- a/src/security.rs +++ b/src/security.rs @@ -42,7 +42,7 @@ pub fn validate_local_path(path: &Path) -> Result { // This will fail if the path doesn't exist yet, so we handle that case let canonical = if path.exists() { path.canonicalize() - .with_context(|| format!("Failed to canonicalize path: {:?}", path))? + .with_context(|| format!("Failed to canonicalize path: {path:?}"))? } else { // For non-existent paths, validate the parent directory if let Some(parent) = path.parent() { @@ -54,7 +54,7 @@ pub fn validate_local_path(path: &Path) -> Result { } else if parent.exists() { let canonical_parent = parent .canonicalize() - .with_context(|| format!("Failed to canonicalize parent path: {:?}", parent))?; + .with_context(|| format!("Failed to canonicalize parent path: {parent:?}"))?; // Get the file name let file_name = path @@ -99,7 +99,7 @@ pub fn validate_remote_path(path: &str) -> Result { // Check path length to prevent DoS const MAX_PATH_LENGTH: usize = 4096; if path.len() > MAX_PATH_LENGTH { - anyhow::bail!("Remote path too long (max {} characters)", MAX_PATH_LENGTH); + anyhow::bail!("Remote path too long (max {MAX_PATH_LENGTH} characters)"); } // Check for shell metacharacters that could cause injection @@ -110,7 +110,7 @@ pub fn validate_remote_path(path: &str) -> Result { for &ch in DANGEROUS_CHARS { if path.contains(ch) { - anyhow::bail!("Remote path contains invalid character: '{}'", ch); + anyhow::bail!("Remote path contains invalid character: '{ch}'"); } } @@ -163,7 +163,7 @@ pub fn validate_hostname(hostname: &str) -> Result { // Check hostname length (RFC 1123) const MAX_HOSTNAME_LENGTH: usize = 253; if hostname.len() > MAX_HOSTNAME_LENGTH { - anyhow::bail!("Hostname too long (max {} characters)", MAX_HOSTNAME_LENGTH); + anyhow::bail!("Hostname too long (max {MAX_HOSTNAME_LENGTH} characters)"); } // Validate hostname format (RFC 1123) @@ -194,7 +194,7 @@ pub fn validate_username(username: &str) -> Result { // Check username length const MAX_USERNAME_LENGTH: usize = 32; if username.len() > MAX_USERNAME_LENGTH { - anyhow::bail!("Username too long (max {} characters)", MAX_USERNAME_LENGTH); + anyhow::bail!("Username too long (max {MAX_USERNAME_LENGTH} characters)"); } // Validate username format (POSIX-compliant) @@ -228,7 +228,7 @@ pub fn sanitize_error_message(message: &str) -> String { if let Some(end) = sanitized[start + 6..].find('\'') { let before = &sanitized[..start + 5]; let after = &sanitized[start + 6 + end + 1..]; - sanitized = format!("{}{}", before, after); + sanitized = format!("{before}{after}"); } } diff --git a/src/ssh/client.rs b/src/ssh/client.rs index 65e86940..c192dba5 100644 --- a/src/ssh/client.rs +++ b/src/ssh/client.rs @@ -225,7 +225,7 @@ impl SshClient { "Host key verification failed. The server's host key was not recognized or has changed.".to_string() } super::tokio_client::Error::KeyInvalid(key_err) => { - format!("Failed to load SSH key: {}. Please check the key file format and passphrase.", key_err) + format!("Failed to load SSH key: {key_err}. Please check the key file format and passphrase.") } super::tokio_client::Error::AgentConnectionFailed => { "Failed to connect to SSH agent. Please ensure SSH_AUTH_SOCK is set and the agent is running.".to_string() @@ -237,18 +237,17 @@ impl SshClient { "SSH agent authentication failed.".to_string() } super::tokio_client::Error::SshError(ssh_err) => { - format!("SSH connection error: {}", ssh_err) + format!("SSH connection error: {ssh_err}") } _ => { - format!("Failed to connect: {}", e) + format!("Failed to connect: {e}") } }; Err(anyhow::anyhow!(error_msg).context(e)) } Err(_) => Err(anyhow::anyhow!( - "Connection timeout after {} seconds. \ - Please check if the host is reachable and SSH service is running.", - SSH_CONNECT_TIMEOUT_SECS + "Connection timeout after {SSH_CONNECT_TIMEOUT_SECS} seconds. \ + Please check if the host is reachable and SSH service is running." )), } } @@ -336,43 +335,37 @@ impl SshClient { let context = format!("SSH connection to {}:{}", self.host, self.port); let detailed = match &e { super::tokio_client::Error::KeyAuthFailed => { - format!( - "{} failed: Authentication rejected with provided SSH key", - context - ) + format!("{context} failed: Authentication rejected with provided SSH key") } super::tokio_client::Error::KeyInvalid(err) => { - format!("{} failed: Invalid SSH key - {}", context, err) + format!("{context} failed: Invalid SSH key - {err}") } super::tokio_client::Error::ServerCheckFailed => { - format!("{} failed: Host key verification failed. The server's host key is not trusted.", context) + format!("{context} failed: Host key verification failed. The server's host key is not trusted.") } super::tokio_client::Error::PasswordWrong => { - format!("{} failed: Password authentication rejected", context) + format!("{context} failed: Password authentication rejected") } super::tokio_client::Error::AgentConnectionFailed => { format!( - "{} failed: Cannot connect to SSH agent. Ensure SSH_AUTH_SOCK is set.", - context + "{context} failed: Cannot connect to SSH agent. Ensure SSH_AUTH_SOCK is set." ) } super::tokio_client::Error::AgentNoIdentities => { format!( - "{} failed: SSH agent has no keys. Use 'ssh-add' to add your key.", - context + "{context} failed: SSH agent has no keys. Use 'ssh-add' to add your key." ) } super::tokio_client::Error::AgentAuthenticationFailed => { - format!("{} failed: SSH agent authentication rejected", context) + format!("{context} failed: SSH agent authentication rejected") } - _ => format!("{} failed: {}", context, e), + _ => format!("{context} failed: {e}"), }; return Err(anyhow::anyhow!(detailed).context(e)); } Err(_) => { return Err(anyhow::anyhow!( - "Connection timeout after {} seconds. Host may be unreachable or SSH service not running.", - SSH_CONNECT_TIMEOUT_SECS + "Connection timeout after {SSH_CONNECT_TIMEOUT_SECS} seconds. Host may be unreachable or SSH service not running." )); } }; @@ -381,7 +374,7 @@ impl SshClient { // Check if local file exists if !local_path.exists() { - anyhow::bail!("Local file does not exist: {:?}", local_path); + anyhow::bail!("Local file does not exist: {local_path:?}"); } let metadata = std::fs::metadata(local_path) @@ -471,43 +464,37 @@ impl SshClient { let context = format!("SSH connection to {}:{}", self.host, self.port); let detailed = match &e { super::tokio_client::Error::KeyAuthFailed => { - format!( - "{} failed: Authentication rejected with provided SSH key", - context - ) + format!("{context} failed: Authentication rejected with provided SSH key") } super::tokio_client::Error::KeyInvalid(err) => { - format!("{} failed: Invalid SSH key - {}", context, err) + format!("{context} failed: Invalid SSH key - {err}") } super::tokio_client::Error::ServerCheckFailed => { - format!("{} failed: Host key verification failed. The server's host key is not trusted.", context) + format!("{context} failed: Host key verification failed. The server's host key is not trusted.") } super::tokio_client::Error::PasswordWrong => { - format!("{} failed: Password authentication rejected", context) + format!("{context} failed: Password authentication rejected") } super::tokio_client::Error::AgentConnectionFailed => { format!( - "{} failed: Cannot connect to SSH agent. Ensure SSH_AUTH_SOCK is set.", - context + "{context} failed: Cannot connect to SSH agent. Ensure SSH_AUTH_SOCK is set." ) } super::tokio_client::Error::AgentNoIdentities => { format!( - "{} failed: SSH agent has no keys. Use 'ssh-add' to add your key.", - context + "{context} failed: SSH agent has no keys. Use 'ssh-add' to add your key." ) } super::tokio_client::Error::AgentAuthenticationFailed => { - format!("{} failed: SSH agent authentication rejected", context) + format!("{context} failed: SSH agent authentication rejected") } - _ => format!("{} failed: {}", context, e), + _ => format!("{context} failed: {e}"), }; return Err(anyhow::anyhow!(detailed).context(e)); } Err(_) => { return Err(anyhow::anyhow!( - "Connection timeout after {} seconds. Host may be unreachable or SSH service not running.", - SSH_CONNECT_TIMEOUT_SECS + "Connection timeout after {SSH_CONNECT_TIMEOUT_SECS} seconds. Host may be unreachable or SSH service not running." )); } }; @@ -602,28 +589,24 @@ impl SshClient { let context = format!("SSH connection to {}:{}", self.host, self.port); let detailed = match &e { super::tokio_client::Error::KeyAuthFailed => { - format!( - "{} failed: Authentication rejected with provided SSH key", - context - ) + format!("{context} failed: Authentication rejected with provided SSH key") } super::tokio_client::Error::KeyInvalid(err) => { - format!("{} failed: Invalid SSH key - {}", context, err) + format!("{context} failed: Invalid SSH key - {err}") } super::tokio_client::Error::ServerCheckFailed => { - format!("{} failed: Host key verification failed. The server's host key is not trusted.", context) + format!("{context} failed: Host key verification failed. The server's host key is not trusted.") } super::tokio_client::Error::PasswordWrong => { - format!("{} failed: Password authentication rejected", context) + format!("{context} failed: Password authentication rejected") } - _ => format!("{} failed: {}", context, e), + _ => format!("{context} failed: {e}"), }; return Err(anyhow::anyhow!(detailed).context(e)); } Err(_) => { return Err(anyhow::anyhow!( - "Connection timeout after {} seconds. Host may be unreachable or SSH service not running.", - SSH_CONNECT_TIMEOUT_SECS + "Connection timeout after {SSH_CONNECT_TIMEOUT_SECS} seconds. Host may be unreachable or SSH service not running." )); } }; @@ -632,11 +615,11 @@ impl SshClient { // Check if local directory exists if !local_dir_path.exists() { - anyhow::bail!("Local directory does not exist: {:?}", local_dir_path); + anyhow::bail!("Local directory does not exist: {local_dir_path:?}"); } if !local_dir_path.is_dir() { - anyhow::bail!("Local path is not a directory: {:?}", local_dir_path); + anyhow::bail!("Local path is not a directory: {local_dir_path:?}"); } tracing::debug!( @@ -721,28 +704,24 @@ impl SshClient { let context = format!("SSH connection to {}:{}", self.host, self.port); let detailed = match &e { super::tokio_client::Error::KeyAuthFailed => { - format!( - "{} failed: Authentication rejected with provided SSH key", - context - ) + format!("{context} failed: Authentication rejected with provided SSH key") } super::tokio_client::Error::KeyInvalid(err) => { - format!("{} failed: Invalid SSH key - {}", context, err) + format!("{context} failed: Invalid SSH key - {err}") } super::tokio_client::Error::ServerCheckFailed => { - format!("{} failed: Host key verification failed. The server's host key is not trusted.", context) + format!("{context} failed: Host key verification failed. The server's host key is not trusted.") } super::tokio_client::Error::PasswordWrong => { - format!("{} failed: Password authentication rejected", context) + format!("{context} failed: Password authentication rejected") } - _ => format!("{} failed: {}", context, e), + _ => format!("{context} failed: {e}"), }; return Err(anyhow::anyhow!(detailed).context(e)); } Err(_) => { return Err(anyhow::anyhow!( - "Connection timeout after {} seconds. Host may be unreachable or SSH service not running.", - SSH_CONNECT_TIMEOUT_SECS + "Connection timeout after {SSH_CONNECT_TIMEOUT_SECS} seconds. Host may be unreachable or SSH service not running." )); } }; @@ -856,7 +835,7 @@ impl SshClient { // Check if local file exists if !local_path.exists() { - anyhow::bail!("Local file does not exist: {:?}", local_path); + anyhow::bail!("Local file does not exist: {local_path:?}"); } let metadata = std::fs::metadata(local_path) @@ -1060,11 +1039,11 @@ impl SshClient { // Check if local directory exists if !local_dir_path.exists() { - anyhow::bail!("Local directory does not exist: {:?}", local_dir_path); + anyhow::bail!("Local directory does not exist: {local_dir_path:?}"); } if !local_dir_path.is_dir() { - anyhow::bail!("Local path is not a directory: {:?}", local_dir_path); + anyhow::bail!("Local path is not a directory: {local_dir_path:?}"); } tracing::debug!( diff --git a/src/ssh/config_cache.rs b/src/ssh/config_cache.rs index 76367c16..ff9adb1d 100644 --- a/src/ssh/config_cache.rs +++ b/src/ssh/config_cache.rs @@ -198,7 +198,7 @@ impl SshConfigCache { }) .await .map_err(|_| anyhow::anyhow!("Timeout acquiring stats write lock"))? - .map_err(|e| anyhow::anyhow!("Stats lock poisoned: {}", e))?; + .map_err(|e| anyhow::anyhow!("Stats lock poisoned: {e}"))?; stats.misses += 1; } @@ -210,7 +210,7 @@ impl SshConfigCache { let mut cache = self .cache .write() - .map_err(|e| anyhow::anyhow!("Cache write lock poisoned: {}", e))?; + .map_err(|e| anyhow::anyhow!("Cache write lock poisoned: {e}"))?; if let Some(entry) = cache.get_mut(path) { // Check if entry is expired @@ -219,7 +219,7 @@ impl SshConfigCache { cache.pop(path); let mut stats = self.stats.write().map_err(|e| { - anyhow::anyhow!("Stats write lock poisoned during TTL eviction: {}", e) + anyhow::anyhow!("Stats write lock poisoned during TTL eviction: {e}") })?; stats.ttl_evictions += 1; return Ok(None); @@ -231,7 +231,7 @@ impl SshConfigCache { cache.pop(path); let mut stats = self.stats.write().map_err(|e| { - anyhow::anyhow!("Stats write lock poisoned during stale eviction: {}", e) + anyhow::anyhow!("Stats write lock poisoned during stale eviction: {e}") })?; stats.stale_evictions += 1; return Ok(None); @@ -243,7 +243,7 @@ impl SshConfigCache { // Update statistics { let mut stats = self.stats.write().map_err(|e| { - anyhow::anyhow!("Stats write lock poisoned during cache hit: {}", e) + anyhow::anyhow!("Stats write lock poisoned during cache hit: {e}") })?; stats.hits += 1; } @@ -260,7 +260,7 @@ impl SshConfigCache { let mut cache = self .cache .write() - .map_err(|e| anyhow::anyhow!("Cache write lock poisoned in put: {}", e))?; + .map_err(|e| anyhow::anyhow!("Cache write lock poisoned in put: {e}"))?; // Check if we're evicting an entry due to LRU policy let will_evict = cache.len() >= cache.cap().get(); @@ -273,7 +273,7 @@ impl SshConfigCache { let mut stats = self .stats .write() - .map_err(|e| anyhow::anyhow!("Stats write lock poisoned in put: {}", e))?; + .map_err(|e| anyhow::anyhow!("Stats write lock poisoned in put: {e}"))?; if will_evict { stats.lru_evictions += 1; } @@ -313,13 +313,13 @@ impl SshConfigCache { let mut cache = self .cache .write() - .map_err(|e| anyhow::anyhow!("Cache write lock poisoned in clear: {}", e))?; + .map_err(|e| anyhow::anyhow!("Cache write lock poisoned in clear: {e}"))?; cache.clear(); let mut stats = self .stats .write() - .map_err(|e| anyhow::anyhow!("Stats write lock poisoned in clear: {}", e))?; + .map_err(|e| anyhow::anyhow!("Stats write lock poisoned in clear: {e}"))?; stats.current_entries = 0; Ok(()) } @@ -331,14 +331,14 @@ impl SshConfigCache { let mut cache = self .cache .write() - .map_err(|e| anyhow::anyhow!("Cache write lock poisoned in remove: {}", e))?; + .map_err(|e| anyhow::anyhow!("Cache write lock poisoned in remove: {e}"))?; let entry = cache.pop(&canonical_path); if entry.is_some() { let mut stats = self .stats .write() - .map_err(|e| anyhow::anyhow!("Stats write lock poisoned in remove: {}", e))?; + .map_err(|e| anyhow::anyhow!("Stats write lock poisoned in remove: {e}"))?; stats.current_entries = cache.len(); } @@ -352,7 +352,7 @@ impl SshConfigCache { pub fn stats(&self) -> Result { self.stats .read() - .map_err(|e| anyhow::anyhow!("Stats read lock poisoned: {}", e)) + .map_err(|e| anyhow::anyhow!("Stats read lock poisoned: {e}")) .map(|stats| stats.clone()) } @@ -403,7 +403,7 @@ impl SshConfigCache { let cache = self .cache .write() - .map_err(|e| anyhow::anyhow!("Cache write lock poisoned in maintain: {}", e))?; + .map_err(|e| anyhow::anyhow!("Cache write lock poisoned in maintain: {e}"))?; for (path, entry) in cache.iter() { if entry.is_expired(self.config.ttl) { @@ -441,10 +441,7 @@ impl SshConfigCache { // Remove expired and stale entries { let mut cache = self.cache.write().map_err(|e| { - anyhow::anyhow!( - "Cache write lock poisoned during maintenance cleanup: {}", - e - ) + anyhow::anyhow!("Cache write lock poisoned during maintenance cleanup: {e}") })?; for path in &to_remove { cache.pop(path); @@ -456,10 +453,10 @@ impl SshConfigCache { // Update statistics { let cache = self.cache.read().map_err(|e| { - anyhow::anyhow!("Cache read lock poisoned during maintenance stats: {}", e) + anyhow::anyhow!("Cache read lock poisoned during maintenance stats: {e}") })?; let mut stats = self.stats.write().map_err(|e| { - anyhow::anyhow!("Stats write lock poisoned during maintenance: {}", e) + anyhow::anyhow!("Stats write lock poisoned during maintenance: {e}") })?; stats.ttl_evictions += expired_count as u64; stats.stale_evictions += stale_count as u64; @@ -481,7 +478,7 @@ impl SshConfigCache { let cache = self .cache .read() - .map_err(|e| anyhow::anyhow!("Cache read lock poisoned in debug_info: {}", e))?; + .map_err(|e| anyhow::anyhow!("Cache read lock poisoned in debug_info: {e}"))?; let mut info = HashMap::new(); for (path, entry) in cache.iter() { diff --git a/src/ssh/ssh_config/parser.rs b/src/ssh/ssh_config/parser.rs index 16b99159..d5c91f3e 100644 --- a/src/ssh/ssh_config/parser.rs +++ b/src/ssh/ssh_config/parser.rs @@ -55,8 +55,7 @@ pub(super) fn parse(content: &str) -> Result> { // Start new host config if args.is_empty() { anyhow::bail!( - "Host directive requires at least one pattern at line {}", - line_number + "Host directive requires at least one pattern at line {line_number}" ); } @@ -102,19 +101,19 @@ pub(super) fn parse_option( match keyword { "hostname" => { if args.is_empty() { - anyhow::bail!("HostName requires a value at line {}", line_number); + anyhow::bail!("HostName requires a value at line {line_number}"); } host.hostname = Some(args[0].to_string()); } "user" => { if args.is_empty() { - anyhow::bail!("User requires a value at line {}", line_number); + anyhow::bail!("User requires a value at line {line_number}"); } host.user = Some(args[0].to_string()); } "port" => { if args.is_empty() { - anyhow::bail!("Port requires a value at line {}", line_number); + anyhow::bail!("Port requires a value at line {line_number}"); } let port: u16 = args[0].parse().with_context(|| { format!("Invalid port number '{}' at line {}", args[0], line_number) @@ -123,7 +122,7 @@ pub(super) fn parse_option( } "identityfile" => { if args.is_empty() { - anyhow::bail!("IdentityFile requires a value at line {}", line_number); + anyhow::bail!("IdentityFile requires a value at line {line_number}"); } let path = secure_validate_path(args[0], "identity", line_number) .with_context(|| format!("Invalid IdentityFile path at line {line_number}"))?; @@ -131,7 +130,7 @@ pub(super) fn parse_option( } "identitiesonly" => { if args.is_empty() { - anyhow::bail!("IdentitiesOnly requires a value at line {}", line_number); + anyhow::bail!("IdentitiesOnly requires a value at line {line_number}"); } // Parse yes/no and store in identity_files behavior (implicit) let value = parse_yes_no(args[0], line_number)?; @@ -142,13 +141,13 @@ pub(super) fn parse_option( } "proxyjump" => { if args.is_empty() { - anyhow::bail!("ProxyJump requires a value at line {}", line_number); + anyhow::bail!("ProxyJump requires a value at line {line_number}"); } host.proxy_jump = Some(args.join(" ")); } "proxycommand" => { if args.is_empty() { - anyhow::bail!("ProxyCommand requires a value at line {}", line_number); + anyhow::bail!("ProxyCommand requires a value at line {line_number}"); } let command = args.join(" "); validate_executable_string(&command, "ProxyCommand", line_number)?; @@ -156,19 +155,13 @@ pub(super) fn parse_option( } "stricthostkeychecking" => { if args.is_empty() { - anyhow::bail!( - "StrictHostKeyChecking requires a value at line {}", - line_number - ); + anyhow::bail!("StrictHostKeyChecking requires a value at line {line_number}"); } host.strict_host_key_checking = Some(args[0].to_string()); } "userknownhostsfile" => { if args.is_empty() { - anyhow::bail!( - "UserKnownHostsFile requires a value at line {}", - line_number - ); + anyhow::bail!("UserKnownHostsFile requires a value at line {line_number}"); } let path = secure_validate_path(args[0], "known_hosts", line_number).with_context(|| { @@ -178,10 +171,7 @@ pub(super) fn parse_option( } "globalknownhostsfile" => { if args.is_empty() { - anyhow::bail!( - "GlobalKnownHostsFile requires a value at line {}", - line_number - ); + anyhow::bail!("GlobalKnownHostsFile requires a value at line {line_number}"); } let path = secure_validate_path(args[0], "known_hosts", line_number).with_context(|| { @@ -191,22 +181,19 @@ pub(super) fn parse_option( } "forwardagent" => { if args.is_empty() { - anyhow::bail!("ForwardAgent requires a value at line {}", line_number); + anyhow::bail!("ForwardAgent requires a value at line {line_number}"); } host.forward_agent = Some(parse_yes_no(args[0], line_number)?); } "forwardx11" => { if args.is_empty() { - anyhow::bail!("ForwardX11 requires a value at line {}", line_number); + anyhow::bail!("ForwardX11 requires a value at line {line_number}"); } host.forward_x11 = Some(parse_yes_no(args[0], line_number)?); } "serveraliveinterval" => { if args.is_empty() { - anyhow::bail!( - "ServerAliveInterval requires a value at line {}", - line_number - ); + anyhow::bail!("ServerAliveInterval requires a value at line {line_number}"); } let interval: u32 = args[0].parse().with_context(|| { format!( @@ -218,10 +205,7 @@ pub(super) fn parse_option( } "serveralivecountmax" => { if args.is_empty() { - anyhow::bail!( - "ServerAliveCountMax requires a value at line {}", - line_number - ); + anyhow::bail!("ServerAliveCountMax requires a value at line {line_number}"); } let count: u32 = args[0].parse().with_context(|| { format!( @@ -233,7 +217,7 @@ pub(super) fn parse_option( } "connecttimeout" => { if args.is_empty() { - anyhow::bail!("ConnectTimeout requires a value at line {}", line_number); + anyhow::bail!("ConnectTimeout requires a value at line {line_number}"); } let timeout: u32 = args[0].parse().with_context(|| { format!( @@ -245,10 +229,7 @@ pub(super) fn parse_option( } "connectionattempts" => { if args.is_empty() { - anyhow::bail!( - "ConnectionAttempts requires a value at line {}", - line_number - ); + anyhow::bail!("ConnectionAttempts requires a value at line {line_number}"); } let attempts: u32 = args[0].parse().with_context(|| { format!( @@ -260,28 +241,25 @@ pub(super) fn parse_option( } "batchmode" => { if args.is_empty() { - anyhow::bail!("BatchMode requires a value at line {}", line_number); + anyhow::bail!("BatchMode requires a value at line {line_number}"); } host.batch_mode = Some(parse_yes_no(args[0], line_number)?); } "compression" => { if args.is_empty() { - anyhow::bail!("Compression requires a value at line {}", line_number); + anyhow::bail!("Compression requires a value at line {line_number}"); } host.compression = Some(parse_yes_no(args[0], line_number)?); } "tcpkeepalive" => { if args.is_empty() { - anyhow::bail!("TCPKeepAlive requires a value at line {}", line_number); + anyhow::bail!("TCPKeepAlive requires a value at line {line_number}"); } host.tcp_keep_alive = Some(parse_yes_no(args[0], line_number)?); } "preferredauthentications" => { if args.is_empty() { - anyhow::bail!( - "PreferredAuthentications requires a value at line {}", - line_number - ); + anyhow::bail!("PreferredAuthentications requires a value at line {line_number}"); } host.preferred_authentications = args .join(",") @@ -291,43 +269,33 @@ pub(super) fn parse_option( } "pubkeyauthentication" => { if args.is_empty() { - anyhow::bail!( - "PubkeyAuthentication requires a value at line {}", - line_number - ); + anyhow::bail!("PubkeyAuthentication requires a value at line {line_number}"); } host.pubkey_authentication = Some(parse_yes_no(args[0], line_number)?); } "passwordauthentication" => { if args.is_empty() { - anyhow::bail!( - "PasswordAuthentication requires a value at line {}", - line_number - ); + anyhow::bail!("PasswordAuthentication requires a value at line {line_number}"); } host.password_authentication = Some(parse_yes_no(args[0], line_number)?); } "kbdinteractiveauthentication" => { if args.is_empty() { anyhow::bail!( - "KbdInteractiveAuthentication requires a value at line {}", - line_number + "KbdInteractiveAuthentication requires a value at line {line_number}" ); } host.keyboard_interactive_authentication = Some(parse_yes_no(args[0], line_number)?); } "gssapiauthentication" => { if args.is_empty() { - anyhow::bail!( - "GSSAPIAuthentication requires a value at line {}", - line_number - ); + anyhow::bail!("GSSAPIAuthentication requires a value at line {line_number}"); } host.gssapi_authentication = Some(parse_yes_no(args[0], line_number)?); } "hostkeyalgorithms" => { if args.is_empty() { - anyhow::bail!("HostKeyAlgorithms requires a value at line {}", line_number); + anyhow::bail!("HostKeyAlgorithms requires a value at line {line_number}"); } host.host_key_algorithms = args .join(",") @@ -337,7 +305,7 @@ pub(super) fn parse_option( } "kexalgorithms" => { if args.is_empty() { - anyhow::bail!("KexAlgorithms requires a value at line {}", line_number); + anyhow::bail!("KexAlgorithms requires a value at line {line_number}"); } host.kex_algorithms = args .join(",") @@ -347,7 +315,7 @@ pub(super) fn parse_option( } "ciphers" => { if args.is_empty() { - anyhow::bail!("Ciphers requires a value at line {}", line_number); + anyhow::bail!("Ciphers requires a value at line {line_number}"); } host.ciphers = args .join(",") @@ -357,7 +325,7 @@ pub(super) fn parse_option( } "macs" => { if args.is_empty() { - anyhow::bail!("MACs requires a value at line {}", line_number); + anyhow::bail!("MACs requires a value at line {line_number}"); } host.macs = args .join(",") @@ -367,13 +335,13 @@ pub(super) fn parse_option( } "sendenv" => { if args.is_empty() { - anyhow::bail!("SendEnv requires a value at line {}", line_number); + anyhow::bail!("SendEnv requires a value at line {line_number}"); } host.send_env.extend(args.iter().map(|s| s.to_string())); } "setenv" => { if args.len() < 2 { - anyhow::bail!("SetEnv requires name=value at line {}", line_number); + anyhow::bail!("SetEnv requires name=value at line {line_number}"); } for arg in args { if let Some(eq_pos) = arg.find('=') { @@ -382,58 +350,56 @@ pub(super) fn parse_option( host.set_env.insert(name, value); } else { anyhow::bail!( - "Invalid SetEnv format '{}' at line {} (expected name=value)", - arg, - line_number + "Invalid SetEnv format '{arg}' at line {line_number} (expected name=value)" ); } } } "localforward" => { if args.is_empty() { - anyhow::bail!("LocalForward requires a value at line {}", line_number); + anyhow::bail!("LocalForward requires a value at line {line_number}"); } host.local_forward.push(args.join(" ")); } "remoteforward" => { if args.is_empty() { - anyhow::bail!("RemoteForward requires a value at line {}", line_number); + anyhow::bail!("RemoteForward requires a value at line {line_number}"); } host.remote_forward.push(args.join(" ")); } "dynamicforward" => { if args.is_empty() { - anyhow::bail!("DynamicForward requires a value at line {}", line_number); + anyhow::bail!("DynamicForward requires a value at line {line_number}"); } host.dynamic_forward.push(args.join(" ")); } "requesttty" => { if args.is_empty() { - anyhow::bail!("RequestTTY requires a value at line {}", line_number); + anyhow::bail!("RequestTTY requires a value at line {line_number}"); } host.request_tty = Some(args[0].to_string()); } "escapechar" => { if args.is_empty() { - anyhow::bail!("EscapeChar requires a value at line {}", line_number); + anyhow::bail!("EscapeChar requires a value at line {line_number}"); } host.escape_char = Some(args[0].to_string()); } "loglevel" => { if args.is_empty() { - anyhow::bail!("LogLevel requires a value at line {}", line_number); + anyhow::bail!("LogLevel requires a value at line {line_number}"); } host.log_level = Some(args[0].to_string()); } "syslogfacility" => { if args.is_empty() { - anyhow::bail!("SyslogFacility requires a value at line {}", line_number); + anyhow::bail!("SyslogFacility requires a value at line {line_number}"); } host.syslog_facility = Some(args[0].to_string()); } "protocol" => { if args.is_empty() { - anyhow::bail!("Protocol requires a value at line {}", line_number); + anyhow::bail!("Protocol requires a value at line {line_number}"); } host.protocol = args .join(",") @@ -443,34 +409,31 @@ pub(super) fn parse_option( } "addressfamily" => { if args.is_empty() { - anyhow::bail!("AddressFamily requires a value at line {}", line_number); + anyhow::bail!("AddressFamily requires a value at line {line_number}"); } host.address_family = Some(args[0].to_string()); } "bindaddress" => { if args.is_empty() { - anyhow::bail!("BindAddress requires a value at line {}", line_number); + anyhow::bail!("BindAddress requires a value at line {line_number}"); } host.bind_address = Some(args[0].to_string()); } "clearallforwardings" => { if args.is_empty() { - anyhow::bail!( - "ClearAllForwardings requires a value at line {}", - line_number - ); + anyhow::bail!("ClearAllForwardings requires a value at line {line_number}"); } host.clear_all_forwardings = Some(parse_yes_no(args[0], line_number)?); } "controlmaster" => { if args.is_empty() { - anyhow::bail!("ControlMaster requires a value at line {}", line_number); + anyhow::bail!("ControlMaster requires a value at line {line_number}"); } host.control_master = Some(args[0].to_string()); } "controlpath" => { if args.is_empty() { - anyhow::bail!("ControlPath requires a value at line {}", line_number); + anyhow::bail!("ControlPath requires a value at line {line_number}"); } let path = args[0].to_string(); // ControlPath has different validation - it allows SSH substitution patterns @@ -479,7 +442,7 @@ pub(super) fn parse_option( } "controlpersist" => { if args.is_empty() { - anyhow::bail!("ControlPersist requires a value at line {}", line_number); + anyhow::bail!("ControlPersist requires a value at line {line_number}"); } host.control_persist = Some(args[0].to_string()); } @@ -501,11 +464,9 @@ pub(super) fn parse_yes_no(value: &str, line_number: usize) -> Result { match value.to_lowercase().as_str() { "yes" | "true" | "1" => Ok(true), "no" | "false" | "0" => Ok(false), - _ => anyhow::bail!( - "Invalid yes/no value '{}' at line {} (expected yes/no)", - value, - line_number - ), + _ => { + anyhow::bail!("Invalid yes/no value '{value}' at line {line_number} (expected yes/no)") + } } } diff --git a/src/ssh/ssh_config/path.rs b/src/ssh/ssh_config/path.rs index 6b4b623d..b0597802 100644 --- a/src/ssh/ssh_config/path.rs +++ b/src/ssh/ssh_config/path.rs @@ -222,9 +222,8 @@ fn secure_expand_environment_variables(input: &str) -> Result { var_name ); anyhow::bail!( - "Security violation: Attempted to expand dangerous environment variable '{}'. \ - Variables like PATH, LD_LIBRARY_PATH, LD_PRELOAD are not allowed for security reasons.", - var_name + "Security violation: Attempted to expand dangerous environment variable '{var_name}'. \ + Variables like PATH, LD_LIBRARY_PATH, LD_PRELOAD are not allowed for security reasons." ); } @@ -282,9 +281,8 @@ fn secure_expand_environment_variables(input: &str) -> Result { // Security check: Did we hit the expansion depth limit? if expansion_depth >= MAX_EXPANSION_DEPTH { anyhow::bail!( - "Security violation: Environment variable expansion depth limit exceeded ({} levels). \ - This could indicate a recursive expansion attack.", - MAX_EXPANSION_DEPTH + "Security violation: Environment variable expansion depth limit exceeded ({MAX_EXPANSION_DEPTH} levels). \ + This could indicate a recursive expansion attack." ); } @@ -325,9 +323,8 @@ fn sanitize_environment_value(value: &str, var_name: &str) -> Result { // Check for null bytes (could be used for path truncation attacks) if value.contains('\0') { anyhow::bail!( - "Security violation: Environment variable '{}' contains null byte. \ - This could be used for path truncation attacks.", - var_name + "Security violation: Environment variable '{var_name}' contains null byte. \ + This could be used for path truncation attacks." ); } @@ -335,19 +332,16 @@ fn sanitize_environment_value(value: &str, var_name: &str) -> Result { const DANGEROUS_CHARS: &[char] = &[';', '&', '|', '`', '\n', '\r']; if let Some(dangerous_char) = value.chars().find(|c| DANGEROUS_CHARS.contains(c)) { anyhow::bail!( - "Security violation: Environment variable '{}' contains dangerous character '{}'. \ - This could enable command injection attacks.", - var_name, - dangerous_char + "Security violation: Environment variable '{var_name}' contains dangerous character '{dangerous_char}'. \ + This could enable command injection attacks." ); } // Check for command substitution patterns if value.contains("$(") || value.contains("${") { anyhow::bail!( - "Security violation: Environment variable '{}' contains command substitution pattern. \ - This could enable command injection attacks.", - var_name + "Security violation: Environment variable '{var_name}' contains command substitution pattern. \ + This could enable command injection attacks." ); } @@ -367,9 +361,8 @@ fn sanitize_environment_value(value: &str, var_name: &str) -> Result { } _ => { anyhow::bail!( - "Security violation: Environment variable '{}' contains path traversal sequence. \ - This could enable directory traversal attacks.", - var_name + "Security violation: Environment variable '{var_name}' contains path traversal sequence. \ + This could enable directory traversal attacks." ); } } diff --git a/src/ssh/ssh_config/security.rs b/src/ssh/ssh_config/security.rs index f068aff0..ff156ef2 100644 --- a/src/ssh/ssh_config/security.rs +++ b/src/ssh/ssh_config/security.rs @@ -59,21 +59,16 @@ pub(super) fn validate_executable_string( // Check for dangerous characters if let Some(dangerous_char) = value.chars().find(|c| DANGEROUS_CHARS.contains(c)) { anyhow::bail!( - "Security violation: {} contains dangerous character '{}' at line {}. \ - This could enable command injection attacks.", - option_name, - dangerous_char, - line_number + "Security violation: {option_name} contains dangerous character '{dangerous_char}' at line {line_number}. \ + This could enable command injection attacks." ); } // Check for dangerous command substitution patterns if value.contains("$(") || value.contains("${") { anyhow::bail!( - "Security violation: {} contains command substitution pattern at line {}. \ - This could enable command injection attacks.", - option_name, - line_number + "Security violation: {option_name} contains command substitution pattern at line {line_number}. \ + This could enable command injection attacks." ); } @@ -104,10 +99,8 @@ pub(super) fn validate_executable_string( // Odd number of unescaped quotes suggests potential quote injection if quote_count % 2 != 0 { anyhow::bail!( - "Security violation: {} contains unmatched quote at line {}. \ - This could enable command injection attacks.", - option_name, - line_number + "Security violation: {option_name} contains unmatched quote at line {line_number}. \ + This could enable command injection attacks." ); } @@ -117,9 +110,8 @@ pub(super) fn validate_executable_string( // and should not start with suspicious patterns if value.trim_start().starts_with('-') { anyhow::bail!( - "Security violation: ControlPath starts with '-' at line {}. \ - This could be interpreted as a command flag.", - line_number + "Security violation: ControlPath starts with '-' at line {line_number}. \ + This could be interpreted as a command flag." ); } @@ -138,10 +130,8 @@ pub(super) fn validate_executable_string( _ => { // Unknown substitution pattern - potentially dangerous anyhow::bail!( - "Security violation: ControlPath contains unknown substitution pattern '%{}' at line {}. \ - Only %h, %p, %r, %u, %L, %l, %n, %d, and %% are allowed.", - next_char, - line_number + "Security violation: ControlPath contains unknown substitution pattern '%{next_char}' at line {line_number}. \ + Only %h, %p, %r, %u, %L, %l, %n, %d, and %% are allowed." ); } } @@ -189,9 +179,8 @@ pub(super) fn validate_executable_string( || lower_value.contains("cat /") { anyhow::bail!( - "Security violation: ProxyCommand contains suspicious command pattern at line {}. \ - Commands like curl, wget, nc, rm, dd are not typical for SSH proxying.", - line_number + "Security violation: ProxyCommand contains suspicious command pattern at line {line_number}. \ + Commands like curl, wget, nc, rm, dd are not typical for SSH proxying." ); } } @@ -235,28 +224,24 @@ pub(super) fn validate_control_path(path: &str, line_number: usize) -> Result<() // Check for dangerous characters if let Some(dangerous_char) = path.chars().find(|c| DANGEROUS_CHARS.contains(c)) { anyhow::bail!( - "Security violation: ControlPath contains dangerous character '{}' at line {}. \ - This could enable command injection attacks.", - dangerous_char, - line_number + "Security violation: ControlPath contains dangerous character '{dangerous_char}' at line {line_number}. \ + This could enable command injection attacks." ); } // Check for command substitution patterns (but allow environment variables) if path.contains("$(") { anyhow::bail!( - "Security violation: ControlPath contains command substitution pattern at line {}. \ - This could enable command injection attacks.", - line_number + "Security violation: ControlPath contains command substitution pattern at line {line_number}. \ + This could enable command injection attacks." ); } // Check for paths starting with suspicious patterns if path.trim_start().starts_with('-') { anyhow::bail!( - "Security violation: ControlPath starts with '-' at line {}. \ - This could be interpreted as a command flag.", - line_number + "Security violation: ControlPath starts with '-' at line {line_number}. \ + This could be interpreted as a command flag." ); } @@ -274,10 +259,8 @@ pub(super) fn validate_control_path(path: &str, line_number: usize) -> Result<() _ => { // Unknown substitution pattern - potentially dangerous anyhow::bail!( - "Security violation: ControlPath contains unknown substitution pattern '%{}' at line {}. \ - Only %h, %p, %r, %u, %L, %l, %n, %d, and %% are allowed.", - next_char, - line_number + "Security violation: ControlPath contains unknown substitution pattern '%{next_char}' at line {line_number}. \ + Only %h, %p, %r, %u, %L, %l, %n, %d, and %% are allowed." ); } } @@ -322,20 +305,16 @@ pub(super) fn secure_validate_path( // Check for directory traversal sequences if path_str.contains("../") || path_str.contains("..\\") { anyhow::bail!( - "Security violation: {} path contains directory traversal sequence '..' at line {}. \ - Path traversal attacks are not allowed.", - path_type, - line_number + "Security violation: {path_type} path contains directory traversal sequence '..' at line {line_number}. \ + Path traversal attacks are not allowed." ); } // Check for null bytes and other dangerous characters if path_str.contains('\0') { anyhow::bail!( - "Security violation: {} path contains null byte at line {}. \ - This could be used for path truncation attacks.", - path_type, - line_number + "Security violation: {path_type} path contains null byte at line {line_number}. \ + This could be used for path truncation attacks." ); } @@ -365,11 +344,8 @@ pub(super) fn secure_validate_path( || canonical_str.split('\\').any(|component| component == "..") { anyhow::bail!( - "Security violation: Canonicalized {} path '{}' contains parent directory references at line {}. \ - This could indicate a path traversal attempt.", - path_type, - canonical_str, - line_number + "Security violation: Canonicalized {path_type} path '{canonical_str}' contains parent directory references at line {line_number}. \ + This could indicate a path traversal attempt." ); } } @@ -416,10 +392,8 @@ pub(super) fn validate_identity_file_security(path: &Path, line_number: usize) - for pattern in &sensitive_patterns { if path_str.contains(pattern) { anyhow::bail!( - "Security violation: Identity file path '{}' at line {} points to sensitive system location. \ - Access to system files is not allowed for security reasons.", - path_str, - line_number + "Security violation: Identity file path '{path_str}' at line {line_number} points to sensitive system location. \ + Access to system files is not allowed for security reasons." ); } } @@ -454,10 +428,8 @@ pub(super) fn validate_identity_file_security(path: &Path, line_number: usize) - // Check if file is world-writable (very dangerous) if mode & 0o002 != 0 { anyhow::bail!( - "Security violation: Identity file '{}' at line {} is world-writable. \ - This is extremely dangerous and must be fixed immediately.", - path_str, - line_number + "Security violation: Identity file '{path_str}' at line {line_number} is world-writable. \ + This is extremely dangerous and must be fixed immediately." ); } } @@ -490,10 +462,8 @@ pub(super) fn validate_known_hosts_file_security(path: &Path, line_number: usize for pattern in &sensitive_patterns { if path_str.contains(pattern) { anyhow::bail!( - "Security violation: Known hosts file path '{}' at line {} points to sensitive system location. \ - Access to system files is not allowed for security reasons.", - path_str, - line_number + "Security violation: Known hosts file path '{path_str}' at line {line_number} points to sensitive system location. \ + Access to system files is not allowed for security reasons." ); } } @@ -544,10 +514,8 @@ pub(super) fn validate_general_file_security(path: &Path, line_number: usize) -> for pattern in &forbidden_patterns { if path_str.contains(pattern) { anyhow::bail!( - "Security violation: File path '{}' at line {} points to forbidden system location. \ - Access to this location is not allowed for security reasons.", - path_str, - line_number + "Security violation: File path '{path_str}' at line {line_number} points to forbidden system location. \ + Access to this location is not allowed for security reasons." ); } } diff --git a/src/ssh/tokio_client/client.rs b/src/ssh/tokio_client/client.rs index 87e72e72..100af734 100644 --- a/src/ssh/tokio_client/client.rs +++ b/src/ssh/tokio_client/client.rs @@ -875,42 +875,36 @@ impl Client { } } - /// Request an interactive shell with PTY support. + /// Request an interactive shell channel. /// - /// This method opens a new SSH channel with PTY (pseudo-terminal) support, - /// suitable for interactive shell sessions. + /// This method opens a new SSH channel suitable for interactive shell sessions. + /// Note: This method no longer requests PTY directly. The PTY should be requested + /// by the caller (e.g., PtySession) with appropriate terminal modes. /// /// # Arguments - /// * `term_type` - Terminal type (e.g., "xterm", "xterm-256color", "vt100") - /// * `width` - Terminal width in columns - /// * `height` - Terminal height in rows + /// * `_term_type` - Terminal type (unused, kept for API compatibility) + /// * `_width` - Terminal width (unused, kept for API compatibility) + /// * `_height` - Terminal height (unused, kept for API compatibility) /// /// # Returns /// A `Channel` that can be used for bidirectional communication with the remote shell. + /// + /// # Note + /// The caller is responsible for: + /// 1. Requesting PTY with proper terminal modes via `channel.request_pty()` + /// 2. Requesting shell via `channel.request_shell()` + /// + /// This change fixes issue #40: PTY should be requested once with proper terminal + /// modes by PtySession::initialize() rather than twice with empty modes. pub async fn request_interactive_shell( &self, - term_type: &str, - width: u32, - height: u32, + _term_type: &str, + _width: u32, + _height: u32, ) -> Result, super::Error> { + // Open a session channel - PTY and shell will be requested by the caller + // (e.g., PtySession::initialize() with proper terminal modes) let channel = self.connection_handle.channel_open_session().await?; - - // Request PTY with the specified terminal type and dimensions - channel - .request_pty( - false, - term_type, - width, - height, - 0, // pixel width (0 means undefined) - 0, // pixel height (0 means undefined) - &[], // terminal modes (empty means use defaults) - ) - .await?; - - // Request shell - channel.request_shell(false).await?; - Ok(channel) } diff --git a/src/utils/fs.rs b/src/utils/fs.rs index f3a649b7..34a37bc0 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -46,8 +46,7 @@ pub fn resolve_source_files(source: &Path, recursive: bool) -> Result Result Result { // For IPv6 with brackets, validate the content between brackets let ipv6_addr = &hostname[1..hostname.len() - 1]; if !ipv6_addr.chars().all(|c| c.is_ascii_hexdigit() || c == ':') { - bail!("Invalid IPv6 address format: {}", hostname); + bail!("Invalid IPv6 address format: {hostname}"); } } else if is_ipv6_raw { // For IPv6 without brackets, validate the entire string if !hostname.chars().all(|c| c.is_ascii_hexdigit() || c == ':') { - bail!("Invalid IPv6 address format: {}", hostname); + bail!("Invalid IPv6 address format: {hostname}"); } } else { // For regular hostnames and IPv4 let valid_chars = |c: char| c.is_ascii_alphanumeric() || c == '.' || c == '-' || c == '_'; if !hostname.chars().all(valid_chars) { - bail!("Invalid characters in hostname: {}", hostname); + bail!("Invalid characters in hostname: {hostname}"); } // Check for double dots which could be path traversal attempts @@ -165,7 +165,7 @@ pub fn sanitize_username(username: &str) -> Result { let valid_chars = |c: char| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '.'; if !username.chars().all(valid_chars) { - bail!("Invalid characters in username: {}", username); + bail!("Invalid characters in username: {username}"); } // Username should start with letter or underscore (Unix convention) diff --git a/tests/glob_pattern_test.rs b/tests/glob_pattern_test.rs index 7a3942b4..f983fdf1 100644 --- a/tests/glob_pattern_test.rs +++ b/tests/glob_pattern_test.rs @@ -24,7 +24,7 @@ fn resolve_source_files(source: &Path) -> anyhow::Result> { let matches: Vec = glob::glob(pattern_str)?.filter_map(Result::ok).collect(); if matches.is_empty() { - anyhow::bail!("No files matched the pattern: {}", pattern_str); + anyhow::bail!("No files matched the pattern: {pattern_str}"); } return Ok(matches); diff --git a/tests/jump_host_timeout_test.rs b/tests/jump_host_timeout_test.rs index bd500d2b..f5aa4493 100644 --- a/tests/jump_host_timeout_test.rs +++ b/tests/jump_host_timeout_test.rs @@ -67,8 +67,7 @@ fn test_timeout_calculation_max_allowed_hops() { assert_eq!(timeout, 180); // 30 + 150 = 180 (under 600 max) assert!( timeout <= 600, - "Timeout should never exceed 600 seconds, got {}", - timeout + "Timeout should never exceed 600 seconds, got {timeout}" ); } From 4f572f1e60fed23dc8b76d0f19201c53336f14c2 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Tue, 14 Oct 2025 23:48:11 +0900 Subject: [PATCH 2/2] fix: complete PTY terminal modes and Shift key input support This commit fixes two critical issues preventing sudo password input from working correctly in interactive PTY sessions: 1. Incomplete Terminal Modes Configuration - Added all OpenSSH-compatible terminal mode settings (RFC 4254) - Control characters: VEOL, VEOL2, VSTART, VSTOP, VREPRINT, VWERASE, VLNEXT, VDISCARD - Input modes: IGNPAR, PARMRK, INPCK, ISTRIP, INLCR, IGNCR - Local modes: NOFLSH, TOSTOP, PENDIN - Output modes: OCRNL, ONOCR, ONLRET - Control modes: PARENB, PARODD - These settings ensure proper terminal behavior for interactive programs 2. Shift Key Input Bug - Fixed key_event_to_bytes() to accept SHIFT modifier - Previously only accepted KeyModifiers::NONE, blocking uppercase letters and special characters that require Shift key - Now accepts characters with no modifiers or SHIFT modifier only - Rejects CONTROL, ALT, META combinations (handled separately) Impact: - Passwords with uppercase letters and special characters now work correctly - Fixes issue where Linux remote hosts rejected password input - macOS localhost previously worked due to simpler password requirements Fixes #40 --- src/pty/session.rs | 54 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 14 deletions(-) diff --git a/src/pty/session.rs b/src/pty/session.rs index 2faaa71a..69c11f56 100644 --- a/src/pty/session.rs +++ b/src/pty/session.rs @@ -116,14 +116,28 @@ const DELETE_SEQUENCE: &[u8] = &[0x1b, 0x5b, 0x33, 0x7e]; // ESC[3~ /// with command-line utilities that depend on specific terminal behaviors. fn configure_terminal_modes() -> Vec<(Pty, u32)> { vec![ - // Special control characters - standard Unix values - (Pty::VINTR, 0x03), // Ctrl+C (SIGINT) - (Pty::VQUIT, 0x1C), // Ctrl+\ (SIGQUIT) - (Pty::VERASE, 0x7F), // DEL (Backspace) - (Pty::VKILL, 0x15), // Ctrl+U (Kill line) - (Pty::VEOF, 0x04), // Ctrl+D (EOF) - (Pty::VSUSP, 0x1A), // Ctrl+Z (SIGTSTP) - // Input modes - configure for proper character handling + // Special control characters - complete set matching OpenSSH + (Pty::VINTR, 0x03), // Ctrl+C (SIGINT) + (Pty::VQUIT, 0x1C), // Ctrl+\ (SIGQUIT) + (Pty::VERASE, 0x7F), // DEL (Backspace) + (Pty::VKILL, 0x15), // Ctrl+U (Kill line) + (Pty::VEOF, 0x04), // Ctrl+D (EOF) + (Pty::VEOL, 0xFF), // Undefined (0xFF = disabled) + (Pty::VEOL2, 0xFF), // Undefined (0xFF = disabled) + (Pty::VSTART, 0x11), // Ctrl+Q (XON - resume output) + (Pty::VSTOP, 0x13), // Ctrl+S (XOFF - stop output) + (Pty::VSUSP, 0x1A), // Ctrl+Z (SIGTSTP) + (Pty::VREPRINT, 0x12), // Ctrl+R (reprint current line) + (Pty::VWERASE, 0x17), // Ctrl+W (erase word) + (Pty::VLNEXT, 0x16), // Ctrl+V (literal next character) + (Pty::VDISCARD, 0x0F), // Ctrl+O (discard output) + // Input modes - comprehensive configuration + (Pty::IGNPAR, 0), // Don't ignore parity errors + (Pty::PARMRK, 0), // Don't mark parity errors + (Pty::INPCK, 0), // Disable input parity checking + (Pty::ISTRIP, 0), // Don't strip 8th bit + (Pty::INLCR, 0), // Don't map NL to CR on input + (Pty::IGNCR, 0), // Don't ignore CR (Pty::ICRNL, 1), // Map CR to NL (Enter key works correctly) (Pty::IXON, 0), // Disable flow control (Ctrl+S/Ctrl+Q usable) (Pty::IXANY, 0), // Don't restart output on any character @@ -136,14 +150,22 @@ fn configure_terminal_modes() -> Vec<(Pty, u32)> { (Pty::ECHOE, 1), // Visual erase (backspace removes characters visually) (Pty::ECHOK, 1), // Echo newline after kill character (Pty::ECHONL, 0), // Don't echo NL when echo is off + (Pty::NOFLSH, 0), // Flush after interrupt/quit (normal behavior) + (Pty::TOSTOP, 0), // Don't stop background processes writing to tty (Pty::IEXTEN, 1), // Enable extended input processing (Pty::ECHOCTL, 1), // Echo control chars as ^X (Pty::ECHOKE, 1), // Visual erase for kill character + (Pty::PENDIN, 0), // Don't retype pending input // Output modes - configure for proper line ending handling - (Pty::OPOST, 1), // Enable output processing - (Pty::ONLCR, 1), // Map NL to CR-NL (proper line endings) + (Pty::OPOST, 1), // Enable output processing + (Pty::ONLCR, 1), // Map NL to CR-NL (proper line endings) + (Pty::OCRNL, 0), // Don't map CR to NL on output + (Pty::ONOCR, 0), // Output CR even at column 0 + (Pty::ONLRET, 0), // NL doesn't do CR function // Control modes - 8-bit character size - (Pty::CS8, 1), // 8-bit character size (standard for modern terminals) + (Pty::CS8, 1), // 8-bit character size (standard for modern terminals) + (Pty::PARENB, 0), // Disable parity + (Pty::PARODD, 0), // Even parity (when enabled) // Baud rate (nominal values for compatibility) (Pty::TTY_OP_ISPEED, 38400), // Input baud rate (Pty::TTY_OP_OSPEED, 38400), // Output baud rate @@ -550,12 +572,16 @@ impl PtySession { } } - // Handle regular characters + // Handle regular characters (including those with Shift modifier) + // Accept characters with no modifiers or only SHIFT modifier + // Reject CONTROL, ALT, META combinations as they have special handling KeyEvent { code: KeyCode::Char(c), - modifiers: KeyModifiers::NONE, + modifiers, .. - } => { + } if !modifiers + .intersects(KeyModifiers::CONTROL | KeyModifiers::ALT | KeyModifiers::META) => + { let bytes = c.to_string().into_bytes(); Some(SmallVec::from_slice(&bytes)) }