From eb85ed58dfd6211dce94f6f2b5c28777218eed45 Mon Sep 17 00:00:00 2001 From: Savas Neto Date: Fri, 2 Oct 2026 17:31:58 -0300 Subject: [PATCH] fix(linux): close the "Secret Service read failed" login loop On Linux images without the secret-tool binary, every login ended in an endless re-login loop: the missing binary resolved as a vault error, and the fail-closed write path then refused to persist the fresh token anywhere. classifySecretToolLookup now names a missing binary for what it is (there is no vault on the machine, so nothing can be shadowed) and routes it to the documented plaintext fallback, while a vault that ran and failed stays an error. Regression-guarded by 6 new tests; isolated-suite, tsc and behavior probes (exit-2 shim stays fail-closed, ENOENT persists via fallback) all green. Co-Authored-By: Verboo Code --- .../secureStorage/linuxSecretStorage.test.ts | 57 +++++++++ src/utils/secureStorage/linuxSecretStorage.ts | 116 ++++++++++++------ 2 files changed, 135 insertions(+), 38 deletions(-) create mode 100644 src/utils/secureStorage/linuxSecretStorage.test.ts diff --git a/src/utils/secureStorage/linuxSecretStorage.test.ts b/src/utils/secureStorage/linuxSecretStorage.test.ts new file mode 100644 index 0000000000..602d8d8171 --- /dev/null +++ b/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) + }) +}) \ No newline at end of file diff --git a/src/utils/secureStorage/linuxSecretStorage.ts b/src/utils/secureStorage/linuxSecretStorage.ts index ffb2315c39..0ab84f2814 100644 --- a/src/utils/secureStorage/linuxSecretStorage.ts +++ b/src/utils/secureStorage/linuxSecretStorage.ts @@ -30,6 +30,60 @@ 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.). @@ -37,51 +91,33 @@ function decodePayload(payload: string): SecureStorageData { 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 { // 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 }