[chaodu-agent]

What problem does this solve?

update_usercron_job writes the updated usercron TOML to a fixed-name temp file (cronjob.toml.tmp) with std::fs::write (O_CREAT|O_TRUNC, no O_EXCL/O_NOFOLLOW) and renames it over the real file. Anyone who can write the containing directory can pre-create that name or plant a symlink there, and either keep ownership of the resulting cronjob.toml or redirect the TOML write to any file OpenAB can write. Today the only defence is the docs advice from #1563 that the directory must not be writable by others. This PR adds the code-side hardening.

Closes #1577

Discord Discussion URL: Not applicable - maintainer-filed security hardening issue (#1577).

Review Contract

Goal

Make usercron writeback safe against a pre-created file or symlink at a predictable temp path in the usercron directory.

Non-goals

• No change to how the usercron file is read, parsed, or hot-reloaded.
• No defence against an attacker who can replace cronjob.toml itself; that requires write access to the directory, which the docs already say must not be granted.
• No change to the docs added in docs(cron): document cron autonomy and permission scope #1562 / docs(cron): drop unsafe shared-writable directory advice #1563.

Accepted Residual Risks

• A writer to the directory can still delete or replace cronjob.toml between writebacks. This is the same as before and is covered by the directory-permission guidance.
• If the process crashes between creating and persisting the temp file, a randomly named .cronjob.toml.*.tmp file may be left behind. It is never reused and is harmless.

Acceptance Criteria

• The temp file is created with a random name and O_EXCL in the same directory as the target (tempfile::Builder::tempfile_in).
• A symlink planted at cronjob.toml.tmp is not followed: its target is unchanged and cronjob.toml is a regular file after writeback.
• A regular file planted at cronjob.toml.tmp is left untouched, and no other temp file remains after a successful writeback.
• If cronjob.toml is itself a symlink, writeback replaces the link with a regular file and leaves the link target unchanged (unix).
• After the rename, the parent directory is fsynced best-effort (unix).
• The original file mode is preserved across writeback (unix).
• Existing update_usercron_job tests still pass; cargo clippy --workspace -- -D warnings passes.

Follow-ups

Other writers in the repo still have the same class of temp-file issue and are left for a separate change (not touched here):

• crates/openab-core/src/acp/pool.rs:383 and :399: fixed json.tmp temp name written with std::fs::write and then renamed (the same pattern this PR fixes in cron.rs).
• crates/openab-core/src/acp_mcp.rs:105: pid+sequence temp name opened with create(true).truncate(true) and no O_EXCL, so a symlink planted at the guessable name is followed.
• crates/openab-mcp/src/auth.rs is already hardened (create_new, mode 0600, parent-dir fsync) and is the model for the fixes above.
• A shared atomic-write helper for all of these is deferred to that follow-up.

The directory-permission guidance from #1563 remains the complementary control.

At a Glance

before: write(cronjob.toml.tmp, O_CREAT|O_TRUNC) -> rename -> cronjob.toml
after:  tempfile_in(dir, ".cronjob.toml.XXXXXX.tmp", O_EXCL)
          -> write -> chmod(orig mode) -> fsync -> persist -> cronjob.toml

Prior Art & Industry Research

Not applicable - small security bug fix using the standard tempfile crate pattern for atomic replacement. tempfile is already a regular dependency of openab-core.

[chaodu-agent]

Note

LGTM ✅ - All round-1 findings and the one new round-2 finding are fixed at ced81574; CI check (clippy + tests) passes. No open findings.

What This PR Does

update_usercron_job used to write cronjob.toml through a fixed temp path (cronjob.toml.tmp) with std::fs::write, which follows a symlink planted at that path and truncates its target. This PR replaces that with a write_atomic helper that uses a randomly named O_EXCL temp file in the same directory, so a planted file or symlink cannot capture or redirect the write. Closes #1577.

How It Works

• tempfile::Builder::tempfile_in(dir) creates .cronjob.toml.XXXXXX.tmp with O_EXCL next to the target.
• The contents are written, the destination's mode is copied onto the temp file, the file is fsynced, then persist renames it over cronjob.toml.
• The parent directory is fsynced after the rename (best-effort, unix), matching crates/openab-mcp/src/auth.rs.
• If cronjob.toml is itself a symlink, the link is replaced by a regular file and its target is left untouched (pinned by a test, required by the Review Contract).
• New in this round: a failure to stat the destination is now returned as an error instead of being ignored. On any error the dropped NamedTempFile unlinks the temp file, and the original file stays intact.
• tempfile 3.27.0 is already a regular dependency of openab-core; no new dependency.

Findings

#	Severity	Finding	Location
1	🟢	Metadata errors are now propagated, so a writeback can no longer succeed while silently dropping the original mode to 0600 (fixed in ced81574)	crates/openab-core/src/cron.rs:995-1000
2	🟢	Every round-1 finding is resolved: parent-dir fsync, destination-symlink test and doc, non-goals in the doc comment, and an accurate Follow-ups list	crates/openab-core/src/cron.rs:957-1010, PR description
3	🟢	Focused regression tests cover each acceptance criterion, including the new missing-destination error path	crates/openab-core/src/cron.rs:1582-1689

Finding Details

🟢 F1: Metadata error is no longer swallowed (round-2 finding, resolved)

At 4061f139, the mode copy was if let Ok(meta) = std::fs::metadata(path) { ... }. Any stat error was ignored and the file was persisted with tempfile's default 0600 mode while the writeback reported success. That broke the "original file mode is preserved" acceptance criterion without any signal.

write_atomic has one caller, update_usercron_job, which has just read path successfully. Any later stat failure, including NotFound from a concurrent delete, is therefore unexpected. ced81574 returns it as failed to stat <path>, updates the doc comment to say path must already exist, and adds test_write_atomic_errors_when_destination_missing_and_leaves_no_temp, which asserts the error, that no destination is created, and that no temp file is left behind.

A concurrent delete or replace by a directory writer remains an accepted residual risk in the Review Contract, so this was rated Important, not Critical.

Note: the round-1 summary described the old behavior ("a metadata failure leaves the stricter 0600 mode") as fail-closed. That was correct for confidentiality but not for the mode-preservation criterion. This round supersedes that note.

🟢 F2: Round-1 findings resolved in 4061f139

Round-1 finding	Resolution
R1-F1: rename not durable, parent directory not fsynced	✅ Best-effort parent-dir sync_all() after persist on unix, matching auth.rs
R1-F2: destination-is-a-symlink behavior untested and undocumented	✅ Doc comment states the link is replaced and its target untouched; test_update_usercron_job_symlinked_destination_is_replaced_and_target_untouched pins it
R1-F3: doc comment lacked guarantees and non-goals	✅ Non-goals listed: no locking (last-writer-wins), mode only (no owner/group/ACL/xattr), defense-in-depth relative to docs/cronjob.md
R1-F4: ### Follow-ups: None was inaccurate	✅ PR body now lists acp/pool.rs:383/399, acp_mcp.rs:105, notes auth.rs as the model, and defers a shared helper

🟢 F3: Test coverage

Tests cover a planted tmp symlink (target unchanged, destination is a regular file), a planted tmp regular file (untouched, no stray temp), a symlinked destination, mode 0640 preservation, and the new missing-destination error path. Unix-only tests are #[cfg(unix)]-gated.

Considered and Not Raised

• Symlinked cronjob.toml becomes a regular file - required by the Review Contract acceptance criteria; it is the safe direction (the link target is never written).
• Read-modify-write race between concurrent writebacks - an explicit non-goal, unchanged from main, and documented in the doc comment.
• Other temp-file writers (acp/pool.rs, acp_mcp.rs) - out of scope and tracked in the PR's Follow-ups.

Validation

• Head ced815746bf3c184ca2adeb59d1630cedf5ccb70, base main, merge-base 02a8b24378e1e2dc1d5e2861525d21a05693d170, 3 commits, 1 file changed (crates/openab-core/src/cron.rs).
• CI on this SHA: check passes (cargo clippy, cargo clippy (unified), cargo test, and the acp test steps); Review Contract, changes, conformance, validate, and validate-packaged-pins pass; operator is skipped. Docker smoke tests and build-builder were still running when this summary was posted. All of them passed on the previous head 4061f139, and the only change since is the error-handling fix and its test in cron.rs.
• There are no inline review threads on this PR.

Baseline Check

• PR opened: 2026-10-28
• Main already has: atomic temp-then-rename writeback for cronjob.toml, but with a fixed, predictable temp name written through std::fs::write (follows symlinks); directory-permission guidance in docs/cronjob.md (docs(cron): drop unsafe shared-writable directory advice #1563)
• Net-new value: unpredictable O_EXCL temp file, mode preservation with error propagation, file and parent-dir fsync, pinned symlink behavior, and regression tests

What's Good (🟢)

• Uses an existing dependency and the standard tempfile pattern
• Fails closed at every step; no fallback to a non-atomic write
• Doc comment states guarantees and non-goals explicitly
• The Review Contract maps one-to-one onto tests

5. Three Reasons We Might Not Need This PR

1. The threat model already excludes it - The hole only matters if the agent can write the directory holding cronjob.toml, which docs/cronjob.md already forbids; such a writer can replace cronjob.toml directly.
2. It narrows one vector, not a boundary - Directory write access allows many equivalent attacks (rename, squat, fill the disk), so this is defense-in-depth.
3. A one-off helper can drift - pool.rs and acp_mcp.rs keep weaker patterns until the follow-up lands a shared helper.

Counterpoint: the change is small, strictly better than main, matches auth.rs, and reason 3 is tracked as a follow-up rather than a reason to reject.