From 318379cdba1804eb840896f6ebd4dd6dd0fb53cb Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 14 Feb 2026 13:02:48 +0100 Subject: [PATCH] fix(gateway): bind system.run approvals to exec approvals --- CHANGELOG.md | 1 + src/agents/openclaw-tools.camera.e2e.test.ts | 7 + src/agents/tools/nodes-tool.ts | 3 + src/cli/nodes-cli.coverage.test.ts | 2 + src/cli/nodes-cli/register.invoke.ts | 6 + src/gateway/exec-approval-manager.ts | 4 + .../node-invoke-system-run-approval.ts | 202 ++++++++++++++++++ src/gateway/server-methods/exec-approval.ts | 5 +- src/gateway/server-methods/nodes.ts | 23 +- src/gateway/server-methods/types.ts | 2 + src/gateway/server.impl.ts | 1 + ...er.node-invoke-approval-bypass.e2e.test.ts | 184 ++++++++++++++++ 12 files changed, 437 insertions(+), 3 deletions(-) create mode 100644 src/gateway/node-invoke-system-run-approval.ts create mode 100644 src/gateway/server.node-invoke-approval-bypass.e2e.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 05cae16fef..8e8e558a87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,6 +80,7 @@ Docs: https://docs.openclaw.ai - Security/Audit: add misconfiguration checks for sandbox Docker config with sandbox mode off, ineffective `gateway.nodes.denyCommands` entries, global minimal tool-profile overrides by agent profiles, and permissive extension-plugin tool reachability. - Security/Audit: distinguish external webhooks (`hooks.enabled`) from internal hooks (`hooks.internal.enabled`) in attack-surface summaries to avoid false exposure signals when only internal hooks are enabled. (#13474) Thanks @mcaxtr. - Security/Onboarding: clarify multi-user DM isolation remediation with explicit `openclaw config set session.dmScope ...` commands in security audit, doctor security, and channel onboarding guidance. (#13129) Thanks @VintLin. +- Security/Gateway: bind node `system.run` approval overrides to gateway exec-approval records (runId-bound), preventing approval-bypass via `node.invoke` param injection. Thanks @222n5. - Agents/Nodes: harden node exec approval decision handling in the `nodes` tool run path by failing closed on unexpected approval decisions, and add regression coverage for approval-required retry/deny/timeout flows. (#4726) Thanks @rmorse. - Android/Nodes: harden `app.update` by requiring HTTPS and gateway-host URL matching plus SHA-256 verification, stream URL camera downloads to disk with size guards to avoid memory spikes, and stop signing release builds with debug keys. (#13541) Thanks @smartprogrammer93. - Routing: enforce strict binding-scope matching across peer/guild/team/roles so peer-scoped Discord/Slack bindings no longer match unrelated guild/team contexts or fallback tiers. (#15274) Thanks @lailoo. diff --git a/src/agents/openclaw-tools.camera.e2e.test.ts b/src/agents/openclaw-tools.camera.e2e.test.ts index 6411b44362..f9860109b8 100644 --- a/src/agents/openclaw-tools.camera.e2e.test.ts +++ b/src/agents/openclaw-tools.camera.e2e.test.ts @@ -135,6 +135,7 @@ describe("nodes run", () => { it("requests approval and retries with allow-once decision", async () => { let invokeCalls = 0; + let approvalId: string | null = null; callGateway.mockImplementation(async ({ method, params }) => { if (method === "node.list") { return { nodes: [{ nodeId: "mac-1", commands: ["system.run"] }] }; @@ -149,6 +150,7 @@ describe("nodes run", () => { command: "system.run", params: { command: ["echo", "hi"], + runId: approvalId, approved: true, approvalDecision: "allow-once", }, @@ -157,10 +159,15 @@ describe("nodes run", () => { } if (method === "exec.approval.request") { expect(params).toMatchObject({ + id: expect.any(String), command: "echo hi", host: "node", timeoutMs: 120_000, }); + approvalId = + typeof (params as { id?: unknown } | undefined)?.id === "string" + ? ((params as { id: string }).id ?? null) + : null; return { decision: "allow-once" }; } throw new Error(`unexpected method: ${String(method)}`); diff --git a/src/agents/tools/nodes-tool.ts b/src/agents/tools/nodes-tool.ts index 4a1d1b2cdf..3cc0076e7a 100644 --- a/src/agents/tools/nodes-tool.ts +++ b/src/agents/tools/nodes-tool.ts @@ -467,10 +467,12 @@ export function createNodesTool(options?: { // the gateway and wait for the user to approve/deny via the UI. const APPROVAL_TIMEOUT_MS = 120_000; const cmdText = command.join(" "); + const approvalId = crypto.randomUUID(); const approvalResult = await callGatewayTool( "exec.approval.request", { ...gatewayOpts, timeoutMs: APPROVAL_TIMEOUT_MS + 5_000 }, { + id: approvalId, command: cmdText, cwd, host: "node", @@ -502,6 +504,7 @@ export function createNodesTool(options?: { command: "system.run", params: { ...runParams, + runId: approvalId, approved: true, approvalDecision, }, diff --git a/src/cli/nodes-cli.coverage.test.ts b/src/cli/nodes-cli.coverage.test.ts index 44d1087979..b321c6f588 100644 --- a/src/cli/nodes-cli.coverage.test.ts +++ b/src/cli/nodes-cli.coverage.test.ts @@ -128,6 +128,7 @@ describe("nodes-cli coverage", () => { agentId: "main", approved: true, approvalDecision: "allow-once", + runId: expect.any(String), }); expect(invoke?.params?.timeoutMs).toBe(5000); }); @@ -153,6 +154,7 @@ describe("nodes-cli coverage", () => { agentId: "main", approved: true, approvalDecision: "allow-once", + runId: expect.any(String), }); }); diff --git a/src/cli/nodes-cli/register.invoke.ts b/src/cli/nodes-cli/register.invoke.ts index 022907fa85..932391ce15 100644 --- a/src/cli/nodes-cli/register.invoke.ts +++ b/src/cli/nodes-cli/register.invoke.ts @@ -269,8 +269,11 @@ export function registerNodesInvokeCommands(nodes: Command) { } const requiresAsk = hostAsk === "always" || hostAsk === "on-miss"; + let approvalId: string | null = null; if (requiresAsk) { + approvalId = crypto.randomUUID(); const decisionResult = (await callGatewayCli("exec.approval.request", opts, { + id: approvalId, command: rawCommand ?? argv.join(" "), cwd: opts.cwd, host: "node", @@ -330,6 +333,9 @@ export function registerNodesInvokeCommands(nodes: Command) { if (approvalDecision) { (invokeParams.params as Record).approvalDecision = approvalDecision; } + if (approvedByAsk && approvalId) { + (invokeParams.params as Record).runId = approvalId; + } if (invokeTimeout !== undefined) { invokeParams.timeoutMs = invokeTimeout; } diff --git a/src/gateway/exec-approval-manager.ts b/src/gateway/exec-approval-manager.ts index f4e7dd9994..f2203a219a 100644 --- a/src/gateway/exec-approval-manager.ts +++ b/src/gateway/exec-approval-manager.ts @@ -20,6 +20,10 @@ export type ExecApprovalRecord = { request: ExecApprovalRequestPayload; createdAtMs: number; expiresAtMs: number; + // Caller metadata (best-effort). Used to prevent other clients from replaying an approval id. + requestedByConnId?: string | null; + requestedByDeviceId?: string | null; + requestedByClientId?: string | null; resolvedAtMs?: number; decision?: ExecApprovalDecision; resolvedBy?: string | null; diff --git a/src/gateway/node-invoke-system-run-approval.ts b/src/gateway/node-invoke-system-run-approval.ts new file mode 100644 index 0000000000..fa19577280 --- /dev/null +++ b/src/gateway/node-invoke-system-run-approval.ts @@ -0,0 +1,202 @@ +import type { ExecApprovalManager, ExecApprovalRecord } from "./exec-approval-manager.js"; +import type { GatewayClient } from "./server-methods/types.js"; + +type SystemRunParamsLike = { + command?: unknown; + rawCommand?: unknown; + cwd?: unknown; + env?: unknown; + timeoutMs?: unknown; + needsScreenRecording?: unknown; + agentId?: unknown; + sessionKey?: unknown; + approved?: unknown; + approvalDecision?: unknown; + runId?: unknown; +}; + +function asRecord(value: unknown): Record | null { + if (!value || typeof value !== "object" || Array.isArray(value)) { + return null; + } + return value as Record; +} + +function normalizeString(value: unknown): string | null { + if (typeof value !== "string") { + return null; + } + const trimmed = value.trim(); + return trimmed ? trimmed : null; +} + +function normalizeApprovalDecision(value: unknown): "allow-once" | "allow-always" | null { + const s = normalizeString(value); + return s === "allow-once" || s === "allow-always" ? s : null; +} + +function clientHasApprovals(client: GatewayClient | null): boolean { + const scopes = Array.isArray(client?.connect?.scopes) ? client?.connect?.scopes : []; + return scopes.includes("operator.admin") || scopes.includes("operator.approvals"); +} + +function getCmdText(params: SystemRunParamsLike): string { + const raw = normalizeString(params.rawCommand); + if (raw) { + return raw; + } + if (Array.isArray(params.command)) { + const parts = params.command.map((v) => String(v)); + if (parts.length > 0) { + return parts.join(" "); + } + } + return ""; +} + +function approvalMatchesRequest(params: SystemRunParamsLike, record: ExecApprovalRecord): boolean { + if (record.request.host !== "node") { + return false; + } + + const cmdText = getCmdText(params); + if (!cmdText || record.request.command !== cmdText) { + return false; + } + + const reqCwd = record.request.cwd ?? null; + const runCwd = normalizeString(params.cwd) ?? null; + if (reqCwd !== runCwd) { + return false; + } + + const reqAgentId = record.request.agentId ?? null; + const runAgentId = normalizeString(params.agentId) ?? null; + if (reqAgentId !== runAgentId) { + return false; + } + + const reqSessionKey = record.request.sessionKey ?? null; + const runSessionKey = normalizeString(params.sessionKey) ?? null; + if (reqSessionKey !== runSessionKey) { + return false; + } + + return true; +} + +/** + * Gate `system.run` approval flags (`approved`, `approvalDecision`) behind a real + * `exec.approval.*` record. This prevents users with only `operator.write` from + * bypassing node-host approvals by injecting control fields into `node.invoke`. + */ +export function sanitizeSystemRunParamsForForwarding(opts: { + rawParams: unknown; + client: GatewayClient | null; + execApprovalManager?: ExecApprovalManager; + nowMs?: number; +}): + | { ok: true; params: unknown } + | { ok: false; message: string; details?: Record } { + const obj = asRecord(opts.rawParams); + if (!obj) { + return { ok: true, params: opts.rawParams }; + } + + const p = obj as SystemRunParamsLike; + const approved = p.approved === true; + const requestedDecision = normalizeApprovalDecision(p.approvalDecision); + const wantsApprovalOverride = approved || requestedDecision !== null; + + // Always strip control fields from user input. If the override is allowed, + // we re-add trusted fields based on the gateway approval record. + const next: Record = { ...obj }; + delete next.approved; + delete next.approvalDecision; + + if (!wantsApprovalOverride) { + return { ok: true, params: next }; + } + + const runId = normalizeString(p.runId); + if (!runId) { + return { + ok: false, + message: "approval override requires params.runId", + details: { code: "MISSING_RUN_ID" }, + }; + } + + const manager = opts.execApprovalManager; + if (!manager) { + return { + ok: false, + message: "exec approvals unavailable", + details: { code: "APPROVALS_UNAVAILABLE" }, + }; + } + + const snapshot = manager.getSnapshot(runId); + if (!snapshot) { + return { + ok: false, + message: "unknown or expired approval id", + details: { code: "UNKNOWN_APPROVAL_ID", runId }, + }; + } + + const nowMs = typeof opts.nowMs === "number" ? opts.nowMs : Date.now(); + if (nowMs > snapshot.expiresAtMs) { + return { + ok: false, + message: "approval expired", + details: { code: "APPROVAL_EXPIRED", runId }, + }; + } + + if (snapshot.requestedByConnId && snapshot.requestedByConnId !== (opts.client?.connId ?? null)) { + return { + ok: false, + message: "approval id not valid for this client", + details: { code: "APPROVAL_CLIENT_MISMATCH", runId }, + }; + } + + if (!approvalMatchesRequest(p, snapshot)) { + return { + ok: false, + message: "approval id does not match request", + details: { code: "APPROVAL_REQUEST_MISMATCH", runId }, + }; + } + + // Normal path: enforce the decision recorded by the gateway. + if (snapshot.decision === "allow-once" || snapshot.decision === "allow-always") { + next.approved = true; + next.approvalDecision = snapshot.decision; + return { ok: true, params: next }; + } + + // If the approval request timed out (decision=null), allow askFallback-driven + // "allow-once" ONLY for clients that are allowed to use exec approvals. + const timedOut = + snapshot.resolvedAtMs !== undefined && + snapshot.decision === undefined && + snapshot.resolvedBy === null; + if ( + timedOut && + approved && + requestedDecision === "allow-once" && + clientHasApprovals(opts.client) + ) { + next.approved = true; + next.approvalDecision = "allow-once"; + return { ok: true, params: next }; + } + + return { + ok: false, + message: "approval required", + details: { code: "APPROVAL_REQUIRED", runId }, + }; +} diff --git a/src/gateway/server-methods/exec-approval.ts b/src/gateway/server-methods/exec-approval.ts index f88e0d6a0b..100982d6ed 100644 --- a/src/gateway/server-methods/exec-approval.ts +++ b/src/gateway/server-methods/exec-approval.ts @@ -15,7 +15,7 @@ export function createExecApprovalHandlers( opts?: { forwarder?: ExecApprovalForwarder }, ): GatewayRequestHandlers { return { - "exec.approval.request": async ({ params, respond, context }) => { + "exec.approval.request": async ({ params, respond, context, client }) => { if (!validateExecApprovalRequestParams(params)) { respond( false, @@ -64,6 +64,9 @@ export function createExecApprovalHandlers( sessionKey: p.sessionKey ?? null, }; const record = manager.create(request, timeoutMs, explicitId); + record.requestedByConnId = client?.connId ?? null; + record.requestedByDeviceId = client?.connect?.device?.id ?? null; + record.requestedByClientId = client?.connect?.client?.id ?? null; // Use register() to synchronously add to pending map before sending any response. // This ensures the approval ID is valid immediately after the "accepted" response. let decisionPromise: Promise< diff --git a/src/gateway/server-methods/nodes.ts b/src/gateway/server-methods/nodes.ts index b4ad29ba4c..c8c9957df5 100644 --- a/src/gateway/server-methods/nodes.ts +++ b/src/gateway/server-methods/nodes.ts @@ -10,6 +10,7 @@ import { verifyNodeToken, } from "../../infra/node-pairing.js"; import { isNodeCommandAllowed, resolveNodeCommandAllowlist } from "../node-command-policy.js"; +import { sanitizeSystemRunParamsForForwarding } from "../node-invoke-system-run-approval.js"; import { ErrorCodes, errorShape, @@ -361,7 +362,7 @@ export const nodeHandlers: GatewayRequestHandlers = { ); }); }, - "node.invoke": async ({ params, respond, context }) => { + "node.invoke": async ({ params, respond, context, client }) => { if (!validateNodeInvokeParams(params)) { respondInvalidParams({ respond, @@ -417,10 +418,28 @@ export const nodeHandlers: GatewayRequestHandlers = { ); return; } + const forwardedParams = + command === "system.run" + ? sanitizeSystemRunParamsForForwarding({ + rawParams: p.params, + client, + execApprovalManager: context.execApprovalManager, + }) + : ({ ok: true, params: p.params } as const); + if (!forwardedParams.ok) { + respond( + false, + undefined, + errorShape(ErrorCodes.INVALID_REQUEST, forwardedParams.message, { + details: forwardedParams.details ?? null, + }), + ); + return; + } const res = await context.nodeRegistry.invoke({ nodeId, command, - params: p.params, + params: forwardedParams.params, timeoutMs: p.timeoutMs, idempotencyKey: p.idempotencyKey, }); diff --git a/src/gateway/server-methods/types.ts b/src/gateway/server-methods/types.ts index aa26b232f1..1b7b34339e 100644 --- a/src/gateway/server-methods/types.ts +++ b/src/gateway/server-methods/types.ts @@ -5,6 +5,7 @@ import type { CronService } from "../../cron/service.js"; import type { createSubsystemLogger } from "../../logging/subsystem.js"; import type { WizardSession } from "../../wizard/session.js"; import type { ChatAbortControllerEntry } from "../chat-abort.js"; +import type { ExecApprovalManager } from "../exec-approval-manager.js"; import type { NodeRegistry } from "../node-registry.js"; import type { ConnectParams, ErrorShape, RequestFrame } from "../protocol/index.js"; import type { ChannelRuntimeSnapshot } from "../server-channels.js"; @@ -28,6 +29,7 @@ export type GatewayRequestContext = { deps: ReturnType; cron: CronService; cronStorePath: string; + execApprovalManager?: ExecApprovalManager; loadGatewayModelCatalog: () => Promise; getHealthCache: () => HealthSummary | null; refreshHealthSnapshot: (opts?: { probe?: boolean }) => Promise; diff --git a/src/gateway/server.impl.ts b/src/gateway/server.impl.ts index b44ec5468f..bf6746ef19 100644 --- a/src/gateway/server.impl.ts +++ b/src/gateway/server.impl.ts @@ -565,6 +565,7 @@ export async function startGatewayServer( deps, cron, cronStorePath, + execApprovalManager, loadGatewayModelCatalog, getHealthCache, refreshHealthSnapshot: refreshGatewayHealthSnapshot, diff --git a/src/gateway/server.node-invoke-approval-bypass.e2e.test.ts b/src/gateway/server.node-invoke-approval-bypass.e2e.test.ts new file mode 100644 index 0000000000..ac97616058 --- /dev/null +++ b/src/gateway/server.node-invoke-approval-bypass.e2e.test.ts @@ -0,0 +1,184 @@ +import crypto from "node:crypto"; +import { afterAll, beforeAll, describe, expect, test } from "vitest"; +import { WebSocket } from "ws"; +import { sleep } from "../utils.js"; +import { GATEWAY_CLIENT_MODES, GATEWAY_CLIENT_NAMES } from "../utils/message-channel.js"; +import { GatewayClient } from "./client.js"; +import { + connectReq, + installGatewayTestHooks, + rpcReq, + startServerWithClient, +} from "./test-helpers.js"; + +installGatewayTestHooks({ scope: "suite" }); + +describe("node.invoke approval bypass", () => { + let server: Awaited>["server"]; + let port: number; + + beforeAll(async () => { + const started = await startServerWithClient("secret", { controlUiEnabled: true }); + server = started.server; + port = started.port; + }); + + afterAll(async () => { + await server.close(); + }); + + const connectOperator = async (scopes: string[]) => { + const ws = new WebSocket(`ws://127.0.0.1:${port}`); + await new Promise((resolve) => ws.once("open", resolve)); + const res = await connectReq(ws, { token: "secret", scopes }); + expect(res.ok).toBe(true); + return ws; + }; + + const connectLinuxNode = async (onInvoke: (payload: unknown) => void) => { + let readyResolve: (() => void) | null = null; + const ready = new Promise((resolve) => { + readyResolve = resolve; + }); + + const client = new GatewayClient({ + url: `ws://127.0.0.1:${port}`, + connectDelayMs: 0, + token: "secret", + role: "node", + clientName: GATEWAY_CLIENT_NAMES.NODE_HOST, + clientVersion: "1.0.0", + platform: "linux", + mode: GATEWAY_CLIENT_MODES.NODE, + scopes: [], + commands: ["system.run"], + onHelloOk: () => readyResolve?.(), + onEvent: (evt) => { + if (evt.event !== "node.invoke.request") { + return; + } + onInvoke(evt.payload); + const payload = evt.payload as { + id?: string; + nodeId?: string; + }; + const id = typeof payload?.id === "string" ? payload.id : ""; + const nodeId = typeof payload?.nodeId === "string" ? payload.nodeId : ""; + if (!id || !nodeId) { + return; + } + void client.request("node.invoke.result", { + id, + nodeId, + ok: true, + payloadJSON: JSON.stringify({ ok: true }), + }); + }, + }); + client.start(); + await Promise.race([ + ready, + sleep(10_000).then(() => { + throw new Error("timeout waiting for node to connect"); + }), + ]); + return client; + }; + + test("rejects injecting approved/approvalDecision without approval id", async () => { + let sawInvoke = false; + const node = await connectLinuxNode(() => { + sawInvoke = true; + }); + const ws = await connectOperator(["operator.write"]); + + const nodes = await rpcReq<{ nodes?: Array<{ nodeId: string; connected?: boolean }> }>( + ws, + "node.list", + {}, + ); + expect(nodes.ok).toBe(true); + const nodeId = nodes.payload?.nodes?.find((n) => n.connected)?.nodeId ?? ""; + expect(nodeId).toBeTruthy(); + + const res = await rpcReq(ws, "node.invoke", { + nodeId, + command: "system.run", + params: { + command: ["echo", "hi"], + rawCommand: "echo hi", + approved: true, + approvalDecision: "allow-once", + }, + idempotencyKey: crypto.randomUUID(), + }); + expect(res.ok).toBe(false); + expect(res.error?.message ?? "").toContain("params.runId"); + + // Ensure the node didn't receive the invoke (gateway should fail early). + await sleep(50); + expect(sawInvoke).toBe(false); + + ws.close(); + node.stop(); + }); + + test("binds system.run approval flags to exec.approval decision (ignores caller escalation)", async () => { + let lastInvokeParams: Record | null = null; + const node = await connectLinuxNode((payload) => { + const obj = payload as { paramsJSON?: unknown }; + const raw = typeof obj?.paramsJSON === "string" ? obj.paramsJSON : ""; + if (!raw) { + lastInvokeParams = null; + return; + } + lastInvokeParams = JSON.parse(raw) as Record; + }); + + const ws = await connectOperator(["operator.write", "operator.approvals"]); + + const nodes = await rpcReq<{ nodes?: Array<{ nodeId: string; connected?: boolean }> }>( + ws, + "node.list", + {}, + ); + expect(nodes.ok).toBe(true); + const nodeId = nodes.payload?.nodes?.find((n) => n.connected)?.nodeId ?? ""; + expect(nodeId).toBeTruthy(); + + const approvalId = crypto.randomUUID(); + const requestP = rpcReq(ws, "exec.approval.request", { + id: approvalId, + command: "echo hi", + cwd: null, + host: "node", + timeoutMs: 30_000, + }); + + await rpcReq(ws, "exec.approval.resolve", { id: approvalId, decision: "allow-once" }); + const requested = await requestP; + expect(requested.ok).toBe(true); + + const invoke = await rpcReq(ws, "node.invoke", { + nodeId, + command: "system.run", + params: { + command: ["echo", "hi"], + rawCommand: "echo hi", + runId: approvalId, + approved: true, + // Try to escalate to allow-always; gateway should clamp to allow-once from record. + approvalDecision: "allow-always", + }, + idempotencyKey: crypto.randomUUID(), + }); + expect(invoke.ok).toBe(true); + + expect(lastInvokeParams).toBeTruthy(); + expect(lastInvokeParams?.approved).toBe(true); + expect(lastInvokeParams?.approvalDecision).toBe("allow-once"); + + ws.close(); + node.stop(); + }); +});