From 7a1e0bce86d9851a9dfa44f97207bc32d92ff9ed Mon Sep 17 00:00:00 2001 From: Tristan von Kries Date: Sat, 10 Oct 2026 21:38:59 +0200 Subject: [PATCH] Issue #813: a cancelled recovery password no longer attaches to the next unlocked document (#858) Recovering an encrypted snapshot parked its metadata in `pending_recovered` until the password prompt was answered. Cancel left it there, and the next encrypted document unlocked in the foreground took the snapshot's original path and recovery key: Save would overwrite the other file and delete its snapshot. The association now travels with the prompt (`PasswordPrompt::recovered`). Cancel puts the entry back on offer and leaves the store alone; a wrong password keeps it on the re-raised prompt; the correct one finishes the recovery of the tab that was just opened. `pending_recovered` is gone. Test: crash with an encrypted snapshot of A, Recover, Cancel, open encrypted B with its password. B keeps its own path and is clean, A's snapshot stays in the store, also after closing B. Fails on main at "B keeps its own path". Co-authored-by: Tristan von Kries Co-authored-by: Claude Fable 5.1 --- crates/ui-egui/src/lib.rs | 33 ++++++++++++++------- crates/ui-egui/src/recovery.rs | 16 +++++----- crates/ui-egui/tests/recovery.rs | 51 ++++++++++++++++++++++++++++---- 3 files changed, 78 insertions(+), 22 deletions(-) diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 157725080..e2dfca1ff 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -349,6 +349,9 @@ pub struct PasswordPrompt { pub error: Option, /// A late startup file must stay in the background even after it is unlocked. activate: bool, + /// The bytes are a recovery snapshot: once the document is open it takes the snapshot's + /// path and recovery entry. Cancelling the prompt drops the association with it (#813). + recovered: Option, } #[derive(serde::Serialize, serde::Deserialize)] @@ -525,7 +528,6 @@ pub struct PdfCraftApp { pub recoverable: Vec, recovery_keys: std::collections::HashMap, last_autosave: f64, - pending_recovered: Option, allow_quit: bool, /// Shortcuts pressed while a text field had the keyboard, run on the next frame (see /// `registry_shortcuts`). @@ -751,7 +753,6 @@ impl PdfCraftApp { recoverable: Vec::new(), recovery_keys: Default::default(), last_autosave: 0.0, - pending_recovered: None, allow_quit: false, deferred_commands: Vec::new(), dialog_seen: None, @@ -869,7 +870,8 @@ impl PdfCraftApp { Ok(id) => id, Err(e @ (OpenError::NeedsPassword | OpenError::WrongPassword)) => { let error = matches!(e, OpenError::WrongPassword).then(|| "Incorrect password. Try again.".to_string()); - self.password_prompt = Some(PasswordPrompt { name: name.to_string(), path, bytes, input: String::new(), error, activate }); + self.password_prompt = + Some(PasswordPrompt { name: name.to_string(), path, bytes, input: String::new(), error, activate, recovered: None }); return Ok(()); } Err(e) => return Err(e.to_string()), @@ -1050,17 +1052,28 @@ impl PdfCraftApp { /// Answer the password prompt (`None` cancels). pub fn submit_password(&mut self, password: Option) { let Some(p) = self.password_prompt.take() else { return }; - let Some(pw) = password else { return }; + let Some(pw) = password else { + // A cancelled recovery goes back on offer; its snapshot stays in the store and must + // not attach itself to whatever document is unlocked next (#813). + if let Some(meta) = p.recovered { + self.recoverable.push(meta); + } + return; + }; match self.try_open(&p.name, p.path, p.bytes, Some(&pw), p.activate) { Err(e) => self.notify_fmt("Couldn't open {name}: {e}", &[("name", &p.name), ("e", &e.to_string())]), - // A recovered encrypted document is open once its foreground prompt is gone. - // Unlocking a background startup file must not finish another file's recovery. - Ok(()) if p.activate && self.password_prompt.is_none() => { - if let Some(meta) = self.pending_recovered.clone() { - self.finish_recovery(&meta); + // A recovered encrypted document is open once its prompt is gone. + Ok(()) if self.password_prompt.is_none() => { + if let Some(meta) = &p.recovered { + self.finish_recovery(meta); + } + } + // Wrong password: the new prompt is for the same bytes, so it keeps the snapshot. + Ok(()) => { + if let Some(prompt) = self.password_prompt.as_mut() { + prompt.recovered = p.recovered; } } - Ok(()) => {} } } diff --git a/crates/ui-egui/src/recovery.rs b/crates/ui-egui/src/recovery.rs index df6b8bddb..e6ae0193a 100644 --- a/crates/ui-egui/src/recovery.rs +++ b/crates/ui-egui/src/recovery.rs @@ -170,13 +170,16 @@ impl PdfCraftApp { continue; } }; - if meta.encrypted { - // The password prompt opens it; mark it recovered once it is open. - self.pending_recovered = Some(meta.clone()); - } match self.open_bytes(&meta.name, None, bytes) { Ok(()) if self.password_prompt.is_none() => self.finish_recovery(&meta), - Ok(()) => {} + // Encrypted: the password prompt opens it, and finishes its recovery once it + // does. The association travels with the prompt, so cancelling it cannot hand + // this snapshot's path to the next document that is unlocked (#813). + Ok(()) => { + if let Some(prompt) = self.password_prompt.as_mut() { + prompt.recovered = Some(meta.clone()); + } + } Err(e) => self.notify_fmt("Couldn't recover {name}: {e}", &[("name", &meta.name), ("e", &e.to_string())]), } } @@ -185,10 +188,9 @@ impl PdfCraftApp { /// The most recently opened tab came from `meta`: keep its recovery entry and path. pub(crate) fn finish_recovery(&mut self, meta: &RecoveryMeta) { - let Some((_, id)) = self.active_ids() else { return }; + let Some(id) = self.views.last().map(|v| v.id) else { return }; self.session.mark_recovered(id, meta.path.clone()); self.recovery_keys.insert(id, meta.key.clone()); - self.pending_recovered = None; } /// Delete recovery entries the user chose not to recover. diff --git a/crates/ui-egui/tests/recovery.rs b/crates/ui-egui/tests/recovery.rs index 8cc9eebfc..8d25965ff 100644 --- a/crates/ui-egui/tests/recovery.rs +++ b/crates/ui-egui/tests/recovery.rs @@ -153,19 +153,24 @@ fn quitting_cleanly_leaves_nothing_behind() { assert_eq!(files_in(&s), 0); } -#[test] -fn encrypted_documents_are_autosaved_encrypted_and_recovered_with_the_password() { - let mut doc = pdfcraft_cos::Document::open(std::sync::Arc::new(fixture(2))).unwrap(); +/// `bytes` protected with an open password (AES-256). +fn encrypted(bytes: Vec, password: &str) -> Vec { + let mut doc = pdfcraft_cos::Document::open(std::sync::Arc::new(bytes)).unwrap(); doc.set_encryption(&pdfcraft_cos::NewEncryption { algorithm: pdfcraft_cos::Algorithm::Aes256, - user_password: "pw", + user_password: password, owner_password: "owner", permissions: -1, encrypt_metadata: true, seed: [2; 32], }) .unwrap(); - let bytes = pdfcraft_cos::write_full(&doc, &Default::default()).unwrap(); + pdfcraft_cos::write_full(&doc, &Default::default()).unwrap() +} + +#[test] +fn encrypted_documents_are_autosaved_encrypted_and_recovered_with_the_password() { + let bytes = encrypted(fixture(2), "pw"); let s = store("encrypted"); crashed_session(&s, bytes, Some("pw"), None); let meta = s.list().remove(0); @@ -190,6 +195,42 @@ fn encrypted_documents_are_autosaved_encrypted_and_recovered_with_the_password() let _ = Modifiers::NONE; } +/// #813: cancelling the password prompt of a recovered snapshot must not hand its path and +/// recovery entry to the next encrypted document that is unlocked (Save would then overwrite +/// the snapshot's original file and delete the snapshot). +#[test] +fn a_cancelled_recovery_password_does_not_attach_to_the_next_unlocked_document() { + let s = store("cancelled"); + crashed_session(&s, encrypted(fixture(2), "pw-a"), Some("pw-a"), Some("/docs/A.pdf")); + assert_eq!(s.list().len(), 1); + let mut h = harness(s.clone()); + h.get_by_label("Recover").click(); + h.run_steps(3); + assert!(h.state().password_prompt.is_some(), "the snapshot asks for its password"); + h.get_by_label("Cancel").click(); + h.run_steps(3); + assert!(h.state().password_prompt.is_none()); + assert!(h.state().views.is_empty()); + + let app = h.state_mut(); + app.open_bytes("B.pdf", Some("/docs/B.pdf".into()), encrypted(fixture(1), "pw-b")).unwrap(); + app.submit_password(Some("pw-b".into())); + h.run_steps(3); + let app = h.state(); + assert_eq!(app.views.len(), 1); + let b = app.session.get(app.views[0].id).unwrap(); + assert_eq!(b.path.as_deref(), Some("/docs/B.pdf"), "B keeps its own path"); + assert!(!b.dirty, "B is clean: it is not the recovered document"); + assert_eq!(s.list().len(), 1, "A's snapshot is still in the store"); + assert_eq!(app.recoverable.len(), 1, "and on offer again"); + + // Closing B (clean, so no prompt) must not remove A's snapshot either. + h.state_mut().execute("file.close"); + h.run_steps(3); + assert!(h.state().views.is_empty()); + assert_eq!(s.list().len(), 1, "A's snapshot survives closing B"); +} + #[test] fn incomplete_entries_are_ignored_and_cleaned_up() { let s = store("partial");