fixed review comments

This commit is contained in:
Arjun 2026-02-23 08:31:39 +05:30
parent 71a3d2ff91
commit bfba0f0682
9 changed files with 85 additions and 157 deletions

View file

@ -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"
}
}

View file

@ -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<string, z.infer<typeof AskHumanRequestEvent>> = {};
allowedToolCallIds: Record<string, true> = {};
deniedToolCallIds: Record<string, true> = {};
sessionAllowedCommands: Set<string> = new Set();
getPendingPermissions(): z.infer<typeof ToolPermissionRequestEvent>[] {
const response: z.infer<typeof ToolPermissionRequestEvent>[] = [];
@ -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,

View file

@ -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<string>();
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>): 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<string>): boolean {
const blocked = findBlockedCommands(command, sessionAllowedCommands);
return blocked.length > 0;
}

View file

@ -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<string>();
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<void> {
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();
}
}

View file

@ -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<typeof CreateRunOptions>): Promise<z.infer<typeof Run>> {
const repo = container.resolve<IRunsRepo>('runsRepo');
@ -28,15 +28,21 @@ export async function createMessage(runId: string, message: string): Promise<str
}
export async function authorizePermission(runId: string, ev: z.infer<typeof ToolPermissionAuthorizePayload>): Promise<void> {
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<IRunsRepo>('runsRepo');
const run = await repo.fetch(runId);
const permReqEvent = run.log.find(
(e): e is z.infer<typeof ToolPermissionRequestEvent> =>
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<typeof Tool
...rest,
runId,
type: "tool-permission-response",
scope,
};
await repo.appendEvents(runId, [event]);
const runtime = container.resolve<IAgentRuntime>('agentRuntime');