Security: Project-level hook warnings (#15470)

This commit is contained in:
Sehoon Shon
2025-12-23 16:10:46 -05:00
committed by GitHub
parent 873d10df42
commit e6344a8c24
13 changed files with 505 additions and 23 deletions
+13 -2
View File
@@ -8,7 +8,6 @@ import yargs from 'yargs/yargs';
import { hideBin } from 'yargs/helpers';
import process from 'node:process';
import { mcpCommand } from '../commands/mcp.js';
import type { OutputFormat } from '@google/gemini-cli-core';
import { extensionsCommand } from '../commands/extensions.js';
import { hooksCommand } from '../commands/hooks.js';
import {
@@ -33,6 +32,9 @@ import {
WEB_FETCH_TOOL_NAME,
getVersion,
PREVIEW_GEMINI_MODEL_AUTO,
type HookDefinition,
type HookEventName,
type OutputFormat,
} from '@google/gemini-cli-core';
import type { Settings } from './settings.js';
import { saveModelChange, loadSettings } from './settings.js';
@@ -380,12 +382,20 @@ export function isDebugMode(argv: CliArgs): boolean {
);
}
export interface LoadCliConfigOptions {
cwd?: string;
projectHooks?: { [K in HookEventName]?: HookDefinition[] } & {
disabled?: string[];
};
}
export async function loadCliConfig(
settings: Settings,
sessionId: string,
argv: CliArgs,
cwd: string = process.cwd(),
options: LoadCliConfigOptions = {},
): Promise<Config> {
const { cwd = process.cwd(), projectHooks } = options;
const debugMode = isDebugMode(argv);
const loadedSettings = loadSettings(cwd);
@@ -696,6 +706,7 @@ export async function loadCliConfig(
// TODO: loading of hooks based on workspace trust
enableHooks: settings.tools?.enableHooks ?? false,
hooks: settings.hooks || {},
projectHooks: projectHooks || {},
onModelChange: (model: string) => saveModelChange(loadedSettings, model),
});
}
+12
View File
@@ -114,6 +114,7 @@ vi.mock('./config/settings.js', () => ({
security: { auth: {} },
ui: {},
},
workspace: { settings: {} },
setValue: vi.fn(),
forScope: () => ({ settings: {}, originalSettings: {}, path: '' }),
errors: [],
@@ -289,6 +290,7 @@ describe('gemini.tsx main function', () => {
security: { auth: {} },
ui: {},
},
workspace: { settings: {} },
setValue: vi.fn(),
forScope: () => ({ settings: {}, originalSettings: {}, path: '' }),
} as never);
@@ -522,6 +524,7 @@ describe('gemini.tsx main function kitty protocol', () => {
security: { auth: {} },
ui: {},
},
workspace: { settings: {} },
setValue: vi.fn(),
forScope: () => ({ settings: {}, originalSettings: {}, path: '' }),
} as never);
@@ -583,6 +586,7 @@ describe('gemini.tsx main function kitty protocol', () => {
security: { auth: {} },
ui: {},
},
workspace: { settings: {} },
setValue: vi.fn(),
forScope: () => ({ settings: {}, originalSettings: {}, path: '' }),
errors: [],
@@ -669,6 +673,7 @@ describe('gemini.tsx main function kitty protocol', () => {
security: { auth: {} },
ui: {},
},
workspace: { settings: {} },
setValue: vi.fn(),
forScope: () => ({ settings: {}, originalSettings: {}, path: '' }),
errors: [],
@@ -737,6 +742,7 @@ describe('gemini.tsx main function kitty protocol', () => {
security: { auth: {} },
ui: { theme: 'non-existent-theme' },
},
workspace: { settings: {} },
setValue: vi.fn(),
forScope: () => ({ settings: {}, originalSettings: {}, path: '' }),
errors: [],
@@ -819,6 +825,7 @@ describe('gemini.tsx main function kitty protocol', () => {
vi.mocked(loadSettings).mockReturnValue({
merged: { advanced: {}, security: { auth: {} }, ui: { theme: 'test' } },
workspace: { settings: {} },
setValue: vi.fn(),
forScope: () => ({ settings: {}, originalSettings: {}, path: '' }),
errors: [],
@@ -898,6 +905,7 @@ describe('gemini.tsx main function kitty protocol', () => {
vi.mocked(loadSettings).mockReturnValue({
merged: { advanced: {}, security: { auth: {} }, ui: {} },
workspace: { settings: {} },
setValue: vi.fn(),
forScope: () => ({ settings: {}, originalSettings: {}, path: '' }),
errors: [],
@@ -971,6 +979,7 @@ describe('gemini.tsx main function kitty protocol', () => {
vi.mocked(loadSettings).mockReturnValue({
merged: { advanced: {}, security: { auth: {} }, ui: {} },
workspace: { settings: {} },
setValue: vi.fn(),
forScope: () => ({ settings: {}, originalSettings: {}, path: '' }),
errors: [],
@@ -1118,6 +1127,7 @@ describe('gemini.tsx main function exit codes', () => {
security: { auth: { selectedType: 'google', useExternal: false } },
ui: {},
},
workspace: { settings: {} },
errors: [],
} as never);
vi.mocked(parseArguments).mockResolvedValue({} as unknown as CliArgs);
@@ -1174,6 +1184,7 @@ describe('gemini.tsx main function exit codes', () => {
} as unknown as Config);
vi.mocked(loadSettings).mockReturnValue({
merged: { security: { auth: {} }, ui: {} },
workspace: { settings: {} },
errors: [],
} as never);
vi.mocked(parseArguments).mockResolvedValue({
@@ -1237,6 +1248,7 @@ describe('gemini.tsx main function exit codes', () => {
} as unknown as Config);
vi.mocked(loadSettings).mockReturnValue({
merged: { security: { auth: {} }, ui: {} },
workspace: { settings: {} },
errors: [],
} as never);
vi.mocked(parseArguments).mockResolvedValue({} as unknown as CliArgs);
+4 -1
View File
@@ -393,6 +393,7 @@ export async function main() {
settings.merged,
sessionId,
argv,
{ projectHooks: settings.workspace.settings.hooks },
);
if (
@@ -464,7 +465,9 @@ export async function main() {
// may have side effects.
{
const loadConfigHandle = startupProfiler.start('load_cli_config');
const config = await loadCliConfig(settings.merged, sessionId, argv);
const config = await loadCliConfig(settings.merged, sessionId, argv, {
projectHooks: settings.workspace.settings.hooks,
});
loadConfigHandle?.end();
// Register config for telemetry shutdown
+2
View File
@@ -60,6 +60,7 @@ vi.mock('./config/settings.js', async (importOriginal) => {
...actual,
loadSettings: vi.fn().mockReturnValue({
merged: { advanced: {}, security: { auth: {} }, ui: {} },
workspace: { settings: {} },
setValue: vi.fn(),
forScope: () => ({ settings: {}, originalSettings: {}, path: '' }),
errors: [],
@@ -171,6 +172,7 @@ describe('gemini.tsx main function cleanup', () => {
vi.mocked(loadSettings).mockReturnValue({
merged: { advanced: {}, security: { auth: {} }, ui: {} },
workspace: { settings: {} },
setValue: vi.fn(),
forScope: () => ({ settings: {}, originalSettings: {}, path: '' }),
errors: [],
@@ -180,7 +180,7 @@ describe('GeminiAgent', () => {
}),
'test-session-id',
mockArgv,
'/tmp',
{ cwd: '/tmp' },
);
});
@@ -218,7 +218,7 @@ export class GeminiAgent {
const settings = { ...this.settings.merged, mcpServers: mergedMcpServers };
const config = await loadCliConfig(settings, sessionId, this.argv, cwd);
const config = await loadCliConfig(settings, sessionId, this.argv, { cwd });
await config.initialize();
startupProfiler.flush(config);
+17 -7
View File
@@ -327,13 +327,10 @@ export interface ConfigParameters {
modelConfigServiceConfig?: ModelConfigServiceConfig;
enableHooks?: boolean;
experiments?: Experiments;
hooks?:
| {
[K in HookEventName]?: HookDefinition[];
}
| ({
[K in HookEventName]?: HookDefinition[];
} & { disabled?: string[] });
hooks?: { [K in HookEventName]?: HookDefinition[] } & { disabled?: string[] };
projectHooks?: { [K in HookEventName]?: HookDefinition[] } & {
disabled?: string[];
};
previewFeatures?: boolean;
enableAgents?: boolean;
experimentalJitContext?: boolean;
@@ -454,6 +451,9 @@ export class Config {
private readonly hooks:
| { [K in HookEventName]?: HookDefinition[] }
| undefined;
private readonly projectHooks:
| ({ [K in HookEventName]?: HookDefinition[] } & { disabled?: string[] })
| undefined;
private readonly disabledHooks: string[];
private experiments: Experiments | undefined;
private experimentsPromise: Promise<void> | undefined;
@@ -624,6 +624,7 @@ export class Config {
this.retryFetchErrors = params.retryFetchErrors ?? false;
this.disableYoloMode = params.disableYoloMode ?? false;
this.hooks = params.hooks;
this.projectHooks = params.projectHooks;
this.experiments = params.experiments;
this.onModelChange = params.onModelChange;
@@ -1714,6 +1715,15 @@ export class Config {
return this.hooks;
}
/**
* Get project-specific hooks configuration
*/
getProjectHooks():
| ({ [K in HookEventName]?: HookDefinition[] } & { disabled?: string[] })
| undefined {
return this.projectHooks;
}
/**
* Get disabled hooks list
*/
+2 -11
View File
@@ -6,7 +6,7 @@
import type { HookRegistry, HookRegistryEntry } from './hookRegistry.js';
import type { HookExecutionPlan } from './types.js';
import type { HookEventName } from './types.js';
import { getHookKey, type HookEventName } from './types.js';
import { debugLogger } from '../utils/debugLogger.js';
/**
@@ -124,7 +124,7 @@ export class HookPlanner {
const deduplicated: HookRegistryEntry[] = [];
for (const entry of entries) {
const key = this.getHookKey(entry);
const key = getHookKey(entry.config);
if (!seen.has(key)) {
seen.add(key);
@@ -136,15 +136,6 @@ export class HookPlanner {
return deduplicated;
}
/**
* Generate a unique key for a hook entry
*/
private getHookKey(entry: HookRegistryEntry): string {
const name = entry.config.name || '';
const command = entry.config.command || '';
return `${name}:${command}`;
}
}
/**
@@ -30,6 +30,25 @@ vi.mock('../utils/debugLogger.js', () => ({
debugLogger: mockDebugLogger,
}));
const { mockTrustedHooksManager, mockCoreEvents } = vi.hoisted(() => ({
mockTrustedHooksManager: {
getUntrustedHooks: vi.fn().mockReturnValue([]),
trustHooks: vi.fn(),
},
mockCoreEvents: {
emitConsoleLog: vi.fn(),
emitFeedback: vi.fn(),
},
}));
vi.mock('./trustedHooks.js', () => ({
TrustedHooksManager: vi.fn(() => mockTrustedHooksManager),
}));
vi.mock('../utils/events.js', () => ({
coreEvents: mockCoreEvents,
}));
describe('HookRegistry', () => {
let hookRegistry: HookRegistry;
let mockConfig: Config;
@@ -46,8 +65,10 @@ describe('HookRegistry', () => {
storage: mockStorage,
getExtensions: vi.fn().mockReturnValue([]),
getHooks: vi.fn().mockReturnValue({}),
getProjectHooks: vi.fn().mockReturnValue({}),
getDisabledHooks: vi.fn().mockReturnValue([]),
isTrustedFolder: vi.fn().mockReturnValue(true),
getProjectRoot: vi.fn().mockReturnValue('/project'),
} as unknown as Config;
hookRegistry = new HookRegistry(mockConfig);
@@ -562,6 +583,7 @@ describe('HookRegistry', () => {
},
],
};
mockTrustedHooksManager.getUntrustedHooks.mockReturnValue([]);
vi.mocked(mockConfig.getHooks).mockReturnValue(
malformedConfig as unknown as {
@@ -624,4 +646,88 @@ describe('HookRegistry', () => {
);
});
});
describe('project hook warnings', () => {
it('should check for untrusted project hooks when folder is trusted', async () => {
const projectHooks = {
BeforeTool: [
{
hooks: [
{
type: 'command',
command: './hooks/untrusted.sh',
},
],
},
],
};
vi.mocked(mockConfig.getHooks).mockReturnValue(
projectHooks as unknown as { [K in HookEventName]?: HookDefinition[] },
);
vi.mocked(mockConfig.getProjectHooks).mockReturnValue(
projectHooks as unknown as { [K in HookEventName]?: HookDefinition[] },
);
// Simulate untrusted hooks found
mockTrustedHooksManager.getUntrustedHooks.mockReturnValue([
'./hooks/untrusted.sh',
]);
await hookRegistry.initialize();
expect(mockTrustedHooksManager.getUntrustedHooks).toHaveBeenCalledWith(
'/project',
projectHooks,
);
expect(mockCoreEvents.emitFeedback).toHaveBeenCalledWith(
'warning',
expect.stringContaining(
'WARNING: The following project-level hooks have been detected',
),
);
expect(mockTrustedHooksManager.trustHooks).toHaveBeenCalledWith(
'/project',
projectHooks,
);
});
it('should not warn if hooks are already trusted', async () => {
const projectHooks = {
BeforeTool: [
{
hooks: [
{
type: 'command',
command: './hooks/trusted.sh',
},
],
},
],
};
vi.mocked(mockConfig.getHooks).mockReturnValue(
projectHooks as unknown as { [K in HookEventName]?: HookDefinition[] },
);
vi.mocked(mockConfig.getProjectHooks).mockReturnValue(
projectHooks as unknown as { [K in HookEventName]?: HookDefinition[] },
);
// Simulate no untrusted hooks
mockTrustedHooksManager.getUntrustedHooks.mockReturnValue([]);
await hookRegistry.initialize();
expect(mockCoreEvents.emitFeedback).not.toHaveBeenCalled();
expect(mockTrustedHooksManager.trustHooks).not.toHaveBeenCalled();
});
it('should not check for untrusted hooks if folder is not trusted', async () => {
vi.mocked(mockConfig.isTrustedFolder).mockReturnValue(false);
await hookRegistry.initialize();
expect(mockTrustedHooksManager.getUntrustedHooks).not.toHaveBeenCalled();
});
});
});
+39
View File
@@ -8,6 +8,8 @@ import type { Config } from '../config/config.js';
import type { HookDefinition, HookConfig } from './types.js';
import { HookEventName, ConfigSource } from './types.js';
import { debugLogger } from '../utils/debugLogger.js';
import { TrustedHooksManager } from './trustedHooks.js';
import { coreEvents } from '../utils/events.js';
/**
* Hook registry entry with source information
@@ -94,10 +96,47 @@ export class HookRegistry {
return entry.config.name || entry.config.command || 'unknown-command';
}
/**
* Check for untrusted project hooks and warn the user
*/
private checkProjectHooksTrust(): void {
const projectHooks = this.config.getProjectHooks();
if (!projectHooks) return;
try {
const trustedHooksManager = new TrustedHooksManager();
const untrusted = trustedHooksManager.getUntrustedHooks(
this.config.getProjectRoot(),
projectHooks,
);
if (untrusted.length > 0) {
const message = `WARNING: The following project-level hooks have been detected in this workspace:
${untrusted.map((h) => ` - ${h}`).join('\n')}
These hooks will be executed. If you did not configure these hooks or do not trust this project,
please review the project settings (.gemini/settings.json) and remove them.`;
coreEvents.emitFeedback('warning', message);
// Trust them so we don't warn again
trustedHooksManager.trustHooks(
this.config.getProjectRoot(),
projectHooks,
);
}
} catch (error) {
debugLogger.warn('Failed to check project hooks trust', error);
}
}
/**
* Process hooks from the config that was already loaded by the CLI
*/
private processHooksFromConfig(): void {
if (this.config.isTrustedFolder()) {
this.checkProjectHooksTrust();
}
// Get hooks from the main config (this comes from the merged settings)
const configHooks = this.config.getHooks();
if (configHooks) {
@@ -0,0 +1,183 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import { describe, it, expect, vi, beforeEach } from 'vitest';
import * as fs from 'node:fs';
import { TrustedHooksManager } from './trustedHooks.js';
import { Storage } from '../config/storage.js';
import { HookEventName, HookType } from './types.js';
vi.mock('node:fs');
vi.mock('../config/storage.js');
vi.mock('../utils/debugLogger.js', () => ({
debugLogger: {
warn: vi.fn(),
error: vi.fn(),
log: vi.fn(),
debug: vi.fn(),
},
}));
describe('TrustedHooksManager', () => {
beforeEach(() => {
vi.resetAllMocks();
vi.mocked(Storage.getGlobalGeminiDir).mockReturnValue('/mock/home/.gemini');
});
describe('initialization', () => {
it('should load existing trusted hooks', () => {
const existingData = {
'/project/a': ['hook1:cmd1'],
};
vi.mocked(fs.existsSync).mockReturnValue(true);
vi.mocked(fs.readFileSync).mockReturnValue(JSON.stringify(existingData));
const manager = new TrustedHooksManager();
const untrusted = manager.getUntrustedHooks('/project/a', {
[HookEventName.BeforeTool]: [
{
hooks: [{ type: HookType.Command, command: 'cmd1', name: 'hook1' }],
},
],
});
expect(untrusted).toHaveLength(0);
});
it('should handle missing config file', () => {
vi.mocked(fs.existsSync).mockReturnValue(false);
const manager = new TrustedHooksManager();
const untrusted = manager.getUntrustedHooks('/project/a', {
[HookEventName.BeforeTool]: [
{
hooks: [{ type: HookType.Command, command: 'cmd1', name: 'hook1' }],
},
],
});
expect(untrusted).toEqual(['hook1']);
});
});
describe('getUntrustedHooks', () => {
it('should return names of untrusted hooks', () => {
vi.mocked(fs.existsSync).mockReturnValue(false);
const manager = new TrustedHooksManager();
const projectHooks = {
[HookEventName.BeforeTool]: [
{
hooks: [
{ name: 'trusted-hook', type: HookType.Command, command: 'cmd1' },
{ name: 'new-hook', type: HookType.Command, command: 'cmd2' },
],
},
],
};
// Initially both are untrusted
expect(manager.getUntrustedHooks('/project', projectHooks)).toEqual([
'trusted-hook',
'new-hook',
]);
// Trust one
manager.trustHooks('/project', {
[HookEventName.BeforeTool]: [
{
hooks: [
{ name: 'trusted-hook', type: HookType.Command, command: 'cmd1' },
],
},
],
});
// Only the other one is untrusted
expect(manager.getUntrustedHooks('/project', projectHooks)).toEqual([
'new-hook',
]);
});
it('should use command if name is missing', () => {
vi.mocked(fs.existsSync).mockReturnValue(false);
const manager = new TrustedHooksManager();
const projectHooks = {
[HookEventName.BeforeTool]: [
{
hooks: [{ type: HookType.Command, command: './script.sh' }],
},
],
};
expect(manager.getUntrustedHooks('/project', projectHooks)).toEqual([
'./script.sh',
]);
});
it('should detect change in command as untrusted', () => {
vi.mocked(fs.existsSync).mockReturnValue(false);
const manager = new TrustedHooksManager();
const originalHook = {
[HookEventName.BeforeTool]: [
{
hooks: [
{ name: 'my-hook', type: HookType.Command, command: 'old-cmd' },
],
},
],
};
const updatedHook = {
[HookEventName.BeforeTool]: [
{
hooks: [
{ name: 'my-hook', type: HookType.Command, command: 'new-cmd' },
],
},
],
};
manager.trustHooks('/project', originalHook);
expect(manager.getUntrustedHooks('/project', updatedHook)).toEqual([
'my-hook',
]);
});
});
describe('persistence', () => {
it('should save to file when trusting hooks', () => {
vi.mocked(fs.existsSync).mockReturnValue(false);
const manager = new TrustedHooksManager();
manager.trustHooks('/project', {
[HookEventName.BeforeTool]: [
{
hooks: [{ name: 'hook1', type: HookType.Command, command: 'cmd1' }],
},
],
});
expect(fs.writeFileSync).toHaveBeenCalledWith(
expect.stringContaining('trusted_hooks.json'),
expect.stringContaining('hook1:cmd1'),
);
});
it('should create directory if missing on save', () => {
vi.mocked(fs.existsSync).mockReturnValue(false);
const manager = new TrustedHooksManager();
manager.trustHooks('/project', {});
expect(fs.mkdirSync).toHaveBeenCalledWith(expect.any(String), {
recursive: true,
});
});
});
});
+116
View File
@@ -0,0 +1,116 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import * as fs from 'node:fs';
import * as path from 'node:path';
import { Storage } from '../config/storage.js';
import {
getHookKey,
type HookDefinition,
type HookEventName,
} from './types.js';
import { debugLogger } from '../utils/debugLogger.js';
interface TrustedHooksConfig {
[projectPath: string]: string[]; // Array of trusted hook keys (name:command)
}
export class TrustedHooksManager {
private configPath: string;
private trustedHooks: TrustedHooksConfig = {};
constructor() {
this.configPath = path.join(
Storage.getGlobalGeminiDir(),
'trusted_hooks.json',
);
this.load();
}
private load(): void {
try {
if (fs.existsSync(this.configPath)) {
const content = fs.readFileSync(this.configPath, 'utf-8');
this.trustedHooks = JSON.parse(content);
}
} catch (error) {
debugLogger.warn('Failed to load trusted hooks config', error);
this.trustedHooks = {};
}
}
private save(): void {
try {
const dir = path.dirname(this.configPath);
if (!fs.existsSync(dir)) {
fs.mkdirSync(dir, { recursive: true });
}
fs.writeFileSync(
this.configPath,
JSON.stringify(this.trustedHooks, null, 2),
);
} catch (error) {
debugLogger.warn('Failed to save trusted hooks config', error);
}
}
/**
* Get untrusted hooks for a project
* @param projectPath Absolute path to the project root
* @param hooks The hooks configuration to check
* @returns List of untrusted hook commands/names
*/
getUntrustedHooks(
projectPath: string,
hooks: { [K in HookEventName]?: HookDefinition[] },
): string[] {
const trustedKeys = new Set(this.trustedHooks[projectPath] || []);
const untrusted: string[] = [];
for (const eventName of Object.keys(hooks)) {
const definitions = hooks[eventName as HookEventName];
if (!Array.isArray(definitions)) continue;
for (const def of definitions) {
if (!def || !Array.isArray(def.hooks)) continue;
for (const hook of def.hooks) {
const key = getHookKey(hook);
if (!trustedKeys.has(key)) {
// Return friendly name or command
untrusted.push(hook.name || hook.command || 'unknown-hook');
}
}
}
}
return Array.from(new Set(untrusted)); // Deduplicate
}
/**
* Trust all provided hooks for a project
*/
trustHooks(
projectPath: string,
hooks: { [K in HookEventName]?: HookDefinition[] },
): void {
const currentTrusted = new Set(this.trustedHooks[projectPath] || []);
for (const eventName of Object.keys(hooks)) {
const definitions = hooks[eventName as HookEventName];
if (!Array.isArray(definitions)) continue;
for (const def of definitions) {
if (!def || !Array.isArray(def.hooks)) continue;
for (const hook of def.hooks) {
currentTrusted.add(getHookKey(hook));
}
}
}
this.trustedHooks[projectPath] = Array.from(currentTrusted);
this.save();
}
}
+9
View File
@@ -74,6 +74,15 @@ export enum HookType {
Command = 'command',
}
/**
* Generate a unique key for a hook configuration
*/
export function getHookKey(hook: HookConfig): string {
const name = hook.name || '';
const command = hook.command || '';
return `${name}:${command}`;
}
/**
* Decision types for hook outputs
*/