diff --git a/packages/core/src/hooks/hookRunner.test.ts b/packages/core/src/hooks/hookRunner.test.ts index 1635e2516e..ea6b535f2e 100644 --- a/packages/core/src/hooks/hookRunner.test.ts +++ b/packages/core/src/hooks/hookRunner.test.ts @@ -114,9 +114,8 @@ describe('HookRunner', () => { mockSpawn.mockStdoutOn.mockImplementation( (event: string, callback: (data: Buffer) => void) => { if (event === 'data') { - setTimeout( - () => callback(Buffer.from(JSON.stringify(mockOutput))), - 10, + setImmediate(() => + callback(Buffer.from(JSON.stringify(mockOutput))), ); } }, @@ -125,7 +124,7 @@ describe('HookRunner', () => { mockSpawn.mockProcessOn.mockImplementation( (event: string, callback: (code: number) => void) => { if (event === 'close') { - setTimeout(() => callback(0), 20); + setImmediate(() => callback(0)); } }, ); @@ -150,7 +149,7 @@ describe('HookRunner', () => { mockSpawn.mockStderrOn.mockImplementation( (event: string, callback: (data: Buffer) => void) => { if (event === 'data') { - setTimeout(() => callback(Buffer.from(errorMessage)), 10); + setImmediate(() => callback(Buffer.from(errorMessage))); } }, ); @@ -158,7 +157,7 @@ describe('HookRunner', () => { mockSpawn.mockProcessOn.mockImplementation( (event: string, callback: (code: number) => void) => { if (event === 'close') { - setTimeout(() => callback(1), 20); + setImmediate(() => callback(1)); } }, ); @@ -223,9 +222,9 @@ describe('HookRunner', () => { killWasCalled = true; // Simulate that killing the process triggers the close event if (closeCallback) { - setTimeout(() => { + setImmediate(() => { closeCallback!(128); // Exit code 128 indicates process was killed by signal - }, 5); + }); } return true; }); @@ -251,7 +250,7 @@ describe('HookRunner', () => { mockSpawn.mockProcessOn.mockImplementation( (event: string, callback: (code: number) => void) => { if (event === 'close') { - setTimeout(() => callback(0), 10); + setImmediate(() => callback(0)); } }, ); @@ -263,9 +262,12 @@ describe('HookRunner', () => { ); expect(spawn).toHaveBeenCalledWith( - '/test/project/hooks/test.sh', + expect.stringMatching(/bash|powershell/), + expect.arrayContaining([ + expect.stringMatching(/['"]?\/test\/project['"]?\/hooks\/test\.sh/), + ]), expect.objectContaining({ - shell: true, + shell: false, env: expect.objectContaining({ GEMINI_PROJECT_DIR: '/test/project', CLAUDE_PROJECT_DIR: '/test/project', @@ -273,6 +275,43 @@ describe('HookRunner', () => { }), ); }); + + it('should not allow command injection via GEMINI_PROJECT_DIR', async () => { + const maliciousCwd = '/test/project; echo "pwned" > /tmp/pwned'; + const mockMaliciousInput: HookInput = { + ...mockInput, + cwd: maliciousCwd, + }; + + const config: HookConfig = { + type: HookType.Command, + command: 'ls $GEMINI_PROJECT_DIR', + }; + + // Mock the process closing immediately + mockSpawn.mockProcessOn.mockImplementation( + (event: string, callback: (code: number) => void) => { + if (event === 'close') { + setImmediate(() => callback(0)); + } + }, + ); + + await hookRunner.executeHook( + config, + HookEventName.BeforeTool, + mockMaliciousInput, + ); + + // If secure, spawn will be called with the shell executable and escaped command + expect(spawn).toHaveBeenCalledWith( + expect.stringMatching(/bash|powershell/), + expect.arrayContaining([ + expect.stringMatching(/ls (['"]).*echo.*pwned.*\1/), + ]), + expect.objectContaining({ shell: false }), + ); + }); }); }); @@ -287,7 +326,7 @@ describe('HookRunner', () => { mockSpawn.mockProcessOn.mockImplementation( (event: string, callback: (code: number) => void) => { if (event === 'close') { - setTimeout(() => callback(0), 10); + setImmediate(() => callback(0)); } }, ); @@ -314,7 +353,7 @@ describe('HookRunner', () => { (event: string, callback: (code: number) => void) => { if (event === 'close') { const exitCode = callCount++ === 0 ? 0 : 1; // First succeeds, second fails - setTimeout(() => callback(exitCode), 10); + setImmediate(() => callback(exitCode)); } }, ); @@ -344,10 +383,12 @@ describe('HookRunner', () => { mockSpawn.mockProcessOn.mockImplementation( (event: string, callback: (code: number) => void) => { if (event === 'close') { - const command = - vi.mocked(spawn).mock.calls[executionOrder.length][0]; + const args = vi.mocked(spawn).mock.calls[ + executionOrder.length + ][1] as string[]; + const command = args[args.length - 1]; executionOrder.push(command); - setTimeout(() => callback(0), 10); + setImmediate(() => callback(0)); } }, ); @@ -377,7 +418,7 @@ describe('HookRunner', () => { (event: string, callback: (data: Buffer) => void) => { if (event === 'data' && callCount === 1) { // Second hook fails - setTimeout(() => callback(Buffer.from('Hook 2 failed')), 10); + setImmediate(() => callback(Buffer.from('Hook 2 failed'))); } }, ); @@ -386,7 +427,7 @@ describe('HookRunner', () => { (event: string, callback: (code: number) => void) => { if (event === 'close') { const exitCode = callCount++ === 1 ? 1 : 0; // Second fails, others succeed - setTimeout(() => callback(exitCode), 20); + setImmediate(() => callback(exitCode)); } }, ); @@ -427,9 +468,8 @@ describe('HookRunner', () => { (event: string, callback: (data: Buffer) => void) => { if (event === 'data') { if (hookCallCount === 0) { - setTimeout( - () => callback(Buffer.from(JSON.stringify(mockOutput1))), - 10, + setImmediate(() => + callback(Buffer.from(JSON.stringify(mockOutput1))), ); } } @@ -440,7 +480,7 @@ describe('HookRunner', () => { (event: string, callback: (code: number) => void) => { if (event === 'close') { hookCallCount++; - setTimeout(() => callback(0), 20); + setImmediate(() => callback(0)); } }, ); @@ -491,9 +531,8 @@ describe('HookRunner', () => { (event: string, callback: (data: Buffer) => void) => { if (event === 'data') { if (hookCallCount === 0) { - setTimeout( - () => callback(Buffer.from(JSON.stringify(mockOutput1))), - 10, + setImmediate(() => + callback(Buffer.from(JSON.stringify(mockOutput1))), ); } } @@ -504,7 +543,7 @@ describe('HookRunner', () => { (event: string, callback: (code: number) => void) => { if (event === 'close') { hookCallCount++; - setTimeout(() => callback(0), 20); + setImmediate(() => callback(0)); } }, ); @@ -535,7 +574,7 @@ describe('HookRunner', () => { mockSpawn.mockStderrOn.mockImplementation( (event: string, callback: (data: Buffer) => void) => { if (event === 'data') { - setTimeout(() => callback(Buffer.from('Hook failed')), 10); + setImmediate(() => callback(Buffer.from('Hook failed'))); } }, ); @@ -543,7 +582,7 @@ describe('HookRunner', () => { mockSpawn.mockProcessOn.mockImplementation( (event: string, callback: (code: number) => void) => { if (event === 'close') { - setTimeout(() => callback(1), 20); // All hooks fail + setImmediate(() => callback(1)); // All hooks fail } }, ); @@ -580,7 +619,7 @@ describe('HookRunner', () => { mockSpawn.mockStdoutOn.mockImplementation( (event: string, callback: (data: Buffer) => void) => { if (event === 'data') { - setTimeout(() => callback(Buffer.from(invalidJson)), 10); + setImmediate(() => callback(Buffer.from(invalidJson))); } }, ); @@ -588,7 +627,7 @@ describe('HookRunner', () => { mockSpawn.mockProcessOn.mockImplementation( (event: string, callback: (code: number) => void) => { if (event === 'close') { - setTimeout(() => callback(0), 20); + setImmediate(() => callback(0)); } }, ); @@ -614,7 +653,7 @@ describe('HookRunner', () => { mockSpawn.mockStdoutOn.mockImplementation( (event: string, callback: (data: Buffer) => void) => { if (event === 'data') { - setTimeout(() => callback(Buffer.from(malformedJson)), 10); + setImmediate(() => callback(Buffer.from(malformedJson))); } }, ); @@ -622,7 +661,7 @@ describe('HookRunner', () => { mockSpawn.mockProcessOn.mockImplementation( (event: string, callback: (code: number) => void) => { if (event === 'close') { - setTimeout(() => callback(0), 20); + setImmediate(() => callback(0)); } }, ); @@ -646,7 +685,7 @@ describe('HookRunner', () => { mockSpawn.mockStderrOn.mockImplementation( (event: string, callback: (data: Buffer) => void) => { if (event === 'data') { - setTimeout(() => callback(Buffer.from(invalidJson)), 10); + setImmediate(() => callback(Buffer.from(invalidJson))); } }, ); @@ -654,7 +693,7 @@ describe('HookRunner', () => { mockSpawn.mockProcessOn.mockImplementation( (event: string, callback: (code: number) => void) => { if (event === 'close') { - setTimeout(() => callback(1), 20); + setImmediate(() => callback(1)); } }, ); @@ -679,7 +718,7 @@ describe('HookRunner', () => { mockSpawn.mockStderrOn.mockImplementation( (event: string, callback: (data: Buffer) => void) => { if (event === 'data') { - setTimeout(() => callback(Buffer.from(invalidJson)), 10); + setImmediate(() => callback(Buffer.from(invalidJson))); } }, ); @@ -687,7 +726,7 @@ describe('HookRunner', () => { mockSpawn.mockProcessOn.mockImplementation( (event: string, callback: (code: number) => void) => { if (event === 'close') { - setTimeout(() => callback(2), 20); + setImmediate(() => callback(2)); } }, ); @@ -710,7 +749,7 @@ describe('HookRunner', () => { mockSpawn.mockStdoutOn.mockImplementation( (event: string, callback: (data: Buffer) => void) => { if (event === 'data') { - setTimeout(() => callback(Buffer.from('')), 10); + setImmediate(() => callback(Buffer.from(''))); } }, ); @@ -718,7 +757,7 @@ describe('HookRunner', () => { mockSpawn.mockProcessOn.mockImplementation( (event: string, callback: (code: number) => void) => { if (event === 'close') { - setTimeout(() => callback(0), 20); + setImmediate(() => callback(0)); } }, ); @@ -741,7 +780,7 @@ describe('HookRunner', () => { mockSpawn.mockStdoutOn.mockImplementation( (event: string, callback: (data: Buffer) => void) => { if (event === 'data') { - setTimeout(() => callback(Buffer.from(doubleEncodedJson)), 10); + setImmediate(() => callback(Buffer.from(doubleEncodedJson))); } }, ); @@ -749,7 +788,7 @@ describe('HookRunner', () => { mockSpawn.mockProcessOn.mockImplementation( (event: string, callback: (code: number) => void) => { if (event === 'close') { - setTimeout(() => callback(0), 20); + setImmediate(() => callback(0)); } }, ); diff --git a/packages/core/src/hooks/hookRunner.ts b/packages/core/src/hooks/hookRunner.ts index 271fb6fffd..70c1e15216 100644 --- a/packages/core/src/hooks/hookRunner.ts +++ b/packages/core/src/hooks/hookRunner.ts @@ -17,6 +17,11 @@ import type { } from './types.js'; import type { LLMRequest } from './hookTranslator.js'; import { debugLogger } from '../utils/debugLogger.js'; +import { + escapeShellArg, + getShellConfiguration, + type ShellType, +} from '../utils/shell-utils.js'; /** * Default timeout for hook execution (60 seconds) @@ -201,7 +206,13 @@ export class HookRunner { let stdout = ''; let stderr = ''; let timedOut = false; - const command = this.expandCommand(hookConfig.command, input); + + const shellConfig = getShellConfiguration(); + const command = this.expandCommand( + hookConfig.command, + input, + shellConfig.shell, + ); // Set up environment variables const env = { @@ -210,12 +221,16 @@ export class HookRunner { CLAUDE_PROJECT_DIR: input.cwd, // For compatibility }; - const child = spawn(command, { - env, - cwd: input.cwd, - stdio: ['pipe', 'pipe', 'pipe'], - shell: true, - }); + const child = spawn( + shellConfig.executable, + [...shellConfig.argsPrefix, command], + { + env, + cwd: input.cwd, + stdio: ['pipe', 'pipe', 'pipe'], + shell: false, + }, + ); // Set up timeout const timeoutHandle = setTimeout(() => { @@ -338,10 +353,16 @@ export class HookRunner { /** * Expand command with environment variables and input context */ - private expandCommand(command: string, input: HookInput): string { + private expandCommand( + command: string, + input: HookInput, + shellType: ShellType, + ): string { + debugLogger.debug(`Expanding hook command: ${command} (cwd: ${input.cwd})`); + const escapedCwd = escapeShellArg(input.cwd, shellType); return command - .replace(/\$GEMINI_PROJECT_DIR/g, input.cwd) - .replace(/\$CLAUDE_PROJECT_DIR/g, input.cwd); // For compatibility + .replace(/\$GEMINI_PROJECT_DIR/g, () => escapedCwd) + .replace(/\$CLAUDE_PROJECT_DIR/g, () => escapedCwd); // For compatibility } /**