From 73f360c2603cf1416c55d4b4e74dea945d7b340b Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Wed, 28 Oct 2026 19:06:49 +0000 Subject: [PATCH 1/3] fix(cron): harden usercron writeback temp file (O_EXCL, random name) The writeback used a fixed temp name (cronjob.toml.tmp) written with O_CREAT|O_TRUNC, so anyone able to write the directory could pre-create the file or plant a symlink and capture or redirect the write. Create the temp file with tempfile::Builder (random name, O_EXCL) in the same directory, keep the original file mode, fsync, then persist over the target. Adds tests for a planted symlink, a planted regular file, and mode preservation. Closes #1577 --- crates/openab-core/src/cron.rs | 110 +++++++++++++++++++++++++++++++-- 1 file changed, 106 insertions(+), 4 deletions(-) diff --git a/crates/openab-core/src/cron.rs b/crates/openab-core/src/cron.rs index ea6bec58a..e9dd0581e 100644 --- a/crates/openab-core/src/cron.rs +++ b/crates/openab-core/src/cron.rs @@ -951,10 +951,39 @@ fn update_usercron_job( anyhow::bail!("usercron job id {:?} not found", id); } - // Atomic write: write to temp file then rename to avoid corruption on crash. - let tmp = path.with_extension("toml.tmp"); - std::fs::write(&tmp, doc.to_string())?; - std::fs::rename(&tmp, path)?; + write_atomic(path, doc.to_string().as_bytes()) +} + +/// Replace `path` with `contents` atomically. +/// +/// The temp file gets a random name and is created with `O_EXCL` +/// (`tempfile::Builder`), so a file or symlink planted next to `path` can't +/// redirect or capture the write. The original file's permissions are kept, +/// because the new file would otherwise be created with mode 0600. If any step +/// fails, the temp file is removed when it is dropped. +fn write_atomic(path: &Path, contents: &[u8]) -> anyhow::Result<()> { + use std::io::Write; + + let dir = match path.parent() { + Some(parent) if !parent.as_os_str().is_empty() => parent, + _ => Path::new("."), + }; + let prefix = format!( + ".{}.", + path.file_name() + .map(|name| name.to_string_lossy().into_owned()) + .unwrap_or_else(|| "usercron".to_string()) + ); + let mut tmp = tempfile::Builder::new() + .prefix(&prefix) + .suffix(".tmp") + .tempfile_in(dir)?; + tmp.write_all(contents)?; + if let Ok(meta) = std::fs::metadata(path) { + tmp.as_file().set_permissions(meta.permissions())?; + } + tmp.as_file().sync_all()?; + tmp.persist(path).map_err(|e| e.error)?; Ok(()) } @@ -1528,6 +1557,79 @@ message = "b" assert_eq!(job_b["thread_id"].as_str(), Some("thread-456")); } + const WRITEBACK_FIXTURE: &str = r#" +[[jobs]] +id = "goal" +enabled = true +schedule = "* * * * *" +channel = "123" +message = "m" +"#; + + #[cfg(unix)] + #[test] + fn test_update_usercron_job_planted_tmp_symlink_is_not_followed() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("cronjob.toml"); + std::fs::write(&path, WRITEBACK_FIXTURE).unwrap(); + let victim = dir.path().join("victim"); + std::fs::write(&victim, "original").unwrap(); + let planted = dir.path().join("cronjob.toml.tmp"); + std::os::unix::fs::symlink(&victim, &planted).unwrap(); + + update_usercron_job(&path, "goal", Some(false), None).unwrap(); + + assert_eq!(std::fs::read_to_string(&victim).unwrap(), "original"); + assert!(!std::fs::symlink_metadata(&path) + .unwrap() + .file_type() + .is_symlink()); + let doc = std::fs::read_to_string(&path) + .unwrap() + .parse::() + .unwrap(); + assert_eq!(doc["jobs"][0]["enabled"].as_bool(), Some(false)); + assert!(std::fs::symlink_metadata(&planted) + .unwrap() + .file_type() + .is_symlink()); + } + + #[test] + fn test_update_usercron_job_planted_tmp_file_is_untouched_and_no_temp_left() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("cronjob.toml"); + std::fs::write(&path, WRITEBACK_FIXTURE).unwrap(); + let planted = dir.path().join("cronjob.toml.tmp"); + std::fs::write(&planted, "planted").unwrap(); + + update_usercron_job(&path, "goal", None, Some("thread-1")).unwrap(); + + assert_eq!(std::fs::read_to_string(&planted).unwrap(), "planted"); + let mut names: Vec = std::fs::read_dir(dir.path()) + .unwrap() + .map(|e| e.unwrap().file_name().to_string_lossy().into_owned()) + .collect(); + names.sort(); + assert_eq!(names, vec!["cronjob.toml", "cronjob.toml.tmp"]); + } + + #[cfg(unix)] + #[test] + fn test_update_usercron_job_preserves_file_mode() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("cronjob.toml"); + std::fs::write(&path, WRITEBACK_FIXTURE).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o640)).unwrap(); + + update_usercron_job(&path, "goal", Some(false), None).unwrap(); + + let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o640); + } + #[test] fn update_usercron_job_errors_for_missing_id() { let dir = tempfile::tempdir().unwrap(); From 4061f1398e21dbaa9226ba7c3ce1cd71c8da2a9c Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Wed, 28 Oct 2026 19:34:17 +0000 Subject: [PATCH 2/3] fix(cron): fsync parent dir after usercron writeback, pin symlink behavior Address review findings on #1584: - write_atomic: after persist, fsync the parent directory best-effort (unix only), matching crates/openab-mcp/src/auth.rs. - Add test_update_usercron_job_symlinked_destination_is_replaced_and_target_untouched (unix): when cronjob.toml is a symlink, its target is unchanged and the path becomes a regular file with the updated TOML. - Doc comment: state the symlinked-destination behavior and the non-goals (no locking, mode only, defense-in-depth per docs/cronjob.md). Checked: git diff --check clean; new lines ASCII-only; new code hand-formatted (cargo fmt not run, to keep pre-existing rustfmt diffs in cron.rs untouched). Not built or tested locally (low-resource host); build, clippy, and tests are verified by GitHub CI on this PR. --- crates/openab-core/src/cron.rs | 42 +++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/crates/openab-core/src/cron.rs b/crates/openab-core/src/cron.rs index e9dd0581e..a75a33c08 100644 --- a/crates/openab-core/src/cron.rs +++ b/crates/openab-core/src/cron.rs @@ -960,7 +960,18 @@ fn update_usercron_job( /// (`tempfile::Builder`), so a file or symlink planted next to `path` can't /// redirect or capture the write. The original file's permissions are kept, /// because the new file would otherwise be created with mode 0600. If any step -/// fails, the temp file is removed when it is dropped. +/// fails, the temp file is removed when it is dropped. After the rename the +/// parent directory is fsynced (best-effort, unix only) so the rename is +/// durable. If `path` is itself a symlink, the link is replaced by a regular +/// file and its target is left untouched. +/// +/// Non-goals: +/// - No locking: concurrent `update_usercron_job` calls are still +/// last-writer-wins (read-modify-write race), unchanged from before. +/// - Only the mode is copied; owner, group, ACLs, and xattrs are not. +/// - This is defense-in-depth. `docs/cronjob.md` already requires that the +/// usercron directory is not writable by the agent, and a writer of that +/// directory can replace `cronjob.toml` directly. fn write_atomic(path: &Path, contents: &[u8]) -> anyhow::Result<()> { use std::io::Write; @@ -984,6 +995,12 @@ fn write_atomic(path: &Path, contents: &[u8]) -> anyhow::Result<()> { } tmp.as_file().sync_all()?; tmp.persist(path).map_err(|e| e.error)?; + // fsync the parent dir so the rename itself is durable (best-effort, + // matching openab-mcp's auth.rs). + #[cfg(unix)] + if let Ok(dir_handle) = std::fs::File::open(dir) { + let _ = dir_handle.sync_all(); + } Ok(()) } @@ -1614,6 +1631,29 @@ message = "m" assert_eq!(names, vec!["cronjob.toml", "cronjob.toml.tmp"]); } + #[cfg(unix)] + #[test] + fn test_update_usercron_job_symlinked_destination_is_replaced_and_target_untouched() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("real.toml"); + std::fs::write(&target, WRITEBACK_FIXTURE).unwrap(); + let path = dir.path().join("cronjob.toml"); + std::os::unix::fs::symlink(&target, &path).unwrap(); + + update_usercron_job(&path, "goal", Some(false), None).unwrap(); + + assert_eq!(std::fs::read_to_string(&target).unwrap(), WRITEBACK_FIXTURE); + assert!(std::fs::symlink_metadata(&path) + .unwrap() + .file_type() + .is_file()); + let doc = std::fs::read_to_string(&path) + .unwrap() + .parse::() + .unwrap(); + assert_eq!(doc["jobs"][0]["enabled"].as_bool(), Some(false)); + } + #[cfg(unix)] #[test] fn test_update_usercron_job_preserves_file_mode() { From ced815746bf3c184ca2adeb59d1630cedf5ccb70 Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Thu, 29 Oct 2026 08:53:44 +0000 Subject: [PATCH 3/3] fix(review): propagate metadata error in usercron write_atomic Silently ignoring fs::metadata errors let a writeback succeed with tempfile's default 0600 mode, breaking the mode-preservation guarantee. The only caller has just read the file, so any stat error (including NotFound from a concurrent delete) is now returned. --- crates/openab-core/src/cron.rs | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/crates/openab-core/src/cron.rs b/crates/openab-core/src/cron.rs index a75a33c08..d67ba2769 100644 --- a/crates/openab-core/src/cron.rs +++ b/crates/openab-core/src/cron.rs @@ -958,8 +958,9 @@ fn update_usercron_job( /// /// The temp file gets a random name and is created with `O_EXCL` /// (`tempfile::Builder`), so a file or symlink planted next to `path` can't -/// redirect or capture the write. The original file's permissions are kept, -/// because the new file would otherwise be created with mode 0600. If any step +/// redirect or capture the write. `path` must already exist: its permissions +/// are copied to the new file, which would otherwise be created with mode +/// 0600, and a failure to read them is returned as an error. If any step /// fails, the temp file is removed when it is dropped. After the rename the /// parent directory is fsynced (best-effort, unix only) so the rename is /// durable. If `path` is itself a symlink, the link is replaced by a regular @@ -973,6 +974,7 @@ fn update_usercron_job( /// usercron directory is not writable by the agent, and a writer of that /// directory can replace `cronjob.toml` directly. fn write_atomic(path: &Path, contents: &[u8]) -> anyhow::Result<()> { + use anyhow::Context as _; use std::io::Write; let dir = match path.parent() { @@ -990,9 +992,12 @@ fn write_atomic(path: &Path, contents: &[u8]) -> anyhow::Result<()> { .suffix(".tmp") .tempfile_in(dir)?; tmp.write_all(contents)?; - if let Ok(meta) = std::fs::metadata(path) { - tmp.as_file().set_permissions(meta.permissions())?; - } + // `path` must already exist (the caller just read it). Propagate any + // metadata error instead of silently persisting with tempfile's 0600 mode. + let perms = std::fs::metadata(path) + .with_context(|| format!("failed to stat {}", path.display()))? + .permissions(); + tmp.as_file().set_permissions(perms)?; tmp.as_file().sync_all()?; tmp.persist(path).map_err(|e| e.error)?; // fsync the parent dir so the rename itself is durable (best-effort, @@ -1670,6 +1675,18 @@ message = "m" assert_eq!(mode, 0o640); } + #[test] + fn test_write_atomic_errors_when_destination_missing_and_leaves_no_temp() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("cronjob.toml"); + + let err = write_atomic(&path, b"x").unwrap_err(); + + assert!(err.to_string().contains("failed to stat"), "{err:#}"); + assert!(!path.exists()); + assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 0); + } + #[test] fn update_usercron_job_errors_for_missing_id() { let dir = tempfile::tempdir().unwrap();