diff --git a/ROADMAP.md b/ROADMAP.md index 85f06fcd1e..1fb7459f49 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -313,6 +313,7 @@ Ranked; detail and the full milestone table in [docs/roadmap.md](docs/roadmap.md - **2026-10-23 (M11, JBIG2):** The PDF Optimizer can store black-and-white images as lossless JBIG2 (a generic region, written from the JBIG2 draft standard and checked against its arithmetic-coder test sequence), about a third smaller than Group 4 on 300 dpi text, and Export ▸ Images reads JBIG2 images. JBIG2 symbol coding, where its largest gains on text are, is next. - **2026-10-23 (M11, monochrome compression):** The PDF Optimizer and Reduce File Size store black-and-white images (scans, image masks) losslessly as CCITT Group 4 when that is smaller, with a Monochrome Images setting in the Optimizer and `doc_optimize` `mono`. JBIG2 and monochrome downsampling are next. - **2026-10-23 (M6, calculation checks):** The calculation order now flags a formula that reads a field the form doesn't have (a field deleted from under a sum, a typo in notation, a script's `getField`) and a script that builds the names it reads at run time. Prepare a form ▸ Calculation order marks both on the row and in a card that names the broken formulas, in place of its all-clear. `form_calculation_order` gives each field's formula, and `form_fields` shows a custom Calculate script. +- **2026-10-23 (M4, #3923):** A recovered document's original path is only a suggestion: the first Save after recovering opens Save As at that path instead of writing it, since anyone who can write the recovery folder (a shared portable `PdfCraftData`, for example) can name any file in a sidecar. The file chosen is the document's file from then on. Sidecars are read through a 64 KiB cap; a longer one is not read, and its snapshot recovers without metadata. Estimates unchanged. - **2026-10-23 (M7, #4280):** Edit text names each standard stand-in font after its face (`PCEdTimesBold`, ...) instead of one shared `PCEdHelv`. Rewriting a bold paragraph whose subset lacks an accent no longer repoints the paragraphs set earlier in a regular stand-in at Times-Bold, and the editor's frame stays on the paragraph being edited. Pages saved by older builds keep their shared name and are not repaired. Estimates unchanged. - **2026-10-23 (M8, #3205 follow-up):** Phone-pattern redaction matches complete labelled Italian geographic numbers such as `Tel. +39 0426/123456`, with synthetic matcher and mark/apply/save/reopen coverage; unlabelled international slash numbers remain unsupported. - **2026-10-23 (M7, #2636 partial):** A new `PasteContent` edit pastes an added text box or image copied in the same document or another, keeping its style, picture, turns, flips and crop, as one undo step; `content_copy` drives it headlessly. A paste lands beside earlier copies (18 pt steps) and stays on the page. A pasted image reuses its picture object in its own document and after the first paste into another; there a colour profile is dropped and CMYK colours stored losslessly are turned to RGB. The desktop keys and menus follow in separate PRs. Estimates unchanged. diff --git a/crates/ui-egui/src/app.rs b/crates/ui-egui/src/app.rs index 010b7f7bd7..fd6d2dcbc6 100644 --- a/crates/ui-egui/src/app.rs +++ b/crates/ui-egui/src/app.rs @@ -279,6 +279,7 @@ impl PdfCraftApp { recovery: None, recoverable: Vec::new(), recovery_keys: Default::default(), + recovered_paths: Default::default(), last_autosave: None, allow_quit: false, deferred_commands: Vec::new(), diff --git a/crates/ui-egui/src/editing/save.rs b/crates/ui-egui/src/editing/save.rs index ffb533b243..35867559f6 100644 --- a/crates/ui-egui/src/editing/save.rs +++ b/crates/ui-egui/src/editing/save.rs @@ -57,7 +57,9 @@ impl PdfCraftApp { (_, _, Some(p)) => p.clone(), // An email attachment's temporary copy is no place to keep work: ask, as for a new // document (`is_mail_temp_copy`). - (SaveTarget::InPlace, Some(p), _) if !is_mail_temp_copy(&p) => p, + // Neither is a path read back from a recovery sidecar until a save confirms it: + // anyone who can write the recovery folder can name any file there (#3923). + (SaveTarget::InPlace, Some(p), _) if !is_mail_temp_copy(&p) && !self.recovered_paths.contains(&id) => p, _ => { let name = if name.to_ascii_lowercase().ends_with(".pdf") { name } else { format!("{name}.pdf") }; let dialog = rfd::AsyncFileDialog::new().add_filter_any_case("PDF", &["pdf"]).set_file_name(name); @@ -164,11 +166,12 @@ impl PdfCraftApp { if !self.commit_typing_in(id) || !self.flatten_fill_sign_for_save(id) { return false; } + let recovered = self.recovered_paths.contains(&id); let Some((name, full)) = self.session.get(id).map(|d| { - // Save As to another file is a clean copy, without the earlier revisions that still - // hold deleted pages or replaced text (#2165). A signed document stays incremental so - // its signatures keep covering the file. - let full = target == SaveTarget::As + // Save As to another file and a recovered document are clean copies, without earlier + // revisions that may retain deleted pages or replaced text (#2165, #3923). A signed + // document stays incremental so its signatures keep covering the file. + let full = (target == SaveTarget::As || recovered) && !d.path.as_deref().is_some_and(|p| crate::same_file(p, dest)) && d.read_only_reason.is_none() && !d.is_signed(); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 0d527d5ef5..0e47d4baff 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -680,6 +680,9 @@ pub struct PdfCraftApp { /// Entries left by a previous session, offered in the Recovery dialog. pub recoverable: Vec, recovery_keys: std::collections::HashMap, + /// Recovered documents whose path came from a recovery sidecar and no save has confirmed + /// yet: their first Save asks where, starting at that path, instead of writing it (#3923). + recovered_paths: std::collections::HashSet, last_autosave: Option, allow_quit: bool, /// Shortcuts pressed while a text field had the keyboard, run on the next frame (see diff --git a/crates/ui-egui/src/persist/recovery.rs b/crates/ui-egui/src/persist/recovery.rs index 34b6bdf76a..aa10cceb19 100644 --- a/crates/ui-egui/src/persist/recovery.rs +++ b/crates/ui-egui/src/persist/recovery.rs @@ -5,6 +5,9 @@ //! `.pdf` plus `.json` (name, original path, time). Saving, discarding or closing a //! document removes its entry. At startup the app offers snapshots even when their metadata is //! missing or damaged; those recover without an original path, so saving asks for a destination. +//! A recovered document's original path is only a suggestion: the folder is the user's, but +//! anyone who can write it can name any file in a sidecar, so the first Save after recovering +//! asks where, starting at that path, and later saves write where the user chose (#3923). //! //! Encrypted documents are stored encrypted (the working file is), so recovery never writes //! plaintext of a protected document to disk. The web build has no recovery store yet. @@ -32,7 +35,8 @@ pub const AUTOSAVE_SECS: f64 = 60.0; pub struct RecoveryMeta { pub key: String, pub name: String, - /// Where the document was saved before (Save writes back there after recovery). + /// Where the document was saved before. After recovery the first Save offers it in the + /// Save As dialog rather than writing it unasked (#3923). pub path: Option, /// Seconds since the Unix epoch when the snapshot was taken. pub saved_at: u64, @@ -230,8 +234,7 @@ impl RecoveryStore { continue; } let Some(key) = path.file_stem().and_then(std::ffi::OsStr::to_str) else { continue }; - let meta = std::fs::read(path.with_extension("json")) - .and_then(|b| serde_json::from_slice::(&b).map_err(std::io::Error::other)) + let meta = read_meta(&path.with_extension("json")) .and_then(|m| if m.key == key { Ok(m) } else { Err(std::io::Error::new(std::io::ErrorKind::InvalidData, "snapshot key mismatch")) }); out.push(meta.unwrap_or_else(|e| { log::warn!("recovering snapshot without metadata: {}: {e}", path.display()); @@ -261,7 +264,7 @@ impl RecoveryStore { /// recovered without metadata), so damaged metadata doesn't stay behind. The PDF is untouched. pub fn repair_meta(&self, meta: &RecoveryMeta) { let path = self.dir.join(format!("{}.json", meta.key)); - let readable = std::fs::read(&path).ok().and_then(|b| serde_json::from_slice::(&b).ok()).is_some_and(|m| m.key == meta.key); + let readable = read_meta(&path).is_ok_and(|m| m.key == meta.key); if readable { return; } @@ -273,8 +276,25 @@ impl RecoveryStore { } } + /// + /// Only a regular file is read, and no more than it held when it was opened: anyone who can + /// write the recovery folder can put a FIFO there, which would hold the read forever, or a + /// link to a device such as `/dev/zero`, which never ends (#3923). Snapshots have no size + /// limit of their own, as autosave writes whatever the document holds. pub fn read(&self, key: &str) -> std::io::Result> { - std::fs::read(self.dir.join(format!("{key}.pdf"))) + use std::io::Read; + let path = self.dir.join(format!("{key}.pdf")); + let meta = std::fs::metadata(&path)?; + if !meta.is_file() { + return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "the recovery snapshot is not a regular file")); + } + let len = meta.len(); + let mut bytes = Vec::new(); + File::open(&path)?.take(len.saturating_add(1)).read_to_end(&mut bytes)?; + if bytes.len() as u64 > len { + return Err(std::io::Error::new(std::io::ErrorKind::InvalidData, "the recovery snapshot grew while it was read")); + } + Ok(bytes) } /// Remove every entry (clean quit, or "Discard all"), and metadata left without a snapshot. @@ -296,6 +316,22 @@ impl RecoveryStore { } } +/// The most a sidecar may hold. One PdfCraft writes is a name, a path and a few fields, well under +/// this; anything longer is not one of ours (#3923). +const MAX_META_BYTES: u64 = 64 * 1024; + +/// A sidecar's metadata, read through [`MAX_META_BYTES`] so a planted file can't make the start-up +/// listing read gigabytes. A longer file is refused, and its snapshot recovers without metadata. +fn read_meta(path: &Path) -> std::io::Result { + use std::io::Read; + let mut bytes = Vec::new(); + File::open(path)?.take(MAX_META_BYTES.saturating_add(1)).read_to_end(&mut bytes)?; + if bytes.len() as u64 > MAX_META_BYTES { + return Err(std::io::Error::new(std::io::ErrorKind::InvalidData, "recovery metadata is too large")); + } + serde_json::from_slice(&bytes).map_err(std::io::Error::other) +} + fn now_secs() -> u64 { std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).map(|d| d.as_secs()).unwrap_or(0) } @@ -329,6 +365,9 @@ impl PdfCraftApp { /// Drop a document's recovery entry (it was saved, discarded or closed). pub(crate) fn forget_recovery(&mut self, doc: pdfcraft_engine::DocId) { + // Saved (to a destination the user chose), closed or reloaded from its file: the path is + // no longer only what the sidecar said. + self.recovered_paths.remove(&doc); if let (Some(store), Some(key)) = (&self.recovery, self.recovery_keys.remove(&doc)) { store.remove(&key); } @@ -422,6 +461,9 @@ impl PdfCraftApp { pub(crate) fn finish_recovery(&mut self, meta: &RecoveryMeta) { let Some(id) = self.views.last().map(|v| v.id) else { return }; self.session.mark_recovered(id, meta.path.clone()); + if meta.path.is_some() { + self.recovered_paths.insert(id); + } self.recovery_keys.insert(id, meta.key.clone()); // A snapshot recovered without readable metadata gets a fresh sidecar now, so the // damaged one doesn't linger until the next autosave (or forever, if none comes). @@ -462,13 +504,13 @@ mod idle_tests { use super::*; use std::time::Duration; - struct TestDir(PathBuf); + pub(super) struct TestDir(PathBuf); impl TestDir { /// A new folder of this test's own. The tests run on parallel threads of one process, and /// two can read the same clock time (it moves in microseconds on macOS), so a counter /// tells their folders apart; the time keeps runs of the process apart. - fn new() -> Self { + pub(super) fn new() -> Self { static NEXT: AtomicU64 = AtomicU64::new(0); let n = NEXT.fetch_add(1, Ordering::Relaxed); let nonce = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos(); @@ -478,7 +520,7 @@ mod idle_tests { } /// The folder. - fn path(&self) -> &Path { + pub(super) fn path(&self) -> &Path { &self.0 } } @@ -519,6 +561,31 @@ mod idle_tests { out.viewport_output[&egui::ViewportId::ROOT].repaint_delay } + /// #3923 (review on #4327): a snapshot that is not a regular file is refused before it is + /// opened. A FIFO in the recovery folder would otherwise hold the read, and the recovery with + /// it, until something wrote to it. + #[cfg(unix)] + #[test] + fn a_snapshot_that_is_not_a_regular_file_is_refused_at_once() { + let dir = TestDir::new(); + let store = RecoveryStore::new(dir.path()); + let fifo = dir.path().join("planted.pdf"); + match std::process::Command::new("mkfifo").arg(&fifo).status() { + Ok(s) if s.success() => {} + _ => return, // No mkfifo here: nothing to plant. + } + let (tx, rx) = std::sync::mpsc::channel(); + let reader = store.clone(); + std::thread::spawn(move || { + let _ = tx.send(reader.read("planted").map(|b| b.len())); + }); + let read = rx.recv_timeout(Duration::from_secs(5)).expect("the read returned instead of waiting on the FIFO"); + assert_eq!(read.map_err(|e| e.kind()), Err(std::io::ErrorKind::InvalidInput)); + // A directory under the name is refused the same way. + std::fs::create_dir(dir.path().join("folder.pdf")).unwrap(); + assert_eq!(store.read("folder").map_err(|e| e.kind()), Err(std::io::ErrorKind::InvalidInput)); + } + /// Check that an idle dirty document schedules a bounded write and autosaves without input, then stops /// waking the window. #[test] @@ -624,3 +691,73 @@ mod idle_tests { } } } + +#[cfg(test)] +mod sidecar_tests { + use super::idle_tests::TestDir; + use super::*; + use crate::SaveTarget; + + /// #3923: a sidecar names a file the user can write (here, one that is not the document's). + /// After Recover, Save asks where instead of writing that file; the file the user picks is + /// the document's file from then on. + #[test] + fn the_first_save_after_recovering_asks_instead_of_writing_the_sidecars_path() { + let dir = TestDir::new(); + let store = RecoveryStore::new(dir.path().join("recovery")); + let named = dir.path().join("named.pdf"); + std::fs::write(&named, b"someone else's file").unwrap(); + { + let mut crashed = PdfCraftApp::new(); + crashed.enable_recovery(store.clone()); + let bytes = crashed.session.create_blank(200.0, 300.0, 1).unwrap(); + crashed.open_bytes("work.pdf", Some(named.to_string_lossy().into_owned()), bytes.to_vec()).unwrap(); + assert!(crashed.apply_edit(pdfcraft_engine::Edit::RotatePages { pages: vec![0], degrees: 90 })); + crashed.autosave_now(); + } + + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.enable_recovery(store.clone()); + let keys: Vec = app.recoverable.iter().map(|m| m.key.clone()).collect(); + assert_eq!(keys.len(), 1); + app.recover(&keys); + assert_eq!(app.views.len(), 1); + let chosen = dir.path().join("chosen.pdf"); + app.pick_override = Some(vec![chosen.to_string_lossy().into_owned()]); + + assert!(!app.save_active(SaveTarget::InPlace), "Save asks instead of writing the sidecar's path"); + app.process_picked(); + assert_eq!(std::fs::read(&named).unwrap(), b"someone else's file", "the named file is untouched"); + assert!(std::fs::read(&chosen).unwrap().starts_with(b"%PDF"), "the document went where the user chose"); + + // The chosen file is the document's file now: the next Save writes it without asking. + app.pick_override = None; + assert!(app.apply_edit(pdfcraft_engine::Edit::RotatePages { pages: vec![0], degrees: 90 })); + assert!(app.save_active(SaveTarget::InPlace)); + } + + /// #3923: a sidecar longer than any PdfCraft writes is not read whole; its snapshot is + /// offered without metadata, so it has no path to save to. + #[test] + fn an_oversized_sidecar_is_not_read() { + let dir = TestDir::new(); + let store = RecoveryStore::new(dir.path()); + let meta = RecoveryMeta { + key: "planted".into(), + // Over 64 KiB, and still valid metadata that names a file. + name: "x".repeat(70_000), + path: Some(dir.path().join("named.pdf").to_string_lossy().into_owned()), + saved_at: 1, + encrypted: false, + instance: None, + }; + store.write(&meta, b"%PDF-1.7\n").unwrap(); + let listed = store.list(); + assert_eq!(listed.len(), 1); + assert_eq!((listed[0].name.as_str(), listed[0].path.as_deref()), ("planted.pdf", None)); + // A sidecar of ordinary size is still read. + store.write(&RecoveryMeta { name: "work.pdf".into(), ..meta.clone() }, b"%PDF-1.7\n").unwrap(); + assert_eq!(store.list()[0].path, meta.path); + } +} diff --git a/crates/ui-egui/tests/it/recovery.rs b/crates/ui-egui/tests/it/recovery.rs index 93ea045b6e..57500b310c 100644 --- a/crates/ui-egui/tests/it/recovery.rs +++ b/crates/ui-egui/tests/it/recovery.rs @@ -90,7 +90,7 @@ fn a_crashed_session_is_offered_and_recovered_unsaved() { assert_eq!(app.views.len(), 1); let doc = app.session.get(app.views[0].id).unwrap(); assert!(doc.dirty, "recovered changes are unsaved"); - assert_eq!(doc.path.as_deref(), Some("/docs/work.pdf"), "Save goes back to the original file"); + assert_eq!(doc.path.as_deref(), Some("/docs/work.pdf"), "the original path is kept, as what Save offers (#3923)"); assert_eq!(doc.info.pages.len(), 2, "the edit survived the crash"); assert_eq!(s.list().len(), 1, "the entry stays until the document is saved or closed"); } diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 927e32df83..3c17a3ed70 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -550,8 +550,8 @@ milestone = "M4" status = "shipped" commands = [] tools = [] -evidence = ["crates/ui-egui/tests/it/recovery.rs::a_crashed_session_is_offered_and_recovered_unsaved", "crates/ui-egui/tests/it/recovery.rs::discarding_removes_the_autosaves", "crates/ui-egui/tests/it/recovery.rs::a_running_instances_autosaves_are_not_offered_to_a_second_instance", "crates/engine/src/tests.rs::recovered_documents_reopen_unsaved_at_their_original_path"] -notes = "A running instance holds an exclusively locked instance file in the recovery folder and names it in its snapshots, so a second instance neither offers nor discards what the first is autosaving (#2166); the OS releases the lock when the process dies. Where the folder can't be written or the platform has no file locks, snapshots are offered as before." +evidence = ["crates/ui-egui/tests/it/recovery.rs::a_crashed_session_is_offered_and_recovered_unsaved", "crates/ui-egui/tests/it/recovery.rs::discarding_removes_the_autosaves", "crates/ui-egui/tests/it/recovery.rs::a_running_instances_autosaves_are_not_offered_to_a_second_instance", "crates/engine/src/tests.rs::recovered_documents_reopen_unsaved_at_their_original_path", "crates/ui-egui/src/persist/recovery.rs::the_first_save_after_recovering_asks_instead_of_writing_the_sidecars_path", "crates/ui-egui/src/persist/recovery.rs::an_oversized_sidecar_is_not_read", "crates/ui-egui/src/persist/recovery.rs::a_snapshot_that_is_not_a_regular_file_is_refused_at_once"] +notes = "A recovered document keeps its original path only as a suggestion: the first Save asks where, starting there, and the file chosen is its file from then on, since anyone who can write the recovery folder can name any file in a sidecar (#3923); sidecars longer than 64 KiB are not read, and their snapshot is offered without metadata. A running instance holds an exclusively locked instance file in the recovery folder and names it in its snapshots, so a second instance neither offers nor discards what the first is autosaving (#2166); the OS releases the lock when the process dies. Where the folder can't be written or the platform has no file locks, snapshots are offered as before." [[feature]] id = "core.encrypted-autosave"