From eb3312e7baaf28f05d475fd7830e1abda82dc0b6 Mon Sep 17 00:00:00 2001 From: Abhi <43648792+abhipatel12@users.noreply.github.com> Date: Wed, 3 Dec 2025 15:07:37 -0500 Subject: [PATCH] feat: Support Extension Hooks with Security Warning (#14460) --- packages/cli/src/config/extension-manager.ts | 65 +++++++++++++++ packages/cli/src/config/extension.test.ts | 79 ++++++++++++++++++- .../cli/src/config/extensions/consent.test.ts | 76 ++++++++++++++++-- packages/cli/src/config/extensions/consent.ts | 15 +++- 4 files changed, 225 insertions(+), 10 deletions(-) diff --git a/packages/cli/src/config/extension-manager.ts b/packages/cli/src/config/extension-manager.ts index e6467d9b96..642e3dc2e6 100644 --- a/packages/cli/src/config/extension-manager.ts +++ b/packages/cli/src/config/extension-manager.ts @@ -41,6 +41,8 @@ import { type MCPServerConfig, type ExtensionInstallMetadata, type GeminiCLIExtension, + type HookDefinition, + type HookEventName, } from '@google/gemini-cli-core'; import { maybeRequestConsentOrFail } from './extensions/consent.js'; import { resolveEnvVarsInObject } from '../utils/envVarResolver.js'; @@ -253,10 +255,20 @@ export class ExtensionManager extends ExtensionLoader { ); } + const newHasHooks = fs.existsSync( + path.join(localSourcePath, 'hooks', 'hooks.json'), + ); + let previousHasHooks = false; + if (isUpdate && previous && previous.hooks) { + previousHasHooks = Object.keys(previous.hooks).length > 0; + } + await maybeRequestConsentOrFail( newExtensionConfig, this.requestConsent, + newHasHooks, previousExtensionConfig, + previousHasHooks, ); const extensionId = getExtensionId(newExtensionConfig, installMetadata); const destinationPath = new ExtensionStorage( @@ -501,6 +513,11 @@ export class ExtensionManager extends ExtensionLoader { ) .filter((contextFilePath) => fs.existsSync(contextFilePath)); + const hooks = await this.loadExtensionHooks(effectiveExtensionPath, { + extensionPath: effectiveExtensionPath, + workspacePath: this.workspaceDir, + }); + const extension = { name: config.name, version: config.version, @@ -509,6 +526,7 @@ export class ExtensionManager extends ExtensionLoader { installMetadata, mcpServers: config.mcpServers, excludeTools: config.excludeTools, + hooks, isActive: this.extensionEnablementManager.isEnabled( config.name, this.workspaceDir, @@ -576,6 +594,53 @@ export class ExtensionManager extends ExtensionLoader { } } + private async loadExtensionHooks( + extensionDir: string, + context: { extensionPath: string; workspacePath: string }, + ): Promise<{ [K in HookEventName]?: HookDefinition[] } | undefined> { + const hooksFilePath = path.join(extensionDir, 'hooks', 'hooks.json'); + + try { + const hooksContent = await fs.promises.readFile(hooksFilePath, 'utf-8'); + const rawHooks = JSON.parse(hooksContent); + + if ( + !rawHooks || + typeof rawHooks !== 'object' || + typeof rawHooks.hooks !== 'object' || + rawHooks.hooks === null || + Array.isArray(rawHooks.hooks) + ) { + debugLogger.warn( + `Invalid hooks configuration in ${hooksFilePath}: "hooks" property must be an object`, + ); + return undefined; + } + + // Hydrate variables in the hooks configuration + const hydratedHooks = recursivelyHydrateStrings( + rawHooks.hooks as unknown as JsonObject, + { + ...context, + '/': path.sep, + pathSeparator: path.sep, + }, + ) as { [K in HookEventName]?: HookDefinition[] }; + + return hydratedHooks; + } catch (e) { + if ((e as NodeJS.ErrnoException).code === 'ENOENT') { + return undefined; // File not found is not an error here. + } + debugLogger.warn( + `Failed to load extension hooks from ${hooksFilePath}: ${getErrorMessage( + e, + )}`, + ); + return undefined; + } + } + toOutputString(extension: GeminiCLIExtension): string { const userEnabled = this.extensionEnablementManager.isEnabled( extension.name, diff --git a/packages/cli/src/config/extension.test.ts b/packages/cli/src/config/extension.test.ts index a30065f612..32ce455d31 100644 --- a/packages/cli/src/config/extension.test.ts +++ b/packages/cli/src/config/extension.test.ts @@ -713,11 +713,88 @@ describe('extension tests', () => { name: 'no-meta-name', version: '1.0.0', }); - const extensions = await extensionManager.loadExtensions(); const extension = extensions.find((e) => e.name === 'no-meta-name'); expect(extension?.id).toBe(hashValue('no-meta-name')); }); + + it('should load extension hooks and hydrate variables', async () => { + const extDir = createExtension({ + extensionsDir: userExtensionsDir, + name: 'hook-extension', + version: '1.0.0', + }); + + const hooksDir = path.join(extDir, 'hooks'); + fs.mkdirSync(hooksDir); + + const hooksConfig = { + hooks: { + BeforeTool: [ + { + matcher: '.*', + hooks: [ + { + type: 'command', + command: 'echo ${extensionPath}', + }, + ], + }, + ], + }, + }; + + fs.writeFileSync( + path.join(hooksDir, 'hooks.json'), + JSON.stringify(hooksConfig), + ); + + const extensions = await extensionManager.loadExtensions(); + expect(extensions).toHaveLength(1); + const extension = extensions[0]; + + expect(extension.hooks).toBeDefined(); + expect(extension.hooks?.BeforeTool).toHaveLength(1); + expect(extension.hooks?.BeforeTool?.[0].hooks[0].command).toBe( + `echo ${extDir}`, + ); + }); + + it('should warn about hooks during installation', async () => { + const requestConsentSpy = vi.fn().mockResolvedValue(true); + extensionManager.setRequestConsent(requestConsentSpy); + + const sourceExtDir = path.join( + tempWorkspaceDir, + 'hook-extension-source', + ); + fs.mkdirSync(sourceExtDir, { recursive: true }); + + const hooksDir = path.join(sourceExtDir, 'hooks'); + fs.mkdirSync(hooksDir); + fs.writeFileSync( + path.join(hooksDir, 'hooks.json'), + JSON.stringify({ hooks: {} }), + ); + + fs.writeFileSync( + path.join(sourceExtDir, 'gemini-extension.json'), + JSON.stringify({ + name: 'hook-extension-install', + version: '1.0.0', + }), + ); + + await extensionManager.loadExtensions(); + await extensionManager.installOrUpdateExtension({ + source: sourceExtDir, + type: 'local', + }); + + expect(requestConsentSpy).toHaveBeenCalledWith( + expect.stringContaining('⚠️ This extension contains Hooks'), + ); + }); }); }); diff --git a/packages/cli/src/config/extensions/consent.test.ts b/packages/cli/src/config/extensions/consent.test.ts index 9101c2a156..ef57161132 100644 --- a/packages/cli/src/config/extensions/consent.test.ts +++ b/packages/cli/src/config/extensions/consent.test.ts @@ -112,20 +112,31 @@ describe('consent', () => { it('should request consent if there is no previous config', async () => { const requestConsent = vi.fn().mockResolvedValue(true); - await maybeRequestConsentOrFail(baseConfig, requestConsent, undefined); + await maybeRequestConsentOrFail( + baseConfig, + requestConsent, + false, + undefined, + ); expect(requestConsent).toHaveBeenCalledTimes(1); }); it('should not request consent if configs are identical', async () => { const requestConsent = vi.fn().mockResolvedValue(true); - await maybeRequestConsentOrFail(baseConfig, requestConsent, baseConfig); + await maybeRequestConsentOrFail( + baseConfig, + requestConsent, + false, + baseConfig, + false, + ); expect(requestConsent).not.toHaveBeenCalled(); }); it('should throw an error if consent is denied', async () => { const requestConsent = vi.fn().mockResolvedValue(false); await expect( - maybeRequestConsentOrFail(baseConfig, requestConsent, undefined), + maybeRequestConsentOrFail(baseConfig, requestConsent, false, undefined), ).rejects.toThrow('Installation cancelled for "test-ext".'); }); @@ -141,7 +152,12 @@ describe('consent', () => { excludeTools: ['tool1', 'tool2'], }; const requestConsent = vi.fn().mockResolvedValue(true); - await maybeRequestConsentOrFail(config, requestConsent, undefined); + await maybeRequestConsentOrFail( + config, + requestConsent, + false, + undefined, + ); const expectedConsentString = [ 'Installing extension "test-ext".', @@ -163,7 +179,13 @@ describe('consent', () => { mcpServers: { server1: { command: 'npm', args: ['start'] } }, }; const requestConsent = vi.fn().mockResolvedValue(true); - await maybeRequestConsentOrFail(newConfig, requestConsent, prevConfig); + await maybeRequestConsentOrFail( + newConfig, + requestConsent, + false, + prevConfig, + false, + ); expect(requestConsent).toHaveBeenCalledTimes(1); }); @@ -174,7 +196,13 @@ describe('consent', () => { contextFileName: 'new-context.md', }; const requestConsent = vi.fn().mockResolvedValue(true); - await maybeRequestConsentOrFail(newConfig, requestConsent, prevConfig); + await maybeRequestConsentOrFail( + newConfig, + requestConsent, + false, + prevConfig, + false, + ); expect(requestConsent).toHaveBeenCalledTimes(1); }); @@ -185,7 +213,41 @@ describe('consent', () => { excludeTools: ['new-tool'], }; const requestConsent = vi.fn().mockResolvedValue(true); - await maybeRequestConsentOrFail(newConfig, requestConsent, prevConfig); + await maybeRequestConsentOrFail( + newConfig, + requestConsent, + false, + prevConfig, + false, + ); + expect(requestConsent).toHaveBeenCalledTimes(1); + }); + + it('should include warning when hooks are present', async () => { + const requestConsent = vi.fn().mockResolvedValue(true); + await maybeRequestConsentOrFail( + baseConfig, + requestConsent, + true, + undefined, + ); + + expect(requestConsent).toHaveBeenCalledWith( + expect.stringContaining( + '⚠️ This extension contains Hooks which can automatically execute commands.', + ), + ); + }); + + it('should request consent if hooks status changes', async () => { + const requestConsent = vi.fn().mockResolvedValue(true); + await maybeRequestConsentOrFail( + baseConfig, + requestConsent, + true, + baseConfig, + false, + ); expect(requestConsent).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/cli/src/config/extensions/consent.ts b/packages/cli/src/config/extensions/consent.ts index ee040cbdee..36297e6386 100644 --- a/packages/cli/src/config/extensions/consent.ts +++ b/packages/cli/src/config/extensions/consent.ts @@ -103,7 +103,10 @@ async function promptForConsentInteractive( * Builds a consent string for installing an extension based on it's * extensionConfig. */ -function extensionConsentString(extensionConfig: ExtensionConfig): string { +function extensionConsentString( + extensionConfig: ExtensionConfig, + hasHooks: boolean, +): string { const sanitizedConfig = escapeAnsiCtrlCodes(extensionConfig); const output: string[] = []; const mcpServerEntries = Object.entries(sanitizedConfig.mcpServers || {}); @@ -130,6 +133,11 @@ function extensionConsentString(extensionConfig: ExtensionConfig): string { `This extension will exclude the following core tools: ${sanitizedConfig.excludeTools}`, ); } + if (hasHooks) { + output.push( + '⚠️ This extension contains Hooks which can automatically execute commands.', + ); + } return output.join('\n'); } @@ -145,12 +153,15 @@ function extensionConsentString(extensionConfig: ExtensionConfig): string { export async function maybeRequestConsentOrFail( extensionConfig: ExtensionConfig, requestConsent: (consent: string) => Promise, + hasHooks: boolean, previousExtensionConfig?: ExtensionConfig, + previousHasHooks?: boolean, ) { - const extensionConsent = extensionConsentString(extensionConfig); + const extensionConsent = extensionConsentString(extensionConfig, hasHooks); if (previousExtensionConfig) { const previousExtensionConsent = extensionConsentString( previousExtensionConfig, + previousHasHooks ?? false, ); if (previousExtensionConsent === extensionConsent) { return;