harden(clients): preserve file mode (0600) + flush on atomic config writes (#545)

* harden(clients): preserve file mode on atomic config writes, default 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>

* harden(clients): flush the temp file before the atomic rename

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>

* harden(clients): address Copilot review on atomic-write permission 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>

* harden(clients): close the temp-file world-readable window before writing 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>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: dsarno

plugin/addons/godot_ai/clients/_atomic_write.gd

@@ -20,23 +20,61 @@ static func write(path: String, content: String) -> bool:
 		if DirAccess.make_dir_recursive_absolute(dir_path) != OK:
 			return false
 
+	# Decide the permission mode the final file (and its backup) must carry
+	# BEFORE we replace anything. A rewrite must preserve the prior file's
+	# mode: the Claude CLI creates ~/.claude.json as 0600 (it holds OAuth
+	# creds + history), and a naive FileAccess write + DirAccess copy would
+	# silently relax that to the umask default (0644) and leak it on shared
+	# machines. A brand-new config defaults to owner-only 0600 since these
+	# files routinely carry tokens. On platforms without POSIX permissions
+	# (Windows) the get/set calls no-op and this logic is inert. See #297
+	# finding TC-1.
+	var had_original := FileAccess.file_exists(path)
+	var target_mode := _resolve_target_mode(path, had_original)
+
 	var tmp_path := path + ".tmp"
 	var file := FileAccess.open(tmp_path, FileAccess.WRITE)
 	if file == null:
 		return false
+	# Lock the temp inode down BEFORE writing any bytes. FileAccess.open creates
+	# it at the umask default (often 0644); chmod'ing the still-empty file first
+	# means the config contents are never on disk under a world-readable mode in
+	# the create->chmod gap. rename preserves the inode mode, so the swapped-in
+	# file lands correct and is never briefly world-readable under the target name.
+	_apply_mode(tmp_path, target_mode)
 	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()
+	# Re-assert the mode on the closed inode. The pre-write chmod above closes
+	# the world-readable window; this second apply is the authoritative one
+	# (a chmod issued while the FileAccess handle is still open doesn't reliably
+	# stick inside the editor) and guarantees the final mode before the rename,
+	# which preserves it.
+	_apply_mode(tmp_path, target_mode)
 
 	# Best-effort: snapshot the prior file before we touch the target so we
 	# can restore on a failed swap. The backup is also kept on success as a
-	# one-shot rollback aid for the user.
+	# one-shot rollback aid for the user — give it the same (preserved) mode
+	# so a 0600 config's backup isn't itself a world-readable copy.
+	#
+	# copy_absolute creates the backup at the umask default and we can only
+	# chmod it afterward, so there's a sub-millisecond window where the backup
+	# carries default perms. Accepted: it duplicates bytes already sitting at
+	# `path` (which the caller created 0600) inside the user's own config dir,
+	# and Godot exposes no API to create the copy pre-chmod'd. Not worth
+	# reimplementing copy by hand to shave that window.
 	var backup_path := path + ".backup"
-	var had_original := FileAccess.file_exists(path)
 	var backup_made := false
 	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)
 
 	if DirAccess.rename_absolute(tmp_path, path) == OK:
 		return true
@@ -46,6 +84,9 @@ static func write(path: String, content: String) -> bool:
 	# removes the original before writing the new bytes, so a failure here
 	# leaves the user's prior config in place rather than nuking it.
 	if DirAccess.copy_absolute(tmp_path, path) == OK and _written_size_matches(path, content):
+		# copy_absolute creates the destination with the default mode, so
+		# re-apply the preserved/owner-only mode after the copy lands.
+		_apply_mode(path, target_mode)
 		DirAccess.remove_absolute(tmp_path)
 		return true
 
@@ -59,6 +100,7 @@ static func write(path: String, content: String) -> bool:
 		# and the false return value tells the caller the swap didn't
 		# complete.
 		DirAccess.copy_absolute(backup_path, path)
+		_apply_mode(path, target_mode)
 	elif not had_original and FileAccess.file_exists(path):
 		# No prior file existed but copy_absolute landed partial bytes at
 		# `path`. Remove them so the failure leaves nothing on disk rather
@@ -76,6 +118,38 @@ static func write(path: String, content: String) -> bool:
 	return false
 
 
+static func _resolve_target_mode(path: String, had_original: bool) -> int:
+	# Preserve the prior file's POSIX mode on a rewrite; default a brand-new
+	# config (or any case we can't read a mode for) to owner read+write (0600).
+	#
+	# get_unix_permissions returns 0 both on Windows (no POSIX perms) and for a
+	# genuine 0000 file. Treating 0 as "use the 0600 floor" is deliberate, not a
+	# missed case: these are config files the plugin must read and write, 0000 is
+	# unusable, and re-applying 0000 would lock the owner out next run. 0600 is
+	# still owner-only so this never widens access. (A genuinely-0000 file can't
+	# reach a rewrite through the config strategies anyway — their read-first
+	# guard fails to open it and refuses the write before we get here.)
+	if had_original:
+		var existing := FileAccess.get_unix_permissions(path)
+		if existing > 0:
+			return existing
+	return FileAccess.UNIX_READ_OWNER | FileAccess.UNIX_WRITE_OWNER
+
+
+static func _apply_mode(path: String, mode: int) -> void:
+	# Best-effort. set_unix_permissions returns ERR_UNAVAILABLE on platforms
+	# without POSIX permissions (Windows); that's expected and ignored so the
+	# write still works 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:
+		return
+	var err := FileAccess.set_unix_permissions(path, mode)
+	# Surface a real chmod failure (not the Windows no-op) so permission
+	# hardening on a sensitive config doesn't fail completely silently.
+	if err != OK and err != ERR_UNAVAILABLE:
+		push_warning("MCP | could not set permissions on %s (error %d)" % [path, err])
+
+
 static func _written_size_matches(path: String, content: String) -> bool:
 	# `store_string` writes UTF-8 bytes with no BOM and no newline translation,
 	# so the byte length on disk must match `to_utf8_buffer().size()` exactly.

