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

plugin/addons/godot_ai/clients/_atomic_write.gd

@@ -20,23 +20,46 @@ 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
 	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)
 
 	# 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.
 	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 +69,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 +85,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 +103,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: