fix(security): fail closed for unknown agent access tiers (#13)

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 <noreply@opencoven.ai>

Author: BunsDev

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
 

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<Vec<Box<dyn claurst_tools::Tool>>>,
     access: &str,
 ) -> Arc<Vec<Box<dyn claurst_tools::Tool>>> {
-    use claurst_tools::PermissionLevel as PL;
-    match access {
-        "read-only" => {
-            // Collect names of tools that are read-only, then rebuild from all_tools
-            // (Box<dyn Tool> is not Clone so we can't directly filter-and-keep).
-            let allowed_names: Vec<String> = 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<Box<dyn claurst_tools::Tool>> = 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<Box<dyn claurst_tools::Tool>> = 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<dyn claurst_tools::Tool>],
+) -> Arc<Vec<Box<dyn claurst_tools::Tool>>> {
+    use claurst_tools::PermissionLevel as PL;
+    // Collect names of tools that are read-only, then rebuild from all_tools
+    // (Box<dyn Tool> is not Clone so we can't directly filter-and-keep).
+    let allowed_names: Vec<String> = 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<Box<dyn claurst_tools::Tool>> = 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<Vec<Box<dyn claurst_tools::Tool>>> {
+    const SEARCH_TOOLS: &[&str] = &["Grep", "Glob", "Read", "WebSearch", "WebFetch"];
+    let filtered: Vec<Box<dyn claurst_tools::Tool>> = 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<String>) -> serde_json::Value {
         None => serde_json::Value::Null,
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn tool_names(tools: &[Box<dyn claurst_tools::Tool>]) -> Vec<String> {
+        let mut names: Vec<String> = 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:?}"
+            );
+        }
+    }
+}

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| {