From 3512a4e138148a97873c96304c40563fd3cd40ae Mon Sep 17 00:00:00 2001 From: Gustavo Madeira Santana Date: Wed, 1 Apr 2026 19:02:10 -0400 Subject: [PATCH] exec: align approval UX with host policy --- docs/cli/approvals.md | 12 ++ docs/tools/exec-approvals.md | 1 + docs/tools/slash-commands.md | 2 +- .../telegram/src/exec-approval-forwarding.ts | 2 + .../src/exec-approvals-handler.test.ts | 44 ++++ .../telegram/src/exec-approvals-handler.ts | 4 + src/agents/bash-tools.exec-host-gateway.ts | 2 + src/agents/bash-tools.exec-host-node.ts | 2 + src/agents/bash-tools.exec-host-shared.ts | 6 + src/agents/bash-tools.exec-runtime.ts | 16 +- src/agents/bash-tools.exec-types.ts | 2 + .../bash-tools.exec.approval-id.test.ts | 22 +- ...-embedded-subscribe.handlers.tools.test.ts | 40 ++++ .../pi-embedded-subscribe.handlers.tools.ts | 9 + src/agents/system-prompt.test.ts | 1 + src/agents/system-prompt.ts | 2 +- src/auto-reply/reply/commands-approve.ts | 9 +- src/cli/exec-approvals-cli.test.ts | 132 +++++++++++- src/cli/exec-approvals-cli.ts | 132 +++++++++++- src/commands/doctor-security.test.ts | 40 ++++ src/commands/doctor-security.ts | 72 ++----- src/gateway/server-methods/exec-approval.ts | 18 ++ .../server-methods/server-methods.test.ts | 50 ++++- src/infra/exec-approval-forwarder.test.ts | 19 ++ src/infra/exec-approval-forwarder.ts | 18 +- src/infra/exec-approval-reply.test.ts | 40 ++++ src/infra/exec-approval-reply.ts | 69 ++++-- src/infra/exec-approvals-effective.ts | 196 ++++++++++++++++++ src/infra/exec-approvals-policy.test.ts | 102 +++++++++ src/infra/exec-approvals.ts | 22 ++ src/plugin-sdk/approval-runtime.ts | 1 + 31 files changed, 992 insertions(+), 95 deletions(-) create mode 100644 src/infra/exec-approvals-effective.ts diff --git a/docs/cli/approvals.md b/docs/cli/approvals.md index 4a6da45429d..f4499a17b74 100644 --- a/docs/cli/approvals.md +++ b/docs/cli/approvals.md @@ -24,6 +24,18 @@ openclaw approvals get --node openclaw approvals get --gateway ``` +`openclaw approvals get` now shows the effective exec policy for local and gateway targets: + +- requested `tools.exec` policy +- host approvals-file policy +- effective result after precedence rules are applied + +Precedence is intentional: + +- the host approvals file is the enforceable source of truth +- requested `tools.exec` policy can narrow or broaden intent, but the effective result is still derived from the host rules +- node output stays host-file-only because gateway `tools.exec` policy is applied later at runtime + ## Replace approvals from a file ```bash diff --git a/docs/tools/exec-approvals.md b/docs/tools/exec-approvals.md index 9d8f61e0059..c42ebe920ce 100644 --- a/docs/tools/exec-approvals.md +++ b/docs/tools/exec-approvals.md @@ -17,6 +17,7 @@ Effective policy is the **stricter** of `tools.exec.*` and approvals defaults; i Host exec also uses the local approvals state on that machine. A host-local `ask: "always"` in `~/.openclaw/exec-approvals.json` keeps prompting even if session or config defaults request `ask: "on-miss"`. +Use `openclaw approvals get` or `openclaw approvals get --gateway` to inspect the requested policy, host policy sources, and the effective result. If the companion app UI is **not available**, any request that requires a prompt is resolved by the **ask fallback** (default: deny). diff --git a/docs/tools/slash-commands.md b/docs/tools/slash-commands.md index da35723233f..595044cd110 100644 --- a/docs/tools/slash-commands.md +++ b/docs/tools/slash-commands.md @@ -80,7 +80,7 @@ Text + native (when enabled): - `/status` (show current status; includes provider usage/quota for the current model provider when available) - `/tasks` (list background tasks for the current session; shows active and recent task details with agent-local fallback counts) - `/allowlist` (list/add/remove allowlist entries) -- `/approve allow-once|allow-always|deny` (resolve exec approval prompts) +- `/approve ` (resolve exec approval prompts; use the pending approval message for the available decisions) - `/context [list|detail|json]` (explain “context”; `detail` shows per-file + per-tool + per-skill + system prompt size) - `/btw ` (ask an ephemeral side question about the current session without changing future session context; see [/tools/btw](/tools/btw)) - `/export-session [path]` (alias: `/export`) (export current session to HTML with full system prompt) diff --git a/extensions/telegram/src/exec-approval-forwarding.ts b/extensions/telegram/src/exec-approval-forwarding.ts index 60a5893a3a8..8d2ea4745d8 100644 --- a/extensions/telegram/src/exec-approval-forwarding.ts +++ b/extensions/telegram/src/exec-approval-forwarding.ts @@ -1,5 +1,6 @@ import { buildExecApprovalPendingReplyPayload, + resolveExecApprovalAllowedDecisions, resolveExecApprovalCommandDisplay, type ExecApprovalRequest, } from "openclaw/plugin-sdk/approval-runtime"; @@ -37,6 +38,7 @@ export function buildTelegramExecApprovalPendingPayload(params: { cwd: params.request.request.cwd ?? undefined, host: params.request.request.host === "node" ? "node" : "gateway", nodeId: params.request.request.nodeId ?? undefined, + allowedDecisions: resolveExecApprovalAllowedDecisions({ ask: params.request.request.ask }), expiresAtMs: params.request.expiresAtMs, nowMs: params.nowMs, }); diff --git a/extensions/telegram/src/exec-approvals-handler.test.ts b/extensions/telegram/src/exec-approvals-handler.test.ts index fb2408a5e33..438e84af3d8 100644 --- a/extensions/telegram/src/exec-approvals-handler.test.ts +++ b/extensions/telegram/src/exec-approvals-handler.test.ts @@ -114,6 +114,50 @@ describe("TelegramExecApprovalHandler", () => { ); }); + it("hides allow-always actions when ask=always", async () => { + const cfg = { + channels: { + telegram: { + execApprovals: { + enabled: true, + approvers: ["8460800771"], + target: "channel", + }, + }, + }, + } as OpenClawConfig; + const { handler, sendMessage } = createHandler(cfg); + + await handler.handleRequested({ + ...baseRequest, + request: { + ...baseRequest.request, + ask: "always", + }, + }); + + expect(sendMessage).toHaveBeenCalledWith( + "-1003841603622", + expect.not.stringContaining("allow-always"), + expect.objectContaining({ + buttons: [ + [ + { + text: "Allow Once", + callback_data: "/approve 9f1c7d5d-b1fb-46ef-ac45-662723b65bb7 allow-once", + style: "success", + }, + { + text: "Deny", + callback_data: "/approve 9f1c7d5d-b1fb-46ef-ac45-662723b65bb7 deny", + style: "danger", + }, + ], + ], + }), + ); + }); + it("falls back to approver DMs when channel routing is unavailable", async () => { const cfg = { channels: { diff --git a/extensions/telegram/src/exec-approvals-handler.ts b/extensions/telegram/src/exec-approvals-handler.ts index 8c28c18e30a..ce9fbd2a43e 100644 --- a/extensions/telegram/src/exec-approvals-handler.ts +++ b/extensions/telegram/src/exec-approvals-handler.ts @@ -2,6 +2,7 @@ import { buildPluginApprovalPendingReplyPayload } from "openclaw/plugin-sdk/appr import type { OpenClawConfig } from "openclaw/plugin-sdk/config-runtime"; import { createChannelNativeApprovalRuntime, + resolveExecApprovalAllowedDecisions, type ExecApprovalChannelRuntime, } from "openclaw/plugin-sdk/infra-runtime"; import { resolveExecApprovalCommandDisplay } from "openclaw/plugin-sdk/infra-runtime"; @@ -124,6 +125,9 @@ export class TelegramExecApprovalHandler { cwd: (request as ExecApprovalRequest).request.cwd ?? undefined, host: (request as ExecApprovalRequest).request.host === "node" ? "node" : "gateway", nodeId: (request as ExecApprovalRequest).request.nodeId ?? undefined, + allowedDecisions: resolveExecApprovalAllowedDecisions({ + ask: (request as ExecApprovalRequest).request.ask, + }), expiresAtMs: request.expiresAtMs, nowMs, } satisfies ExecApprovalPendingReplyParams); diff --git a/src/agents/bash-tools.exec-host-gateway.ts b/src/agents/bash-tools.exec-host-gateway.ts index caeb1ac7dd8..05420887b8d 100644 --- a/src/agents/bash-tools.exec-host-gateway.ts +++ b/src/agents/bash-tools.exec-host-gateway.ts @@ -3,6 +3,7 @@ import { addDurableCommandApproval, addAllowlistEntry, type ExecAsk, + resolveExecApprovalAllowedDecisions, type ExecSecurity, buildEnforcedShellCommand, evaluateShellAllowlist, @@ -410,6 +411,7 @@ export async function processGatewayAllowlist( initiatingSurface, sentApproverDms, unavailableReason, + allowedDecisions: resolveExecApprovalAllowedDecisions({ ask: hostAsk }), }), }; } diff --git a/src/agents/bash-tools.exec-host-node.ts b/src/agents/bash-tools.exec-host-node.ts index 8b29109ae01..3cc56b5c6c9 100644 --- a/src/agents/bash-tools.exec-host-node.ts +++ b/src/agents/bash-tools.exec-host-node.ts @@ -7,6 +7,7 @@ import { evaluateShellAllowlist, hasDurableExecApproval, requiresExecApproval, + resolveExecApprovalAllowedDecisions, resolveExecApprovalsFromFile, } from "../infra/exec-approvals.js"; import { @@ -415,6 +416,7 @@ export async function executeNodeHostCommand( initiatingSurface, sentApproverDms, unavailableReason, + allowedDecisions: resolveExecApprovalAllowedDecisions({ ask: hostAsk }), nodeId, }); } diff --git a/src/agents/bash-tools.exec-host-shared.ts b/src/agents/bash-tools.exec-host-shared.ts index 0187cda95d2..f5ecd70012c 100644 --- a/src/agents/bash-tools.exec-host-shared.ts +++ b/src/agents/bash-tools.exec-host-shared.ts @@ -10,8 +10,10 @@ import { import { maxAsk, minSecurity, + resolveExecApprovalAllowedDecisions, resolveExecApprovals, type ExecAsk, + type ExecApprovalDecision, type ExecSecurity, } from "../infra/exec-approvals.js"; import { logWarn } from "../logger.js"; @@ -409,8 +411,10 @@ export function buildExecApprovalPendingToolResult(params: { initiatingSurface: ExecApprovalInitiatingSurfaceState; sentApproverDms: boolean; unavailableReason: ExecApprovalUnavailableReason | null; + allowedDecisions?: readonly ExecApprovalDecision[]; nodeId?: string; }): AgentToolResult { + const allowedDecisions = params.allowedDecisions ?? resolveExecApprovalAllowedDecisions(); return { content: [ { @@ -427,6 +431,7 @@ export function buildExecApprovalPendingToolResult(params: { warningText: params.warningText, approvalSlug: params.approvalSlug, approvalId: params.approvalId, + allowedDecisions, command: params.command, cwd: params.cwd, host: params.host, @@ -452,6 +457,7 @@ export function buildExecApprovalPendingToolResult(params: { approvalId: params.approvalId, approvalSlug: params.approvalSlug, expiresAtMs: params.expiresAtMs, + allowedDecisions, host: params.host, command: params.command, cwd: params.cwd, diff --git a/src/agents/bash-tools.exec-runtime.ts b/src/agents/bash-tools.exec-runtime.ts index 08ae6277e84..5a4a08e3bbb 100644 --- a/src/agents/bash-tools.exec-runtime.ts +++ b/src/agents/bash-tools.exec-runtime.ts @@ -3,7 +3,9 @@ import type { AgentToolResult } from "@mariozechner/pi-agent-core"; import { Type } from "@sinclair/typebox"; import { DEFAULT_EXEC_APPROVAL_TIMEOUT_MS, + resolveExecApprovalAllowedDecisions, type ExecHost, + type ExecApprovalDecision, type ExecTarget, } from "../infra/exec-approvals.js"; import { requestHeartbeatNow } from "../infra/heartbeat-wake.js"; @@ -336,6 +338,7 @@ export function buildApprovalPendingMessage(params: { warningText?: string; approvalSlug: string; approvalId: string; + allowedDecisions?: readonly ExecApprovalDecision[]; command: string; cwd: string; host: "gateway" | "node"; @@ -347,6 +350,8 @@ export function buildApprovalPendingMessage(params: { } const commandBlock = `${fence}sh\n${params.command}\n${fence}`; const lines: string[] = []; + const allowedDecisions = params.allowedDecisions ?? resolveExecApprovalAllowedDecisions(); + const decisionText = allowedDecisions.join("|"); const warningText = params.warningText?.trim(); if (warningText) { lines.push(warningText, ""); @@ -360,8 +365,15 @@ export function buildApprovalPendingMessage(params: { lines.push("Command:"); lines.push(commandBlock); lines.push("Mode: foreground (interactive approvals available)."); - lines.push("Background mode requires pre-approved policy (allow-always or ask=off)."); - lines.push(`Reply with: /approve ${params.approvalSlug} allow-once|allow-always|deny`); + lines.push( + allowedDecisions.includes("allow-always") + ? "Background mode requires pre-approved policy (allow-always or ask=off)." + : "Background mode requires host policy that allows pre-approval (for example ask=off).", + ); + lines.push(`Reply with: /approve ${params.approvalSlug} ${decisionText}`); + if (!allowedDecisions.includes("allow-always")) { + lines.push("Host policy requires approval every time, so Allow Always is unavailable."); + } lines.push("If the short code is ambiguous, use the full id in /approve."); return lines.join("\n"); } diff --git a/src/agents/bash-tools.exec-types.ts b/src/agents/bash-tools.exec-types.ts index a23ab24d367..c542d5ef67d 100644 --- a/src/agents/bash-tools.exec-types.ts +++ b/src/agents/bash-tools.exec-types.ts @@ -1,3 +1,4 @@ +import type { ExecApprovalDecision } from "../infra/exec-approvals.js"; import type { ExecAsk, ExecHost, ExecSecurity, ExecTarget } from "../infra/exec-approvals.js"; import type { SafeBinProfileFixture } from "../infra/exec-safe-bin-policy.js"; import type { BashSandboxConfig } from "./bash-tools.shared.js"; @@ -58,6 +59,7 @@ export type ExecToolDetails = approvalId: string; approvalSlug: string; expiresAtMs: number; + allowedDecisions?: readonly ExecApprovalDecision[]; host: ExecHost; command: string; cwd?: string; diff --git a/src/agents/bash-tools.exec.approval-id.test.ts b/src/agents/bash-tools.exec.approval-id.test.ts index 074fc52f3f1..ddfc8fa27d9 100644 --- a/src/agents/bash-tools.exec.approval-id.test.ts +++ b/src/agents/bash-tools.exec.approval-id.test.ts @@ -93,13 +93,14 @@ function expectPendingApprovalText( host: "gateway" | "node"; nodeId?: string; interactive?: boolean; + allowedDecisions?: string; }, ) { expect(result.details.status).toBe("approval-pending"); const details = result.details as { approvalId: string; approvalSlug: string }; const pendingText = getResultText(result); expect(pendingText).toContain( - `Reply with: /approve ${details.approvalSlug} allow-once|allow-always|deny`, + `Reply with: /approve ${details.approvalSlug} ${options.allowedDecisions ?? "allow-once|allow-always|deny"}`, ); expect(pendingText).toContain(`full ${details.approvalId}`); expect(pendingText).toContain(`Host: ${options.host}`); @@ -111,7 +112,11 @@ function expectPendingApprovalText( expect(pendingText).toContain(options.command); if (options.interactive) { expect(pendingText).toContain("Mode: foreground (interactive approvals available)."); - expect(pendingText).toContain("Background mode requires pre-approved policy"); + expect(pendingText).toContain( + (options.allowedDecisions ?? "").includes("allow-always") + ? "Background mode requires pre-approved policy" + : "Background mode requires host policy that allows pre-approval", + ); } return details; } @@ -277,6 +282,7 @@ describe("exec approvals", () => { host: "node", nodeId: "node-1", interactive: true, + allowedDecisions: "allow-once|deny", }); const approvalId = details.approvalId; @@ -460,6 +466,13 @@ describe("exec approvals", () => { }); expect(result.details.status).toBe("approval-pending"); + expect(result.details).toMatchObject({ + allowedDecisions: ["allow-once", "deny"], + }); + expect(getResultText(result)).toContain("Reply with: /approve "); + expect(getResultText(result)).toContain("allow-once|deny"); + expect(getResultText(result)).not.toContain("allow-once|allow-always|deny"); + expect(getResultText(result)).toContain("Allow Always is unavailable"); }); it("keeps ask=always prompts for static allowlist entries without allow-always trust", async () => { @@ -528,6 +541,9 @@ describe("exec approvals", () => { }); expect(result.details.status).toBe("approval-pending"); + expect(result.details).toMatchObject({ + allowedDecisions: ["allow-once", "deny"], + }); expect(calls).toContain("exec.approval.request"); }); @@ -1240,6 +1256,7 @@ describe("exec approvals", () => { expectPendingApprovalText(result, { command: "npm view diver name version description", host: "gateway", + allowedDecisions: "allow-once|deny", }); }); @@ -1278,6 +1295,7 @@ describe("exec approvals", () => { const details = expectPendingApprovalText(result, { command: "npm view diver name version description", host: "gateway", + allowedDecisions: "allow-once|deny", }); expect(getResultText(result)).toContain(`/approve ${details.approvalSlug} allow-once`); expect(getResultText(result)).not.toContain(getExecApprovalApproverDmNoticeText()); diff --git a/src/agents/pi-embedded-subscribe.handlers.tools.test.ts b/src/agents/pi-embedded-subscribe.handlers.tools.test.ts index 86eb5fab649..5c07d297e17 100644 --- a/src/agents/pi-embedded-subscribe.handlers.tools.test.ts +++ b/src/agents/pi-embedded-subscribe.handlers.tools.test.ts @@ -281,6 +281,46 @@ describe("handleToolExecutionEnd exec approval prompts", () => { expect(ctx.state.deterministicApprovalPromptSent).toBe(true); }); + it("preserves filtered approval decisions from tool details", async () => { + const { ctx } = createTestContext(); + const onToolResult = vi.fn(); + ctx.params.onToolResult = onToolResult; + + await handleToolExecutionEnd( + ctx as never, + { + type: "tool_execution_end", + toolName: "exec", + toolCallId: "tool-exec-approval-ask-always", + isError: false, + result: { + details: { + status: "approval-pending", + approvalId: "12345678-1234-1234-1234-123456789012", + approvalSlug: "12345678", + expiresAtMs: 1_800_000_000_000, + allowedDecisions: ["allow-once", "deny"], + host: "gateway", + command: "npm view diver name version description", + }, + }, + } as never, + ); + + expect(onToolResult).toHaveBeenCalledWith( + expect.objectContaining({ + text: expect.not.stringContaining("allow-always"), + channelData: { + execApproval: { + approvalId: "12345678-1234-1234-1234-123456789012", + approvalSlug: "12345678", + allowedDecisions: ["allow-once", "deny"], + }, + }, + }), + ); + }); + it("emits a deterministic unavailable payload when the initiating surface cannot approve", async () => { const { ctx } = createTestContext(); const onToolResult = vi.fn(); diff --git a/src/agents/pi-embedded-subscribe.handlers.tools.ts b/src/agents/pi-embedded-subscribe.handlers.tools.ts index ad55fd02723..3a1b6fa4b86 100644 --- a/src/agents/pi-embedded-subscribe.handlers.tools.ts +++ b/src/agents/pi-embedded-subscribe.handlers.tools.ts @@ -4,6 +4,7 @@ import { buildExecApprovalPendingReplyPayload, buildExecApprovalUnavailableReplyPayload, } from "../infra/exec-approval-reply.js"; +import type { ExecApprovalDecision } from "../infra/exec-approvals.js"; import { getGlobalHookRunner } from "../plugins/hook-runner-global.js"; import type { PluginHookAfterToolCallEvent } from "../plugins/types.js"; import { normalizeTextForComparison } from "./pi-embedded-helpers.js"; @@ -165,6 +166,7 @@ function readExecApprovalPendingDetails(result: unknown): { approvalId: string; approvalSlug: string; expiresAtMs?: number; + allowedDecisions?: readonly ExecApprovalDecision[]; host: "gateway" | "node"; command: string; cwd?: string; @@ -193,6 +195,12 @@ function readExecApprovalPendingDetails(result: unknown): { approvalId, approvalSlug, expiresAtMs: typeof details.expiresAtMs === "number" ? details.expiresAtMs : undefined, + allowedDecisions: Array.isArray(details.allowedDecisions) + ? details.allowedDecisions.filter( + (decision): decision is ExecApprovalDecision => + decision === "allow-once" || decision === "allow-always" || decision === "deny", + ) + : undefined, host, command, cwd: typeof details.cwd === "string" ? details.cwd : undefined, @@ -263,6 +271,7 @@ async function emitToolResultOutput(params: { buildExecApprovalPendingReplyPayload({ approvalId: approvalPending.approvalId, approvalSlug: approvalPending.approvalSlug, + allowedDecisions: approvalPending.allowedDecisions, command: approvalPending.command, cwd: approvalPending.cwd, host: approvalPending.host, diff --git a/src/agents/system-prompt.test.ts b/src/agents/system-prompt.test.ts index dd6e2d9c6d6..d99d78a01ff 100644 --- a/src/agents/system-prompt.test.ts +++ b/src/agents/system-prompt.test.ts @@ -168,6 +168,7 @@ describe("buildAgentSystemPrompt", () => { expect(prompt).toContain( "When exec returns approval-pending, include the concrete /approve command from tool output", ); + expect(prompt).not.toContain("allow-once|allow-always|deny"); }); it("tells native approval channels not to duplicate plain chat /approve instructions", () => { diff --git a/src/agents/system-prompt.ts b/src/agents/system-prompt.ts index 5a0ef6f8718..90616ed241e 100644 --- a/src/agents/system-prompt.ts +++ b/src/agents/system-prompt.ts @@ -183,7 +183,7 @@ function buildExecApprovalPromptGuidance(params: { runtimeChannel?: string }) { ) { return "When exec returns approval-pending on Discord, Slack, Telegram, or WebChat, rely on the native approval card/buttons when they appear and do not also send plain chat /approve instructions. Only include the concrete /approve command if the tool result says chat approvals are unavailable or only manual approval is possible."; } - return "When exec returns approval-pending, include the concrete /approve command from tool output (with allow-once|allow-always|deny) as plain chat text for the user, and do not ask for a different or rotated code."; + return "When exec returns approval-pending, include the concrete /approve command from tool output as plain chat text for the user, and do not ask for a different or rotated code."; } export function buildAgentSystemPrompt(params: { diff --git a/src/auto-reply/reply/commands-approve.ts b/src/auto-reply/reply/commands-approve.ts index 2fcadce4476..0540109a064 100644 --- a/src/auto-reply/reply/commands-approve.ts +++ b/src/auto-reply/reply/commands-approve.ts @@ -30,6 +30,9 @@ type ParsedApproveCommand = | { ok: true; id: string; decision: "allow-once" | "allow-always" | "deny" } | { ok: false; error: string }; +const APPROVE_USAGE_TEXT = + "Usage: /approve (see the pending approval message for available decisions)"; + function parseApproveCommand(raw: string): ParsedApproveCommand | null { const trimmed = raw.trim(); if (FOREIGN_COMMAND_MENTION_REGEX.test(trimmed)) { @@ -41,11 +44,11 @@ function parseApproveCommand(raw: string): ParsedApproveCommand | null { } const rest = trimmed.slice(commandMatch[0].length).trim(); if (!rest) { - return { ok: false, error: "Usage: /approve allow-once|allow-always|deny" }; + return { ok: false, error: APPROVE_USAGE_TEXT }; } const tokens = rest.split(/\s+/).filter(Boolean); if (tokens.length < 2) { - return { ok: false, error: "Usage: /approve allow-once|allow-always|deny" }; + return { ok: false, error: APPROVE_USAGE_TEXT }; } const first = tokens[0].toLowerCase(); @@ -65,7 +68,7 @@ function parseApproveCommand(raw: string): ParsedApproveCommand | null { id: tokens[0], }; } - return { ok: false, error: "Usage: /approve allow-once|allow-always|deny" }; + return { ok: false, error: APPROVE_USAGE_TEXT }; } function buildResolvedByLabel(params: Parameters[0]): string { diff --git a/src/cli/exec-approvals-cli.test.ts b/src/cli/exec-approvals-cli.test.ts index 235a3cc00f0..4f503c0bf2a 100644 --- a/src/cli/exec-approvals-cli.test.ts +++ b/src/cli/exec-approvals-cli.test.ts @@ -1,11 +1,13 @@ import { Command } from "commander"; import { beforeEach, describe, expect, it, vi } from "vitest"; import * as execApprovals from "../infra/exec-approvals.js"; +import type { ExecApprovalsFile } from "../infra/exec-approvals.js"; import { registerExecApprovalsCli } from "./exec-approvals-cli.js"; const mocks = vi.hoisted(() => { const runtimeErrors: string[] = []; const stringifyArgs = (args: unknown[]) => args.map((value) => String(value)).join(" "); + const readBestEffortConfig = vi.fn(async () => ({})); const defaultRuntime = { log: vi.fn(), error: vi.fn((...args: unknown[]) => { @@ -24,6 +26,18 @@ const mocks = vi.hoisted(() => { return { callGatewayFromCli: vi.fn(async (method: string, _opts: unknown, params?: unknown) => { if (method.endsWith(".get")) { + if (method === "config.get") { + return { + config: { + tools: { + exec: { + security: "full", + ask: "off", + }, + }, + }, + }; + } return { path: "/tmp/exec-approvals.json", exists: true, @@ -34,18 +48,19 @@ const mocks = vi.hoisted(() => { return { method, params }; }), defaultRuntime, + readBestEffortConfig, runtimeErrors, }; }); -const { callGatewayFromCli, defaultRuntime, runtimeErrors } = mocks; +const { callGatewayFromCli, defaultRuntime, readBestEffortConfig, runtimeErrors } = mocks; const localSnapshot = { path: "/tmp/local-exec-approvals.json", exists: true, raw: "{}", hash: "hash-local", - file: { version: 1, agents: {} }, + file: { version: 1, agents: {} } as ExecApprovalsFile, }; function resetLocalSnapshot() { @@ -69,6 +84,14 @@ vi.mock("../runtime.js", () => ({ defaultRuntime: mocks.defaultRuntime, })); +vi.mock("../config/config.js", async () => { + const actual = await vi.importActual("../config/config.js"); + return { + ...actual, + readBestEffortConfig: mocks.readBestEffortConfig, + }; +}); + vi.mock("../infra/exec-approvals.js", async () => { const actual = await vi.importActual( "../infra/exec-approvals.js", @@ -97,6 +120,7 @@ describe("exec approvals CLI", () => { resetLocalSnapshot(); runtimeErrors.length = 0; callGatewayFromCli.mockClear(); + readBestEffortConfig.mockClear(); defaultRuntime.log.mockClear(); defaultRuntime.error.mockClear(); defaultRuntime.writeStdout.mockClear(); @@ -108,12 +132,19 @@ describe("exec approvals CLI", () => { await runApprovalsCommand(["approvals", "get"]); expect(callGatewayFromCli).not.toHaveBeenCalled(); + expect(readBestEffortConfig).toHaveBeenCalledTimes(1); expect(runtimeErrors).toHaveLength(0); callGatewayFromCli.mockClear(); await runApprovalsCommand(["approvals", "get", "--gateway"]); - expect(callGatewayFromCli).toHaveBeenCalledWith("exec.approvals.get", expect.anything(), {}); + expect(callGatewayFromCli).toHaveBeenNthCalledWith( + 1, + "exec.approvals.get", + expect.anything(), + {}, + ); + expect(callGatewayFromCli).toHaveBeenNthCalledWith(2, "config.get", expect.anything(), {}); expect(runtimeErrors).toHaveLength(0); callGatewayFromCli.mockClear(); @@ -125,6 +156,101 @@ describe("exec approvals CLI", () => { expect(runtimeErrors).toHaveLength(0); }); + it("adds effective policy to json output", async () => { + localSnapshot.file = { + version: 1, + defaults: { security: "allowlist", ask: "always", askFallback: "deny" }, + agents: {}, + }; + readBestEffortConfig.mockResolvedValue({ + tools: { + exec: { + security: "full", + ask: "off", + }, + }, + }); + + await runApprovalsCommand(["approvals", "get", "--json"]); + + expect(defaultRuntime.writeJson).toHaveBeenCalledWith( + expect.objectContaining({ + effectivePolicy: { + note: "Effective exec policy is the host approvals file intersected with requested tools.exec policy.", + scopes: [ + expect.objectContaining({ + scopeLabel: "tools.exec", + security: expect.objectContaining({ + requested: "full", + host: "allowlist", + effective: "allowlist", + }), + ask: expect.objectContaining({ + requested: "off", + host: "always", + effective: "always", + }), + }), + ], + }, + }), + 0, + ); + }); + + it("reports wildcard host policy sources in effective policy output", async () => { + localSnapshot.file = { + version: 1, + defaults: { security: "full", ask: "off", askFallback: "full" }, + agents: { + "*": { + security: "allowlist", + ask: "always", + askFallback: "deny", + }, + }, + }; + readBestEffortConfig.mockResolvedValue({ + agents: { + list: [ + { + id: "runner", + tools: { + exec: { + security: "full", + ask: "off", + }, + }, + }, + ], + }, + }); + + await runApprovalsCommand(["approvals", "get", "--json"]); + + expect(defaultRuntime.writeJson).toHaveBeenCalledWith( + expect.objectContaining({ + effectivePolicy: expect.objectContaining({ + scopes: expect.arrayContaining([ + expect.objectContaining({ + scopeLabel: "agent:runner", + security: expect.objectContaining({ + hostSource: "~/.openclaw/exec-approvals.json agents.*.security", + }), + ask: expect.objectContaining({ + hostSource: "~/.openclaw/exec-approvals.json agents.*.ask", + }), + askFallback: expect.objectContaining({ + source: "~/.openclaw/exec-approvals.json agents.*.askFallback", + }), + }), + ]), + }), + }), + 0, + ); + }); + it("defaults allowlist add to wildcard agent", async () => { const saveExecApprovals = vi.mocked(execApprovals.saveExecApprovals); saveExecApprovals.mockClear(); diff --git a/src/cli/exec-approvals-cli.ts b/src/cli/exec-approvals-cli.ts index 37f8b03bf95..2211ced116e 100644 --- a/src/cli/exec-approvals-cli.ts +++ b/src/cli/exec-approvals-cli.ts @@ -1,6 +1,8 @@ import fs from "node:fs/promises"; import type { Command } from "commander"; import JSON5 from "json5"; +import { readBestEffortConfig, type OpenClawConfig } from "../config/config.js"; +import { resolveExecPolicyScopeSummary } from "../infra/exec-approvals-effective.js"; import { readExecApprovalsSnapshot, saveExecApprovals, @@ -24,6 +26,15 @@ type ExecApprovalsSnapshot = { file: ExecApprovalsFile; }; +type ConfigSnapshotLike = { + config?: OpenClawConfig; +}; +type ApprovalsTargetSource = "gateway" | "node" | "local"; +type EffectivePolicyReport = { + scopes: ReturnType; + note?: string; +}; + type ExecApprovalsCliOpts = NodesRpcOpts & { node?: string; gateway?: boolean; @@ -79,7 +90,7 @@ function saveSnapshotLocal(file: ExecApprovalsFile): ExecApprovalsSnapshot { async function loadSnapshotTarget(opts: ExecApprovalsCliOpts): Promise<{ snapshot: ExecApprovalsSnapshot; nodeId: string | null; - source: "gateway" | "node" | "local"; + source: ApprovalsTargetSource; }> { if (!opts.gateway && !opts.node) { return { snapshot: loadSnapshotLocal(), nodeId: null, source: "local" }; @@ -106,7 +117,7 @@ function requireTrimmedNonEmpty(value: string, message: string): string { async function loadWritableSnapshotTarget(opts: ExecApprovalsCliOpts): Promise<{ snapshot: ExecApprovalsSnapshot; nodeId: string | null; - source: "gateway" | "node" | "local"; + source: ApprovalsTargetSource; targetLabel: string; baseHash: string; }> { @@ -124,7 +135,7 @@ async function loadWritableSnapshotTarget(opts: ExecApprovalsCliOpts): Promise<{ async function saveSnapshotTargeted(params: { opts: ExecApprovalsCliOpts; - source: "gateway" | "node" | "local"; + source: ApprovalsTargetSource; nodeId: string | null; file: ExecApprovalsFile; baseHash: string; @@ -147,6 +158,112 @@ function formatCliError(err: unknown): string { return msg.includes("\n") ? msg.split("\n")[0] : msg; } +async function loadConfigForApprovalsTarget(params: { + opts: ExecApprovalsCliOpts; + source: ApprovalsTargetSource; +}): Promise { + if (params.source === "node") { + return null; + } + if (params.source === "local") { + return await readBestEffortConfig(); + } + const snapshot = (await callGatewayFromCli("config.get", params.opts, {})) as ConfigSnapshotLike; + return snapshot.config && typeof snapshot.config === "object" ? snapshot.config : null; +} + +function collectExecPolicySummaries(params: { cfg: OpenClawConfig; approvals: ExecApprovalsFile }) { + const summaries = [ + resolveExecPolicyScopeSummary({ + approvals: params.approvals, + execConfig: params.cfg.tools?.exec, + configPath: "tools.exec", + scopeLabel: "tools.exec", + }), + ]; + const configAgentIds = new Set((params.cfg.agents?.list ?? []).map((agent) => agent.id)); + const approvalAgentIds = Object.keys(params.approvals.agents ?? {}).filter( + (agentId) => agentId !== "*" && agentId !== "default", + ); + const agentIds = Array.from(new Set([...configAgentIds, ...approvalAgentIds])).toSorted(); + for (const agentId of agentIds) { + const agentConfig = params.cfg.agents?.list?.find((agent) => agent.id === agentId); + summaries.push( + resolveExecPolicyScopeSummary({ + approvals: params.approvals, + execConfig: agentConfig?.tools?.exec, + configPath: `agents.list.${agentId}.tools.exec`, + scopeLabel: `agent:${agentId}`, + agentId, + }), + ); + } + return summaries; +} + +function buildEffectivePolicyReport(params: { + cfg: OpenClawConfig | null; + source: ApprovalsTargetSource; + approvals: ExecApprovalsFile; +}): EffectivePolicyReport { + if (params.source === "node") { + return { + scopes: [], + note: "Node output shows host approvals state only. Gateway tools.exec policy still intersects at runtime.", + }; + } + if (!params.cfg) { + return { + scopes: [], + note: "Config unavailable.", + }; + } + return { + scopes: collectExecPolicySummaries({ + cfg: params.cfg, + approvals: params.approvals, + }), + note: "Effective exec policy is the host approvals file intersected with requested tools.exec policy.", + }; +} + +function renderEffectivePolicy(params: { report: EffectivePolicyReport }) { + const rich = isRich(); + const heading = (text: string) => (rich ? theme.heading(text) : text); + const muted = (text: string) => (rich ? theme.muted(text) : text); + if (params.report.scopes.length === 0 && !params.report.note) { + return; + } + defaultRuntime.log(""); + defaultRuntime.log(heading("Effective Policy")); + if (params.report.scopes.length === 0) { + defaultRuntime.log(muted(params.report.note ?? "No effective policy details available.")); + return; + } + const rows = params.report.scopes.map((summary) => ({ + Scope: summary.scopeLabel, + Requested: `security=${summary.security.requested} (${summary.security.requestedSource})\nask=${summary.ask.requested} (${summary.ask.requestedSource})`, + Host: `security=${summary.security.host} (${summary.security.hostSource})\nask=${summary.ask.host} (${summary.ask.hostSource})\naskFallback=${summary.askFallback.effective} (${summary.askFallback.source})`, + Effective: `security=${summary.security.effective}\nask=${summary.ask.effective}`, + Notes: `${summary.security.note}; ${summary.ask.note}`, + })); + defaultRuntime.log( + renderTable({ + width: getTerminalTableWidth(), + columns: [ + { key: "Scope", header: "Scope", minWidth: 12 }, + { key: "Requested", header: "Requested", minWidth: 24, flex: true }, + { key: "Host", header: "Host", minWidth: 24, flex: true }, + { key: "Effective", header: "Effective", minWidth: 16 }, + { key: "Notes", header: "Notes", minWidth: 20, flex: true }, + ], + rows, + }).trimEnd(), + ); + defaultRuntime.log(""); + defaultRuntime.log(muted(`Precedence: ${params.report.note}`)); +} + function renderApprovalsSnapshot(snapshot: ExecApprovalsSnapshot, targetLabel: string) { const rich = isRich(); const heading = (text: string) => (rich ? theme.heading(text) : text); @@ -364,8 +481,14 @@ export function registerExecApprovalsCli(program: Command) { .action(async (opts: ExecApprovalsCliOpts) => { try { const { snapshot, nodeId, source } = await loadSnapshotTarget(opts); + const cfg = await loadConfigForApprovalsTarget({ opts, source }); + const effectivePolicy = buildEffectivePolicyReport({ + cfg, + source, + approvals: snapshot.file, + }); if (opts.json) { - defaultRuntime.writeJson(snapshot, 0); + defaultRuntime.writeJson({ ...snapshot, effectivePolicy }, 0); return; } @@ -376,6 +499,7 @@ export function registerExecApprovalsCli(program: Command) { } const targetLabel = source === "local" ? "local" : nodeId ? `node:${nodeId}` : "gateway"; renderApprovalsSnapshot(snapshot, targetLabel); + renderEffectivePolicy({ report: effectivePolicy }); } catch (err) { defaultRuntime.error(formatCliError(err)); defaultRuntime.exit(1); diff --git a/src/commands/doctor-security.test.ts b/src/commands/doctor-security.test.ts index 009d2beafc7..0bd657721c9 100644 --- a/src/commands/doctor-security.test.ts +++ b/src/commands/doctor-security.test.ts @@ -188,6 +188,46 @@ describe("noteSecurityWarnings gateway exposure", () => { expect(message).toContain("stricter side wins"); }); + it("attributes broader host policy warnings to wildcard agent entries", async () => { + await withExecApprovalsFile( + { + version: 1, + defaults: { + security: "full", + ask: "off", + }, + agents: { + "*": { + security: "allowlist", + ask: "always", + }, + }, + }, + async () => { + await noteSecurityWarnings({ + agents: { + list: [ + { + id: "runner", + tools: { + exec: { + security: "full", + ask: "off", + }, + }, + }, + ], + }, + } as OpenClawConfig); + }, + ); + + const message = lastMessage(); + expect(message).toContain("agents.list.runner.tools.exec is broader than the host exec policy"); + expect(message).toContain('agents.*.security="allowlist"'); + expect(message).toContain('agents.*.ask="always"'); + }); + it("does not invent a deny host policy when exec-approvals defaults.security is unset", async () => { await withExecApprovalsFile( { diff --git a/src/commands/doctor-security.ts b/src/commands/doctor-security.ts index 6c65d6112e1..b1029636932 100644 --- a/src/commands/doctor-security.ts +++ b/src/commands/doctor-security.ts @@ -6,15 +6,8 @@ import type { AgentConfig } from "../config/types.agents.js"; import { hasConfiguredSecretInput } from "../config/types.secrets.js"; import { resolveGatewayAuth } from "../gateway/auth.js"; import { isLoopbackHost, resolveGatewayBindHost } from "../gateway/net.js"; -import { - loadExecApprovals, - maxAsk, - minSecurity, - resolveExecApprovalsFromFile, - type ExecApprovalsFile, - type ExecAsk, - type ExecSecurity, -} from "../infra/exec-approvals.js"; +import { resolveExecPolicyScopeSummary } from "../infra/exec-approvals-effective.js"; +import { loadExecApprovals, type ExecAsk, type ExecSecurity } from "../infra/exec-approvals.js"; import { resolveDmAllowState } from "../security/dm-policy-shared.js"; import { note } from "../terminal/note.js"; import { resolveDefaultChannelAccountContext } from "./channel-account-context.js"; @@ -79,50 +72,6 @@ function execAskRank(value: ExecAsk): number { } } -function resolveHostExecPolicy(params: { - approvals: ExecApprovalsFile; - execConfig: { security?: ExecSecurity; ask?: ExecAsk } | undefined; - agentId?: string; -}): { - security: ExecSecurity; - ask: ExecAsk; - securitySource: string; - askSource: string; -} { - const basePath = "~/.openclaw/exec-approvals.json"; - const agentEntry = - params.agentId && params.approvals.agents && params.approvals.agents[params.agentId] - ? params.approvals.agents[params.agentId] - : undefined; - const defaults = params.approvals.defaults; - const configuredSecurity = params.execConfig?.security ?? "allowlist"; - const configuredAsk = params.execConfig?.ask ?? "on-miss"; - const resolved = resolveExecApprovalsFromFile({ - file: params.approvals, - agentId: params.agentId, - overrides: { - security: configuredSecurity, - ask: configuredAsk, - }, - }); - const security = minSecurity(configuredSecurity, resolved.agent.security); - const ask = resolved.agent.ask === "off" ? "off" : maxAsk(configuredAsk, resolved.agent.ask); - return { - security, - ask, - securitySource: agentEntry?.security - ? `${basePath} agents.${params.agentId}.security` - : defaults?.security - ? `${basePath} defaults.security` - : "caller tool policy fallback", - askSource: agentEntry?.ask - ? `${basePath} agents.${params.agentId}.ask` - : defaults?.ask - ? `${basePath} defaults.ask` - : "caller tool policy fallback", - }; -} - function collectExecPolicyConflictWarnings(cfg: OpenClawConfig): string[] { const warnings: string[] = []; const approvals = loadExecApprovals(); @@ -136,16 +85,21 @@ function collectExecPolicyConflictWarnings(cfg: OpenClawConfig): string[] { if (!execConfig || (!execConfig.security && !execConfig.ask)) { return; } - const host = resolveHostExecPolicy({ + const host = resolveExecPolicyScopeSummary({ approvals, execConfig, + configPath: + params.scopeLabel === "tools.exec" + ? "tools.exec" + : `agents.list.${params.agentId}.tools.exec`, + scopeLabel: params.scopeLabel, agentId: params.agentId, }); const securityConflict = execConfig.security !== undefined && - execSecurityRank(execConfig.security) > execSecurityRank(host.security); + execSecurityRank(execConfig.security) > execSecurityRank(host.security.effective); const askConflict = - execConfig.ask !== undefined && execAskRank(execConfig.ask) < execAskRank(host.ask); + execConfig.ask !== undefined && execAskRank(execConfig.ask) < execAskRank(host.ask.effective); if (!securityConflict && !askConflict) { return; } @@ -154,11 +108,11 @@ function collectExecPolicyConflictWarnings(cfg: OpenClawConfig): string[] { const hostParts: string[] = []; if (execConfig.security !== undefined) { configParts.push(`security="${execConfig.security}"`); - hostParts.push(`${host.securitySource}="${host.security}"`); + hostParts.push(`${host.security.hostSource}="${host.security.host}"`); } if (execConfig.ask !== undefined) { configParts.push(`ask="${execConfig.ask}"`); - hostParts.push(`${host.askSource}="${host.ask}"`); + hostParts.push(`${host.ask.hostSource}="${host.ask.host}"`); } warnings.push( @@ -166,7 +120,7 @@ function collectExecPolicyConflictWarnings(cfg: OpenClawConfig): string[] { `- ${params.scopeLabel} is broader than the host exec policy.`, ` Config: ${configParts.join(", ")}`, ` Host: ${hostParts.join(", ")}`, - ` Effective host exec stays security="${host.security}" ask="${host.ask}" because the stricter side wins.`, + ` Effective host exec stays security="${host.security.effective}" ask="${host.ask.effective}" because the stricter side wins.`, " Headless runs like isolated cron cannot answer approval prompts; align both files or enable Web UI, terminal UI, or chat exec approvals.", ` Inspect with: ${formatCliCommand("openclaw approvals get --gateway")}`, ].join("\n"), diff --git a/src/gateway/server-methods/exec-approval.ts b/src/gateway/server-methods/exec-approval.ts index a8252a8995a..1946d739caf 100644 --- a/src/gateway/server-methods/exec-approval.ts +++ b/src/gateway/server-methods/exec-approval.ts @@ -3,6 +3,7 @@ import { sanitizeExecApprovalDisplayText } from "../../infra/exec-approval-comma import type { ExecApprovalForwarder } from "../../infra/exec-approval-forwarder.js"; import { DEFAULT_EXEC_APPROVAL_TIMEOUT_MS, + isExecApprovalDecisionAllowed, type ExecApprovalDecision, } from "../../infra/exec-approvals.js"; import { @@ -327,6 +328,23 @@ export function createExecApprovalHandlers( } const approvalId = resolvedId.id; const snapshot = manager.getSnapshot(approvalId); + if ( + snapshot && + !isExecApprovalDecisionAllowed({ + decision, + ask: snapshot.request.ask, + }) + ) { + respond( + false, + undefined, + errorShape( + ErrorCodes.INVALID_REQUEST, + "allow-always is unavailable because host policy requires approval every time", + ), + ); + return; + } const resolvedBy = client?.connect?.client?.displayName ?? client?.connect?.client?.id; const ok = manager.resolve(approvalId, decision, resolvedBy ?? null); if (!ok) { diff --git a/src/gateway/server-methods/server-methods.test.ts b/src/gateway/server-methods/server-methods.test.ts index bf534cae23c..918f8c2f02e 100644 --- a/src/gateway/server-methods/server-methods.test.ts +++ b/src/gateway/server-methods/server-methods.test.ts @@ -420,11 +420,15 @@ describe("exec approval handlers", () => { async function resolveExecApproval(params: { handlers: ExecApprovalHandlers; id: string; + decision?: "allow-once" | "allow-always" | "deny"; respond: ReturnType; context: { broadcast: (event: string, payload: unknown) => void }; }) { return params.handlers["exec.approval.resolve"]({ - params: { id: params.id, decision: "allow-once" } as ExecApprovalResolveArgs["params"], + params: { + id: params.id, + decision: params.decision ?? "allow-once", + } as ExecApprovalResolveArgs["params"], respond: params.respond as unknown as ExecApprovalResolveArgs["respond"], context: toExecApprovalResolveContext(params.context), client: null, @@ -566,6 +570,50 @@ describe("exec approval handlers", () => { expect(broadcasts.some((entry) => entry.event === "exec.approval.resolved")).toBe(true); }); + it("rejects allow-always when the request ask mode is always", async () => { + const { handlers, broadcasts, respond, context } = createExecApprovalFixture(); + + const requestPromise = requestExecApproval({ + handlers, + respond, + context, + params: { twoPhase: true, ask: "always" }, + }); + + const requested = broadcasts.find((entry) => entry.event === "exec.approval.requested"); + const id = (requested?.payload as { id?: string })?.id ?? ""; + expect(id).not.toBe(""); + + const resolveRespond = vi.fn(); + await resolveExecApproval({ + handlers, + id, + decision: "allow-always", + respond: resolveRespond, + context, + }); + + expect(resolveRespond).toHaveBeenCalledWith( + false, + undefined, + expect.objectContaining({ + message: "allow-always is unavailable because host policy requires approval every time", + }), + ); + + const denyRespond = vi.fn(); + await resolveExecApproval({ + handlers, + id, + decision: "deny", + respond: denyRespond, + context, + }); + + await requestPromise; + expect(denyRespond).toHaveBeenCalledWith(true, { ok: true }, undefined); + }); + it("does not reuse a resolved exact id as a prefix for another pending approval", () => { const manager = new ExecApprovalManager(); const resolvedRecord = manager.create({ command: "echo old", host: "gateway" }, 2_000, "abc"); diff --git a/src/infra/exec-approval-forwarder.test.ts b/src/infra/exec-approval-forwarder.test.ts index 76dfcd8c612..bdd781a6bd9 100644 --- a/src/infra/exec-approval-forwarder.test.ts +++ b/src/infra/exec-approval-forwarder.test.ts @@ -495,6 +495,25 @@ describe("exec approval forwarder", () => { expect(text).toContain("Reply with: /approve allow-once|allow-always|deny"); }); + it("omits allow-always from forwarded fallback text when ask=always", async () => { + vi.useFakeTimers(); + const { deliver, forwarder } = createForwarder({ cfg: TARGETS_CFG }); + await expect( + forwarder.handleRequested({ + ...baseRequest, + request: { + ...baseRequest.request, + ask: "always", + }, + }), + ).resolves.toBe(true); + await Promise.resolve(); + const text = getFirstDeliveryText(deliver); + expect(text).toContain("Reply with: /approve allow-once|deny"); + expect(text).not.toContain("allow-once|allow-always|deny"); + expect(text).toContain("Allow Always is unavailable"); + }); + it.each([ { command: "bash safe\u200B.sh", diff --git a/src/infra/exec-approval-forwarder.ts b/src/infra/exec-approval-forwarder.ts index 3c1d7837d5d..5f4edade787 100644 --- a/src/infra/exec-approval-forwarder.ts +++ b/src/infra/exec-approval-forwarder.ts @@ -22,7 +22,11 @@ import { matchesApprovalRequestFilters } from "./approval-request-filters.js"; import { resolveExecApprovalCommandDisplay } from "./exec-approval-command-display.js"; import { formatExecApprovalExpiresIn } from "./exec-approval-reply.js"; import { resolveExecApprovalSessionTarget } from "./exec-approval-session-target.js"; -import type { ExecApprovalRequest, ExecApprovalResolved } from "./exec-approvals.js"; +import { + resolveExecApprovalAllowedDecisions, + type ExecApprovalRequest, + type ExecApprovalResolved, +} from "./exec-approvals.js"; import { deliverOutboundPayloads } from "./outbound/deliver.js"; import { approvalDecisionLabel, @@ -196,6 +200,8 @@ function formatApprovalCommand(command: string): { inline: boolean; text: string } function buildRequestMessage(request: ExecApprovalRequest, nowMs: number) { + const allowedDecisions = resolveExecApprovalAllowedDecisions({ ask: request.request.ask }); + const decisionText = allowedDecisions.join("|"); const lines: string[] = ["🔒 Exec approval required", `ID: ${request.id}`]; const command = formatApprovalCommand( resolveExecApprovalCommandDisplay(request.request).commandText, @@ -230,9 +236,14 @@ function buildRequestMessage(request: ExecApprovalRequest, nowMs: number) { lines.push(`Expires in: ${formatExecApprovalExpiresIn(request.expiresAtMs, nowMs)}`); lines.push("Mode: foreground (interactive approvals available in this chat)."); lines.push( - "Background mode note: non-interactive runs cannot wait for chat approvals; use pre-approved policy (allow-always or ask=off).", + allowedDecisions.includes("allow-always") + ? "Background mode note: non-interactive runs cannot wait for chat approvals; use pre-approved policy (allow-always or ask=off)." + : "Background mode note: non-interactive runs cannot wait for chat approvals; host policy still requires per-run approval unless ask=off.", ); - lines.push("Reply with: /approve allow-once|allow-always|deny"); + lines.push(`Reply with: /approve ${decisionText}`); + if (!allowedDecisions.includes("allow-always")) { + lines.push("Allow Always is unavailable because host policy requires approval every time."); + } return lines.join("\n"); } @@ -338,6 +349,7 @@ function buildExecPendingPayload(params: { approvalId: params.request.id, approvalSlug: params.request.id.slice(0, 8), text: buildRequestMessage(params.request, params.nowMs), + allowedDecisions: resolveExecApprovalAllowedDecisions({ ask: params.request.request.ask }), }); } diff --git a/src/infra/exec-approval-reply.test.ts b/src/infra/exec-approval-reply.test.ts index 26e61ac7f76..d24ca0af5f9 100644 --- a/src/infra/exec-approval-reply.test.ts +++ b/src/infra/exec-approval-reply.test.ts @@ -138,6 +138,46 @@ describe("exec approval reply helpers", () => { expect(payload.text).toContain("Full id: `req-1`"); }); + it("omits allow-always actions when host policy requires approval every time", () => { + const payload = buildExecApprovalPendingReplyPayload({ + approvalId: "req-ask-always", + approvalSlug: "slug-always", + ask: "always", + command: "echo ok", + host: "gateway", + }); + + expect(payload.channelData).toEqual({ + execApproval: { + approvalId: "req-ask-always", + approvalSlug: "slug-always", + allowedDecisions: ["allow-once", "deny"], + }, + }); + expect(payload.text).toContain("```txt\n/approve slug-always allow-once\n```"); + expect(payload.text).not.toContain("allow-always"); + expect(payload.text).toContain("Allow Always is unavailable"); + expect(payload.interactive).toEqual({ + blocks: [ + { + type: "buttons", + buttons: [ + { + label: "Allow Once", + value: "/approve req-ask-always allow-once", + style: "success", + }, + { + label: "Deny", + value: "/approve req-ask-always deny", + style: "danger", + }, + ], + }, + ], + }); + }); + it("uses a longer fence for commands containing triple backticks", () => { const payload = buildExecApprovalPendingReplyPayload({ approvalId: "req-2", diff --git a/src/infra/exec-approval-reply.ts b/src/infra/exec-approval-reply.ts index 3fe7f6300a6..4f2734da50d 100644 --- a/src/infra/exec-approval-reply.ts +++ b/src/infra/exec-approval-reply.ts @@ -1,8 +1,12 @@ import type { ReplyPayload } from "../auto-reply/types.js"; import type { InteractiveReply, InteractiveReplyButton } from "../interactive/payload.js"; -import type { ExecHost } from "./exec-approvals.js"; +import { + resolveExecApprovalAllowedDecisions, + type ExecApprovalDecision, + type ExecHost, +} from "./exec-approvals.js"; -export type ExecApprovalReplyDecision = "allow-once" | "allow-always" | "deny"; +export type ExecApprovalReplyDecision = ExecApprovalDecision; export type ExecApprovalUnavailableReason = | "initiating-platform-disabled" | "initiating-platform-unsupported" @@ -26,6 +30,8 @@ export type ExecApprovalPendingReplyParams = { approvalId: string; approvalSlug: string; approvalCommandId?: string; + ask?: string | null; + allowedDecisions?: readonly ExecApprovalReplyDecision[]; command: string; cwd?: string; host: ExecHost; @@ -41,7 +47,21 @@ export type ExecApprovalUnavailableReplyParams = { sentApproverDms?: boolean; }; -const DEFAULT_ALLOWED_DECISIONS = ["allow-once", "allow-always", "deny"] as const; +function resolveAllowedDecisions(params: { + ask?: string | null; + allowedDecisions?: readonly ExecApprovalReplyDecision[]; +}): readonly ExecApprovalReplyDecision[] { + return params.allowedDecisions ?? resolveExecApprovalAllowedDecisions({ ask: params.ask }); +} + +function buildApprovalCommandFence( + descriptors: readonly ExecApprovalActionDescriptor[], +): string | null { + if (descriptors.length === 0) { + return null; + } + return buildFence(descriptors.map((descriptor) => descriptor.command).join("\n"), "txt"); +} export function buildExecApprovalCommandText(params: { approvalCommandId: string; @@ -52,13 +72,14 @@ export function buildExecApprovalCommandText(params: { export function buildExecApprovalActionDescriptors(params: { approvalCommandId: string; + ask?: string | null; allowedDecisions?: readonly ExecApprovalReplyDecision[]; }): ExecApprovalActionDescriptor[] { const approvalCommandId = params.approvalCommandId.trim(); if (!approvalCommandId) { return []; } - const allowedDecisions = params.allowedDecisions ?? DEFAULT_ALLOWED_DECISIONS; + const allowedDecisions = resolveAllowedDecisions(params); const descriptors: ExecApprovalActionDescriptor[] = []; if (allowedDecisions.includes("allow-once")) { descriptors.push({ @@ -112,10 +133,11 @@ function buildApprovalInteractiveButtons( export function buildApprovalInteractiveReply(params: { approvalId: string; + ask?: string | null; allowedDecisions?: readonly ExecApprovalReplyDecision[]; }): InteractiveReply | undefined { const buttons = buildApprovalInteractiveButtons( - params.allowedDecisions ?? DEFAULT_ALLOWED_DECISIONS, + resolveAllowedDecisions(params), params.approvalId, ); return buttons.length > 0 ? { blocks: [{ type: "buttons", buttons }] } : undefined; @@ -123,10 +145,12 @@ export function buildApprovalInteractiveReply(params: { export function buildExecApprovalInteractiveReply(params: { approvalCommandId: string; + ask?: string | null; allowedDecisions?: readonly ExecApprovalReplyDecision[]; }): InteractiveReply | undefined { return buildApprovalInteractiveReply({ approvalId: params.approvalCommandId, + ask: params.ask, allowedDecisions: params.allowedDecisions, }); } @@ -218,23 +242,33 @@ export function buildExecApprovalPendingReplyPayload( params: ExecApprovalPendingReplyParams, ): ReplyPayload { const approvalCommandId = params.approvalCommandId?.trim() || params.approvalSlug; + const allowedDecisions = resolveAllowedDecisions(params); + const descriptors = buildExecApprovalActionDescriptors({ + approvalCommandId, + allowedDecisions, + }); + const primaryAction = descriptors[0] ?? null; + const secondaryActions = descriptors.slice(1); const lines: string[] = []; const warningText = params.warningText?.trim(); if (warningText) { lines.push(warningText); } lines.push("Approval required."); - lines.push("Run:"); - lines.push(buildFence(`/approve ${approvalCommandId} allow-once`, "txt")); + if (primaryAction) { + lines.push("Run:"); + lines.push(buildFence(primaryAction.command, "txt")); + } lines.push("Pending command:"); lines.push(buildFence(params.command, "sh")); - lines.push("Other options:"); - lines.push( - buildFence( - `/approve ${approvalCommandId} allow-always\n/approve ${approvalCommandId} deny`, - "txt", - ), - ); + const secondaryFence = buildApprovalCommandFence(secondaryActions); + if (secondaryFence) { + lines.push("Other options:"); + lines.push(secondaryFence); + } + if (!allowedDecisions.includes("allow-always")) { + lines.push("Host policy requires approval every time, so Allow Always is unavailable."); + } const info: string[] = []; info.push(`Host: ${params.host}`); if (params.nodeId) { @@ -253,12 +287,15 @@ export function buildExecApprovalPendingReplyPayload( return { text: lines.join("\n\n"), - interactive: buildApprovalInteractiveReply({ approvalId: params.approvalId }), + interactive: buildApprovalInteractiveReply({ + approvalId: params.approvalId, + allowedDecisions, + }), channelData: { execApproval: { approvalId: params.approvalId, approvalSlug: params.approvalSlug, - allowedDecisions: DEFAULT_ALLOWED_DECISIONS, + allowedDecisions, }, }, }; diff --git a/src/infra/exec-approvals-effective.ts b/src/infra/exec-approvals-effective.ts new file mode 100644 index 00000000000..5960c75362c --- /dev/null +++ b/src/infra/exec-approvals-effective.ts @@ -0,0 +1,196 @@ +import { DEFAULT_AGENT_ID } from "../routing/session-key.js"; +import { + maxAsk, + minSecurity, + resolveExecApprovalsFromFile, + type ExecApprovalsFile, + type ExecAsk, + type ExecSecurity, +} from "./exec-approvals.js"; + +const DEFAULT_REQUESTED_SECURITY: ExecSecurity = "allowlist"; +const DEFAULT_REQUESTED_ASK: ExecAsk = "on-miss"; +const DEFAULT_HOST_PATH = "~/.openclaw/exec-approvals.json"; +const REQUESTED_DEFAULT_LABEL = { + security: DEFAULT_REQUESTED_SECURITY, + ask: DEFAULT_REQUESTED_ASK, +} as const; + +export type ExecPolicyFieldSummary = { + requested: TValue; + requestedSource: string; + host: TValue; + hostSource: string; + effective: TValue; + note: string; +}; + +export type ExecPolicyScopeSummary = { + scopeLabel: string; + configPath: string; + agentId?: string; + security: ExecPolicyFieldSummary; + ask: ExecPolicyFieldSummary; + askFallback: { + effective: ExecSecurity; + source: string; + }; +}; + +function formatRequestedSource(params: { + path: string; + field: "security" | "ask"; + explicit: boolean; +}): string { + return params.explicit + ? `${params.path}.${params.field}` + : `OpenClaw default (${REQUESTED_DEFAULT_LABEL[params.field]})`; +} + +type ExecPolicyField = "security" | "ask" | "askFallback"; + +function readExecPolicyField(params: { + field: ExecPolicyField; + entry?: { + security?: ExecSecurity; + ask?: ExecAsk; + askFallback?: ExecSecurity; + }; +}): ExecSecurity | ExecAsk | undefined { + switch (params.field) { + case "security": + return params.entry?.security; + case "ask": + return params.entry?.ask; + case "askFallback": + return params.entry?.askFallback; + } +} + +function resolveHostFieldSource(params: { + hostPath: string; + agentId?: string; + field: ExecPolicyField; + approvals: ExecApprovalsFile; +}): string { + const agentKey = params.agentId ?? DEFAULT_AGENT_ID; + const explicitAgentEntry = params.approvals.agents?.[agentKey]; + if (readExecPolicyField({ field: params.field, entry: explicitAgentEntry }) !== undefined) { + return `${params.hostPath} agents.${agentKey}.${params.field}`; + } + const wildcardEntry = params.approvals.agents?.["*"]; + if (readExecPolicyField({ field: params.field, entry: wildcardEntry }) !== undefined) { + return `${params.hostPath} agents.*.${params.field}`; + } + if ( + readExecPolicyField({ + field: params.field, + entry: params.approvals.defaults, + }) !== undefined + ) { + return `${params.hostPath} defaults.${params.field}`; + } + return "inherits requested tool policy"; +} + +function resolveAskNote(params: { + requestedAsk: ExecAsk; + hostAsk: ExecAsk; + effectiveAsk: ExecAsk; +}): string { + if (params.hostAsk === "off" && params.requestedAsk !== "off") { + return "host ask=off suppresses prompts"; + } + if (params.effectiveAsk === params.requestedAsk) { + return "requested ask applies"; + } + return "more aggressive ask wins"; +} + +function formatHostSource(params: { + hostPath: string; + agentId?: string; + field: ExecPolicyField; + approvals: ExecApprovalsFile; +}): string { + return resolveHostFieldSource(params); +} + +export function resolveExecPolicyScopeSummary(params: { + approvals: ExecApprovalsFile; + execConfig?: { security?: ExecSecurity; ask?: ExecAsk } | undefined; + configPath: string; + scopeLabel: string; + agentId?: string; + hostPath?: string; +}): ExecPolicyScopeSummary { + const requestedSecurity = params.execConfig?.security ?? DEFAULT_REQUESTED_SECURITY; + const requestedAsk = params.execConfig?.ask ?? DEFAULT_REQUESTED_ASK; + const resolved = resolveExecApprovalsFromFile({ + file: params.approvals, + agentId: params.agentId, + overrides: { + security: requestedSecurity, + ask: requestedAsk, + }, + }); + const hostPath = params.hostPath ?? DEFAULT_HOST_PATH; + const effectiveSecurity = minSecurity(requestedSecurity, resolved.agent.security); + const effectiveAsk = + resolved.agent.ask === "off" ? "off" : maxAsk(requestedAsk, resolved.agent.ask); + return { + scopeLabel: params.scopeLabel, + configPath: params.configPath, + ...(params.agentId ? { agentId: params.agentId } : {}), + security: { + requested: requestedSecurity, + requestedSource: formatRequestedSource({ + path: params.configPath, + field: "security", + explicit: params.execConfig?.security !== undefined, + }), + host: resolved.agent.security, + hostSource: formatHostSource({ + hostPath, + agentId: params.agentId, + field: "security", + approvals: params.approvals, + }), + effective: effectiveSecurity, + note: + effectiveSecurity === requestedSecurity + ? "requested security applies" + : "stricter host security wins", + }, + ask: { + requested: requestedAsk, + requestedSource: formatRequestedSource({ + path: params.configPath, + field: "ask", + explicit: params.execConfig?.ask !== undefined, + }), + host: resolved.agent.ask, + hostSource: formatHostSource({ + hostPath, + agentId: params.agentId, + field: "ask", + approvals: params.approvals, + }), + effective: effectiveAsk, + note: resolveAskNote({ + requestedAsk, + hostAsk: resolved.agent.ask, + effectiveAsk, + }), + }, + askFallback: { + effective: resolved.agent.askFallback, + source: formatHostSource({ + hostPath, + agentId: params.agentId, + field: "askFallback", + approvals: params.approvals, + }), + }, + }; +} diff --git a/src/infra/exec-approvals-policy.test.ts b/src/infra/exec-approvals-policy.test.ts index 135d3a664c0..70ad08765ea 100644 --- a/src/infra/exec-approvals-policy.test.ts +++ b/src/infra/exec-approvals-policy.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from "vitest"; +import { resolveExecPolicyScopeSummary } from "./exec-approvals-effective.js"; import { makeMockCommandResolution, makeMockExecutableResolution, @@ -189,4 +190,105 @@ describe("exec approvals policy helpers", () => { }), ).toBe(false); }); + + it("explains stricter host security and ask precedence", () => { + const summary = resolveExecPolicyScopeSummary({ + approvals: { + version: 1, + defaults: { + security: "allowlist", + ask: "always", + askFallback: "deny", + }, + }, + execConfig: { + security: "full", + ask: "off", + }, + configPath: "tools.exec", + scopeLabel: "tools.exec", + }); + + expect(summary.security).toMatchObject({ + requested: "full", + host: "allowlist", + effective: "allowlist", + hostSource: "~/.openclaw/exec-approvals.json defaults.security", + note: "stricter host security wins", + }); + expect(summary.ask).toMatchObject({ + requested: "off", + host: "always", + effective: "always", + hostSource: "~/.openclaw/exec-approvals.json defaults.ask", + note: "more aggressive ask wins", + }); + expect(summary.askFallback).toEqual({ + effective: "deny", + source: "~/.openclaw/exec-approvals.json defaults.askFallback", + }); + }); + + it("explains host ask=off suppression separately from stricter ask", () => { + const summary = resolveExecPolicyScopeSummary({ + approvals: { + version: 1, + defaults: { + ask: "off", + }, + }, + execConfig: { + ask: "always", + }, + configPath: "tools.exec", + scopeLabel: "tools.exec", + }); + + expect(summary.ask).toMatchObject({ + requested: "always", + host: "off", + effective: "off", + note: "host ask=off suppresses prompts", + }); + }); + + it("attributes host policy to wildcard agent entries before defaults", () => { + const summary = resolveExecPolicyScopeSummary({ + approvals: { + version: 1, + defaults: { + security: "full", + ask: "off", + askFallback: "full", + }, + agents: { + "*": { + security: "allowlist", + ask: "always", + askFallback: "deny", + }, + }, + }, + execConfig: { + security: "full", + ask: "off", + }, + configPath: "agents.list.runner.tools.exec", + scopeLabel: "agent:runner", + agentId: "runner", + }); + + expect(summary.security).toMatchObject({ + host: "allowlist", + hostSource: "~/.openclaw/exec-approvals.json agents.*.security", + }); + expect(summary.ask).toMatchObject({ + host: "always", + hostSource: "~/.openclaw/exec-approvals.json agents.*.ask", + }); + expect(summary.askFallback).toEqual({ + effective: "deny", + source: "~/.openclaw/exec-approvals.json agents.*.askFallback", + }); + }); }); diff --git a/src/infra/exec-approvals.ts b/src/infra/exec-approvals.ts index 98f8444decb..2bc2602ee46 100644 --- a/src/infra/exec-approvals.ts +++ b/src/infra/exec-approvals.ts @@ -659,6 +659,28 @@ export function maxAsk(a: ExecAsk, b: ExecAsk): ExecAsk { } export type ExecApprovalDecision = "allow-once" | "allow-always" | "deny"; +export const DEFAULT_EXEC_APPROVAL_DECISIONS = [ + "allow-once", + "allow-always", + "deny", +] as const satisfies readonly ExecApprovalDecision[]; + +export function resolveExecApprovalAllowedDecisions(params?: { + ask?: string | null; +}): readonly ExecApprovalDecision[] { + const ask = normalizeExecAsk(params?.ask); + if (ask === "always") { + return ["allow-once", "deny"]; + } + return DEFAULT_EXEC_APPROVAL_DECISIONS; +} + +export function isExecApprovalDecisionAllowed(params: { + decision: ExecApprovalDecision; + ask?: string | null; +}): boolean { + return resolveExecApprovalAllowedDecisions({ ask: params.ask }).includes(params.decision); +} export async function requestExecApprovalViaSocket(params: { socketPath: string; diff --git a/src/plugin-sdk/approval-runtime.ts b/src/plugin-sdk/approval-runtime.ts index 9cc5997969b..3e3db6aed0d 100644 --- a/src/plugin-sdk/approval-runtime.ts +++ b/src/plugin-sdk/approval-runtime.ts @@ -2,6 +2,7 @@ export { DEFAULT_EXEC_APPROVAL_TIMEOUT_MS, + resolveExecApprovalAllowedDecisions, type ExecApprovalDecision, type ExecApprovalRequest, type ExecApprovalRequestPayload,