UI #1114: sanitize suggested attachment save filenames (#1630)

Refs #1114, item 3 only. Preserve stored metadata, payload bytes, and the user-selected destination.

Co-authored-by: OpenAI Codex <[email protected]>

crates/ui-egui/src/files.rs

@@ -190,6 +190,28 @@ pub(crate) fn file_stem_from_user(name: &str) -> String {
     if reserved { format!("_{stem}") } else { stem.to_string() }
 }
 
+/// A safe native Save suggestion for a PDF-supplied attachment name.
+/// This changes only the suggestion, never the stored name or the selected destination.
+#[cfg(any(test, not(target_arch = "wasm32")))]
+pub(crate) fn attachment_file_name(name: &str) -> String {
+    let cleaned: String = name
+        .chars()
+        .filter(|c| !c.is_control() && !matches!(c, '\u{061c}' | '\u{200e}' | '\u{200f}' | '\u{202a}'..='\u{202e}' | '\u{2066}'..='\u{2069}'))
+        .map(|c| if matches!(c, '/' | '\\' | ':' | '*' | '?' | '"' | '<' | '>' | '|') { '_' } else { c })
+        .collect();
+    let name = cleaned.trim_start().trim_end_matches(|c: char| c == '.' || c.is_whitespace());
+    if name.is_empty() {
+        return "attachment".into();
+    }
+    let device = name.split('.').next().unwrap_or_default().trim_end().to_ascii_uppercase();
+    let reserved = matches!(device.as_str(), "CON" | "PRN" | "AUX" | "NUL")
+        || device
+            .strip_prefix("COM")
+            .or_else(|| device.strip_prefix("LPT"))
+            .is_some_and(|number| matches!(number, "1" | "2" | "3" | "4" | "5" | "6" | "7" | "8" | "9" | "¹" | "²" | "³"));
+    if reserved { format!("_{name}") } else { name.to_string() }
+}
+
 /// Pages ▸ Rotate Pages.
 #[derive(Clone, Debug, PartialEq)]
 pub struct RotateDraft {
@@ -969,7 +991,7 @@ impl PdfCraftApp {
 
 #[cfg(test)]
 mod stem_tests {
-    use super::file_stem_from_user;
+    use super::{attachment_file_name, file_stem_from_user};
 
     #[test]
     fn user_file_names_stay_inside_the_folder_and_writable_everywhere() {
@@ -986,4 +1008,40 @@ mod stem_tests {
         let long = file_stem_from_user(&"文".repeat(300));
         assert!(long.len() <= 200 && long.chars().all(|c| c == '文'), "{}", long.len());
     }
+    #[test]
+    fn attachment_save_name_removes_bidi_without_hiding_the_extension() {
+        assert_eq!(attachment_file_name("invoice\u{202e}fdp.exe"), "invoicefdp.exe");
+        for c in [
+            '\u{061c}', '\u{200e}', '\u{200f}', '\u{202a}', '\u{202b}', '\u{202c}', '\u{202d}', '\u{202e}', '\u{2066}', '\u{2067}', '\u{2068}',
+            '\u{2069}',
+        ] {
+            assert_eq!(attachment_file_name(&format!("a{c}b.pdf")), "ab.pdf");
+        }
+    }
+    #[test]
+    fn attachment_save_name_preserves_ordinary_unicode_and_extensions() {
+        for name in ["report.pdf", "archive.tar.gz", "مرحبا.pdf", "שלום.txt", "資料.pdf", "می\u{200c}روم.txt", "👩\u{200d}💻.png", "COM10.txt"]
+        {
+            assert_eq!(attachment_file_name(name), name);
+        }
+    }
+    #[test]
+    fn attachment_save_name_handles_controls_paths_and_device_names() {
+        assert_eq!(attachment_file_name("a\u{0}\n\u{85}b.pdf"), "ab.pdf");
+        assert_eq!(attachment_file_name("../a\\b:c*?\"<>|.pdf"), ".._a_b_c______.pdf");
+        for name in ["NUL.tar.gz", "con.txt", "COM1.exe", "LPT9.txt", "COM¹.txt", "LPT³.pdf"] {
+            assert_eq!(attachment_file_name(name), format!("_{name}"));
+        }
+    }
+    #[test]
+    fn attachment_save_name_has_a_stable_nonempty_fallback() {
+        for name in ["", " ", ".", "..", " . . ", "\u{202e}", "\u{0}\n"] {
+            assert_eq!(attachment_file_name(name), "attachment");
+        }
+        assert_eq!(attachment_file_name(" report.pdf . \u{2003}."), "report.pdf");
+        for name in ["invoice\u{202e}fdp.exe", "../a.pdf", "NUL.txt", " . . ", "report.pdf . ", "資料.pdf"] {
+            let once = attachment_file_name(name);
+            assert_eq!(attachment_file_name(&once), once);
+        }
+    }
 }

crates/ui-egui/src/lib.rs

@@ -1429,12 +1429,14 @@ impl PdfCraftApp {
             }
             (Ok(bytes), false) => {
                 #[cfg(not(target_arch = "wasm32"))]
-                self.ask_one(pickers::Ask::Save(rfd::AsyncFileDialog::new().set_file_name(&att.name)), None, move |app, path| {
-                    match crate::editing::write_atomically(&path.to_string_lossy(), &bytes) {
+                self.ask_one(
+                    pickers::Ask::Save(rfd::AsyncFileDialog::new().set_file_name(crate::files::attachment_file_name(&att.name))),
+                    None,
+                    move |app, path| match crate::editing::write_atomically(&path.to_string_lossy(), &bytes) {
                         Ok(()) => app.notify_fmt("Saved {name}", &[("name", &path.display().to_string())]),
                         Err(e) => app.notify_fmt("Couldn't save: {e}", &[("e", &e.to_string())]),
-                    }
-                });
+                    },
+                );
                 #[cfg(target_arch = "wasm32")]
                 self.notify_fmt("Downloading attachments on the web arrives with M3.10 ({n} bytes ready)", &[("n", &bytes.len().to_string())]);
             }

crates/ui-egui/tests/pickers.rs

@@ -711,3 +711,82 @@ fn a_background_startup_forms_script_notice_waits_until_its_tab_is_selected() {
     h.run_steps(2);
     assert!(h.state().toast.is_none(), "the notice is delivered once");
 }
+
+/// Embedded bytes and the PDF's original /UF name stay independent of the Save suggestion.
+fn attachment_fixture() -> Vec<u8> {
+    let name = "invoice\u{202e}fdp.exe";
+    let uf = std::iter::once(0xfeffu16).chain(name.encode_utf16()).map(|c| format!("{c:04X}")).collect::<String>();
+    let objs = [
+        "<< /Type /Catalog /Pages 2 0 R /Names << /EmbeddedFiles << /Names [(notes.txt) 5 0 R] >> >> >>".to_string(),
+        "<< /Type /Pages /Kids [3 0 R] /Count 1 >>".to_string(),
+        "<< /Type /Page /Parent 2 0 R /MediaBox [0 0 200 200] /Resources << >> >>".to_string(),
+        "<< /Type /EmbeddedFile /Length 11 >>\nstream\nhello notes\nendstream".to_string(),
+        format!("<< /Type /Filespec /F (notes.txt) /UF <{uf}> /EF << /F 4 0 R >> >>"),
+    ];
+    let mut out = b"%PDF-1.7\n".to_vec();
+    let mut offsets = Vec::new();
+    for (i, o) in objs.iter().enumerate() {
+        offsets.push(out.len());
+        out.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes());
+    }
+    let xref = out.len();
+    out.extend_from_slice(b"xref\n0 6\n0000000000 65535 f \n");
+    for o in offsets {
+        out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes());
+    }
+    out.extend_from_slice(format!("trailer\n<< /Size 6 /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n").as_bytes());
+    out
+}
+
+struct AttachmentDir(std::path::PathBuf);
+
+impl AttachmentDir {
+    fn new() -> Self {
+        use std::sync::atomic::{AtomicU64, Ordering};
+        static NEXT: AtomicU64 = AtomicU64::new(0);
+        for _ in 0..128 {
+            let path = std::env::temp_dir().join(format!("pdfcraft-attachment-save-{}-{}", std::process::id(), NEXT.fetch_add(1, Ordering::Relaxed)));
+            match std::fs::create_dir(&path) {
+                Ok(()) => return Self(path),
+                Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => continue,
+                Err(error) => panic!("creating attachment Save test directory: {error}"),
+            }
+        }
+        panic!("no unused attachment Save test directory after 128 attempts");
+    }
+}
+
+impl Drop for AttachmentDir {
+    fn drop(&mut self) {
+        let _ = std::fs::remove_dir_all(&self.0);
+    }
+}
+
+#[test]
+fn attachment_save_preserves_bytes_metadata_and_selected_destination() {
+    let dir = AttachmentDir::new();
+    let chosen = dir.0.join("user chosen 資料.bin");
+    let original = attachment_fixture();
+    let mut h = harness(&[]);
+    h.state_mut().open_bytes("attachment.pdf", None, original.clone()).unwrap();
+    h.run_steps(2);
+    let id = h.state().views[0].id;
+    let raw_name = "invoice\u{202e}fdp.exe";
+    let index = h.state().session.get(id).unwrap().info.attachments.iter().position(|a| a.name == raw_name).unwrap();
+    h.state_mut().pick_override = Some(vec![chosen.to_string_lossy().into_owned()]);
+    h.state_mut().attachment_action(id, index, false);
+    h.run_steps(2);
+    assert_eq!(std::fs::read(&chosen).unwrap(), b"hello notes");
+    let doc = h.state().session.get(id).unwrap();
+    assert_eq!(doc.info.attachments[index].name, raw_name);
+    assert_eq!(doc.bytes.as_ref(), &original);
+    assert!(!dir.0.join("invoicefdp.exe").exists());
+    std::fs::remove_file(&chosen).unwrap();
+    h.state_mut().pick_override = Some(Vec::new());
+    h.state_mut().attachment_action(id, index, false);
+    h.run_steps(2);
+    assert_eq!(std::fs::read_dir(&dir.0).unwrap().count(), 0);
+    let doc = h.state().session.get(id).unwrap();
+    assert_eq!(doc.info.attachments[index].name, raw_name);
+    assert_eq!(doc.bytes.as_ref(), &original);
+}