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 <noreply@anthropic.com>

crates/engine/src/cmd/table.rs

@@ -253,9 +253,10 @@ fn cell_param(p: &Value, key: &str, cmd: &str) -> Result<Option<(usize, usize)>>
         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<Value> {
     })
 }
 
+/// 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<impl Fn(&Table) -> Vec<(usize, usize)> + use<>> {
-    let cells: Option<Vec<(usize, usize)>> = 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<impl Fn(&Table) -> Result<Vec<(usize, usize)>> + 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<Vec<(usize, usize)>> = 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::<Result<_>>()?,
+        ),
+        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<Value> {
     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<Value> {
     }
     // "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<Value> {
             }
             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<Value> {
     };
     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<Value> {
             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;

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());
+}