MediaMetz/gemini-cli
1
0
Fork 0
mirror of https://github.com/google-gemini/gemini-cli.git synced 2026-07-11 02:20:48 -07:00
Code Issues Packages Projects Releases Wiki Activity

fix(core): secure shell execution with AST validation

Browse Source 
Replaces simplistic prefix-matching for shell command policies with robust Abstract Syntax Tree (AST) parsing using `bash-parser`.

Previously, policies for shell tools only checked if the command string started with an allowed prefix (e.g., `echo`), allowing trivial bypasses via shell operators like `&&` or `;` (e.g., `echo "ok" && rm -rf /`).

This update secures the execution pipeline by parsing the shell string and validating *every* extracted sub-command against the allowed policies.

Key changes:
- Integrated `bash-parser` to synchronously extract executable commands from pipelines, lists, and subshells.
- Updated `doesToolInvocationMatch` to enforce policy on all extracted sub-commands instead of just the string prefix.
- Enforced `coreTools` validation at execution time within `ShellTool` to prevent bypasses when tools are configured via `settings.json`.
- Updated the CLI `useShellCommandProcessor` to run human-input commands through the AST `PolicyEngine` check before spawning the process.
- Fixed asynchronous test flakiness in the CLI package caused by the new policy enforcement.
This commit is contained in:
galz10
2026-03-16 15:07:51 -07:00
parent dfe22aae21
commit 459db523e2
9 changed files with 557 additions and 61 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/cli/src/ui/hooks/shellCommandProcessor.test.tsx
+28 -26
View File
@@ -77,6 +77,7 @@ import {
type ShellExecutionResult,
type ShellOutputEvent,
CoreToolCallStatus,
PolicyDecision,
} from '@google/gemini-cli-core';
import * as fs from 'node:fs';
import * as os from 'node:os';
@@ -107,6 +108,7 @@ describe('useShellCommandProcessor', () => {
mockConfig = {
getTargetDir: () => '/test/dir',
getEnableInteractiveShell: () => false,
getPolicyEngine: () => ({ check: vi.fn().mockResolvedValue({ decision: PolicyDecision.ALLOW }) }),
getShellExecutionConfig: () => ({
terminalHeight: 20,
terminalWidth: 80,
@@ -228,8 +230,8 @@ describe('useShellCommandProcessor', () => {
it('should handle successful execution and update history correctly', async () => {
const { result } = renderProcessorHook();
act(() => {
result.current.handleShellCommand(
await act(async () => {
result.current.handleShellCommand(
'echo "ok"',
new AbortController().signal,
);
@@ -260,8 +262,8 @@ describe('useShellCommandProcessor', () => {
it('should handle command failure and display error status', async () => {
const { result } = renderProcessorHook();
act(() => {
result.current.handleShellCommand(
await act(async () => {
result.current.handleShellCommand(
'bad-cmd',
new AbortController().signal,
);
@@ -357,8 +359,8 @@ describe('useShellCommandProcessor', () => {
it('should show binary progress messages correctly', async () => {
const { result } = renderProcessorHook();
act(() => {
result.current.handleShellCommand(
await act(async () => {
result.current.handleShellCommand(
'cat img',
new AbortController().signal,
);
@@ -449,8 +451,8 @@ describe('useShellCommandProcessor', () => {
const { result } = renderProcessorHook();
const abortController = new AbortController();
act(() => {
result.current.handleShellCommand('sleep 5', abortController.signal);
await act(async () => {
result.current.handleShellCommand('sleep 5', abortController.signal);
});
const execPromise = onExecMock.mock.calls[0][0];
@@ -474,8 +476,8 @@ describe('useShellCommandProcessor', () => {
const binaryBuffer = Buffer.from([0x89, 0x50, 0x4e, 0x47]);
mockIsBinary.mockReturnValue(true);
act(() => {
result.current.handleShellCommand(
await act(async () => {
result.current.handleShellCommand(
'cat image.png',
new AbortController().signal,
);
@@ -504,8 +506,8 @@ describe('useShellCommandProcessor', () => {
result: Promise.reject(testError),
}));
act(() => {
result.current.handleShellCommand(
await act(async () => {
result.current.handleShellCommand(
'a-command',
new AbortController().signal,
);
@@ -533,8 +535,8 @@ describe('useShellCommandProcessor', () => {
const { result } = renderProcessorHook();
act(() => {
result.current.handleShellCommand(
await act(async () => {
result.current.handleShellCommand(
'a-command',
new AbortController().signal,
);
@@ -562,8 +564,8 @@ describe('useShellCommandProcessor', () => {
vi.mocked(fs.readFileSync).mockReturnValue('/test/dir/new'); // A different directory
const { result } = renderProcessorHook();
act(() => {
result.current.handleShellCommand(
await act(async () => {
result.current.handleShellCommand(
'cd new',
new AbortController().signal,
);
@@ -587,8 +589,8 @@ describe('useShellCommandProcessor', () => {
vi.mocked(fs.readFileSync).mockReturnValue('/test/dir'); // The same directory
const { result } = renderProcessorHook();
act(() => {
result.current.handleShellCommand('ls', new AbortController().signal);
await act(async () => {
result.current.handleShellCommand('ls', new AbortController().signal);
});
const execPromise = onExecMock.mock.calls[0][0];
@@ -729,8 +731,8 @@ describe('useShellCommandProcessor', () => {
expect(result.current.activeShellPtyId).toBeNull(); // Pre-condition
act(() => {
result.current.handleShellCommand('cmd', new AbortController().signal);
await act(async () => {
result.current.handleShellCommand('cmd', new AbortController().signal);
});
const execPromise = onExecMock.mock.calls[0][0];
@@ -756,8 +758,8 @@ describe('useShellCommandProcessor', () => {
const { result } = renderProcessorHook();
act(() => {
result.current.handleShellCommand('ls', new AbortController().signal);
await act(async () => {
result.current.handleShellCommand('ls', new AbortController().signal);
});
// Let microtasks run
@@ -1104,8 +1106,8 @@ describe('useShellCommandProcessor', () => {
expect(result.current.isBackgroundShellVisible).toBe(true);
// 2. Start foreground shell
act(() => {
result.current.handleShellCommand('ls', new AbortController().signal);
await act(async () => {
result.current.handleShellCommand('ls', new AbortController().signal);
});
// Wait for PID to be set
@@ -1140,8 +1142,8 @@ describe('useShellCommandProcessor', () => {
expect(result.current.isBackgroundShellVisible).toBe(true);
// 2. Start foreground shell
act(() => {
result.current.handleShellCommand('ls', new AbortController().signal);
await act(async () => {
result.current.handleShellCommand('ls', new AbortController().signal);
});
await waitFor(() => expect(result.current.activeShellPtyId).toBe(12345));
expect(result.current.isBackgroundShellVisible).toBe(false);
packages/cli/src/ui/hooks/shellCommandProcessor.ts
+40
View File
@@ -14,6 +14,7 @@ import {
isBinary,
ShellExecutionService,
CoreToolCallStatus,
PolicyDecision,
} from '@google/gemini-cli-core';
import { type PartListUnion } from '@google/genai';
import type { UseHistoryManagerReturn } from './useHistoryManager.js';
@@ -298,6 +299,45 @@ export const useShellCommandProcessor = (
}
const executeCommand = async () => {
try {
const policyEngine = config.getPolicyEngine();
const { decision } = await policyEngine.check(
{ name: 'run_shell_command', args: { command: rawQuery } },
undefined,
);
if (decision === PolicyDecision.DENY) {
addItemToHistory(
{
type: 'error',
text: `Command cannot be run. Blocked command: "${rawQuery}". Reason: Blocked by policy.`,
},
userMessageTimestamp,
);
if (pwdFilePath && fs.existsSync(pwdFilePath)) {
fs.unlinkSync(pwdFilePath);
}
dispatch({ type: 'SET_ACTIVE_PTY', pid: null });
setShellInputFocused(false);
return;
}
} catch (err) {
const errorMessage = err instanceof Error ? err.message : String(err);
addItemToHistory(
{
type: 'error',
text: `Policy validation error: ${errorMessage}`,
},
userMessageTimestamp,
);
if (pwdFilePath && fs.existsSync(pwdFilePath)) {
fs.unlinkSync(pwdFilePath);
}
dispatch({ type: 'SET_ACTIVE_PTY', pid: null });
setShellInputFocused(false);
return;
}
let cumulativeStdout: string | AnsiOutput = '';
let isBinaryStream = false;
let binaryBytesReceived = 0;