diff --git a/packages/core/src/policy/policy-updater.test.ts b/packages/core/src/policy/policy-updater.test.ts index 0a6aa1fa62..965d83be63 100644 --- a/packages/core/src/policy/policy-updater.test.ts +++ b/packages/core/src/policy/policy-updater.test.ts @@ -28,7 +28,7 @@ vi.mock('../utils/shell-utils.js', () => ({ getCommandSegments: vi.fn(), stripShellWrapper: vi.fn(), hasRedirection: vi.fn(), - isNakedSensitiveCommand: vi.fn(), + isArgumentRestrictedCommand: vi.fn(), })); interface ParsedPolicy { rule?: Array<{ @@ -261,7 +261,7 @@ describe('ShellToolInvocation Policy Update', () => { (c: string) => c, ); vi.mocked(shellUtils.hasRedirection).mockReturnValue(false); - vi.mocked(shellUtils.isNakedSensitiveCommand).mockReturnValue(false); + vi.mocked(shellUtils.isArgumentRestrictedCommand).mockReturnValue(false); }); it('should extract multiple command segments for chained commands', () => { diff --git a/packages/core/src/tools/glob.ts b/packages/core/src/tools/glob.ts index 9cef63759d..7dd1b0068c 100644 --- a/packages/core/src/tools/glob.ts +++ b/packages/core/src/tools/glob.ts @@ -22,7 +22,6 @@ import { type Config } from '../config/config.js'; import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js'; import { ToolErrorType } from './tool-error.js'; import { GLOB_TOOL_NAME, GLOB_DISPLAY_NAME } from './tool-names.js'; -import { buildPatternArgsPattern } from '../policy/utils.js'; import { getErrorMessage } from '../utils/errors.js'; import { debugLogger } from '../utils/debugLogger.js'; import { GLOB_DEFINITION } from './definitions/coreTools.js'; @@ -124,9 +123,7 @@ class GlobToolInvocation extends BaseToolInvocation< override getPolicyUpdateOptions( _outcome: ToolConfirmationOutcome, ): PolicyUpdateOptions | undefined { - return { - argsPattern: buildPatternArgsPattern(this.params.pattern), - }; + return {}; } async execute(signal: AbortSignal): Promise { diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index ac7dc6cf02..c595e7c659 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -31,7 +31,6 @@ import type { Config } from '../config/config.js'; import type { FileExclusions } from '../utils/ignorePatterns.js'; import { ToolErrorType } from './tool-error.js'; import { GREP_TOOL_NAME, GREP_DISPLAY_NAME } from './tool-names.js'; -import { buildPatternArgsPattern } from '../policy/utils.js'; import { debugLogger } from '../utils/debugLogger.js'; import { GREP_DEFINITION } from './definitions/coreTools.js'; import { resolveToolDeclaration } from './definitions/resolver.js'; @@ -306,9 +305,7 @@ class GrepToolInvocation extends BaseToolInvocation< override getPolicyUpdateOptions( _outcome: ToolConfirmationOutcome, ): PolicyUpdateOptions | undefined { - return { - argsPattern: buildPatternArgsPattern(this.params.pattern), - }; + return {}; } /** diff --git a/packages/core/src/tools/ls.ts b/packages/core/src/tools/ls.ts index b8e2e6a803..d398ab57d8 100644 --- a/packages/core/src/tools/ls.ts +++ b/packages/core/src/tools/ls.ts @@ -21,7 +21,6 @@ import type { Config } from '../config/config.js'; import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js'; import { ToolErrorType } from './tool-error.js'; import { LS_TOOL_NAME, LS_DISPLAY_NAME } from './tool-names.js'; -import { buildDirPathArgsPattern } from '../policy/utils.js'; import { debugLogger } from '../utils/debugLogger.js'; import { LS_DEFINITION } from './definitions/coreTools.js'; import { resolveToolDeclaration } from './definitions/resolver.js'; @@ -130,9 +129,7 @@ class LSToolInvocation extends BaseToolInvocation { override getPolicyUpdateOptions( _outcome: ToolConfirmationOutcome, ): PolicyUpdateOptions | undefined { - return { - argsPattern: buildDirPathArgsPattern(this.params.dir_path), - }; + return {}; } // Helper for consistent error formatting diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 399f5f7ab6..7e7231b7a8 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -46,7 +46,7 @@ import { stripShellWrapper, parseCommandDetails, hasRedirection, - isNakedSensitiveCommand, + isArgumentRestrictedCommand, normalizeCommand, } from '../utils/shell-utils.js'; import { buildParamArgsPattern } from '../policy/utils.js'; @@ -238,11 +238,10 @@ export class ShellToolInvocation extends BaseToolInvocation< const segments = getCommandSegments(command); const allowRedirection = hasRedirection(command) ? true : undefined; - // Filter out "naked" sensitive commands to prevent over-broad matching + // Filter out "naked" restricted commands to prevent over-broad matching const safeSegments = segments.filter( - (seg) => !isNakedSensitiveCommand(seg), + (seg) => !isArgumentRestrictedCommand(seg), ); - if (safeSegments.length > 0) { return { commandPrefix: safeSegments, allowRedirection }; } diff --git a/packages/core/src/tools/tool-names.ts b/packages/core/src/tools/tool-names.ts index 224f2ab0d5..0a165aba98 100644 --- a/packages/core/src/tools/tool-names.ts +++ b/packages/core/src/tools/tool-names.ts @@ -171,11 +171,8 @@ export const EDIT_TOOL_NAMES = new Set([EDIT_TOOL_NAME, WRITE_FILE_TOOL_NAME]); * when granting persistent or session-wide approval. */ export const TOOLS_REQUIRING_NARROWING = new Set([ - GLOB_TOOL_NAME, - GREP_TOOL_NAME, READ_MANY_FILES_TOOL_NAME, READ_FILE_TOOL_NAME, - LS_TOOL_NAME, WRITE_FILE_TOOL_NAME, EDIT_TOOL_NAME, SHELL_TOOL_NAME, diff --git a/packages/core/src/tools/trackerTools.test.ts b/packages/core/src/tools/trackerTools.test.ts index 6513a71dd5..3f042b3439 100644 --- a/packages/core/src/tools/trackerTools.test.ts +++ b/packages/core/src/tools/trackerTools.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { Config } from '../config/config.js'; import { MessageBus } from '../confirmation-bus/message-bus.js'; import type { PolicyEngine } from '../policy/policy-engine.js'; @@ -30,6 +30,7 @@ describe('Tracker Tools Integration', () => { beforeEach(async () => { tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'tracker-tools-test-')); + vi.stubEnv('GEMINI_CLI_HOME', tempDir); config = new Config({ sessionId: 'test-session', targetDir: tempDir, @@ -41,6 +42,7 @@ describe('Tracker Tools Integration', () => { }); afterEach(async () => { + vi.unstubAllEnvs(); await fs.rm(tempDir, { recursive: true, force: true }); }); diff --git a/packages/core/src/utils/shell-utils.getCommandSegments.test.ts b/packages/core/src/utils/shell-utils.getCommandSegments.test.ts new file mode 100644 index 0000000000..f5517daff0 --- /dev/null +++ b/packages/core/src/utils/shell-utils.getCommandSegments.test.ts @@ -0,0 +1,81 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeAll } from 'vitest'; +import { + getCommandSegments, + initializeShellParsers, + isArgumentRestrictedCommand, +} from './shell-utils.js'; + +describe('getCommandSegments', () => { + beforeAll(async () => { + await initializeShellParsers(); + }); + + it('should include first positional argument for restricted commands', () => { + expect(getCommandSegments('git status')).toEqual([['git', 'status']]); + expect(getCommandSegments('npm install')).toEqual([['npm', 'install']]); + expect(getCommandSegments('rm -rf /tmp/foo')).toEqual([['rm', '/tmp/foo']]); + expect(getCommandSegments('node script.js')).toEqual([ + ['node', 'script.js'], + ]); + }); + + it('should only include the binary for unrestricted commands', () => { + expect(getCommandSegments('grep pattern file.txt')).toEqual([['grep']]); + expect(getCommandSegments('sed "s/a/b/g" file.txt')).toEqual([['sed']]); + expect(getCommandSegments('ls -la /tmp')).toEqual([['ls']]); + expect(getCommandSegments('cat file.txt')).toEqual([['cat']]); + expect(getCommandSegments('head -n 10 file.txt')).toEqual([['head']]); + expect(getCommandSegments('rg "search term" .')).toEqual([['rg']]); + }); + + it('should handle chained commands with mixed sensitivity', () => { + expect( + getCommandSegments('grep pattern file.txt && git add file.txt'), + ).toEqual([['grep'], ['git', 'add']]); + }); + + it('should handle pipes', () => { + expect(getCommandSegments('ls | grep pattern')).toEqual([['ls'], ['grep']]); + }); + + it('should handle subshells', () => { + // Current behavior for subshells might vary depending on how they are parsed, + // but we expect at least the outer commands to be captured. + expect(getCommandSegments('(cd /tmp && ls)')).toEqual([['cd'], ['ls']]); + }); + + it('should ignore flags but keep positional arguments for restricted commands', () => { + expect(getCommandSegments('git --no-pager log --oneline')).toEqual([ + ['git', 'log'], + ]); + expect(getCommandSegments('npm run test -- --grep="foo"')).toEqual([ + ['npm', 'run'], + ]); + }); +}); + +describe('isArgumentRestrictedCommand', () => { + it('should return true for naked restricted commands', () => { + expect(isArgumentRestrictedCommand(['git'])).toBe(true); + expect(isArgumentRestrictedCommand(['rm'])).toBe(true); + expect(isArgumentRestrictedCommand(['node'])).toBe(true); + }); + + it('should return false for restricted commands with arguments', () => { + expect(isArgumentRestrictedCommand(['git', 'status'])).toBe(false); + expect(isArgumentRestrictedCommand(['rm', '/tmp'])).toBe(false); + }); + + it('should return false for unrestricted commands (naked or not)', () => { + expect(isArgumentRestrictedCommand(['grep'])).toBe(false); + expect(isArgumentRestrictedCommand(['ls'])).toBe(false); + expect(isArgumentRestrictedCommand(['cd'])).toBe(false); + expect(isArgumentRestrictedCommand(['grep', 'foo'])).toBe(false); + }); +}); diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index b3985db0e7..92f47dd4d5 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -797,9 +797,40 @@ export function getCommandRoots(command: string): string[] { } /** - * Binaries that are considered sensitive and should not be allowed to be saved as "naked" rules. + * Binaries that are generally safe and whose arguments are typically variable and non-sensitive. + * Rules for these binaries should be saved "naked" (binary only) without positional arguments. */ -export const SENSITIVE_BINARIES = new Set([ +export const ARGUMENT_UNRESTRICTED_BINARIES = new Set([ + 'grep', + 'sed', + 'awk', + 'rg', + 'ls', + 'cat', + 'head', + 'tail', + 'wc', + 'sort', + 'uniq', + 'cut', + 'tr', + 'echo', + 'printf', + 'true', + 'false', + 'pwd', + 'whoami', + 'id', + 'hostname', + 'uname', + 'cd', +]); + +/** + * Binaries that are considered sensitive and should not be allowed to be saved as "naked" rules. + * Rules for these binaries must include at least one positional argument (e.g., "git diff", "rm /tmp"). + */ +export const ARGUMENT_RESTRICTED_BINARIES = new Set([ 'git', 'node', 'npm', @@ -825,9 +856,6 @@ export const SENSITIVE_BINARIES = new Set([ 'mv', 'cp', 'find', - 'grep', - 'sed', - 'awk', 'curl', 'wget', 'ssh', @@ -836,18 +864,18 @@ export const SENSITIVE_BINARIES = new Set([ ]); /** - * Checks if a command sequence represents a "naked" sensitive command. + * Checks if a command sequence represents a "naked" restricted command. * A command is naked if it only contains the binary name without any subcommands or scripts. * * @param sequence - The command sequence (e.g., ['git', 'log']). - * @returns true if the command is a naked sensitive command. + * @returns true if the command is a naked restricted command. */ -export function isNakedSensitiveCommand(sequence: string[]): boolean { +export function isArgumentRestrictedCommand(sequence: string[]): boolean { if (sequence.length === 0) return false; const binary = sequence[0]; // A command is "naked" if it has only one token (the binary itself) - // and that binary is in the sensitive list. - return sequence.length === 1 && SENSITIVE_BINARIES.has(binary); + // and that binary is in the restricted list. + return sequence.length === 1 && ARGUMENT_RESTRICTED_BINARIES.has(binary); } /** @@ -883,11 +911,20 @@ export function getCommandSegments(command: string): string[][] { if (node.type === 'command') { const sequence: string[] = []; const nameNode = node.childForFieldName('name'); + let binaryName = ''; if (nameNode) { - sequence.push(normalizeCommandName(nameNode.text)); + binaryName = normalizeCommandName(nameNode.text); + sequence.push(binaryName); } + // Safe utilities like grep/sed should be allowed "nakedly" (binary only) + // because their arguments are highly variable and non-sensitive. + const limit = ARGUMENT_UNRESTRICTED_BINARIES.has(binaryName) ? 1 : 2; + for (let i = 0; i < node.childCount; i++) { + // Limit sequence length based on binary sensitivity + if (sequence.length >= limit) break; + const child = node.child(i); if (!child || child.id === nameNode?.id) continue; @@ -902,9 +939,6 @@ export function getCommandSegments(command: string): string[][] { sequence.push(normalizeCommandName(text)); } } - // Limit sequence length to command + first positional argument - // to avoid over-specifying (e.g., git commit "message" -> git commit) - if (sequence.length >= 2) break; } if (sequence.length > 0) { segments.push(sequence);