collective
Browse / github.com/storytold/wordcraft / patch
Formats: ODT table repeats expand, and malformed XML is an error (#1342) — rebased, conflicts resolved
Formats: ODT table repeats expand, and malformed XML is an error (#1342) — rebased, conflicts resolved Conflict-resolved, rebased copy of #1862 (ODT table repeats expand + malformed XML is an error, #1342). `#1862` is open but shows **CONFLICTING** against current main; CI had never run on it. This branch is `#1862` rebased onto `main` with the six `crates/formats/src/odt.rs` conflicts resolved. ## What was resolved Main added `FTable.widths` (column widths from `table:table-column`) and refactored the body reader into `Body::new`/`Body::feed`/`Body::take_blocks` between the two sides. The conflict resolution keeps both: - `TableB` gains both `widths` (main) and `row_repeat` (#1862); `repeat_
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- rodolfomatos (imported from pull request #4608, open)
- Base
8e986366c7a5a261e058644a9446d4c73aa8a063- Size
- +137 −21 in 2 files
- Review
- pending
- Shared
- 2026-10-28
- Id
28e7ab128dc4fe529203fb7d
How it went
No other operator has reported using it yet.
Diff
raw · crates/formats/src/odt.rs crates/formats/src/sheet.rs
@@ -1152,8 +1152,10 @@ enum Ev { Text(String), } -/// Stream XML events (empty elements become start + end). Malformed XML ends the stream.-fn events(xml: &[u8], mut f: impl FnMut(Ev)) {+/// Stream XML events (empty elements become start + end). `false` when the reader stopped on+/// malformed XML before the end of the stream: a truncated or corrupt file, whose partial+/// content the caller must not mistake for a complete read (#1342).+fn events(xml: &[u8], mut f: impl FnMut(Ev)) -> bool { let mut r = quick_xml::Reader::from_reader(xml); let cfg = r.config_mut(); cfg.trim_text(false);@@ -1184,7 +1186,8 @@ fn events(xml: &[u8], mut f: impl FnMut(Ev)) { }; f(Ev::Text(s)); }- Ok(Event::Eof) | Err(_) => break,+ Ok(Event::Eof) => return true,+ Err(_) => return false, Ok(_) => {} } }@@ -1555,6 +1558,8 @@ struct TableB { hcover: usize, /// Widths in points of the `table:table-column`s so far, 0 where the style gives none. widths: Vec<f32>,+ /// The open row's `table:number-rows-repeated` (bounded).+ row_repeat: usize, } impl TableB {@@ -1565,6 +1570,12 @@ impl TableB { } } +/// An ODF repeat count (`table:number-rows-repeated` / `table:number-columns-repeated`, ODF 1.3+/// §19.678): 1 when absent or unreadable, never zero, at most `max`.+fn repeat_count(a: &Attrs, key: &str, max: usize) -> usize {+ get(a, key).and_then(|v| v.parse::<u64>().ok()).unwrap_or(1).max(1).min(max as u64) as usize+}+ /// A `text:changed-region` read from `text:tracked-changes`. #[derive(Default)] struct Region {@@ -1612,6 +1623,8 @@ struct Body<'a> { lists_saved: Vec<(Vec<(Option<String>, bool)>, Vec<bool>)>, tables: Vec<TableB>, cells: Vec<Cell>,+ /// The open cells' `table:number-columns-repeated`, in step with `cells`.+ cell_repeat: Vec<usize>, in_text: bool, skip: usize, depth: usize,@@ -1663,6 +1676,7 @@ impl<'a> Body<'a> { item_used: Vec::new(), tables: Vec::new(), cells: Vec::new(),+ cell_repeat: Vec::new(), in_text: false, skip: 0, depth: 0,@@ -2102,7 +2116,7 @@ impl Body<'_> { self.skip = 1; return; }- self.tables.push(TableB { rows: Vec::new(), row: Vec::new(), hcover: 0, widths: Vec::new() });+ self.tables.push(TableB { rows: Vec::new(), row: Vec::new(), hcover: 0, widths: Vec::new(), row_repeat: 1 }); } "table-column" => { let w = get(a, "table:style-name").and_then(|n| self.styles.column_width(n)).unwrap_or(0.0);@@ -2116,27 +2130,34 @@ impl Body<'_> { if let Some(t) = self.tables.last_mut() { t.row.clear(); t.hcover = 0;+ t.row_repeat = repeat_count(a, "table:number-rows-repeated", wordcraft_doc::table::MAX_ROWS); } } "table-cell" => { self.end_para(); let span = get(a, "table:number-columns-spanned").and_then(|v| v.trim().parse::<u32>().ok()).unwrap_or(1).clamp(1, 63); let rows = get(a, "table:number-rows-spanned").and_then(|v| v.trim().parse::<u32>().ok()).unwrap_or(1).clamp(1, 1000);+ let repeat = repeat_count(a, "table:number-columns-repeated", wordcraft_doc::table::MAX_COLS); let shading = get(a, "table:style-name").and_then(|s| self.styles.cell_background(s)); let valign = get(a, "table:style-name").and_then(|s| self.styles.cell_valign(s)); if let Some(t) = self.tables.last_mut() {- t.hcover = span as usize - 1;+ // Every copy of a spanning cell is owed its own covered columns.+ t.hcover = (span as usize - 1) * repeat; } self.cells.push(Cell { colspan: span, rowspan: rows, shading, valign, ..Default::default() });+ self.cell_repeat.push(repeat); self.containers.push(Vec::new()); self.lists_saved.push((std::mem::take(&mut self.lists), std::mem::take(&mut self.item_used))); } "covered-table-cell" => { if let Some(t) = self.tables.last_mut() {- if t.hcover > 0 {- t.hcover -= 1;- } else if t.row.len() < 63 {+ let mut n = repeat_count(a, "table:number-columns-repeated", wordcraft_doc::table::MAX_COLS);+ let owed = t.hcover.min(n);+ t.hcover -= owed;+ n -= owed;+ while n > 0 && t.row.len() < wordcraft_doc::table::MAX_COLS { t.row.push(Cell { covered: true, ..Default::default() });+ n -= 1; } } }@@ -2241,6 +2262,7 @@ impl Body<'_> { } self.tables.truncate(d.tables); self.cells.truncate(d.cells);+ self.cell_repeat.truncate(d.cells); self.lists_saved.truncate(d.lists_saved); self.fmt.truncate(d.fmt); self.lists = d.lists;@@ -2391,19 +2413,27 @@ impl Body<'_> { let blocks = if self.containers.len() > 1 { self.containers.pop().unwrap_or_default() } else { Vec::new() }; (self.lists, self.item_used) = self.lists_saved.pop().unwrap_or_default(); let mut cell = self.cells.pop().unwrap_or_default();+ let repeat = self.cell_repeat.pop().unwrap_or(1); cell.blocks = blocks;- if let Some(t) = self.tables.last_mut()- && t.row.len() < 63- {- t.row.push(cell);+ if let Some(t) = self.tables.last_mut() {+ for _ in 0..repeat {+ if t.row.len() >= wordcraft_doc::table::MAX_COLS {+ break;+ }+ t.row.push(cell.clone());+ } } } "table-row" => { if let Some(t) = self.tables.last_mut() { let r = std::mem::take(&mut t.row);- if !r.is_empty() && t.rows.len() < wordcraft_doc::table::MAX_ROWS {- t.rows.push(r);+ if !r.is_empty() {+ let room = wordcraft_doc::table::MAX_ROWS.saturating_sub(t.rows.len());+ for _ in 0..t.row_repeat.min(room) {+ t.rows.push(r.clone());+ } }+ t.row_repeat = 1; } } "table-header-rows" => {@@ -2587,7 +2617,10 @@ fn parse_tracked(bytes: &[u8]) -> Result<(Flow, SectionProps, Option<bool>, Opti }; let parts = styles_xml.as_deref().map(|x| page_parts::read(x, &styles, &mut pages, &mut fetch)).unwrap_or_default(); let mut body = Body::new(&styles, &pages, &mut fetch);- events(&content, |ev| body.feed(ev));+ let complete = events(&content, |ev| body.feed(ev));+ if !complete {+ return Err("ODT: content.xml is not well-formed (the document is truncated or corrupt)".into());+ } let mut blocks = body.take_blocks(); // An open section with nothing in it (after a column section at the end) isn't one: the // section before it ends the document.@@ -3100,6 +3133,62 @@ mod tests { assert_eq!(t2.rows[0][1].valign, Some(VAlign::Bottom)); } + #[test]+ fn table_repeats_expand_rows_and_cells() {+ // #1342: `table:number-rows-repeated` and `table:number-columns-repeated` are counted, so+ // repeated populated rows and cells survive and repeated blanks keep later cells in place.+ let content = format!(+ r#"<office:document-content {NS_T}><office:body><office:text><table:table>+ <table:table-column table:number-columns-repeated="4"/>+ <table:table-row table:number-rows-repeated="2"><table:table-cell table:number-columns-repeated="2"><text:p>R</text:p></table:table-cell><table:table-cell><text:p>End</text:p></table:table-cell></table:table-row>+ <table:table-row><table:table-cell><text:p>A</text:p></table:table-cell><table:covered-table-cell table:number-columns-repeated="2"/><table:table-cell><text:p>B</text:p></table:table-cell></table:table-row>+ <table:table-row><table:table-cell table:number-columns-spanned="2" table:number-columns-repeated="2"><text:p>S</text:p></table:table-cell><table:covered-table-cell/><table:covered-table-cell/></table:table-row>+ </table:table></office:text></office:body></office:document-content>"#+ );+ let f = parse(&package(&content, None)).unwrap();+ let Some(FBlock::Table(t)) = f.blocks.iter().find(|b| matches!(b, FBlock::Table(_))) else { panic!("no table") };+ let texts = |r: &[model::Cell]| r.iter().map(Cell::text).collect::<Vec<_>>();+ // The row repeated twice, its first cell twice.+ assert_eq!(t.rows.len(), 4);+ assert_eq!(texts(&t.rows[0]), ["R", "R", "End"]);+ assert_eq!(texts(&t.rows[1]), ["R", "R", "End"]);+ // A covered cell repeated twice keeps B in the fourth column.+ let cells: Vec<(String, bool)> = t.rows[2].iter().map(|c| (c.text(), c.covered)).collect();+ assert_eq!(cells, [("A".to_string(), false), (String::new(), true), (String::new(), true), ("B".to_string(), false)]);+ // A spanning cell repeated twice, its covered columns owed twice.+ let cells: Vec<(String, u32)> = t.rows[3].iter().map(|c| (c.text(), c.colspan)).collect();+ assert_eq!(cells, [("S".to_string(), 2), ("S".to_string(), 2)]);+ }++ #[test]+ fn table_repeats_stop_at_the_caps() {+ // Hostile repeat counts are clamped to the document table's own limits.+ let content = format!(+ r#"<office:document-content {NS_T}><office:body><office:text><table:table>+ <table:table-row><table:table-cell table:number-columns-repeated="4000000000"><text:p>x</text:p></table:table-cell></table:table-row>+ <table:table-row table:number-rows-repeated="4000000000"><table:table-cell><text:p>y</text:p></table:table-cell></table:table-row>+ </table:table></office:text></office:body></office:document-content>"#+ );+ let f = parse(&package(&content, None)).unwrap();+ let Some(FBlock::Table(t)) = f.blocks.iter().find(|b| matches!(b, FBlock::Table(_))) else { panic!("no table") };+ assert_eq!(t.rows.first().map(Vec::len), Some(wordcraft_doc::table::MAX_COLS));+ assert_eq!(t.rows.len(), wordcraft_doc::table::MAX_ROWS);+ }++ #[test]+ fn malformed_content_is_an_error_not_a_partial_document() {+ // #1342: a valid prefix followed by corruption fails the import, so a partial read+ // cannot be mistaken for the file's content and saved or merged.+ let prefix = format!(r#"<office:document-content {NS_T}><office:body><office:text><text:p>Good</text:p>"#);+ let corrupt = format!("{prefix}<text:p><<bad");+ let err = import(&package(&corrupt, None)).unwrap_err();+ assert!(err.contains("not well-formed"), "{err}");+ // The same prefix, well-formed, still imports.+ let ok = format!("{prefix}</office:text></office:body></office:document-content>");+ let doc = import(&package(&ok, None)).unwrap();+ assert_eq!(doc.plain_text(wordcraft_doc::StoryRef::Body), "Good");+ }+ /// #1379: three master pages (A4 upright; A4 wide with page numbering restarting at 1; A4 /// upright again with numbering continuing) and a two-column `text:section` in between. fn sections_package() -> Vec<u8> {@@ -239,14 +239,16 @@ impl<'a> Package<'a> { // XML /// A streamed XML event with namespace prefixes dropped; elements deeper than-/// [`MAX_XML_DEPTH`] (and their text) are skipped.+/// [`MAX_XML_DEPTH`] (and their text) are skipped. `false` when the reader stopped on malformed+/// XML before the end of the stream: a truncated or corrupt part, whose partial content the+/// caller must not mistake for a complete read (#1342). enum Ev<'e> { Start(&'e BytesStart<'e>), End(Vec<u8>), Text(String), } -fn walk(xml: &[u8], mut f: impl FnMut(Ev<'_>)) {+fn walk(xml: &[u8], mut f: impl FnMut(Ev<'_>)) -> bool { let mut r = quick_xml::Reader::from_reader(xml); let cfg = r.config_mut(); cfg.trim_text(false);@@ -288,7 +290,8 @@ fn walk(xml: &[u8], mut f: impl FnMut(Ev<'_>)) { }; f(Ev::Text(s)); }- Ok(Event::Eof) | Err(_) => break,+ Ok(Event::Eof) => return true,+ Err(_) => return false, Ok(_) => {} } }@@ -434,7 +437,7 @@ fn workbook(pkg: &mut Package<'_>) -> Result<Workbook, String> { let rels = pkg.read(&rels_of(&path)).map(|r| relationships(&r, dir_of(&path))).unwrap_or_default(); let mut listed: Vec<(String, String)> = Vec::new(); let mut date1904 = false;- walk(&xml, |ev| {+ let complete = walk(&xml, |ev| { if let Ev::Start(e) = ev { match e.local_name().as_ref() { b"sheet" if listed.len() < MAX_SHEETS => listed.push((attr(e, b"name").unwrap_or_default(), attr(e, b"id").unwrap_or_default())),@@ -443,6 +446,9 @@ fn workbook(pkg: &mut Package<'_>) -> Result<Workbook, String> { } } });+ if !complete {+ return Err("the workbook part's XML is not well-formed (the file is truncated or corrupt)".into());+ } let sheets = listed .into_iter() .filter_map(|(name, id)| {@@ -622,7 +628,7 @@ fn xlsx_rows(pkg: &mut Package<'_>, path: &str) -> Result<Vec<Vec<String>>, Stri let (mut in_data, mut row_no, mut next_col) = (false, 0u64, 0usize); let mut cell: Option<XCell> = None; let (mut in_v, mut in_is_t, mut phonetic) = (false, false, 0usize);- walk(&xml, |ev| match ev {+ let complete = walk(&xml, |ev| match ev { Ev::Start(e) => match e.local_name().as_ref() { b"sheetData" => in_data = true, b"row" if in_data => {@@ -686,6 +692,9 @@ fn xlsx_rows(pkg: &mut Package<'_>, path: &str) -> Result<Vec<Vec<String>>, Stri _ => {} }, });+ if !complete {+ return Err("the sheet's XML is not well-formed (the file is truncated or corrupt)".into());+ } let mut grid = Grid::default(); for (_, r) in rows { grid.push(r, 1);@@ -856,7 +865,7 @@ fn ods_read(xml: &[u8], want: Option<usize>) -> Result<(Vec<String>, Vec<Vec<Str let mut row: Option<(Vec<String>, u64, u64)> = None; // cells, pending blank cells, repeat let mut cell: Option<OCell> = None; let (mut in_p, mut skip) = (0usize, 0usize);- walk(xml, |ev| match ev {+ let complete = walk(xml, |ev| match ev { Ev::Start(e) => { let n = e.local_name(); let n = n.as_ref();@@ -981,6 +990,9 @@ fn ods_read(xml: &[u8], want: Option<usize>) -> Result<(Vec<String>, Vec<Vec<Str } } });+ if !complete {+ return Err("the spreadsheet's XML is not well-formed (the file is truncated or corrupt)".into());+ } if names.is_empty() { return Err("the spreadsheet has no sheets".into()); }@@ -1134,6 +1146,21 @@ mod tests { } } + #[test]+ fn malformed_xml_is_an_error_not_a_partial_list() {+ // #1342: a valid prefix followed by corruption fails the import, so a partial sheet+ // cannot be mistaken for the file's rows and merged or saved. (Truncation inside a tag:+ // quick-xml tolerates some junk mid-stream, but not an unfinished tag.)+ let ods = r#"<office:document-content xmlns:office="o" xmlns:table="t" xmlns:text="x"><office:body><office:spreadsheet><table:table table:name="Guests"><table:table-row><table:table-cell office:value-type="string"><text:p>Ada</text:p></table:table-cell></table:table-row><table:table-row><table:table-cell office:value"#;+ let book = zip(&[("mimetype", "application/vnd.oasis.opendocument.spreadsheet"), ("content.xml", ods)]);+ let err = read_rows(&book, Pick::Index(0)).unwrap_err();+ assert!(err.contains("not well-formed"), "{err}");+ // The same corruption in an xlsx sheet part.+ let sheet = r#"<worksheet><sheetData><row r="1"><c r="A1" t="inlineStr"><is><t>Ada</t></is></c></row><row r="2"><c r="A2" t="inline"#;+ let err = read_rows(&xlsx(sheet), Pick::Index(0)).unwrap_err();+ assert!(err.contains("not well-formed"), "{err}");+ }+ #[test] fn dates_and_formats() { assert_eq!(custom_date("[$-409]mmmm d, yyyy;@"), Some(DateKind::Date));