From 0085cbc7569ed523aef49119d9d1fdd389cf833c Mon Sep 17 00:00:00 2001 From: SnaetWarre Date: Sun, 20 Sep 2026 23:58:39 +0200 Subject: [PATCH 1/6] Harden the extension download and staging path Refuse extension installs without a checksum instead of skipping verification, and enforce HTTPS at the crate layer so a future caller cannot regress the command-level allowlist. Stream response bodies with a 512 MiB cap matching the tool installer instead of buffering an unbounded body into memory. Stage archives under unpredictable, exclusively created names so a pre-planted symlink in the temp directory cannot redirect the install write. --- Cargo.lock | 1 + crates/extensions/Cargo.toml | 3 +- crates/extensions/src/installer.rs | 135 ++++++++++++++++++++++++----- 3 files changed, 117 insertions(+), 22 deletions(-) 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/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) } From 56b5794d932c56afc06ce2783518ef45f8af54f6 Mon Sep 17 00:00:00 2001 From: SnaetWarre Date: Mon, 21 Sep 2026 00:00:55 +0200 Subject: [PATCH 2/6] Block shells and temp paths in tool exec configs Reject known shell interpreters by basename so a formatter or linter config can no longer smuggle `command: "sh"` with `-c` arguments into a spawned process. Reject commands staged under temporary directories and NUL bytes in commands or arguments, which would otherwise panic the process spawn. Forbid NODE_OPTIONS and JVM tool options in tool environments since they inject code into spawned runtimes. Wire argument validation into the format and lint call sites. --- src-tauri/src/commands/editor/exec_guard.rs | 83 ++++++++++++++++++++- src-tauri/src/commands/editor/format.rs | 5 +- src-tauri/src/commands/editor/lint.rs | 11 ++- 3 files changed, 94 insertions(+), 5 deletions(-) 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) { From 573122741b06b1a43102374ab6e12816c357cd54 Mon Sep 17 00:00:00 2001 From: SnaetWarre Date: Mon, 21 Sep 2026 00:02:58 +0200 Subject: [PATCH 3/6] Cap language server and debug adapter frame sizes Bound protocol reads to 64 MiB so a rogue server or adapter cannot OOM the backend by advertising a huge Content-Length. The LSP reader stops the offending server; the debug adapter reader rejects the message as a protocol violation. --- crates/debugger/src/lib.rs | 4 ++++ crates/lsp/src/client.rs | 16 ++++++++++++++++ 2 files changed, 20 insertions(+) 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/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() { From 73065ab7b1bb4daff14cb17051935f0415315e57 Mon Sep 17 00:00:00 2001 From: SnaetWarre Date: Mon, 21 Sep 2026 00:05:11 +0200 Subject: [PATCH 4/6] Quote debugger args and terminal drops unconditionally Always single-quote debug adapter terminal arguments instead of passing them through raw when the adapter claims shell readiness. That flag arrives from workspace-controlled adapters, so honoring it splices metacharacters straight into the user's terminal. Quote dropped terminal paths with POSIX single quotes and strip line breaks first, since a pasted carriage return submits the line even inside quotes. A file name carrying newlines or pipes can no longer break out of its argument. --- .../debugger/services/debug-adapter-events.ts | 5 +---- src/features/debugger/tests/debugger-command.test.ts | 9 ++++++--- src/features/debugger/utils/debugger-command.ts | 7 +++++-- .../terminal/tests/terminal-file-drop.test.ts | 11 ++++++++++- src/features/terminal/utils/terminal-file-drop.ts | 9 ++++++--- 5 files changed, 28 insertions(+), 13 deletions(-) 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(" ")} `; } From 3d272fe5529a548b713486e8f6e8b7177fa2e779 Mon Sep 17 00:00:00 2001 From: SnaetWarre Date: Mon, 21 Sep 2026 00:08:59 +0200 Subject: [PATCH 5/6] Confine agent file and terminal access to the workspace Resolve agent file paths against the session workspace and reject absolute paths, parent traversal, and symlink escapes instead of passing them to the filesystem verbatim. Deny terminal sessions whose working directory leaves the workspace, and strip loader and runtime code-execution variables from agent-supplied environments. Honest agents working inside the project see no behavior change. --- crates/ai/src/acp/client.rs | 63 +++++++++++++++----- crates/ai/src/acp/workspace_path.rs | 91 ++++++++++++++++++++++++++--- 2 files changed, 131 insertions(+), 23 deletions(-) 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")); + } } From 166785b99664fd620c45f4ac53a329e3ec7f6669 Mon Sep 17 00:00:00 2001 From: SnaetWarre Date: Mon, 21 Sep 2026 00:12:40 +0200 Subject: [PATCH 6/6] Keep credentials out of connection strings and git errors Percent-encode database usernames and passwords when building connection URLs so metacharacters cannot shift the parsed host and redirect credentials to the wrong server. Reject the git ext transport on clone because it executes local commands, and redact embedded userinfo from git failure output before it reaches UI errors and logs. --- crates/database/src/connection_manager.rs | 70 +++++++++++++++++++++-- crates/version-control/src/git/remote.rs | 60 ++++++++++++++++++- 2 files changed, 123 insertions(+), 7 deletions(-) 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/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" + ); + } }