diff --git a/CHANGELOG.md b/CHANGELOG.md index e851e2bfab..b44a1a111a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- Security/Gateway + ACP: block high-risk tools (`sessions_spawn`, `sessions_send`, `gateway`, `whatsapp_login`) from HTTP `/tools/invoke` by default with `gateway.tools.{allow,deny}` overrides, and harden ACP permission selection to fail closed when tool identity/options are ambiguous while supporting `allow_always`/`reject_always`. (#15390) Thanks @aether-ai-agent. - Security/Audit: distinguish external webhooks (`hooks.enabled`) from internal hooks (`hooks.internal.enabled`) in attack-surface summaries to avoid false exposure signals when only internal hooks are enabled. (#13474) Thanks @mcaxtr. - Auto-reply/Threading: auto-inject implicit reply threading so `replyToMode` works without requiring model-emitted `[[reply_to_current]]`, while preserving `replyToMode: "off"` behavior for implicit Slack replies and keeping block-streaming chunk coalescing stable under `replyToMode: "first"`. (#14976) Thanks @Diaspar4u. - Sandbox: pass configured `sandbox.docker.env` variables to sandbox containers at `docker create` time. (#15138) Thanks @stevebot-alive. diff --git a/docs/gateway/configuration-reference.md b/docs/gateway/configuration-reference.md index 11e2ea5fd1..902a432676 100644 --- a/docs/gateway/configuration-reference.md +++ b/docs/gateway/configuration-reference.md @@ -1912,6 +1912,12 @@ See [Plugins](/tools/plugin). // password: "your-password", }, trustedProxies: ["10.0.0.1"], + tools: { + // Additional /tools/invoke HTTP denies + deny: ["browser"], + // Remove tools from the default HTTP deny list + allow: ["gateway"], + }, }, } ``` @@ -1927,6 +1933,8 @@ See [Plugins](/tools/plugin). - `remote.transport`: `ssh` (default) or `direct` (ws/wss). For `direct`, `remote.url` must be `ws://` or `wss://`. - `gateway.remote.token` is for remote CLI calls only; does not enable local gateway auth. - `trustedProxies`: reverse proxy IPs that terminate TLS. Only list proxies you control. +- `gateway.tools.deny`: extra tool names blocked for HTTP `POST /tools/invoke` (extends default deny list). +- `gateway.tools.allow`: remove tool names from the default HTTP deny list. diff --git a/docs/gateway/tools-invoke-http-api.md b/docs/gateway/tools-invoke-http-api.md index 6f14308df1..88a98471aa 100644 --- a/docs/gateway/tools-invoke-http-api.md +++ b/docs/gateway/tools-invoke-http-api.md @@ -58,6 +58,28 @@ Tool availability is filtered through the same policy chain used by Gateway agen If a tool is not allowed by policy, the endpoint returns **404**. +Gateway HTTP also applies a hard deny list by default (even if session policy allows the tool): + +- `sessions_spawn` +- `sessions_send` +- `gateway` +- `whatsapp_login` + +You can customize this deny list via `gateway.tools`: + +```json5 +{ + gateway: { + tools: { + // Additional tools to block over HTTP /tools/invoke + deny: ["browser"], + // Remove tools from the default deny list + allow: ["gateway"], + }, + }, +} +``` + To help group policies resolve context, you can optionally set: - `x-openclaw-message-channel: ` (example: `slack`, `telegram`) diff --git a/src/acp/client.test.ts b/src/acp/client.test.ts index 7f4dd01537..778fa5272f 100644 --- a/src/acp/client.test.ts +++ b/src/acp/client.test.ts @@ -1,60 +1,92 @@ -import { describe, expect, it } from "vitest"; +import type { RequestPermissionRequest } from "@agentclientprotocol/sdk"; +import { describe, expect, it, vi } from "vitest"; +import { resolvePermissionRequest } from "./client.js"; -// Structural tests verify security-critical code exists in client.ts. -// Full integration tests with ACP SDK mocks deferred to future enhancement. +function makePermissionRequest( + overrides: Partial = {}, +): RequestPermissionRequest { + const { toolCall: toolCallOverride, options: optionsOverride, ...restOverrides } = overrides; + const base: RequestPermissionRequest = { + sessionId: "session-1", + toolCall: { + toolCallId: "tool-1", + title: "read: src/index.ts", + status: "pending", + }, + options: [ + { kind: "allow_once", name: "Allow once", optionId: "allow" }, + { kind: "reject_once", name: "Reject once", optionId: "reject" }, + ], + }; -describe("ACP client permission classification", () => { - it("should define dangerous tools that include exec and sessions_spawn", async () => { - const fs = await import("node:fs/promises"); - const path = await import("node:path"); - const source = await fs.readFile( - path.resolve(__dirname, "client.ts"), - "utf-8", - ); + return { + ...base, + ...restOverrides, + toolCall: toolCallOverride ? { ...base.toolCall, ...toolCallOverride } : base.toolCall, + options: optionsOverride ?? base.options, + }; +} - expect(source).toContain("DANGEROUS_ACP_TOOLS"); - expect(source).toContain('"exec"'); - expect(source).toContain('"sessions_spawn"'); - expect(source).toContain('"sessions_send"'); - expect(source).toContain('"gateway"'); +describe("resolvePermissionRequest", () => { + it("auto-approves safe tools without prompting", async () => { + const prompt = vi.fn(async () => true); + const res = await resolvePermissionRequest(makePermissionRequest(), { prompt, log: () => {} }); + expect(res).toEqual({ outcome: { outcome: "selected", optionId: "allow" } }); + expect(prompt).not.toHaveBeenCalled(); }); - it("should not auto-approve when options array is empty", async () => { - const fs = await import("node:fs/promises"); - const path = await import("node:path"); - const source = await fs.readFile( - path.resolve(__dirname, "client.ts"), - "utf-8", + it("prompts for dangerous tool names inferred from title", async () => { + const prompt = vi.fn(async () => true); + const res = await resolvePermissionRequest( + makePermissionRequest({ + toolCall: { toolCallId: "tool-2", title: "exec: uname -a", status: "pending" }, + }), + { prompt, log: () => {} }, ); - - // Verify the empty-options guard exists - expect(source).toContain("options.length === 0"); - // Verify it denies rather than approves - expect(source).toContain("no options available"); + expect(prompt).toHaveBeenCalledTimes(1); + expect(prompt).toHaveBeenCalledWith("exec", "exec: uname -a"); + expect(res).toEqual({ outcome: { outcome: "selected", optionId: "allow" } }); }); - it("should use stderr for permission logging (not stdout)", async () => { - const fs = await import("node:fs/promises"); - const path = await import("node:path"); - const source = await fs.readFile( - path.resolve(__dirname, "client.ts"), - "utf-8", + 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" }, + { kind: "reject_always", name: "Always reject", optionId: "reject-always" }, + ]; + const prompt = vi.fn(async () => false); + const res = await resolvePermissionRequest( + makePermissionRequest({ + toolCall: { toolCallId: "tool-3", title: "gateway: reload", status: "pending" }, + options, + }), + { prompt, log: () => {} }, ); - - // Permission logs should go to stderr to avoid corrupting ACP protocol on stdout - expect(source).toContain("console.error"); - expect(source).toContain("[permission"); + expect(res).toEqual({ outcome: { outcome: "selected", optionId: "reject-always" } }); }); - it("should have a 30-second timeout for interactive prompts", async () => { - const fs = await import("node:fs/promises"); - const path = await import("node:path"); - const source = await fs.readFile( - path.resolve(__dirname, "client.ts"), - "utf-8", + it("prompts when tool identity is unknown and can still approve", async () => { + const prompt = vi.fn(async () => true); + const res = await resolvePermissionRequest( + makePermissionRequest({ + toolCall: { + toolCallId: "tool-4", + title: "Modifying critical configuration file", + status: "pending", + }, + }), + { prompt, log: () => {} }, ); + expect(prompt).toHaveBeenCalledWith(undefined, "Modifying critical configuration file"); + expect(res).toEqual({ outcome: { outcome: "selected", optionId: "allow" } }); + }); - expect(source).toContain("30_000"); - expect(source).toContain("[permission timeout]"); + it("returns cancelled when no permission options are present", async () => { + const prompt = vi.fn(async () => true); + const res = await resolvePermissionRequest(makePermissionRequest({ options: [] }), { + prompt, + log: () => {}, + }); + expect(prompt).not.toHaveBeenCalled(); + expect(res).toEqual({ outcome: { outcome: "cancelled" } }); }); }); diff --git a/src/acp/client.ts b/src/acp/client.ts index bb6e2ac34b..f6d3aa274d 100644 --- a/src/acp/client.ts +++ b/src/acp/client.ts @@ -3,6 +3,7 @@ import { PROTOCOL_VERSION, ndJsonStream, type RequestPermissionRequest, + type RequestPermissionResponse, type SessionNotification, } from "@agentclientprotocol/sdk"; import { spawn, type ChildProcess } from "node:child_process"; @@ -28,30 +29,171 @@ const DANGEROUS_ACP_TOOLS = new Set([ "apply_patch", ]); -function promptUserPermission(toolName: string, toolTitle?: string): Promise { +type PermissionOption = RequestPermissionRequest["options"][number]; + +type PermissionResolverDeps = { + prompt?: (toolName: string | undefined, toolTitle?: string) => Promise; + log?: (line: string) => void; +}; + +function asRecord(value: unknown): Record | undefined { + return value && typeof value === "object" && !Array.isArray(value) + ? (value as Record) + : undefined; +} + +function readFirstStringValue( + source: Record | undefined, + keys: string[], +): string | undefined { + if (!source) { + return undefined; + } + for (const key of keys) { + const value = source[key]; + if (typeof value === "string" && value.trim()) { + return value.trim(); + } + } + return undefined; +} + +function normalizeToolName(value: string): string | undefined { + const normalized = value.trim().toLowerCase(); + if (!normalized) { + return undefined; + } + return normalized; +} + +function parseToolNameFromTitle(title: string | undefined | null): string | undefined { + if (!title) { + return undefined; + } + const head = title.split(":", 1)[0]?.trim(); + if (!head || !/^[a-zA-Z0-9._-]+$/.test(head)) { + return undefined; + } + return normalizeToolName(head); +} + +function resolveToolNameForPermission(params: RequestPermissionRequest): string | undefined { + const toolCall = params.toolCall; + const toolMeta = asRecord(toolCall?._meta); + const rawInput = asRecord(toolCall?.rawInput); + + const fromMeta = readFirstStringValue(toolMeta, ["toolName", "tool_name", "name"]); + const fromRawInput = readFirstStringValue(rawInput, ["tool", "toolName", "tool_name", "name"]); + const fromTitle = parseToolNameFromTitle(toolCall?.title); + return normalizeToolName(fromMeta ?? fromRawInput ?? fromTitle ?? ""); +} + +function pickOption( + options: PermissionOption[], + kinds: PermissionOption["kind"][], +): PermissionOption | undefined { + for (const kind of kinds) { + const match = options.find((option) => option.kind === kind); + if (match) { + return match; + } + } + return undefined; +} + +function selectedPermission(optionId: string): RequestPermissionResponse { + return { outcome: { outcome: "selected", optionId } }; +} + +function cancelledPermission(): RequestPermissionResponse { + return { outcome: { outcome: "cancelled" } }; +} + +function promptUserPermission(toolName: string | undefined, toolTitle?: string): Promise { + if (!process.stdin.isTTY || !process.stderr.isTTY) { + console.error(`[permission denied] ${toolName ?? "unknown"}: non-interactive terminal`); + return Promise.resolve(false); + } return new Promise((resolve) => { + let settled = false; const rl = readline.createInterface({ input: process.stdin, output: process.stderr, }); - const timeout = setTimeout(() => { - console.error(`\n[permission timeout] denied: ${toolName}`); - rl.close(); - resolve(false); - }, 30_000); - - const label = toolTitle ? `${toolTitle} (${toolName})` : toolName; - rl.question(`\n[permission] Allow "${label}"? (y/N) `, (answer) => { + const finish = (approved: boolean) => { + if (settled) { + return; + } + settled = true; clearTimeout(timeout); rl.close(); - const approved = answer.trim().toLowerCase() === "y"; - console.error(`[permission ${approved ? "approved" : "denied"}] ${toolName}`); resolve(approved); + }; + + const timeout = setTimeout(() => { + console.error(`\n[permission timeout] denied: ${toolName ?? "unknown"}`); + finish(false); + }, 30_000); + + const label = toolTitle + ? toolName + ? `${toolTitle} (${toolName})` + : toolTitle + : (toolName ?? "unknown tool"); + rl.question(`\n[permission] Allow "${label}"? (y/N) `, (answer) => { + const approved = answer.trim().toLowerCase() === "y"; + console.error(`[permission ${approved ? "approved" : "denied"}] ${toolName ?? "unknown"}`); + finish(approved); }); }); } +export async function resolvePermissionRequest( + params: RequestPermissionRequest, + deps: PermissionResolverDeps = {}, +): Promise { + const log = deps.log ?? ((line: string) => console.error(line)); + const prompt = deps.prompt ?? promptUserPermission; + const options = params.options ?? []; + const toolTitle = params.toolCall?.title ?? "tool"; + const toolName = resolveToolNameForPermission(params); + + if (options.length === 0) { + log(`[permission cancelled] ${toolName ?? "unknown"}: no options available`); + return cancelledPermission(); + } + + const allowOption = pickOption(options, ["allow_once", "allow_always"]); + const rejectOption = pickOption(options, ["reject_once", "reject_always"]); + const promptRequired = !toolName || DANGEROUS_ACP_TOOLS.has(toolName); + + if (!promptRequired) { + const option = allowOption ?? options[0]; + if (!option) { + log(`[permission cancelled] ${toolName}: no selectable options`); + return cancelledPermission(); + } + log(`[permission auto-approved] ${toolName}`); + return selectedPermission(option.optionId); + } + + log(`\n[permission requested] ${toolTitle}${toolName ? ` (${toolName})` : ""}`); + const approved = await prompt(toolName, toolTitle); + + if (approved && allowOption) { + return selectedPermission(allowOption.optionId); + } + if (!approved && rejectOption) { + return selectedPermission(rejectOption.optionId); + } + + log( + `[permission cancelled] ${toolName ?? "unknown"}: missing ${approved ? "allow" : "reject"} option`, + ); + return cancelledPermission(); +} + export type AcpClientOptions = { cwd?: string; serverCommand?: string; @@ -146,42 +288,7 @@ export async function createAcpClient(opts: AcpClientOptions = {}): Promise { - // toolCall may include a `name` field not in the SDK type - const toolCall = params.toolCall as Record | undefined; - const toolName = (typeof toolCall?.name === "string" ? toolCall.name : "") as string; - const toolTitle = (params.toolCall?.title ?? "tool") as string; - const options = params.options ?? []; - const allowOnce = options.find((o) => o.kind === "allow_once"); - const rejectOption = options.find((o) => o.kind === "reject_once"); - - // No options available — deny by default (fixes empty-options exploit) - if (options.length === 0) { - console.error(`[permission denied] ${toolName}: no options available`); - return { outcome: { outcome: "selected", optionId: "deny" } }; - } - - // Safe tools: auto-approve (backward compatible) - if (!DANGEROUS_ACP_TOOLS.has(toolName)) { - console.error(`[permission auto-approved] ${toolName}`); - return { - outcome: { - outcome: "selected", - optionId: allowOnce?.optionId ?? options[0]?.optionId ?? "allow", - }, - }; - } - - // Dangerous tools: require interactive confirmation - console.error(`\n[permission requested] ${toolTitle} (${toolName})`); - const approved = await promptUserPermission(toolName, toolTitle); - - if (approved && allowOnce) { - return { outcome: { outcome: "selected", optionId: allowOnce.optionId } }; - } - - // Denied — use reject option if available, otherwise reject - const rejectId = rejectOption?.optionId ?? "deny"; - return { outcome: { outcome: "selected", optionId: rejectId } }; + return resolvePermissionRequest(params); }, }), stream, diff --git a/src/config/config.gateway-tools-config.test.ts b/src/config/config.gateway-tools-config.test.ts new file mode 100644 index 0000000000..87531ffc42 --- /dev/null +++ b/src/config/config.gateway-tools-config.test.ts @@ -0,0 +1,33 @@ +import { describe, expect, it, vi } from "vitest"; + +describe("gateway.tools config", () => { + it("accepts gateway.tools allow and deny lists", async () => { + vi.resetModules(); + const { validateConfigObject } = await import("./config.js"); + const res = validateConfigObject({ + gateway: { + tools: { + allow: ["gateway"], + deny: ["sessions_spawn", "sessions_send"], + }, + }, + }); + expect(res.ok).toBe(true); + }); + + it("rejects invalid gateway.tools values", async () => { + vi.resetModules(); + const { validateConfigObject } = await import("./config.js"); + const res = validateConfigObject({ + gateway: { + tools: { + allow: "gateway", + }, + }, + }); + expect(res.ok).toBe(false); + if (!res.ok) { + expect(res.issues[0]?.path).toBe("gateway.tools.allow"); + } + }); +}); diff --git a/src/config/zod-schema.ts b/src/config/zod-schema.ts index 982ab7fa88..da1b76dc77 100644 --- a/src/config/zod-schema.ts +++ b/src/config/zod-schema.ts @@ -404,6 +404,13 @@ export const OpenClawSchema = z .strict() .optional(), trustedProxies: z.array(z.string()).optional(), + tools: z + .object({ + deny: z.array(z.string()).optional(), + allow: z.array(z.string()).optional(), + }) + .strict() + .optional(), tailscale: z .object({ mode: z.union([z.literal("off"), z.literal("serve"), z.literal("funnel")]).optional(), diff --git a/src/gateway/tools-invoke-http.test.ts b/src/gateway/tools-invoke-http.test.ts index a71b19a6b0..b7d2cbd37c 100644 --- a/src/gateway/tools-invoke-http.test.ts +++ b/src/gateway/tools-invoke-http.test.ts @@ -233,6 +233,7 @@ describe("POST /tools/invoke", () => { tools: { allow: ["sessions_spawn"] }, }, ], + // oxlint-disable-next-line typescript/no-explicit-any } as any; const port = await getFreePort(); @@ -256,6 +257,7 @@ describe("POST /tools/invoke", () => { it("denies sessions_send via HTTP gateway", async () => { testState.agentsConfig = { list: [{ id: "main", tools: { allow: ["sessions_send"] } }], + // oxlint-disable-next-line typescript/no-explicit-any } as any; const port = await getFreePort(); @@ -275,6 +277,7 @@ describe("POST /tools/invoke", () => { it("denies gateway tool via HTTP", async () => { testState.agentsConfig = { list: [{ id: "main", tools: { allow: ["gateway"] } }], + // oxlint-disable-next-line typescript/no-explicit-any } as any; const port = await getFreePort(); diff --git a/src/gateway/tools-invoke-http.ts b/src/gateway/tools-invoke-http.ts index a9d6c74f46..0962a6e411 100644 --- a/src/gateway/tools-invoke-http.ts +++ b/src/gateway/tools-invoke-http.ts @@ -315,9 +315,9 @@ export async function handleToolsInvokeHttpRequest( // Gateway HTTP-specific deny list — applies to ALL sessions via HTTP. const gatewayToolsCfg = cfg.gateway?.tools; - const gatewayDenyNames = DEFAULT_GATEWAY_HTTP_TOOL_DENY - .filter((name) => !gatewayToolsCfg?.allow?.includes(name)) - .concat(Array.isArray(gatewayToolsCfg?.deny) ? gatewayToolsCfg.deny : []); + const gatewayDenyNames = DEFAULT_GATEWAY_HTTP_TOOL_DENY.filter( + (name) => !gatewayToolsCfg?.allow?.includes(name), + ).concat(Array.isArray(gatewayToolsCfg?.deny) ? gatewayToolsCfg.deny : []); const gatewayDenySet = new Set(gatewayDenyNames); const gatewayFiltered = subagentFiltered.filter((t) => !gatewayDenySet.has(t.name));