mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-25 11:55:47 -06:00
fix(approvals): sanitize plugin titles and exec paths at the creation boundary (#123742)
* fix(approvals): sanitize plugin titles and exec paths at the creation boundary Plugin approval title/description/detail were stored raw and flowed unescaped into channel messages, iOS lock-screen push, and the web modal — bidi/invisible characters could spoof what the operator approves. Exec approvals already sanitize command text at creation; mirror that for the plugin request and for exec's display-only cwd/resolvedPath fields. Re-check the protocol size caps after escaping so a spoof-heavy title fails as INVALID_REQUEST instead of a misleading registration throw. * fix(gateway): sanitize node-policy plugin approvals through the same boundary The node-policy approval runtime truncated title/description but never sanitized them, so the identical broadcast/forwarder/push paths stayed spoofable through this sibling creator (ClawSweeper P1 finding). Normalize first so a whitespace-only title still fails closed at register. * fix(approvals): sanitize plugin metadata and cap stored detail ClawSweeper follow-ups: pluginId/toolName/fallback agentId are interpolated into channel approval text and were stored raw; the stored detail skipped the 16,384 cap the durable presentation applies, so escape expansion could exceed it. Sanitize the metadata at the same boundary (host-minted runtime identity stays authoritative) and cap detail at storage. * fix(gateway): sanitize node-policy approval metadata too toolName and fallback agentId from node-policy code are interpolated into channel approval text; escape them like the RPC ingress. Host-minted runtime identity values stay authoritative. * fix(approvals): normalize exec policy enums and escape host at creation security/ask are closed enums — arbitrary strings now null out via the canonical normalizers instead of reaching reviewer meta rows; host gets the display escape (identity for valid values). nodeId/agentId/sessionKey stay raw by design: they are matched against the node registry and session routing, noted inline.
This commit is contained in:
committed by
GitHub
parent
8cd749f2ed
commit
a96d1359dd
@@ -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<PluginApprovalRequestPayload>();
|
||||
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: "Deployyolped",
|
||||
description: "safetext",
|
||||
toolName: "toolrun",
|
||||
agentId: "agentx",
|
||||
}),
|
||||
]);
|
||||
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<PluginApprovalRequestPayload>({
|
||||
validateAgentRuntimeDelegatedAuthority: () => true,
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<string, unknown>).cwd = "/tmp/safeevil";
|
||||
(opts.params as Record<string, unknown>).resolvedPath = "/usr/bin/echox";
|
||||
// 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<string, unknown>).security = "fulllooks-deny";
|
||||
(opts.params as Record<string, unknown>).ask = "alwaysish";
|
||||
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({
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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: "Deployyolped",
|
||||
description: "safetext",
|
||||
// Passes the protocol's 16,384 raw cap but expands past it once
|
||||
// invisibles become \u{...} escapes — the storage cap must re-apply.
|
||||
detail: `lineone${"".repeat(2_100)}`,
|
||||
severity: "warning",
|
||||
// Metadata is interpolated into channel approval text lines.
|
||||
pluginId: "plugin",
|
||||
toolName: "toolrun",
|
||||
agentId: "agentx",
|
||||
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, {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user