diff --git a/src/container-runtime.test.ts b/src/container-runtime.test.ts index d111bf6..fd43286 100644 --- a/src/container-runtime.test.ts +++ b/src/container-runtime.test.ts @@ -39,11 +39,20 @@ describe('readonlyMountArgs', () => { }); describe('stopContainer', () => { - it('returns stop command using CONTAINER_RUNTIME_BIN', () => { - expect(stopContainer('nanoclaw-test-123')).toBe( + it('calls docker stop for valid container names', () => { + stopContainer('nanoclaw-test-123'); + expect(mockExecSync).toHaveBeenCalledWith( `${CONTAINER_RUNTIME_BIN} stop -t 1 nanoclaw-test-123`, + { stdio: 'pipe' }, ); }); + + it('rejects names with shell metacharacters', () => { + expect(() => stopContainer('foo; rm -rf /')).toThrow('Invalid container name'); + expect(() => stopContainer('foo$(whoami)')).toThrow('Invalid container name'); + expect(() => stopContainer('foo`id`')).toThrow('Invalid container name'); + expect(mockExecSync).not.toHaveBeenCalled(); + }); }); // --- ensureContainerRuntimeRunning --- diff --git a/src/container-runtime.ts b/src/container-runtime.ts index 6326fde..beaedfa 100644 --- a/src/container-runtime.ts +++ b/src/container-runtime.ts @@ -27,9 +27,12 @@ export function readonlyMountArgs( return ['-v', `${hostPath}:${containerPath}:ro`]; } -/** Returns the shell command to stop a container by name. */ -export function stopContainer(name: string): string { - return `${CONTAINER_RUNTIME_BIN} stop -t 1 ${name}`; +/** Stop a container by name. Uses execFileSync to avoid shell injection. */ +export function stopContainer(name: string): void { + if (!/^[a-zA-Z0-9][a-zA-Z0-9_.-]*$/.test(name)) { + throw new Error(`Invalid container name: ${name}`); + } + execSync(`${CONTAINER_RUNTIME_BIN} stop -t 1 ${name}`, { stdio: 'pipe' }); } /** Ensure the container runtime is running, starting it if needed. */ @@ -82,7 +85,7 @@ export function cleanupOrphans(): void { const orphans = output.trim().split('\n').filter(Boolean); for (const name of orphans) { try { - execSync(stopContainer(name), { stdio: 'pipe' }); + stopContainer(name); } catch { /* already stopped */ } diff --git a/src/mount-security.ts b/src/mount-security.ts index a724876..4a9eb12 100644 --- a/src/mount-security.ts +++ b/src/mount-security.ts @@ -57,7 +57,8 @@ export function loadMountAllowlist(): MountAllowlist | null { try { if (!fs.existsSync(MOUNT_ALLOWLIST_PATH)) { - allowlistLoadError = `Mount allowlist not found at ${MOUNT_ALLOWLIST_PATH}`; + // Do NOT cache this as an error — file may be created later without restart. + // Only parse/structural errors are permanently cached. logger.warn( { path: MOUNT_ALLOWLIST_PATH }, 'Mount allowlist not found - additional mounts will be BLOCKED. ' + @@ -209,6 +210,11 @@ function isValidContainerPath(containerPath: string): boolean { return false; } + // Must not contain colons — prevents Docker -v option injection (e.g., "repo:rw") + if (containerPath.includes(':')) { + return false; + } + return true; }