diff --git a/Cargo.lock b/Cargo.lock index e3290a4cd..42f8bcea5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -771,6 +771,7 @@ dependencies = [ "anyhow", "chrono", "flate2", + "futures-util", "log", "reqwest 0.12.28", "serde", diff --git a/crates/ai/src/acp/client.rs b/crates/ai/src/acp/client.rs index b8793e603..66facb86e 100644 --- a/crates/ai/src/acp/client.rs +++ b/crates/ai/src/acp/client.rs @@ -75,8 +75,9 @@ impl AthasAcpClient { } } - fn resolve_path(&self, path: &str) -> PathBuf { + fn resolve_path(&self, path: &str) -> Result { resolve_path_against_workspace(self.workspace_path.as_deref(), path) + .map_err(|e| format!("Path escapes the agent workspace: {}", e)) } fn map_plan_priority(priority: acp::PlanEntryPriority) -> AcpPlanEntryPriority { @@ -668,7 +669,10 @@ impl AthasAcpClient { args: acp::ReadTextFileRequest, ) -> acp::Result { let path_str = args.path.to_string_lossy(); - let path = self.resolve_path(&path_str); + let path = match self.resolve_path(&path_str) { + Ok(path) => path, + Err(message) => return Err(acp::Error::new(-32602, message)), + }; match tokio::fs::read_to_string(&path).await { Ok(content) => { // Handle line and limit parameters for partial file reading @@ -701,7 +705,10 @@ impl AthasAcpClient { args: acp::WriteTextFileRequest, ) -> acp::Result { let path_str = args.path.to_string_lossy(); - let path = self.resolve_path(&path_str); + let path = match self.resolve_path(&path_str) { + Ok(path) => path, + Err(message) => return Err(acp::Error::new(-32602, message)), + }; // Create parent directories if needed if let Some(parent) = path.parent() @@ -736,22 +743,50 @@ impl AthasAcpClient { )); } - let working_dir = args - .cwd - .as_ref() - .map(|p| p.to_string_lossy().to_string()) - .or_else(|| self.workspace_path.as_deref().map(path_to_string)); + let working_dir = match args.cwd.as_ref() { + Some(cwd) => { + let cwd_str = cwd.to_string_lossy(); + match self.resolve_path(&cwd_str) { + Ok(path) => Some(path.to_string_lossy().to_string()), + Err(message) => return Err(acp::Error::new(-32602, message)), + } + } + None => self.workspace_path.as_deref().map(path_to_string), + }; + // Agent-supplied environments must not smuggle loader or runtime + // options that execute code inside the spawned process. let env_map: Option> = if args.env.is_empty() { None } else { - Some( - args - .env + let filtered: HashMap = args + .env + .iter() + .filter(|e| { + let upper = e.name.to_ascii_uppercase(); + let blocked = [ + "PATH", + "LD_PRELOAD", + "LD_LIBRARY_PATH", + "LD_AUDIT", + "NODE_OPTIONS", + "JAVA_TOOL_OPTIONS", + "JDK_JAVA_OPTIONS", + ] .iter() - .map(|e| (e.name.clone(), e.value.clone())) - .collect(), - ) + .any(|key| upper == *key || upper.starts_with("LD_") || upper.starts_with("DYLD_")); + if blocked { + log::warn!("Dropping agent-supplied environment variable {}", e.name); + } + !blocked + }) + .map(|e| (e.name.clone(), e.value.clone())) + .collect(); + if filtered.is_empty() { + None + } else { + Some(filtered) + } }; let command = args.command.clone(); let command_args = if args.args.is_empty() { diff --git a/crates/ai/src/acp/workspace_path.rs b/crates/ai/src/acp/workspace_path.rs index 44825b13c..faf5d264e 100644 --- a/crates/ai/src/acp/workspace_path.rs +++ b/crates/ai/src/acp/workspace_path.rs @@ -35,15 +35,54 @@ pub(super) fn path_to_string(path: &Path) -> String { path.to_string_lossy().to_string() } -pub(super) fn resolve_path_against_workspace(workspace_path: Option<&Path>, path: &str) -> PathBuf { +pub(super) fn resolve_path_against_workspace( + workspace_path: Option<&Path>, + path: &str, +) -> Result { let candidate = PathBuf::from(path); - if candidate.is_absolute() { - return candidate; + let joined = if candidate.is_absolute() { + candidate + } else if let Some(workspace) = workspace_path { + workspace.join(candidate) + } else { + std::env::current_dir().unwrap_or_default().join(candidate) + }; + let normalized = lexical_normalize(&joined); + let Some(workspace) = workspace_path else { + return Ok(normalized); + }; + + let workspace_normalized = lexical_normalize(workspace); + if !normalized.starts_with(&workspace_normalized) { + bail!("Path escapes the agent workspace"); } + enforce_no_symlink_escape(&workspace_normalized, &normalized)?; + Ok(normalized) +} - workspace_path - .map(|workspace| workspace.join(candidate.clone())) - .unwrap_or_else(|| std::env::current_dir().unwrap_or_default().join(candidate)) +fn enforce_no_symlink_escape(workspace: &Path, path: &Path) -> Result<()> { + let canonical_workspace = fs::canonicalize(workspace) + .with_context(|| format!("Workspace path is not reachable: {}", workspace.display()))?; + let mut ancestor = path; + loop { + match fs::canonicalize(ancestor) { + Ok(canonical) => { + if !canonical.starts_with(&canonical_workspace) { + bail!("Path escapes the agent workspace through a symlink"); + } + return Ok(()); + } + Err(_) => { + let Some(parent) = ancestor.parent() else { + bail!("Path escapes the agent workspace"); + }; + if parent.as_os_str().is_empty() { + bail!("Path escapes the agent workspace"); + } + ancestor = parent; + } + } + } } fn path_from_workspace_input(input: &str) -> Result { @@ -198,11 +237,45 @@ mod tests { #[test] fn resolves_relative_paths_against_workspace() { - let workspace = PathBuf::from("/workspace"); + let temp_dir = tempfile::tempdir().unwrap(); + let workspace = temp_dir.path().join("repo"); + fs::create_dir(&workspace).unwrap(); assert_eq!( - resolve_path_against_workspace(Some(&workspace), "src/main.ts"), - PathBuf::from("/workspace/src/main.ts") + resolve_path_against_workspace(Some(&workspace), "src/main.ts").unwrap(), + workspace.join("src/main.ts") ); } + + #[test] + fn rejects_absolute_paths_outside_the_workspace() { + let temp_dir = tempfile::tempdir().unwrap(); + let workspace = temp_dir.path().join("repo"); + fs::create_dir(&workspace).unwrap(); + + let err = resolve_path_against_workspace(Some(&workspace), "/etc/passwd").unwrap_err(); + assert!(err.to_string().contains("escapes the agent workspace")); + } + + #[test] + fn rejects_parent_traversal_outside_the_workspace() { + let temp_dir = tempfile::tempdir().unwrap(); + let workspace = temp_dir.path().join("repo"); + fs::create_dir(&workspace).unwrap(); + + let err = resolve_path_against_workspace(Some(&workspace), "../outside.txt").unwrap_err(); + assert!(err.to_string().contains("escapes the agent workspace")); + } + + #[cfg(unix)] + #[test] + fn rejects_symlink_escape_from_the_workspace() { + let temp_dir = tempfile::tempdir().unwrap(); + let workspace = temp_dir.path().join("repo"); + fs::create_dir(&workspace).unwrap(); + std::os::unix::fs::symlink("/etc", workspace.join("link")).unwrap(); + + let err = resolve_path_against_workspace(Some(&workspace), "link/passwd").unwrap_err(); + assert!(err.to_string().contains("escapes the agent workspace")); + } } diff --git a/crates/database/src/connection_manager.rs b/crates/database/src/connection_manager.rs index 7db6c04e5..fdb0510e8 100644 --- a/crates/database/src/connection_manager.rs +++ b/crates/database/src/connection_manager.rs @@ -267,29 +267,32 @@ fn network_connection_string( return Ok(cs.clone()); } - let pass = password.unwrap_or_default(); + // Percent-encode credentials so metacharacters such as @ : / ? # + // cannot shift the parsed host and send the password elsewhere. + let user = encode_url_userinfo(&config.username); + let pass = encode_url_userinfo(&password.unwrap_or_default()); match config.db_type.as_str() { #[cfg(feature = "postgres")] "postgres" => Ok(format!( "postgres://{}:{}@{}:{}/{}", - config.username, pass, config.host, config.port, config.database + user, pass, config.host, config.port, config.database )), #[cfg(feature = "mysql")] "mysql" => Ok(format!( "mysql://{}:{}@{}:{}/{}", - config.username, pass, config.host, config.port, config.database + user, pass, config.host, config.port, config.database )), #[cfg(feature = "mongodb")] "mongodb" => Ok(format!( "mongodb://{}:{}@{}:{}/{}", - config.username, pass, config.host, config.port, config.database + user, pass, config.host, config.port, config.database )), #[cfg(feature = "redis")] "redis" => { if !config.username.is_empty() { Ok(format!( "redis://{}:{}@{}:{}", - config.username, pass, config.host, config.port + user, pass, config.host, config.port )) } else if !pass.is_empty() { Ok(format!("redis://:{}@{}:{}", pass, config.host, config.port)) @@ -300,3 +303,60 @@ fn network_connection_string( _ => Err(format!("Unsupported database type: {}", config.db_type)), } } + +#[cfg(any( + feature = "postgres", + feature = "mysql", + feature = "mongodb", + feature = "redis" +))] +fn encode_url_userinfo(input: &str) -> String { + const UNRESERVED: &[u8] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~"; + const HEX: &[u8; 16] = b"0123456789ABCDEF"; + let mut encoded = String::with_capacity(input.len()); + for byte in input.bytes() { + if UNRESERVED.contains(&byte) { + encoded.push(byte as char); + } else { + encoded.push('%'); + encoded.push(HEX[(byte >> 4) as usize] as char); + encoded.push(HEX[(byte & 15) as usize] as char); + } + } + encoded +} + +#[cfg(all(test, feature = "postgres"))] +mod tests { + use super::*; + + fn test_config() -> ConnectionConfig { + ConnectionConfig { + id: "test".to_string(), + name: "test".to_string(), + db_type: "postgres".to_string(), + host: "db.internal".to_string(), + port: 5432, + database: "app".to_string(), + username: "user@corp".to_string(), + connection_string: None, + } + } + + #[test] + fn encodes_credential_metacharacters() { + let url = network_connection_string(&test_config(), Some("p@ss:w/rd?#".to_string())).unwrap(); + assert_eq!( + url, + "postgres://user%40corp:p%40ss%3Aw%2Frd%3F%23@db.internal:5432/app" + ); + } + + #[test] + fn leaves_plain_credentials_untouched() { + let mut config = test_config(); + config.username = "app".to_string(); + let url = network_connection_string(&config, Some("s3cret".to_string())).unwrap(); + assert_eq!(url, "postgres://app:s3cret@db.internal:5432/app"); + } +} diff --git a/crates/debugger/src/lib.rs b/crates/debugger/src/lib.rs index c8ae842c3..6fd6971d3 100644 --- a/crates/debugger/src/lib.rs +++ b/crates/debugger/src/lib.rs @@ -477,6 +477,7 @@ fn spawn_exit_watcher( } fn read_protocol_message(reader: &mut impl BufRead) -> Result> { + const MAX_PROTOCOL_FRAME_BYTES: usize = 64 * 1024 * 1024; let mut content_length = None; let mut line = String::new(); @@ -499,6 +500,9 @@ fn read_protocol_message(reader: &mut impl BufRead) -> Result> { } let content_length = content_length.context("Debug adapter message missing Content-Length")?; + if content_length > MAX_PROTOCOL_FRAME_BYTES { + anyhow::bail!("Debug adapter sent an oversized protocol frame"); + } let mut content = vec![0u8; content_length]; reader.read_exact(&mut content)?; diff --git a/crates/extensions/Cargo.toml b/crates/extensions/Cargo.toml index 021abe55b..0c73e651d 100644 --- a/crates/extensions/Cargo.toml +++ b/crates/extensions/Cargo.toml @@ -12,8 +12,9 @@ linux = ["tauri/cef"] anyhow = "1.0" chrono = { version = "0.4.41", features = ["serde"] } flate2 = "1.0" +futures-util = "0.3" log = "0.4" -reqwest = "0.12" +reqwest = { version = "0.12", features = ["stream"] } serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" sha256 = "1.5" diff --git a/crates/extensions/src/installer.rs b/crates/extensions/src/installer.rs index 0accc94a9..77d340a1f 100644 --- a/crates/extensions/src/installer.rs +++ b/crates/extensions/src/installer.rs @@ -1,13 +1,22 @@ use super::types::{DownloadInfo, ExtensionMetadata, InstallProgress, InstallStatus}; use crate::runtime::AthasAppHandle as AppHandle; use anyhow::{Context, Result}; +use futures_util::StreamExt; use serde::Deserialize; use std::{ fs, path::{Path, PathBuf}, + sync::atomic::{AtomicU64, Ordering}, }; use tauri::{Emitter, Manager}; +/// Absolute upper bound for a single extension download. Mirrors the tool +/// installer's binary cap so a compromised distribution point cannot OOM the +/// backend by serving an unbounded body. +const MAX_EXTENSION_DOWNLOAD_BYTES: u64 = 512 * 1024 * 1024; + +static STAGING_COUNTER: AtomicU64 = AtomicU64::new(0); + pub struct ExtensionInstaller { app_handle: AppHandle, extensions_dir: PathBuf, @@ -38,6 +47,57 @@ pub fn validate_extension_id(extension_id: &str) -> Result<()> { Ok(()) } +fn require_download_checksum(extension_id: &str, download_info: &DownloadInfo) -> Result<()> { + if download_info.checksum.is_empty() { + anyhow::bail!( + "Refusing to install integration {} without a checksum", + extension_id + ); + } + Ok(()) +} + +fn require_https_download_url(url: &str) -> Result<()> { + if url.starts_with("https://") { + return Ok(()); + } + #[cfg(debug_assertions)] + if url.starts_with("http://localhost:") || url.starts_with("http://127.0.0.1:") { + return Ok(()); + } + anyhow::bail!("Integration download URL must use HTTPS"); +} + +fn create_exclusive_staging_file(extension_id: &str, bytes: &[u8]) -> Result { + let temp_dir = std::env::temp_dir(); + for _ in 0..100 { + let unique = STAGING_COUNTER.fetch_add(1, Ordering::Relaxed); + let candidate = temp_dir.join(format!( + ".{}-{}-{}.tar.gz", + extension_id, + std::process::id(), + unique + )); + match std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&candidate) + { + Ok(mut file) => { + use std::io::Write; + file.write_all(bytes)?; + return Ok(candidate); + } + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => continue, + Err(error) => return Err(error.into()), + } + } + anyhow::bail!( + "Could not create a unique staging file for integration {}", + extension_id + ); +} + impl ExtensionInstaller { pub fn new(app_handle: AppHandle) -> Result { let app_data_dir = app_handle @@ -63,6 +123,8 @@ impl ExtensionInstaller { download_info: &DownloadInfo, ) -> Result { validate_extension_id(extension_id)?; + require_download_checksum(extension_id, download_info)?; + require_https_download_url(&download_info.url)?; log::info!( "Downloading integration {} from {}", @@ -97,7 +159,42 @@ impl ExtensionInstaller { anyhow::bail!("Failed to download integration {extension_id}: HTTP {status}{hint}"); } - let bytes = response.bytes().await?; + // Stream the body with a hard cap so a compromised distribution + // point cannot OOM the backend with an unbounded response. + let mut limit = MAX_EXTENSION_DOWNLOAD_BYTES; + if download_info.size > 0 { + if download_info.size > MAX_EXTENSION_DOWNLOAD_BYTES { + anyhow::bail!( + "Integration {} declares an excessive size of {} bytes", + extension_id, + download_info.size + ); + } + limit = download_info.size; + } + if let Some(advertised) = response.content_length() + && advertised > limit + { + anyhow::bail!( + "Integration {} advertises {} bytes, above the {} byte limit", + extension_id, + advertised, + limit + ); + } + let mut bytes = Vec::new(); + let mut stream = response.bytes_stream(); + while let Some(chunk) = stream.next().await { + let chunk = chunk?; + if bytes.len() as u64 + chunk.len() as u64 > limit { + anyhow::bail!( + "Integration {} exceeded the {} byte download limit", + extension_id, + limit + ); + } + bytes.extend_from_slice(&chunk); + } if download_info.size > 0 && bytes.len() as u64 != download_info.size { anyhow::bail!( @@ -125,31 +222,27 @@ impl ExtensionInstaller { }, ); - if !download_info.checksum.is_empty() { - let checksum = sha256::digest(bytes.as_ref()); - if checksum != download_info.checksum { - anyhow::bail!( - "Checksum mismatch for integration {}: expected {}, got {}", - extension_id, - download_info.checksum, - checksum - ); - } - } - if download_info.checksum.is_empty() { - log::info!( - "Checksum verification skipped for integration {}", + anyhow::bail!( + "Refusing to install integration {} without a checksum", extension_id ); - } else { - log::info!("Checksum verified for integration {}", extension_id); } + let checksum = sha256::digest(bytes.as_slice()); + if checksum != download_info.checksum { + anyhow::bail!( + "Checksum mismatch for integration {}: expected {}, got {}", + extension_id, + download_info.checksum, + checksum + ); + } + + log::info!("Checksum verified for integration {}", extension_id); - // Save to temporary file - let temp_dir = std::env::temp_dir(); - let temp_file = temp_dir.join(format!("{}.tar.gz", extension_id)); - fs::write(&temp_file, bytes)?; + // Stage under an unpredictable, exclusively created name so a local + // attacker cannot pre-plant a symlink at the staging path. + let temp_file = create_exclusive_staging_file(extension_id, &bytes)?; Ok(temp_file) } diff --git a/crates/lsp/src/client.rs b/crates/lsp/src/client.rs index 619c5b9a9..4a1e60b0e 100644 --- a/crates/lsp/src/client.rs +++ b/crates/lsp/src/client.rs @@ -25,6 +25,10 @@ type PendingRequests = Arc>>>>; pub type LspServerEnv = HashMap; static NEXT_CLIENT_ID: AtomicU64 = AtomicU64::new(1); +/// Largest protocol frame accepted from a language server. Guards the +/// stdout reader against a rogue server advertising gigabytes. +const MAX_PROTOCOL_FRAME_BYTES: usize = 64 * 1024 * 1024; + #[derive(Default)] struct LspServerContext { root_uri: Option, @@ -313,6 +317,18 @@ impl LspClient { continue; } + // A rogue server must not be able to OOM the backend by + // advertising a gigabyte Content-Length. + if content_length > MAX_PROTOCOL_FRAME_BYTES { + log::warn!("LSP server advertised an oversized frame; stopping server"); + mark_stopped( + "LSP server sent an oversized protocol frame".to_string(), + &pending_requests_clone, + &is_running_clone, + ); + return; + } + // Read content let mut content = vec![0u8; content_length]; if reader.read_exact(&mut content).is_err() { diff --git a/crates/version-control/src/git/remote.rs b/crates/version-control/src/git/remote.rs index 426b6796f..03c53062f 100644 --- a/crates/version-control/src/git/remote.rs +++ b/crates/version-control/src/git/remote.rs @@ -18,6 +18,9 @@ fn _git_clone(repository_url: String, destination_path: String) -> Result<()> { if repository_url.starts_with('-') { bail!("Repository URL cannot start with an option prefix"); } + if repository_url.len() >= 5 && repository_url[..5].eq_ignore_ascii_case("ext::") { + bail!("The ext transport can execute local commands and is not allowed"); + } let destination = Path::new(&destination_path); if !destination.is_absolute() { @@ -61,7 +64,7 @@ fn _git_clone(repository_url: String, destination_path: String) -> Result<()> { "Git returned a non-zero exit status without output.".to_string() }; - bail!("Git clone failed: {details}"); + bail!("Git clone failed: {}", redact_url_credentials(&details)); } pub fn git_push(repo_path: String, branch: Option, remote: String) -> Result<(), String> { @@ -98,7 +101,37 @@ pub(crate) fn execute_remote_git_command( "Git returned a non-zero exit status without output.".to_string() }; - bail!("Git {operation} failed: {details}"); + bail!( + "Git {operation} failed: {}", + redact_url_credentials(&details) + ); +} + +/// Replace `user:password@` userinfo in URLs with `***` so embedded +/// credentials never reach UI errors or logs. +fn redact_url_credentials(text: &str) -> String { + let mut redacted = String::with_capacity(text.len()); + let mut rest = text; + while let Some(scheme_end) = rest.find("://") { + let after_scheme = &rest[scheme_end + 3..]; + let userinfo_end = after_scheme + .find(['@', '/', ' ', '\n', '"', '\'']) + .map(|index| (index, after_scheme.as_bytes().get(index))); + match userinfo_end { + Some((index, Some(b'@'))) => { + redacted.push_str(&rest[..scheme_end + 3]); + redacted.push_str("***@"); + rest = &after_scheme[index + 1..]; + } + _ => { + let keep = scheme_end + 3; + redacted.push_str(&rest[..keep]); + rest = &rest[keep..]; + } + } + } + redacted.push_str(rest); + redacted } fn _git_push(repo_path: String, branch: Option, remote: String) -> Result<()> { @@ -207,4 +240,27 @@ mod tests { ); assert!(result.is_err()); } + + #[test] + fn clone_rejects_ext_transport_urls() { + let result = _git_clone( + "ext::sh -c cp".to_string(), + "/tmp/athas-clone-target".to_string(), + ); + assert!(result.unwrap_err().to_string().contains("ext transport")); + } + + #[test] + fn redacts_embedded_credentials_from_git_output() { + assert_eq!( + redact_url_credentials( + "repository 'https://user:s3cret@github.com/org/repo.git' not found" + ), + "repository 'https://***@github.com/org/repo.git' not found" + ); + assert_eq!( + redact_url_credentials("https://github.com/org/repo.git"), + "https://github.com/org/repo.git" + ); + } } diff --git a/src-tauri/src/commands/editor/exec_guard.rs b/src-tauri/src/commands/editor/exec_guard.rs index e1d163cde..188fa3f8e 100644 --- a/src-tauri/src/commands/editor/exec_guard.rs +++ b/src-tauri/src/commands/editor/exec_guard.rs @@ -25,6 +25,28 @@ const FORBIDDEN_ENV_KEYS: &[&str] = &[ "DYLD_FALLBACK_FRAMEWORK_PATH", "DYLD_FORCE_FLAT_NAMESPACE", "DYLD_IMAGE_SUFFIX", + "NODE_OPTIONS", + "JAVA_TOOL_OPTIONS", + "JDK_JAVA_OPTIONS", +]; + +/// Binary names that interpret a `-c`-style argument as code. Blocking them +/// by basename closes the `command: "sh", args: ["-c", payload]` shape no +/// matter how the binary is referenced. +const SHELL_BINARIES: &[&str] = &[ + "sh", + "bash", + "dash", + "ash", + "zsh", + "fish", + "ksh", + "csh", + "tcsh", + "powershell", + "pwsh", + "cmd", + "wsl", ]; /// Validate the `command` field of a formatter/linter config. @@ -32,27 +54,60 @@ const FORBIDDEN_ENV_KEYS: &[&str] = &[ /// The name must be a bare executable (looked up via `PATH`) or an absolute /// path. Relative paths that traverse the filesystem (containing `..` or a /// path separator) are rejected so callers cannot smuggle a project-relative -/// binary that would be resolved against a surprising CWD. +/// binary that would be resolved against a surprising CWD. Known shells are +/// rejected by basename because their `-c` arguments execute arbitrary code, +/// and binaries staged under temporary directories are rejected because +/// those locations are writable by other local users. pub fn validate_exec_command(command: &str) -> Result<(), String> { let trimmed = command.trim(); if trimmed.is_empty() { return Err("Command must not be empty".to_string()); } + if trimmed.contains('\0') { + return Err("Command must not contain NUL bytes".to_string()); + } if trimmed.contains("..") { return Err("Command must not contain '..'".to_string()); } + let file_name = trimmed.rsplit(['/', '\\']).next().unwrap_or(trimmed); + if SHELL_BINARIES + .iter() + .any(|shell| file_name.eq_ignore_ascii_case(shell)) + { + return Err("Shell interpreters are not allowed as tool commands".to_string()); + } + let has_separator = trimmed.contains('/') || trimmed.contains('\\'); if has_separator { - let is_absolute = std::path::Path::new(trimmed).is_absolute(); - if !is_absolute { + let path = std::path::Path::new(trimmed); + if !path.is_absolute() { return Err( "Command with path separators must be an absolute path, not relative".to_string(), ); } + if is_temporary_path(path) { + return Err("Tool commands must not run from temporary directories".to_string()); + } + } + + Ok(()) +} + +fn is_temporary_path(path: &std::path::Path) -> bool { + if path.starts_with(std::env::temp_dir()) { + return true; } + path.starts_with("/dev/shm") +} +/// Validate the `args` of a formatter/linter config. NUL bytes would panic +/// the process spawn, turning a malicious config into a backend crash. +pub fn validate_exec_args(args: &[String]) -> Result<(), String> { + if args.iter().any(|arg| arg.contains('\0')) { + return Err("Tool arguments must not contain NUL bytes".to_string()); + } Ok(()) } @@ -135,6 +190,28 @@ mod tests { assert!(validate_exec_env(&env).is_err()); } + #[test] + fn rejects_shell_interpreters() { + assert!(validate_exec_command("sh").is_err()); + assert!(validate_exec_command("bash").is_err()); + assert!(validate_exec_command("/bin/sh").is_err()); + assert!(validate_exec_command("C:\\Windows\\System32\\cmd.exe").is_err()); + } + + #[test] + fn rejects_temporary_directory_commands() { + let staged = std::env::temp_dir().join("evil"); + assert!(validate_exec_command(staged.to_str().unwrap()).is_err()); + assert!(validate_exec_command("/dev/shm/evil").is_err()); + } + + #[test] + fn rejects_nul_bytes() { + assert!(validate_exec_command("prettier\0").is_err()); + assert!(validate_exec_args(&["--write\0".to_string()]).is_err()); + assert!(validate_exec_args(&["--write".to_string()]).is_ok()); + } + #[test] fn accepts_benign_env() { let mut env = HashMap::new(); diff --git a/src-tauri/src/commands/editor/format.rs b/src-tauri/src/commands/editor/format.rs index 8eb775084..a7a0d0cba 100644 --- a/src-tauri/src/commands/editor/format.rs +++ b/src-tauri/src/commands/editor/format.rs @@ -1,5 +1,5 @@ use super::{ - exec_guard::{validate_exec_command, validate_exec_env}, + exec_guard::{validate_exec_args, validate_exec_command, validate_exec_env}, extension_command::build_extension_command, }; use athas_runtime::process::configure_background_command; @@ -72,6 +72,9 @@ async fn format_with_generic( // before the template variables get a chance to be substituted. validate_exec_command(&config.command) .map_err(|e| format!("Invalid formatter config: {}", e))?; + if let Some(args) = config.args.as_deref() { + validate_exec_args(args).map_err(|e| format!("Invalid formatter config: {}", e))?; + } if let Some(env) = &config.env { validate_exec_env(env).map_err(|e| format!("Invalid formatter config: {}", e))?; } diff --git a/src-tauri/src/commands/editor/lint.rs b/src-tauri/src/commands/editor/lint.rs index ae63d635e..d9edb06e7 100644 --- a/src-tauri/src/commands/editor/lint.rs +++ b/src-tauri/src/commands/editor/lint.rs @@ -1,5 +1,5 @@ use super::{ - exec_guard::{validate_exec_command, validate_exec_env}, + exec_guard::{validate_exec_args, validate_exec_command, validate_exec_env}, extension_command::build_extension_command, }; use serde::{Deserialize, Serialize}; @@ -87,6 +87,15 @@ async fn lint_with_generic( error: Some(format!("Invalid linter config: {}", e)), }); } + if let Some(args) = config.args.as_deref() + && let Err(e) = validate_exec_args(args) + { + return Ok(LintResponse { + diagnostics: vec![], + success: false, + error: Some(format!("Invalid linter config: {}", e)), + }); + } if let Some(env) = &config.env && let Err(e) = validate_exec_env(env) { diff --git a/src/features/debugger/services/debug-adapter-events.ts b/src/features/debugger/services/debug-adapter-events.ts index 51cf6ea95..b88e23dea 100644 --- a/src/features/debugger/services/debug-adapter-events.ts +++ b/src/features/debugger/services/debug-adapter-events.ts @@ -78,10 +78,7 @@ async function handleDebugRequest(sessionId: string, message: Record typeof arg === "string") : []; - const terminalCommand = buildDebugTerminalCommand( - args, - request?.argsCanBeInterpretedByShell === true, - ); + const terminalCommand = buildDebugTerminalCommand(args); if (!terminalCommand) { await sendDebugAdapterResponse( sessionId, diff --git a/src/features/debugger/tests/debugger-command.test.ts b/src/features/debugger/tests/debugger-command.test.ts index 6c99d9493..00805378a 100644 --- a/src/features/debugger/tests/debugger-command.test.ts +++ b/src/features/debugger/tests/debugger-command.test.ts @@ -27,12 +27,15 @@ describe("debugger command helpers", () => { expect(buildDebugCommand(config)).toBe("bun --inspect-brk /repo/src/main.ts"); }); - test("quotes debug adapter terminal arguments unless they are shell-ready", () => { + test("quotes debug adapter terminal arguments even when shell-ready", () => { expect(buildDebugTerminalCommand(["java", "-cp", "/tmp/my app", "Main"])).toBe( "java -cp '/tmp/my app' Main", ); - expect(buildDebugTerminalCommand(["java", "-cp '/tmp/my app' Main"], true)).toBe( - "java -cp '/tmp/my app' Main", + expect(buildDebugTerminalCommand(["java", "-cp '/tmp/my app' Main"])).toBe( + `java '-cp '\\''/tmp/my app'\\'' Main'`, + ); + expect(buildDebugTerminalCommand(["run", "$(touch /tmp/pwned)"])).toBe( + `run '$(touch /tmp/pwned)'`, ); }); diff --git a/src/features/debugger/utils/debugger-command.ts b/src/features/debugger/utils/debugger-command.ts index 27a401602..1b626b30e 100644 --- a/src/features/debugger/utils/debugger-command.ts +++ b/src/features/debugger/utils/debugger-command.ts @@ -23,8 +23,11 @@ const joinCommand = (parts: Array) => .map(quoteShellArg) .join(" "); -export function buildDebugTerminalCommand(args: string[], interpretedByShell = false): string { - return interpretedByShell ? args.join(" ") : joinCommand(args); +export function buildDebugTerminalCommand(args: string[]): string { + // Always quote. A debug adapter can set argsCanBeInterpretedByShell, but + // honoring that flag would splice adapter-controlled shell metacharacters + // straight into the user's terminal. + return joinCommand(args); } export function inferDebuggerRuntime(file?: DebuggableFile | null): DebuggerRuntime { diff --git a/src/features/terminal/tests/terminal-file-drop.test.ts b/src/features/terminal/tests/terminal-file-drop.test.ts index b850ba2c4..79694b9a3 100644 --- a/src/features/terminal/tests/terminal-file-drop.test.ts +++ b/src/features/terminal/tests/terminal-file-drop.test.ts @@ -8,10 +8,19 @@ describe("formatDroppedPathsForTerminal", () => { "file:///Users/test/My%20Image.png", "/Users/test/project/file.ts", ]), - ).toBe('"/Users/test/My Image.png" /Users/test/project/file.ts '); + ).toBe("'/Users/test/My Image.png' /Users/test/project/file.ts "); }); it("drops unsupported payload entries", () => { expect(formatDroppedPathsForTerminal(["https://athas.dev", "relative/path.ts"])).toBe(""); }); + + it("neutralizes newlines and shell metacharacters in file names", () => { + expect(formatDroppedPathsForTerminal(['/tmp/x"\n/tmp/evil|sh\n'])).toBe( + `'/tmp/x"' '/tmp/evil|sh' `, + ); + expect(formatDroppedPathsForTerminal(["/tmp/$(touch pwned)"])).toBe( + "'/tmp/$(touch pwned)' ", + ); + }); }); diff --git a/src/features/terminal/utils/terminal-file-drop.ts b/src/features/terminal/utils/terminal-file-drop.ts index 9b6c4bc83..5b239d2b4 100644 --- a/src/features/terminal/utils/terminal-file-drop.ts +++ b/src/features/terminal/utils/terminal-file-drop.ts @@ -1,12 +1,15 @@ import { parseDroppedPaths } from "@/features/file-system/utils/file-system-dropped-paths"; function quoteTerminalPath(path: string): string { - const escaped = path.replace(/(["\\$`])/g, "\\$1"); - return /[\s"'\\$`]/.test(path) ? `"${escaped}"` : escaped; + // Strip line breaks first: a pasted CR submits the line even inside + // quotes, so no filename may contribute control characters. + const sanitized = path.replace(/[\r\n]/g, ""); + if (/^[A-Za-z0-9_@%+=:,./-]+$/.test(sanitized)) return sanitized; + return `'${sanitized.replace(/'/g, "'\\''")}'`; } export function formatDroppedPathsForTerminal(rawPaths: string[]): string { - const paths = parseDroppedPaths(rawPaths); + const paths = parseDroppedPaths(rawPaths).filter((path) => !/[\r\n]/.test(path)); if (paths.length === 0) return ""; return `${paths.map(quoteTerminalPath).join(" ")} `; }