Security: print jobs, new digital IDs and the --control file no longer write through planted links (#338)

* Security: pipe print jobs to lp instead of a shared temp folder

On macOS, Linux and FreeBSD, Print wrote each job to
<temp>/pdfcraft-print-<pid>/job-<time>.pdf and passed that name to lp.
The folder name is predictable and was made with create_dir_all, which
accepts a folder another local user created first in a shared /tmp and
follows a symlink planted there; the job was then written with fs::write.
That user could read every printed document, or swap the file before lp
read it. The folder was also never removed.

The job now goes to lp on stdin, so nothing is written to disk. lp_args
no longer takes a file. The job is fed on its own thread while lp's
output is collected, so neither side can block on a full pipe; if that
thread can't be started, lp is stopped and an error returned rather than
a panic. When lp refuses a job before reading it (an unknown printer)
the user sees lp's message rather than a broken pipe. CUPS ends its reply
for a job read from stdin with "(0 file(s))", which reads as if nothing
was sent, so that is dropped from the message shown.

submit_via takes the spooler command so tests can run this test binary
as a stand-in lp on every platform; the new test failed on the old code
(it received the temp file as an argument and nothing on stdin).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Security: create a new digital ID file fresh, owner-only

Configure New Digital ID > Create picked the first free name with
`!p.exists()` and then wrote the .p12 with fs::write. That is
check-then-write: a file or link planted at the name between the two is
written through. exists() also follows links, so a dangling symlink
looked free and the private key was created wherever it pointed. The
file got default permissions, readable by other local users on most
Unix systems.

save_new_id_file now creates the file with create_new, never opening
anything already at the name, and with mode 0o600 on Unix. A taken name
moves on to the next ("<name> 2.p12", ...), including a folder, which
Windows reports as access denied rather than already exists. A failed
write removes the partial file.

The dangling-link test failed on the old code (the key was written
through the link); the hard-link and folder test guards the retry loop,
and a Unix-only test checks the 0o600 mode.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Security: never write the --control token through a planted link

write_control_file opened the --control path with create + truncate. A
symlink or hard link already at that path was followed: its target was
truncated and overwritten with the control token before set_permissions
ran, so the token could land in a file another user can read, and any
file the link pointed at was destroyed.

The JSON now goes to a fresh file next to the path, with a random name
(create_new, 0o600 on Unix, another name if one is taken, including a
folder on Windows), which is then renamed over the path. The rename
replaces a link instead of following it, a control file left by an
earlier run is still replaced as before, and a reader polling for the
file never sees it half-written. A failed write or rename removes the
temporary file.

The hard-link and symlink tests failed on the old code (the token was
written into the linked file); a third test covers replacing a previous
run's file, and the owner-only mode on Unix.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Contributors: credit jcl-bit as James Locke

My own entry in contributors/people.toml (AGENTS.md, Contributor
credits), added in my PR as asked there.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Brandon Thomas <echelon@gmail.com>

apps/pdfcraft/src/main.rs

@@ -193,23 +193,51 @@ fn main() -> eframe::Result {
 }
 
 /// Write the control endpoint so that only the current user can read the token.
+///
+/// The JSON goes to a new file next to `path`, created fresh (owner-only on Unix), which then
+/// replaces `path`. Opening `path` itself would follow a link planted there and truncate whatever
+/// it points at; a rename replaces the link and leaves its target alone. A reader polling for the
+/// file also never sees it half-written.
 fn write_control_file(path: &str, port: u16, token: &str) -> std::io::Result<()> {
+    use std::io::{ErrorKind, Write};
     let json = serde_json::json!({ "port": port, "token": token, "pid": std::process::id() }).to_string();
-    let mut opts = std::fs::OpenOptions::new();
-    opts.write(true).create(true).truncate(true);
-    #[cfg(unix)]
-    {
-        use std::os::unix::fs::OpenOptionsExt;
-        opts.mode(0o600);
+    let path = std::path::Path::new(path);
+    let name = path.file_name().ok_or_else(|| std::io::Error::new(ErrorKind::InvalidInput, "not a file name"))?;
+    // Unpredictable suffixes, so the name can't be planted in advance. `RandomState` is keyed from
+    // the operating system's random source.
+    let random = std::hash::RandomState::new();
+    for i in 0..16u32 {
+        let mut staged = std::ffi::OsString::from(".");
+        staged.push(name);
+        staged.push(format!(".{:016x}.tmp", std::hash::BuildHasher::hash_one(&random, i)));
+        let staged = path.with_file_name(staged);
+        let mut opts = std::fs::OpenOptions::new();
+        opts.write(true).create_new(true);
+        #[cfg(unix)]
+        {
+            use std::os::unix::fs::OpenOptionsExt;
+            opts.mode(0o600);
+        }
+        let file = match opts.open(&staged) {
+            Ok(f) => f,
+            Err(e) if e.kind() == ErrorKind::AlreadyExists => continue,
+            // Windows refuses `create_new` on a folder with "access denied"; the name is taken all
+            // the same. A folder we can't write to, with nothing at the name, still fails here.
+            Err(e) if e.kind() == ErrorKind::PermissionDenied && staged.symlink_metadata().is_ok() => continue,
+            Err(e) => return Err(e),
+        };
+        // The block closes the file before it is renamed.
+        let written = {
+            let mut file = file;
+            file.write_all(json.as_bytes())
+        };
+        let done = written.and_then(|()| std::fs::rename(&staged, path));
+        if done.is_err() {
+            let _ = std::fs::remove_file(&staged);
+        }
+        return done;
     }
-    use std::io::Write;
-    let mut f = opts.open(path)?;
-    #[cfg(unix)]
-    {
-        use std::os::unix::fs::PermissionsExt;
-        f.set_permissions(std::fs::Permissions::from_mode(0o600))?;
-    }
-    f.write_all(json.as_bytes())
+    Err(std::io::Error::new(ErrorKind::AlreadyExists, "no free name for the control file's temporary copy"))
 }
 
 /// How wgpu finds a GPU. Each choice yields to its wgpu environment variable.
@@ -377,4 +405,76 @@ mod tests {
         assert!(super::linux_display_gpus(std::path::Path::new("/nonexistent/drm")).is_empty());
         Ok(())
     }
+
+    /// A fresh folder per call: tests run in parallel.
+    fn scratch() -> std::path::PathBuf {
+        static N: std::sync::atomic::AtomicU32 = std::sync::atomic::AtomicU32::new(0);
+        let n = N.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
+        let d = std::env::temp_dir().join(format!("pdfcraft-control-file-{}-{n}", std::process::id()));
+        let _ = std::fs::remove_dir_all(&d);
+        std::fs::create_dir_all(&d).unwrap();
+        d
+    }
+
+    /// The control file's port and token, and every name in its folder.
+    fn written(dir: &std::path::Path) -> (serde_json::Value, Vec<String>) {
+        let json = serde_json::from_str(&std::fs::read_to_string(dir.join("ctl.json")).unwrap()).unwrap();
+        let mut names: Vec<String> = std::fs::read_dir(dir).unwrap().map(|e| e.unwrap().file_name().to_string_lossy().into_owned()).collect();
+        names.sort();
+        (json, names)
+    }
+
+    #[test]
+    fn the_control_file_replaces_a_hard_link_instead_of_writing_through_it() {
+        // The file used to be opened with create + truncate, so a link planted at the path had its
+        // target truncated and overwritten with the token.
+        let dir = scratch();
+        std::fs::write(dir.join("victim.txt"), "keep me").unwrap();
+        std::fs::hard_link(dir.join("victim.txt"), dir.join("ctl.json")).unwrap();
+        super::write_control_file(&dir.join("ctl.json").to_string_lossy(), 4242, "t0ken").unwrap();
+        assert_eq!(std::fs::read_to_string(dir.join("victim.txt")).unwrap(), "keep me");
+        let (json, names) = written(&dir);
+        assert_eq!((json["port"].as_u64(), json["token"].as_str()), (Some(4242), Some("t0ken")));
+        assert_eq!(names, ["ctl.json", "victim.txt"], "no temporary file left behind");
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
+    #[test]
+    fn the_control_file_replaces_a_symlink_instead_of_writing_through_it() {
+        let dir = scratch();
+        let (link, target) = (dir.join("ctl.json"), dir.join("victim.txt"));
+        std::fs::write(&target, "keep me").unwrap();
+        #[cfg(unix)]
+        let made = std::os::unix::fs::symlink(&target, &link);
+        #[cfg(windows)]
+        let made = std::os::windows::fs::symlink_file(&target, &link);
+        if let Err(e) = made {
+            // Windows needs Developer Mode (or admin) for symlinks.
+            eprintln!("skipped: can't create a symlink here: {e}");
+            let _ = std::fs::remove_dir_all(&dir);
+            return;
+        }
+        super::write_control_file(&link.to_string_lossy(), 4242, "t0ken").unwrap();
+        assert_eq!(std::fs::read_to_string(&target).unwrap(), "keep me");
+        assert!(!std::fs::symlink_metadata(&link).unwrap().file_type().is_symlink());
+        assert_eq!(written(&dir).0["token"].as_str(), Some("t0ken"));
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
+    #[test]
+    fn the_control_file_replaces_the_one_a_previous_run_left() {
+        let dir = scratch();
+        std::fs::write(dir.join("ctl.json"), r#"{"port":1,"token":"old"}"#).unwrap();
+        super::write_control_file(&dir.join("ctl.json").to_string_lossy(), 4242, "t0ken").unwrap();
+        let (json, names) = written(&dir);
+        assert_eq!(json["token"].as_str(), Some("t0ken"));
+        assert_eq!(names, ["ctl.json"]);
+        #[cfg(unix)]
+        {
+            use std::os::unix::fs::PermissionsExt;
+            let mode = std::fs::metadata(dir.join("ctl.json")).unwrap().permissions().mode();
+            assert_eq!(mode & 0o077, 0, "only the owner can read the token: {mode:o}");
+        }
+        let _ = std::fs::remove_dir_all(&dir);
+    }
 }

contributors/people.toml

@@ -22,6 +22,11 @@ real_name = "Brandon Thomas"
 display_name = "Brandon Thomas"
 sync_github_name = false
 
+[people.jcl-bit]
+real_name = "James Locke"
+display_name = "James Locke"
+sync_github_name = false
+
 [people.switch87]
 real_name = "Gert Pellin"
 display_name = "Gert Pellin"

crates/print/README.md

@@ -33,7 +33,8 @@ annotations); sheets place them with a clip. The result is a fresh, unencrypted,
 garbage-collected file (callers check the print permission).
 
 `spool` talks to CUPS (`lpstat -p -d`, `lp` with copies, collation, duplex and monochrome
-options). Other platforms report that printing to a printer isn't available yet; the
+options). The job is piped to `lp` on stdin and never written to a temp file, where another
+local user could read or swap it. Other platforms report that printing to a printer isn't available yet; the
 print-ready PDF can always be saved.
 
 Not yet: Windows and web spoolers, print as image, poster labels, PostScript output, colour

crates/print/src/spool.rs

@@ -1,6 +1,6 @@
 //! The system print spooler. On macOS and Linux this is CUPS: printers come from `lpstat`, jobs
-//! go to `lp` with the job options (copies, collation, duplex, colour). Other platforms report
-//! that printing isn't available yet; the print-ready PDF can still be saved.
+//! are piped to `lp` with the job options (copies, collation, duplex, colour). Other platforms
+//! report that printing isn't available yet; the print-ready PDF can still be saved.
 
 use crate::PrintError;
 
@@ -45,8 +45,8 @@ pub fn parse_lpstat(out: &str) -> Vec<Printer> {
         .collect()
 }
 
-/// The `lp` arguments for a job printing `file`.
-pub fn lp_args(job: &Job, file: &str) -> Vec<String> {
+/// The `lp` arguments for a job. There is no file argument: `lp` reads the job from stdin.
+pub fn lp_args(job: &Job) -> Vec<String> {
     let mut a = Vec::new();
     if let Some(p) = &job.printer {
         a.extend(["-d".to_string(), p.clone()]);
@@ -65,8 +65,6 @@ pub fn lp_args(job: &Job, file: &str) -> Vec<String> {
     }
     // The sheets are already laid out at their final size.
     opt("fit-to-page=false");
-    a.push("--".into());
-    a.push(file.to_string());
     a
 }
 
@@ -99,19 +97,7 @@ pub fn printers() -> Vec<Printer> {
 pub fn submit(pdf: &[u8], job: &Job) -> Result<String, PrintError> {
     #[cfg(all(unix, not(target_arch = "wasm32")))]
     {
-        let dir = std::env::temp_dir().join(format!("pdfcraft-print-{}", std::process::id()));
-        std::fs::create_dir_all(&dir).map_err(|e| PrintError::Spool(e.to_string()))?;
-        let file = dir.join(format!("job-{}.pdf", std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).map_or(0, |d| d.as_nanos())));
-        std::fs::write(&file, pdf).map_err(|e| PrintError::Spool(e.to_string()))?;
-        let out = std::process::Command::new("lp").args(lp_args(job, &file.to_string_lossy())).output();
-        let _ = std::fs::remove_file(&file);
-        let out = out.map_err(|e| PrintError::Spool(format!("the print spooler is not available: {e}")))?;
-        if out.status.success() {
-            Ok(String::from_utf8_lossy(&out.stdout).trim().to_string())
-        } else {
-            let err = String::from_utf8_lossy(&out.stderr).trim().to_string();
-            Err(PrintError::Spool(if err.is_empty() { "the print job was refused".into() } else { err }))
-        }
+        submit_via(std::process::Command::new("lp"), pdf, job)
     }
     #[cfg(not(all(unix, not(target_arch = "wasm32"))))]
     {
@@ -119,3 +105,64 @@ pub fn submit(pdf: &[u8], job: &Job) -> Result<String, PrintError> {
         Err(PrintError::Spool("printing to a printer isn't available on this platform yet; save the print-ready PDF instead".into()))
     }
 }
+
+/// [`submit`] with the spooler command given, so tests can stand in for `lp`.
+///
+/// The job goes to `lp` on stdin, never through a file: a predictable job folder in a shared
+/// temp directory lets another local user read printed documents or swap the file before `lp`
+/// reads it.
+#[cfg(any(test, all(unix, not(target_arch = "wasm32"))))]
+pub(crate) fn submit_via(mut lp: std::process::Command, pdf: &[u8], job: &Job) -> Result<String, PrintError> {
+    use std::io::Write;
+    use std::process::Stdio;
+    let mut child = lp
+        .args(lp_args(job))
+        .stdin(Stdio::piped())
+        .stdout(Stdio::piped())
+        .stderr(Stdio::piped())
+        .spawn()
+        .map_err(|e| PrintError::Spool(format!("the print spooler is not available: {e}")))?;
+    let stdin = child.stdin.take();
+    // Feed the job on its own thread while collecting the output, so neither side can fill a
+    // pipe and wait on the other. Dropping `stdin` at the end closes it: the end of the job.
+    let fed_and_out = std::thread::scope(|s| {
+        let feeder = std::thread::Builder::new().name("print job".into()).spawn_scoped(s, move || match stdin {
+            Some(mut stdin) => stdin.write_all(pdf),
+            None => Err(std::io::Error::other("no pipe to the spooler")),
+        });
+        match feeder {
+            Ok(feeder) => {
+                let out = child.wait_with_output();
+                Ok((feeder.join(), out))
+            }
+            Err(e) => {
+                // No thread to feed it: stop `lp` rather than leave it with an empty job.
+                let _ = child.kill();
+                let _ = child.wait();
+                Err(PrintError::Spool(format!("the print job could not be sent to the spooler: {e}")))
+            }
+        }
+    });
+    let (fed, out) = fed_and_out?;
+    let out = out.map_err(|e| PrintError::Spool(format!("the print spooler is not available: {e}")))?;
+    if !out.status.success() {
+        // `lp` may refuse before reading the job (an unknown printer); its message says why,
+        // and the broken pipe that leaves behind does not.
+        let err = String::from_utf8_lossy(&out.stderr).trim().to_string();
+        return Err(PrintError::Spool(if err.is_empty() { "the print job was refused".into() } else { err }));
+    }
+    match fed {
+        Ok(Ok(())) => Ok(job_message(&out.stdout)),
+        Ok(Err(e)) => Err(PrintError::Spool(format!("the print job could not be sent to the spooler: {e}"))),
+        Err(_) => Err(PrintError::Spool("the print job could not be sent to the spooler".into())),
+    }
+}
+
+/// `lp`'s reply ("request id is Office-12"), without the "(0 file(s))" CUPS adds when the job came
+/// on stdin: it reads as if nothing was sent.
+#[cfg(any(test, all(unix, not(target_arch = "wasm32"))))]
+pub(crate) fn job_message(stdout: &[u8]) -> String {
+    let s = String::from_utf8_lossy(stdout);
+    let s = s.trim();
+    s.strip_suffix("(0 file(s))").map_or(s, str::trim_end).to_string()
+}

crates/print/src/tests.rs

@@ -193,10 +193,10 @@ fn spooler_arguments_and_printer_list() {
     let job =
         Job { printer: Some("Office_Laser".into()), copies: 3, collate: false, duplex: Duplex::LongEdge, grayscale: true, title: "memo.pdf".into() };
     assert_eq!(
-        lp_args(&job, "/tmp/x.pdf").join(" "),
-        "-d Office_Laser -n 3 -t memo.pdf -o collate=false -o sides=two-sided-long-edge -o print-color-mode=monochrome -o fit-to-page=false -- /tmp/x.pdf"
+        lp_args(&job).join(" "),
+        "-d Office_Laser -n 3 -t memo.pdf -o collate=false -o sides=two-sided-long-edge -o print-color-mode=monochrome -o fit-to-page=false"
     );
-    assert_eq!(lp_args(&Job::default(), "f.pdf")[0], "-n", "no -d: the default printer");
+    assert_eq!(lp_args(&Job::default())[0], "-n", "no -d: the default printer");
 }
 
 #[test]
@@ -275,3 +275,87 @@ fn multiple_rejects_hostile_grids_without_panicking() {
         assert!(matches!(layout(&[size], &settings(vec![0], cut_stack(2, 2))), Err(PrintError::Invalid(_))));
     }
 }
+
+/// Set on the child process when a test runs this test binary as a stand-in for `lp`.
+const STAND_IN_LP: &str = "PDFCRAFT_STAND_IN_LP";
+
+/// Not a test of its own: [`stand_in_lp`] re-runs this binary with only this test selected, and it
+/// then plays `lp`. It records its arguments and stdin in the folder named by [`STAND_IN_LP`], or,
+/// when the folder is `refuse`, exits without reading stdin, the way `lp` refuses an unknown
+/// printer.
+#[test]
+fn stand_in_for_lp() {
+    let Some(record) = std::env::var_os(STAND_IN_LP) else { return };
+    if record == "refuse" {
+        eprintln!("lp: The printer or class does not exist.");
+        std::process::exit(1);
+    }
+    let record = std::path::PathBuf::from(record);
+    let args: Vec<String> = std::env::args().skip_while(|a| a != "--").skip(1).collect();
+    let mut stdin = Vec::new();
+    std::io::Read::read_to_end(&mut std::io::stdin(), &mut stdin).unwrap();
+    std::fs::write(record.join("args"), args.join(" ")).unwrap();
+    std::fs::write(record.join("stdin"), stdin).unwrap();
+}
+
+/// A spooler command that runs [`stand_in_for_lp`] with `record` (see there).
+fn stand_in_lp(record: &std::ffi::OsStr) -> std::process::Command {
+    let mut c = std::process::Command::new(std::env::current_exe().unwrap());
+    c.args(["--exact", "tests::stand_in_for_lp", "--nocapture", "--"]).env(STAND_IN_LP, record);
+    c
+}
+
+#[test]
+fn print_jobs_reach_lp_on_stdin_never_through_a_shared_temp_folder() {
+    // The job used to be written to `<temp>/pdfcraft-print-<pid>/job-<time>.pdf` and handed to `lp`
+    // by name. In a shared /tmp another local user can predict that folder, create it first (or
+    // plant a symlink there) and so read every printed document or swap the file before `lp`
+    // reads it. Piping the job to `lp` leaves nothing on disk.
+    static N: std::sync::atomic::AtomicU32 = std::sync::atomic::AtomicU32::new(0);
+    let record =
+        std::env::temp_dir().join(format!("pdfcraft-print-test-{}-{}", std::process::id(), N.fetch_add(1, std::sync::atomic::Ordering::Relaxed)));
+    std::fs::create_dir(&record).unwrap();
+    // Bigger than any pipe buffer, so the spooler must read while the job is still being written.
+    let pdf: Vec<u8> = b"%PDF-1.7 synthetic print job\n".iter().copied().cycle().take(3 << 20).collect();
+    let job = Job { title: "memo.pdf".into(), ..Job::default() };
+    let sent = spool::submit_via(stand_in_lp(record.as_os_str()), &pdf, &job);
+    let args = std::fs::read_to_string(record.join("args"));
+    let stdin = std::fs::read(record.join("stdin"));
+    let _ = std::fs::remove_dir_all(&record);
+    assert!(sent.is_ok(), "{sent:?}");
+    assert_eq!(
+        args.unwrap(),
+        "-n 1 -t memo.pdf -o collate=true -o sides=one-sided -o fit-to-page=false",
+        "no file argument: lp reads the job from stdin"
+    );
+    assert!(stdin.unwrap() == pdf, "lp receives the whole job on stdin");
+    let predictable = std::env::temp_dir().join(format!("pdfcraft-print-{}", std::process::id()));
+    assert!(!predictable.exists(), "nothing is created at {predictable:?}, a name other local users can predict");
+}
+
+#[test]
+fn a_refused_print_job_reports_the_spoolers_message() {
+    // `lp` refuses an unknown printer without reading the job; the user sees why, not a broken pipe.
+    let pdf = vec![b'%'; 3 << 20];
+    let err = spool::submit_via(stand_in_lp("refuse".as_ref()), &pdf, &Job::default());
+    assert_eq!(err, Err(PrintError::Spool("lp: The printer or class does not exist.".into())));
+}
+
+#[test]
+fn the_spoolers_reply_drops_the_file_count_of_a_job_sent_on_stdin() {
+    assert_eq!(
+        spool::job_message(
+            b"request id is Office_Laser-12 (0 file(s))
+"
+        ),
+        "request id is Office_Laser-12"
+    );
+    assert_eq!(
+        spool::job_message(
+            b"request id is Office_Laser-13 (1 file(s))
+"
+        ),
+        "request id is Office_Laser-13 (1 file(s))"
+    );
+    assert_eq!(spool::job_message(b""), "");
+}

crates/ui-egui/src/sign_ui.rs

@@ -162,6 +162,40 @@ fn id_dir() -> Option<PathBuf> {
     crate::recovery::RecoveryStore::default_dir().and_then(|d| d.parent().map(|p| p.join("Digital IDs")))
 }
 
+/// Save a new digital ID as `<stem>.p12`, or `<stem> 2.p12`, … when the name is taken. The file
+/// is always created fresh, never opened through a file or link already at the name (checking
+/// first and then writing would let one be planted in between), and on Unix only its owner can
+/// read it: it holds the private key.
+fn save_new_id_file(dir: &std::path::Path, stem: &str, p12: &[u8]) -> std::io::Result<PathBuf> {
+    use std::io::{ErrorKind, Write};
+    for i in 1..=10_000u32 {
+        let path = dir.join(if i == 1 { format!("{stem}.p12") } else { format!("{stem} {i}.p12") });
+        let mut opts = std::fs::OpenOptions::new();
+        opts.write(true).create_new(true);
+        #[cfg(unix)]
+        std::os::unix::fs::OpenOptionsExt::mode(&mut opts, 0o600);
+        let file = match opts.open(&path) {
+            Ok(f) => f,
+            Err(e) if e.kind() == ErrorKind::AlreadyExists => continue,
+            // Windows refuses `create_new` on a folder with "access denied"; the name is taken all
+            // the same. A folder we can't write to, with nothing at the name, still fails here.
+            Err(e) if e.kind() == ErrorKind::PermissionDenied && path.symlink_metadata().is_ok() => continue,
+            Err(e) => return Err(e),
+        };
+        // The block closes the file before a failed one is removed (Windows can't remove an open file).
+        let written = {
+            let mut file = file;
+            file.write_all(p12).and_then(|()| file.sync_all())
+        };
+        if let Err(e) = written {
+            let _ = std::fs::remove_file(&path);
+            return Err(e);
+        }
+        return Ok(path);
+    }
+    Err(std::io::Error::new(ErrorKind::AlreadyExists, "no free file name for the digital ID"))
+}
+
 /// A Keychain identity by its `keychain:` reference.
 fn keychain_id(reference: &str) -> Result<DigitalId, String> {
     #[cfg(target_os = "macos")]
@@ -266,11 +300,7 @@ impl PdfCraftApp {
         .ok_or("There is no folder to save the digital ID in.")?;
         std::fs::create_dir_all(&dir).map_err(|e| e.to_string())?;
         let stem: String = d.name.trim().chars().map(|c| if c.is_alphanumeric() || c == ' ' || c == '-' { c } else { '_' }).collect();
-        let path = (1..=u64::MAX)
-            .map(|i| dir.join(if i == 1 { format!("{stem}.p12") } else { format!("{stem} {i}.p12") }))
-            .find(|p| !p.exists())
-            .ok_or("no free file name for the digital ID")?;
-        std::fs::write(&path, &p12).map_err(|e| e.to_string())?;
+        let path = save_new_id_file(&dir, &stem, &p12).map_err(|e| e.to_string())?;
         Ok(self.add_digital_id(&path.to_string_lossy(), &cert))
     }
 
@@ -1079,3 +1109,86 @@ pub(crate) fn cert_viewer(ui: &mut egui::Ui, v: &mut CertViewer, trusted: &[Cert
 pub fn certificate_pem(c: &Certificate) -> String {
     sign::x509::to_pem(c)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// A fresh folder per call: tests run in parallel.
+    fn scratch() -> PathBuf {
+        static N: std::sync::atomic::AtomicU32 = std::sync::atomic::AtomicU32::new(0);
+        let n = N.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
+        let d = std::env::temp_dir().join(format!("pdfcraft-sign-ui-{}-{n}", std::process::id()));
+        let _ = std::fs::remove_dir_all(&d);
+        std::fs::create_dir_all(&d).unwrap();
+        d
+    }
+
+    /// Create a digital ID named "Grace Hopper" in `dir`, as Configure New Digital ID ▸ Create
+    /// does. Returns where it was saved.
+    fn create_in(dir: &std::path::Path) -> Result<PathBuf, String> {
+        let mut app = PdfCraftApp::new();
+        app.export_dir_override = Some(dir.to_string_lossy().into_owned());
+        let mut draft = SignDraft::new(0, None, None, None, 0);
+        // P-256 keeps the test fast.
+        let key = KEY_ALGORITHMS.iter().position(|k| k.1 == "p256").unwrap();
+        draft.new_id =
+            NewIdDraft { name: "Grace Hopper".into(), key, password: "secret1".into(), confirm: "secret1".into(), ..NewIdDraft::default() };
+        app.sign_draft = Some(draft);
+        let i = app.create_digital_id()?;
+        Ok(PathBuf::from(&app.digital_ids[i].path))
+    }
+
+    #[test]
+    fn a_new_digital_id_skips_names_already_taken_without_writing_through_them() {
+        let dir = scratch();
+        // Another file's hard link at the first name, and a folder at the second (Windows refuses
+        // `create_new` on a folder with "access denied" rather than "already exists").
+        std::fs::write(dir.join("victim.txt"), "keep me").unwrap();
+        std::fs::hard_link(dir.join("victim.txt"), dir.join("Grace Hopper.p12")).unwrap();
+        std::fs::create_dir(dir.join("Grace Hopper 2.p12")).unwrap();
+        let saved = create_in(&dir).unwrap();
+        assert_eq!(saved, dir.join("Grace Hopper 3.p12"));
+        assert_eq!(std::fs::read_to_string(dir.join("victim.txt")).unwrap(), "keep me");
+        assert!(sign::pkcs12::open(&std::fs::read(&saved).unwrap(), "secret1").is_ok());
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
+    #[test]
+    fn a_new_digital_id_is_not_written_through_a_dangling_link() {
+        // `exists()` follows links, so a link to a file that doesn't exist yet looked free, and
+        // `fs::write` then created the private key wherever the link pointed.
+        let dir = scratch();
+        let (link, target) = (dir.join("Grace Hopper.p12"), dir.join("elsewhere.p12"));
+        #[cfg(unix)]
+        let made = std::os::unix::fs::symlink(&target, &link);
+        #[cfg(windows)]
+        let made = std::os::windows::fs::symlink_file(&target, &link);
+        #[cfg(not(any(unix, windows)))]
+        let made: std::io::Result<()> = {
+            let _ = (&target, &link);
+            Err(std::io::ErrorKind::Unsupported.into())
+        };
+        if let Err(e) = made {
+            // Windows needs Developer Mode (or admin) for symlinks.
+            eprintln!("skipped: can't create a symlink here: {e}");
+            let _ = std::fs::remove_dir_all(&dir);
+            return;
+        }
+        let saved = create_in(&dir).unwrap();
+        assert!(!target.exists(), "nothing written through the link");
+        assert_eq!(saved, dir.join("Grace Hopper 2.p12"));
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn a_new_digital_id_is_readable_by_its_owner_only() {
+        use std::os::unix::fs::PermissionsExt;
+        let dir = scratch();
+        let saved = create_in(&dir).unwrap();
+        let mode = std::fs::metadata(&saved).unwrap().permissions().mode();
+        assert_eq!(mode & 0o077, 0, "the private key is not readable by others: {mode:o}");
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+}