Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AndyMik90 merged 11 commits into from
Jan 13, 2026
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6d57383
fix: use absolute cmd.exe for Claude CLI validation
g1331 Jan 13, 2026
b9d6f66
fix: make cmd.exe validation type-safe for tests
g1331 Jan 13, 2026
cc0af2e
fix: satisfy frontend typecheck for cli tool tests
g1331 Jan 13, 2026
7e0b2ab
test: mock windows-paths exports for isSecurePath
g1331 Jan 13, 2026
8e4e41e
test: make cli env tests platform-aware
g1331 Jan 13, 2026
eaf7a72
test: cover isSecurePath guard in claude detection
g1331 Jan 13, 2026
640f2d8
test: align env-utils mocks with shouldUseShell
g1331 Jan 13, 2026
696baba
test: assert isSecurePath for cmd path
g1331 Jan 13, 2026
0025f93
fix(frontend): handle quoted claude.cmd paths in validation
g1331 Jan 13, 2026
781432c
Merge branch 'develop' into fix/claude-cli-cmd-exe-rebased
AndyMik90 Jan 13, 2026
1875c3f
Merge branch 'develop' into fix/claude-cli-cmd-exe-rebased
AndyMik90 Jan 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 94 additions & 36 deletions apps/frontend/src/main/__tests__/cli-tool-manager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down Expand Up @@ -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() },
Expand All @@ -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() },
Expand All @@ -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 {
Expand All @@ -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();
Expand All @@ -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', () => ({
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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');
Expand All @@ -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(() => {
Expand All @@ -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');
Expand Down Expand Up @@ -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');

Expand Down
8 changes: 5 additions & 3 deletions apps/frontend/src/main/__tests__/insights-config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand All @@ -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 () => {
Expand All @@ -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'));
});
});
81 changes: 59 additions & 22 deletions apps/frontend/src/main/cli-tool-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,27 @@
* - 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';
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';
Comment on lines +23 to +31
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Minor: Unused import alias.

The import on line 31 renames findHomebrewPython to findHomebrewPythonUtil, but the function is only called once (line 1717). This alias doesn't add clarity. Consider keeping the original name unless there's a naming conflict.

Optional simplification 
-import { findHomebrewPython as findHomebrewPythonUtil } from './utils/homebrew-python';
+import { findHomebrewPython } from './utils/homebrew-python';

And update line 1717:

-    return findHomebrewPythonUtil(
+    return findHomebrewPython(
🤖 Prompt for AI Agents 
In @apps/frontend/src/main/cli-tool-manager.ts around lines 23 - 31, The import
alias findHomebrewPythonUtil is unnecessary; change the import to use the
original name findHomebrewPython and update its usage in the call site (the
invocation currently calling findHomebrewPythonUtil) to call findHomebrewPython
instead; verify there are no naming conflicts in this module and update any
other occurrences of findHomebrewPythonUtil to the original identifier.


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,
Expand Down Expand Up @@ -927,27 +936,43 @@ class CLIToolManager {
private validateClaude(claudeCmd: string): ToolValidation {
try {
const needsShell = shouldUseShell(claudeCmd);
const env = getAugmentedEnv([path.dirname(claudeCmd)]);

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 (process.platform === 'win32' && !isSecurePath(claudeCmd)) {
return {
valid: false,
message: `Claude CLI path failed security validation: ${claudeCmd}`,
};
}
const cmdExe = process.env.ComSpec
|| path.join(process.env.SystemRoot || 'C:\\Windows', 'System32', 'cmd.exe');
const cmdLine = `""${claudeCmd}" --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(claudeCmd, ['--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
Expand Down Expand Up @@ -1043,18 +1068,30 @@ class CLIToolManager {
private async validateClaudeAsync(claudeCmd: string): Promise<ToolValidation> {
try {
const needsShell = shouldUseShell(claudeCmd);
const env = await getAugmentedEnvAsync([path.dirname(claudeCmd)]);

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 (process.platform === 'win32' && !isSecurePath(claudeCmd)) {
return {
valid: false,
message: `Claude CLI path failed security validation: ${claudeCmd}`,
};
}
const cmdExe = process.env.ComSpec
|| path.join(process.env.SystemRoot || 'C:\\Windows', 'System32', 'cmd.exe');
const cmdLine = `""${claudeCmd}" --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
Expand All @@ -1063,12 +1100,12 @@ class CLIToolManager {
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];

Expand Down
Loading
Loading