fix(secrets): don't double-consume keychain CLI streams (Buffer.concat crash)

jdalton · jdalton · commit 4df603979ac9 · 2026-06-09T02:01:52.000-04:00
macos/linux/windows runners attached .setEncoding('utf8') + .on('data')
to streams the lib spawn was concurrently buffering as Buffers, so its
Buffer.concat-on-close threw 'TypeError: list[0] must be a Buffer' when a
chunk arrived as a string (the security/secret-tool/PowerShell error
output triggered it on macos). Await the lib spawn's result instead
(stdioString → string stdout/stderr; reject on non-zero carries
{code,stdout,stderr}); write secrets to stdin via the returned process.
Fixes the macos secrets/keychain suite (was 8/20) + the latent linux +
windows variants.
diff --git a/src/secrets/linux.ts b/src/secrets/linux.ts
@@ -59,27 +59,22 @@ export async function readLinux(
   service: string,
   account: string,
 ): Promise<string | undefined> {
-  return new PromiseCtor(resolve => {
-    const { process: cp } = spawn(
+  // Let the lib spawn own output buffering (stdioString → string stdout). Don't
+  // attach `.setEncoding('utf8')` + `.on('data')` — that flips the stream to
+  // string chunks while the lib buffers it as Buffers + does Buffer.concat on
+  // close, throwing `TypeError: list[0] must be a Buffer`. The lib rejects on a
+  // non-zero exit (a missing entry), which is just "undefined" here.
+  try {
+    const r = await spawn(
       SECRET_TOOL_BIN,
       ['lookup', 'service', service, 'user', account],
-      { stdio: ['ignore', 'pipe', 'pipe'] },
+      { stdio: ['ignore', 'pipe', 'pipe'], stdioString: true },
     )
-    let stdout = ''
-    cp.stdout!.setEncoding('utf8')
-    cp.stdout!.on('data', chunk => {
-      stdout += chunk
-    })
-    cp.on('error', () => resolve(undefined))
-    cp.on('close', status => {
-      if (status !== 0) {
-        resolve(undefined)
-        return
-      }
-      const out = stdout.trim()
-      resolve(out || undefined)
-    })
-  })
+    const out = String(r.stdout ?? '').trim()
+    return out || undefined
+  } catch {
+    return undefined
+  }
 }
 
 export function readLinuxSync(
@@ -104,43 +99,37 @@ export async function writeLinux(
   value: string,
   label: string,
 ): Promise<void> {
-  return new PromiseCtor((resolve, reject) => {
-    const { process: cp } = spawn(
-      SECRET_TOOL_BIN,
-      ['store', `--label=${label}`, 'service', service, 'user', account],
-      { stdio: ['pipe', 'pipe', 'pipe'] },
-    )
-    let stderr = ''
-    cp.stderr!.setEncoding('utf8')
-    cp.stderr!.on('data', chunk => {
-      stderr += chunk
-    })
-    cp.on('error', err =>
-      reject(
-        new Error(
-          `secret-tool store failed: ${err.message}. ` +
-            'Install libsecret-tools (apt install libsecret-tools / dnf install libsecret) ' +
-            'or ensure a Secret Service provider (gnome-keyring, kwallet) is running.',
-        ),
-      ),
+  const hint =
+    'Install libsecret-tools (apt install libsecret-tools / dnf install libsecret) ' +
+    'or ensure a Secret Service provider (gnome-keyring, kwallet) is running.'
+  // Let the lib spawn own output buffering (stdioString); write the value to
+  // stdin via the returned `process` so it stays out of the process listing
+  // (`ps`) and the kernel argv view. Don't attach `.setEncoding`/`.on('data')`
+  // — that races the lib's Buffer.concat-on-close (see readLinux). The lib
+  // rejects on a non-zero exit with `{ code, stderr }`; re-throw it as the
+  // helpful error.
+  const child = spawn(
+    SECRET_TOOL_BIN,
+    ['store', `--label=${label}`, 'service', service, 'user', account],
+    { stdio: ['pipe', 'pipe', 'pipe'], stdioString: true },
+  )
+  child.process.stdin!.end(value)
+  try {
+    await child
+  } catch (e) {
+    const err = e as
+      | {
+          code?: number | undefined
+          stderr?: unknown | undefined
+          message?: string | undefined
+        }
+      | undefined
+    const status = typeof err?.code === 'number' ? err.code : -1
+    const stderr = String(err?.stderr ?? err?.message ?? '').trim()
+    throw new ErrorCtor(
+      `secret-tool store failed (status=${status}, user=${account}): ${stderr}. ${hint}`,
     )
-    cp.on('close', status => {
-      if (status === 0) {
-        resolve()
-        return
-      }
-      reject(
-        new Error(
-          `secret-tool store failed (status=${status}, user=${account}): ${stderr.trim()}. ` +
-            'Install libsecret-tools (apt install libsecret-tools / dnf install libsecret) ' +
-            'or ensure a Secret Service provider (gnome-keyring, kwallet) is running.',
-        ),
-      )
-    })
-    // `secret-tool store` reads the password from stdin so the value
-    // never appears in `ps(1)` / `/proc/<pid>/cmdline`.
-    cp.stdin!.end(value)
-  })
+  }
 }
 
 export function writeLinuxSync(
diff --git a/src/secrets/macos.ts b/src/secrets/macos.ts
@@ -26,8 +26,6 @@ import {
 
 import { ErrorCtor } from '../primordials/error'
 
-import { PromiseCtor } from '../primordials/promise'
-
 const SECURITY_BIN = 'security'
 
 export async function deleteMacOS(
@@ -102,35 +100,50 @@ export interface SpawnOpts {
   stdio?: 'ignore' | 'pipe' | ['ignore', 'pipe', 'pipe'] | undefined
 }
 
-export function runAsync(
+export async function runAsync(
   args: readonly string[],
   opts: SpawnOpts = {},
 ): Promise<{
   status: number | null
   stdout: string
   stderr: string
 }> {
-  return new PromiseCtor(resolve => {
-    const { process: cp } = spawn(SECURITY_BIN, args as string[], {
-      stdio: opts.stdio ?? ['ignore', 'pipe', 'pipe'],
-    })
-    let stdout = ''
-    let stderr = ''
-    if (cp.stdout) {
-      cp.stdout.setEncoding('utf8')
-      cp.stdout.on('data', chunk => {
-        stdout += chunk
-      })
+  // Let the lib's spawn own output buffering. It collects stdout/stderr as
+  // Buffer chunks and returns them as strings (stdioString) on the awaited
+  // result. Do NOT also attach `.setEncoding('utf8')` + `.on('data')` here:
+  // setEncoding flips the stream to emit STRING chunks, but the lib is
+  // concurrently buffering the same stream as Buffers and does
+  // `Buffer.concat(chunks)` on close — a string in that array throws
+  // `TypeError: list[0] argument must be an instance of Buffer` (the
+  // `security` CLI's "security: SecKeychainSearch…" stderr was the trigger).
+  // The lib REJECTS on a non-zero exit, so a failed `security` call (e.g. a
+  // missing entry) arrives in the catch with its `{ code, stdout, stderr }`.
+  const child = spawn(SECURITY_BIN, args as string[], {
+    stdio: opts.stdio ?? ['ignore', 'pipe', 'pipe'],
+    stdioString: true,
+  })
+  try {
+    const r = await child
+    return {
+      // oxlint-disable-next-line socket/prefer-undefined-over-null -- the return contract is `status: number | null` (matches Node's ChildProcess exit-code shape); callers branch on `=== 0`.
+      status: typeof r.code === 'number' ? r.code : null,
+      stderr: String(r.stderr ?? ''),
+      stdout: String(r.stdout ?? ''),
     }
-    if (cp.stderr) {
-      cp.stderr.setEncoding('utf8')
-      cp.stderr.on('data', chunk => {
-        stderr += chunk
-      })
+  } catch (e) {
+    const err = e as
+      | {
+          code?: number | undefined
+          stdout?: unknown | undefined
+          stderr?: unknown | undefined
+        }
+      | undefined
+    return {
+      status: typeof err?.code === 'number' ? err.code : -1,
+      stderr: String(err?.stderr ?? ''),
+      stdout: String(err?.stdout ?? ''),
     }
-    cp.on('error', () => resolve({ status: -1, stdout, stderr }))
-    cp.on('close', status => resolve({ status, stdout, stderr }))
-  })
+  }
 }
 
 export async function writeMacOS(
diff --git a/src/secrets/windows.ts b/src/secrets/windows.ts
@@ -30,8 +30,6 @@ import { ErrorCtor } from '../primordials/error'
 
 import { JSONStringify } from '../primordials/json'
 
-import { PromiseCtor } from '../primordials/promise'
-
 const POWERSHELL_BIN = 'powershell'
 
 export function buildTarget(service: string, account: string): string {
@@ -195,40 +193,46 @@ export function readWindowsSync(
   return readDpapiSync(getDpapiFilePath(service, account))
 }
 
-export function runPsAsync(
+export async function runPsAsync(
   script: string,
   input?: string,
 ): Promise<{
   status: number | null
   stdout: string
   stderr: string
 }> {
-  return new PromiseCtor(resolve => {
-    const { process: cp } = spawn(
-      POWERSHELL_BIN,
-      ['-NoProfile', '-Command', script],
-      {
-        stdio: ['pipe', 'pipe', 'pipe'],
-      },
-    )
-    let stdout = ''
-    let stderr = ''
-    cp.stdout!.setEncoding('utf8')
-    cp.stdout!.on('data', chunk => {
-      stdout += chunk
-    })
-    cp.stderr!.setEncoding('utf8')
-    cp.stderr!.on('data', chunk => {
-      stderr += chunk
-    })
-    cp.on('error', () => resolve({ status: -1, stdout, stderr }))
-    cp.on('close', status => resolve({ status, stdout, stderr }))
-    if (input !== undefined) {
-      cp.stdin!.end(input)
-    } else {
-      cp.stdin!.end()
-    }
+  // Let the lib spawn own output buffering (stdioString → string stdout/
+  // stderr). Don't attach `.setEncoding('utf8')` + `.on('data')`: that flips
+  // the streams to string chunks while the lib buffers them as Buffers + does
+  // Buffer.concat on close, throwing `TypeError: list[0] must be a Buffer`.
+  // The lib rejects on a non-zero exit; capture its `{ code, stdout, stderr }`.
+  const child = spawn(POWERSHELL_BIN, ['-NoProfile', '-Command', script], {
+    stdio: ['pipe', 'pipe', 'pipe'],
+    stdioString: true,
   })
+  child.process.stdin!.end(input ?? '')
+  try {
+    const r = await child
+    return {
+      // oxlint-disable-next-line socket/prefer-undefined-over-null -- the return contract is `status: number | null` (matches Node's ChildProcess exit-code shape); callers branch on `=== 0`.
+      status: typeof r.code === 'number' ? r.code : null,
+      stderr: String(r.stderr ?? ''),
+      stdout: String(r.stdout ?? ''),
+    }
+  } catch (e) {
+    const err = e as
+      | {
+          code?: number | undefined
+          stdout?: unknown | undefined
+          stderr?: unknown | undefined
+        }
+      | undefined
+    return {
+      status: typeof err?.code === 'number' ? err.code : -1,
+      stderr: String(err?.stderr ?? ''),
+      stdout: String(err?.stdout ?? ''),
+    }
+  }
 }
 
 export function runPsSync(
