mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-09 15:59:30 +00:00
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
This commit is contained in:
Pavan Kumar Gondhi
GitHub
parent
36227737c5
commit
9775f09194
3 changed files with 248 additions and 35 deletions
|
|
@ -4,6 +4,7 @@ import { classifyAcpToolApproval } from "./approval-classifier.js";
|
|||
|
||||
function classify(params: {
|
||||
title: string;
|
||||
locations?: Array<{ path: string; line?: number }>;
|
||||
rawInput?: Record<string, unknown>;
|
||||
meta?: Record<string, unknown>;
|
||||
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({
|
||||
|
|
|
|||
|
|
@ -46,6 +46,13 @@ type AcpApprovalClassification = {
|
|||
autoApprove: boolean;
|
||||
};
|
||||
|
||||
type AcpApprovalToolCall = {
|
||||
title?: string | null;
|
||||
_meta?: unknown;
|
||||
rawInput?: unknown;
|
||||
locations?: unknown;
|
||||
};
|
||||
|
||||
function readFirstStringValue(
|
||||
source: Record<string, unknown> | 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)) {
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue