fix(frontend): validate Windows claude.cmd reliably in GUI (#1023)

* fix: use absolute cmd.exe for Claude CLI validation * fix: make cmd.exe validation type-safe for tests * fix: satisfy frontend typecheck for cli tool tests Signed-off-by: Umaru <caleb.1331@outlook.com> * test: mock windows-paths exports for isSecurePath Signed-off-by: Umaru <caleb.1331@outlook.com> * test: make cli env tests platform-aware Signed-off-by: Umaru <caleb.1331@outlook.com> * test: cover isSecurePath guard in claude detection Signed-off-by: Umaru <caleb.1331@outlook.com> * test: align env-utils mocks with shouldUseShell Signed-off-by: Umaru <caleb.1331@outlook.com> * test: assert isSecurePath for cmd path * fix(frontend): handle quoted claude.cmd paths in validation --------- Signed-off-by: Umaru <caleb.1331@outlook.com> Co-authored-by: Andy <119136210+AndyMik90@users.noreply.github.com>
2026-01-14 05:22:28 +08:00
parent c8374bc104
commit 1ae3359b7e
4 changed files with 182 additions and 69 deletions
@@ -17,10 +17,27 @@ import {
 } from '../cli-tool-manager';
 import {
  findWindowsExecutableViaWhere,
-  findWindowsExecutableViaWhereAsync
+  findWindowsExecutableViaWhereAsync,
+  isSecurePath
 } from '../utils/windows-paths';
 import { findExecutable, findExecutableAsync } from '../env-utils';

+type SpawnOptions = Parameters<(typeof import('../env-utils'))['getSpawnOptions']>[1];
+type MockDirent = import('fs').Dirent<import('node:buffer').NonSharedBuffer>;
+
+const createDirent = (name: string, isDir: boolean): MockDirent =>
+  ({
+    name,
+    parentPath: '',
+    isDirectory: () => isDir,
+    isFile: () => !isDir,
+    isBlockDevice: () => false,
+    isCharacterDevice: () => false,
+    isSymbolicLink: () => false,
+    isFIFO: () => false,
+    isSocket: () => false
+  }) as unknown as MockDirent;
+
 // Mock Electron app
 vi.mock('electron', () => ({
  app: {
@@ -52,7 +69,7 @@ vi.mock('child_process', () => {
  // so when tests call vi.mocked(execFileSync).mockReturnValue(), it affects execSync too
  const sharedSyncMock = vi.fn();

-  const mockExecFile = vi.fn((cmd: any, args: any, options: any, callback: any) => {
+const mockExecFile = vi.fn((cmd: unknown, args: unknown, options: unknown, callback: unknown) => {
    // Return a minimal ChildProcess-like object
    const childProcess = {
      stdout: { on: vi.fn() },
@@ -62,13 +79,14 @@ vi.mock('child_process', () => {

    // If callback is provided, call it asynchronously
    if (typeof callback === 'function') {
-      setImmediate(() => callback(null, 'claude-code version 1.0.0\n', ''));
+      const cb = callback as (error: Error | null, stdout: string, stderr: string) => void;
+      setImmediate(() => cb(null, 'claude-code version 1.0.0\n', ''));
    }

-    return childProcess as any;
+    return childProcess as unknown as import('child_process').ChildProcess;
  });

-  const mockExec = vi.fn((cmd: any, options: any, callback: any) => {
+  const mockExec = vi.fn((cmd: unknown, options: unknown, callback: unknown) => {
    // Return a minimal ChildProcess-like object
    const childProcess = {
      stdout: { on: vi.fn() },
@@ -78,10 +96,11 @@ vi.mock('child_process', () => {

    // If callback is provided, call it asynchronously
    if (typeof callback === 'function') {
-      setImmediate(() => callback(null, 'claude-code version 1.0.0\n', ''));
+      const cb = callback as (error: Error | null, stdout: string, stderr: string) => void;
+      setImmediate(() => cb(null, 'claude-code version 1.0.0\n', ''));
    }

-    return childProcess as any;
+    return childProcess as unknown as import('child_process').ChildProcess;
  });

  return {
@@ -93,18 +112,23 @@ vi.mock('child_process', () => {
 });

 // Mock env-utils to avoid PATH augmentation complexity
-vi.mock('../env-utils', () => ({
+vi.mock('../env-utils', () => {
+  const mockShouldUseShell = vi.fn((command: string) => {
+    if (process.platform !== 'win32') {
+      return false;
+    }
+    const trimmed = command.trim();
+    const unquoted =
+      trimmed.startsWith('"') && trimmed.endsWith('"') ? trimmed.slice(1, -1) : trimmed;
+    return /\.(cmd|bat)$/i.test(unquoted);
+  });
+
+  return ({
  findExecutable: vi.fn(() => null), // Return null to force platform-specific path checking
  findExecutableAsync: vi.fn(() => Promise.resolve(null)),
  getAugmentedEnv: vi.fn(() => ({ PATH: '' })),
  getAugmentedEnvAsync: vi.fn(() => Promise.resolve({ PATH: '' })),
-  shouldUseShell: vi.fn((command: string) => {
-    // Mock shouldUseShell to match actual behavior
-    if (process.platform !== 'win32') {
-      return false;
-    }
-    return /\.(cmd|bat)$/i.test(command);
-  }),
+  shouldUseShell: mockShouldUseShell,
  getSpawnCommand: vi.fn((command: string) => {
    // Mock getSpawnCommand to match actual behavior
    const trimmed = command.trim();
@@ -122,12 +146,13 @@ vi.mock('../env-utils', () => ({
    }
    return trimmed;
  }),
-  getSpawnOptions: vi.fn((command: string, baseOptions?: any) => ({
+  getSpawnOptions: vi.fn((command: string, baseOptions?: SpawnOptions) => ({
    ...baseOptions,
-    shell: /\.(cmd|bat)$/i.test(command) && process.platform === 'win32'
+    shell: mockShouldUseShell(command)
  })),
  existsAsync: vi.fn(() => Promise.resolve(false))
-}));
+  });
+});

 // Mock homebrew-python utility
 vi.mock('../utils/homebrew-python', () => ({
@@ -137,7 +162,11 @@ vi.mock('../utils/homebrew-python', () => ({
 // Mock windows-paths utility
 vi.mock('../utils/windows-paths', () => ({
  findWindowsExecutableViaWhere: vi.fn(() => null),
-  findWindowsExecutableViaWhereAsync: vi.fn(() => Promise.resolve(null))
+  findWindowsExecutableViaWhereAsync: vi.fn(() => Promise.resolve(null)),
+  isSecurePath: vi.fn(() => true),
+  getWindowsExecutablePaths: vi.fn(() => []),
+  getWindowsExecutablePathsAsync: vi.fn(() => Promise.resolve([])),
+  WINDOWS_GIT_PATHS: {}
 }));

 describe('cli-tool-manager - Claude CLI NVM detection', () => {
@@ -176,12 +205,12 @@ describe('cli-tool-manager - Claude CLI NVM detection', () => {
      });

      // Mock Node.js version directories (three versions)
-      const mockDirents = [
-        { name: 'v20.0.0', isDirectory: () => true },
-        { name: 'v22.17.0', isDirectory: () => true },
-        { name: 'v18.20.0', isDirectory: () => true },
+      const mockDirents: MockDirent[] = [
+        createDirent('v20.0.0', true),
+        createDirent('v22.17.0', true),
+        createDirent('v18.20.0', true),
      ];
-      vi.mocked(readdirSync).mockReturnValue(mockDirents as any);
+      vi.mocked(readdirSync).mockReturnValue(mockDirents);

      // Mock execFileSync to simulate successful version check
      vi.mocked(execFileSync).mockReturnValue('claude-code version 1.0.0\n');
@@ -245,11 +274,11 @@ describe('cli-tool-manager - Claude CLI NVM detection', () => {
        return false;
      });

-      const mockDirents = [
-        { name: 'v22.17.0', isDirectory: () => true },
-        { name: 'v20.0.0', isDirectory: () => true },
+      const mockDirents: MockDirent[] = [
+        createDirent('v22.17.0', true),
+        createDirent('v20.0.0', true),
      ];
-      vi.mocked(readdirSync).mockReturnValue(mockDirents as any);
+      vi.mocked(readdirSync).mockReturnValue(mockDirents);
      vi.mocked(execFileSync).mockReturnValue('claude-code version 1.5.0\n');

      const result = getToolInfo('claude');
@@ -272,10 +301,10 @@ describe('cli-tool-manager - Claude CLI NVM detection', () => {
        return false;
      });

-      const mockDirents = [
-        { name: 'v22.17.0', isDirectory: () => true },
+      const mockDirents: MockDirent[] = [
+        createDirent('v22.17.0', true),
      ];
-      vi.mocked(readdirSync).mockReturnValue(mockDirents as any);
+      vi.mocked(readdirSync).mockReturnValue(mockDirents);

      // Mock validation failure
      vi.mocked(execFileSync).mockImplementation(() => {
@@ -301,12 +330,12 @@ describe('cli-tool-manager - Claude CLI NVM detection', () => {
      });

      // Versions in random order
-      const mockDirents = [
-        { name: 'v18.20.0', isDirectory: () => true },
-        { name: 'v22.17.0', isDirectory: () => true },
-        { name: 'v20.5.0', isDirectory: () => true },
+      const mockDirents: MockDirent[] = [
+        createDirent('v18.20.0', true),
+        createDirent('v22.17.0', true),
+        createDirent('v20.5.0', true),
      ];
-      vi.mocked(readdirSync).mockReturnValue(mockDirents as any);
+      vi.mocked(readdirSync).mockReturnValue(mockDirents);
      vi.mocked(execFileSync).mockReturnValue('claude-code version 1.0.0\n');

      const result = getToolInfo('claude');
@@ -345,6 +374,35 @@ describe('cli-tool-manager - Claude CLI NVM detection', () => {
      expect(result.source).toBe('system-path');
    });

+    it('should ignore insecure Windows Claude CLI path from where.exe', () => {
+      Object.defineProperty(process, 'platform', {
+        value: 'win32',
+        writable: true
+      });
+
+      vi.mocked(os.homedir).mockReturnValue('C:\\Users\\test');
+      vi.mocked(findExecutable).mockReturnValue(null);
+      vi.mocked(findWindowsExecutableViaWhere).mockReturnValue(
+        'D:\\Tools\\claude.cmd'
+      );
+      vi.mocked(isSecurePath).mockReturnValueOnce(false);
+
+      vi.mocked(existsSync).mockImplementation((filePath) => {
+        const pathStr = String(filePath);
+        if (pathStr.includes('Tools') && pathStr.includes('claude.cmd')) {
+          return true;
+        }
+        return false;
+      });
+
+      const result = getToolInfo('claude');
+
+      expect(result.found).toBe(false);
+      expect(result.source).toBe('fallback');
+      expect(execFileSync).not.toHaveBeenCalled();
+      expect(isSecurePath).toHaveBeenCalledWith('D:\\Tools\\claude.cmd');
+    });
+
    it('should detect Claude CLI in Unix .local/bin path', () => {
      vi.mocked(os.homedir).mockReturnValue('/home/user');

@@ -59,7 +59,9 @@ describe('InsightsConfig', () => {
    expect(env.CLAUDE_CODE_OAUTH_TOKEN).toBe('oauth-token');
    expect(env.ANTHROPIC_BASE_URL).toBe('https://api.z.ai');
    expect(env.ANTHROPIC_AUTH_TOKEN).toBe('key');
-    expect(env.PYTHONPATH).toBe(['/site-packages', '/backend'].join(path.delimiter));
+    expect(env.PYTHONPATH).toBe(
+      [path.resolve('/site-packages'), path.resolve('/backend')].join(path.delimiter)
+    );
  });

  it('should clear ANTHROPIC env vars in OAuth mode when no API profile is set', async () => {
@@ -84,7 +86,7 @@ describe('InsightsConfig', () => {

    const env = await config.getProcessEnv();

-    expect(env.PYTHONPATH).toBe('/backend');
+    expect(env.PYTHONPATH).toBe(path.resolve('/backend'));
  });

  it('should keep PYTHONPATH from python env when auto-build path is missing', async () => {
@@ -94,6 +96,6 @@ describe('InsightsConfig', () => {

    const env = await config.getProcessEnv();

-    expect(env.PYTHONPATH).toBe('/site-packages');
+    expect(env.PYTHONPATH).toBe(path.resolve('/site-packages'));
  });
 });
@@ -20,7 +20,7 @@
 * - Graceful fallbacks when tools not found
 */

-import { execFileSync, execFile, execSync, exec } from 'child_process';
+import { execFileSync, execFile } from 'child_process';
 import { existsSync, readdirSync, promises as fsPromises } from 'fs';
 import path from 'path';
 import os from 'os';
@@ -28,10 +28,19 @@ import { promisify } from 'util';
 import { app } from 'electron';
 import { findExecutable, findExecutableAsync, getAugmentedEnv, getAugmentedEnvAsync, shouldUseShell, existsAsync } from './env-utils';
 import type { ToolDetectionResult } from '../shared/types';
+import { findHomebrewPython as findHomebrewPythonUtil } from './utils/homebrew-python';

 const execFileAsync = promisify(execFile);
-const execAsync = promisify(exec);
-import { findHomebrewPython as findHomebrewPythonUtil } from './utils/homebrew-python';
+
+type ExecFileSyncOptionsWithVerbatim = import('child_process').ExecFileSyncOptions & {
+  windowsVerbatimArguments?: boolean;
+};
+type ExecFileAsyncOptionsWithVerbatim = import('child_process').ExecFileOptionsWithStringEncoding & {
+  windowsVerbatimArguments?: boolean;
+};
+
+const normalizeExecOutput = (output: string | Buffer): string =>
+  typeof output === 'string' ? output : output.toString('utf-8');
 import {
  getWindowsExecutablePaths,
  getWindowsExecutablePathsAsync,
@@ -926,28 +935,51 @@ class CLIToolManager {
   */
  private validateClaude(claudeCmd: string): ToolValidation {
    try {
-      const needsShell = shouldUseShell(claudeCmd);
+      const trimmedCmd = claudeCmd.trim();
+      const unquotedCmd =
+        trimmedCmd.startsWith('"') && trimmedCmd.endsWith('"')
+          ? trimmedCmd.slice(1, -1)
+          : trimmedCmd;
+
+      const needsShell = shouldUseShell(trimmedCmd);
+      const cmdDir = path.dirname(unquotedCmd);
+      const env = getAugmentedEnv(cmdDir && cmdDir !== '.' ? [cmdDir] : []);

      let version: string;

      if (needsShell) {
-        // For .cmd/.bat files on Windows, use cmd.exe with argument array
-        // This avoids shell command injection while handling spaces in paths
-        version = execFileSync('cmd.exe', ['/c', claudeCmd, '--version'], {
+        // For .cmd/.bat files on Windows, use cmd.exe with a quoted command line
+        // /s preserves quotes so paths with spaces are handled correctly.
+        if (!isSecurePath(unquotedCmd)) {
+          return {
+            valid: false,
+            message: `Claude CLI path failed security validation: ${unquotedCmd}`,
+          };
+        }
+        const cmdExe = process.env.ComSpec
+          || path.join(process.env.SystemRoot || 'C:\\Windows', 'System32', 'cmd.exe');
+        const cmdLine = `""${unquotedCmd}" --version"`;
+        const execOptions: ExecFileSyncOptionsWithVerbatim = {
          encoding: 'utf-8',
          timeout: 5000,
          windowsHide: true,
-          env: getAugmentedEnv(),
-        }).trim();
+          windowsVerbatimArguments: true,
+          env,
+        };
+        version = normalizeExecOutput(
+          execFileSync(cmdExe, ['/d', '/s', '/c', cmdLine], execOptions)
+        ).trim();
      } else {
        // For .exe files and non-Windows, use execFileSync
-        version = execFileSync(claudeCmd, ['--version'], {
-          encoding: 'utf-8',
-          timeout: 5000,
-          windowsHide: true,
-          shell: false,
-          env: getAugmentedEnv(),
-        }).trim();
+        version = normalizeExecOutput(
+          execFileSync(unquotedCmd, ['--version'], {
+            encoding: 'utf-8',
+            timeout: 5000,
+            windowsHide: true,
+            shell: false,
+            env,
+          })
+        ).trim();
      }

      // Claude CLI version output format: "claude-code version X.Y.Z" or similar
@@ -1042,33 +1074,52 @@ class CLIToolManager {
   */
  private async validateClaudeAsync(claudeCmd: string): Promise<ToolValidation> {
    try {
-      const needsShell = shouldUseShell(claudeCmd);
+      const trimmedCmd = claudeCmd.trim();
+      const unquotedCmd =
+        trimmedCmd.startsWith('"') && trimmedCmd.endsWith('"')
+          ? trimmedCmd.slice(1, -1)
+          : trimmedCmd;
+
+      const needsShell = shouldUseShell(trimmedCmd);
+      const cmdDir = path.dirname(unquotedCmd);
+      const env = await getAugmentedEnvAsync(cmdDir && cmdDir !== '.' ? [cmdDir] : []);

      let stdout: string;

      if (needsShell) {
-        // For .cmd/.bat files on Windows, use cmd.exe with argument array
-        // This avoids shell command injection while handling spaces in paths
-        const result = await execFileAsync('cmd.exe', ['/c', claudeCmd, '--version'], {
+        // For .cmd/.bat files on Windows, use cmd.exe with a quoted command line
+        // /s preserves quotes so paths with spaces are handled correctly.
+        if (!isSecurePath(unquotedCmd)) {
+          return {
+            valid: false,
+            message: `Claude CLI path failed security validation: ${unquotedCmd}`,
+          };
+        }
+        const cmdExe = process.env.ComSpec
+          || path.join(process.env.SystemRoot || 'C:\\Windows', 'System32', 'cmd.exe');
+        const cmdLine = `""${unquotedCmd}" --version"`;
+        const execOptions: ExecFileAsyncOptionsWithVerbatim = {
          encoding: 'utf-8',
          timeout: 5000,
          windowsHide: true,
-          env: await getAugmentedEnvAsync(),
-        });
+          windowsVerbatimArguments: true,
+          env,
+        };
+        const result = await execFileAsync(cmdExe, ['/d', '/s', '/c', cmdLine], execOptions);
        stdout = result.stdout;
      } else {
        // For .exe files and non-Windows, use execFileAsync
-        const result = await execFileAsync(claudeCmd, ['--version'], {
+        const result = await execFileAsync(unquotedCmd, ['--version'], {
          encoding: 'utf-8',
          timeout: 5000,
          windowsHide: true,
          shell: false,
-          env: await getAugmentedEnvAsync(),
+          env,
        });
        stdout = result.stdout;
      }

-      const version = stdout.trim();
+      const version = normalizeExecOutput(stdout).trim();
      const match = version.match(/(\d+\.\d+\.\d+)/);
      const versionStr = match ? match[1] : version.split('\n')[0];

@@ -1,8 +1,10 @@
 import { writeFileSync } from 'fs';
 import { tmpdir } from 'os';
+import path from 'path';
 import { describe, expect, it, vi, beforeEach } from 'vitest';
 import type * as pty from '@lydell/node-pty';
 import type { TerminalProcess } from '../types';
+import { buildCdCommand } from '../../../shared/utils/shell-escape';

 /** Escape special regex characters in a string for safe use in RegExp constructor */
 const escapeForRegex = (str: string): string => str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
@@ -100,7 +102,7 @@ describe('claude-integration-handler', () => {
    invokeClaude(terminal, '/tmp/project', undefined, () => null, vi.fn());

    const written = vi.mocked(terminal.pty.write).mock.calls[0][0] as string;
-    expect(written).toContain("cd '/tmp/project' && ");
+    expect(written).toContain(buildCdCommand('/tmp/project'));
    expect(written).toContain("PATH='/opt/claude/bin:/usr/bin' ");
    expect(written).toContain("'/opt/claude bin/claude'\\''s'");
    expect(mockReleaseSessionId).toHaveBeenCalledWith('term-1');
@@ -233,8 +235,8 @@ describe('claude-integration-handler', () => {

    const tokenPath = vi.mocked(writeFileSync).mock.calls[0]?.[0] as string;
    const tokenContents = vi.mocked(writeFileSync).mock.calls[0]?.[1] as string;
-    const tmpDir = escapeForRegex(tmpdir());
-    expect(tokenPath).toMatch(new RegExp(`^${tmpDir}/\\.claude-token-1234-[0-9a-f]{16}$`));
+    const tokenPrefix = path.join(tmpdir(), '.claude-token-1234-');
+    expect(tokenPath).toMatch(new RegExp(`^${escapeForRegex(tokenPrefix)}[0-9a-f]{16}$`));
    expect(tokenContents).toBe("export CLAUDE_CODE_OAUTH_TOKEN='token-value'\n");
    const written = vi.mocked(terminal.pty.write).mock.calls[0][0] as string;
    expect(written).toContain("HISTFILE= HISTCONTROL=ignorespace ");
@@ -276,8 +278,8 @@ describe('claude-integration-handler', () => {

    const tokenPath = vi.mocked(writeFileSync).mock.calls[0]?.[0] as string;
    const tokenContents = vi.mocked(writeFileSync).mock.calls[0]?.[1] as string;
-    const tmpDir = escapeForRegex(tmpdir());
-    expect(tokenPath).toMatch(new RegExp(`^${tmpDir}/\\.claude-token-5678-[0-9a-f]{16}$`));
+    const tokenPrefix = path.join(tmpdir(), '.claude-token-5678-');
+    expect(tokenPath).toMatch(new RegExp(`^${escapeForRegex(tokenPrefix)}[0-9a-f]{16}$`));
    expect(tokenContents).toBe("export CLAUDE_CODE_OAUTH_TOKEN='token-value'\n");
    const written = vi.mocked(terminal.pty.write).mock.calls[0][0] as string;
    expect(written).toContain(`source '${tokenPath}'`);