From 269caf5dc9c2b3166bf2275c4aa57ce84e7128db Mon Sep 17 00:00:00 2001 From: juan <2930882+juacker@users.noreply.github.com> Date: Thu, 2 Jul 2026 13:08:32 +0200 Subject: [PATCH 1/7] fix(tools): harden web_fetch against private targets --- src-tauri/src/assistant/tools/local.rs | 400 ++++++++++++++++++++----- 1 file changed, 322 insertions(+), 78 deletions(-) diff --git a/src-tauri/src/assistant/tools/local.rs b/src-tauri/src/assistant/tools/local.rs index e013e93..57962ef 100644 --- a/src-tauri/src/assistant/tools/local.rs +++ b/src-tauri/src/assistant/tools/local.rs @@ -1,5 +1,7 @@ +use futures::StreamExt; use glob::{MatchOptions, Pattern}; use std::fs; +use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}; use std::path::{Component, Path, PathBuf}; use std::sync::OnceLock; @@ -30,8 +32,10 @@ const DEFAULT_BASH_OUTPUT_LIMIT: usize = 20_000; const MAX_BASH_OUTPUT_LIMIT: usize = 200_000; const DEFAULT_WEB_FETCH_CONTENT_LIMIT: usize = 20_000; const MAX_WEB_FETCH_CONTENT_LIMIT: usize = 100_000; +const MAX_WEB_FETCH_BODY_BYTES: usize = MAX_WEB_FETCH_CONTENT_LIMIT * 4; const DEFAULT_WEB_FETCH_TIMEOUT_MS: u64 = 15_000; const MAX_WEB_FETCH_TIMEOUT_MS: u64 = 30_000; +const MAX_WEB_FETCH_REDIRECTS: usize = 5; const DEFAULT_WEB_SEARCH_MAX_RESULTS: usize = 10; const MAX_WEB_SEARCH_MAX_RESULTS: usize = 20; const WEB_SEARCH_TIMEOUT_MS: u64 = 10_000; @@ -2007,30 +2011,9 @@ async fn execute_web_fetch(params: WebFetchParams) -> Result Result Result content_limit; - let content = if truncated { + let markdown_truncated = markdown.len() > content_limit; + let content = if markdown_truncated { let chars: Vec = markdown.chars().collect(); let end = content_limit.min(chars.len()); chars[..end].iter().collect::() } else { markdown }; + let truncated = body_truncated || markdown_truncated; Ok(serde_json::json!({ - "url": url, + "url": final_url.as_str(), "contentType": content_type, "content": content, "truncated": truncated, @@ -2082,42 +2064,219 @@ fn html_to_markdown(html: &str) -> String { }) } -/// Check if a URL points to a private/local address (SSRF protection). -fn is_private_url(url: &str) -> bool { - // Extract host from URL +fn parse_web_fetch_url(raw: &str) -> Result { + let url = reqwest::Url::parse(raw) + .map_err(|_| "URL must be an absolute http:// or https:// URL".to_string())?; + validate_web_fetch_scheme(&url)?; + Ok(url) +} + +fn validate_web_fetch_scheme(url: &reqwest::Url) -> Result<(), String> { + match url.scheme() { + "http" | "https" => Ok(()), + _ => Err("URL must start with http:// or https://".to_string()), + } +} + +async fn fetch_public_url( + mut url: reqwest::Url, + timeout_ms: u64, +) -> Result { + let mut redirects_followed = 0; + loop { + let dns_override = resolve_public_web_fetch_target(&url).await?; + let mut builder = reqwest::Client::builder() + .timeout(Duration::from_millis(timeout_ms)) + .redirect(reqwest::redirect::Policy::none()); + + if let Some((host, addrs)) = &dns_override { + builder = builder.resolve_to_addrs(host, addrs); + } + + let client = builder + .build() + .map_err(|e| format!("Failed to create HTTP client: {}", e))?; + + let response = client + .get(url.clone()) + .header("User-Agent", "CLAI/1.0") + .header("Accept", "text/html, text/plain, application/xhtml+xml") + .send() + .await + .map_err(|e| format!("Fetch failed: {}", e))?; + + if is_followable_redirect(response.status()) { + if redirects_followed >= MAX_WEB_FETCH_REDIRECTS { + return Err("Fetch exceeded redirect limit".to_string()); + } + redirects_followed += 1; + + let location = response + .headers() + .get(reqwest::header::LOCATION) + .ok_or_else(|| { + format!( + "Fetch returned HTTP {} without a Location header", + response.status().as_u16() + ) + })? + .to_str() + .map_err(|_| "Redirect location is not valid UTF-8".to_string())?; + url = url + .join(location) + .map_err(|e| format!("Invalid redirect location: {}", e))?; + validate_web_fetch_scheme(&url)?; + continue; + } + + return Ok(response); + } +} + +fn is_followable_redirect(status: reqwest::StatusCode) -> bool { + matches!(status.as_u16(), 301 | 302 | 303 | 307 | 308) +} + +async fn resolve_public_web_fetch_target( + url: &reqwest::Url, +) -> Result)>, String> { + validate_web_fetch_scheme(url)?; let host = url - .trim_start_matches("https://") - .trim_start_matches("http://") - .split('/') - .next() - .unwrap_or("") - .split(':') - .next() - .unwrap_or(""); - - matches!( - host, - "localhost" | "127.0.0.1" | "0.0.0.0" | "::1" | "[::1]" - ) || host.starts_with("10.") - || host.starts_with("192.168.") - || host.starts_with("172.16.") - || host.starts_with("172.17.") - || host.starts_with("172.18.") - || host.starts_with("172.19.") - || host.starts_with("172.20.") - || host.starts_with("172.21.") - || host.starts_with("172.22.") - || host.starts_with("172.23.") - || host.starts_with("172.24.") - || host.starts_with("172.25.") - || host.starts_with("172.26.") - || host.starts_with("172.27.") - || host.starts_with("172.28.") - || host.starts_with("172.29.") - || host.starts_with("172.30.") - || host.starts_with("172.31.") - || host.ends_with(".local") - || host.ends_with(".internal") + .host_str() + .ok_or_else(|| "URL must include a host".to_string())?; + let host_literal = strip_ipv6_url_brackets(host); + let normalized_host = host_literal.trim_end_matches('.').to_ascii_lowercase(); + if is_blocked_web_fetch_hostname(&normalized_host) { + return Err("Fetching private/local URLs is not allowed".to_string()); + } + + let port = url + .port_or_known_default() + .ok_or_else(|| "URL must include a valid port".to_string())?; + + if let Ok(ip) = host_literal.parse::() { + reject_private_web_fetch_ip(ip)?; + return Ok(None); + } + + let addrs: Vec = tokio::net::lookup_host((host, port)) + .await + .map_err(|e| format!("Failed to resolve URL host: {}", e))? + .collect(); + if addrs.is_empty() { + return Err("URL host did not resolve to any address".to_string()); + } + + reject_private_web_fetch_addrs(&addrs)?; + + // Pin the public DNS result into reqwest for this request so a hostname + // cannot validate as public and then resolve privately during connect. + Ok(Some((host.to_string(), addrs))) +} + +fn is_blocked_web_fetch_hostname(normalized_host: &str) -> bool { + normalized_host == "localhost" + || normalized_host.ends_with(".localhost") + || normalized_host.ends_with(".local") + || normalized_host.ends_with(".internal") +} + +fn strip_ipv6_url_brackets(host: &str) -> &str { + host.strip_prefix('[') + .and_then(|value| value.strip_suffix(']')) + .unwrap_or(host) +} + +fn reject_private_web_fetch_ip(ip: IpAddr) -> Result<(), String> { + if is_private_web_fetch_ip(ip) { + Err("Fetching private/local URLs is not allowed".to_string()) + } else { + Ok(()) + } +} + +fn reject_private_web_fetch_addrs(addrs: &[SocketAddr]) -> Result<(), String> { + for addr in addrs { + reject_private_web_fetch_ip(addr.ip())?; + } + Ok(()) +} + +fn is_private_web_fetch_ip(ip: IpAddr) -> bool { + match ip { + IpAddr::V4(ip) => is_private_web_fetch_ipv4(ip), + IpAddr::V6(ip) => is_private_web_fetch_ipv6(ip), + } +} + +fn is_private_web_fetch_ipv4(ip: Ipv4Addr) -> bool { + let [a, b, c, _] = ip.octets(); + ip.is_private() + || ip.is_loopback() + || ip.is_link_local() + || ip.is_multicast() + || ip.is_broadcast() + || ip.is_documentation() + || ip.is_unspecified() + || (a == 100 && (64..=127).contains(&b)) + || (a == 198 && (b == 18 || b == 19)) + || (a == 192 && b == 0 && c == 0) + || a >= 240 +} + +fn is_private_web_fetch_ipv6(ip: Ipv6Addr) -> bool { + if let Some(mapped) = ip.to_ipv4_mapped() { + return is_private_web_fetch_ipv4(mapped); + } + + let octets = ip.octets(); + let segments = ip.segments(); + ip.is_loopback() + || ip.is_unspecified() + || ip.is_multicast() + || (octets[0] & 0xfe) == 0xfc + || (segments[0] & 0xffc0) == 0xfe80 + || (segments[0] & 0xffc0) == 0xfec0 + || (segments[0] == 0x2001 && segments[1] == 0x0db8) + || segments[0] == 0x2002 +} + +fn web_fetch_body_limit(content_limit: usize) -> usize { + content_limit + .saturating_mul(4) + .min(MAX_WEB_FETCH_BODY_BYTES) +} + +async fn read_limited_web_fetch_body( + response: reqwest::Response, + body_limit: usize, +) -> Result<(String, bool), String> { + let mut body = Vec::with_capacity(body_limit.min(16 * 1024)); + let mut stream = response.bytes_stream(); + + while let Some(chunk) = stream.next().await { + let chunk = chunk.map_err(|e| format!("Failed to read response body: {}", e))?; + if append_limited_web_fetch_body_chunk(&mut body, &chunk, body_limit) { + return Ok((String::from_utf8_lossy(&body).into_owned(), true)); + } + } + + Ok((String::from_utf8_lossy(&body).into_owned(), false)) +} + +fn append_limited_web_fetch_body_chunk(body: &mut Vec, chunk: &[u8], limit: usize) -> bool { + let remaining = limit.saturating_sub(body.len()); + if remaining == 0 { + return true; + } + + if chunk.len() > remaining { + body.extend_from_slice(&chunk[..remaining]); + return true; + } + + body.extend_from_slice(chunk); + false } #[cfg(test)] @@ -2548,21 +2707,87 @@ mod tests { } #[test] - fn is_private_url_blocks_local_addresses() { - assert!(is_private_url("http://localhost/foo")); - assert!(is_private_url("http://127.0.0.1:8080/api")); - assert!(is_private_url("http://192.168.1.1/")); - assert!(is_private_url("http://10.0.0.1/")); - assert!(is_private_url("http://172.16.0.1/")); - assert!(is_private_url("http://myhost.local/")); - assert!(is_private_url("http://service.internal/")); + fn web_fetch_ip_filter_blocks_non_public_ranges() { + for ip in [ + "0.0.0.0", + "10.0.0.1", + "100.64.0.1", + "127.0.0.1", + "169.254.169.254", + "172.16.0.1", + "192.168.1.1", + "192.0.0.1", + "198.18.0.1", + "224.0.0.1", + "240.0.0.1", + "::", + "::1", + "::ffff:127.0.0.1", + "fc00::1", + "fe80::1", + "fec0::1", + "2001:db8::1", + "2002:0a00:0001::1", + ] { + assert!(is_private_web_fetch_ip(ip.parse().unwrap()), "{ip}"); + } } #[test] - fn is_private_url_allows_public_addresses() { - assert!(!is_private_url("https://example.com/")); - assert!(!is_private_url("https://docs.rust-lang.org/")); - assert!(!is_private_url("http://8.8.8.8/")); + fn web_fetch_ip_filter_allows_public_addresses() { + for ip in ["8.8.8.8", "1.1.1.1", "2606:4700:4700::1111"] { + assert!(!is_private_web_fetch_ip(ip.parse().unwrap()), "{ip}"); + } + } + + #[test] + fn web_fetch_blocks_private_dns_results() { + let addrs = [ + SocketAddr::new("93.184.216.34".parse().unwrap(), 80), + SocketAddr::new("192.168.1.1".parse().unwrap(), 80), + ]; + + let err = reject_private_web_fetch_addrs(&addrs).unwrap_err(); + assert!(err.contains("private/local")); + } + + #[test] + fn web_fetch_hostname_filter_blocks_local_names() { + for host in [ + "localhost", + "api.localhost", + "myhost.local", + "service.internal", + ] { + assert!(is_blocked_web_fetch_hostname(host), "{host}"); + } + assert!(!is_blocked_web_fetch_hostname("example.com")); + } + + #[test] + fn parse_web_fetch_url_rejects_non_absolute_http_urls() { + assert!(parse_web_fetch_url("/relative/path").is_err()); + assert!(parse_web_fetch_url("ftp://example.com/file").is_err()); + } + + #[test] + fn web_fetch_body_reader_truncates_chunks_at_limit() { + let mut body = Vec::new(); + + assert!(!append_limited_web_fetch_body_chunk(&mut body, b"hello", 8)); + assert!(append_limited_web_fetch_body_chunk(&mut body, b" world", 8)); + assert_eq!(body, b"hello wo"); + } + + #[test] + fn web_fetch_body_limit_scales_but_stays_bounded() { + assert_eq!(web_fetch_body_limit(0), 0); + assert_eq!(web_fetch_body_limit(1_000), 4_000); + assert_eq!(web_fetch_body_limit(MAX_WEB_FETCH_CONTENT_LIMIT), 400_000); + assert_eq!( + web_fetch_body_limit(MAX_WEB_FETCH_CONTENT_LIMIT * 10), + MAX_WEB_FETCH_BODY_BYTES + ); } #[test] @@ -2632,6 +2857,25 @@ mod tests { assert!(result.unwrap_err().contains("private/local")); } + #[tokio::test] + async fn web_fetch_rejects_private_ipv6_literals() { + let url = parse_web_fetch_url("http://[::1]/admin").unwrap(); + + let err = resolve_public_web_fetch_target(&url).await.unwrap_err(); + assert!(err.contains("private/local")); + } + + #[tokio::test] + async fn web_fetch_rejects_redirect_targets_to_private_urls() { + let start = parse_web_fetch_url("https://example.com/start").unwrap(); + let redirected = start.join("http://127.0.0.1/admin").unwrap(); + + let err = resolve_public_web_fetch_target(&redirected) + .await + .unwrap_err(); + assert!(err.contains("private/local")); + } + // ------------------------------------------------------------------ // enforce_command_policy — agent policy wiring // ------------------------------------------------------------------ From 79b03f0b3749f038cf20387be59fc1395352b39d Mon Sep 17 00:00:00 2001 From: juan <2930882+juacker@users.noreply.github.com> Date: Thu, 2 Jul 2026 16:52:11 +0200 Subject: [PATCH 2/7] fix(workspace): block deletion while runs are active --- src-tauri/src/assistant/repository.rs | 17 ++++++ src-tauri/src/assistant/repository_tests.rs | 61 +++++++++++++++++++++ src-tauri/src/commands/workspace.rs | 10 ++++ 3 files changed, 88 insertions(+) diff --git a/src-tauri/src/assistant/repository.rs b/src-tauri/src/assistant/repository.rs index 02d8dd2..187a533 100644 --- a/src-tauri/src/assistant/repository.rs +++ b/src-tauri/src/assistant/repository.rs @@ -880,6 +880,23 @@ pub async fn session_has_active_run(pool: &DbPool, session_id: &str) -> Result 0) } +/// Whether the workspace database contains any non-terminal run. Workspace +/// deletion uses this as a coarse guard before removing the root directory. +pub async fn workspace_has_active_run(pool: &DbPool) -> Result { + let (count,): (i64,) = sqlx::query_as( + r#" + SELECT COUNT(*) + FROM assistant_runs + WHERE status IN ('"queued"', '"running"', '"waiting_for_tool"') + "#, + ) + .fetch_one(pool) + .await + .map_err(|e| format!("Failed to check workspace for active runs: {}", e))?; + + Ok(count > 0) +} + pub async fn get_active_run( pool: &DbPool, session_id: &str, diff --git a/src-tauri/src/assistant/repository_tests.rs b/src-tauri/src/assistant/repository_tests.rs index b8476a0..71d26e3 100644 --- a/src-tauri/src/assistant/repository_tests.rs +++ b/src-tauri/src/assistant/repository_tests.rs @@ -708,6 +708,67 @@ async fn test_get_active_run_ignores_terminal_runs() { ); } +#[tokio::test] +async fn test_workspace_has_active_run_tracks_any_session() { + let pool = setup_test_pool().await; + + let first = create_session( + &pool, + CreateSessionParams { + kind: SessionKind::Interactive, + title: Some("first".to_string()), + context: sample_context(), + }, + ) + .await + .unwrap(); + let second = create_session( + &pool, + CreateSessionParams { + kind: SessionKind::Interactive, + title: Some("second".to_string()), + context: sample_context(), + }, + ) + .await + .unwrap(); + + assert!(!workspace_has_active_run(&pool).await.unwrap()); + + create_run( + &pool, + CreateRunParams { + session_id: first.id.clone(), + status: RunStatus::Completed, + trigger: RunTrigger::UserMessage, + connection_id: "conn-1".to_string(), + provider_id: "openai".to_string(), + model_id: "gpt-4".to_string(), + usage: None, + error: None, + }, + ) + .await + .unwrap(); + create_run( + &pool, + CreateRunParams { + session_id: second.id.clone(), + status: RunStatus::WaitingForTool, + trigger: RunTrigger::UserMessage, + connection_id: "conn-1".to_string(), + provider_id: "openai".to_string(), + model_id: "gpt-4".to_string(), + usage: None, + error: None, + }, + ) + .await + .unwrap(); + + assert!(workspace_has_active_run(&pool).await.unwrap()); +} + // --------------------------------------------------------------------------- // Run CRUD // --------------------------------------------------------------------------- diff --git a/src-tauri/src/commands/workspace.rs b/src-tauri/src/commands/workspace.rs index 4b9d5d5..89f4b7e 100644 --- a/src-tauri/src/commands/workspace.rs +++ b/src-tauri/src/commands/workspace.rs @@ -2794,6 +2794,16 @@ pub async fn workspace_delete( ) -> Result<(), String> { let workspace_id = resolve_workspace_id(state.inner(), Some(workspace_id))?; + { + let workspace_pool = state.workspace_db(&workspace_id).await?; + if repository::workspace_has_active_run(&workspace_pool).await? { + return Err(format!( + "Workspace {} has an active run. Cancel or wait for it to finish before deleting the workspace.", + workspace_id + )); + } + } + // Remove from the workspace index first; the returned locator carries // the on-disk root we still need below for both config-loading and // recursive removal. Closes the cached sqlx pool too. From 269693dc83253b3cee8f9ee34fbf85587c092c54 Mon Sep 17 00:00:00 2001 From: juan <2930882+juacker@users.noreply.github.com> Date: Thu, 2 Jul 2026 16:54:51 +0200 Subject: [PATCH 3/7] fix(skills): bound git source sync commands --- src-tauri/src/commands/skills.rs | 104 +++++++++++++++++++++++++++++-- 1 file changed, 98 insertions(+), 6 deletions(-) diff --git a/src-tauri/src/commands/skills.rs b/src-tauri/src/commands/skills.rs index e1492d9..807459c 100644 --- a/src-tauri/src/commands/skills.rs +++ b/src-tauri/src/commands/skills.rs @@ -2,8 +2,11 @@ use std::ffi::OsStr; use std::fs; +use std::io::Read; use std::path::{Path, PathBuf}; -use std::process::Command; +use std::process::{Command, Output, Stdio}; +use std::thread; +use std::time::{Duration, Instant}; use serde::{Deserialize, Serialize}; use tauri::State; @@ -15,6 +18,9 @@ use crate::config::{ }; use crate::AppState; +const GIT_SYNC_TIMEOUT: Duration = Duration::from_secs(120); +const GIT_SYNC_POLL_INTERVAL: Duration = Duration::from_millis(50); + /// Removes all workspace-local skill references that belong to the given source. /// /// Walks every `workspace_agents` row whose JSON-array `selected_skill_ids` @@ -517,7 +523,11 @@ fn build_git_command(in_flatpak: bool, current_dir: Option<&Path>) -> Command { if let Some(current_dir) = current_dir { command.arg(format!("--directory={}", current_dir.display())); } - command.arg("--host").arg("git"); + command + .arg("--env=GIT_TERMINAL_PROMPT=0") + .arg("--env=GIT_ASKPASS=true") + .arg("--host") + .arg("git"); command } else { let mut command = Command::new("git"); @@ -525,6 +535,9 @@ fn build_git_command(in_flatpak: bool, current_dir: Option<&Path>) -> Command { command.current_dir(current_dir); } command + .env("GIT_TERMINAL_PROMPT", "0") + .env("GIT_ASKPASS", "true"); + command } } @@ -536,9 +549,7 @@ where let mut command = build_git_command(crate::providers::is_flatpak(), current_dir); command.args(args); - let output = command - .output() - .map_err(|error| format!("Failed to execute git: {}", error))?; + let output = run_git_command_with_timeout(command, GIT_SYNC_TIMEOUT)?; if output.status.success() { return Ok(()); } @@ -555,6 +566,63 @@ where Err(message) } +fn run_git_command_with_timeout(mut command: Command, timeout: Duration) -> Result { + command.stdout(Stdio::piped()).stderr(Stdio::piped()); + let mut child = command + .spawn() + .map_err(|error| format!("Failed to execute git: {}", error))?; + + let mut stdout = child + .stdout + .take() + .ok_or_else(|| "Failed to capture git stdout".to_string())?; + let stdout_reader = thread::spawn(move || { + let mut buf = Vec::new(); + stdout.read_to_end(&mut buf).map(|_| buf) + }); + + let mut stderr = child + .stderr + .take() + .ok_or_else(|| "Failed to capture git stderr".to_string())?; + let stderr_reader = thread::spawn(move || { + let mut buf = Vec::new(); + stderr.read_to_end(&mut buf).map(|_| buf) + }); + + let deadline = Instant::now() + timeout; + loop { + if let Some(status) = child + .try_wait() + .map_err(|error| format!("Failed to wait for git: {}", error))? + { + let stdout = stdout_reader + .join() + .map_err(|_| "Failed to join git stdout reader".to_string())? + .map_err(|error| format!("Failed to read git stdout: {}", error))?; + let stderr = stderr_reader + .join() + .map_err(|_| "Failed to join git stderr reader".to_string())? + .map_err(|error| format!("Failed to read git stderr: {}", error))?; + return Ok(Output { + status, + stdout, + stderr, + }); + } + + if Instant::now() >= deadline { + let _ = child.kill(); + let _ = child.wait(); + let _ = stdout_reader.join(); + let _ = stderr_reader.join(); + return Err(format!("git timed out after {} seconds", timeout.as_secs())); + } + + thread::sleep(GIT_SYNC_POLL_INTERVAL); + } +} + fn remove_git_cache_if_owned(source: &SkillSourceConfig) -> Result<(), String> { let SkillSourceKind::Git { local_path: Some(local_path), @@ -577,12 +645,26 @@ fn remove_git_cache_if_owned(source: &SkillSourceConfig) -> Result<(), String> { mod tests { use super::*; + fn command_env_value<'a>(command: &'a Command, key: &str) -> Option<&'a OsStr> { + command + .get_envs() + .find_map(|(name, value)| (name == OsStr::new(key)).then_some(value).flatten()) + } + #[test] fn build_git_command_native_invokes_git_directly() { let command = build_git_command(false, None); assert_eq!(command.get_program(), OsStr::new("git")); assert_eq!(command.get_args().count(), 0); assert_eq!(command.get_current_dir(), None); + assert_eq!( + command_env_value(&command, "GIT_TERMINAL_PROMPT"), + Some(OsStr::new("0")) + ); + assert_eq!( + command_env_value(&command, "GIT_ASKPASS"), + Some(OsStr::new("true")) + ); } #[test] @@ -600,7 +682,15 @@ mod tests { let command = build_git_command(true, None); assert_eq!(command.get_program(), OsStr::new("flatpak-spawn")); let args: Vec<&OsStr> = command.get_args().collect(); - assert_eq!(args, vec![OsStr::new("--host"), OsStr::new("git")]); + assert_eq!( + args, + vec![ + OsStr::new("--env=GIT_TERMINAL_PROMPT=0"), + OsStr::new("--env=GIT_ASKPASS=true"), + OsStr::new("--host"), + OsStr::new("git") + ] + ); // No cwd requested -> no --directory and no wrapper cwd. assert_eq!(command.get_current_dir(), None); } @@ -617,6 +707,8 @@ mod tests { args, vec![ OsStr::new("--directory=/home/user/.clai/cache/skill-sources/abc"), + OsStr::new("--env=GIT_TERMINAL_PROMPT=0"), + OsStr::new("--env=GIT_ASKPASS=true"), OsStr::new("--host"), OsStr::new("git"), ] From 7be70bd795adc29968f5c37d5cf336a75f68d69a Mon Sep 17 00:00:00 2001 From: juan <2930882+juacker@users.noreply.github.com> Date: Thu, 2 Jul 2026 17:00:12 +0200 Subject: [PATCH 4/7] refactor(runtime): unregister active runs on guard drop --- src-tauri/src/agents/runner.rs | 5 +- src-tauri/src/assistant/engine.rs | 2 - src-tauri/src/assistant/runtime.rs | 89 ++++++++++++++----- .../src/assistant/tools/workspace_tasks.rs | 7 +- src-tauri/src/commands/assistant.rs | 5 +- src-tauri/src/lib.rs | 2 +- src-tauri/tests/cancel_run.rs | 27 +++--- 7 files changed, 92 insertions(+), 45 deletions(-) diff --git a/src-tauri/src/agents/runner.rs b/src-tauri/src/agents/runner.rs index 75e1057..3431ca4 100644 --- a/src-tauri/src/agents/runner.rs +++ b/src-tauri/src/agents/runner.rs @@ -709,7 +709,8 @@ async fn run_scheduled_agent_with_fallback( // synthetic `scheduled:{instance_id}:{uuid}` string, which meant // `assistant_cancel_run(run.id)` couldn't find the token and // scheduled runs were effectively un-cancellable from the UI. - let cancel_token = runtime::register_run(&run.id); + let run_registration = runtime::register_run(&run.id); + let cancel_token = run_registration.token(); let input = RunTurnInput { session_id: session.id.clone(), run_id: Some(run.id.clone()), @@ -720,7 +721,7 @@ async fn run_scheduled_agent_with_fallback( trigger_message_id: None, }; let result = engine::run_session_turn(&deps, input).await; - runtime::unregister_run(&run.id); + drop(run_registration); match result { Ok(()) => { diff --git a/src-tauri/src/assistant/engine.rs b/src-tauri/src/assistant/engine.rs index e70753f..180f358 100644 --- a/src-tauri/src/assistant/engine.rs +++ b/src-tauri/src/assistant/engine.rs @@ -10,7 +10,6 @@ use crate::assistant::providers; use crate::assistant::providers::types::ProviderError; use crate::assistant::repository; use crate::assistant::repository::{CreateMessageParams, CreateRunParams, CreateToolCallParams}; -use crate::assistant::runtime; use crate::assistant::tools::{self, ToolExecutionContext}; use crate::assistant::types::{ AssistantMessage, CompactionTrigger, CompletionRequest, ContentPart, MessageRole, @@ -3153,6 +3152,5 @@ async fn cancel_run( Some(run_id), AssistantUiEvent::RunCancelled { run }, ); - runtime::unregister_run(run_id); Ok(()) } diff --git a/src-tauri/src/assistant/runtime.rs b/src-tauri/src/assistant/runtime.rs index 6ba30b4..3fea7aa 100644 --- a/src-tauri/src/assistant/runtime.rs +++ b/src-tauri/src/assistant/runtime.rs @@ -1,27 +1,72 @@ use std::collections::HashMap; +use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{Mutex, OnceLock}; use tokio_util::sync::CancellationToken; -type ActiveRuns = HashMap; +#[derive(Clone)] +struct ActiveRunEntry { + token: CancellationToken, + generation: u64, +} + +type ActiveRuns = HashMap; static ACTIVE_RUNS: OnceLock> = OnceLock::new(); +static NEXT_GENERATION: AtomicU64 = AtomicU64::new(1); + +pub struct RunRegistration { + run_id: String, + token: CancellationToken, + generation: u64, +} + +impl RunRegistration { + pub fn token(&self) -> CancellationToken { + self.token.clone() + } +} + +impl Drop for RunRegistration { + fn drop(&mut self) { + let mut active = active_runs().lock().unwrap(); + if active + .get(&self.run_id) + .is_some_and(|entry| entry.generation == self.generation) + { + active.remove(&self.run_id); + } + } +} fn active_runs() -> &'static Mutex { ACTIVE_RUNS.get_or_init(|| Mutex::new(HashMap::new())) } -pub fn register_run(run_id: &str) -> CancellationToken { +pub fn register_run(run_id: &str) -> RunRegistration { let token = CancellationToken::new(); - active_runs() - .lock() - .unwrap() - .insert(run_id.to_string(), token.clone()); - token + let generation = NEXT_GENERATION.fetch_add(1, Ordering::Relaxed); + active_runs().lock().unwrap().insert( + run_id.to_string(), + ActiveRunEntry { + token: token.clone(), + generation, + }, + ); + RunRegistration { + run_id: run_id.to_string(), + token, + generation, + } } pub fn cancel_run(run_id: &str) -> bool { - if let Some(token) = active_runs().lock().unwrap().get(run_id).cloned() { + if let Some(token) = active_runs() + .lock() + .unwrap() + .get(run_id) + .map(|entry| entry.token.clone()) + { token.cancel(); return true; } @@ -29,10 +74,6 @@ pub fn cancel_run(run_id: &str) -> bool { false } -pub fn unregister_run(run_id: &str) { - active_runs().lock().unwrap().remove(run_id); -} - #[cfg(test)] mod tests { use super::*; @@ -45,7 +86,8 @@ mod tests { #[test] fn register_then_cancel_propagates_to_token() { let id = "runtime-test-register-then-cancel"; - let token = register_run(id); + let registration = register_run(id); + let token = registration.token(); assert!(!token.is_cancelled(), "fresh token must start uncancelled"); let was_found = cancel_run(id); @@ -54,8 +96,6 @@ mod tests { token.is_cancelled(), "the original token handle must observe the cancel" ); - - unregister_run(id); } #[test] @@ -71,8 +111,8 @@ mod tests { #[test] fn unregister_removes_from_active_set() { let id = "runtime-test-unregister-removes"; - let _token = register_run(id); - unregister_run(id); + let registration = register_run(id); + drop(registration); let was_found = cancel_run(id); assert!(!was_found, "unregistered ids must no longer be cancellable"); @@ -86,8 +126,10 @@ mod tests { // registration wins. Pin the behavior so a future refactor // doesn't accidentally start de-duping or asserting. let id = "runtime-test-double-register"; - let first = register_run(id); - let second = register_run(id); + let first_registration = register_run(id); + let first = first_registration.token(); + let second_registration = register_run(id); + let second = second_registration.token(); // Cancelling now should signal the second (current) token. assert!(cancel_run(id)); @@ -96,6 +138,13 @@ mod tests { // cancel_run never reaches it. assert!(!first.is_cancelled()); - unregister_run(id); + drop(first_registration); + assert!( + cancel_run(id), + "dropping an older guard must not unregister the newer token" + ); + + drop(second_registration); + assert!(!cancel_run(id)); } } diff --git a/src-tauri/src/assistant/tools/workspace_tasks.rs b/src-tauri/src/assistant/tools/workspace_tasks.rs index 1cd127c..9331ec5 100644 --- a/src-tauri/src/assistant/tools/workspace_tasks.rs +++ b/src-tauri/src/assistant/tools/workspace_tasks.rs @@ -382,19 +382,20 @@ fn spawn_task_run( ) .await; - let cancel_token = crate::assistant::runtime::register_run(&run_id); + let run_registration = crate::assistant::runtime::register_run(&run_id); + let cancel_token = run_registration.token(); let input = RunTurnInput { session_id: session_id.clone(), run_id: Some(run_id.clone()), trigger: RunTrigger::WorkspaceTask, connection_id, - cancel_token: cancel_token.clone(), + cancel_token, inter_agent_call_depth: None, trigger_message_id: None, }; let result = engine::run_session_turn(&deps, input).await; - crate::assistant::runtime::unregister_run(&run_id); + drop(run_registration); match result { Ok(()) => { diff --git a/src-tauri/src/commands/assistant.rs b/src-tauri/src/commands/assistant.rs index b40e235..d30eb63 100644 --- a/src-tauri/src/commands/assistant.rs +++ b/src-tauri/src/commands/assistant.rs @@ -925,8 +925,10 @@ pub(crate) fn spawn_run_task( // messages are linked via assistant_message_queue.delivered_run_id. trigger_message_id: Option, ) { - let cancel_token = runtime::register_run(&run_id); + let run_registration = runtime::register_run(&run_id); + let cancel_token = run_registration.token(); tauri::async_runtime::spawn(async move { + let _run_registration = run_registration; let deps = AssistantDeps { pool: pool.clone(), app: app.clone(), @@ -943,7 +945,6 @@ pub(crate) fn spawn_run_task( if let Err(e) = engine::run_session_turn(&deps, input).await { tracing::error!("Assistant engine error for run {}: {}", run_id, e); } - runtime::unregister_run(&run_id); if let Err(e) = start_queued_followup_if_idle(pool.clone(), app.clone(), session_id.clone()).await { diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index dfa44cf..4045b9d 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -56,7 +56,7 @@ pub use workspace_index::WorkspaceIndex; /// module's name signals intent at the use site. #[doc(hidden)] pub mod runtime { - pub use crate::assistant::runtime::{cancel_run, register_run, unregister_run}; + pub use crate::assistant::runtime::{cancel_run, register_run}; } /// Shared application state accessible from all commands. diff --git a/src-tauri/tests/cancel_run.rs b/src-tauri/tests/cancel_run.rs index b414680..84e4c94 100644 --- a/src-tauri/tests/cancel_run.rs +++ b/src-tauri/tests/cancel_run.rs @@ -5,27 +5,26 @@ //! synthetic `scheduled:{instance_id}:{uuid}` key, making scheduled //! runs effectively un-cancellable from the UI. //! -//! These tests use the public `register_run` / `cancel_run` / -//! `unregister_run` surface and simulate the registration calls each -//! path makes. A real end-to-end test would also need an engine turn -//! to consume the cancel token, but the value lives at the key-naming -//! layer: every spawn site must register under run.id. +//! These tests use the public `register_run` / `cancel_run` surface +//! and simulate the registration calls each path makes. A real +//! end-to-end test would also need an engine turn to consume the +//! cancel token, but the value lives at the key-naming layer: every +//! spawn site must register under run.id. -use clai_lib::runtime::{cancel_run, register_run, unregister_run}; +use clai_lib::runtime::{cancel_run, register_run}; #[test] fn chat_path_registers_and_cancels_under_run_id() { // Mimics commands::assistant::spawn_run_task. let run_id = "cancel-test-chat-run-id"; - let token = register_run(run_id); + let registration = register_run(run_id); + let token = registration.token(); assert!(!token.is_cancelled()); // Mimics commands::assistant::assistant_cancel_run, which calls // runtime::cancel_run(&run_id). assert!(cancel_run(run_id)); assert!(token.is_cancelled()); - - unregister_run(run_id); } #[test] @@ -36,13 +35,12 @@ fn scheduler_path_registers_and_cancels_under_run_id() { // test pins that convention so a future refactor can't drift // back to a separate key. let run_id = "cancel-test-scheduler-run-id"; - let token = register_run(run_id); + let registration = register_run(run_id); + let token = registration.token(); assert!(!token.is_cancelled()); assert!(cancel_run(run_id)); assert!(token.is_cancelled()); - - unregister_run(run_id); } #[test] @@ -51,13 +49,12 @@ fn workspace_task_path_registers_and_cancels_under_run_id() { // convention as the other two paths; tested for symmetry so all // three spawn sites are documented to use the DB run.id. let run_id = "cancel-test-workspace-task-run-id"; - let token = register_run(run_id); + let registration = register_run(run_id); + let token = registration.token(); assert!(!token.is_cancelled()); assert!(cancel_run(run_id)); assert!(token.is_cancelled()); - - unregister_run(run_id); } #[test] From b7cdd96ec64d067ae238b6c91c35aedb71a2e023 Mon Sep 17 00:00:00 2001 From: juan <2930882+juacker@users.noreply.github.com> Date: Thu, 2 Jul 2026 17:01:43 +0200 Subject: [PATCH 5/7] fix(import): avoid overwriting file collisions --- src-tauri/src/commands/system_apps.rs | 91 ++++++++++++++++++++++++--- 1 file changed, 83 insertions(+), 8 deletions(-) diff --git a/src-tauri/src/commands/system_apps.rs b/src-tauri/src/commands/system_apps.rs index 9376374..36c3e99 100644 --- a/src-tauri/src/commands/system_apps.rs +++ b/src-tauri/src/commands/system_apps.rs @@ -1,6 +1,8 @@ //! Tauri commands for "open in app" actions and the Settings → //! Applications section. See `crate::system_apps` for the mechanics. +use std::fs::{File, OpenOptions}; +use std::io::{self, Seek}; use std::path::Path; use tauri::State; @@ -109,9 +111,7 @@ pub fn workspace_import_files( .ok_or_else(|| format!("`{}` has no file name.", source.display()))? .to_string_lossy() .to_string(); - let dest = unique_destination(&dest_dir, &name); - std::fs::copy(source, &dest) - .map_err(|e| format!("Failed to copy `{}`: {}", source.display(), e))?; + let dest = copy_to_unique_destination(source, &dest_dir, &name)?; copied.push( dest.file_name() .map(|n| n.to_string_lossy().to_string()) @@ -122,17 +122,60 @@ pub fn workspace_import_files( } /// `report.md` → `report (1).md` → `report (2).md` … until free. -fn unique_destination(dir: &Path, name: &str) -> std::path::PathBuf { - let candidate = dir.join(name); - if !candidate.exists() { - return candidate; +fn destination_candidate(dir: &Path, name: &str, copy_index: u32) -> std::path::PathBuf { + if copy_index == 0 { + return dir.join(name); } let (stem, ext) = match name.rsplit_once('.') { Some((stem, ext)) if !stem.is_empty() => (stem.to_string(), format!(".{}", ext)), _ => (name.to_string(), String::new()), }; + dir.join(format!("{} ({}){}", stem, copy_index, ext)) +} + +fn copy_to_unique_destination( + source: &Path, + dir: &Path, + name: &str, +) -> Result { + let mut input = + File::open(source).map_err(|e| format!("Failed to open `{}`: {}", source.display(), e))?; + + for n in 0u32.. { + let candidate = destination_candidate(dir, name, n); + let mut output = match OpenOptions::new() + .write(true) + .create_new(true) + .open(&candidate) + { + Ok(file) => file, + Err(error) if error.kind() == io::ErrorKind::AlreadyExists => continue, + Err(error) => { + return Err(format!( + "Failed to create `{}`: {}", + candidate.display(), + error + )); + } + }; + + input + .rewind() + .map_err(|e| format!("Failed to rewind `{}`: {}", source.display(), e))?; + if let Err(error) = io::copy(&mut input, &mut output) { + let _ = std::fs::remove_file(&candidate); + return Err(format!("Failed to copy `{}`: {}", source.display(), error)); + } + return Ok(candidate); + } + + unreachable!("u32 exhausted finding a unique file name") +} + +#[cfg(test)] +fn unique_destination(dir: &Path, name: &str) -> std::path::PathBuf { for n in 1u32.. { - let candidate = dir.join(format!("{} ({}){}", stem, n, ext)); + let candidate = destination_candidate(dir, name, n - 1); if !candidate.exists() { return candidate; } @@ -168,4 +211,36 @@ mod tests { dir.path().join("Makefile (1)") ); } + + #[test] + fn copy_to_unique_destination_does_not_overwrite_existing_file() { + let source_dir = tempfile::tempdir().unwrap(); + let dest_dir = tempfile::tempdir().unwrap(); + let source = source_dir.path().join("report.md"); + std::fs::write(&source, "new").unwrap(); + std::fs::write(dest_dir.path().join("report.md"), "old").unwrap(); + + let copied = copy_to_unique_destination(&source, dest_dir.path(), "report.md").unwrap(); + + assert_eq!(copied, dest_dir.path().join("report (1).md")); + assert_eq!( + std::fs::read_to_string(dest_dir.path().join("report.md")).unwrap(), + "old" + ); + assert_eq!(std::fs::read_to_string(copied).unwrap(), "new"); + } + + #[test] + fn copy_to_unique_destination_handles_extensionless_names() { + let source_dir = tempfile::tempdir().unwrap(); + let dest_dir = tempfile::tempdir().unwrap(); + let source = source_dir.path().join("Makefile"); + std::fs::write(&source, "new").unwrap(); + std::fs::write(dest_dir.path().join("Makefile"), "old").unwrap(); + + let copied = copy_to_unique_destination(&source, dest_dir.path(), "Makefile").unwrap(); + + assert_eq!(copied, dest_dir.path().join("Makefile (1)")); + assert_eq!(std::fs::read_to_string(copied).unwrap(), "new"); + } } From aa798536873f8835abfca91f22a83356f90e521f Mon Sep 17 00:00:00 2001 From: juan <2930882+juacker@users.noreply.github.com> Date: Thu, 2 Jul 2026 17:03:30 +0200 Subject: [PATCH 6/7] refactor(compaction): share context-limit recovery helper --- src-tauri/src/assistant/compaction.rs | 17 +++++++++++++++++ src-tauri/src/assistant/engine.rs | 6 ++---- src-tauri/src/assistant/local_agent.rs | 6 ++---- 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/src-tauri/src/assistant/compaction.rs b/src-tauri/src/assistant/compaction.rs index a89b408..af4887a 100644 --- a/src-tauri/src/assistant/compaction.rs +++ b/src-tauri/src/assistant/compaction.rs @@ -214,6 +214,23 @@ pub async fn compact_session_history( })) } +pub async fn compact_for_context_limit_recovery( + pool: &DbPool, + session: &AssistantSession, + connection: &ProviderConnection, + run_id: &str, +) -> Result, String> { + compact_session_history( + pool, + session, + connection, + CompactionTrigger::ErrorRecovery, + Some(run_id), + true, + ) + .await +} + fn provider_history_messages_with_compaction( messages: &[AssistantMessage], latest: Option<&AssistantCompaction>, diff --git a/src-tauri/src/assistant/engine.rs b/src-tauri/src/assistant/engine.rs index 180f358..4982dff 100644 --- a/src-tauri/src/assistant/engine.rs +++ b/src-tauri/src/assistant/engine.rs @@ -318,13 +318,11 @@ pub async fn run_session_turn( && compaction::is_context_limit_error(&e.to_string()) { retried_after_context_compaction = true; - match compaction::compact_session_history( + match compaction::compact_for_context_limit_recovery( &deps.pool, &session, &connection, - CompactionTrigger::ErrorRecovery, - Some(&run_id), - true, + &run_id, ) .await { diff --git a/src-tauri/src/assistant/local_agent.rs b/src-tauri/src/assistant/local_agent.rs index 45bc005..99479fe 100644 --- a/src-tauri/src/assistant/local_agent.rs +++ b/src-tauri/src/assistant/local_agent.rs @@ -425,13 +425,11 @@ pub async fn run_session_turn( "{} reported a context limit; compacting local history and restarting with a fresh session", provider_runtime.display_name() ); - match compaction::compact_session_history( + match compaction::compact_for_context_limit_recovery( &deps.pool, &session, &connection, - CompactionTrigger::ErrorRecovery, - Some(&run_id), - true, + &run_id, ) .await { From fa13b4cee5fd8429ddce452798c328edcef7a7fc Mon Sep 17 00:00:00 2001 From: juan <2930882+juacker@users.noreply.github.com> Date: Thu, 2 Jul 2026 23:53:34 +0200 Subject: [PATCH 7/7] fix(tools): close web_fetch scheme-downgrade + NAT64 SSRF gaps Addresses the two in-scope security minors from the PR #88 static review: - Refuse https->http redirects. fetch_public_url now records whether the original request was https and rejects any redirect that downgrades the transport, so a redirect can't strip TLS. - Handle NAT64 addresses in the IPv6 private check. The well-known prefix 64:ff9b::/96 embeds a target IPv4 in its low 32 bits, so 64:ff9b::7f00:1 would reach 127.0.0.1. Extract the embedded IPv4 and apply the IPv4 blocklist (only the embedded address decides, so public NAT64 targets like 64:ff9b::808:808 still resolve). The local-use prefix 64:ff9b:1::/48 (RFC 8215) is site-internal by definition and is blocked wholesale. Tests: NAT64 embedded-loopback + local-use added to the private-literal set, NAT64 embedded-public added to the allowed set. cargo fmt/clippy/test green (726 lib tests). --- src-tauri/src/assistant/tools/local.rs | 38 +++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/src-tauri/src/assistant/tools/local.rs b/src-tauri/src/assistant/tools/local.rs index 57962ef..5cbd1fe 100644 --- a/src-tauri/src/assistant/tools/local.rs +++ b/src-tauri/src/assistant/tools/local.rs @@ -2082,6 +2082,10 @@ async fn fetch_public_url( mut url: reqwest::Url, timeout_ms: u64, ) -> Result { + // A redirect must never downgrade the transport: if the original request + // was https, refuse to follow a redirect to plain http (a MITM/attacker + // could otherwise strip TLS by 30x-ing to http). + let started_https = url.scheme() == "https"; let mut redirects_followed = 0; loop { let dns_override = resolve_public_web_fetch_target(&url).await?; @@ -2126,6 +2130,11 @@ async fn fetch_public_url( .join(location) .map_err(|e| format!("Invalid redirect location: {}", e))?; validate_web_fetch_scheme(&url)?; + if started_https && url.scheme() != "https" { + return Err( + "Refusing to follow an https->http redirect (transport downgrade)".to_string(), + ); + } continue; } @@ -2231,6 +2240,22 @@ fn is_private_web_fetch_ipv6(ip: Ipv6Addr) -> bool { let octets = ip.octets(); let segments = ip.segments(); + + // NAT64: the well-known prefix 64:ff9b::/96 embeds a target IPv4 in the + // low 32 bits, so `64:ff9b::7f00:1` would reach 127.0.0.1. Extract and + // apply the IPv4 blocklist rather than trusting the v6 literal. The + // local-use prefix 64:ff9b:1::/48 (RFC 8215) is site-internal by + // definition, so block it wholesale. + if segments[0] == 0x0064 && segments[1] == 0xff9b { + if segments[2] == 0x0001 { + return true; + } + if segments[2] == 0 && segments[3] == 0 && segments[4] == 0 && segments[5] == 0 { + let embedded = std::net::Ipv4Addr::new(octets[12], octets[13], octets[14], octets[15]); + return is_private_web_fetch_ipv4(embedded); + } + } + ip.is_loopback() || ip.is_unspecified() || ip.is_multicast() @@ -2728,6 +2753,10 @@ mod tests { "fec0::1", "2001:db8::1", "2002:0a00:0001::1", + // NAT64 well-known prefix embedding 127.0.0.1 (64:ff9b::7f00:1). + "64:ff9b::7f00:1", + // NAT64 local-use prefix 64:ff9b:1::/48 (site-internal by definition). + "64:ff9b:1::1", ] { assert!(is_private_web_fetch_ip(ip.parse().unwrap()), "{ip}"); } @@ -2735,7 +2764,14 @@ mod tests { #[test] fn web_fetch_ip_filter_allows_public_addresses() { - for ip in ["8.8.8.8", "1.1.1.1", "2606:4700:4700::1111"] { + for ip in [ + "8.8.8.8", + "1.1.1.1", + "2606:4700:4700::1111", + // NAT64 well-known prefix embedding a public IPv4 (8.8.8.8) must + // still be reachable — only the embedded IPv4 decides. + "64:ff9b::808:808", + ] { assert!(!is_private_web_fetch_ip(ip.parse().unwrap()), "{ip}"); } }