From d2f504e29a3d05e4513647fab5d2cf22dee0be68 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Wed, 4 Nov 2026 12:50:36 +0100 Subject: [PATCH] Review fixes: package-wide PBKDF2 budget, CLI keeps the ODF style, turned linked boxes, path close, centred PDF fill, doc status (gap 19) --- apps/wordcraft-cli/src/main.rs | 18 ++++++++- apps/wordcraft-cli/src/tests.rs | 18 +++++++++ crates/formats/src/odt/crypt.rs | 37 +++++++++++++++++ crates/formats/src/odt/path.rs | 21 +++++++++- crates/layout/src/display/page.rs | 64 +++++++++++++++++++----------- crates/layout/src/tests/textbox.rs | 42 +++++++++++++++++++- crates/pdf/src/exporter/draw.rs | 26 +++++++++++- docs/gaps.md | 1 + docs/odf-interpretation.md | 2 +- 9 files changed, 199 insertions(+), 30 deletions(-) diff --git a/apps/wordcraft-cli/src/main.rs b/apps/wordcraft-cli/src/main.rs index 2b2a32771f..480484dac1 100644 --- a/apps/wordcraft-cli/src/main.rs +++ b/apps/wordcraft-cli/src/main.rs @@ -90,11 +90,18 @@ fn missing_argument() -> Failure { /// ([`password_from`]). As in the app, a document opened with one is saved with it again, so /// `convert` does not quietly write a copy the password no longer protects. fn open(path: &Path, password: Option<&str>) -> Result { + open_in(path, password, odf_profile()?) +} + +/// [`open`] with the OpenDocument style `odf`, which the session keeps: a later `file.open` or +/// `file.new` in a `run` script reads files the same way. +fn open_in(path: &Path, password: Option<&str>, odf: wordcraft_doc::odf_profile::OdfProfile) -> Result { // A file that gives no page size (HTML, Markdown, plain text) opens on the system region's // paper, as in the app: A4 in Italy, Letter in the US (#1381). let region = system_region(); - let (doc, encrypted) = wordcraft_engine::io::open_path_in(path, password, ®ion, &odf_profile()?)?; + let (doc, encrypted) = wordcraft_engine::io::open_path_in(path, password, ®ion, &odf)?; let mut s = session(doc); + s.prefs.odf = odf; s.path = Some(path.into()); if encrypted { s.password = password.map(wordcraft_engine::io::Password::new); @@ -179,8 +186,13 @@ fn session(doc: wordcraft_doc::Document) -> Session { /// How OpenDocument files fill in what their standard leaves open: `WORDCRAFT_ODF` names a preset /// (`word`, Microsoft Word's style and the default, or `libreoffice`; `wordcraft` is taken too). fn odf_profile() -> Result { + odf_from(std::env::var("WORDCRAFT_ODF").ok()) +} + +/// [`odf_profile`] from `WORDCRAFT_ODF`'s value, if set. +fn odf_from(var: Option) -> Result { use wordcraft_doc::odf_profile::{OdfPreset, OdfProfile}; - match std::env::var("WORDCRAFT_ODF").ok().filter(|v| !v.trim().is_empty()) { + match var.filter(|v| !v.trim().is_empty()) { None => Ok(OdfProfile::default()), Some(v) => match OdfPreset::from_id(&v).filter(|p| *p != OdfPreset::Custom) { Some(p) => Ok(OdfProfile::of(p)), @@ -521,6 +533,8 @@ fn run>(args: &[S]) -> Result<(), Failure> { // run starts with one open document. (None, t) => { let mut s = session(wordcraft_doc::Document::new()); + // The script's own `file.open`s read OpenDocument files in `WORDCRAFT_ODF`'s style. + s.prefs.odf = odf_profile()?; let first = s.document_id(); let params = t.map_or_else(|| json!({}), |t| json!({"template": t})); s.run("file.new", ¶ms).map_err(|e| e.to_string())?; diff --git a/apps/wordcraft-cli/src/tests.rs b/apps/wordcraft-cli/src/tests.rs index a87f778808..1201fb0caa 100644 --- a/apps/wordcraft-cli/src/tests.rs +++ b/apps/wordcraft-cli/src/tests.rs @@ -543,3 +543,21 @@ fn script_parse_resolves_paths_against_the_base() { assert_eq!(s.save, vec!["o.docx".to_string()]); assert_eq!(s.commands.len(), 1); } + +/// `WORDCRAFT_ODF` names a display style, and a session opened in one keeps it, so a `run` +/// script's later `file.open` reads OpenDocument files the same way. +#[test] +fn the_odf_style_is_kept_by_the_session() { + use wordcraft_doc::odf_profile::OdfProfile; + assert_eq!(odf_from(None), Ok(OdfProfile::default())); + assert_eq!(odf_from(Some("libreoffice".into())), Ok(OdfProfile::libreoffice())); + assert_eq!(odf_from(Some("word".into())), Ok(OdfProfile::word())); + assert!(odf_from(Some("custom".into())).is_err()); + let dir = std::env::temp_dir().join(format!("wc-cli-odf-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("a.txt"); + std::fs::write(&path, "Hello").unwrap(); + let s = open_in(&path, None, OdfProfile::libreoffice()).unwrap(); + assert_eq!(s.prefs.odf, OdfProfile::libreoffice()); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/crates/formats/src/odt/crypt.rs b/crates/formats/src/odt/crypt.rs index 7da6cee4fb..d6ddcd57b5 100644 --- a/crates/formats/src/odt/crypt.rs +++ b/crates/formats/src/odt/crypt.rs @@ -45,6 +45,20 @@ const ITERATIONS: u32 = 100_000; const MAX_ITERATIONS: u32 = 10_000_000; /// Most files in a package decrypted. const MAX_FILES: usize = 10_000; +/// Most PBKDF2 work (iterations times the key's 20-byte blocks) for a whole package: a thousand +/// files as LibreOffice and WordCraft write them (100,000 iterations, a 32-byte key), so many +/// files at the per-file cap can't add up to a hang. +const MAX_WORK: u64 = 200_000_000; + +/// `p`'s key derivation added to `spent`, an error once the package's work passes [`MAX_WORK`]. +fn charge(spent: &mut u64, p: &Params) -> Result<(), CryptError> { + let blocks = p.key_size.unwrap_or(16).clamp(16, 32).div_ceil(20) as u64; + *spent = spent.saturating_add(u64::from(p.iterations.min(MAX_ITERATIONS)).saturating_mul(blocks)); + if *spent > MAX_WORK { + return Err(CryptError::Unsupported("the package's key derivations would take too long".into())); + } + Ok(()) +} /// How one file is encrypted (a `manifest:encryption-data`). #[derive(Default, Debug, Clone)] @@ -254,6 +268,7 @@ pub fn decrypt(bytes: &[u8], password: &str) -> Result, CryptError> { let deflated = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); let io = |e: std::io::Error| CryptError::Unsupported(e.to_string()); let mut checked = false; + let mut work = 0u64; for i in 0..z.len().min(MAX_FILES) { let Ok(mut f) = z.by_index(i) else { continue }; let name = f.name().to_string(); @@ -264,6 +279,7 @@ pub fn decrypt(bytes: &[u8], password: &str) -> Result, CryptError> { f.by_ref().take(MAX_ENTRY).read_to_end(&mut data).map_err(io)?; let data = match encrypted.iter().find(|(p, _)| *p == name) { Some((_, p)) => { + charge(&mut work, p)?; let plain = decrypt_file(&data, p, password)?; checked = true; plain @@ -392,3 +408,24 @@ pub fn encrypt(bytes: &[u8], password: &str) -> Result, String> { } Ok(w.finish().map_err(err)?.into_inner()) } + +#[cfg(test)] +mod work_tests { + use super::*; + + /// Key derivations are budgeted over the whole package, not only per file: a thousand of + /// LibreOffice's fit, twenty-one at the per-file cap don't. + #[test] + fn a_packages_key_derivations_share_one_budget() { + let ours = Params { iterations: ITERATIONS, key_size: Some(32), ..Default::default() }; + let mut spent = 0; + for _ in 0..1000 { + assert!(charge(&mut spent, &ours).is_ok()); + } + assert!(charge(&mut spent, &ours).is_err()); + let hostile = Params { iterations: MAX_ITERATIONS, key_size: Some(16), ..Default::default() }; + let mut spent = 0; + let ok = (0..30).take_while(|_| charge(&mut spent, &hostile).is_ok()).count(); + assert_eq!(ok, 20); + } +} diff --git a/crates/formats/src/odt/path.rs b/crates/formats/src/odt/path.rs index 49c73db13b..2e077efe24 100644 --- a/crates/formats/src/odt/path.rs +++ b/crates/formats/src/odt/path.rs @@ -88,8 +88,10 @@ pub(super) fn path(d: &str, vb: Option<[f32; 4]>) -> Option { Token::Num(_) => match cmd { Some('M') => 'L', Some('m') => 'l', + // A close takes no numbers: a number after one ends the data (it would be read + // again for ever otherwise). + Some('Z' | 'z') | None => break, Some(c) => c, - None => break, }, }; if let Token::Num(_) = t { @@ -395,3 +397,20 @@ fn path_text(f: &Freeform, vw: u32, vh: u32, enhanced: bool) -> String { } d.trim_end().to_string() } + +#[cfg(test)] +mod tests { + /// A number after a close ends the path data; it used to be read again for ever (a hang on a + /// hostile or malformed `svg:d`). + #[test] + fn a_number_after_a_close_ends_the_path() { + let f = super::path("M 0 0 L 10 0 L 10 10 Z 5 5", Some([0.0, 0.0, 10.0, 10.0])); + let paths = f.map(|f| f.paths).unwrap_or_default(); + assert_eq!(paths.len(), 1, "{paths:?}"); + assert!(paths[0].closed); + assert_eq!(paths[0].pts.len(), 3); + // Lower case too, and a close ending the data as before. + assert!(super::path("m 0 0 l 10 0 l 0 10 z 1", None).is_some()); + assert!(super::path("M 0 0 L 10 0 L 10 10 Z", None).is_some()); + } +} diff --git a/crates/layout/src/display/page.rs b/crates/layout/src/display/page.rs index 61ed34e7f5..6c6c9b69b2 100644 --- a/crates/layout/src/display/page.rs +++ b/crates/layout/src/display/page.rs @@ -48,7 +48,7 @@ pub struct ItemDisplay<'a> { page: &'a Page, authors: Authors<'a>, clips: Clips, - /// The page's turned text boxes (see [`turned_boxes`]): their lines are drawn turned. + /// The page's text boxes (see [`text_frames`]): a turned one's lines are drawn turned. turned: Vec<(u32, Rect, Spin)>, } @@ -56,7 +56,7 @@ impl<'a> ItemDisplay<'a> { pub fn new(doc: &'a Document, page: &'a Page) -> Self { let cells: Vec = page.header.iter().chain(&page.footer).chain(&page.items).filter(|c| matches!(c, Placed::Cell { clip: true, .. })).cloned().collect(); - let turned = [&page.items, &page.header, &page.footer].into_iter().flat_map(|items| turned_boxes(items)).collect(); + let turned = [&page.items, &page.header, &page.footer].into_iter().flat_map(|items| text_frames(items)).collect(); ItemDisplay { doc, page, authors: Authors::new(doc), clips: clipped_cells(&cells), turned } } @@ -68,9 +68,7 @@ impl<'a> ItemDisplay<'a> { let (_, ba) = dimming(opts); stories_display(self.doc, &[items], Some(&self.clips), opts, &self.authors, ba, &mut out); // A line of a turned text box turns with it (as `stories_display` turns them on a page). - if let Placed::Lines { story: StoryRef::Part(id), .. } = it - && let Some((_, rect, spin)) = self.turned.iter().find(|(t, ..)| t == id) - { + if let Some((_, rect, spin)) = turned_frame(&self.turned, it).and_then(|k| self.turned.get(k)) { let mut turned = Vec::new(); spun(*spin, *rect, std::mem::take(&mut out), &mut turned); out = turned; @@ -119,7 +117,8 @@ fn stories_display( let hidden = opts.hide_object.as_ref().map(|p| Hidden::new(doc, p)); for items in stories { let warps = warped_boxes(doc, items); - let turned = turned_boxes(items); + let frames = text_frames(items); + let mut spun_frames: Vec = Vec::new(); let own; let clips = match shared { Some(c) => c, @@ -155,41 +154,58 @@ fn stories_display( warped_text(WarpedText { doc, lines: &lines, rect: *rect, warp, opts, authors, alpha: a }, out); } } - Placed::Lines { story: StoryRef::Part(id), .. } if turned.iter().any(|(t, ..)| t == id) => { + _ => match turned_frame(&frames, it) { // A turned text box's lines are drawn together, turned with it about its - // centre, where its first one is. - if done.contains(id) { - continue; - } - done.push(*id); - if let Some((_, rect, spin)) = turned.iter().find(|(t, ..)| t == id) { - let mut inner = Vec::new(); - for line in items.iter().filter(|i| matches!(i, Placed::Lines { story: StoryRef::Part(s), .. } if s == id)) { - item(doc, line, opts, authors, a, &mut inner); + // centre, where its first one is. Linked boxes show one story: each line goes + // with the box it was laid out in. + Some(k) => { + if spun_frames.contains(&k) { + continue; + } + spun_frames.push(k); + if let Some((_, rect, spin)) = frames.get(k) { + let mut inner = Vec::new(); + for line in items.iter().filter(|i| turned_frame(&frames, i) == Some(k)) { + item(doc, line, opts, authors, a, &mut inner); + } + super::objects::spun(*spin, *rect, inner, out); } - super::objects::spun(*spin, *rect, inner, out); } - } - _ => item(doc, it, opts, authors, a, out), + None => item(doc, it, opts, authors, a, out), + }, } } } } -/// The text boxes among `items` that are turned: their story, frame (unturned) and how their -/// text turns with them. As in Word, the text turns with the box but isn't mirrored: a box -/// flipped across turns its text upside down, one flipped side to side leaves it readable. -fn turned_boxes(items: &[Placed]) -> Vec<(u32, Rect, Spin)> { +/// The text boxes among `items`: the story each shows, its frame (unturned) and how its text +/// turns with it (not at all for most). As in Word, the text turns with the box but isn't +/// mirrored: a box flipped across turns its text upside down, one flipped side to side leaves it +/// readable. Linked boxes all show the first box's story, so a story can have several frames. +fn text_frames(items: &[Placed]) -> Vec<(u32, Rect, Spin)> { items .iter() .filter_map(|it| { let Placed::Object { rect, spin, text_box: Some(id), .. } = it else { return None }; let text = Spin::new(spin.deg + if spin.flip_v { 180.0 } else { 0.0 }, false, false); - (!text.is_identity()).then_some((*id, *rect, text)) + Some((*id, *rect, text)) }) .collect() } +/// Which of `frames` the text box line `it` was laid out in, if that frame is turned: of the +/// frames showing its story, the one it is in (or nearest, for a line on an edge). +fn turned_frame(frames: &[(u32, Rect, Spin)], it: &Placed) -> Option { + let Placed::Lines { story: StoryRef::Part(id), x, y, .. } = it else { return None }; + let away = |r: &Rect| { + let dx = (r.x - x).max(x - r.right()).max(0.0); + let dy = (r.y - y).max(y - r.bottom()).max(0.0); + dx * dx + dy * dy + }; + let (k, _) = frames.iter().enumerate().filter(|(_, (s, ..))| s == id).min_by(|(_, (_, a, _)), (_, (_, b, _))| away(a).total_cmp(&away(b)))?; + frames.get(k).is_some_and(|(.., spin)| !spin.is_identity()).then_some(k) +} + /// One object drawn alone, as its page draws it: the picture, shape or group `o` in `rect` (its /// unturned frame) and the text of the text box story `text_box` on `page`. A dragged object's /// preview, which mustn't show what was behind it (#1010). diff --git a/crates/layout/src/tests/textbox.rs b/crates/layout/src/tests/textbox.rs index b5fa746afc..e9074a57b4 100644 --- a/crates/layout/src/tests/textbox.rs +++ b/crates/layout/src/tests/textbox.rs @@ -12,6 +12,11 @@ fn lay(doc: &Document) -> DocLayout { /// A text box `w` × `h` showing `text`, inline at the end of body paragraph `para`; its story id. fn text_box(d: &mut Document, para: usize, text: &str, w: f32, h: f32, body: TextBody) -> u32 { + text_box_with(d, para, text, w, h, body, Default::default()) +} + +/// [`text_box`] placed as `float` says. +fn text_box_with(d: &mut Document, para: usize, text: &str, w: f32, h: f32, body: TextBody, float: wordcraft_doc::para::Float) -> u32 { let id = d.add_part(PartKind::TextBox, vec![para_block(Paragraph::with_text(text, Default::default()))]); if let Some(p) = d.parts.get_mut(&id) { p.body = body; @@ -23,7 +28,7 @@ fn text_box(d: &mut Document, para: usize, text: &str, w: f32, h: f32, body: Tex fill: None, stroke: None, stroke_width: 1.0, - float: Default::default(), + float, story: Some(id), effects: Default::default(), freeform: None, @@ -318,3 +323,38 @@ fn floating_box_that_resizes_to_fit_takes_its_texts_height() { let shrunk = rect(true, 300.0); assert!((shrunk.h - grown.h).abs() < 0.5, "{shrunk:?} {grown:?}"); } + +#[test] +fn a_turned_linked_box_turns_only_its_own_lines() { + // Linked boxes show the first box's story; turning the second box turns the lines it shows, + // not the ones the first box shows (on screen and drawn item by item, as PDF export draws). + let mut d = Document::from_text("one\ntwo"); + let text = "Words that run on well past the first little box. ".repeat(6); + let a = text_box(&mut d, 0, &text, 144.0, 48.0, TextBody::default()); + let float = wordcraft_doc::para::Float { wrap: wordcraft_doc::para::Wrap::Square, x: 20.0, y: 300.0, rot: 30.0, ..Default::default() }; + let b = text_box_with(&mut d, 1, "", 144.0, 400.0, TextBody::default(), float); + d.parts.get_mut(&a).unwrap().body.next = Some(b); + let l = lay(&d); + // (Words drawn turned, words drawn upright.) + let turned_texts = |draws: &[display::Draw]| -> (bool, bool) { + let mut inside = String::new(); + let mut outside = String::new(); + for x in draws { + match x { + display::Draw::Rotated { items, .. } => items.iter().for_each(|g| { + if let display::Draw::Glyphs { text, .. } = g { + inside.push_str(text) + } + }), + display::Draw::Glyphs { text, .. } => outside.push_str(text), + _ => {} + } + } + (inside.contains("Words"), outside.contains("Words")) + }; + let page = display::page_display(&d, &l.pages[0], &display::DisplayOptions::default()); + assert_eq!(turned_texts(&page), (true, true), "the first box's lines stay upright, the second's turn"); + let shown = display::ItemDisplay::new(&d, &l.pages[0]); + let items: Vec = l.pages[0].items.iter().flat_map(|it| shown.draws(it, &Default::default())).collect(); + assert_eq!(turned_texts(&items), (true, true)); +} diff --git a/crates/pdf/src/exporter/draw.rs b/crates/pdf/src/exporter/draw.rs index 69aa6d1b54..76440d1c82 100644 --- a/crates/pdf/src/exporter/draw.rs +++ b/crates/pdf/src/exporter/draw.rs @@ -288,7 +288,7 @@ impl Exporter<'_> { .and_then(|b| image::ImageReader::new(std::io::Cursor::new(b.as_slice())).with_guessed_format().ok()?.into_dimensions().ok()); // Centred once at its natural size (an ODT page background that doesn't repeat). if let Some((pw, ph)) = dims.filter(|_| pic.fill_type.eq_ignore_ascii_case("center")) { - let (iw, ih) = ((pw as f32 * 0.75).min(w * 4.0), (ph as f32 * 0.75).min(h * 4.0)); + let (iw, ih) = centred_size(pw, ph, w, h); self.picture(s, &Rect::new((w - iw) / 2.0, (h - ih) / 2.0, iw, ih), &pic.media, &[0.0; 4], 1.0); return; } @@ -340,3 +340,27 @@ impl Exporter<'_> { } } } + +/// A centred page picture's size: its natural size (pixels at 96 dpi), scaled down by one factor +/// for both sides when it is more than four pages wide or tall, so it keeps its shape. +fn centred_size(pw: u32, ph: u32, w: f32, h: f32) -> (f32, f32) { + let (nw, nh) = (pw as f32 * 0.75, ph as f32 * 0.75); + if !(nw > 0.0 && nh > 0.0) { + return (0.0, 0.0); + } + let k = ((w * 4.0) / nw).min((h * 4.0) / nh).min(1.0); + (nw * k, nh * k) +} + +#[cfg(test)] +mod centred_tests { + #[test] + fn a_centred_picture_too_big_keeps_its_shape() { + // Fits: natural size. + assert_eq!(super::centred_size(400, 200, 612.0, 792.0), (300.0, 150.0)); + // Very wide: scaled by one factor to four pages wide, height in proportion. + let (w, h) = super::centred_size(40_000, 1_000, 100.0, 100.0); + assert!((w - 400.0).abs() < 0.01 && (h - 10.0).abs() < 0.01, "{w} x {h}"); + assert_eq!(super::centred_size(0, 10, 100.0, 100.0), (0.0, 0.0)); + } +} diff --git a/docs/gaps.md b/docs/gaps.md index edc1bfbf8e..e2a10c18ed 100644 --- a/docs/gaps.md +++ b/docs/gaps.md @@ -155,6 +155,7 @@ Table rows and cells inside `mc:AlternateContent` use the same selected-branch p | Date | Change | Summary | |---|---|---| +| 2026-11-04 | trivial | #19: ODT element audit; files from other applications checked | | 2026-11-01 | trivial | #19 narrowed: ODT read and written nearly whole on this branch (styles, tables, notes, comments, tracked changes including formatting, fields, indexes and marks, drawings, charts, embedded objects, forms, ruby, password protection, flat ODT), checked against LibreOffice; what's left listed; estimate 18–27 h → 10–16 h | | 2026-11-01 | trivial | #10 narrowed: text in a turned text box is drawn turned with it on screen and in PNG and PDF (upside down when flipped across, never mirrored); editing it still goes by its upright text; estimate unchanged | | 2026-10-30 | trivial | #20 narrowed: layout resource caps and linear-time passes (#3867, #3866) | diff --git a/docs/odf-interpretation.md b/docs/odf-interpretation.md index 2a0ead068b..ba06ecd683 100644 --- a/docs/odf-interpretation.md +++ b/docs/odf-interpretation.md @@ -1,6 +1,6 @@ # How WordCraft interprets OpenDocument files -> **Last reviewed:** 2026-11-02 · **Last updated:** 2026-11-03 · **Change:** minor (deviations from the standard listed; ODF 1.2 → 1.3 checked); previously major (new: the ODF interpretation profile and the list of every case) +> **Last reviewed:** 2026-11-02 · **Last updated:** 2026-11-04 · **Change:** minor (the two presets are Microsoft Word style and LibreOffice style; CLI values listed one by one); previously minor (deviations from the standard listed; ODF 1.2 → 1.3 checked); previously major (new: the ODF interpretation profile and the list of every case) OpenDocument (ODF 1.3, OASIS) leaves some of a document's look to the application. Part 3 §16.2 says that when no style sets a property and the standard gives no default, "an