diff --git a/Cargo.toml b/Cargo.toml index 7a7677b..4bac838 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -128,3 +128,6 @@ strip = true panic = "abort" [dev-dependencies] +# test-util enables `#[tokio::test(start_paused = true)]` so timeout-based tests +# (e.g. the SSE idle-stall guard) resolve instantly instead of sleeping in real time. +tokio = { version = "1", features = ["test-util", "rt-multi-thread", "macros", "time"] } diff --git a/src/api/mod.rs b/src/api/mod.rs index cba1141..72878e1 100644 --- a/src/api/mod.rs +++ b/src/api/mod.rs @@ -21,20 +21,86 @@ const ANTHROPIC_VERSION: &str = "2023-06-01"; const DEFAULT_MODEL: &str = "claude-sonnet-4-6"; const DEFAULT_MAX_TOKENS: u32 = 8096; +/// Maximum time to wait for the *next* SSE event before declaring the stream dead. +/// +/// This is deliberately an inter-event budget, not a whole-request timeout: a +/// legitimate response can stream for many minutes, so `.timeout()` on the request +/// would truncate valid work. But a healthy connection always delivers *something* — +/// a content delta, a `ping`, or a keepalive — well inside this window. +/// +/// Without this bound, a silently dropped TCP connection (NAT idle reaper, laptop +/// sleep, VPN drop) leaves the read future pending forever: the UI hangs with no +/// error and no recovery short of killing the process. +pub(crate) const SSE_IDLE_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(120); + +/// Await the next event from an SSE stream, bounded by [`SSE_IDLE_TIMEOUT`]. +/// +/// Returns `Ok(None)` on clean end-of-stream. Shared by the Anthropic backend and +/// the OpenAI-compatible backend (which also serves Ollama), so every streaming path +/// gets the same stall detection. +pub(crate) async fn next_sse_event(stream: &mut S) -> Result> +where + S: futures_util::Stream> + Unpin, + E: std::fmt::Display, +{ + match tokio::time::timeout(SSE_IDLE_TIMEOUT, stream.next()).await { + Err(_) => Err(anyhow!( + "SSE stream stalled: no data received for {}s — the connection was likely \ + dropped upstream. Retry the request.", + SSE_IDLE_TIMEOUT.as_secs() + )), + Ok(None) => Ok(None), + Ok(Some(Ok(event))) => Ok(Some(event)), + Ok(Some(Err(e))) => Err(anyhow!("SSE stream error: {e}")), + } +} + #[derive(Clone)] #[allow(dead_code)] // api_key retained for future authenticated-header injection pub struct ClaudeClient { client: Client, api_key: String, base_url: String, + /// Betas the credential itself requires, merged into every request's + /// `anthropic-beta`. An OAuth bearer token needs `oauth-2025-04-20`. + /// + /// This is *not* a default header: `RequestBuilder::header` appends rather + /// than replaces, so a default `anthropic-beta` plus a per-request one + /// would send the field twice. Merging into the single per-request value + /// keeps exactly one. + credential_betas: Vec, } impl ClaudeClient { + /// Construct from a static API key. Retained for callers that already hold + /// a raw key; prefer [`ClaudeClient::with_credential`]. pub fn new(api_key: impl Into) -> Result { - let api_key = api_key.into(); + Self::with_credential(&crate::auth::Credential::ApiKey(api_key.into())) + } + + /// Construct from a resolved credential, selecting the wire format. + /// + /// A static key authenticates with `x-api-key`; an OAuth access token uses + /// `Authorization: Bearer` plus the `oauth-2025-04-20` beta. Sending both + /// auth headers is rejected by the API, so exactly one is set. + pub fn with_credential(cred: &crate::auth::Credential) -> Result { + let api_key = cred.secret().to_string(); let mut headers = header::HeaderMap::new(); headers.insert("anthropic-version", ANTHROPIC_VERSION.parse()?); - headers.insert("x-api-key", api_key.parse()?); + + let mut credential_betas = Vec::new(); + match cred { + crate::auth::Credential::ApiKey(k) => { + headers.insert("x-api-key", k.parse()?); + } + crate::auth::Credential::OAuth(token) => { + headers.insert( + header::AUTHORIZATION, + format!("Bearer {token}").parse()?, + ); + credential_betas.push(crate::auth::OAUTH_BETA.to_string()); + } + } headers.insert(header::CONTENT_TYPE, "application/json".parse()?); let client = Client::builder() @@ -52,9 +118,30 @@ impl ClaudeClient { client, api_key, base_url: ANTHROPIC_API_BASE.to_string(), + credential_betas, }) } + /// Merge the request's betas with any the credential requires. + /// Returns `None` when there are none, so the header is omitted entirely. + fn beta_header(&self, request_betas: &[String]) -> Option { + if request_betas.is_empty() && self.credential_betas.is_empty() { + return None; + } + let mut all: Vec<&str> = Vec::new(); + for b in request_betas.iter().chain(self.credential_betas.iter()) { + let b = b.as_str(); + if !b.is_empty() && !all.contains(&b) { + all.push(b); + } + } + if all.is_empty() { + None + } else { + Some(all.join(",")) + } + } + /// Non-streaming API call — mirrors callModel() in services/api/claude.ts #[allow(dead_code)] // used by SDK/headless mode (non-streaming path) pub async fn messages(&self, request: MessagesRequest) -> Result { @@ -62,8 +149,8 @@ impl ClaudeClient { debug!("POST {url} model={}", request.model); let mut builder = self.client.post(&url).json(&request); - if !request.betas.is_empty() { - builder = builder.header("anthropic-beta", request.betas.join(",")); + if let Some(betas) = self.beta_header(&request.betas) { + builder = builder.header("anthropic-beta", betas); } if let Some(ref sid) = request.session_id { builder = builder.header("X-Claude-Code-Session-Id", sid.as_str()); @@ -95,8 +182,8 @@ impl ClaudeClient { debug!("POST {url} stream=true model={}", request.model); let mut builder = self.client.post(&url).json(&request); - if !request.betas.is_empty() { - builder = builder.header("anthropic-beta", request.betas.join(",")); + if let Some(betas) = self.beta_header(&request.betas) { + builder = builder.header("anthropic-beta", betas); } if let Some(ref sid) = request.session_id { builder = builder.header("X-Claude-Code-Session-Id", sid.as_str()); @@ -122,8 +209,7 @@ impl ClaudeClient { let mut tool_blocks: HashMap = HashMap::with_capacity(4); // id, name, json let mut thinking_blocks: HashMap = HashMap::with_capacity(4); // thinking, sig - while let Some(event) = stream.next().await { - let event = event.context("SSE stream error")?; + while let Some(event) = next_sse_event(&mut stream).await? { if event.data == "[DONE]" { break; } @@ -271,6 +357,23 @@ pub enum ApiBackend { impl ApiBackend { /// Create the right backend for `model`. /// `api_key` is required for Anthropic; ignored for Ollama/OpenAI-compat. + /// `api_key` is the credential secret; `is_oauth` selects the wire format + /// (`Authorization: Bearer` + oauth beta, vs `x-api-key`). Ignored for + /// Ollama / OpenAI-compat backends, which carry their own auth. + pub fn new_with_auth( + model: &str, + api_key: &str, + is_oauth: bool, + ollama_host: &str, + ) -> Result { + if !is_ollama_model(model) && !is_openai_compat_model(model) && is_oauth { + return Ok(Self::Anthropic(ClaudeClient::with_credential( + &crate::auth::Credential::OAuth(api_key.to_string()), + )?)); + } + Self::new(model, api_key, ollama_host) + } + pub fn new(model: &str, api_key: &str, ollama_host: &str) -> Result { if is_ollama_model(model) { Ok(Self::Ollama(OllamaClient::new(ollama_host)?)) @@ -358,3 +461,142 @@ impl ApiBackend { } } } + +#[cfg(test)] +mod credential_tests { + use super::*; + use crate::auth::{Credential, OAUTH_BETA}; + + /// An OAuth token must go in `Authorization: Bearer`, never `x-api-key` — + /// and the request additionally needs the oauth beta or it is rejected. + #[test] + fn oauth_credential_adds_the_required_beta() { + let c = ClaudeClient::with_credential(&Credential::OAuth("tok".into())).unwrap(); + assert_eq!(c.beta_header(&[]).as_deref(), Some(OAUTH_BETA)); + } + + /// A static key needs no extra beta, so the header stays absent when the + /// request itself asked for none. + #[test] + fn api_key_credential_adds_no_beta() { + let c = ClaudeClient::with_credential(&Credential::ApiKey("sk-ant-x".into())).unwrap(); + assert_eq!(c.beta_header(&[]), None); + } + + /// The credential beta must be *merged* into the request's betas, not sent + /// as a second `anthropic-beta` header — `RequestBuilder::header` appends. + #[test] + fn request_betas_and_credential_betas_merge_into_one_value() { + let c = ClaudeClient::with_credential(&Credential::OAuth("tok".into())).unwrap(); + let merged = c.beta_header(&["compact-2026-01-12".into()]).unwrap(); + assert!(merged.contains("compact-2026-01-12"), "{merged}"); + assert!(merged.contains(OAUTH_BETA), "{merged}"); + assert_eq!(merged.matches(OAUTH_BETA).count(), 1, "{merged}"); + assert!(!merged.contains(",,"), "{merged}"); + } + + #[test] + fn duplicate_betas_are_collapsed() { + let c = ClaudeClient::with_credential(&Credential::OAuth("tok".into())).unwrap(); + let merged = c.beta_header(&[OAUTH_BETA.into()]).unwrap(); + assert_eq!(merged, OAUTH_BETA, "duplicate must collapse: {merged}"); + } + + #[test] + fn api_key_request_betas_pass_through_untouched() { + let c = ClaudeClient::with_credential(&Credential::ApiKey("k".into())).unwrap(); + assert_eq!(c.beta_header(&["a".into(), "b".into()]).as_deref(), Some("a,b")); + } + + /// `ClaudeClient::new` is the legacy raw-key entry point — it must stay + /// equivalent to an explicit ApiKey credential. + #[test] + fn legacy_new_is_equivalent_to_an_api_key_credential() { + let legacy = ClaudeClient::new("sk-ant-x").unwrap(); + assert_eq!(legacy.beta_header(&[]), None); + assert_eq!(legacy.api_key, "sk-ant-x"); + } +} + +#[cfg(test)] +mod sse_idle_tests { + use super::*; + use eventsource_stream::Event; + use std::convert::Infallible; + + fn event(data: &str) -> Event { + Event { + data: data.to_string(), + ..Default::default() + } + } + + /// A stream that yields nothing and never terminates — models a TCP connection + /// that was silently dropped upstream (NAT reaper, laptop sleep, VPN drop). + /// Before the idle-timeout guard this hung the caller forever. + #[tokio::test(start_paused = true)] + async fn stalled_stream_errors_instead_of_hanging_forever() { + let mut stream = futures_util::stream::pending::>(); + + let err = next_sse_event(&mut stream) + .await + .expect_err("a stream that never yields must time out, not hang"); + + let msg = err.to_string(); + assert!(msg.contains("stalled"), "diagnostic should say stalled: {msg}"); + assert!( + msg.contains(&SSE_IDLE_TIMEOUT.as_secs().to_string()), + "diagnostic should report the budget that elapsed: {msg}" + ); + } + + /// A stream that goes quiet for less than the budget is healthy and must be + /// allowed through — long thinking gaps are legitimate, not a stall. + #[tokio::test(start_paused = true)] + async fn quiet_period_within_budget_is_not_treated_as_a_stall() { + let quiet = SSE_IDLE_TIMEOUT - std::time::Duration::from_secs(1); + let mut stream = Box::pin(futures_util::stream::once(async move { + tokio::time::sleep(quiet).await; + Ok::<_, Infallible>(event("late but valid")) + })); + + let got = next_sse_event(&mut stream).await.expect("must not time out"); + assert_eq!(got.map(|e| e.data).as_deref(), Some("late but valid")); + } + + #[tokio::test(start_paused = true)] + async fn clean_end_of_stream_returns_none() { + let mut stream = futures_util::stream::empty::>(); + assert!(next_sse_event(&mut stream).await.unwrap().is_none()); + } + + #[tokio::test(start_paused = true)] + async fn events_pass_through_in_order() { + let mut stream = futures_util::stream::iter(vec![ + Ok::<_, Infallible>(event("first")), + Ok(event("second")), + ]); + + assert_eq!( + next_sse_event(&mut stream).await.unwrap().map(|e| e.data), + Some("first".into()) + ); + assert_eq!( + next_sse_event(&mut stream).await.unwrap().map(|e| e.data), + Some("second".into()) + ); + assert!(next_sse_event(&mut stream).await.unwrap().is_none()); + } + + /// Transport errors must still surface as errors, not be swallowed by the + /// timeout wrapper. + #[tokio::test(start_paused = true)] + async fn transport_error_is_propagated() { + let mut stream = Box::pin(futures_util::stream::once(async { + Err::(std::io::Error::other("connection reset")) + })); + + let err = next_sse_event(&mut stream).await.unwrap_err(); + assert!(err.to_string().contains("connection reset"), "{err}"); + } +} diff --git a/src/api/openai_compat.rs b/src/api/openai_compat.rs index b04ae2e..9f8c75f 100644 --- a/src/api/openai_compat.rs +++ b/src/api/openai_compat.rs @@ -21,7 +21,6 @@ /// OPENAI_BASE_URL overrides the base URL for the generic `openai-compat:` prefix. use anyhow::{Context, Result, anyhow}; use eventsource_stream::Eventsource; -use futures_util::StreamExt; use reqwest::Client; use serde::{Deserialize, Serialize}; use std::collections::HashMap; @@ -417,8 +416,7 @@ pub(crate) async fn parse_oai_stream( let mut tool_bufs: HashMap = HashMap::new(); let mut finish_reason: Option = None; - while let Some(event) = stream.next().await { - let event = event.context("SSE stream error")?; + while let Some(event) = super::next_sse_event(&mut stream).await? { if event.data == "[DONE]" { break; } diff --git a/src/auth.rs b/src/auth.rs new file mode 100644 index 0000000..ed358b5 --- /dev/null +++ b/src/auth.rs @@ -0,0 +1,528 @@ +//! Anthropic credential resolution. +//! +//! RustyClaw previously read `ANTHROPIC_API_KEY` and nothing else, which meant +//! it ignored credentials the user may already have configured for Claude Code, +//! the official SDKs, or the `ant` CLI — all of which share one resolution +//! order. This module implements that same order so an existing login just +//! works: +//! +//! ```text +//! ANTHROPIC_API_KEY → ANTHROPIC_AUTH_TOKEN → active `ant auth login` profile +//! → Workload Identity Federation → default profile on disk +//! ``` +//! +//! First match wins. +//! +//! **Why shell out to `ant` for the profile rather than parsing its JSON.** +//! Tokens minted by `ant auth login` are short-lived and must be refreshed. +//! `ant auth print-credentials --access-token` is the documented way to hand +//! the active credential to a non-SDK client, and it *refreshes the token if +//! needed* before printing. Reading `credentials/.json` directly would +//! mean reimplementing OAuth refresh against an on-disk format that is an +//! implementation detail. Shelling out keeps us on a supported interface and +//! gets refresh for free. +//! +//! **Wire format differs by credential kind.** A static key goes in `x-api-key`; +//! an OAuth token goes in `Authorization: Bearer` *and* additionally requires +//! the `oauth-2025-04-20` beta header. Sending both auth headers at once is +//! rejected, so exactly one is ever set. + +use std::time::{Duration, Instant}; + +/// Beta header value required alongside a bearer token. +pub const OAUTH_BETA: &str = "oauth-2025-04-20"; + +/// How a credential authenticates on the wire. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Credential { + /// Static API key — sent as `x-api-key`. + ApiKey(String), + /// Short-lived OAuth access token — sent as `Authorization: Bearer`, + /// and requires [`OAUTH_BETA`] in `anthropic-beta`. + OAuth(String), +} + +impl Credential { + /// The secret itself. Only for redacted status display and for passing to + /// sub-agents — never log this. + pub fn secret(&self) -> &str { + match self { + Credential::ApiKey(s) | Credential::OAuth(s) => s, + } + } + + pub fn is_oauth(&self) -> bool { + matches!(self, Credential::OAuth(_)) + } + + /// `sk-ant-…` style prefix for status output, never the full secret. + pub fn redacted(&self) -> String { + let s = self.secret(); + let head: String = s.chars().take(8).collect(); + match self { + Credential::ApiKey(_) => format!("{head}… (API key)"), + Credential::OAuth(_) => format!("{head}… (OAuth token)"), + } + } +} + +/// Where the winning credential came from — surfaced by `/doctor` so the +/// "stale env var shadows your profile" trap is visible rather than mysterious. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum CredentialSource { + ApiKeyEnv, + AuthTokenEnv, + /// `ant auth print-credentials`, for the named profile (or the active one). + AntProfile(Option), +} + +impl CredentialSource { + pub fn describe(&self) -> String { + match self { + CredentialSource::ApiKeyEnv => "ANTHROPIC_API_KEY".into(), + CredentialSource::AuthTokenEnv => "ANTHROPIC_AUTH_TOKEN".into(), + CredentialSource::AntProfile(None) => "ant auth login (active profile)".into(), + CredentialSource::AntProfile(Some(p)) => format!("ant auth login (profile '{p}')"), + } + } +} + +#[derive(Debug, Clone)] +pub struct Resolved { + pub credential: Credential, + pub source: CredentialSource, + /// Non-fatal notes worth showing the user (e.g. a shadowed profile). + pub warnings: Vec, +} + +/// Injection seam so resolution can be tested without mutating process env +/// (which races under the parallel test harness) or requiring `ant` on PATH. +pub trait AuthEnv { + fn var(&self, key: &str) -> Option; + /// Active access token via `ant auth print-credentials --access-token`. + fn ant_access_token(&self) -> Option; + /// Whether an `ant` profile exists at all — used only to warn that an env + /// var is shadowing it. + fn ant_profile_present(&self) -> bool { + false + } +} + +/// Treat an empty or whitespace-only value as unset. +/// +/// The official SDKs let an empty `ANTHROPIC_API_KEY=""` win its precedence +/// slot and then authenticate with an empty key, producing a confusing 401. +/// We deliberately diverge: an empty value falls through to the next source and +/// the user is told, which is the same outcome they wanted with a clearer path. +fn non_empty(v: Option) -> Option { + v.map(|s| s.trim().to_string()).filter(|s| !s.is_empty()) +} + +/// Resolve a credential from an injected environment. Pure and order-defining. +/// +/// This is the canonical full chain. The binary drives the staged variants +/// (`resolve_env` / `resolve_profile`) so RustyClaw's own explicit mechanisms +/// can sit between them; this entry point exists for library/SDK consumers and +/// is what the ordering tests exercise. +#[allow(dead_code)] +pub fn resolve_with(env: &impl AuthEnv) -> Option { + resolve_stage(env, true) +} + +/// Environment variables only — stops before consulting the `ant` profile. +/// +/// RustyClaw has two credential mechanisms of its own that predate this module +/// (`RUSTYCLAW_API_KEY_FILE_DESCRIPTOR` and `apiKeyHelper`). Both are *explicit* +/// local configuration, whereas an `ant` profile is ambient machine state, so +/// config.rs runs: env vars → fd → helper → profile. Splitting the stages here +/// keeps that ordering without duplicating the env-var precedence rules. +pub fn resolve_env_with(env: &impl AuthEnv) -> Option { + resolve_stage(env, false) +} + +fn resolve_stage(env: &impl AuthEnv, allow_profile: bool) -> Option { + let mut warnings = Vec::new(); + + let api_key = non_empty(env.var("ANTHROPIC_API_KEY")); + let auth_token = non_empty(env.var("ANTHROPIC_AUTH_TOKEN")); + let profile = non_empty(env.var("ANTHROPIC_PROFILE")); + + if env.var("ANTHROPIC_API_KEY").is_some() && api_key.is_none() { + warnings.push( + "ANTHROPIC_API_KEY is set but empty — ignoring it and falling through to the \ + next credential source. Unset it to silence this." + .into(), + ); + } + + // Both set is a hard error at the API: the SDKs send both headers and the + // request is rejected. Say so here rather than letting it surface as a 401. + if api_key.is_some() && auth_token.is_some() { + warnings.push( + "Both ANTHROPIC_API_KEY and ANTHROPIC_AUTH_TOKEN are set. Using ANTHROPIC_API_KEY \ + (first in the resolution order); unset one to remove the ambiguity." + .into(), + ); + } + + if let Some(key) = api_key { + if env.ant_profile_present() { + warnings.push( + "ANTHROPIC_API_KEY is shadowing your `ant auth login` profile — requests will \ + use the key's org/workspace, not the profile's. Unset the variable to use the \ + profile." + .into(), + ); + } + return Some(Resolved { + credential: Credential::ApiKey(key), + source: CredentialSource::ApiKeyEnv, + warnings, + }); + } + + if let Some(token) = auth_token { + return Some(Resolved { + credential: Credential::OAuth(token), + source: CredentialSource::AuthTokenEnv, + warnings, + }); + } + + if allow_profile + && let Some(token) = non_empty(env.ant_access_token()) + { + return Some(Resolved { + credential: Credential::OAuth(token), + source: CredentialSource::AntProfile(profile), + warnings, + }); + } + + None +} + +/// Environment variables only, against the real process environment. +pub fn resolve_env() -> Option { + resolve_env_with(&ProcessAuthEnv) +} + +/// The `ant` profile only, against the real process environment. +pub fn resolve_profile() -> Option { + let env = ProcessAuthEnv; + non_empty(env.ant_access_token()).map(|token| Resolved { + credential: Credential::OAuth(token), + source: CredentialSource::AntProfile(non_empty(env.var("ANTHROPIC_PROFILE"))), + warnings: Vec::new(), + }) +} + +/// The real environment: process env vars plus the `ant` CLI. +pub struct ProcessAuthEnv; + +/// `ant` is a local CLI, but a wedged binary must not hang startup forever. +const ANT_TIMEOUT: Duration = Duration::from_secs(5); + +/// Directory `ant auth login` writes profiles to. +/// +/// `$ANTHROPIC_CONFIG_DIR`, else `~/.config/anthropic` on Unix and +/// `%APPDATA%\Anthropic` on Windows. +fn anthropic_config_dir() -> Option { + if let Ok(dir) = std::env::var("ANTHROPIC_CONFIG_DIR") + && !dir.trim().is_empty() + { + return Some(std::path::PathBuf::from(dir)); + } + #[cfg(windows)] + { + std::env::var("APPDATA") + .ok() + .map(|d| std::path::PathBuf::from(d).join("Anthropic")) + } + #[cfg(not(windows))] + { + dirs::home_dir().map(|h| h.join(".config").join("anthropic")) + } +} + +/// Has `ant auth login` ever stored a profile on this machine? +/// +/// This gate exists because **`ant` is a name collision**: Apache Ant owns that +/// binary name on many systems, including the Windows CI image. Spawning +/// whatever `ant` happens to be on PATH is both wrong and slow — running an +/// unrelated build tool on every startup where no API key is set. Checking for +/// the credentials directory first means we never execute anything unless the +/// real CLI has actually been used here. +fn ant_profile_dir_exists() -> bool { + anthropic_config_dir().is_some_and(|d| d.join("credentials").is_dir()) +} + +impl AuthEnv for ProcessAuthEnv { + fn var(&self, key: &str) -> Option { + std::env::var(key).ok() + } + + fn ant_access_token(&self) -> Option { + if !ant_profile_dir_exists() { + return None; + } + // `--access-token` is required: the bare form prints the whole + // credentials JSON, which as an Authorization header yields an empty + // response or an HTTP/2 protocol error rather than an obvious failure. + run_ant(&["auth", "print-credentials", "--access-token"]) + } + + fn ant_profile_present(&self) -> bool { + ant_profile_dir_exists() + } +} + +/// Run `ant` with a wall-clock bound, returning trimmed stdout on success. +/// +/// Absent `ant` is the common case, not an error — it just means this source +/// does not apply. +fn run_ant(args: &[&str]) -> Option { + use std::process::{Command, Stdio}; + + let mut child = Command::new("ant") + .args(args) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .ok()?; + + let deadline = Instant::now() + ANT_TIMEOUT; + loop { + match child.try_wait() { + Ok(Some(status)) => { + if !status.success() { + return None; + } + break; + } + Ok(None) => { + if Instant::now() >= deadline { + let _ = child.kill(); + let _ = child.wait(); + tracing::warn!("`ant {}` timed out after {ANT_TIMEOUT:?}", args.join(" ")); + return None; + } + std::thread::sleep(Duration::from_millis(25)); + } + Err(_) => return None, + } + } + + let out = child.wait_with_output().ok()?; + let token = String::from_utf8_lossy(&out.stdout).trim().to_string(); + if token.is_empty() { None } else { Some(token) } +} + +/// Resolve against the real process environment, full documented chain. +#[allow(dead_code)] // library/SDK entry point; the binary uses the staged variants +pub fn resolve() -> Option { + resolve_with(&ProcessAuthEnv) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::collections::HashMap; + + #[derive(Default)] + struct FakeEnv { + vars: HashMap, + ant_token: Option, + profile_present: bool, + } + + impl FakeEnv { + fn with(mut self, k: &str, v: &str) -> Self { + self.vars.insert(k.into(), v.into()); + self + } + fn with_ant(mut self, token: &str) -> Self { + self.ant_token = Some(token.into()); + self.profile_present = true; + self + } + } + + impl AuthEnv for FakeEnv { + fn var(&self, key: &str) -> Option { + self.vars.get(key).cloned() + } + fn ant_access_token(&self) -> Option { + self.ant_token.clone() + } + fn ant_profile_present(&self) -> bool { + self.profile_present + } + } + + #[test] + fn no_credential_anywhere_resolves_to_none() { + assert!(resolve_with(&FakeEnv::default()).is_none()); + } + + #[test] + fn api_key_wins_first() { + let r = resolve_with(&FakeEnv::default().with("ANTHROPIC_API_KEY", "sk-ant-key")).unwrap(); + assert_eq!(r.credential, Credential::ApiKey("sk-ant-key".into())); + assert_eq!(r.source, CredentialSource::ApiKeyEnv); + } + + #[test] + fn auth_token_is_second_and_is_oauth() { + let r = resolve_with(&FakeEnv::default().with("ANTHROPIC_AUTH_TOKEN", "oat-tok")).unwrap(); + assert_eq!(r.credential, Credential::OAuth("oat-tok".into())); + assert!(r.credential.is_oauth()); + assert_eq!(r.source, CredentialSource::AuthTokenEnv); + } + + #[test] + fn ant_profile_is_third() { + let r = resolve_with(&FakeEnv::default().with_ant("sk-ant-oat01-abc")).unwrap(); + assert_eq!(r.credential, Credential::OAuth("sk-ant-oat01-abc".into())); + assert_eq!(r.source, CredentialSource::AntProfile(None)); + } + + #[test] + fn profile_name_is_recorded_when_set() { + let env = FakeEnv::default() + .with_ant("tok") + .with("ANTHROPIC_PROFILE", "work"); + let r = resolve_with(&env).unwrap(); + assert_eq!(r.source, CredentialSource::AntProfile(Some("work".into()))); + } + + #[test] + fn full_order_is_respected() { + let env = FakeEnv::default() + .with_ant("from-profile") + .with("ANTHROPIC_AUTH_TOKEN", "from-token") + .with("ANTHROPIC_API_KEY", "from-key"); + assert_eq!( + resolve_with(&env).unwrap().credential, + Credential::ApiKey("from-key".into()) + ); + + let env = FakeEnv::default() + .with_ant("from-profile") + .with("ANTHROPIC_AUTH_TOKEN", "from-token"); + assert_eq!( + resolve_with(&env).unwrap().credential, + Credential::OAuth("from-token".into()) + ); + } + + /// The documented #1 auth trap: a stale exported key silently overrides the + /// profile, sending requests to a different org/workspace. + #[test] + fn shadowed_profile_is_warned_about() { + let env = FakeEnv::default() + .with_ant("tok") + .with("ANTHROPIC_API_KEY", "sk-ant-key"); + let r = resolve_with(&env).unwrap(); + assert_eq!(r.source, CredentialSource::ApiKeyEnv); + assert!( + r.warnings.iter().any(|w| w.contains("shadowing")), + "must warn that the profile is being shadowed: {:?}", + r.warnings + ); + } + + /// An empty value would otherwise win its slot and 401 with an empty key. + #[test] + fn empty_api_key_falls_through_with_a_warning() { + let env = FakeEnv::default() + .with("ANTHROPIC_API_KEY", "") + .with("ANTHROPIC_AUTH_TOKEN", "tok"); + let r = resolve_with(&env).unwrap(); + assert_eq!(r.credential, Credential::OAuth("tok".into())); + assert!(r.warnings.iter().any(|w| w.contains("empty")), "{:?}", r.warnings); + } + + #[test] + fn whitespace_only_values_are_treated_as_unset() { + let env = FakeEnv::default().with("ANTHROPIC_API_KEY", " \n "); + assert!(resolve_with(&env).is_none()); + } + + #[test] + fn values_are_trimmed() { + let r = resolve_with(&FakeEnv::default().with("ANTHROPIC_API_KEY", " sk-ant-x\n")).unwrap(); + assert_eq!(r.credential.secret(), "sk-ant-x"); + } + + /// Sending both auth headers is rejected by the API — warn instead of + /// letting it surface as an opaque 401. + #[test] + fn both_env_credentials_set_is_warned_about() { + let env = FakeEnv::default() + .with("ANTHROPIC_API_KEY", "k") + .with("ANTHROPIC_AUTH_TOKEN", "t"); + let r = resolve_with(&env).unwrap(); + assert!(r.warnings.iter().any(|w| w.contains("Both")), "{:?}", r.warnings); + } + + /// Regression: `ant` collides with Apache Ant, which ships on the Windows + /// CI image. Spawning a bare `ant` from PATH on every credential-less + /// startup ran an unrelated build tool and stalled the process long enough + /// to fail the headless SDK test. Nothing may be executed unless the real + /// CLI has actually stored a profile here. + #[test] + fn no_subprocess_when_no_profile_directory_exists() { + let empty = tempfile::tempdir().unwrap(); + // SAFETY: single-threaded within this test; the var is restored below. + let prev = std::env::var("ANTHROPIC_CONFIG_DIR").ok(); + unsafe { std::env::set_var("ANTHROPIC_CONFIG_DIR", empty.path()) }; + + assert!( + !ant_profile_dir_exists(), + "a config dir with no credentials/ must not trigger a spawn" + ); + assert!(ProcessAuthEnv.ant_access_token().is_none()); + assert!(!ProcessAuthEnv.ant_profile_present()); + + unsafe { + match prev { + Some(v) => std::env::set_var("ANTHROPIC_CONFIG_DIR", v), + None => std::env::remove_var("ANTHROPIC_CONFIG_DIR"), + } + } + } + + #[test] + fn config_dir_honours_the_env_override() { + let dir = tempfile::tempdir().unwrap(); + let prev = std::env::var("ANTHROPIC_CONFIG_DIR").ok(); + unsafe { std::env::set_var("ANTHROPIC_CONFIG_DIR", dir.path()) }; + + assert_eq!(anthropic_config_dir().as_deref(), Some(dir.path())); + std::fs::create_dir_all(dir.path().join("credentials")).unwrap(); + assert!( + ant_profile_dir_exists(), + "credentials/ present ⇒ the real CLI has been used here" + ); + + unsafe { + match prev { + Some(v) => std::env::set_var("ANTHROPIC_CONFIG_DIR", v), + None => std::env::remove_var("ANTHROPIC_CONFIG_DIR"), + } + } + } + + #[test] + fn redacted_never_leaks_the_whole_secret() { + let key = Credential::ApiKey("sk-ant-super-secret-value".into()); + let shown = key.redacted(); + assert!(!shown.contains("super-secret-value"), "{shown}"); + assert!(shown.contains("API key"), "{shown}"); + + let tok = Credential::OAuth("sk-ant-oat01-secret".into()); + assert!(tok.redacted().contains("OAuth token")); + } +} diff --git a/src/browser/approval_gate.rs b/src/browser/approval_gate.rs index 59c2f5e..5529b4c 100644 --- a/src/browser/approval_gate.rs +++ b/src/browser/approval_gate.rs @@ -177,14 +177,14 @@ impl ApprovalGate { // 5. Visible prices — skip zero amounts for price in &c.visible_prices { - if let Some(caps) = self.price_re.captures(price) { - if let Some(amount_str) = caps.get(1) { - let normalized = amount_str.as_str().replace(',', "."); - if let Ok(val) = normalized.parse::() { - if val >= 0.01 { - reasons.push(format!("visible_price: {price}")); - } - } + if let Some(caps) = self.price_re.captures(price) + && let Some(amount_str) = caps.get(1) + { + let normalized = amount_str.as_str().replace(',', "."); + if let Ok(val) = normalized.parse::() + && val >= 0.01 + { + reasons.push(format!("visible_price: {price}")); } } } diff --git a/src/browser/browse_loop.rs b/src/browser/browse_loop.rs index 9791181..8dd7985 100644 --- a/src/browser/browse_loop.rs +++ b/src/browser/browse_loop.rs @@ -126,10 +126,10 @@ fn parse_browse_done(text: &str) -> Option<(bool, String)> { /// Prefers `url` → `ref` → `selector` → `key` → empty string. fn extract_target(input: &serde_json::Value) -> String { for key in ["url", "ref", "selector", "key"] { - if let Some(s) = input.get(key).and_then(|v| v.as_str()) { - if !s.is_empty() { - return s.to_string(); - } + if let Some(s) = input.get(key).and_then(|v| v.as_str()) + && !s.is_empty() + { + return s.to_string(); } } String::new() diff --git a/src/commands/mod.rs b/src/commands/mod.rs index a384fb5..90630e6 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -1102,12 +1102,26 @@ fn cmd_doctor(ctx: &CommandContext) -> CommandAction { // API key if ctx.config.api_key.len() >= 4 { - checks.push(format!( - "✓ ANTHROPIC_API_KEY set ({}...)", - &ctx.config.api_key[..4] - )); + let src = ctx + .config + .auth_source + .as_deref() + .unwrap_or("apiKeyHelper / file descriptor"); + let cred = if ctx.config.auth_is_oauth { + crate::auth::Credential::OAuth(ctx.config.api_key.clone()) + } else { + crate::auth::Credential::ApiKey(ctx.config.api_key.clone()) + }; + checks.push(format!("✓ Anthropic credential: {} via {src}", cred.redacted())); + // Surface the "stale env var shadows your profile" trap, which is + // otherwise invisible and sends requests to the wrong org/workspace. + for w in &ctx.config.auth_warnings { + checks.push(format!("⚠ {w}")); + } } else { - checks.push("✗ ANTHROPIC_API_KEY not set — run: export ANTHROPIC_API_KEY=sk-...".into()); + checks.push( + "✗ No Anthropic credential — set ANTHROPIC_API_KEY, or run `ant auth login`".into(), + ); } // cwd / git / config diff --git a/src/config.rs b/src/config.rs index dd22a03..6b48d8a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -126,6 +126,21 @@ pub struct Config { #[serde(skip)] pub api_key: String, + /// True when `api_key` holds an OAuth access token rather than a static + /// key. Selects the wire format: `Authorization: Bearer` + the + /// `oauth-2025-04-20` beta, instead of `x-api-key`. + #[serde(skip)] + pub auth_is_oauth: bool, + + /// Human-readable description of which source the credential came from, + /// surfaced by /doctor so a shadowed profile is diagnosable. + #[serde(skip)] + pub auth_source: Option, + + /// Non-fatal credential warnings (e.g. an env var shadowing a profile). + #[serde(skip)] + pub auth_warnings: Vec, + /// Model to use for the main loop. /// Use `ollama:` to route to a local Ollama instance instead. pub model: String, @@ -380,6 +395,9 @@ impl Default for Config { fn default() -> Self { Self { api_key: String::new(), + auth_is_oauth: false, + auth_source: None, + auth_warnings: Vec::new(), model: crate::api::default_model().to_string(), max_tokens: crate::api::default_max_tokens(), max_tokens_by_model: HashMap::new(), @@ -670,8 +688,23 @@ impl Config { // base defaults in settings and override individual phases in CLAUDE.md. Self::apply_phase_routing_from_claudemd(&cfg.claudemd, &mut cfg.phase_router); - // ── API key from environment (not required for Ollama models) - cfg.api_key = std::env::var("ANTHROPIC_API_KEY").unwrap_or_default(); + // ── Credential from the environment (not required for Ollama models). + // + // Honours the same resolution order as the official SDKs, the `ant` CLI, + // and Claude Code, so an existing login works without reconfiguration: + // + // ANTHROPIC_API_KEY → ANTHROPIC_AUTH_TOKEN → ant auth login profile + // + // RustyClaw's own explicit mechanisms (RUSTYCLAW_API_KEY_FILE_DESCRIPTOR, + // apiKeyHelper) run between the env vars and the profile — see the + // `ant`-profile fallback further down. Explicit local configuration + // should beat ambient machine state. + if let Some(resolved) = crate::auth::resolve_env() { + cfg.auth_is_oauth = resolved.credential.is_oauth(); + cfg.auth_source = Some(resolved.source.describe()); + cfg.auth_warnings = resolved.warnings; + cfg.api_key = resolved.credential.secret().to_string(); + } // ── RUSTYCLAW_API_KEY_FILE_DESCRIPTOR: read API key from an open fd. // Unix-only — Windows uses HANDLEs, not POSIX fds, and the @@ -720,6 +753,20 @@ impl Config { } } + // ── Last resort: the active `ant auth login` profile. + // + // Runs after the explicit mechanisms above so local configuration always + // wins over ambient machine state. `ant auth print-credentials` refreshes + // the short-lived token before printing, so there is no refresh flow to + // implement here. + if cfg.api_key.is_empty() + && let Some(resolved) = crate::auth::resolve_profile() + { + cfg.auth_is_oauth = resolved.credential.is_oauth(); + cfg.auth_source = Some(resolved.source.describe()); + cfg.api_key = resolved.credential.secret().to_string(); + } + // ── Optional env var overrides (env wins over settings files) if let Ok(model) = std::env::var("ANTHROPIC_MODEL") { cfg.model = model; diff --git a/src/cost.rs b/src/cost.rs index 5f56803..58a5458 100644 --- a/src/cost.rs +++ b/src/cost.rs @@ -172,7 +172,11 @@ impl CostTracker { lines.push("Per-model breakdown:".into()); let mut models: Vec<_> = self.by_model.iter().collect(); - models.sort_by(|a, b| b.1.cost_usd.partial_cmp(&a.1.cost_usd).unwrap()); + // `total_cmp` rather than `partial_cmp().unwrap()`: the release profile + // sets `panic = "abort"`, so a NaN cost would take the whole process down + // just to render a cost summary. NaN is not reachable today, but a total + // order costs nothing and removes the failure mode permanently. + models.sort_by(|a, b| b.1.cost_usd.total_cmp(&a.1.cost_usd)); for (model, usage) in models { let short = short_model_name(model); @@ -282,6 +286,35 @@ fn format_tokens(n: u64) -> String { mod tests { use super::*; + /// `summary()` sorts per-model costs. With `partial_cmp().unwrap()` a non-finite + /// cost aborted the process (release sets `panic = "abort"`); `total_cmp` orders + /// it instead. Rendering a cost report must never be able to kill the session. + #[test] + fn summary_survives_non_finite_costs() { + let mut tracker = CostTracker::new(); + tracker.record("claude-sonnet-4-6", 1_000, 100); + + for (name, cost) in [ + ("model-nan", f64::NAN), + ("model-inf", f64::INFINITY), + ("model-neg-inf", f64::NEG_INFINITY), + ] { + tracker.by_model.insert( + name.to_string(), + ModelUsage { + input_tokens: 1, + output_tokens: 1, + turns: 1, + cost_usd: cost, + }, + ); + } + + let summary = tracker.summary(); + assert!(summary.contains("Per-model breakdown:")); + assert!(summary.contains("model-nan"), "every model must still render"); + } + #[test] fn test_cost_tracking() { let mut tracker = CostTracker::new(); diff --git a/src/hooks.rs b/src/hooks.rs index 46ab371..d292c68 100644 --- a/src/hooks.rs +++ b/src/hooks.rs @@ -263,6 +263,98 @@ struct HookEnvVars<'a> { cwd: &'a std::path::Path, } +/// Wall-clock bound on a single hook. Hooks sit on the critical path of every +/// tool call, so one that waits on input, a network call, or a lock would +/// otherwise block the agent indefinitely with no diagnostic. +const HOOK_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(60); + +/// Cap on captured stdout/stderr per stream. The pipe is still drained past +/// this point so the hook can exit rather than blocking on a full pipe. +const MAX_HOOK_OUTPUT_BYTES: usize = 256 * 1024; + +/// Cap on a single env var handed to a hook. +/// +/// Linux limits one env entry to ~128 KB (`MAX_ARG_STRLEN`). A large +/// `TOOL_INPUT` — a big Write, a long diff — would push `spawn` over that and +/// fail with E2BIG. Combined with the old fail-open behaviour that meant a +/// PreToolUse gate was *silently skipped precisely on the largest tool calls*. +/// Truncating keeps the hook running on the inputs that matter most. +const MAX_HOOK_ENV_BYTES: usize = 64 * 1024; + +/// Does this event's result actually gate anything? +/// +/// Only `PreToolUse` can block a tool call, so it is the only event where a +/// failure to *evaluate* the hook is a security-relevant outcome. For every +/// other event there is nothing to gate — a notification or post-hoc hook that +/// fails is genuinely non-blocking, and failing closed there would break +/// sessions for no safety benefit. +fn is_gating_event(event: &str) -> bool { + event == "PreToolUse" +} + +/// A hook that could not be evaluated. +/// +/// For a gating event this **fails closed**: a gate that did not run has not +/// approved anything, and the previous behaviour (return `allow()` after a +/// `tracing::warn!` the user never sees in the TUI) meant a broken or +/// missing PreToolUse hook silently disabled itself. +fn hook_unevaluable(event: &str, hook: &HookEntry, why: &str) -> HookResult { + if is_gating_event(event) { + tracing::error!("Blocking: PreToolUse hook '{}' {}", hook.command, why); + HookResult { + should_continue: false, + stop_reason: Some(format!( + "PreToolUse hook could not be evaluated: it {why}.\n \ + Hook: {}\n\ + Blocking the tool call — a hook that cannot run has not approved it. \ + Fix or remove the hook in settings.json.", + hook.command + )), + ..Default::default() + } + } else { + tracing::warn!("Hook '{}' {} (non-blocking event)", hook.command, why); + HookResult::allow() + } +} + +/// Truncate an env value to stay under the per-entry limit, on a char boundary. +fn cap_env_value(v: &str) -> String { + if v.len() <= MAX_HOOK_ENV_BYTES { + return v.to_string(); + } + let cut = (0..=MAX_HOOK_ENV_BYTES) + .rev() + .find(|&i| v.is_char_boundary(i)) + .unwrap_or(0); + format!("{}…[truncated by rustyclaw]", &v[..cut]) +} + +/// Read a pipe to EOF, keeping at most `cap` bytes. +/// +/// Draining past the cap matters: if we stopped reading, the hook would block +/// writing to a full pipe and only die at the timeout, turning a fast hook into +/// a 60-second stall. +async fn read_capped(reader: &mut R, cap: usize) -> std::io::Result +where + R: tokio::io::AsyncRead + Unpin, +{ + use tokio::io::AsyncReadExt; + let mut buf = vec![0u8; 8192]; + let mut kept: Vec = Vec::new(); + loop { + let n = reader.read(&mut buf).await?; + if n == 0 { + break; + } + if kept.len() < cap { + let room = cap - kept.len(); + kept.extend_from_slice(&buf[..room.min(n)]); + } + } + Ok(String::from_utf8_lossy(&kept).into_owned()) +} + async fn execute_hook(hook: &HookEntry, env: HookEnvVars<'_>) -> HookResult { use tokio::process::Command; @@ -281,30 +373,81 @@ async fn execute_hook(hook: &HookEntry, env: HookEnvVars<'_>) -> HookResult { cmd.env("TOOL_NAME", name); } if let Some(inp) = env.tool_input { - cmd.env("TOOL_INPUT", inp); + cmd.env("TOOL_INPUT", cap_env_value(inp)); } if let Some(res) = env.tool_result { - cmd.env("TOOL_RESULT", res); + cmd.env("TOOL_RESULT", cap_env_value(res)); } if let Some(msg) = env.prompt { - cmd.env("CLAUDE_MESSAGE", msg); + cmd.env("CLAUDE_MESSAGE", cap_env_value(msg)); } - // Capture stdout and stderr + // Capture stdout and stderr. stdin is /dev/null: inherited stdin would let + // a hook that reads input compete with the TUI for the user's keystrokes + // and hang until the timeout. + cmd.stdin(std::process::Stdio::null()); cmd.stdout(std::process::Stdio::piped()); cmd.stderr(std::process::Stdio::piped()); + cmd.kill_on_drop(true); + // Own process group so a timeout can take out anything the hook spawned, + // rather than leaving orphans reparented to init. + #[cfg(unix)] + cmd.process_group(0); + + let mut child = match cmd.spawn() { + Ok(c) => c, + Err(e) => return hook_unevaluable(env.event, hook, &format!("failed to start: {e}")), + }; + + #[cfg(unix)] + let pgid = child.id().map(|id| id as i32); - let output = match cmd.output().await { - Ok(o) => o, - Err(e) => { - tracing::warn!("Hook command failed to start: {}", e); - return HookResult::allow(); + let Some(mut child_stdout) = child.stdout.take() else { + return hook_unevaluable(env.event, hook, "produced no stdout pipe"); + }; + let Some(mut child_stderr) = child.stderr.take() else { + return hook_unevaluable(env.event, hook, "produced no stderr pipe"); + }; + + // Read both pipes concurrently. Draining one to EOF before starting the + // other deadlocks if the hook fills the second pipe first. + let collect = async { + let (out, err) = tokio::join!( + read_capped(&mut child_stdout, MAX_HOOK_OUTPUT_BYTES), + read_capped(&mut child_stderr, MAX_HOOK_OUTPUT_BYTES), + ); + let status = child.wait().await?; + Ok::<_, std::io::Error>((status, out?, err?)) + }; + + let (status, stdout, stderr) = match tokio::time::timeout(HOOK_TIMEOUT, collect).await { + Ok(Ok(v)) => v, + Ok(Err(e)) => { + return hook_unevaluable(env.event, hook, &format!("could not be read: {e}")); + } + Err(_) => { + // SAFETY: libc::kill with a negative pid signals the whole process + // group. Unsafe only because of FFI; the pid is one we just spawned. + #[cfg(unix)] + if let Some(pgid) = pgid { + unsafe { + libc::kill(-pgid, libc::SIGKILL); + } + } + return hook_unevaluable( + env.event, + hook, + &format!("timed out after {}s", HOOK_TIMEOUT.as_secs()), + ); } }; - let exit_code = output.status.code().unwrap_or(0); - let stdout = String::from_utf8_lossy(&output.stdout).to_string(); - let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + // A hook killed by a signal (OOM killer, external SIGKILL) has no exit + // code. `unwrap_or(0)` previously read that as success — a second silent + // fail-open, and the one an attacker would reach for. + let Some(exit_code) = status.code() else { + return hook_unevaluable(env.event, hook, "was killed by a signal"); + }; if !stderr.is_empty() { tracing::debug!("Hook stderr: {stderr}"); @@ -380,3 +523,240 @@ async fn execute_hook(hook: &HookEntry, env: HookEnvVars<'_>) -> HookResult { HookResult::allow() } + +#[cfg(test)] +mod tests { + use super::*; + use crate::settings::{HookEntry, HooksConfig}; + + fn entry(command: &str) -> HookEntry { + HookEntry { + matcher: String::new(), + command: command.to_string(), + } + } + + fn cfg_pre(command: &str) -> HooksConfig { + HooksConfig { + pre_tool_use: vec![entry(command)], + ..Default::default() + } + } + + fn cfg_post(command: &str) -> HooksConfig { + HooksConfig { + post_tool_use: vec![entry(command)], + ..Default::default() + } + } + + // ── Which events fail closed ───────────────────────────────────────────── + + #[test] + fn only_pre_tool_use_gates() { + assert!(is_gating_event("PreToolUse")); + for e in [ + "PostToolUse", + "UserPromptSubmit", + "Notification", + "Stop", + "SessionStart", + "PreCompact", + "PostCompact", + ] { + assert!(!is_gating_event(e), "{e} does not gate a tool call"); + } + } + + /// A gate that could not run has not approved anything. + #[test] + fn unevaluable_gating_hook_blocks() { + let r = hook_unevaluable("PreToolUse", &entry("/bin/broken"), "failed to start: ENOENT"); + assert!(!r.should_continue, "PreToolUse must fail closed"); + let reason = r.stop_reason.expect("must explain why it blocked"); + assert!(reason.contains("/bin/broken"), "{reason}"); + assert!(reason.contains("has not approved"), "{reason}"); + } + + /// Nothing to gate — failing closed here would break sessions for no gain. + #[test] + fn unevaluable_non_gating_hook_allows() { + let r = hook_unevaluable("PostToolUse", &entry("/bin/broken"), "timed out"); + assert!(r.should_continue, "non-gating events stay non-blocking"); + assert!(r.stop_reason.is_none()); + } + + // ── Signal-killed hooks ────────────────────────────────────────────────── + + /// `status.code()` is None when a process dies by signal. The old + /// `unwrap_or(0)` read that as exit 0 — success — so a PreToolUse gate + /// killed by the OOM killer (or anything else) silently allowed the call. + /// + /// Unix-only: Windows has no POSIX signal termination and `ExitStatus::code()` + /// there always returns `Some`, so neither the bug nor this test applies. + #[cfg(unix)] + #[tokio::test] + async fn signal_killed_gating_hook_blocks() { + let r = run_pre_tool_hooks( + &cfg_pre("kill -9 $$"), + "Bash", + "{}", + "sess", + std::path::Path::new("."), + ) + .await; + assert!( + !r.should_continue, + "a signal-killed PreToolUse hook must not be read as approval" + ); + let reason = r.stop_reason.unwrap_or_default(); + assert!(reason.contains("signal"), "reason should say why: {reason}"); + } + + /// The same failure on a non-gating event is still non-blocking. + #[cfg(unix)] + #[tokio::test] + async fn signal_killed_non_gating_hook_is_tolerated() { + // Must simply return without blocking anything. + run_post_tool_hooks( + &cfg_post("kill -9 $$"), + "Bash", + "ok", + "sess", + std::path::Path::new("."), + ) + .await; + } + + // ── Documented exit-code contract is preserved ─────────────────────────── + + #[tokio::test] + async fn exit_zero_allows() { + let r = run_pre_tool_hooks( + &cfg_pre("exit 0"), + "Bash", + "{}", + "sess", + std::path::Path::new("."), + ) + .await; + assert!(r.should_continue); + } + + #[tokio::test] + async fn exit_two_blocks_with_reason() { + let r = run_pre_tool_hooks( + &cfg_pre("echo 'nope, dangerous' >&2; exit 2"), + "Bash", + "{}", + "sess", + std::path::Path::new("."), + ) + .await; + assert!(!r.should_continue, "exit 2 is the documented block signal"); + } + + /// Documented contract: a non-zero exit other than 2 is a *non-blocking* + /// error. Preserved deliberately — fail-closed applies to hooks that could + /// not be evaluated, not to hooks that ran and reported failure. + #[tokio::test] + async fn other_nonzero_exit_stays_non_blocking() { + let r = run_pre_tool_hooks( + &cfg_pre("exit 127"), + "Bash", + "{}", + "sess", + std::path::Path::new("."), + ) + .await; + assert!(r.should_continue, "exit 127 is documented as non-blocking"); + } + + // ── Resource bounds ────────────────────────────────────────────────────── + + /// A hook emitting far more than the cap must still complete promptly — + /// capping without draining would leave it blocked on a full pipe until + /// the 60s timeout. + #[tokio::test] + async fn large_hook_output_does_not_stall() { + let start = std::time::Instant::now(); + let r = run_pre_tool_hooks( + &cfg_pre("head -c 4000000 /dev/zero | tr '\\0' 'a'"), + "Bash", + "{}", + "sess", + std::path::Path::new("."), + ) + .await; + assert!(r.should_continue); + assert!( + start.elapsed() < std::time::Duration::from_secs(20), + "took {:?} — the pipe is not being drained", + start.elapsed() + ); + } + + /// An oversized TOOL_INPUT previously pushed `spawn` past the per-entry env + /// limit (E2BIG), which under the old fail-open meant the gate was skipped + /// exactly on the biggest tool calls. + #[tokio::test] + async fn oversized_tool_input_still_runs_the_hook() { + let huge = "x".repeat(2 * 1024 * 1024); + let r = run_pre_tool_hooks( + &cfg_pre("test -n \"$TOOL_INPUT\" && exit 2"), + "Bash", + &huge, + "sess", + std::path::Path::new("."), + ) + .await; + assert!( + !r.should_continue, + "hook must still receive TOOL_INPUT and be able to block" + ); + // Distinguish "the hook ran and blocked" from "spawn failed and the new + // fail-closed path caught it" — both set should_continue=false, so + // asserting that alone would pass even with the env cap removed. + let reason = r.stop_reason.unwrap_or_default(); + assert!( + !reason.contains("could not be evaluated"), + "the hook must actually have run, not been rescued by fail-closed: {reason}" + ); + } + + /// Direct check that oversized values are handed to the process at a size + /// it will accept, independent of how the hook reports its decision. + #[tokio::test] + async fn oversized_env_reaches_the_hook_truncated() { + let huge = "x".repeat(2 * 1024 * 1024); + // Echo the length the hook actually observed; exit 2 carries it back + // through stop_reason. + let r = run_pre_tool_hooks( + &cfg_pre("echo \"len=${#TOOL_INPUT}\"; exit 2"), + "Bash", + &huge, + "sess", + std::path::Path::new("."), + ) + .await; + let reason = r.stop_reason.unwrap_or_default(); + assert!(reason.contains("len="), "hook did not run: {reason}"); + assert!( + !reason.contains(&format!("len={}", huge.len())), + "value should have been truncated before spawn: {reason}" + ); + } + + #[test] + fn env_values_are_capped_on_a_char_boundary() { + let small = "hello"; + assert_eq!(cap_env_value(small), small); + + let big = "é".repeat(MAX_HOOK_ENV_BYTES); + let capped = cap_env_value(&big); + assert!(capped.len() <= MAX_HOOK_ENV_BYTES + 64, "len {}", capped.len()); + assert!(capped.contains("truncated")); + // Round-trips as valid UTF-8 (would have panicked on a bad slice). + assert!(!capped.is_empty()); + } +} diff --git a/src/lib.rs b/src/lib.rs index 8f60dd2..027bd46 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -5,6 +5,7 @@ //! of the `rustyclaw` binary. pub mod api; +pub mod auth; pub mod autocommit; pub mod autofix; pub mod browser; diff --git a/src/main.rs b/src/main.rs index 9891ba8..26c378c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,6 +1,7 @@ /// rustyclaw — Rust-native AI coding CLI /// Entry point mod api; +mod auth; mod autofix; mod browser; mod commands; @@ -396,8 +397,16 @@ enum McpSubcommand { /// If a user legitimately needs one of the blocked vars set, they can export /// it in their shell — project `.env` is not the right place. const SAFE_ENV_KEYS: &[&str] = &[ - // Anthropic + // Anthropic credentials. The whole documented resolution chain must be + // settable from .env, not just the API key — otherwise a project that + // authenticates with an OAuth token silently falls back to whatever key + // happens to be in the ambient environment. + // + // ANTHROPIC_BASE_URL is deliberately NOT here: it redirects every API call, + // so a hostile .env could point credentials at an attacker-controlled host. "ANTHROPIC_API_KEY", + "ANTHROPIC_AUTH_TOKEN", + "ANTHROPIC_PROFILE", "RUSTYCLAW_API_KEY_FILE_DESCRIPTOR", "ANTHROPIC_MODEL", // Verbose logging toggle — no exec side-effects @@ -1388,6 +1397,35 @@ mod dotenv_allowlist_tests { } } + /// Every source in the documented credential chain must be settable from + /// .env. ANTHROPIC_AUTH_TOKEN was missing when OAuth support landed, so a + /// project authenticating with a token silently fell back to whatever key + /// was in the ambient environment. + #[test] + fn dotenv_allowlist_covers_the_whole_credential_chain() { + for key in [ + "ANTHROPIC_API_KEY", + "ANTHROPIC_AUTH_TOKEN", + "ANTHROPIC_PROFILE", + "RUSTYCLAW_API_KEY_FILE_DESCRIPTOR", + ] { + assert!( + SAFE_ENV_KEYS.contains(&key), + "{key} must be loadable from .env — it is part of credential resolution" + ); + } + } + + /// Redirecting the API base URL from a project .env would let a hostile + /// repo point real credentials at an attacker-controlled host. + #[test] + fn dotenv_allowlist_excludes_base_url_redirect() { + assert!( + !SAFE_ENV_KEYS.contains(&"ANTHROPIC_BASE_URL"), + "ANTHROPIC_BASE_URL must never be settable from .env" + ); + } + /// A malicious .env that sets dangerous vars must not leak into the /// process environment when we run `load_dotenv` against it. /// diff --git a/src/permissions/mod.rs b/src/permissions/mod.rs index 07d5523..6597307 100644 --- a/src/permissions/mod.rs +++ b/src/permissions/mod.rs @@ -22,7 +22,11 @@ pub enum PermissionDecision { /// Tools that require explicit permission before execution. /// Mirrors the hasPermissionsToUseTool logic in permissions.ts. -pub const SENSITIVE_TOOLS: &[&str] = &["Bash", "Write", "Edit"]; +/// +/// `PowerShell` executes arbitrary commands exactly like `Bash` and must be +/// gated the same way. It was previously absent, so on any machine with `pwsh` +/// installed the model could run shell commands with no approval prompt at all. +pub const SENSITIVE_TOOLS: &[&str] = &["Bash", "PowerShell", "Write", "Edit"]; /// Session-scoped permission state — shared between tool executor and TUI. #[derive(Clone, Default)] @@ -131,7 +135,7 @@ fn rule_matches(rule: &str, tool_name: &str, input: Option<&serde_json::Value>) // Check the input's "command" field for Bash, or "file_path" for file tools if let Some(inp) = input { let value = match tool_name { - "Bash" => inp["command"].as_str().unwrap_or(""), + "Bash" | "PowerShell" => inp["command"].as_str().unwrap_or(""), "Write" | "Edit" | "Read" => inp["file_path"].as_str().unwrap_or(""), _ => return false, }; @@ -258,6 +262,10 @@ pub fn describe_tool_call(tool_name: &str, input: &serde_json::Value) -> String let cmd = input["command"].as_str().unwrap_or("(unknown)"); format!("Run shell command:\n {cmd}") } + "PowerShell" => { + let cmd = input["command"].as_str().unwrap_or("(unknown)"); + format!("Run PowerShell command:\n {cmd}") + } "Write" => { let path = input["file_path"].as_str().unwrap_or("(unknown)"); format!("Write/overwrite file:\n {path}") @@ -285,3 +293,80 @@ fn truncate(s: &str, max: usize) -> &str { &s[..end] } } + +#[cfg(test)] +mod tests { + use super::*; + + fn state() -> PermissionState { + PermissionState::new(false, &[], &[]) + } + + /// `PowerShell` was absent from SENSITIVE_TOOLS, so `check_with_input` + /// returned Allow immediately — the model could run arbitrary shell commands + /// via `pwsh` with no approval prompt at all. + #[test] + fn powershell_requires_approval() { + let input = serde_json::json!({ "command": "Remove-Item -Recurse -Force C:\\" }); + assert!( + matches!( + state().check_with_input("PowerShell", Some(&input)), + CheckResult::Ask + ), + "PowerShell must prompt like Bash, not auto-allow" + ); + } + + #[test] + fn every_command_executing_tool_is_gated() { + for tool in ["Bash", "PowerShell"] { + let input = serde_json::json!({ "command": "whoami" }); + assert!( + matches!(state().check_with_input(tool, Some(&input)), CheckResult::Ask), + "{tool} must require approval" + ); + } + } + + #[test] + fn non_sensitive_tools_still_pass_through() { + let input = serde_json::json!({ "file_path": "/tmp/x" }); + assert!(matches!( + state().check_with_input("Read", Some(&input)), + CheckResult::Allow + )); + } + + #[test] + fn deny_rules_beat_the_sensitive_list() { + let st = PermissionState::new(false, &[], &["PowerShell".to_string()]); + let input = serde_json::json!({ "command": "whoami" }); + assert!(matches!( + st.check_with_input("PowerShell", Some(&input)), + CheckResult::Deny + )); + } + + #[test] + fn prefix_rules_work_for_powershell() { + let st = PermissionState::new(false, &["PowerShell(prefix:Get-)".to_string()], &[]); + let allowed = serde_json::json!({ "command": "Get-ChildItem" }); + let asked = serde_json::json!({ "command": "Remove-Item x" }); + assert!(matches!( + st.check_with_input("PowerShell", Some(&allowed)), + CheckResult::Allow + )); + assert!(matches!( + st.check_with_input("PowerShell", Some(&asked)), + CheckResult::Ask + )); + } + + #[test] + fn powershell_calls_are_described_for_the_approval_dialog() { + let input = serde_json::json!({ "command": "Get-Process" }); + let desc = describe_tool_call("PowerShell", &input); + assert!(desc.contains("PowerShell"), "{desc}"); + assert!(desc.contains("Get-Process"), "{desc}"); + } +} diff --git a/src/query_engine.rs b/src/query_engine.rs index 0d604db..dd48eef 100644 --- a/src/query_engine.rs +++ b/src/query_engine.rs @@ -45,14 +45,18 @@ impl QueryEngine { || crate::api::is_openai_compat_model(&config.model); if !is_non_anthropic && config.api_key.is_empty() { return Err(anyhow::anyhow!( - "ANTHROPIC_API_KEY is not set.\n\ - Set it with: export ANTHROPIC_API_KEY=sk-ant-...\n\ + "No Anthropic credential found.\n\ + RustyClaw checks, in order:\n\ + 1. ANTHROPIC_API_KEY export ANTHROPIC_API_KEY=sk-ant-...\n\ + 2. ANTHROPIC_AUTH_TOKEN an OAuth access token\n\ + 3. apiKeyHelper / RUSTYCLAW_API_KEY_FILE_DESCRIPTOR\n\ + 4. ant auth login shared with Claude Code and the official SDKs\n\ To use a local model instead: --model ollama:\n\ Or a cloud OpenAI-compatible model: --model groq:, --model openrouter:, ..." )); } - let client = ApiBackend::new(&config.model, &config.api_key, &config.ollama_host) + let client = ApiBackend::new_with_auth(&config.model, &config.api_key, config.auth_is_oauth, &config.ollama_host) .context("Failed to create API client")?; let system_prompt = config.build_system_prompt(); Ok(Self { diff --git a/src/sandbox.rs b/src/sandbox.rs index 348a0f3..8689f55 100644 --- a/src/sandbox.rs +++ b/src/sandbox.rs @@ -113,10 +113,15 @@ pub fn bwrap_wrap(command: &str, cwd: &std::path::Path, allow_network: bool) -> // ── firejail wrapper ────────────────────────────────────────────────────────── -pub fn firejail_wrap(command: &str, cwd: &std::path::Path) -> String { +pub fn firejail_wrap(command: &str, cwd: &std::path::Path, allow_network: bool) -> String { let cwd_quoted = shell_quote(&cwd.display().to_string()); + // `--net=none` is firejail's equivalent of bwrap's `--unshare-net`. Without + // it, firejail mode silently ignored `sandbox_allow_network` and always had + // full egress, so the same setting meant different things in the two modes. + let net_flag = if allow_network { "" } else { "--net=none " }; format!( - "firejail --quiet --private-tmp --noroot --chdir={cwd} -- /bin/sh -c {cmd}", + "firejail --quiet --private-tmp --noroot {net_flag}--chdir={cwd} -- /bin/sh -c {cmd}", + net_flag = net_flag, cwd = cwd_quoted, cmd = shell_quote(command), ) @@ -163,9 +168,45 @@ pub fn apply_sandbox( .into(), ); } - Ok(firejail_wrap(command, cwd)) + Ok(firejail_wrap(command, cwd, allow_network)) } - _ => Ok(command.to_string()), + // Fail CLOSED on an unrecognised mode. `ctx.sandbox_mode` is only `Some` + // when the sandbox is enabled, so reaching this arm means the configured + // mode string is invalid — a typo or a stale value in settings.json, + // which (unlike `/sandbox enable`) does not validate the field. + // + // Returning the command unchanged here used to run it fully unsandboxed + // AND skip `strict_check`, while the UI still reported the sandbox as + // enabled. A security control that silently does nothing is worse than + // one that is off, so refuse the command and name the bad value. + other => Err(format!( + "Sandbox is enabled but the configured mode '{other}' is not recognised. \ + Valid modes: strict, bwrap, firejail. Refusing to run the command \ + unsandboxed — fix `sandboxMode` in settings.json or run: /sandbox enable strict" + )), + } +} + +/// Sandbox gate for command-executing tools that the namespace wrappers cannot +/// wrap. `bwrap_wrap` / `firejail_wrap` hard-code `/bin/sh -c`, so routing a +/// PowerShell command through them would hand the script to `sh` and change its +/// meaning entirely. +/// +/// Pattern blocking still applies in every mode. For the namespace modes there +/// is no correct wrapping, so this fails closed: better to refuse than to run +/// outside the sandbox the user believes is active. +pub fn guard_unwrappable_tool(command: &str, mode: &str, tool: &str) -> Result<(), String> { + if let Some(reason) = strict_check(command) { + return Err(reason); + } + match mode { + "strict" => Ok(()), + other => Err(format!( + "The {tool} tool cannot be sandboxed under mode '{other}' — the {other} \ + wrapper executes through /bin/sh, which would not run a PowerShell \ + script correctly. Refusing rather than running it unsandboxed. \ + Use /sandbox enable strict, or use the Bash tool instead." + )), } } @@ -214,3 +255,92 @@ pub fn sandbox_status(enabled: bool, mode: &str) -> String { fn shell_quote(s: &str) -> String { format!("'{}'", s.replace('\'', "'\\''")) } + +#[cfg(test)] +mod tests { + use super::*; + use std::path::Path; + + /// An unrecognised mode used to return the command unchanged — running it + /// fully unsandboxed, skipping `strict_check`, while the UI still reported + /// the sandbox as enabled. Reachable via an unvalidated `sandboxMode` in + /// settings.json. + #[test] + fn unknown_mode_fails_closed() { + let err = apply_sandbox("echo hi", "strict-typo", Path::new("/tmp"), false) + .expect_err("an unrecognised mode must not run the command unsandboxed"); + assert!(err.contains("strict-typo"), "error should name the bad mode: {err}"); + assert!(err.contains("strict"), "error should list valid modes: {err}"); + } + + #[test] + fn known_modes_still_pass_through() { + let out = apply_sandbox("echo hi", "strict", Path::new("/tmp"), false) + .expect("strict mode is valid"); + assert_eq!(out, "echo hi"); + } + + #[test] + fn strict_mode_still_blocks_dangerous_patterns() { + assert!(apply_sandbox("rm -rf /", "strict", Path::new("/tmp"), false).is_err()); + } + + /// `firejail_wrap` ignored `allow_network` entirely, so firejail mode always + /// had full egress while bwrap mode honoured the setting — the same config + /// meaning two different things. + #[test] + fn firejail_honours_network_setting() { + let blocked = firejail_wrap("echo hi", Path::new("/tmp"), false); + assert!(blocked.contains("--net=none"), "network must be blocked: {blocked}"); + + let allowed = firejail_wrap("echo hi", Path::new("/tmp"), true); + assert!(!allowed.contains("--net=none"), "network must be allowed: {allowed}"); + } + + #[test] + fn bwrap_and_firejail_agree_on_network_policy() { + let bw = bwrap_wrap("echo hi", Path::new("/tmp"), false); + let fj = firejail_wrap("echo hi", Path::new("/tmp"), false); + assert!(bw.contains("--unshare-net")); + assert!(fj.contains("--net=none")); + } + + #[test] + fn shell_quote_escapes_embedded_single_quotes() { + assert_eq!(shell_quote("it's"), r#"'it'\''s'"#); + let wrapped = firejail_wrap("echo 'pwn'", Path::new("/tmp/a b"), true); + assert!(wrapped.contains(r#"'/tmp/a b'"#), "cwd must stay quoted: {wrapped}"); + } + + /// PowerShell cannot be wrapped by the namespace modes (they exec /bin/sh), + /// so the gate must refuse rather than run it outside the active sandbox. + #[test] + fn unwrappable_tool_gate_fails_closed_on_namespace_modes() { + assert!(guard_unwrappable_tool("Get-ChildItem", "strict", "PowerShell").is_ok()); + + for mode in ["bwrap", "firejail"] { + let err = guard_unwrappable_tool("Get-ChildItem", mode, "PowerShell") + .unwrap_err_or_else_msg(); + assert!(err.contains(mode), "error should name the mode: {err}"); + } + } + + #[test] + fn unwrappable_tool_gate_applies_pattern_blocking_in_every_mode() { + for mode in ["strict", "bwrap", "firejail"] { + assert!( + guard_unwrappable_tool("rm -rf /", mode, "PowerShell").is_err(), + "dangerous pattern must be blocked under {mode}" + ); + } + } + + trait UnwrapErrMsg { + fn unwrap_err_or_else_msg(self) -> String; + } + impl UnwrapErrMsg for Result<(), String> { + fn unwrap_err_or_else_msg(self) -> String { + self.expect_err("expected the gate to refuse") + } + } +} diff --git a/src/sdk/session.rs b/src/sdk/session.rs index 8746fcc..2cfe09f 100644 --- a/src/sdk/session.rs +++ b/src/sdk/session.rs @@ -49,7 +49,7 @@ impl SdkSession { approval_tx: mpsc::UnboundedSender, approval_rx: mpsc::UnboundedReceiver<(String, Option)>, ) -> Result { - let client = ApiBackend::new(&config.model, &config.api_key, &config.ollama_host) + let client = ApiBackend::new_with_auth(&config.model, &config.api_key, config.auth_is_oauth, &config.ollama_host) .context("Failed to create API client")?; let system_prompt = config.build_system_prompt(); let session_id = uuid::Uuid::new_v4().to_string(); diff --git a/src/tools/bash.rs b/src/tools/bash.rs index c70fd40..b210e10 100644 --- a/src/tools/bash.rs +++ b/src/tools/bash.rs @@ -4,7 +4,7 @@ use anyhow::Result; use serde::Deserialize; use serde_json::json; use std::process::Stdio; -use tokio::io::{AsyncBufReadExt, BufReader}; +use tokio::io::AsyncReadExt; use tokio::process::{Child, Command}; use tokio::time::{Duration, timeout}; @@ -66,6 +66,67 @@ impl Drop for ProcessGroupGuard { const DEFAULT_TIMEOUT_MS: u64 = 120_000; // 2 minutes, same as TypeScript default const MAX_OUTPUT_BYTES: usize = 1_000_000; // 1MB cap +const CHUNK_SIZE: usize = 8192; + +type StreamTx = Option>; + +/// Append one output line to the captured buffer and forward it to the TUI. +/// +/// Storage stops at [`MAX_OUTPUT_BYTES`], but the caller keeps *reading* past +/// that point so the child process never blocks on a full pipe. +fn emit_line(raw: &str, tx: &StreamTx, combined: &mut String, truncated: &mut bool) { + let clean = strip_ansi(raw); + if clean.is_empty() { + return; + } + if let Some(tx) = tx { + let _ = tx.send(clean.clone()); + } + if combined.len() >= MAX_OUTPUT_BYTES { + *truncated = true; + return; + } + // Trim the final line so the buffer never overshoots the cap, however long + // a single line happens to be. + let room = MAX_OUTPUT_BYTES - combined.len(); + if clean.len() > room { + let cut = (0..=room) + .rev() + .find(|&i| clean.is_char_boundary(i)) + .unwrap_or(0); + combined.push_str(&clean[..cut]); + *truncated = true; + } else { + combined.push_str(&clean); + } + combined.push('\n'); +} + +/// Split a freshly-read chunk into complete lines, carrying any trailing +/// partial line over to the next chunk. +fn absorb( + chunk: &[u8], + partial: &mut Vec, + tx: &StreamTx, + combined: &mut String, + truncated: &mut bool, +) { + partial.extend_from_slice(chunk); + while let Some(nl) = partial.iter().position(|&b| b == b'\n') { + let line = partial.drain(..=nl).collect::>(); + let text = String::from_utf8_lossy(&line[..line.len() - 1]).into_owned(); + emit_line(&text, tx, combined, truncated); + } + // A single line longer than the cap would otherwise grow `partial` without + // bound — flush it early rather than waiting for a newline that may never + // arrive. + if partial.len() > MAX_OUTPUT_BYTES { + let text = String::from_utf8_lossy(partial).into_owned(); + emit_line(&text, tx, combined, truncated); + partial.clear(); + *truncated = true; + } +} /// Strip ANSI escape sequences and carriage returns from terminal output. /// Prevents progress-bar output (e.g. from `ollama pull`) from corrupting the TUI. @@ -173,6 +234,12 @@ impl Tool for BashTool { cmd.arg("-c") .arg(&command) .current_dir(&cwd) + // stdin defaults to *inherit*, which hands the spawned command the + // TUI's own terminal. An interactive command (`sudo`, `ssh`, a bare + // `read`) then competes with crossterm for the user's keystrokes and + // hangs until the timeout. Nothing here can answer a prompt, so give + // it EOF immediately and let the command fail fast instead. + .stdin(Stdio::null()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) // Defense in depth: if our Drop guard somehow doesn't fire, @@ -190,70 +257,67 @@ impl Tool for BashTool { let mut guard = ProcessGroupGuard::new(cmd.spawn()?); let child = guard.child_mut(); - let stdout = child + let mut stdout = child .stdout .take() .ok_or_else(|| anyhow::anyhow!("Failed to capture stdout"))?; - let stderr = child + let mut stderr = child .stderr .take() .ok_or_else(|| anyhow::anyhow!("Failed to capture stderr"))?; + // Read in fixed-size chunks rather than by line. + // + // `BufReader::lines()` accumulates until it sees a newline, so output + // with no newlines at all (`yes | tr -d '\n'`) buffered the entire + // stream into one String — the MAX_OUTPUT_BYTES check ran per line and + // never got a chance to trip. + // + // Simply capping the reader is not enough either: once we stop reading, + // the child blocks on a full pipe and never exits, so every command + // over the cap would burn the full timeout instead of finishing. + // Chunked reads let us bound what we *keep* while still draining to + // EOF, so the child always completes. let mut combined = String::new(); - let mut stdout_reader = BufReader::new(stdout).lines(); - let mut stderr_reader = BufReader::new(stderr).lines(); + let mut truncated = false; + let mut stdout_buf = vec![0u8; CHUNK_SIZE]; + let mut stderr_buf = vec![0u8; CHUNK_SIZE]; + // Partial trailing line per stream, kept as bytes so a multi-byte UTF-8 + // character split across a chunk boundary is not mangled. + let mut stdout_partial: Vec = Vec::new(); + let mut stderr_partial: Vec = Vec::new(); + let mut stdout_done = false; + let mut stderr_done = false; - // Drain both stdout and stderr concurrently, streaming each line - loop { + while !(stdout_done && stderr_done) { tokio::select! { - line = stdout_reader.next_line() => { - match line? { - None => break, - Some(l) => { - let clean = strip_ansi(&l); - if !clean.is_empty() { - if let Some(ref tx) = stream_tx { - let _ = tx.send(clean.clone()); - } - if combined.len() < MAX_OUTPUT_BYTES { - combined.push_str(&clean); - combined.push('\n'); - } - } - } + r = stdout.read(&mut stdout_buf), if !stdout_done => { + match r? { + 0 => stdout_done = true, + n => absorb( + &stdout_buf[..n], &mut stdout_partial, + &stream_tx, &mut combined, &mut truncated, + ), } } - line = stderr_reader.next_line() => { - match line? { - None => {} - Some(l) => { - let clean = strip_ansi(&l); - if !clean.is_empty() { - if let Some(ref tx) = stream_tx { - let _ = tx.send(clean.clone()); - } - if combined.len() < MAX_OUTPUT_BYTES { - combined.push_str(&clean); - combined.push('\n'); - } - } - } + r = stderr.read(&mut stderr_buf), if !stderr_done => { + match r? { + 0 => stderr_done = true, + n => absorb( + &stderr_buf[..n], &mut stderr_partial, + &stream_tx, &mut combined, &mut truncated, + ), } } } } - // Drain remaining stderr after stdout closes - while let Some(l) = stderr_reader.next_line().await? { - let clean = strip_ansi(&l); - if !clean.is_empty() { - if let Some(ref tx) = stream_tx { - let _ = tx.send(clean.clone()); - } - if combined.len() < MAX_OUTPUT_BYTES { - combined.push_str(&clean); - combined.push('\n'); - } + // Flush any trailing text that never ended in a newline. + for partial in [&mut stdout_partial, &mut stderr_partial] { + if !partial.is_empty() { + let line = String::from_utf8_lossy(partial).into_owned(); + emit_line(&line, &stream_tx, &mut combined, &mut truncated); + partial.clear(); } } @@ -262,7 +326,7 @@ impl Tool for BashTool { // doesn't try to signal a pid that has already been reaped. guard.disarm(); - if combined.len() >= MAX_OUTPUT_BYTES { + if truncated { combined.push_str("\n... (output truncated)"); } diff --git a/src/tools/powershell.rs b/src/tools/powershell.rs index 41e7672..6e6fe05 100644 --- a/src/tools/powershell.rs +++ b/src/tools/powershell.rs @@ -53,12 +53,26 @@ impl Tool for PowerShellTool { let input: Input = serde_json::from_value(input)?; let timeout_ms = input.timeout_ms.min(120_000); + // This tool executes arbitrary commands exactly like Bash, so it must + // honour the sandbox. It previously bypassed it entirely — `apply_sandbox` + // was called from bash.rs and nowhere else — so an enabled sandbox had no + // effect here at all. + if let Some(ref mode) = ctx.sandbox_mode + && let Err(reason) = + crate::sandbox::guard_unwrappable_tool(&input.command, mode, "PowerShell") + { + return Ok(ToolOutput::error(reason)); + } + use tokio::process::Command; use tokio::time::{Duration, timeout}; let fut = Command::new("pwsh") .args(["-NoProfile", "-NonInteractive", "-Command", &input.command]) .current_dir(&ctx.cwd) + // Same reason as the Bash tool: inherited stdin lets an interactive + // prompt fight the TUI for keystrokes. + .stdin(std::process::Stdio::null()) .output(); let result = timeout(Duration::from_millis(timeout_ms), fut).await; diff --git a/src/tui/app.rs b/src/tui/app.rs index 31ef975..a843df5 100644 --- a/src/tui/app.rs +++ b/src/tui/app.rs @@ -537,6 +537,15 @@ impl Drop for WatcherHandle { // ── App ─────────────────────────────────────────────────────────────────────── +/// Scrollback cap applied while the viewport is following the newest content. +/// Generous enough that no realistic session scrolls back this far, small enough +/// that a long-running agent loop cannot exhaust memory. +pub const MAX_ENTRIES: usize = 2000; + +/// Absolute ceiling, enforced even while the user is scrolled up reading history. +/// Prevents a session parked in scrollback from growing without bound. +pub const MAX_ENTRIES_HARD: usize = 10_000; + pub struct App { pub entries: Vec, /// Text currently being streamed (incomplete assistant message) @@ -1237,6 +1246,40 @@ impl App { self.follow_bottom = true; } + /// Evict the oldest chat entries once scrollback exceeds its cap. + /// + /// `entries` is *display* state only — the conversation itself lives in the + /// query engine's `messages` and is persisted to the session file — so evicting + /// here bounds memory without losing transcript data, exactly like a terminal's + /// scrollback buffer. + /// + /// Two caps, because eviction is only invisible when the viewport is pinned to + /// the newest content: + /// - Following the bottom → trim at [`MAX_ENTRIES`]; the user never sees it. + /// - Scrolled up reading history → defer to [`MAX_ENTRIES_HARD`] so we don't yank + /// content out from under them, but still refuse to grow without bound. + pub fn trim_entries(&mut self) { + let cap = if self.follow_bottom { + MAX_ENTRIES + } else { + MAX_ENTRIES_HARD + }; + if self.entries.len() <= cap { + return; + } + let excess = self.entries.len() - cap; + self.entries.drain(..excess); + + if !self.follow_bottom { + // Hard ceiling reached while reading scrollback. `scroll` counts rendered + // lines from the top, and the lines it referred to are now gone, so there + // is no honest offset to keep. Snap to the newest content instead of + // leaving the viewport at an arbitrary position. + self.follow_bottom = true; + self.scroll = 0; + } + } + // ── Streaming helpers ───────────────────────────────────────────────────── pub fn flush_streaming(&mut self) { @@ -1508,3 +1551,103 @@ fn truncate(s: &str, max: usize) -> String { format!("{}…", &s[..end]) } } + +#[cfg(test)] +mod trim_entries_tests { + use super::*; + + fn app_with(n: usize) -> App { + let mut app = App::new("claude-sonnet-4-6", std::path::Path::new("/tmp")); + app.entries = (0..n).map(|i| ChatEntry::assistant(i.to_string())).collect(); + app + } + + fn first_last(app: &App) -> (String, String) { + ( + app.entries.first().unwrap().text.clone(), + app.entries.last().unwrap().text.clone(), + ) + } + + #[test] + fn under_cap_is_left_completely_alone() { + let mut app = app_with(MAX_ENTRIES); + app.trim_entries(); + assert_eq!(app.entries.len(), MAX_ENTRIES); + assert_eq!(first_last(&app).0, "0", "must not evict while under cap"); + } + + #[test] + fn following_bottom_evicts_oldest_and_keeps_newest() { + let mut app = app_with(MAX_ENTRIES + 500); + app.follow_bottom = true; + + app.trim_entries(); + + assert_eq!(app.entries.len(), MAX_ENTRIES); + let (first, last) = first_last(&app); + assert_eq!(first, "500", "oldest 500 should have been evicted"); + assert_eq!( + last, + (MAX_ENTRIES + 499).to_string(), + "newest entry must survive" + ); + } + + /// Reading scrollback must not have content yanked out from under you at the + /// soft cap — that would make the viewport jump mid-read. + #[test] + fn scrolled_up_defers_past_the_soft_cap() { + let mut app = app_with(MAX_ENTRIES + 500); + app.follow_bottom = false; + app.scroll = 42; + + app.trim_entries(); + + assert_eq!(app.entries.len(), MAX_ENTRIES + 500, "no eviction yet"); + assert_eq!(app.scroll, 42, "viewport must not move"); + assert!(!app.follow_bottom); + } + + /// ...but the hard ceiling is absolute, so a session parked in scrollback + /// still cannot grow without bound. + #[test] + fn hard_ceiling_is_enforced_even_when_scrolled_up() { + let mut app = app_with(MAX_ENTRIES_HARD + 1); + app.follow_bottom = false; + app.scroll = 42; + + app.trim_entries(); + + assert_eq!(app.entries.len(), MAX_ENTRIES_HARD); + assert!( + app.follow_bottom, + "snapping to bottom is the only honest option once the referenced lines are gone" + ); + assert_eq!(app.scroll, 0); + } + + /// The actual regression: an agent loop appending forever must reach a steady + /// state rather than growing without bound. + #[test] + fn repeated_appends_reach_a_steady_state() { + let mut app = app_with(0); + app.follow_bottom = true; + + for i in 0..(MAX_ENTRIES * 3) { + app.entries.push(ChatEntry::assistant(i.to_string())); + app.trim_entries(); + assert!( + app.entries.len() <= MAX_ENTRIES, + "scrollback exceeded its cap at append {i}" + ); + } + + assert_eq!(app.entries.len(), MAX_ENTRIES); + assert_eq!( + app.entries.last().unwrap().text, + (MAX_ENTRIES * 3 - 1).to_string(), + "newest content must always be retained" + ); + } +} diff --git a/src/tui/run.rs b/src/tui/run.rs index 5ca7b74..5fe2009 100644 --- a/src/tui/run.rs +++ b/src/tui/run.rs @@ -419,14 +419,18 @@ async fn run_loop(mut config: Config, resume_id: Option) -> Result<()> { || crate::api::is_openai_compat_model(&config.model); if !is_non_anthropic && config.api_key.is_empty() { return Err(anyhow::anyhow!( - "ANTHROPIC_API_KEY is not set.\n\ - Set it with: export ANTHROPIC_API_KEY=sk-ant-...\n\ - To use a local model: --model ollama:\n\ - Or a cloud OpenAI-compatible model: --model groq:, --model openrouter:, ..." + "No Anthropic credential found.\n\ + RustyClaw checks, in order:\n\ + 1. ANTHROPIC_API_KEY export ANTHROPIC_API_KEY=sk-ant-...\n\ + 2. ANTHROPIC_AUTH_TOKEN an OAuth access token\n\ + 3. apiKeyHelper / RUSTYCLAW_API_KEY_FILE_DESCRIPTOR\n\ + 4. ant auth login shared with Claude Code and the official SDKs\n\ + To use a local model instead: --model ollama:\n\ + Or a cloud OpenAI-compatible model: --model groq:, --model openrouter:, ..." )); } let mut client: ApiBackend = - ApiBackend::new(&config.model, &config.api_key, &config.ollama_host)?; + ApiBackend::new_with_auth(&config.model, &config.api_key, config.auth_is_oauth, &config.ollama_host)?; // Start MCP servers (failures are logged and skipped — never fatal) let settings = crate::settings::Settings::load(&config.cwd); @@ -801,7 +805,7 @@ async fn run_loop(mut config: Config, resume_id: Option) -> Result<()> { crate::config::Config::save_user_setting("model", serde_json::Value::String(model)); system_prompt.clear(); system_prompt.push_str(&config.build_system_prompt()); - match ApiBackend::new(&config.model, &config.api_key, &config.ollama_host) { + match ApiBackend::new_with_auth(&config.model, &config.api_key, config.auth_is_oauth, &config.ollama_host) { Ok(new_client) => { client = new_client; } @@ -927,6 +931,11 @@ async fn run_loop(mut config: Config, resume_id: Option) -> Result<()> { } // Uses cached term size — no syscall per frame; updated on Resize events. + // Bound scrollback before measuring/drawing. Every path that appends to + // `app.entries` reaches the renderer through here, so this single call is + // sufficient — no need to police ~40 individual push sites. + app.trim_entries(); + { let needed = viewport_height(&app, last_term_cols, last_term_rows); if needed != current_vp_h { @@ -1991,7 +2000,7 @@ async fn handle_key(ctx: KeyCtx<'_>) -> Result<()> { ); *system_prompt = config.build_system_prompt(); // Re-create backend when switching between Anthropic ↔ Ollama - match ApiBackend::new(&config.model, &config.api_key, &config.ollama_host) { + match ApiBackend::new_with_auth(&config.model, &config.api_key, config.auth_is_oauth, &config.ollama_host) { Ok(new_client) => { *client = new_client; } diff --git a/src/voice.rs b/src/voice.rs index e50fd40..b802ffd 100644 --- a/src/voice.rs +++ b/src/voice.rs @@ -840,22 +840,19 @@ pub async fn await_voice_approval(timeout_secs: u64) -> bool { // Record for at most timeout_secs then stop automatically. let record_task = tokio::spawn(async move { - match start_recording(&backend).await { - Ok(mut child) => { - tokio::select! { - _ = stop_rx => { - if let Some(pid) = child.id() { - let _ = tokio::process::Command::new("kill") - .args(["-2", &pid.to_string()]) - .status() - .await; - } - let _ = child.wait().await; + if let Ok(mut child) = start_recording(&backend).await { + tokio::select! { + _ = stop_rx => { + if let Some(pid) = child.id() { + let _ = tokio::process::Command::new("kill") + .args(["-2", &pid.to_string()]) + .status() + .await; } - _ = child.wait() => {} + let _ = child.wait().await; } + _ = child.wait() => {} } - Err(_) => {} } }); diff --git a/tests/bash_output_bounds_tests.rs b/tests/bash_output_bounds_tests.rs new file mode 100644 index 0000000..9538ce4 --- /dev/null +++ b/tests/bash_output_bounds_tests.rs @@ -0,0 +1,119 @@ +//! Regression: BashTool must bound how much command output it buffers. +//! +//! `BufReader::lines()` accumulates bytes until it sees a newline, so a command +//! that produces a large newline-free stream (`yes | tr -d '\n'`, +//! `cat /dev/urandom | tr -d '\n'`) buffered the entire stream into a single +//! String. The MAX_OUTPUT_BYTES check ran *per line* and so never got a chance +//! to trip — memory grew until the 2-minute timeout fired, or the process died. +//! +//! The fix caps each pipe at the source with `AsyncReadExt::take`, which makes +//! the limit real regardless of whether the output contains newlines. +//! +//! Unix-only: uses `head`/`tr` and /dev/zero. + +#![cfg(unix)] + +use rustyclaw::tools::{Tool, ToolContext, bash::BashTool}; +use serde_json::json; +use std::path::PathBuf; +use tempfile::TempDir; + +/// BashTool's internal cap. Output may exceed it slightly (the final line plus +/// the truncation notice), so assertions allow generous headroom while still +/// being far below the unbounded size. +const MAX_OUTPUT_BYTES: usize = 1_000_000; + +fn ctx(dir: &TempDir) -> ToolContext { + ToolContext::new(PathBuf::from(dir.path())) +} + +/// Flatten a ToolOutput's content blocks into one string. +fn text(out: &rustyclaw::tools::ToolOutput) -> String { + out.content + .iter() + .map(|c| match c { + rustyclaw::api::types::ToolResultContent::Text { text } => text.as_str(), + }) + .collect::>() + .join("") +} + +/// 8 MB of output with **no newline at all** — the pathological shape. +/// Before the fix this buffered all 8 MB into one String. +#[tokio::test] +async fn newline_free_output_is_bounded() { + let dir = TempDir::new().unwrap(); + let out = BashTool + .execute( + json!({ + "command": "head -c 8000000 /dev/zero | tr '\\0' 'a'", + "timeout": 60000 + }), + &ctx(&dir), + ) + .await + .expect("tool should return a result, not hang or error out"); + + let len = text(&out).len(); + assert!( + len < MAX_OUTPUT_BYTES * 3, + "newline-free output must be bounded, got {len} bytes (input was 8 MB)" + ); +} + +/// The same volume split across many lines — the case the per-line check +/// already handled. Kept so a future refactor can't regress one shape while +/// fixing the other. +#[tokio::test] +async fn line_delimited_output_is_bounded() { + let dir = TempDir::new().unwrap(); + let out = BashTool + .execute( + json!({ + "command": "for i in $(seq 1 200000); do echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa; done", + "timeout": 60000 + }), + &ctx(&dir), + ) + .await + .expect("tool should return a result"); + + let len = text(&out).len(); + assert!( + len < MAX_OUTPUT_BYTES * 3, + "line-delimited output must be bounded, got {len} bytes" + ); +} + +/// Bounding the reader must not truncate ordinary small output. +#[tokio::test] +async fn small_output_is_unaffected() { + let dir = TempDir::new().unwrap(); + let out = BashTool + .execute(json!({ "command": "echo hello world" }), &ctx(&dir)) + .await + .expect("tool should return a result"); + + let body = text(&out); + assert!(body.contains("hello world"), "got: {body}"); +} + +/// stdin is redirected from /dev/null, so a command that reads stdin gets EOF +/// immediately instead of competing with the TUI for the user's keystrokes. +#[tokio::test] +async fn stdin_is_not_inherited() { + let dir = TempDir::new().unwrap(); + let out = BashTool + .execute( + json!({ "command": "cat; echo EOF_REACHED", "timeout": 10000 }), + &ctx(&dir), + ) + .await + .expect("reading stdin must not hang — it should hit EOF immediately"); + + let body = text(&out); + assert!( + body.contains("EOF_REACHED"), + "command should complete on stdin EOF, got: {body}" + ); +}