fix(apps): consolidate duplicated atomic-JSON write into one shared helper#245
Merged
Merged
Conversation
Two byte-for-byte-identical atomic-JSON writers had drifted between
src/apps/claude-code/config.ts (writeClaudeCodeSettings) and
src/apps/claude-desktop/config.ts (atomicWriteJson). Consolidate both onto a
single shared helper, atomicWriteJson(filePath, value, opts?), in
src/lib/atomic-json.ts, with both app functions kept as thin wrappers (public
signatures unchanged, all call sites untouched).
The shared helper preserves the ORIGINAL, correct pattern exactly:
unlink stale <file>.tmp (ignore ENOENT) → open O_WRONLY|O_CREAT|O_EXCL @0o600
→ write → fsync → rename.
Both properties matter and are preserved:
- O_EXCL is the real symlink guard: the kernel refuses to open THROUGH a
symlink at the final path component, so neither writer was ever vulnerable
to a pre-planted symlink temp.
- The unlink is crash recovery: it clears a stale regular <file>.tmp left by
a write that died between open and rename (and removes a symlink entry
without following it), so the next write self-heals instead of failing
forever. Dropping it would have been a net regression — a self-inflicted
denial-of-writes on the hot enable/disable/boot-reconcile path.
The EEXIST branch is kept as a concurrency/attack backstop with a clear
message. Adds tests/atomic-json-write.test.ts asserting, for the shared helper
and both app writers: happy-path 0o600/content/no-.tmp-leak, self-heal over a
stale regular temp, and clearing a planted symlink temp without following or
clobbering its target.
Closes #231
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two near-identical atomic-JSON writers had drifted:
atomicWriteJsoninsrc/apps/claude-desktop/config.tswriteClaudeCodeSettingsinsrc/apps/claude-code/config.tsSame mechanics (mkdir,
O_WRONLY|O_CREAT|O_EXCL@0o600, write+fsync+rename), but only the claude-code copy carried a friendly EEXIST error. Two copies of security-sensitive file I/O is exactly the drift #231 flags.Fix
One shared
src/lib/atomic-json.ts—atomicWriteJson(filePath, value, opts?: { label? })— that both app writers now call as thin wrappers (public signatures + all call sites unchanged;labelcustomizes the error text).Preserved the correct semantics (not a naive rewrite)
An initial pass removed the pre-write
unlinkSync(tmp), reasoning it defeated the symlink guard. That was wrong and was reverted after adversarial review of the syscall semantics:O_EXCLrefuses to open through a symlink at the final path component (EEXIST), andunlinkremoves the link itself without following it. Neither app was vulnerable.unlink's real job is crash recovery. A.tmpleft by a crashed prior write is cleared on the next attempt so writes self-heal. Removing it would hard-fail every subsequent write with a misleading "symlink attack" error until manual cleanup — a real availability regression for zero security gain.So the shared helper consolidates onto the original unlink-then-O_EXCL pattern:
mkdir -p→unlinkstale temp (ENOENT-swallowed, symlink-safe) →O_EXCLcreate @0o600 → write+fsync+rename. The EEXIST→friendly-error branch is retained as a concurrency/attack backstop.Tests —
tests/atomic-json-write.test.ts(9)Assert both properties for the shared helper and both app writers:
<file>.tmpis cleared and the write succeeds.<file>.tmpis not followed — the real write lands at the intended path, and the symlink's target keeps its original content (proven with avictimfile)..tmpleak) + parent-dir creation.Verification
bun run typecheck✅atomic-json-write+ both app config tests) → 66 pass / 0 fail ✅bun run lint:all✅Closes #231