From a005d8235f65f950b19a404b76df2c44c1b0dd8e Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 06:15:24 -0400 Subject: [PATCH] M2.9: close protection gaps a trusted script or other sheet would slip through (sec scan findings 17,20,24) (#2420) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - scripts: refuse `window.*` steps like file and link steps — activating another window repointed every later step at a different open document (finding 17). - format: on a protected sheet, formatting may not change a cell's protection flags: the format-cells permission can't drop `locked` and then authorize content edits the password gates (finding 20). - sheet.moveColumns: the insert/delete protection checks now carry the command's own `sheet` parameter, so the sheet modified is the sheet authorized, not whichever is active (finding 24). Tests: unlock-via-format refused, cross-sheet move refused, window switch refused, plus the positive controls. Engine: 462 tests pass; clippy clean. --- crates/engine/src/cmd/extra.rs | 5 +++ crates/engine/src/cmd/format.rs | 16 ++++++++++ crates/engine/src/cmd/sheet.rs | 6 ++-- crates/engine/src/tests.rs | 56 +++++++++++++++++++++++++++++++++ 4 files changed, 81 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/extra.rs b/crates/engine/src/cmd/extra.rs index 009d77f2a..7396d5356 100644 --- a/crates/engine/src/cmd/extra.rs +++ b/crates/engine/src/cmd/extra.rs @@ -1034,6 +1034,11 @@ fn denied_script_step(id: &str, params: &Json) -> Option { if id.starts_with("file.") || id.starts_with("link.") { return Some(format!("scripts can't run `{id}`: it reaches outside the workbook")); } + // Scripts are scoped to their workbook: a step that switches the active window would + // point every later step at another open document, so those are refused too. + if id.starts_with("window.") { + return Some(format!("scripts can't run `{id}`: it reaches outside the workbook")); + } if params.get("path").is_some() { return Some(format!("scripts can't pass a file `path` to `{id}`")); } diff --git a/crates/engine/src/cmd/format.rs b/crates/engine/src/cmd/format.rs index cd28cfbfa..691b531c7 100644 --- a/crates/engine/src/cmd/format.rs +++ b/crates/engine/src/cmd/format.rs @@ -233,10 +233,23 @@ fn with(p: &Json, key: &str, v: Json) -> Json { pub(crate) fn apply_style(s: &mut Session, p: &Json, f: impl Fn(&mut Style)) -> Result { let sheet = target_sheet(s, p)?; let ranges = target_ranges(s, p)?; + let guarded = s.doc()?.wb.sheet(sheet).is_some_and(|sh| sh.protection.is_some()); if s.doc()?.wb.sheet(sheet).is_some_and(|sh| sh.protection.as_ref().is_some_and(|pr| !pr.format_cells)) { return Err(EngineError::Other("The cell or chart you're trying to change is on a protected sheet.".into())); } edit(s, |cx| { + // Formatting on a protected sheet keeps the cell's permission flags: a format call + // that flips `locked` from the format permission would authorize content edits that + // the password should still gate, so those flags may not change (Excel keeps that + // checkbox disabled while the sheet is protected). + const FLAG_GUARD: &str = "You can't change cell protection on a protected sheet. Unprotect it first."; + macro_rules! guard_flags { + ($old:expr, $new:expr) => { + if guarded && cx.wb.styles.get($old).protection != cx.wb.styles.get($new).protection { + return Err(EngineError::Other(FLAG_GUARD.into())); + } + }; + } let mut cache: std::collections::HashMap = std::collections::HashMap::new(); for r in &ranges { let full_rows = r.is_full_rows(); @@ -251,6 +264,7 @@ pub(crate) fn apply_style(s: &mut Session, p: &Json, f: impl Fn(&mut Style)) -> info.and_then(|i| i.style).unwrap_or_default() }; let new = *cache.entry(cur).or_insert_with(|| cx.wb.styles.derive(cur, &f)); + guard_flags!(cur, new); let sh = cx.sheet_mut(sheet)?; let map = if full_rows && !full_cols { &mut sh.rows } else { &mut sh.cols }; map.entry(line).or_default().style = Some(new); @@ -260,6 +274,7 @@ pub(crate) fn apply_style(s: &mut Session, p: &Json, f: impl Fn(&mut Style)) -> cx.wb.sheet(sheet).map(|sh| sh.cells.iter_range(*r).map(|(c, x)| (c, x.style)).collect()).unwrap_or_default(); for (c, st) in &cells { let new = *cache.entry(*st).or_insert_with(|| cx.wb.styles.derive(*st, &f)); + guard_flags!(*st, new); cx.sheet_mut(sheet)?.set_style(*c, new); } // Formatting touched these cells: re-fit their rows (Excel auto-fits). @@ -276,6 +291,7 @@ pub(crate) fn apply_style(s: &mut Session, p: &Json, f: impl Fn(&mut Style)) -> let st = cx.wb.sheet(sheet).map(|sh| sh.style_id(c)).unwrap_or_default(); let new = *cache.entry(st).or_insert_with(|| cx.wb.styles.derive(st, &f)); if new != st { + guard_flags!(st, new); cx.sheet_mut(sheet)?.set_style(c, new); } } diff --git a/crates/engine/src/cmd/sheet.rs b/crates/engine/src/cmd/sheet.rs index a453f250b..e6caa1dc7 100644 --- a/crates/engine/src/cmd/sheet.rs +++ b/crates/engine/src/cmd/sheet.rs @@ -401,8 +401,10 @@ fn move_columns(s: &mut Session, p: &Json) -> Result { { return Err(EngineError::Other("To prevent possible loss of data, columns cannot be shifted off of the worksheet.".into())); } - check_lines_protection(s, &json!({"cols": RangeRef::cols(before, before).a1()}), Axis::Cols, before, count, true)?; - check_lines_protection(s, &json!({"cols": RangeRef::cols(from, end - 1).a1()}), Axis::Cols, from, count, false)?; + // The checks must authorize the sheet this command modifies (`target_sheet`), not whatever + // sheet is active: both carry the command's own `sheet` parameter on. + check_lines_protection(s, &json!({"cols": RangeRef::cols(before, before).a1(), "sheet": p.get("sheet")}), Axis::Cols, before, count, true)?; + check_lines_protection(s, &json!({"cols": RangeRef::cols(from, end - 1).a1(), "sheet": p.get("sheet")}), Axis::Cols, from, count, false)?; let moved_from = if before <= from { from + count } else { from }; if moved_from.checked_add(count).is_none_or(|end| end > MAX_COLS) { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 1be90d56c..f4c47bb62 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -4604,4 +4604,60 @@ fn html_export_without_pictures_or_charts_has_no_fixed_geometry() { let html = crate::io::to_html(&s.doc().unwrap().wb, 0).unwrap(); assert!(!html.contains("") && !html.contains("table-layout") && !html.contains(" = { + let d = s.doc().unwrap(); + let sh = &d.wb.sheets[d.wb.sheet_index("P").unwrap()]; + (0..4).map(|c| sh.value(CellRef::new(0, c as u32))).collect() + }; + assert_eq!(pv, vec![Value::number(1.0), Value::number(2.0), Value::number(3.0), Value::number(4.0)]); + // The active, unprotected sheet moves fine. + s.execute("range.setValues", json!({"range": "A1", "values": [["A", "B", "C", "D"]]})).unwrap(); + s.execute("sheet.moveColumns", json!({"cols": "B:B", "before": 4})).unwrap(); + assert_eq!(v(&s, "A1"), Value::text("A")); +} + +#[test] +fn a_script_cannot_switch_windows() { + // Stored scripts are scoped to their workbook: a step that activates another window would + // point every later step at another open document, so it is refused before any step runs. + let mut s = s(); + s.execute("automate.newScript", json!({"name": "t", "commands": [{"command": "window.activate", "params": {"index": 0}}]})).unwrap(); + s.execute("automate.trustScripts", json!({"trusted": true})).unwrap(); + let err = s.execute("automate.runScript", json!({"name": "t"})).expect_err("script switching windows"); + assert!(err.to_string().contains("window.activate"), "{err}"); + // A benign script runs. + s.execute("automate.newScript", json!({"name": "ok", "commands": [{"command": "cell.set", "params": {"cell": "A1", "input": "=1+1"}}]})).unwrap(); + s.execute("automate.runScript", json!({"name": "ok"})).unwrap(); + assert_eq!(v(&s, "A1"), Value::number(2.0)); +} + mod seams;