From 40dec0474542f1f9be43f9e123bd21a800b13d60 Mon Sep 17 00:00:00 2001 From: brhutchins Date: Tue, 16 Jun 2026 09:07:02 +0200 Subject: [PATCH] refactor: store api keys in a SecretString - Add local `SecretString` type, which - redacts the secret in `Debug` implementation - centralises masking implementation - Use `SecretString` for existing secrets **NOTE**: Since TinyHarness aims to avoid pulling in external dependencies where feasible, this recreates minimal functionality from the `secrecy` crate. The local implementation isn't as flexible or secure as the external crate's, but should be sufficient for the use case. **Why?**: Previously, API keys were stored as plain strings, making leaks in e.g. logs likely, and making inadvertent leaks to models relatively easy. Revealing a `SecretString` can only be done explicitly, constraining the potential leak surface. --- .gitignore | 2 + src/agent/setup.rs | 90 +++++++---- src/commands/apikey.rs | 10 +- src/commands/settings.rs | 11 +- src/main.rs | 3 +- tinyharness-lib/src/config/mod.rs | 7 +- tinyharness-lib/src/lib.rs | 2 + tinyharness-lib/src/provider/openai_compat.rs | 21 +-- .../src/provider/openai_compat_provider.rs | 7 +- tinyharness-lib/src/provider/sockudo.rs | 40 +++-- tinyharness-lib/src/secret.rs | 153 ++++++++++++++++++ tinyharness-lib/src/tools/web_search.rs | 14 +- tinyharness-lib/tests/sockudo_integration.rs | 7 +- tinyharness-ui/src/tui/app.rs | 6 +- tinyharness-ui/src/tui/terminal.rs | 11 ++ 15 files changed, 302 insertions(+), 82 deletions(-) create mode 100644 tinyharness-lib/src/secret.rs diff --git a/.gitignore b/.gitignore index b46afef..786b9f1 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,5 @@ /.tinyharness /todo +.envrc +.direnv diff --git a/src/agent/setup.rs b/src/agent/setup.rs index 79c4a52..0c51484 100644 --- a/src/agent/setup.rs +++ b/src/agent/setup.rs @@ -12,6 +12,7 @@ use std::io::{IsTerminal, Write}; +use tinyharness_lib::SecretString; use tinyharness_lib::config::{ProviderKind, Settings, load_settings, save_settings}; use tinyharness_ui::output::Output; use tinyharness_ui::style::*; @@ -30,19 +31,18 @@ pub struct SetupResult { pub enum ApiKeyChoice { /// Keep the existing key (or no key) as-is. Keep, - /// Set a new key (the new value is in the `String`). - Set(String), + /// Set a new key (the new version is in the `SecretString`) + Set(SecretString), /// Remove the existing key. Clear, } /// Display a stored API key in masked form (first 4 + last 4 chars). /// Returns "****" for short keys and "(not set)" for `None`. -pub fn mask_api_key(key: Option<&String>) -> String { +pub fn mask_api_key(key: Option<&SecretString>) -> String { match key { None => "(not set)".to_string(), - Some(k) if k.len() > 8 => format!("{}...{}", &k[..4], &k[k.len() - 4..]), - Some(_) => "****".to_string(), + Some(k) => k.masked(), } } @@ -93,7 +93,7 @@ pub fn prompt_for_api_key(out: &mut Output) -> Result { } else if trimmed.eq_ignore_ascii_case("clear") { Ok(ApiKeyChoice::Clear) } else { - Ok(ApiKeyChoice::Set(trimmed.to_string())) + Ok(ApiKeyChoice::Set(SecretString::new(trimmed))) } } @@ -258,13 +258,13 @@ pub fn save_provider_settings(kind: ProviderKind, url: &str) { /// `cli_api_key` semantics: /// * `None` — flag not passed; fall through to env/settings. /// * `Some("-")` — sentinel that clears any saved key. Returns `None`. -/// * `Some("")` — explicit opt-out from auth. Returns `Some(String::new())` +/// * `Some("")` — explicit opt-out from auth. Returns `Some(SecretString::default())` /// so the provider can be constructed without an `Authorization` header. /// * `Some(key)` — use this key (and persist it). /// /// Returns `None` when no key should be sent. Only the `OpenAiCompat` /// provider uses this value; Ollama, llama.cpp, vLLM, and Sockudo ignore it. -pub fn resolve_api_key(cli_api_key: Option<&str>, settings: &Settings) -> Option { +pub fn resolve_api_key(cli_api_key: Option<&str>, settings: &Settings) -> Option { let result = resolve_api_key_pure(cli_api_key, settings); // Persist side-effects only when the user actually passed `--api-key` on // the command line (cli_api_key is Some). The pure function is used in @@ -278,7 +278,7 @@ pub fn resolve_api_key(cli_api_key: Option<&str>, settings: &Settings) -> Option s.openai_compat_api_key = None; } _ => { - s.openai_compat_api_key = Some(raw.to_string()); + s.openai_compat_api_key = Some(SecretString::new(raw)); } } save_settings(&s); @@ -288,17 +288,20 @@ pub fn resolve_api_key(cli_api_key: Option<&str>, settings: &Settings) -> Option /// Pure (side-effect-free) API key resolution. Used by tests so they never /// touch the real `~/.config/tinyharness/settings.json`. -pub fn resolve_api_key_pure(cli_api_key: Option<&str>, settings: &Settings) -> Option { +pub fn resolve_api_key_pure( + cli_api_key: Option<&str>, + settings: &Settings, +) -> Option { match cli_api_key { Some("-") => return None, - Some("") => return Some(String::new()), - Some(k) => return Some(k.to_string()), + Some("") => return Some(SecretString::default()), + Some(k) => return Some(SecretString::new(k)), None => {} } if let Ok(env_key) = std::env::var("OPENAI_API_KEY") && !env_key.is_empty() { - return Some(env_key); + return Some(SecretString::new(env_key)); } settings .openai_compat_api_key @@ -405,7 +408,7 @@ fn prompt_for_sockudo_credentials(out: &mut Output) -> Result<(), String> { if trimmed.is_empty() { s.sockudo_app_secret.clone().unwrap_or_default() } else { - trimmed.to_string() + SecretString::new(trimmed) } }; @@ -541,12 +544,12 @@ mod tests { fn resolve_api_key_settings_fallback() { // When no CLI flag and no env var, fall back to settings. let s = Settings { - openai_compat_api_key: Some("sk-settings".to_string()), + openai_compat_api_key: Some(SecretString::new("sk-settings")), ..Settings::default() }; assert_eq!( resolve_api_key_pure(None, &s), - Some("sk-settings".to_string()) + Some(SecretString::new("sk-settings")) ); } @@ -559,7 +562,7 @@ mod tests { #[test] fn resolve_api_key_empty_string_in_settings_is_ignored() { let s = Settings { - openai_compat_api_key: Some(String::new()), + openai_compat_api_key: Some(SecretString::default()), ..Settings::default() }; assert_eq!(resolve_api_key_pure(None, &s), None); @@ -569,12 +572,12 @@ mod tests { fn resolve_api_key_cli_value_wins() { // CLI key takes precedence over env and settings. let s = Settings { - openai_compat_api_key: Some("sk-settings".to_string()), + openai_compat_api_key: Some(SecretString::new("sk-settings")), ..Settings::default() }; assert_eq!( resolve_api_key_pure(Some("sk-from-cli"), &s), - Some("sk-from-cli".to_string()) + Some(SecretString::new("sk-from-cli")) ); } @@ -582,7 +585,7 @@ mod tests { fn resolve_api_key_clear_sentinel() { // The sentinel "-" should return None (clear). let s = Settings { - openai_compat_api_key: Some("sk-settings".to_string()), + openai_compat_api_key: Some(SecretString::new("sk-settings")), ..Settings::default() }; assert_eq!(resolve_api_key_pure(Some("-"), &s), None); @@ -593,10 +596,13 @@ mod tests { // Explicit `--api-key ""` returns Some("") so the caller can // construct a no-auth provider, ignoring any stored key. let s = Settings { - openai_compat_api_key: Some("sk-settings".to_string()), + openai_compat_api_key: Some(SecretString::new("sk-settings")), ..Settings::default() }; - assert_eq!(resolve_api_key_pure(Some(""), &s), Some(String::new())); + assert_eq!( + resolve_api_key_pure(Some(""), &s), + Some(SecretString::default()) + ); } #[test] @@ -630,7 +636,7 @@ mod tests { fn mask_api_key_handles_long_keys() { // 12-char key → first 4 + "..." + last 4 assert_eq!( - mask_api_key(Some(&"abcdef123456".to_string())), + mask_api_key(Some(&SecretString::new("abcdef123456".to_string()))), "abcd...3456" ); } @@ -638,14 +644,23 @@ mod tests { #[test] fn mask_api_key_handles_short_keys() { // 8 chars or fewer → fully masked - assert_eq!(mask_api_key(Some(&"abc".to_string())), "****"); - assert_eq!(mask_api_key(Some(&"abcdefgh".to_string())), "****"); + assert_eq!( + mask_api_key(Some(&SecretString::new("abc".to_string()))), + "****" + ); + assert_eq!( + mask_api_key(Some(&SecretString::new("abcdefgh".to_string()))), + "****" + ); } #[test] fn mask_api_key_handles_exactly_9_chars() { // 9 chars is the threshold: still show first/last 4 - assert_eq!(mask_api_key(Some(&"abcdefghi".to_string())), "abcd...fghi"); + assert_eq!( + mask_api_key(Some(&SecretString::new("abcdefghi".to_string()))), + "abcd...fghi" + ); } #[test] @@ -653,12 +668,27 @@ mod tests { assert_eq!(ApiKeyChoice::Keep, ApiKeyChoice::Keep); assert_ne!(ApiKeyChoice::Keep, ApiKeyChoice::Clear); assert_eq!( - ApiKeyChoice::Set("k".to_string()), - ApiKeyChoice::Set("k".to_string()) + ApiKeyChoice::Set(SecretString::new("k")), + ApiKeyChoice::Set(SecretString::new("k")) ); assert_ne!( - ApiKeyChoice::Set("a".to_string()), - ApiKeyChoice::Set("b".to_string()) + ApiKeyChoice::Set(SecretString::new("a")), + ApiKeyChoice::Set(SecretString::new("b")) + ); + } + + #[test] + fn api_key_choice_debug_redacts_set_value() { + // The whole point of `Set(SecretString)` is that `Debug` cannot leak + // the freshly-typed key. If a future refactor unwraps the + // `SecretString` back to `String`, this test fails loudly. + let choice = ApiKeyChoice::Set(SecretString::new("sk-supersecret")); + let dbg = format!("{:?}", choice); + assert!(dbg.contains("[REDACTED]"), "expected REDACTED in {:?}", dbg); + assert!( + !dbg.contains("sk-supersecret"), + "key leaked via Debug: {:?}", + dbg ); } diff --git a/src/commands/apikey.rs b/src/commands/apikey.rs index 8ed3d61..b3abab8 100644 --- a/src/commands/apikey.rs +++ b/src/commands/apikey.rs @@ -1,5 +1,6 @@ use std::io::Write; +use tinyharness_lib::SecretString; use tinyharness_lib::config::{load_settings, save_settings}; use tinyharness_ui::output::Output; @@ -7,7 +8,7 @@ use tinyharness_ui::style::*; pub fn execute_set(out: &mut Output, key: &str) { let mut settings = load_settings(); - settings.ollama_api_key = Some(key.to_string()); + settings.ollama_api_key = Some(SecretString::new(key)); save_settings(&settings); let _ = writeln!(out, "{BOLD}Ollama API key saved.{RESET}"); } @@ -16,12 +17,7 @@ pub fn execute_show(out: &mut Output) { let settings = load_settings(); match &settings.ollama_api_key { Some(key) => { - let masked = if key.len() > 8 { - format!("{}...{}", &key[..4], &key[key.len() - 4..]) - } else { - "****".to_string() - }; - let _ = writeln!(out, "{BOLD}Ollama API key:{RESET} {masked}"); + let _ = writeln!(out, "{BOLD}Ollama API key:{RESET} {}", key.masked()); } None => { let _ = writeln!( diff --git a/src/commands/settings.rs b/src/commands/settings.rs index b814b65..653e288 100644 --- a/src/commands/settings.rs +++ b/src/commands/settings.rs @@ -48,12 +48,11 @@ fn execute_summary(out: &mut Output, settings: &tinyharness_lib::config::Setting match &settings.ollama_api_key { Some(key) => { - let masked = if key.len() > 8 { - format!("{}...{}", &key[..4], &key[key.len() - 4..]) - } else { - "****".to_string() - }; - let _ = writeln!(out, "{BOLD}│{RESET} API Key: {BLUE}{masked}{RESET}"); + let _ = writeln!( + out, + "{BOLD}│{RESET} API Key: {BLUE}{}{RESET}", + key.masked() + ); } None => { let _ = writeln!(out, "{BOLD}│{RESET} API Key: {ORANGE}not set{RESET}"); diff --git a/src/main.rs b/src/main.rs index 209aa6e..9f65919 100644 --- a/src/main.rs +++ b/src/main.rs @@ -9,6 +9,7 @@ use std::{ }; use tinyharness_lib::{ + SecretString, config::{ProviderKind, Settings, ensure_prompts_initialized, load_settings, save_settings}, context::WorkspaceContext, mode::AgentMode, @@ -121,7 +122,7 @@ fn resolve_provider_kind(args: &Args, settings: &Settings) -> ProviderKind { async fn create_provider( kind: ProviderKind, url: String, - api_key: Option, + api_key: Option, skip_health_check: bool, skip_health_check_source: &str, settings: &Settings, diff --git a/tinyharness-lib/src/config/mod.rs b/tinyharness-lib/src/config/mod.rs index ae3aba3..de56440 100644 --- a/tinyharness-lib/src/config/mod.rs +++ b/tinyharness-lib/src/config/mod.rs @@ -4,6 +4,7 @@ use std::{fmt, str::FromStr}; use serde::{Deserialize, Serialize}; +use crate::SecretString; use crate::mode::AgentMode; // ── AutoAccept Mode ──────────────────────────────────────────────────────── @@ -436,17 +437,17 @@ pub struct Settings { #[serde(default)] pub preferred_mode: AgentMode, #[serde(default)] - pub ollama_api_key: Option, + pub ollama_api_key: Option, /// API key sent as `Authorization: Bearer ` by the OpenAI-compatible /// provider (`--openai-compat`). Set via `--api-key` or `OPENAI_API_KEY` /// env var. Not used by Ollama, llama.cpp, vLLM, or Sockudo. - pub openai_compat_api_key: Option, + pub openai_compat_api_key: Option, /// Sockudo app ID for the AI Transport provider. pub sockudo_app_id: Option, /// Sockudo app key (used as auth_key in signed API requests and WebSocket URL). pub sockudo_app_key: Option, /// Sockudo app secret (used to sign API requests via HMAC-SHA256). - pub sockudo_app_secret: Option, + pub sockudo_app_secret: Option, /// Timeout in seconds for Ollama requests (default: 5) #[serde(default)] pub ollama_timeout_secs: u64, diff --git a/tinyharness-lib/src/lib.rs b/tinyharness-lib/src/lib.rs index b8f714d..76f83d5 100644 --- a/tinyharness-lib/src/lib.rs +++ b/tinyharness-lib/src/lib.rs @@ -3,6 +3,7 @@ pub mod context; pub mod image; pub mod mode; pub mod provider; +pub mod secret; pub mod session; pub mod skill; pub mod token; @@ -22,6 +23,7 @@ pub use provider::{ ChatMessage, ChatMessageResponse, Message, Provider, Role, TokenUsage, ToolCall, ToolCallFunction, ToolDefinition, }; +pub use secret::SecretString; pub use session::{Session, SessionEntry, SessionMeta, SessionStore}; pub use skill::{Skill, SkillRegistry, SkillSource, discover_skills}; pub use token::ContextWindowSize; diff --git a/tinyharness-lib/src/provider/openai_compat.rs b/tinyharness-lib/src/provider/openai_compat.rs index de60c0e..27063b9 100644 --- a/tinyharness-lib/src/provider/openai_compat.rs +++ b/tinyharness-lib/src/provider/openai_compat.rs @@ -7,8 +7,11 @@ use reqwest::Client; use serde::{Deserialize, Serialize}; use tokio_stream::StreamExt; -use crate::provider::{ - ChatMessage, ChatMessageResponse, Message, Role, ToolCall, ToolCallFunction, ToolDefinition, +use crate::{ + SecretString, + provider::{ + ChatMessage, ChatMessageResponse, Message, Role, ToolCall, ToolCallFunction, ToolDefinition, + }, }; /// Shared inner state for OpenAI-compatible providers (llama.cpp, vLLM, etc.). @@ -23,7 +26,7 @@ pub struct OpenAiCompatInner { /// Optional bearer token sent as `Authorization: Bearer ` on every /// request. Used by hosted OpenAI-compatible APIs (e.g. OpenRouter, /// Together, self-hosted gateways) that require authentication. - api_key: Option, + api_key: Option, } impl OpenAiCompatInner { @@ -32,7 +35,7 @@ impl OpenAiCompatInner { } /// Create a new inner state with an optional bearer token. - pub fn with_api_key(base_url: String, api_key: Option) -> Self { + pub fn with_api_key(base_url: String, api_key: Option) -> Self { let client = Client::builder() .connect_timeout(Duration::from_secs(15)) .read_timeout(Duration::from_secs(300)) @@ -54,7 +57,7 @@ impl OpenAiCompatInner { Box::pin(async move { let mut req = client.get(&url); if let Some(key) = &api_key { - req = req.bearer_auth(key); + req = req.bearer_auth(key.expose_secret()); } match req.send().await { Ok(resp) if resp.status().is_success() => Ok(()), @@ -97,7 +100,7 @@ impl OpenAiCompatInner { Box::pin(async move { let mut req = client.get(&url); if let Some(key) = &api_key { - req = req.bearer_auth(key); + req = req.bearer_auth(key.expose_secret()); } match req.send().await { Ok(resp) if resp.status().is_success() => { @@ -146,7 +149,7 @@ impl OpenAiCompatInner { // Spawn the streaming work on a background task tokio::spawn(async move { let _usage = - stream_chat_completions(&client, &chat_url, &body, api_key.as_deref(), &send).await; + stream_chat_completions(&client, &chat_url, &body, api_key.as_ref(), &send).await; }); Box::pin(async move { Ok(recv) }) @@ -391,12 +394,12 @@ pub async fn stream_chat_completions( client: &reqwest::Client, url: &str, body: &ChatRequest, - api_key: Option<&str>, + api_key: Option<&SecretString>, send: &tokio::sync::mpsc::Sender, ) -> Option { let mut request = client.post(url).json(body); if let Some(key) = api_key { - request = request.bearer_auth(key); + request = request.bearer_auth(key.expose_secret()); } let response = match request.send().await { Ok(r) => r, diff --git a/tinyharness-lib/src/provider/openai_compat_provider.rs b/tinyharness-lib/src/provider/openai_compat_provider.rs index 4db51d4..4116537 100644 --- a/tinyharness-lib/src/provider/openai_compat_provider.rs +++ b/tinyharness-lib/src/provider/openai_compat_provider.rs @@ -1,7 +1,10 @@ use std::future::Future; use std::pin::Pin; -use crate::provider::{ChatMessageResponse, Message, Provider, ToolDefinition}; +use crate::{ + SecretString, + provider::{ChatMessageResponse, Message, Provider, ToolDefinition}, +}; use super::openai_compat::OpenAiCompatInner; @@ -39,7 +42,7 @@ impl OpenAiCompatProvider { /// Create a new provider that sends `Authorization: Bearer ` /// on every request. Required by hosted OpenAI-compatible gateways /// (OpenRouter, Together, custom proxies, etc.). - pub fn with_api_key(base_url: String, api_key: String) -> Self { + pub fn with_api_key(base_url: String, api_key: SecretString) -> Self { OpenAiCompatProvider { inner: OpenAiCompatInner::with_api_key(base_url, Some(api_key)), static_models: None, diff --git a/tinyharness-lib/src/provider/sockudo.rs b/tinyharness-lib/src/provider/sockudo.rs index 3031d45..a2d8758 100644 --- a/tinyharness-lib/src/provider/sockudo.rs +++ b/tinyharness-lib/src/provider/sockudo.rs @@ -25,6 +25,7 @@ use sha2::Sha256; use tokio::sync::mpsc; use tokio_tungstenite::tungstenite::Message as WsMessage; +use crate::SecretString; use crate::provider::{ ChatMessage, ChatMessageResponse, Message, Provider, Role, TokenUsage, ToolCall, ToolCallFunction, ToolDefinition, @@ -50,7 +51,7 @@ pub struct SockudoProvider { base_url: String, app_id: String, app_key: String, - app_secret: String, + app_secret: SecretString, model: Arc>>, timeout_secs: u64, } @@ -60,7 +61,12 @@ impl SockudoProvider { /// /// `base_url` is the Sockudo server HTTP root (e.g. `http://127.0.0.1:6001`). /// `app_id`, `app_key`, `app_secret` are the Sockudo app credentials. - pub fn new(base_url: String, app_id: String, app_key: String, app_secret: String) -> Self { + pub fn new( + base_url: String, + app_id: String, + app_key: String, + app_secret: SecretString, + ) -> Self { let http_client = Client::builder() .connect_timeout(Duration::from_secs(15)) .read_timeout(Duration::from_secs(300)) @@ -133,8 +139,8 @@ impl SockudoProvider { let string_to_sign = format!("{method}\n{path}\n{qs}"); let signature = { - let mut mac = - HmacSha256::new_from_slice(self.app_secret.as_bytes()).expect("HMAC key invalid"); + let mut mac = HmacSha256::new_from_slice(self.app_secret.expose_secret().as_bytes()) + .expect("HMAC key invalid"); mac.update(string_to_sign.as_bytes()); hex_encode(&mac.finalize().into_bytes()) }; @@ -946,7 +952,7 @@ mod tests { "http://127.0.0.1:6001".to_string(), "app-id".to_string(), "app-key".to_string(), - "app-secret".to_string(), + SecretString::new("app-secret"), ); assert_eq!( provider.ws_url(), @@ -960,7 +966,7 @@ mod tests { "https://example.com".to_string(), "app-id".to_string(), "app-key".to_string(), - "app-secret".to_string(), + SecretString::new("app-secret"), ); assert_eq!( provider.ws_url(), @@ -974,7 +980,7 @@ mod tests { "http://127.0.0.1:6001/".to_string(), "app-id".to_string(), "app-key".to_string(), - "app-secret".to_string(), + SecretString::new("app-secret"), ); assert_eq!( provider.ws_url(), @@ -988,7 +994,7 @@ mod tests { "http://127.0.0.1:6001".to_string(), "my-app".to_string(), "key".to_string(), - "secret".to_string(), + SecretString::new("secret"), ); assert_eq!( provider.events_url(), @@ -1002,7 +1008,7 @@ mod tests { "http://127.0.0.1:6001".to_string(), "my-app".to_string(), "key".to_string(), - "secret".to_string(), + SecretString::new("secret"), ); assert_eq!(provider.health_url(), "http://127.0.0.1:6001/up/my-app"); } @@ -1013,7 +1019,7 @@ mod tests { "http://127.0.0.1:6001".to_string(), "app-id".to_string(), "my-key".to_string(), - "my-secret".to_string(), + SecretString::new("my-secret"), ); let params = provider.sign_request("POST", "/apps/app-id/events", r#"{"test":true}"#); @@ -1044,7 +1050,7 @@ mod tests { "http://127.0.0.1:6001".to_string(), "app-id".to_string(), "key".to_string(), - "secret".to_string(), + SecretString::new("secret"), ); let params = provider.sign_request("GET", "/apps/app-id/events", "body"); @@ -1062,7 +1068,7 @@ mod tests { "http://127.0.0.1:6001".to_string(), "app-id".to_string(), "key".to_string(), - "secret".to_string(), + SecretString::new("secret"), ); // Same request → same signature (when timestamp is the same second) let p1 = provider.sign_request("POST", "/apps/app-id/events", "body"); @@ -1106,7 +1112,7 @@ mod tests { "http://127.0.0.1:6001".to_string(), "app-id".to_string(), "key".to_string(), - "secret".to_string(), + SecretString::new("secret"), ); let p1 = provider.sign_request("POST", "/apps/app-id/events", "body1"); let p2 = provider.sign_request("POST", "/apps/app-id/events", "body2"); @@ -1148,7 +1154,7 @@ mod tests { "http://127.0.0.1:6001".to_string(), "app-id".to_string(), "key".to_string(), - "secret".to_string(), + SecretString::new("secret"), ); let p1 = provider.sign_request("POST", "/apps/app-id/events", "body"); let p2 = provider.sign_request("GET", "/apps/app-id/events", "body"); @@ -1307,7 +1313,7 @@ mod tests { "http://127.0.0.1:6001".to_string(), "app".to_string(), "key".to_string(), - "secret".to_string(), + SecretString::new("secret"), ); assert!(p.current_model().is_none()); p.select_model("gpt-4".to_string()); @@ -1320,7 +1326,7 @@ mod tests { "http://127.0.0.1:6001".to_string(), "app".to_string(), "key".to_string(), - "secret".to_string(), + SecretString::new("secret"), ); assert_eq!(p.timeout_secs, 120); p.set_timeout(60); @@ -1412,7 +1418,7 @@ mod tests { "http://127.0.0.1:6001".to_string(), "app-id".to_string(), "key".to_string(), - "secret".to_string(), + SecretString::new("secret"), ); let data = serde_json::json!({ diff --git a/tinyharness-lib/src/secret.rs b/tinyharness-lib/src/secret.rs new file mode 100644 index 0000000..6b07f78 --- /dev/null +++ b/tinyharness-lib/src/secret.rs @@ -0,0 +1,153 @@ +use serde::{Deserialize, Serialize, Serializer}; + +/// Minimal no-new-dependency replacement for `secrecy::SecretString`. +/// Exposes the secret only on explicit `.expose_secret()` calls. The secret +/// cannot be implicitly leaked by the `Debug` implementation, e.g. in logs. +/// +/// Retains serialization/deserialization via `serde`. +/// +/// Deviations from `secrecy`: +/// 1. Does not zero on `Drop`: the heap bytes remain after deallocation, and, +/// e.g., a core dump could contain leaked secrets. +/// 2. Implements `Clone`: `secrecy` uses its own `SecretBox` smart pointer for +/// space efficiency. Since we can reasonably expect only a handful of +/// secrets in memory for TinyHarness, that optimization is not worth the +/// added complexity. +#[derive(Clone, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct SecretString(String); + +impl SecretString { + pub fn new(s: impl Into) -> Self { + Self(s.into()) + } + + /// Expose the underlying secret. Use this as the point the actual value is + /// required (e.g., in constructing an HTTP request). Do not log the exposed + /// value. + pub fn expose_secret(&self) -> &str { + &self.0 + } + + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } + + pub fn len(&self) -> usize { + self.0.len() + } + + pub fn masked(&self) -> String { + if self.0.is_ascii() && self.len() > 8 { + format!("{}...{}", &self.0[..4], &self.0[self.len() - 4..]) + } else { + "****".to_string() + } + } +} + +impl std::fmt::Debug for SecretString { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("[REDACTED]") + } +} + +impl Serialize for SecretString { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_str(&self.0) + } +} + +impl<'de> Deserialize<'de> for SecretString { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + String::deserialize(deserializer).map(SecretString) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn debug_redacts_value() { + let s = SecretString::new("sk-supersecret"); + assert_eq!(format!("{:?}", s), "[REDACTED]"); + assert!(!format!("{:?}", s).contains("sk-supersecret")); + } + + #[test] + fn expose_secret_returns_inner() { + let s = SecretString::new("sk-supersecret"); + assert_eq!(s.expose_secret(), "sk-supersecret"); + } + + #[test] + fn is_empty_and_len() { + assert!(SecretString::new("").is_empty()); + assert_eq!(SecretString::new("").len(), 0); + assert!(!SecretString::new("abc").is_empty()); + assert_eq!(SecretString::new("abc").len(), 3); + } + + #[test] + fn masked_long_ascii() { + assert_eq!(SecretString::new("abcdef123456").masked(), "abcd...3456"); + } + + #[test] + fn masked_short_ascii() { + assert_eq!(SecretString::new("abc").masked(), "****"); + assert_eq!(SecretString::new("abcdefgh").masked(), "****"); + } + + #[test] + fn masked_exactly_9_chars() { + // 9 chars is the threshold: still show first/last 4 + assert_eq!(SecretString::new("abcdefghi").masked(), "abcd...fghi"); + } + + #[test] + fn masked_non_ascii_falls_through_to_stars() { + // Non-ASCII keys of any length are masked fully, since slicing on a + // non-ASCII boundary would panic and revealing a slice of UTF-8 bytes + // could split a code point. + assert_eq!(SecretString::new("пароль12345").masked(), "****"); + } + + #[test] + fn clone_yields_equal_value() { + let s = SecretString::new("sk-supersecret"); + assert_eq!(s.clone(), s); + } + + #[test] + fn default_is_empty() { + let s = SecretString::default(); + assert!(s.is_empty()); + assert_eq!(s.expose_secret(), ""); + } + + #[test] + fn serde_round_trip_preserves_value() { + let s = SecretString::new("sk-supersecret"); + let json = serde_json::to_string(&s).unwrap(); + let back: SecretString = serde_json::from_str(&json).unwrap(); + assert_eq!(back, s); + assert_eq!(back.expose_secret(), "sk-supersecret"); + } + + #[test] + fn serde_wire_format_is_bare_string() { + // Pin the on-disk format: a bare JSON string, not a wrapper object. + // This is what guarantees `settings.json` keeps loading with no + // migration. + let s = SecretString::new("sk-supersecret"); + let v = serde_json::to_value(&s).unwrap(); + assert_eq!(v, serde_json::Value::String("sk-supersecret".to_string())); + } +} diff --git a/tinyharness-lib/src/tools/web_search.rs b/tinyharness-lib/src/tools/web_search.rs index 6e5f165..2d72bd5 100644 --- a/tinyharness-lib/src/tools/web_search.rs +++ b/tinyharness-lib/src/tools/web_search.rs @@ -1,8 +1,8 @@ use std::collections::HashMap; use crate::config::load_settings; -use crate::extract_args; use crate::tools::tool::{BoxFuture, ToolCategory, build_string_params_schema, make_tool}; +use crate::{SecretString, extract_args}; pub fn web_search_tool_entry() -> crate::tools::tool::Tool { make_tool( @@ -32,7 +32,7 @@ pub fn web_fetch_tool_entry() -> crate::tools::tool::Tool { } /// Load the Ollama API key from settings, returning an error string if not set. -fn get_api_key() -> Result { +fn get_api_key() -> Result { let settings = load_settings(); settings .ollama_api_key @@ -62,7 +62,10 @@ fn web_search_tool(args: HashMap) -> BoxFuture<'static, String> let resp = match client .post("https://ollama.com/api/web_search") - .header("Authorization", format!("Bearer {}", api_key)) + .header( + "Authorization", + format!("Bearer {}", api_key.expose_secret()), + ) .json(&body) .send() .await @@ -129,7 +132,10 @@ fn web_fetch_tool(args: HashMap) -> BoxFuture<'static, String> { let resp = match client .post("https://ollama.com/api/web_fetch") - .header("Authorization", format!("Bearer {}", api_key)) + .header( + "Authorization", + format!("Bearer {}", api_key.expose_secret()), + ) .json(&body) .send() .await diff --git a/tinyharness-lib/tests/sockudo_integration.rs b/tinyharness-lib/tests/sockudo_integration.rs index 6a8705d..3502a34 100644 --- a/tinyharness-lib/tests/sockudo_integration.rs +++ b/tinyharness-lib/tests/sockudo_integration.rs @@ -18,7 +18,10 @@ use std::time::{Duration, SystemTime, UNIX_EPOCH}; use futures_util::{SinkExt, StreamExt}; use hmac::{Hmac, Mac}; use sha2::Sha256; -use tinyharness_lib::provider::{Message, Provider, Role, sockudo::SockudoProvider}; +use tinyharness_lib::{ + SecretString, + provider::{Message, Provider, Role, sockudo::SockudoProvider}, +}; use tokio_tungstenite::tungstenite::Message as WsMessage; type HmacSha256 = Hmac; @@ -34,7 +37,7 @@ fn make_provider() -> SockudoProvider { SOCKUDO_URL.to_string(), APP_ID.to_string(), APP_KEY.to_string(), - APP_SECRET.to_string(), + SecretString::new(APP_SECRET), ) } diff --git a/tinyharness-ui/src/tui/app.rs b/tinyharness-ui/src/tui/app.rs index 5d292a6..d591b96 100644 --- a/tinyharness-ui/src/tui/app.rs +++ b/tinyharness-ui/src/tui/app.rs @@ -1619,7 +1619,11 @@ mod tests { fn make_app() -> TuiApp { let backend = TestBackend::new(Size::new(80, 24)); - let terminal = Terminal::new(backend).unwrap(); + let mut terminal = Terminal::new(backend).unwrap(); + // `Terminal::new` discovers size from the real terminal/env vars and ignores the writer's + // size. Pin it explicitly so layout- and scroll-dependent assertions don't flake on + // terminals of unexpected size. + terminal.set_size(Size::new(80, 24)); let (user_action_tx, _user_action_rx) = mpsc::channel(); let (_, agent_event_rx) = mpsc::channel(); TuiApp::new(terminal, user_action_tx, agent_event_rx).unwrap() diff --git a/tinyharness-ui/src/tui/terminal.rs b/tinyharness-ui/src/tui/terminal.rs index b074ba3..6fe3e8d 100644 --- a/tinyharness-ui/src/tui/terminal.rs +++ b/tinyharness-ui/src/tui/terminal.rs @@ -247,6 +247,17 @@ impl Terminal { .unwrap_or_else(|_| Size::from_env().unwrap_or_else(Size::default_size)); } + /// Override the cached terminal size. + /// + /// In production, the size is discovered via `ioctl`/`COLUMNS`/`LINES`. + /// Tests construct a `Terminal` with a writer that has no real size + /// (e.g. `TestWriter`) or with a `TestBackend` whose size is ignored by + /// `new()`. This lets them pin a deterministic size so layout-dependent + /// assertions don't flake based on the host terminal. + pub fn set_size(&mut self, size: Size) { + self.size = size; + } + /// Get the current terminal size. pub fn size(&self) -> Size { self.size