From f2468b8bfab6a427e37abbc6abfe3e11a92213e0 Mon Sep 17 00:00:00 2001 From: Val Alexander <68980965+BunsDev@users.noreply.github.com> Date: Sat, 30 May 2026 22:45:10 -0500 Subject: [PATCH] fix(security): fail closed for unknown agent access tiers (#13) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tool-filter pipeline in `filter_tools_for_agent` matched on the `access` string with a permissive default arm: _ => tools, // "full" — allow all tools unchanged so any unrecognized value (typo, case mismatch, whitespace) silently granted the full tool set — write, exec, everything. That directly contradicts the opt-in semantics of `DEFAULT_FAMILIAR_ACCESS = "read-only"` and meant a typo in `~/.coven/familiars.toml` or `settings.json` could quietly hand a familiar `Bash`/`Edit`/`Write` access. This commit: - Adds `coven_shared::canonicalize_access_tier` to normalize case and surrounding whitespace silently (so `"READ-ONLY"`, `" full "`, etc. map to the canonical tier without complaint). - Adds `coven_shared::resolve_access_tier` as the single chokepoint that returns one of the three canonical tiers, emitting a one-line stderr warning when it falls back to the default for a truly unknown value. - Rewrites `filter_tools_for_agent` to route through `resolve_access_tier` and split the read-only / search-only branches into named helpers, with an `unreachable!()` arm that documents the canonicalization contract. - Updates `CovenFamiliar::resolved_access` to return `&'static str` and canonicalize through the same helper, so unknown values from `~/.coven/familiars.toml` are normalized at load. - Updates `docs/familiars.md` to describe the silent case/whitespace normalization vs. the loud fail-closed for unknown values, and corrects the `search-only` tier description to reflect what the code actually allows (`Grep`, `Glob`, `Read`, `WebSearch`, `WebFetch`) instead of the inaccurate "no filesystem access". Tests added in both crates: - `claurst_core::coven_shared::tests` — canonicalize accepts canonical forms, normalizes case/whitespace, rejects unknown strings; resolve falls back to default on unknown, passes canonical through. - `claurst::tests` (main.rs) — full passes through unchanged, read-only excludes Bash/Edit/Write/NotebookEdit, search-only stays within the documented whitelist, case/whitespace canonicalize silently, and genuinely unknown strings (`"readonly"`, `"i-am-evil"`, `""`, `"writeable"`, `"full-access"`) fail closed to read-only. Fixes #13. Co-Authored-By: OpenCoven --- docs/familiars.md | 6 +- src-rust/crates/cli/src/main.rs | 160 +++++++++++++++++++---- src-rust/crates/core/src/coven_shared.rs | 144 +++++++++++++++++++- 3 files changed, 277 insertions(+), 33 deletions(-) diff --git a/docs/familiars.md b/docs/familiars.md index 0d548af..8fd6034 100644 --- a/docs/familiars.md +++ b/docs/familiars.md @@ -171,8 +171,10 @@ The `access` field controls **which tools** a familiar may invoke once you selec | Tier | What the familiar can do | Typical role | |---|---|---| | `full` | Read, write, and execute — full tool set (Edit/Write/Bash/etc.) | Build-tier familiars: `cody`, `nova`, `kitty` | -| `read-only` | Read & search the workspace, no writes or shell. **Default.** | Research/strategy familiars: `sage`, `astra`, `echo` | -| `search-only` | Web/search lookups only — no filesystem access | Pure-research personas with no codebase context | +| `read-only` | Read & search the workspace plus `AskUserQuestion`, no writes or shell. **Default.** | Research/strategy familiars: `sage`, `astra`, `echo` | +| `search-only` | Narrow read+search whitelist: `Grep`, `Glob`, `Read`, `WebSearch`, `WebFetch`. No writes or shell. | Pure-research personas with minimal codebase footprint | + +> **Unknown values fail closed.** Case and surrounding whitespace are normalized silently — `"READ-ONLY"`, `"Read-Only"`, and `" full "` all map to their canonical tier. Anything else (a typo like `"readonly"`, an invented tier like `"super-admin"`, an empty string) is treated as `"read-only"` and a warning is printed to stderr. Typos cannot silently grant write/exec power. ### Why the default is restrictive diff --git a/src-rust/crates/cli/src/main.rs b/src-rust/crates/cli/src/main.rs index 198c454..044c904 100644 --- a/src-rust/crates/cli/src/main.rs +++ b/src-rust/crates/cli/src/main.rs @@ -1268,41 +1268,62 @@ fn normalize_provider_from_model(config: &mut Config) { /// - "full" → all tools allowed (no filtering) /// - "read-only" → only ReadOnly/None permission tools and AskUserQuestion /// - "search-only" → only Grep, Glob, Read, WebSearch, WebFetch tools +/// +/// The raw `access` string is normalized through +/// [`claurst_core::coven_shared::resolve_access_tier`] before the match — that +/// canonicalizes case/whitespace silently (so `"READ-ONLY"` and `" full "` +/// round-trip cleanly) and fails **closed** to `"read-only"` with a stderr +/// warning for anything genuinely unknown. This preserves the opt-in +/// semantics of `DEFAULT_FAMILIAR_ACCESS` so that typos in +/// `~/.coven/familiars.toml` or `settings.json` cannot silently grant +/// write/exec privileges. fn filter_tools_for_agent( tools: Arc>>, access: &str, ) -> Arc>> { - use claurst_tools::PermissionLevel as PL; - match access { - "read-only" => { - // Collect names of tools that are read-only, then rebuild from all_tools - // (Box is not Clone so we can't directly filter-and-keep). - let allowed_names: Vec = tools - .iter() - .filter(|t| { - matches!(t.permission_level(), PL::ReadOnly | PL::None) - || t.name() == "AskUserQuestion" - }) - .map(|t| t.name().to_string()) - .collect(); - let filtered: Vec> = claurst_tools::all_tools() - .into_iter() - .filter(|t| allowed_names.iter().any(|n| n == t.name())) - .collect(); - Arc::new(filtered) - } - "search-only" => { - const SEARCH_TOOLS: &[&str] = &["Grep", "Glob", "Read", "WebSearch", "WebFetch"]; - let filtered: Vec> = claurst_tools::all_tools() - .into_iter() - .filter(|t| SEARCH_TOOLS.contains(&t.name())) - .collect(); - Arc::new(filtered) - } - _ => tools, // "full" — allow all tools unchanged + match claurst_core::coven_shared::resolve_access_tier(access) { + "full" => tools, + "read-only" => filter_read_only_tools(&tools), + "search-only" => filter_search_only_tools(), + // `resolve_access_tier` is contracted to return one of the canonical + // tiers in `ACCESS_TIERS`, so anything else is a coven_shared bug. + other => unreachable!( + "resolve_access_tier returned non-canonical tier {other:?}; \ + coven_shared::ACCESS_TIERS contract violated" + ), } } +fn filter_read_only_tools( + tools: &[Box], +) -> Arc>> { + use claurst_tools::PermissionLevel as PL; + // Collect names of tools that are read-only, then rebuild from all_tools + // (Box is not Clone so we can't directly filter-and-keep). + let allowed_names: Vec = tools + .iter() + .filter(|t| { + matches!(t.permission_level(), PL::ReadOnly | PL::None) + || t.name() == "AskUserQuestion" + }) + .map(|t| t.name().to_string()) + .collect(); + let filtered: Vec> = claurst_tools::all_tools() + .into_iter() + .filter(|t| allowed_names.iter().any(|n| n == t.name())) + .collect(); + Arc::new(filtered) +} + +fn filter_search_only_tools() -> Arc>> { + const SEARCH_TOOLS: &[&str] = &["Grep", "Glob", "Read", "WebSearch", "WebFetch"]; + let filtered: Vec> = claurst_tools::all_tools() + .into_iter() + .filter(|t| SEARCH_TOOLS.contains(&t.name())) + .collect(); + Arc::new(filtered) +} + // --------------------------------------------------------------------------- // Headless mode: read prompt from arg/stdin, run, print response // --------------------------------------------------------------------------- @@ -4462,3 +4483,86 @@ fn json_null_or_string(opt: &Option) -> serde_json::Value { None => serde_json::Value::Null, } } + +#[cfg(test)] +mod tests { + use super::*; + + fn tool_names(tools: &[Box]) -> Vec { + let mut names: Vec = tools.iter().map(|t| t.name().to_string()).collect(); + names.sort(); + names + } + + #[test] + fn filter_full_returns_input_unchanged() { + let all = Arc::new(claurst_tools::all_tools()); + let before = tool_names(&all); + let filtered = filter_tools_for_agent(all.clone(), "full"); + assert_eq!(tool_names(&filtered), before); + } + + #[test] + fn filter_read_only_excludes_write_and_exec_tools() { + let all = Arc::new(claurst_tools::all_tools()); + let filtered = filter_tools_for_agent(all, "read-only"); + let names = tool_names(&filtered); + // Write/exec tools must not appear. + for forbidden in ["Bash", "Edit", "Write", "NotebookEdit"] { + assert!( + !names.contains(&forbidden.to_string()), + "read-only must not include {forbidden}, got {names:?}" + ); + } + } + + #[test] + fn filter_unknown_access_falls_back_to_read_only() { + let all = Arc::new(claurst_tools::all_tools()); + let read_only = tool_names(&filter_tools_for_agent(all.clone(), "read-only")); + // Genuinely unknown tiers must fail closed to read-only — they trip + // the warning path inside `resolve_access_tier`, not the silent + // case/whitespace canonicalization. + for unknown in ["readonly", "i-am-evil", "", "writeable", "full-access"] { + let got = tool_names(&filter_tools_for_agent(all.clone(), unknown)); + assert_eq!( + got, read_only, + "unknown access {unknown:?} must fail closed to read-only" + ); + } + } + + #[test] + fn filter_case_and_whitespace_canonicalize_silently() { + let all = Arc::new(claurst_tools::all_tools()); + let full = tool_names(&filter_tools_for_agent(all.clone(), "full")); + let read_only = tool_names(&filter_tools_for_agent(all.clone(), "read-only")); + let search_only = tool_names(&filter_tools_for_agent(all.clone(), "search-only")); + // Common surface variants should match their canonical tier rather + // than fail closed — typos warn, case/whitespace do not. + for (variant, expected) in [ + ("FULL", &full), + (" full ", &full), + ("READ-ONLY", &read_only), + ("Read-Only", &read_only), + (" search-only\n", &search_only), + ] { + let got = tool_names(&filter_tools_for_agent(all.clone(), variant)); + assert_eq!(&got, expected, "variant {variant:?} should canonicalize"); + } + } + + #[test] + fn filter_search_only_limits_to_known_search_tools() { + let all = Arc::new(claurst_tools::all_tools()); + let names = tool_names(&filter_tools_for_agent(all, "search-only")); + // Must be a subset of the documented search-tool whitelist. + const ALLOWED: &[&str] = &["Grep", "Glob", "Read", "WebSearch", "WebFetch"]; + for name in &names { + assert!( + ALLOWED.contains(&name.as_str()), + "search-only emitted disallowed tool {name}, expected subset of {ALLOWED:?}" + ); + } + } +} diff --git a/src-rust/crates/core/src/coven_shared.rs b/src-rust/crates/core/src/coven_shared.rs index 50d3b16..c07a9ff 100644 --- a/src-rust/crates/core/src/coven_shared.rs +++ b/src-rust/crates/core/src/coven_shared.rs @@ -45,6 +45,50 @@ pub(crate) static COVEN_HOME_ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex:: /// `access = "full"` per familiar in `~/.coven/familiars.toml`. pub const DEFAULT_FAMILIAR_ACCESS: &str = "read-only"; +/// All recognized access tiers, in canonical form. Anything outside this set +/// is treated as untrusted input by [`resolve_access_tier`] and fails closed +/// to [`DEFAULT_FAMILIAR_ACCESS`]. +pub const ACCESS_TIERS: &[&str] = &["full", "read-only", "search-only"]; + +/// Normalize an access string to a canonical tier without emitting any +/// diagnostic. Trims whitespace and lowercases the input so common surface +/// variants (`" Read-Only "`, `"READ-ONLY"`) round-trip cleanly. Returns +/// `None` for anything that isn't a known tier — callers MUST treat that as +/// untrusted input. +/// +/// The returned `&'static str` is one of the canonical entries in +/// [`ACCESS_TIERS`], never the caller's allocation. +pub fn canonicalize_access_tier(input: &str) -> Option<&'static str> { + match input.trim().to_ascii_lowercase().as_str() { + "full" => Some("full"), + "read-only" => Some("read-only"), + "search-only" => Some("search-only"), + _ => None, + } +} + +/// Resolve an access string to a canonical tier, failing closed for unknown +/// values. On unknown input, prints a single warning to stderr (so a typo in +/// `~/.coven/familiars.toml` or `settings.json` is visible at the moment the +/// tool filter is applied) and returns [`DEFAULT_FAMILIAR_ACCESS`]. +/// +/// This is the single entry point the CLI tool-filter pipeline should use — +/// the security model depends on unknown tiers collapsing to the most +/// restrictive option rather than silently passing the full tool list +/// through. +pub fn resolve_access_tier(input: &str) -> &'static str { + match canonicalize_access_tier(input) { + Some(canonical) => canonical, + None => { + eprintln!( + "warning: unknown access tier {:?} — falling back to {:?}. Valid tiers: {:?}", + input, DEFAULT_FAMILIAR_ACCESS, ACCESS_TIERS, + ); + DEFAULT_FAMILIAR_ACCESS + } + } +} + /// One entry in `~/.coven/familiars.toml`. /// /// Schema mirrors what the daemon serves at `GET /api/v1/familiars`. @@ -68,9 +112,19 @@ pub struct CovenFamiliar { } impl CovenFamiliar { - /// Resolved access tier — the explicit value or [`DEFAULT_FAMILIAR_ACCESS`]. - pub fn resolved_access(&self) -> &str { - self.access.as_deref().unwrap_or(DEFAULT_FAMILIAR_ACCESS) + /// Resolved access tier — canonicalized to one of [`ACCESS_TIERS`]. + /// + /// Absent values use [`DEFAULT_FAMILIAR_ACCESS`]. Present-but-unknown + /// values are normalized silently here (case/whitespace) and otherwise + /// fall back to [`DEFAULT_FAMILIAR_ACCESS`]; the warning for a true typo + /// fires at the tool-filter chokepoint so it lands at the moment the + /// security decision is made instead of at parse time when nothing is + /// listening. + pub fn resolved_access(&self) -> &'static str { + match self.access.as_deref() { + None => DEFAULT_FAMILIAR_ACCESS, + Some(raw) => canonicalize_access_tier(raw).unwrap_or(DEFAULT_FAMILIAR_ACCESS), + } } } @@ -405,6 +459,90 @@ access = "search-only" assert_eq!(merged.get("cody").map(|d| d.access.as_str()), Some("full")); } + #[test] + fn canonicalize_access_tier_accepts_canonical_lowercase() { + assert_eq!(canonicalize_access_tier("full"), Some("full")); + assert_eq!(canonicalize_access_tier("read-only"), Some("read-only")); + assert_eq!(canonicalize_access_tier("search-only"), Some("search-only")); + } + + #[test] + fn canonicalize_access_tier_normalizes_case_and_whitespace() { + assert_eq!(canonicalize_access_tier("FULL"), Some("full")); + assert_eq!(canonicalize_access_tier("Read-Only"), Some("read-only")); + assert_eq!(canonicalize_access_tier(" search-only\n"), Some("search-only")); + assert_eq!(canonicalize_access_tier(" full "), Some("full")); + } + + #[test] + fn canonicalize_access_tier_rejects_unknown_strings() { + // Typos and near-matches must NOT round-trip — callers depend on + // `None` to trigger fail-closed behavior. + for unknown in &["readonly", "Full Access", "writable", "", "rad-only", "search only"] { + assert!( + canonicalize_access_tier(unknown).is_none(), + "expected {unknown:?} to be rejected" + ); + } + } + + #[test] + fn resolve_access_tier_falls_back_to_default_on_unknown() { + assert_eq!(resolve_access_tier("readonly"), DEFAULT_FAMILIAR_ACCESS); + assert_eq!(resolve_access_tier(""), DEFAULT_FAMILIAR_ACCESS); + assert_eq!(resolve_access_tier("i-am-evil"), DEFAULT_FAMILIAR_ACCESS); + } + + #[test] + fn resolve_access_tier_passes_canonical_through() { + assert_eq!(resolve_access_tier("full"), "full"); + assert_eq!(resolve_access_tier("read-only"), "read-only"); + assert_eq!(resolve_access_tier("search-only"), "search-only"); + // Case + whitespace are part of the "canonicalize silently" contract. + assert_eq!(resolve_access_tier(" FULL "), "full"); + assert_eq!(resolve_access_tier("READ-ONLY"), "read-only"); + } + + #[test] + fn familiar_resolved_access_normalizes_case_variants() { + // Typos and case mismatches in `~/.coven/familiars.toml` must NOT + // grant a familiar more power than the user intended. Case variants + // canonicalize silently; truly unknown values fail closed to the + // restrictive default. + let case_variant = CovenFamiliar { + id: "rogue".into(), + display_name: None, + emoji: None, + role: None, + description: None, + pronouns: None, + access: Some("READ-ONLY".into()), + }; + assert_eq!(case_variant.resolved_access(), "read-only"); + + let typo = CovenFamiliar { + id: "rogue".into(), + display_name: None, + emoji: None, + role: None, + description: None, + pronouns: None, + access: Some("readonly".into()), + }; + assert_eq!(typo.resolved_access(), DEFAULT_FAMILIAR_ACCESS); + + let garbage = CovenFamiliar { + id: "rogue".into(), + display_name: None, + emoji: None, + role: None, + description: None, + pronouns: None, + access: Some("super-admin".into()), + }; + assert_eq!(garbage.resolved_access(), DEFAULT_FAMILIAR_ACCESS); + } + #[test] fn list_daemon_skills_scans_metadata_files() { let _g = with_coven_home(|home| {