fix(gateway): bind system.run approvals to exec approvals

This commit is contained in:
Peter Steinberger
2026-02-14 13:02:48 +01:00
parent 233483d2b9
commit 318379cdba
12 changed files with 437 additions and 3 deletions

View File

@@ -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.

View File

@@ -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)}`);

View File

@@ -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,
},

View File

@@ -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),
});
});

View File

@@ -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<string, unknown>).approvalDecision = approvalDecision;
}
if (approvedByAsk && approvalId) {
(invokeParams.params as Record<string, unknown>).runId = approvalId;
}
if (invokeTimeout !== undefined) {
invokeParams.timeoutMs = invokeTimeout;
}

View File

@@ -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;

View File

@@ -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<string, unknown> | null {
if (!value || typeof value !== "object" || Array.isArray(value)) {
return null;
}
return value as Record<string, unknown>;
}
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<string, unknown> } {
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<string, unknown> = { ...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 },
};
}

View File

@@ -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<

View File

@@ -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,
});

View File

@@ -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<typeof createDefaultDeps>;
cron: CronService;
cronStorePath: string;
execApprovalManager?: ExecApprovalManager;
loadGatewayModelCatalog: () => Promise<ModelCatalogEntry[]>;
getHealthCache: () => HealthSummary | null;
refreshHealthSnapshot: (opts?: { probe?: boolean }) => Promise<HealthSummary>;

View File

@@ -565,6 +565,7 @@ export async function startGatewayServer(
deps,
cron,
cronStorePath,
execApprovalManager,
loadGatewayModelCatalog,
getHealthCache,
refreshHealthSnapshot: refreshGatewayHealthSnapshot,

View File

@@ -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<ReturnType<typeof startServerWithClient>>["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<void>((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<void>((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<string, unknown> | 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<string, unknown>;
});
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();
});
});