mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-07 16:31:23 +00:00
fix(acp): harden permission auto-approval policy
This commit is contained in:
Peter Steinberger
@@ -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" },
|
||||
|
||||
@@ -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<string, string>([
|
||||
["read", "read"],
|
||||
["search", "search"],
|
||||
["web_search", "search"],
|
||||
["memory_search", "search"],
|
||||
]);
|
||||
|
||||
type PermissionOption = RequestPermissionRequest["options"][number];
|
||||
|
||||
type PermissionResolverDeps = {
|
||||
prompt?: (toolName: string | undefined, toolTitle?: string) => Promise<boolean>;
|
||||
log?: (line: string) => void;
|
||||
cwd?: string;
|
||||
};
|
||||
|
||||
function asRecord(value: unknown): Record<string, unknown> | 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<RequestPermissionResponse> {
|
||||
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<AcpC
|
||||
printSessionUpdate(params);
|
||||
},
|
||||
requestPermission: async (params: RequestPermissionRequest) => {
|
||||
return resolvePermissionRequest(params);
|
||||
return resolvePermissionRequest(params, { cwd });
|
||||
},
|
||||
}),
|
||||
stream,
|
||||
|
||||
Reference in New Issue
Block a user