diff --git a/ROADMAP.md b/ROADMAP.md index b927fc4..1d31b92 100644 --- a/ROADMAP.md +++ b/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) | diff --git a/crates/engine/src/cmd/review.rs b/crates/engine/src/cmd/review.rs index 8931bf5..0dc5962 100644 --- a/crates/engine/src/cmd/review.rs +++ b/crates/engine/src/cmd/review.rs @@ -134,7 +134,8 @@ fn protect_sheet(s: &mut Session, p: &Json) -> Result { 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 { 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 { fn unprotect_sheet(s: &mut Session, p: &Json) -> Result { 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(); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index dfe8254..9be4bb9 100644 --- a/crates/engine/src/tests.rs +++ b/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()); diff --git a/crates/model/src/features.rs b/crates/model/src/features.rs index 1864d6d..c8dc28b 100644 --- a/crates/model/src/features.rs +++ b/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, + /// 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, + /// 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, 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() { diff --git a/crates/ui-egui/tests/protect_password.rs b/crates/ui-egui/tests/protect_password.rs index dc2b1c5..ee68cf2 100644 --- a/crates/ui-egui/tests/protect_password.rs +++ b/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!({})); diff --git a/crates/xlsx/src/sheet_read.rs b/crates/xlsx/src/sheet_read.rs index 2c33d1f..3e9e374 100644 --- a/crates/xlsx/src/sheet_read.rs +++ b/crates/xlsx/src/sheet_read.rs @@ -143,11 +143,9 @@ pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result 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), diff --git a/crates/xlsx/src/sheet_write.rs b/crates/xlsx/src/sheet_write.rs index acc98f6..a56cf5e 100644 --- a/crates/xlsx/src/sheet_write.rs +++ b/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)); diff --git a/crates/xlsx/src/tests/fixtures.rs b/crates/xlsx/src/tests/fixtures.rs index 42adcb5..9cd09a3 100644 --- a/crates/xlsx/src/tests/fixtures.rs +++ b/crates/xlsx/src/tests/fixtures.rs @@ -223,7 +223,7 @@ fn hyperlinks_comments_tables_validation_filters() { &CTitlePage &P - + Other!$A$1:$A$3G1:G4 Data!A1:E1F1"#; @@ -270,7 +270,10 @@ Check this"#; 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); } diff --git a/crates/xlsx/src/tests/roundtrip.rs b/crates/xlsx/src/tests/roundtrip.rs index 4038908..be59af4 100644 --- a/crates/xlsx/src/tests/roundtrip.rs +++ b/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(),