Desktop: harden auth-token storage and local automation bridge

[kodjima33]

Summary

Security fixes for the macOS desktop app based on a horizontal-authz audit — each issue lets one user access another's data on a shared Mac or network. Three changes:

1. Firebase auth tokens moved from UserDefaults to Keychain. The id/refresh tokens were written to ~/Library/Preferences/<bundle>.plist, readable by any non-sandboxed process running as the same macOS user. They now live in the Keychain under the app's bundle-scoped service, marked kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly. On first launch after upgrade, stored tokens are migrated out of UserDefaults lazily and the plaintext copy is deleted.
2. DesktopAutomationBridge locked down. When enabled (via --automation-bridge / OMI_ENABLE_LOCAL_AUTOMATION=1), the NWListener bound to all interfaces and every route ran unauthenticated — so POST /gmail-read could exfiltrate the signed-in user's inbox from any process or LAN host. Now: (a) requiredInterfaceType = .loopback, (b) non-loopback endpoints are rejected before reading a byte, (c) a 256-bit per-launch bearer token is generated via SecRandomCopyBytes, written to ~/Library/Application Support/Omi/automation-bridge.token (mode 0600), and required on every route with constant-time comparison.
3. playwrightExtensionToken cleared on sign-out. The token persisted across signOut(); next sign-in on the same Mac inherited the previous user's Chrome session, so "summarize my emails" could return the previous user's Gmail. Now cleared alongside the other onboarding keys in AuthService.signOut.

Test plan

• Mac mini: fresh install, sign in, confirm auth works end-to-end (REST token exchange + Firebase SDK path).
• Mac mini: upgrade from a version with tokens in UserDefaults; confirm tokens migrate on first launch and app stays signed in.
• Mac mini: sign out; confirm Keychain entries for auth_idToken / auth_refreshToken are removed and playwrightExtensionToken is gone from UserDefaults.
• Mac mini: launch with OMI_ENABLE_LOCAL_AUTOMATION=1; without the bearer token, curl http://127.0.0.1:47777/health returns 401; with Authorization: Bearer <token-from-file>, returns 200.
• Mac mini: confirm the bridge is not reachable from another host on the LAN (connect refused).

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

This PR hardens three security gaps in the macOS desktop app: Firebase auth tokens moved from UserDefaults to Keychain with kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly, the DesktopAutomationBridge locked to loopback with per-launch bearer-token auth, and playwrightExtensionToken cleared on sign-out.

• P1 (migration data loss): In readStoredToken(), UserDefaults.removeObject() is called unconditionally before verifying the AuthKeychain.set() write succeeded. If SecItemAdd fails (ad-hoc signing, Keychain locked, or any transient error), the token is permanently erased from UserDefaults while absent from Keychain — silently signing out all users who upgraded. The fix is to read back with AuthKeychain.get() and only remove the UserDefaults copy on success.

Confidence Score: 4/5

Hold for the migration data-loss fix before shipping to users upgrading from UserDefaults storage.

One P1 defect exists on the migration path: UserDefaults is cleared unconditionally even when the Keychain write fails, which will silently sign out any upgrading user whose Keychain write errors. This affects the entire upgrading population and is the primary user-visible risk of this PR. All other findings are P2. The automation bridge hardening and playwrightExtensionToken clearance are correct.

desktop/Desktop/Sources/AuthService.swift — readStoredToken() migration logic (lines 809–819)

Security Review

• Token file permissions (low): writeTokenFile() in DesktopAutomationBridge.swift writes the session token with default umask permissions (typically 0644), then narrows to 0600 via setAttributes. The brief window exposes the token to other processes running as the same macOS user. Risk is limited because the parent directory is already 0700.
• No other injection, CSRF, auth-bypass, or credential-leakage issues identified. The loopback binding, constant-time token comparison, and per-launch token generation are all correctly implemented.

Important Files Changed

Filename	Overview
desktop/Desktop/Sources/AuthService.swift	Token migration in readStoredToken() unconditionally deletes the UserDefaults copy before confirming the Keychain write succeeded, risking silent sign-out on upgrade for any user where SecItemAdd fails; remaining changes (clearTokens, playwrightExtensionToken clearance) are correct.
desktop/Desktop/Sources/DesktopAutomationBridge.swift	Loopback binding, defense-in-depth endpoint check, and bearer token auth are correctly implemented; token file has a brief window with loose permissions between atomic write and setAttributes() — low-severity given the 0700 parent directory.
desktop/Desktop/Sources/AuthKeychain.swift	New Keychain helper for Firebase token storage; correct use of kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly, but clearAll() is dead code that may cause future divergence with clearTokens().
desktop/CHANGELOG.json	Three user-facing changelog entries correctly summarizing the security hardening changes.