test_project/tests/test_clients.gd

@@ -1401,6 +1401,113 @@ func test_atomic_write_preserves_existing_file_when_swap_fails() -> void:
 	DirAccess.remove_absolute(backup_path)
 
 
+# ----- atomic write: permission preservation (#297 finding TC-1) -----
+#
+# The Claude CLI creates ~/.claude.json as 0600 (it holds OAuth creds). A
+# rewrite must preserve that mode rather than relaxing it to the umask default,
+# and the .backup must not become a world-readable copy of a private file.
+# These bits don't exist on Windows, so the suite skips there.
+
+const _PERM_MASK := 0x1FF  # 0o777 — the rwx bits for owner/group/other
+
+
+func _owner_only_mode() -> int:
+	return FileAccess.UNIX_READ_OWNER | FileAccess.UNIX_WRITE_OWNER
+
+
+func test_atomic_write_preserves_restrictive_mode_on_rewrite() -> void:
+	if OS.get_name() == "Windows":
+		skip("POSIX file permissions are unavailable on Windows")
+		return
+	var path := _scratch_dir.path_join("perm_preserve_0600.txt")
+	var f := FileAccess.open(path, FileAccess.WRITE)
+	f.store_string("secret v1")
+	f.close()
+	var owner_only := _owner_only_mode()
+	assert_eq(
+		FileAccess.set_unix_permissions(path, owner_only), OK, "test setup: chmod 0600 must succeed"
+	)
+
+	assert_true(McpAtomicWrite.write(path, "secret v2"))
+
+	assert_eq(
+		FileAccess.get_unix_permissions(path) & _PERM_MASK,
+		owner_only,
+		"a rewrite must preserve the prior 0600 mode, not relax it to 0644",
+	)
+	DirAccess.remove_absolute(path + ".backup")
+
+
+func test_atomic_write_backup_inherits_restrictive_mode() -> void:
+	if OS.get_name() == "Windows":
+		skip("POSIX file permissions are unavailable on Windows")
+		return
+	var path := _scratch_dir.path_join("perm_backup_0600.txt")
+	var f := FileAccess.open(path, FileAccess.WRITE)
+	f.store_string("secret v1")
+	f.close()
+	var owner_only := _owner_only_mode()
+	assert_eq(FileAccess.set_unix_permissions(path, owner_only), OK, "test setup: chmod 0600")
+
+	assert_true(McpAtomicWrite.write(path, "secret v2"))
+
+	var backup_path := path + ".backup"
+	assert_true(FileAccess.file_exists(backup_path), "backup must exist")
+	assert_eq(
+		FileAccess.get_unix_permissions(backup_path) & _PERM_MASK,
+		owner_only,
+		"the .backup of a 0600 file must itself be 0600, not a world-readable copy",
+	)
+	DirAccess.remove_absolute(backup_path)
+
+
+func test_atomic_write_new_file_defaults_to_owner_only() -> void:
+	if OS.get_name() == "Windows":
+		skip("POSIX file permissions are unavailable on Windows")
+		return
+	var path := _scratch_dir.path_join("perm_new_file.txt")
+	# No prior file: nothing to preserve, so a fresh config defaults to 0600
+	# regardless of the process umask.
+	assert_false(FileAccess.file_exists(path), "test setup: target must not pre-exist")
+
+	assert_true(McpAtomicWrite.write(path, "fresh token config"))
+
+	assert_eq(
+		FileAccess.get_unix_permissions(path) & _PERM_MASK,
+		_owner_only_mode(),
+		"a brand-new config must default to owner-only 0600",
+	)
+
+
+func test_atomic_write_preserves_relaxed_mode_on_rewrite() -> void:
+	## We preserve the prior mode — we do NOT force 0600 on a file that was
+	## already group/other-readable (e.g. a 0644 cursor config). This proves
+	## the fix is "preserve", not "clamp everything to 0600".
+	if OS.get_name() == "Windows":
+		skip("POSIX file permissions are unavailable on Windows")
+		return
+	var path := _scratch_dir.path_join("perm_preserve_0644.txt")
+	var f := FileAccess.open(path, FileAccess.WRITE)
+	f.store_string("public v1")
+	f.close()
+	var relaxed := (
+		FileAccess.UNIX_READ_OWNER
+		| FileAccess.UNIX_WRITE_OWNER
+		| FileAccess.UNIX_READ_GROUP
+		| FileAccess.UNIX_READ_OTHER
+	)  # 0644
+	assert_eq(FileAccess.set_unix_permissions(path, relaxed), OK, "test setup: chmod 0644")
+
+	assert_true(McpAtomicWrite.write(path, "public v2"))
+
+	assert_eq(
+		FileAccess.get_unix_permissions(path) & _PERM_MASK,
+		relaxed,
+		"a 0644 file must stay 0644 — preserve the prior mode, don't clamp to 0600",
+	)
+	DirAccess.remove_absolute(path + ".backup")
+
+
 # ----- handler -----
 
 func test_handler_rejects_unknown_client() -> void: