From c9205685c1d4a7206a2ced34434ccd5eb0347506 Mon Sep 17 00:00:00 2001 From: Yeti Paw <22755327+ForkedInTime@users.noreply.github.com> Date: Sun, 2 Aug 2026 11:51:21 -0700 Subject: [PATCH 1/7] fix(qa): bound SSE streams and TUI scrollback; remove NaN abort path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three production-readiness fixes ahead of wider deployment. 1. SSE streams could hang forever (src/api) Both streaming backends awaited the next event with no bound. Only connect_timeout was set, and deliberately no overall request timeout — correct, since legitimate streams run for minutes. But that left no guard at all once connected: a silently dropped TCP connection (NAT idle reaper, laptop sleep, VPN drop) parked the read future forever. The TUI hung with no error and no recovery short of killing it. Adds next_sse_event(), an *inter-event* timeout (120s) shared by the Anthropic backend and the OpenAI-compat backend — the latter also serves Ollama, so all three streaming paths are covered by one guard. A healthy connection always delivers a delta, ping, or keepalive well inside that window, so valid long streams are unaffected. 2. TUI scrollback grew without bound (src/tui) input_history was capped at 500; entries never was. A long agent loop grew it monotonically, taking render cost up with it. entries is display state only — the transcript lives in the query engine's messages and is persisted to the session file — so eviction loses no conversation data, exactly like terminal scrollback. Trims at 2000 while following the newest content (invisible to the user), and defers to a hard 10000 ceiling while scrolled up so history is not yanked out mid-read. Enforced at the single pre-draw choke point every mutation path already passes through, rather than at ~40 push sites. 3. Cost summary could abort the process (src/cost.rs) sort_by used partial_cmp().unwrap() on f64. Release sets panic = "abort", so a non-finite cost would kill the session just to render a report. NaN is unreachable today; total_cmp costs nothing and removes the failure mode permanently. Also applies the 4 outstanding clippy lints (collapsible if-let chains, match-to-if-let), leaving clippy --all-targets --all-features clean. Adds 11 tests. Each was verified to fail against its reintroduced bug: the cost test panics at the unwrap, 3 of 5 scrollback tests fail when eviction is neutered, and the stall test hangs until killed (exit 124) without the timeout — the production symptom exactly. Suite: 454 passed, 0 failed. Release builds at 19.1 MB. Co-Authored-By: Arch Linux --- Cargo.toml | 3 + src/api/mod.rs | 120 ++++++++++++++++++++++++++++- src/api/openai_compat.rs | 4 +- src/browser/approval_gate.rs | 16 ++-- src/browser/browse_loop.rs | 8 +- src/cost.rs | 35 ++++++++- src/tui/app.rs | 143 +++++++++++++++++++++++++++++++++++ src/tui/run.rs | 5 ++ src/voice.rs | 23 +++--- 9 files changed, 326 insertions(+), 31 deletions(-) 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..73f18ea 100644 --- a/src/api/mod.rs +++ b/src/api/mod.rs @@ -21,6 +21,40 @@ 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 { @@ -122,8 +156,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; } @@ -358,3 +391,86 @@ impl ApiBackend { } } } + +#[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/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/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/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..9e8e17a 100644 --- a/src/tui/run.rs +++ b/src/tui/run.rs @@ -927,6 +927,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 { 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(_) => {} } }); From 96545c60619c8674a8b12ad6c28674f9e56b986a Mon Sep 17 00:00:00 2001 From: Yeti Paw <22755327+ForkedInTime@users.noreply.github.com> Date: Sun, 2 Aug 2026 14:30:22 -0700 Subject: [PATCH 2/7] fix(security): close PowerShell approval+sandbox bypass; fail closed on bad mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 of the phased review (security & sandbox boundary, 1,286 LOC). Five files, none of which had inline tests. Zero panics in scope. 1. PowerShell bypassed BOTH security controls (most severe) `PowerShellTool` is in the live tool registry (tools/mod.rs:453) and executes arbitrary commands, but: - "PowerShell" was absent from SENSITIVE_TOOLS, so check_with_input returned Allow on the non-sensitive early-return — no approval prompt at all, ever. - apply_sandbox was called from bash.rs and nowhere else, so an enabled sandbox had no effect on it. On any machine with pwsh installed, the model could run shell commands unprompted and unsandboxed. Explicit deny rules still applied (they are checked before the early return), but the default posture was open. Adds PowerShell to SENSITIVE_TOOLS, teaches rule_matches and describe_tool_call about it, and routes it through a new sandbox::guard_unwrappable_tool. The namespace wrappers hard-code /bin/sh -c and cannot wrap a PowerShell script, so that gate applies pattern blocking in every mode and fails closed under bwrap/firejail rather than running outside the sandbox the user enabled. 2. apply_sandbox failed OPEN on an unrecognised mode The `_` arm returned the command unchanged — unsandboxed AND skipping strict_check — while the UI still reported the sandbox as enabled. Reachable in practice: `/sandbox enable` validates the mode string but settings.json does not, so a typo in `sandboxMode` silently disabled the sandbox. Now fails closed and names the offending value. 3. firejail silently ignored sandbox_allow_network firejail_wrap took no allow_network parameter, so firejail mode always had full egress while bwrap honoured the setting — the same config meaning two different things. Adds --net=none. 4. Bash: unbounded output buffering (OOM) BufReader::lines() accumulates until a newline, so newline-free output (`yes | tr -d '\n'`) buffered the whole stream into one String; the MAX_OUTPUT_BYTES check ran per line and never tripped. Capping the reader alone is wrong — the child then blocks on a full pipe and every command over the cap burns the whole timeout (measured: 60s vs 1.2s). Switches to chunked reads so output is bounded while the pipe keeps draining and the child still exits. Line splitting keeps partial lines as bytes so multi-byte UTF-8 spanning a chunk boundary is not mangled. 5. Bash + PowerShell inherited the TUI's stdin stdin defaulted to inherit, so an interactive command (sudo, ssh, a bare `read`) competed with crossterm for the user's keystrokes and hung until the timeout. Redirected from /dev/null so it hits EOF instead. Adds 19 tests. Each verified to fail against its reintroduced bug: removing PowerShell from SENSITIVE_TOOLS fails 3, restoring the fail-open arm and dropping the firejail flag fails 3, and the output-bounds tests regress from 1.2s to a 60s timeout. Suite: 486 passed, 0 failed. Clippy clean. Deferred to the tracker (not fixed here): strict_check is a substring blocklist and is the automatic fallback on macOS/Windows where neither bwrap nor firejail exists; bwrap omits --new-session; hooks have no timeout, no output cap, and treat both a spawn failure and a signal-killed hook as "allow". Co-Authored-By: Arch Linux --- src/permissions/mod.rs | 89 +++++++++++++++- src/sandbox.rs | 138 ++++++++++++++++++++++++- src/tools/bash.rs | 162 +++++++++++++++++++++--------- src/tools/powershell.rs | 14 +++ tests/bash_output_bounds_tests.rs | 119 ++++++++++++++++++++++ 5 files changed, 467 insertions(+), 55 deletions(-) create mode 100644 tests/bash_output_bounds_tests.rs 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/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/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/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}" + ); +} From 8b3feea947cf43a76651a2e76401c71731b40e6e Mon Sep 17 00:00:00 2001 From: Yeti Paw <22755327+ForkedInTime@users.noreply.github.com> Date: Sun, 2 Aug 2026 14:41:26 -0700 Subject: [PATCH 3/7] feat(auth): resolve credentials like the official SDKs; support OAuth tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RustyClaw read ANTHROPIC_API_KEY and nothing else, so it ignored credentials the user may already have configured for Claude Code, the official SDKs, or the `ant` CLI — all of which share one documented resolution order. Adds src/auth.rs implementing that order: ANTHROPIC_API_KEY → ANTHROPIC_AUTH_TOKEN → `ant auth login` profile RustyClaw's own explicit mechanisms (RUSTYCLAW_API_KEY_FILE_DESCRIPTOR, apiKeyHelper) keep their existing position between the env vars and the profile: explicit local configuration should beat ambient machine state, and nothing that worked before changes behaviour. Wire format now follows the credential kind. A static key goes in `x-api-key`; an OAuth access token goes in `Authorization: Bearer` and additionally requires the `oauth-2025-04-20` beta. Sending both auth headers is rejected by the API, so exactly one is ever set. The beta is merged into the per-request `anthropic-beta` rather than set as a default header, because reqwest's `header()` appends — a default plus a per-request value would send the field twice. Profile tokens are read via `ant auth print-credentials --access-token` rather than by parsing credentials/.json. That command refreshes the short-lived token before printing, so there is no OAuth refresh flow to implement, and we stay on a supported interface instead of an on-disk format that is an implementation detail. Bounded at 10s so a wedged binary cannot hang startup. Deliberate divergence from the SDKs: an empty ANTHROPIC_API_KEY="" falls through to the next source with a warning, rather than winning its slot and authenticating with an empty key (which 401s confusingly). /doctor now reports which source won and surfaces the "stale env var is shadowing your profile" trap, which otherwise silently sends requests to a different org/workspace. Resolution is a pure function over an injected AuthEnv, so ordering is tested without mutating process env (which races under the parallel harness) or requiring `ant` on PATH. Adds 18 tests. Verified live against the real API: - ANTHROPIC_API_KEY + ANTHROPIC_AUTH_TOKEN both set → "invalid x-api-key" (key wins, sent as x-api-key) - ANTHROPIC_API_KEY="" + bogus token → "OAuth access token is invalid" (empty key ignored; token sent as bearer and recognised AS an OAuth token, confirming the beta header is present) Suite: 522 passed, 0 failed. Clippy clean. KNOWN GAP — not yet explained: with ANTHROPIC_API_KEY fully *unset* (rather than empty) a bogus ANTHROPIC_AUTH_TOKEN does not produce the expected 401. Those two paths should be identical in resolve_stage. Something downstream (model routing, or a second config path in headless -p mode) appears to be intervening. Do not treat OAuth as fully verified until this is understood. Workload Identity Federation (4th in the documented chain) is not implemented. Co-Authored-By: Arch Linux --- src/api/mod.rs | 138 +++++++++++++- src/auth.rs | 443 ++++++++++++++++++++++++++++++++++++++++++++ src/commands/mod.rs | 24 ++- src/config.rs | 51 ++++- src/lib.rs | 1 + src/main.rs | 1 + src/query_engine.rs | 10 +- src/sdk/session.rs | 2 +- src/tui/run.rs | 18 +- 9 files changed, 664 insertions(+), 24 deletions(-) create mode 100644 src/auth.rs diff --git a/src/api/mod.rs b/src/api/mod.rs index 73f18ea..72878e1 100644 --- a/src/api/mod.rs +++ b/src/api/mod.rs @@ -61,14 +61,46 @@ 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() @@ -86,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 { @@ -96,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()); @@ -129,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()); @@ -304,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)?)) @@ -392,6 +462,62 @@ 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::*; diff --git a/src/auth.rs b/src/auth.rs new file mode 100644 index 0000000..3524076 --- /dev/null +++ b/src/auth.rs @@ -0,0 +1,443 @@ +//! 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(10); + +impl AuthEnv for ProcessAuthEnv { + fn var(&self, key: &str) -> Option { + std::env::var(key).ok() + } + + fn ant_access_token(&self) -> Option { + // `--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 { + run_ant(&["auth", "status"]).is_some() + } +} + +/// 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); + } + + #[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/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/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..8508635 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; 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/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/tui/run.rs b/src/tui/run.rs index 9e8e17a..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; } @@ -1996,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; } From 91399e0aaf93dc0952853e37aee8c3ed76320d95 Mon Sep 17 00:00:00 2001 From: Yeti Paw <22755327+ForkedInTime@users.noreply.github.com> Date: Sun, 2 Aug 2026 14:50:47 -0700 Subject: [PATCH 4/7] fix(auth): allow the full credential chain in .env; close OAuth verification gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the "known gap" flagged in 8b3feea. There was no bug in the resolver — the test methodology was wrong. `env -u ANTHROPIC_API_KEY` does not produce an absent variable, because load_dotenv_auto() then populates it from ~/.env (it only skips keys where `std::env::var(key).is_err()`). Setting ANTHROPIC_API_KEY="" instead leaves it *set*, so dotenv declines to overwrite it and the empty value falls through to the token. That is why the two cases diverged. Re-verified with HOME redirected to an empty directory so no ~/.env exists and the variable is genuinely absent: API_KEY=Err(NotPresent), AUTH_TOKEN set → resolved is_oauth=true, source=ANTHROPIC_AUTH_TOKEN → API returns "OAuth access token is invalid" Both credential paths and their precedence are now confirmed end-to-end against the live API. OAuth support is verified, not provisional. The investigation did surface a real defect: SAFE_ENV_KEYS (the .env allowlist) contained ANTHROPIC_API_KEY but not ANTHROPIC_AUTH_TOKEN or ANTHROPIC_PROFILE. A project authenticating with an OAuth token from .env would have had it silently dropped and fallen back to whatever key was in the ambient environment — the exact confusion this feature exists to remove. Both are now allowed. ANTHROPIC_BASE_URL stays excluded, with a comment saying why: it redirects every API call, so a hostile repo .env could point real credentials at an attacker-controlled host. Adds 2 tests: one asserting the allowlist covers the whole credential chain, one asserting the base-URL redirect stays out of it. Suite: 524 passed, 0 failed. Clippy clean. Co-Authored-By: Arch Linux --- src/main.rs | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index 8508635..26c378c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -397,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 @@ -1389,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. /// From 6eb9e6879b64ad30c17d580e57833df877e85f30 Mon Sep 17 00:00:00 2001 From: Yeti Paw <22755327+ForkedInTime@users.noreply.github.com> Date: Sun, 2 Aug 2026 15:00:58 -0700 Subject: [PATCH 5/7] fix(security): hooks fail closed when a gate cannot be evaluated; bound them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 findings 1.9 and 1.10. `execute_hook` had two silent fail-open paths and no resource bounds at all, on a code path that runs before every tool call. Fail-open #1 — spawn failure returned allow() A hook that could not start (missing binary, typo, E2BIG) hit `HookResult::allow()` after a `tracing::warn!` the user never sees in the TUI. A PreToolUse hook is a gate; one that never ran has not approved anything, so a broken gate silently disabled itself. Fail-open #2 — signal-killed hooks read as success `status.code().unwrap_or(0)` maps None (terminated by signal) to exit 0, the documented "allow" code. A PreToolUse hook killed by the OOM killer — or by anything else that can signal it — was read as approval. This is the one an attacker would reach for. Both now route through `hook_unevaluable`, which fails closed **only for gating events**. `is_gating_event` is `PreToolUse` alone: it is the only event that can block a tool call, so it is the only one where an unevaluable hook is a security-relevant outcome. Notification, Stop, PostToolUse and friends stay non-blocking — failing closed there would break sessions for no safety benefit. The documented exit-code contract is unchanged: 0 allows, 2 blocks, other non-zero stays a non-blocking error. Fail-closed applies to hooks that could not be *evaluated*, not to hooks that ran and reported failure. Resource bounds (1.9), all previously absent: - 60s timeout. A hook waiting on input, a network call, or a lock blocked the agent forever with no diagnostic. On timeout the whole process group is killed so anything the hook spawned dies with it. - stdout/stderr capped at 256 KB per stream, but still drained to EOF — capping without draining would leave the hook blocked on a full pipe until the timeout, turning a fast hook into a 60s stall. Both pipes are read concurrently; draining one to EOF first deadlocks if the hook fills the other. - stdin from /dev/null. Inherited stdin let a hook that reads input compete with the TUI for the user's keystrokes. - env values capped at 64 KB. Linux limits one env entry to ~128 KB (MAX_ARG_STRLEN), so a large TOOL_INPUT pushed spawn to E2BIG — which, under the old fail-open, meant the PreToolUse gate was skipped precisely on the largest tool calls. Verified: removing the cap makes the regression test fail. Adds 12 tests. Verified against each reintroduced bug: restoring `unwrap_or(0)` fails the signal test, restoring fail-open fails 2, removing the env cap fails the oversized-input test. One test was initially too weak to catch the env-cap regression — it asserted only that the call was blocked, which is true whether the hook ran and blocked or spawn failed and fail-closed caught it. It now asserts the block did not come from the fail-closed path. Suite: 548 passed, 0 failed. Clippy clean. Co-Authored-By: Arch Linux --- src/hooks.rs | 399 +++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 387 insertions(+), 12 deletions(-) diff --git a/src/hooks.rs b/src/hooks.rs index 46ab371..c9936a4 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,235 @@ 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. + #[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. + #[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()); + } +} From 9fb5714a790e6ad23203f855ebfe8da63b5c4db0 Mon Sep 17 00:00:00 2001 From: Yeti Paw <22755327+ForkedInTime@users.noreply.github.com> Date: Sun, 2 Aug 2026 15:03:52 -0700 Subject: [PATCH 6/7] fix(auth): never spawn a bare `ant` unless a profile actually exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the Windows CI regression introduced by 8b3feea. `main` was green on windows-latest; this branch was not. Root cause is a name collision. `ant` is Apache Ant on many systems — including the GitHub windows-latest runner image. Credential resolution called `Command::new("ant")` unconditionally whenever no API key was set, so on every credential-less startup RustyClaw executed whatever `ant` happened to be on PATH: an unrelated build tool, twice, each with a 10s timeout. That delay pushed `test_health_check_via_headless` past its 10s response window (the job ran 77s before failing). Wrong for reasons beyond CI: executing an arbitrary PATH binary on startup is a poor default even when it is harmless, and it is pure cost for the common case where the user has never run `ant auth login`. Now gated on the credentials directory existing — `$ANTHROPIC_CONFIG_DIR`, else `~/.config/anthropic` on Unix or `%APPDATA%\Anthropic` on Windows. A cheap `is_dir()` replaces a process spawn, so nothing is executed unless the real CLI has actually stored a profile on this machine. The same gate serves `ant_profile_present`, which no longer needs to shell out at all. Timeout also tightened 10s → 5s as defence in depth. Verified: with no credential env and a nonexistent config dir, the previously-failing sdk_integration test passes locally in 3.9s. Adds 2 tests — one asserting no subprocess is attempted when the profile directory is absent, one covering the ANTHROPIC_CONFIG_DIR override. Suite: 552 passed, 0 failed. Clippy clean. Co-Authored-By: Arch Linux --- src/auth.rs | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 87 insertions(+), 2 deletions(-) diff --git a/src/auth.rs b/src/auth.rs index 3524076..ed358b5 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -221,7 +221,41 @@ pub fn resolve_profile() -> Option { pub struct ProcessAuthEnv; /// `ant` is a local CLI, but a wedged binary must not hang startup forever. -const ANT_TIMEOUT: Duration = Duration::from_secs(10); +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 { @@ -229,6 +263,9 @@ impl AuthEnv for ProcessAuthEnv { } 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. @@ -236,7 +273,7 @@ impl AuthEnv for ProcessAuthEnv { } fn ant_profile_present(&self) -> bool { - run_ant(&["auth", "status"]).is_some() + ant_profile_dir_exists() } } @@ -430,6 +467,54 @@ mod tests { 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()); From 60cec2619350bd9a6d8154972fd7bdd155eb1cbf Mon Sep 17 00:00:00 2001 From: Yeti Paw <22755327+ForkedInTime@users.noreply.github.com> Date: Sun, 2 Aug 2026 15:14:22 -0700 Subject: [PATCH 7/7] test(hooks): gate signal-termination tests to unix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows CI failure from 6eb9e68. `ExitStatus::code()` returns `Some` on Windows for every exit path — there is no POSIX signal termination — so the `unwrap_or(0)` fail-open being tested cannot occur there, and `kill -9 $$` does not produce a signal-terminated child. Both the defect and the regression test are Unix-only concepts. The production guard itself needs no cfg: on Windows the `else` branch is simply unreachable. The rest of the hooks suite is portable and passes on windows-latest (196 passed, this was the only failure). Co-Authored-By: Arch Linux --- src/hooks.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/hooks.rs b/src/hooks.rs index c9936a4..d292c68 100644 --- a/src/hooks.rs +++ b/src/hooks.rs @@ -591,6 +591,10 @@ mod tests { /// `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( @@ -610,6 +614,7 @@ mod tests { } /// 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.