From 90efee7636c7922a9f7ebe34241fb819f1a1c7ed Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 19:12:38 +0100 Subject: [PATCH 01/10] #3692: Zotero lists each note's fields once, within a field and code-byte budget Audit finding 12 (CWE-400): fields::list expanded a note's fields again for every body reference to it, so a crafted file with thousands of references to one citation-heavy endnote multiplied into millions of copied fields on the UI thread. Each note part is now enumerated once, at its first reference, and the list stops at MAX_FIELDS fields / MAX_CODE_BYTES of codes. Note references are walked with the paragraph's objects directly instead of object_at per marker. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/zotero/src/fields.rs | 36 +++++++++++++++++---- crates/zotero/src/lib.rs | 2 ++ crates/zotero/src/tests_limits.rs | 52 +++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 6 deletions(-) create mode 100644 crates/zotero/src/tests_limits.rs diff --git a/crates/zotero/src/fields.rs b/crates/zotero/src/fields.rs index c3776f9a5..21f7d2cc0 100644 --- a/crates/zotero/src/fields.rs +++ b/crates/zotero/src/fields.rs @@ -63,7 +63,14 @@ fn story_fields(doc: &Document, story: StoryRef, note_index: u32) -> Vec .collect() } +/// Most fields [`list`] hands Zotero (no real document comes near it). +pub const MAX_FIELDS: usize = 100_000; +/// Most bytes of field codes [`list`] hands Zotero, all fields together. +pub const MAX_CODE_BYTES: usize = 64 * 1024 * 1024; + /// Every Zotero field in document order: the body's, with each note's fields at its reference. +/// A note the body references more than once (a crafted file) is listed at its first reference +/// only. Past [`MAX_FIELDS`] fields or [`MAX_CODE_BYTES`] of codes the rest are left out. pub fn list(doc: &Document) -> Vec { enum Ev { Field(ZField), @@ -71,10 +78,12 @@ pub fn list(doc: &Document) -> Vec { } let mut evs: Vec<(Pos, Ev)> = story_fields(doc, StoryRef::Body, 0).into_iter().map(|f| (f.start.clone(), Ev::Field(f))).collect(); let (mut foot, mut end) = (0u32, 0u32); + let mut seen = std::collections::HashSet::new(); for path in doc.para_paths(StoryRef::Body) { let Some(p) = doc.para(StoryRef::Body, &path) else { continue }; - for off in p.object_offsets() { - if let Some(InlineObject::NoteRef { kind, id, custom }) = p.object_at(off) { + // The k-th object marker holds the k-th object. + for (off, obj) in p.object_offsets().into_iter().zip(&p.objects) { + if let InlineObject::NoteRef { kind, id, custom } = obj { let n = match kind { NoteKind::Footnote => &mut foot, NoteKind::Endnote => &mut end, @@ -82,16 +91,31 @@ pub fn list(doc: &Document) -> Vec { if custom.is_empty() { *n = n.saturating_add(1); } - evs.push((Pos::new(StoryRef::Body, path.clone(), off), Ev::Note(*id, (*n).max(1)))); + if seen.insert(*id) { + evs.push((Pos::new(StoryRef::Body, path.clone(), off), Ev::Note(*id, (*n).max(1)))); + } } } } evs.sort_by(|a, b| a.0.cmp(&b.0)); let mut out = Vec::new(); + let mut bytes = 0usize; + let mut take = |f: ZField, out: &mut Vec| { + bytes = bytes.saturating_add(f.code.len()); + let fits = out.len() < MAX_FIELDS && bytes <= MAX_CODE_BYTES; + if fits { + out.push(f); + } + fits + }; for (_, ev) in evs { - match ev { - Ev::Field(f) => out.push(f), - Ev::Note(id, n) => out.extend(story_fields(doc, StoryRef::Part(id), n)), + let fits = match ev { + Ev::Field(f) => take(f, &mut out), + Ev::Note(id, n) => story_fields(doc, StoryRef::Part(id), n).into_iter().all(|f| take(f, &mut out)), + }; + if !fits { + log::warn!("Zotero: the document has more citation fields than WordCraft hands Zotero; the rest are left out"); + break; } } out diff --git a/crates/zotero/src/lib.rs b/crates/zotero/src/lib.rs index 40eba55eb..4c50bab04 100644 --- a/crates/zotero/src/lib.rs +++ b/crates/zotero/src/lib.rs @@ -41,3 +41,5 @@ pub enum ZoteroError { #[cfg(test)] mod tests; +#[cfg(test)] +mod tests_limits; diff --git a/crates/zotero/src/tests_limits.rs b/crates/zotero/src/tests_limits.rs new file mode 100644 index 000000000..93cefc62c --- /dev/null +++ b/crates/zotero/src/tests_limits.rs @@ -0,0 +1,52 @@ +//! Hostile documents and calls stay cheap: work grows with the document, not with how often a +//! crafted file or call repeats something (#3692). + +use std::time::{Duration, Instant}; + +use wordcraft_doc::para::{NoteKind, OBJ}; +use wordcraft_doc::{CharProps, Document, InlineObject, Paragraph, PartKind, Run, StoryRef, para_block}; + +use crate::fields; + +const CITE: &str = r#"ADDIN ZOTERO_ITEM CSL_CITATION {"citationID":"k1","citationItems":[{"id":7}]}"#; + +/// A paragraph of `objects`, one marker each, in one plain run. +fn objects_para(objects: Vec) -> Paragraph { + let mut p = Paragraph::new(); + p.text = std::iter::repeat_n(OBJ, objects.len()).collect(); + p.runs = vec![Run { len: p.text.len(), props: CharProps::default() }]; + p.objects = objects; + p +} + +/// `n` empty Zotero citation fields in a row. +fn citations(n: usize) -> Vec { + (0..n).flat_map(|_| [InlineObject::FieldStart { instr: CITE.into(), locked: false, dirty: false }, InlineObject::FieldEnd]).collect() +} + +// Finding 12: one endnote referenced many times is listed once, not once per reference. +#[test] +fn a_note_referenced_many_times_is_listed_once() { + let mut doc = Document::from_text(""); + let note = doc.add_part(PartKind::Endnote, vec![para_block(objects_para(citations(200)))]); + let refs = (0..5_000).map(|_| InlineObject::NoteRef { kind: NoteKind::Endnote, id: note, custom: String::new() }).collect(); + doc.body = vec![para_block(objects_para(refs))]; + let t = Instant::now(); + let list = fields::list(&doc); + assert_eq!(list.len(), 200, "each field once, at the note's first reference"); + assert!(list.iter().all(|f| f.start.story == StoryRef::Part(note) && f.note_index == 1)); + assert!(t.elapsed() < Duration::from_secs(2), "{:?}", t.elapsed()); +} + +#[test] +fn listing_stops_at_the_field_budget() { + let mut doc = Document::from_text(""); + // Many short paragraphs: the per-paragraph object scan is another finding's (#3614). + doc.body = (0..fields::MAX_FIELDS / 100 + 1).map(|_| para_block(objects_para(citations(100)))).collect(); + assert_eq!(fields::list(&doc).len(), fields::MAX_FIELDS); + // Codes count too: a few huge codes fill the byte budget before the field budget. + let big = format!("{CITE}{}", "x".repeat(fields::MAX_CODE_BYTES / 4 - 1000)); + let huge = (0..5).flat_map(|_| [InlineObject::FieldStart { instr: big.clone(), locked: false, dirty: false }, InlineObject::FieldEnd]).collect(); + doc.body = vec![para_block(objects_para(huge))]; + assert_eq!(fields::list(&doc).len(), 4); +} From 020194ebdb5e1a80963d35e6a41015b959593c91 Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 19:13:00 +0100 Subject: [PATCH 02/10] #3692: Zotero rich text is built in one pass, not re-normalized per run Audit finding 16 (CWE-407): rich::from_doc added each formatting run through Paragraph::insert_text, which splits and normalizes every earlier run, so a compact paragraph of alternating formatting was quadratic on the UI thread (100,000 runs: over 9 minutes; now 0.1 s). Runs are appended, merged only with the run before, and the paragraph is normalized once. A cumulative MAX_RUNS budget keeps the text but stops adding runs past it. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/zotero/src/rich.rs | 22 +++++++++++++++++++--- crates/zotero/src/tests_limits.rs | 20 ++++++++++++++++++++ 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/crates/zotero/src/rich.rs b/crates/zotero/src/rich.rs index 2fabb62bb..6285ecf3e 100644 --- a/crates/zotero/src/rich.rs +++ b/crates/zotero/src/rich.rs @@ -4,10 +4,12 @@ use wordcraft_doc::edit::Fragment; use wordcraft_doc::para::OBJ; -use wordcraft_doc::{Block, CharProps, Document, Paragraph}; +use wordcraft_doc::{Block, CharProps, Document, Paragraph, Run}; /// Most paragraphs taken from one text (a bibliography of thousands of entries is fine). const MAX_PARAS: usize = 100_000; +/// Most formatting runs taken from one text; past it, text keeps the formatting before it. +pub(crate) const MAX_RUNS: usize = 1_000_000; fn style_props(base: &CharProps, src: &CharProps) -> CharProps { let mut p = base.clone(); @@ -40,16 +42,30 @@ fn style_props(base: &CharProps, src: &CharProps) -> CharProps { fn from_doc(d: &Document, base: &CharProps) -> Vec { let mut out = Vec::new(); + let mut runs = 0usize; for b in d.body.iter().take(MAX_PARAS) { let Some(src) = b.as_para() else { continue }; let mut p = Paragraph::new(); p.mark = base.clone(); + // Appended run by run, merging only with the run before; normalized once at the end. for (range, props) in src.run_ranges() { let Some(t) = src.text.get(range) else { continue }; let t: String = t.chars().filter(|c| *c != OBJ).collect(); - let at = p.len(); - let _ = p.insert_text(at, &t, &style_props(base, props)); + if t.is_empty() { + continue; + } + let props = style_props(base, props); + p.text.push_str(&t); + match p.runs.last_mut() { + Some(last) if last.props == props || runs >= MAX_RUNS => last.len += t.len(), + _ => { + p.runs.push(Run { len: t.len(), props }); + runs += 1; + } + } } + p.normalize(); + p.touch(); out.push(p); } out diff --git a/crates/zotero/src/tests_limits.rs b/crates/zotero/src/tests_limits.rs index 93cefc62c..471d38592 100644 --- a/crates/zotero/src/tests_limits.rs +++ b/crates/zotero/src/tests_limits.rs @@ -50,3 +50,23 @@ fn listing_stops_at_the_field_budget() { doc.body = vec![para_block(objects_para(huge))]; assert_eq!(fields::list(&doc).len(), 4); } + +// Finding 16: rich text with alternating formatting is built in one pass, not run by run with +// every earlier run merged again. +#[test] +fn rich_text_with_many_runs_is_linear() { + let html: String = "ab".repeat(50_000); + let t = Instant::now(); + let frag = crate::rich::fragment(&html, true, &CharProps::default()); + let elapsed = t.elapsed(); + let Some(wordcraft_doc::Block::Para(p)) = frag.blocks.first() else { panic!("no paragraph") }; + assert_eq!(p.text, "ab".repeat(50_000)); + assert_eq!(p.runs.len(), 100_000); + assert_eq!(p.runs.iter().map(|r| r.len).sum::(), p.text.len()); + assert!(elapsed < Duration::from_secs(2), "{elapsed:?}"); + // Neighbours with the same formatting are one run. + let frag = crate::rich::fragment("abc", true, &CharProps::default()); + let Some(wordcraft_doc::Block::Para(p)) = frag.blocks.first() else { panic!("no paragraph") }; + assert_eq!((p.text.as_str(), p.runs.len()), ("abc", 2)); + assert_eq!(p.props_of_char(1).italic, Some(true)); +} From ab26f22c4ac8729c1a72760aafd7c9b1a595f247 Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 19:15:34 +0100 Subject: [PATCH 03/10] #3692: Document_convert checks each distinct field id once against one indexed list Audit finding 32 (CWE-407): Document_convert rebuilt the whole field list for every id it was given, duplicates included, before deciding whether anything had to move, so one valid id repeated 100,000 times took 18 s on the UI thread (now milliseconds). Ids are deduplicated (a repeated id takes its last note type), all of them are checked against one indexed snapshot before anything moves, already-placed citations are skipped from the snapshot, and the index is rebuilt only after a citation actually moves. The id list is capped at fields::MAX_FIELDS, the most fields Zotero is ever handed. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/zotero/src/bridge.rs | 76 +++++++++++++++++++++++++------ crates/zotero/src/tests_limits.rs | 50 +++++++++++++++++++- 2 files changed, 110 insertions(+), 16 deletions(-) diff --git a/crates/zotero/src/bridge.rs b/crates/zotero/src/bridge.rs index b8dfaedac..36cb2d0b4 100644 --- a/crates/zotero/src/bridge.rs +++ b/crates/zotero/src/bridge.rs @@ -5,6 +5,8 @@ //! Field ids handed to Zotero stay valid for the transaction: they are kept in a list parallel //! to the document-order list of Zotero fields, updated as fields are inserted and removed. +use std::collections::HashMap; + use serde_json::{Value, json}; use wordcraft_doc::edit::Fragment; use wordcraft_doc::para::OBJ; @@ -274,21 +276,8 @@ impl Bridge { // citation moves between the text, footnotes and endnotes (switching styles). "Document_convert" => { let Value::Array(ids) = arg(c, 1)? else { return Err("field ids must be an array".into()) }; - let ids: Vec = ids.iter().take(100_000).cloned().collect(); - let notes = c.args.get(3).cloned().unwrap_or(Value::Null); - for (i, idv) in ids.iter().enumerate() { - let id = match idv { - Value::Number(n) => n.as_u64(), - Value::String(x) => x.trim().parse().ok(), - _ => None, - } - .ok_or("bad field id")?; - let note = match ¬es { - Value::Array(a) => a.get(i).and_then(Value::as_i64).unwrap_or(0), - v => v.as_i64().unwrap_or(0), - }; - self.convert_field(s, id, note)?; - } + let notes = c.args.get(3).unwrap_or(&Value::Null); + self.convert_fields(s, ids, notes)?; Ok(Value::Null) } "Field_convert" => { @@ -484,6 +473,49 @@ impl Bridge { Ok(()) } + /// `Document_convert`: each id once (listed again, its last note type wins), every id + /// checked against one indexed list of the fields before anything moves. Only a citation + /// that actually moves costs a fresh list. + fn convert_fields(&mut self, s: &mut Session, ids: &[Value], notes: &Value) -> Result<(), String> { + let mut todo: Vec<(u64, i64)> = Vec::new(); + let mut slot: HashMap = HashMap::new(); + for (i, idv) in ids.iter().take(fields::MAX_FIELDS).enumerate() { + let id = match idv { + Value::Number(n) => n.as_u64(), + Value::String(x) => x.trim().parse().ok(), + _ => None, + } + .ok_or("bad field id")?; + let note = match notes { + Value::Array(a) => a.get(i).and_then(Value::as_i64).unwrap_or(0), + v => v.as_i64().unwrap_or(0), + }; + match slot.get(&id).and_then(|k| todo.get_mut(*k)) { + Some(t) => t.1 = note, + None => { + slot.insert(id, todo.len()); + todo.push((id, note)); + } + } + } + let index = |ids: &[u64]| -> HashMap { ids.iter().enumerate().map(|(k, id)| (*id, k)).collect() }; + let mut list = self.fields(s); + let mut at = index(&self.ids); + if let Some((id, _)) = todo.iter().find(|(id, _)| !at.contains_key(id)) { + return Err(format!("no field with id {id}")); + } + for (id, note) in todo { + let f = at.get(&id).and_then(|k| list.get(*k)).ok_or_else(|| format!("no field with id {id}"))?; + if !moves(s, f, note) { + continue; + } + self.convert_field(s, id, note)?; + list = self.fields(s); + at = index(&self.ids); + } + Ok(()) + } + fn convert_placeholders(&mut self, s: &mut Session, ph: &[Value], note: i64) -> R { let mut ids = Vec::new(); for v in ph { @@ -519,6 +551,20 @@ impl Bridge { } } +/// Whether moving `f` to the text (`note` 0), a footnote (1) or an endnote (2) changes anything, +/// as [`Bridge::convert_field`] decides it. +fn moves(s: &Session, f: &ZField, note: i64) -> bool { + let current = match f.start.story { + StoryRef::Body => 0, + StoryRef::Part(pid) => match s.doc.parts.get(&pid).map(|p| p.kind) { + Some(PartKind::Footnote) => 1, + Some(PartKind::Endnote) => 2, + _ => return false, + }, + }; + current != if matches!(note, 1 | 2) { note } else { 0 } +} + /// Delete the selection; returns where it collapsed. fn delete_selection(s: &mut Session) -> Result { let (a, b) = s.sel.ordered(); diff --git a/crates/zotero/src/tests_limits.rs b/crates/zotero/src/tests_limits.rs index 471d38592..53d8eaafc 100644 --- a/crates/zotero/src/tests_limits.rs +++ b/crates/zotero/src/tests_limits.rs @@ -3,10 +3,13 @@ use std::time::{Duration, Instant}; +use serde_json::{Value, json}; use wordcraft_doc::para::{NoteKind, OBJ}; use wordcraft_doc::{CharProps, Document, InlineObject, Paragraph, PartKind, Run, StoryRef, para_block}; +use wordcraft_engine::Session; -use crate::fields; +use crate::wire::Call; +use crate::{Bridge, Headless, fields}; const CITE: &str = r#"ADDIN ZOTERO_ITEM CSL_CITATION {"citationID":"k1","citationItems":[{"id":7}]}"#; @@ -70,3 +73,48 @@ fn rich_text_with_many_runs_is_linear() { assert_eq!((p.text.as_str(), p.runs.len()), ("abc", 2)); assert_eq!(p.props_of_char(1).italic, Some(true)); } + +/// Call `method` on a bridge over `s`. +fn call(b: &mut Bridge, s: &mut Session, method: &str, args: Value) -> Result { + let Value::Array(args) = args else { return Err("args must be an array".into()) }; + b.handle(s, &mut Headless::default(), &Call::new(method, args)) +} + +// Finding 32: Document_convert checks each distinct id once against one list of the fields. +#[test] +fn convert_with_one_id_repeated_is_one_check() { + let mut doc = Document::from_text(""); + doc.body = (0..300).map(|_| para_block(objects_para(citations(1)))).collect(); + let mut s = Session::new(doc); + let mut b = Bridge::new(); + call(&mut b, &mut s, "Application_getActiveDocument", json!([3])).unwrap(); + let got = call(&mut b, &mut s, "Document_getFields", json!([1, "Field"])).unwrap(); + let first = got[0][0].clone(); + let ids = vec![first.clone(); 100_000]; + let before = format!("{:?}", s.doc.body); + let t = Instant::now(); + call(&mut b, &mut s, "Document_convert", json!([1, ids, 0, 0])).unwrap(); + assert!(t.elapsed() < Duration::from_secs(2), "{:?}", t.elapsed()); + assert_eq!(format!("{:?}", s.doc.body), before, "already in the text: nothing to do"); + // An unknown id is refused before anything moves. + let r = call(&mut b, &mut s, "Document_convert", json!([1, [first.clone(), 999_999_999u64], 0, [1, 1]])); + assert!(r.is_err()); + assert!(s.doc.parts.is_empty(), "nothing converted"); +} + +#[test] +fn convert_with_a_repeated_id_takes_its_last_note_type() { + let mut doc = Document::from_text(""); + doc.body = (0..3).map(|_| para_block(objects_para(citations(1)))).collect(); + let mut s = Session::new(doc); + let mut b = Bridge::new(); + call(&mut b, &mut s, "Application_getActiveDocument", json!([3])).unwrap(); + let got = call(&mut b, &mut s, "Document_getFields", json!([1, "Field"])).unwrap(); + let (a, c) = (got[0][0].clone(), got[0][2].clone()); + call(&mut b, &mut s, "Document_convert", json!([1, [a.clone(), c, a], 0, [2, 1, 1]])).unwrap(); + let kinds: Vec = s.doc.parts.values().map(|p| p.kind).collect(); + assert_eq!(kinds, vec![PartKind::Footnote, PartKind::Footnote]); + // Ids stay valid: the fields are where Zotero now expects them. + let got = call(&mut b, &mut s, "Document_getFields", json!([1, "Field"])).unwrap(); + assert_eq!(got[2], json!([1, 0, 2])); +} From d8d75e44739370102ff53be42d5fdc857b08bc0d Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 19:21:10 +0100 Subject: [PATCH 04/10] #3692: wordcraft-cli zotero escapes peer text on stderr Audit finding 79 (CWE-117): the CLI printed Zotero's alert text (and, with --trace, the method name and call/reply JSON) to stderr as received, so a peer on Zotero's port could end the "Zotero:" line, forge following output or send terminal escape sequences. escape_peer shows backslashes, control characters (C0, DEL, C1), line separators and bidi overrides as visible escapes (\\, \n, \u{1b}), keeping each record on its own attributable line. Co-Authored-By: Claude Opus 5.5 (1M context) --- apps/wordcraft-cli/src/main.rs | 36 +++++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/apps/wordcraft-cli/src/main.rs b/apps/wordcraft-cli/src/main.rs index 752f9f68b..0817d18b3 100644 --- a/apps/wordcraft-cli/src/main.rs +++ b/apps/wordcraft-cli/src/main.rs @@ -657,14 +657,14 @@ fn run>(args: &[S]) -> Result<(), Failure> { let out = wordcraft_zotero::client::run_command(&opts, command, &mut |call| { let r = bridge.handle(&mut s, &mut host, call); if trace { - eprintln!("← {} {}", call.method, clip(&Value::Array(call.args.clone()).to_string())); - eprintln!("→ {}", clip(&String::from_utf8_lossy(&wordcraft_zotero::wire::encode_reply(&r)))); + eprintln!("← {} {}", escape_peer(&call.method), escape_peer(&clip(&Value::Array(call.args.clone()).to_string()))); + eprintln!("→ {}", escape_peer(&clip(&String::from_utf8_lossy(&wordcraft_zotero::wire::encode_reply(&r))))); } r }) .map_err(|e| e.to_string())?; for a in &host.alerts { - eprintln!("Zotero: {a}"); + eprintln!("Zotero: {}", escape_peer(a)); } eprintln!("{} calls, {}", out.calls, if out.completed { "completed" } else { "ended without completing (cancelled?)" }); if let Some(o) = arg_value(&rest, "--save") { @@ -687,6 +687,27 @@ fn clip(s: &str) -> String { } } +/// Text from another program (Zotero, or whatever answered on its port) for one diagnostic +/// line: backslashes, control characters (newline, escape, C1…), line separators and bidi +/// overrides are shown as escapes (`\\`, `\n`, `\u{1b}`), so it can't end its line, drive the +/// terminal or pass for other output. +fn escape_peer(s: &str) -> String { + let mut out = String::with_capacity(s.len()); + for c in s.chars() { + match c { + '\\' => out.push_str("\\\\"), + '\n' => out.push_str("\\n"), + '\r' => out.push_str("\\r"), + '\t' => out.push_str("\\t"), + c if c.is_control() || matches!(c, '\u{061c}' | '\u{200e}' | '\u{200f}' | '\u{2028}'..='\u{202e}' | '\u{2066}'..='\u{2069}') => { + out.extend(c.escape_unicode()) + } + c => out.push(c), + } + } + out +} + fn parity_markdown(p: &Value) -> String { let mut s = String::from( "# WordCraft feature parity\n\nGenerated by `cargo xtask parity` (`wordcraft-cli parity --markdown`): the word-processor feature catalog (`crates/engine/src/catalog.rs`) compared with the live command registry.\n\n", @@ -733,6 +754,15 @@ mod tests { values.iter().map(|value| (*value).into()).collect() } + #[test] + fn peer_text_stays_on_one_visible_line() { + let alert = "Done.\nwrote /etc/passwd\r\u{1b}[2J\u{9b}31m\u{202e}txt.exe\\n\tok é"; + let shown = escape_peer(alert); + assert_eq!(shown, "Done.\\nwrote /etc/passwd\\r\\u{1b}[2J\\u{9b}31m\\u{202e}txt.exe\\\\n\\tok é"); + assert!(!shown.chars().any(|c| c.is_control())); + assert_eq!(escape_peer("Citation (Doe, 2020) — “ok”"), "Citation (Doe, 2020) — “ok”"); + } + #[test] fn validators_accept_only_the_documented_options() { assert!(validate_options("info", &args(&["file.docx"]), PASSWORD_OPTIONS).is_ok()); From a9cc06dc0f326f502a29598114a0e8b3c51ace96 Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 19:27:19 +0100 Subject: [PATCH 05/10] #3692: Zotero client talks only to this computer and, on Linux, only to our own user Audit finding 21 (CWE-306): run_command handed document callback authority to whatever accepted the TCP connection. Zotero's protocol is fixed and has no authentication, so WordCraft now establishes what it can about the peer itself: - the address must be loopback, refused before connecting (ZoteroError::Refused); - on Linux/Android, before the command is sent, the kernel socket table (/proc/net/tcp, tcp6) must show the listener's end of this connection owned by the same UID as ours; another user's listener, or one whose owner can't be established, is refused. No unsafe, no new dependencies. macOS, Windows and the BSDs have no such table without OS calls; there the loopback rule is the guard. The residual risk is documented in docs/zotero.md. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/zotero/src/client.rs | 3 + crates/zotero/src/lib.rs | 5 + crates/zotero/src/peer.rs | 191 ++++++++++++++++++++++++++++++++++++ docs/zotero.md | 21 ++++ 4 files changed, 220 insertions(+) create mode 100644 crates/zotero/src/peer.rs diff --git a/crates/zotero/src/client.rs b/crates/zotero/src/client.rs index bb6fc98b9..e7a73354e 100644 --- a/crates/zotero/src/client.rs +++ b/crates/zotero/src/client.rs @@ -88,7 +88,10 @@ pub struct Outcome { /// Run `command`, answering every call with `answer`. pub fn run_command(opts: &Options, command: Command, answer: &mut dyn FnMut(&Call) -> Result) -> Result { + crate::peer::check_addr(&opts.addr)?; let mut stream = TcpStream::connect_timeout(&opts.addr, opts.connect_timeout).map_err(|e| ZoteroError::NotRunning(e.to_string()))?; + // Before Zotero sees anything: the command, and then the document, go only to our own user. + crate::peer::check(&stream)?; let _ = stream.set_nodelay(true); stream.set_read_timeout(Some(opts.idle_timeout)).map_err(|e| ZoteroError::Io(e.to_string()))?; wire::write_frame(&mut stream, 0, &wire::command_payload(command.wire_name()))?; diff --git a/crates/zotero/src/lib.rs b/crates/zotero/src/lib.rs index 4c50bab04..7dc26eb8e 100644 --- a/crates/zotero/src/lib.rs +++ b/crates/zotero/src/lib.rs @@ -18,6 +18,7 @@ mod bridge; pub mod client; pub mod fields; +mod peer; mod rich; pub mod wire; @@ -37,6 +38,10 @@ pub enum ZoteroError { /// Zotero sent something that isn't the protocol. #[error("unexpected data from Zotero: {0}")] Protocol(String), + /// What answered on Zotero's port may not be Zotero (another user's program, or another + /// computer); it gets no access to the document. + #[error("WordCraft won't let this program edit the document: {0}")] + Refused(String), } #[cfg(test)] diff --git a/crates/zotero/src/peer.rs b/crates/zotero/src/peer.rs new file mode 100644 index 000000000..00173c5c4 --- /dev/null +++ b/crates/zotero/src/peer.rs @@ -0,0 +1,191 @@ +//! Who answered on Zotero's port. Zotero's protocol has no handshake that proves the other end +//! is Zotero, and whatever answers gets to read and rewrite the open document. So WordCraft +//! only connects to this computer (a loopback address), and on Linux checks that the program +//! listening there runs as the same user, from the kernel's socket table (`/proc/net/tcp`): +//! another account on a shared machine that took the port while Zotero wasn't running is +//! refused. Other systems have no such table to read without OS calls this crate doesn't make; +//! there the loopback rule is the guard (see `docs/zotero.md`). + +use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr, TcpStream}; + +use crate::ZoteroError; + +/// Whether `addr` is on this computer; anything else is refused before connecting. +pub(crate) fn check_addr(addr: &SocketAddr) -> Result<(), ZoteroError> { + if addr.ip().to_canonical().is_loopback() { + Ok(()) + } else { + Err(ZoteroError::Refused(format!("{addr} is not on this computer; WordCraft only talks to Zotero on 127.0.0.1"))) + } +} + +/// Refuse the program at the other end of `stream` when it may not be ours (see the module +/// documentation). +pub(crate) fn check(stream: &TcpStream) -> Result<(), ZoteroError> { + let io = |e: std::io::Error| ZoteroError::Io(e.to_string()); + let (local, peer) = (stream.local_addr().map_err(io)?, stream.peer_addr().map_err(io)?); + check_addr(&peer)?; + owner_check(local, peer) +} + +#[cfg(any(target_os = "linux", target_os = "android"))] +fn owner_check(local: SocketAddr, peer: SocketAddr) -> Result<(), ZoteroError> { + use std::io::Read; + /// Longest socket table read (a busy server's table is a few MB). + const MAX_TABLE: u64 = 64 * 1024 * 1024; + let mut table = String::new(); + for path in ["/proc/net/tcp", "/proc/net/tcp6"] { + if let Ok(f) = std::fs::File::open(path) { + let mut part = String::new(); + if f.take(MAX_TABLE).read_to_string(&mut part).is_ok() { + table.push_str(&part); + } + } + } + same_owner(&table, local, peer) +} + +#[cfg(not(any(target_os = "linux", target_os = "android")))] +fn owner_check(_local: SocketAddr, _peer: SocketAddr) -> Result<(), ZoteroError> { + Ok(()) +} + +/// From a `/proc/net/tcp`-format socket table: whether the socket at `peer` connected to +/// `local` (the listener's side) belongs to the same user as ours. A peer that can't be found +/// is refused too: its owner can't be established. +#[cfg_attr(not(any(target_os = "linux", target_os = "android")), allow(dead_code))] +pub(crate) fn same_owner(table: &str, local: SocketAddr, peer: SocketAddr) -> Result<(), ZoteroError> { + let key = |a: SocketAddr| (a.ip().to_canonical(), a.port()); + let (local, peer) = (key(local), key(peer)); + let (mut ours, mut theirs) = (None, None); + for line in table.lines() { + let f: Vec<&str> = line.split_whitespace().take(8).collect(); + let (Some(l), Some(r), Some(uid)) = (f.get(1).and_then(|a| parse_addr(a)), f.get(2).and_then(|a| parse_addr(a)), f.get(7)) else { + continue; + }; + let Ok(uid) = uid.parse::() else { continue }; + let (l, r) = (key(l), key(r)); + if l == local && r == peer { + ours = Some(uid); + } else if l == peer && r == local { + theirs = Some(uid); + } + } + match (ours, theirs) { + (Some(a), Some(b)) if a == b => Ok(()), + (Some(_), Some(b)) => { + Err(ZoteroError::Refused(format!("the program on port {} runs as another user (uid {b}), so it isn't your Zotero", peer.1))) + } + _ => Err(ZoteroError::Refused(format!("WordCraft couldn't tell which user's program is on port {}", peer.1))), + } +} + +/// `0100007F:5A5C` (IPv4) or 32 hex digits and a port (IPv6): each 32-bit word as the kernel +/// prints it, in this machine's byte order. +#[cfg_attr(not(any(target_os = "linux", target_os = "android")), allow(dead_code))] +fn parse_addr(s: &str) -> Option { + let (ip, port) = s.split_once(':')?; + let port = u16::from_str_radix(port, 16).ok()?; + let word = |k: usize| ip.get(k * 8..k * 8 + 8).and_then(|w| u32::from_str_radix(w, 16).ok()).map(u32::to_ne_bytes); + let ip = match ip.len() { + 8 => IpAddr::V4(Ipv4Addr::from(word(0)?)), + 32 => { + let mut b = [0u8; 16]; + for (k, w) in b.as_chunks_mut::<4>().0.iter_mut().enumerate() { + *w = word(k)?; + } + IpAddr::V6(Ipv6Addr::from(b)) + } + _ => return None, + }; + Some(SocketAddr::new(ip, port)) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// `a` the way the kernel's socket table prints it. + fn hex(a: SocketAddr) -> String { + let ip = match a.ip() { + IpAddr::V4(v4) => format!("{:08X}", u32::from_ne_bytes(v4.octets())), + IpAddr::V6(v6) => v6.octets().as_chunks::<4>().0.iter().map(|w| format!("{:08X}", u32::from_ne_bytes(*w))).collect(), + }; + format!("{ip}:{:04X}", a.port()) + } + + fn row(l: SocketAddr, r: SocketAddr, uid: u32) -> String { + format!(" 0: {} {} 01 00000000:00000000 00:00000000 00000000 {uid:>4} 0 12345 1 0000000000000000 20 4 30 10 -1\n", hex(l), hex(r)) + } + + const HEADER: &str = " sl local_address rem_address st tx_queue rx_queue tr tm->when retrnsmt uid timeout inode\n"; + + fn addrs() -> (SocketAddr, SocketAddr) { + ("127.0.0.1:41234".parse().unwrap(), "127.0.0.1:23116".parse().unwrap()) + } + + #[test] + fn addresses_parse_as_the_kernel_prints_them() { + for a in ["127.0.0.1:23116", "[::1]:23116", "[::ffff:127.0.0.1]:80"] { + let a: SocketAddr = a.parse().unwrap(); + assert_eq!(parse_addr(&hex(a)), Some(a)); + } + for bad in ["", "0100007F", "0100007F:", "XYZ00000:0050", "0100007F:12345", "0100:0050"] { + assert_eq!(parse_addr(bad), None, "{bad}"); + } + } + + #[test] + fn a_listener_of_another_user_is_refused() { + let (local, peer) = addrs(); + let same = format!("{HEADER}{}{}", row(local, peer, 1000), row(peer, local, 1000)); + assert_eq!(same_owner(&same, local, peer), Ok(())); + let other = format!("{HEADER}{}{}", row(local, peer, 1000), row(peer, local, 1001)); + assert!(matches!(same_owner(&other, local, peer), Err(ZoteroError::Refused(e)) if e.contains("another user"))); + // Not found (or the table couldn't be read): the owner can't be established. + let missing = format!("{HEADER}{}", row(local, peer, 1000)); + assert!(matches!(same_owner(&missing, local, peer), Err(ZoteroError::Refused(_)))); + assert!(matches!(same_owner("", local, peer), Err(ZoteroError::Refused(_)))); + // Junk lines are skipped. + let junk = format!("{HEADER}garbage\n 1: zz:zz yy:yy 01 x x x x\n{}{}", row(local, peer, 7), row(peer, local, 7)); + assert_eq!(same_owner(&junk, local, peer), Ok(())); + } + + #[test] + fn a_dual_stack_listener_is_matched_through_its_mapped_address() { + let (local, peer) = addrs(); + let mapped = |a: SocketAddr| match a.ip() { + IpAddr::V4(v4) => SocketAddr::new(IpAddr::V6(v4.to_ipv6_mapped()), a.port()), + IpAddr::V6(_) => a, + }; + let table = format!("{HEADER}{}{}", row(local, peer, 1000), row(mapped(peer), mapped(local), 1001)); + assert!(matches!(same_owner(&table, local, peer), Err(ZoteroError::Refused(e)) if e.contains("another user"))); + } + + #[test] + fn only_this_computer_is_contacted() { + for ok in ["127.0.0.1:23116", "127.9.9.9:1", "[::1]:23116", "[::ffff:127.0.0.1]:23116"] { + assert_eq!(check_addr(&ok.parse().unwrap()), Ok(()), "{ok}"); + } + for bad in ["192.0.2.1:23116", "0.0.0.0:23116", "[2001:db8::1]:23116", "[::]:23116"] { + assert!(matches!(check_addr(&bad.parse().unwrap()), Err(ZoteroError::Refused(_))), "{bad}"); + } + } + + #[test] + fn another_computer_is_refused_before_connecting() { + let opts = crate::client::Options { addr: "192.0.2.1:23116".parse().unwrap(), ..Default::default() }; + let r = crate::client::run_command(&opts, crate::Command::Refresh, &mut |_| Ok(serde_json::Value::Null)); + assert!(matches!(r, Err(ZoteroError::Refused(_))), "{r:?}"); + } + + /// A listener this process opened runs as us: the real socket table agrees. + #[cfg(any(target_os = "linux", target_os = "android"))] + #[test] + fn our_own_listener_passes_the_owner_check() { + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let stream = TcpStream::connect(listener.local_addr().unwrap()).unwrap(); + let _accepted = listener.accept().unwrap(); + assert_eq!(check(&stream), Ok(())); + } +} diff --git a/docs/zotero.md b/docs/zotero.md index f05b62c6e..ed486b4ff 100644 --- a/docs/zotero.md +++ b/docs/zotero.md @@ -61,6 +61,27 @@ Word's "Field" in the file.) - Zotero's document transfer (Export/Import Document) isn't needed and isn't supported. - The web version can't reach Zotero (browsers can't open the socket). +## Who can drive the document + +Zotero's protocol has no handshake that proves the program on port 23116 is Zotero, and that +program can read the citations and edit the open document. WordCraft limits who that can be: + +- It only connects to this computer (a loopback address such as `127.0.0.1`); any other address + is refused before connecting. +- On Linux it checks, in the kernel's socket table (`/proc/net/tcp`, `/proc/net/tcp6`), that the + program listening on the port runs as the same user as WordCraft, before sending anything. If + another account on a shared machine took the port while Zotero wasn't running, or the owner + can't be established (the table can't be read), WordCraft refuses with an error and the + document isn't touched. +- Remaining risk: macOS, Windows and the BSDs have no such table to read without OS calls + WordCraft doesn't make (it uses no `unsafe` code), so there only the loopback rule applies: + on a computer shared with other accounts, a program another user leaves on port 23116 while + Zotero is closed would be taken for Zotero. A program running as you is trusted on every + system; it could change your documents anyway. +- `wordcraft-cli zotero` prints Zotero's messages (and `--trace` lines) with control + characters, line breaks and backslashes shown as escapes (`\n`, `\u{1b}`, `\\`), so a peer + can't forge other output or drive the terminal. + ## How it works `crates/zotero` implements Zotero's published word-processor wire protocol (port 23116): From 7ee101a5b99f2f5e25b31a3aecf80d609b63da2b Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 22:05:54 +0100 Subject: [PATCH 06/10] #3692: Zotero owner check refuses only another user's listener; unlisted falls back to loopback Ultrareview finding 3: on Linux, when /proc/net/tcp(6) doesn't list the listener on Zotero's port (WSL2 mirrored networking with Zotero on Windows, a local proxy, a sandbox), every connection was refused, so Zotero couldn't be used at all there. Per the owner's decision, refuse only when both sockets are found and the listener's uid differs from ours; otherwise log at debug and apply the plain loopback rule, as on macOS/Windows/BSD. Test: an_unlisted_listener_falls_back_to_the_loopback_rule. docs/zotero.md and the progress-log row say so. Non-Linux owner check stays with #3831. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/zotero/src/peer.rs | 31 +++++++++++++++++++++---------- docs/zotero.md | 14 ++++++++------ 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/crates/zotero/src/peer.rs b/crates/zotero/src/peer.rs index 00173c5c4..a9c2ccbe4 100644 --- a/crates/zotero/src/peer.rs +++ b/crates/zotero/src/peer.rs @@ -3,8 +3,9 @@ //! only connects to this computer (a loopback address), and on Linux checks that the program //! listening there runs as the same user, from the kernel's socket table (`/proc/net/tcp`): //! another account on a shared machine that took the port while Zotero wasn't running is -//! refused. Other systems have no such table to read without OS calls this crate doesn't make; -//! there the loopback rule is the guard (see `docs/zotero.md`). +//! refused. A listener the table doesn't show (WSL2 mirrored networking with Zotero on Windows, +//! a local proxy, a sandbox) leaves only the loopback rule, as on other systems, which have no +//! such table to read without OS calls this crate doesn't make (see `docs/zotero.md`). use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr, TcpStream}; @@ -50,9 +51,9 @@ fn owner_check(_local: SocketAddr, _peer: SocketAddr) -> Result<(), ZoteroError> Ok(()) } -/// From a `/proc/net/tcp`-format socket table: whether the socket at `peer` connected to -/// `local` (the listener's side) belongs to the same user as ours. A peer that can't be found -/// is refused too: its owner can't be established. +/// From a `/proc/net/tcp`-format socket table: refuse when the socket at `peer` connected to +/// `local` (the listener's side) belongs to another user than ours. When either socket isn't +/// listed (or the table couldn't be read) the owner is unknown and the loopback rule decides. #[cfg_attr(not(any(target_os = "linux", target_os = "android")), allow(dead_code))] pub(crate) fn same_owner(table: &str, local: SocketAddr, peer: SocketAddr) -> Result<(), ZoteroError> { let key = |a: SocketAddr| (a.ip().to_canonical(), a.port()); @@ -76,7 +77,10 @@ pub(crate) fn same_owner(table: &str, local: SocketAddr, peer: SocketAddr) -> Re (Some(_), Some(b)) => { Err(ZoteroError::Refused(format!("the program on port {} runs as another user (uid {b}), so it isn't your Zotero", peer.1))) } - _ => Err(ZoteroError::Refused(format!("WordCraft couldn't tell which user's program is on port {}", peer.1))), + _ => { + log::debug!("Zotero: port {} isn't in this system's socket table; trusting the loopback address alone", peer.1); + Ok(()) + } } } @@ -142,15 +146,22 @@ mod tests { assert_eq!(same_owner(&same, local, peer), Ok(())); let other = format!("{HEADER}{}{}", row(local, peer, 1000), row(peer, local, 1001)); assert!(matches!(same_owner(&other, local, peer), Err(ZoteroError::Refused(e)) if e.contains("another user"))); - // Not found (or the table couldn't be read): the owner can't be established. - let missing = format!("{HEADER}{}", row(local, peer, 1000)); - assert!(matches!(same_owner(&missing, local, peer), Err(ZoteroError::Refused(_)))); - assert!(matches!(same_owner("", local, peer), Err(ZoteroError::Refused(_)))); // Junk lines are skipped. let junk = format!("{HEADER}garbage\n 1: zz:zz yy:yy 01 x x x x\n{}{}", row(local, peer, 7), row(peer, local, 7)); assert_eq!(same_owner(&junk, local, peer), Ok(())); } + /// WSL2 mirrored networking (Zotero on Windows), a local proxy or a sandbox: the listener + /// isn't in our table, so the loopback rule decides, as on other systems. + #[test] + fn an_unlisted_listener_falls_back_to_the_loopback_rule() { + let (local, peer) = addrs(); + let missing = format!("{HEADER}{}", row(local, peer, 1000)); + assert_eq!(same_owner(&missing, local, peer), Ok(())); + // The table couldn't be read. + assert_eq!(same_owner("", local, peer), Ok(())); + } + #[test] fn a_dual_stack_listener_is_matched_through_its_mapped_address() { let (local, peer) = addrs(); diff --git a/docs/zotero.md b/docs/zotero.md index ed486b4ff..601749d59 100644 --- a/docs/zotero.md +++ b/docs/zotero.md @@ -70,13 +70,15 @@ program can read the citations and edit the open document. WordCraft limits who is refused before connecting. - On Linux it checks, in the kernel's socket table (`/proc/net/tcp`, `/proc/net/tcp6`), that the program listening on the port runs as the same user as WordCraft, before sending anything. If - another account on a shared machine took the port while Zotero wasn't running, or the owner - can't be established (the table can't be read), WordCraft refuses with an error and the - document isn't touched. + another account on a shared machine took the port while Zotero wasn't running, WordCraft + refuses with an error and the document isn't touched. When the table doesn't show the + listener (WSL2 mirrored networking with Zotero running on Windows, a local proxy, a sandbox) + or can't be read, its owner is unknown and only the loopback rule applies, as below. - Remaining risk: macOS, Windows and the BSDs have no such table to read without OS calls - WordCraft doesn't make (it uses no `unsafe` code), so there only the loopback rule applies: - on a computer shared with other accounts, a program another user leaves on port 23116 while - Zotero is closed would be taken for Zotero. A program running as you is trusted on every + WordCraft doesn't make (it uses no `unsafe` code), so there only the loopback rule applies + (and on Linux when the listener isn't in the table): on a computer shared with other + accounts, a program another user leaves on port 23116 while Zotero is closed would be taken + for Zotero. A program running as you is trusted on every system; it could change your documents anyway. - `wordcraft-cli zotero` prints Zotero's messages (and `--trace` lines) with control characters, line breaks and backslashes shown as escapes (`\n`, `\u{1b}`, `\\`), so a peer From 65f3c3418f494dd64e47157ac5e27ca5e0a7c878 Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Thu, 22 Oct 2026 22:27:01 +0100 Subject: [PATCH 07/10] #3692: Zotero owner check falls back to this process's uid when our socket row is missing Ultrareview finding 3 follow-up (owner's call): with our own socket's row missing from /proc/net/tcp(6), a listed listener of another user was let through on the loopback rule. Our uid now falls back to the owner of /proc/self, so a listed listener with a different uid is refused either way. An unlisted listener still falls back to the loopback rule. Test: a_listed_listener_is_checked_against_our_process_uid. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/zotero/src/peer.rs | 46 +++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/crates/zotero/src/peer.rs b/crates/zotero/src/peer.rs index a9c2ccbe4..e808f03d2 100644 --- a/crates/zotero/src/peer.rs +++ b/crates/zotero/src/peer.rs @@ -43,7 +43,9 @@ fn owner_check(local: SocketAddr, peer: SocketAddr) -> Result<(), ZoteroError> { } } } - same_owner(&table, local, peer) + // Our uid when our own socket's row is missing: `/proc/self` belongs to this process's user. + let me = std::fs::metadata("/proc/self").ok().map(|m| std::os::unix::fs::MetadataExt::uid(&m)); + same_owner(&table, local, peer, me) } #[cfg(not(any(target_os = "linux", target_os = "android")))] @@ -52,10 +54,11 @@ fn owner_check(_local: SocketAddr, _peer: SocketAddr) -> Result<(), ZoteroError> } /// From a `/proc/net/tcp`-format socket table: refuse when the socket at `peer` connected to -/// `local` (the listener's side) belongs to another user than ours. When either socket isn't -/// listed (or the table couldn't be read) the owner is unknown and the loopback rule decides. +/// `local` (the listener's side) belongs to another user than ours. Our uid is our own socket's +/// row, else `me` (this process's). When the listener isn't listed (or the table couldn't be +/// read, or our uid is unknown) its owner is unknown and the loopback rule decides. #[cfg_attr(not(any(target_os = "linux", target_os = "android")), allow(dead_code))] -pub(crate) fn same_owner(table: &str, local: SocketAddr, peer: SocketAddr) -> Result<(), ZoteroError> { +pub(crate) fn same_owner(table: &str, local: SocketAddr, peer: SocketAddr, me: Option) -> Result<(), ZoteroError> { let key = |a: SocketAddr| (a.ip().to_canonical(), a.port()); let (local, peer) = (key(local), key(peer)); let (mut ours, mut theirs) = (None, None); @@ -72,13 +75,16 @@ pub(crate) fn same_owner(table: &str, local: SocketAddr, peer: SocketAddr) -> Re theirs = Some(uid); } } - match (ours, theirs) { + match (ours.or(me), theirs) { (Some(a), Some(b)) if a == b => Ok(()), (Some(_), Some(b)) => { Err(ZoteroError::Refused(format!("the program on port {} runs as another user (uid {b}), so it isn't your Zotero", peer.1))) } _ => { - log::debug!("Zotero: port {} isn't in this system's socket table; trusting the loopback address alone", peer.1); + log::debug!( + "Zotero: couldn't tell which user runs the program on port {} (not in the socket table); trusting the loopback address alone", + peer.1 + ); Ok(()) } } @@ -143,12 +149,12 @@ mod tests { fn a_listener_of_another_user_is_refused() { let (local, peer) = addrs(); let same = format!("{HEADER}{}{}", row(local, peer, 1000), row(peer, local, 1000)); - assert_eq!(same_owner(&same, local, peer), Ok(())); + assert_eq!(same_owner(&same, local, peer, None), Ok(())); let other = format!("{HEADER}{}{}", row(local, peer, 1000), row(peer, local, 1001)); - assert!(matches!(same_owner(&other, local, peer), Err(ZoteroError::Refused(e)) if e.contains("another user"))); + assert!(matches!(same_owner(&other, local, peer, None), Err(ZoteroError::Refused(e)) if e.contains("another user"))); // Junk lines are skipped. let junk = format!("{HEADER}garbage\n 1: zz:zz yy:yy 01 x x x x\n{}{}", row(local, peer, 7), row(peer, local, 7)); - assert_eq!(same_owner(&junk, local, peer), Ok(())); + assert_eq!(same_owner(&junk, local, peer, None), Ok(())); } /// WSL2 mirrored networking (Zotero on Windows), a local proxy or a sandbox: the listener @@ -157,9 +163,25 @@ mod tests { fn an_unlisted_listener_falls_back_to_the_loopback_rule() { let (local, peer) = addrs(); let missing = format!("{HEADER}{}", row(local, peer, 1000)); - assert_eq!(same_owner(&missing, local, peer), Ok(())); + assert_eq!(same_owner(&missing, local, peer, None), Ok(())); // The table couldn't be read. - assert_eq!(same_owner("", local, peer), Ok(())); + assert_eq!(same_owner("", local, peer, None), Ok(())); + assert_eq!(same_owner("", local, peer, Some(1000)), Ok(())); + } + + /// Our own socket's row missing doesn't hide another user's listener: our uid comes from + /// the process itself. + #[test] + fn a_listed_listener_is_checked_against_our_process_uid() { + let (local, peer) = addrs(); + let only_theirs = format!("{HEADER}{}", row(peer, local, 1001)); + assert!(matches!(same_owner(&only_theirs, local, peer, Some(1000)), Err(ZoteroError::Refused(e)) if e.contains("another user"))); + assert_eq!(same_owner(&only_theirs, local, peer, Some(1001)), Ok(())); + // Neither our row nor our process uid: unknown, so the loopback rule decides. + assert_eq!(same_owner(&only_theirs, local, peer, None), Ok(())); + // Our socket's row is ours by definition and wins over the process uid. + let both = format!("{HEADER}{}{}", row(local, peer, 1001), row(peer, local, 1001)); + assert_eq!(same_owner(&both, local, peer, Some(1000)), Ok(())); } #[test] @@ -170,7 +192,7 @@ mod tests { IpAddr::V6(_) => a, }; let table = format!("{HEADER}{}{}", row(local, peer, 1000), row(mapped(peer), mapped(local), 1001)); - assert!(matches!(same_owner(&table, local, peer), Err(ZoteroError::Refused(e)) if e.contains("another user"))); + assert!(matches!(same_owner(&table, local, peer, None), Err(ZoteroError::Refused(e)) if e.contains("another user"))); } #[test] From e251c960d9745eb791452543129b3fa173a1279e Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Fri, 23 Oct 2026 01:57:00 +0100 Subject: [PATCH 08/10] =?UTF-8?q?#3692:=20review=20=E2=80=94=20doc=20comme?= =?UTF-8?q?nts=20on=20the=20functions=20the=20Zotero=20hardening=20added?= =?UTF-8?q?=20or=20changed?= 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 --- apps/wordcraft-cli/src/main.rs | 2 ++ crates/zotero/src/bridge.rs | 1 + crates/zotero/src/peer.rs | 4 ++++ crates/zotero/src/rich.rs | 3 +++ 4 files changed, 10 insertions(+) diff --git a/apps/wordcraft-cli/src/main.rs b/apps/wordcraft-cli/src/main.rs index 0817d18b3..c8e46c93c 100644 --- a/apps/wordcraft-cli/src/main.rs +++ b/apps/wordcraft-cli/src/main.rs @@ -387,6 +387,8 @@ fn valid_connect_addr(addr: &str) -> bool { !host.is_empty() && port.parse::().is_ok() } +/// Run the subcommand named by `args` (the command line after the program name). Files named +/// there are opened by their real names, which may not be valid Unicode. fn run>(args: &[S]) -> Result<(), Failure> { let script_path = script_operand(args); let (args, file) = text_args(args)?; diff --git a/crates/zotero/src/bridge.rs b/crates/zotero/src/bridge.rs index 36cb2d0b4..50ea5fcd5 100644 --- a/crates/zotero/src/bridge.rs +++ b/crates/zotero/src/bridge.rs @@ -194,6 +194,7 @@ impl Bridge { } } + /// Answer one call by its method name; [`Bridge::handle`] wraps it so a panic becomes an error. fn dispatch(&mut self, s: &mut Session, host: &mut dyn Host, c: &Call) -> R { match c.method.as_str() { "Application_getActiveDocument" => { diff --git a/crates/zotero/src/peer.rs b/crates/zotero/src/peer.rs index e808f03d2..8e6ea6c4e 100644 --- a/crates/zotero/src/peer.rs +++ b/crates/zotero/src/peer.rs @@ -29,6 +29,7 @@ pub(crate) fn check(stream: &TcpStream) -> Result<(), ZoteroError> { owner_check(local, peer) } +/// Refuse a listener at `peer` that the kernel's socket tables say belongs to another user. #[cfg(any(target_os = "linux", target_os = "android"))] fn owner_check(local: SocketAddr, peer: SocketAddr) -> Result<(), ZoteroError> { use std::io::Read; @@ -48,6 +49,7 @@ fn owner_check(local: SocketAddr, peer: SocketAddr) -> Result<(), ZoteroError> { same_owner(&table, local, peer, me) } +/// No socket table to read here: the loopback rule alone decides. #[cfg(not(any(target_os = "linux", target_os = "android")))] fn owner_check(_local: SocketAddr, _peer: SocketAddr) -> Result<(), ZoteroError> { Ok(()) @@ -124,12 +126,14 @@ mod tests { format!("{ip}:{:04X}", a.port()) } + /// A socket table row for the connection from `l` to `r`, owned by `uid`. fn row(l: SocketAddr, r: SocketAddr, uid: u32) -> String { format!(" 0: {} {} 01 00000000:00000000 00:00000000 00000000 {uid:>4} 0 12345 1 0000000000000000 20 4 30 10 -1\n", hex(l), hex(r)) } const HEADER: &str = " sl local_address rem_address st tx_queue rx_queue tr tm->when retrnsmt uid timeout inode\n"; + /// Our side and Zotero's listening side of a loopback connection. fn addrs() -> (SocketAddr, SocketAddr) { ("127.0.0.1:41234".parse().unwrap(), "127.0.0.1:23116".parse().unwrap()) } diff --git a/crates/zotero/src/rich.rs b/crates/zotero/src/rich.rs index 6285ecf3e..7ff3377f1 100644 --- a/crates/zotero/src/rich.rs +++ b/crates/zotero/src/rich.rs @@ -40,6 +40,9 @@ fn style_props(base: &CharProps, src: &CharProps) -> CharProps { p } +/// The body paragraphs of `d` (Zotero's RTF or HTML, read), at most [`MAX_PARAS`], formatted on +/// top of `base` with the run styling they carry (past [`MAX_RUNS`] runs, text keeps the +/// formatting before it). Objects are dropped. fn from_doc(d: &Document, base: &CharProps) -> Vec { let mut out = Vec::new(); let mut runs = 0usize; From bf2a379ec6fd0b08144f79d343c1a4ede6be72db Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Fri, 23 Oct 2026 11:13:33 +0100 Subject: [PATCH 09/10] =?UTF-8?q?#3692:=20review=20=E2=80=94=20say=20plain?= =?UTF-8?q?ly=20the=20same-user=20Zotero=20check=20is=20Linux=20only;=20Wi?= =?UTF-8?q?ndows=20and=20macOS=20rely=20on=20loopback=20alone?= 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/zotero/src/client.rs | 3 ++- crates/zotero/src/lib.rs | 4 ++-- crates/zotero/src/peer.rs | 22 ++++++++++++++-------- docs/zotero.md | 23 +++++++++++++---------- 4 files changed, 31 insertions(+), 21 deletions(-) diff --git a/crates/zotero/src/client.rs b/crates/zotero/src/client.rs index e7a73354e..f6e31c83a 100644 --- a/crates/zotero/src/client.rs +++ b/crates/zotero/src/client.rs @@ -90,7 +90,8 @@ pub struct Outcome { pub fn run_command(opts: &Options, command: Command, answer: &mut dyn FnMut(&Call) -> Result) -> Result { crate::peer::check_addr(&opts.addr)?; let mut stream = TcpStream::connect_timeout(&opts.addr, opts.connect_timeout).map_err(|e| ZoteroError::NotRunning(e.to_string()))?; - // Before Zotero sees anything: the command, and then the document, go only to our own user. + // Before Zotero sees anything: the command, and then the document, go only to this computer + // and, on Linux only, not to a listener the socket table shows belongs to another user. crate::peer::check(&stream)?; let _ = stream.set_nodelay(true); stream.set_read_timeout(Some(opts.idle_timeout)).map_err(|e| ZoteroError::Io(e.to_string()))?; diff --git a/crates/zotero/src/lib.rs b/crates/zotero/src/lib.rs index 7dc26eb8e..cf4c93443 100644 --- a/crates/zotero/src/lib.rs +++ b/crates/zotero/src/lib.rs @@ -38,8 +38,8 @@ pub enum ZoteroError { /// Zotero sent something that isn't the protocol. #[error("unexpected data from Zotero: {0}")] Protocol(String), - /// What answered on Zotero's port may not be Zotero (another user's program, or another - /// computer); it gets no access to the document. + /// What answered on Zotero's port may not be Zotero (another computer, on any system; on + /// Linux only, another user's program); it gets no access to the document. #[error("WordCraft won't let this program edit the document: {0}")] Refused(String), } diff --git a/crates/zotero/src/peer.rs b/crates/zotero/src/peer.rs index 8e6ea6c4e..07df98e79 100644 --- a/crates/zotero/src/peer.rs +++ b/crates/zotero/src/peer.rs @@ -1,11 +1,16 @@ //! Who answered on Zotero's port. Zotero's protocol has no handshake that proves the other end -//! is Zotero, and whatever answers gets to read and rewrite the open document. So WordCraft -//! only connects to this computer (a loopback address), and on Linux checks that the program -//! listening there runs as the same user, from the kernel's socket table (`/proc/net/tcp`): -//! another account on a shared machine that took the port while Zotero wasn't running is -//! refused. A listener the table doesn't show (WSL2 mirrored networking with Zotero on Windows, -//! a local proxy, a sandbox) leaves only the loopback rule, as on other systems, which have no -//! such table to read without OS calls this crate doesn't make (see `docs/zotero.md`). +//! is Zotero, and whatever answers gets to read and rewrite the open document. Two checks: +//! +//! - **Every system:** WordCraft only connects to this computer (a loopback address). +//! - **Linux only:** the program listening there must not run as another user, from the +//! kernel's socket table (`/proc/net/tcp`): another account on a shared machine that took the +//! port while Zotero wasn't running is refused. A listener the table doesn't show (WSL2 +//! mirrored networking with Zotero on Windows, a local proxy, a sandbox) leaves only the +//! loopback rule. +//! +//! On Windows, macOS and the BSDs there is **no same-user check**: the loopback rule is the +//! only one, so any local account's listener on the port is taken for Zotero. Those systems have +//! no such table to read without OS calls this crate doesn't make (see `docs/zotero.md`, #3831). use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr, TcpStream}; @@ -49,7 +54,8 @@ fn owner_check(local: SocketAddr, peer: SocketAddr) -> Result<(), ZoteroError> { same_owner(&table, local, peer, me) } -/// No socket table to read here: the loopback rule alone decides. +/// Windows, macOS, the BSDs: no same-user check (no socket table to read without OS calls this +/// crate doesn't make, #3831); the loopback rule alone decides. #[cfg(not(any(target_os = "linux", target_os = "android")))] fn owner_check(_local: SocketAddr, _peer: SocketAddr) -> Result<(), ZoteroError> { Ok(()) diff --git a/docs/zotero.md b/docs/zotero.md index 601749d59..cf5813494 100644 --- a/docs/zotero.md +++ b/docs/zotero.md @@ -64,22 +64,25 @@ Word's "Field" in the file.) ## Who can drive the document Zotero's protocol has no handshake that proves the program on port 23116 is Zotero, and that -program can read the citations and edit the open document. WordCraft limits who that can be: +program can read the citations and edit the open document. WordCraft limits who that can be. +In short: **every system** gets the loopback rule; **only Linux** also checks that the listener +isn't another user's. On Windows and macOS (and the BSDs) the loopback rule is the only check. -- It only connects to this computer (a loopback address such as `127.0.0.1`); any other address - is refused before connecting. -- On Linux it checks, in the kernel's socket table (`/proc/net/tcp`, `/proc/net/tcp6`), that the +- Every system: it only connects to this computer (a loopback address such as `127.0.0.1`); + any other address is refused before connecting. +- Linux only: it checks, in the kernel's socket table (`/proc/net/tcp`, `/proc/net/tcp6`), that the program listening on the port runs as the same user as WordCraft, before sending anything. If another account on a shared machine took the port while Zotero wasn't running, WordCraft refuses with an error and the document isn't touched. When the table doesn't show the listener (WSL2 mirrored networking with Zotero running on Windows, a local proxy, a sandbox) or can't be read, its owner is unknown and only the loopback rule applies, as below. -- Remaining risk: macOS, Windows and the BSDs have no such table to read without OS calls - WordCraft doesn't make (it uses no `unsafe` code), so there only the loopback rule applies - (and on Linux when the listener isn't in the table): on a computer shared with other - accounts, a program another user leaves on port 23116 while Zotero is closed would be taken - for Zotero. A program running as you is trusted on every - system; it could change your documents anyway. +- Windows, macOS and the BSDs: **no same-user check.** They have no such table to read + without OS calls WordCraft doesn't make (it uses no `unsafe` code), so only the loopback rule + applies there (and on Linux when the listener isn't in the table). Remaining risk: on a + computer shared with other accounts, a program another user leaves on port 23116 while + Zotero is closed would be taken for Zotero. An owner check for these systems is tracked in + #3831. A program running as you is trusted on every system; it could change your documents + anyway. - `wordcraft-cli zotero` prints Zotero's messages (and `--trace` lines) with control characters, line breaks and backslashes shown as escapes (`\n`, `\u{1b}`, `\\`), so a peer can't forge other output or drive the terminal. From d785c05c53583b663937ab55c0d4d0ed4ea590df Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Fri, 23 Oct 2026 16:22:50 +0100 Subject: [PATCH 10/10] =?UTF-8?q?#3692:=20review=20=E2=80=94=20zotero=20ti?= =?UTF-8?q?ming=20tests=20get=20a=20margin=20a=20busy=20CI=20machine=20can?= =?UTF-8?q?'t=20break?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wall-clock bounds of 2 s failed on loaded runners (checked against the pre-fix code, put back in the worktree): - note referenced many times: no clock; listing the note once per reference gave a million fields, which the count of 200 catches. - rich text with many runs: 2 s -> 10 s; 0.35 s now, merging after every run ran past 9 minutes. - Document_convert with one id repeated: 2 s -> 10 s, 1,000 fields (was 300); 22 ms now, the per-id check took 81 s with 300 fields and grows with the field count. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01CX1hMKX28764Lrx5moxs1o --- crates/zotero/src/tests_limits.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/crates/zotero/src/tests_limits.rs b/crates/zotero/src/tests_limits.rs index 53d8eaafc..ce6b12c67 100644 --- a/crates/zotero/src/tests_limits.rs +++ b/crates/zotero/src/tests_limits.rs @@ -34,11 +34,10 @@ fn a_note_referenced_many_times_is_listed_once() { let note = doc.add_part(PartKind::Endnote, vec![para_block(objects_para(citations(200)))]); let refs = (0..5_000).map(|_| InlineObject::NoteRef { kind: NoteKind::Endnote, id: note, custom: String::new() }).collect(); doc.body = vec![para_block(objects_para(refs))]; - let t = Instant::now(); + // No clock: listing the note once per reference gave a million fields, which the count catches. let list = fields::list(&doc); assert_eq!(list.len(), 200, "each field once, at the note's first reference"); assert!(list.iter().all(|f| f.start.story == StoryRef::Part(note) && f.note_index == 1)); - assert!(t.elapsed() < Duration::from_secs(2), "{:?}", t.elapsed()); } #[test] @@ -66,7 +65,8 @@ fn rich_text_with_many_runs_is_linear() { assert_eq!(p.text, "ab".repeat(50_000)); assert_eq!(p.runs.len(), 100_000); assert_eq!(p.runs.iter().map(|r| r.len).sum::(), p.text.len()); - assert!(elapsed < Duration::from_secs(2), "{elapsed:?}"); + // Under half a second on a loaded machine; merging again after every run ran past 9 minutes. + assert!(elapsed < Duration::from_secs(10), "{elapsed:?}"); // Neighbours with the same formatting are one run. let frag = crate::rich::fragment("abc", true, &CharProps::default()); let Some(wordcraft_doc::Block::Para(p)) = frag.blocks.first() else { panic!("no paragraph") }; @@ -84,7 +84,9 @@ fn call(b: &mut Bridge, s: &mut Session, method: &str, args: Value) -> Result