fix(core): sanitize hook command expansion and prevent injection (#15343)

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));
           }
         },
       );

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
   }
 
   /**