fix: address remaining CodeQL alerts (#332)
This commit is contained in:
25
src/utils/execFileNoThrow.test.ts
Normal file
25
src/utils/execFileNoThrow.test.ts
Normal file
@@ -0,0 +1,25 @@
|
|||||||
|
import { expect, test } from 'bun:test'
|
||||||
|
import { execFileNoThrowWithCwd } from './execFileNoThrow.js'
|
||||||
|
|
||||||
|
test('execFileNoThrowWithCwd rejects shell-like executable names', async () => {
|
||||||
|
const result = await execFileNoThrowWithCwd('openclaude && whoami', [])
|
||||||
|
|
||||||
|
expect(result.code).toBe(1)
|
||||||
|
expect(result.error).toContain('Unsafe executable')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('execFileNoThrowWithCwd rejects cwd values with control characters', async () => {
|
||||||
|
const result = await execFileNoThrowWithCwd(process.execPath, ['--version'], {
|
||||||
|
cwd: 'C:\\repo\nmalicious',
|
||||||
|
})
|
||||||
|
|
||||||
|
expect(result.code).toBe(1)
|
||||||
|
expect(result.error).toContain('Unsafe working directory')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('execFileNoThrowWithCwd rejects arguments with control characters', async () => {
|
||||||
|
const result = await execFileNoThrowWithCwd(process.execPath, ['--version\nmalicious'])
|
||||||
|
|
||||||
|
expect(result.code).toBe(1)
|
||||||
|
expect(result.error).toContain('Unsafe argument')
|
||||||
|
})
|
||||||
@@ -3,6 +3,7 @@
|
|||||||
// By using execa, Windows automatically gets shell escaping + BAT / CMD handling
|
// By using execa, Windows automatically gets shell escaping + BAT / CMD handling
|
||||||
|
|
||||||
import { type ExecaError, execa } from 'execa'
|
import { type ExecaError, execa } from 'execa'
|
||||||
|
import path from 'node:path'
|
||||||
import { getCwd } from '../utils/cwd.js'
|
import { getCwd } from '../utils/cwd.js'
|
||||||
import { logError } from './log.js'
|
import { logError } from './log.js'
|
||||||
|
|
||||||
@@ -59,6 +60,46 @@ type ExecaResultWithError = {
|
|||||||
signal?: string
|
signal?: string
|
||||||
}
|
}
|
||||||
|
|
||||||
|
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)
|
||||||
|
}
|
||||||
|
|
||||||
|
function validateExecutable(file: string): string | null {
|
||||||
|
const normalized = file.trim()
|
||||||
|
if (!normalized) {
|
||||||
|
return 'Unsafe executable: empty command'
|
||||||
|
}
|
||||||
|
if (CONTROL_CHAR_PATTERN.test(normalized)) {
|
||||||
|
return 'Unsafe executable: control characters are not allowed'
|
||||||
|
}
|
||||||
|
if (!hasPathSyntax(normalized) && !SAFE_BARE_EXECUTABLE_PATTERN.test(normalized)) {
|
||||||
|
return 'Unsafe executable: bare command names may only contain letters, numbers, ".", "_" and "-"'
|
||||||
|
}
|
||||||
|
return null
|
||||||
|
}
|
||||||
|
|
||||||
|
function validateArgs(args: string[]): string | null {
|
||||||
|
for (const arg of args) {
|
||||||
|
if (CONTROL_CHAR_PATTERN.test(arg)) {
|
||||||
|
return 'Unsafe argument: control characters are not allowed'
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return null
|
||||||
|
}
|
||||||
|
|
||||||
|
function validateWorkingDirectory(cwd: string | undefined): string | null {
|
||||||
|
if (!cwd) {
|
||||||
|
return null
|
||||||
|
}
|
||||||
|
if (CONTROL_CHAR_PATTERN.test(cwd)) {
|
||||||
|
return 'Unsafe working directory: control characters are not allowed'
|
||||||
|
}
|
||||||
|
return null
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Extracts a human-readable error message from an execa result.
|
* Extracts a human-readable error message from an execa result.
|
||||||
*
|
*
|
||||||
@@ -103,6 +144,21 @@ export function execFileNoThrowWithCwd(
|
|||||||
maxBuffer: 1_000_000,
|
maxBuffer: 1_000_000,
|
||||||
},
|
},
|
||||||
): Promise<{ stdout: string; stderr: string; code: number; error?: string }> {
|
): Promise<{ stdout: string; stderr: string; code: number; error?: string }> {
|
||||||
|
const executableError = validateExecutable(file)
|
||||||
|
if (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 })
|
||||||
|
}
|
||||||
|
|
||||||
|
const cwdError = validateWorkingDirectory(finalCwd)
|
||||||
|
if (cwdError) {
|
||||||
|
return Promise.resolve({ stdout: '', stderr: '', code: 1, error: cwdError })
|
||||||
|
}
|
||||||
|
|
||||||
return new Promise(resolve => {
|
return new Promise(resolve => {
|
||||||
// Use execa for cross-platform .bat/.cmd compatibility on Windows
|
// Use execa for cross-platform .bat/.cmd compatibility on Windows
|
||||||
execa(file, args, {
|
execa(file, args, {
|
||||||
|
|||||||
@@ -109,6 +109,19 @@ function isLocalBaseUrl(baseUrl) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function getHostname(baseUrl) {
|
||||||
|
const normalized = asNonEmptyString(baseUrl);
|
||||||
|
if (!normalized) {
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
|
try {
|
||||||
|
return new URL(normalized).hostname.toLowerCase();
|
||||||
|
} catch {
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
function resolveCommandCheckPath(command, workspacePath) {
|
function resolveCommandCheckPath(command, workspacePath) {
|
||||||
const normalized = asNonEmptyString(command);
|
const normalized = asNonEmptyString(command);
|
||||||
if (!normalized) {
|
if (!normalized) {
|
||||||
@@ -241,6 +254,7 @@ function hasCodexAlias(model) {
|
|||||||
function getOpenAICompatibleLabel(baseUrl, model) {
|
function getOpenAICompatibleLabel(baseUrl, model) {
|
||||||
const normalizedBaseUrl = (asNonEmptyString(baseUrl) || '').toLowerCase();
|
const normalizedBaseUrl = (asNonEmptyString(baseUrl) || '').toLowerCase();
|
||||||
const normalizedModel = (asNonEmptyString(model) || '').toLowerCase();
|
const normalizedModel = (asNonEmptyString(model) || '').toLowerCase();
|
||||||
|
const hostname = getHostname(baseUrl);
|
||||||
|
|
||||||
if (hasCodexBaseUrl(baseUrl) || (!baseUrl && hasCodexAlias(model))) {
|
if (hasCodexBaseUrl(baseUrl) || (!baseUrl && hasCodexAlias(model))) {
|
||||||
return 'Codex';
|
return 'Codex';
|
||||||
@@ -278,7 +292,7 @@ function getOpenAICompatibleLabel(baseUrl, model) {
|
|||||||
return 'Azure OpenAI';
|
return 'Azure OpenAI';
|
||||||
}
|
}
|
||||||
|
|
||||||
if (normalizedBaseUrl.includes('api.openai.com') || !normalizedBaseUrl) {
|
if (hostname === 'api.openai.com' || !normalizedBaseUrl) {
|
||||||
return 'OpenAI';
|
return 'OpenAI';
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -158,6 +158,44 @@ test('describeProviderState reports LM Studio from openai profile base url', ()
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('describeProviderState does not treat substring-matched OpenAI hosts as OpenAI', () => {
|
||||||
|
assert.deepEqual(
|
||||||
|
describeProviderState({
|
||||||
|
shimEnabled: false,
|
||||||
|
env: {
|
||||||
|
CLAUDE_CODE_USE_OPENAI: '1',
|
||||||
|
OPENAI_BASE_URL: 'https://evil.example/path/api.openai.com/v1',
|
||||||
|
OPENAI_MODEL: 'gpt-4o',
|
||||||
|
},
|
||||||
|
profile: null,
|
||||||
|
}),
|
||||||
|
{
|
||||||
|
label: 'OpenAI-compatible',
|
||||||
|
detail: 'gpt-4o',
|
||||||
|
source: 'env',
|
||||||
|
},
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('describeProviderState reports OpenAI when the parsed host is api.openai.com', () => {
|
||||||
|
assert.deepEqual(
|
||||||
|
describeProviderState({
|
||||||
|
shimEnabled: false,
|
||||||
|
env: {
|
||||||
|
CLAUDE_CODE_USE_OPENAI: '1',
|
||||||
|
OPENAI_BASE_URL: 'https://api.openai.com/v1',
|
||||||
|
OPENAI_MODEL: 'gpt-4o',
|
||||||
|
},
|
||||||
|
profile: null,
|
||||||
|
}),
|
||||||
|
{
|
||||||
|
label: 'OpenAI',
|
||||||
|
detail: 'gpt-4o',
|
||||||
|
source: 'env',
|
||||||
|
},
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
test('describeProviderState reports environment-backed provider details', () => {
|
test('describeProviderState reports environment-backed provider details', () => {
|
||||||
assert.deepEqual(
|
assert.deepEqual(
|
||||||
describeProviderState({
|
describeProviderState({
|
||||||
|
|||||||
Reference in New Issue
Block a user