From 08da5c5433d0b494bc3ab6e7dcb5d9de34cd73c2 Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 15:11:51 +0100 Subject: [PATCH 01/16] #3699: MCP bridge sends the control key only to 127.0.0.1, where the app listens Audit finding 9 (CWE-522). The bridge sent the reusable control key to any loopback peer (127.0.0.0/8, ::1) and tried every address `localhost` resolved to, while the app binds only 127.0.0.1. Another local user could listen on ::1 or 127.0.0.2 and receive the key. Now `localhost` connects to 127.0.0.1 only, and the key goes only to a 127.0.0.1 peer. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/mcp/src/backend.rs | 86 +++++++++++++++++++++++++++------------ docs/control-protocol.md | 4 +- 2 files changed, 63 insertions(+), 27 deletions(-) diff --git a/crates/mcp/src/backend.rs b/crates/mcp/src/backend.rs index ac32f5d32..1cf17c61d 100644 --- a/crates/mcp/src/backend.rs +++ b/crates/mcp/src/backend.rs @@ -1,7 +1,7 @@ //! Where MCP tool calls end up: a control-channel method call. use std::io::{BufRead, BufReader, Write}; -use std::net::{IpAddr, SocketAddr, TcpStream, ToSocketAddrs}; +use std::net::{IpAddr, Ipv4Addr, SocketAddr, TcpStream, ToSocketAddrs}; use std::path::Path; use std::time::Duration; @@ -26,7 +26,7 @@ pub struct Remote { addr: String, conn: Option<(BufReader, TcpStream)>, next_id: u64, - /// The connection leads to a loopback address: requests carry the control key. + /// The connection leads to the app's 127.0.0.1: requests carry the control key. send_key: bool, /// The control key, looked up once per connection (the app picks a new one at every start). key: Option, @@ -55,25 +55,46 @@ fn find_key(addr: &str) -> Result { lookup_key(std::env::var("WORDCRAFT_CONTROL_KEY").ok(), wordcraft_control_key::settings_dir().as_deref(), port) } -/// Whether `addr` (`HOST:PORT`, as given to `--connect`) names a loopback address: 127.0.0.0/8, -/// ::1 or `localhost`. Other host names are not resolved: they never count. -fn is_loopback_addr(addr: &str) -> bool { - let loopback = |ip: IpAddr| ip.to_canonical().is_loopback(); - if let Ok(sa) = addr.parse::() { - return loopback(sa.ip()); - } - if let Ok(ip) = addr.parse::() { - return loopback(ip); - } +/// The only address the app's control server binds (`apps/wordcraft/src/control_server.rs`). +const APP_IP: IpAddr = IpAddr::V4(Ipv4Addr::LOCALHOST); + +/// The host part of `addr` (`HOST:PORT`, `[V6]:PORT`), without brackets. +fn host_of(addr: &str) -> &str { let host = addr.rsplit_once(':').map_or(addr, |(h, _)| h); - let host = host.strip_prefix('[').and_then(|h| h.strip_suffix(']')).unwrap_or(host); - host.parse::().map_or_else(|_| host.eq_ignore_ascii_case("localhost"), loopback) + host.strip_prefix('[').and_then(|h| h.strip_suffix(']')).unwrap_or(host) } -/// The key goes only to a loopback `--connect` address, and only when the connection really -/// leads to one (`peer`, so a `localhost` that resolves elsewhere gets no key either). +/// Whether `addr` (`HOST:PORT`, as given to `--connect`) names the app's own address: +/// 127.0.0.1 or `localhost`. Other loopback addresses (127.0.0.2, ::1) don't count: the app never +/// listens there, so whatever does is not the app. Other host names are not resolved. +fn is_app_addr(addr: &str) -> bool { + let app = |ip: IpAddr| ip.to_canonical() == APP_IP; + if let Ok(sa) = addr.parse::() { + return app(sa.ip()); + } + if let Ok(ip) = addr.parse::() { + return app(ip); + } + let host = host_of(addr); + host.parse::().map_or_else(|_| host.eq_ignore_ascii_case("localhost"), app) +} + +/// The key goes only to the app's `--connect` address, and only when the connection really +/// leads to 127.0.0.1 (`peer`): a listener on another loopback address (::1, 127.0.0.2), which +/// any local user can open, never sees it. fn may_send_key(addr: &str, peer: Option) -> bool { - is_loopback_addr(addr) && peer.is_some_and(|p| p.ip().to_canonical().is_loopback()) + is_app_addr(addr) && peer.is_some_and(|p| p.ip().to_canonical() == APP_IP) +} + +/// Where to connect for `addr`: `localhost` means the app's 127.0.0.1 (not whatever the resolver +/// lists first, which may be ::1); anything else as resolved. +fn targets(addr: &str) -> std::io::Result> { + if host_of(addr).eq_ignore_ascii_case("localhost") + && let Some(port) = addr.rsplit_once(':').and_then(|(_, p)| p.parse::().ok()) + { + return Ok(vec![SocketAddr::new(APP_IP, port)]); + } + Ok(addr.to_socket_addrs()?.collect()) } impl Remote { @@ -95,11 +116,12 @@ impl Remote { &self.addr } - /// Why requests go without the control key (the address is not loopback), if they do. + /// Why requests go without the control key (the address is not the app's 127.0.0.1), if they + /// do. pub fn key_warning(&self) -> Option { (!self.send_key).then(|| { format!( - "{} is not a loopback address, so the control key is not sent (it only goes to 127.0.0.0/8, ::1 and localhost) and the app refuses requests; reach the app through 127.0.0.1, for example with an SSH tunnel", + "{} is not the app's address, so the control key is not sent (it only goes to 127.0.0.1 and localhost, where the app listens) and the app refuses requests; reach the app through 127.0.0.1, for example with an SSH tunnel", self.addr ) }) @@ -109,7 +131,7 @@ impl Remote { self.conn = None; self.key = None; let mut last = std::io::Error::new(std::io::ErrorKind::NotFound, format!("cannot resolve {}", self.addr)); - for sa in self.addr.to_socket_addrs()? { + for sa in targets(&self.addr)? { match TcpStream::connect_timeout(&sa, Duration::from_millis(800)) { Ok(s) => { s.set_nodelay(true).ok(); @@ -256,15 +278,12 @@ mod tests { } #[test] - fn key_only_for_loopback_addresses() { + fn key_only_for_the_apps_address() { let peer = |s: &str| s.parse::().ok(); for (addr, at) in [ ("127.0.0.1:7981", "127.0.0.1:7981"), - ("127.0.0.2:7981", "127.0.0.2:7981"), - ("[::1]:7981", "[::1]:7981"), ("[::ffff:127.0.0.1]:7981", "[::ffff:127.0.0.1]:7981"), ("localhost:7981", "127.0.0.1:7981"), - ("localhost:7981", "[::1]:7981"), ("LocalHost:7981", "127.0.0.1:7981"), ] { assert!(may_send_key(addr, peer(at)), "{addr} at {at}"); @@ -274,6 +293,11 @@ mod tests { ("10.0.0.7:7981", "10.0.0.7:7981"), ("[fe80::1]:7981", "[fe80::1]:7981"), ("0.0.0.0:7981", "127.0.0.1:7981"), + // Loopback addresses the app never binds: another local user's listener (#3699). + ("127.0.0.2:7981", "127.0.0.2:7981"), + ("[::1]:7981", "[::1]:7981"), + ("localhost:7981", "[::1]:7981"), + ("127.0.0.1:7981", "127.0.0.2:7981"), // Host names other than localhost: even when they lead to this machine. ("workstation:7981", "127.0.1.1:7981"), ("example.org:7981", "93.184.215.14:7981"), @@ -287,6 +311,16 @@ mod tests { assert!(!may_send_key("127.0.0.1:7981", None)); } + #[test] + fn localhost_connects_to_the_apps_address_only() { + // Not the resolver's list, which may put ::1 (where anyone can listen) first. + let at: SocketAddr = "127.0.0.1:7981".parse().unwrap(); + assert_eq!(targets("localhost:7981").unwrap(), vec![at]); + assert_eq!(targets("LOCALHOST:7981").unwrap(), vec![at]); + assert_eq!(targets("127.0.0.1:7981").unwrap(), vec![at]); + assert_eq!(targets("[::1]:7981").unwrap(), vec!["[::1]:7981".parse::().unwrap()]); + } + /// A stand-in for the app on 127.0.0.1: every request line goes to the returned channel and /// gets `answer(request)`: a reply and whether to close the connection after it, or `None` /// to close it without a reply (the app quit). @@ -377,9 +411,9 @@ mod tests { let finder: KeyFinder = Box::new(|_| Err("the key must not be read for another host".to_string())); let mut r = Remote::connect_with(&format!("127.0.0.1:{port}"), finder).unwrap(); r.send_key = false; - assert!(r.key_warning().is_some_and(|w| w.contains("not a loopback address"))); + assert!(r.key_warning().is_some_and(|w| w.contains("not the app's address"))); let e = r.call("document.inspect", json!({})).unwrap_err(); - assert!(e.contains("not a loopback address") && e.contains("not sent"), "{e}"); + assert!(e.contains("not the app's address") && e.contains("not sent"), "{e}"); assert!(seen.recv_timeout(WAIT).unwrap().get("key").is_none()); } } diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 08c62c3d9..a88e0be9f 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -43,7 +43,9 @@ cat ~/.config/wordcraft/control-key.local-7981 # Linux, outside Flatpak, port `wordcraft-cli mcp --connect` takes the key from `WORDCRAFT_CONTROL_KEY`, or else from the key file for the port it connects to, and reads it again on every new connection (the app may have -restarted). It sends the key only to loopback addresses (`127.0.0.0/8`, `::1`, `localhost`). For +restarted). It sends the key only to the app's own address, `127.0.0.1` (`localhost` connects +there too, never to `::1`): another loopback address such as `::1` or `127.0.0.2` is not where the +app listens, so whoever listens there gets no key (#3699). For any other host it sends no key, says so, and the app refuses its requests: to drive an app on another machine, forward its port to `127.0.0.1` (for example `ssh -L 7981:127.0.0.1:7981 host`) and set `WORDCRAFT_CONTROL_KEY` to that app's key. From 3d51cd3db264f01ff36bf973824e97abec4d8193 Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 15:13:28 +0100 Subject: [PATCH 02/16] #3692: MCP type_text types its paragraphs as one edit and one undo step Audit finding 18 (CWE-407). `type_text` with `paragraphs: true` ran a text.insert and a text.newParagraph per line, each a mutating command with its own whole-document undo snapshot, so a long text did quadratic work and filled the undo history. It now sends one raw text.insert with `\r` paragraph breaks (Enter's split): one edit, one undo step, all or nothing. More than MAX_TYPED_PARAGRAPHS (10 000) lines is refused before anything is typed. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/mcp/src/tests.rs | 43 +++++++++++++++++++++++++++++++++-------- crates/mcp/src/tools.rs | 30 ++++++++++++++-------------- 2 files changed, 49 insertions(+), 24 deletions(-) diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index 781f24d02..e00037134 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -120,17 +120,16 @@ fn headless_sessions_keep_the_dictionary_in_memory() { assert!(h.session.user_dict_path.is_none()); } -/// A backend whose paragraph break is refused while text insertion would succeed. +/// A backend that keeps every command it is sent and refuses text with a paragraph break. struct RefusesParagraph { - calls: std::sync::Arc>>, + calls: std::sync::Arc>>, } impl crate::backend::Backend for RefusesParagraph { fn call(&mut self, _method: &str, params: Value) -> Result { - let command = params["command"].as_str().unwrap_or("").to_string(); - self.calls.lock().unwrap().push(command.clone()); - match command.as_str() { - "text.newParagraph" => Err("paragraph break refused".into()), + self.calls.lock().unwrap().push(params.clone()); + match params["params"]["text"].as_str() { + Some(t) if t.contains('\r') => Err("paragraph break refused".into()), _ => Ok(json!({"ok": true})), } } @@ -142,7 +141,8 @@ impl crate::backend::Backend for RefusesParagraph { } } -/// #1356: a refused paragraph break is reported and the next line is not typed after it. +/// #1356: a refused paragraph break is reported, and nothing of the text goes in without it: the +/// lines are one command, all or nothing. #[test] fn type_text_stops_at_a_refused_paragraph_break() { let calls = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); @@ -150,5 +150,32 @@ fn type_text_stops_at_a_refused_paragraph_break() { let r = call(&mut s, 11, "tools/call", json!({"name": "type_text", "arguments": {"text": "one\ntwo", "paragraphs": true}})); assert_eq!(r["result"]["isError"], true, "{r}"); assert!(r["result"]["content"][0]["text"].as_str().unwrap().contains("paragraph break refused"), "{r}"); - assert_eq!(*calls.lock().unwrap(), vec!["text.insert", "text.newParagraph"]); + let calls = calls.lock().unwrap(); + assert_eq!(calls.len(), 1, "{calls:?}"); + assert_eq!(calls[0]["command"], "text.insert"); + assert_eq!(calls[0]["params"]["text"], "one\rtwo"); +} + +/// #3692: typed paragraphs are one edit and one undo step, not a command (and a whole-document +/// undo snapshot) per line; too many lines are refused before anything is typed. +#[test] +fn type_text_paragraphs_are_one_undo_step_with_a_line_cap() { + let mut s = Server::new(Box::new(Headless::default())); + tool(&mut s, "new_document", json!({})); + let lines: Vec = (0..2000).map(|i| format!("Line {i}")).collect(); + tool(&mut s, "type_text", json!({"text": lines.join("\r\n"), "paragraphs": true})); + let text = tool(&mut s, "get_text", json!({}))["text"].as_str().unwrap().to_string(); + assert!(text.contains("Line 0") && text.contains("Line 1999"), "{}", text.len()); + let paras = |s: &mut Server| tool(s, "inspect_document", json!({}))["blocks"].as_array().unwrap().len(); + assert_eq!(paras(&mut s), 2000); + tool(&mut s, "execute", json!({"command": "edit.undo"})); + assert_eq!(paras(&mut s), 1, "one Undo takes all the lines back out"); + assert!(!tool(&mut s, "get_text", json!({}))["text"].as_str().unwrap().contains("Line")); + + let calls = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); + let mut s = Server::new(Box::new(RefusesParagraph { calls: calls.clone() })); + let many = "x\n".repeat(crate::tools::MAX_TYPED_PARAGRAPHS); + let r = call(&mut s, 12, "tools/call", json!({"name": "type_text", "arguments": {"text": many, "paragraphs": true}})); + assert_eq!(r["result"]["isError"], true, "{r}"); + assert!(calls.lock().unwrap().is_empty(), "refused before any command"); } diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 47e162a0b..3a46629b2 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -102,7 +102,7 @@ pub fn tool_definitions() -> Value { tool( "type_text", "Type text", - "Type at the caret (replacing the selection). Use \\n... for line breaks inside text; use execute text.newParagraph for new paragraphs, or `paragraphs: true` to split lines into paragraphs.", + "Type at the caret (replacing the selection). Use \\n... for line breaks inside text; use execute text.newParagraph for new paragraphs, or `paragraphs: true` to split lines into paragraphs (one undo step, at most 10000 lines).", obj(json!({"text": s("Text"), "paragraphs": {"type": "boolean"}}), &["text"]), false ), @@ -154,6 +154,9 @@ pub fn tool_definitions() -> Value { ]) } +/// Most paragraphs `type_text` with `paragraphs: true` types in one call. +pub const MAX_TYPED_PARAGRAPHS: usize = 10_000; + fn exec(b: &mut dyn Backend, id: &str, params: Value) -> Result { b.call("engine.execute", json!({"command": id, "params": params})) } @@ -229,22 +232,17 @@ pub fn call_tool(b: &mut dyn Backend, name: &str, a: &Value) -> ToolResult { "type_text" => { let t = st("text").unwrap_or(""); if a.get("paragraphs").and_then(Value::as_bool).unwrap_or(false) { - let mut last = Ok(Value::Null); - for (i, line) in t.split('\n').enumerate() { - if i > 0 { - last = exec(b, "text.newParagraph", json!({})); - if last.is_err() { - break; - } - } - if !line.is_empty() { - last = exec(b, "text.insert", json!({"text": line, "raw": true})); - } - if last.is_err() { - break; - } + // One `text.insert`: `\r` is a paragraph break there (Enter's split), so the + // lines go in as one edit and one undo step, all or nothing, instead of a command + // (and a whole-document undo snapshot) per line. + let t = t.replace("\r\n", "\n").replace('\r', "\n"); + let lines = t.split(['\n', '\u{2029}']).count(); + if lines > MAX_TYPED_PARAGRAPHS { + return ToolResult::error(format!( + "`text` has {lines} lines; type at most {MAX_TYPED_PARAGRAPHS} paragraphs per call" + )); } - wrap(last) + wrap(exec(b, "text.insert", json!({"text": t.replace('\n', "\r"), "raw": true}))) } else { wrap(exec(b, "text.insert", json!({"text": t, "raw": true}))) } From 8ce3eb58c73b6da7b35b957e3cb17da7bb281c13 Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 15:20:45 +0100 Subject: [PATCH 03/16] #3735: automation writes check the directory entry they replace, not only the link's target Findings 41 and 69. check_write resolved a path through links and compared the target with the write root, but safe_save puts its temporary file beside the path given and renames it over that entry; rename_file removes it. A link outside the root pointing in passed the check while the write landed outside. Write access now also requires the entry itself (resolved parent + literal name) inside the root; the existing target check still refuses an inside link that points out. Tests: file_access::tests::links_are_followed_before_comparing (extended), mcp automation_roots::a_link_outside_the_write_root_into_it_is_not_written. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/engine/src/file_access.rs | 43 ++++++++++++++++++++++++++-- crates/mcp/tests/automation_roots.rs | 28 ++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/file_access.rs b/crates/engine/src/file_access.rs index 3d1e80880..8e6fdaf17 100644 --- a/crates/engine/src/file_access.rs +++ b/crates/engine/src/file_access.rs @@ -10,6 +10,9 @@ //! is a link that leads nowhere). //! - The result must lie inside the root, compared by whole components (`/work2` isn't inside //! `/work`), without regard to case on Windows. +//! - Writing replaces or removes a directory entry, so for write access the entry itself (its +//! folder's links followed, its own name as given) must lie inside the root as well: a link +//! outside the root that leads into it is not a way to write outside (#3735). //! - Read and write access are separate: a root given alone grants only its own kind of access. //! //! Roots are in force on a thread inside [`confine`], and in the whole process after @@ -95,7 +98,14 @@ impl AutomationRoots { Access::Write => Err(format!("automation filesystem access is not granted: write authority is absent: {path}")), }; }; - if inside(&resolve(path)?, folder) { + // Writing replaces or removes the directory entry `path` names (a safe save renames its + // temporary file over it, a rename removes it): that entry must be inside the root too, + // not only what a link there leads to (#3735). + let entry_inside = || match access { + Access::Read => Ok(true), + Access::Write => resolve_entry(path).map(|entry| inside(&entry, folder)), + }; + if inside(&resolve(path)?, folder) && entry_inside()? { return Ok(()); } let root = shown.display(); @@ -201,9 +211,27 @@ fn resolve(path: &str) -> Result { return Err(format!("automation path rejected: reserved device names are not allowed: {path}")); } let absolute = std::path::absolute(path).map_err(|e| format!("automation path rejected: cannot resolve {path}: {e}"))?; + resolve_absolute(path, &absolute) +} + +/// The directory entry `path` names: its folder resolved as [`resolve`] resolves it, then its own +/// name as given, so a link there is the entry itself, not what it leads to. That entry is what +/// replacing or removing the file changes. +fn resolve_entry(path: &str) -> Result { + let absolute = std::path::absolute(path).map_err(|e| format!("automation path rejected: cannot resolve {path}: {e}"))?; + let (Some(folder), Some(name)) = (absolute.parent(), absolute.file_name()) else { + return Err(format!("automation path rejected: not a file name: {path}")); + }; + let mut entry = resolve_absolute(path, folder)?; + entry.push(name); + Ok(entry) +} + +/// [`resolve`] for `absolute`, `path` made absolute (`path` as given, for messages). +fn resolve_absolute(path: &str, absolute: &Path) -> Result { // The names below the nearest existing folder, last first. let mut missing: Vec = vec![]; - let mut at = absolute.as_path(); + let mut at = absolute; loop { match std::fs::canonicalize(at) { Ok(mut found) => { @@ -382,6 +410,17 @@ mod tests { // A link to a folder inside the root stays inside. assert!(made(&base.join("in/sub"), &base.join("in/alias"))); assert_eq!(roots.check(&s(base.join("in/alias/x.docx")), Access::Read), Ok(())); + // A link outside the write root that leads into it: writing would replace the link, + // outside (#3735). One inside that leads to a file inside is fine. + std::fs::write(base.join("out/doc.docx"), "x").unwrap(); + assert!(made(&base.join("out/doc.docx"), &base.join("outside/alias.docx"))); + let e = roots.check(&s(base.join("outside/alias.docx")), Access::Write).unwrap_err(); + assert!(e.contains("outside the write root"), "the link itself is outside: {e}"); + assert!(made(&base.join("out/doc.docx"), &base.join("out/alias.docx"))); + assert_eq!(roots.check(&s(base.join("out/alias.docx")), Access::Write), Ok(())); + // A folder link outside that leads into the root: the entry is inside, as the system opens it. + assert!(made(&base.join("out"), &base.join("outside/to-out"))); + assert_eq!(roots.check(&s(base.join("outside/to-out/new.docx")), Access::Write), Ok(())); // A root given through a link is the folder it leads to. assert!(made(&base.join("in"), &base.join("in-link"))); let linked = AutomationRoots::new(Some(&base.join("in-link")), None).unwrap().unwrap(); diff --git a/crates/mcp/tests/automation_roots.rs b/crates/mcp/tests/automation_roots.rs index 415ab99d8..7b618ac49 100644 --- a/crates/mcp/tests/automation_roots.rs +++ b/crates/mcp/tests/automation_roots.rs @@ -208,3 +208,31 @@ fn links_out_of_a_root_are_refused() { refused(&mut server, "save_document", json!({"path": s(&base.join("out/away/doc.docx"))}), "outside the write root"); assert!(!base.join("outside/doc.docx").exists()); } + +/// The names in `dir`, sorted. +fn names(dir: &Path) -> Vec { + let mut names: Vec = std::fs::read_dir(dir).unwrap().map(|e| e.unwrap().file_name().to_string_lossy().into_owned()).collect(); + names.sort(); + names +} + +#[cfg(unix)] +#[test] +fn a_link_outside_the_write_root_into_it_is_not_written() { + // #3735: the link's target is inside the write root, but a save replaces the link itself, + // and its temporary file would sit beside it, outside. + let (mut server, base) = confined("link-in", true, true); + std::fs::write(base.join("out/doc.docx"), "inside").unwrap(); + std::os::unix::fs::symlink(base.join("out/doc.docx"), base.join("outside/alias.docx")).unwrap(); + let before = names(&base.join("outside")); + ok(&mut server, "type_text", json!({"text": "Hello"})); + refused(&mut server, "save_document", json!({"path": s(&base.join("outside/alias.docx"))}), "outside the write root"); + let (error, text) = run(&mut server, "file.exportPdf", json!({"path": s(&base.join("outside/alias.docx"))})); + assert!(error && text.contains("outside the write root"), "{text}"); + assert_eq!(names(&base.join("outside")), before, "no temporary file, nothing replaced"); + assert!(std::fs::symlink_metadata(base.join("outside/alias.docx")).unwrap().file_type().is_symlink()); + assert_eq!(std::fs::read_to_string(base.join("out/doc.docx")).unwrap(), "inside"); + // Saving to the real file inside still works. + ok(&mut server, "save_document", json!({"path": s(&base.join("out/doc.docx"))})); + assert_ne!(std::fs::read(base.join("out/doc.docx")).unwrap(), b"inside"); +} From 02fffea7c99c4bc528f18bd7fbf852bed02eff2a Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 15:24:14 +0100 Subject: [PATCH 04/16] #3736: HTML pictures load only from inside the automation read root Finding 54. Opening an HTML file checks the file through its links, but its pictures load relative to the folder the given path names. Through a link outside the read root to a page inside it, pictures beside the link (outside the root) were embedded. Each picture's canonical path is now checked with check_read before it is read; unconfined opens are unchanged. Test: tests::html_pictures_load_only_from_inside_the_read_root. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/engine/src/io.rs | 5 ++++- crates/engine/src/tests.rs | 23 +++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/io.rs b/crates/engine/src/io.rs index 27e702053..1e959c49a 100644 --- a/crates/engine/src/io.rs +++ b/crates/engine/src/io.rs @@ -506,7 +506,7 @@ impl LocalImages { left.map(|_| ()) } - /// The canonical path `src` names, if it's inside the folder. + /// The canonical path `src` names, if it's inside the folder and may be read. fn resolve(&self, src: &str) -> Option { let src = src.split(['#', '?']).next().unwrap_or(""); if src.is_empty() || src.starts_with("//") || src.split_once(':').is_some_and(|(scheme, _)| !scheme.contains('/')) { @@ -520,6 +520,9 @@ impl LocalImages { let dir = self.dir.as_ref()?; // Canonical paths have `..` and symlinks resolved, so this keeps both inside the folder. let path = std::fs::canonicalize(dir.join(rel)).ok()?; + // The folder is the one the HTML path names, which may be a link from outside the + // automation read root; the picture itself must be inside it (#3736). + crate::file_access::check_read(&path.to_string_lossy()).ok()?; path.starts_with(dir).then_some(path) } } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 44747c0ab..1a9cc4538 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -4112,6 +4112,29 @@ fn html_pictures_through_a_symlink_out_of_the_folder_are_not_loaded() { assert!(doc.plain_text(StoryRef::Body).contains("Link")); } +#[cfg(unix)] +#[test] +fn html_pictures_load_only_from_inside_the_read_root() { + // #3736: the HTML file is checked through its link, but its pictures load from beside the + // link. Opened through a link outside the read root, they must not come from there. + let root = scratch_dir("html-img-root"); + for d in ["in", "outside"] { + std::fs::create_dir_all(root.join(d)).unwrap(); + } + std::fs::write(root.join("in/page.html"), r#"

