Adding tools that are allowed without positional arguments

This commit is contained in:
Keith Schaab
2026-04-09 03:32:30 +00:00
parent c0a37e824c
commit ae56b87e81
9 changed files with 140 additions and 36 deletions
@@ -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', () => {
+1 -4
View File
@@ -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<ToolResult> {
+1 -4
View File
@@ -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 {};
}
/**
+1 -4
View File
@@ -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<LSToolParams, ToolResult> {
override getPolicyUpdateOptions(
_outcome: ToolConfirmationOutcome,
): PolicyUpdateOptions | undefined {
return {
argsPattern: buildDirPathArgsPattern(this.params.dir_path),
};
return {};
}
// Helper for consistent error formatting
+3 -4
View File
@@ -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 };
}
-3
View File
@@ -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,
+3 -1
View File
@@ -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 });
});
@@ -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);
});
});
+48 -14
View File
@@ -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);