fix(api): make secret writes durable by houko · Pull Request #6944 · librefang/librefang

houko · 2026-08-12T08:32:28Z

Summary

propagate secrets.env staging-file sync failures instead of discarding them
remove the 0600 secret-bearing staging file when writing or renaming fails
fsync the parent directory after rename on Unix so the directory entry is durable
retain the existing single-line validation and 0600 permission behavior
assert successful writes leave no staging files behind

Verification

cargo test -p librefang-api 'routes::secrets_env::tests::' --lib (3 passed)
cargo test -p librefang-api --test secrets_env_test (4 passed)
cargo clippy -p librefang-api --lib -- -D warnings
cargo fmt --check
git diff --check

Out of scope

other secret/config writers outside routes/secrets_env.rs
changing dotenv parsing or accepted secret value syntax

PR #6944 landed the fsync/atomic-rename durability fix for secrets.env writes without a changelog.d/ fragment.

houko

Automated daily review of #6944. Durability logic (fsync-before-rename, fsync-parent-dir-after-rename, tmp cleanup on both write and rename failure) looks correct and matches the standard POSIX atomic-durable-write pattern. Pushed one mechanical fix (missing changelog.d/ fragment); left two inline notes on ambiguous points for maintainer judgment — no blocking issues found.

Generated by Claude Code

houko · 2026-08-12T08:39:55Z

+        .and_then(|dir| dir.sync_all())
+        .map_err(|e| format!("sync parent directory {parent:?}: {e}"))?;
+
    Ok(())


By the time this fsync on the parent directory can fail, fs::rename has already succeeded — the new secret value is already live at path.
Returning Err here makes that indistinguishable from a write that never took effect: the channels.rs caller maps any Err from upsert_secret to a 500 via ApiErrorResponse::internal_scrub, so an operator who hits this narrow failure mode sees "save failed" and may retype/resubmit a secret that is, in fact, already persisted (just not confirmed crash-durable).

Worth considering whether this specific failure should log a WARN and still return Ok(()) (the write itself succeeded; only the durability guarantee for a crash in the following instant is unconfirmed), or at least whether the error message should say "value was written but durability could not be confirmed" rather than reading like an outright write failure. Not blocking — just flagging since the whole point of this PR is making these failure semantics correct, and this is the one path where "succeeded" and "returned Err" now diverge.

Generated by Claude Code

houko · 2026-08-12T08:40:32Z

Not part of this diff's changed lines, so flagging as a general file comment rather than a line comment: at lines 100-104 (post-edit numbering), the tmp file is created via OpenOptions::new().write(true).create(true).truncate(true).open(&tmp) and only chmod'd to 0600 after open() returns.
Between those two calls the file briefly exists with default (umask-dependent, commonly 0644) permissions — a secret-bearing file momentarily world/group-readable.
std::os::unix::fs::OpenOptionsExt::mode(0o600) on the OpenOptions before .open() would set the permission bits atomically at creation and close that window entirely.
This is pre-existing (unchanged by this PR) and outside the PR's stated diff/out-of-scope list, but since this PR is specifically hardening the durability/security properties of this exact function, it seemed worth surfacing rather than staying silent about it.

Generated by Claude Code

houko

Reviewed against LibreFang's CLAUDE.md engineering rules (changelog format, build/verify workflow, secrets-handling caution). Changelog fragment, commit hygiene, and prose formatting all check out. Left three inline comments on the staging-file sequence itself rather than editing it — all are file-permission / durable-write-semantics judgment calls on secret-handling code, which is exactly the kind of change this repo's rules ask a human to weigh in on rather than an automated pass.

Generated by Claude Code

houko · 2026-08-12T09:00:42Z

+    #[cfg(unix)]
+    fs::File::open(parent)
+        .and_then(|dir| dir.sync_all())
+        .map_err(|e| format!("sync parent directory {parent:?}: {e}"))?;


By this point fs::rename(&tmp, path) has already succeeded, so the secret is durably in its final location as far as every other reader of secrets.env is concerned. If the parent-directory fsync here fails, upsert_secret still returns Err, so the HTTP handler will very likely surface this to the caller/dashboard as "the secret was not saved" — which is misleading, since it was.

