feat(core): enhance shell command validation and add core tools allowlist

- Implements recursive validation for shell command substitutions ($(...), `...`) and subshells.
- Adds support for settings.tools.core to allow explicit, strict allowlisting of core tools.
- Refactors PolicyEngine.check to handle shell wrappers (e.g., bash -c) and complex chained commands.
- Introduces comprehensive regression tests for shell safety, pipes, and redirection.
- Updates shell-utils to better identify and extract shell structure details using Tree-sitter.
This commit is contained in:
galz10 2026-04-20 16:44:22 -07:00
parent 6d7974f1ef
commit 5746424b18
10 changed files with 458 additions and 93 deletions

View file

@ -75,6 +75,7 @@ export const ADMIN_POLICY_TIER = 5;
export const MCP_EXCLUDED_PRIORITY = USER_POLICY_TIER + 0.9;
export const EXCLUDE_TOOLS_FLAG_PRIORITY = USER_POLICY_TIER + 0.4;
export const ALLOWED_TOOLS_FLAG_PRIORITY = USER_POLICY_TIER + 0.3;
export const CORE_TOOLS_FLAG_PRIORITY = USER_POLICY_TIER + 0.25;
export const TRUSTED_MCP_SERVER_PRIORITY = USER_POLICY_TIER + 0.2;
export const ALLOWED_MCP_SERVER_PRIORITY = USER_POLICY_TIER + 0.1;
@ -434,10 +435,12 @@ export async function createPolicyEngineConfig(
}
}
// Tools that are explicitly allowed in the settings.
// Priority: ALLOWED_TOOLS_FLAG_PRIORITY (user tier - explicit temporary allows)
if (settings.tools?.allowed) {
for (const tool of settings.tools.allowed) {
const mapToolsToRules = (
tools: string[],
priority: number,
source: string,
) => {
for (const tool of tools) {
// Check for legacy format: toolName(args)
const match = tool.match(/^([a-zA-Z0-9_-]+)\((.*)\)$/);
if (match) {
@ -455,9 +458,9 @@ export async function createPolicyEngineConfig(
rules.push({
toolName,
decision: PolicyDecision.ALLOW,
priority: ALLOWED_TOOLS_FLAG_PRIORITY,
priority,
argsPattern: new RegExp(pattern),
source: 'Settings (Tools Allowed)',
source,
});
}
}
@ -467,8 +470,8 @@ export async function createPolicyEngineConfig(
rules.push({
toolName,
decision: PolicyDecision.ALLOW,
priority: ALLOWED_TOOLS_FLAG_PRIORITY,
source: 'Settings (Tools Allowed)',
priority,
source,
});
}
} else {
@ -479,11 +482,40 @@ export async function createPolicyEngineConfig(
rules.push({
toolName,
decision: PolicyDecision.ALLOW,
priority: ALLOWED_TOOLS_FLAG_PRIORITY,
source: 'Settings (Tools Allowed)',
priority,
source,
});
}
}
};
// Tools that are explicitly allowed in the settings.
// Priority: ALLOWED_TOOLS_FLAG_PRIORITY (user tier - explicit temporary allows)
if (settings.tools?.allowed) {
mapToolsToRules(
settings.tools.allowed,
ALLOWED_TOOLS_FLAG_PRIORITY,
'Settings (Tools Allowed)',
);
}
// Core tools that are restricted in the settings.
// Priority: CORE_TOOLS_FLAG_PRIORITY (user tier - core tool allowlist)
if (settings.tools?.core) {
mapToolsToRules(
settings.tools.core,
CORE_TOOLS_FLAG_PRIORITY,
'Settings (Core Tools)',
);
// If core tools are restricted, we should add a default DENY rule for everything else
// at a slightly lower priority than the explicit allows.
rules.push({
toolName: '*',
decision: PolicyDecision.DENY,
priority: CORE_TOOLS_FLAG_PRIORITY - 0.01,
source: 'Settings (Core Tools Allowlist Enforcement)',
});
}
// MCP servers that are trusted in the settings.

View file

@ -0,0 +1,76 @@
/**
* @license
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import { describe, it, expect } from 'vitest';
import { createPolicyEngineConfig } from './config.js';
import { PolicyEngine } from './policy-engine.js';
import { PolicyDecision, ApprovalMode } from './types.js';
describe('PolicyEngine - Core Tools Mapping', () => {
it('should allow tools explicitly listed in settings.tools.core', async () => {
const settings = {
tools: {
core: ['run_shell_command(ls)', 'run_shell_command(git status)'],
},
};
const config = await createPolicyEngineConfig(
settings,
ApprovalMode.DEFAULT,
undefined,
true, // interactive
);
const engine = new PolicyEngine(config);
// Test simple tool name
const result1 = await engine.check(
{ name: 'run_shell_command', args: { command: 'ls' } },
undefined,
);
expect(result1.decision).toBe(PolicyDecision.ALLOW);
// Test tool name with args
const result2 = await engine.check(
{ name: 'run_shell_command', args: { command: 'git status' } },
undefined,
);
expect(result2.decision).toBe(PolicyDecision.ALLOW);
// Test tool not in core list
const result3 = await engine.check(
{ name: 'run_shell_command', args: { command: 'npm test' } },
undefined,
);
// Should be DENIED because of strict allowlist
expect(result3.decision).toBe(PolicyDecision.DENY);
});
it('should allow tools in tools.core even if they are restricted by default policies', async () => {
// By default run_shell_command is ASK_USER.
// Putting it in tools.core should make it ALLOW.
const settings = {
tools: {
core: ['run_shell_command'],
},
};
const config = await createPolicyEngineConfig(
settings,
ApprovalMode.DEFAULT,
undefined,
true,
);
const engine = new PolicyEngine(config);
const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'any command' } },
undefined,
);
expect(result.decision).toBe(PolicyDecision.ALLOW);
});
});

View file

@ -43,6 +43,35 @@ vi.mock('../utils/shell-utils.js', async (importOriginal) => {
}
return [command];
}),
parseCommandDetails: vi.fn().mockImplementation((command: string) => {
// Basic mock implementation for PolicyEngine test needs
const commands = command.includes('&&')
? command.split('&&').map((c) => c.trim())
: [command.trim()];
// Detect $(...) or `...` and add as sub-commands for recursion tests
const subCommands = [...commands];
for (const cmd of commands) {
const subMatch = cmd.match(/\$\((.*)\)/) || cmd.match(/`(.*)`/);
if (subMatch?.[1]) {
subCommands.push(subMatch[1].trim());
}
}
return {
details: subCommands.map((c, i) => ({
name: c.split(' ')[0],
text: c,
startIndex: i === 0 ? 0 : -1, // Simple root indication
})),
hasError: false,
};
}),
stripShellWrapper: vi.fn().mockImplementation((command: string) => {
// Simple mock for stripping wrappers
const match = command.match(/^(?:bash|sh|zsh)\s+-c\s+["'](.*)["']$/i);
return match ? match[1] : command;
}),
hasRedirection: vi.fn().mockImplementation(
(command: string) =>
// Simple mock: true if '>' is present, unless it looks like "-> arrow"
@ -1862,7 +1891,6 @@ describe('PolicyEngine', () => {
});
it('should return ASK_USER in non-YOLO mode if shell command parsing fails', async () => {
const { splitCommands } = await import('../utils/shell-utils.js');
const rules: PolicyRule[] = [
{
toolName: 'run_shell_command',
@ -1877,7 +1905,11 @@ describe('PolicyEngine', () => {
});
// Simulate parsing failure
vi.mocked(splitCommands).mockReturnValueOnce([]);
const { parseCommandDetails } = await import('../utils/shell-utils.js');
vi.mocked(parseCommandDetails).mockReturnValueOnce({
details: [],
hasError: true,
});
const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'complex command' } },

View file

@ -7,8 +7,10 @@
import { type FunctionCall } from '@google/genai';
import {
SHELL_TOOL_NAMES,
REDIRECTION_NAMES,
initializeShellParsers,
splitCommands,
parseCommandDetails,
stripShellWrapper,
hasRedirection,
extractStringFromParseEntry,
} from '../utils/shell-utils.js';
@ -359,7 +361,8 @@ export class PolicyEngine {
}
await initializeShellParsers();
const subCommands = splitCommands(command);
const parsed = parseCommandDetails(command);
const subCommands = parsed?.details ?? [];
if (subCommands.length === 0) {
// If the matched rule says DENY, we should respect it immediately even if parsing fails.
@ -380,115 +383,103 @@ export class PolicyEngine {
);
// Parsing logic failed, we can't trust it. Use default decision ASK_USER (or DENY in non-interactive).
// We return the rule that matched so the evaluation loop terminates.
return {
decision: this.defaultDecision,
rule,
};
}
// If there are multiple parts, or if we just want to validate the single part against DENY rules
if (subCommands.length > 0) {
debugLogger.debug(
`[PolicyEngine.check] Validating shell command: ${subCommands.length} parts`,
);
if (ruleDecision === PolicyDecision.DENY) {
return { decision: PolicyDecision.DENY, rule };
}
// Start optimistically. If all parts are ALLOW, the whole is ALLOW.
// We will downgrade if any part is ASK_USER or DENY.
let aggregateDecision = PolicyDecision.ALLOW;
let responsibleRule: PolicyRule | undefined;
// Check for redirection on the full command string
if (this.shouldDowngradeForRedirection(command, allowRedirection)) {
debugLogger.debug(
`[PolicyEngine.check] Validating shell command: ${subCommands.length} parts`,
`[PolicyEngine.check] Downgrading ALLOW to ASK_USER for redirected command: ${command}`,
);
aggregateDecision = PolicyDecision.ASK_USER;
responsibleRule = undefined; // Inherent policy
}
if (ruleDecision === PolicyDecision.DENY) {
return { decision: PolicyDecision.DENY, rule };
for (const detail of subCommands) {
if (REDIRECTION_NAMES.has(detail.name)) {
continue;
}
// Start optimistically. If all parts are ALLOW, the whole is ALLOW.
// We will downgrade if any part is ASK_USER or DENY.
let aggregateDecision = PolicyDecision.ALLOW;
let responsibleRule: PolicyRule | undefined;
const subCmd = detail.text.trim();
const isAtomic =
subCmd === command ||
(detail.startIndex === 0 && detail.text.length === command.length);
// Check for redirection on the full command string
if (this.shouldDowngradeForRedirection(command, allowRedirection)) {
debugLogger.debug(
`[PolicyEngine.check] Downgrading ALLOW to ASK_USER for redirected command: ${command}`,
// Recursive check for shell wrappers (bash -c, etc.)
const stripped = stripShellWrapper(subCmd);
if (stripped !== subCmd) {
const wrapperResult = await this.check(
{ name: toolName, args: { command: stripped, dir_path } },
serverName,
toolAnnotations,
subagent,
true,
);
aggregateDecision = PolicyDecision.ASK_USER;
responsibleRule = undefined; // Inherent policy
if (wrapperResult.decision === PolicyDecision.DENY)
return wrapperResult;
if (wrapperResult.decision === PolicyDecision.ASK_USER) {
aggregateDecision = PolicyDecision.ASK_USER;
responsibleRule ??= wrapperResult.rule;
}
}
for (const rawSubCmd of subCommands) {
const subCmd = rawSubCmd.trim();
// Prevent infinite recursion for the root command
if (subCmd === command) {
if (this.shouldDowngradeForRedirection(subCmd, allowRedirection)) {
debugLogger.debug(
`[PolicyEngine.check] Downgrading ALLOW to ASK_USER for redirected command: ${subCmd}`,
);
// Redirection always downgrades ALLOW to ASK_USER
if (aggregateDecision === PolicyDecision.ALLOW) {
aggregateDecision = PolicyDecision.ASK_USER;
responsibleRule = undefined; // Inherent policy
}
} else {
// Atomic command matching the rule.
if (
ruleDecision === PolicyDecision.ASK_USER &&
aggregateDecision === PolicyDecision.ALLOW
) {
aggregateDecision = PolicyDecision.ASK_USER;
responsibleRule = rule;
}
}
continue;
if (isAtomic) {
if (
ruleDecision === PolicyDecision.ASK_USER &&
aggregateDecision === PolicyDecision.ALLOW
) {
aggregateDecision = PolicyDecision.ASK_USER;
responsibleRule = rule;
}
} else {
const subResult = await this.check(
{ name: toolName, args: { command: subCmd, dir_path } },
serverName,
toolAnnotations,
subagent,
true,
);
// subResult.decision is already filtered through applyNonInteractiveMode by this.check()
const subDecision = subResult.decision;
if (subResult.decision === PolicyDecision.DENY) return subResult;
// If any part is DENIED, the whole command is DENY
if (subDecision === PolicyDecision.DENY) {
return {
decision: PolicyDecision.DENY,
rule: subResult.rule,
};
}
// If any part requires ASK_USER, the whole command requires ASK_USER
if (subDecision === PolicyDecision.ASK_USER) {
if (subResult.decision === PolicyDecision.ASK_USER) {
aggregateDecision = PolicyDecision.ASK_USER;
if (!responsibleRule) {
responsibleRule = subResult.rule;
}
responsibleRule ??= subResult.rule;
}
// Check for redirection in allowed sub-commands
// Downgrade if sub-command has redirection
if (
subDecision === PolicyDecision.ALLOW &&
subResult.decision === PolicyDecision.ALLOW &&
this.shouldDowngradeForRedirection(subCmd, allowRedirection)
) {
debugLogger.debug(
`[PolicyEngine.check] Downgrading ALLOW to ASK_USER for redirected command: ${subCmd}`,
);
if (aggregateDecision === PolicyDecision.ALLOW) {
aggregateDecision = PolicyDecision.ASK_USER;
responsibleRule = undefined;
}
}
}
return {
decision: aggregateDecision,
// If we stayed at ALLOW, we return the original rule (if any).
// If we downgraded, we return the responsible rule (or undefined if implicit).
rule: aggregateDecision === ruleDecision ? rule : responsibleRule,
};
}
return {
decision: ruleDecision,
rule,
decision: aggregateDecision,
rule: aggregateDecision === ruleDecision ? rule : responsibleRule,
};
}
@ -501,6 +492,7 @@ export class PolicyEngine {
serverName: string | undefined,
toolAnnotations?: Record<string, unknown>,
subagent?: string,
skipHeuristics = false,
): Promise<CheckResult> {
// Case 1: Metadata injection is the primary and safest way to identify an MCP server.
// If we have explicit `_serverName` metadata (usually injected by tool-registry for active tools), use it.
@ -594,6 +586,7 @@ export class PolicyEngine {
let ruleDecision = rule.decision;
if (
!skipHeuristics &&
isShellCommand &&
command &&
!('commandPrefix' in rule) &&
@ -615,12 +608,10 @@ export class PolicyEngine {
subagent,
);
decision = shellResult.decision;
if (shellResult.rule) {
matchedRule = shellResult.rule;
break;
}
matchedRule = shellResult.rule;
break;
} else {
decision = rule.decision;
decision = ruleDecision;
matchedRule = rule;
break;
}
@ -643,7 +634,7 @@ export class PolicyEngine {
);
if (toolName && SHELL_TOOL_NAMES.includes(toolName)) {
let heuristicDecision = this.defaultDecision;
if (command) {
if (!skipHeuristics && command) {
heuristicDecision = await this.applyShellHeuristics(
command,
heuristicDecision,

View file

@ -0,0 +1,134 @@
/**
* @license
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import { describe, it, expect, beforeAll } from 'vitest';
import { PolicyEngine } from './policy-engine.js';
import { PolicyDecision, ApprovalMode } from './types.js';
import { initializeShellParsers } from '../utils/shell-utils.js';
import { buildArgsPatterns } from './utils.js';
describe('PolicyEngine - Shell Safety Regression Suite', () => {
let engine: PolicyEngine;
beforeAll(async () => {
await initializeShellParsers();
});
const setupEngine = (allowedCommands: string[]) => {
const rules = allowedCommands.map((cmd) => ({
toolName: 'run_shell_command',
decision: PolicyDecision.ALLOW,
argsPattern: new RegExp(buildArgsPatterns(undefined, cmd)[0]!),
priority: 10,
}));
return new PolicyEngine({
rules,
approvalMode: ApprovalMode.DEFAULT,
defaultDecision: PolicyDecision.ASK_USER,
});
};
it('should block unauthorized chained command with &&', async () => {
engine = setupEngine(['echo']);
const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'echo hi && ls' } },
undefined,
);
expect(result.decision).toBe(PolicyDecision.ASK_USER);
});
it('should allow authorized chained command with &&', async () => {
engine = setupEngine(['echo', 'ls']);
const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'echo hi && ls' } },
undefined,
);
expect(result.decision).toBe(PolicyDecision.ALLOW);
});
it('should block unauthorized chained command with ||', async () => {
engine = setupEngine(['false']);
const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'false || ls' } },
undefined,
);
expect(result.decision).toBe(PolicyDecision.ASK_USER);
});
it('should block unauthorized chained command with ;', async () => {
engine = setupEngine(['echo']);
const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'echo hi; ls' } },
undefined,
);
expect(result.decision).toBe(PolicyDecision.ASK_USER);
});
it('should block unauthorized command in pipe |', async () => {
engine = setupEngine(['echo']);
const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'echo hi | grep "hi"' } },
undefined,
);
expect(result.decision).toBe(PolicyDecision.ASK_USER);
});
it('should allow authorized command in pipe |', async () => {
engine = setupEngine(['echo', 'grep']);
const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'echo hi | grep "hi"' } },
undefined,
);
expect(result.decision).toBe(PolicyDecision.ALLOW);
});
it('should block unauthorized chained command with &', async () => {
engine = setupEngine(['echo']);
const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'echo hi & ls' } },
undefined,
);
expect(result.decision).toBe(PolicyDecision.ASK_USER);
});
it('should allow authorized chained command with &', async () => {
engine = setupEngine(['echo', 'ls']);
const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'echo hi & ls' } },
undefined,
);
expect(result.decision).toBe(PolicyDecision.ALLOW);
});
it('should block unauthorized command in nested substitution', async () => {
engine = setupEngine(['echo', 'cat']);
const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'echo $(cat $(ls))' } },
undefined,
);
expect(result.decision).toBe(PolicyDecision.ASK_USER);
});
it('should allow authorized command in nested substitution', async () => {
engine = setupEngine(['echo', 'cat', 'ls']);
const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'echo $(cat $(ls))' } },
undefined,
);
expect(result.decision).toBe(PolicyDecision.ALLOW);
});
it('should block command redirection if not explicitly allowed', async () => {
engine = setupEngine(['echo']);
const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'echo hi > /tmp/test' } },
undefined,
);
// Inherent policy: redirection downgrades to ASK_USER
expect(result.decision).toBe(PolicyDecision.ASK_USER);
});
});

View file

@ -59,6 +59,30 @@ vi.mock('../utils/shell-utils.js', async (importOriginal) => {
return {
...actual,
initializeShellParsers: vi.fn(),
parseCommandDetails: (command: string) => {
if (Object.prototype.hasOwnProperty.call(commandMap, command)) {
const subcommands = commandMap[command];
return {
details: subcommands.map((text) => ({
name: text.split(' ')[0],
text,
startIndex: command.indexOf(text),
})),
hasError: subcommands.length === 0 && command.includes('&&&'),
};
}
return {
details: [
{
name: command.split(' ')[0],
text: command,
startIndex: 0,
},
],
hasError: false,
};
},
stripShellWrapper: (command: string) => command,
splitCommands: (command: string) => {
if (Object.prototype.hasOwnProperty.call(commandMap, command)) {
return commandMap[command];

View file

@ -0,0 +1,67 @@
/**
* @license
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import { expect, describe, it, beforeAll } from 'vitest';
import { PolicyEngine } from './policy-engine.js';
import { PolicyDecision } from './types.js';
import { initializeShellParsers } from '../utils/shell-utils.js';
describe('PolicyEngine Command Substitution Validation', () => {
beforeAll(async () => {
await initializeShellParsers();
});
const setupEngine = (blockedCmd: string) =>
new PolicyEngine({
defaultDecision: PolicyDecision.ALLOW,
rules: [
{
toolName: 'run_shell_command',
argsPattern: new RegExp(`"command":"${blockedCmd}"`),
decision: PolicyDecision.DENY,
},
],
});
it('should block echo $(dangerous_cmd) when dangerous_cmd is explicitly blocked', async () => {
const engine = setupEngine('dangerous_cmd');
const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'echo $(dangerous_cmd)' } },
'test-server',
);
expect(result.decision).toBe(PolicyDecision.DENY);
});
it('should block backtick substitution `dangerous_cmd`', async () => {
const engine = setupEngine('dangerous_cmd');
const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'echo `dangerous_cmd`' } },
'test-server',
);
expect(result.decision).toBe(PolicyDecision.DENY);
});
it('should block commands inside subshells (dangerous_cmd)', async () => {
const engine = setupEngine('dangerous_cmd');
const result = await engine.check(
{ name: 'run_shell_command', args: { command: '(dangerous_cmd)' } },
'test-server',
);
expect(result.decision).toBe(PolicyDecision.DENY);
});
it('should handle nested substitutions deeply', async () => {
const engine = setupEngine('deep_danger');
const result = await engine.check(
{
name: 'run_shell_command',
args: { command: 'echo $(ls $(deep_danger))' },
},
'test-server',
);
expect(result.decision).toBe(PolicyDecision.DENY);
});
});

View file

@ -335,6 +335,7 @@ export interface PolicySettings {
allowed?: string[];
};
tools?: {
core?: string[];
exclude?: string[];
allowed?: string[];
};

View file

@ -1993,13 +1993,15 @@ describe('getRipgrepPath', () => {
vi.mocked(fileExists).mockImplementation(
async (checkPath) =>
checkPath.includes(path.normalize('core/vendor/ripgrep')) &&
!checkPath.includes('tools'),
!checkPath.includes(path.join(path.sep, 'tools', path.sep)),
);
const resolvedPath = await getRipgrepPath();
expect(resolvedPath).not.toBeNull();
expect(resolvedPath).toContain(path.normalize('core/vendor/ripgrep'));
expect(resolvedPath).not.toContain('tools');
expect(resolvedPath).not.toContain(
path.join(path.sep, 'tools', path.sep),
);
});
it('should return null if binary is missing from both paths', async () => {

View file

@ -240,11 +240,13 @@ foreach ($commandAst in $commandAsts) {
'utf16le',
).toString('base64');
const REDIRECTION_NAMES = new Set([
export const REDIRECTION_NAMES = new Set([
'redirection (<)',
'redirection (>)',
'heredoc (<<)',
'herestring (<<<)',
'command substitution',
'subshell',
]);
function createParser(): Parser | null {
@ -360,6 +362,10 @@ function extractNameFromNode(node: Node): string | null {
return 'heredoc (<<)';
case 'herestring_redirect':
return 'herestring (<<<)';
case 'command_substitution':
return 'command substitution';
case 'subshell':
return 'subshell';
default:
return null;
}