Pic

"#).unwrap(); + std::fs::write(root.join("in/pic.png"), tiny_png()).unwrap(); + std::fs::write(root.join("outside/pic.png"), tiny_png()).unwrap(); + std::os::unix::fs::symlink(root.join("in/page.html"), root.join("outside/page.html")).unwrap(); + let roots = crate::file_access::AutomationRoots::new(Some(&root.join("in")), None).unwrap(); + let through_link = crate::file_access::confine(roots.as_ref(), || crate::io::open_path(&root.join("outside/page.html"))); + let direct = crate::file_access::confine(roots.as_ref(), || crate::io::open_path(&root.join("in/page.html"))); + let _ = std::fs::remove_dir_all(&root); + let through_link = through_link.unwrap(); + assert!(through_link.media.is_empty(), "no picture from outside the read root"); + assert!(through_link.plain_text(StoryRef::Body).contains("Pic")); + assert_eq!(direct.unwrap().media.len(), 1, "pictures inside the root still load"); +} + /// `file.open` on a path reads only a regular file: a named pipe blocked until a writer came, and /// `/dev/zero` read until memory ran out. #[cfg(unix)] From 6142567b6a180cd15dd805bfd17f3981d4281a18 Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 15:29:05 +0100 Subject: [PATCH 05/16] #3737: dictionary file and folder settings held to the automation roots Finding 34 and #3737 (WC-35), same root cause: proofing paths bypassed file_access. persist_user_dictionary now checks the dictionary file for read and write (entry included, so its sibling temporary file too) and keeps the words in memory when refused. folder_setting checks dictionariesFolder and userTemplatesFolder with check_read before probing the folder, so a confined session can't install or save one outside the read root. Finding 34 is defensive today: only the desktop app sets user_dict_path, and it runs unconfined. Test: tests::confined_sessions_neither_save_the_dictionary_nor_choose_folders_outside_their_roots. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/engine/src/cmd/options.rs | 2 ++ crates/engine/src/session.rs | 10 +++++++--- crates/engine/src/tests.rs | 27 +++++++++++++++++++++++++++ 3 files changed, 36 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/cmd/options.rs b/crates/engine/src/cmd/options.rs index 74777db99..0c676b750 100644 --- a/crates/engine/src/cmd/options.rs +++ b/crates/engine/src/cmd/options.rs @@ -263,6 +263,8 @@ fn folder_setting(key: &str, f: &str, local_only: bool) -> Result if !path.is_absolute() { return Err(CmdError::Params(format!("`{key}`: {f:?} isn't a full path"))); } + // Its files are read from now on, and on every start: only from inside the read root (#3737). + crate::file_access::check_read(&path.to_string_lossy()).map_err(CmdError::Failed)?; if !path.is_dir() { return Err(CmdError::Failed(format!("no folder at {}", path.display()))); } diff --git a/crates/engine/src/session.rs b/crates/engine/src/session.rs index fb9999c74..fa2490988 100644 --- a/crates/engine/src/session.rs +++ b/crates/engine/src/session.rs @@ -663,10 +663,14 @@ impl Session { /// Save the custom dictionary through [`Session::user_dict_path`], if the host wired one up, /// with `removed` (words just taken out) left out of the merge with the file. A failed save - /// keeps the words in memory and never fails the command (#40). + /// keeps the words in memory and never fails the command (#40), as does a file outside the + /// automation roots in force: it is read, then replaced beside itself (#3737). pub(crate) fn persist_user_dictionary(&self, removed: &[String]) { - if let Some(path) = self.user_dict_path.as_deref() - && let Err(e) = wordcraft_proof::save_user_dictionary(path, removed) + let Some(path) = self.user_dict_path.as_deref() else { return }; + let name = path.to_string_lossy(); + if let Err(e) = crate::file_access::check_read(&name) + .and_then(|()| crate::file_access::check_write(&name)) + .and_then(|()| wordcraft_proof::save_user_dictionary(path, removed).map_err(|e| e.to_string())) { log::warn!("custom dictionary: save failed: {e}"); } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 1a9cc4538..4f1aabf0b 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -224,6 +224,33 @@ fn custom_dictionary_words_persist_through_their_file() { let _ = std::fs::remove_dir_all(&dir); } +#[test] +fn confined_sessions_neither_save_the_dictionary_nor_choose_folders_outside_their_roots() { + // #3737 (findings 34 and WC-35): the custom dictionary file and the folder settings are + // held to the automation roots like any other path. + let base = scratch_dir("dict-roots"); + for d in ["in", "out", "outside"] { + std::fs::create_dir_all(base.join(d)).unwrap(); + } + let roots = crate::file_access::AutomationRoots::new(Some(&base.join("in")), Some(&base.join("out"))).unwrap(); + let mut s = s(); + let outside = base.join("outside/custom-dictionary.txt"); + s.user_dict_path = Some(outside.clone()); + let confined = |s: &mut Session, cmd: &str, v: serde_json::Value| crate::file_access::confine(roots.as_ref(), || s.run(cmd, &v)); + assert!(confined(&mut s, "review.addToDictionary", json!({"word": "zxqengconfined"})).is_ok()); + assert!(wordcraft_proof::user_dictionary().contains(&"zxqengconfined".to_string()), "kept in memory"); + assert!(!outside.exists(), "nothing written outside the write root"); + for key in ["dictionariesFolder", "userTemplatesFolder"] { + assert!(confined(&mut s, "file.options", json!({key: base.join("outside").to_string_lossy()})).is_err(), "{key}"); + } + assert_eq!((s.prefs.dictionaries_folder.as_deref(), s.prefs.user_templates_folder.as_deref()), (None, None)); + // A folder inside the read root is still taken. + let inside = base.join("in"); + assert!(confined(&mut s, "file.options", json!({"userTemplatesFolder": inside.to_string_lossy()})).is_ok()); + let _ = std::fs::remove_dir_all(&base); + assert_eq!(s.prefs.user_templates_folder.as_deref(), Some(inside.to_string_lossy().as_ref())); +} + #[test] fn add_to_dictionary_takes_only_words_the_file_keeps() { // #40: the same rule as loading the dictionary file, so nothing added is dropped on the next load. From 66e92e36985e34d09c8279997a3556cff66c2a63 Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 15:32:43 +0100 Subject: [PATCH 06/16] #3692: formatting restriction refuses Copy Styles Finding 50. check_formatting blocked Update Styles and Attach Template with update, but not styles.copyFrom, which replaces style definitions (locked flag included) from another file. Under "limit formatting to a selection of styles" an editor could import changed formatting and unlock a locked style without the password. styles.copyFrom is now refused like Update Styles. copy_styles' callers are all blocked under the restriction, so its lock handling is left as is. Test: cmd::protect::tests::formatting_restriction_refuses_attach_template_update (extended). Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/engine/src/cmd/protect.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/cmd/protect.rs b/crates/engine/src/cmd/protect.rs index a1824a863..5beb67ead 100644 --- a/crates/engine/src/cmd/protect.rs +++ b/crates/engine/src/cmd/protect.rs @@ -406,6 +406,8 @@ fn check_formatting(s: &Session, id: &str, params: &Value) -> Result<(), CmdErro | "table.modifyStyle" | "table.deleteStyle" | "tools.updateStyles" + // Copies style definitions in from another file, locked flags included (#3692). + | "styles.copyFrom" ) // Attach Template updates the styles from it when asked (`linkStyles` is only a setting). || (id == "tools.attachTemplate" && p::bool(params, "update") == Some(true)); @@ -745,7 +747,8 @@ mod tests { } /// Formatting limited to styles: attaching a template refuses to update the styles from it - /// (it would bypass the Update Styles block); attaching without updating still works. + /// (it would bypass the Update Styles block), nor can Copy Styles; attaching without + /// updating still works. #[test] fn formatting_restriction_refuses_attach_template_update() { let mut t = Document::from_text("Template."); @@ -759,6 +762,12 @@ mod tests { let e = s.run("tools.attachTemplate", &json!({"path": "Report.dotx", "update": true, "data": data})).unwrap_err().to_string(); assert!(e.contains("formatting is limited"), "{e}"); assert_eq!(s.doc, before); + // Copying styles in replaces their definitions, locked flag included (#3692). + s.doc.styles.styles.iter_mut().filter(|st| st.id == "Heading1").for_each(|st| st.locked = true); + let before = s.doc.clone(); + let e = s.run("styles.copyFrom", &json!({"path": "Report.dotx", "styles": ["Heading 1"], "data": data})).unwrap_err().to_string(); + assert!(e.contains("formatting is limited"), "{e}"); + assert_eq!(s.doc, before); s.run("tools.attachTemplate", &json!({"path": "Report.dotx"})).unwrap(); assert_eq!(s.doc.settings.attached_template.as_deref(), Some("Report.dotx")); } From c7dfd92ff1722a6c329830ac71760b5599d2134b Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 15:34:26 +0100 Subject: [PATCH 07/16] #3698: template paths refuse the NT namespace and are checked again after joining the document's folder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 78 and WC-02. is_network_path looked only at the first two bytes, so `\??\UNC\host\share\T.dotx` (any separator mix) passed as local, and local_path checked the reference before joining it to the document's folder, so a relative reference from a document named `\\host\share\x` (file.open with data) became a remote path after the check. The classifier now also refuses `\??\…` unless a drive letter follows, and local_path checks the completed path. Pure string logic, the same on every platform; the same helper guards the folder settings. Test: cmd::templates::tests::template_references_become_local_paths (extended). Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/engine/src/cmd/templates.rs | 39 ++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/cmd/templates.rs b/crates/engine/src/cmd/templates.rs index 60d4044a0..c59f3e1e4 100644 --- a/crates/engine/src/cmd/templates.rs +++ b/crates/engine/src/cmd/templates.rs @@ -231,18 +231,27 @@ pub fn local_path(target: &str, base: Option<&Path>) -> Option { return None; } let p = PathBuf::from(&path); - if p.is_relative() - && !is_windows_absolute(&path) - && let Some(dir) = base.and_then(Path::parent) - { - return Some(dir.join(p)); - } - Some(p) + let p = match base.and_then(Path::parent) { + Some(dir) if p.is_relative() && !is_windows_absolute(&path) => dir.join(p), + _ => p, + }; + // Checked again once joined: the document's own folder may be on another machine (#3698). + (!is_network_path(&p.to_string_lossy())).then_some(p) } +/// Does `p` name another machine or a device, whatever separators it mixes: `\\host\share`, +/// `//host`, `\\?\UNC\…`, `\\.\…`, and the NT namespace `\??\…` unless a drive letter +/// follows (`\??\C:\…`, #3698). Purely lexical, so it is the same on every platform. pub(crate) fn is_network_path(p: &str) -> bool { + let sep = |c: Option<&u8>| matches!(c, Some(b'\\' | b'/')); let b = p.as_bytes(); - matches!((b.first(), b.get(1)), (Some(b'\\' | b'/'), Some(b'\\' | b'/'))) + if sep(b.first()) && sep(b.get(1)) { + return true; + } + // `\??\` then anything but `X:` (and the end or a separator). + let nt = sep(b.first()) && b.get(1) == Some(&b'?') && b.get(2) == Some(&b'?') && sep(b.get(3)); + let drive = b.get(4).is_some_and(u8::is_ascii_alphabetic) && b.get(5) == Some(&b':') && (b.len() == 6 || sep(b.get(6))); + nt && !drive } fn is_windows_absolute(p: &str) -> bool { @@ -504,9 +513,23 @@ mod tests { "file://evil.example/share/T.dotx", "file:////evil.example/share/T.dotx", "FILE://Evil.Example/T.dotx", + // The NT namespace (#3698), with any separators. + "\\??\\UNC\\evil.example\\share\\T.dotx", + "/??/UNC/evil.example/share/T.dotx", + "\\??/UNC\\evil.example/share/T.dotx", + "\\??\\GLOBALROOT\\Device\\Mup\\evil.example\\share\\T.dotx", + "\\\\.\\UNC\\evil.example\\share\\T.dotx", ] { assert_eq!(local_path(remote, Some(Path::new("/docs/Letter.docx"))), None, "{remote}"); + assert_eq!(local_path(remote, None), None, "{remote} without a folder"); } + // A relative reference from a document whose folder is on another machine (#3698). + assert_eq!(local_path("T.dotx", Some(Path::new("//evil.example/share/Letter.docx"))), None); + assert_eq!(local_path("file:T.dotx", Some(Path::new("//evil.example/share/Letter.docx"))), None); + // A drive in the NT namespace is local. + assert!(!is_network_path("\\??\\C:\\Templates\\T.dotx")); + assert!(!is_network_path("\\??\\C:")); + assert!(is_network_path("\\??\\C:evil")); assert_eq!(percent_decode("100%"), "100%"); assert_eq!(percent_decode("%zz%4"), "%zz%4"); } From 4f5e596c715ee2d827eed4e6078f5108fee60ebf Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 15:35:02 +0100 Subject: [PATCH 08/16] =?UTF-8?q?#3735:=20docs=20=E2=80=94=20automation=20?= =?UTF-8?q?roots,=20type=5Ftext=20limit,=20Copy=20Styles=20under=20protect?= =?UTF-8?q?ion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Progress log row for group J (findings 9, 18, 34, 41, 50, 54, 69, 78); docs/mcp.md says what the roots now cover; the Restrict Editing row notes Copy Styles. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/file-format-parity.md | 3 ++- docs/mcp.md | 12 ++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/docs/file-format-parity.md b/docs/file-format-parity.md index 076855fcf..cde6c8e80 100644 --- a/docs/file-format-parity.md +++ b/docs/file-format-parity.md @@ -93,7 +93,7 @@ Measured by grepping `crates/docx/src/read` and `src/write` for the OOXML elemen | Fields (`w:fldChar`, `w:fldSimple`), TOC, cross-references, `ADDIN` citations | ✅ | ✅ | Field codes Word supports but we don't evaluate keep their cached result. Textual `REF` fields with `\h` export internal PDF links without refreshing cached text; existing enclosing links take precedence. `wordcraft-cli convert --update-fields` explicitly runs field updates before saving; ordinary conversion preserves caches and locked fields stay unchanged. Instruction-only paragraphs carry hidden marks instead of blank lines; an empty result paragraph keeps its line. Imported TOCs preserve actual result boundaries, formatted entries and nested fields without relying on TOC styles. `EQ \o` (overstrike, enclosed characters) is drawn; enclosed characters are written as Word stores them, their style in the code's run sizes (#297); a cached picture (`INCLUDEPICTURE`) stays as the content of a range field (#465). A `HYPERLINK` field reads as a link, its result ordinary text (tabs, nested `PAGEREF`), and is written back as `w:hyperlink`; its tooltip (`\o`) and target frame (`\t`) are dropped (#2174) | | Move tracking (`w:moveFrom`/`w:moveTo`, `w:moveFromRangeStart/End`, `w:moveToRangeStart/End`), moved paragraph marks | ✅ | ✅ | #400: both ends linked by the range name; ranges rewritten around each moved stretch (ids renumbered); a move missing an end or a range end opens as a plain deletion/insertion. ODT/PDF export the result (moved-away text hidden like deletions); RTF writes a move as a plain tracked deletion and insertion | | Content controls (`w:sdt`) | ✅ | ✅ | Block, inline, row and cell level, nested; title, tag, id, lock, placeholder, temporary, rich/plain text, `w14:checkbox`, combo box, drop-down list, date, picture, building block gallery, `w15` repeating sections; other `w:sdtPr` children (data binding, appearance) kept verbatim (#381). A TOC's control is rebuilt by the TOC when the TOC has the heading-and-entries form; around any other TOC it is kept as read (#2427). Replace All respects content-edit and control-deletion locks and refuses a batch before making any edit | -| Restrict Editing (`w:documentProtection` edit/formatting/enforcement and password hash, `w:locked` styles, `w:permStart`/`w:permEnd` exceptions) | ✅ | ✅ | #412: SHA-512 hash with salt and spin count written; Word's legacy `w:cryptAlgorithmSid`/`w:hash` form (SHA-1/-256/-384/-512, ECMA-376 Part 4 §14.8.1) is kept and verified; MD2/MD4/MD5, `w:algIdExt` and other unsupported forms are refused, never unlocked; `w:colFirst`/`w:colLast` table-column ranges read as plain ranges; forms mode (`w:edit="forms"`) lets edits inside fillable content controls (`w:lock` content-editable, innermost wins) run, everything else rolls back; row-wrapped controls and control deletion remain refused; `review.restrict` reports `formFields`; Find Next Region covers form fields (the pane button shows for forms documents); under active protection, `developer.properties` refuses `lockEdit`/`lockDelete` changes, as Word blocks property edits until Stop Protection; under forms protection it refuses every property, and inserting or removing content controls is refused too (Word greys out the Developer tab's control buttons and Properties and offers no Remove Content Control), while under the other modes the title, tag and placeholder stay editable; a file command that changes the open document is checked like any other, so the privacy options, the properties and the inspector's removals are refused on a protected document, and restoring a stored version keeps the protection and its password rather than the snapshot's stale ones; the protection of a protected document only changes through the explicit stop (`review.restrict {"stop": true}` — the "none" mode is the shorthand for it and doesn't stop a protected document), the session author used for new tracked changes and comments can't change through `file.setAuthor` while the document is protected (the CLI's `--author` uses the same protection check), and the post-check compares the document's settings, metadata, custom properties, list definitions, bibliography sources and the comments' resolved flags as well as its text and formatting; named exceptions match a self-declared user name, not an authenticated identity | +| Restrict Editing (`w:documentProtection` edit/formatting/enforcement and password hash, `w:locked` styles, `w:permStart`/`w:permEnd` exceptions) | ✅ | ✅ | #412: SHA-512 hash with salt and spin count written; Word's legacy `w:cryptAlgorithmSid`/`w:hash` form (SHA-1/-256/-384/-512, ECMA-376 Part 4 §14.8.1) is kept and verified; MD2/MD4/MD5, `w:algIdExt` and other unsupported forms are refused, never unlocked; `w:colFirst`/`w:colLast` table-column ranges read as plain ranges; forms mode (`w:edit="forms"`) lets edits inside fillable content controls (`w:lock` content-editable, innermost wins) run, everything else rolls back; row-wrapped controls and control deletion remain refused; `review.restrict` reports `formFields`; Find Next Region covers form fields (the pane button shows for forms documents); under active protection, `developer.properties` refuses `lockEdit`/`lockDelete` changes, as Word blocks property edits until Stop Protection; under forms protection it refuses every property, and inserting or removing content controls is refused too (Word greys out the Developer tab's control buttons and Properties and offers no Remove Content Control), while under the other modes the title, tag and placeholder stay editable; a file command that changes the open document is checked like any other, so the privacy options, the properties and the inspector's removals are refused on a protected document, and restoring a stored version keeps the protection and its password rather than the snapshot's stale ones; the protection of a protected document only changes through the explicit stop (`review.restrict {"stop": true}` — the "none" mode is the shorthand for it and doesn't stop a protected document), the session author used for new tracked changes and comments can't change through `file.setAuthor` while the document is protected (the CLI's `--author` uses the same protection check), and the post-check compares the document's settings, metadata, custom properties, list definitions, bibliography sources and the comments' resolved flags as well as its text and formatting; named exceptions match a self-declared user name, not an authenticated identity; formatting-only protection also refuses Copy Styles (`styles.copyFrom`, #3692) | | Legacy form fields (`w:ffData`), check boxes, drop-downs | 🟡 | ✅ | The field definition is kept raw and saved back. Check boxes (`FORMCHECKBOX`) are drawn as Word draws them: a square as tall as the text's line, inset 1.16 pt, 0.72 pt border, an X when checked (`w:checked`, else `w:default`), `w:size` honoured; drop-downs (`FORMDROPDOWN`) show their selected entry; text fields (`FORMTEXT`) show their result. Not yet: filling them in, Protect Document's forms mode for legacy fields | | DrawingML pictures inline and anchored (`wp:anchor`), wrap square/tight/through/top-bottom | ✅ | ✅ | Tight/through read and written with their `wp:wrapPolygon` (and `edited`); without one, a shape's outline is written (a picture's rectangle). `wrapText` (both sides, left, right, largest) read and written for square, tight and through | | Legacy form fields (`w:ffData`), check boxes, drop-downs | ❌ | ❌ | | @@ -252,6 +252,7 @@ Imported text fields use the first nonempty cached result run's formatting on th | Date | Change | Summary | |---|---|---| +| 2026-10-22 | trivial | Formatting-only protection refuses Copy Styles, as it does Update Styles (#3692) | | 2026-10-23 | trivial | A relationship the markup names without the prefix is read as it wrote: an unqualified `id` on an element that carries a relationship names that relationship, and the writer keeps the attribute with the id the new package holds the part under; an ordinary `id` — a VML shape's, a drawing's — names no part (#3411 follow-up) | | 2026-10-21 | trivial | The reader takes the relationship ids the markup refers to from the relationship attributes of the main document it has already parsed, so a document with many relationships opens in the time its part takes, and an id that only appears as text is not a reference (#3411) | | 2026-10-21 | trivial | HTML export writes ruby (furigana, phonetic guides) as `basereading`, in the base text's run formatting, and HTML import reads ``/``/`` back into one ruby object: `` fallback parentheses and the tags are not text, an unclosed element ends at the next block, and malformed ruby keeps its text. HTML states no alignment, size or raise, so the Phonetic Guide's defaults apply (#1681) | diff --git a/docs/mcp.md b/docs/mcp.md index 5ed66634d..99bf754d5 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -63,11 +63,15 @@ config works across the suite) keep the agent to the folders you give it: `file.newFromTemplate`, `insert.textFromFile` and `insert.object`, `insert.picture` and `picture.change` with a `path`, `mailings.recipients` with a `path` (csv, tsv, txt, xlsx, xlsm, ods), `review.compare` (`path` and `original`), the templates and documents `tools.updateStyles`, - `styles.organizer` and `styles.copyFrom` read. The pictures an opened HTML file names are read - from beside that file, so they are inside the read root too (a link that leads out is not followed). + `styles.organizer` and `styles.copyFrom` read, and the folders `file.options` takes + (`dictionariesFolder`, `userTemplatesFolder`). The pictures an opened HTML file names are read + from beside that file and must be inside the read root too, even when the file was opened through + a link (#3736). - Every file a command writes must lie inside the **write root**: `save_document`, `file.save`, `file.saveAs`, `file.rename` (the file and its new name), `file.exportPdf`, `file.exportPng`, `file.printPdf`, - `file.saveTemplate`, and `mailings.finish` with a `path`. + `file.saveTemplate`, and `mailings.finish` with a `path`. A save replaces the name it is given, + so that name must be inside the write root as well as what it leads to: a link outside the root + pointing in is refused (#3735). - Read and write access are separate. Give the same folder twice for both; a root left out grants none of its access (with only `--automation-read-root`, nothing can be written to disk). Without either flag nothing changes. @@ -108,7 +112,7 @@ files it was given rather than guarding against other software on the computer. | `batch` | run several commands in order | | `new_document` | blank, sample, letter, resume, report | | `open_document` / `save_document` | docx, doc (open only), odt, rtf, html, md, tex, txt, json; save also pdf, png | -| `type_text` | type at the caret (`paragraphs: true` splits lines into paragraphs) | +| `type_text` | type at the caret (`paragraphs: true` splits lines into paragraphs: one undo step, at most 10,000 lines) | | `select_text` | select the n-th occurrence of some text | | `get_text` / `inspect_document` | read the document back (verify without screenshots) | | `render_page` | a page as PNG | From 8a18026a13acee7514e151ed496d38d218a89682 Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 15:57:23 +0100 Subject: [PATCH 09/16] #3692: rustfmt type_text Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/mcp/src/tools.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 3a46629b2..c666fbe3f 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -238,9 +238,7 @@ pub fn call_tool(b: &mut dyn Backend, name: &str, a: &Value) -> ToolResult { let t = t.replace("\r\n", "\n").replace('\r', "\n"); let lines = t.split(['\n', '\u{2029}']).count(); if lines > MAX_TYPED_PARAGRAPHS { - return ToolResult::error(format!( - "`text` has {lines} lines; type at most {MAX_TYPED_PARAGRAPHS} paragraphs per call" - )); + return ToolResult::error(format!("`text` has {lines} lines; type at most {MAX_TYPED_PARAGRAPHS} paragraphs per call")); } wrap(exec(b, "text.insert", json!({"text": t.replace('\n', "\r"), "raw": true}))) } else { From 3a8ff6b6bc071a6e0dc85ae7bc0884d612efa8af Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 22:21:35 +0100 Subject: [PATCH 10/16] #3692: typed paragraphs break like Enter, still one command and one undo step Ultrareview finding 2. MCP `type_text` with `paragraphs: true` sent one raw text.insert with `\r` breaks, which skipped Enter: a break could land in a one-line, check-box or drop-down content control (text.newParagraph refuses there), and Enter's AutoFormat (--- borders, +---+ tables) no longer ran. text.insert takes `paragraphs: true`: each line is typed raw and each break runs text.newParagraph's own implementation (`enter`), inside the one command, so there is one checkpoint. Where Enter would refuse, the command fails and Session::run puts the document and history back (all or nothing). Enter's AutoFormat normally records the plain Enter as its own undo step after the fact (push_undo, a whole-document clone); inside typed paragraphs that step is skipped, so AutoFormatted lines add no snapshot each. MCP sends `paragraphs: true` and keeps MAX_TYPED_PARAGRAPHS. Tests: mcp type_text_paragraphs_are_refused_in_a_one_line_control, type_text_paragraphs_autoformat_like_enter_in_one_undo_step. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/engine/src/cmd/text.rs | 60 ++++++++++++++++++++++++++++------- crates/mcp/src/tests.rs | 52 +++++++++++++++++++++++++++--- crates/mcp/src/tools.rs | 9 +++--- 3 files changed, 102 insertions(+), 19 deletions(-) diff --git a/crates/engine/src/cmd/text.rs b/crates/engine/src/cmd/text.rs index d83d16425..614086256 100644 --- a/crates/engine/src/cmd/text.rs +++ b/crates/engine/src/cmd/text.rs @@ -10,7 +10,7 @@ use crate::{CmdError, CmdResult, CommandSpec, Selection, Session, p}; pub fn specs() -> Vec { vec![ - CommandSpec::new("text.insert", "Type Text", "Editing", insert).params(r#"{"text": string}"#), + CommandSpec::new("text.insert", "Type Text", "Editing", insert).params(r#"{"text": string, "raw"?: bool, "paragraphs"?: bool}"#), CommandSpec::new("text.newParagraph", "New Paragraph", "Editing", new_paragraph).key("Enter"), CommandSpec::new("text.lineBreak", "Line Break", "Editing", |s, _| ins_char(s, '\n')).key("Shift+Enter"), CommandSpec::new("text.pageBreak", "Page Break", "Insert › Pages", |s, _| ins_char(s, PAGE_BREAK)).key("Mod+Enter"), @@ -99,6 +99,9 @@ fn typing_options(s: &mut Session, text: &str) { fn insert(s: &mut Session, v: &Value) -> CmdResult { let text = p::req_str(v, "text")?; let raw = p::bool(v, "raw").unwrap_or(false); + if p::bool(v, "paragraphs").unwrap_or(false) { + return insert_paragraphs(s, text); + } typing_options(s, text); let t = if raw { None } else { autoformat(s, text) }; type_text(s, t.as_deref().unwrap_or(text))?; @@ -576,30 +579,65 @@ fn new_paragraph(s: &mut Session, _: &Value) -> CmdResult { if super::controls::refuses_paragraph(s) { return sel_result(s); } + enter(s, true)?; + sel_result(s) +} + +/// `text.insert` with `paragraphs`: each line typed as it is (`raw`), and between lines the +/// paragraph break Enter makes ([`enter`]: its AutoFormat and AutoCorrect), all in one command, so +/// one undo step instead of a command and a whole-document snapshot per line (#3692). Where +/// Enter would do nothing (a one-line control, a check box, a list) the text is refused and, +/// like any failed command, nothing of it stays. +fn insert_paragraphs(s: &mut Session, text: &str) -> CmdResult { + typing_options(s, text); + let text = text.replace("\r\n", "\n"); + for (i, line) in text.split(['\n', '\r', '\u{2029}']).enumerate() { + if i > 0 { + if super::controls::refuses_paragraph(s) { + return Err(CmdError::Disabled("text.insert: this content control takes no paragraph breaks".into())); + } + enter(s, false)?; + } + if !line.is_empty() { + type_text(s, line)?; + } + } + sel_result(s) +} + +/// Enter's paragraph break once a control has allowed it: `=rand()` sample text, AutoFormat of a +/// border or table line, AutoCorrect of the word it ends, else a plain split. With `undo_step` +/// an AutoFormat is recorded as its own undo step after the plain Enter, as in Word; without it +/// (typed paragraphs, one undo step for all) no extra snapshot is taken. +fn enter(s: &mut Session, undo_step: bool) -> Result<(), CmdError> { super::controls::prepare_edit(s)?; if let Some(text) = sample_text(s) { let end = s.sel.focus.clone(); s.sel = Selection { anchor: Pos { off: 0, ..end.clone() }, focus: end }; - type_text(s, &text)?; - return sel_result(s); + return type_text(s, &text); } let mut at = delete_selection(s)?; let auto = border_line(s, &at).map(EnterAutoFormat::Border).or_else(|| table_line(s, &at).map(EnterAutoFormat::Table)); if let Some(auto) = auto { // Word: Undo right after reverts only the AutoFormat, so the typed characters come back // and the Enter stays. Record that plain Enter as its own undo step. - let (doc, sel) = (s.doc.clone(), s.sel.clone()); - let new = split_para(s, &at)?; - s.sel = Selection::caret(new); - let plain_doc = std::mem::replace(&mut s.doc, doc); - let plain_sel = std::mem::replace(&mut s.sel, sel); + let plain = if undo_step { + let (doc, sel) = (s.doc.clone(), s.sel.clone()); + let new = split_para(s, &at)?; + s.sel = Selection::caret(new); + Some((std::mem::replace(&mut s.doc, doc), std::mem::replace(&mut s.sel, sel))) + } else { + None + }; match auto { EnterAutoFormat::Border(line) => apply_border_line(s, &at, line)?, EnterAutoFormat::Table(cols) => apply_table_line(s, &at, &cols)?, } s.goal_x = None; - s.push_undo("AutoFormat", plain_doc, plain_sel); - return sel_result(s); + if let Some((plain_doc, plain_sel)) = plain { + s.push_undo("AutoFormat", plain_doc, plain_sel); + } + return Ok(()); } // Enter finishes a word like a space does ("teh" → "the", sentence capitals, links). if s.sel.is_collapsed() && at.off > 0 { @@ -610,7 +648,7 @@ fn new_paragraph(s: &mut Session, _: &Value) -> CmdResult { let new = split_para(s, &at)?; s.sel = Selection::caret(new); s.goal_x = None; - sel_result(s) + Ok(()) } /// In a table: next cell (adds a row at the end). At the start of a list item: demote. Else a tab. diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index e00037134..de99ddd57 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -120,7 +120,7 @@ fn headless_sessions_keep_the_dictionary_in_memory() { assert!(h.session.user_dict_path.is_none()); } -/// A backend that keeps every command it is sent and refuses text with a paragraph break. +/// A backend that keeps every command it is sent and refuses typed paragraphs. struct RefusesParagraph { calls: std::sync::Arc>>, } @@ -128,8 +128,8 @@ struct RefusesParagraph { impl crate::backend::Backend for RefusesParagraph { fn call(&mut self, _method: &str, params: Value) -> Result { self.calls.lock().unwrap().push(params.clone()); - match params["params"]["text"].as_str() { - Some(t) if t.contains('\r') => Err("paragraph break refused".into()), + match params["params"]["paragraphs"].as_bool() { + Some(true) => Err("paragraph break refused".into()), _ => Ok(json!({"ok": true})), } } @@ -153,7 +153,8 @@ fn type_text_stops_at_a_refused_paragraph_break() { let calls = calls.lock().unwrap(); assert_eq!(calls.len(), 1, "{calls:?}"); assert_eq!(calls[0]["command"], "text.insert"); - assert_eq!(calls[0]["params"]["text"], "one\rtwo"); + assert_eq!(calls[0]["params"]["text"], "one\ntwo"); + assert_eq!(calls[0]["params"]["paragraphs"], true); } /// #3692: typed paragraphs are one edit and one undo step, not a command (and a whole-document @@ -179,3 +180,46 @@ fn type_text_paragraphs_are_one_undo_step_with_a_line_cap() { assert_eq!(r["result"]["isError"], true, "{r}"); assert!(calls.lock().unwrap().is_empty(), "refused before any command"); } + +/// Ultrareview of #3692: each typed paragraph break is Enter's own (text.newParagraph's +/// implementation), so a one-line content control refuses it, and all of the text with it. +#[test] +fn type_text_paragraphs_are_refused_in_a_one_line_control() { + let mut s = Server::new(Box::new(Headless::default())); + tool(&mut s, "new_document", json!({})); + tool(&mut s, "type_text", json!({"text": "Name: "})); + tool(&mut s, "execute", json!({"command": "developer.plainText", "params": {"tag": "name"}})); + tool(&mut s, "execute", json!({"command": "caret.right"})); + let before = tool(&mut s, "get_text", json!({}))["text"].clone(); + let r = call(&mut s, 13, "tools/call", json!({"name": "type_text", "arguments": {"text": "Ada\nLovelace", "paragraphs": true}})); + assert_eq!(r["result"]["isError"], true, "{r}"); + assert_eq!(tool(&mut s, "get_text", json!({}))["text"], before, "nothing typed"); + assert_eq!(tool(&mut s, "inspect_document", json!({}))["blocks"].as_array().unwrap().len(), 1); +} + +/// Ultrareview of #3692: Enter's AutoFormat runs on each typed paragraph break ("---" becomes a +/// border, "+---+---+" a table), and the lines are still one undo step however many of them +/// AutoFormat changes. +#[test] +fn type_text_paragraphs_autoformat_like_enter_in_one_undo_step() { + let mut s = Server::new(Box::new(Headless::default())); + tool(&mut s, "new_document", json!({})); + tool(&mut s, "type_text", json!({"text": "Above\n---\nBelow\n+---+---+\nCell", "paragraphs": true})); + let blocks = tool(&mut s, "inspect_document", json!({}))["blocks"].clone(); + let blocks = blocks.as_array().unwrap(); + assert_eq!(blocks[0]["text"], "Above", "{blocks:?}"); + assert!(blocks[0]["props"]["borders"]["bottom"].is_object(), "--- is a border: {blocks:?}"); + assert_eq!(blocks[1]["text"], "Below", "{blocks:?}"); + assert_eq!(blocks[2]["type"], "table", "+---+---+ is a table: {blocks:?}"); + assert!(blocks[2].to_string().contains("Cell"), "typing goes on in its first cell: {blocks:?}"); + tool(&mut s, "execute", json!({"command": "edit.undo"})); + assert_eq!(tool(&mut s, "inspect_document", json!({}))["blocks"].as_array().unwrap().len(), 1, "one Undo"); + assert_eq!(tool(&mut s, "get_text", json!({}))["text"].as_str().unwrap().trim(), ""); + + // Many AutoFormatted lines: still one undo step (no snapshot per line). + let lines: Vec<&str> = (0..500).flat_map(|_| ["Text", "==="]).collect(); + tool(&mut s, "type_text", json!({"text": lines.join("\n"), "paragraphs": true})); + assert_eq!(tool(&mut s, "inspect_document", json!({}))["blocks"].as_array().unwrap().len(), 501); + tool(&mut s, "execute", json!({"command": "edit.undo"})); + assert_eq!(tool(&mut s, "inspect_document", json!({}))["blocks"].as_array().unwrap().len(), 1, "one Undo"); +} diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index c666fbe3f..d706d49e9 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -232,15 +232,16 @@ pub fn call_tool(b: &mut dyn Backend, name: &str, a: &Value) -> ToolResult { "type_text" => { let t = st("text").unwrap_or(""); if a.get("paragraphs").and_then(Value::as_bool).unwrap_or(false) { - // One `text.insert`: `\r` is a paragraph break there (Enter's split), so the - // lines go in as one edit and one undo step, all or nothing, instead of a command - // (and a whole-document undo snapshot) per line. + // One `text.insert` with `paragraphs`: each break is Enter's own (its AutoFormat, + // and its refusal in a one-line control), but the lines go in as one edit and one + // undo step, all or nothing, instead of a command (and a whole-document undo + // snapshot) per line. let t = t.replace("\r\n", "\n").replace('\r', "\n"); let lines = t.split(['\n', '\u{2029}']).count(); if lines > MAX_TYPED_PARAGRAPHS { return ToolResult::error(format!("`text` has {lines} lines; type at most {MAX_TYPED_PARAGRAPHS} paragraphs per call")); } - wrap(exec(b, "text.insert", json!({"text": t.replace('\n', "\r"), "raw": true}))) + wrap(exec(b, "text.insert", json!({"text": t, "raw": true, "paragraphs": true}))) } else { wrap(exec(b, "text.insert", json!({"text": t, "raw": true}))) } From 6b23f3b368d5b6e0b4e16e905217597ab6200a46 Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 22:21:35 +0100 Subject: [PATCH 11/16] #3698: template bytes need no local path; verbatim and device paths with a drive are local MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ultrareview finding 4. (a) load_other resolved the template name against the document's folder even when its bytes came in `data`, so a document opened from a share or a `\\?\` path could not attach, update or organise styles from a relative name. With bytes nothing is read: the name only gives the format. Without bytes local_path still refuses a relative reference from a document on a share (no NTLM leak). (b) is_network_path called `\\?\C:\…` and `\\.\C:\…` network; a drive letter after the verbatim, device or NT prefix is local. `\\?\UNC\…`, `\\.\UNC\…`, other devices and `\\host\…` stay refused. Tests: cmd::templates::tests::template_bytes_need_no_local_path (new), template_references_become_local_paths (extended). Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/engine/src/cmd/templates.rs | 69 ++++++++++++++++++++++++------ 1 file changed, 57 insertions(+), 12 deletions(-) diff --git a/crates/engine/src/cmd/templates.rs b/crates/engine/src/cmd/templates.rs index c59f3e1e4..110427879 100644 --- a/crates/engine/src/cmd/templates.rs +++ b/crates/engine/src/cmd/templates.rs @@ -185,17 +185,22 @@ fn load_other(name: &str, data: Option<&str>, base: Option<&Path>) -> Result MAX_PATH { return Err(CmdError::Params("`path` is too long".into())); } - let local = local_path(name, base).ok_or_else(|| CmdError::Failed(format!("`{name}` is not a local file")))?; - let ext = local.extension().map(|e| e.to_string_lossy().to_ascii_lowercase()).unwrap_or_default(); + // With the bytes given nothing is read, so the name only gives the format: no path is + // resolved, and a document on a share (or a `\\?\` path) can still use a relative name. + let file = match data { + Some(_) => PathBuf::from(name), + None => local_path(name, base).ok_or_else(|| CmdError::Failed(format!("`{name}` is not a local file")))?, + }; + let ext = file.extension().map(|e| e.to_string_lossy().to_ascii_lowercase()).unwrap_or_default(); if matches!(ext.as_str(), "" | "txt" | "text") { return Err(CmdError::Params(format!("`{name}` is not a document or template with styles"))); } match data { Some(d) => { let bytes = super::insert::base64_decode(d).ok_or_else(|| CmdError::Params("bad base64".into()))?; - crate::io::open_bytes(&local.to_string_lossy(), &bytes).map_err(CmdError::Failed) + crate::io::open_bytes(name, &bytes).map_err(CmdError::Failed) } - None => crate::io::open_path(&local).map_err(CmdError::Failed), + None => crate::io::open_path(&file).map_err(CmdError::Failed), } } @@ -240,18 +245,25 @@ pub fn local_path(target: &str, base: Option<&Path>) -> Option { } /// Does `p` name another machine or a device, whatever separators it mixes: `\\host\share`, -/// `//host`, `\\?\UNC\…`, `\\.\…`, and the NT namespace `\??\…` unless a drive letter -/// follows (`\??\C:\…`, #3698). Purely lexical, so it is the same on every platform. +/// `//host`, `\\?\UNC\…`, `\\.\UNC\…`, other devices, and the NT namespace `\??\…`. The +/// verbatim, device and NT prefixes are local when a drive letter follows (`\\?\C:\…`, +/// `\\.\C:\…`, `\??\C:\…`, #3698). Purely lexical, so it is the same on every platform. pub(crate) fn is_network_path(p: &str) -> bool { let sep = |c: Option<&u8>| matches!(c, Some(b'\\' | b'/')); let b = p.as_bytes(); - if sep(b.first()) && sep(b.get(1)) { - return true; - } - // `\??\` then anything but `X:` (and the end or a separator). - let nt = sep(b.first()) && b.get(1) == Some(&b'?') && b.get(2) == Some(&b'?') && sep(b.get(3)); + // `\\?\`, `\\.\` or `\??\`, then `X:` and the end or a separator. + let prefix = sep(b.first()) + && sep(b.get(3)) + && match (b.get(1), b.get(2)) { + (Some(b'?'), Some(b'?')) => true, + (c, Some(b'?' | b'.')) => sep(c), + _ => false, + }; let drive = b.get(4).is_some_and(u8::is_ascii_alphabetic) && b.get(5) == Some(&b':') && (b.len() == 6 || sep(b.get(6))); - nt && !drive + if prefix { + return !drive; + } + sep(b.first()) && sep(b.get(1)) } fn is_windows_absolute(p: &str) -> bool { @@ -498,6 +510,29 @@ mod tests { let _ = std::fs::remove_dir_all(path.parent().unwrap()); } + /// Ultrareview of #3698: with the template's bytes given (`data`) nothing is read from a path, + /// so a document opened from a share or a `\\?\` path can still attach, update and organise + /// styles from a template named relative to it. Without the bytes, the share is never read. + #[test] + fn template_bytes_need_no_local_path() { + let data = crate::cmd::insert::base64_encode(&wordcraft_docx::write(&template()).unwrap()); + for doc_path in ["//fileserver/share/Letter.docx", "//?/C:/docs/Letter.docx"] { + let mut s = Session::new(Document::new()); + s.path = Some(PathBuf::from(doc_path)); + s.run("tools.attachTemplate", &json!({"path": "Report.dotx", "data": data, "update": true})).unwrap(); + assert_eq!(s.doc.styles.get("Heading1").unwrap().chr.size, Some(31.0), "{doc_path}"); + s.run("tools.updateStyles", &json!({"path": "Report.dotx", "data": data})).unwrap(); + let listed = s.run("styles.organizer", &json!({"path": "Report.dotx", "data": data})).unwrap(); + assert!(listed["file"]["styles"].as_array().unwrap().iter().any(|x| x["name"] == "Callout"), "{doc_path}"); + s.run("styles.copyFrom", &json!({"path": "Report.dotx", "data": data, "styles": ["Callout"]})).unwrap(); + assert!(s.doc.styles.get("Callout").is_some(), "{doc_path}"); + } + let mut s = Session::new(Document::new()); + s.path = Some(PathBuf::from("//fileserver/share/Letter.docx")); + let err = s.run("tools.updateStyles", &json!({"path": "Report.dotx"})).unwrap_err().to_string(); + assert!(err.contains("not a local file"), "{err}"); + } + #[test] fn template_references_become_local_paths() { assert_eq!(local_path("file:///C:/Users/A%20B/T.dotx", None), Some(PathBuf::from("C:/Users/A B/T.dotx"))); @@ -530,6 +565,16 @@ mod tests { assert!(!is_network_path("\\??\\C:\\Templates\\T.dotx")); assert!(!is_network_path("\\??\\C:")); assert!(is_network_path("\\??\\C:evil")); + // Verbatim and device paths with a drive are local; with `UNC` or anything else they aren't. + for local in ["\\\\?\\C:\\Templates\\T.dotx", "\\\\.\\C:\\Templates\\T.dotx", "//?/C:/Templates/T.dotx", "\\\\?\\C:"] { + assert!(!is_network_path(local), "{local}"); + } + for remote in + ["\\\\?\\UNC\\host\\share\\T.dotx", "//./UNC/host/share/T.dotx", "\\\\host\\share\\T.dotx", "\\\\?\\C:evil", "\\\\.\\PhysicalDrive0"] + { + assert!(is_network_path(remote), "{remote}"); + } + assert_eq!(local_path("T.dotx", Some(Path::new("//?/C:/docs/Letter.docx"))), Some(PathBuf::from("//?/C:/docs/T.dotx"))); assert_eq!(percent_decode("100%"), "100%"); assert_eq!(percent_decode("%zz%4"), "%zz%4"); } From 6766118d05cc4d987833929f168a96500573b381 Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Fri, 23 Oct 2026 04:27:23 +0100 Subject: [PATCH 12/16] =?UTF-8?q?#3692:=20review=20=E2=80=94=20doc=20comme?= =?UTF-8?q?nts=20on=20the=20functions=20this=20branch=20touched=20that=20h?= =?UTF-8?q?ad=20none?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01CX1hMKX28764Lrx5moxs1o --- crates/engine/src/cmd/text.rs | 4 ++++ crates/engine/src/session.rs | 1 + crates/mcp/src/backend.rs | 3 +++ crates/mcp/src/tools.rs | 3 +++ 4 files changed, 11 insertions(+) diff --git a/crates/engine/src/cmd/text.rs b/crates/engine/src/cmd/text.rs index 614086256..5ce435e27 100644 --- a/crates/engine/src/cmd/text.rs +++ b/crates/engine/src/cmd/text.rs @@ -8,6 +8,7 @@ use wordcraft_doc::{Block, ListKind, Path, Pos, StoryRef, Table}; use super::{delete_selection, non_mac, sel_result, split_para, type_text}; use crate::{CmdError, CmdResult, CommandSpec, Selection, Session, p}; +/// The text-entry commands: typing, Enter, breaks, Tab, deleting and typed symbols. pub fn specs() -> Vec { vec![ CommandSpec::new("text.insert", "Type Text", "Editing", insert).params(r#"{"text": string, "raw"?: bool, "paragraphs"?: bool}"#), @@ -96,6 +97,8 @@ fn typing_options(s: &mut Session, text: &str) { } } +/// `text.insert`: type `text` at the selection as the keyboard does (AutoCorrect and +/// AutoFormat unless `raw`), or, with `paragraphs`, line by line with Enter between. fn insert(s: &mut Session, v: &Value) -> CmdResult { let text = p::req_str(v, "text")?; let raw = p::bool(v, "raw").unwrap_or(false); @@ -574,6 +577,7 @@ fn sample_text(s: &Session) -> Option { Some(text) } +/// `text.newParagraph`: Enter, unless the selection is in a control that takes no paragraph break. fn new_paragraph(s: &mut Session, _: &Value) -> CmdResult { // Enter in a one-line plain text control (or a check box or list) does nothing, as in Word. if super::controls::refuses_paragraph(s) { diff --git a/crates/engine/src/session.rs b/crates/engine/src/session.rs index fa2490988..7b25a21f7 100644 --- a/crates/engine/src/session.rs +++ b/crates/engine/src/session.rs @@ -637,6 +637,7 @@ fn typed_range(text: &str, off: usize) -> Option<(usize, usize)> { text.get(start..off)?.chars().any(char::is_alphanumeric).then_some((start, end)) } +/// The name of `doc`'s bibliography style, APA when it names none we know. fn doc_bib_style(doc: &Document) -> String { doc.settings.bib_style.as_deref().and_then(crate::cmd::citations::find_style).map_or("APA", |st| st.name).to_string() } diff --git a/crates/mcp/src/backend.rs b/crates/mcp/src/backend.rs index 1cf17c61d..fe26b2dbb 100644 --- a/crates/mcp/src/backend.rs +++ b/crates/mcp/src/backend.rs @@ -112,6 +112,7 @@ impl Remote { Ok(r) } + /// The address the app is reached at, as given. pub fn addr(&self) -> &str { &self.addr } @@ -127,6 +128,8 @@ impl Remote { }) } + /// Drop the connection and connect again to the first of the address's targets that answers, + /// deciding afresh whether the control key may go to it. fn reconnect(&mut self) -> std::io::Result<()> { self.conn = None; self.key = None; diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index d706d49e9..cc127d8a7 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -52,6 +52,7 @@ fn tool(name: &str, title: &str, desc: &str, schema: Value, ro: bool) -> Value { json!({"name": name, "title": title, "description": desc, "inputSchema": schema, "annotations": {"title": title, "readOnlyHint": ro, "openWorldHint": false}}) } +/// Every MCP tool with its title, description, input schema and annotations (`tools/list`). pub fn tool_definitions() -> Value { json!([ tool( @@ -168,6 +169,8 @@ fn wrap(r: Result) -> ToolResult { } } +/// Run tool `name` with arguments `a` through backend `b` (`tools/call`); unknown tools and +/// failed calls are error results, never panics. pub fn call_tool(b: &mut dyn Backend, name: &str, a: &Value) -> ToolResult { let st = |k: &str| a.get(k).and_then(Value::as_str); match name { From 3d944e735af51ab42a66cdc66e6a5a23b6a68d44 Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Fri, 23 Oct 2026 11:32:58 +0100 Subject: [PATCH 13/16] =?UTF-8?q?#3692:=20review=20=E2=80=94=20the=20typed?= =?UTF-8?q?-paragraph=20line=20cap=20is=20text.insert's=20own,=20so=20cont?= =?UTF-8?q?rol-channel=20and=20CLI=20calls=20are=20held=20to=20it=20too?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MCP tool no longer counts lines itself; it quotes the engine's MAX_TYPED_PARAGRAPHS. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01CX1hMKX28764Lrx5moxs1o --- crates/engine/src/cmd/text.rs | 16 +++++++++++++--- crates/engine/src/tests.rs | 30 ++++++++++++++++++++++++++++++ crates/mcp/src/tests.rs | 11 ++++++----- crates/mcp/src/tools.rs | 16 ++++++---------- docs/mcp.md | 2 +- 5 files changed, 56 insertions(+), 19 deletions(-) diff --git a/crates/engine/src/cmd/text.rs b/crates/engine/src/cmd/text.rs index 5ce435e27..1781de945 100644 --- a/crates/engine/src/cmd/text.rs +++ b/crates/engine/src/cmd/text.rs @@ -11,7 +11,7 @@ use crate::{CmdError, CmdResult, CommandSpec, Selection, Session, p}; /// The text-entry commands: typing, Enter, breaks, Tab, deleting and typed symbols. pub fn specs() -> Vec { vec![ - CommandSpec::new("text.insert", "Type Text", "Editing", insert).params(r#"{"text": string, "raw"?: bool, "paragraphs"?: bool}"#), + CommandSpec::new("text.insert", "Type Text", "Editing", insert).params(r#"{"text": string, "raw"?: bool, "paragraphs"?: bool} (paragraphs: each line a paragraph, one undo step, at most 10000 lines)"#), CommandSpec::new("text.newParagraph", "New Paragraph", "Editing", new_paragraph).key("Enter"), CommandSpec::new("text.lineBreak", "Line Break", "Editing", |s, _| ins_char(s, '\n')).key("Shift+Enter"), CommandSpec::new("text.pageBreak", "Page Break", "Insert › Pages", |s, _| ins_char(s, PAGE_BREAK)).key("Mod+Enter"), @@ -587,14 +587,24 @@ fn new_paragraph(s: &mut Session, _: &Value) -> CmdResult { sel_result(s) } +/// Most lines `text.insert` with `paragraphs` types in one call, for every caller (control +/// channel, CLI, MCP): each line is an Enter, with its AutoFormat and AutoCorrect, so the work +/// grows with the line count. More is refused before anything is typed (#3692). +pub const MAX_TYPED_PARAGRAPHS: usize = 10_000; + /// `text.insert` with `paragraphs`: each line typed as it is (`raw`), and between lines the /// paragraph break Enter makes ([`enter`]: its AutoFormat and AutoCorrect), all in one command, so /// one undo step instead of a command and a whole-document snapshot per line (#3692). Where /// Enter would do nothing (a one-line control, a check box, a list) the text is refused and, -/// like any failed command, nothing of it stays. +/// like any failed command, nothing of it stays. More than [`MAX_TYPED_PARAGRAPHS`] lines are +/// refused before anything is typed. fn insert_paragraphs(s: &mut Session, text: &str) -> CmdResult { - typing_options(s, text); let text = text.replace("\r\n", "\n"); + let lines = text.split(['\n', '\r', '\u{2029}']).count(); + if lines > MAX_TYPED_PARAGRAPHS { + return Err(CmdError::Params(format!("text.insert: `text` has {lines} lines; type at most {MAX_TYPED_PARAGRAPHS} paragraphs per call"))); + } + typing_options(s, &text); for (i, line) in text.split(['\n', '\r', '\u{2029}']).enumerate() { if i > 0 { if super::controls::refuses_paragraph(s) { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 4f1aabf0b..08a0c04d2 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3368,6 +3368,36 @@ fn failed_command_leaves_document_unchanged() { assert_eq!(text(&s), before); } +/// Review of #3692: the line cap on typed paragraphs is the engine's, so a control-channel or +/// CLI `text.insert` is held to it as the MCP tool is. At the cap every line goes in as one undo +/// step; one line more is refused before anything is typed. +#[test] +fn typed_paragraphs_are_capped_for_every_caller() { + use crate::CmdError; + use crate::cmd::text::MAX_TYPED_PARAGRAPHS; + let mut s = s(); + run(&mut s, "text.insert", json!({"text": "keep"})); + let (before, depth) = (body_texts(&s), s.undo_depth()); + // "x\n" repeated ends in an empty line: one over the cap. A lone "\r" counts as a break too. + for over in ["x\n".repeat(MAX_TYPED_PARAGRAPHS), format!("{}x", "x\r\n".repeat(MAX_TYPED_PARAGRAPHS - 1)) + "\rx"] { + match s.run("text.insert", &json!({"text": over, "paragraphs": true})) { + Err(CmdError::Params(m)) => assert!(m.contains("10001 lines") && m.contains("at most 10000"), "{m}"), + r => panic!("not refused: {r:?}"), + } + assert_eq!(body_texts(&s), before, "nothing typed"); + assert_eq!(s.undo_depth(), depth, "no undo step"); + } + let at_cap = vec!["y"; MAX_TYPED_PARAGRAPHS].join("\r\n"); + run(&mut s, "text.insert", json!({"text": at_cap, "paragraphs": true})); + let paras = body_texts(&s); + assert_eq!(paras.len(), MAX_TYPED_PARAGRAPHS); + // Each break is Enter's own, AutoCorrect included: the first sentence gets its capital. + assert_eq!((paras.first().map(String::as_str), paras.last().map(String::as_str)), (Some("Keepy"), Some("y"))); + assert!(s.undo(), "undo"); + assert_eq!(body_texts(&s).len(), 1, "one undo step takes every line back out"); + assert!(!body_texts(&s).iter().any(|p| p.contains('y'))); +} + #[test] fn hostile_params_never_panic() { let reg = cmd::registry(); diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index de99ddd57..8bc4a444e 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -158,7 +158,7 @@ fn type_text_stops_at_a_refused_paragraph_break() { } /// #3692: typed paragraphs are one edit and one undo step, not a command (and a whole-document -/// undo snapshot) per line; too many lines are refused before anything is typed. +/// undo snapshot) per line; too many lines are refused by the engine before anything is typed. #[test] fn type_text_paragraphs_are_one_undo_step_with_a_line_cap() { let mut s = Server::new(Box::new(Headless::default())); @@ -173,12 +173,13 @@ fn type_text_paragraphs_are_one_undo_step_with_a_line_cap() { assert_eq!(paras(&mut s), 1, "one Undo takes all the lines back out"); assert!(!tool(&mut s, "get_text", json!({}))["text"].as_str().unwrap().contains("Line")); - let calls = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); - let mut s = Server::new(Box::new(RefusesParagraph { calls: calls.clone() })); - let many = "x\n".repeat(crate::tools::MAX_TYPED_PARAGRAPHS); + // One line over the engine's cap ("x\n" repeated gives a last, empty line): refused, nothing typed. + let many = "x\n".repeat(wordcraft_engine::cmd::text::MAX_TYPED_PARAGRAPHS); let r = call(&mut s, 12, "tools/call", json!({"name": "type_text", "arguments": {"text": many, "paragraphs": true}})); assert_eq!(r["result"]["isError"], true, "{r}"); - assert!(calls.lock().unwrap().is_empty(), "refused before any command"); + assert!(r["result"]["content"][0]["text"].as_str().unwrap().contains("at most 10000 paragraphs"), "{r}"); + assert_eq!(paras(&mut s), 1, "nothing typed"); + assert!(!tool(&mut s, "get_text", json!({}))["text"].as_str().unwrap().contains('x')); } /// Ultrareview of #3692: each typed paragraph break is Enter's own (text.newParagraph's diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index cc127d8a7..54f3873fb 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -103,7 +103,10 @@ pub fn tool_definitions() -> Value { tool( "type_text", "Type text", - "Type at the caret (replacing the selection). Use \\n... for line breaks inside text; use execute text.newParagraph for new paragraphs, or `paragraphs: true` to split lines into paragraphs (one undo step, at most 10000 lines).", + &format!( + "Type at the caret (replacing the selection). Use \\n... for line breaks inside text; use execute text.newParagraph for new paragraphs, or `paragraphs: true` to split lines into paragraphs (one undo step, at most {} lines).", + wordcraft_engine::cmd::text::MAX_TYPED_PARAGRAPHS + ), obj(json!({"text": s("Text"), "paragraphs": {"type": "boolean"}}), &["text"]), false ), @@ -155,9 +158,6 @@ pub fn tool_definitions() -> Value { ]) } -/// Most paragraphs `type_text` with `paragraphs: true` types in one call. -pub const MAX_TYPED_PARAGRAPHS: usize = 10_000; - fn exec(b: &mut dyn Backend, id: &str, params: Value) -> Result { b.call("engine.execute", json!({"command": id, "params": params})) } @@ -238,12 +238,8 @@ pub fn call_tool(b: &mut dyn Backend, name: &str, a: &Value) -> ToolResult { // One `text.insert` with `paragraphs`: each break is Enter's own (its AutoFormat, // and its refusal in a one-line control), but the lines go in as one edit and one // undo step, all or nothing, instead of a command (and a whole-document undo - // snapshot) per line. - let t = t.replace("\r\n", "\n").replace('\r', "\n"); - let lines = t.split(['\n', '\u{2029}']).count(); - if lines > MAX_TYPED_PARAGRAPHS { - return ToolResult::error(format!("`text` has {lines} lines; type at most {MAX_TYPED_PARAGRAPHS} paragraphs per call")); - } + // snapshot) per line. The engine splits the lines and holds them to its cap + // (`MAX_TYPED_PARAGRAPHS`), for this tool and every other caller alike. wrap(exec(b, "text.insert", json!({"text": t, "raw": true, "paragraphs": true}))) } else { wrap(exec(b, "text.insert", json!({"text": t, "raw": true}))) diff --git a/docs/mcp.md b/docs/mcp.md index 99bf754d5..e0dde5f7d 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -112,7 +112,7 @@ files it was given rather than guarding against other software on the computer. | `batch` | run several commands in order | | `new_document` | blank, sample, letter, resume, report | | `open_document` / `save_document` | docx, doc (open only), odt, rtf, html, md, tex, txt, json; save also pdf, png | -| `type_text` | type at the caret (`paragraphs: true` splits lines into paragraphs: one undo step, at most 10,000 lines) | +| `type_text` | type at the caret (`paragraphs: true` splits lines into paragraphs: one undo step, at most 10,000 lines — the cap is `text.insert`'s own, so a control-channel or CLI call is held to it too) | | `select_text` | select the n-th occurrence of some text | | `get_text` / `inspect_document` | read the document back (verify without screenshots) | | `render_page` | a page as PNG | From 66601038df94ce538741fb31a025c6ce0fc6bdec Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sun, 25 Oct 2026 21:52:01 -0400 Subject: [PATCH 14/16] tests(layout): build the nbsp test document without struct-update syntax (#4380) --- crates/layout/src/tests_nbsp.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/layout/src/tests_nbsp.rs b/crates/layout/src/tests_nbsp.rs index b8dbc9aad..d2908fba7 100644 --- a/crates/layout/src/tests_nbsp.rs +++ b/crates/layout/src/tests_nbsp.rs @@ -37,7 +37,8 @@ fn wide_nbsp_family() -> &'static str { /// The width of the first line of `text` set in the wide-nbsp font. fn line_width(text: &str) -> f32 { let chr = wordcraft_doc::CharProps { font: Some(wide_nbsp_family().into()), size: Some(12.0), ..Default::default() }; - let d = Document { body: vec![wordcraft_doc::para_block(wordcraft_doc::Paragraph::with_text(text, chr))], ..Document::new() }; + let mut d = Document::new(); + d.body = vec![wordcraft_doc::para_block(wordcraft_doc::Paragraph::with_text(text, chr))]; let mut c = LayoutCache::new(); let l = layout(&d, &mut c, &LayoutOptions::default()); for p in &l.pages { From b654049545aa41cae0bd50f4b2c4221c05ea961e Mon Sep 17 00:00:00 2001 From: Martin Ball <98639962+benandez@users.noreply.github.com> Date: Mon, 26 Oct 2026 02:30:32 +0000 Subject: [PATCH 15/16] =?UTF-8?q?#3874:=20review=20=E2=80=94=20text=20box?= =?UTF-8?q?=20paste=20charges=20only=20what=20it=20copies;=20stories=20sha?= =?UTF-8?q?ring=20Arc=20blocks=20(PDF=20imports)=20keep=20their=20text=20(?= =?UTF-8?q?#4381)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Claude Opus 5.5 (1M context) --- crates/doc/src/edit.rs | 42 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/crates/doc/src/edit.rs b/crates/doc/src/edit.rs index 1c5b87766..64a75b444 100644 --- a/crates/doc/src/edit.rs +++ b/crates/doc/src/edit.rs @@ -188,8 +188,9 @@ fn fit_tables(b: Block, room: usize, out: &mut Vec) { const MAX_BOX_DEPTH: usize = 8; /// Most text box stories one paste may create (hostile documents can fan out). const MAX_PASTED_BOXES: usize = 4096; -/// Most text box content (estimated bytes, see [`block_cost`]) one paste may adopt: thousands of -/// boxes can show the same large story, and each needs a story of its own. +/// Most text box content (estimated bytes, see [`block_cost`]) one paste may copy: thousands of +/// boxes can show the same story, and each needs a story of its own. Blocks without text boxes +/// are shared between those stories, not copied, so they are not charged per box. const MAX_PASTED_BOX_BYTES: usize = 32 << 20; /// What a paste may still spend on text box stories. @@ -901,9 +902,16 @@ impl Document { return None; } let src = parts.get(&old).or_else(|| self.parts.get(&old).filter(|p| p.kind == PartKind::TextBox))?; - // Charge the whole story before copying any of it. Its cost is worked out once per paste - // (that walks its payloads), however many boxes show it. - let cost = *budget.costs.entry(old).or_insert_with(|| src.blocks.iter().fold(0usize, |a, b| a.saturating_add(block_cost(b).0))); + // Charge what the copy really allocates before making any of it: the block slots, and the + // blocks holding text boxes (deep-copied below). The rest are shared `Arc`s, however many + // boxes show them, so they cost nothing more. Worked out once per paste. + let cost = *budget.costs.entry(old).or_insert_with(|| { + let slots = src.blocks.len().saturating_mul(std::mem::size_of::>()); + src.blocks.iter().fold(slots, |a, b| match block_cost(b) { + (c, true) => a.saturating_add(c), + (_, false) => a, + }) + }); let Some(left) = budget.bytes.checked_sub(cost) else { budget.skipped = budget.skipped.saturating_add(1); return None; @@ -1385,7 +1393,10 @@ mod tests { fn a_box_over_the_budget_leaves_later_smaller_boxes_their_stories() { // Review of #3874: once one story didn't fit, every later box in the paste was left // empty, however small its story. Only the box that doesn't fit is. - let big = para_block(Paragraph::with_text(&"x".repeat(100_000), CharProps::default())); + // (Story 1 holds a text box, so each box showing it needs a real copy.) + let mut bp = Paragraph::with_text(&"x".repeat(100_000), CharProps::default()); + bp.insert_object(0, box_shape(Some(999)), &CharProps::default()).unwrap(); + let big = para_block(bp); let small = para_block(Paragraph::with_text("small", CharProps::default())); let mut p = Paragraph::new(); for story in [2, 1, 2, 1] { @@ -1405,6 +1416,25 @@ mod tests { assert_eq!(budget.costs.len(), 2, "each story costed once"); } + #[test] + fn many_boxes_sharing_one_story_paste_with_their_text() { + // Review of #3874: a document converted from PDF has thousands of boxes showing story + // data they share. 64 boxes x a 1 MiB block is 64 MiB counted per box (over the 32 MiB + // cap) but the block is shared, not copied, so every box keeps its text. + let big = para_block(Paragraph::with_text(&"x".repeat(1 << 20), CharProps::default())); + let f = fan_out(vec![big.clone()], 64); + let mut d = doc(&["x"]); + let (_, warned) = crate::test_log::warnings(|| d.insert_fragment(&Pos::body(0, 1), &f).unwrap()); + assert!(!warned.iter().any(|w| w.contains("text boxes are left empty")), "{warned:?}"); + let ids: Vec = boxes_in(&d, &Path::top(0)).into_iter().flatten().collect(); + assert_eq!(ids.len(), 64, "every box keeps a story"); + for id in ids { + let b = d.parts[&id].blocks.first().unwrap(); + assert!(Arc::ptr_eq(b, &big), "shared, so not charged per box"); + assert_eq!(d.parts[&id].blocks.len(), 1); + } + } + #[test] fn pasted_box_copies_are_capped_in_bytes() { // Story 1 is a 1 MiB paragraph that holds a text box itself, so each box showing it needs From ed1296a1d4a1c72618a69bec8c45341aebe9a868 Mon Sep 17 00:00:00 2001 From: Martin Ball <98639962+benandez@users.noreply.github.com> Date: Mon, 26 Oct 2026 02:37:10 +0000 Subject: [PATCH 16/16] #3871 follow-up: .doc picture decode charges zlib input and DIB copies; budget scales with the file (#4382) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * #3871: review — metafile input charged to the decode budget; inline BLIPs deduped; budget scales with input - A compressed metafile is charged max(inflated size, zlib input), so a record claiming to inflate to nothing still costs its input. - Inline pictures share one decode per BLIP record, cached by (stream, foDelay, record's own length) instead of the FBSE size field. - The decode budget is max(512 MB, 64 x input bytes), capped at 2 GiB, so documents of many large legitimate DIBs decode; high-ratio bombs stay capped. - Decode-budget refusals get their own log line. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01HUodYU9r4mPHed4pPsjHC2 * #3871: review — a DIB's copy into a BMP is charged to the decode budget before it is made The charge was w*h*4 after the copy, so a 1x1 header followed by megabytes of tail cost 4 bytes per picture. It is now max(pixels, DIB length), charged first; a bitmap whose copy doesn't fit is dropped rather than copied. The doc comment on the budget scale now says RLE8/RLE4 bitmaps (about 510x) rely on the 512 MB floor. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01HUodYU9r4mPHed4pPsjHC2 --------- Co-authored-by: Claude Sonnet 5.5 --- crates/docbin/src/lib.rs | 2 +- crates/docbin/src/media.rs | 59 +++++++---- crates/docbin/src/officeart.rs | 81 +++++++++++---- crates/docbin/src/tests/drawing.rs | 158 +++++++++++++++++++++++++---- 4 files changed, 241 insertions(+), 59 deletions(-) diff --git a/crates/docbin/src/lib.rs b/crates/docbin/src/lib.rs index 845171251..7734c7222 100644 --- a/crates/docbin/src/lib.rs +++ b/crates/docbin/src/lib.rs @@ -259,7 +259,7 @@ pub fn read_with_password(bytes: &[u8], password: Option<&str>) -> Result usize { + input.saturating_mul(DECODE_WORK_PER_INPUT_BYTE).clamp(floor, floor.max(MAX_DECODE_WORK_CEILING)) +} /// The document's pictures, keyed by their location in the Data stream: anchors that share a /// `fc_pic` share one media entry (or one failure), and the total bytes copied are capped, so a @@ -32,16 +46,17 @@ pub(crate) struct Pictures { by_fc: HashMap>, /// BLIP store index → media key (`None`: unreadable, not retried). by_bse: HashMap>, - /// (foDelay, end) of a BLIP record outside the store → media key: store entries that alias - /// one record share one decode (`None`: failed, not retried). - by_delay: HashMap<(usize, usize), Option>, + /// (stream, foDelay, record length) of a BLIP record outside the store → media key: store + /// entries and inline pictures that alias one record share one decode (`None`: failed, not + /// retried). The stream is told apart by its address. + by_delay: HashMap<(usize, usize, usize), Option>, /// The media entries, merged into the document at the end. pub(crate) media: BTreeMap>>, total: usize, /// The byte cap (`MAX_MEDIA_BYTES`; smaller in tests). pub(crate) limit: usize, /// Decode work left (`MAX_DECODE_WORK`; smaller in tests). - pub(crate) work: usize, + pub(crate) work: officeart::Decode, /// Anchors whose picture was dropped (only the first few are logged). dropped: usize, /// Pictures kept as placeholders (formats we can't draw). @@ -57,7 +72,7 @@ impl Default for Pictures { media: BTreeMap::new(), total: 0, limit: MAX_MEDIA_BYTES, - work: MAX_DECODE_WORK, + work: officeart::Decode::new(MAX_DECODE_WORK), dropped: 0, placeholders: 0, } @@ -75,6 +90,12 @@ pub(crate) struct Inline { } impl Pictures { + /// An empty store whose decode budget scales with the `input` stream bytes (WordDocument + /// plus Data) the pictures can come from. + pub(crate) fn for_input(input: usize) -> Self { + Pictures { work: officeart::Decode::new(decode_work(input, MAX_DECODE_WORK)), ..Pictures::default() } + } + /// The inline picture at `fc_pic` in the Data stream, copying its bytes the first time. /// `None` when the header is malformed, the picture can't be read, or a picture /// count/byte cap is reached. @@ -96,16 +117,13 @@ impl Pictures { if room == 0 { return None; } - match locate(data, fc_pic, room, &mut self.work)? { + match locate(data, fc_pic, self)? { Found::Range(pic) => { let bytes = data.get(pic.range.clone()).filter(|b| b.len() <= room)?.to_vec(); let key = self.insert(bytes, pic.ext, false)?; Some(Inline { key, w: pic.w, h: pic.h, crop: [0.0; 4], alt: String::new() }) } - Found::Shape { w, h, blip, props } => { - let key = self.insert(blip.bytes, blip.ext, blip.placeholder)?; - Some(Inline { key, w, h, crop: props.crop, alt: props.alt }) - } + Found::Shape { w, h, key, props } => Some(Inline { key: key?, w, h, crop: props.crop, alt: props.alt }), } } @@ -127,6 +145,7 @@ impl Pictures { /// Decode a store entry's BLIP, once per delay-stream record however many entries name it. fn decode_bse(&mut self, fbse: &[u8], delay: &[u8]) -> Option { let (rec, at) = officeart::fbse_record(fbse, delay)?; + let at = at.map(|(fo, used)| (delay.as_ptr() as usize, fo, used)); if let Some(hit) = at.and_then(|at| self.by_delay.get(&at)) { return hit.clone(); } @@ -174,7 +193,7 @@ struct Located { /// `OfficeArtInlineSpContainer` (Word 97 and later) whose BLIP is decoded. enum Found { Range(Located), - Shape { w: f32, h: f32, blip: officeart::Blip, props: officeart::Props }, + Shape { w: f32, h: f32, key: Option, props: officeart::Props }, } /// `PICF.mfpf.mm` values of pictures stored as a shape (`MM_SHAPE`) or a shape with a file @@ -183,8 +202,8 @@ const MM_SHAPE: u16 = 0x0064; const MM_SHAPEFILE: u16 = 0x0066; /// Locate the picture at `fc_pic` in the Data stream ([MS-DOC] §2.9.192 PICFAndOfficeArtData); -/// a shape's picture is decoded only up to `room` bytes. -fn locate(data: &[u8], fc_pic: u32, room: usize, work: &mut usize) -> Option { +/// the BLIP is decoded once per record and charged to the decode budget. +fn locate(data: &[u8], fc_pic: u32, pics: &mut Pictures) -> Option { let base = fc_pic as usize; let pic = data.get(base..)?; let lcb = u32::from_le_bytes(pic.get(0..4)?.try_into().ok()?) as usize; @@ -203,12 +222,12 @@ fn locate(data: &[u8], fc_pic: u32, room: usize, work: &mut usize) -> Option Option Option<(officeart::Blip, officeart::Props)> { +/// `pib` indexes (BLIPs embedded, or at `foDelay` in the Data stream). The BLIP is decoded +/// once per record, however many pictures (and FBSE sizes) name it. +fn inline_shape(buf: &[u8], data: &[u8], pics: &mut Pictures) -> Option<(Option, officeart::Props)> { let mut budget = 10_000usize; let recs = officeart::children(buf, &mut budget); let sp = recs.iter().find(|r| r.typ == officeart::rt::SP_CONTAINER).map(|r| officeart::shape(r.body, &mut budget))?; let fbses: Vec<&[u8]> = recs.iter().filter(|r| r.typ == officeart::rt::FBSE).map(|r| r.body).collect(); let fbse = sp.props.pib.and_then(|p| fbses.get((p as usize).checked_sub(1)?)).or(fbses.first())?; - let blip = officeart::fbse_blip(fbse, data, room, work)?; - Some((blip, sp.props)) + let key = pics.decode_bse(fbse, data); + Some((key, sp.props)) } /// Offset of the first embedded raster image inside the payload, skipping the small diff --git a/crates/docbin/src/officeart.rs b/crates/docbin/src/officeart.rs index 84d68b9d7..6d35d929d 100644 --- a/crates/docbin/src/officeart.rs +++ b/crates/docbin/src/officeart.rs @@ -112,16 +112,50 @@ pub(crate) struct Blip { /// The picture of an `OfficeArtFBSE` body: its embedded BLIP, or the BLIP stored at `foDelay` /// in `delay` (the WordDocument stream for the document's BLIP store, the Data stream for /// inline pictures). `limit` caps the bytes produced. -pub(crate) fn fbse_blip(fbse: &[u8], delay: &[u8], limit: usize, work: &mut usize) -> Option { +#[cfg(test)] +pub(crate) fn fbse_blip(fbse: &[u8], delay: &[u8], limit: usize, work: &mut Decode) -> Option { let (rec, _) = fbse_record(fbse, delay)?; blip(&rec, limit, work) } +/// The document's decode budget: bytes of inflating and pixel decoding left, and how many +/// attempts it has refused (only the first few are logged). +pub(crate) struct Decode { + pub(crate) left: usize, + refused: usize, +} + +impl Decode { + pub(crate) fn new(left: usize) -> Self { + Decode { left, refused: 0 } + } + + /// Take `cost` off the budget; `false` (and a log line) when it doesn't fit. + fn charge(&mut self, cost: usize, what: &str) -> bool { + match self.left.checked_sub(cost) { + Some(left) if cost > 0 => { + self.left = left; + true + } + _ => { + if self.refused < 10 { + log::warn!( + "docbin: a {what} needing {cost} bytes of decode work is over the decode budget ({} left) and was not decoded", + self.left + ); + } + self.refused = self.refused.saturating_add(1); + false + } + } + } +} + /// The BLIP record of an `OfficeArtFBSE`: embedded after it, or in the delay stream, where -/// its (`foDelay`, end) location is returned too (store entries can share one record). +/// its (`foDelay`, record length) location is returned too (entries can share one record, +/// whatever sizes their FBSEs claim: the record's own header is what counts). pub(crate) fn fbse_record<'a>(fbse: &'a [u8], delay: &'a [u8]) -> Option<(Rec<'a>, Option<(usize, usize)>)> { // btWin32, btMacOS, rgbUid[16], tag, size, cRef, foDelay, unused, cbName, unused×2. - let size = u32_at(fbse, 20)? as usize; let fo_delay = u32_at(fbse, 28)? as usize; let cb_name = *fbse.get(33)? as usize; let at = 36usize.checked_add(cb_name)?; @@ -129,15 +163,14 @@ pub(crate) fn fbse_record<'a>(fbse: &'a [u8], delay: &'a [u8]) -> Option<(Rec<'a return Some((rec, None)); } // Not embedded: the record sits in the delay stream. - let end = fo_delay.checked_add(size.max(8))?.min(delay.len()); - let (rec, _) = record(delay.get(fo_delay..end)?)?; - Some((rec, Some((fo_delay, end)))) + let (rec, used) = record(delay.get(fo_delay..)?)?; + Some((rec, Some((fo_delay, used)))) } /// Decode one `OfficeArtBlip*` record ([MS-ODRAW] §2.2.23-§2.2.31). `work` is the /// document's decode budget: inflating a metafile or decoding a DIB is charged to it before /// it runs, whether it succeeds or not, and nothing is decoded once it is spent. -pub(crate) fn blip(rec: &Rec, limit: usize, work: &mut usize) -> Option { +pub(crate) fn blip(rec: &Rec, limit: usize, work: &mut Decode) -> Option { // Each type has two instance values; the odd one carries a second 16-byte UID. let uids = if rec.inst & 1 == 1 { 32 } else { 16 }; match rec.typ { @@ -163,11 +196,12 @@ pub(crate) fn blip(rec: &Rec, limit: usize, work: &mut usize) -> Option { let raw = raw.get(..cb_save.min(raw.len()))?; let cap = limit.min(MAX_METAFILE); let bytes = if compression == 0 { - let want = (if cb_size > 0 { cb_size.min(cap) } else { cap }).min(*work); - if want == 0 { + let want = if cb_size > 0 { cb_size.min(cap) } else { cap }; + // The compressed stream is read whole whatever it inflates to: a record that + // claims to inflate to nothing still costs its input (#3871). + if !work.charge(want.max(raw.len()), "metafile") { return None; } - *work -= want; miniz_oxide::inflate::decompress_to_vec_zlib_with_limit(raw, want).ok()? } else { if raw.len() > cap { @@ -192,7 +226,7 @@ pub(crate) fn blip(rec: &Rec, limit: usize, work: &mut usize) -> Option { /// the 14-byte BMP file header it lacks, is decoded with size limits and re-encoded. When the /// bitmap can't be decoded, or its declared pixels (4 bytes each) don't fit in `work`, the /// BMP bytes are kept (and render as a grey placeholder). -fn dib_to_png(dib: &[u8], limit: usize, work: &mut usize) -> Option { +fn dib_to_png(dib: &[u8], limit: usize, work: &mut Decode) -> Option { let header = u32_at(dib, 0)? as usize; if !(12..=124).contains(&header) { return None; @@ -211,6 +245,20 @@ fn dib_to_png(dib: &[u8], limit: usize, work: &mut usize) -> Option { let masks = if header == 40 && (compression == 3 || compression == 6) { if compression == 3 { 12 } else { 16 } } else { 0 }; let off_bits = 14usize.checked_add(header)?.checked_add(colors * entry)?.checked_add(masks)?; let file_size = 14usize.checked_add(dib.len()).filter(|&size| size <= limit)?; + // The declared size is what decoding allocates, however small the (RLE) data is. + let (w, h) = if header == 12 { + (u64::from(u16_at(dib, 4)?), u64::from(u16_at(dib, 6)?)) + } else { + (u64::from(u32_at(dib, 4)?), u64::from((u32_at(dib, 8)? as i32).unsigned_abs())) + }; + let cost = w.saturating_mul(h).saturating_mul(4).try_into().unwrap_or(usize::MAX); + // Copying the DIB into a BMP is work too, however few pixels it declares: it is charged + // before the copy is made (#3871). A bitmap whose pixels don't fit still keeps its BMP + // bytes as a placeholder, if the copy fits. + let decode = work.charge(cost.max(dib.len()), "bitmap"); + if !decode && !work.charge(dib.len(), "bitmap") { + return None; + } let mut bmp = Vec::with_capacity(file_size); bmp.extend_from_slice(b"BM"); bmp.extend_from_slice(&u32::try_from(file_size).ok()?.to_le_bytes()); @@ -218,16 +266,9 @@ fn dib_to_png(dib: &[u8], limit: usize, work: &mut usize) -> Option { bmp.extend_from_slice(&u32::try_from(off_bits).ok()?.to_le_bytes()); bmp.extend_from_slice(dib); let placeholder = |bmp| Some(Blip { bytes: bmp, ext: "bmp", placeholder: true }); - // The declared size is what decoding allocates, however small the (RLE) data is. - let (w, h) = if header == 12 { - (u64::from(u16_at(dib, 4)?), u64::from(u16_at(dib, 6)?)) - } else { - (u64::from(u32_at(dib, 4)?), u64::from((u32_at(dib, 8)? as i32).unsigned_abs())) - }; - let Some(left) = w.saturating_mul(h).saturating_mul(4).try_into().ok().and_then(|cost: usize| work.checked_sub(cost)) else { + if !decode { return placeholder(bmp); - }; - *work = left; + } let mut reader = image::ImageReader::new(std::io::Cursor::new(&bmp)); reader.set_format(image::ImageFormat::Bmp); let mut limits = image::Limits::default(); diff --git a/crates/docbin/src/tests/drawing.rs b/crates/docbin/src/tests/drawing.rs index 041267c16..b75420632 100644 --- a/crates/docbin/src/tests/drawing.rs +++ b/crates/docbin/src/tests/drawing.rs @@ -247,7 +247,7 @@ fn dib_becomes_png_and_compressed_metafiles_inflate() { dib.extend_from_slice(&[0, 0, 255, 0, 255, 0, 0, 0]); let r = rec(0, 0x7A8, 0xF01F, &[[0u8; 16].as_slice(), &[0xFF], &dib].concat()); let (dib_rec, _) = officeart::record(&r).unwrap(); - let blip = officeart::blip(&dib_rec, 1 << 20, &mut { usize::MAX }).expect("DIB decodes"); + let blip = officeart::blip(&dib_rec, 1 << 20, &mut officeart::Decode::new(usize::MAX)).expect("DIB decodes"); assert_eq!(blip.ext, "png"); let img = image::load_from_memory(&blip.bytes).unwrap().to_rgb8(); assert_eq!((img.width(), img.height(), img.get_pixel(0, 0).0), (2, 1, [255, 0, 0])); @@ -262,7 +262,7 @@ fn dib_becomes_png_and_compressed_metafiles_inflate() { hdr.extend_from_slice(&[0x00, 0xFE]); // compression = deflate, filter let r = rec(0, 0x216, 0xF01B, &[[0u8; 16].as_slice(), &hdr, &packed].concat()); let (wmf_rec, _) = officeart::record(&r).unwrap(); - let blip = officeart::blip(&wmf_rec, 1 << 20, &mut { usize::MAX }).expect("WMF inflates"); + let blip = officeart::blip(&wmf_rec, 1 << 20, &mut officeart::Decode::new(usize::MAX)).expect("WMF inflates"); assert_eq!((blip.ext, blip.bytes), ("wmf", wmf)); } @@ -281,10 +281,10 @@ fn deeply_nested_groups_and_lying_lengths_are_bounded() { hdr.extend_from_slice(&[0, 0]); let r = rec(0, 0x3D4, 0xF01A, &[[0u8; 16].as_slice(), &hdr, &[0x78, 0x9C, 1, 2, 3]].concat()); let (emf, _) = officeart::record(&r).unwrap(); - assert!(officeart::blip(&emf, 1 << 20, &mut { usize::MAX }).is_none()); + assert!(officeart::blip(&emf, 1 << 20, &mut officeart::Decode::new(usize::MAX)).is_none()); let mut fbse = vec![0u8; 36]; fbse[28..32].copy_from_slice(&u32::MAX.to_le_bytes()); - assert!(officeart::fbse_blip(&fbse, &[0u8; 64], 1 << 20, &mut { usize::MAX }).is_none()); + assert!(officeart::fbse_blip(&fbse, &[0u8; 64], 1 << 20, &mut officeart::Decode::new(usize::MAX)).is_none()); } proptest! { @@ -322,12 +322,12 @@ fn dib_conversion_caps_header_and_encoded_bytes() { let record = rec(0, 0x7A8, officeart::rt::BLIP_DIB, &[[0u8; 16].as_slice(), &[0xFF], &dib].concat()); let (record, _) = officeart::record(&record).unwrap(); let bmp_size = 14 + dib.len(); - assert!(officeart::blip(&record, bmp_size - 1, &mut { usize::MAX }).is_none(), "the BMP header also needs room"); - let fallback = officeart::blip(&record, bmp_size, &mut { usize::MAX }).expect("the BMP fallback fits exactly"); + assert!(officeart::blip(&record, bmp_size - 1, &mut officeart::Decode::new(usize::MAX)).is_none(), "the BMP header also needs room"); + let fallback = officeart::blip(&record, bmp_size, &mut officeart::Decode::new(usize::MAX)).expect("the BMP fallback fits exactly"); assert_eq!((fallback.ext, fallback.placeholder, fallback.bytes.len()), ("bmp", true, bmp_size)); let img = image::load_from_memory(&fallback.bytes).unwrap().to_rgb8(); assert_eq!((img.width(), img.height(), img.get_pixel(0, 0).0), (1, 1, [90, 60, 30])); - let normal = officeart::blip(&record, 512, &mut { usize::MAX }).expect("normal conversion fits"); + let normal = officeart::blip(&record, 512, &mut officeart::Decode::new(usize::MAX)).expect("normal conversion fits"); assert_eq!((normal.ext, normal.placeholder), ("png", false)); assert!(normal.bytes.len() > bmp_size && normal.bytes.len() <= 512); assert_eq!(image::load_from_memory(&normal.bytes).unwrap().width(), 1); @@ -434,10 +434,10 @@ fn store_entries_aliasing_one_delayed_metafile_decode_it_once() { let fbse = delayed_fbse(delay.len(), 0); let store: Vec<&[u8]> = vec![&fbse; 5_000]; let mut pics = crate::media::Pictures::default(); - pics.work = 8 << 20; + pics.work = officeart::Decode::new(8 << 20); let keys: Vec<_> = (1..=store.len() as u32).map(|pib| pics.bse(pib, &store, &delay)).collect(); assert!(keys.iter().all(|k| k.is_some() && *k == keys[0]), "every entry shares one picture"); - assert_eq!(pics.work, (8 << 20) - wmf.len(), "inflated once"); + assert_eq!(pics.work.left, (8 << 20) - wmf.len(), "inflated once"); } /// #3711: distinct metafiles that fail to inflate are charged their declared size anyway, so @@ -456,11 +456,11 @@ fn failing_metafile_inflations_spend_the_decode_budget() { .collect(); let store: Vec<&[u8]> = fbses.iter().map(Vec::as_slice).collect(); let mut pics = crate::media::Pictures::default(); - pics.work = 3 << 20; + pics.work = officeart::Decode::new(3 << 20); for pib in 1..=store.len() as u32 { assert!(pics.bse(pib, &store, &delay).is_none()); } - assert_eq!(pics.work, 0, "three attempts spent the budget; the rest weren't tried"); + assert_eq!(pics.work.left, 0, "three attempts spent the budget; the rest weren't tried"); } /// #3712: a DIB's declared pixels are charged before decoding (an RLE bitmap is tiny on disk but @@ -470,12 +470,13 @@ fn dib_decoding_is_charged_its_declared_pixels() { let blip_of = |dib: &[u8]| rec(0, 0x7A8, officeart::rt::BLIP_DIB, &[[0u8; 16].as_slice(), &[0xFF], dib].concat()); let one = blip_of(&one_pixel_dib()); let (one, _) = officeart::record(&one).unwrap(); - let mut work = 3; - assert_eq!(officeart::blip(&one, 512, &mut work).map(|b| (b.ext, b.placeholder)), Some(("bmp", true))); - assert_eq!(work, 3, "nothing decoded, nothing charged"); - let mut work = 4; + let mut work = officeart::Decode::new(3); + assert!(officeart::blip(&one, 512, &mut work).is_none(), "not even the copy fits: nothing is copied"); + assert_eq!(work.left, 3, "nothing decoded, nothing charged"); + let dib_len = one.body.len() - 17; + let mut work = officeart::Decode::new(dib_len); assert_eq!(officeart::blip(&one, 512, &mut work).map(|b| (b.ext, b.placeholder)), Some(("png", false))); - assert_eq!(work, 0); + assert_eq!(work.left, 0, "the copy is charged, not just 4 bytes a pixel"); // 8-bit RLE declaring 16384 × 16384 (1 GiB decoded) in a few bytes. let mut big = [40u32, 16_384, 16_384].iter().flat_map(|v| v.to_le_bytes()).collect::>(); @@ -489,7 +490,128 @@ fn dib_decoding_is_charged_its_declared_pixels() { big.extend_from_slice(&[0, 0, 0, 1]); // end of line, end of bitmap let big = blip_of(&big); let (big, _) = officeart::record(&big).unwrap(); - let mut work = 512 << 20; + let mut work = officeart::Decode::new(512 << 20); assert_eq!(officeart::blip(&big, 1 << 20, &mut work).map(|b| (b.ext, b.placeholder)), Some(("bmp", true))); - assert_eq!(work, 512 << 20); + assert_eq!(work.left, (512 << 20) - (big.body.len() - 17), "only the copy of the DIB was charged, not its declared pixels"); +} + +/// An EMF `OfficeArtBlip` record (compressed) with `raw` as its stream, claiming `cb_size`. +fn emf_blip(cb_size: u32, raw: &[u8]) -> Vec { + let mut hdr = vec![0u8; 34]; + hdr[0..4].copy_from_slice(&cb_size.to_le_bytes()); + hdr[28..32].copy_from_slice(&(raw.len() as u32).to_le_bytes()); + hdr[32] = 0; // compressed + hdr[33] = 0xFE; + rec(0, 0x3D4, 0xF01A, &[[0u8; 16].as_slice(), &hdr, raw].concat()) +} + +/// #3871 review: the zlib input is charged to the decode budget, not just what it inflates to. +#[test] +fn compressed_metafile_input_is_charged_to_the_decode_budget() { + let raw = vec![0x5Au8; 1 << 20]; // not even zlib: costs its input all the same + let r = emf_blip(1, &raw); + let (emf, _) = officeart::record(&r).unwrap(); + let mut work = officeart::Decode::new(10 << 20); + assert!(officeart::blip(&emf, 64 << 20, &mut work).is_none()); + assert_eq!(work.left, (10 << 20) - raw.len(), "1 MB of input charged, however little it claims to inflate to"); + // Ten more of them exhaust the budget; none is inflated past it. + let mut work = officeart::Decode::new(3 << 20); + for _ in 0..10 { + let _ = officeart::blip(&emf, 64 << 20, &mut work); + } + assert_eq!(work.left, 0, "three attempts fit, the rest are refused"); +} + +/// An inline `PICFAndOfficeArtData` (shape, FBSE) whose BLIP sits at `fo_delay` in Data; the +/// FBSE claims `size` bytes. +fn inline_pic(fo_delay: u32, size: u32) -> Vec { + let mut fbse = vec![0u8; 36]; + fbse[20..24].copy_from_slice(&size.to_le_bytes()); + fbse[28..32].copy_from_slice(&fo_delay.to_le_bytes()); + let body = [container(0xF004, &[]), rec(2, 6, 0xF007, &fbse)].concat(); + let mut picf = vec![0u8; 0x44]; + picf[0..4].copy_from_slice(&((0x44 + body.len()) as u32).to_le_bytes()); + picf[4..6].copy_from_slice(&0x44u16.to_le_bytes()); + picf[6..8].copy_from_slice(&0x64u16.to_le_bytes()); // MM_SHAPE + picf[28..30].copy_from_slice(&1440i16.to_le_bytes()); + picf[30..32].copy_from_slice(&1440i16.to_le_bytes()); + [picf, body].concat() +} + +/// #3871 review: inline pictures that alias one BLIP record share one decode, whatever sizes +/// their FBSEs claim and wherever their PICFs sit. +#[test] +fn inline_pictures_aliasing_one_blip_record_are_decoded_once() { + let blip = emf_blip(1, &vec![0x5Au8; 64 << 10]); + let a = inline_pic(0, 70_000); + let b = inline_pic(0, 80_000); + let c = inline_pic(0, 90_000); + let blip_at = (a.len() + b.len() + c.len()) as u32; + let (a, b, c) = (inline_pic(blip_at, 70_000), inline_pic(blip_at, 80_000), inline_pic(blip_at, 90_000)); + let data = [a.clone(), b.clone(), c, blip].concat(); + let mut pics = crate::media::Pictures::default(); + let start = pics.work.left; + let _ = pics.get(&data, 0); + let _ = pics.get(&data, a.len() as u32); + let _ = pics.get(&data, (a.len() + b.len()) as u32); + assert_eq!(start - pics.work.left, 64 << 10, "one record, one charge"); +} + +/// A 1-bit-per-pixel DIB of `side`×`side` pixels with `bytes` of pixel data. +fn dib_1bpp(side: u32, bytes: usize) -> Vec { + let mut d = vec![0u8; 40]; + d[0..4].copy_from_slice(&40u32.to_le_bytes()); + d[4..8].copy_from_slice(&side.to_le_bytes()); + d[8..12].copy_from_slice(&side.to_le_bytes()); + d[12..14].copy_from_slice(&1u16.to_le_bytes()); + d[14..16].copy_from_slice(&1u16.to_le_bytes()); + d.extend_from_slice(&[0, 0, 0, 0, 255, 255, 255, 0]); // palette + d.extend(std::iter::repeat_n(0u8, bytes)); + d +} + +fn dib_record(dib: &[u8]) -> Vec { + rec(0, 0x7A8, 0xF01F, &[[0u8; 16].as_slice(), &[0xFF], dib].concat()) +} + +/// #3871 review: the decode budget scales with the input, so a document of many legitimate +/// large scans decodes them all; a high-ratio set is still capped. +#[test] +fn the_decode_budget_scales_with_the_input_but_still_caps_bombs() { + let floor = 1 << 20; + // 20 legitimate 1024² 1-bit scans: 4 MB charged each, 128 KB of file each. + let real = dib_record(&dib_1bpp(1024, 1024 * 1024 / 8)); + let (rec_real, _) = officeart::record(&real).unwrap(); + let input = 20 * real.len(); + let mut work = officeart::Decode::new(crate::media::decode_work(input, floor)); + for i in 0..20 { + let b = officeart::blip(&rec_real, 64 << 20, &mut work).unwrap(); + assert!(!b.placeholder, "scan {i} decodes"); + } + // Declared 20000² (1.6 GB each) in a few dozen bytes each: a placeholder, never decoded. + let bomb = dib_record(&dib_1bpp(20_000, 16)); + let (rec_bomb, _) = officeart::record(&bomb).unwrap(); + let mut work = officeart::Decode::new(crate::media::decode_work(20 * bomb.len(), floor)); + for _ in 0..20 { + assert!(officeart::blip(&rec_bomb, 64 << 20, &mut work).unwrap().placeholder); + } + assert!(work.left <= crate::media::decode_work(20 * bomb.len(), floor), "nothing was refunded or invented"); +} + +/// #3871 review: the copy of a DIB into a BMP is charged before it is made, however few pixels +/// the header declares; with the budget spent nothing is copied at all. +#[test] +fn a_dib_with_a_large_tail_is_charged_for_its_copy() { + let big = dib_record(&dib_1bpp(1, 1 << 20)); // 1 pixel declared, 1 MB of trailing bytes + let (big, _) = officeart::record(&big).unwrap(); + let dib_len = big.body.len() - 17; + let mut work = officeart::Decode::new(10 << 20); + assert!(!officeart::blip(&big, 64 << 20, &mut work).unwrap().placeholder); + assert_eq!(10 << 20, work.left + dib_len, "charged for the bytes, not just 4 per pixel"); + // Room for the copy: decoded. Not even that: nothing is copied. + let mut work = officeart::Decode::new(dib_len + 1); + assert!(officeart::blip(&big, 64 << 20, &mut work).is_some()); + let mut work = officeart::Decode::new(dib_len - 1); + assert!(officeart::blip(&big, 64 << 20, &mut work).is_none()); + assert_eq!(work.left, dib_len - 1); }