From 0ce545fcb82d8223dfcf85af34d13e10e0fe7e03 Mon Sep 17 00:00:00 2001 From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com> Date: Thu, 22 Oct 2026 18:46:51 +0200 Subject: [PATCH 1/2] Reject invalid row/col index in table row and column commands Found while testing crates/engine/src/cmd/table.rs:234-238 (at), via crates/engine/src/cmd/mod.rs:318 (usize_param) Co-Authored-By: Claude Haiku 5.5 Co-Authored-By: Claude Sonnet 5.5 --- crates/engine/src/cmd/table.rs | 16 ++++++++++------ crates/engine/src/tests.rs | 20 ++++++++++++++++++++ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/cmd/table.rs b/crates/engine/src/cmd/table.rs index 8ab95549..a65b61e7 100644 --- a/crates/engine/src/cmd/table.rs +++ b/crates/engine/src/cmd/table.rs @@ -231,10 +231,11 @@ fn cell_param(p: &Value, key: &str) -> Option<(usize, usize)> { } /// The table's row/col to act on: params, the edited cell, or the selected cells. -fn at(s: &Session, p: &Value) -> (usize, usize) { +/// A present but invalid `row`/`col` is an error; only an absent one falls back. +fn at(s: &Session, p: &Value, cmd: &str) -> Result<(usize, usize)> { let st = s.active(); let cell = st.and_then(|d| d.selection.text.as_ref().and_then(|t| t.cell)).or_else(|| st.and_then(|d| d.selection.cells.map(|c| c.1))); - (usize_param(p, "row").or(cell.map(|c| c.0)).unwrap_or(0), usize_param(p, "col").or(cell.map(|c| c.1)).unwrap_or(0)) + Ok((opt_usize_param(p, "row", cmd)?.or(cell.map(|c| c.0)).unwrap_or(0), opt_usize_param(p, "col", cmd)?.or(cell.map(|c| c.1)).unwrap_or(0))) } fn with_table(s: &mut Session, p: &Value, cmd: &str, f: impl Fn(&mut Table) -> Result<()>) -> Result { @@ -283,7 +284,7 @@ fn with_table(s: &mut Session, p: &Value, cmd: &str, f: impl Fn(&mut Table) -> R } fn rows_cols(s: &mut Session, p: &Value, op: &str) -> Result { - let (r, c) = at(s, p); + let (r, c) = at(s, p, &format!("table.{op}"))?; let op = op.to_string(); with_table(s, p, "table", move |t| { match op.as_str() { @@ -319,7 +320,10 @@ fn merge(s: &mut Session, p: &Value) -> Result { } fn split(s: &mut Session, p: &Value) -> Result { - let cell = cell_param(p, "cell").unwrap_or_else(|| at(s, p)); + let cell = match cell_param(p, "cell") { + Some(c) => c, + None => at(s, p, "table.split")?, + }; with_table(s, p, "table.split", move |t| if t.split(cell.0, cell.1) { Ok(()) } else { Err(bad("table.split", "that cell isn't merged")) }) } @@ -531,7 +535,7 @@ fn distribute(s: &mut Session, p: &Value, rows: bool) -> Result { } fn column_width(s: &mut Session, p: &Value) -> Result { - let (_, c) = at(s, p); + let (_, c) = at(s, p, "table.columnWidth")?; let w = f64_param(p, "width").ok_or_else(|| bad("table.columnWidth", "missing `width`"))?.clamp(4.0, 4000.0); let keep_total = bool_or(p, "keepTotal", false); with_table(s, p, "table.columnWidth", move |t| { @@ -560,7 +564,7 @@ fn column_width(s: &mut Session, p: &Value) -> Result { } fn row_height(s: &mut Session, p: &Value) -> Result { - let (r, _) = at(s, p); + let (r, _) = at(s, p, "table.rowHeight")?; let h = f64_param(p, "height").ok_or_else(|| bad("table.rowHeight", "missing `height`"))?.clamp(4.0, 4000.0); with_table(s, p, "table.rowHeight", move |t| { if let Some(x) = t.rows.get_mut(r) { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index dd64b08c..6aaf51ae 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3672,6 +3672,26 @@ fn table_insert_below_or_right_of_a_huge_index_appends_at_the_end() { assert_eq!(t.cell(0, 1).unwrap().text.text(), "b", "the original columns keep their place"); } +#[test] +fn table_row_col_commands_reject_an_invalid_index() { + let mut s = session(); + let id = table_at(&mut s, &[100.0, 100.0], json!([["a", "b"], ["c", "d"], ["e", "f"]])); + for (cmd, key) in [ + ("table.deleteRow", "row"), + ("table.deleteColumn", "col"), + ("table.insertRowAbove", "row"), + ("table.insertColumnLeft", "col"), + ("table.split", "row"), + ] { + for bad in [json!(-1), json!(1.5), json!("1")] { + let r = s.execute(cmd, &json!({"id": id.0, key: bad})); + assert!(r.is_err(), "{cmd} {key}={bad} should be rejected, got {r:?}"); + let t = table_of(&s, id); + assert_eq!((t.n_rows(), t.n_cols()), (3, 2), "{cmd} {key}={bad} must leave the table unchanged"); + } + } +} + #[test] fn table_column_width_of_a_huge_column_is_ignored() { let mut s = session(); From 9679c6e16f5185bf2fafac026445e66b65c47439 Mon Sep 17 00:00:00 2001 From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com> Date: Sat, 24 Oct 2026 01:10:13 +0200 Subject: [PATCH 2/2] fix: Name registered table commands in errors and reject invalid cell params Co-Authored-By: Claude Sonnet 5.5 --- crates/engine/src/cmd/table.rs | 65 ++++++++++++++++++++++++---------- crates/engine/src/tests.rs | 29 +++++++++++++++ 2 files changed, 75 insertions(+), 19 deletions(-) diff --git a/crates/engine/src/cmd/table.rs b/crates/engine/src/cmd/table.rs index a65b61e7..ca81ded4 100644 --- a/crates/engine/src/cmd/table.rs +++ b/crates/engine/src/cmd/table.rs @@ -10,22 +10,40 @@ use crate::{DocState, Result, Session}; pub fn specs() -> Vec { vec![ cmd!("table.insertRowAbove", "Insert Above", ["Table Layout", "Rows & Columns"], None, "{row?, id?}", has_selection, |s, p| rows_cols( - s, p, "rowAbove" + s, + p, + "rowAbove", + "table.insertRowAbove" )), cmd!("table.insertRowBelow", "Insert Below", ["Table Layout", "Rows & Columns"], None, "{row?, id?}", has_selection, |s, p| rows_cols( - s, p, "rowBelow" + s, + p, + "rowBelow", + "table.insertRowBelow" )), cmd!("table.insertColumnLeft", "Insert Left", ["Table Layout", "Rows & Columns"], None, "{col?, id?}", has_selection, |s, p| rows_cols( - s, p, "colLeft" + s, + p, + "colLeft", + "table.insertColumnLeft" )), cmd!("table.insertColumnRight", "Insert Right", ["Table Layout", "Rows & Columns"], None, "{col?, id?}", has_selection, |s, p| rows_cols( - s, p, "colRight" + s, + p, + "colRight", + "table.insertColumnRight" )), cmd!("table.deleteRow", "Delete Rows", ["Table Layout", "Rows & Columns"], None, "{row?, id?}", has_selection, |s, p| rows_cols( - s, p, "delRow" + s, + p, + "delRow", + "table.deleteRow" )), cmd!("table.deleteColumn", "Delete Columns", ["Table Layout", "Rows & Columns"], None, "{col?, id?}", has_selection, |s, p| rows_cols( - s, p, "delCol" + s, + p, + "delCol", + "table.deleteColumn" )), cmd!("table.merge", "Merge Cells", ["Table Layout", "Merge"], None, "{from: [r, c], to: [r, c], id?}", has_selection, merge), cmd!( @@ -225,9 +243,18 @@ impl TableFrame { } } -fn cell_param(p: &Value, key: &str) -> Option<(usize, usize)> { - let a = p.get(key)?.as_array()?; - Some((a.first()?.as_u64()? as usize, a.get(1)?.as_u64()? as usize)) +/// A `[r, c]` cell param: `Ok(None)` when absent (callers may fall back to the selection), `Err` when +/// present but not a valid cell, so a bad cell never acts on the selection. +fn cell_param(p: &Value, key: &str, cmd: &str) -> Result> { + let v = match p.get(key) { + 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))), + _ => Err(bad(cmd, format!("`{key}` is not a valid cell: {v}"))), + } } /// The table's row/col to act on: params, the edited cell, or the selected cells. @@ -283,10 +310,10 @@ fn with_table(s: &mut Session, p: &Value, cmd: &str, f: impl Fn(&mut Table) -> R Ok(r) } -fn rows_cols(s: &mut Session, p: &Value, op: &str) -> Result { - let (r, c) = at(s, p, &format!("table.{op}"))?; +fn rows_cols(s: &mut Session, p: &Value, op: &str, name: &'static str) -> Result { + let (r, c) = at(s, p, name)?; let op = op.to_string(); - with_table(s, p, "table", move |t| { + with_table(s, p, name, move |t| { match op.as_str() { "rowAbove" => t.insert_row(r), "rowBelow" => t.insert_row(r.saturating_add(1)), @@ -294,12 +321,12 @@ fn rows_cols(s: &mut Session, p: &Value, op: &str) -> Result { "colRight" => t.insert_col(c.saturating_add(1)), "delRow" => { if !t.delete_row(r) { - return Err(bad("table.deleteRow", "can't delete the last row")); + return Err(bad(name, "can't delete the last row")); } } _ => { if !t.delete_col(c) { - return Err(bad("table.deleteColumn", "can't delete the last column")); + return Err(bad(name, "can't delete the last column")); } } } @@ -309,8 +336,8 @@ fn rows_cols(s: &mut Session, p: &Value, op: &str) -> Result { fn merge(s: &mut Session, p: &Value) -> Result { let sel = s.doc()?.selection.cells; - let from = cell_param(p, "from").or(sel.map(|c| c.1)).ok_or_else(|| bad("table.merge", "missing `from`"))?; - let to = cell_param(p, "to").or(sel.map(|c| c.2)).ok_or_else(|| bad("table.merge", "missing `to`"))?; + let from = cell_param(p, "from", "table.merge")?.or(sel.map(|c| c.1)).ok_or_else(|| bad("table.merge", "missing `from`"))?; + let to = cell_param(p, "to", "table.merge")?.or(sel.map(|c| c.2)).ok_or_else(|| bad("table.merge", "missing `to`"))?; with_table( s, p, @@ -320,7 +347,7 @@ fn merge(s: &mut Session, p: &Value) -> Result { } fn split(s: &mut Session, p: &Value) -> Result { - let cell = match cell_param(p, "cell") { + let cell = match cell_param(p, "cell", "table.split")? { Some(c) => c, None => at(s, p, "table.split")?, }; @@ -579,8 +606,8 @@ fn select_cells(s: &mut Session, p: &Value) -> Result { Some(i) => i, None => s.doc()?.selection.shapes.first().copied().ok_or_else(|| bad("table.selectCells", "select a table"))?, }; - let from = cell_param(p, "from").unwrap_or((0, 0)); - let to = cell_param(p, "to").unwrap_or(from); + let from = cell_param(p, "from", "table.selectCells")?.unwrap_or((0, 0)); + let to = cell_param(p, "to", "table.selectCells")?.unwrap_or(from); s.select(|_, sel| { sel.cells = Some((id, from, to)); sel.shapes = vec![id]; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 6aaf51ae..d7f71999 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3692,6 +3692,35 @@ fn table_row_col_commands_reject_an_invalid_index() { } } +#[test] +fn table_cell_commands_reject_an_invalid_cell_and_name_the_registered_command() { + let mut s = session(); + let id = table_at(&mut s, &[100.0, 100.0], json!([["a", "b"], ["c", "d"], ["e", "f"]])); + s.execute("table.selectCells", &json!({"id": id.0, "from": [0, 0], "to": [1, 1]})).unwrap(); + for (cmd, params) in [ + ("table.split", json!({"id": id.0, "cell": [-1, 0]})), + ("table.merge", json!({"id": id.0, "from": [-1, 0], "to": [1, 1]})), + ("table.merge", json!({"id": id.0, "from": [0, 0], "to": "x"})), + ("table.rowHeight", json!({"id": id.0, "row": -1, "height": 50.0})), + ("table.rowHeight", json!({"id": id.0, "row": 1.5, "height": 50.0})), + ("table.columnWidth", json!({"id": id.0, "col": -1, "width": 50.0})), + ("table.columnWidth", json!({"id": id.0, "col": "1", "width": 50.0, "keepTotal": true})), + ] { + let before = table_of(&s, id); + let (rows, cols) = (before.rows.iter().map(|r| r.height).collect::>(), before.cols.clone()); + let r = s.execute(cmd, ¶ms); + assert!(r.is_err(), "{cmd} {params} should be rejected, got {r:?}"); + let t = table_of(&s, id); + assert_eq!((t.n_rows(), t.n_cols()), (3, 2), "{cmd} {params} must leave the table unchanged"); + assert_eq!(t.rows.iter().map(|r| r.height).collect::>(), rows, "{cmd} {params} must not change row heights"); + assert_eq!(t.cols, cols, "{cmd} {params} must not change column widths"); + } + let e = s.execute("table.deleteRow", &json!({"id": id.0, "row": -1})).unwrap_err().to_string(); + assert!(e.contains("table.deleteRow"), "error should name the registered command: {e}"); + let e = s.execute("table.insertColumnLeft", &json!({"id": id.0, "col": -1})).unwrap_err().to_string(); + assert!(e.contains("table.insertColumnLeft"), "error should name the registered command: {e}"); +} + #[test] fn table_column_width_of_a_huge_column_is_ignored() { let mut s = session();