diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a712c74de6..03b2e4ecaae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ Docs: https://docs.openclaw.ai ### Fixes - Security/Shell env fallback: remove trusted-prefix shell-path fallback and only trust login shells explicitly registered in `/etc/shells`, defaulting to `/bin/sh` when `SHELL` is not registered. This ships in the next npm release. Thanks @tdjackey for reporting. +- Security/Exec approvals: bind `host=node` approvals to explicit `nodeId`, reject cross-node replay of approved `system.run` requests, and include the target node in approval prompts. This ships in the next npm release. Thanks @tdjackey for reporting. - Security/Voice Call: harden Twilio webhook replay handling by preserving provider event IDs through normalization, adding bounded replay dedupe, and enforcing per-call turn-token matching for call-state transitions. This ships in the next npm release. Thanks @jiseoung for reporting. - Security/Export session HTML: escape raw HTML markdown tokens in the exported session viewer, harden tree/header metadata rendering against HTML injection, and sanitize image data-URL MIME types in export output to prevent stored XSS when opening exported HTML files. This ships in the next npm release. Thanks @allsmog for reporting. - Security/iOS deep links: require local confirmation (or trusted key) before forwarding `openclaw://agent` requests from iOS to gateway `agent.request`, and strip unkeyed delivery-routing fields to reduce exfiltration risk. This ships in the next npm release. Thanks @GCXWLP for reporting. diff --git a/src/agents/bash-tools.exec-approval-request.test.ts b/src/agents/bash-tools.exec-approval-request.test.ts index 35f5e040869..a0722002c64 100644 --- a/src/agents/bash-tools.exec-approval-request.test.ts +++ b/src/agents/bash-tools.exec-approval-request.test.ts @@ -44,6 +44,7 @@ describe("requestExecApprovalDecision", () => { id: "approval-id", command: "echo hi", cwd: "/tmp", + nodeId: undefined, host: "gateway", security: "allowlist", ask: "always", @@ -62,6 +63,7 @@ describe("requestExecApprovalDecision", () => { id: "approval-id", command: "echo hi", cwd: "/tmp", + nodeId: "node-1", host: "node", security: "allowlist", ask: "on-miss", @@ -74,6 +76,7 @@ describe("requestExecApprovalDecision", () => { id: "approval-id-2", command: "echo hi", cwd: "/tmp", + nodeId: "node-1", host: "node", security: "allowlist", ask: "on-miss", diff --git a/src/agents/bash-tools.exec-approval-request.ts b/src/agents/bash-tools.exec-approval-request.ts index 2b08495a400..7f0b59736d5 100644 --- a/src/agents/bash-tools.exec-approval-request.ts +++ b/src/agents/bash-tools.exec-approval-request.ts @@ -9,6 +9,7 @@ export type RequestExecApprovalDecisionParams = { id: string; command: string; cwd: string; + nodeId?: string; host: "gateway" | "node"; security: ExecSecurity; ask: ExecAsk; @@ -27,6 +28,7 @@ export async function requestExecApprovalDecision( id: params.id, command: params.command, cwd: params.cwd, + nodeId: params.nodeId, host: params.host, security: params.security, ask: params.ask, @@ -48,6 +50,7 @@ export async function requestExecApprovalDecisionForHost(params: { command: string; workdir: string; host: "gateway" | "node"; + nodeId?: string; security: ExecSecurity; ask: ExecAsk; agentId?: string; @@ -58,6 +61,7 @@ export async function requestExecApprovalDecisionForHost(params: { id: params.approvalId, command: params.command, cwd: params.workdir, + nodeId: params.nodeId, host: params.host, security: params.security, ask: params.ask, diff --git a/src/agents/bash-tools.exec-host-node.ts b/src/agents/bash-tools.exec-host-node.ts index 9a663c2a088..fc6893b93bf 100644 --- a/src/agents/bash-tools.exec-host-node.ts +++ b/src/agents/bash-tools.exec-host-node.ts @@ -193,6 +193,7 @@ export async function executeNodeHostCommand( command: params.command, workdir: params.workdir, host: "node", + nodeId, security: hostSecurity, ask: hostAsk, agentId: params.agentId, diff --git a/src/agents/openclaw-tools.camera.test.ts b/src/agents/openclaw-tools.camera.test.ts index fb927d33888..3082c849609 100644 --- a/src/agents/openclaw-tools.camera.test.ts +++ b/src/agents/openclaw-tools.camera.test.ts @@ -165,6 +165,7 @@ describe("nodes run", () => { expect(params).toMatchObject({ id: expect.any(String), command: "echo hi", + nodeId: NODE_ID, host: "node", timeoutMs: 120_000, }); diff --git a/src/agents/tools/nodes-tool.ts b/src/agents/tools/nodes-tool.ts index 3188d7dc1b8..c17ff9f9c48 100644 --- a/src/agents/tools/nodes-tool.ts +++ b/src/agents/tools/nodes-tool.ts @@ -482,6 +482,7 @@ export function createNodesTool(options?: { id: approvalId, command: cmdText, cwd, + nodeId, host: "node", agentId, sessionKey, diff --git a/src/cli/nodes-cli/register.invoke.ts b/src/cli/nodes-cli/register.invoke.ts index 2a7ec004f84..a53cc783041 100644 --- a/src/cli/nodes-cli/register.invoke.ts +++ b/src/cli/nodes-cli/register.invoke.ts @@ -253,6 +253,7 @@ export function registerNodesInvokeCommands(nodes: Command) { id: approvalId, command: rawCommand ?? argv.join(" "), cwd: opts.cwd, + nodeId, host: "node", security: hostSecurity, ask: hostAsk, diff --git a/src/gateway/exec-approval-manager.ts b/src/gateway/exec-approval-manager.ts index 8a2828da970..a065be1916a 100644 --- a/src/gateway/exec-approval-manager.ts +++ b/src/gateway/exec-approval-manager.ts @@ -7,6 +7,7 @@ const RESOLVED_ENTRY_GRACE_MS = 15_000; export type ExecApprovalRequestPayload = { command: string; cwd?: string | null; + nodeId?: string | null; host?: string | null; security?: string | null; ask?: string | null; diff --git a/src/gateway/node-invoke-sanitize.ts b/src/gateway/node-invoke-sanitize.ts index c794405ddea..651399dce08 100644 --- a/src/gateway/node-invoke-sanitize.ts +++ b/src/gateway/node-invoke-sanitize.ts @@ -3,6 +3,7 @@ import { sanitizeSystemRunParamsForForwarding } from "./node-invoke-system-run-a import type { GatewayClient } from "./server-methods/types.js"; export function sanitizeNodeInvokeParamsForForwarding(opts: { + nodeId: string; command: string; rawParams: unknown; client: GatewayClient | null; @@ -12,6 +13,7 @@ export function sanitizeNodeInvokeParamsForForwarding(opts: { | { ok: false; message: string; details?: Record } { if (opts.command === "system.run") { return sanitizeSystemRunParamsForForwarding({ + nodeId: opts.nodeId, rawParams: opts.rawParams, client: opts.client, execApprovalManager: opts.execApprovalManager, diff --git a/src/gateway/node-invoke-system-run-approval.test.ts b/src/gateway/node-invoke-system-run-approval.test.ts index a5a7c3d9f0d..ddae856048b 100644 --- a/src/gateway/node-invoke-system-run-approval.test.ts +++ b/src/gateway/node-invoke-system-run-approval.test.ts @@ -18,6 +18,7 @@ describe("sanitizeSystemRunParamsForForwarding", () => { id: "approval-1", request: { host: "node", + nodeId: "node-1", command, cwd: null, agentId: null, @@ -61,6 +62,7 @@ describe("sanitizeSystemRunParamsForForwarding", () => { approved: true, approvalDecision: "allow-once", }, + nodeId: "node-1", client, execApprovalManager: manager(makeRecord("echo")), nowMs: now, @@ -82,6 +84,7 @@ describe("sanitizeSystemRunParamsForForwarding", () => { approved: true, approvalDecision: "allow-once", }, + nodeId: "node-1", client, execApprovalManager: manager(makeRecord("echo SAFE&&whoami")), nowMs: now, @@ -97,6 +100,7 @@ describe("sanitizeSystemRunParamsForForwarding", () => { approved: true, approvalDecision: "allow-once", }, + nodeId: "node-1", client, execApprovalManager: manager(makeRecord("echo SAFE")), nowMs: now, @@ -117,6 +121,7 @@ describe("sanitizeSystemRunParamsForForwarding", () => { approved: true, approvalDecision: "allow-once", }, + nodeId: "node-1", client, execApprovalManager: manager( makeRecord('/usr/bin/env BASH_ENV=/tmp/payload.sh bash -lc "echo SAFE"'), @@ -125,4 +130,48 @@ describe("sanitizeSystemRunParamsForForwarding", () => { }); expectAllowOnceForwardingResult(result); }); + + test("rejects approval ids that do not bind a nodeId", () => { + const record = makeRecord("echo SAFE"); + record.request.nodeId = null; + const result = sanitizeSystemRunParamsForForwarding({ + rawParams: { + command: ["echo", "SAFE"], + runId: "approval-1", + approved: true, + approvalDecision: "allow-once", + }, + nodeId: "node-1", + client, + execApprovalManager: manager(record), + nowMs: now, + }); + expect(result.ok).toBe(false); + if (result.ok) { + throw new Error("unreachable"); + } + expect(result.message).toContain("missing node binding"); + expect(result.details?.code).toBe("APPROVAL_NODE_BINDING_MISSING"); + }); + + test("rejects approval ids replayed against a different nodeId", () => { + const result = sanitizeSystemRunParamsForForwarding({ + rawParams: { + command: ["echo", "SAFE"], + runId: "approval-1", + approved: true, + approvalDecision: "allow-once", + }, + nodeId: "node-2", + client, + execApprovalManager: manager(makeRecord("echo SAFE")), + nowMs: now, + }); + expect(result.ok).toBe(false); + if (result.ok) { + throw new Error("unreachable"); + } + expect(result.message).toContain("not valid for this node"); + expect(result.details?.code).toBe("APPROVAL_NODE_MISMATCH"); + }); }); diff --git a/src/gateway/node-invoke-system-run-approval.ts b/src/gateway/node-invoke-system-run-approval.ts index 5684f4221f5..5bf31db8fb5 100644 --- a/src/gateway/node-invoke-system-run-approval.ts +++ b/src/gateway/node-invoke-system-run-approval.ts @@ -114,6 +114,7 @@ function pickSystemRunParams(raw: Record): Record 0 ? p.id.trim() : null; + const host = typeof p.host === "string" ? p.host.trim() : ""; + const nodeId = typeof p.nodeId === "string" ? p.nodeId.trim() : ""; + if (host === "node" && !nodeId) { + respond( + false, + undefined, + errorShape(ErrorCodes.INVALID_REQUEST, "nodeId is required for host=node"), + ); + return; + } if (explicitId && manager.getSnapshot(explicitId)) { respond( false, @@ -68,7 +79,8 @@ export function createExecApprovalHandlers( const request = { command: p.command, cwd: p.cwd ?? null, - host: p.host ?? null, + nodeId: host === "node" ? nodeId : null, + host: host || null, security: p.security ?? null, ask: p.ask ?? null, agentId: p.agentId ?? null, diff --git a/src/gateway/server-methods/nodes.ts b/src/gateway/server-methods/nodes.ts index 4f076abd59c..f0221033155 100644 --- a/src/gateway/server-methods/nodes.ts +++ b/src/gateway/server-methods/nodes.ts @@ -698,6 +698,7 @@ export const nodeHandlers: GatewayRequestHandlers = { return; } const forwardedParams = sanitizeNodeInvokeParamsForForwarding({ + nodeId, command, rawParams: p.params, client, diff --git a/src/gateway/server-methods/server-methods.test.ts b/src/gateway/server-methods/server-methods.test.ts index 60349d9c0e4..b19a6d8c608 100644 --- a/src/gateway/server-methods/server-methods.test.ts +++ b/src/gateway/server-methods/server-methods.test.ts @@ -248,6 +248,7 @@ describe("exec approval handlers", () => { const defaultExecApprovalRequestParams = { command: "echo ok", cwd: "/tmp", + nodeId: "node-1", host: "node", timeoutMs: 2000, } as const; @@ -323,6 +324,7 @@ describe("exec approval handlers", () => { const params = { command: "echo hi", cwd: "/tmp", + nodeId: "node-1", host: "node", }; expect(validateExecApprovalRequestParams(params)).toBe(true); @@ -332,6 +334,7 @@ describe("exec approval handlers", () => { const params = { command: "echo hi", cwd: "/tmp", + nodeId: "node-1", host: "node", resolvedPath: "/usr/bin/echo", }; @@ -342,6 +345,7 @@ describe("exec approval handlers", () => { const params = { command: "echo hi", cwd: "/tmp", + nodeId: "node-1", host: "node", resolvedPath: undefined, }; @@ -352,6 +356,7 @@ describe("exec approval handlers", () => { const params = { command: "echo hi", cwd: "/tmp", + nodeId: "node-1", host: "node", resolvedPath: null, }; @@ -359,6 +364,25 @@ describe("exec approval handlers", () => { }); }); + it("rejects host=node approval requests without nodeId", async () => { + const { handlers, respond, context } = createExecApprovalFixture(); + await requestExecApproval({ + handlers, + respond, + context, + params: { + nodeId: undefined, + }, + }); + expect(respond).toHaveBeenCalledWith( + false, + undefined, + expect.objectContaining({ + message: "nodeId is required for host=node", + }), + ); + }); + it("broadcasts request + resolve", async () => { const { handlers, broadcasts, respond, context } = createExecApprovalFixture(); diff --git a/src/gateway/server.node-invoke-approval-bypass.test.ts b/src/gateway/server.node-invoke-approval-bypass.test.ts index 9a78453a199..7cc84b5b8d8 100644 --- a/src/gateway/server.node-invoke-approval-bypass.test.ts +++ b/src/gateway/server.node-invoke-approval-bypass.test.ts @@ -3,6 +3,7 @@ import { afterAll, beforeAll, describe, expect, test } from "vitest"; import { WebSocket } from "ws"; import { deriveDeviceIdFromPublicKey, + type DeviceIdentity, publicKeyRawBase64UrlFromPem, signDevicePayload, } from "../infra/device-identity.js"; @@ -23,6 +24,22 @@ installGatewayTestHooks({ scope: "suite" }); const NODE_CONNECT_TIMEOUT_MS = 3_000; const CONNECT_REQ_TIMEOUT_MS = 2_000; +function createDeviceIdentity(): DeviceIdentity { + const { publicKey, privateKey } = crypto.generateKeyPairSync("ed25519"); + const publicKeyPem = publicKey.export({ type: "spki", format: "pem" }).toString(); + const privateKeyPem = privateKey.export({ type: "pkcs8", format: "pem" }).toString(); + const publicKeyRaw = publicKeyRawBase64UrlFromPem(publicKeyPem); + const deviceId = deriveDeviceIdFromPublicKey(publicKeyRaw); + if (!deviceId) { + throw new Error("failed to create test device identity"); + } + return { + deviceId, + publicKeyPem, + privateKeyPem, + }; +} + async function expectNoForwardedInvoke(hasInvoke: () => boolean): Promise { // Yield a couple of macrotasks so any accidental async forwarding would fire. await new Promise((resolve) => setImmediate(resolve)); @@ -42,11 +59,26 @@ async function getConnectedNodeId(ws: WebSocket): Promise { return nodeId; } -async function requestAllowOnceApproval(ws: WebSocket, command: string): Promise { +async function getConnectedNodeIds(ws: WebSocket): Promise { + const nodes = await rpcReq<{ nodes?: Array<{ nodeId: string; connected?: boolean }> }>( + ws, + "node.list", + {}, + ); + expect(nodes.ok).toBe(true); + return (nodes.payload?.nodes ?? []).filter((n) => n.connected).map((n) => n.nodeId); +} + +async function requestAllowOnceApproval( + ws: WebSocket, + command: string, + nodeId: string, +): Promise { const approvalId = crypto.randomUUID(); const requestP = rpcReq(ws, "exec.approval.request", { id: approvalId, command, + nodeId, cwd: null, host: "node", timeoutMs: 30_000, @@ -161,7 +193,10 @@ describe("node.invoke approval bypass", () => { }); }; - const connectLinuxNode = async (onInvoke: (payload: unknown) => void) => { + const connectLinuxNode = async ( + onInvoke: (payload: unknown) => void, + deviceIdentity?: DeviceIdentity, + ) => { let readyResolve: (() => void) | null = null; const ready = new Promise((resolve) => { readyResolve = resolve; @@ -180,6 +215,7 @@ describe("node.invoke approval bypass", () => { mode: GATEWAY_CLIENT_MODES.NODE, scopes: [], commands: ["system.run"], + deviceIdentity, onHelloOk: () => readyResolve?.(), onEvent: (evt) => { if (evt.event !== "node.invoke.request") { @@ -295,7 +331,7 @@ describe("node.invoke approval bypass", () => { try { const nodeId = await getConnectedNodeId(wsApprover); - const approvalId = await requestAllowOnceApproval(wsApprover, "echo hi"); + const approvalId = await requestAllowOnceApproval(wsApprover, "echo hi", nodeId); // Separate caller connection simulates per-call clients. const invoke = await rpcReq(wsCaller, "node.invoke", { nodeId, @@ -316,7 +352,7 @@ describe("node.invoke approval bypass", () => { expect(lastInvokeParams?.["approvalDecision"]).toBe("allow-once"); expect(lastInvokeParams?.["injected"]).toBeUndefined(); - const replayApprovalId = await requestAllowOnceApproval(wsApprover, "echo hi"); + const replayApprovalId = await requestAllowOnceApproval(wsApprover, "echo hi", nodeId); const invokeCountBeforeReplay = invokeCount; const replay = await rpcReq(wsOtherDevice, "node.invoke", { nodeId, @@ -340,4 +376,63 @@ describe("node.invoke approval bypass", () => { node.stop(); } }); + + test("blocks cross-node replay on same device", async () => { + const invokeCounts = new Map(); + const onInvoke = (payload: unknown) => { + const obj = payload as { nodeId?: unknown }; + const nodeId = typeof obj?.nodeId === "string" ? obj.nodeId : ""; + if (!nodeId) { + return; + } + invokeCounts.set(nodeId, (invokeCounts.get(nodeId) ?? 0) + 1); + }; + const nodeA = await connectLinuxNode(onInvoke, createDeviceIdentity()); + const nodeB = await connectLinuxNode(onInvoke, createDeviceIdentity()); + + const wsApprover = await connectOperator(["operator.write", "operator.approvals"]); + const wsCaller = await connectOperator(["operator.write"]); + + try { + await expect + .poll(async () => (await getConnectedNodeIds(wsApprover)).length, { + timeout: 3_000, + interval: 50, + }) + .toBeGreaterThanOrEqual(2); + const connectedNodeIds = await getConnectedNodeIds(wsApprover); + const approvedNodeId = connectedNodeIds[0] ?? ""; + const replayNodeId = connectedNodeIds.find((id) => id !== approvedNodeId) ?? ""; + expect(approvedNodeId).toBeTruthy(); + expect(replayNodeId).toBeTruthy(); + + const approvalId = await requestAllowOnceApproval(wsApprover, "echo hi", approvedNodeId); + const beforeReplayApprovedNode = invokeCounts.get(approvedNodeId) ?? 0; + const beforeReplayOtherNode = invokeCounts.get(replayNodeId) ?? 0; + const replay = await rpcReq(wsCaller, "node.invoke", { + nodeId: replayNodeId, + command: "system.run", + params: { + command: ["echo", "hi"], + rawCommand: "echo hi", + runId: approvalId, + approved: true, + approvalDecision: "allow-once", + }, + idempotencyKey: crypto.randomUUID(), + }); + expect(replay.ok).toBe(false); + expect(replay.error?.message ?? "").toContain("not valid for this node"); + await expectNoForwardedInvoke( + () => + (invokeCounts.get(approvedNodeId) ?? 0) > beforeReplayApprovedNode || + (invokeCounts.get(replayNodeId) ?? 0) > beforeReplayOtherNode, + ); + } finally { + wsApprover.close(); + wsCaller.close(); + nodeA.stop(); + nodeB.stop(); + } + }); }); diff --git a/src/infra/exec-approval-forwarder.ts b/src/infra/exec-approval-forwarder.ts index 46617f07d7d..7af7489baf2 100644 --- a/src/infra/exec-approval-forwarder.ts +++ b/src/infra/exec-approval-forwarder.ts @@ -168,6 +168,9 @@ function buildRequestMessage(request: ExecApprovalRequest, nowMs: number) { if (request.request.cwd) { lines.push(`CWD: ${request.request.cwd}`); } + if (request.request.nodeId) { + lines.push(`Node: ${request.request.nodeId}`); + } if (request.request.host) { lines.push(`Host: ${request.request.host}`); } diff --git a/src/infra/exec-approvals.ts b/src/infra/exec-approvals.ts index 4fd3f63470d..be4264e22ec 100644 --- a/src/infra/exec-approvals.ts +++ b/src/infra/exec-approvals.ts @@ -16,6 +16,7 @@ export type ExecApprovalRequest = { request: { command: string; cwd?: string | null; + nodeId?: string | null; host?: string | null; security?: string | null; ask?: string | null;