diff --git a/crates/skald-core/src/approval/mod.rs b/crates/skald-core/src/approval/mod.rs index 4d8e5a0..9432596 100644 --- a/crates/skald-core/src/approval/mod.rs +++ b/crates/skald-core/src/approval/mod.rs @@ -172,13 +172,28 @@ pub const PERSISTED_REQUEST_ID: i64 = 0; // ── Session bypass ──────────────────────────────────────────────────────────── /// What a session bypass entry applies to. +/// +/// [`Tool`](Self::Tool) is the **default** scope of the "15 min" / "Session" +/// buttons on an approval card, and the only one narrow enough to be safe to +/// pick on the user's behalf: a human answering a card has read *that* call, +/// and nothing else. The wider scopes stay reachable through the REST +/// `bypass_scope` field, where choosing one is a deliberate act. pub enum BypassScope { /// Covers every tool regardless of category. All, + /// Covers exactly one tool, matched on its full name + /// (`mcp__gmail__send_message`, `write_file`, …). + Tool(String), /// Covers only tools of the given registered category. Category(ToolCategory), /// Covers only tools belonging to the named MCP server /// (matched by the `mcp____` prefix in the tool name). + /// + /// **A connector is not a permission unit**: its read tools and its write + /// tools live under one name, so this scope reads "trust everything Gmail + /// can do" — including sending mail — from a click on a card that asked + /// about labelling a message. Never auto-detect it; require the caller to + /// name it. McpServer(String), } @@ -715,6 +730,22 @@ impl ApprovalManager { info!(session_id, secs = duration.as_secs(), "approval: bypass active (timed)"); } + /// Bypasses approval prompts for one tool, matched on its full name. + /// `duration` is `None` for an indefinite (session-scoped) bypass. + pub async fn bypass_session_for_tool( + &self, + session_id: i64, + tool: String, + duration: Option, + ) { + let expires_at = duration.map(|d| Instant::now() + d); + self.session_bypasses.lock().await + .entry(session_id) + .or_default() + .push(ApprovalBypass { scope: BypassScope::Tool(tool.clone()), expires_at }); + info!(session_id, tool, secs = duration.map(|d| d.as_secs()), "approval: bypass active (tool)"); + } + /// Bypasses approval prompts for a specific tool `category`. /// `duration` is `None` for an indefinite (session-scoped) bypass. pub async fn bypass_session_for_category( @@ -892,14 +923,21 @@ impl ApprovalManager { Ok(()) } - /// Approve + register a session bypass so future tool calls of the same - /// category / MCP server are auto-approved. + /// Approve + register a session bypass so future calls of the **same tool** + /// are auto-approved. /// /// - `bypass_secs = Some(n)`: bypass lasts `n` seconds (0 is treated as indefinite) /// - `bypass_secs = None`: bypass lasts until the session ends /// - /// Scope is auto-detected from the pending request's tool metadata, - /// mirroring the web-inbox logic in `src/frontend/api/inbox.rs`. + /// The scope is always [`BypassScope::Tool`] and is deliberately **not** + /// inferred from the tool's category or MCP server. It used to be: a click + /// on a Gmail card registered a bypass over the whole connector, so + /// approving `mcp__gmail__modify_message` silently un-gated + /// `mcp__gmail__send_message` — an explicit `require` rule on it and all — + /// and the only trace was a log line. A human answering a card has read one + /// call; that call is the widest thing their click may authorise. The + /// broader scopes remain available to a caller that names one (the REST + /// `bypass_scope` field in `src/frontend/api/inbox.rs`). pub async fn approve_with_bypass(&self, request_id: i64, bypass_secs: Option) { let info = self.get_pending(request_id).await; self.approve(request_id).await; @@ -907,16 +945,7 @@ impl ApprovalManager { let duration = bypass_secs .filter(|&s| s > 0) .map(Duration::from_secs); - if let Some(cat) = info.tool_category { - self.bypass_session_for_category(info.session_id, cat, duration).await; - } else if let Some(srv) = info.mcp_server { - self.bypass_session_for_mcp(info.session_id, srv, duration).await; - } else { - match duration { - Some(d) => self.bypass_session_for(info.session_id, d).await, - None => self.bypass_session(info.session_id).await, - } - } + self.bypass_session_for_tool(info.session_id, info.tool_name, duration).await; } } @@ -1022,6 +1051,7 @@ pub(crate) fn pattern_matches(pattern: &str, tool_name: &str) -> bool { fn bypass_matches(bypass: &ApprovalBypass, category: Option, tool_name: &str) -> bool { match &bypass.scope { BypassScope::All => true, + BypassScope::Tool(name) => name == tool_name, BypassScope::Category(bc) => category.map_or(false, |tc| tc == *bc), BypassScope::McpServer(server) => { mcp_server_from_tool_name(tool_name).map_or(false, |s| s == *server) @@ -1112,6 +1142,76 @@ mod tests { assert!(pattern_matches("data/*", "data/x")); } + /// A bypass answered from a card covers **that tool only**. + /// + /// The regression: approving `mcp__gmail__modify_message` with "15 min" used to + /// register a bypass over the whole `gmail` connector, so the very next + /// `mcp__gmail__send_message` executed without a prompt — through an explicit + /// `require` rule written for it — and the only evidence was a log line. + #[tokio::test] + async fn a_tool_bypass_does_not_cover_its_connector() { + use super::{ApprovalManager, GateResult}; + use serde_json::json; + use std::sync::Arc; + use tokio::sync::broadcast; + + let path = std::env::temp_dir().join(format!("skald_bypass_test_{}.db", std::process::id())); + let path_str = path.to_string_lossy().to_string(); + let _ = std::fs::remove_file(&path); + let pool = crate::db::init_system_pool(&path_str).await.expect("init_system_pool"); + let db = Arc::new(pool); + + sqlx::query("INSERT INTO tool_permission_groups (id, name) VALUES ('default', 'Default')") + .execute(db.as_ref()).await.unwrap(); + for (tool, action) in [ + ("mcp__gmail__modify_message", "require"), + ("mcp__gmail__send_message", "require"), + ] { + sqlx::query( + "INSERT INTO approval_rules (tool_pattern, action, priority, group_id) + VALUES (?, ?, 0, 'default')", + ) + .bind(tool).bind(action).execute(db.as_ref()).await.unwrap(); + } + + let (tx, _rx) = broadcast::channel(16); + let mgr = ApprovalManager::new(Arc::clone(&db), tx); + mgr.seed_default_catch_all().await.unwrap(); + + let decide = |tool: &'static str| { + let mgr = &mgr; + async move { + mgr.check(1, None, "assistant", "web", tool, &json!({}), Some("default")).await + } + }; + + // Both gated to begin with. + assert!(matches!(decide("mcp__gmail__modify_message").await, GateResult::Require)); + assert!(matches!(decide("mcp__gmail__send_message").await, GateResult::Require)); + + // The human approves ONE call with a bypass. + mgr.bypass_session_for_tool(1, "mcp__gmail__modify_message".into(), None).await; + + // It covers that tool… + assert!(matches!(decide("mcp__gmail__modify_message").await, GateResult::Allow)); + // …and nothing else on the same connector. + assert!(matches!(decide("mcp__gmail__send_message").await, GateResult::Require)); + // Nor another session's calls (the map is keyed by conversation). + let other = mgr + .check(2, None, "assistant", "web", "mcp__gmail__modify_message", &json!({}), Some("default")) + .await; + assert!(matches!(other, GateResult::Require)); + + // The connector-wide scope still exists for a caller that names it. + mgr.bypass_session_for_mcp(1, "gmail".into(), None).await; + assert!(matches!(decide("mcp__gmail__send_message").await, GateResult::Allow)); + + db.close().await; + for suffix in ["", "-wal", "-shm"] { + let _ = std::fs::remove_file(format!("{path_str}{suffix}")); + } + } + // End-to-end: run the real startup pipeline (migrate → seed) against a temp SQLite // DB pre-loaded with legacy rules, then assert the gate decisions through `check()`. #[tokio::test] diff --git a/src/frontend/api/inbox.rs b/src/frontend/api/inbox.rs index 014ccf3..b9e43e6 100644 --- a/src/frontend/api/inbox.rs +++ b/src/frontend/api/inbox.rs @@ -145,7 +145,12 @@ pub struct ApproveBody { /// Seconds for the bypass duration. `0` means indefinite (session-scoped). /// Absent means no bypass. pub bypass_secs: Option, - /// `"category"` | `"mcp_server"` | `"all"`. Defaults to auto-detect from tool info. + /// `"tool"` | `"category"` | `"mcp_server"` | `"all"`. Defaults to `"tool"`. + /// + /// Deliberately **not** auto-detected from the tool's metadata any more: a + /// click on an approval card authorises the call the human just read, and + /// widening that to the tool's category or its whole MCP connector is a + /// decision only an explicit value may make (see `approve_with_bypass`). pub bypass_scope: Option, } @@ -171,28 +176,28 @@ pub async fn resolve_approval( if let (Some(info), Some(bypass_secs)) = (info, body.bypass_secs) { let duration = if bypass_secs == 0 { None } else { Some(Duration::from_secs(bypass_secs)) }; - let scope = body.bypass_scope.as_deref().unwrap_or_else(|| { - if info.tool_category.is_some() { "category" } - else if info.mcp_server.is_some() { "mcp_server" } - else { "all" } - }); + let scope = body.bypass_scope.as_deref().unwrap_or("tool"); + // Every fallback here narrows, never widens: a scope that cannot be + // honoured (a category-less tool, a non-MCP one) and an unknown + // scope string both degrade to the tool itself. Only a literal + // `"all"` disables the gate session-wide, and only because the + // caller spelled it out. match scope { - "category" => { - if let Some(cat) = info.tool_category { - ctx.approval.bypass_session_for_category(info.session_id, cat, duration).await; - } else { - apply_all_bypass(&ctx, info.session_id, duration).await; - } + "category" if info.tool_category.is_some() => { + let cat = info.tool_category.unwrap(); + ctx.approval.bypass_session_for_category(info.session_id, cat, duration).await; } - "mcp_server" => { - if let Some(server) = info.mcp_server { - ctx.approval.bypass_session_for_mcp(info.session_id, server, duration).await; - } else { - apply_all_bypass(&ctx, info.session_id, duration).await; - } + "mcp_server" if info.mcp_server.is_some() => { + let server = info.mcp_server.clone().unwrap(); + ctx.approval.bypass_session_for_mcp(info.session_id, server, duration).await; + } + "all" => apply_all_bypass(&ctx, info.session_id, duration).await, + _ => { + ctx.approval + .bypass_session_for_tool(info.session_id, info.tool_name.clone(), duration) + .await; } - _ => apply_all_bypass(&ctx, info.session_id, duration).await, } } } diff --git a/web/i18n/en.js b/web/i18n/en.js index 4ad5b69..3dee846 100644 --- a/web/i18n/en.js +++ b/web/i18n/en.js @@ -321,8 +321,8 @@ export default { 'approval.reject': 'Deny', 'approval.confirm_reject': 'Confirm deny', 'approval.reject_hint': 'Optional: say why (the assistant will read it)', - 'approval.bypass_15': 'Allow and skip similar requests for 15 minutes', - 'approval.bypass_all': 'Allow and skip all requests for this session', + 'approval.bypass_15': 'Allow, and stop asking for this same tool for 15 minutes', + 'approval.bypass_all': 'Allow, and stop asking for this same tool for the rest of this conversation', // ── Login ────────────────────────────────────────────────────────────────── 'login.title': 'Welcome back', diff --git a/web/i18n/fr.js b/web/i18n/fr.js index 844eb2e..2e2792e 100644 --- a/web/i18n/fr.js +++ b/web/i18n/fr.js @@ -321,8 +321,8 @@ export default { 'approval.reject': 'Refuser', 'approval.confirm_reject': 'Confirmer le refus', 'approval.reject_hint': 'Facultatif : dites pourquoi (l\'assistant le lira)', - 'approval.bypass_15': 'Autoriser et ignorer les demandes similaires pendant 15 minutes', - 'approval.bypass_all': 'Autoriser et ignorer toutes les demandes pour cette session', + 'approval.bypass_15': 'Autoriser et ne plus demander pour ce même outil pendant 15 minutes', + 'approval.bypass_all': 'Autoriser et ne plus demander pour ce même outil jusqu\'à la fin de la conversation', // ── Login ────────────────────────────────────────────────────────────────── 'login.title': 'Bon retour', diff --git a/web/i18n/it.js b/web/i18n/it.js index c53b7b7..5b9263e 100644 --- a/web/i18n/it.js +++ b/web/i18n/it.js @@ -321,8 +321,8 @@ export default { 'approval.reject': 'Nega', 'approval.confirm_reject': 'Conferma il rifiuto', 'approval.reject_hint': 'Facoltativo: spiega perché (lo leggerà l\'assistente)', - 'approval.bypass_15': 'Consenti e salta richieste simili per 15 minuti', - 'approval.bypass_all': 'Consenti e salta tutte le richieste di questa sessione', + 'approval.bypass_15': 'Consenti e non chiedere più per questo stesso strumento per 15 minuti', + 'approval.bypass_all': 'Consenti e non chiedere più per questo stesso strumento per il resto della conversazione', // ── Accesso ──────────────────────────────────────────────────────────────── 'login.title': 'Bentornato', diff --git a/web/lib/inbox-cards.js b/web/lib/inbox-cards.js index 13e6966..f728db9 100644 --- a/web/lib/inbox-cards.js +++ b/web/lib/inbox-cards.js @@ -73,19 +73,22 @@ export const InboxCardsMixin = (Base) => class extends Base { this._resolveApproval(requestId, 'reject', note, null, null, toolCallId); } - /** Approve + set a timed or session bypass scoped to the tool's category or MCP server. */ + /** + * Approve + skip approval for **this same tool** for a while. + * + * Scoped to the tool and nothing wider: the card the human just read is + * about one call, and the previous category/MCP-server auto-detect meant a + * click on "label this message" also un-gated "send this message" for the + * rest of the session. + */ _approveWithBypass(item, bypassSecs) { - const scope = item.tool_category ? 'category' - : item.mcp_server ? 'mcp_server' - : 'all'; - this._resolveApproval(item.request_id, 'approve', '', bypassSecs, scope); + this._resolveApproval(item.request_id, 'approve', '', bypassSecs, 'tool'); } - /** Human-readable bypass scope label, e.g. "filesystem" or "Gmail". */ + /** Short label for the bypass scope — the tool, `mcp__x__y` shown as `y`. */ _bypassLabel(item) { - if (item.tool_category) return item.tool_category; - if (item.mcp_server) return item.mcp_server; - return 'session'; + const name = item.tool_name ?? ''; + return name.startsWith('mcp__') ? name.split('__').pop() : name; } async _resolveClarification(requestId, inputEl) {