From 8ff06e215e0523bc1f556e805bf83db8a77a247a Mon Sep 17 00:00:00 2001 From: bfenty <1398236+bfenty@users.noreply.github.com> Date: Tue, 20 Oct 2026 02:48:58 +0000 Subject: [PATCH 1/2] fix(engine): reject unparseable conditional-format formulas at creation Closes #1999 Co-Authored-By: Claude --- crates/engine/src/cmd/insert.rs | 42 ++++++++++++++++ crates/engine/tests/cf_formula_validate.rs | 57 ++++++++++++++++++++++ 2 files changed, 99 insertions(+) create mode 100644 crates/engine/tests/cf_formula_validate.rs diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index 9f044613d..fb804778b 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -1335,6 +1335,47 @@ fn preset_style(name: &str) -> Style { s } +/// #1999: a formula that doesn't parse must never reach the workbook — Excel refuses to open +/// files carrying it, even in repair mode. Validated here so the API and the New Rule dialog +/// (which dispatches this command) share one gate, the way Excel's object model refuses. +fn check_cf_formula(cmd: &str, what: &str, formula: &str) -> Result<()> { + if gridcraft_formula::parse(formula.trim_start_matches('=')).is_err() { + return Err(bad(cmd, format!("invalid {what} formula `{formula}` — it does not parse; Excel would reject the saved file"))); + } + Ok(()) +} + +fn check_cf_value(what: &str, v: &gridcraft_model::features::CfValueKind) -> Result<()> { + if let gridcraft_model::features::CfValueKind::Formula(f) = v { + check_cf_formula("home.conditionalFormat", what, f)?; + } + Ok(()) +} + +fn check_cf_rule(rule: &CfRule) -> Result<()> { + const CMD: &str = "home.conditionalFormat"; + match rule { + CfRule::CellIs { a, b, .. } => { + check_cf_formula(CMD, "comparison value", a)?; + if let Some(b) = b { + check_cf_formula(CMD, "second comparison value", b)?; + } + } + CfRule::Expression { formula, .. } => check_cf_formula(CMD, "rule", formula)?, + CfRule::DataBar { min, max, .. } => { + check_cf_value("data bar minimum", min)?; + check_cf_value("data bar maximum", max)?; + } + CfRule::ColorScale { stops } => { + for (kind, _) in stops { + check_cf_value("color scale threshold", kind)?; + } + } + _ => {} + } + Ok(()) +} + fn add_cf(s: &mut Session, p: &Json) -> Result { let ranges = target_ranges(s, p)?; let sheet = target_sheet(s, p)?; @@ -1443,6 +1484,7 @@ fn add_cf(s: &mut Session, p: &Json) -> Result { }, other => return Err(bad("home.conditionalFormat", format!("unknown rule type `{other}`"))), }; + check_cf_rule(&rule)?; edit(s, |cx| { let sh = cx.sheet_mut(sheet)?; let priority = sh.cond_formats.iter().map(|c| c.priority).max().unwrap_or(0) + 1; diff --git a/crates/engine/tests/cf_formula_validate.rs b/crates/engine/tests/cf_formula_validate.rs new file mode 100644 index 000000000..45aa13275 --- /dev/null +++ b/crates/engine/tests/cf_formula_validate.rs @@ -0,0 +1,57 @@ +//! #1999: a conditional-format rule whose formula doesn't parse must be rejected at +//! creation (Excel's object model refuses it too), not saved and poison the file. + +use gridcraft_engine::Session; +use serde_json::json; + +fn session() -> Session { + let mut s = Session::new(); + s.new_workbook(); + s +} + +#[test] +fn invalid_cf_formulas_are_rejected() { + for rule in [ + // exact repros from the issue's measured table + json!({"type": "expression", "formula": "=A1>", "preset": "lightRedFill"}), + json!({"type": "cellIs", "operator": "greaterThan", "value": "=1+", "preset": "lightRedFill"}), + json!({"type": "dataBar", "min": {"type": "formula", "value": "1+"}}), + // more garbage that the parser rejects + json!({"type": "expression", "formula": "=IF(A1", "preset": "lightRedFill"}), + json!({"type": "cellIs", "operator": "equal", "value": "=((A1", "preset": "lightRedFill"}), + ] { + let err = session() + .execute("home.conditionalFormat", json!({"range": "A1:A5", "rule": rule.clone()})) + .expect_err("invalid formula {rule} must be rejected"); + let msg = err.to_string(); + assert!(msg.to_lowercase().contains("formula"), "error should mention the formula: {msg}"); + } +} + +#[test] +fn valid_cf_formulas_still_work() { + let mut s = session(); + // bare comparison without '=' (the issue: this one saves fine today) + s.execute("home.conditionalFormat", json!({"range": "A1:A5", "rule": {"type": "expression", "formula": "A1>5", "preset": "lightRedFill"}})) + .unwrap(); + // leading '=' is accepted and stripped + s.execute("home.conditionalFormat", json!({"range": "B1:B5", "rule": {"type": "expression", "formula": "=B1>5", "preset": "lightRedFill"}})) + .unwrap(); + // cellIs numeric and text values are not formulas, must pass untouched + s.execute( + "home.conditionalFormat", + json!({"range": "C1:C5", "rule": {"type": "cellIs", "operator": "greaterThan", "value": "5", "preset": "lightRedFill"}}), + ) + .unwrap(); + s.execute( + "home.conditionalFormat", + json!({"range": "D1:D5", "rule": {"type": "cellIs", "operator": "equal", "value": "hello there", "preset": "lightRedFill"}}), + ) + .unwrap(); + // dataBar formula thresholds that parse + s.execute("home.conditionalFormat", json!({"range": "E1:E5", "rule": {"type": "dataBar", "min": {"type": "formula", "value": "MIN(E1:E5)"}, "max": {"type": "formula", "value": "MAX(E1:E5)"}}})) + .unwrap(); + // non-formula rule types are untouched + s.execute("home.conditionalFormat", json!({"range": "F1:F5", "rule": {"type": "duplicate", "preset": "lightRedFill"}})).unwrap(); +} From ad0ce61b8ae554f9a41604381b009d75f6c69e98 Mon Sep 17 00:00:00 2001 From: bfenty <1398236+bfenty@users.noreply.github.com> Date: Tue, 20 Oct 2026 12:15:27 +0000 Subject: [PATCH 2/2] fix(engine): strip one leading = when validating CF formulas Co-Authored-By: Claude --- crates/engine/src/cmd/insert.rs | 4 +++- crates/engine/tests/cf_formula_validate.rs | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index fb804778b..3f3fe80a3 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -1339,7 +1339,9 @@ fn preset_style(name: &str) -> Style { /// files carrying it, even in repair mode. Validated here so the API and the New Rule dialog /// (which dispatches this command) share one gate, the way Excel's object model refuses. fn check_cf_formula(cmd: &str, what: &str, formula: &str) -> Result<()> { - if gridcraft_formula::parse(formula.trim_start_matches('=')).is_err() { + // Strip exactly one leading '=', the way the XLSX writer does: a value like '===A1' + // only parses after several strips and would still be saved unparseable. (#1999 review) + if gridcraft_formula::parse(formula.strip_prefix('=').unwrap_or(formula)).is_err() { return Err(bad(cmd, format!("invalid {what} formula `{formula}` — it does not parse; Excel would reject the saved file"))); } Ok(()) diff --git a/crates/engine/tests/cf_formula_validate.rs b/crates/engine/tests/cf_formula_validate.rs index 45aa13275..5e9d2e280 100644 --- a/crates/engine/tests/cf_formula_validate.rs +++ b/crates/engine/tests/cf_formula_validate.rs @@ -20,6 +20,10 @@ fn invalid_cf_formulas_are_rejected() { // more garbage that the parser rejects json!({"type": "expression", "formula": "=IF(A1", "preset": "lightRedFill"}), json!({"type": "cellIs", "operator": "equal", "value": "=((A1", "preset": "lightRedFill"}), + // multiple leading '=' on a stored-as-typed threshold: the writer strips one, + // the rest must still parse (#1999 review) + json!({"type": "dataBar", "min": {"type": "formula", "value": "===A1"}}), + json!({"type": "dataBar", "max": {"type": "formula", "value": "==1+"}}), ] { let err = session() .execute("home.conditionalFormat", json!({"range": "A1:A5", "rule": rule.clone()}))