From 7c47d0a9ac98b5cab02e7d4eaca7d52c709e1e55 Mon Sep 17 00:00:00 2001 From: Zach Berry <1112351+zberry@users.noreply.github.com> Date: Wed, 4 Nov 2026 19:40:18 -0600 Subject: [PATCH] Table cell commands refuse a malformed `cells` list instead of dropping entries `cells` must be a list of exact [row, col] pairs inside the table; an entry that is not one, a cell past the table, or more than 100000 entries is refused and names the command, instead of being dropped or read as absent (which acted on the selection or the whole table). Borders use checked arithmetic for the neighbour cell. Split out of the 0-based index PR at review. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/table.rs | 64 +++++++++++++++++++++++----------- crates/engine/src/tests.rs | 37 ++++++++++++++++++++ 2 files changed, 80 insertions(+), 21 deletions(-) diff --git a/crates/engine/src/cmd/table.rs b/crates/engine/src/cmd/table.rs index 91ac7ac70..86d753a2c 100644 --- a/crates/engine/src/cmd/table.rs +++ b/crates/engine/src/cmd/table.rs @@ -253,9 +253,10 @@ fn cell_param(p: &Value, key: &str, cmd: &str) -> Result> None | Some(Value::Null) => return Ok(None), Some(v) => v, }; - let idx = |i: usize| v.as_array().and_then(|a| a.get(i)).and_then(Value::as_u64).and_then(|x| usize::try_from(x).ok()); - match (idx(0), idx(1)) { - (Some(r), Some(c)) => Ok(Some((r, c))), + // Exactly [row, col]: an entry with more is as malformed as one with less. + let idx = |x: &Value| x.as_u64().and_then(|x| usize::try_from(x).ok()); + match v.as_array().map(Vec::as_slice) { + Some([r, c]) if let (Some(r), Some(c)) = (idx(r), idx(c)) => Ok(Some((r, c))), _ => Err(bad(cmd, format!("`{key}` is not a valid cell: {v}"))), } } @@ -389,33 +390,54 @@ fn options(s: &mut Session, p: &Value) -> Result { }) } +/// At most this many `cells` in one table cell command. +const MAX_CELLS: usize = 100_000; + /// The cells a command acts on: `cells`, else the selected cell range, else the cell holding the /// caret, else every cell. -fn target_cells(s: &Session, p: &Value) -> Result Vec<(usize, usize)> + use<>> { - let cells: Option> = p - .get("cells") - .and_then(Value::as_array) - .map(|a| a.iter().take(100_000).filter_map(|c| Some((c.get(0)?.as_u64()? as usize, c.get(1)?.as_u64()? as usize))).collect()); +fn target_cells(s: &Session, p: &Value, cmd: &str) -> Result Result> + use<>> { + // `cells` is a list of [row, col]: anything else, or an entry that is not one, is refused, never + // dropped or read as absent (which would act on the selection or the whole table). + let cells: Option> = match p.get("cells") { + None | Some(Value::Null) => None, + Some(Value::Array(a)) if a.len() > MAX_CELLS => { + return Err(bad(cmd, format!("`cells` lists {} cells; at most {MAX_CELLS}", a.len()))); + } + Some(Value::Array(a)) => Some( + a.iter() + .map(|c| { + cell_param(&json!({ "cell": c }), "cell", cmd)?.ok_or_else(|| bad(cmd, format!("`cells` entry {c} is not a [row, col] cell"))) + }) + .collect::>()?, + ), + Some(v) => return Err(bad(cmd, format!("`cells` is a list of [row, col] cells, not {v}"))), + }; let st = s.doc()?; // A selected range grows over the merged cells it touches; with only a caret, the caret's cell is used. let sel = st.selection.cells.map(|(_, a, b)| (a, b, true)).or_else(|| st.selection.text.as_ref().and_then(|t| t.cell).map(|c| (c, c, false))); + let cmd = cmd.to_string(); Ok(move |t: &Table| match (&cells, sel) { - (Some(c), _) => c.clone(), + // Each listed cell must be in the table, so a command never reports success for a cell + // it could not edit. + (Some(c), _) => match c.iter().find(|(r, c)| *r >= t.n_rows() || *c >= t.n_cols()) { + Some((r, c)) => Err(bad(&cmd, format!("no cell [{r}, {c}]: the table has {} rows and {} columns", t.n_rows(), t.n_cols()))), + None => Ok(c.clone()), + }, (None, Some((a, b, range))) => { let (a, b) = if range { cell_block(t, a, b) } else { (a, b) }; // The selection can name cells outside the table; only walk the ones that exist. let (r1, c1) = (a.0.max(b.0).min(t.n_rows().saturating_sub(1)), a.1.max(b.1).min(t.n_cols().saturating_sub(1))); - (a.0.min(b.0)..=r1).flat_map(|r| (a.1.min(b.1)..=c1).map(move |c| (r, c))).collect() + Ok((a.0.min(b.0)..=r1).flat_map(|r| (a.1.min(b.1)..=c1).map(move |c| (r, c))).collect()) } - _ => (0..t.n_rows()).flat_map(|r| (0..t.n_cols()).map(move |c| (r, c))).collect(), + _ => Ok((0..t.n_rows()).flat_map(|r| (0..t.n_cols()).map(move |c| (r, c))).collect()), }) } fn cell_fill(s: &mut Session, p: &Value) -> Result { let fill = if bool_or(p, "none", false) { Some(deckcraft_model::Fill::None) } else { color_param(p, "color").map(deckcraft_model::Fill::solid) }; - let targets = target_cells(s, p)?; + let targets = target_cells(s, p, "table.cellFill")?; with_table(s, p, "table.cellFill", move |t| { - for (r, c) in targets(t) { + for (r, c) in targets(t)? { if let Some(cell) = t.cell_mut(r, c) { cell.fill.clone_from(&fill); } @@ -447,9 +469,9 @@ fn borders(s: &mut Session, p: &Value) -> Result { } // "none" clears to no line, so the table style's own lines don't show through either. let line = if which == "none" { Line::none() } else { line }; - let targets = target_cells(s, p)?; + let targets = target_cells(s, p, "table.borders")?; with_table(s, p, "table.borders", move |t| { - let list = targets(t); + let list = targets(t)?; let (Some(r0), Some(r1), Some(c0), Some(c1)) = (list.iter().map(|x| x.0).min(), list.iter().map(|x| x.0).max(), list.iter().map(|x| x.1).min(), list.iter().map(|x| x.1).max()) else { @@ -463,9 +485,9 @@ fn borders(s: &mut Session, p: &Value) -> Result { } let n = match side { LEFT => c.checked_sub(1).map(|c| (r, c, RIGHT)), - RIGHT => (c + 1 < cols).then_some((r, c + 1, LEFT)), + RIGHT => c.checked_add(1).filter(|c| *c < cols).map(|c| (r, c, LEFT)), TOP => r.checked_sub(1).map(|r| (r, c, BOTTOM)), - _ => (r + 1 < rows).then_some((r + 1, c, TOP)), + _ => r.checked_add(1).filter(|r| *r < rows).map(|r| (r, c, TOP)), }; if let Some((r, c, side)) = n && let Some(cell) = t.cell_mut(r, c) @@ -523,9 +545,9 @@ fn cell_margins(s: &mut Session, p: &Value) -> Result { }; let side = |k: &str, i: usize| f64_param(p, k).filter(|v| v.is_finite()).map_or(base[i], |v| v.clamp(0.0, 400.0)); let m = [side("left", 0), side("right", 1), side("top", 2), side("bottom", 3)]; - let targets = target_cells(s, p)?; + let targets = target_cells(s, p, "table.cellMargins")?; with_table(s, p, "table.cellMargins", move |t| { - for (r, c) in targets(t) { + for (r, c) in targets(t)? { if let Some(cell) = t.cell_mut(r, c) { cell.margins = Some(m); } @@ -560,9 +582,9 @@ fn cell_align(s: &mut Session, p: &Value) -> Result { o => return Err(bad("table.cellAlign", format!("unknown anchor {o:?}; one of top, middle, bottom"))), }), }; - let targets = target_cells(s, p)?; + let targets = target_cells(s, p, "table.cellAlign")?; with_table(s, p, "table.cellAlign", move |t| { - for (r, c) in targets(t) { + for (r, c) in targets(t)? { if let Some(cell) = t.cell_mut(r, c) { if anchor.is_some() { cell.anchor = anchor; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 12fac5efc..e27c1e08b 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -6509,3 +6509,40 @@ fn save_to_image_path_is_an_error_not_zip_bytes() { } let _ = std::fs::remove_dir_all(&dir); } + +/// Table cell commands refuse a `cells` that is not a list of [row, col] cells in the table, +/// instead of dropping the entry or reading the list as absent (which acts on the selection). +#[test] +fn table_cell_commands_refuse_a_malformed_cells_list() { + let mut s = session(); + let t = table_at(&mut s, &[100.0, 100.0], json!([["a", "b"], ["c", "d"]])); + let rev = s.doc().unwrap().revision; + for cells in [ + json!([[-1, 0]]), + json!([[0]]), + json!([[0, 0], "x"]), + json!([[0, 0, "x"]]), + json!([[0, 2]]), + json!([[0, u64::MAX]]), + json!("0,0"), + json!({"0": 0}), + ] { + for (cmd, extra) in [ + ("table.cellFill", json!({"color": "#FF0000"})), + ("table.cellMargins", json!({"left": 4})), + ("table.cellAlign", json!({"align": "center"})), + ("table.borders", json!({"which": "right"})), + ] { + let mut p = extra.clone(); + p["id"] = json!(t); + p["cells"] = cells.clone(); + let e = s.execute(cmd, &p).unwrap_err().to_string(); + assert!(e.contains(cmd) && e.contains("cell"), "{cmd} {cells}: {e}"); + } + } + assert_eq!(s.doc().unwrap().revision, rev, "nothing changed"); + let many = json!(vec![[0, 0]; 100_001]); + assert!(s.execute("table.cellFill", &json!({"id": t, "color": "#FF0000", "cells": many})).unwrap_err().to_string().contains("at most")); + s.execute("table.cellFill", &json!({"id": t, "color": "#FF0000", "cells": [[1, 1]]})).unwrap(); + assert!(table_of(&s, t).cell(1, 1).unwrap().fill.is_some() && table_of(&s, t).cell(0, 0).unwrap().fill.is_none()); +}