Security: cell hyperlinks open only http, https and mailto, after a confirmation (#777)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
fc45b7f6d8
commit
2797d468c5
|
|
@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in
|
|||
|
||||
| Date | What landed |
|
||||
|---|---|
|
||||
| 2026-10-12 | Security: cell hyperlinks open only http, https and mailto, after a confirmation; other schemes (file:, smb:, UNC, custom handlers) are refused (#777, part 1) |
|
||||
| 2026-10-12 | File › Print selects a system printer, copies, duplex and grayscale, submits PDF jobs through craft-print on a worker and shows spooler errors; browser printing and PDF export remain available |
|
||||
| 2026-10-11 | Command-sequence fuzzing: random command sequences checked against undo/redo, XLSX round-trip and full recalculation; fixes for the table, legacy-array, spill, AutoSum, Watch Window and sheet-copy bugs it found |
|
||||
| 2026-10-11 | Password-protected workbooks open: GridCraft asks for the password and decrypts Agile and Standard encryption (#11) |
|
||||
|
|
|
|||
|
|
@ -144,8 +144,10 @@ fn services(inbox: Inbox, ctx: egui::Context) -> Services {
|
|||
}
|
||||
})),
|
||||
open_url: Some(Box::new(|url: &str| {
|
||||
if let Some(w) = web_sys::window() {
|
||||
let _ = w.open_with_url_and_target(url, "_blank");
|
||||
if let Some(w) = web_sys::window()
|
||||
&& let gridcraft_engine::links::LinkKind::External(url) = gridcraft_engine::links::classify(url)
|
||||
{
|
||||
let _ = w.open_with_url_and_target(&url, "_blank");
|
||||
}
|
||||
})),
|
||||
inbox: Some(inbox),
|
||||
|
|
|
|||
|
|
@ -194,6 +194,13 @@ fn services() -> Services {
|
|||
|
||||
/// Opens a URL or file with the system's default application.
|
||||
fn open_with_system(target: &str) {
|
||||
// Last line of defence behind `link.open` and the click prompt: the system only ever gets
|
||||
// http, https and mailto addresses.
|
||||
let gridcraft_engine::links::LinkKind::External(target) = gridcraft_engine::links::classify(target) else {
|
||||
log::warn!("refused to open {target:?}: only http, https and mailto links are opened");
|
||||
return;
|
||||
};
|
||||
let target = target.as_str();
|
||||
let cmd = if cfg!(target_os = "macos") {
|
||||
"open"
|
||||
} else if cfg!(windows) {
|
||||
|
|
|
|||
|
|
@ -513,7 +513,7 @@ fn select_col(s: &mut Session, _: &Json) -> Result<Json> {
|
|||
Ok(json!({"selection": d.selection.a1()}))
|
||||
}
|
||||
|
||||
fn go_to(s: &mut Session, p: &Json) -> Result<Json> {
|
||||
pub(super) fn go_to(s: &mut Session, p: &Json) -> Result<Json> {
|
||||
let Some(reference) = str_param(p, "reference").or_else(|| str_param(p, "range")) else { return Err(bad("edit.goTo", "missing `reference`")) };
|
||||
// A defined name or table?
|
||||
let d = s.doc()?;
|
||||
|
|
|
|||
|
|
@ -157,6 +157,15 @@ pub fn specs() -> Vec<CommandSpec> {
|
|||
has_doc,
|
||||
insert_link
|
||||
),
|
||||
cmd!(
|
||||
"link.open",
|
||||
"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",
|
||||
has_doc,
|
||||
open_link
|
||||
),
|
||||
cmd!("review.newNote", "New Note", ["Review", "Notes"], Some("Shift+F2"), "{cell?, text, author?}", has_doc, |s, p| comment(s, p, false)),
|
||||
cmd!("review.newComment", "New Comment", ["Review", "Comments"], Some("Cmd+Shift+F2"), "{cell?, text, author?}", has_doc, |s, p| comment(
|
||||
s, p, true
|
||||
|
|
@ -1173,6 +1182,31 @@ 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.
|
||||
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(),
|
||||
None => {
|
||||
let sheet = target_sheet(s, p)?;
|
||||
let at = cell_param_on(s, p, "cell", sheet)?.unwrap_or(s.doc()?.selection.active);
|
||||
let Some(h) = s.doc()?.wb.sheet(sheet).and_then(|sh| sh.hyperlinks.get(&at)) else {
|
||||
return Err(EngineError::Other("There's no hyperlink in this cell.".into()));
|
||||
};
|
||||
h.target.clone()
|
||||
}
|
||||
};
|
||||
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) => {
|
||||
s.ui_requests.push(crate::UiRequest::OpenUrl(u.clone()));
|
||||
Ok(json!({"opened": u}))
|
||||
}
|
||||
crate::links::LinkKind::Refused(why) => Err(EngineError::Other(why)),
|
||||
}
|
||||
}
|
||||
|
||||
fn comment(s: &mut Session, p: &Json, threaded: bool) -> Result<Json> {
|
||||
let sheet = target_sheet(s, p)?;
|
||||
let at = cell_param_on(s, p, "cell", sheet)?.unwrap_or(s.doc()?.selection.active);
|
||||
|
|
|
|||
|
|
@ -13,6 +13,7 @@ pub mod cmd;
|
|||
pub mod display;
|
||||
pub mod fill;
|
||||
pub mod io;
|
||||
pub mod links;
|
||||
pub mod outline;
|
||||
pub mod paper;
|
||||
pub mod pivot;
|
||||
|
|
|
|||
|
|
@ -0,0 +1,83 @@
|
|||
//! Which hyperlink targets may be opened. A cell's link comes from the document (often a file
|
||||
//! 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.
|
||||
|
||||
use gridcraft_core::RangeRef;
|
||||
|
||||
/// What a link target is.
|
||||
#[derive(Clone, Debug, PartialEq, Eq)]
|
||||
pub enum LinkKind {
|
||||
/// A cell, range, name or table of the workbook (`Sheet2!A1`, `Total`).
|
||||
Internal(String),
|
||||
/// An address to hand to the system, trimmed: `http`, `https` or `mailto` only.
|
||||
External(String),
|
||||
/// Refused, with the reason to show.
|
||||
Refused(String),
|
||||
}
|
||||
|
||||
const ALLOWED: [&str; 3] = ["http", "https", "mailto"];
|
||||
|
||||
/// Classifies a link target. Never panics; the single place that decides what may be opened.
|
||||
pub fn classify(target: &str) -> LinkKind {
|
||||
let t = target.trim();
|
||||
if t.chars().any(char::is_control) {
|
||||
return LinkKind::Refused("The link contains control characters.".into());
|
||||
}
|
||||
if t.starts_with("\\\\") || t.starts_with("//") {
|
||||
return LinkKind::Refused("Links to network locations aren't opened.".into());
|
||||
}
|
||||
if RangeRef::parse(t).is_some() {
|
||||
return LinkKind::Internal(t.to_string());
|
||||
}
|
||||
let Some((scheme, rest)) = t.split_once(':') else { return LinkKind::Internal(t.to_string()) };
|
||||
let scheme_like = scheme.chars().next().is_some_and(|c| c.is_ascii_alphabetic())
|
||||
&& scheme.chars().all(|c| c.is_ascii_alphanumeric() || matches!(c, '+' | '-' | '.'));
|
||||
if !scheme_like {
|
||||
return LinkKind::Internal(t.to_string());
|
||||
}
|
||||
let lower = scheme.to_ascii_lowercase();
|
||||
if ALLOWED.contains(&lower.as_str()) && !rest.trim().is_empty() {
|
||||
LinkKind::External(t.to_string())
|
||||
} else {
|
||||
LinkKind::Refused(format!("Links of type \"{lower}:\" aren't opened. Only http, https and mailto links are allowed."))
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn only_http_https_and_mailto_leave_the_workbook() {
|
||||
for ok in ["http://a.example/x", "HTTPS://A.example", " https://a.example ", "MailTo:a@b.example"] {
|
||||
assert!(matches!(classify(ok), LinkKind::External(_)), "{ok}");
|
||||
}
|
||||
assert_eq!(classify(" https://a.example "), LinkKind::External("https://a.example".into()));
|
||||
for bad in [
|
||||
"javascript:alert(1)",
|
||||
"JaVaScRiPt:alert(1)",
|
||||
"file:///etc/passwd",
|
||||
"smb://host/share",
|
||||
"ms-excel:ofe|u|http://x",
|
||||
"\\\\host\\share\\x.exe",
|
||||
"//host/share",
|
||||
"C:\\Windows\\calc.exe",
|
||||
"\tjavascript:1",
|
||||
"java\nscript:1",
|
||||
"\u{1}http://a.example",
|
||||
"http:",
|
||||
"mailto:",
|
||||
"x-custom://a",
|
||||
] {
|
||||
assert!(matches!(classify(bad), LinkKind::Refused(_)), "{bad:?} -> {:?}", classify(bad));
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn references_stay_internal() {
|
||||
for r in ["Sheet2!A1", "'My Sheet'!B2:C3", "A1:B2", "#Sheet1!A1", "Total", "", " "] {
|
||||
assert!(matches!(classify(r), LinkKind::Internal(_)), "{r:?} -> {:?}", classify(r));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -2795,3 +2795,19 @@ fn a_decrypted_workbook_never_overwrites_its_encrypted_original() {
|
|||
assert_eq!(std::fs::read(&path).unwrap(), bytes, "the encrypted original is untouched");
|
||||
std::fs::remove_dir_all(dir).unwrap();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn link_open_refuses_every_scheme_but_http_https_and_mailto() {
|
||||
let mut s = s();
|
||||
s.execute("insert.link", json!({"cell": "A1", "target": "file:///etc/passwd"})).unwrap();
|
||||
s.take_ui_requests();
|
||||
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.execute("link.open", json!({"cell": "A1"})).is_err());
|
||||
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());
|
||||
}
|
||||
|
|
|
|||
|
|
@ -338,6 +338,20 @@ 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.
|
||||
"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}),
|
||||
)
|
||||
}
|
||||
"insertLink" => Dialog::form(
|
||||
"insertLink",
|
||||
"Insert Hyperlink",
|
||||
|
|
|
|||
|
|
@ -2281,16 +2281,19 @@ fn delete_selected_object(app: &mut SheetApp, sh: &Sheet) -> bool {
|
|||
true
|
||||
}
|
||||
|
||||
/// 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.
|
||||
pub fn follow_link(app: &mut SheetApp, target: &str) {
|
||||
if target.contains("://") || target.starts_with("mailto:") {
|
||||
if let Some(f) = &app.services.open_url {
|
||||
f(target);
|
||||
}
|
||||
} else {
|
||||
let t = target.trim_start_matches('#');
|
||||
let _ = app.session.run("edit.goTo", json!({"reference": t}));
|
||||
use gridcraft_engine::links::{LinkKind, classify};
|
||||
match classify(target) {
|
||||
LinkKind::Internal(r) => {
|
||||
let _ = app.session.run("edit.goTo", json!({"reference": r.trim_start_matches('#')}));
|
||||
app.grid.ensure_visible = true;
|
||||
}
|
||||
LinkKind::External(url) => app.open_dialog("openLink", json!({"url": url})),
|
||||
LinkKind::Refused(why) => app.message = Some(("Hyperlink".into(), why)),
|
||||
}
|
||||
}
|
||||
|
||||
/// The areas after a click: with `add` (Ctrl) the clicked area is added, or removed when it is already
|
||||
|
|
|
|||
Loading…
Reference in New Issue