diff --git a/apps/x/apps/renderer/src/App.tsx b/apps/x/apps/renderer/src/App.tsx index 016bd8c3..cfb50665 100644 --- a/apps/x/apps/renderer/src/App.tsx +++ b/apps/x/apps/renderer/src/App.tsx @@ -1532,7 +1532,6 @@ function App() { subflow: string[], response: 'approve' | 'deny', scope?: 'once' | 'session' | 'always', - command?: string, ) => { if (!runId) return @@ -1551,7 +1550,7 @@ function App() { try { await window.ipc.invoke('runs:authorizePermission', { runId, - authorization: { subflow, toolCallId, response, scope, command } + authorization: { subflow, toolCallId, response, scope } }) } catch (error) { console.error('Failed to authorize permission:', error) @@ -2951,14 +2950,8 @@ function App() { handlePermissionResponse(permRequest.toolCall.toolCallId, permRequest.subflow, 'approve')} - onApproveSession={() => { - const cmd = permRequest.toolCall.toolName === 'executeCommand' && typeof permRequest.toolCall.arguments === 'object' && permRequest.toolCall.arguments !== null && 'command' in permRequest.toolCall.arguments ? String(permRequest.toolCall.arguments.command) : undefined - handlePermissionResponse(permRequest.toolCall.toolCallId, permRequest.subflow, 'approve', 'session', cmd) - }} - onApproveAlways={() => { - const cmd = permRequest.toolCall.toolName === 'executeCommand' && typeof permRequest.toolCall.arguments === 'object' && permRequest.toolCall.arguments !== null && 'command' in permRequest.toolCall.arguments ? String(permRequest.toolCall.arguments.command) : undefined - handlePermissionResponse(permRequest.toolCall.toolCallId, permRequest.subflow, 'approve', 'always', cmd) - }} + onApproveSession={() => handlePermissionResponse(permRequest.toolCall.toolCallId, permRequest.subflow, 'approve', 'session')} + onApproveAlways={() => handlePermissionResponse(permRequest.toolCall.toolCallId, permRequest.subflow, 'approve', 'always')} onDeny={() => handlePermissionResponse(permRequest.toolCall.toolCallId, permRequest.subflow, 'deny')} isProcessing={isActive && isProcessing} response={response} diff --git a/apps/x/apps/renderer/src/components/chat-sidebar.tsx b/apps/x/apps/renderer/src/components/chat-sidebar.tsx index 357847d3..fcf8b004 100644 --- a/apps/x/apps/renderer/src/components/chat-sidebar.tsx +++ b/apps/x/apps/renderer/src/components/chat-sidebar.tsx @@ -101,7 +101,7 @@ interface ChatSidebarProps { pendingAskHumanRequests?: ChatTabViewState['pendingAskHumanRequests'] allPermissionRequests?: ChatTabViewState['allPermissionRequests'] permissionResponses?: ChatTabViewState['permissionResponses'] - onPermissionResponse?: (toolCallId: string, subflow: string[], response: PermissionResponse, scope?: 'once' | 'session' | 'always', command?: string) => void + onPermissionResponse?: (toolCallId: string, subflow: string[], response: PermissionResponse, scope?: 'once' | 'session' | 'always') => void onAskHumanResponse?: (toolCallId: string, subflow: string[], response: string) => void isToolOpenForTab?: (tabId: string, toolId: string) => boolean onToolOpenChangeForTab?: (tabId: string, toolId: string, open: boolean) => void @@ -444,14 +444,8 @@ export function ChatSidebar({ onPermissionResponse(permRequest.toolCall.toolCallId, permRequest.subflow, 'approve')} - onApproveSession={() => { - const cmd = permRequest.toolCall.toolName === 'executeCommand' && typeof permRequest.toolCall.arguments === 'object' && permRequest.toolCall.arguments !== null && 'command' in permRequest.toolCall.arguments ? String(permRequest.toolCall.arguments.command) : undefined - onPermissionResponse(permRequest.toolCall.toolCallId, permRequest.subflow, 'approve', 'session', cmd) - }} - onApproveAlways={() => { - const cmd = permRequest.toolCall.toolName === 'executeCommand' && typeof permRequest.toolCall.arguments === 'object' && permRequest.toolCall.arguments !== null && 'command' in permRequest.toolCall.arguments ? String(permRequest.toolCall.arguments.command) : undefined - onPermissionResponse(permRequest.toolCall.toolCallId, permRequest.subflow, 'approve', 'always', cmd) - }} + onApproveSession={() => onPermissionResponse(permRequest.toolCall.toolCallId, permRequest.subflow, 'approve', 'session')} + onApproveAlways={() => onPermissionResponse(permRequest.toolCall.toolCallId, permRequest.subflow, 'approve', 'always')} onDeny={() => onPermissionResponse(permRequest.toolCall.toolCallId, permRequest.subflow, 'deny')} isProcessing={isActive && isProcessing} response={response} diff --git a/apps/x/packages/core/package.json b/apps/x/packages/core/package.json index bcfac93d..4ea4331c 100644 --- a/apps/x/packages/core/package.json +++ b/apps/x/packages/core/package.json @@ -35,6 +35,7 @@ "papaparse": "^5.5.3", "pdf-parse": "^2.4.5", "react": "^19.2.3", + "shell-quote": "^1.8.3", "xlsx": "^0.18.5", "yaml": "^2.8.2", "zod": "^4.2.1" @@ -42,6 +43,7 @@ "devDependencies": { "@types/node": "^25.0.3", "@types/papaparse": "^5.5.2", - "@types/pdf-parse": "^1.1.5" + "@types/pdf-parse": "^1.1.5", + "@types/shell-quote": "^1.7.5" } } diff --git a/apps/x/packages/core/src/agents/runtime.ts b/apps/x/packages/core/src/agents/runtime.ts index 4012e9b3..abeeb53a 100644 --- a/apps/x/packages/core/src/agents/runtime.ts +++ b/apps/x/packages/core/src/agents/runtime.ts @@ -12,7 +12,7 @@ import { execTool } from "../application/lib/exec-tool.js"; import { AskHumanRequestEvent, RunEvent, ToolPermissionRequestEvent } from "@x/shared/dist/runs.js"; import { BuiltinTools } from "../application/lib/builtin-tools.js"; import { CopilotAgent } from "../application/assistant/agent.js"; -import { isBlocked } from "../application/lib/command-executor.js"; +import { isBlocked, extractCommandNames } from "../application/lib/command-executor.js"; import container from "../di/container.js"; import { IModelConfigRepo } from "../models/repo.js"; import { createProvider } from "../models/models.js"; @@ -462,6 +462,7 @@ export class AgentState { pendingAskHumanRequests: Record> = {}; allowedToolCallIds: Record = {}; deniedToolCallIds: Record = {}; + sessionAllowedCommands: Set = new Set(); getPendingPermissions(): z.infer[] { const response: z.infer[] = []; @@ -598,6 +599,16 @@ export class AgentState { switch (event.response) { case "approve": this.allowedToolCallIds[event.toolCallId] = true; + // For session scope, extract command names and add to session allowlist + if (event.scope === "session") { + const toolCall = this.toolCallIdMap[event.toolCallId]; + if (toolCall && typeof toolCall.arguments === 'object' && toolCall.arguments !== null && 'command' in toolCall.arguments) { + const names = extractCommandNames(String(toolCall.arguments.command)); + for (const name of names) { + this.sessionAllowedCommands.add(name); + } + } + } break; case "deny": this.deniedToolCallIds[event.toolCallId] = true; @@ -882,7 +893,7 @@ export async function* streamAgent({ } if (underlyingTool.type === "builtin" && underlyingTool.name === "executeCommand") { // if command is blocked, then seek permission - if (isBlocked(part.arguments.command)) { + if (isBlocked(part.arguments.command, state.sessionAllowedCommands)) { loopLogger.log('emitting tool-permission-request, toolCallId:', part.toolCallId); yield* processEvent({ runId, diff --git a/apps/x/packages/core/src/application/lib/command-executor.ts b/apps/x/packages/core/src/application/lib/command-executor.ts index 59484a16..c8ac67c0 100644 --- a/apps/x/packages/core/src/application/lib/command-executor.ts +++ b/apps/x/packages/core/src/application/lib/command-executor.ts @@ -1,129 +1,62 @@ import { exec, execSync, spawn, ChildProcess } from 'child_process'; import { promisify } from 'util'; +import { parse } from 'shell-quote'; import { getSecurityAllowList } from '../../config/security.js'; const execPromise = promisify(exec); -const ENV_ASSIGNMENT_REGEX = /^[A-Za-z_][A-Za-z0-9_]*=.*/; +const ENV_ASSIGNMENT_REGEX = /^[A-Za-z_][A-Za-z0-9_]*=/; const WRAPPER_COMMANDS = new Set(['sudo', 'env', 'time', 'command']); -function sanitizeToken(token: string): string { - return token.trim().replace(/^['"()]+|['"()]+$/g, ''); -} - -/** - * Split a shell command string on command separators (||, &&, ;, |, \n) - * while respecting single and double quotes. - */ -function splitCommandSegments(command: string): string[] { - const segments: string[] = []; - let current = ''; - let inSingle = false; - let inDouble = false; - - for (let i = 0; i < command.length; i++) { - const ch = command[i]; - - if (ch === "'" && !inDouble) { - inSingle = !inSingle; - current += ch; - continue; - } - if (ch === '"' && !inSingle) { - inDouble = !inDouble; - current += ch; - continue; - } - - if (inSingle || inDouble) { - current += ch; - continue; - } - - // Outside quotes — check for separators - if (ch === '\n' || ch === ';') { - segments.push(current); - current = ''; - continue; - } - if (ch === '|') { - if (command[i + 1] === '|') { - // || - segments.push(current); - current = ''; - i++; // skip second | - } else { - // single pipe - segments.push(current); - current = ''; - } - continue; - } - if (ch === '&' && command[i + 1] === '&') { - segments.push(current); - current = ''; - i++; // skip second & - continue; - } - - current += ch; - } - - if (current) segments.push(current); - return segments; -} - export function extractCommandNames(command: string): string[] { const discovered = new Set(); - const segments = splitCommandSegments(command); + const tokens = parse(command); - for (const segment of segments) { - const tokens = segment.trim().split(/\s+/).filter(Boolean); - if (!tokens.length) continue; - - let index = 0; - while (index < tokens.length && ENV_ASSIGNMENT_REGEX.test(tokens[index])) { - index++; + let expectCommand = true; + for (const token of tokens) { + // Operator tokens (|, &&, ;, ||, etc.) reset the "expect command" flag + if (typeof token === 'object' && token !== null && 'op' in token) { + expectCommand = true; + continue; } - if (index >= tokens.length) continue; + if (typeof token !== 'string') continue; - const primary = sanitizeToken(tokens[index]).toLowerCase(); - if (!primary) continue; + if (!expectCommand) continue; - discovered.add(primary); + // Skip env assignments (VAR=val) + if (ENV_ASSIGNMENT_REGEX.test(token)) continue; - if (WRAPPER_COMMANDS.has(primary) && index + 1 < tokens.length) { - const wrapped = sanitizeToken(tokens[index + 1]).toLowerCase(); - if (wrapped) { - discovered.add(wrapped); - } + const name = token.toLowerCase(); + if (!name) continue; + + discovered.add(name); + + if (WRAPPER_COMMANDS.has(name)) { + // Don't mark command as found yet — the next token is the real command + continue; } + + expectCommand = false; } return Array.from(discovered); } -function findBlockedCommands(command: string): string[] { +function findBlockedCommands(command: string, sessionAllowedCommands?: Set): string[] { const invoked = extractCommandNames(command); if (!invoked.length) return []; const allowList = getSecurityAllowList(); - if (!allowList.length) return invoked; + if (!allowList.length && (!sessionAllowedCommands || sessionAllowedCommands.size === 0)) return invoked; const allowSet = new Set(allowList); if (allowSet.has('*')) return []; - return invoked.filter((cmd) => !allowSet.has(cmd)); + return invoked.filter((cmd) => !allowSet.has(cmd) && !sessionAllowedCommands?.has(cmd)); } -// export const BlockedResult = { -// stdout: '', -// stderr: `Command blocked by security policy. Update ${SECURITY_CONFIG_PATH} to allow them before retrying.`, -// exitCode: 126, -// }; - -export function isBlocked(command: string): boolean { - const blocked = findBlockedCommands(command); +export function isBlocked(command: string, sessionAllowedCommands?: Set): boolean { + const blocked = findBlockedCommands(command, sessionAllowedCommands); return blocked.length > 0; } diff --git a/apps/x/packages/core/src/config/security.ts b/apps/x/packages/core/src/config/security.ts index b079f534..ac1afa58 100644 --- a/apps/x/packages/core/src/config/security.ts +++ b/apps/x/packages/core/src/config/security.ts @@ -20,16 +20,6 @@ const DEFAULT_ALLOW_LIST = [ let cachedAllowList: string[] | null = null; let cachedMtimeMs: number | null = null; -/** In-memory session allowlist — resets on app restart */ -const sessionAllowSet = new Set(); - -export function addToSessionAllowList(commands: string[]): void { - for (const cmd of commands) { - const normalized = cmd.trim().toLowerCase(); - if (normalized) sessionAllowSet.add(normalized); - } -} - export async function addToSecurityConfig(commands: string[]): Promise { ensureSecurityConfigSync(); const current = readAllowList(); @@ -126,28 +116,16 @@ export function getSecurityAllowList(): string[] { ensureSecurityConfigSync(); try { const stats = fs.statSync(SECURITY_CONFIG_PATH); - let fileList: string[]; if (cachedAllowList && cachedMtimeMs === stats.mtimeMs) { - fileList = cachedAllowList; - } else { - fileList = readAllowList(); - cachedAllowList = fileList; - cachedMtimeMs = stats.mtimeMs; + return cachedAllowList; } - - // Merge session allowlist - if (sessionAllowSet.size === 0) return fileList; - const merged = new Set(fileList); - for (const cmd of sessionAllowSet) merged.add(cmd); - return Array.from(merged); + cachedAllowList = readAllowList(); + cachedMtimeMs = stats.mtimeMs; + return cachedAllowList; } catch { cachedAllowList = null; cachedMtimeMs = null; - const fileList = readAllowList(); - if (sessionAllowSet.size === 0) return fileList; - const merged = new Set(fileList); - for (const cmd of sessionAllowSet) merged.add(cmd); - return Array.from(merged); + return readAllowList(); } } diff --git a/apps/x/packages/core/src/runs/runs.ts b/apps/x/packages/core/src/runs/runs.ts index 8bfc2e46..75f71d5f 100644 --- a/apps/x/packages/core/src/runs/runs.ts +++ b/apps/x/packages/core/src/runs/runs.ts @@ -1,7 +1,7 @@ import z from "zod"; import container from "../di/container.js"; import { IMessageQueue } from "../application/lib/message-queue.js"; -import { AskHumanResponseEvent, ToolPermissionResponseEvent, CreateRunOptions, Run, ListRunsResponse, ToolPermissionAuthorizePayload, AskHumanResponsePayload } from "@x/shared/dist/runs.js"; +import { AskHumanResponseEvent, ToolPermissionRequestEvent, ToolPermissionResponseEvent, CreateRunOptions, Run, ListRunsResponse, ToolPermissionAuthorizePayload, AskHumanResponsePayload } from "@x/shared/dist/runs.js"; import { IRunsRepo } from "./repo.js"; import { IAgentRuntime } from "../agents/runtime.js"; import { IBus } from "../application/lib/bus.js"; @@ -9,7 +9,7 @@ import { IAbortRegistry } from "./abort-registry.js"; import { IRunsLock } from "./lock.js"; import { forceCloseAllMcpClients } from "../mcp/mcp.js"; import { extractCommandNames } from "../application/lib/command-executor.js"; -import { addToSessionAllowList, addToSecurityConfig } from "../config/security.js"; +import { addToSecurityConfig } from "../config/security.js"; export async function createRun(opts: z.infer): Promise> { const repo = container.resolve('runsRepo'); @@ -28,15 +28,21 @@ export async function createMessage(runId: string, message: string): Promise): Promise { - const { scope, command, ...rest } = ev; + const { scope, ...rest } = ev; - // Handle scope side-effects when approving - if (rest.response === "approve" && command && scope && scope !== "once") { - const commandNames = extractCommandNames(command); - if (commandNames.length > 0) { - if (scope === "session") { - addToSessionAllowList(commandNames); - } else if (scope === "always") { + // For "always" scope, derive command from the run log and persist to security config + if (rest.response === "approve" && scope === "always") { + const repo = container.resolve('runsRepo'); + const run = await repo.fetch(runId); + const permReqEvent = run.log.find( + (e): e is z.infer => + e.type === "tool-permission-request" + && e.toolCall.toolCallId === rest.toolCallId + && JSON.stringify(e.subflow) === JSON.stringify(rest.subflow) + ); + if (permReqEvent && typeof permReqEvent.toolCall.arguments === 'object' && permReqEvent.toolCall.arguments !== null && 'command' in permReqEvent.toolCall.arguments) { + const commandNames = extractCommandNames(String(permReqEvent.toolCall.arguments.command)); + if (commandNames.length > 0) { await addToSecurityConfig(commandNames); } } @@ -47,6 +53,7 @@ export async function authorizePermission(runId: string, ev: z.infer('agentRuntime'); diff --git a/apps/x/packages/shared/src/runs.ts b/apps/x/packages/shared/src/runs.ts index f2ee49cb..5f52f611 100644 --- a/apps/x/packages/shared/src/runs.ts +++ b/apps/x/packages/shared/src/runs.ts @@ -73,6 +73,7 @@ export const ToolPermissionResponseEvent = BaseRunEvent.extend({ type: z.literal("tool-permission-response"), toolCallId: z.string(), response: z.enum(["approve", "deny"]), + scope: z.enum(["once", "session", "always"]).optional(), }); export const RunErrorEvent = BaseRunEvent.extend({ @@ -106,9 +107,7 @@ export const ToolPermissionAuthorizePayload = ToolPermissionResponseEvent.pick({ subflow: true, toolCallId: true, response: true, -}).extend({ - scope: z.enum(["once", "session", "always"]).optional(), - command: z.string().optional(), + scope: true, }); export const AskHumanResponsePayload = AskHumanResponseEvent.pick({ diff --git a/apps/x/pnpm-lock.yaml b/apps/x/pnpm-lock.yaml index fa8765d6..ac5e5d55 100644 --- a/apps/x/pnpm-lock.yaml +++ b/apps/x/pnpm-lock.yaml @@ -380,6 +380,9 @@ importers: react: specifier: ^19.2.3 version: 19.2.3 + shell-quote: + specifier: ^1.8.3 + version: 1.8.3 xlsx: specifier: ^0.18.5 version: 0.18.5 @@ -399,6 +402,9 @@ importers: '@types/pdf-parse': specifier: ^1.1.5 version: 1.1.5 + '@types/shell-quote': + specifier: ^1.7.5 + version: 1.7.5 packages/shared: dependencies: @@ -3346,6 +3352,9 @@ packages: '@types/responselike@1.0.3': resolution: {integrity: sha512-H/+L+UkTV33uf49PH5pCAUBVPNj2nDBXTN+qS1dOwyyg24l3CcicicCA7ca+HMvJBZcFgl5r8e+RR6elsb4Lyw==} + '@types/shell-quote@1.7.5': + resolution: {integrity: sha512-+UE8GAGRPbJVQDdxi16dgadcBfQ+KG2vgZhV1+3A1XmHbmwcdwhCUwIdy+d3pAGrbvgRoVSjeI9vOWyq376Yzw==} + '@types/trusted-types@2.0.7': resolution: {integrity: sha512-ScaPdn1dQczgbl0QFTeTOmVHFULt394XJgOQNoyVhZ6r2vLnMLJfBPd53SB52T/3G36VI1/g2MZaX0cwDuXsfw==} @@ -11105,6 +11114,8 @@ snapshots: dependencies: '@types/node': 25.0.3 + '@types/shell-quote@1.7.5': {} + '@types/trusted-types@2.0.7': optional: true