From a8eb09604d44ef516f40293f0be19f2a029257cf Mon Sep 17 00:00:00 2001 From: lloyd Date: Sun, 11 Oct 2026 16:55:13 +0200 Subject: [PATCH] Review: mask the Protect / Unprotect Sheet password (#347) The password fields in Protect Sheet and Unprotect Sheet were plain text fields, so the password was readable on screen and in the accessibility tree. A new Password form field masks it as typed and adds a "Show password" check box that reveals it on purpose (off each time the dialog opens). These are the only password fields in the UI: Protect Workbook runs without a dialog and encrypted files are refused on open. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/dialogs.rs | 20 ++++++- crates/ui-egui/tests/protect_password.rs | 70 ++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 2 deletions(-) create mode 100644 crates/ui-egui/tests/protect_password.rs diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 7dd3acb10..3c75d1a82 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -13,6 +13,7 @@ use crate::theme; #[derive(Clone, Debug, PartialEq)] pub enum Field { Text { key: &'static str, label: &'static str }, + Password { key: &'static str, label: &'static str }, Number { key: &'static str, label: &'static str }, Choice { key: &'static str, label: &'static str, options: Vec<(&'static str, &'static str)> }, Check { key: &'static str, label: &'static str }, @@ -31,6 +32,8 @@ pub struct Dialog { pub search: String, pub list_index: usize, pub result: Option, + /// "Show password" is ticked: password fields show their text. + pub show_password: bool, picture_inbox: Option, picture_target: Option<(u64, usize)>, } @@ -51,6 +54,7 @@ impl Dialog { search: String::new(), list_index: 0, result: None, + show_password: false, picture_inbox: None, picture_target: None, } @@ -67,6 +71,7 @@ impl Dialog { search: String::new(), list_index: 0, result: None, + show_password: false, picture_inbox: None, picture_target: None, } @@ -151,7 +156,7 @@ impl Dialog { "Protect Sheet", "review.protectSheet", vec![ - Text { key: "password", label: "Password (optional):" }, + Password { key: "password", label: "Password (optional):" }, Note("Allow all users of this sheet to:".into()), Check { key: "formatCells", label: "Format cells" }, Check { key: "formatColumns", label: "Format columns" }, @@ -167,7 +172,7 @@ impl Dialog { "unprotectSheet", "Unprotect Sheet", "review.unprotectSheet", - vec![Text { key: "password", label: "Password:" }], + vec![Password { key: "password", label: "Password:" }], json!({}), ), "unhideSheet" => { @@ -989,6 +994,17 @@ fn form(ui: &mut egui::Ui, d: &mut Dialog, confirm: &mut bool, open: &mut bool) } }); } + Field::Password { key, label } => { + // Masked as typed (Excel shows dots); "Show password" reveals it on purpose. + ui.vertical(|ui| { + ui.label(label); + let mut s = d.values.get(key).and_then(Json::as_str).unwrap_or_default().to_string(); + if ui.add(egui::TextEdit::singleline(&mut s).password(!d.show_password).desired_width(320.0)).changed() { + d.values.insert(key.into(), json!(s)); + } + ui.checkbox(&mut d.show_password, "Show password"); + }); + } Field::Number { key, label } => { ui.horizontal(|ui| { ui.label(label); diff --git a/crates/ui-egui/tests/protect_password.rs b/crates/ui-egui/tests/protect_password.rs new file mode 100644 index 000000000..dc2b1c59a --- /dev/null +++ b/crates/ui-egui/tests/protect_password.rs @@ -0,0 +1,70 @@ +//! Protect Sheet / Unprotect Sheet mask the password as it's typed (issue #347). + +use egui::accesskit::Role; +use egui_kittest::kittest::Queryable as _; +use gridcraft_engine::Session; +use gridcraft_ui_egui::SheetApp; +use serde_json::json; + +const SECRET: &str = "s3cret"; + +fn harness() -> egui_kittest::Harness<'static, SheetApp> { + let mut session = Session::new(); + session.new_workbook(); + let mut h = egui_kittest::Harness::builder().with_size(egui::vec2(1200.0, 800.0)).build_ui_state( + |ui, app: &mut SheetApp| { + let ctx = ui.ctx().clone(); + app.logic(&ctx); + app.ui(ui); + }, + SheetApp::new(session, Default::default()), + ); + h.run_steps(4); + h +} + +fn type_password(h: &mut egui_kittest::Harness<'static, SheetApp>) { + h.get_by_role(Role::PasswordInput).click(); + h.run_steps(2); + h.input_mut().events.push(egui::Event::Text(SECRET.into())); + h.run_steps(2); +} + +fn secret_on_screen(h: &egui_kittest::Harness<'static, SheetApp>) -> bool { + h.query_all_by(|n| n.value().is_some_and(|v| v.contains(SECRET)) || n.label().is_some_and(|l| l.contains(SECRET))).next().is_some() +} + +#[test] +fn protect_and_unprotect_sheet_mask_the_password() { + let mut h = harness(); + h.state_mut().open_dialog("protectSheet", json!({})); + h.run_steps(3); + type_password(&mut h); + assert_eq!(h.state().dialog.as_ref().and_then(|d| d.values.get("password")), Some(&json!(SECRET))); + assert!(!secret_on_screen(&h), "the typed password must not be readable"); + assert!(h.get_by_role(Role::PasswordInput).value().is_some_and(|v| !v.is_empty() && !v.contains(SECRET))); + + // Show password reveals it on purpose; unticking masks it again. + h.get_by_label("Show password").click(); + h.run_steps(3); + assert!(h.query_by_role(Role::PasswordInput).is_none()); + assert!(secret_on_screen(&h)); + h.get_by_label("Show password").click(); + h.run_steps(3); + assert!(h.query_by_role(Role::PasswordInput).is_some()); + assert!(!secret_on_screen(&h)); + + 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())); + + // Unprotect asks for the password with a masked field too, and it starts hidden again. + h.state_mut().open_dialog("unprotectSheet", json!({})); + h.run_steps(3); + type_password(&mut h); + assert!(!secret_on_screen(&h), "the typed password must not be readable"); + h.get_by_label(" OK ").click(); + h.run_steps(3); + assert!(h.state().session.active().unwrap().wb.active().unwrap().protection.is_none()); +}