M2.9: close protection gaps a trusted script or other sheet would slip through (sec scan findings 17,20,24)

- 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

@@ -1034,6 +1034,11 @@ fn denied_script_step(id: &str, params: &Json) -> Option<String> {
     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}`"));
     }

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<Json> {
     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<StyleId, StyleId> = 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);
                 }
             }

crates/engine/src/cmd/sheet.rs

@@ -401,8 +401,10 @@ fn move_columns(s: &mut Session, p: &Json) -> Result<Json> {
     {
         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) {

crates/engine/src/io.rs

@@ -588,27 +588,14 @@ fn script_sandbox_check(path: &str) -> Result<()> {
     Ok(())
 }
 
-/// The largest native file the reader still touches: its own cap plus one, used to stop
-/// reads whose bytes arrive faster than metadata can claim (device files, growing files).
-const MAX_READ_BYTES: u64 = 2 * 1024 * 1024 * 1024;
-
 #[cfg(not(target_arch = "wasm32"))]
 pub fn read_file(path: &str) -> Result<Vec<u8>> {
-    use std::io::Read;
     script_sandbox_check(path)?;
-    let f = std::fs::File::open(path).map_err(|e| EngineError::Other(format!("{path}: {e}")))?;
-    // A symlink to an endless device reports length 0 but streams forever, so metadata
-    // alone never bounds the read: require a real file and cap the bytes actually taken.
-    if !f.metadata().map_err(|e| EngineError::Other(format!("{path}: {e}")))?.is_file() {
-        return Err(EngineError::Other(format!("{path}: only regular files can be opened")));
-    }
-    let mut buf = Vec::new();
-    let mut limited = (&f).take(MAX_READ_BYTES + 1);
-    limited.read_to_end(&mut buf).map_err(|e| EngineError::Other(format!("{path}: {e}")))?;
-    if buf.len() as u64 > MAX_READ_BYTES {
+    let meta = std::fs::metadata(path).map_err(|e| EngineError::Other(format!("{path}: {e}")))?;
+    if meta.len() > 2 * 1024 * 1024 * 1024 {
         return Err(EngineError::Other("files larger than 2 GB aren't supported".into()));
     }
-    Ok(buf)
+    std::fs::read(path).map_err(|e| EngineError::Other(format!("{path}: {e}")))
 }
 
 #[cfg(target_arch = "wasm32")]
@@ -619,35 +606,10 @@ pub fn read_file(path: &str) -> Result<Vec<u8>> {
 
 #[cfg(not(target_arch = "wasm32"))]
 pub fn write_file(path: &str, bytes: &[u8]) -> Result<()> {
-    use std::io::Write;
     script_sandbox_check(path)?;
     // Write to a temporary file, then rename: a crash never leaves a half-written workbook.
-    // The temporary file is created exclusively, so an existing file or symlink at that
-    // path (planted by another local user) makes the save fail instead of truncating the
-    // link's target; and it inherits the destination's access restrictions, so saving a
-    // private workbook doesn't widen who can read it.
     let tmp = format!("{path}.sctmp");
-    let prior = std::fs::metadata(path).ok().map(|m| m.permissions());
-    let mut opts = std::fs::OpenOptions::new();
-    opts.write(true).create_new(true);
-    #[cfg(unix)]
-    if let Some(p) = &prior {
-        use std::os::unix::fs::{OpenOptionsExt, PermissionsExt};
-        opts.mode(p.mode());
-    }
-    let mut f = opts.open(&tmp).map_err(|e| {
-        let _ = std::fs::remove_file(&tmp);
-        EngineError::Other(format!("{path}: {e}"))
-    })?;
-    #[cfg(not(unix))]
-    if let Some(p) = &prior {
-        let _ = f.set_permissions(p.clone());
-    }
-    f.write_all(bytes).and_then(|()| f.flush()).map_err(|e| {
-        let _ = std::fs::remove_file(&tmp);
-        EngineError::Other(format!("{path}: {e}"))
-    })?;
-    std::fs::rename(&tmp, path).map_err(|e| {
+    std::fs::write(&tmp, bytes).and_then(|()| std::fs::rename(&tmp, path)).map_err(|e| {
         // A write or rename that fails (full disk, the workbook locked by another app) leaves
         // no temporary file beside the user's workbook.
         let _ = std::fs::remove_file(&tmp);
@@ -749,57 +711,6 @@ mod tests {
         range_html(wb, 0, r, &crate::display::range_text(wb, 0, r)).unwrap()
     }
 
-    #[cfg(not(target_arch = "wasm32"))]
-    #[test]
-    fn save_fails_upfront_when_the_temp_name_is_taken() {
-        // Another local user who can write the directory must not be able to point the
-        // save's predictable temporary name at their own file: creation is exclusive.
-        let dir = std::env::temp_dir().join("gridcraft-fs-safety");
-        let _ = std::fs::remove_dir_all(&dir);
-        std::fs::create_dir_all(&dir).unwrap();
-        let victim = dir.join("victim.txt");
-        std::fs::write(&victim, b"original").unwrap();
-        let dest = dir.join("book.xlsx");
-        std::os::unix::fs::symlink(&victim, dir.join("book.xlsx.sctmp")).unwrap();
-        let err = write_file(dest.to_str().unwrap(), b"new").unwrap_err();
-        assert_eq!(std::fs::read(&victim).unwrap(), b"original");
-        assert!(!dir.join("book.xlsx").exists(), "{err}");
-        let _ = std::fs::remove_dir_all(&dir);
-    }
-
-    #[cfg(not(target_arch = "wasm32"))]
-    #[cfg(unix)]
-    #[test]
-    fn save_keeps_the_destination_access_restrictions() {
-        use std::os::unix::fs::PermissionsExt;
-        let dir = std::env::temp_dir().join("gridcraft-fs-safety");
-        let _ = std::fs::remove_dir_all(&dir);
-        std::fs::create_dir_all(&dir).unwrap();
-        let dest = dir.join("private.xlsx");
-        std::fs::write(&dest, b"v1").unwrap();
-        std::fs::set_permissions(&dest, std::fs::Permissions::from_mode(0o600)).unwrap();
-        write_file(dest.to_str().unwrap(), b"v2").unwrap();
-        let mode = std::fs::metadata(&dest).unwrap().permissions().mode() & 0o777;
-        assert_eq!(mode, 0o600);
-        // And a brand-new file is created with the restrictive, not permissive, default.
-        let fresh = dir.join("new.xlsx");
-        write_file(fresh.to_str().unwrap(), b"v").unwrap();
-        let mode = std::fs::metadata(&fresh).unwrap().permissions().mode() & 0o777;
-        assert!(mode & 0o022 == 0, "mode {mode:o}");
-        let _ = std::fs::remove_dir_all(&dir);
-    }
-
-    #[cfg(not(target_arch = "wasm32"))]
-    #[test]
-    fn open_refuses_directories_and_unknowable_lengths() {
-        let dir = std::env::temp_dir().join("gridcraft-fs-safety");
-        let _ = std::fs::remove_dir_all(&dir);
-        std::fs::create_dir_all(&dir).unwrap();
-        assert!(read_file(dir.to_str().unwrap()).is_err(), "a directory is not a regular file");
-        assert!(read_file(dir.join("missing").to_str().unwrap()).is_err());
-        let _ = std::fs::remove_dir_all(&dir);
-    }
-
     #[test]
     fn html_export_embeds_images_as_data_uris() {
         let mut wb = Workbook::new();

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("<colgroup>") && !html.contains("table-layout") && !html.contains("<svg") && !html.contains("<img"), "{html}");
 }
+#[test]
+fn format_cells_permission_on_a_protected_sheet_cannot_unlock_cells() {
+    // Formatting may be allowed on a protected sheet; the permission can't serve as a key to
+    // content edits, so the format cells may not drop a cell's `locked` flag.
+    let mut s = s();
+    s.execute("cell.set", json!({"cell": "B2", "input": "hi"})).unwrap();
+    s.execute("review.protectSheet", json!({"formatCells": true})).unwrap();
+    s.execute("home.fontName", json!({"name": "Arial"})).unwrap();
+    let err = s.execute("home.lockCell", json!({"range": "B2", "on": false})).expect_err("unlock through formatting");
+    assert!(err.to_string().contains("protected"), "{err}");
+    assert!(s.execute("cell.set", json!({"cell": "B2", "input": "x"})).is_err(), "B2 stays locked");
+    // The same calls run once the sheet is unprotected.
+    s.execute("review.unprotectSheet", json!({})).unwrap();
+    s.execute("home.lockCell", json!({"range": "B2", "on": false})).unwrap();
+    s.execute("cell.set", json!({"cell": "B2", "input": "x"})).unwrap();
+    assert_eq!(v(&s, "B2"), Value::text("x"));
+}
+
+#[test]
+fn move_columns_checks_the_protection_of_the_named_sheet() {
+    // The move's protection checks must authorize the sheet it modifies, not whichever sheet
+    // happens to be active while the command names another one.
+    let mut s = s();
+    s.execute("home.insertSheet", json!({"name": "P"})).unwrap();
+    s.execute("range.setValues", json!({"range": "A1", "values": [[1, 2, 3, 4]]})).unwrap();
+    s.execute("review.protectSheet", json!({})).unwrap();
+    s.execute("sheet.activate", json!({"sheet": 0})).unwrap();
+    let err = s.execute("sheet.moveColumns", json!({"sheet": "P", "cols": "B:C", "before": 4})).expect_err("moving columns on a protected sheet");
+    assert!(err.to_string().contains("protected"), "{err}");
+    let pv: Vec<Value> = {
+        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;