A retry is harmless (upsert_secret is an idempotent upsert), but I'm not fully confident what the right call is here: silently downgrade to a logged WARN and still return Ok(()) (the data write succeeded; only crash-durability confirmation failed), or keep returning Err but reword the message to make clear the value round-tripped and only the fsync confirmation failed? Flagging rather than changing since it's the exact durability-semantics tradeoff this PR is about.

Generated by Claude Code

houko · 2026-08-12T09:00:44Z

+
+    if let Err(error) = fs::rename(&tmp, path) {
+        let _ = fs::remove_file(&tmp);
+        return Err(format!("rename {tmp:?} -> {path:?}: {error}"));


The new well_formed_key_still_writes_a_single_line test covers the success path's "no leftover staging file" claim, but neither of the two new cleanup branches this PR adds (the write_result Err arm above, and this rename Err arm) has a regression test exercising it — e.g. pointing path at an existing directory to force rename() to fail, then asserting the .secrets.env.tmp.* file is gone from parent. Since "no secret-bearing staging file left behind on failure" is the PR's stated goal, that seems worth asserting directly rather than inferring from reading the code.

Generated by Claude Code

houko · 2026-08-12T09:01:15Z

@@ -105,9 +105,24 @@ pub fn upsert_secret(path: &Path, key: &str, value: &str) -> Result<(), String>
        }
        f.write_all(out.as_bytes())


Not part of this diff's changed lines (a few lines up, outside both hunks), but since this PR is specifically about hardening this staging-file sequence, flagging it here where I can comment: OpenOptions::open() a few lines above creates the staging file at the process umask (0644 under a common 022 umask), and set_permissions(0o600) only lands a couple of instructions later — before write_all here puts the secret bytes into the file, but after the file already exists on disk with the wider, umask-derived mode. So there's a short window where a newly-created (still-empty) .secrets.env.tmp.* file has non-0600 permissions, immediately before it receives the secret.

Since this PR is already reworking this exact block to close the "secret-bearing staging file left behind" gap, it seems like the natural place to also close the permission gap: use std::os::unix::fs::OpenOptionsExt::mode(0o600) on the OpenOptions builder before .open() instead of chmod-after-open, so the file is created with the correct mode atomically and there's no window at all.

Leaving this as a comment rather than a suggested diff since it's file-permission-sensitive code touching secret handling.

Generated by Claude Code

upsert_secret already removes the .secrets.env.tmp staging file when fs::rename fails, but that path had no regression test, and the directory-fsync step added right after the rename had no comment explaining why it's needed on Unix. Add a test that forces the rename to fail (destination path is an existing directory) and asserts the staging file does not survive, and document the crash-durability reasoning for the post-rename directory fsync.

houko

Automated review pass against CLAUDE.md conventions.

Pushed a small follow-up commit (6f48b35) adding a regression test for the rename-failure staging-file cleanup path and a comment explaining why the post-rename directory fsync is needed — both were new code paths this PR introduced with no test/rationale attached.

Left two comments for maintainer judgment: whether the post-rename directory-fsync failure should fail the whole call given the rename (and thus the actual secret write) already succeeded by that point, and a pre-existing AI-attributed commit author in this branch's history that can't be corrected without a history rewrite.

Generated by Claude Code

houko · 2026-08-12T09:18:45Z

+    #[cfg(unix)]
+    fs::File::open(parent)
+        .and_then(|dir| dir.sync_all())
+        .map_err(|e| format!("sync parent directory {parent:?}: {e}"))?;


Design question rather than a bug: at this point the rename has already succeeded, so the secret content is durably visible at path — only the directory entry's durability against a crash is in question. Propagating this fsync failure as Err(...) means the HTTP caller (routes/channels.rs) will report the write as failed even though it actually took effect, which could confuse an operator into retrying or alarming on a save that already landed.

Worth considering whether this should instead be best-effort (log a warning and still return Ok(())), the same way the old f.sync_all().ok() treated the pre-rename fsync before this PR. Not fixing this myself since it's a real behavior/API-contract choice, not a clear-cut bug — flagging for a maintainer call.

Generated by Claude Code

houko · 2026-08-12T09:18:55Z

