fix(linux): close the "Secret Service read failed" login loop

src/utils/secureStorage/linuxSecretStorage.test.ts

@@ -0,0 +1,57 @@
+// NOTE: these tests exercise the real secret-tool spawn, so they must run in
+// their own process (CI does this via `bun run test:isolated`, one file per
+// process). platformStorage.test.ts installs a process-global mock.module("execa")
+// that would shadow the real spawn if both files ran in one process.
+import { afterAll, describe, expect, test } from 'bun:test'
+import { mkdtempSync } from 'node:fs'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
+import {
+  classifySecretToolLookup,
+  linuxSecretStorage,
+} from './linuxSecretStorage.js'
+
+describe('classifySecretToolLookup', () => {
+  test('a spawn failure means the vault cannot exist: missing, not error', () => {
+    // Regression guard: this used to classify as `error`, which made the
+    // fail-closed write path refuse to persist fresh login tokens and left
+    // customers without secret-tool in an endless re-login loop.
+    expect(classifySecretToolLookup({ spawnFailed: true, exitCode: null })).toBe('missing')
+  })
+
+  test('exit 0 is a successful lookup', () => {
+    expect(classifySecretToolLookup({ spawnFailed: false, exitCode: 0 })).toBe('ok')
+  })
+
+  test('exit 1 is a missing item (secret-tool convention)', () => {
+    expect(classifySecretToolLookup({ spawnFailed: false, exitCode: 1 })).toBe('missing')
+  })
+
+  test('any other exit code is a vault/service failure', () => {
+    // A native record may still exist: this must stay `error` so a fallback
+    // write never shadows it.
+    expect(classifySecretToolLookup({ spawnFailed: false, exitCode: 2 })).toBe('vault-error')
+    expect(classifySecretToolLookup({ spawnFailed: false, exitCode: 127 })).toBe('vault-error')
+  })
+})
+
+describe('linuxSecretStorage without the secret-tool binary', () => {
+  const originalPath = process.env.PATH
+  const emptyDir = mkdtempSync(join(tmpdir(), 'no-secret-tool-'))
+
+  afterAll(() => {
+    process.env.PATH = originalPath
+  })
+
+  test('readResult is missing (not error) when secret-tool is absent', () => {
+    process.env.PATH = emptyDir
+    const result = linuxSecretStorage.readResult?.()
+    expect(result).toEqual({ kind: 'missing' })
+  })
+
+  test('update fails cleanly so the fallback storage takes the write', () => {
+    process.env.PATH = emptyDir
+    const result = linuxSecretStorage.update({})
+    expect(result.success).toBe(false)
+  })
+})

src/utils/secureStorage/linuxSecretStorage.ts

@@ -30,58 +30,94 @@ function decodePayload(payload: string): SecureStorageData {
   return jsonParse(json)
 }
 
