diff --git a/CHANGELOG.md b/CHANGELOG.md index 71dcb752b8a..86f9e8a6a79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,7 @@ Docs: https://docs.openclaw.ai - Gateway/Restart: treat child listener PIDs as owned by the service runtime PID during restart health checks to avoid false stale-process kills and restart timeouts on launchd/systemd. (#24696) Thanks @gumadeiras. - Config/Write: apply `unsetPaths` with immutable path-copy updates so config writes never mutate caller-provided objects, and harden `openclaw config get/set/unset` path traversal by rejecting prototype-key segments and inherited-property traversal. (#24134) thanks @frankekn. - Security/Exec: detect obfuscated commands before exec allowlist decisions and require explicit approval for obfuscation patterns. (#8592) Thanks @CornBrother0x and @vincentkoc. +- Security/ACP: harden ACP client permission auto-approval to require trusted core tool IDs, ignore untrusted `toolCall.kind` hints, and scope `read` auto-approval to the active working directory so unknown tool names and out-of-scope file reads always prompt. - Security/Skills: escape user-controlled prompt, filename, and output-path values in `openai-image-gen` HTML gallery generation to prevent stored XSS in generated `index.html` output. (#12538) Thanks @CornBrother0x. - Security/Skills: harden `skill-creator` packaging by skipping symlink entries and rejecting files whose resolved paths escape the selected skill root. (#24260, #16959) Thanks @CornBrother0x and @vincentkoc. - Security/OTEL: redact sensitive values (API keys, tokens, credential fields) from diagnostics-otel log bodies, log attributes, and error/reason span fields before OTLP export. (#12542) Thanks @brandonwise. diff --git a/docs/cli/acp.md b/docs/cli/acp.md index 9535509016d..1b1981395e4 100644 --- a/docs/cli/acp.md +++ b/docs/cli/acp.md @@ -49,6 +49,13 @@ openclaw acp client --server-args --url wss://gateway-host:18789 --token-file ~/ openclaw acp client --server "node" --server-args openclaw.mjs acp --url ws://127.0.0.1:19001 ``` +Permission model (client debug mode): + +- Auto-approval is allowlist-based and only applies to trusted core tool IDs. +- `read` auto-approval is scoped to the current working directory (`--cwd` when set). +- Unknown/non-core tool names, out-of-scope reads, and dangerous tools always require explicit prompt approval. +- Server-provided `toolCall.kind` is treated as untrusted metadata (not an authorization source). + ## How to use this Use ACP when an IDE (or other client) speaks Agent Client Protocol and you want diff --git a/src/acp/client.test.ts b/src/acp/client.test.ts index 90fad779619..b7596ff42a9 100644 --- a/src/acp/client.test.ts +++ b/src/acp/client.test.ts @@ -74,6 +74,32 @@ describe("resolvePermissionRequest", () => { expect(prompt).not.toHaveBeenCalled(); }); + it("prompts for read outside cwd scope", async () => { + const prompt = vi.fn(async () => false); + const res = await resolvePermissionRequest( + makePermissionRequest({ + toolCall: { toolCallId: "tool-r", title: "read: ~/.ssh/id_rsa", status: "pending" }, + }), + { prompt, log: () => {} }, + ); + expect(prompt).toHaveBeenCalledTimes(1); + expect(prompt).toHaveBeenCalledWith("read", "read: ~/.ssh/id_rsa"); + expect(res).toEqual({ outcome: { outcome: "selected", optionId: "reject" } }); + }); + + it("prompts for non-core read-like tool names", async () => { + const prompt = vi.fn(async () => false); + const res = await resolvePermissionRequest( + makePermissionRequest({ + toolCall: { toolCallId: "tool-fr", title: "fs_read: ~/.ssh/id_rsa", status: "pending" }, + }), + { prompt, log: () => {} }, + ); + expect(prompt).toHaveBeenCalledTimes(1); + expect(prompt).toHaveBeenCalledWith("fs_read", "fs_read: ~/.ssh/id_rsa"); + expect(res).toEqual({ outcome: { outcome: "selected", optionId: "reject" } }); + }); + it.each([ { caseName: "prompts for fetch even when tool name is known", @@ -100,6 +126,24 @@ describe("resolvePermissionRequest", () => { expect(res).toEqual({ outcome: { outcome: "selected", optionId: "reject" } }); }); + it("prompts when kind is spoofed as read", async () => { + const prompt = vi.fn(async () => false); + const res = await resolvePermissionRequest( + makePermissionRequest({ + toolCall: { + toolCallId: "tool-kind-spoof", + title: "thread: reply", + status: "pending", + kind: "read", + }, + }), + { prompt, log: () => {} }, + ); + expect(prompt).toHaveBeenCalledTimes(1); + expect(prompt).toHaveBeenCalledWith("thread", "thread: reply"); + expect(res).toEqual({ outcome: { outcome: "selected", optionId: "reject" } }); + }); + it("uses allow_always and reject_always when once options are absent", async () => { const options: RequestPermissionRequest["options"] = [ { kind: "allow_always", name: "Always allow", optionId: "allow-always" }, diff --git a/src/acp/client.ts b/src/acp/client.ts index 1eaf70c005f..ca88e18bce4 100644 --- a/src/acp/client.ts +++ b/src/acp/client.ts @@ -1,5 +1,6 @@ import { spawn, type ChildProcess } from "node:child_process"; import fs from "node:fs"; +import { homedir } from "node:os"; import path from "node:path"; import * as readline from "node:readline"; import { Readable, Writable } from "node:stream"; @@ -12,16 +13,26 @@ import { type RequestPermissionResponse, type SessionNotification, } from "@agentclientprotocol/sdk"; +import { isKnownCoreToolId } from "../agents/tool-catalog.js"; import { ensureOpenClawCliOnPath } from "../infra/path-env.js"; import { DANGEROUS_ACP_TOOLS } from "../security/dangerous-tools.js"; -const SAFE_AUTO_APPROVE_KINDS = new Set(["read", "search"]); +const SAFE_AUTO_APPROVE_TOOL_IDS = new Set(["read", "search", "web_search", "memory_search"]); +const TRUSTED_SAFE_TOOL_ALIASES = new Set(["search"]); +const READ_TOOL_PATH_KEYS = ["path", "file_path", "filePath"]; +const TOOL_KIND_BY_ID = new Map([ + ["read", "read"], + ["search", "search"], + ["web_search", "search"], + ["memory_search", "search"], +]); type PermissionOption = RequestPermissionRequest["options"][number]; type PermissionResolverDeps = { prompt?: (toolName: string | undefined, toolTitle?: string) => Promise; log?: (line: string) => void; + cwd?: string; }; function asRecord(value: unknown): Record | undefined { @@ -65,52 +76,11 @@ function parseToolNameFromTitle(title: string | undefined | null): string | unde return normalizeToolName(head); } -function resolveToolKindForPermission( - params: RequestPermissionRequest, - toolName: string | undefined, -): string | undefined { - const toolCall = params.toolCall as unknown as { kind?: unknown; title?: unknown } | undefined; - const kindRaw = typeof toolCall?.kind === "string" ? toolCall.kind.trim().toLowerCase() : ""; - if (kindRaw) { - return kindRaw; - } - const name = - toolName ?? - parseToolNameFromTitle(typeof toolCall?.title === "string" ? toolCall.title : undefined); - if (!name) { +function resolveToolKindForPermission(toolName: string | undefined): string | undefined { + if (!toolName) { return undefined; } - const normalized = name.toLowerCase(); - - const hasToken = (token: string) => { - // Tool names tend to be snake_case. Avoid substring heuristics (ex: "thread" contains "read"). - const re = new RegExp(`(?:^|[._-])${token}(?:$|[._-])`); - return re.test(normalized); - }; - - // Prefer a conservative classifier: only classify safe kinds when confident. - if (normalized === "read" || hasToken("read")) { - return "read"; - } - if (normalized === "search" || hasToken("search") || hasToken("find")) { - return "search"; - } - if (normalized.includes("fetch") || normalized.includes("http")) { - return "fetch"; - } - if (normalized.includes("write") || normalized.includes("edit") || normalized.includes("patch")) { - return "edit"; - } - if (normalized.includes("delete") || normalized.includes("remove")) { - return "delete"; - } - if (normalized.includes("move") || normalized.includes("rename")) { - return "move"; - } - if (normalized.includes("exec") || normalized.includes("run") || normalized.includes("bash")) { - return "execute"; - } - return "other"; + return TOOL_KIND_BY_ID.get(toolName) ?? "other"; } function resolveToolNameForPermission(params: RequestPermissionRequest): string | undefined { @@ -124,6 +94,109 @@ function resolveToolNameForPermission(params: RequestPermissionRequest): string return normalizeToolName(fromMeta ?? fromRawInput ?? fromTitle ?? ""); } +function extractPathFromToolTitle( + toolTitle: string | undefined, + toolName: string | undefined, +): string | undefined { + if (!toolTitle) { + return undefined; + } + const separator = toolTitle.indexOf(":"); + if (separator < 0) { + return undefined; + } + const tail = toolTitle.slice(separator + 1).trim(); + if (!tail) { + return undefined; + } + const keyedMatch = tail.match(/(?:^|,\s*)(?:path|file_path|filePath)\s*:\s*([^,]+)/); + if (keyedMatch?.[1]) { + return keyedMatch[1].trim(); + } + if (toolName === "read") { + return tail; + } + return undefined; +} + +function resolveToolPathCandidate( + params: RequestPermissionRequest, + toolName: string | undefined, + toolTitle: string | undefined, +): string | undefined { + const rawInput = asRecord(params.toolCall?.rawInput); + const fromRawInput = readFirstStringValue(rawInput, READ_TOOL_PATH_KEYS); + const fromTitle = extractPathFromToolTitle(toolTitle, toolName); + return fromRawInput ?? fromTitle; +} + +function resolveAbsoluteScopedPath(value: string, cwd: string): string | undefined { + let candidate = value.trim(); + if (!candidate) { + return undefined; + } + if (candidate.startsWith("file://")) { + try { + const parsed = new URL(candidate); + candidate = decodeURIComponent(parsed.pathname || ""); + } catch { + return undefined; + } + } + if (candidate === "~") { + candidate = homedir(); + } else if (candidate.startsWith("~/")) { + candidate = path.join(homedir(), candidate.slice(2)); + } + const absolute = path.isAbsolute(candidate) + ? path.normalize(candidate) + : path.resolve(cwd, candidate); + return absolute; +} + +function isPathWithinRoot(candidatePath: string, root: string): boolean { + const relative = path.relative(root, candidatePath); + return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative)); +} + +function isReadToolCallScopedToCwd( + params: RequestPermissionRequest, + toolName: string | undefined, + toolTitle: string | undefined, + cwd: string, +): boolean { + if (toolName !== "read") { + return false; + } + const rawPath = resolveToolPathCandidate(params, toolName, toolTitle); + if (!rawPath) { + return false; + } + const absolutePath = resolveAbsoluteScopedPath(rawPath, cwd); + if (!absolutePath) { + return false; + } + return isPathWithinRoot(absolutePath, path.resolve(cwd)); +} + +function shouldAutoApproveToolCall( + params: RequestPermissionRequest, + toolName: string | undefined, + toolTitle: string | undefined, + cwd: string, +): boolean { + const isTrustedToolId = + typeof toolName === "string" && + (isKnownCoreToolId(toolName) || TRUSTED_SAFE_TOOL_ALIASES.has(toolName)); + if (!toolName || !isTrustedToolId || !SAFE_AUTO_APPROVE_TOOL_IDS.has(toolName)) { + return false; + } + if (toolName === "read") { + return isReadToolCallScopedToCwd(params, toolName, toolTitle, cwd); + } + return true; +} + function pickOption( options: PermissionOption[], kinds: PermissionOption["kind"][], @@ -191,10 +264,11 @@ export async function resolvePermissionRequest( ): Promise { const log = deps.log ?? ((line: string) => console.error(line)); const prompt = deps.prompt ?? promptUserPermission; + const cwd = deps.cwd ?? process.cwd(); const options = params.options ?? []; const toolTitle = params.toolCall?.title ?? "tool"; const toolName = resolveToolNameForPermission(params); - const toolKind = resolveToolKindForPermission(params, toolName); + const toolKind = resolveToolKindForPermission(toolName); if (options.length === 0) { log(`[permission cancelled] ${toolName ?? "unknown"}: no options available`); @@ -203,8 +277,8 @@ export async function resolvePermissionRequest( const allowOption = pickOption(options, ["allow_once", "allow_always"]); const rejectOption = pickOption(options, ["reject_once", "reject_always"]); - const isSafeKind = Boolean(toolKind && SAFE_AUTO_APPROVE_KINDS.has(toolKind)); - const promptRequired = !toolName || !isSafeKind || DANGEROUS_ACP_TOOLS.has(toolName); + const autoApproveAllowed = shouldAutoApproveToolCall(params, toolName, toolTitle, cwd); + const promptRequired = !toolName || !autoApproveAllowed || DANGEROUS_ACP_TOOLS.has(toolName); if (!promptRequired) { const option = allowOption ?? options[0]; @@ -350,7 +424,7 @@ export async function createAcpClient(opts: AcpClientOptions = {}): Promise { - return resolvePermissionRequest(params); + return resolvePermissionRequest(params, { cwd }); }, }), stream, diff --git a/src/agents/tool-catalog.ts b/src/agents/tool-catalog.ts index 0b0d37ae5ed..705656889cb 100644 --- a/src/agents/tool-catalog.ts +++ b/src/agents/tool-catalog.ts @@ -320,3 +320,7 @@ export function resolveCoreToolProfiles(toolId: string): ToolProfileId[] { } return [...tool.profiles]; } + +export function isKnownCoreToolId(toolId: string): boolean { + return CORE_TOOL_BY_ID.has(toolId); +}