fix(security): bind node system.run approvals to env

This commit is contained in:
Peter Steinberger
2026-02-26 16:37:54 +01:00
parent f877e7e74c
commit 9a4b2266cc
18 changed files with 401 additions and 22 deletions

View File

@@ -19,6 +19,7 @@ Docs: https://docs.openclaw.ai
- Security/Sandbox path alias guard: reject broken symlink targets by resolving through existing ancestors and failing closed on out-of-root targets, preventing workspace-only `apply_patch` writes from escaping sandbox/workspace boundaries via dangling symlinks. This ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting.
- Security/Workspace FS boundary aliases: harden canonical boundary resolution for non-existent-leaf symlink aliases while preserving valid in-root aliases, preventing first-write workspace escapes via out-of-root symlink targets. This ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting.
- Security/Config includes: harden `$include` file loading with verified-open reads, reject hardlinked include aliases, and enforce include file-size guardrails so config include resolution remains bounded to trusted in-root files. This ships in the next npm release (`2026.2.26`). Thanks @zpbrent for reporting.
- Security/Node exec approvals: bind `system.run` approvals to canonicalized env overrides (`envHash`/`envKeys`) and fail closed on env-binding mismatches/missing bindings, while adding `GIT_EXTERNAL_DIFF` to blocked host env keys. This ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting.
- Security/Plugin channel HTTP auth: normalize protected `/api/channels` path checks against canonicalized request paths (case + percent-decoding + slash normalization), and fail closed on malformed `%`-encoded channel prefixes so alternate-path variants cannot bypass gateway auth.
- Security/Exec approvals forwarding: prefer turn-source channel/account/thread metadata when resolving approval delivery targets so stale session routes do not misroute approval prompts.
- Queue/Drain/Cron reliability: harden lane draining with guaranteed `draining` flag reset on synchronous pump failures, reject new queue enqueues during gateway restart drain windows (instead of silently killing accepted tasks), add `/stop` queued-backlog cutoff metadata with stale-message skipping (while avoiding cross-session native-stop cutoff bleed), and raise isolated cron `agentTurn` outer safety timeout to avoid false 10-minute timeout races against longer agent session timeouts. (#27407, #27332, #27427)

View File

@@ -14,6 +14,7 @@ enum HostEnvSanitizer {
"RUBYOPT",
"BASH_ENV",
"ENV",
"GIT_EXTERNAL_DIFF",
"SHELL",
"SHELLOPTS",
"PS4",

View File

@@ -9,6 +9,7 @@ export type RequestExecApprovalDecisionParams = {
id: string;
command: string;
commandArgv?: string[];
env?: Record<string, string>;
cwd: string;
nodeId?: string;
host: "gateway" | "node";
@@ -68,6 +69,7 @@ export async function registerExecApprovalRequest(
id: params.id,
command: params.command,
commandArgv: params.commandArgv,
env: params.env,
cwd: params.cwd,
nodeId: params.nodeId,
host: params.host,
@@ -127,6 +129,7 @@ export async function requestExecApprovalDecisionForHost(params: {
approvalId: string;
command: string;
commandArgv?: string[];
env?: Record<string, string>;
workdir: string;
host: "gateway" | "node";
nodeId?: string;
@@ -144,6 +147,7 @@ export async function requestExecApprovalDecisionForHost(params: {
id: params.approvalId,
command: params.command,
commandArgv: params.commandArgv,
env: params.env,
cwd: params.workdir,
nodeId: params.nodeId,
host: params.host,
@@ -163,6 +167,7 @@ export async function registerExecApprovalRequestForHost(params: {
approvalId: string;
command: string;
commandArgv?: string[];
env?: Record<string, string>;
workdir: string;
host: "gateway" | "node";
nodeId?: string;
@@ -180,6 +185,7 @@ export async function registerExecApprovalRequestForHost(params: {
id: params.approvalId,
command: params.command,
commandArgv: params.commandArgv,
env: params.env,
cwd: params.workdir,
nodeId: params.nodeId,
host: params.host,

View File

@@ -199,6 +199,7 @@ export async function executeNodeHostCommand(
approvalId,
command: params.command,
commandArgv: argv,
env: nodeEnv,
workdir: params.workdir,
host: "node",
nodeId,

View File

@@ -213,6 +213,9 @@ function buildExecApprovalMetadataLines(request: ExecApprovalRequest): string[]
if (request.request.host) {
lines.push(`- Host: ${request.request.host}`);
}
if (Array.isArray(request.request.envKeys) && request.request.envKeys.length > 0) {
lines.push(`- Env Overrides: ${request.request.envKeys.join(", ")}`);
}
if (request.request.agentId) {
lines.push(`- Agent: ${request.request.agentId}`);
}

View File

@@ -1,5 +1,6 @@
import { describe, expect, test } from "vitest";
import { approvalMatchesSystemRunRequest } from "./node-invoke-system-run-approval-match.js";
import { buildSystemRunApprovalEnvBinding } from "./system-run-approval-env-binding.js";
describe("approvalMatchesSystemRunRequest", () => {
test("matches legacy command text when binding fields match", () => {
@@ -75,6 +76,49 @@ describe("approvalMatchesSystemRunRequest", () => {
expect(result).toBe(false);
});
test("rejects env overrides when approval record lacks env hash", () => {
const result = approvalMatchesSystemRunRequest({
cmdText: "git diff",
argv: ["git", "diff"],
request: {
host: "node",
command: "git diff",
commandArgv: ["git", "diff"],
},
binding: {
cwd: null,
agentId: null,
sessionKey: null,
env: { GIT_EXTERNAL_DIFF: "/tmp/pwn.sh" },
},
});
expect(result).toBe(false);
});
test("accepts matching env hash with reordered keys", () => {
const binding = buildSystemRunApprovalEnvBinding({
SAFE_A: "1",
SAFE_B: "2",
});
const result = approvalMatchesSystemRunRequest({
cmdText: "git diff",
argv: ["git", "diff"],
request: {
host: "node",
command: "git diff",
commandArgv: ["git", "diff"],
envHash: binding.envHash,
},
binding: {
cwd: null,
agentId: null,
sessionKey: null,
env: { SAFE_B: "2", SAFE_A: "1" },
},
});
expect(result).toBe(true);
});
test("rejects non-node host requests", () => {
const result = approvalMatchesSystemRunRequest({
cmdText: "echo SAFE",

View File

@@ -1,9 +1,11 @@
import type { ExecApprovalRequestPayload } from "../infra/exec-approvals.js";
import { matchSystemRunApprovalEnvBinding } from "./system-run-approval-env-binding.js";
export type SystemRunApprovalBinding = {
cwd: string | null;
agentId: string | null;
sessionKey: string | null;
env?: unknown;
};
function argvMatchesRequest(requestedArgv: string[], argv: string[]): boolean {
@@ -24,28 +26,78 @@ export function approvalMatchesSystemRunRequest(params: {
request: ExecApprovalRequestPayload;
binding: SystemRunApprovalBinding;
}): boolean {
return evaluateSystemRunApprovalMatch(params).ok;
}
export type SystemRunApprovalMatchResult =
| { ok: true }
| {
ok: false;
code: "APPROVAL_REQUEST_MISMATCH" | "APPROVAL_ENV_BINDING_MISSING" | "APPROVAL_ENV_MISMATCH";
message: string;
details?: Record<string, unknown>;
};
export function evaluateSystemRunApprovalMatch(params: {
cmdText: string;
argv: string[];
request: ExecApprovalRequestPayload;
binding: SystemRunApprovalBinding;
}): SystemRunApprovalMatchResult {
if (params.request.host !== "node") {
return false;
return {
ok: false,
code: "APPROVAL_REQUEST_MISMATCH",
message: "approval id does not match request",
};
}
const requestedArgv = params.request.commandArgv;
if (Array.isArray(requestedArgv)) {
if (!argvMatchesRequest(requestedArgv, params.argv)) {
return false;
return {
ok: false,
code: "APPROVAL_REQUEST_MISMATCH",
message: "approval id does not match request",
};
}
} else if (!params.cmdText || params.request.command !== params.cmdText) {
return false;
return {
ok: false,
code: "APPROVAL_REQUEST_MISMATCH",
message: "approval id does not match request",
};
}
if ((params.request.cwd ?? null) !== params.binding.cwd) {
return false;
return {
ok: false,
code: "APPROVAL_REQUEST_MISMATCH",
message: "approval id does not match request",
};
}
if ((params.request.agentId ?? null) !== params.binding.agentId) {
return false;
return {
ok: false,
code: "APPROVAL_REQUEST_MISMATCH",
message: "approval id does not match request",
};
}
if ((params.request.sessionKey ?? null) !== params.binding.sessionKey) {
return false;
return {
ok: false,
code: "APPROVAL_REQUEST_MISMATCH",
message: "approval id does not match request",
};
}
return true;
const envMatch = matchSystemRunApprovalEnvBinding({
request: params.request,
env: params.binding.env,
});
if (!envMatch.ok) {
return envMatch;
}
return { ok: true };
}

View File

@@ -1,6 +1,7 @@
import { describe, expect, test } from "vitest";
import { ExecApprovalManager, type ExecApprovalRecord } from "./exec-approval-manager.js";
import { sanitizeSystemRunParamsForForwarding } from "./node-invoke-system-run-approval.js";
import { buildSystemRunApprovalEnvBinding } from "./system-run-approval-env-binding.js";
describe("sanitizeSystemRunParamsForForwarding", () => {
const now = Date.now();
@@ -198,6 +199,74 @@ describe("sanitizeSystemRunParamsForForwarding", () => {
});
expectAllowOnceForwardingResult(result);
});
test("rejects env overrides when approval record lacks env binding", () => {
const result = sanitizeSystemRunParamsForForwarding({
rawParams: {
command: ["git", "diff"],
rawCommand: "git diff",
env: { GIT_EXTERNAL_DIFF: "/tmp/pwn.sh" },
runId: "approval-1",
approved: true,
approvalDecision: "allow-once",
},
nodeId: "node-1",
client,
execApprovalManager: manager(makeRecord("git diff", ["git", "diff"])),
nowMs: now,
});
expect(result.ok).toBe(false);
if (result.ok) {
throw new Error("unreachable");
}
expect(result.details?.code).toBe("APPROVAL_ENV_BINDING_MISSING");
});
test("rejects env hash mismatch", () => {
const record = makeRecord("git diff", ["git", "diff"]);
record.request.envHash = buildSystemRunApprovalEnvBinding({ SAFE: "1" }).envHash;
const result = sanitizeSystemRunParamsForForwarding({
rawParams: {
command: ["git", "diff"],
rawCommand: "git diff",
env: { SAFE: "2" },
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.details?.code).toBe("APPROVAL_ENV_MISMATCH");
});
test("accepts matching env hash with reordered keys", () => {
const record = makeRecord("git diff", ["git", "diff"]);
const binding = buildSystemRunApprovalEnvBinding({ SAFE_A: "1", SAFE_B: "2" });
record.request.envHash = binding.envHash;
const result = sanitizeSystemRunParamsForForwarding({
rawParams: {
command: ["git", "diff"],
rawCommand: "git diff",
env: { SAFE_B: "2", SAFE_A: "1" },
runId: "approval-1",
approved: true,
approvalDecision: "allow-once",
},
nodeId: "node-1",
client,
execApprovalManager: manager(record),
nowMs: now,
});
expectAllowOnceForwardingResult(result);
});
test("consumes allow-once approvals and blocks same runId replay", async () => {
const approvalManager = new ExecApprovalManager();
const runId = "approval-replay-1";

View File

@@ -1,6 +1,6 @@
import { resolveSystemRunCommand } from "../infra/system-run-command.js";
import type { ExecApprovalRecord } from "./exec-approval-manager.js";
import { approvalMatchesSystemRunRequest } from "./node-invoke-system-run-approval-match.js";
import { evaluateSystemRunApprovalMatch } from "./node-invoke-system-run-approval-match.js";
type SystemRunParamsLike = {
command?: unknown;
@@ -204,22 +204,26 @@ export function sanitizeSystemRunParamsForForwarding(opts: {
};
}
if (
!approvalMatchesSystemRunRequest({
cmdText,
argv: cmdTextResolution.argv,
request: snapshot.request,
binding: {
cwd: normalizeString(p.cwd) ?? null,
agentId: normalizeString(p.agentId) ?? null,
sessionKey: normalizeString(p.sessionKey) ?? null,
},
})
) {
const approvalMatch = evaluateSystemRunApprovalMatch({
cmdText,
argv: cmdTextResolution.argv,
request: snapshot.request,
binding: {
cwd: normalizeString(p.cwd) ?? null,
agentId: normalizeString(p.agentId) ?? null,
sessionKey: normalizeString(p.sessionKey) ?? null,
env: p.env,
},
});
if (!approvalMatch.ok) {
return {
ok: false,
message: "approval id does not match request",
details: { code: "APPROVAL_REQUEST_MISMATCH", runId },
message: approvalMatch.message,
details: {
code: approvalMatch.code,
runId,
...approvalMatch.details,
},
};
}

View File

@@ -90,6 +90,7 @@ export const ExecApprovalRequestParamsSchema = Type.Object(
id: Type.Optional(NonEmptyString),
command: NonEmptyString,
commandArgv: Type.Optional(Type.Array(Type.String())),
env: Type.Optional(Type.Record(NonEmptyString, Type.String())),
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()])),

View File

@@ -11,6 +11,7 @@ import {
validateExecApprovalRequestParams,
validateExecApprovalResolveParams,
} from "../protocol/index.js";
import { buildSystemRunApprovalEnvBinding } from "../system-run-approval-env-binding.js";
import type { GatewayRequestHandlers } from "./types.js";
export function createExecApprovalHandlers(
@@ -44,6 +45,7 @@ export function createExecApprovalHandlers(
id?: string;
command: string;
commandArgv?: string[];
env?: Record<string, string>;
cwd?: string;
nodeId?: string;
host?: string;
@@ -68,6 +70,7 @@ export function createExecApprovalHandlers(
const commandArgv = Array.isArray(p.commandArgv)
? p.commandArgv.map((entry) => String(entry))
: undefined;
const envBinding = buildSystemRunApprovalEnvBinding(p.env);
if (host === "node" && !nodeId) {
respond(
false,
@@ -87,6 +90,8 @@ export function createExecApprovalHandlers(
const request = {
command: p.command,
commandArgv,
envHash: envBinding.envHash,
envKeys: envBinding.envKeys.length > 0 ? envBinding.envKeys : undefined,
cwd: p.cwd ?? null,
nodeId: host === "node" ? nodeId : null,
host: host || null,

View File

@@ -9,6 +9,7 @@ import { formatZonedTimestamp } from "../../infra/format-time/format-datetime.js
import { resetLogger, setLoggerOverride } from "../../logging.js";
import { ExecApprovalManager } from "../exec-approval-manager.js";
import { validateExecApprovalRequestParams } from "../protocol/index.js";
import { buildSystemRunApprovalEnvBinding } from "../system-run-approval-env-binding.js";
import { waitForAgentJob } from "./agent-job.js";
import { injectTimestamp, timestampOptsFromConfig } from "./agent-timestamp.js";
import { normalizeRpcAttachmentsToChatAttachments } from "./attachment-normalize.js";
@@ -423,6 +424,30 @@ describe("exec approval handlers", () => {
expect(broadcasts.some((entry) => entry.event === "exec.approval.resolved")).toBe(true);
});
it("stores env binding hash and sorted env keys on approval request", async () => {
const { handlers, broadcasts, respond, context } = createExecApprovalFixture();
await requestExecApproval({
handlers,
respond,
context,
params: {
env: {
Z_VAR: "z",
A_VAR: "a",
},
},
});
const requested = broadcasts.find((entry) => entry.event === "exec.approval.requested");
expect(requested).toBeTruthy();
const request = (requested?.payload as { request?: Record<string, unknown> })?.request ?? {};
const expected = buildSystemRunApprovalEnvBinding({
A_VAR: "a",
Z_VAR: "z",
});
expect(request["envHash"]).toBe(expected.envHash);
expect(request["envKeys"]).toEqual(["A_VAR", "Z_VAR"]);
});
it("accepts resolve during broadcast", async () => {
const manager = new ExecApprovalManager();
const handlers = createExecApprovalHandlers(manager);

View File

@@ -0,0 +1,71 @@
import { describe, expect, test } from "vitest";
import {
buildSystemRunApprovalEnvBinding,
matchSystemRunApprovalEnvBinding,
} from "./system-run-approval-env-binding.js";
describe("buildSystemRunApprovalEnvBinding", () => {
test("normalizes keys and produces stable hash regardless of input order", () => {
const a = buildSystemRunApprovalEnvBinding({
Z_VAR: "z",
A_VAR: "a",
" BAD KEY": "ignored",
});
const b = buildSystemRunApprovalEnvBinding({
A_VAR: "a",
Z_VAR: "z",
});
expect(a.envKeys).toEqual(["A_VAR", "Z_VAR"]);
expect(a.envHash).toBe(b.envHash);
});
});
describe("matchSystemRunApprovalEnvBinding", () => {
test("accepts missing env hash when request has no env overrides", () => {
const result = matchSystemRunApprovalEnvBinding({
request: {},
env: undefined,
});
expect(result).toEqual({ ok: true });
});
test("rejects non-empty env overrides when approval has no env hash", () => {
const result = matchSystemRunApprovalEnvBinding({
request: {},
env: { GIT_EXTERNAL_DIFF: "/tmp/pwn.sh" },
});
expect(result.ok).toBe(false);
if (result.ok) {
throw new Error("unreachable");
}
expect(result.code).toBe("APPROVAL_ENV_BINDING_MISSING");
});
test("rejects env hash mismatch", () => {
const approved = buildSystemRunApprovalEnvBinding({ SAFE: "1" });
const result = matchSystemRunApprovalEnvBinding({
request: { envHash: approved.envHash },
env: { SAFE: "2" },
});
expect(result.ok).toBe(false);
if (result.ok) {
throw new Error("unreachable");
}
expect(result.code).toBe("APPROVAL_ENV_MISMATCH");
});
test("accepts matching env hash with key order differences", () => {
const approved = buildSystemRunApprovalEnvBinding({
SAFE_A: "1",
SAFE_B: "2",
});
const result = matchSystemRunApprovalEnvBinding({
request: { envHash: approved.envHash },
env: {
SAFE_B: "2",
SAFE_A: "1",
},
});
expect(result).toEqual({ ok: true });
});
});

View File

@@ -0,0 +1,88 @@
import crypto from "node:crypto";
import type { ExecApprovalRequestPayload } from "../infra/exec-approvals.js";
import { normalizeEnvVarKey } from "../infra/host-env-security.js";
type NormalizedSystemRunEnvEntry = [key: string, value: string];
function normalizeSystemRunEnvEntries(env: unknown): NormalizedSystemRunEnvEntry[] {
if (!env || typeof env !== "object" || Array.isArray(env)) {
return [];
}
const entries: NormalizedSystemRunEnvEntry[] = [];
for (const [rawKey, rawValue] of Object.entries(env as Record<string, unknown>)) {
if (typeof rawValue !== "string") {
continue;
}
const key = normalizeEnvVarKey(rawKey, { portable: true });
if (!key) {
continue;
}
entries.push([key, rawValue]);
}
entries.sort((a, b) => a[0].localeCompare(b[0]));
return entries;
}
function hashSystemRunEnvEntries(entries: NormalizedSystemRunEnvEntry[]): string | null {
if (entries.length === 0) {
return null;
}
return crypto.createHash("sha256").update(JSON.stringify(entries)).digest("hex");
}
export function buildSystemRunApprovalEnvBinding(env: unknown): {
envHash: string | null;
envKeys: string[];
} {
const entries = normalizeSystemRunEnvEntries(env);
return {
envHash: hashSystemRunEnvEntries(entries),
envKeys: entries.map(([key]) => key),
};
}
export type SystemRunEnvBindingMatchResult =
| { ok: true }
| {
ok: false;
code: "APPROVAL_ENV_BINDING_MISSING" | "APPROVAL_ENV_MISMATCH";
message: string;
details?: Record<string, unknown>;
};
export function matchSystemRunApprovalEnvBinding(params: {
request: Pick<ExecApprovalRequestPayload, "envHash">;
env: unknown;
}): SystemRunEnvBindingMatchResult {
const expectedEnvHash =
typeof params.request.envHash === "string" && params.request.envHash.trim().length > 0
? params.request.envHash.trim()
: null;
const actual = buildSystemRunApprovalEnvBinding(params.env);
const actualEnvHash = actual.envHash;
if (!expectedEnvHash && !actualEnvHash) {
return { ok: true };
}
if (!expectedEnvHash && actualEnvHash) {
return {
ok: false,
code: "APPROVAL_ENV_BINDING_MISSING",
message: "approval id missing env binding for requested env overrides",
details: { envKeys: actual.envKeys },
};
}
if (expectedEnvHash !== actualEnvHash) {
return {
ok: false,
code: "APPROVAL_ENV_MISMATCH",
message: "approval id env binding mismatch",
details: {
envKeys: actual.envKeys,
expectedEnvHash,
actualEnvHash,
},
};
}
return { ok: true };
}

View File

@@ -175,6 +175,9 @@ function buildRequestMessage(request: ExecApprovalRequest, nowMs: number) {
if (request.request.nodeId) {
lines.push(`Node: ${request.request.nodeId}`);
}
if (Array.isArray(request.request.envKeys) && request.request.envKeys.length > 0) {
lines.push(`Env overrides: ${request.request.envKeys.join(", ")}`);
}
if (request.request.host) {
lines.push(`Host: ${request.request.host}`);
}

View File

@@ -14,6 +14,8 @@ export type ExecAsk = "off" | "on-miss" | "always";
export type ExecApprovalRequestPayload = {
command: string;
commandArgv?: string[];
envHash?: string | null;
envKeys?: string[];
cwd?: string | null;
nodeId?: string | null;
host?: string | null;

View File

@@ -10,6 +10,7 @@
"RUBYOPT",
"BASH_ENV",
"ENV",
"GIT_EXTERNAL_DIFF",
"SHELL",
"SHELLOPTS",
"PS4",

View File

@@ -16,6 +16,7 @@ describe("isDangerousHostEnvVarName", () => {
expect(isDangerousHostEnvVarName("BASH_ENV")).toBe(true);
expect(isDangerousHostEnvVarName("bash_env")).toBe(true);
expect(isDangerousHostEnvVarName("SHELL")).toBe(true);
expect(isDangerousHostEnvVarName("GIT_EXTERNAL_DIFF")).toBe(true);
expect(isDangerousHostEnvVarName("SHELLOPTS")).toBe(true);
expect(isDangerousHostEnvVarName("ps4")).toBe(true);
expect(isDangerousHostEnvVarName("DYLD_INSERT_LIBRARIES")).toBe(true);
@@ -32,6 +33,7 @@ describe("sanitizeHostExecEnv", () => {
baseEnv: {
PATH: "/usr/bin:/bin",
BASH_ENV: "/tmp/pwn.sh",
GIT_EXTERNAL_DIFF: "/tmp/pwn.sh",
LD_PRELOAD: "/tmp/pwn.so",
OK: "1",
},