From 4548c6f7fc3fe6e758dc4feeb5a54668a9d31103 Mon Sep 17 00:00:00 2001 From: harrisonoconnorhover <130776285+harrisonoconnorhover@users.noreply.github.com> Date: Fri, 16 Oct 2026 07:44:44 -0400 Subject: [PATCH] fix(xlsx): bound repeated column expansion (#1022) --- crates/xlsx/src/sheet_read.rs | 12 +++++- crates/xlsx/src/tests/malformed.rs | 65 ++++++++++++++++++++++++++++++ docs/decisions.md | 9 +++++ docs/file-format-parity.md | 9 ++++- 4 files changed, 92 insertions(+), 3 deletions(-) diff --git a/crates/xlsx/src/sheet_read.rs b/crates/xlsx/src/sheet_read.rs index 0194d2268..a1094ef91 100644 --- a/crates/xlsx/src/sheet_read.rs +++ b/crates/xlsx/src/sheet_read.rs @@ -38,6 +38,7 @@ struct RowState { } const MAX_HYPERLINK_CELLS: u64 = 10_000; +const MAX_COLUMN_ASSIGNMENTS: u64 = 1_000_000; pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result { let bytes = cx.pkg.read(part)?.ok_or_else(|| IoError::Format(format!("missing part {part}")))?; @@ -61,6 +62,7 @@ pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result, part: &str, name: &str) -> Result read_cols(cx, e, &mut sheet), + "cols" => read_cols(cx, e, &mut sheet, &mut column_assignments_left)?, "mergeCells" => { for m in e.kids("mergeCell") { if let Some(r) = m.attr("ref").and_then(RangeRef::parse) @@ -300,7 +302,7 @@ fn read_views(e: &El, sheet: &mut Sheet) { } } -fn read_cols(cx: &Ctx<'_>, e: &El, sheet: &mut Sheet) { +fn read_cols(cx: &Ctx<'_>, e: &El, sheet: &mut Sheet, remaining: &mut u64) -> Result<(), IoError> { for c in e.kids("col") { let (Some(min), Some(max)) = (c.attr_u32("min"), c.attr_u32("max")) else { continue }; if min == 0 || min > MAX_COLS || max < min { @@ -328,10 +330,16 @@ fn read_cols(cx: &Ctx<'_>, e: &El, sheet: &mut Sheet) { if info == LineInfo::default() || (info.size == Some(sheet.default_col_width) && LineInfo { size: None, ..info } == LineInfo::default()) { continue; } + // Overlapping declarations repeat work even when the final map stays small. + // Charge the clipped span before expansion, sharing the budget across blocks. + *remaining = remaining + .checked_sub(u64::from(max - min + 1)) + .ok_or_else(|| IoError::TooLarge(format!("sheet \"{}\": more than {MAX_COLUMN_ASSIGNMENTS} expanded column assignments", sheet.name)))?; for col in (min - 1)..max { sheet.cols.insert(col, info); } } + Ok(()) } fn read_row(cx: &mut Ctx<'_>, sheet: &mut Sheet, st: &mut RowState, row: &El) { diff --git a/crates/xlsx/src/tests/malformed.rs b/crates/xlsx/src/tests/malformed.rs index d029acaa4..6d9d4379f 100644 --- a/crates/xlsx/src/tests/malformed.rs +++ b/crates/xlsx/src/tests/malformed.rs @@ -256,3 +256,68 @@ fn hostile_formula_text_does_not_overflow_the_stack() { assert_eq!(a_len, 1_000_001); assert_eq!(a_value, gridcraft_core::Value::Number(3.0)); } + +#[test] +fn repeated_column_ranges_have_one_worksheet_work_budget() { + // 62 * 16,384 = 1,015,808 assignments, while the map holds at most 16,384 columns. + // This stays small even on the unbounded reader, but exceeds the one-million work cap. + let col = r#""#; + for (first, second) in [(62, 0), (31, 31)] { + let body = format!("{}{}", col.repeat(first), col.repeat(second)); + assert!(matches!(read_xlsx(&minimal(&body, &[], "", "")), Err(IoError::TooLarge(_))), "blocks: {first}, {second}"); + } +} + +#[test] +fn column_work_budget_counts_only_clipped_nondefault_assignments() { + let full = r#""#; + // Default-only records and invalid coordinates do not expand into column assignments. + let noise = format!( + "{}{}", + r#""#.repeat(62), + r#" + + + "# + ); + // The last declaration clips to 576 columns: 61 * 16,384 + 576 = 1,000,000. + let cols = format!("{noise}{}", full.repeat(61)); + let body = format!("{cols}"); + let (wb, _) = read_xlsx(&minimal(&body, &[], "", "")).unwrap(); + let sheet = wb.sheet(0).unwrap(); + assert_eq!(sheet.cols.len(), 16_384); + assert_eq!(sheet.cols.get(&15_807).unwrap().size, Some(crate::tables::col_width_to_px(10.0))); + assert_eq!(sheet.cols.get(&15_808).unwrap().size, Some(crate::tables::col_width_to_px(20.0))); + assert_eq!(sheet.cols.get(&16_383).unwrap().size, Some(crate::tables::col_width_to_px(20.0))); + + let body = format!("{cols}"); + assert!(matches!(read_xlsx(&minimal(&body, &[], "", "")), Err(IoError::TooLarge(_)))); +} + +#[test] +fn ordinary_overlapping_column_ranges_keep_last_declaration() { + let styles = r#" + "#; + let rels = r#""#; + let body = r#" + + + "#; + let (wb, _) = read_xlsx(&minimal(body, &[("xl/styles.xml", styles)], rels, "")).unwrap(); + let sheet = wb.sheet(0).unwrap(); + assert_eq!(sheet.cols.len(), 5); + let first = sheet.cols.get(&0).unwrap(); + assert_eq!(first.size, Some(crate::tables::col_width_to_px(10.0))); + assert!(first.hidden); + assert_eq!(wb.styles.get(first.style.unwrap()).num_fmt.as_str(), "0.00"); + for col in 1..4 { + let info = sheet.cols.get(&col).unwrap(); + assert_eq!(info.size, Some(crate::tables::col_width_to_px(20.0))); + assert!(!info.hidden); + assert_eq!(info.style, None); + } + let last = sheet.cols.get(&4).unwrap(); + assert_eq!(last.size, None); + assert!(last.hidden); + assert_eq!(wb.styles.get(last.style.unwrap()).num_fmt.as_str(), "0.00"); +} diff --git a/docs/decisions.md b/docs/decisions.md index 7b0a99a38..2aa18069f 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -1,5 +1,14 @@ # Decisions +## Bound repeated XLSX column expansion (#1022) + +Limit each worksheet to 1,000,000 nondefault column assignments across all `` blocks. +This allows more than 60 full-grid passes while bounding repeated work on overlapping spans; +counting only the final map size would not limit that work. Charge each clipped span before +inserting it, after the existing default-record skip. Exceeding the limit returns the existing +`IoError::TooLarge`, which aborts the import instead of silently dropping a partially read sheet. +Keep the current last-wins semantics rather than introduce interval storage or normalization. + ## Formula localization uses the existing interface preference PR #98 builds on the persisted `UiState.language` introduced by #31 and extended by #104. diff --git a/docs/file-format-parity.md b/docs/file-format-parity.md index 158149fb3..760109480 100644 --- a/docs/file-format-parity.md +++ b/docs/file-format-parity.md @@ -1,6 +1,6 @@ # File-format parity -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-14 · **Change:** trivial (formulas over Excel's 8,192 characters, and `%` or call chains past the nesting limit, are parse errors instead of stack overflows when a file is opened; previously trivial: XLSX chart sheets are kept on save (#727)) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-16 · **Change:** minor (XLSX column declarations share a per-sheet expansion-work limit, #1022) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Every format Excel opens or saves, with GridCraft's read and write support, fidelity and tests. Part of [target-app-parity.md](target-app-parity.md). **File formats: ~50% ready, 80–120 h** @@ -13,6 +13,12 @@ support comes from `crates/engine/src/io.rs` (`FileKind`), `crates/xlsx/src` and ## Workbook formats +XLSX column declarations are limited to 1,000,000 expanded column assignments per worksheet, +including repeated overlaps across `` blocks. Spans are clipped to the grid and records +that would be skipped as defaults are excluded before charging the limit. Excess work returns +the existing file-too-large error; accepted overlaps retain their existing last-wins behavior +(#1022). + Ink created with Draw is written as standard, unfilled DrawingML polylines, without visible point captions or private metadata. Supported numeric polylines reopen as editable ink, retaining points, colour, integer width and anchor. Curves, filled paths, transforms and unsupported line styles are @@ -85,6 +91,7 @@ refresh. Counted under features (Get & Transform, ~10% ready), not here. | Date | Change | Summary | |---|---|---| +| 2026-10-16 | minor | XLSX column imports refuse more than 1,000,000 cumulative expanded assignments per sheet before expanding the next span; repeated overlaps cannot bypass the work limit (#1022) | | 2026-10-14 | trivial | Formula parser: text longer than Excel's 8,192 characters, and postfix chains (`1%%%…`, `f(1)(1)…`) past the 256-level nesting limit, are parse errors (the cell keeps its formula text and cached value) instead of trees deep enough to overflow the stack when a hostile file is opened, saved or dropped | | 2026-10-12 | trivial | XLSX chart sheets are kept on save (#727) | | 2026-10-12 | trivial | CSV and TSV export is capped at 512 MiB and refuses a larger used range with an error (#528) |