fix: harden execFileNoThrow for CodeQL (#338)

bun.lock

@@ -36,6 +36,7 @@
         "cli-highlight": "2.1.11",
         "code-excerpt": "4.0.0",
         "commander": "12.1.0",
+        "cross-spawn": "7.0.6",
         "diff": "8.0.3",
         "duck-duck-scrape": "^2.2.7",
         "emoji-regex": "10.6.0",

package.json

@@ -76,6 +76,7 @@
     "cli-highlight": "2.1.11",
     "code-excerpt": "4.0.0",
     "commander": "12.1.0",
+    "cross-spawn": "7.0.6",
     "diff": "8.0.3",
     "duck-duck-scrape": "^2.2.7",
     "emoji-regex": "10.6.0",

src/utils/execFileNoThrow.test.ts

@@ -1,4 +1,7 @@
 import { expect, test } from 'bun:test'
+import { mkdtempSync, writeFileSync } from 'node:fs'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
 import { execFileNoThrowWithCwd } from './execFileNoThrow.js'
 
 test('execFileNoThrowWithCwd rejects shell-like executable names', async () => {
@@ -18,8 +21,37 @@ test('execFileNoThrowWithCwd rejects cwd values with control characters', async
 })
 
 test('execFileNoThrowWithCwd rejects arguments with control characters', async () => {
-  const result = await execFileNoThrowWithCwd(process.execPath, ['--version\nmalicious'])
+  const result = await execFileNoThrowWithCwd(process.execPath, [
+    '--version\nmalicious',
+  ])
 
   expect(result.code).toBe(1)
   expect(result.error).toContain('Unsafe argument')
 })
+
+test('execFileNoThrowWithCwd rejects environment entries with control characters', async () => {
+  const result = await execFileNoThrowWithCwd(process.execPath, ['--version'], {
+    env: {
+      ...process.env,
+      BAD_ENV: 'line1\nline2',
+    },
+  })
+
+  expect(result.code).toBe(1)
+  expect(result.error).toContain('Unsafe environment')
+})
+
+test('execFileNoThrowWithCwd preserves Windows .cmd compatibility', async () => {
+  if (process.platform !== 'win32') {
+    return
+  }
+
+  const dir = mkdtempSync(join(tmpdir(), 'openclaude-execfile-'))
+  const file = join(dir, 'hello.cmd')
+  writeFileSync(file, '@echo off\r\necho hello\r\n')
+
+  const result = await execFileNoThrowWithCwd(file, [])
+
+  expect(result.code).toBe(0)
+  expect(result.stdout).toContain('hello')
+})

src/utils/execFileNoThrow.ts

@@ -1,8 +1,9 @@
 // This file represents useful wrappers over node:child_process
-// These wrappers ease error handling and cross-platform compatbility
+// These wrappers ease error handling and cross-platform compatibility.
-// By using execa, Windows automatically gets shell escaping + BAT / CMD handling
+// By using cross-spawn, Windows gets .cmd/.bat compatibility without falling
+// back to a generic shell command string.
 
-import { type ExecaError, execa } from 'execa'
+import { spawn } from 'cross-spawn'
 import path from 'node:path'
 import { getCwd } from '../utils/cwd.js'
 import { logError } from './log.js'
@@ -11,6 +12,7 @@ export { execSyncWithDefaults_DEPRECATED } from './execFileNoThrowPortable.js'
 
 const MS_IN_SECOND = 1000
 const SECONDS_IN_MINUTE = 60
+const DEFAULT_MAX_BUFFER = 1_000_000
 
 type ExecFileOptions = {
   abortSignal?: AbortSignal
@@ -24,26 +26,6 @@ type ExecFileOptions = {
   input?: string
 }
 
-export function execFileNoThrow(
-  file: string,
-  args: string[],
-  options: ExecFileOptions = {
-    timeout: 10 * SECONDS_IN_MINUTE * MS_IN_SECOND,
-    preserveOutputOnError: true,
-    useCwd: true,
-  },
-): Promise<{ stdout: string; stderr: string; code: number; error?: string }> {
-  return execFileNoThrowWithCwd(file, args, {
-    abortSignal: options.abortSignal,
-    timeout: options.timeout,
-    preserveOutputOnError: options.preserveOutputOnError,
-    cwd: options.useCwd ? getCwd() : undefined,
-    env: options.env,
-    stdin: options.stdin,
-    input: options.input,
-  })
-}
-
 type ExecFileWithCwdOptions = {
   abortSignal?: AbortSignal
   timeout?: number
@@ -55,8 +37,7 @@ type ExecFileWithCwdOptions = {
   input?: string
 }
 
-type ExecaResultWithError = {
+type ProcessResultWithError = {
-  shortMessage?: string
   signal?: string
 }
 
@@ -64,7 +45,11 @@ const CONTROL_CHAR_PATTERN = /[\0\r\n]/
 const SAFE_BARE_EXECUTABLE_PATTERN = /^[A-Za-z0-9_.-]+$/
 
 function hasPathSyntax(value: string): boolean {
-  return value.includes(path.sep) || value.includes('/') || path.isAbsolute(value)
+  return (
+    value.includes(path.sep) ||
+    value.includes('/') ||
+    path.isAbsolute(value)
+  )
 }
 
 function validateExecutable(file: string): string | null {
@@ -75,7 +60,10 @@ function validateExecutable(file: string): string | null {
   if (CONTROL_CHAR_PATTERN.test(normalized)) {
     return 'Unsafe executable: control characters are not allowed'
   }
-  if (!hasPathSyntax(normalized) && !SAFE_BARE_EXECUTABLE_PATTERN.test(normalized)) {
+  if (
+    !hasPathSyntax(normalized) &&
+    !SAFE_BARE_EXECUTABLE_PATTERN.test(normalized)
+  ) {
     return 'Unsafe executable: bare command names may only contain letters, numbers, ".", "_" and "-"'
   }
   return null
@@ -100,29 +88,67 @@ function validateWorkingDirectory(cwd: string | undefined): string | null {
   return null
 }
 
+function sanitizeEnvironment(
+  env: NodeJS.ProcessEnv | undefined,
+): { value?: NodeJS.ProcessEnv; error?: string } {
+  if (!env) {
+    return {}
+  }
+
+  for (const [key, value] of Object.entries(env)) {
+    if (CONTROL_CHAR_PATTERN.test(key)) {
+      return {
+        error: 'Unsafe environment: control characters are not allowed in keys',
+      }
+    }
+    if (typeof value === 'string' && CONTROL_CHAR_PATTERN.test(value)) {
+      return {
+        error:
+          'Unsafe environment: control characters are not allowed in values',
+      }
+    }
+  }
+
+  return { value: env }
+}
+
 /**
- * Extracts a human-readable error message from an execa result.
+ * Extracts a human-readable error message from a process result.
  *
  * Priority order:
- * 1. shortMessage - execa's human-readable error (e.g., "Command failed with exit code 1: ...")
+ * 1. signal - the signal that killed the process (e.g., "SIGTERM")
- *    This is preferred because it already includes signal info when a process is killed,
+ * 2. errorCode - fallback to just the numeric exit code
- *    making it more informative than just the signal name.
- * 2. signal - the signal that killed the process (e.g., "SIGTERM")
- * 3. errorCode - fallback to just the numeric exit code
  */
 function getErrorMessage(
-  result: ExecaResultWithError,
+  result: ProcessResultWithError,
   errorCode: number,
 ): string {
-  if (result.shortMessage) {
-    return result.shortMessage
-  }
   if (typeof result.signal === 'string') {
     return result.signal
   }
   return String(errorCode)
 }
 
+export function execFileNoThrow(
+  file: string,
+  args: string[],
+  options: ExecFileOptions = {
+    timeout: 10 * SECONDS_IN_MINUTE * MS_IN_SECOND,
+    preserveOutputOnError: true,
+    useCwd: true,
+  },
+): Promise<{ stdout: string; stderr: string; code: number; error?: string }> {
+  return execFileNoThrowWithCwd(file, args, {
+    abortSignal: options.abortSignal,
+    timeout: options.timeout,
+    preserveOutputOnError: options.preserveOutputOnError,
+    cwd: options.useCwd ? getCwd() : undefined,
+    env: options.env,
+    stdin: options.stdin,
+    input: options.input,
+  })
+}
+
 /**
  * execFile, but always resolves (never throws)
  */
@@ -135,70 +161,172 @@ export function execFileNoThrowWithCwd(
     preserveOutputOnError: finalPreserveOutput = true,
     cwd: finalCwd,
     env: finalEnv,
-    maxBuffer,
+    maxBuffer = DEFAULT_MAX_BUFFER,
     stdin: finalStdin,
     input: finalInput,
   }: ExecFileWithCwdOptions = {
     timeout: 10 * SECONDS_IN_MINUTE * MS_IN_SECOND,
     preserveOutputOnError: true,
-    maxBuffer: 1_000_000,
+    maxBuffer: DEFAULT_MAX_BUFFER,
   },
 ): Promise<{ stdout: string; stderr: string; code: number; error?: string }> {
   const executableError = validateExecutable(file)
   if (executableError) {
-    return Promise.resolve({ stdout: '', stderr: '', code: 1, error: executableError })
+    return Promise.resolve({
+      stdout: '',
+      stderr: '',
+      code: 1,
+      error: executableError,
+    })
   }
 
   const argsError = validateArgs(args)
   if (argsError) {
-    return Promise.resolve({ stdout: '', stderr: '', code: 1, error: argsError })
+    return Promise.resolve({
+      stdout: '',
+      stderr: '',
+      code: 1,
+      error: argsError,
+    })
   }
 
   const cwdError = validateWorkingDirectory(finalCwd)
   if (cwdError) {
-    return Promise.resolve({ stdout: '', stderr: '', code: 1, error: cwdError })
+    return Promise.resolve({
+      stdout: '',
+      stderr: '',
+      code: 1,
+      error: cwdError,
+    })
+  }
+
+  const sanitizedEnv = sanitizeEnvironment(finalEnv)
+  if (sanitizedEnv.error) {
+    return Promise.resolve({
+      stdout: '',
+      stderr: '',
+      code: 1,
+      error: sanitizedEnv.error,
+    })
   }
 
   return new Promise(resolve => {
-    // Use execa for cross-platform .bat/.cmd compatibility on Windows
+    const stdinMode = finalInput !== undefined ? 'pipe' : finalStdin ?? 'pipe'
-    execa(file, args, {
+    const child = spawn(file, args, {
-      maxBuffer,
-      cancelSignal: abortSignal,
-      timeout: finalTimeout,
       cwd: finalCwd,
-      env: finalEnv,
+      env: sanitizedEnv.value,
       shell: false,
-      stdin: finalStdin,
+      signal: abortSignal,
-      input: finalInput,
+      stdio: [stdinMode, 'pipe', 'pipe'],
-      reject: false, // Don't throw on non-zero exit codes
     })
-      .then(result => {
+
-        if (result.failed) {
+    let settled = false
-          if (finalPreserveOutput) {
+    let stdout = ''
-            const errorCode = result.exitCode ?? 1
+    let stderr = ''
-            void resolve({
+    let combinedBufferSize = 0
-              stdout: result.stdout || '',
+    let signal: string | undefined
-              stderr: result.stderr || '',
+    let timedOut = false
-              code: errorCode,
+
-              error: getErrorMessage(
+    const finish = (result: {
-                result as unknown as ExecaResultWithError,
+      stdout: string
-                errorCode,
+      stderr: string
-              ),
+      code: number
-            })
+      error?: string
-          } else {
+    }) => {
-            void resolve({ stdout: '', stderr: '', code: result.exitCode ?? 1 })
+      if (settled) {
-          }
+        return
+      }
+      settled = true
+      void resolve(result)
+    }
+
+    const appendOutput = (
+      chunk: string | Buffer,
+      target: 'stdout' | 'stderr',
+    ) => {
+      const text = typeof chunk === 'string' ? chunk : chunk.toString('utf8')
+      combinedBufferSize += Buffer.byteLength(text, 'utf8')
+      if (combinedBufferSize > maxBuffer) {
+        child.kill()
+        finish({
+          stdout: finalPreserveOutput ? stdout : '',
+          stderr: finalPreserveOutput ? stderr : '',
+          code: 1,
+          error: 'maxBuffer exceeded',
+        })
+        return
+      }
+
+      if (target === 'stdout') {
+        stdout += text
+      } else {
+        stderr += text
+      }
+    }
+
+    child.stdout?.on('data', chunk => appendOutput(chunk, 'stdout'))
+    child.stderr?.on('data', chunk => appendOutput(chunk, 'stderr'))
+
+    child.once('spawn', () => {
+      if (stdinMode === 'pipe' && child.stdin) {
+        if (finalInput !== undefined) {
+          child.stdin.end(finalInput)
         } else {
-          void resolve({
+          child.stdin.end()
-            stdout: result.stdout,
-            stderr: result.stderr,
-            code: 0,
-          })
         }
+      }
+    })
+
+    child.once('error', error => {
+      logError(error)
+      finish({ stdout: '', stderr: '', code: 1, error: error.message })
+    })
+
+    const timeoutId =
+      finalTimeout > 0
+        ? setTimeout(() => {
+            timedOut = true
+            child.kill()
+          }, finalTimeout)
+        : undefined
+
+    child.once('close', (code, closeSignal) => {
+      if (timeoutId) {
+        clearTimeout(timeoutId)
+      }
+
+      signal = closeSignal ?? undefined
+      const errorCode = code ?? 1
+
+      if (timedOut) {
+        finish({
+          stdout: finalPreserveOutput ? stdout : '',
+          stderr: finalPreserveOutput ? stderr : '',
+          code: errorCode,
+          error: `Command timed out after ${finalTimeout}ms`,
+        })
+        return
+      }
+
+      if (errorCode !== 0) {
+        if (finalPreserveOutput) {
+          finish({
+            stdout,
+            stderr,
+            code: errorCode,
+            error: getErrorMessage({ signal }, errorCode),
+          })
+        } else {
+          finish({ stdout: '', stderr: '', code: errorCode })
+        }
+        return
+      }
+
+      finish({
+        stdout,
+        stderr,
+        code: 0,
       })
-      .catch((error: ExecaError) => {
+    })
-        logError(error)
-        void resolve({ stdout: '', stderr: '', code: 1 })
-      })
   })
 }