fix(swift): create the client-id file with owner-only permissions

clients/swift/AgentHostProtocol/Sources/AgentHostProtocolClient/Hosts/ClientIdStore.swift

@@ -57,10 +57,10 @@ public final class InMemoryClientIdStore: ClientIdStore {
 
 /// Filesystem-backed `ClientIdStore` that survives process restarts.
 ///
-/// Stores one file per host id under a configurable directory; writes are
-/// atomic (`.atomic` Data option) and best-effort restrict file permissions
-/// to owner-read/write on POSIX platforms so the persisted ids aren't
-/// world-readable. Per-store mutations are serialised through an internal
+/// Stores one file per host id under a configurable directory. Writes go
+/// through a temp file opened with owner-read/write permissions and are then
+/// renamed into place, so the persisted ids are atomic on the same volume and
+/// are never present on disk at world-readable permissions. Per-store mutations are serialised through an internal
 /// actor so concurrent `load`/`store` calls from different hosts don't race
 /// on the directory's contents.
 ///
@@ -118,15 +118,70 @@ public final class FileClientIdStore: ClientIdStore {
             ensureDirectory()
             let url = fileURL(for: hostId)
             guard let data = clientId.data(using: .utf8) else { return }
-            do {
-                try data.write(to: url, options: [.atomic])
-                // Best-effort restrict permissions to owner-only on POSIX
-                // platforms. Silently ignore on platforms where this
-                // attribute isn't applicable.
-                try? fm.setAttributes([.posixPermissions: 0o600], ofItemAtPath: url.path)
-            } catch {
+
+            // Create the temp file with owner-only permissions BEFORE any bytes
+            // land in it, then rename(2) it into place.
+            //
+            // The previous shape — `data.write(to:options:.atomic)` followed by a
+            // `setAttributes(.posixPermissions)` — put the id on disk at the
+            // umask default (0644 under a typical umask) and only tightened it
+            // afterwards, so the first store for a host id was briefly world
+            // readable at a predictable path. The 0700 on `directory` does not
+            // cover that: `ensureDirectory()` only applies it on the branch where
+            // it CREATES the directory, so a pre-existing Application Support or
+            // XDG path keeps its own mode.
+            //
+            // `open(2)` with an explicit mode has no such window, and rename(2) is
+            // atomic and carries the 0600 across — including over a destination
+            // that was already left at looser permissions by an older build.
+            let tempURL = directory.appendingPathComponent(".\(UUID().uuidString).tmp")
+            let fd = tempURL.withUnsafeFileSystemRepresentation { path -> Int32 in
+                guard let path else { return -1 }
+                return open(path, O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC, 0o600)
+            }
+            guard fd >= 0 else {
+                #if DEBUG
+                print("[FileClientIdStore] failed to create temp file for \(hostId)")
+                #endif
+                return
+            }
+            // open(2)'s mode argument is masked by the process umask, so a umask
+            // carrying 0o200 would leave the file read-only (0o400) and a stricter one
+            // could strip owner access entirely. fchmod is not masked, so it pins the
+            // mode to exactly 0o600 — the guarantee the previous chmod-after-write
+            // shape did provide, kept here without reopening the window it left. This
+            // mirrors the netstandard2.0 leg of the .NET fix in #411.
+            _ = fchmod(fd, 0o600)
+
+            var written = 0
+            data.withUnsafeBytes { buffer in
+                guard let base = buffer.baseAddress else { return }
+                while written < buffer.count {
+                    let n = write(fd, base + written, buffer.count - written)
+                    if n <= 0 { break }
+                    written += n
+                }
+            }
+            close(fd)
+
+            guard written == data.count else {
+                try? fm.removeItem(at: tempURL)
+                #if DEBUG
+                print("[FileClientIdStore] short write persisting id for \(hostId)")
+                #endif
+                return
+            }
+
+            let renamed = tempURL.withUnsafeFileSystemRepresentation { src -> Bool in
+                url.withUnsafeFileSystemRepresentation { dst -> Bool in
+                    guard let src, let dst else { return false }
+                    return rename(src, dst) == 0
+                }
+            }
+            if !renamed {
+                try? fm.removeItem(at: tempURL)
                 #if DEBUG
-                print("[FileClientIdStore] failed to persist id for \(hostId): \(error)")
+                print("[FileClientIdStore] failed to persist id for \(hostId)")
                 #endif
             }
         }

clients/swift/AgentHostProtocol/Tests/AgentHostProtocolClientTests/FileClientIdStoreTests.swift

@@ -89,18 +89,141 @@ final class FileClientIdStoreTests: XCTestCase {
     }
 
     func testFileIsRestrictedToOwnerWhenPossible() async throws {
-        // Smoke test for the perm-restriction code path on POSIX
-        // platforms. We don't assert on non-POSIX file systems where
-        // this is a no-op.
         let store = FileClientIdStore(directory: tempDir)
         await store.store("h", clientId: "value")
 
         let url = tempDir.appendingPathComponent("h.clientid")
-        if let attrs = try? FileManager.default.attributesOfItem(atPath: url.path),
-           let perms = attrs[.posixPermissions] as? NSNumber {
-            // 0o600 = 384
-            XCTAssertEqual(perms.intValue & 0o777, 0o600,
-                           "expected owner-only permissions on the persisted file")
+        // Assert unconditionally. This used to be wrapped in `if let attrs = try?`
+        // with no `else`, so a failure to read the attributes passed the test
+        // silently rather than failing it.
+        let attrs = try FileManager.default.attributesOfItem(atPath: url.path)
+        let perms = try XCTUnwrap(attrs[.posixPermissions] as? NSNumber)
+        XCTAssertEqual(perms.intValue & 0o777, 0o600,
+                       "expected owner-only permissions on the persisted file")
+        // This runs after `store()` returns, so on its own it cannot tell a file
+        // created 0600 from one created loose and chmod'd afterwards. The two tests
+        // below cover that difference directly.
+    }
+
+    /// The window itself. `Data.write(to:options:.atomic)` writes a temp file in the
+    /// destination directory at the umask default and only chmods after the rename, so
+    /// the client id is briefly readable by any local user. This watches the directory
+    /// while a store runs and fails if ANY file in it is ever observed carrying group or
+    /// other permission bits.
+    ///
+    /// Measured against the pre-fix implementation this observes the leak in 19 of 20
+    /// runs; against the current one, 0 of 20. The asymmetry is deliberate — the
+    /// assertion is "nothing was ever loose", which the current implementation satisfies
+    /// by construction, so the test cannot fail spuriously. Only a real regression fails
+    /// it, and the repeat count makes missing one vanishingly unlikely.
+    func testClientIdIsNeverObservableAtLoosePermissionsDuringStore() async throws {
+        for attempt in 0..<5 {
+            let dir = tempDir.appendingPathComponent("attempt-\(attempt)")
+            try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true)
+            // 0755, and pre-existing: `ensureDirectory()` only applies 0700 on the branch
+            // where it CREATES the directory, so this is the reachable configuration.
+            try FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: dir.path)
+
+            let observer = LoosePermissionObserver()
+            let stop = DispatchSemaphore(value: 0)
+            DispatchQueue.global().async {
+                let fm = FileManager.default
+                while stop.wait(timeout: .now()) == .timedOut {
+                    guard let entries = try? fm.contentsOfDirectory(atPath: dir.path) else { continue }
+                    for entry in entries {
+                        let path = dir.appendingPathComponent(entry).path
+                        guard let attrs = try? fm.attributesOfItem(atPath: path),
+                              let perms = attrs[.posixPermissions] as? NSNumber,
+                              perms.intValue & 0o077 != 0
+                        else { continue }
+                        observer.record("\(entry) was 0\(String(perms.intValue & 0o777, radix: 8))")
+                    }
+                }
+            }
+
+            // A payload large enough that the write is not instantaneous. 1 MB detects the
+            // pre-fix leak in 20 of 20 trials; the value is about widening the window, not
+            // about any realistic client-id length.
+            let store = FileClientIdStore(directory: dir)
+            await store.store("h", clientId: String(repeating: "s", count: 1_000_000))
+
+            stop.signal()
+            try await Task.sleep(nanoseconds: 20_000_000)
+
+            XCTAssertNil(observer.first,
+                         "client id was observable at loose permissions during store(): \(observer.first ?? "")")
+        }
+    }
+
+    /// `open(2)`'s mode argument is masked by the process umask, so opening with 0600
+    /// under a umask carrying 0o200 yields a read-only 0400 file. The explicit `fchmod`
+    /// is what pins it to exactly 0600. Without that call this test fails with 0400.
+    func testPersistedModeIsExactlyOwnerOnlyRegardlessOfUmask() async throws {
+        try FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true)
+        try FileManager.default.setAttributes([.posixPermissions: 0o700], ofItemAtPath: tempDir.path)
+
+        // umask is process-wide; restore it immediately so no sibling test sees it.
+        let saved = umask(0o277)
+        let store = FileClientIdStore(directory: tempDir)
+        await store.store("h", clientId: "value")
+        umask(saved)
+
+        let url = tempDir.appendingPathComponent("h.clientid")
+        let attrs = try FileManager.default.attributesOfItem(atPath: url.path)
+        let perms = try XCTUnwrap(attrs[.posixPermissions] as? NSNumber)
+        XCTAssertEqual(perms.intValue & 0o777, 0o600,
+                       "expected exactly 0600 under a restrictive umask, got 0\(String(perms.intValue & 0o777, radix: 8))")
+        let value = await store.load("h")
+        XCTAssertEqual(value, "value", "a file the owner cannot read back is not a fix")
+    }
+
+    /// `rename(2)` carries the temp file's mode onto the destination, so a file
+    /// left at loose permissions by an older build is repaired by the next store
+    /// rather than kept.
+    func testStoreRepairsPreExistingLoosePermissions() async throws {
+        try FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true)
+        let url = tempDir.appendingPathComponent("h.clientid")
+        XCTAssertTrue(FileManager.default.createFile(
+            atPath: url.path,
+            contents: Data("stale".utf8),
+            attributes: [.posixPermissions: 0o644]))
+
+        let store = FileClientIdStore(directory: tempDir)
+        await store.store("h", clientId: "fresh")
+
+        let attrs = try FileManager.default.attributesOfItem(atPath: url.path)
+        let perms = try XCTUnwrap(attrs[.posixPermissions] as? NSNumber)
+        XCTAssertEqual(perms.intValue & 0o777, 0o600,
+                       "a pre-existing 0644 client-id file should be repaired to 0600")
+        let value = await store.load("h")
+        XCTAssertEqual(value, "fresh")
+    }
+
+    /// A failed or interrupted store must not leave its temp file behind.
+    func testNoTempFilesLeftBehind() async throws {
+        let store = FileClientIdStore(directory: tempDir)
+        for i in 0..<8 {
+            await store.store(HostId("h-\(i)"), clientId: "id-\(i)")
         }
+        let entries = try FileManager.default.contentsOfDirectory(atPath: tempDir.path)
+        let temps = entries.filter { $0.hasSuffix(".tmp") }
+        XCTAssertTrue(temps.isEmpty, "left temp files behind: \(temps)")
+    }
+}
+
+/// Records the first loose-permission observation. A class with a lock rather than an
+/// actor so the polling closure can write to it without an await.
+private final class LoosePermissionObserver: @unchecked Sendable {
+    private let lock = NSLock()
+    private var _first: String?
+
+    var first: String? {
+        lock.lock(); defer { lock.unlock() }
+        return _first
+    }
+
+    func record(_ description: String) {
+        lock.lock(); defer { lock.unlock() }
+        if _first == nil { _first = description }
     }
 }

docs/.changes/20260824-swift-client-id-permissions.json

@@ -0,0 +1,5 @@
+{
+  "type": "security",
+  "message": "Swift `FileClientIdStore` creates its client-id file with owner-only permissions before writing, instead of writing at the umask default and restricting afterwards, so the persisted id is never briefly readable by other local users.",
+  "targets": ["swift"]
+}