@@ -0,0 +1,2 @@
+Secret writes to `secrets.env` could report success while a staging-file `fsync` failure went unnoticed, or leave a 0600 secret-bearing staging file behind after a failed write or rename.
+The staging file now propagates `fsync` errors instead of discarding them, gets removed on any write or rename failure, and the parent directory is fsynced after the atomic rename on Unix so a completed write survives a crash immediately afterward (#6944) (@houko)


Not about this fragment's content (format/attribution line look correct) — flagging the commit that added it. 5b23d952 ("docs(changelog): add fragment for secrets-env durable write fix") is authored as Claude <noreply@anthropic.com> in the pushed branch history (git log --format='%an <%ae>'). That's exactly the AI-attributed-author case this repo's own commit-msg hook is written to reject (per CLAUDE.md: "rejects a commit whose author identity ... resolves to Claude / Anthropic even when the message itself is clean"), so it must have landed through a path that bypassed the hook (e.g. GitHub UI/API commit rather than a hook-enabled local git commit).

I can't fix this myself without rewriting that commit (changing its SHA) and force-pushing, which is out of bounds for this session. Recommend squashing or rebasing with a corrected author before merge so the branch history doesn't carry AI attribution into main.

Generated by Claude Code

fix(runtime) PR was missing a changelog.d/ fragment for the durable config write; add one matching the pattern already used for the sibling durable-write fixes (#6942, #6944, #6945, #6947, #6948).

PR #6984 fixes a real TOCTOU race and adds rollback-on-reject behavior for registry content writes but was missing the changelog.d fragment other PRs in this durable-write series (#6942, #6944, #6945, #6947, #6948) already carry.

* fix(api): durably persist registry content * docs(changelog): add fragment for durable registry content write fix PR #6984 fixes a real TOCTOU race and adds rollback-on-reject behavior for registry content writes but was missing the changelog.d fragment other PRs in this durable-write series (#6942, #6944, #6945, #6947, #6948) already carry. --------- Co-authored-by: Evan <tonymo2048@gmail.com>

Evan and others added 2 commits August 12, 2026 17:32

fix(api): make secret writes durable

53b2ecf

docs(changelog): add fragment for secrets-env durable write fix

5b23d95

PR #6944 landed the fsync/atomic-rename durability fix for secrets.env writes without a changelog.d/ fragment.

houko commented Aug 12, 2026

View reviewed changes

houko mentioned this pull request Aug 12, 2026

fix(api): make sidecar config writes durable #6945

Merged

github-actions Bot added the size/M 50-249 lines changed label Aug 12, 2026

houko commented Aug 12, 2026

View reviewed changes

This was referenced Aug 12, 2026

fix(api): harden skill secret writes #6947

Merged

fix(api): make atomic renames durable #6942

Merged

houko merged commit 1b65cd0 into main Aug 12, 2026
36 checks passed

houko deleted the fix/secrets-env-durable-write branch August 12, 2026 10:11

houko mentioned this pull request Aug 12, 2026

fix(runtime): atomically persist MCP migration #6975

Closed

Navigation Menu

Repository navigation

fix(api): make secret writes durable - #6944

fix(api): make secret writes durable#6944
houko merged 3 commits into
mainfrom
fix/secrets-env-durable-write

houko commented Aug 12, 2026

Uh oh!

houko left a comment

Uh oh!

houko Aug 12, 2026

Uh oh!

houko Aug 12, 2026

Uh oh!

houko left a comment

Uh oh!

houko Aug 12, 2026

Uh oh!

houko Aug 12, 2026

Uh oh!

houko Aug 12, 2026

Uh oh!

houko left a comment

Uh oh!

houko Aug 12, 2026

Uh oh!

houko Aug 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -105,9 +105,24 @@ pub fn upsert_secret(path: &Path, key: &str, value: &str) -> Result<(), String>
		}
		f.write_all(out.as_bytes())

		@@ -0,0 +1,2 @@
		Secret writes to `secrets.env` could report success while a staging-file `fsync` failure went unnoticed, or leave a 0600 secret-bearing staging file behind after a failed write or rename.
		The staging file now propagates `fsync` errors instead of discarding them, gets removed on any write or rename failure, and the parent directory is fsynced after the atomic rename on Unix so a completed write survives a crash immediately afterward (#6944) (@houko)

Conversation

houko commented Aug 12, 2026

Summary

Verification

Out of scope

Uh oh!

houko left a comment

Choose a reason for hiding this comment

Uh oh!

houko Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

houko Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

houko left a comment

Choose a reason for hiding this comment

Uh oh!

houko Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

houko Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

houko Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

houko left a comment

Choose a reason for hiding this comment

Uh oh!

houko Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

houko Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants