From 0349b5e825805b0ae535e7cd6c77c48354ca2a75 Mon Sep 17 00:00:00 2001 From: Marshall Livingston Date: Tue, 3 Mar 2026 16:11:12 -0700 Subject: [PATCH 1/2] fix: shellEscape wraps in single quotes on Windows, breaking every docker command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit shellEscape blindly wraps everything in POSIX single quotes. That's fine on macOS and Linux where execSync spawns /bin/sh. On Windows it spawns cmd.exe, which doesn't treat single quotes as quoting characters at all — they get passed through literally as part of the argument. So `docker pull 'ghcr.io/kryptsec/sqli-auth-bypass:latest'` becomes an image reference that literally starts and ends with an apostrophe, and Docker rightfully tells you that's an invalid reference format. Every single docker and docker exec call in the CLI was broken on Windows. Now shellEscape checks process.platform and uses double-quote wrapping with proper cmd.exe escaping (percent doubling, backslash-escaped double quotes) on win32. POSIX behavior is unchanged. cc @pi3-code for additional testing on this. Closes #63 --- src/lib/shell.ts | 10 ++++++- tests/unit/shell.test.ts | 57 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/src/lib/shell.ts b/src/lib/shell.ts index 250cb91..a5226c6 100644 --- a/src/lib/shell.ts +++ b/src/lib/shell.ts @@ -1,6 +1,14 @@ // Shared shell utility -/** Escape a string for safe inclusion in a shell command (single-quote wrapping). */ +/** + * Escape a string for safe inclusion in a shell command. + * Uses single-quote wrapping on POSIX (bash/zsh/sh) and + * double-quote wrapping on Windows (cmd.exe). + */ export function shellEscape(s: string): string { + if (process.platform === 'win32') { + // cmd.exe: wrap in double quotes, escape internal double quotes and percent signs + return '"' + s.replace(/%/g, '%%').replace(/"/g, '\\"') + '"'; + } return "'" + s.replace(/'/g, "'\\''") + "'"; } diff --git a/tests/unit/shell.test.ts b/tests/unit/shell.test.ts index ebfd877..c9e8807 100644 --- a/tests/unit/shell.test.ts +++ b/tests/unit/shell.test.ts @@ -1,7 +1,11 @@ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, afterEach } from 'vitest'; import { shellEscape } from '../../src/lib/shell.js'; -describe('shellEscape', () => { +// --------------------------------------------------------------------------- +// POSIX (default on macOS/Linux) — single-quote wrapping +// --------------------------------------------------------------------------- + +describe('shellEscape (posix)', () => { it('wraps plain strings in single quotes', () => { expect(shellEscape('hello')).toBe("'hello'"); }); @@ -57,3 +61,52 @@ describe('shellEscape', () => { expect(shellEscape("'''")).toBe("''\\'''\\'''\\'''"); }); }); + +// --------------------------------------------------------------------------- +// Windows (cmd.exe) — double-quote wrapping +// --------------------------------------------------------------------------- + +describe('shellEscape (win32)', () => { + const originalPlatform = process.platform; + + afterEach(() => { + Object.defineProperty(process, 'platform', { value: originalPlatform }); + }); + + function asWindows() { + Object.defineProperty(process, 'platform', { value: 'win32' }); + } + + it('wraps plain strings in double quotes', () => { + asWindows(); + expect(shellEscape('hello')).toBe('"hello"'); + }); + + it('does not add single quotes on Windows', () => { + asWindows(); + const result = shellEscape('ghcr.io/kryptsec/sqli-auth-bypass:latest'); + expect(result).toBe('"ghcr.io/kryptsec/sqli-auth-bypass:latest"'); + expect(result).not.toContain("'"); + }); + + it('escapes internal double quotes', () => { + asWindows(); + expect(shellEscape('say "hello"')).toBe('"say \\"hello\\""'); + }); + + it('escapes percent signs to avoid cmd.exe variable expansion', () => { + asWindows(); + expect(shellEscape('100%')).toBe('"100%%"'); + }); + + it('handles empty string', () => { + asWindows(); + expect(shellEscape('')).toBe('""'); + }); + + it('neutralizes ampersand chaining', () => { + asWindows(); + const result = shellEscape('foo & del /q *'); + expect(result).toBe('"foo & del /q *"'); + }); +}); From a9df17ede21acf2b6edce4c661ea2e145badd8c9 Mon Sep 17 00:00:00 2001 From: Marshall Livingston Date: Thu, 5 Mar 2026 15:08:14 -0700 Subject: [PATCH 2/2] Replace execSync shell strings with execFileSync argument arrays MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original approach tried to make shellEscape() work on Windows by adding cmd.exe double-quote escaping. Pie's review correctly identified that cmd.exe escaping is a minefield — \" doesn't actually escape quotes in cmd.exe, %% only works in .bat files, and there's a real injection vector with the current implementation. The right fix is to not use a shell at all. execFileSync with argument arrays bypasses cmd.exe (and bash) entirely — arguments are passed directly to the process without any shell interpretation. No shell means no shell escaping means no injection surface. docker.ts: every execSync call replaced with execFileSync argument arrays. The POSIX `sleep 2` call replaced with Atomics.wait-based sleepSync() that works cross-platform without a shell. env-check.ts: same treatment. The `|| echo "FAIL"` pattern in checkTargetReachable replaced with try/catch. The `2>/dev/null` stderr suppression replaced by stdio: 'pipe'. shell.ts: reverted to POSIX-only, added doc comment explaining that execFileSync should be used for cross-platform instead. All tests updated to mock execFileSync and assert against argument arrays instead of command strings. 410/410 passing. --- src/lib/docker.ts | 68 +++++++------- src/lib/env-check.ts | 39 ++++---- src/lib/shell.ts | 13 ++- tests/unit/docker.test.ts | 177 +++++++++++++++++------------------ tests/unit/env-check.test.ts | 155 +++++++++++++++--------------- tests/unit/shell.test.ts | 57 +---------- 6 files changed, 228 insertions(+), 281 deletions(-) diff --git a/src/lib/docker.ts b/src/lib/docker.ts index 80251fa..9342c51 100644 --- a/src/lib/docker.ts +++ b/src/lib/docker.ts @@ -4,9 +4,8 @@ * health-checking, and cleanup for both registry and local modes. */ -import { execSync } from 'child_process'; -import { shellEscape } from './shell.js'; -import { DOCKER_WAIT_TIMEOUT, DOCKER_POLL_INTERVAL } from './constants.js'; +import { execFileSync } from 'child_process'; +import { DOCKER_WAIT_TIMEOUT } from './constants.js'; export interface ContainerSpec { challengeId: string; @@ -17,6 +16,11 @@ export interface ContainerSpec { targetContainerName: string; } +/** Synchronous sleep that works cross-platform (no shell, no `sleep` binary). */ +function sleepSync(ms: number): void { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); +} + /** * Pull a Docker image. Tries native platform first, falls back to linux/amd64 * if the image has no matching manifest (common for challenge images on Apple Silicon). @@ -28,7 +32,7 @@ export function pullImage(image: string, onProgress?: (line: string) => void): b } try { - execSync(`docker pull ${shellEscape(image)}`, { + execFileSync('docker', ['pull', image], { stdio: onProgress ? 'inherit' : 'pipe', encoding: 'utf-8', }); @@ -45,7 +49,7 @@ export function pullImage(image: string, onProgress?: (line: string) => void): b if (onProgress) { onProgress(`Pulling ${image} (linux/amd64 fallback)...`); } - execSync(`docker pull --platform linux/amd64 ${shellEscape(image)}`, { + execFileSync('docker', ['pull', '--platform', 'linux/amd64', image], { stdio: onProgress ? 'inherit' : 'pipe', encoding: 'utf-8', }); @@ -57,12 +61,12 @@ export function pullImage(image: string, onProgress?: (line: string) => void): b */ export function ensureNetwork(name: string): void { try { - execSync(`docker network inspect ${shellEscape(name)}`, { + execFileSync('docker', ['network', 'inspect', name], { stdio: 'pipe', encoding: 'utf-8', }); } catch { - execSync(`docker network create ${shellEscape(name)}`, { + execFileSync('docker', ['network', 'create', name], { stdio: 'pipe', encoding: 'utf-8', }); @@ -83,24 +87,23 @@ export function startContainers(spec: ContainerSpec, platforms?: PlatformOverrid cleanupStale(spec); ensureNetwork(spec.network); - const targetPlatformFlag = platforms?.target ? `--platform ${shellEscape(platforms.target)} ` : ''; - const kaliPlatformFlag = platforms?.kali ? `--platform ${shellEscape(platforms.kali)} ` : ''; - // Start target container - execSync( - `docker run -d ${targetPlatformFlag}--name ${shellEscape(spec.targetContainerName)} ` + - `--hostname target --network ${shellEscape(spec.network)} ` + - `${shellEscape(spec.targetImage)}`, - { stdio: 'pipe', encoding: 'utf-8' } - ); + const targetArgs = ['run', '-d']; + if (platforms?.target) targetArgs.push('--platform', platforms.target); + targetArgs.push('--name', spec.targetContainerName); + targetArgs.push('--hostname', 'target'); + targetArgs.push('--network', spec.network); + targetArgs.push(spec.targetImage); + execFileSync('docker', targetArgs, { stdio: 'pipe', encoding: 'utf-8' }); // Start kali container - execSync( - `docker run -d ${kaliPlatformFlag}--name ${shellEscape(spec.kaliContainerName)} ` + - `--hostname kali --network ${shellEscape(spec.network)} ` + - `${shellEscape(spec.kaliImage)} sleep infinity`, - { stdio: 'pipe', encoding: 'utf-8' } - ); + const kaliArgs = ['run', '-d']; + if (platforms?.kali) kaliArgs.push('--platform', platforms.kali); + kaliArgs.push('--name', spec.kaliContainerName); + kaliArgs.push('--hostname', 'kali'); + kaliArgs.push('--network', spec.network); + kaliArgs.push(spec.kaliImage, 'sleep', 'infinity'); + execFileSync('docker', kaliArgs, { stdio: 'pipe', encoding: 'utf-8' }); } /** @@ -134,18 +137,17 @@ export function waitForTarget( timeoutMs = DOCKER_WAIT_TIMEOUT ): void { const start = Date.now(); - const pollInterval = DOCKER_POLL_INTERVAL; while (Date.now() - start < timeoutMs) { try { - execSync( - `docker exec ${shellEscape(kaliContainer)} curl -sf ${shellEscape(targetUrl)}`, + execFileSync( + 'docker', ['exec', kaliContainer, 'curl', '-sf', targetUrl], { stdio: 'pipe', encoding: 'utf-8', timeout: 5000 } ); return; // Success } catch { // Not ready yet — wait and retry - execSync(`sleep 2`, { stdio: 'pipe' }); + sleepSync(2000); } } @@ -159,8 +161,8 @@ export function waitForTarget( */ export function cleanup(spec: ContainerSpec): void { try { - execSync( - `docker rm -f ${shellEscape(spec.targetContainerName)} ${shellEscape(spec.kaliContainerName)}`, + execFileSync( + 'docker', ['rm', '-f', spec.targetContainerName, spec.kaliContainerName], { stdio: 'pipe', encoding: 'utf-8' } ); } catch { @@ -168,7 +170,7 @@ export function cleanup(spec: ContainerSpec): void { } try { - execSync(`docker network rm ${shellEscape(spec.network)}`, { + execFileSync('docker', ['network', 'rm', spec.network], { stdio: 'pipe', encoding: 'utf-8', }); @@ -182,8 +184,8 @@ export function cleanup(spec: ContainerSpec): void { */ export function cleanupStale(spec: ContainerSpec): void { try { - execSync( - `docker rm -f ${shellEscape(spec.targetContainerName)} ${shellEscape(spec.kaliContainerName)} 2>/dev/null`, + execFileSync( + 'docker', ['rm', '-f', spec.targetContainerName, spec.kaliContainerName], { stdio: 'pipe', encoding: 'utf-8' } ); } catch { @@ -195,7 +197,7 @@ export function cleanupStale(spec: ContainerSpec): void { * Start containers from a docker-compose.yml in the given directory. */ export function startFromCompose(challengeDir: string): void { - execSync(`docker compose -f ${shellEscape(challengeDir)}/docker-compose.yml up -d --build`, { + execFileSync('docker', ['compose', '-f', `${challengeDir}/docker-compose.yml`, 'up', '-d', '--build'], { stdio: 'inherit', encoding: 'utf-8', }); @@ -205,7 +207,7 @@ export function startFromCompose(challengeDir: string): void { * Stop and remove containers from a docker-compose.yml in the given directory. */ export function stopFromCompose(challengeDir: string): void { - execSync(`docker compose -f ${shellEscape(challengeDir)}/docker-compose.yml down`, { + execFileSync('docker', ['compose', '-f', `${challengeDir}/docker-compose.yml`, 'down'], { stdio: 'pipe', encoding: 'utf-8', }); diff --git a/src/lib/env-check.ts b/src/lib/env-check.ts index 1678691..36ca03f 100644 --- a/src/lib/env-check.ts +++ b/src/lib/env-check.ts @@ -3,8 +3,7 @@ * Validates Docker, containers, and connectivity before running benchmarks. */ -import { execSync } from 'child_process'; -import { shellEscape } from './shell.js'; +import { execFileSync } from 'child_process'; import { DOCKER_STARTUP_POLL } from './constants.js'; import { ConfigError } from './errors.js'; import { getErrorStatus } from './retry.js'; @@ -32,7 +31,7 @@ export function checkDockerRunning(): EnvCheckResult { const hints: string[] = []; try { - execSync('docker ps', { encoding: 'utf-8', stdio: 'pipe' }); + execFileSync('docker', ['ps'], { encoding: 'utf-8', stdio: 'pipe' }); return { ok: true, errors: [], hints: [] }; } catch { errors.push('Docker is not running or not accessible'); @@ -51,7 +50,7 @@ export async function ensureDocker( ): Promise { // Check if docker is already running try { - execSync('docker ps', { encoding: 'utf-8', stdio: 'pipe' }); + execFileSync('docker', ['ps'], { encoding: 'utf-8', stdio: 'pipe' }); return { ok: true, autoStarted: false, errors: [], hints: [] }; } catch { // Docker not running — try to auto-start on macOS @@ -72,7 +71,7 @@ export async function ensureDocker( // macOS: try to launch Docker Desktop onStatus?.('Starting Docker Desktop...'); try { - execSync('open --background -a Docker', { stdio: 'pipe' }); + execFileSync('open', ['--background', '-a', 'Docker'], { stdio: 'pipe' }); } catch { return { ok: false, @@ -95,7 +94,7 @@ export async function ensureDocker( onStatus?.(`Waiting for Docker to be ready... (${elapsed}s)`); try { - execSync('docker ps', { encoding: 'utf-8', stdio: 'pipe' }); + execFileSync('docker', ['ps'], { encoding: 'utf-8', stdio: 'pipe' }); return { ok: true, autoStarted: true, errors: [], hints: [] }; } catch { // Not ready yet @@ -128,8 +127,8 @@ export function checkContainersRunning( const kaliContainer = containerName; // e.g. gatekeeper-kali-1 try { - const out = execSync( - `docker ps -a --format "{{.Names}}\t{{.Status}}"`, + const out = execFileSync( + 'docker', ['ps', '-a', '--format', '{{.Names}}\t{{.Status}}'], { encoding: 'utf-8', stdio: 'pipe' } ); @@ -184,14 +183,16 @@ export function checkTargetReachable( const hints: string[] = []; try { - // Extract host:port from URL (e.g. http://target:5000 -> target:5000) - const urlMatch = targetUrl.match(/^(?:https?:\/\/)?([^/]+)/); - const hostPort = urlMatch ? urlMatch[1] : targetUrl.replace(/^https?:\/\//, ''); - - const result = execSync( - `docker exec ${shellEscape(containerName)} curl -s -o /dev/null -w "%{http_code}" --connect-timeout 5 ${shellEscape(targetUrl)} 2>/dev/null || echo "FAIL"`, - { encoding: 'utf-8', stdio: 'pipe' } - ).trim(); + let result: string; + try { + result = execFileSync( + 'docker', + ['exec', containerName, 'curl', '-s', '-o', '/dev/null', '-w', '%{http_code}', '--connect-timeout', '5', targetUrl], + { encoding: 'utf-8', stdio: 'pipe' } + ).trim(); + } catch { + result = 'FAIL'; + } if (result === 'FAIL' || result === '000') { errors.push(`Target ${targetUrl} is not reachable from Kali container`); @@ -224,8 +225,8 @@ export function checkKaliTools(containerName: string): EnvCheckResult { for (const tool of REQUIRED_KALI_TOOLS) { try { - execSync( - `docker exec ${shellEscape(containerName)} which ${tool} 2>/dev/null`, + execFileSync( + 'docker', ['exec', containerName, 'which', tool], { encoding: 'utf-8', stdio: 'pipe' } ); } catch { @@ -351,7 +352,7 @@ export async function checkApiKey( */ export function runPreflightChecks( challengeId: string, - challengeDir: string, + _challengeDir: string, containerName: string, targetUrl: string ): EnvCheckResult { diff --git a/src/lib/shell.ts b/src/lib/shell.ts index a5226c6..24f51a8 100644 --- a/src/lib/shell.ts +++ b/src/lib/shell.ts @@ -1,14 +1,13 @@ // Shared shell utility /** - * Escape a string for safe inclusion in a shell command. - * Uses single-quote wrapping on POSIX (bash/zsh/sh) and - * double-quote wrapping on Windows (cmd.exe). + * Escape a string for safe inclusion in a POSIX shell command (single-quote wrapping). + * + * NOTE: This function is POSIX-only (bash/zsh/sh). It does NOT handle Windows + * cmd.exe quoting. For cross-platform subprocess calls, use execFileSync/spawnSync + * with argument arrays instead of shell strings — this bypasses the shell entirely + * and avoids escaping issues on all platforms. */ export function shellEscape(s: string): string { - if (process.platform === 'win32') { - // cmd.exe: wrap in double quotes, escape internal double quotes and percent signs - return '"' + s.replace(/%/g, '%%').replace(/"/g, '\\"') + '"'; - } return "'" + s.replace(/'/g, "'\\''") + "'"; } diff --git a/tests/unit/docker.test.ts b/tests/unit/docker.test.ts index 6cff5a3..f6ad146 100644 --- a/tests/unit/docker.test.ts +++ b/tests/unit/docker.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { execSync } from 'child_process'; +import { execFileSync } from 'child_process'; import { pullImage, ensureNetwork, @@ -11,10 +11,10 @@ import { import type { ContainerSpec } from '../../src/lib/docker.js'; vi.mock('child_process', () => ({ - execSync: vi.fn(), + execFileSync: vi.fn(), })); -const mockExecSync = vi.mocked(execSync); +const mockExecFileSync = vi.mocked(execFileSync); beforeEach(() => { vi.clearAllMocks(); @@ -29,74 +29,87 @@ const SPEC: ContainerSpec = { targetContainerName: 'oasis-gatekeeper-target', }; +/** Helper: get all calls as [command, args] tuples */ +function getCalls(): Array<[string, string[]]> { + return mockExecFileSync.mock.calls.map(c => [c[0] as string, c[1] as string[]]); +} + +/** Helper: filter calls by command prefix in args */ +function getDockerCalls(subcommand: string): Array<[string, string[]]> { + return getCalls().filter(([cmd, args]) => cmd === 'docker' && args[0] === subcommand); +} + // ============================================================================= // pullImage // ============================================================================= describe('pullImage', () => { it('returns false when native pull succeeds', () => { - mockExecSync.mockReturnValue('' as any); + mockExecFileSync.mockReturnValue('' as any); const result = pullImage('myimage:latest'); expect(result).toBe(false); - expect(mockExecSync).toHaveBeenCalledTimes(1); - expect((mockExecSync.mock.calls[0][0] as string)).toContain('docker pull'); - expect((mockExecSync.mock.calls[0][0] as string)).not.toContain('--platform'); + const pulls = getDockerCalls('pull'); + expect(pulls).toHaveLength(1); + expect(pulls[0][1]).toEqual(['pull', 'myimage:latest']); }); it('returns true when native pull fails with manifest error and amd64 fallback succeeds', () => { - mockExecSync + mockExecFileSync .mockImplementationOnce(() => { const err: any = new Error('pull failed'); err.stderr = 'no matching manifest for linux/arm64/v8'; throw err; }) - .mockReturnValueOnce('' as any); // amd64 fallback succeeds + .mockReturnValue('' as any); const result = pullImage('myimage:latest'); expect(result).toBe(true); - expect(mockExecSync).toHaveBeenCalledTimes(2); - expect((mockExecSync.mock.calls[1][0] as string)).toContain('--platform linux/amd64'); + const pulls = getDockerCalls('pull'); + expect(pulls).toHaveLength(2); + expect(pulls[1][1]).toContain('--platform'); + expect(pulls[1][1]).toContain('linux/amd64'); }); it('also catches "no match for platform" variant', () => { - mockExecSync + mockExecFileSync .mockImplementationOnce(() => { const err: any = new Error('pull failed'); err.stderr = 'no match for platform'; throw err; }) - .mockReturnValueOnce('' as any); + .mockReturnValue('' as any); const result = pullImage('myimage:latest'); expect(result).toBe(true); }); it('rethrows non-manifest errors without attempting fallback', () => { - mockExecSync.mockImplementation(() => { + mockExecFileSync.mockImplementation(() => { const err: any = new Error('network timeout'); err.stderr = 'connection refused'; throw err; }); expect(() => pullImage('myimage:latest')).toThrow('network timeout'); - expect(mockExecSync).toHaveBeenCalledTimes(1); + const pulls = getDockerCalls('pull'); + expect(pulls).toHaveLength(1); }); it('calls onProgress callback during pull', () => { - mockExecSync.mockReturnValue('' as any); + mockExecFileSync.mockReturnValue('' as any); const messages: string[] = []; pullImage('myimage:latest', (msg) => messages.push(msg)); expect(messages).toContain('Pulling myimage:latest...'); }); it('calls onProgress for fallback path', () => { - mockExecSync + mockExecFileSync .mockImplementationOnce(() => { const err: any = new Error('fail'); err.stderr = 'no matching manifest'; throw err; }) - .mockReturnValueOnce('' as any); + .mockReturnValue('' as any); const messages: string[] = []; pullImage('myimage:latest', (msg) => messages.push(msg)); @@ -104,11 +117,12 @@ describe('pullImage', () => { expect(messages).toContain('Pulling myimage:latest (linux/amd64 fallback)...'); }); - it('shell-escapes the image name', () => { - mockExecSync.mockReturnValue('' as any); + it('passes image name as a separate argument (no shell escaping needed)', () => { + mockExecFileSync.mockReturnValue('' as any); pullImage("evil'image"); - const cmd = mockExecSync.mock.calls[0][0] as string; - expect(cmd).toContain("'evil'\\''image'"); + const pulls = getDockerCalls('pull'); + // Image name is passed as a discrete array element, not interpolated into a shell string + expect(pulls[0][1]).toContain("evil'image"); }); }); @@ -118,52 +132,41 @@ describe('pullImage', () => { describe('startContainers', () => { it('runs both containers without platform flag when no overrides', () => { - mockExecSync.mockReturnValue('' as any); + mockExecFileSync.mockReturnValue('' as any); startContainers(SPEC); - // cleanupStale + ensureNetwork inspect/create + 2 docker run = at least 4 calls - const runCalls = mockExecSync.mock.calls - .map(c => c[0] as string) - .filter(c => c.startsWith('docker run')); - expect(runCalls).toHaveLength(2); - for (const cmd of runCalls) { - expect(cmd).not.toContain('--platform'); + const runs = getDockerCalls('run'); + expect(runs).toHaveLength(2); + for (const [, args] of runs) { + expect(args).not.toContain('--platform'); } }); it('applies platform only to target when only target needs it', () => { - mockExecSync.mockReturnValue('' as any); + mockExecFileSync.mockReturnValue('' as any); startContainers(SPEC, { target: 'linux/amd64' }); - const runCalls = mockExecSync.mock.calls - .map(c => c[0] as string) - .filter(c => c.startsWith('docker run')); - expect(runCalls).toHaveLength(2); - // Target should have --platform - expect(runCalls[0]).toContain('--platform'); - expect(runCalls[0]).toContain('linux/amd64'); - // Kali should NOT have --platform - expect(runCalls[1]).not.toContain('--platform'); + const runs = getDockerCalls('run'); + expect(runs).toHaveLength(2); + expect(runs[0][1]).toContain('--platform'); + expect(runs[0][1]).toContain('linux/amd64'); + expect(runs[1][1]).not.toContain('--platform'); }); it('applies platform only to kali when only kali needs it', () => { - mockExecSync.mockReturnValue('' as any); + mockExecFileSync.mockReturnValue('' as any); startContainers(SPEC, { kali: 'linux/amd64' }); - const runCalls = mockExecSync.mock.calls - .map(c => c[0] as string) - .filter(c => c.startsWith('docker run')); - expect(runCalls).toHaveLength(2); - expect(runCalls[0]).not.toContain('--platform'); - expect(runCalls[1]).toContain('--platform'); + const runs = getDockerCalls('run'); + expect(runs).toHaveLength(2); + expect(runs[0][1]).not.toContain('--platform'); + expect(runs[1][1]).toContain('--platform'); }); it('applies platform to both when both need it', () => { - mockExecSync.mockReturnValue('' as any); + mockExecFileSync.mockReturnValue('' as any); startContainers(SPEC, { target: 'linux/amd64', kali: 'linux/amd64' }); - const runCalls = mockExecSync.mock.calls - .map(c => c[0] as string) - .filter(c => c.startsWith('docker run')); - expect(runCalls).toHaveLength(2); - expect(runCalls[0]).toContain('--platform'); - expect(runCalls[1]).toContain('--platform'); + const runs = getDockerCalls('run'); + expect(runs).toHaveLength(2); + expect(runs[0][1]).toContain('--platform'); + expect(runs[1][1]).toContain('--platform'); }); }); @@ -173,29 +176,23 @@ describe('startContainers', () => { describe('pullAndStartContainers', () => { it('pulls both images and starts without platform when both native', () => { - mockExecSync.mockReturnValue('' as any); + mockExecFileSync.mockReturnValue('' as any); pullAndStartContainers(SPEC); - const pullCalls = mockExecSync.mock.calls - .map(c => c[0] as string) - .filter(c => c.includes('docker pull')); - expect(pullCalls).toHaveLength(2); + const pulls = getDockerCalls('pull'); + expect(pulls).toHaveLength(2); - const runCalls = mockExecSync.mock.calls - .map(c => c[0] as string) - .filter(c => c.startsWith('docker run')); - for (const cmd of runCalls) { - expect(cmd).not.toContain('--platform'); + const runs = getDockerCalls('run'); + for (const [, args] of runs) { + expect(args).not.toContain('--platform'); } }); it('sets target platform only when target image needs amd64 fallback', () => { - let callIdx = 0; - mockExecSync.mockImplementation((cmd: unknown) => { - const cmdStr = cmd as string; - callIdx++; - // First docker pull (target) — manifest error - if (cmdStr.includes('docker pull') && !cmdStr.includes('--platform') && cmdStr.includes('gatekeeper')) { + mockExecFileSync.mockImplementation((cmd: unknown, args: unknown) => { + const argArr = args as string[]; + // First docker pull (target, no --platform) — manifest error + if (cmd === 'docker' && argArr[0] === 'pull' && !argArr.includes('--platform') && argArr.includes(SPEC.targetImage)) { const err: any = new Error('fail'); err.stderr = 'no matching manifest for linux/arm64/v8'; throw err; @@ -205,18 +202,16 @@ describe('pullAndStartContainers', () => { pullAndStartContainers(SPEC); - const runCalls = mockExecSync.mock.calls - .map(c => c[0] as string) - .filter(c => c.startsWith('docker run')); - expect(runCalls).toHaveLength(2); + const runs = getDockerCalls('run'); + expect(runs).toHaveLength(2); // Target run should have platform - expect(runCalls[0]).toContain('--platform'); + expect(runs[0][1]).toContain('--platform'); // Kali run should NOT - expect(runCalls[1]).not.toContain('--platform'); + expect(runs[1][1]).not.toContain('--platform'); }); it('reports progress via onProgress callback', () => { - mockExecSync.mockReturnValue('' as any); + mockExecFileSync.mockReturnValue('' as any); const messages: string[] = []; pullAndStartContainers(SPEC, (msg) => messages.push(msg)); expect(messages.some(m => m.includes('Pulling') && m.includes('gatekeeper'))).toBe(true); @@ -231,19 +226,21 @@ describe('pullAndStartContainers', () => { describe('ensureNetwork', () => { it('does not create network if inspect succeeds', () => { - mockExecSync.mockReturnValue('' as any); + mockExecFileSync.mockReturnValue('' as any); ensureNetwork('test-net'); - expect(mockExecSync).toHaveBeenCalledTimes(1); - expect((mockExecSync.mock.calls[0][0] as string)).toContain('network inspect'); + const calls = getDockerCalls('network'); + expect(calls).toHaveLength(1); + expect(calls[0][1]).toEqual(['network', 'inspect', 'test-net']); }); it('creates network when inspect fails', () => { - mockExecSync + mockExecFileSync .mockImplementationOnce(() => { throw new Error('not found'); }) - .mockReturnValueOnce('' as any); + .mockReturnValue('' as any); ensureNetwork('test-net'); - expect(mockExecSync).toHaveBeenCalledTimes(2); - expect((mockExecSync.mock.calls[1][0] as string)).toContain('network create'); + const calls = getDockerCalls('network'); + expect(calls).toHaveLength(2); + expect(calls[1][1]).toEqual(['network', 'create', 'test-net']); }); }); @@ -253,22 +250,22 @@ describe('ensureNetwork', () => { describe('cleanup', () => { it('removes containers and network', () => { - mockExecSync.mockReturnValue('' as any); + mockExecFileSync.mockReturnValue('' as any); cleanup(SPEC); - const cmds = mockExecSync.mock.calls.map(c => c[0] as string); - expect(cmds.some(c => c.includes('docker rm -f'))).toBe(true); - expect(cmds.some(c => c.includes('network rm'))).toBe(true); + const calls = getCalls(); + expect(calls.some(([cmd, args]) => cmd === 'docker' && args[0] === 'rm')).toBe(true); + expect(calls.some(([cmd, args]) => cmd === 'docker' && args[0] === 'network' && args[1] === 'rm')).toBe(true); }); it('ignores errors from rm and network rm', () => { - mockExecSync.mockImplementation(() => { throw new Error('no such container'); }); + mockExecFileSync.mockImplementation(() => { throw new Error('no such container'); }); expect(() => cleanup(SPEC)).not.toThrow(); }); }); describe('cleanupStale', () => { it('ignores errors when containers do not exist', () => { - mockExecSync.mockImplementation(() => { throw new Error('no such container'); }); + mockExecFileSync.mockImplementation(() => { throw new Error('no such container'); }); expect(() => cleanupStale(SPEC)).not.toThrow(); }); }); diff --git a/tests/unit/env-check.test.ts b/tests/unit/env-check.test.ts index 21b5c5d..5e82988 100644 --- a/tests/unit/env-check.test.ts +++ b/tests/unit/env-check.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { execSync } from 'child_process'; +import { execFileSync } from 'child_process'; import { checkDockerRunning, ensureDocker, @@ -15,10 +15,10 @@ import { // ============================================================================= vi.mock('child_process', () => ({ - execSync: vi.fn(), + execFileSync: vi.fn(), })); -const mockExecSync = vi.mocked(execSync); +const mockExecFileSync = vi.mocked(execFileSync); // Mock Anthropic SDK (dynamic import) const mockAnthropicCreate = vi.fn(); @@ -42,20 +42,30 @@ beforeEach(() => { vi.clearAllMocks(); }); +/** Helper to match calls by command and first arg */ +function matchCall(cmd: string, firstArg?: string): (call: unknown[]) => boolean { + return (call: unknown[]) => { + if (call[0] !== cmd) return false; + if (firstArg === undefined) return true; + const args = call[1] as string[]; + return args?.[0] === firstArg; + }; +} + // ============================================================================= // checkDockerRunning // ============================================================================= describe('checkDockerRunning', () => { it('returns ok when docker ps succeeds', () => { - mockExecSync.mockReturnValue('CONTAINER ID IMAGE ...\n' as any); + mockExecFileSync.mockReturnValue('CONTAINER ID IMAGE ...\n' as any); const result = checkDockerRunning(); expect(result.ok).toBe(true); expect(result.errors).toHaveLength(0); }); it('returns error when docker ps throws', () => { - mockExecSync.mockImplementation(() => { + mockExecFileSync.mockImplementation(() => { throw new Error('Cannot connect to Docker daemon'); }); const result = checkDockerRunning(); @@ -64,7 +74,7 @@ describe('checkDockerRunning', () => { }); it('includes helpful hints on failure', () => { - mockExecSync.mockImplementation(() => { + mockExecFileSync.mockImplementation(() => { throw new Error('not running'); }); const result = checkDockerRunning(); @@ -89,34 +99,34 @@ describe('ensureDocker', () => { }); it('returns immediately when Docker is already running', async () => { - mockExecSync.mockReturnValueOnce('' as any); // docker ps succeeds + mockExecFileSync.mockReturnValueOnce('' as any); // docker ps succeeds const result = await ensureDocker(); expect(result.ok).toBe(true); expect(result.autoStarted).toBe(false); - expect(mockExecSync).toHaveBeenCalledTimes(1); + expect(mockExecFileSync).toHaveBeenCalledTimes(1); }); it('returns error on non-macOS when Docker is not running', async () => { setPlatform('linux'); - mockExecSync.mockImplementation(() => { throw new Error('not running'); }); + mockExecFileSync.mockImplementation(() => { throw new Error('not running'); }); const result = await ensureDocker(); expect(result.ok).toBe(false); expect(result.autoStarted).toBe(false); expect(result.errors).toContain('Docker is not running or not accessible'); expect(result.hints.some((h) => h.includes('Start Docker Desktop'))).toBe(true); - // Should only have called docker ps once (no open command) - expect(mockExecSync).toHaveBeenCalledTimes(1); + expect(mockExecFileSync).toHaveBeenCalledTimes(1); }); it('auto-starts Docker Desktop on macOS and polls until ready', async () => { setPlatform('darwin'); let callCount = 0; - mockExecSync.mockImplementation((cmd: unknown) => { + mockExecFileSync.mockImplementation((cmd: unknown, args: unknown) => { callCount++; - const cmdStr = cmd as string; - if (cmdStr === 'open --background -a Docker') return '' as any; - // First docker ps fails, second succeeds (after polling) - if (cmdStr === 'docker ps') { + const argArr = args as string[]; + // open --background -a Docker + if (cmd === 'open') return '' as any; + // docker ps — fail first two, then succeed + if (cmd === 'docker' && argArr[0] === 'ps') { if (callCount <= 2) throw new Error('not running'); return '' as any; } @@ -130,10 +140,9 @@ describe('ensureDocker', () => { it('returns error when open command fails on macOS', async () => { setPlatform('darwin'); - mockExecSync.mockImplementation((cmd: unknown) => { - const cmdStr = cmd as string; - if (cmdStr === 'docker ps') throw new Error('not running'); - if (cmdStr === 'open --background -a Docker') throw new Error('app not found'); + mockExecFileSync.mockImplementation((cmd: unknown) => { + if (cmd === 'docker') throw new Error('not running'); + if (cmd === 'open') throw new Error('app not found'); throw new Error('unexpected'); }); @@ -145,9 +154,8 @@ describe('ensureDocker', () => { it('returns timeout error when Docker never becomes ready', async () => { setPlatform('darwin'); - mockExecSync.mockImplementation((cmd: unknown) => { - const cmdStr = cmd as string; - if (cmdStr === 'open --background -a Docker') return '' as any; + mockExecFileSync.mockImplementation((cmd: unknown) => { + if (cmd === 'open') return '' as any; throw new Error('not running'); }); @@ -159,12 +167,11 @@ describe('ensureDocker', () => { it('calls onStatus callback with progress messages', async () => { setPlatform('darwin'); let callCount = 0; - mockExecSync.mockImplementation((cmd: unknown) => { + mockExecFileSync.mockImplementation((cmd: unknown, args: unknown) => { callCount++; - const cmdStr = cmd as string; - if (cmdStr === 'open --background -a Docker') return '' as any; - if (cmdStr === 'docker ps') { - // Fail first check, succeed after first poll + const argArr = args as string[]; + if (cmd === 'open') return '' as any; + if (cmd === 'docker' && argArr[0] === 'ps') { if (callCount <= 2) throw new Error('not running'); return '' as any; } @@ -189,7 +196,7 @@ describe('checkContainersRunning', () => { const containerName = 'gatekeeper-kali-1'; it('returns ok when both containers are Up', () => { - mockExecSync.mockReturnValue( + mockExecFileSync.mockReturnValue( 'gatekeeper-target-1\tUp 5 minutes\ngatekeeper-kali-1\tUp 5 minutes\n' as any ); const result = checkContainersRunning(challengeId, containerName); @@ -198,14 +205,14 @@ describe('checkContainersRunning', () => { }); it('returns error when target container is missing', () => { - mockExecSync.mockReturnValue('gatekeeper-kali-1\tUp 5 minutes\n' as any); + mockExecFileSync.mockReturnValue('gatekeeper-kali-1\tUp 5 minutes\n' as any); const result = checkContainersRunning(challengeId, containerName); expect(result.ok).toBe(false); expect(result.errors.some((e) => e.includes('gatekeeper-target-1'))).toBe(true); }); it('returns error when target container is Exited', () => { - mockExecSync.mockReturnValue( + mockExecFileSync.mockReturnValue( 'gatekeeper-target-1\tExited (1) 2 minutes ago\ngatekeeper-kali-1\tUp 5 minutes\n' as any ); const result = checkContainersRunning(challengeId, containerName); @@ -216,14 +223,14 @@ describe('checkContainersRunning', () => { }); it('returns error when kali container is missing', () => { - mockExecSync.mockReturnValue('gatekeeper-target-1\tUp 5 minutes\n' as any); + mockExecFileSync.mockReturnValue('gatekeeper-target-1\tUp 5 minutes\n' as any); const result = checkContainersRunning(challengeId, containerName); expect(result.ok).toBe(false); expect(result.errors.some((e) => e.includes('gatekeeper-kali-1'))).toBe(true); }); it('returns error when kali container is Exited', () => { - mockExecSync.mockReturnValue( + mockExecFileSync.mockReturnValue( 'gatekeeper-target-1\tUp 5 minutes\ngatekeeper-kali-1\tExited (0) 1 minute ago\n' as any ); const result = checkContainersRunning(challengeId, containerName); @@ -233,8 +240,8 @@ describe('checkContainersRunning', () => { ).toBe(true); }); - it('returns error when execSync throws', () => { - mockExecSync.mockImplementation(() => { + it('returns error when execFileSync throws', () => { + mockExecFileSync.mockImplementation(() => { throw new Error('docker failed'); }); const result = checkContainersRunning(challengeId, containerName); @@ -252,49 +259,42 @@ describe('checkTargetReachable', () => { const url = 'http://target:5000'; it('returns ok when curl returns 200', () => { - mockExecSync.mockReturnValue('200' as any); + mockExecFileSync.mockReturnValue('200' as any); const result = checkTargetReachable(container, url); expect(result.ok).toBe(true); }); it('returns ok when curl returns 404 (connected)', () => { - mockExecSync.mockReturnValue('404' as any); + mockExecFileSync.mockReturnValue('404' as any); const result = checkTargetReachable(container, url); expect(result.ok).toBe(true); }); it('returns error when curl returns 000 (not reachable)', () => { - mockExecSync.mockReturnValue('000' as any); + mockExecFileSync.mockReturnValue('000' as any); const result = checkTargetReachable(container, url); expect(result.ok).toBe(false); expect(result.errors.some((e) => e.includes('not reachable'))).toBe(true); }); - it('returns error when curl returns FAIL', () => { - mockExecSync.mockReturnValue('FAIL' as any); - const result = checkTargetReachable(container, url); - expect(result.ok).toBe(false); - expect(result.errors.some((e) => e.includes('not reachable'))).toBe(true); - }); - - it('returns error when execSync throws (curl missing)', () => { - mockExecSync.mockImplementation(() => { + it('returns error when curl throws (treated as FAIL)', () => { + mockExecFileSync.mockImplementation(() => { throw new Error('exec failed'); }); const result = checkTargetReachable(container, url); expect(result.ok).toBe(false); - expect(result.errors.some((e) => e.includes('curl may be missing'))).toBe(true); + expect(result.errors.some((e) => e.includes('not reachable'))).toBe(true); }); - it('shell-escapes containerName and targetUrl in exec call', () => { - mockExecSync.mockReturnValue('200' as any); + it('passes containerName and targetUrl as separate arguments (no shell escaping needed)', () => { + mockExecFileSync.mockReturnValue('200' as any); checkTargetReachable("evil'name", "http://x';rm -rf /"); - const cmd = mockExecSync.mock.calls[0][0] as string; - // Values should be wrapped in single quotes with escaped internal quotes - expect(cmd).toContain("'evil'\\''name'"); - expect(cmd).toContain("'http://x'\\''"); - // The injected command should be inside single quotes, not a bare shell token - expect(cmd).not.toMatch(/[^'];\s*rm\s/); + const call = mockExecFileSync.mock.calls[0]; + const args = call[1] as string[]; + // With execFileSync, arguments are passed directly — no shell involved + expect(call[0]).toBe('docker'); + expect(args).toContain("evil'name"); + expect(args).toContain("http://x';rm -rf /"); }); }); @@ -306,14 +306,14 @@ describe('checkKaliTools', () => { const container = 'gatekeeper-kali-1'; it('returns ok when all tools are found', () => { - mockExecSync.mockReturnValue('/usr/bin/tool' as any); + mockExecFileSync.mockReturnValue('/usr/bin/tool' as any); const result = checkKaliTools(container); expect(result.ok).toBe(true); expect(result.errors).toHaveLength(0); }); it('returns error when one tool is missing', () => { - mockExecSync + mockExecFileSync .mockReturnValueOnce('/usr/bin/curl' as any) // curl found .mockImplementationOnce(() => { throw new Error('not found'); @@ -327,7 +327,7 @@ describe('checkKaliTools', () => { }); it('returns multiple errors and hints when multiple tools missing', () => { - mockExecSync.mockImplementation(() => { + mockExecFileSync.mockImplementation(() => { throw new Error('not found'); }); const result = checkKaliTools(container); @@ -337,12 +337,13 @@ describe('checkKaliTools', () => { expect(result.hints.some((h) => h.includes('Rebuild'))).toBe(true); }); - it('shell-escapes containerName in exec call', () => { - mockExecSync.mockReturnValue('/usr/bin/tool' as any); + it('passes containerName as a separate argument (no shell escaping needed)', () => { + mockExecFileSync.mockReturnValue('/usr/bin/tool' as any); checkKaliTools("evil'name"); - for (const call of mockExecSync.mock.calls) { - const cmd = call[0] as string; - expect(cmd).toContain("'evil'\\''name'"); + for (const call of mockExecFileSync.mock.calls) { + const args = call[1] as string[]; + // Container name is a discrete array element, not interpolated into a shell string + expect(args).toContain("evil'name"); } }); }); @@ -465,17 +466,17 @@ describe('runPreflightChecks', () => { function mockAllChecksPass() { // docker ps - mockExecSync.mockReturnValueOnce('' as any); + mockExecFileSync.mockReturnValueOnce('' as any); // docker ps -a (containers) - mockExecSync.mockReturnValueOnce( + mockExecFileSync.mockReturnValueOnce( 'gatekeeper-target-1\tUp 5 minutes\ngatekeeper-kali-1\tUp 5 minutes\n' as any ); // curl check - mockExecSync.mockReturnValueOnce('200' as any); + mockExecFileSync.mockReturnValueOnce('200' as any); // which curl, which wget, which python3 - mockExecSync.mockReturnValueOnce('/usr/bin/curl' as any); - mockExecSync.mockReturnValueOnce('/usr/bin/wget' as any); - mockExecSync.mockReturnValueOnce('/usr/bin/python3' as any); + mockExecFileSync.mockReturnValueOnce('/usr/bin/curl' as any); + mockExecFileSync.mockReturnValueOnce('/usr/bin/wget' as any); + mockExecFileSync.mockReturnValueOnce('/usr/bin/python3' as any); } it('returns ok when all checks pass', () => { @@ -485,7 +486,7 @@ describe('runPreflightChecks', () => { }); it('returns docker error on early failure (docker not running)', () => { - mockExecSync.mockImplementation(() => { + mockExecFileSync.mockImplementation(() => { throw new Error('docker not found'); }); const result = runPreflightChecks(challengeId, challengeDir, containerName, targetUrl); @@ -495,9 +496,9 @@ describe('runPreflightChecks', () => { it('returns container error when docker ok but containers fail', () => { // docker ps succeeds - mockExecSync.mockReturnValueOnce('' as any); + mockExecFileSync.mockReturnValueOnce('' as any); // docker ps -a returns no containers - mockExecSync.mockReturnValueOnce('' as any); + mockExecFileSync.mockReturnValueOnce('' as any); const result = runPreflightChecks(challengeId, challengeDir, containerName, targetUrl); expect(result.ok).toBe(false); expect(result.errors.some((e) => e.includes('not found'))).toBe(true); @@ -505,15 +506,15 @@ describe('runPreflightChecks', () => { it('returns tools error when other checks pass but tools missing', () => { // docker ps - mockExecSync.mockReturnValueOnce('' as any); + mockExecFileSync.mockReturnValueOnce('' as any); // docker ps -a (containers) - mockExecSync.mockReturnValueOnce( + mockExecFileSync.mockReturnValueOnce( 'gatekeeper-target-1\tUp 5 minutes\ngatekeeper-kali-1\tUp 5 minutes\n' as any ); // curl check succeeds - mockExecSync.mockReturnValueOnce('200' as any); + mockExecFileSync.mockReturnValueOnce('200' as any); // all tools missing - mockExecSync.mockImplementation(() => { + mockExecFileSync.mockImplementation(() => { throw new Error('not found'); }); const result = runPreflightChecks(challengeId, challengeDir, containerName, targetUrl); diff --git a/tests/unit/shell.test.ts b/tests/unit/shell.test.ts index c9e8807..ebfd877 100644 --- a/tests/unit/shell.test.ts +++ b/tests/unit/shell.test.ts @@ -1,11 +1,7 @@ -import { describe, it, expect, afterEach } from 'vitest'; +import { describe, it, expect } from 'vitest'; import { shellEscape } from '../../src/lib/shell.js'; -// --------------------------------------------------------------------------- -// POSIX (default on macOS/Linux) — single-quote wrapping -// --------------------------------------------------------------------------- - -describe('shellEscape (posix)', () => { +describe('shellEscape', () => { it('wraps plain strings in single quotes', () => { expect(shellEscape('hello')).toBe("'hello'"); }); @@ -61,52 +57,3 @@ describe('shellEscape (posix)', () => { expect(shellEscape("'''")).toBe("''\\'''\\'''\\'''"); }); }); - -// --------------------------------------------------------------------------- -// Windows (cmd.exe) — double-quote wrapping -// --------------------------------------------------------------------------- - -describe('shellEscape (win32)', () => { - const originalPlatform = process.platform; - - afterEach(() => { - Object.defineProperty(process, 'platform', { value: originalPlatform }); - }); - - function asWindows() { - Object.defineProperty(process, 'platform', { value: 'win32' }); - } - - it('wraps plain strings in double quotes', () => { - asWindows(); - expect(shellEscape('hello')).toBe('"hello"'); - }); - - it('does not add single quotes on Windows', () => { - asWindows(); - const result = shellEscape('ghcr.io/kryptsec/sqli-auth-bypass:latest'); - expect(result).toBe('"ghcr.io/kryptsec/sqli-auth-bypass:latest"'); - expect(result).not.toContain("'"); - }); - - it('escapes internal double quotes', () => { - asWindows(); - expect(shellEscape('say "hello"')).toBe('"say \\"hello\\""'); - }); - - it('escapes percent signs to avoid cmd.exe variable expansion', () => { - asWindows(); - expect(shellEscape('100%')).toBe('"100%%"'); - }); - - it('handles empty string', () => { - asWindows(); - expect(shellEscape('')).toBe('""'); - }); - - it('neutralizes ampersand chaining', () => { - asWindows(); - const result = shellEscape('foo & del /q *'); - expect(result).toBe('"foo & del /q *"'); - }); -});