Merge pull request #355 from LloydNicholson/fix/journal-redact-passwords

Journal: sheet protection passwords aren't recorded in plain text

# Conflicts:
#	crates/engine/src/tests.rs

crates/engine/src/cmd/mod.rs

@@ -69,6 +69,21 @@ impl CommandSpec {
     }
 }
 
+/// Params that hold a secret: `password`, or a name ending in `Password`.
+pub fn is_secret_param(key: &str) -> bool {
+    key == "password" || key.ends_with("Password")
+}
+
+/// The params as the journal records them: secret params are left out, so a password never
+/// reaches the journal dialog, `engine.journal` or a recorded script (saved in the workbook).
+/// Replaying such an entry runs the command without that param.
+pub fn journal_params(params: &Json) -> Json {
+    match params {
+        Json::Object(o) => Json::Object(o.iter().filter(|(k, _)| !is_secret_param(k)).map(|(k, v)| (k.clone(), v.clone())).collect()),
+        other => other.clone(),
+    }
+}
+
 pub fn always(_: &Session) -> std::result::Result<(), String> {
     Ok(())
 }

crates/engine/src/lib.rs

@@ -188,6 +188,7 @@ pub struct Session {
     pub clipboard: Option<Clipboard>,
     pub prefs: Prefs,
     pub mode: Mode,
+    /// Commands run so far, without their passwords ([`cmd::journal_params`]).
     pub journal: Vec<(String, Json)>,
     pub ui_requests: Vec<UiRequest>,
     /// Format painter source style (sticky when double-clicked).
@@ -288,7 +289,7 @@ impl Session {
             }
         };
         if spec.journal && result.is_ok() {
-            self.journal.push((id.to_string(), params.clone()));
+            self.journal.push((id.to_string(), cmd::journal_params(&params)));
             if self.journal.len() > 10_000 {
                 self.journal.drain(..5_000);
             }

crates/engine/src/tests.rs

@@ -611,6 +611,43 @@ fn filter_on_a_selected_header_row_covers_the_data_below() {
     assert_eq!(r["range"], "A1:B1");
 }
 
+#[test]
+fn protection_passwords_stay_out_of_the_journal_and_recorded_scripts() {
+    const PW: &str = "synthetic-pw-7319";
+    let mut s = s();
+    s.execute("automate.recordActions", json!({})).unwrap();
+    s.execute("review.protectSheet", json!({"password": PW, "formatCells": true})).unwrap();
+    // The password still protects the sheet; the refused attempt isn't journaled.
+    assert!(s.execute("review.unprotectSheet", json!({})).is_err());
+    s.execute("review.unprotectSheet", json!({"password": PW})).unwrap();
+    s.execute("automate.recordActions", json!({"stop": true, "name": "Lock"})).unwrap();
+
+    let journal = serde_json::to_string(&s.journal).unwrap();
+    assert!(!journal.contains(PW), "{journal}");
+    let params = |s: &Session, id: &str| s.journal.iter().find(|(c, _)| c == id).map(|(_, p)| p.clone());
+    assert_eq!(params(&s, "review.protectSheet"), Some(json!({"formatCells": true})));
+    assert_eq!(params(&s, "review.unprotectSheet"), Some(json!({})));
+
+    // The recorded script is stored in the workbook, so it is saved with the file.
+    let bytes = crate::io::save_bytes(&s.doc().unwrap().wb, "book.xlsx").unwrap();
+    let (reopened, _) = crate::io::open_bytes("book.xlsx", &bytes).unwrap();
+    let script = reopened.names.iter().find(|n| n.name == "_sc_scripts").map(|n| n.formula.clone()).unwrap();
+    assert!(script.contains("review.protectSheet"), "{script}");
+    assert!(!script.contains(PW), "{script}");
+
+    // The journal still replays: protection without a password, then unprotection.
+    let mut r = self::s();
+    for (id, p) in s.journal.iter().filter(|(id, _)| !id.starts_with("automate.")) {
+        r.execute(id, p.clone()).unwrap_or_else(|e| panic!("{id}: {e}"));
+        let prot = r.doc().unwrap().wb.active().unwrap().protection.clone();
+        if id == "review.protectSheet" {
+            assert!(prot.as_ref().is_some_and(|p| p.format_cells && p.password_hash.is_none()), "{prot:?}");
+        }
+    }
+    assert!(r.doc().unwrap().wb.active().unwrap().protection.is_none());
+    s.execute("automate.runScript", json!({"name": "Lock"})).unwrap();
+}
+
 /// A sheet with locked data in A1:B3, unlocked cells in D1:D3 and row 10, then protected.
 fn protected(extra: serde_json::Value) -> Session {
     let mut s = s();

docs/control-protocol.md

@@ -21,7 +21,7 @@ Errors come back as `{"id": …, "ok": false, "error": "…"}`.
 |---|---|---|
 | `engine.execute` | `{command, params}` | Runs any engine command (see `engine.commands` or `gridcraft-cli commands`) |
 | `engine.commands` | `{search?}` | Every command: id, label, ribbon path, shortcut, params doc, enabled; `search` keeps those whose id, label or ribbon path contains it (case-insensitive) |
-| `engine.journal` | | Commands run so far (replayable) |
+| `engine.journal` | | Commands run so far (replayable). Passwords (`password`, `…Password` params) are left out, so a replayed Protect Sheet has no password |
 | `document.inspect` | | Workbook summary: sheets, used ranges, tables, charts, names, selection, undo labels |
 | `sheet.read` | `{range?, sheet?, formulas?, formatted?}` | Values (or formulas / displayed text) of a range |
 | `cell.get` | `{cell?}` | One cell: value, formula, displayed text, style, merge, comment, link |