From dced409ac42daf5cd76d2df0d8e1b269ef2d18d6 Mon Sep 17 00:00:00 2001 From: Sehoon Shon Date: Mon, 22 Dec 2025 11:46:38 -0500 Subject: [PATCH] Add Folder Trust Support To Hooks (#15325) --- .../config/policy-engine.integration.test.ts | 17 +++- packages/cli/src/config/trustedFolders.ts | 1 - packages/core/src/hooks/hookPlanner.test.ts | 3 +- packages/core/src/hooks/hookRegistry.test.ts | 34 ++++++- packages/core/src/hooks/hookRegistry.ts | 23 +++-- packages/core/src/hooks/hookRunner.test.ts | 92 ++++++++++++++++++- packages/core/src/hooks/hookRunner.ts | 26 +++++- packages/core/src/hooks/hookSystem.ts | 2 +- packages/core/src/hooks/index.ts | 2 +- packages/core/src/hooks/types.ts | 11 +++ 10 files changed, 188 insertions(+), 23 deletions(-) diff --git a/packages/cli/src/config/policy-engine.integration.test.ts b/packages/cli/src/config/policy-engine.integration.test.ts index 092567fb6d..e9a94836cf 100644 --- a/packages/cli/src/config/policy-engine.integration.test.ts +++ b/packages/cli/src/config/policy-engine.integration.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi } from 'vitest'; import { ApprovalMode, PolicyDecision, @@ -13,6 +13,21 @@ import { import { createPolicyEngineConfig } from './policy.js'; import type { Settings } from './settings.js'; +// Mock Storage to ensure tests are hermetic and don't read from user's home directory +vi.mock('@google/gemini-cli-core', async (importOriginal) => { + const actual = + await importOriginal(); + const Storage = actual.Storage; + // Monkey-patch static methods + Storage.getUserPoliciesDir = () => '/non-existent/user/policies'; + Storage.getSystemPoliciesDir = () => '/non-existent/system/policies'; + + return { + ...actual, + Storage, + }; +}); + describe('Policy Engine Integration Tests', () => { describe('Policy configuration produces valid PolicyEngine config', () => { it('should create a working PolicyEngine from basic settings', async () => { diff --git a/packages/cli/src/config/trustedFolders.ts b/packages/cli/src/config/trustedFolders.ts index 684a91639c..9a894c76cb 100644 --- a/packages/cli/src/config/trustedFolders.ts +++ b/packages/cli/src/config/trustedFolders.ts @@ -161,7 +161,6 @@ export function loadTrustedFolders(): LoadedTrustedFolders { const userConfig: Record = {}; const userPath = getTrustedFoldersPath(); - // Load user trusted folders try { if (fs.existsSync(userPath)) { diff --git a/packages/core/src/hooks/hookPlanner.test.ts b/packages/core/src/hooks/hookPlanner.test.ts index 245385f6af..987336ee0b 100644 --- a/packages/core/src/hooks/hookPlanner.test.ts +++ b/packages/core/src/hooks/hookPlanner.test.ts @@ -7,8 +7,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { HookPlanner } from './hookPlanner.js'; import type { HookRegistry, HookRegistryEntry } from './hookRegistry.js'; -import { HookEventName, HookType } from './types.js'; -import { ConfigSource } from './hookRegistry.js'; +import { ConfigSource, HookEventName, HookType } from './types.js'; // Mock debugLogger using vi.hoisted const mockDebugLogger = vi.hoisted(() => ({ diff --git a/packages/core/src/hooks/hookRegistry.test.ts b/packages/core/src/hooks/hookRegistry.test.ts index b3537c2581..731018c9a7 100644 --- a/packages/core/src/hooks/hookRegistry.test.ts +++ b/packages/core/src/hooks/hookRegistry.test.ts @@ -6,9 +6,9 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import * as fs from 'node:fs'; -import { HookRegistry, ConfigSource } from './hookRegistry.js'; +import { HookRegistry } from './hookRegistry.js'; import type { Storage } from '../config/storage.js'; -import { HookEventName, HookType } from './types.js'; +import { ConfigSource, HookEventName, HookType } from './types.js'; import type { Config } from '../config/config.js'; import type { HookDefinition } from './types.js'; @@ -47,6 +47,7 @@ describe('HookRegistry', () => { getExtensions: vi.fn().mockReturnValue([]), getHooks: vi.fn().mockReturnValue({}), getDisabledHooks: vi.fn().mockReturnValue([]), + isTrustedFolder: vi.fn().mockReturnValue(true), } as unknown as Config; hookRegistry = new HookRegistry(mockConfig); @@ -68,6 +69,35 @@ describe('HookRegistry', () => { ); }); + it('should not load hooks if folder is not trusted', async () => { + vi.mocked(mockConfig.isTrustedFolder).mockReturnValue(false); + const mockHooksConfig = { + BeforeTool: [ + { + hooks: [ + { + type: 'command', + command: './hooks/test.sh', + }, + ], + }, + ], + }; + + vi.mocked(mockConfig.getHooks).mockReturnValue( + mockHooksConfig as unknown as { + [K in HookEventName]?: HookDefinition[]; + }, + ); + + await hookRegistry.initialize(); + + expect(hookRegistry.getAllHooks()).toHaveLength(0); + expect(mockDebugLogger.warn).toHaveBeenCalledWith( + 'Project hooks disabled because the folder is not trusted.', + ); + }); + it('should load hooks from project configuration', async () => { const mockHooksConfig = { BeforeTool: [ diff --git a/packages/core/src/hooks/hookRegistry.ts b/packages/core/src/hooks/hookRegistry.ts index 53fc42bde4..f6f204ac8a 100644 --- a/packages/core/src/hooks/hookRegistry.ts +++ b/packages/core/src/hooks/hookRegistry.ts @@ -6,19 +6,9 @@ import type { Config } from '../config/config.js'; import type { HookDefinition, HookConfig } from './types.js'; -import { HookEventName } from './types.js'; +import { HookEventName, ConfigSource } from './types.js'; import { debugLogger } from '../utils/debugLogger.js'; -/** - * Configuration source levels in precedence order (highest to lowest) - */ -export enum ConfigSource { - Project = 'project', - User = 'user', - System = 'system', - Extensions = 'extensions', -} - /** * Hook registry entry with source information */ @@ -111,7 +101,13 @@ export class HookRegistry { // Get hooks from the main config (this comes from the merged settings) const configHooks = this.config.getHooks(); if (configHooks) { - this.processHooksConfiguration(configHooks, ConfigSource.Project); + if (this.config.isTrustedFolder()) { + this.processHooksConfiguration(configHooks, ConfigSource.Project); + } else { + debugLogger.warn( + 'Project hooks disabled because the folder is not trusted.', + ); + } } // Get hooks from extensions @@ -189,6 +185,9 @@ export class HookRegistry { } as HookRegistryEntry); const isDisabled = disabledHooks.includes(hookName); + // Add source to hook config + hookConfig.source = source; + this.entries.push({ config: hookConfig, source, diff --git a/packages/core/src/hooks/hookRunner.test.ts b/packages/core/src/hooks/hookRunner.test.ts index ea6b535f2e..1853718aa9 100644 --- a/packages/core/src/hooks/hookRunner.test.ts +++ b/packages/core/src/hooks/hookRunner.test.ts @@ -11,6 +11,8 @@ import { HookEventName, HookType } from './types.js'; import type { HookConfig } from './types.js'; import type { HookInput } from './types.js'; import type { Readable, Writable } from 'node:stream'; +import type { Config } from '../config/config.js'; +import { ConfigSource } from './types.js'; // Mock type for the child_process spawn type MockChildProcessWithoutNullStreams = ChildProcessWithoutNullStreams & { @@ -53,6 +55,7 @@ vi.stubGlobal('console', mockConsole); describe('HookRunner', () => { let hookRunner: HookRunner; let mockSpawn: MockChildProcessWithoutNullStreams; + let mockConfig: Config; const mockInput: HookInput = { session_id: 'test-session', @@ -65,7 +68,11 @@ describe('HookRunner', () => { beforeEach(() => { vi.resetAllMocks(); - hookRunner = new HookRunner(); + mockConfig = { + isTrustedFolder: vi.fn().mockReturnValue(true), + } as unknown as Config; + + hookRunner = new HookRunner(mockConfig); // Mock spawn with accessible mock functions const mockStdoutOn = vi.fn(); @@ -100,6 +107,89 @@ describe('HookRunner', () => { }); describe('executeHook', () => { + describe('security checks', () => { + it('should block project hooks in untrusted folders', async () => { + vi.mocked(mockConfig.isTrustedFolder).mockReturnValue(false); + + const projectHookConfig: HookConfig = { + type: HookType.Command, + command: './hooks/test.sh', + source: ConfigSource.Project, + }; + + const result = await hookRunner.executeHook( + projectHookConfig, + HookEventName.BeforeTool, + mockInput, + ); + + expect(result.success).toBe(false); + expect(result.error?.message).toContain( + 'Security: Blocked execution of project hook in untrusted folder', + ); + expect(mockDebugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('Security: Blocked execution'), + ); + expect(spawn).not.toHaveBeenCalled(); + }); + + it('should allow project hooks in trusted folders', async () => { + vi.mocked(mockConfig.isTrustedFolder).mockReturnValue(true); + + const projectHookConfig: HookConfig = { + type: HookType.Command, + command: './hooks/test.sh', + source: ConfigSource.Project, + }; + + // Mock successful execution + mockSpawn.mockProcessOn.mockImplementation( + (event: string, callback: (code: number) => void) => { + if (event === 'close') { + setTimeout(() => callback(0), 10); + } + }, + ); + + const result = await hookRunner.executeHook( + projectHookConfig, + HookEventName.BeforeTool, + mockInput, + ); + + expect(result.success).toBe(true); + expect(spawn).toHaveBeenCalled(); + }); + + it('should allow non-project hooks even in untrusted folders', async () => { + vi.mocked(mockConfig.isTrustedFolder).mockReturnValue(false); + + const systemHookConfig: HookConfig = { + type: HookType.Command, + command: './hooks/test.sh', + source: ConfigSource.System, + }; + + // Mock successful execution + mockSpawn.mockProcessOn.mockImplementation( + (event: string, callback: (code: number) => void) => { + if (event === 'close') { + setTimeout(() => callback(0), 10); + } + }, + ); + + const result = await hookRunner.executeHook( + systemHookConfig, + HookEventName.BeforeTool, + mockInput, + ); + + expect(result.success).toBe(true); + expect(spawn).toHaveBeenCalled(); + }); + }); + describe('command hooks', () => { const commandConfig: HookConfig = { type: HookType.Command, diff --git a/packages/core/src/hooks/hookRunner.ts b/packages/core/src/hooks/hookRunner.ts index 70c1e15216..a70e2fe8ad 100644 --- a/packages/core/src/hooks/hookRunner.ts +++ b/packages/core/src/hooks/hookRunner.ts @@ -6,7 +6,8 @@ import { spawn } from 'node:child_process'; import type { HookConfig } from './types.js'; -import { HookEventName } from './types.js'; +import { HookEventName, ConfigSource } from './types.js'; +import type { Config } from '../config/config.js'; import type { HookInput, HookOutput, @@ -39,7 +40,11 @@ const EXIT_CODE_NON_BLOCKING_ERROR = 1; * Hook runner that executes command hooks */ export class HookRunner { - constructor() {} + private readonly config: Config; + + constructor(config: Config) { + this.config = config; + } /** * Execute a single hook @@ -51,6 +56,23 @@ export class HookRunner { ): Promise { const startTime = Date.now(); + // Secondary security check: Ensure project hooks are not executed in untrusted folders + if ( + hookConfig.source === ConfigSource.Project && + !this.config.isTrustedFolder() + ) { + const errorMessage = + 'Security: Blocked execution of project hook in untrusted folder'; + debugLogger.warn(errorMessage); + return { + hookConfig, + eventName, + success: false, + error: new Error(errorMessage), + duration: 0, + }; + } + try { return await this.executeCommandHook( hookConfig, diff --git a/packages/core/src/hooks/hookSystem.ts b/packages/core/src/hooks/hookSystem.ts index 059340e27c..3f170368a9 100644 --- a/packages/core/src/hooks/hookSystem.ts +++ b/packages/core/src/hooks/hookSystem.ts @@ -31,7 +31,7 @@ export class HookSystem { // Initialize components this.hookRegistry = new HookRegistry(config); - this.hookRunner = new HookRunner(); + this.hookRunner = new HookRunner(config); this.hookAggregator = new HookAggregator(); this.hookPlanner = new HookPlanner(this.hookRegistry); this.hookEventHandler = new HookEventHandler( diff --git a/packages/core/src/hooks/index.ts b/packages/core/src/hooks/index.ts index a2346a3972..223ac25e42 100644 --- a/packages/core/src/hooks/index.ts +++ b/packages/core/src/hooks/index.ts @@ -17,7 +17,7 @@ export { HookEventHandler } from './hookEventHandler.js'; // Export interfaces and enums export type { HookRegistryEntry } from './hookRegistry.js'; -export { ConfigSource } from './hookRegistry.js'; +export { ConfigSource } from './types.js'; export type { AggregatedHookResult } from './hookAggregator.js'; export type { HookEventContext } from './hookPlanner.js'; diff --git a/packages/core/src/hooks/types.ts b/packages/core/src/hooks/types.ts index f2d2887513..8e9f8cd047 100644 --- a/packages/core/src/hooks/types.ts +++ b/packages/core/src/hooks/types.ts @@ -17,6 +17,16 @@ import type { } from './hookTranslator.js'; import { defaultHookTranslator } from './hookTranslator.js'; +/** + * Configuration source levels in precedence order (highest to lowest) + */ +export enum ConfigSource { + Project = 'project', + User = 'user', + System = 'system', + Extensions = 'extensions', +} + /** * Event names for the hook system */ @@ -43,6 +53,7 @@ export interface CommandHookConfig { name?: string; description?: string; timeout?: number; + source?: ConfigSource; } export type HookConfig = CommandHookConfig;