From 9b77b29dcbad530c534db322d7bb4caffa0dbd96 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 04:29:28 +0000 Subject: [PATCH] address review: refuse table edits past the cell cap, warn on oversized PPTX tables, fail export instead of trimming --- apps/deckcraft-cli/src/main.rs | 3 +++ crates/engine/src/cmd/file.rs | 26 +++++++++++++------- crates/engine/src/cmd/table.rs | 19 +++++++++++---- crates/engine/src/tests.rs | 39 ++++++++++++++++++++++++++++++ crates/model/src/table.rs | 40 +++++++++++++++++++++++++++---- crates/pptx/src/lib.rs | 6 +++++ crates/pptx/src/read.rs | 22 ++++++++++++++++- crates/pptx/src/read/shapes.rs | 3 +++ crates/pptx/src/write.rs | 9 +++++++ crates/pptx/src/write/shapes.rs | 4 ++-- crates/pptx/tests/it/malformed.rs | 38 +++++++++++++++++++++++++++++ 11 files changed, 188 insertions(+), 21 deletions(-) diff --git a/apps/deckcraft-cli/src/main.rs b/apps/deckcraft-cli/src/main.rs index 5701b72f8..420f4f1a5 100644 --- a/apps/deckcraft-cli/src/main.rs +++ b/apps/deckcraft-cli/src/main.rs @@ -148,6 +148,9 @@ fn warn_if_damaged(path: &str, opened: &serde_json::Value) { if slides > 0 { eprintln!("deckcraft-cli: warning: {path}: {slides} slide{} couldn't be read and left out", if slides == 1 { "" } else { "s" }); } + for w in opened.get("importWarnings").and_then(|v| v.as_array()).into_iter().flatten().filter_map(|w| w.as_str()) { + eprintln!("deckcraft-cli: warning: {path}: {w}"); + } if media > 0 { eprintln!("deckcraft-cli: warning: {path}: {media} picture or media file{} couldn't be read", if media == 1 { "" } else { "s" }); } diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index 0d52e6358..2e786a440 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -139,11 +139,13 @@ pub struct Damage { pub media: Vec, /// Slides a `.pptx` lists but that couldn't be read, so they are not in the presentation. pub slides: Vec, + /// Content a `.pptx` had that was cut to fit a limit; the file still opened. + pub warnings: Vec, } impl Damage { pub fn is_empty(&self) -> bool { - self.media.is_empty() && self.slides.is_empty() + self.media.is_empty() && self.slides.is_empty() && self.warnings.is_empty() } } @@ -151,17 +153,17 @@ impl Damage { pub fn open_checked(name: &str, bytes: &[u8]) -> Result<(Presentation, Damage)> { if deckcraft_format::sniff(bytes) { let (p, media) = deckcraft_format::load_checked(bytes).map_err(|e| EngineError::Other(e.to_string()))?; - return Ok((p, Damage { media, slides: vec![] })); + return Ok((p, Damage { media, ..Default::default() })); } - open_other(name, bytes).map(|(p, slides)| (p, Damage { media: vec![], slides })) + open_other(name, bytes).map(|(p, slides, warnings)| (p, Damage { slides, warnings, ..Default::default() })) } -fn open_other(name: &str, bytes: &[u8]) -> Result<(Presentation, Vec)> { +fn open_other(name: &str, bytes: &[u8]) -> Result<(Presentation, Vec, Vec)> { if deckcraft_pptx::sniff(bytes) { - let (mut p, lost_slides) = deckcraft_pptx::import_checked(bytes).map_err(|e| EngineError::Other(e.to_string()))?; + let (mut p, lost_slides, warnings) = deckcraft_pptx::import_report(bytes).map_err(|e| EngineError::Other(e.to_string()))?; deckcraft_format::repair(&mut p); probe_clips(&mut p); - return Ok((p, lost_slides)); + return Ok((p, lost_slides, warnings)); } if deckcraft_pptx::is_encrypted(bytes) { return Err(EngineError::Other(format!("{name} is password-protected. DeckCraft can't open password-protected presentations yet."))); @@ -180,7 +182,7 @@ fn open_other(name: &str, bytes: &[u8]) -> Result<(Presentation, Vec)> { if p.slides.is_empty() { p = defaults::new_presentation(None); } - return Ok((p, vec![])); + return Ok((p, vec![], vec![])); } Err(EngineError::Other(format!("{name}: not a presentation DeckCraft can read"))) } @@ -269,7 +271,7 @@ fn adopt_loaded(s: &mut Session, f: LoadedFile) -> Result { let st = s.doc()?; Ok(json!({ "document": i, "slides": st.doc.slides.len(), "missingFonts": st.missing_fonts, - "damagedMedia": damaged.media, "lostSlides": damaged.slides, + "damagedMedia": damaged.media, "lostSlides": damaged.slides, "importWarnings": damaged.warnings, })) } @@ -304,7 +306,9 @@ fn open_bytes(s: &mut Session, p: &Value) -> Result { let (doc, damaged) = open_checked(name, &data)?; let i = add_opened(s, name, None, doc); report_damage(s, &damaged)?; - Ok(json!({"document": i, "missingFonts": s.doc()?.missing_fonts, "damagedMedia": damaged.media, "lostSlides": damaged.slides})) + Ok( + json!({"document": i, "missingFonts": s.doc()?.missing_fonts, "damagedMedia": damaged.media, "lostSlides": damaged.slides, "importWarnings": damaged.warnings}), + ) } /// Tell the user which pictures, media and slides a damaged file lost, and mark the copy repaired. @@ -332,6 +336,10 @@ fn report_damage(s: &mut Session, damaged: &Damage) -> Result<()> { if n == 1 { "was" } else { "were" }, )); } + for w in &damaged.warnings { + text.push_str(w); + text.push_str(". "); + } text.push_str("It was opened as a repaired copy; save it under a new name to keep the original file."); s.ui_requests.push(crate::UiRequest::Message { text }); Ok(()) diff --git a/crates/engine/src/cmd/table.rs b/crates/engine/src/cmd/table.rs index 5329fd630..f8baf5102 100644 --- a/crates/engine/src/cmd/table.rs +++ b/crates/engine/src/cmd/table.rs @@ -286,6 +286,10 @@ fn with_table(s: &mut Session, p: &Value, cmd: &str, f: impl Fn(&mut Table) -> R let r = edit_shapes(s, &json!({"id": id}), cmd, |sh| match &mut sh.kind { ShapeKind::Table(t) => { // Edits walk the grid: bound it first (a loaded table need not be normalized yet). + // An over-cap table is refused, not trimmed: a trim would delete rows while reporting success. + if t.over_cap() { + return Err(bad(cmd, format!("the table is larger than {} cells; edit refused", deckcraft_model::table::MAX_CELLS))); + } t.normalize(); f(t)?; t.normalize(); @@ -320,10 +324,17 @@ fn rows_cols(s: &mut Session, p: &Value, op: &str, name: &'static str) -> Result let op = op.to_string(); with_table(s, p, name, move |t| { match op.as_str() { - "rowAbove" => t.insert_row(r), - "rowBelow" => t.insert_row(r.saturating_add(1)), - "colLeft" => t.insert_col(c), - "colRight" => t.insert_col(c.saturating_add(1)), + "rowAbove" | "rowBelow" | "colLeft" | "colRight" => { + let ok = match op.as_str() { + "rowAbove" => t.insert_row(r), + "rowBelow" => t.insert_row(r.saturating_add(1)), + "colLeft" => t.insert_col(c), + _ => t.insert_col(c.saturating_add(1)), + }; + if !ok { + return Err(bad(name, format!("the table is at its {}-cell limit", deckcraft_model::table::MAX_CELLS))); + } + } "delRow" => { if !t.delete_row(r) { return Err(bad(name, "can't delete the last row")); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 68de13418..10dc56733 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -6458,3 +6458,42 @@ fn master_body_placeholder_format_keeps_per_level_formatting() { assert_eq!((level(&s, 0).run.size, level(&s, 1).run.size), (Some(28.0), Some(24.0))); assert_eq!(shown(&s), vec![Some(28.0), Some(24.0)]); } + +#[test] +fn table_edits_past_the_cell_cap_are_refused_not_trimmed() { + use deckcraft_model::table::{MAX_CELLS, Row}; + let mut s = session(); + let id = table_at(&mut s, &[100.0, 100.0], json!([["a", "b"], ["c", "d"]])); + let cols = 200; + let rows = MAX_CELLS / cols; + let big = deckcraft_model::Table { + cols: vec![1.0; cols], + rows: vec![Row { height: 1.0, cells: vec![Default::default(); cols] }; rows], + ..Default::default() + }; + for sl in std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().doc).slides.iter_mut() { + for sh in std::sync::Arc::make_mut(sl).shapes.iter_mut() { + if sh.id == id { + sh.kind = deckcraft_model::ShapeKind::Table(big.clone()); + } + } + } + for cmd in ["table.insertRowAbove", "table.insertRowBelow"] { + let e = s.execute(cmd, &json!({"id": id.0, "row": 0})).unwrap_err().to_string(); + assert!(e.contains("cell limit"), "{cmd}: {e}"); + } + let e = s.execute("table.insertColumnLeft", &json!({"id": id.0, "col": 0})).unwrap_err().to_string(); + assert!(e.contains("cell limit"), "{e}"); + assert_eq!(table_of(&s, id), big, "a refused insert leaves every row in place"); + // A table already over the cap is refused too, not trimmed behind a success. + let huge = deckcraft_model::Table { rows: vec![Row { height: 1.0, cells: vec![Default::default(); cols] }; rows + 1], ..big.clone() }; + for sl in std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().doc).slides.iter_mut() { + for sh in std::sync::Arc::make_mut(sl).shapes.iter_mut() { + if sh.id == id { + sh.kind = deckcraft_model::ShapeKind::Table(huge.clone()); + } + } + } + assert!(s.execute("table.deleteRow", &json!({"id": id.0, "row": 0})).is_err()); + assert_eq!(table_of(&s, id).n_rows(), rows + 1); +} diff --git a/crates/model/src/table.rs b/crates/model/src/table.rs index 9ff39697e..9b5c32b6c 100644 --- a/crates/model/src/table.rs +++ b/crates/model/src/table.rs @@ -113,15 +113,24 @@ impl Table { self.rows.iter().map(|r| r.height).sum() } /// Insert an empty row before `at`. A merge the new row falls inside grows to cover it. - pub fn insert_row(&mut self, at: usize) { + /// Returns false, leaving the table as it was, when the row would take it past [`MAX_CELLS`]. + pub fn insert_row(&mut self, at: usize) -> bool { + if self.cols.len().saturating_mul(self.rows.len().saturating_add(1)) > MAX_CELLS { + return false; + } let at = at.min(self.rows.len()); let merges = self.merges(); let h = self.rows.get(at.saturating_sub(1)).map(|r| r.height).unwrap_or(30.0); self.rows.insert(at, Row { height: h, cells: (0..self.cols.len()).map(|_| Cell::new("")).collect() }); self.set_merges(merges.into_iter().map(|m| m.insert_row(at))); + true } /// Insert an empty column before `at`. A merge the new column falls inside grows to cover it. - pub fn insert_col(&mut self, at: usize) { + /// Returns false, leaving the table as it was, when the column would take it past [`MAX_CELLS`]. + pub fn insert_col(&mut self, at: usize) -> bool { + if self.cols.len().saturating_add(1).saturating_mul(self.rows.len()) > MAX_CELLS { + return false; + } let at = at.min(self.cols.len()); let merges = self.merges(); let w = self.cols.get(at.saturating_sub(1)).copied().unwrap_or(80.0); @@ -131,6 +140,7 @@ impl Table { r.cells.insert(i, Cell::new("")); } self.set_merges(merges.into_iter().map(|m| m.insert_col(at))); + true } /// Delete row `at`. Merges through it shrink; a merge anchored on it moves its anchor (and /// text) to the next row it covers. @@ -322,6 +332,11 @@ impl Table { let n = self.cols.len(); n <= MAX_CELLS && self.rows.len() <= (MAX_CELLS / n.max(1)).max(1) && self.rows.iter().all(|r| r.cells.len() == n) } + /// Whether [`Table::normalize`] would drop rows or columns to fit [`MAX_CELLS`]. + pub fn over_cap(&self) -> bool { + let n = self.cols.len(); + n > MAX_CELLS || self.rows.len() > (MAX_CELLS / n.max(1)).max(1) + } /// What [`Table::normalize`] would leave of this table, cloning only the rows and cells that /// survive (an oversized table is not copied whole first). pub fn normalized(&self) -> Table { @@ -609,11 +624,26 @@ mod tests { } } assert_eq!(t.merges().len(), 1, "the first anchor already covers the whole grid"); - t.insert_row(n); + assert!(t.insert_row(n)); assert_eq!(t.n_rows(), n + 1); assert!(t.split(0, 0)); } + /// A table at the cell cap refuses another row or column instead of losing rows to + /// normalization, and the cap sits above PowerPoint's own 75 x 75. + #[test] + fn insert_past_the_cap_is_refused() { + const { assert!(MAX_CELLS >= 75 * 75) }; + let mut t = + Table { cols: vec![1.0; 200], rows: vec![Row { height: 1.0, cells: vec![Cell::new(""); 200] }; MAX_CELLS / 200], ..Default::default() }; + assert!(!t.over_cap()); + let before = t.clone(); + assert!(!t.insert_row(0)); + assert!(!t.insert_col(0)); + assert_eq!(t, before); + assert!(Table::new(75, 75, 100.0, 10.0).insert_row(0)); + } + fn merged() -> Table { // 4x4 with a 2x2 merge at (1,1) holding "M". let mut t = Table::new(4, 4, 400.0, 30.0); @@ -647,10 +677,10 @@ mod tests { fn row_and_column_edits_keep_merges_consistent() { for at in 0..=4 { let mut t = merged(); - t.insert_row(at); + assert!(t.insert_row(at)); assert_topology(&t); let mut t = merged(); - t.insert_col(at); + assert!(t.insert_col(at)); assert_topology(&t); } for at in 0..4 { diff --git a/crates/pptx/src/lib.rs b/crates/pptx/src/lib.rs index be08c680f..d2ee4f9b9 100644 --- a/crates/pptx/src/lib.rs +++ b/crates/pptx/src/lib.rs @@ -70,6 +70,12 @@ pub fn import_checked(bytes: &[u8]) -> Result<(Presentation, Vec), PptxE read::import_checked(bytes) } +/// [`import_checked`], plus warnings for content cut to fit a limit (a table past the cell cap +/// keeps its leading rows); the file still opens. +pub fn import_report(bytes: &[u8]) -> Result<(Presentation, Vec, Vec), PptxError> { + read::import_report(bytes) +} + /// Read the theme of a `.thmx` file, or of a `.pptx` / `.potx`'s first master, with its media. pub fn import_theme(bytes: &[u8]) -> Result<(deckcraft_model::theme::Theme, Vec), PptxError> { read::import_theme(bytes) diff --git a/crates/pptx/src/read.rs b/crates/pptx/src/read.rs index 288e497f8..4c5dc1be3 100644 --- a/crates/pptx/src/read.rs +++ b/crates/pptx/src/read.rs @@ -46,9 +46,21 @@ pub struct Imp<'a> { pub tag_bytes: usize, /// The tag budget ran out and the warning was given. pub tags_capped: bool, + /// What the file lost that the reader kept going past (see [`MAX_IMPORT_WARNINGS`]). + pub warnings: Vec, } +/// Import warnings kept; a hostile file with many oversized tables adds no more than this. +const MAX_IMPORT_WARNINGS: usize = 32; + impl Imp<'_> { + /// Record something the file lost, and log it. + pub fn warn(&mut self, msg: String) { + log::warn!("pptx: {msg}"); + if self.warnings.len() < MAX_IMPORT_WARNINGS { + self.warnings.push(msg); + } + } pub fn alloc(&mut self) -> u32 { self.next = self.next.saturating_add(1); self.next @@ -446,6 +458,7 @@ pub fn import_theme(bytes: &[u8]) -> Result<(deckcraft_model::theme::Theme, Vec< tags_by_part: HashMap::new(), tag_bytes: 0, tags_capped: false, + warnings: vec![], }; let part = Part::new(&pkg, &theme_path, &doc); let theme = theme::read_theme(&mut imp, &part, &doc.root); @@ -459,6 +472,12 @@ pub fn import(bytes: &[u8]) -> Result { /// [`import`], plus the parts of the slide list the file names but that couldn't be read (a /// missing or truncated slide): those slides are left out, so a caller can say so (#203). pub fn import_checked(bytes: &[u8]) -> Result<(Presentation, Vec), PptxError> { + import_report(bytes).map(|(p, lost, _)| (p, lost)) +} + +/// [`import_checked`], plus warnings for content the file had that was cut to fit a limit (a +/// table past the cell cap keeps its leading rows). +pub fn import_report(bytes: &[u8]) -> Result<(Presentation, Vec, Vec), PptxError> { let mut lost_slides: Vec = vec![]; let pkg = Package::open(bytes)?; let root_rels = pkg.rels(""); @@ -487,6 +506,7 @@ pub fn import_checked(bytes: &[u8]) -> Result<(Presentation, Vec), PptxE tags_by_part: HashMap::new(), tag_bytes: 0, tags_capped: false, + warnings: vec![], }; // Slide ids first so hyperlinks anywhere can point at slides. let slide_ids = register_slides(&mut imp, &docs, &slide_entries, &mut lost_slides); @@ -517,7 +537,7 @@ pub fn import_checked(bytes: &[u8]) -> Result<(Presentation, Vec), PptxE p.media = std::mem::take(&mut imp.media); p.next_id = imp.next; p.fix_next_id(); - Ok((p, lost_slides)) + Ok((p, lost_slides, std::mem::take(&mut imp.warnings))) } /// Masters, layouts and slides to read, in file order and without duplicates: (master paths, diff --git a/crates/pptx/src/read/shapes.rs b/crates/pptx/src/read/shapes.rs index 6d8acc4ad..50c42b45c 100644 --- a/crates/pptx/src/read/shapes.rs +++ b/crates/pptx/src/read/shapes.rs @@ -780,6 +780,9 @@ fn table(imp: &mut Imp, part: &Part, tbl: &El) -> Table { let n = t.rows.iter().map(|r| r.cells.len()).max().unwrap_or(0); t.cols = vec![72.0; n]; } + if t.over_cap() { + imp.warn(format!("a table in {} is over {} cells; its last rows were left out", part.name, deckcraft_model::table::MAX_CELLS)); + } t.normalize(); t } diff --git a/crates/pptx/src/write.rs b/crates/pptx/src/write.rs index 5d3e60e92..42010e8d3 100644 --- a/crates/pptx/src/write.rs +++ b/crates/pptx/src/write.rs @@ -63,6 +63,8 @@ pub struct Exp<'a> { n_kept: u32, pub slide_parts: HashMap, pub table_styles: Vec, + /// A table past the cell cap was met; export fails rather than write a trimmed one. + pub table_over_cap: bool, placeholder: Option, } @@ -541,6 +543,7 @@ pub fn export(p: &Presentation, main_ct: &str) -> Result, PptxError> { n_kept: 0, slide_parts: HashMap::new(), table_styles: vec![], + table_over_cap: false, placeholder: None, }; for (i, s) in p.slides.iter().enumerate() { @@ -748,6 +751,12 @@ pub fn export(p: &Presentation, main_ct: &str) -> Result, PptxError> { root.add_new(&k.rel_type, &k.name, false); } x.pkg.rels("", &root); + if x.table_over_cap { + return Err(PptxError::Unsupported(format!( + "a table has more than {} cells; export refused rather than drop rows", + deckcraft_model::table::MAX_CELLS + ))); + } x.pkg.finish() } diff --git a/crates/pptx/src/write/shapes.rs b/crates/pptx/src/write/shapes.rs index 87200a890..638fe42ab 100644 --- a/crates/pptx/src/write/shapes.rs +++ b/crates/pptx/src/write/shapes.rs @@ -696,8 +696,8 @@ fn frame_close(w: &mut W) { } fn table(w: &mut W, x: &mut Exp, o: &mut Out, t: &Table) { - let mut t = t.clone(); - t.normalize(); + x.table_over_cap |= t.over_cap(); + let t = t.normalized(); w.open0("a:tbl"); let (guid, _) = crate::tables::guid_for_style(&t.style); if !x.table_styles.contains(&t.style) { diff --git a/crates/pptx/tests/it/malformed.rs b/crates/pptx/tests/it/malformed.rs index 774a5ee92..e5dd8c2cc 100644 --- a/crates/pptx/tests/it/malformed.rs +++ b/crates/pptx/tests/it/malformed.rs @@ -287,3 +287,41 @@ fn unreadable_slide_is_reported() { assert_eq!(lost, vec!["ppt/slides/slide2.xml".to_string()]); assert_eq!(deckcraft_pptx::import(&broken).expect("import").slides.len(), 1); } + +fn table_deck(rows: usize, cols: usize) -> deckcraft_model::Presentation { + use deckcraft_model::table::Row; + use deckcraft_model::{Shape, ShapeId, ShapeKind, Table, Xfrm}; + let mut p = defaults::new_presentation(None); + let id = ShapeId(p.alloc_id().expect("ids")); + let t = Table { cols: vec![1.0; cols], rows: vec![Row { height: 1.0, cells: vec![Default::default(); cols] }; rows], ..Default::default() }; + let slide = std::sync::Arc::make_mut(&mut p.slides[0]); + slide.shapes.push(Shape { id, name: "T".into(), xfrm: Some(Xfrm::new(0.0, 0.0, 10.0, 10.0)), kind: ShapeKind::Table(t), ..Default::default() }); + p +} + +/// A table past the cell cap opens with its leading rows and an import warning; saving one that +/// is over the cap is an error, never a silent trim. +#[test] +fn oversized_table_warns_on_import_and_refuses_export() { + let max = deckcraft_model::table::MAX_CELLS; + let cols = 1000; + let rows = max / cols; + // A table exactly at the cap round-trips with no warning. + let bytes = deckcraft_pptx::export(&table_deck(rows, cols)).expect("export at the cap"); + let (_, _, warnings) = deckcraft_pptx::import_report(&bytes).expect("import"); + assert!(warnings.is_empty(), "{warnings:?}"); + // One more row in the file: kept truncated, and reported. + let mut es = entries(&bytes); + let slide = es.iter_mut().find(|(n, _)| n == "ppt/slides/slide1.xml").expect("slide1"); + let xml = String::from_utf8(std::mem::take(&mut slide.1)).expect("utf8"); + let start = xml.rfind("").expect("row end") + "".len(); + slide.1 = format!("{}{}{}", &xml[..end], &xml[start..end], &xml[end..]).into_bytes(); + let (q, _, warnings) = deckcraft_pptx::import_report(&rezip(&es)).expect("import"); + assert_eq!(warnings.len(), 1, "{warnings:?}"); + let t = q.slides[0].shapes.iter().find_map(|s| if let deckcraft_model::ShapeKind::Table(t) = &s.kind { Some(t) } else { None }).expect("table"); + assert_eq!((t.rows.len(), t.cols.len()), (rows, cols)); + // A model table past the cap is refused on export. + let err = deckcraft_pptx::export(&table_deck(rows + 1, cols)).expect_err("over the cap"); + assert!(err.to_string().contains("table"), "{err}"); +}