diff --git a/src/gateway/node-invoke-plugin-policy.test.ts b/src/gateway/node-invoke-plugin-policy.test.ts index be16220811c9..63705a3090d9 100644 --- a/src/gateway/node-invoke-plugin-policy.test.ts +++ b/src/gateway/node-invoke-plugin-policy.test.ts @@ -159,11 +159,15 @@ function createApprovalRequestPolicy(params?: { timeoutMs?: number; title?: string; description?: string; + toolName?: string; + agentId?: string; }): NodeInvokePolicyRegistration { return createDemoPolicy(async (ctx: OpenClawPluginNodeInvokePolicyContext) => { const approval = await ctx.approvals?.request({ title: params?.title ?? "Sensitive action", description: params?.description ?? "Needs approval", + ...(params?.toolName === undefined ? {} : { toolName: params.toolName }), + ...(params?.agentId === undefined ? {} : { agentId: params.agentId }), ...(params?.timeoutMs === undefined ? {} : { timeoutMs: params.timeoutMs }), }); return { ok: true, payload: approval ?? null }; @@ -643,6 +647,37 @@ describe("applyPluginNodeInvokePolicy", () => { await expectApprovalResolution(resultPromise, manager, record); }); + it("sanitizes node-policy approval titles at creation like the RPC ingress", async () => { + const manager = new ExecApprovalManager(); + const getApprovalClientConnIds = createApprovalClientLookup([ + createApprovalClient({ + connId: "conn-owner-approval", + clientId: "client-owner", + deviceId: "device-owner", + }), + ]); + setDangerousDemoCommandRegistry([ + // Bidi override + zero-width space: reviewer-spoofing characters. + createApprovalRequestPolicy({ + title: "Deploy‮yolped", + description: "safe​text", + toolName: "tool‮run", + agentId: "agent​x", + }), + ]); + const { context } = createContext({ pluginApprovalManager: manager, getApprovalClientConnIds }); + const resultPromise = invokeDemoPolicy(context, createOperatorClient()); + + const record = await expectSinglePendingApproval(manager); + expect(record.request.title).toBe("Deploy\\u{202E}yolped"); + expect(record.request.description).toBe("safe\\u{200B}text"); + // Metadata is interpolated into channel approval text lines. + expect(record.request.toolName).toBe("tool\\u{202E}run"); + expect(record.request.agentId).toBe("agent\\u{200B}x"); + + await expectApprovalResolution(resultPromise, manager, record); + }); + it("forwards plugin policy approvals to the originating turn source", async () => { const manager = new ExecApprovalManager({ validateAgentRuntimeDelegatedAuthority: () => true, diff --git a/src/gateway/node-invoke-plugin-policy.ts b/src/gateway/node-invoke-plugin-policy.ts index 29f6aa14b551..ce4f4fb0a4f1 100644 --- a/src/gateway/node-invoke-plugin-policy.ts +++ b/src/gateway/node-invoke-plugin-policy.ts @@ -3,6 +3,10 @@ import { randomUUID } from "node:crypto"; import { normalizeOptionalString } from "@openclaw/normalization-core/string-coerce"; import { truncateUtf16Safe } from "@openclaw/normalization-core/utf16-slice"; +import { + sanitizeExecApprovalDisplayText, + sanitizeExecApprovalWarningText, +} from "../infra/exec-approval-command-display.js"; import type { PluginApprovalRequestPayload } from "../infra/plugin-approvals.js"; import { resolvePluginApprovalTimeoutMs } from "../infra/plugin-approvals.js"; import type { PluginRegistry } from "../plugins/registry-types.js"; @@ -24,6 +28,11 @@ import type { GatewayClient, GatewayRequestContext, RespondFn } from "./server-m // Plugin node.invoke policies are the last gateway-side guard before a // plugin-declared dangerous node command reaches the node transport. +function sanitizeOptionalMeta(value?: string | null): string | null { + const normalized = normalizeOptionalString(value); + return normalized ? sanitizeExecApprovalDisplayText(normalized) : null; +} + function parseScopes(client: GatewayClient | null): string[] { return Array.isArray(client?.connect?.scopes) ? client.connect.scopes.filter((scope): scope is string => typeof scope === "string") @@ -107,12 +116,24 @@ function createApprovalRuntime(params: { } const request: PluginApprovalRequestPayload = { pluginId: params.pluginId, - title: truncateUtf16Safe(input.title, 80), - description: truncateUtf16Safe(input.description, 256), + // Same creation-boundary sanitize as the RPC ingress: this record + // feeds the identical broadcast/forwarder/push paths. Normalize first + // so a whitespace-only title still fails closed at register (escaping + // the whitespace would make an unrenderable prompt look renderable). + title: truncateUtf16Safe( + sanitizeExecApprovalDisplayText(normalizeOptionalString(input.title) ?? ""), + 80, + ), + description: truncateUtf16Safe( + sanitizeExecApprovalWarningText(normalizeOptionalString(input.description) ?? ""), + 256, + ), severity: input.severity ?? "warning", - toolName: normalizeOptionalString(input.toolName) ?? null, + // toolName/agentId are interpolated into channel approval text; only + // host-minted runtime identity values skip the display escape. + toolName: sanitizeOptionalMeta(input.toolName), toolCallId: normalizeOptionalString(input.toolCallId) ?? null, - agentId: callerIdentity?.agentId ?? normalizeOptionalString(input.agentId) ?? null, + agentId: callerIdentity?.agentId ?? sanitizeOptionalMeta(input.agentId), sessionKey: callerIdentity?.sessionKey ?? normalizeOptionalString(input.sessionKey) ?? null, runId: callerIdentity?.operationalRunInstance.runId ?? null, turnSourceChannel: turnSource.turnSourceChannel, diff --git a/src/gateway/server-methods/exec-approval.agent-runtime.test.ts b/src/gateway/server-methods/exec-approval.agent-runtime.test.ts index 36f64f6923ac..fb2dcef7aeb7 100644 --- a/src/gateway/server-methods/exec-approval.agent-runtime.test.ts +++ b/src/gateway/server-methods/exec-approval.agent-runtime.test.ts @@ -112,6 +112,30 @@ describe("exec approval signed agent runtime", () => { }); }); + it("sanitizes display-only cwd and resolvedPath in the stored request", async () => { + const manager = new ExecApprovalManager({ + validateAgentRuntimeDelegatedAuthority: () => true, + }); + const handler = createExecApprovalHandlers(manager)["exec.approval.request"]!; + const opts = requestOptions(identity(false)); + // Bidi override in cwd/resolvedPath can spoof what path reviewers see. + (opts.params as Record).cwd = "/tmp/safe‮evil"; + (opts.params as Record).resolvedPath = "/usr/bin/echo​x"; + // Free-form policy strings must not reach reviewer meta rows: security/ask + // are closed enums (arbitrary values null out), host is escape-hardened. + (opts.params as Record).security = "full‮looks-deny"; + (opts.params as Record).ask = "always​ish"; + const pending = handler(opts); + await vi.waitFor(() => expect(manager.listPendingRecords()).toHaveLength(1)); + const record = manager.listPendingRecords()[0]!; + expect(record.request.cwd).toBe("/tmp/safe\\u{202E}evil"); + expect(record.request.resolvedPath).toBe("/usr/bin/echo\\u{200B}x"); + expect(record.request.security).toBeNull(); + expect(record.request.ask).toBeNull(); + manager.resolve(record.id, "deny"); + await pending; + }); + it("cancels an exec approval when authority closes after the handshake", async () => { let active = true; const manager = new ExecApprovalManager({ diff --git a/src/gateway/server-methods/exec-approval.ts b/src/gateway/server-methods/exec-approval.ts index 78f877b1b711..b13c4c613ff8 100644 --- a/src/gateway/server-methods/exec-approval.ts +++ b/src/gateway/server-methods/exec-approval.ts @@ -17,6 +17,7 @@ import { sanitizeExecApprovalWarningText, } from "../../infra/exec-approval-command-display.js"; import type { ExecApprovalForwarder } from "../../infra/exec-approval-forwarder.js"; +import { normalizeExecAsk, normalizeExecSecurity } from "../../infra/exec-approvals-core.js"; import { DEFAULT_EXEC_APPROVAL_TIMEOUT_MS, normalizeExecApprovalUnavailableDecisions, @@ -324,11 +325,21 @@ export function createExecApprovalHandlers( envKeys: envBinding.envKeys.length > 0 ? envBinding.envKeys : undefined, systemRunBinding: systemRunBinding?.binding ?? null, systemRunPlan: approvalContext.plan, - cwd: effectiveCwd ?? null, + // cwd/resolvedPath are display-only in the stored record (execution + // binds effectiveCwd via systemRunBinding above); sanitize like the + // command so bidi/invisible chars cannot spoof reviewer surfaces. + cwd: effectiveCwd ? sanitizeExecApprovalDisplayText(effectiveCwd) : null, + // nodeId/agentId/sessionKey stay raw: they are matched against the + // node registry and session routing, so escaping would break real + // lookups without display gain (hostile values match nothing). nodeId: host === "node" ? nodeId : null, - host: host || null, - security: p.security ?? null, - ask: p.ask ?? null, + // host is enum-gated ("node" checks); escape is identity for valid + // values and defuses invisible-char spoofing in reviewer meta rows. + host: host ? sanitizeExecApprovalDisplayText(host) : null, + // Closed enums: arbitrary strings become null instead of reaching + // reviewer surfaces; decision resolution already treats them as null. + security: normalizeExecSecurity(p.security) ?? null, + ask: normalizeExecAsk(p.ask) ?? null, warningText: warningText ? sanitizeExecApprovalWarningText(warningText) : null, commandAnalysis, commandSpans, @@ -338,7 +349,7 @@ export function createExecApprovalHandlers( unavailableDecisions, }), agentId: effectiveAgentId ?? null, - resolvedPath: p.resolvedPath ?? null, + resolvedPath: p.resolvedPath ? sanitizeExecApprovalDisplayText(p.resolvedPath) : null, sessionKey: effectiveSessionKey ?? null, sessionId: trustedAgentRuntime ? null : (normalizeOptionalString(p.sessionId) ?? null), runId: requestRunId ?? null, diff --git a/src/gateway/server-methods/plugin-approval.test.ts b/src/gateway/server-methods/plugin-approval.test.ts index dcb3a42b75d1..d1f5c441ba8d 100644 --- a/src/gateway/server-methods/plugin-approval.test.ts +++ b/src/gateway/server-methods/plugin-approval.test.ts @@ -321,6 +321,75 @@ describe("createPluginApprovalHandlers", () => { expect(finalResult.decision).toBe("allow-once"); }); + it("sanitizes title/description/detail at creation so every surface gets safe text", async () => { + const handlers = createPluginApprovalHandlers(manager); + const respond = vi.fn(); + const opts = createMockOptions( + "plugin.approval.request", + { + // Bidi override + zero-width space: the classic reviewer-spoof pair. + title: "Deploy‮yolped", + description: "safe​text", + // Passes the protocol's 16,384 raw cap but expands past it once + // invisibles become \u{...} escapes — the storage cap must re-apply. + detail: `line‪one${"‮".repeat(2_100)}`, + severity: "warning", + // Metadata is interpolated into channel approval text lines. + pluginId: "plug‮in", + toolName: "tool​run", + agentId: "agent‪x", + twoPhase: true, + }, + { respond }, + ); + const handlerPromise = expectDefined( + handlers["plugin.approval.request"], + 'handlers["plugin.approval.request"] test invariant', + )(opts); + const approvalId = await waitForAcceptedApproval(respond); + const stored = manager.getSnapshot(approvalId)?.request; + expect(stored?.title).toBe("Deploy\\u{202E}yolped"); + expect(stored?.description).toBe("safe\\u{200B}text"); + expect(stored?.detail?.startsWith("line\\u{202A}one")).toBe(true); + // Stored detail is capped like the durable presentation's copy. + expect(Array.from(stored?.detail ?? "").length).toBeLessThanOrEqual(16_384); + expect(stored?.detail?.endsWith("…[truncated]")).toBe(true); + expect(stored?.pluginId).toBe("plug\\u{202E}in"); + expect(stored?.toolName).toBe("tool\\u{200B}run"); + expect(stored?.agentId).toBe("agent\\u{202A}x"); + // The live broadcast payload is built from the stored record, so it is + // now safe for channels/push/web without per-surface re-sanitizing. + const requestedBroadcast = broadcastCall(opts); + expect(requestedBroadcast.payload.request).toMatchObject({ + title: "Deploy\\u{202E}yolped", + description: "safe\\u{200B}text", + }); + manager.resolve(approvalId, "deny"); + await handlerPromise; + }); + + it("rejects a title whose sanitized form exceeds the display limit", async () => { + const handlers = createPluginApprovalHandlers(manager); + const respond = vi.fn(); + // 20 invisibles expand to \u{202E} escapes (8 chars each = 160 > 80 cap) + // while the raw title passes protocol validation at 26 code points. + const opts = createMockOptions( + "plugin.approval.request", + { + title: `spoof${"‮".repeat(20)}x`, + description: "plain description", + twoPhase: true, + }, + { respond }, + ); + await expectDefined( + handlers["plugin.approval.request"], + 'handlers["plugin.approval.request"] test invariant', + )(opts); + const error = expectResponseRejected(respond); + expect(error.message).toContain("exceeds the display limit"); + }); + it("delivers requests to iOS push with the exec-equivalent visibility gate", async () => { const handleRequested = vi.fn(async () => true); const handlers = createPluginApprovalHandlers(manager, { diff --git a/src/gateway/server-methods/plugin-approval.ts b/src/gateway/server-methods/plugin-approval.ts index 331dfd50b32b..6a981dbc1d14 100644 --- a/src/gateway/server-methods/plugin-approval.ts +++ b/src/gateway/server-methods/plugin-approval.ts @@ -7,6 +7,10 @@ import { validatePluginApprovalRequestParams, validatePluginApprovalResolveParams, } from "../../../packages/gateway-protocol/src/index.js"; +import { + sanitizeExecApprovalDisplayText, + sanitizeExecApprovalWarningText, +} from "../../infra/exec-approval-command-display.js"; import type { ExecApprovalForwarder } from "../../infra/exec-approval-forwarder.js"; import { resolveCanonicalPluginApprovalRequestAllowedDecisions } from "../../infra/plugin-approval-canonical-decisions.js"; import type { @@ -14,7 +18,12 @@ import type { PluginApprovalRequestPayload, PluginApprovalResolved, } from "../../infra/plugin-approvals.js"; -import { resolvePluginApprovalTimeoutMs } from "../../infra/plugin-approvals.js"; +import { + PLUGIN_APPROVAL_DESCRIPTION_MAX_LENGTH, + PLUGIN_APPROVAL_TITLE_MAX_LENGTH, + resolvePluginApprovalTimeoutMs, + truncatePluginApprovalDetail, +} from "../../infra/plugin-approvals.js"; import type { ExecApprovalManager } from "../exec-approval-manager.js"; import { resolveRequestedSessionAgentId } from "../session-request-agent.js"; import { resolveStoredSessionKeyForAgentStore } from "../session-store-key.js"; @@ -137,13 +146,45 @@ export function createPluginApprovalHandlers( }) : null; + // Sanitize once at the creation boundary, like exec command text: the + // raw record otherwise reaches channel messages, iOS push, and the web + // modal unescaped (bidi/invisible spoofing). Escaping expands invisible + // chars to \u{...}, so re-check the protocol caps: a spoof-heavy title + // must fail loud here, not as a misleading registration throw later. + const sanitizedTitle = sanitizeExecApprovalDisplayText(p.title); + const sanitizedDescription = sanitizeExecApprovalWarningText(p.description); + if ( + Array.from(sanitizedTitle).length > PLUGIN_APPROVAL_TITLE_MAX_LENGTH || + Array.from(sanitizedDescription).length > PLUGIN_APPROVAL_DESCRIPTION_MAX_LENGTH + ) { + respond( + false, + undefined, + errorShape( + ErrorCodes.INVALID_REQUEST, + "approval title or description exceeds the display limit after sanitization", + ), + ); + return; + } + const rawDetail = normalizeTrimmedString(p.detail); + // Untrusted display metadata gets the same escape as title/description: + // pluginId/toolName/agentId are interpolated into channel approval text. + // Host-minted runtime identity values stay authoritative and unescaped. + const sanitizeMeta = (value?: string | null): string | null => + normalizeTrimmedString(value) === null + ? null + : sanitizeExecApprovalDisplayText(normalizeTrimmedString(value)!); const request: PluginApprovalRequestPayload = { - pluginId: trustedAgentRuntime?.approvalOwnerPluginId ?? p.pluginId ?? null, - title: p.title, - description: p.description, - detail: normalizeTrimmedString(p.detail), + pluginId: trustedAgentRuntime?.approvalOwnerPluginId ?? sanitizeMeta(p.pluginId), + title: sanitizedTitle, + description: sanitizedDescription, + detail: + rawDetail === null + ? null + : truncatePluginApprovalDetail(sanitizeExecApprovalWarningText(rawDetail)), severity: (p.severity as PluginApprovalRequestPayload["severity"]) ?? null, - toolName: p.toolName ?? null, + toolName: sanitizeMeta(p.toolName), toolCallId: p.toolCallId ?? null, ...(Array.isArray(p.allowedDecisions) ? { @@ -154,7 +195,7 @@ export function createPluginApprovalHandlers( : {}), agentId: trustedAgentRuntime?.agentId ?? - (sessionOwner?.ok ? sessionOwner.agentId : (p.agentId ?? null)), + (sessionOwner?.ok ? sessionOwner.agentId : sanitizeMeta(p.agentId)), sessionKey, runId: trustedAgentRuntime?.operationalRunInstance.runId ?? null, turnSourceChannel: trustedAgentRuntime