From 03e689fc89bbecbcd02876a95957ef1ad9caa176 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 26 Feb 2026 03:40:42 +0100 Subject: [PATCH] fix(security): bind system.run approvals to argv identity --- CHANGELOG.md | 1 + .../bash-tools.exec-approval-request.ts | 6 ++ src/agents/bash-tools.exec-host-node.ts | 1 + src/agents/tools/nodes-tool.ts | 4 +- src/gateway/exec-approval-manager.ts | 1 + .../node-invoke-system-run-approval.test.ts | 61 ++++++++++++++++++- .../node-invoke-system-run-approval.ts | 17 +++++- src/gateway/protocol/schema/exec-approvals.ts | 1 + src/gateway/server-methods/exec-approval.ts | 5 ++ src/infra/exec-approvals.ts | 1 + src/infra/system-run-command.test.ts | 4 ++ src/infra/system-run-command.ts | 9 ++- 12 files changed, 102 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b55a00e44f2..9874384fd66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ Docs: https://docs.openclaw.ai - Slack/Session threads: prevent oversized parent-session inheritance from silently bricking new thread sessions, surface embedded context-overflow empty-result failures to users, and add configurable `session.parentForkMaxTokens` (default `100000`, `0` disables). (#26912) Thanks @markshields-tl. - Models/Auth probes: map permanent auth failover reasons (`auth_permanent`, for example revoked keys) into probe auth status instead of `unknown`, so `openclaw models status --probe` reports actionable auth failures. (#25754) thanks @rrenamed. - Security/Signal: enforce DM/group authorization before reaction-only notification enqueue so unauthorized senders can no longer inject Signal reaction system events under `dmPolicy`/`groupPolicy`; reaction notifications now require channel access checks first. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. +- Security/Exec approvals: bind `system.run` approval matching to exact argv identity and preserve argv whitespace in rendered command text, preventing trailing-space executable path swaps from reusing a mismatched approval. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. - Security/Discord + Slack reactions: enforce DM policy/allowlist authorization before reaction-event system enqueue in direct messages; Discord reaction handling now also honors DM/group-DM enablement and guild `groupPolicy` channel gating to keep reaction ingress aligned with normal message preflight. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. - Security/Telegram reactions: enforce `dmPolicy`/`allowFrom` and group allowlist authorization on `message_reaction` events before enqueueing reaction system events, preventing unauthorized reaction-triggered input in DMs and groups; ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. - Security/Slack interactions: enforce channel/DM authorization and modal actor binding (`private_metadata.userId`) before enqueueing `block_action`/`view_submission`/`view_closed` system events, with regression coverage for unauthorized senders and missing/mismatched actor metadata. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. diff --git a/src/agents/bash-tools.exec-approval-request.ts b/src/agents/bash-tools.exec-approval-request.ts index 83323845c0c..cda30757e26 100644 --- a/src/agents/bash-tools.exec-approval-request.ts +++ b/src/agents/bash-tools.exec-approval-request.ts @@ -8,6 +8,7 @@ import { callGatewayTool } from "./tools/gateway.js"; export type RequestExecApprovalDecisionParams = { id: string; command: string; + commandArgv?: string[]; cwd: string; nodeId?: string; host: "gateway" | "node"; @@ -62,6 +63,7 @@ export async function registerExecApprovalRequest( { id: params.id, command: params.command, + commandArgv: params.commandArgv, cwd: params.cwd, nodeId: params.nodeId, host: params.host, @@ -116,6 +118,7 @@ export async function requestExecApprovalDecision( export async function requestExecApprovalDecisionForHost(params: { approvalId: string; command: string; + commandArgv?: string[]; workdir: string; host: "gateway" | "node"; nodeId?: string; @@ -128,6 +131,7 @@ export async function requestExecApprovalDecisionForHost(params: { return await requestExecApprovalDecision({ id: params.approvalId, command: params.command, + commandArgv: params.commandArgv, cwd: params.workdir, nodeId: params.nodeId, host: params.host, @@ -142,6 +146,7 @@ export async function requestExecApprovalDecisionForHost(params: { export async function registerExecApprovalRequestForHost(params: { approvalId: string; command: string; + commandArgv?: string[]; workdir: string; host: "gateway" | "node"; nodeId?: string; @@ -154,6 +159,7 @@ export async function registerExecApprovalRequestForHost(params: { return await registerExecApprovalRequest({ id: params.approvalId, command: params.command, + commandArgv: params.commandArgv, cwd: params.workdir, nodeId: params.nodeId, host: params.host, diff --git a/src/agents/bash-tools.exec-host-node.ts b/src/agents/bash-tools.exec-host-node.ts index 5a45c869292..47f2931b980 100644 --- a/src/agents/bash-tools.exec-host-node.ts +++ b/src/agents/bash-tools.exec-host-node.ts @@ -194,6 +194,7 @@ export async function executeNodeHostCommand( const registration = await registerExecApprovalRequestForHost({ approvalId, command: params.command, + commandArgv: argv, workdir: params.workdir, host: "node", nodeId, diff --git a/src/agents/tools/nodes-tool.ts b/src/agents/tools/nodes-tool.ts index c17ff9f9c48..4cfd84dc474 100644 --- a/src/agents/tools/nodes-tool.ts +++ b/src/agents/tools/nodes-tool.ts @@ -18,6 +18,7 @@ import { } from "../../cli/nodes-screen.js"; import { parseDurationMs } from "../../cli/parse-duration.js"; import type { OpenClawConfig } from "../../config/config.js"; +import { formatExecCommand } from "../../infra/system-run-command.js"; import { imageMimeFromFormat } from "../../media/mime.js"; import { resolveSessionAgentId } from "../agent-scope.js"; import { resolveImageSanitizationLimits } from "../image-sanitization.js"; @@ -473,7 +474,7 @@ export function createNodesTool(options?: { // Node requires approval – create a pending approval request on // the gateway and wait for the user to approve/deny via the UI. const APPROVAL_TIMEOUT_MS = 120_000; - const cmdText = command.join(" "); + const cmdText = formatExecCommand(command); const approvalId = crypto.randomUUID(); const approvalResult = await callGatewayTool( "exec.approval.request", @@ -481,6 +482,7 @@ export function createNodesTool(options?: { { id: approvalId, command: cmdText, + commandArgv: command, cwd, nodeId, host: "node", diff --git a/src/gateway/exec-approval-manager.ts b/src/gateway/exec-approval-manager.ts index 5e582d42a03..127d5feae09 100644 --- a/src/gateway/exec-approval-manager.ts +++ b/src/gateway/exec-approval-manager.ts @@ -6,6 +6,7 @@ const RESOLVED_ENTRY_GRACE_MS = 15_000; export type ExecApprovalRequestPayload = { command: string; + commandArgv?: string[] | null; cwd?: string | null; nodeId?: string | null; host?: string | null; diff --git a/src/gateway/node-invoke-system-run-approval.test.ts b/src/gateway/node-invoke-system-run-approval.test.ts index 196b5947f45..c2d3cbe1dfa 100644 --- a/src/gateway/node-invoke-system-run-approval.test.ts +++ b/src/gateway/node-invoke-system-run-approval.test.ts @@ -13,13 +13,14 @@ describe("sanitizeSystemRunParamsForForwarding", () => { }, }; - function makeRecord(command: string): ExecApprovalRecord { + function makeRecord(command: string, commandArgv?: string[] | null): ExecApprovalRecord { return { id: "approval-1", request: { host: "node", nodeId: "node-1", command, + commandArgv: commandArgv ?? null, cwd: null, agentId: null, sessionKey: null, @@ -139,6 +140,64 @@ describe("sanitizeSystemRunParamsForForwarding", () => { }); expectAllowOnceForwardingResult(result); }); + + test("rejects trailing-space argv mismatch against legacy command-only approval", () => { + const result = sanitizeSystemRunParamsForForwarding({ + rawParams: { + command: ["runner "], + runId: "approval-1", + approved: true, + approvalDecision: "allow-once", + }, + nodeId: "node-1", + client, + execApprovalManager: manager(makeRecord("runner")), + nowMs: now, + }); + expect(result.ok).toBe(false); + if (result.ok) { + throw new Error("unreachable"); + } + expect(result.message).toContain("approval id does not match request"); + expect(result.details?.code).toBe("APPROVAL_REQUEST_MISMATCH"); + }); + + test("enforces commandArgv identity when approval includes argv binding", () => { + const result = sanitizeSystemRunParamsForForwarding({ + rawParams: { + command: ["echo", "SAFE"], + runId: "approval-1", + approved: true, + approvalDecision: "allow-once", + }, + nodeId: "node-1", + client, + execApprovalManager: manager(makeRecord("echo SAFE", ["echo SAFE"])), + nowMs: now, + }); + expect(result.ok).toBe(false); + if (result.ok) { + throw new Error("unreachable"); + } + expect(result.message).toContain("approval id does not match request"); + expect(result.details?.code).toBe("APPROVAL_REQUEST_MISMATCH"); + }); + + test("accepts matching commandArgv binding for trailing-space argv", () => { + const result = sanitizeSystemRunParamsForForwarding({ + rawParams: { + command: ["runner "], + runId: "approval-1", + approved: true, + approvalDecision: "allow-once", + }, + nodeId: "node-1", + client, + execApprovalManager: manager(makeRecord('"runner "', ["runner "])), + nowMs: now, + }); + expectAllowOnceForwardingResult(result); + }); test("consumes allow-once approvals and blocks same runId replay", async () => { const approvalManager = new ExecApprovalManager(); const runId = "approval-replay-1"; diff --git a/src/gateway/node-invoke-system-run-approval.ts b/src/gateway/node-invoke-system-run-approval.ts index d5600adf032..9623eb1b518 100644 --- a/src/gateway/node-invoke-system-run-approval.ts +++ b/src/gateway/node-invoke-system-run-approval.ts @@ -55,6 +55,7 @@ function clientHasApprovals(client: ApprovalClient | null): boolean { function approvalMatchesRequest( cmdText: string, + argv: string[], params: SystemRunParamsLike, record: ExecApprovalRecord, ): boolean { @@ -62,7 +63,19 @@ function approvalMatchesRequest( return false; } - if (!cmdText || record.request.command !== cmdText) { + const requestedArgv = Array.isArray(record.request.commandArgv) + ? record.request.commandArgv + : null; + if (requestedArgv) { + if (requestedArgv.length === 0 || requestedArgv.length !== argv.length) { + return false; + } + for (let i = 0; i < requestedArgv.length; i += 1) { + if (requestedArgv[i] !== argv[i]) { + return false; + } + } + } else if (!cmdText || record.request.command !== cmdText) { return false; } @@ -237,7 +250,7 @@ export function sanitizeSystemRunParamsForForwarding(opts: { }; } - if (!approvalMatchesRequest(cmdText, p, snapshot)) { + if (!approvalMatchesRequest(cmdText, cmdTextResolution.argv, p, snapshot)) { return { ok: false, message: "approval id does not match request", diff --git a/src/gateway/protocol/schema/exec-approvals.ts b/src/gateway/protocol/schema/exec-approvals.ts index a7c5fcf09bb..1482ae4cfef 100644 --- a/src/gateway/protocol/schema/exec-approvals.ts +++ b/src/gateway/protocol/schema/exec-approvals.ts @@ -89,6 +89,7 @@ export const ExecApprovalRequestParamsSchema = Type.Object( { id: Type.Optional(NonEmptyString), command: NonEmptyString, + commandArgv: Type.Optional(Type.Union([Type.Array(Type.String()), Type.Null()])), cwd: Type.Optional(Type.Union([Type.String(), Type.Null()])), nodeId: Type.Optional(Type.Union([NonEmptyString, Type.Null()])), host: Type.Optional(Type.Union([Type.String(), Type.Null()])), diff --git a/src/gateway/server-methods/exec-approval.ts b/src/gateway/server-methods/exec-approval.ts index d1cfc9ec0d9..555348bc777 100644 --- a/src/gateway/server-methods/exec-approval.ts +++ b/src/gateway/server-methods/exec-approval.ts @@ -43,6 +43,7 @@ export function createExecApprovalHandlers( const p = params as { id?: string; command: string; + commandArgv?: string[] | null; cwd?: string; nodeId?: string; host?: string; @@ -60,6 +61,9 @@ export function createExecApprovalHandlers( const explicitId = typeof p.id === "string" && p.id.trim().length > 0 ? p.id.trim() : null; const host = typeof p.host === "string" ? p.host.trim() : ""; const nodeId = typeof p.nodeId === "string" ? p.nodeId.trim() : ""; + const commandArgv = Array.isArray(p.commandArgv) + ? p.commandArgv.map((entry) => String(entry)) + : null; if (host === "node" && !nodeId) { respond( false, @@ -78,6 +82,7 @@ export function createExecApprovalHandlers( } const request = { command: p.command, + commandArgv, cwd: p.cwd ?? null, nodeId: host === "node" ? nodeId : null, host: host || null, diff --git a/src/infra/exec-approvals.ts b/src/infra/exec-approvals.ts index be4264e22ec..688972d8361 100644 --- a/src/infra/exec-approvals.ts +++ b/src/infra/exec-approvals.ts @@ -15,6 +15,7 @@ export type ExecApprovalRequest = { id: string; request: { command: string; + commandArgv?: string[] | null; cwd?: string | null; nodeId?: string | null; host?: string | null; diff --git a/src/infra/system-run-command.test.ts b/src/infra/system-run-command.test.ts index 7186823d84b..7f7d4fee96c 100644 --- a/src/infra/system-run-command.test.ts +++ b/src/infra/system-run-command.test.ts @@ -21,6 +21,10 @@ describe("system run command helpers", () => { expect(formatExecCommand(["echo", "hi there"])).toBe('echo "hi there"'); }); + test("formatExecCommand preserves trailing whitespace in argv tokens", () => { + expect(formatExecCommand(["runner "])).toBe('"runner "'); + }); + test("extractShellCommandFromArgv extracts sh -lc command", () => { expect(extractShellCommandFromArgv(["/bin/sh", "-lc", "echo hi"])).toBe("echo hi"); }); diff --git a/src/infra/system-run-command.ts b/src/infra/system-run-command.ts index b03d715fc72..dc54bf7b561 100644 --- a/src/infra/system-run-command.ts +++ b/src/infra/system-run-command.ts @@ -35,15 +35,14 @@ export type ResolvedSystemRunCommand = export function formatExecCommand(argv: string[]): string { return argv .map((arg) => { - const trimmed = arg.trim(); - if (!trimmed) { + if (arg.length === 0) { return '""'; } - const needsQuotes = /\s|"/.test(trimmed); + const needsQuotes = /\s|"/.test(arg); if (!needsQuotes) { - return trimmed; + return arg; } - return `"${trimmed.replace(/"/g, '\\"')}"`; + return `"${arg.replace(/"/g, '\\"')}"`; }) .join(" "); }