Sequence Diagram

sequenceDiagram
    participant App as macOS App
    participant KCH as AuthKeychain
    participant UD as UserDefaults
    participant DAB as DesktopAutomationBridge
    participant TF as token file (0600)
    participant Client as Automation Client

    Note over App,UD: First launch after upgrade (migration)
    App->>KCH: get(key) returns nil
    App->>UD: string(forKey: key) returns legacy value
    App->>KCH: set(legacy, forKey:) via SecItemAdd
    Note over KCH: If SecItemAdd fails, deletion still proceeds
    App->>UD: removeObject(forKey:) unconditional
    App-->>App: returns legacy (OK this session)
    Note over App,UD: Next launch: token gone from both stores

    Note over DAB,Client: Bridge startup (hardened)
    App->>DAB: startIfNeeded()
    DAB->>DAB: generateSessionToken() via SecRandomCopyBytes
    DAB->>TF: writeTokenFile() dir=0700 file 0644 then 0600
    DAB->>DAB: NWListener with requiredInterfaceType loopback
    Client->>TF: read token from file
    Client->>DAB: GET /health with Authorization header
    DAB->>DAB: isLoopbackEndpoint() + secureCompare()
    DAB-->>Client: 200 OK or 401 Unauthorized

Loading

_{Reviews (1): Last reviewed commit: "Add changelog entries for desktop securi..." | Re-trigger Greptile}

[greptile-apps]

Migration deletes UserDefaults token before confirming Keychain write

UserDefaults.removeObject(forKey: key) is called unconditionally, even when AuthKeychain.set() silently fails. If SecItemAdd fails (Keychain locked at first launch, ad-hoc signing on dev builds, Keychain corruption), the token is permanently gone from UserDefaults while absent from Keychain — causing a silent, unexplained sign-out the next time the app launches, for every user who upgrades.

The fix is to call AuthKeychain.get(key) after the write and only purge the UserDefaults copy when the read-back succeeds:

if let legacy = UserDefaults.standard.string(forKey: key), !legacy.isEmpty {
    AuthKeychain.set(legacy, forKey: key)
    if AuthKeychain.get(key) != nil {
        UserDefaults.standard.removeObject(forKey: key)
    } else {
        // Keychain write failed — keep the UserDefaults copy so the next
        // launch can retry migration instead of losing the session.
    }
    return legacy
}

[greptile-apps]

Brief world-readable window between atomic write and permission tightening

String.write(to:atomically:encoding:) creates a temp file with umask-derived permissions (typically 0644 with the default macOS umask of 022), renames it to the final path, and only then does setAttributes([.posixPermissions: 0o600]) narrow it to 0600. Any process running as the same macOS user can read the token file during this window.

The parent directory is correctly set to 0700, so processes owned by other users are already blocked. The risk is limited to other processes running as the same user watching this specific file. A more robust approach is to use a POSIX open() call with permissions set at creation time:

let fileMode: UInt16 = 0o600
let fd = open(url.path, O_CREAT | O_WRONLY | O_TRUNC, fileMode)
guard fd >= 0 else { throw CocoaError(.fileNoSuchFile) }
defer { close(fd) }
let tokenData = Data(token.utf8)
_ = tokenData.withUnsafeBytes { write(fd, $0.baseAddress!, $0.count) }

[greptile-apps]

clearAll() is unused; clearTokens() deletes keys individually

AuthKeychain.clearAll() deletes every Keychain item for the service in one call, but AuthService.clearTokens() calls AuthKeychain.set(nil, forKey:) twice (once per key). Both approaches work for the current two-key schema, but if a future commit adds a third Keychain key and only updates clearAll() (not clearTokens()), that key won't be purged on sign-out.

Consider either (a) calling AuthKeychain.clearAll() from clearTokens() and removing the individual set(nil, …) calls, or (b) removing clearAll() to eliminate the confusion. As-is, clearAll() is dead code.

[afurm]

set() function: SecItemDelete removes the existing keychain item before the guard check. If value is nil or encoding fails after the delete, the old token is permanently lost — the update is not atomic.

Fix: Move SecItemDelete inside the guard block, or use SecItemUpdate for atomic upsert.

[afurm]

clearAll() calls SecItemDelete but ignores the returned OSStatus. If deletion fails due to Keychain access restrictions, the function silently succeeds — the caller has no way to know tokens were not cleared, leaving stale credentials in place.

Fix: Check and log the status, or propagate an error to the caller.