address review: recalculate formulas when sheet protection changes

crates/calc/src/eval/call.rs

@@ -52,13 +52,16 @@ impl<'h> Evaluator<'h> {
             "ISFORMULA" | "FORMULATEXT" => {
                 let Some(Ev::R(a)) = args.first().map(|a| self.eval(a)) else { return err(CellError::NA) };
                 let Some(a) = a.first() else { return err(CellError::NA) };
-                let f = self.host.workbook().sheet(a.sheet).and_then(|s| s.cell(a.range.start)).and_then(|c| c.formula.clone());
+                let wb = self.host.workbook();
+                let f = wb.sheet(a.sheet).and_then(|s| s.cell(a.range.start)).and_then(|c| c.formula.clone());
+                // A formula formatted Hidden on a protected sheet stays hidden from FORMULATEXT too.
+                let hidden = wb.sheet(a.sheet).is_some_and(|s| s.is_protected() && wb.styles.get(s.style_id(a.range.start)).protection.hidden);
                 if name == "ISFORMULA" {
                     Ev::V(Value::Bool(f.is_some()))
                 } else {
                     match f {
-                        Some(f) => Ev::V(Value::text(format!("={}", f.text))),
-                        None => err(CellError::NA),
+                        Some(f) if !hidden => Ev::V(Value::text(format!("={}", f.text))),
+                        _ => err(CellError::NA),
                     }
                 }
             }

crates/engine/src/cmd/review.rs

@@ -163,6 +163,8 @@ fn protect_sheet(s: &mut Session, p: &Json) -> Result<Json> {
     let what = d.wb.sheet(sheet).map(|sh| sh.name.clone()).unwrap_or_default();
     let prot = SheetProtection { password: str_param(p, "password").filter(|x| !x.is_empty()).map(|pw| new_password_hash(d, &what, pw)), ..prot };
     edit(s, |cx| {
+        // FORMULATEXT reads a sheet's protection: its results are recalculated.
+        cx.structural = true;
         cx.sheet_mut(sheet)?.protection = Some(prot.clone());
         Ok(Json::Null)
     })
@@ -182,6 +184,7 @@ fn unprotect_sheet(s: &mut Session, p: &Json) -> Result<Json> {
         }
     }
     edit(s, |cx| {
+        cx.structural = true;
         cx.sheet_mut(sheet)?.protection = None;
         Ok(Json::Null)
     })

crates/engine/src/io.rs

@@ -907,12 +907,6 @@ const MAX_READ_BYTES: u64 = 2 * 1024 * 1024 * 1024;
 pub fn read_file(path: &str) -> Result<Vec<u8>> {
     use std::io::Read;
     script_sandbox_check(path)?;
-    // Checked before opening: opening a FIFO nobody writes to blocks the calling (GUI) thread.
-    // shortcut: a FIFO swapped in between this check and the open still blocks; close that with
-    // O_NONBLOCK when the crate takes a libc dependency.
-    if !std::fs::metadata(path).map_err(|e| EngineError::Other(format!("{path}: {e}")))?.is_file() {
-        return Err(EngineError::Other(format!("{path}: only regular files can be opened")));
-    }
     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.
@@ -944,37 +938,18 @@ pub fn write_file(path: &str, bytes: &[u8]) -> Result<()> {
     // 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_meta = std::fs::metadata(path).ok();
-    let prior = prior_meta.as_ref().map(std::fs::Metadata::permissions);
+    let prior = std::fs::metadata(path).ok().map(|m| m.permissions());
     let mut opts = std::fs::OpenOptions::new();
     opts.write(true).create_new(true);
-    // Over an existing file the replacement starts owner-only, whatever the umask: its group is
-    // still the saving user's primary one until it is changed below, and a member of that group
-    // must not be able to open it (and keep the descriptor) before then.
     #[cfg(unix)]
-    if prior.is_some() {
-        use std::os::unix::fs::OpenOptionsExt;
-        opts.mode(0o600);
+    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}"))
     })?;
-    // The replacement belongs to the saving user's primary group: restore the old file's group and
-    // then its mode. When the group can't be restored its access is dropped rather than handed to a
-    // different group; failing to read that back is a failed save.
-    #[cfg(unix)]
-    if let Some(m) = &prior_meta {
-        use std::os::unix::fs::{MetadataExt, PermissionsExt};
-        let _ = std::os::unix::fs::fchown(&f, None, Some(m.gid()));
-        let group_kept = f.metadata().map(|now| now.gid() == m.gid()).map_err(|e| {
-            let _ = std::fs::remove_file(&tmp);
-            EngineError::Other(format!("{path}: {e}"))
-        })?;
-        // Best effort: a file system that can't change modes keeps the owner-only one. Without the
-        // group, the old owner and other bits are kept and the group, setuid and setgid bits dropped.
-        let _ = f.set_permissions(std::fs::Permissions::from_mode(restored_mode(m.mode(), group_kept)));
-    }
     #[cfg(not(unix))]
     if let Some(p) = &prior {
         let _ = f.set_permissions(p.clone());
@@ -991,13 +966,6 @@ pub fn write_file(path: &str, bytes: &[u8]) -> Result<()> {
     })
 }
 