+type SecretToolLookupRun =
+  | { spawnFailed: true; exitCode: null; stdout: ''; stderr: '' }
+  | { spawnFailed: false; exitCode: number; stdout: string; stderr: string }
+
+/**
+ * Classifies the outcome of a `secret-tool lookup` invocation.
+ *
+ * A spawn failure means the secret-tool binary is not installed: there is no
+ * vault on this machine, so no native record exists and none can be shadowed.
+ * It is classified as `missing` so the documented plaintext fallback engages.
+ * It used to be an `error`, and the fail-closed path in
+ * `preserveProviderAccountsOnSharedWrites` (index.ts) then refused to persist
+ * a freshly obtained login token anywhere — customers on bare Linux images
+ * (no libsecret-tools) experienced an endless re-login loop reporting
+ * "Secret Service read failed.".
+ *
+ * A tool that ran but whose vault/service failed keeps the `error`
+ * classification: a native record may still exist, and a fallback write must
+ * not shadow it (see the delete-on-migration logic in fallbackStorage.ts).
+ */
+export function classifySecretToolLookup(outcome: {
+  spawnFailed: boolean
+  exitCode: number | null
+}): 'ok' | 'missing' | 'vault-error' {
+  if (outcome.spawnFailed) return 'missing'
+  if (outcome.exitCode === 0) return 'ok'
+  if (outcome.exitCode === 1) return 'missing'
+  return 'vault-error'
+}
+
+function runSecretToolLookup(serviceName: string, username: string): SecretToolLookupRun {
+  try {
+    const result = execaSync(
+      'secret-tool',
+      ['lookup', 'service', serviceName, 'account', username],
+      { reject: false },
+    )
+    // With reject:false, a missing binary RESOLVES (it does not throw) with
+    // failed:true, code 'ENOENT' and an undefined exit code. A regular
+    // non-zero exit (e.g. 1 for a missing item) always carries a number.
+    if (typeof result.exitCode !== 'number') {
+      return { spawnFailed: true, exitCode: null, stdout: '', stderr: '' }
+    }
+    return {
+      spawnFailed: false,
+      exitCode: result.exitCode,
+      stdout: result.stdout ?? '',
+      stderr: result.stderr ?? '',
+    }
+  } catch {
+    return { spawnFailed: true, exitCode: null, stdout: '', stderr: '' }
+  }
+}
+
 /**
  * Linux-specific secure storage implementation using the secret-tool CLI.
  * secret-tool interacts with the Secret Service API (GNOME Keyring, KWallet, etc.).
  */
 export const linuxSecretStorage: SecureStorage = {
   name: 'libsecret',
   read(): SecureStorageData | null {
-    try {
-      const username = getUsername()
-      const serviceName = getSecureStorageServiceName(
-        CREDENTIALS_SERVICE_SUFFIX,
-      )
-      // secret-tool lookup service [service] account [account]
-      const result = execaSync(
-        'secret-tool',
-        ['lookup', 'service', serviceName, 'account', username],
-        { reject: false },
-      )
-
-      if (result.exitCode === 0 && result.stdout) {
-        return decodePayload(result.stdout)
+    const serviceName = getSecureStorageServiceName(CREDENTIALS_SERVICE_SUFFIX)
+    const run = runSecretToolLookup(serviceName, getUsername())
+    if (!run.spawnFailed && run.exitCode === 0 && run.stdout) {
+      try {
+        return decodePayload(run.stdout)
+      } catch {
+        // fall through
       }
-    } catch {
-      // fall through
     }
     return null
   },
   readResult(): SecureStorageReadResult {
-    try {
-      const username = getUsername()
-      const serviceName = getSecureStorageServiceName(
-        CREDENTIALS_SERVICE_SUFFIX,
-      )
-      const result = execaSync(
-        'secret-tool',
-        ['lookup', 'service', serviceName, 'account', username],
-        { reject: false },
-      )
-      if (result.exitCode === 0 && result.stdout) {
-        try {
-          return { kind: 'ok', data: decodePayload(result.stdout) }
-        } catch {
-          return { kind: 'error', warning: 'Secret Service returned malformed JSON.' }
-        }
+    const serviceName = getSecureStorageServiceName(CREDENTIALS_SERVICE_SUFFIX)
+    const run = runSecretToolLookup(serviceName, getUsername())
+    const classification = classifySecretToolLookup(run)
+    if (classification === 'ok') {
+      if (!run.stdout) {
+        return { kind: 'error', warning: 'Secret Service read failed.' }
+      }
+      try {
+        return { kind: 'ok', data: decodePayload(run.stdout) }
+      } catch {
+        return { kind: 'error', warning: 'Secret Service returned malformed JSON.' }
       }
-      // secret-tool uses exit code 1 for a missing item. A thrown spawn (for
-      // example, secret-tool is not installed) is handled as an error below.
-      if (result.exitCode === 1) return { kind: 'missing' }
-      return { kind: 'error', warning: result.stderr?.trim() || 'Secret Service read failed.' }
-    } catch {
-      return { kind: 'error', warning: 'Secret Service read failed.' }
     }
+    if (classification === 'missing') return { kind: 'missing' }
+    return { kind: 'error', warning: run.stderr.trim() || 'Secret Service read failed.' }
   },
   async readAsync(): Promise<SecureStorageData | null> {
     // Reusing sync implementation for simplicity as it wraps a CLI call
@@ -116,6 +152,10 @@ export const linuxSecretStorage: SecureStorage = {
         { input: payload, reject: false },
       )
 
+      // A missing secret-tool binary resolves with failed:true and an
+      // undefined exit code (see runSecretToolLookup), so this returns
+      // success:false and fallbackStorage.update routes the write to the
+      // plaintext fallback.
       return { success: result.exitCode === 0 }
     } catch {
       return { success: false }