M4: a recovered document's first Save asks where instead of writing the sidecar's path (#3923) (#4327)

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.

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(),

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();

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<RecoveryMeta>,
     recovery_keys: std::collections::HashMap<DocId, String>,
+    /// 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<DocId>,
     last_autosave: Option<web_time::Instant>,
     allow_quit: bool,
     /// Shortcuts pressed while a text field had the keyboard, run on the next frame (see

crates/ui-egui/src/persist/recovery.rs

@@ -5,6 +5,9 @@
 //! `<key>.pdf` plus `<key>.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<String>,
     /// 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::<RecoveryMeta>(&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::<RecoveryMeta>(&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<Vec<u8>> {
-        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<RecoveryMeta> {
+    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<String> = 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);
+    }
+}

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");
 }

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"