harden(clients): preserve file mode (0600) + flush on atomic config writes#545
Merged
Conversation
…new configs to 0600 McpAtomicWrite created the tmp file with the umask default (0644) and used DirAccess.copy_absolute for the backup, neither of which preserves the prior file's permissions. The Claude Code strategy writes directly into ~/.claude.json, which the Claude CLI creates 0600 (it holds OAuth creds + history); every "Configure" click silently relaxed it to 0644 and left a 0644 .backup, leaking it on shared machines. Capture the prior file's POSIX mode before replacing it and re-apply it to the swapped-in file and the backup, so a 0600 config stays 0600 (and a 0644 config stays 0644 — we preserve, we don't clamp). Brand-new configs default to owner-only 0600 since these files routinely carry tokens. The mode is set on the tmp inode before the rename so the target is never briefly world-readable. On platforms without POSIX permissions (Windows) get/set_unix_permissions no-op and the logic is inert. GDScript tests (run against live Godot 4.6.3, real on-disk chmod): preserve 0600 on rewrite, backup inherits 0600, new file defaults to 0600, and a 0644 file stays 0644. Skipped on Windows. Full suite: 1365/1382 passed, 0 failed. Addresses advisory GHSA-gw6h-44vw-g5rc. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
store_string buffers in Godot's FileAccess; close() flushes, but doing the flush() explicitly before the rename makes the ordering intent clear and ensures the bytes are handed to the OS before the swap. Godot exposes no fsync, so durability against power loss still depends on the OS cache — the comment documents that residual. Pairs with the permission-preservation fix on this branch (atomic config writes). Addresses review finding TC-7 (#531). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dsarno
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the Godot plugin’s client config “atomic write” path so sensitive config files (e.g., ~/.claude.json) don’t accidentally become world-readable during rewrites, and ensures the temp file buffer is flushed before the swap to reduce app-layer reordering risk.
Changes:
- Preserve existing file permissions on rewrite; default newly created configs to owner-only (
0600) and apply the same mode to.backup. - Call
flush()on the temp file prior to rename/copy to reduce ordering risk at the application layer. - Add GDScript tests covering permission preservation for rewrites, backups, and new files (skipped on Windows).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| plugin/addons/godot_ai/clients/_atomic_write.gd | Adds permission preservation/defaulting + flush prior to swap; applies mode to tmp/backup/fallback-copy paths. |
| test_project/tests/test_clients.gd | Adds coverage for permission preservation/defaulting behavior of McpAtomicWrite.write(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+112
to
+116
| if had_original: | ||
| var existing := FileAccess.get_unix_permissions(path) | ||
| if existing > 0: | ||
| return existing | ||
| return FileAccess.UNIX_READ_OWNER | FileAccess.UNIX_WRITE_OWNER |
Comment on lines
+119
to
+125
| static func _apply_mode(path: String, mode: int) -> void: | ||
| # Best-effort. set_unix_permissions returns ERR_UNAVAILABLE on platforms | ||
| # without POSIX permissions (Windows); ignoring it keeps the write working | ||
| # there. mode <= 0 should never happen (resolve always returns >0) but is | ||
| # guarded so a future caller can't chmod a file to nothing. | ||
| if mode > 0: | ||
| FileAccess.set_unix_permissions(path, mode) |
- Clarify why get_unix_permissions == 0 maps to the 0600 floor rather than "preserving" 0000: these are config files the plugin must read/write, 0000 is unusable, re-applying it would lock the owner out, and 0600 never widens access. (A genuinely-0000 file can't reach a rewrite anyway — the config strategies' read-first guard refuses it first.) Copilot's literal "preserve 0000" suggestion would regress: _apply_mode skips mode 0, leaving the file at copy_absolute's umask default (potentially 0644). - _apply_mode now captures set_unix_permissions' return and push_warning()s on a real failure (not the Windows ERR_UNAVAILABLE no-op), so permission hardening on a sensitive config no longer fails completely silently. Live Godot 4.6.3: clients suite 97/97. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment on lines
35
to
+50
| var tmp_path := path + ".tmp" | ||
| var file := FileAccess.open(tmp_path, FileAccess.WRITE) | ||
| if file == null: | ||
| return false | ||
| file.store_string(content) | ||
| # Push Godot's internal buffer out to the OS before the rename. Godot | ||
| # exposes no fsync, so the bytes aren't guaranteed durable on the physical | ||
| # disk until the OS flushes its own cache — a power loss in that window can | ||
| # still lose the data. But flush() ensures the rename can't be ordered ahead | ||
| # of the write at the application layer, which is the failure this guards. | ||
| file.flush() | ||
| file.close() | ||
| # Set the mode on the tmp inode before the rename — rename preserves the | ||
| # inode's mode, so the swapped-in file lands with the right permissions | ||
| # and is never briefly world-readable under the target name. | ||
| _apply_mode(tmp_path, target_mode) |
Comment on lines
58
to
+62
| if had_original: | ||
| DirAccess.remove_absolute(backup_path) | ||
| if DirAccess.copy_absolute(path, backup_path) == OK: | ||
| backup_made = true | ||
| _apply_mode(backup_path, target_mode) |
…ting secrets Copilot re-review: the temp file was created at the umask default, the config contents written, and only then chmod'd — leaving a brief window where secrets sat on disk world-readable under path+".tmp". Now chmod the still-empty temp inode to the target mode BEFORE store_string, so the contents never land under a permissive mode. A chmod issued while the FileAccess handle is open does not reliably stick inside the editor, so the post-close apply is kept as the authoritative one (the rename then preserves the mode). Documented the small residual window on the .backup copy (copy_absolute can't be created pre-chmod'd; it duplicates already-0600 bytes in the user's own dir, chmod'd immediately). Live Godot 4.6.3: clients suite 97/97. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
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.
Wave 1 advisory
GHSA-gw6h-44vw-g5rc(TC-1) plus review finding TC-7.0600, so rewriting~/.claude.json(0600, OAuth + history) no longer downgrades it to 0644 or leaves a world-readable backup.flush()the temp file before the rename so the write can't be ordered behind the swap at the app layer. Godot exposes no fsync; the comment documents the residual power-loss caveat.Test
clientssuite 97/97 (incl. the new permission tests); parse clean.Closes #531.
🤖 Generated with Claude Code