Review: Protect Sheet passwords use Excel-compatible hashes, kept in XLSX

Sheet protection stored an internal FNV hash that was never written to
XLSX, and an Excel sheet password was dropped on import, so imported
protected sheets could be unprotected without it. Protect Sheet now
stores a salted SHA-512 hash (spin count 100000) via PasswordHash; the
XLSX sheetProtection password (iterated or legacy) is read and written,
and Unprotect Sheet really verifies it. Hashes from older GridCraft JSON
workbooks are still read and verified.

Refs #346

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

ROADMAP.md

@@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in
 
 | Date | What landed |
 |---|---|
+| 2026-10-11 | Protect Sheet passwords use Excel-compatible hashes and round-trip in XLSX `sheetProtection` (imported passwords verify instead of being dropped) |
 | 2026-10-11 | Review › Protect Workbook opens a dialog with an optional password (Excel-compatible hash, kept in XLSX `workbookProtection`); unprotecting verifies it (#345) |
 | 2026-10-11 | Legacy Excel 97-2003 `.xls` (BIFF8) import with formulas, number formats, merges, sizes and names (#121) |
 | 2026-10-11 | XLSX pass-through: custom XML, external links, connections, query tables, printer settings, form/ActiveX controls and OLE objects are kept on save (#330, #324) |

crates/engine/src/cmd/review.rs

@@ -134,7 +134,8 @@ fn protect_sheet(s: &mut Session, p: &Json) -> Result<Json> {
     let sheet = target_sheet(s, p)?;
     let b = |k: &str| bool_param(p, k).unwrap_or(false);
     let prot = SheetProtection {
-        password_hash: str_param(p, "password").filter(|x| !x.is_empty()).map(password_hash),
+        password: None,
+        legacy_password_hash: None,
         select_locked: true,
         select_unlocked: true,
         format_cells: b("formatCells"),
@@ -147,6 +148,9 @@ fn protect_sheet(s: &mut Session, p: &Json) -> Result<Json> {
         sort: b("sort"),
         autofilter: b("autofilter"),
     };
+    let d = s.doc()?;
+    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| {
         cx.sheet_mut(sheet)?.protection = Some(prot.clone());
         Ok(Json::Null)
@@ -155,11 +159,16 @@ fn protect_sheet(s: &mut Session, p: &Json) -> Result<Json> {
 
 fn unprotect_sheet(s: &mut Session, p: &Json) -> Result<Json> {
     let sheet = target_sheet(s, p)?;
-    let ph = s.doc()?.wb.sheet(sheet).and_then(|sh| sh.protection.as_ref()).and_then(|pr| pr.password_hash);
-    if let Some(h) = ph
-        && str_param(p, "password").map(password_hash) != Some(h)
-    {
-        return Err(EngineError::Other("The password you supplied is not correct.".into()));
+    if let Some(pr) = s.doc()?.wb.sheet(sheet).and_then(|sh| sh.protection.as_ref()) {
+        let pw = str_param(p, "password").unwrap_or("");
+        let ok = match (&pr.password, pr.legacy_password_hash) {
+            (Some(h), _) => h.verify(pw).map_err(EngineError::Other)?,
+            (None, Some(h)) => password_hash(pw) == h,
+            (None, None) => true,
+        };
+        if !ok {
+            return Err(EngineError::Other("The password you supplied is not correct.".into()));
+        }
     }
     edit(s, |cx| {
         cx.sheet_mut(sheet)?.protection = None;
@@ -384,6 +393,30 @@ mod tests {
         assert_eq!(at(&s), (0, "A1".to_string()));
     }
 
+    #[test]
+    fn sheet_passwords_use_excel_hashes_and_old_json_hashes_still_verify() {
+        let mut s = book();
+        s.execute("review.protectSheet", json!({"password": "pa55"})).unwrap();
+        let prot = s.doc().unwrap().wb.active().unwrap().protection.clone().unwrap();
+        assert!(matches!(&prot.password, Some(PasswordHash::Iterated { algorithm, .. }) if algorithm == "SHA-512"));
+        assert!(s.execute("review.unprotectSheet", json!({})).is_err());
+        assert!(s.execute("review.unprotectSheet", json!({"password": "pass"})).is_err());
+        s.execute("review.unprotectSheet", json!({"password": "pa55"})).unwrap();
+        assert!(!s.doc().unwrap().wb.active().unwrap().is_protected());
+        // A GridCraft JSON workbook saved before carried the internal hash under `password_hash`.
+        let old = format!(
+            r#"{{"password_hash": {}, "select_locked": true, "select_unlocked": true, "format_cells": false, "format_columns": false, "format_rows": false, "insert_columns": false, "insert_rows": false, "delete_columns": false, "delete_rows": false, "sort": false, "autofilter": false}}"#,
+            password_hash("old")
+        );
+        let prot: SheetProtection = serde_json::from_str(&old).unwrap();
+        assert_eq!((prot.legacy_password_hash, prot.password.is_none()), (Some(password_hash("old")), true));
+        let i = s.doc().unwrap().wb.active_sheet;
+        let wb = std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().wb);
+        std::sync::Arc::make_mut(&mut wb.sheets[i]).protection = Some(prot);
+        assert!(s.execute("review.unprotectSheet", json!({"password": "new"})).is_err());
+        s.execute("review.unprotectSheet", json!({"password": "old"})).unwrap();
+    }
+
     #[test]
     fn protect_workbook_with_a_password_really_verifies() {
         let mut s = book();

crates/engine/src/tests.rs

@@ -816,7 +816,7 @@ fn protection_passwords_stay_out_of_the_journal_and_recorded_scripts() {
         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!(prot.as_ref().is_some_and(|p| p.format_cells && p.password.is_none()), "{prot:?}");
         }
     }
     assert!(r.doc().unwrap().wb.active().unwrap().protection.is_none());

crates/model/src/features.rs

@@ -554,8 +554,14 @@ impl Default for PrintSettings {
 
 #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)]
 pub struct SheetProtection {
-    /// Hash of the password (our own salted FNV; never the password itself).
-    pub password_hash: Option<u64>,
+    /// Hash of the password (Excel-compatible, written to XLSX `sheetProtection`; never the
+    /// password itself).
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub password: Option<crate::PasswordHash>,
+    /// The internal hash older GridCraft JSON workbooks stored ([`password_hash`]); still
+    /// verified when unprotecting, never written for new passwords.
+    #[serde(rename = "password_hash", default, skip_serializing_if = "Option::is_none")]
+    pub legacy_password_hash: Option<u64>,
     pub select_locked: bool,
     pub select_unlocked: bool,
     pub format_cells: bool,
@@ -587,8 +593,8 @@ impl EditRange {
     }
 }
 
-/// Simple non-cryptographic password hash for sheet protection (like Excel's, protection is a
-/// convenience, not security).
+/// The non-cryptographic password hash older GridCraft versions stored for sheet protection
+/// (only verified now, see [`SheetProtection::legacy_password_hash`]).
 pub fn password_hash(pw: &str) -> u64 {
     let mut h: u64 = 0xcbf29ce484222325 ^ 0x5348_4545_5443;
     for b in pw.bytes() {

crates/ui-egui/tests/protect_password.rs

@@ -57,7 +57,7 @@ fn protect_and_unprotect_sheet_mask_the_password() {
     h.get_by_label("   OK   ").click();
     h.run_steps(3);
     assert!(h.state().dialog.is_none());
-    assert!(h.state().session.active().unwrap().wb.active().unwrap().protection.as_ref().is_some_and(|p| p.password_hash.is_some()));
+    assert!(h.state().session.active().unwrap().wb.active().unwrap().protection.as_ref().is_some_and(|p| p.password.is_some()));
 
     // Unprotect asks for the password with a masked field too, and it starts hidden again.
     h.state_mut().open_dialog("unprotectSheet", json!({}));

crates/xlsx/src/sheet_read.rs

@@ -143,11 +143,9 @@ pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result<Sheet, IoE
             "rowBreaks" => sheet.print.row_breaks = e.kids("brk").filter_map(|b| b.attr_u32("id")).filter(|&r| r > 0 && r < MAX_ROWS).collect(),
             "colBreaks" => sheet.print.col_breaks = e.kids("brk").filter_map(|b| b.attr_u32("id")).filter(|&c| c > 0 && c < MAX_COLS).collect(),
             "sheetProtection" if e.flag("sheet", false) => {
-                if e.attr("password").is_some() || e.attr("hashValue").is_some() {
-                    cx.warn(format!("sheet \"{name}\": the protection password was not imported"));
-                }
                 sheet.protection = Some(SheetProtection {
-                    password_hash: None,
+                    password: read_password(e, ""),
+                    legacy_password_hash: None,
                     select_locked: !e.flag("selectLockedCells", false),
                     select_unlocked: !e.flag("selectUnlockedCells", false),
                     format_cells: !e.flag("formatCells", true),

crates/xlsx/src/sheet_write.rs

@@ -332,6 +332,9 @@ pub fn write_sheet(wb: &Workbook, si: usize, selected: bool, out: &mut Out) -> (
         if !p.select_unlocked {
             s.push_str(" selectUnlockedCells=\"1\"");
         }
+        if let Some(h) = &p.password {
+            s.push_str(&password_attrs(h, ""));
+        }
         s.push_str("/>");
     }
     s.push_str(&protected_ranges_xml(sheet));

crates/xlsx/src/tests/fixtures.rs

@@ -223,7 +223,7 @@ fn hyperlinks_comments_tables_validation_filters() {
       <printOptions gridLines="1" horizontalCentered="1"/><pageMargins left="0.5" right="0.5" top="1" bottom="1" header="0.4" footer="0.4"/>
       <headerFooter><oddHeader>&amp;CTitle</oddHeader><oddFooter>Page &amp;P</oddFooter></headerFooter>
       <rowBreaks count="1"><brk id="10" max="16383" man="1"/></rowBreaks>
-      <sheetProtection sheet="1" formatCells="0" sort="0" password="CC1A"/>
+      <sheetProtection sheet="1" formatCells="0" sort="0" password="CBEB"/>
       <tableParts count="1"><tablePart r:id="rId2"/></tableParts>
       <extLst><ext uri="{CCE6A557-97BC-4b89-ADB6-D9C93CAAB3DF}"><x14:dataValidations xmlns:x14="x" xmlns:xm="m" count="1"><x14:dataValidation type="list"><x14:formula1><xm:f>Other!$A$1:$A$3</xm:f></x14:formula1><xm:sqref>G1:G4</xm:sqref></x14:dataValidation></x14:dataValidations></ext>
         <ext uri="{05C60535-1F16-4fd2-B633-F4F36F0B64E0}"><x14:sparklineGroups xmlns:x14="x" xmlns:xm="m"><x14:sparklineGroup type="column" markers="1"><x14:colorSeries rgb="FF376092"/><x14:sparklines><x14:sparkline><xm:f>Data!A1:E1</xm:f><xm:sqref>F1</xm:sqref></x14:sparkline></x14:sparklines></x14:sparklineGroup></x14:sparklineGroups></ext></extLst>"#;
@@ -270,7 +270,10 @@ Check this</t></r></text></comment></commentList></comments>"#;
     assert_eq!(s.print.row_breaks, vec![10]);
     let p = s.protection.as_ref().unwrap();
     assert!(p.format_cells && p.sort && !p.insert_rows && p.select_locked);
-    assert!(rep.warnings.iter().any(|w| w.contains("password")));
+    // The legacy sheet password is imported (CBEB is the hash of "test") and verifies.
+    let pw = p.password.as_ref().unwrap();
+    assert!(pw.verify("test").unwrap() && !pw.verify("Test").unwrap());
+    assert!(!rep.warnings.iter().any(|w| w.contains("password")), "{:?}", rep.warnings);
     assert_eq!(s.sparklines.len(), 1);
     assert_eq!(s.sparklines[0].kind, gridcraft_model::SparklineKind::Column);
 }

crates/xlsx/src/tests/roundtrip.rs

@@ -248,7 +248,14 @@ fn sample() -> Workbook {
             (3, FilterCriterion::FillColor(Color::Rgb(0xFF0000))),
         ],
     });
-    s.protection = Some(SheetProtection { format_cells: true, sort: true, select_locked: true, select_unlocked: true, ..Default::default() });
+    s.protection = Some(SheetProtection {
+        format_cells: true,
+        sort: true,
+        select_locked: true,
+        select_unlocked: true,
+        password: Some(gridcraft_model::PasswordHash::new_sha512("sheet pw", b"fedcba9876543210")),
+        ..Default::default()
+    });
     s.print = PrintSettings {
         orientation: Orientation::Landscape,
         paper: "A4".into(),