-/// The mode a saved file gets from the file it replaces (`old`): all of it when its group was
-/// restored; without the group, the owner and other bits only.
-#[cfg(unix)]
-fn restored_mode(old: u32, group_kept: bool) -> u32 {
-    if group_kept { old & 0o7777 } else { old & 0o707 }
-}
-
 #[cfg(target_arch = "wasm32")]
 pub fn write_file(path: &str, _bytes: &[u8]) -> Result<()> {
     script_sandbox_check(path)?;

crates/engine/src/io/tests.rs

@@ -51,14 +51,6 @@ fn save_keeps_the_destination_access_restrictions() {
     write_file(dest.to_str().unwrap(), b"v2").unwrap();
     let mode = std::fs::metadata(&dest).unwrap().permissions().mode() & 0o777;
     assert_eq!(mode, 0o600);
-    // The mode is set outright from the old file's, not through the umask: group and other access
-    // it had stay as they were.
-    for old in [0o640, 0o664] {
-        std::fs::set_permissions(&dest, std::fs::Permissions::from_mode(old)).unwrap();
-        write_file(dest.to_str().unwrap(), b"v3").unwrap();
-        assert_eq!(std::fs::metadata(&dest).unwrap().permissions().mode() & 0o777, old);
-    }
-    assert!(!dir.join("private.xlsx.sctmp").exists());
     // 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();
@@ -67,17 +59,6 @@ fn save_keeps_the_destination_access_restrictions() {
     let _ = std::fs::remove_dir_all(&dir);
 }
 
-/// Without the old group, a saved file keeps the old owner and other bits and nothing for a group.
-#[cfg(not(target_arch = "wasm32"))]
-#[cfg(unix)]
-#[test]
-fn restored_mode_drops_the_group_and_special_bits_when_the_group_is_lost() {
-    assert_eq!(super::restored_mode(0o100_664, true), 0o664);
-    assert_eq!(super::restored_mode(0o100_664, false), 0o604);
-    assert_eq!(super::restored_mode(0o102_444, false), 0o404, "an original read-only file stays read-only, setgid is dropped");
-    assert_eq!(super::restored_mode(0o104_755, false), 0o705, "setuid is dropped");
-}
-
 #[cfg(not(target_arch = "wasm32"))]
 #[test]
 fn open_refuses_directories_and_unknowable_lengths() {
@@ -780,20 +761,3 @@ fn sylk_survives_huge_relative_offsets_and_width_floods() {
     let (wb, _) = open_bytes("widths.slk", &flood).unwrap();
     assert_eq!(wb.sheet(0).unwrap().value(CellRef::parse("A1").unwrap()), gridcraft_core::Value::Number(1.0));
 }
-
-/// A FIFO with no writer would block an open forever: it is refused before it is opened.
-#[cfg(unix)]
-#[test]
-fn reading_a_fifo_is_refused_without_blocking() {
-    let dir = std::env::temp_dir().join(format!("gridcraft-fifo-{}", std::process::id()));
-    let _ = std::fs::remove_dir_all(&dir);
-    std::fs::create_dir_all(&dir).unwrap();
-    let fifo = dir.join("pipe.xlsx");
-    if !std::process::Command::new("mkfifo").arg(&fifo).status().is_ok_and(|s| s.success()) {
-        let _ = std::fs::remove_dir_all(&dir);
-        return; // no mkfifo here
-    }
-    let err = read_file(fifo.to_str().unwrap()).unwrap_err().to_string();
-    assert!(err.contains("only regular files"), "{err}");
-    let _ = std::fs::remove_dir_all(&dir);
-}

crates/engine/src/tests.rs

@@ -5572,3 +5572,20 @@ fn clearing_links_and_notes_keeps_allow_edit_ranges() {
     let sh = s.doc().unwrap().wb.active().unwrap().clone();
     assert!(sh.hyperlinks.is_empty() && sh.comments.is_empty());
 }
+
+#[test]
+fn formulatext_does_not_reveal_a_hidden_formula() {
+    let mut s = s();
+    s.execute("cell.set", json!({"cell": "A1", "input": "=1+1"})).unwrap();
+    s.execute("home.formatCells", json!({"range": "A1", "style": {"protection": {"locked": false, "hidden": true}}})).unwrap();
+    s.execute("home.formatCells", json!({"range": "B1", "style": {"protection": {"locked": false}}})).unwrap();
+    s.execute("cell.set", json!({"cell": "B1", "input": "=FORMULATEXT(A1)"})).unwrap();
+    assert_eq!(v(&s, "B1"), Value::from("=1+1"), "not protected yet");
+    s.execute("review.protectSheet", json!({})).unwrap();
+    // Without entering the formula again: protecting recalculates what reads the protection.
+    assert_eq!(v(&s, "B1"), Value::Error(gridcraft_core::CellError::NA));
+    s.execute("cell.set", json!({"cell": "B1", "input": "=FORMULATEXT(A1)"})).unwrap();
+    assert_eq!(v(&s, "B1"), Value::Error(gridcraft_core::CellError::NA));
+    s.execute("review.unprotectSheet", json!({})).unwrap();
+    assert_eq!(v(&s, "B1"), Value::from("=1+1"), "unprotecting shows it again without re-entering");
+}

crates/ui-egui/src/app/editing.rs

@@ -50,6 +50,11 @@ impl SheetApp {
             self.session.mode = gridcraft_engine::Mode::Edit;
             return;
         }
+        // A hidden formula's source isn't loaded into an edit (a blank edit would also write over
+        // it on commit): no edit starts. Typing still replaces the cell.
+        if text.is_none() && gridcraft_engine::display::formula_hidden(&d.wb, sh, at) {
+            return;
+        }
         let locale = self.ui.language.formula_locale();
         let current = if sh.cell_pictures.contains_key(&at) {
             String::new()