From be0a90deb68e9d4fe4e6c27ea6eb4cb0bd2345c5 Mon Sep 17 00:00:00 2001 From: David McKay Date: Fri, 13 Nov 2026 19:19:57 -0800 Subject: [PATCH 1/3] Desktop: an approval answered too late runs nothing, says so, and frees its Bot A native dialog cannot be closed from outside, so "Allow this command?" stayed on screen after OpenBot stopped waiting for it, still offering Allow once. Pressing it did nothing anyone could see, and until it was dismissed every folder action that Bot asked for failed with "That Bot already has a host operation in progress." Allow on an operation that was cancelled or expired now runs nothing and opens a notice that says the command didn't run (or the file wasn't changed) and to ask the Bot again. When an operation is cancelled, its Bot's slot is freed at once, so the next action asks straight away, and a cancelled operation no longer counts toward the waiting limit. --- desktop/src-tauri/src/desktop_host_access.rs | 56 ++- desktop/src-tauri/src/host_access.rs | 366 ++++++++++++++++++- 2 files changed, 408 insertions(+), 14 deletions(-) diff --git a/desktop/src-tauri/src/desktop_host_access.rs b/desktop/src-tauri/src/desktop_host_access.rs index 5819d540d2..daeb57a2ba 100644 --- a/desktop/src-tauri/src/desktop_host_access.rs +++ b/desktop/src-tauri/src/desktop_host_access.rs @@ -1,8 +1,8 @@ //! Owner approval is collected by native dialogs, never by an app/tool-provided answer. use openbot_desktop_lib::host_access::{ command_approval, ApprovedFolder, ChooseFolderPrompt, CommandApproval, CommandPolicy, - CommandPrompt, HostAccessError, HostAccessResult, HostApprovalUi, LocalCommandAllowList, - WritePrompt, + CommandPrompt, HostAccessError, HostAccessResult, HostApprovalUi, LapsedAction, LapsedApproval, + LocalCommandAllowList, WritePrompt, }; use openbot_desktop_lib::paths; use std::path::Path; @@ -84,6 +84,25 @@ fn command_message( ) } +/// The notice shown when the person allowed an action after OpenBot had stopped waiting for the +/// answer: its title and its text. +fn lapsed_notice(bot: &str, action: LapsedAction) -> (&'static str, String) { + match action { + LapsedAction::Command => ( + "This command didn't run", + format!( + "OpenBot stopped waiting for your answer, so {bot}'s command didn't run. Ask {bot} again to run it." + ), + ), + LapsedAction::FileChange => ( + "This file wasn't changed", + format!( + "OpenBot stopped waiting for your answer, so {bot}'s change wasn't made. Ask {bot} again to make it." + ), + ), + } +} + /// What the person pressed on the three-button command dialog. The plugin reports custom buttons by /// their label; anything else, including a plain Yes or No, is a denial. #[derive(Debug, PartialEq, Eq)] @@ -258,12 +277,45 @@ impl HostApprovalUi for NativeApproval { CommandChoice::Deny => Err(refused(DECLINED)), } } + + fn approval_lapsed(&self, notice: &LapsedApproval) { + let bot = bot_label(notice.bot_name.as_deref(), ¬ice.bot_id); + let (title, message) = lapsed_notice(&bot, notice.action); + let mut dialog = self + .0 + .dialog() + .message(message) + .title(title) + .kind(MessageDialogKind::Info) + .buttons(MessageDialogButtons::Ok); + if let Some(window) = self.0.get_webview_window("main") { + dialog = dialog.parent(&window); + } + // Not waited for: the Bot's next action must not queue behind a notice. + dialog.show(|_| {}); + } } #[cfg(test)] mod tests { use super::*; + #[test] + fn a_late_allow_says_nothing_ran_and_names_the_bot() { + let (title, message) = lapsed_notice("Research Bot", LapsedAction::Command); + assert_eq!(title, "This command didn't run"); + assert!( + message.contains("Research Bot's command didn't run"), + "{message}" + ); + let (title, message) = lapsed_notice("Research Bot", LapsedAction::FileChange); + assert_eq!(title, "This file wasn't changed"); + assert!( + message.contains("Research Bot's change wasn't made"), + "{message}" + ); + } + #[test] fn only_a_named_button_allows_a_command() { assert_eq!( diff --git a/desktop/src-tauri/src/host_access.rs b/desktop/src-tauri/src/host_access.rs index b431fa958c..e291ff3826 100644 --- a/desktop/src-tauri/src/host_access.rs +++ b/desktop/src-tauri/src/host_access.rs @@ -93,7 +93,9 @@ struct State { completed: HashSet, canceled_operations: HashSet, active_by_operation: HashSet, - active_by_bot: HashSet, + /// Each Bot's one operation in progress, by its id. A Bot's slot is freed when its operation + /// ends, or when the server cancels it while a dialog for it is still on screen. + active_by_bot: HashMap, } #[derive(Clone, Debug)] @@ -150,6 +152,27 @@ pub trait HostApprovalUi: Send + Sync + 'static { fn choose_folder(&self, request: &ChooseFolderPrompt) -> HostAccessResult; fn confirm_write(&self, request: &WritePrompt) -> HostAccessResult<()>; fn confirm_command(&self, request: &CommandPrompt) -> HostAccessResult<()>; + /// The person allowed an action after OpenBot had stopped waiting for the answer, so it did + /// not run. Told so they do not believe it did. Nothing by default. + /// + /// FOUND ON A MAC: a native dialog cannot be closed from outside, so "Allow this command?" stayed + /// on screen after the server gave up on it, still offering Allow once, and Allow did nothing + /// anyone could see. + fn approval_lapsed(&self, _notice: &LapsedApproval) {} +} + +/// An action the person allowed too late. See `HostApprovalUi::approval_lapsed`. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct LapsedApproval { + pub bot_id: String, + pub bot_name: Option, + pub action: LapsedAction, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum LapsedAction { + Command, + FileChange, } pub struct DenyAllApprovalUi; @@ -471,7 +494,7 @@ impl HostAccess { completed: HashSet::new(), canceled_operations: HashSet::new(), active_by_operation: HashSet::new(), - active_by_bot: HashSet::new(), + active_by_bot: HashMap::new(), }), }); let thread_inner = inner.clone(); @@ -626,15 +649,25 @@ impl Inner { } if state.stopped || state.canceled_operations.contains(&operation.operation_id) { Err("Host access is stopped.".to_string()) - } else if state.active_by_operation.len() >= MAX_ACTIVE_OPERATIONS { + } else if state + .active_by_operation + .iter() + .filter(|id| !state.canceled_operations.contains(*id)) + .count() + >= MAX_ACTIVE_OPERATIONS + { + // One the server cancelled is waiting only on a dialog nobody answered; it does not + // count, or four walked-away dialogs would refuse every Bot. Err("Too many host operations are already waiting.".to_string()) - } else if state.active_by_bot.contains(&operation.bot_id) { + } else if state.active_by_bot.contains_key(&operation.bot_id) { Err("That Bot already has a host operation in progress.".to_string()) } else { state .active_by_operation .insert(operation.operation_id.clone()); - state.active_by_bot.insert(operation.bot_id.clone()); + state + .active_by_bot + .insert(operation.bot_id.clone(), operation.operation_id.clone()); Ok(()) } }; @@ -654,12 +687,18 @@ impl Inner { inner.handle_and_post(&worker_client, operation); let mut state = inner.state.lock().expect("host state poisoned"); state.active_by_operation.remove(&worker_operation_id); - state.active_by_bot.remove(&worker_bot_id); + // The slot may already be another operation's: this one's was freed when the + // server cancelled it. + if state.active_by_bot.get(&worker_bot_id) == Some(&worker_operation_id) { + state.active_by_bot.remove(&worker_bot_id); + } }) { let mut state = self.state.lock().expect("host state poisoned"); state.active_by_operation.remove(&operation_id); - state.active_by_bot.remove(&bot_id); + if state.active_by_bot.get(&bot_id) == Some(&operation_id) { + state.active_by_bot.remove(&bot_id); + } drop(state); self.post_error( &client, @@ -900,7 +939,7 @@ impl Inner { content: content.clone(), writable: true, })?; - self.ensure_operation_still_allowed(operation, &grant)?; + self.ensure_approved_in_time(operation, &grant, LapsedAction::FileChange)?; let relative = host_relative_for_write(&grant.root, &target)?; let target_path = container_path(&relative); let backup_name = backup_name_for(&relative); @@ -955,7 +994,7 @@ impl Inner { writable, command_policy, })?; - self.ensure_operation_still_allowed(operation, &grant)?; + self.ensure_approved_in_time(operation, &grant, LapsedAction::Command)?; let command_directory = if let Some(directory) = working_directory { let relative = host_relative(&grant.root, &directory)?; container_path(&relative) @@ -986,6 +1025,13 @@ impl Inner { state .canceled_operations .insert(target_operation_id.clone()); + // FOUND ON A MAC: the server gave up on a command whose "Allow this command?" was still + // on screen, and every folder action that Bot asked for after it failed with "That Bot + // already has a host operation in progress." until someone dismissed the dialog. The + // cancelled one can no longer run (`ensure_approved_in_time`), so its Bot is free. + state + .active_by_bot + .retain(|_, operation_id| *operation_id != target_operation_id); if let Some(grant_id) = operation.grant_id.as_deref() { if let Some(grant) = state.grants.get_mut(grant_id) { if grant.actor_id == operation.actor_id || operation.actor_id == "*" { @@ -1041,6 +1087,46 @@ impl Inner { } Ok(grant.clone()) } + /// After the person allowed an action: whether it may still run. When the server stopped + /// waiting for the answer first (it expired, or the server cancelled it alone), the person is + /// told it did not run, and the server is answered that it expired. + fn ensure_approved_in_time( + &self, + operation: &DesktopOperation, + grant: &LocalGrant, + action: LapsedAction, + ) -> HostAccessResult<()> { + let Err(refused) = self.ensure_operation_still_allowed(operation, grant) else { + return Ok(()); + }; + let lapsed = { + let state = self.state.lock().expect("host state poisoned"); + let grant_still_held = state + .grants + .get(&grant.id) + .is_some_and(|current| !current.revoked); + !state.stopped + && grant_still_held + && (state.canceled_operations.contains(&operation.operation_id) + || self.operation_expired(operation)) + }; + if !lapsed { + return Err(refused); + } + log::info!( + "[host access] operation {} was allowed after the server stopped waiting; nothing ran", + operation.operation_id + ); + self.approval.approval_lapsed(&LapsedApproval { + bot_id: operation.bot_id.clone(), + bot_name: grant.bot_name.clone(), + action, + }); + Err(HostAccessError::Denied( + "Host operation approval expired.".into(), + )) + } + fn ensure_operation_still_allowed( &self, operation: &DesktopOperation, @@ -2667,7 +2753,7 @@ mod tests { completed: HashSet::new(), canceled_operations: HashSet::new(), active_by_operation: HashSet::new(), - active_by_bot: HashSet::new(), + active_by_bot: HashMap::new(), }), }); fn redelivered_operation() -> DesktopOperation { @@ -2710,6 +2796,262 @@ mod tests { fs::remove_dir_all(root).unwrap(); } + /// The person's first "Allow this command?" is left open; every later one is denied. + struct LateCommandApprovalUi { + asked: AtomicUsize, + asked_tx: Mutex>, + release_rx: Mutex>, + lapsed: Mutex>, + } + + impl HostApprovalUi for LateCommandApprovalUi { + fn choose_folder(&self, _: &ChooseFolderPrompt) -> HostAccessResult { + unreachable!("the grant is made by the test") + } + + fn confirm_write(&self, _: &WritePrompt) -> HostAccessResult<()> { + unreachable!("only commands are asked about here") + } + + fn confirm_command(&self, _: &CommandPrompt) -> HostAccessResult<()> { + let first = self.asked.fetch_add(1, Ordering::SeqCst) == 0; + self.asked_tx.lock().unwrap().send(()).unwrap(); + if first { + self.release_rx.lock().unwrap().recv().unwrap(); + return Ok(()); + } + Err(HostAccessError::Denied( + "Declined in OpenBot Desktop.".into(), + )) + } + + fn approval_lapsed(&self, notice: &LapsedApproval) { + self.lapsed.lock().unwrap().push(notice.clone()); + } + } + + /* + * FOUND ON A MAC: an "Allow this command?" left past the server's two minutes ended the folder + * for good, and the dialog stayed up offering an Allow that did nothing. The server now + * cancels an operation that ran out its time without naming its grant; this is what the + * desktop does with that cancel and with the Allow pressed after it. + */ + #[test] + fn a_command_allowed_after_the_server_stopped_waiting_does_not_run_and_the_folder_still_asks() { + let (asked_tx, asked_rx) = mpsc::channel(); + let (release_tx, release_rx) = mpsc::channel(); + let approval = Arc::new(LateCommandApprovalUi { + asked: AtomicUsize::new(0), + asked_tx: Mutex::new(asked_tx), + release_rx: Mutex::new(release_rx), + lapsed: Mutex::new(Vec::new()), + }); + let config = HostAccessConfig::new( + "http://127.0.0.1:9", + "token", + Address::new(Engine::Docker, None), + DEFAULT_IMAGE, + vec![], + ); + let inner = Arc::new(Inner { + config, + approval: approval.clone(), + instance_label: "test-instance".into(), + effect_lock: Mutex::new(()), + state: Mutex::new(State { + stopped: false, + thread: None, + grants: HashMap::from([( + "grant".into(), + LocalGrant { + id: "grant".into(), + bot_id: "bot-a".into(), + actor_id: "actor".into(), + bot_name: Some("Research Bot".into()), + root: temp_root("host-access-late-allow"), + revoked: false, + }, + )]), + running: HashMap::new(), + completed: HashSet::new(), + canceled_operations: HashSet::new(), + active_by_operation: HashSet::new(), + active_by_bot: HashMap::new(), + }), + }); + let operation = + |id: &str, kind: HostOperationKind, target: Option<&str>| DesktopOperation { + operation_id: id.into(), + kind, + bot_id: "bot-a".into(), + actor_id: "actor".into(), + bot_name: None, + grant_id: matches!(kind, HostOperationKind::RunCommand).then(|| "grant".into()), + target_operation_id: target.map(str::to_string), + relative_path: None, + content: None, + command: matches!(kind, HostOperationKind::RunCommand).then(|| "ls -la".into()), + writable: Some(false), + command_policy: None, + expires_at: None, + received_at_ms: now_millis(), + }; + + let worker = inner.clone(); + let first = operation("op-late", HostOperationKind::RunCommand, None); + let answered = thread::spawn(move || worker.handle_operation(&first).map(|_| ())); + asked_rx.recv_timeout(Duration::from_secs(2)).unwrap(); + + // The server stops waiting and cancels that one operation, naming no grant. + inner + .handle_operation(&operation( + "op-cancel", + HostOperationKind::Cancel, + Some("op-late"), + )) + .unwrap(); + // Then the person presses Allow once on the dialog still on screen. + release_tx.send(()).unwrap(); + let error = answered.join().unwrap().unwrap_err(); + assert_eq!( + public_error_message(&error, &inner.config), + "Host operation approval expired." + ); + assert_eq!( + *approval.lapsed.lock().unwrap(), + vec![LapsedApproval { + bot_id: "bot-a".into(), + bot_name: Some("Research Bot".into()), + action: LapsedAction::Command, + }] + ); + + // The next command on the same folder asks again. + let next = inner + .handle_operation(&operation("op-next", HostOperationKind::RunCommand, None)) + .map(|_| ()) + .unwrap_err(); + assert_eq!(approval.asked.load(Ordering::SeqCst), 2); + assert_eq!( + public_error_message(&next, &inner.config), + "Declined in OpenBot Desktop." + ); + assert!(!inner.state.lock().unwrap().grants["grant"].revoked); + } + + /* + * FOUND ON A MAC: with an "Allow this command?" the server had given up on still on screen, + * every folder action that Bot asked for next failed with "That Bot already has a host + * operation in progress." until someone dismissed the stale dialog. The server's cancel frees + * the Bot, so the next action asks at once. + */ + #[test] + fn a_cancelled_operation_left_on_screen_does_not_hold_up_the_bots_next_one() { + let collector = ResultCollector::start(); + let (asked_tx, asked_rx) = mpsc::channel(); + let (release_tx, release_rx) = mpsc::channel(); + let approval = Arc::new(LateCommandApprovalUi { + asked: AtomicUsize::new(0), + asked_tx: Mutex::new(asked_tx), + release_rx: Mutex::new(release_rx), + lapsed: Mutex::new(Vec::new()), + }); + let config = HostAccessConfig::new( + collector.base_url.clone(), + "token", + Address::new(Engine::Docker, None), + DEFAULT_IMAGE, + vec![], + ); + let inner = Arc::new(Inner { + config, + approval: approval.clone(), + instance_label: "test-instance".into(), + effect_lock: Mutex::new(()), + state: Mutex::new(State { + stopped: false, + thread: None, + grants: HashMap::from([( + "grant".into(), + LocalGrant { + id: "grant".into(), + bot_id: "bot-a".into(), + actor_id: "actor".into(), + bot_name: Some("Research Bot".into()), + root: temp_root("host-access-stale-dialog"), + revoked: false, + }, + )]), + running: HashMap::new(), + completed: HashSet::new(), + canceled_operations: HashSet::new(), + active_by_operation: HashSet::new(), + active_by_bot: HashMap::new(), + }), + }); + let operation = + |id: &str, kind: HostOperationKind, target: Option<&str>| DesktopOperation { + operation_id: id.into(), + kind, + bot_id: "bot-a".into(), + actor_id: "actor".into(), + bot_name: None, + grant_id: matches!(kind, HostOperationKind::RunCommand).then(|| "grant".into()), + target_operation_id: target.map(str::to_string), + relative_path: None, + content: None, + command: matches!(kind, HostOperationKind::RunCommand).then(|| "ls -la".into()), + writable: Some(false), + command_policy: None, + expires_at: None, + received_at_ms: now_millis(), + }; + let client = Client::new(); + + inner.spawn_operation( + client.clone(), + operation("op-stale", HostOperationKind::RunCommand, None), + ); + asked_rx.recv_timeout(Duration::from_secs(1)).unwrap(); + inner.spawn_operation( + client.clone(), + operation("op-cancel", HostOperationKind::Cancel, Some("op-stale")), + ); + + // The stale dialog is still open, and the Bot asks again: the person is asked at once. + inner.spawn_operation( + client.clone(), + operation("op-next", HostOperationKind::RunCommand, None), + ); + asked_rx + .recv_timeout(Duration::from_secs(1)) + .expect("the next command was not asked about while the stale dialog was open"); + collector.wait_for_posts(2); + let posted = |id: &str| { + collector + .bodies() + .iter() + .map(|body| serde_json::from_str::(body).unwrap()) + .find(|body| body["operationId"] == id) + }; + assert_eq!( + posted("op-next").unwrap()["error"], + "Declined in OpenBot Desktop." + ); + + // The stale dialog is answered last, with Allow: nothing runs, and the Bot is free. + release_tx.send(()).unwrap(); + collector.wait_for_posts(3); + assert_eq!( + posted("op-stale").unwrap()["error"], + "Host operation approval expired." + ); + thread::sleep(Duration::from_millis(50)); + let state = inner.state.lock().unwrap(); + assert!(state.active_by_bot.is_empty()); + assert!(!state.grants["grant"].revoked); + } + #[test] fn grant_binding_refuses_wrong_bot_or_actor() { let config = HostAccessConfig::new( @@ -2742,7 +3084,7 @@ mod tests { completed: HashSet::new(), canceled_operations: HashSet::new(), active_by_operation: HashSet::new(), - active_by_bot: HashSet::new(), + active_by_bot: HashMap::new(), }), }; let operation = DesktopOperation { @@ -2918,7 +3260,7 @@ mod live { completed: HashSet::new(), canceled_operations: HashSet::new(), active_by_operation: HashSet::new(), - active_by_bot: HashSet::new(), + active_by_bot: HashMap::new(), }), }; Self { inner, root } From c763e6b5a30474324c3d870749d2c92bb3291a72 Mon Sep 17 00:00:00 2001 From: David McKay Date: Fri, 13 Nov 2026 20:01:38 -0800 Subject: [PATCH 2/3] Desktop: one approval dialog at a time, and one grant per Bot and folder With a stale "Allow this command?" still on screen, the next dialog sat exactly on top of it: two identical dialogs nobody could tell apart. Every dialog now waits for the one before it, the "didn't run" notice included, and one that is no longer wanted by its turn is never shown. Choosing a folder the Bot already had made a second grant beside the first, so Settings listed it twice and the Bot was handed two grants with one name. Choosing it again now makes the existing grant live again, under the id the Bot already knows. --- desktop/src-tauri/src/desktop_host_access.rs | 5 +- desktop/src-tauri/src/host_access.rs | 362 ++++++++++++++----- 2 files changed, 267 insertions(+), 100 deletions(-) diff --git a/desktop/src-tauri/src/desktop_host_access.rs b/desktop/src-tauri/src/desktop_host_access.rs index daeb57a2ba..4a1d2bd11a 100644 --- a/desktop/src-tauri/src/desktop_host_access.rs +++ b/desktop/src-tauri/src/desktop_host_access.rs @@ -291,8 +291,9 @@ impl HostApprovalUi for NativeApproval { if let Some(window) = self.0.get_webview_window("main") { dialog = dialog.parent(&window); } - // Not waited for: the Bot's next action must not queue behind a notice. - dialog.show(|_| {}); + // Waited for, like any dialog here: the next one is shown when this one is closed, never + // on top of it (`prompt_turn` in `host_access.rs`). + dialog.blocking_show(); } } diff --git a/desktop/src-tauri/src/host_access.rs b/desktop/src-tauri/src/host_access.rs index e291ff3826..cfdc46d227 100644 --- a/desktop/src-tauri/src/host_access.rs +++ b/desktop/src-tauri/src/host_access.rs @@ -83,6 +83,8 @@ struct Inner { instance_label: String, state: Mutex, effect_lock: Mutex<()>, + /// Held while a dialog is on screen, so the person is shown one at a time. See `prompt_turn`. + prompt_lock: Mutex<()>, } struct State { @@ -486,6 +488,7 @@ impl HostAccess { approval, instance_label: fresh_id("host-instance"), effect_lock: Mutex::new(()), + prompt_lock: Mutex::new(()), state: Mutex::new(State { stopped: false, thread: None, @@ -841,13 +844,22 @@ impl Inner { } fn choose_folder(&self, operation: &DesktopOperation) -> HostAccessResult { - let approved = self.approval.choose_folder(&ChooseFolderPrompt { - operation_id: operation.operation_id.clone(), - bot_id: operation.bot_id.clone(), - actor_id: operation.actor_id.clone(), - bot_name: operation.bot_name.clone(), - writable_requested: operation.writable == Some(true), - })?; + let approved = { + let _turn = self.prompt_turn(); + self.ensure_operation_fresh(operation)?; + if self.operation_was_canceled(&operation.operation_id) { + return Err(HostAccessError::Denied( + "Host operation was canceled before the folder was granted.".into(), + )); + } + self.approval.choose_folder(&ChooseFolderPrompt { + operation_id: operation.operation_id.clone(), + bot_id: operation.bot_id.clone(), + actor_id: operation.actor_id.clone(), + bot_name: operation.bot_name.clone(), + writable_requested: operation.writable == Some(true), + })? + }; let root = validate_grant_root(&approved.root, &self.config.forbidden_paths)?; self.ensure_operation_fresh(operation)?; if self.operation_was_canceled(&operation.operation_id) { @@ -855,21 +867,34 @@ impl Inner { "Host operation was canceled before the folder was granted.".into(), )); } - let grant_id = fresh_id("host-grant"); let display_name = display_name_for(&root); - let grant = LocalGrant { - id: grant_id.clone(), - bot_id: operation.bot_id.clone(), - actor_id: operation.actor_id.clone(), - bot_name: operation.bot_name.clone(), - root, - revoked: false, - }; - self.state - .lock() - .expect("host state poisoned") + let mut state = self.state.lock().expect("host state poisoned"); + // FOUND ON A MAC: choosing a folder the Bot already had made a second grant beside the + // first, so Settings listed it twice and the Bot was handed two grants with one name. One + // grant per Bot, person and folder: choosing it again makes that grant live again, under the + // id the Bot already knows. + let existing = state .grants - .insert(grant_id.clone(), grant); + .values() + .find(|grant| { + grant.bot_id == operation.bot_id + && grant.actor_id == operation.actor_id + && grant.root == root + }) + .map(|grant| grant.id.clone()); + let grant_id = existing.unwrap_or_else(|| fresh_id("host-grant")); + state.grants.insert( + grant_id.clone(), + LocalGrant { + id: grant_id.clone(), + bot_id: operation.bot_id.clone(), + actor_id: operation.actor_id.clone(), + bot_name: operation.bot_name.clone(), + root, + revoked: false, + }, + ); + drop(state); Ok(OperationSuccess { output: None, grant: Some(OperationGrant { @@ -930,16 +955,20 @@ impl Inner { .clone() .ok_or_else(|| HostAccessError::Denied("Host write missing content.".into()))?; let target = resolve_relative_for_write(&grant.root, relative_path)?; - self.approval.confirm_write(&WritePrompt { - operation_id: operation.operation_id.clone(), - bot_id: operation.bot_id.clone(), - bot_name: grant.bot_name.clone(), - root: grant.root.clone(), - relative_path: relative_path.into(), - content: content.clone(), - writable: true, - })?; - self.ensure_approved_in_time(operation, &grant, LapsedAction::FileChange)?; + { + let _turn = self.prompt_turn(); + self.ensure_operation_still_allowed(operation, &grant)?; + self.approval.confirm_write(&WritePrompt { + operation_id: operation.operation_id.clone(), + bot_id: operation.bot_id.clone(), + bot_name: grant.bot_name.clone(), + root: grant.root.clone(), + relative_path: relative_path.into(), + content: content.clone(), + writable: true, + })?; + self.ensure_approved_in_time(operation, &grant, LapsedAction::FileChange)?; + } let relative = host_relative_for_write(&grant.root, &target)?; let target_path = container_path(&relative); let backup_name = backup_name_for(&relative); @@ -984,17 +1013,21 @@ impl Inner { "Commands on this computer are set to never run.".into(), )); } - self.approval.confirm_command(&CommandPrompt { - operation_id: operation.operation_id.clone(), - bot_id: operation.bot_id.clone(), - bot_name: grant.bot_name.clone(), - root: grant.root.clone(), - working_directory: working_directory.clone(), - command: command.into(), - writable, - command_policy, - })?; - self.ensure_approved_in_time(operation, &grant, LapsedAction::Command)?; + { + let _turn = self.prompt_turn(); + self.ensure_operation_still_allowed(operation, &grant)?; + self.approval.confirm_command(&CommandPrompt { + operation_id: operation.operation_id.clone(), + bot_id: operation.bot_id.clone(), + bot_name: grant.bot_name.clone(), + root: grant.root.clone(), + working_directory: working_directory.clone(), + command: command.into(), + writable, + command_policy, + })?; + self.ensure_approved_in_time(operation, &grant, LapsedAction::Command)?; + } let command_directory = if let Some(directory) = working_directory { let relative = host_relative(&grant.root, &directory)?; container_path(&relative) @@ -1087,6 +1120,17 @@ impl Inner { } Ok(grant.clone()) } + /// The person's turn to be shown a dialog. A native dialog cannot be closed from outside, so one + /// OpenBot stopped waiting for stays up until the person answers it; a second one shown beside it + /// sat exactly on top of it, two identical dialogs nobody could tell apart. Each dialog waits + /// for the one before it, the "didn't run" notice included, and an action that is no longer + /// wanted by the time its turn comes is never shown (`ensure_operation_still_allowed`). + fn prompt_turn(&self) -> std::sync::MutexGuard<'_, ()> { + self.prompt_lock + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + } + /// After the person allowed an action: whether it may still run. When the server stopped /// waiting for the answer first (it expired, or the server cancelled it alone), the person is /// told it did not run, and the server is answered that it expired. @@ -2745,6 +2789,7 @@ mod tests { approval: approval.clone(), instance_label: "test-instance".into(), effect_lock: Mutex::new(()), + prompt_lock: Mutex::new(()), state: Mutex::new(State { stopped: false, thread: None, @@ -2858,6 +2903,7 @@ mod tests { approval: approval.clone(), instance_label: "test-instance".into(), effect_lock: Mutex::new(()), + prompt_lock: Mutex::new(()), state: Mutex::new(State { stopped: false, thread: None, @@ -2942,11 +2988,14 @@ mod tests { /* * FOUND ON A MAC: with an "Allow this command?" the server had given up on still on screen, * every folder action that Bot asked for next failed with "That Bot already has a host - * operation in progress." until someone dismissed the stale dialog. The server's cancel frees - * the Bot, so the next action asks at once. + * operation in progress." until someone dismissed the stale dialog. Freed of that, the next + * dialog then sat exactly on top of the stale one, two identical dialogs nobody could tell + * apart. The next action is taken, and its dialog waits its turn: shown once the stale one is + * answered and its "didn't run" notice closed, and never shown at all if it is no longer wanted + * by then. */ #[test] - fn a_cancelled_operation_left_on_screen_does_not_hold_up_the_bots_next_one() { + fn a_dialog_left_on_screen_holds_up_no_bot_and_the_next_dialog_waits_its_turn() { let collector = ResultCollector::start(); let (asked_tx, asked_rx) = mpsc::channel(); let (release_tx, release_rx) = mpsc::channel(); @@ -2963,25 +3012,27 @@ mod tests { DEFAULT_IMAGE, vec![], ); + let grant = |bot: &str| LocalGrant { + id: format!("grant-{bot}"), + bot_id: bot.into(), + actor_id: "actor".into(), + bot_name: Some("Research Bot".into()), + root: temp_root("host-access-stale-dialog"), + revoked: false, + }; let inner = Arc::new(Inner { config, approval: approval.clone(), instance_label: "test-instance".into(), effect_lock: Mutex::new(()), + prompt_lock: Mutex::new(()), state: Mutex::new(State { stopped: false, thread: None, - grants: HashMap::from([( - "grant".into(), - LocalGrant { - id: "grant".into(), - bot_id: "bot-a".into(), - actor_id: "actor".into(), - bot_name: Some("Research Bot".into()), - root: temp_root("host-access-stale-dialog"), - revoked: false, - }, - )]), + grants: HashMap::from([ + ("grant-bot-a".into(), grant("bot-a")), + ("grant-bot-b".into(), grant("bot-b")), + ]), running: HashMap::new(), completed: HashSet::new(), canceled_operations: HashSet::new(), @@ -2989,44 +3040,29 @@ mod tests { active_by_bot: HashMap::new(), }), }); - let operation = - |id: &str, kind: HostOperationKind, target: Option<&str>| DesktopOperation { - operation_id: id.into(), - kind, - bot_id: "bot-a".into(), - actor_id: "actor".into(), - bot_name: None, - grant_id: matches!(kind, HostOperationKind::RunCommand).then(|| "grant".into()), - target_operation_id: target.map(str::to_string), - relative_path: None, - content: None, - command: matches!(kind, HostOperationKind::RunCommand).then(|| "ls -la".into()), - writable: Some(false), - command_policy: None, - expires_at: None, - received_at_ms: now_millis(), - }; - let client = Client::new(); - - inner.spawn_operation( - client.clone(), - operation("op-stale", HostOperationKind::RunCommand, None), - ); - asked_rx.recv_timeout(Duration::from_secs(1)).unwrap(); - inner.spawn_operation( - client.clone(), - operation("op-cancel", HostOperationKind::Cancel, Some("op-stale")), - ); - - // The stale dialog is still open, and the Bot asks again: the person is asked at once. - inner.spawn_operation( - client.clone(), - operation("op-next", HostOperationKind::RunCommand, None), - ); - asked_rx - .recv_timeout(Duration::from_secs(1)) - .expect("the next command was not asked about while the stale dialog was open"); - collector.wait_for_posts(2); + let command = |id: &str, bot: &str| DesktopOperation { + operation_id: id.into(), + kind: HostOperationKind::RunCommand, + bot_id: bot.into(), + actor_id: "actor".into(), + bot_name: None, + grant_id: Some(format!("grant-{bot}")), + target_operation_id: None, + relative_path: None, + content: None, + command: Some("ls -la".into()), + writable: Some(false), + command_policy: None, + expires_at: None, + received_at_ms: now_millis(), + }; + let cancel = |id: &str, target: &str, bot: &str| DesktopOperation { + kind: HostOperationKind::Cancel, + grant_id: None, + command: None, + target_operation_id: Some(target.into()), + ..command(id, bot) + }; let posted = |id: &str| { collector .bodies() @@ -3034,22 +3070,150 @@ mod tests { .map(|body| serde_json::from_str::(body).unwrap()) .find(|body| body["operationId"] == id) }; + let client = Client::new(); + + inner.spawn_operation(client.clone(), command("op-stale", "bot-a")); + asked_rx.recv_timeout(Duration::from_secs(1)).unwrap(); + inner.spawn_operation(client.clone(), cancel("op-c1", "op-stale", "bot-a")); + + // The person is away. The Bot asks again, and another Bot asks for something that is then + // given up on too, while the stale dialog is still open. + inner.spawn_operation(client.clone(), command("op-next", "bot-a")); + inner.spawn_operation(client.clone(), command("op-gone", "bot-b")); + thread::sleep(Duration::from_millis(150)); + inner.spawn_operation(client.clone(), cancel("op-c2", "op-gone", "bot-b")); + thread::sleep(Duration::from_millis(150)); + assert!( + posted("op-next").is_none(), + "the Bot's next action was turned away: {:?}", + posted("op-next") + ); assert_eq!( - posted("op-next").unwrap()["error"], - "Declined in OpenBot Desktop." + approval.asked.load(Ordering::SeqCst), + 1, + "a second dialog was shown on top of the stale one" ); - // The stale dialog is answered last, with Allow: nothing runs, and the Bot is free. + // The person comes back and presses Allow on the stale dialog: nothing runs, they are told + // so, and only then is the next dialog shown. The one given up on meanwhile never is. release_tx.send(()).unwrap(); - collector.wait_for_posts(3); + asked_rx + .recv_timeout(Duration::from_secs(1)) + .expect("the waiting dialog was not shown after the stale one was answered"); + collector.wait_for_posts(5); + assert_eq!(approval.lapsed.lock().unwrap().len(), 1); + assert_eq!(approval.asked.load(Ordering::SeqCst), 2); assert_eq!( posted("op-stale").unwrap()["error"], "Host operation approval expired." ); + assert_eq!( + posted("op-next").unwrap()["error"], + "Declined in OpenBot Desktop." + ); + assert_eq!(posted("op-gone").unwrap()["ok"], false); thread::sleep(Duration::from_millis(50)); let state = inner.state.lock().unwrap(); assert!(state.active_by_bot.is_empty()); - assert!(!state.grants["grant"].revoked); + assert!(!state.grants["grant-bot-a"].revoked); + } + + struct SameFolderApprovalUi { + root: PathBuf, + } + + impl HostApprovalUi for SameFolderApprovalUi { + fn choose_folder(&self, _: &ChooseFolderPrompt) -> HostAccessResult { + Ok(ApprovedFolder { + root: self.root.clone(), + }) + } + + fn confirm_write(&self, _: &WritePrompt) -> HostAccessResult<()> { + unreachable!("only folders are chosen here") + } + + fn confirm_command(&self, _: &CommandPrompt) -> HostAccessResult<()> { + unreachable!("only folders are chosen here") + } + } + + /* + * FOUND ON A MAC: choosing a folder the Bot already had made a second grant beside the first, + * so Settings → Computer read "2 read-only folders" and the Bot was handed two grants with one + * name. + */ + #[test] + fn choosing_a_folder_the_bot_already_has_keeps_one_grant_and_makes_it_live_again() { + let root = fs::canonicalize(temp_root("host-access-same-folder")).unwrap(); + let inner = Inner { + config: HostAccessConfig::new( + "http://127.0.0.1:9", + "token", + Address::new(Engine::Docker, None), + DEFAULT_IMAGE, + vec![], + ), + approval: Arc::new(SameFolderApprovalUi { root: root.clone() }), + instance_label: "test-instance".into(), + effect_lock: Mutex::new(()), + prompt_lock: Mutex::new(()), + state: Mutex::new(State { + stopped: false, + thread: None, + grants: HashMap::new(), + running: HashMap::new(), + completed: HashSet::new(), + canceled_operations: HashSet::new(), + active_by_operation: HashSet::new(), + active_by_bot: HashMap::new(), + }), + }; + let choose = |id: &str, bot: &str| DesktopOperation { + operation_id: id.into(), + kind: HostOperationKind::ChooseFolder, + bot_id: bot.into(), + actor_id: "actor".into(), + bot_name: Some("Research Bot".into()), + grant_id: None, + target_operation_id: None, + relative_path: None, + content: None, + command: None, + writable: Some(false), + command_policy: None, + expires_at: None, + received_at_ms: now_millis(), + }; + let granted = |id: &str, bot: &str| { + inner + .handle_operation(&choose(id, bot)) + .unwrap() + .grant + .unwrap() + .grant_id + }; + + let first = granted("op-1", "bot-a"); + inner + .state + .lock() + .unwrap() + .grants + .get_mut(&first) + .unwrap() + .revoked = true; + let again = granted("op-2", "bot-a"); + assert_eq!(again, first); + { + let state = inner.state.lock().unwrap(); + assert_eq!(state.grants.len(), 1); + assert!(!state.grants[&first].revoked); + } + // Another Bot given the same folder has its own grant. + assert_ne!(granted("op-3", "bot-b"), first); + assert_eq!(inner.state.lock().unwrap().grants.len(), 2); + fs::remove_dir_all(root).unwrap(); } #[test] @@ -3066,6 +3230,7 @@ mod tests { approval: Arc::new(DenyAllApprovalUi), instance_label: "test-instance".into(), effect_lock: Mutex::new(()), + prompt_lock: Mutex::new(()), state: Mutex::new(State { stopped: false, thread: None, @@ -3252,6 +3417,7 @@ mod live { approval: Arc::new(AllowAll), instance_label: fresh_id("host-live"), effect_lock: Mutex::new(()), + prompt_lock: Mutex::new(()), state: Mutex::new(State { stopped: false, thread: None, From 6f28db84eeed3497fe150dfc9eaa047c6d44742c Mon Sep 17 00:00:00 2001 From: David McKay Date: Fri, 13 Nov 2026 20:07:36 -0800 Subject: [PATCH 3/3] Desktop: pin that a folder chosen again carries no write access A grant is read-only whichever time it was chosen. The test now checks that after the same folder is chosen again, a file change and a command that can change files still ask the person, even under always allow. --- desktop/src-tauri/src/host_access.rs | 47 +++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 4 deletions(-) diff --git a/desktop/src-tauri/src/host_access.rs b/desktop/src-tauri/src/host_access.rs index cfdc46d227..1d2f04aae5 100644 --- a/desktop/src-tauri/src/host_access.rs +++ b/desktop/src-tauri/src/host_access.rs @@ -3120,6 +3120,7 @@ mod tests { struct SameFolderApprovalUi { root: PathBuf, + asked_to_change: AtomicUsize, } impl HostApprovalUi for SameFolderApprovalUi { @@ -3130,11 +3131,21 @@ mod tests { } fn confirm_write(&self, _: &WritePrompt) -> HostAccessResult<()> { - unreachable!("only folders are chosen here") + self.asked_to_change.fetch_add(1, Ordering::SeqCst); + Err(HostAccessError::Denied( + "Declined in OpenBot Desktop.".into(), + )) } - fn confirm_command(&self, _: &CommandPrompt) -> HostAccessResult<()> { - unreachable!("only folders are chosen here") + fn confirm_command(&self, request: &CommandPrompt) -> HostAccessResult<()> { + assert!( + request.writable, + "only commands that change files are asked here" + ); + self.asked_to_change.fetch_add(1, Ordering::SeqCst); + Err(HostAccessError::Denied( + "Declined in OpenBot Desktop.".into(), + )) } } @@ -3146,6 +3157,11 @@ mod tests { #[test] fn choosing_a_folder_the_bot_already_has_keeps_one_grant_and_makes_it_live_again() { let root = fs::canonicalize(temp_root("host-access-same-folder")).unwrap(); + fs::write(root.join("notes.txt"), "hello").unwrap(); + let approval = Arc::new(SameFolderApprovalUi { + root: root.clone(), + asked_to_change: AtomicUsize::new(0), + }); let inner = Inner { config: HostAccessConfig::new( "http://127.0.0.1:9", @@ -3154,7 +3170,7 @@ mod tests { DEFAULT_IMAGE, vec![], ), - approval: Arc::new(SameFolderApprovalUi { root: root.clone() }), + approval: approval.clone(), instance_label: "test-instance".into(), effect_lock: Mutex::new(()), prompt_lock: Mutex::new(()), @@ -3210,6 +3226,29 @@ mod tests { assert_eq!(state.grants.len(), 1); assert!(!state.grants[&first].revoked); } + // A grant is read-only whichever time it was chosen: the grant it comes back as carries + // nothing over, and every change to the folder still asks the person, each time. + let change = |id: &str, kind: HostOperationKind| DesktopOperation { + kind, + grant_id: Some(first.clone()), + relative_path: matches!(kind, HostOperationKind::WriteFile).then(|| "notes.txt".into()), + content: matches!(kind, HostOperationKind::WriteFile).then(|| "changed".into()), + command: matches!(kind, HostOperationKind::RunCommand).then(|| "rm notes.txt".into()), + writable: Some(matches!(kind, HostOperationKind::RunCommand)), + command_policy: Some("allow".into()), + ..choose(id, "bot-a") + }; + for (id, kind) in [ + ("op-write", HostOperationKind::WriteFile), + ("op-edit", HostOperationKind::RunCommand), + ] { + let refused = inner.handle_operation(&change(id, kind)).map(|_| ()); + assert_eq!( + public_error_message(&refused.unwrap_err(), &inner.config), + "Declined in OpenBot Desktop." + ); + } + assert_eq!(approval.asked_to_change.load(Ordering::SeqCst), 2); // Another Bot given the same folder has its own grant. assert_ne!(granted("op-3", "bot-b"), first); assert_eq!(inner.state.lock().unwrap().grants.len(), 2);