diff --git a/packages/core/src/confirmation-bus/types.ts b/packages/core/src/confirmation-bus/types.ts index b4d3af9cc80..2cf0c5202e8 100644 --- a/packages/core/src/confirmation-bus/types.ts +++ b/packages/core/src/confirmation-bus/types.ts @@ -151,6 +151,7 @@ export interface UpdatePolicy { commandPrefix?: string | string[]; mcpName?: string; allowRedirection?: boolean; + allowEnv?: boolean; modes?: ApprovalMode[]; } diff --git a/packages/core/src/policy/config.ts b/packages/core/src/policy/config.ts index 359054add33..539203da44c 100644 --- a/packages/core/src/policy/config.ts +++ b/packages/core/src/policy/config.ts @@ -642,6 +642,7 @@ export function createPolicyUpdater( modes: message.modes, source: 'Dynamic (Confirmed)', allowRedirection: message.allowRedirection, + allowEnv: message.allowEnv, }); } } @@ -680,6 +681,7 @@ export function createPolicyUpdater( modes: message.modes, source: 'Dynamic (Confirmed)', allowRedirection: message.allowRedirection, + allowEnv: message.allowEnv, }); } diff --git a/packages/core/src/policy/policy-engine.test.ts b/packages/core/src/policy/policy-engine.test.ts index b6c11a079be..358dd732608 100644 --- a/packages/core/src/policy/policy-engine.test.ts +++ b/packages/core/src/policy/policy-engine.test.ts @@ -819,6 +819,57 @@ describe('PolicyEngine', () => { expect(result.decision).toBe(PolicyDecision.ALLOW); }); + it('should allow env prefixed shell commands when allowEnv is true', async () => { + const engineWithEnvRule = new PolicyEngine({ + rules: [ + { + toolName: 'run_shell_command', + decision: PolicyDecision.ALLOW, + priority: 1, + allowEnv: true, + }, + ], + }); + + const command = 'PAGER=cat git log'; + + const result = await engineWithEnvRule.check( + { + name: 'run_shell_command', + args: { command }, + }, + undefined, + undefined, + ); + + expect(result.decision).toBe(PolicyDecision.ALLOW); + }); + + it('should NOT allow env prefixed shell commands by default', async () => { + const engineWithRule = new PolicyEngine({ + rules: [ + { + toolName: 'run_shell_command', + decision: PolicyDecision.ALLOW, + priority: 1, + }, + ], + }); + + const command = 'PAGER=cat git log'; + + const result = await engineWithRule.check( + { + name: 'run_shell_command', + args: { command }, + }, + undefined, + undefined, + ); + + expect(result.decision).toBe(PolicyDecision.ASK_USER); + }); + it('should handle tools with no args', async () => { const rules: PolicyRule[] = [ { diff --git a/packages/core/src/policy/policy-engine.ts b/packages/core/src/policy/policy-engine.ts index eb5b141ba57..962b196ae02 100644 --- a/packages/core/src/policy/policy-engine.ts +++ b/packages/core/src/policy/policy-engine.ts @@ -10,6 +10,7 @@ import { initializeShellParsers, splitCommands, hasRedirection, + hasEnvPrefix, extractStringFromParseEntry, } from '../utils/shell-utils.js'; import { parse as shellParse } from 'shell-quote'; @@ -299,6 +300,26 @@ export class PolicyEngine { return true; } + private shouldDowngradeForEnvPrefix( + command: string, + allowEnv?: boolean, + ): boolean { + if (allowEnv) return false; + if (!hasEnvPrefix(command)) return false; + + // Do not downgrade (do not ask user) if sandboxing is enabled and in AUTO_EDIT or YOLO + const sandboxEnabled = !(this.sandboxManager instanceof NoopSandboxManager); + if ( + sandboxEnabled && + (this.approvalMode === ApprovalMode.AUTO_EDIT || + this.approvalMode === ApprovalMode.YOLO) + ) { + return false; + } + + return true; + } + /** * Check if a shell command is allowed. */ @@ -340,6 +361,7 @@ export class PolicyEngine { serverName: string | undefined, dir_path: string | undefined, allowRedirection?: boolean, + allowEnv?: boolean, rule?: PolicyRule, toolAnnotations?: Record, subagent?: string, @@ -404,6 +426,14 @@ export class PolicyEngine { responsibleRule = undefined; // Inherent policy } + if (this.shouldDowngradeForEnvPrefix(command, allowEnv)) { + debugLogger.debug( + `[PolicyEngine.check] Downgrading ALLOW to ASK_USER for command with env prefix: ${command}`, + ); + aggregateDecision = PolicyDecision.ASK_USER; + responsibleRule = undefined; // Inherent policy + } + for (const rawSubCmd of subCommands) { const subCmd = rawSubCmd.trim(); // Prevent infinite recursion for the root command @@ -417,6 +447,14 @@ export class PolicyEngine { aggregateDecision = PolicyDecision.ASK_USER; responsibleRule = undefined; // Inherent policy } + } else if (this.shouldDowngradeForEnvPrefix(subCmd, allowEnv)) { + debugLogger.debug( + `[PolicyEngine.check] Downgrading ALLOW to ASK_USER for command with env prefix: ${subCmd}`, + ); + if (aggregateDecision === PolicyDecision.ALLOW) { + aggregateDecision = PolicyDecision.ASK_USER; + responsibleRule = undefined; // Inherent policy + } } else { // Atomic command matching the rule. if ( @@ -469,6 +507,20 @@ export class PolicyEngine { responsibleRule = undefined; } } + + // Check for env prefix in allowed sub-commands + if ( + subDecision === PolicyDecision.ALLOW && + this.shouldDowngradeForEnvPrefix(subCmd, allowEnv) + ) { + debugLogger.debug( + `[PolicyEngine.check] Downgrading ALLOW to ASK_USER for command with env prefix: ${subCmd}`, + ); + if (aggregateDecision === PolicyDecision.ALLOW) { + aggregateDecision = PolicyDecision.ASK_USER; + responsibleRule = undefined; + } + } } return { @@ -603,6 +655,7 @@ export class PolicyEngine { serverName, shellDirPath, rule.allowRedirection, + rule.allowEnv, rule, toolAnnotations, subagent, @@ -650,6 +703,7 @@ export class PolicyEngine { serverName, shellDirPath, false, + false, undefined, toolAnnotations, subagent, diff --git a/packages/core/src/policy/policy-updater.test.ts b/packages/core/src/policy/policy-updater.test.ts index 5ee9d65df48..a0a079ea392 100644 --- a/packages/core/src/policy/policy-updater.test.ts +++ b/packages/core/src/policy/policy-updater.test.ts @@ -27,6 +27,7 @@ vi.mock('../utils/shell-utils.js', () => ({ getCommandRoots: vi.fn(), stripShellWrapper: vi.fn(), hasRedirection: vi.fn(), + hasEnvPrefix: vi.fn(), })); interface ParsedPolicy { rule?: Array<{ @@ -99,6 +100,25 @@ describe('createPolicyUpdater', () => { ); }); + it('should pass allowEnv to policyEngine.addRule', async () => { + createPolicyUpdater(policyEngine, messageBus, mockStorage); + + await messageBus.publish({ + type: MessageBusType.UPDATE_POLICY, + toolName: 'run_shell_command', + commandPrefix: 'ls', + persist: false, + allowEnv: true, + }); + + expect(policyEngine.addRule).toHaveBeenCalledWith( + expect.objectContaining({ + toolName: 'run_shell_command', + allowEnv: true, + }), + ); + }); + it('should pass mcpName to policyEngine.addRule for argsPattern updates', async () => { createPolicyUpdater(policyEngine, messageBus, mockStorage); @@ -259,6 +279,7 @@ describe('ShellToolInvocation Policy Update', () => { (c: string) => c, ); vi.mocked(shellUtils.hasRedirection).mockReturnValue(false); + vi.mocked(shellUtils.hasEnvPrefix).mockReturnValue(false); }); it('should extract multiple root commands for chained commands', () => { @@ -322,4 +343,31 @@ describe('ShellToolInvocation Policy Update', () => { 'echo "hello" > file.txt', ); }); + + it('should include allowEnv if command has environment prefix', () => { + vi.mocked(shellUtils.getCommandRoots).mockReturnValue(['ls']); + vi.mocked(shellUtils.hasEnvPrefix).mockReturnValue(true); + + const invocation = new ShellToolInvocation( + mockConfig, + { command: 'PAGER=cat ls -la' }, + mockMessageBus, + 'run_shell_command', + 'Shell', + ); + + const options = ( + invocation as unknown as { + getPolicyUpdateOptions: (o: ToolConfirmationOutcome) => { + commandPrefix: string[]; + allowEnv?: boolean; + }; + } + ).getPolicyUpdateOptions(ToolConfirmationOutcome.ProceedAlways); + expect(options!.commandPrefix).toEqual(['ls']); + expect(options!.allowEnv).toBe(true); + expect(shellUtils.hasEnvPrefix).toHaveBeenCalledWith( + 'PAGER=cat ls -la', + ); + }); }); diff --git a/packages/core/src/policy/types.ts b/packages/core/src/policy/types.ts index b843129c99a..70fba10765e 100644 --- a/packages/core/src/policy/types.ts +++ b/packages/core/src/policy/types.ts @@ -179,6 +179,13 @@ export interface PolicyRule { */ allowRedirection?: boolean; + /** + * If true, allows environment variable prefixes even if the policy engine would normally + * downgrade ALLOW to ASK_USER for commands with env prefixes. + * Only applies when decision is ALLOW. + */ + allowEnv?: boolean; + /** * Effect of the rule's source. * e.g. "my-policies.toml", "Settings (MCP Trusted)", etc. diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 44f0c853167..453b53c1b3b 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -41,6 +41,7 @@ import { stripShellWrapper, parseCommandDetails, hasRedirection, + hasEnvPrefix, normalizeCommand, } from '../utils/shell-utils.js'; import { SHELL_TOOL_NAME } from './tool-names.js'; @@ -236,11 +237,12 @@ export class ShellToolInvocation extends BaseToolInvocation< const command = stripShellWrapper(this.params.command); const rootCommands = [...new Set(getCommandRoots(command))]; const allowRedirection = hasRedirection(command) ? true : undefined; + const allowEnv = hasEnvPrefix(command) ? true : undefined; if (rootCommands.length > 0) { - return { commandPrefix: rootCommands, allowRedirection }; + return { commandPrefix: rootCommands, allowRedirection, allowEnv }; } - return { commandPrefix: this.params.command, allowRedirection }; + return { commandPrefix: this.params.command, allowRedirection, allowEnv }; } return undefined; } diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index cd6209079c3..79f2420533b 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -149,6 +149,7 @@ export interface PolicyUpdateOptions { mcpName?: string; toolName?: string; allowRedirection?: boolean; + allowEnv?: boolean; } /** diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts index 0dda7c48815..8ab2f1049de 100644 --- a/packages/core/src/utils/shell-utils.test.ts +++ b/packages/core/src/utils/shell-utils.test.ts @@ -23,6 +23,7 @@ import { stripShellWrapper, normalizeCommand, hasRedirection, + hasEnvPrefix, resolveExecutable, } from './shell-utils.js'; import path from 'node:path'; @@ -288,6 +289,12 @@ describe('hasRedirection', () => { mockPlatform.mockReturnValue('linux'); expect(hasRedirection('echo "a > b"')).toBe(false); }); + + it('should not detect redirection in chained commands', () => { + expect(hasRedirection('cmd1 && echo hello > world')).toBe(false); + expect(hasRedirection('echo hello > world || cmd2')).toBe(false); + expect(hasRedirection('cmd1 ; echo hello > world')).toBe(false); + }); }); describeWindowsOnly('PowerShell integration', () => { @@ -621,3 +628,34 @@ describe('resolveExecutable', () => { expect(await resolveExecutable('unknown')).toBeUndefined(); }); }); + +describe('hasEnvPrefix', () => { + it('should detect simple environment variable assignments', () => { + expect(hasEnvPrefix('FOO=bar cmd')).toBe(true); + expect(hasEnvPrefix('FOO=bar')).toBe(true); + expect(hasEnvPrefix('FOO=bar baz=qux cmd')).toBe(true); + }); + + it('should detect quoted environment variable assignments', () => { + expect(hasEnvPrefix('FOO="bar baz" cmd')).toBe(true); + expect(hasEnvPrefix("FOO='bar baz' cmd")).toBe(true); + }); + + it('should detect environment variable assignments using env', () => { + expect(hasEnvPrefix('env FOO=bar cmd')).toBe(true); + expect(hasEnvPrefix('FOO=bar env cmd')).toBe(true); + expect(hasEnvPrefix('env FOO="bar baz" cmd')).toBe(true); + expect(hasEnvPrefix('env cmd')).toBe(true); + }); + + it('should not detect environment variables used in the command', () => { + expect(hasEnvPrefix('echo $FOO')).toBe(false); + expect(hasEnvPrefix('echo "${FOO}"')).toBe(false); + }); + + it('should not detect environment variables in chained commands', () => { + expect(hasEnvPrefix('FOO=bar cmd && cmd2')).toBe(false); + expect(hasEnvPrefix('cmd1 || FOO=bar cmd2')).toBe(false); + expect(hasEnvPrefix('cmd1 ; FOO=bar cmd2')).toBe(false); + }); +}); diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 46cffa1d35c..65863620fe6 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -186,6 +186,7 @@ interface CommandParseResult { details: ParsedCommandDetail[]; hasError: boolean; hasRedirection?: boolean; + hasEnvPrefix?: boolean; } const POWERSHELL_COMMAND_ENV = '__GCLI_POWERSHELL_COMMAND__'; @@ -506,6 +507,8 @@ export function parseBashCommandDetails( return { details: details.sort((a, b) => a.startIndex - b.startIndex), hasError, + hasRedirection: hasRedirection(command), + hasEnvPrefix: hasEnvPrefix(command), }; } @@ -729,20 +732,102 @@ export function hasRedirection(command: string): boolean { const tree = parseCommandTree(command); if (!tree) return fallbackCheck(); - const stack: Node[] = [tree.rootNode]; + const root = tree.rootNode; + // tree-sitter-bash wraps everything in a program/document root. + // We check if the command is a simple, non-compound command. + const stack: Node[] = [root]; while (stack.length > 0) { const current = stack.pop()!; + if (current.type === 'list' || current.type === 'pipeline') { + return false; + } + for (let i = current.childCount - 1; i >= 0; i -= 1) { + const child = current.child(i); + if (child) stack.push(child); + } + } + + if (root.namedChildCount === 1) { + const firstChild = root.namedChild(0); if ( - current.type === 'redirected_statement' || - current.type === 'file_redirect' || - current.type === 'heredoc_redirect' || - current.type === 'herestring_redirect' + firstChild?.type === 'command' || + firstChild?.type === 'redirected_statement' ) { - return true; + const stack: Node[] = [firstChild]; + while (stack.length > 0) { + const current = stack.pop()!; + if ( + current.type === 'redirected_statement' || + current.type === 'file_redirect' || + current.type === 'heredoc_redirect' || + current.type === 'herestring_redirect' + ) { + return true; + } + for (let i = current.childCount - 1; i >= 0; i -= 1) { + const child = current.child(i); + if (child) stack.push(child); + } + } + } + } + return false; + } + + return fallbackCheck(); +} + +/** + * Checks if a command contains environment variable prefixes (e.g. `FOO=bar cmd`). + */ +export function hasEnvPrefix(command: string): boolean { + const fallbackCheck = () => + /^(?:env\s+)?[a-zA-Z_][a-zA-Z0-9_]*=/.test(command.trim()) || + /^env\s+/.test(command.trim()); + + const configuration = getShellConfiguration(); + + if (configuration.shell === 'bash' && bashLanguage) { + const tree = parseCommandTree(command); + if (!tree) return fallbackCheck(); + + // Check if the root is a single command. If it's a compound command + // (pipeline, list, etc.), we return false because we only want to allow + // environment prefixes for simple, standalone commands. + const root = tree.rootNode; + // tree-sitter-bash wraps everything in a program/document root. + const compoundStack: Node[] = [root]; + while (compoundStack.length > 0) { + const current = compoundStack.pop()!; + if (current.type === 'list' || current.type === 'pipeline') { + return false; } for (let i = current.childCount - 1; i >= 0; i -= 1) { const child = current.child(i); - if (child) stack.push(child); + if (child) compoundStack.push(child); + } + } + + if (root.namedChildCount === 1) { + const firstChild = root.namedChild(0); + if ( + firstChild?.type === 'command' || + firstChild?.type === 'variable_assignment' + ) { + const stack: Node[] = [firstChild]; + while (stack.length > 0) { + const current = stack.pop()!; + if (current.type === 'variable_assignment') { + return true; + } + if (current.type === 'command_name' && current.text === 'env') { + return true; + } + for (let i = current.childCount - 1; i >= 0; i -= 1) { + const child = current.child(i); + if (child) stack.push(child); + } + } } } return false;