Merge pull request #1610 from kaitranntt/kai/fix/1608-launch-settings-hardening

fix(cliproxy): harden launch-settings overlay (cleanup-on-throw, coverage, naming)

src/cliproxy/executor/__tests__/claude-launcher.test.ts

@@ -87,6 +87,16 @@ mock.module('../../quota/quota-manager', () => ({
   stopQuotaMonitor: jest.fn(),
 }));
 
+const mockCleanupLaunchSettings = jest.fn();
+const mockPrepareLaunchSettings = jest.fn().mockReturnValue({
+  settingsPath: '/tmp/fake-settings-overlay.json',
+  cleanup: mockCleanupLaunchSettings,
+});
+
+mock.module('../launch-settings', () => ({
+  prepareLaunchSettings: mockPrepareLaunchSettings,
+}));
+
 // ── Subject under test ────────────────────────────────────────────────────────
 
 import { launchClaude } from '../claude-launcher';
@@ -131,6 +141,12 @@ describe('launchClaude', () => {
     mockSpawn.mockClear();
     mockSetupCleanupHandlers.mockClear();
     mockEscapeShellArg.mockClear();
+    mockCleanupLaunchSettings.mockClear();
+    mockPrepareLaunchSettings.mockClear();
+    mockPrepareLaunchSettings.mockReturnValue({
+      settingsPath: '/tmp/fake-settings-overlay.json',
+      cleanup: mockCleanupLaunchSettings,
+    });
   });
 
   it('calls spawn with claudeCli and includes --settings arg', async () => {
@@ -189,6 +205,16 @@ describe('launchClaude', () => {
     expect(result).toBe(mockSpawnResult);
   });
 
+  it('calls cleanup and rethrows when spawn throws synchronously', async () => {
+    const spawnErr = new Error('ERR_INVALID_ARG_VALUE');
+    mockSpawn.mockImplementationOnce(() => {
+      throw spawnErr;
+    });
+
+    await expect(launchClaude(baseContext())).rejects.toThrow('ERR_INVALID_ARG_VALUE');
+    expect(mockCleanupLaunchSettings).toHaveBeenCalledTimes(1);
+  });
+
   describe('Windows shell escaping', () => {
     const originalPlatform = process.platform;
 

src/cliproxy/executor/__tests__/launch-settings.test.ts

@@ -91,6 +91,17 @@ describe('buildLaunchSettingsOverlay', () => {
       'http://127.0.0.1:50118/api/provider/codex'
     );
   });
+
+  it('falls back to an env-only overlay when the settings file is corrupt', () => {
+    fs.writeFileSync(settingsPath, '{ not valid json');
+    const { settings, changed } = buildLaunchSettingsOverlay(settingsPath, {
+      ANTHROPIC_BASE_URL: 'http://127.0.0.1:50118/api/provider/codex',
+    } as NodeJS.ProcessEnv);
+    expect(changed).toBe(true);
+    expect((settings.env as Record<string, string>).ANTHROPIC_BASE_URL).toBe(
+      'http://127.0.0.1:50118/api/provider/codex'
+    );
+  });
 });
 
 describe('prepareLaunchSettings', () => {
@@ -127,4 +138,20 @@ describe('prepareLaunchSettings', () => {
     result.cleanup();
     expect(fs.existsSync(settingsPath)).toBe(true);
   });
+
+  it('writes the overlay with 0600 file mode inside a 0700 dir (POSIX)', () => {
+    if (process.platform === 'win32') return; // chmod modes are not enforced on Windows
+    writePersisted({
+      env: { ANTHROPIC_BASE_URL: 'http://127.0.0.1:8317/api/provider/codex' },
+    });
+    const result = prepareLaunchSettings(settingsPath, {
+      ANTHROPIC_BASE_URL: 'http://127.0.0.1:50118/api/provider/codex',
+    } as NodeJS.ProcessEnv);
+    try {
+      expect(fs.statSync(result.settingsPath).mode & 0o777).toBe(0o600);
+      expect(fs.statSync(path.dirname(result.settingsPath)).mode & 0o777).toBe(0o700);
+    } finally {
+      result.cleanup();
+    }
+  });
 });

src/cliproxy/executor/claude-launcher.ts

@@ -144,20 +144,28 @@ export async function launchClaude(context: ClaudeLaunchContext): Promise<ChildP
 
   // Spawn: Windows .cmd/.bat/.ps1 need shell escaping; all others spawn directly
   let claude: ChildProcess;
-  if (needsShell) {
-    const cmdString = [claudeCli, ...launchArgs].map(escapeShellArg).join(' ');
-    claude = spawn(cmdString, {
-      stdio: 'inherit',
-      windowsHide: true,
-      shell: getWindowsEscapedCommandShell(),
-      env: tracedEnv,
-    });
-  } else {
-    claude = spawn(claudeCli, launchArgs, {
-      stdio: 'inherit',
-      windowsHide: true,
-      env: tracedEnv,
-    });
+  try {
+    if (needsShell) {
+      const cmdString = [claudeCli, ...launchArgs].map(escapeShellArg).join(' ');
+      claude = spawn(cmdString, {
+        stdio: 'inherit',
+        windowsHide: true,
+        shell: getWindowsEscapedCommandShell(),
+        env: tracedEnv,
+      });
+    } else {
+      claude = spawn(claudeCli, launchArgs, {
+        stdio: 'inherit',
+        windowsHide: true,
+        env: tracedEnv,
+      });
+    }
+  } catch (spawnError) {
+    // spawn() can throw synchronously (e.g. invalid arg/env). Remove the
+    // runtime settings overlay before propagating so the secret-bearing temp
+    // file is not orphaned. cleanup is idempotent.
+    cleanupLaunchSettings();
+    throw spawnError;
   }
 
   // Remove the runtime settings overlay once Claude has read it and exited.

src/cliproxy/executor/launch-settings.ts

@@ -27,6 +27,13 @@ import * as path from 'path';
 
 import { ANTHROPIC_MODEL_ENV_KEYS, ANTHROPIC_ROUTING_ENV_KEYS } from '../../utils/shell-executor';
 
+// SIBLING HELPER: src/utils/openai-compat-launch-settings.ts solves the same
+// "persisted --settings env clobbers runtime routing env" problem by STRIPPING
+// routing keys (process env wins by absence). This module instead OVERLAYS the
+// resolved values (settings wins by overwrite). The two differ intentionally:
+// strip vs overlay diverge when a key is present on disk but absent from the
+// process env. Unifying them needs an explicit force-absent API — see issue #1609.
+
 /**
  * Environment keys that control provider routing/model selection and are read
  * by Claude from the settings `env` block. These must reflect the resolved

src/utils/openai-compat-launch-settings.ts

@@ -10,6 +10,12 @@ export interface OpenAICompatLaunchSettings {
   cleanup: () => void;
 }
 
+// SIBLING HELPER: src/cliproxy/executor/launch-settings.ts (prepareLaunchSettings)
+// solves the same problem by OVERLAYING resolved routing values instead of
+// stripping them. This strip-based variant is required where callers deliberately
+// delete a routing key (e.g. ANTHROPIC_API_KEY in settings-flow) and need it
+// ABSENT from the launch settings. Do not unify without an explicit force-absent
+// key list — see issue #1609.
 export function createOpenAICompatLaunchSettings(
   settingsPath: string,
   settings: Settings

src/utils/shell-executor.ts

@@ -40,6 +40,10 @@ export const ANTHROPIC_ROUTING_ENV_KEYS = [
   'ANTHROPIC_API_KEY',
 ];
 const ANTHROPIC_ROUTING_ENV_KEY_SET = new Set(ANTHROPIC_ROUTING_ENV_KEYS);
+// NOTE: This is the intentional routing-overlay SUPERSET of model env keys
+// (includes ANTHROPIC_SMALL_FAST_MODEL). A separate 4-key `ANTHROPIC_MODEL_ENV_KEYS`
+// exists in src/shared/extended-context-utils.ts (re-exported as MODEL_ENV_VAR_KEYS).
+// The two are NOT interchangeable — import deliberately by purpose. See issue #1609.
 export const ANTHROPIC_MODEL_ENV_KEYS = [
   'ANTHROPIC_MODEL',
   'ANTHROPIC_DEFAULT_OPUS_MODEL',