Skip to content

Commit 0fdfc9f

Browse files
jesse-merhijesse-merhi
authored
fix(exec): harden backend sandbox exec cleanup (#96926)
1 parent 448b7c7 commit 0fdfc9f

6 files changed

Lines changed: 234 additions & 13 deletions

‎src/agents/agent-tool-definition-adapter.test.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,38 @@ describe("agent tool definition adapter", () => {
189189
});
190190
});
191191

192+
it("does not throw WeakMap errors when preparing malformed backend sandbox exec params", async () => {
193+
const validateWorkdir = vi.fn(async (workdir: string) => workdir);
194+
const tool = createExecTool({
195+
host: "sandbox",
196+
security: "full",
197+
ask: "off",
198+
sandbox: {
199+
containerName: "remote-sandbox-workdir-test",
200+
workspaceDir: process.cwd(),
201+
containerWorkdir: "/remote/workspace",
202+
workdirValidation: "backend",
203+
validateWorkdir,
204+
},
205+
});
206+
const [definition] = toToolDefinitions([tool]);
207+
208+
const result = await definition.execute(
209+
"call-malformed-backend-sandbox-exec-params",
210+
"not-an-object",
211+
undefined,
212+
undefined,
213+
extensionContext,
214+
);
215+
216+
expect(result.details).toMatchObject({
217+
status: "error",
218+
error: "Provide a command to start.",
219+
});
220+
expect(JSON.stringify(result)).not.toContain("WeakMap");
221+
expect(validateWorkdir).not.toHaveBeenCalled();
222+
});
223+
192224
it("reports malformed exec params when elevated logging is enabled", async () => {
193225
const tool = createExecTool({
194226
security: "full",

‎src/agents/bash-tools.exec-foreground-failures.test.ts‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -367,6 +367,62 @@ describe("exec foreground failures", () => {
367367
}
368368
});
369369

370+
it("finalizes backend sandbox exec tokens when process spawn fails", async () => {
371+
const workspaceDir = tempDirs.make("openclaw-sandbox-workdir-");
372+
const finalizeToken = { session: "remote-session" };
373+
const buildExecSpec = vi.fn<NonNullable<BashSandboxConfig["buildExecSpec"]>>(
374+
async (params) => ({
375+
argv: ["remote-shell", params.command],
376+
env: {},
377+
stdinMode: "pipe-open" as const,
378+
finalizeToken,
379+
}),
380+
);
381+
const finalizeExec = vi.fn<NonNullable<BashSandboxConfig["finalizeExec"]>>(async () => {});
382+
const validateWorkdir = vi.fn<NonNullable<BashSandboxConfig["validateWorkdir"]>>(
383+
async (workdir) => workdir,
384+
);
385+
supervisorMock.spawn.mockRejectedValueOnce(new Error("spawn failed"));
386+
387+
const tool = createExecTool({
388+
host: "sandbox",
389+
security: "full",
390+
ask: "off",
391+
allowBackground: false,
392+
sandbox: {
393+
containerName: "remote-sandbox-workdir-test",
394+
workspaceDir,
395+
containerWorkdir: "/remote/workspace",
396+
workdirValidation: "backend",
397+
validateWorkdir,
398+
buildExecSpec,
399+
finalizeExec,
400+
},
401+
});
402+
403+
try {
404+
await expect(
405+
tool.execute("call-remote-sandbox-spawn-failure", {
406+
command: "echo ok",
407+
workdir: "/remote/workspace/generated",
408+
}),
409+
).rejects.toThrow("spawn failed");
410+
411+
expect(validateWorkdir).toHaveBeenCalledWith("/remote/workspace/generated");
412+
expect(buildExecSpec).toHaveBeenCalledOnce();
413+
expect(supervisorMock.spawn).toHaveBeenCalledOnce();
414+
expect(finalizeExec).toHaveBeenCalledOnce();
415+
expect(finalizeExec).toHaveBeenCalledWith({
416+
status: "failed",
417+
exitCode: null,
418+
timedOut: false,
419+
token: finalizeToken,
420+
});
421+
} finally {
422+
fs.rmSync(workspaceDir, { recursive: true, force: true });
423+
}
424+
});
425+
370426
it("rejects unsafe commands before backend workdir validation", async () => {
371427
const workspaceDir = tempDirs.make("openclaw-sandbox-workdir-");
372428
const buildExecSpec = vi.fn<NonNullable<BashSandboxConfig["buildExecSpec"]>>(

‎src/agents/bash-tools.exec-runtime.ts‎

Lines changed: 34 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -715,6 +715,21 @@ export async function runExecProcess(opts: {
715715

716716
const timeoutMs = resolveExecTimeoutMs(opts.timeoutSec);
717717
let sandboxFinalizeToken: unknown;
718+
let sandboxFinalized = false;
719+
const finalizeSandboxExec = async (params: {
720+
status: "completed" | "failed";
721+
exitCode: number | null;
722+
timedOut: boolean;
723+
}) => {
724+
if (sandboxFinalized || !opts.sandbox?.finalizeExec) {
725+
return;
726+
}
727+
sandboxFinalized = true;
728+
await opts.sandbox.finalizeExec({
729+
...params,
730+
token: sandboxFinalizeToken,
731+
});
732+
};
718733

719734
const spawnSpec:
720735
| {
@@ -861,6 +876,13 @@ export async function runExecProcess(opts: {
861876
} catch (retryErr) {
862877
markExited(session, null, null, "failed");
863878
maybeNotifyOnExit(session, "failed");
879+
await finalizeSandboxExec({
880+
status: "failed",
881+
exitCode: null,
882+
timedOut: false,
883+
}).catch((finalizeErr: unknown) => {
884+
logWarn(`exec: sandbox finalize after spawn failure failed (${String(finalizeErr)}).`);
885+
});
864886
emitExecProcessCompleted({
865887
command: opts.command,
866888
mode: "child",
@@ -877,6 +899,13 @@ export async function runExecProcess(opts: {
877899
} else {
878900
markExited(session, null, null, "failed");
879901
maybeNotifyOnExit(session, "failed");
902+
await finalizeSandboxExec({
903+
status: "failed",
904+
exitCode: null,
905+
timedOut: false,
906+
}).catch((finalizeErr: unknown) => {
907+
logWarn(`exec: sandbox finalize after spawn failure failed (${String(finalizeErr)}).`);
908+
});
880909
emitExecProcessCompleted({
881910
command: opts.command,
882911
mode: spawnSpec.mode,
@@ -915,14 +944,11 @@ export async function runExecProcess(opts: {
915944
if (!session.child && session.stdin) {
916945
session.stdin.destroyed = true;
917946
}
918-
if (opts.sandbox?.finalizeExec) {
919-
await opts.sandbox.finalizeExec({
920-
status: outcome.status,
921-
exitCode: exit.exitCode ?? null,
922-
timedOut: exit.timedOut,
923-
token: sandboxFinalizeToken,
924-
});
925-
}
947+
await finalizeSandboxExec({
948+
status: outcome.status,
949+
exitCode: exit.exitCode ?? null,
950+
timedOut: exit.timedOut,
951+
});
926952
emitExecProcessCompleted({
927953
command: opts.command,
928954
mode: usingPty ? "pty" : "child",

‎src/agents/bash-tools.exec.resolve-env-hook.test.ts‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
77
import { OPENCLAW_CLI_ENV_VALUE } from "../infra/openclaw-exec-env.js";
88
import type { ExecuteNodeHostCommandParams } from "./bash-tools.exec-host-node.types.js";
9+
import type { BashSandboxConfig } from "./bash-tools.shared.js";
910
import type { ExtensionContext } from "./sessions/index.js";
1011

1112
declare module "../plugins/hook-types.js" {
@@ -516,6 +517,71 @@ describe("exec resolve_exec_env hook wiring", () => {
516517
expect(mocks.spawnInputs).toHaveLength(0);
517518
});
518519

520+
it("preserves hook context when backend sandbox env resolution is deferred", async () => {
521+
const validateWorkdir = vi.fn(async (workdir: string) => workdir);
522+
const buildExecSpec = vi.fn<NonNullable<BashSandboxConfig["buildExecSpec"]>>(
523+
async (params) => ({
524+
argv: ["remote-shell", params.command],
525+
env: {},
526+
stdinMode: "pipe-open" as const,
527+
}),
528+
);
529+
mocks.hookRunner = {
530+
hasHooks: vi.fn(
531+
(hookName: string) => hookName === "resolve_exec_env" || hookName === "before_tool_call",
532+
),
533+
runResolveExecEnv: vi.fn(async () => ({ PLUGIN_SAFE: "yes" })),
534+
runBeforeToolCall: vi.fn(async () => undefined),
535+
};
536+
const tool = createExecTool({
537+
host: "sandbox",
538+
security: "full",
539+
ask: "off",
540+
sandbox: {
541+
containerName: "remote-sandbox-workdir-test",
542+
workspaceDir: process.cwd(),
543+
containerWorkdir: "/remote/workspace",
544+
workdirValidation: "backend",
545+
validateWorkdir,
546+
buildExecSpec,
547+
},
548+
});
549+
const [definition] = toToolDefinitions([tool], {
550+
agentId: "ctx-agent",
551+
sessionKey: "agent:ctx-agent:telegram:chat-2",
552+
channelId: "ctx-channel",
553+
});
554+
555+
const result = await definition.execute(
556+
"call-backend-deferred-env-context",
557+
{
558+
command: "echo ok",
559+
workdir: "/remote/workspace/generated",
560+
},
561+
undefined,
562+
undefined,
563+
testExtensionContext,
564+
);
565+
566+
expect((result.details as { status?: unknown } | undefined)?.status).toBe("completed");
567+
expect(validateWorkdir).toHaveBeenCalledWith("/remote/workspace/generated");
568+
expect(mocks.hookRunner.runBeforeToolCall!).toHaveBeenCalledOnce();
569+
expect(mocks.hookRunner.runResolveExecEnv!).toHaveBeenCalledOnce();
570+
expect(mocks.hookRunner.runResolveExecEnv!.mock.calls[0]?.[0]).toMatchObject({
571+
sessionKey: "agent:ctx-agent:telegram:chat-2",
572+
toolName: "exec",
573+
host: "sandbox",
574+
});
575+
expect(mocks.hookRunner.runResolveExecEnv!.mock.calls[0]?.[1]).toMatchObject({
576+
agentId: "ctx-agent",
577+
sessionKey: "agent:ctx-agent:telegram:chat-2",
578+
channelId: "ctx-channel",
579+
});
580+
expect(buildExecSpec.mock.calls[0]?.[0]?.env).toMatchObject({
581+
PLUGIN_SAFE: "yes",
582+
});
583+
});
584+
519585
it("lets lazy before_tool_call see invalid workdirs before failing unchanged params", async () => {
520586
mocks.hookRunner = {
521587
hasHooks: vi.fn(

‎src/agents/bash-tools.exec.ts‎

Lines changed: 46 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,13 @@ type ResolvedExecEnvPreparedState = {
143143
pluginEnv?: Record<string, string>;
144144
};
145145
const resolvedExecEnvPreparedStates = new WeakMap<ExecToolArgs, ResolvedExecEnvPreparedState>();
146+
type DeferredResolveExecEnvPreparedState = {
147+
hookContext?: HookContext;
148+
};
149+
const deferredResolveExecEnvPreparedStates = new WeakMap<
150+
ExecToolArgs,
151+
DeferredResolveExecEnvPreparedState
152+
>();
146153
type ResolvedExecWorkdirPreparedState = {
147154
host: ExecHost;
148155
inputWorkdir?: string;
@@ -162,6 +169,10 @@ const XML_ARG_VALUE_EXEC_PARAM_KEYS = [
162169
"node",
163170
] as const;
164171

172+
function isExecToolArgsObject(value: unknown): value is ExecToolArgs {
173+
return typeof value === "object" && value !== null && !Array.isArray(value);
174+
}
175+
165176
function filterPluginExecEnv(rawEnv: Record<string, string>): Record<string, string> | undefined {
166177
const env: Record<string, string> = {};
167178
for (const [rawKey, value] of Object.entries(rawEnv)) {
@@ -201,6 +212,20 @@ function isResolveExecEnvPrepared(params: ExecToolArgs): boolean {
201212
return Boolean(getResolvedExecEnvPreparedState(params));
202213
}
203214

215+
function markDeferredResolveExecEnvPrepared<T extends ExecToolArgs>(
216+
params: T,
217+
state: DeferredResolveExecEnvPreparedState,
218+
): T {
219+
deferredResolveExecEnvPreparedStates.set(params, state);
220+
return params;
221+
}
222+
223+
function getDeferredResolveExecEnvPreparedState(
224+
params: ExecToolArgs,
225+
): DeferredResolveExecEnvPreparedState | undefined {
226+
return deferredResolveExecEnvPreparedStates.get(params);
227+
}
228+
204229
function markResolvedExecWorkdirPrepared<T extends ExecToolArgs>(
205230
params: T,
206231
state: ResolvedExecWorkdirPreparedState,
@@ -1475,20 +1500,31 @@ export function createExecTool(
14751500
if (workdirState?.resolution.kind === "unavailable") {
14761501
return params;
14771502
}
1478-
if (shouldDeferResolveExecEnvUntilWorkdirValidated(params)) {
1503+
if (!isExecToolArgsObject(params)) {
14791504
return params;
14801505
}
1506+
if (shouldDeferResolveExecEnvUntilWorkdirValidated(params)) {
1507+
return markDeferredResolveExecEnvPrepared(params, {
1508+
hookContext: context.hookContext as HookContext | undefined,
1509+
});
1510+
}
14811511
return prepareParamsWithResolvedExecEnv(params, {
14821512
hookContext: context.hookContext as HookContext | undefined,
14831513
});
14841514
},
14851515
finalizeBeforeToolCallParams: (params, preparedParams) => {
1486-
const execParams = params as ExecToolArgs;
14871516
const envState = getResolvedExecEnvPreparedState(preparedParams as ExecToolArgs);
1517+
const deferredEnvState = getDeferredResolveExecEnvPreparedState(
1518+
preparedParams as ExecToolArgs,
1519+
);
14881520
const workdirState = getResolvedExecWorkdirPreparedState(preparedParams as ExecToolArgs);
1489-
if (!envState && !workdirState) {
1521+
if (!envState && !deferredEnvState && !workdirState) {
1522+
return params;
1523+
}
1524+
if (!isExecToolArgsObject(params)) {
14901525
return params;
14911526
}
1527+
const execParams = params;
14921528
let host: ExecHost | undefined;
14931529
const resolveFinalHost = () => {
14941530
host ??= resolveHostForParams(execParams);
@@ -1511,6 +1547,9 @@ export function createExecTool(
15111547
if (envState) {
15121548
markResolveExecEnvPrepared(execParams, envState);
15131549
}
1550+
if (deferredEnvState) {
1551+
markDeferredResolveExecEnvPrepared(execParams, deferredEnvState);
1552+
}
15141553
if (workdirState) {
15151554
markResolvedExecWorkdirPrepared(execParams, workdirState);
15161555
}
@@ -1522,6 +1561,7 @@ export function createExecTool(
15221561
XML_ARG_VALUE_EXEC_PARAM_KEYS,
15231562
);
15241563
const resolveExecEnvPrepared = isResolveExecEnvPrepared(args as ExecToolArgs);
1564+
const deferredResolveExecEnvState = getDeferredResolveExecEnvPreparedState(params);
15251565
const preparedWorkdirState = getResolvedExecWorkdirPreparedState(params);
15261566

15271567
const maxOutput = DEFAULT_MAX_OUTPUT;
@@ -1724,7 +1764,9 @@ export function createExecTool(
17241764
logInfo(`exec: elevated command ${truncateMiddle(params.command, 120)}`);
17251765
}
17261766
if (!resolveExecEnvPrepared) {
1727-
params = await prepareParamsWithResolvedExecEnv(params);
1767+
params = await prepareParamsWithResolvedExecEnv(params, {
1768+
hookContext: deferredResolveExecEnvState?.hookContext,
1769+
});
17281770
}
17291771

17301772
const inheritedBaseEnv = coerceEnv(process.env);

‎src/agents/sandbox.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ export {
5353
runSshSandboxCommand,
5454
shellEscape,
5555
uploadDirectoryToSshTarget,
56-
VALIDATE_REMOTE_WORKDIR_SCRIPT,
5756
} from "./sandbox/ssh.js";
5857
export { sanitizeEnvVars } from "./sandbox/sanitize-env-vars.js";
5958
export { createRemoteShellSandboxFsBridge } from "./sandbox/remote-fs-bridge.js";

0 commit comments

Comments
 (0)