From 9775f09194e1fc83a757dfba4198a2f4ac290b19 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Gondhi Date: Thu, 9 Jul 2026 20:33:18 +0530 Subject: [PATCH] fix: scope ACP search path approvals [AI] (#102416) * fix: scope acp search path approvals * fix: include acp search locations in approval scope * fix: keep acp read path approval unchanged * fix: keep acp search title text display-only * fix: parse explicit acp search title paths --- src/acp/approval-classifier.test.ts | 95 ++++++++++++++++++++++++++++ src/acp/approval-classifier.ts | 90 +++++++++++++++----------- src/acp/client.test.ts | 98 +++++++++++++++++++++++++++++ 3 files changed, 248 insertions(+), 35 deletions(-) diff --git a/src/acp/approval-classifier.test.ts b/src/acp/approval-classifier.test.ts index 88fdb4c2b06c..4123840b0428 100644 --- a/src/acp/approval-classifier.test.ts +++ b/src/acp/approval-classifier.test.ts @@ -4,6 +4,7 @@ import { classifyAcpToolApproval } from "./approval-classifier.js"; function classify(params: { title: string; + locations?: Array<{ path: string; line?: number }>; rawInput?: Record; meta?: Record; cwd?: string; @@ -12,6 +13,7 @@ function classify(params: { cwd: params.cwd ?? "/workspace", toolCall: { title: params.title, + locations: params.locations, rawInput: params.rawInput, _meta: params.meta, }, @@ -45,6 +47,19 @@ describe("classifyAcpToolApproval", () => { }); }); + it("does not auto-approve reads from locations-only metadata", () => { + expect( + classify({ + title: "read", + locations: [{ path: "src/index.ts" }], + }), + ).toEqual({ + toolName: "read", + approvalClass: "other", + autoApprove: false, + }); + }); + it("auto-approves readonly search tools", () => { expect( classify({ @@ -58,6 +73,86 @@ describe("classifyAcpToolApproval", () => { }); }); + it("auto-approves alias search when its path stays inside cwd", () => { + expect( + classify({ + title: "search: query: TODO, path: src", + rawInput: { name: "search", query: "TODO", path: "src" }, + }), + ).toEqual({ + toolName: "search", + approvalClass: "readonly_search", + autoApprove: true, + }); + }); + + it("does not auto-approve alias search when its rawInput path escapes cwd", () => { + expect( + classify({ + title: "search: ignored-by-raw-input", + rawInput: { name: "search", query: "key", path: "~/.ssh" }, + }), + ).toEqual({ + toolName: "search", + approvalClass: "other", + autoApprove: false, + }); + }); + + it("auto-approves alias search when query-like title text contains a path label", () => { + expect( + classify({ + title: "search: query: literal text, path: /etc", + rawInput: { name: "search", query: "literal text, path: /etc" }, + }), + ).toEqual({ + toolName: "search", + approvalClass: "readonly_search", + autoApprove: true, + }); + }); + + it("does not auto-approve alias search when explicit title path escapes cwd", () => { + expect( + classify({ + title: "search: path: /etc", + rawInput: { name: "search", query: "shadow" }, + }), + ).toEqual({ + toolName: "search", + approvalClass: "other", + autoApprove: false, + }); + }); + + it("does not auto-approve alias search when only locations escape cwd", () => { + expect( + classify({ + title: "search: TODO", + rawInput: { name: "search", query: "TODO" }, + locations: [{ path: "/etc/passwd" }], + }), + ).toEqual({ + toolName: "search", + approvalClass: "other", + autoApprove: false, + }); + }); + + it("does not auto-approve alias search when any location escapes cwd", () => { + expect( + classify({ + title: "search: TODO", + rawInput: { name: "search", query: "TODO" }, + locations: [{ path: "src/index.ts" }, { path: "/etc/passwd" }], + }), + ).toEqual({ + toolName: "search", + approvalClass: "other", + autoApprove: false, + }); + }); + it("classifies process as exec-capable even for readonly-like actions", () => { expect( classify({ diff --git a/src/acp/approval-classifier.ts b/src/acp/approval-classifier.ts index 7082728f0bbf..c3e0c5dbde75 100644 --- a/src/acp/approval-classifier.ts +++ b/src/acp/approval-classifier.ts @@ -46,6 +46,13 @@ type AcpApprovalClassification = { autoApprove: boolean; }; +type AcpApprovalToolCall = { + title?: string | null; + _meta?: unknown; + rawInput?: unknown; + locations?: unknown; +}; + function readFirstStringValue( source: Record | undefined, keys: string[], @@ -119,25 +126,42 @@ function extractPathFromToolTitle( if (!tail) { return undefined; } - const keyedMatch = tail.match(/(?:^|,\s*)(?:path|file_path|filePath)\s*:\s*([^,]+)/); + const keyedMatch = + toolName === "read" + ? tail.match(/(?:^|,\s*)(?:path|file_path|filePath)\s*:\s*([^,]+)/) + : tail.match(/^(?:path|file_path|filePath)\s*:\s*([^,]+)/); if (keyedMatch?.[1]) { return keyedMatch[1].trim(); } return toolName === "read" ? tail : undefined; } -function resolveToolPathCandidate( - params: { - toolCall?: { rawInput?: unknown }; - }, - toolName: string | undefined, - toolTitle: string | undefined, -): string | undefined { +function readLocationPaths(locations: unknown): string[] { + if (!Array.isArray(locations)) { + return []; + } + const paths: string[] = []; + for (const location of locations) { + const pathValue = readFirstStringValue(asRecord(location), ["path", "file_path", "filePath"]); + if (pathValue) { + paths.push(pathValue); + } + } + return paths; +} + +function resolveToolPathCandidates(params: { + includeLocations?: boolean; + toolCall?: AcpApprovalToolCall; + toolName: string | undefined; + toolTitle: string | undefined; +}): string[] { const rawInput = asRecord(params.toolCall?.rawInput); - return ( - readFirstStringValue(rawInput, ["path", "file_path", "filePath"]) ?? - extractPathFromToolTitle(toolTitle, toolName) - ); + return [ + readFirstStringValue(rawInput, ["path", "file_path", "filePath"]), + extractPathFromToolTitle(params.toolTitle, params.toolName), + ...(params.includeLocations ? readLocationPaths(params.toolCall?.locations) : []), + ].filter((value): value is string => value !== undefined); } function resolveAbsoluteScopedPath(value: string, cwd: string): string | undefined { @@ -161,19 +185,7 @@ function resolveAbsoluteScopedPath(value: string, cwd: string): string | undefin return path.isAbsolute(candidate) ? path.normalize(candidate) : path.resolve(cwd, candidate); } -function isReadToolCallScopedToCwd( - params: { toolCall?: { rawInput?: unknown } }, - toolName: string | undefined, - toolTitle: string | undefined, - cwd: string, -): boolean { - if (toolName !== "read") { - return false; - } - const rawPath = resolveToolPathCandidate(params, toolName, toolTitle); - if (!rawPath) { - return false; - } +function isToolPathScopedToCwd(rawPath: string, cwd: string): boolean { const absolutePath = resolveAbsoluteScopedPath(rawPath, cwd); if (!absolutePath) { return false; @@ -183,11 +195,7 @@ function isReadToolCallScopedToCwd( /** Resolves the ACP approval class for one tool call, failing closed on spoofed tool identity. */ export function classifyAcpToolApproval(params: { - toolCall?: { - title?: string | null; - _meta?: unknown; - rawInput?: unknown; - }; + toolCall?: AcpApprovalToolCall; cwd: string; }): AcpApprovalClassification { const toolName = resolveToolNameForPermission(params); @@ -197,12 +205,15 @@ export function classifyAcpToolApproval(params: { const isTrustedToolId = isKnownCoreToolId(toolName) || TRUSTED_SAFE_TOOL_ALIASES.has(toolName); if (toolName === "read" && isTrustedToolId) { - const autoApprove = isReadToolCallScopedToCwd( - params, + const rawPaths = resolveToolPathCandidates({ + includeLocations: false, + toolCall: params.toolCall, toolName, - params.toolCall?.title ?? undefined, - params.cwd, - ); + toolTitle: params.toolCall?.title ?? undefined, + }); + const autoApprove = + rawPaths.length > 0 && + rawPaths.every((rawPath) => isToolPathScopedToCwd(rawPath, params.cwd)); return { toolName, approvalClass: autoApprove ? "readonly_scoped" : "other", @@ -210,6 +221,15 @@ export function classifyAcpToolApproval(params: { }; } if (SAFE_SEARCH_TOOL_IDS.has(toolName) && isTrustedToolId) { + const rawPaths = resolveToolPathCandidates({ + includeLocations: true, + toolCall: params.toolCall, + toolName, + toolTitle: params.toolCall?.title ?? undefined, + }); + if (rawPaths.some((rawPath) => !isToolPathScopedToCwd(rawPath, params.cwd))) { + return { toolName, approvalClass: "other", autoApprove: false }; + } return { toolName, approvalClass: "readonly_search", autoApprove: true }; } if (EXEC_CAPABLE_TOOL_IDS.has(toolName)) { diff --git a/src/acp/client.test.ts b/src/acp/client.test.ts index c7ba369810b8..d9c2c043b544 100644 --- a/src/acp/client.test.ts +++ b/src/acp/client.test.ts @@ -505,6 +505,104 @@ describe("resolvePermissionRequest", () => { expect(prompt).not.toHaveBeenCalled(); }); + it("auto-approves search when rawInput path resolves inside cwd", async () => { + await expectAutoAllowWithoutPrompt({ + request: { + toolCall: { + toolCallId: "tool-search-inside-cwd", + title: "search: ignored-by-raw-input", + status: "pending", + rawInput: { name: "search", query: "TODO", path: "src" }, + }, + }, + cwd: "/tmp/openclaw-acp-cwd", + }); + }); + + it("prompts for search when rawInput path escapes cwd", async () => { + const prompt = vi.fn(async () => false); + const res = await resolvePermissionRequest( + makePermissionRequest({ + toolCall: { + toolCallId: "tool-search-escape-cwd", + title: "search: ignored-by-raw-input", + status: "pending", + rawInput: { name: "search", query: "key", path: "../.ssh" }, + }, + }), + { prompt, log: () => {}, cwd: "/tmp/openclaw-acp-cwd/workspace" }, + ); + expect(prompt).toHaveBeenCalledTimes(1); + expect(prompt).toHaveBeenCalledWith("search", "search: ignored-by-raw-input"); + expect(res).toEqual({ outcome: { outcome: "selected", optionId: "reject" } }); + }); + + it("auto-approves search when query-like title text contains a path label", async () => { + await expectAutoAllowWithoutPrompt({ + request: { + toolCall: { + toolCallId: "tool-search-title-query-path-label", + title: "search: query: literal text, path: ~/.ssh", + status: "pending", + rawInput: { name: "search", query: "literal text, path: ~/.ssh" }, + }, + }, + cwd: "/tmp/openclaw-acp-cwd/workspace", + }); + }); + + it("prompts for search when explicit title path escapes cwd", async () => { + const prompt = vi.fn(async () => false); + const res = await resolvePermissionRequest( + makePermissionRequest({ + toolCall: { + toolCallId: "tool-search-title-escape-cwd", + title: "search: path: ~/.ssh", + status: "pending", + rawInput: { name: "search", query: "key" }, + }, + }), + { prompt, log: () => {}, cwd: "/tmp/openclaw-acp-cwd/workspace" }, + ); + expect(prompt).toHaveBeenCalledTimes(1); + expect(prompt).toHaveBeenCalledWith("search", "search: path: ~/.ssh"); + expect(res).toEqual({ outcome: { outcome: "selected", optionId: "reject" } }); + }); + + it("auto-approves search when only locations resolve inside cwd", async () => { + await expectAutoAllowWithoutPrompt({ + request: { + toolCall: { + toolCallId: "tool-search-location-inside-cwd", + title: "search: TODO", + status: "pending", + rawInput: { name: "search", query: "TODO" }, + locations: [{ path: "src/index.ts" }], + }, + }, + cwd: "/tmp/openclaw-acp-cwd", + }); + }); + + it("prompts for search when only locations escape cwd", async () => { + const prompt = vi.fn(async () => false); + const res = await resolvePermissionRequest( + makePermissionRequest({ + toolCall: { + toolCallId: "tool-search-location-escape-cwd", + title: "search: TODO", + status: "pending", + rawInput: { name: "search", query: "TODO" }, + locations: [{ path: "/etc/passwd" }], + }, + }), + { prompt, log: () => {}, cwd: "/tmp/openclaw-acp-cwd/workspace" }, + ); + expect(prompt).toHaveBeenCalledTimes(1); + expect(prompt).toHaveBeenCalledWith("search", "search: TODO"); + expect(res).toEqual({ outcome: { outcome: "selected", optionId: "reject" } }); + }); + it("prompts when raw input spoofs a safe tool name for a dangerous title", async () => { const prompt = vi.fn(async () => false); const res = await resolvePermissionRequest(