mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-28 13:26:04 -06:00
fix(exec): harden backend sandbox exec cleanup (#96926)
This commit is contained in:
@@ -189,6 +189,38 @@ describe("agent tool definition adapter", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("does not throw WeakMap errors when preparing malformed backend sandbox exec params", async () => {
|
||||
const validateWorkdir = vi.fn(async (workdir: string) => workdir);
|
||||
const tool = createExecTool({
|
||||
host: "sandbox",
|
||||
security: "full",
|
||||
ask: "off",
|
||||
sandbox: {
|
||||
containerName: "remote-sandbox-workdir-test",
|
||||
workspaceDir: process.cwd(),
|
||||
containerWorkdir: "/remote/workspace",
|
||||
workdirValidation: "backend",
|
||||
validateWorkdir,
|
||||
},
|
||||
});
|
||||
const [definition] = toToolDefinitions([tool]);
|
||||
|
||||
const result = await definition.execute(
|
||||
"call-malformed-backend-sandbox-exec-params",
|
||||
"not-an-object",
|
||||
undefined,
|
||||
undefined,
|
||||
extensionContext,
|
||||
);
|
||||
|
||||
expect(result.details).toMatchObject({
|
||||
status: "error",
|
||||
error: "Provide a command to start.",
|
||||
});
|
||||
expect(JSON.stringify(result)).not.toContain("WeakMap");
|
||||
expect(validateWorkdir).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("reports malformed exec params when elevated logging is enabled", async () => {
|
||||
const tool = createExecTool({
|
||||
security: "full",
|
||||
|
||||
@@ -367,6 +367,62 @@ describe("exec foreground failures", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("finalizes backend sandbox exec tokens when process spawn fails", async () => {
|
||||
const workspaceDir = tempDirs.make("openclaw-sandbox-workdir-");
|
||||
const finalizeToken = { session: "remote-session" };
|
||||
const buildExecSpec = vi.fn<NonNullable<BashSandboxConfig["buildExecSpec"]>>(
|
||||
async (params) => ({
|
||||
argv: ["remote-shell", params.command],
|
||||
env: {},
|
||||
stdinMode: "pipe-open" as const,
|
||||
finalizeToken,
|
||||
}),
|
||||
);
|
||||
const finalizeExec = vi.fn<NonNullable<BashSandboxConfig["finalizeExec"]>>(async () => {});
|
||||
const validateWorkdir = vi.fn<NonNullable<BashSandboxConfig["validateWorkdir"]>>(
|
||||
async (workdir) => workdir,
|
||||
);
|
||||
supervisorMock.spawn.mockRejectedValueOnce(new Error("spawn failed"));
|
||||
|
||||
const tool = createExecTool({
|
||||
host: "sandbox",
|
||||
security: "full",
|
||||
ask: "off",
|
||||
allowBackground: false,
|
||||
sandbox: {
|
||||
containerName: "remote-sandbox-workdir-test",
|
||||
workspaceDir,
|
||||
containerWorkdir: "/remote/workspace",
|
||||
workdirValidation: "backend",
|
||||
validateWorkdir,
|
||||
buildExecSpec,
|
||||
finalizeExec,
|
||||
},
|
||||
});
|
||||
|
||||
try {
|
||||
await expect(
|
||||
tool.execute("call-remote-sandbox-spawn-failure", {
|
||||
command: "echo ok",
|
||||
workdir: "/remote/workspace/generated",
|
||||
}),
|
||||
).rejects.toThrow("spawn failed");
|
||||
|
||||
expect(validateWorkdir).toHaveBeenCalledWith("/remote/workspace/generated");
|
||||
expect(buildExecSpec).toHaveBeenCalledOnce();
|
||||
expect(supervisorMock.spawn).toHaveBeenCalledOnce();
|
||||
expect(finalizeExec).toHaveBeenCalledOnce();
|
||||
expect(finalizeExec).toHaveBeenCalledWith({
|
||||
status: "failed",
|
||||
exitCode: null,
|
||||
timedOut: false,
|
||||
token: finalizeToken,
|
||||
});
|
||||
} finally {
|
||||
fs.rmSync(workspaceDir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it("rejects unsafe commands before backend workdir validation", async () => {
|
||||
const workspaceDir = tempDirs.make("openclaw-sandbox-workdir-");
|
||||
const buildExecSpec = vi.fn<NonNullable<BashSandboxConfig["buildExecSpec"]>>(
|
||||
|
||||
@@ -715,6 +715,21 @@ export async function runExecProcess(opts: {
|
||||
|
||||
const timeoutMs = resolveExecTimeoutMs(opts.timeoutSec);
|
||||
let sandboxFinalizeToken: unknown;
|
||||
let sandboxFinalized = false;
|
||||
const finalizeSandboxExec = async (params: {
|
||||
status: "completed" | "failed";
|
||||
exitCode: number | null;
|
||||
timedOut: boolean;
|
||||
}) => {
|
||||
if (sandboxFinalized || !opts.sandbox?.finalizeExec) {
|
||||
return;
|
||||
}
|
||||
sandboxFinalized = true;
|
||||
await opts.sandbox.finalizeExec({
|
||||
...params,
|
||||
token: sandboxFinalizeToken,
|
||||
});
|
||||
};
|
||||
|
||||
const spawnSpec:
|
||||
| {
|
||||
@@ -861,6 +876,13 @@ export async function runExecProcess(opts: {
|
||||
} catch (retryErr) {
|
||||
markExited(session, null, null, "failed");
|
||||
maybeNotifyOnExit(session, "failed");
|
||||
await finalizeSandboxExec({
|
||||
status: "failed",
|
||||
exitCode: null,
|
||||
timedOut: false,
|
||||
}).catch((finalizeErr: unknown) => {
|
||||
logWarn(`exec: sandbox finalize after spawn failure failed (${String(finalizeErr)}).`);
|
||||
});
|
||||
emitExecProcessCompleted({
|
||||
command: opts.command,
|
||||
mode: "child",
|
||||
@@ -877,6 +899,13 @@ export async function runExecProcess(opts: {
|
||||
} else {
|
||||
markExited(session, null, null, "failed");
|
||||
maybeNotifyOnExit(session, "failed");
|
||||
await finalizeSandboxExec({
|
||||
status: "failed",
|
||||
exitCode: null,
|
||||
timedOut: false,
|
||||
}).catch((finalizeErr: unknown) => {
|
||||
logWarn(`exec: sandbox finalize after spawn failure failed (${String(finalizeErr)}).`);
|
||||
});
|
||||
emitExecProcessCompleted({
|
||||
command: opts.command,
|
||||
mode: spawnSpec.mode,
|
||||
@@ -915,14 +944,11 @@ export async function runExecProcess(opts: {
|
||||
if (!session.child && session.stdin) {
|
||||
session.stdin.destroyed = true;
|
||||
}
|
||||
if (opts.sandbox?.finalizeExec) {
|
||||
await opts.sandbox.finalizeExec({
|
||||
status: outcome.status,
|
||||
exitCode: exit.exitCode ?? null,
|
||||
timedOut: exit.timedOut,
|
||||
token: sandboxFinalizeToken,
|
||||
});
|
||||
}
|
||||
await finalizeSandboxExec({
|
||||
status: outcome.status,
|
||||
exitCode: exit.exitCode ?? null,
|
||||
timedOut: exit.timedOut,
|
||||
});
|
||||
emitExecProcessCompleted({
|
||||
command: opts.command,
|
||||
mode: usingPty ? "pty" : "child",
|
||||
|
||||
@@ -6,6 +6,7 @@
|
||||
import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { OPENCLAW_CLI_ENV_VALUE } from "../infra/openclaw-exec-env.js";
|
||||
import type { ExecuteNodeHostCommandParams } from "./bash-tools.exec-host-node.types.js";
|
||||
import type { BashSandboxConfig } from "./bash-tools.shared.js";
|
||||
import type { ExtensionContext } from "./sessions/index.js";
|
||||
|
||||
declare module "../plugins/hook-types.js" {
|
||||
@@ -516,6 +517,71 @@ describe("exec resolve_exec_env hook wiring", () => {
|
||||
expect(mocks.spawnInputs).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("preserves hook context when backend sandbox env resolution is deferred", async () => {
|
||||
const validateWorkdir = vi.fn(async (workdir: string) => workdir);
|
||||
const buildExecSpec = vi.fn<NonNullable<BashSandboxConfig["buildExecSpec"]>>(
|
||||
async (params) => ({
|
||||
argv: ["remote-shell", params.command],
|
||||
env: {},
|
||||
stdinMode: "pipe-open" as const,
|
||||
}),
|
||||
);
|
||||
mocks.hookRunner = {
|
||||
hasHooks: vi.fn(
|
||||
(hookName: string) => hookName === "resolve_exec_env" || hookName === "before_tool_call",
|
||||
),
|
||||
runResolveExecEnv: vi.fn(async () => ({ PLUGIN_SAFE: "yes" })),
|
||||
runBeforeToolCall: vi.fn(async () => undefined),
|
||||
};
|
||||
const tool = createExecTool({
|
||||
host: "sandbox",
|
||||
security: "full",
|
||||
ask: "off",
|
||||
sandbox: {
|
||||
containerName: "remote-sandbox-workdir-test",
|
||||
workspaceDir: process.cwd(),
|
||||
containerWorkdir: "/remote/workspace",
|
||||
workdirValidation: "backend",
|
||||
validateWorkdir,
|
||||
buildExecSpec,
|
||||
},
|
||||
});
|
||||
const [definition] = toToolDefinitions([tool], {
|
||||
agentId: "ctx-agent",
|
||||
sessionKey: "agent:ctx-agent:telegram:chat-2",
|
||||
channelId: "ctx-channel",
|
||||
});
|
||||
|
||||
const result = await definition.execute(
|
||||
"call-backend-deferred-env-context",
|
||||
{
|
||||
command: "echo ok",
|
||||
workdir: "/remote/workspace/generated",
|
||||
},
|
||||
undefined,
|
||||
undefined,
|
||||
testExtensionContext,
|
||||
);
|
||||
|
||||
expect((result.details as { status?: unknown } | undefined)?.status).toBe("completed");
|
||||
expect(validateWorkdir).toHaveBeenCalledWith("/remote/workspace/generated");
|
||||
expect(mocks.hookRunner.runBeforeToolCall!).toHaveBeenCalledOnce();
|
||||
expect(mocks.hookRunner.runResolveExecEnv!).toHaveBeenCalledOnce();
|
||||
expect(mocks.hookRunner.runResolveExecEnv!.mock.calls[0]?.[0]).toMatchObject({
|
||||
sessionKey: "agent:ctx-agent:telegram:chat-2",
|
||||
toolName: "exec",
|
||||
host: "sandbox",
|
||||
});
|
||||
expect(mocks.hookRunner.runResolveExecEnv!.mock.calls[0]?.[1]).toMatchObject({
|
||||
agentId: "ctx-agent",
|
||||
sessionKey: "agent:ctx-agent:telegram:chat-2",
|
||||
channelId: "ctx-channel",
|
||||
});
|
||||
expect(buildExecSpec.mock.calls[0]?.[0]?.env).toMatchObject({
|
||||
PLUGIN_SAFE: "yes",
|
||||
});
|
||||
});
|
||||
|
||||
it("lets lazy before_tool_call see invalid workdirs before failing unchanged params", async () => {
|
||||
mocks.hookRunner = {
|
||||
hasHooks: vi.fn(
|
||||
|
||||
@@ -143,6 +143,13 @@ type ResolvedExecEnvPreparedState = {
|
||||
pluginEnv?: Record<string, string>;
|
||||
};
|
||||
const resolvedExecEnvPreparedStates = new WeakMap<ExecToolArgs, ResolvedExecEnvPreparedState>();
|
||||
type DeferredResolveExecEnvPreparedState = {
|
||||
hookContext?: HookContext;
|
||||
};
|
||||
const deferredResolveExecEnvPreparedStates = new WeakMap<
|
||||
ExecToolArgs,
|
||||
DeferredResolveExecEnvPreparedState
|
||||
>();
|
||||
type ResolvedExecWorkdirPreparedState = {
|
||||
host: ExecHost;
|
||||
inputWorkdir?: string;
|
||||
@@ -162,6 +169,10 @@ const XML_ARG_VALUE_EXEC_PARAM_KEYS = [
|
||||
"node",
|
||||
] as const;
|
||||
|
||||
function isExecToolArgsObject(value: unknown): value is ExecToolArgs {
|
||||
return typeof value === "object" && value !== null && !Array.isArray(value);
|
||||
}
|
||||
|
||||
function filterPluginExecEnv(rawEnv: Record<string, string>): Record<string, string> | undefined {
|
||||
const env: Record<string, string> = {};
|
||||
for (const [rawKey, value] of Object.entries(rawEnv)) {
|
||||
@@ -201,6 +212,20 @@ function isResolveExecEnvPrepared(params: ExecToolArgs): boolean {
|
||||
return Boolean(getResolvedExecEnvPreparedState(params));
|
||||
}
|
||||
|
||||
function markDeferredResolveExecEnvPrepared<T extends ExecToolArgs>(
|
||||
params: T,
|
||||
state: DeferredResolveExecEnvPreparedState,
|
||||
): T {
|
||||
deferredResolveExecEnvPreparedStates.set(params, state);
|
||||
return params;
|
||||
}
|
||||
|
||||
function getDeferredResolveExecEnvPreparedState(
|
||||
params: ExecToolArgs,
|
||||
): DeferredResolveExecEnvPreparedState | undefined {
|
||||
return deferredResolveExecEnvPreparedStates.get(params);
|
||||
}
|
||||
|
||||
function markResolvedExecWorkdirPrepared<T extends ExecToolArgs>(
|
||||
params: T,
|
||||
state: ResolvedExecWorkdirPreparedState,
|
||||
@@ -1475,20 +1500,31 @@ export function createExecTool(
|
||||
if (workdirState?.resolution.kind === "unavailable") {
|
||||
return params;
|
||||
}
|
||||
if (shouldDeferResolveExecEnvUntilWorkdirValidated(params)) {
|
||||
if (!isExecToolArgsObject(params)) {
|
||||
return params;
|
||||
}
|
||||
if (shouldDeferResolveExecEnvUntilWorkdirValidated(params)) {
|
||||
return markDeferredResolveExecEnvPrepared(params, {
|
||||
hookContext: context.hookContext as HookContext | undefined,
|
||||
});
|
||||
}
|
||||
return prepareParamsWithResolvedExecEnv(params, {
|
||||
hookContext: context.hookContext as HookContext | undefined,
|
||||
});
|
||||
},
|
||||
finalizeBeforeToolCallParams: (params, preparedParams) => {
|
||||
const execParams = params as ExecToolArgs;
|
||||
const envState = getResolvedExecEnvPreparedState(preparedParams as ExecToolArgs);
|
||||
const deferredEnvState = getDeferredResolveExecEnvPreparedState(
|
||||
preparedParams as ExecToolArgs,
|
||||
);
|
||||
const workdirState = getResolvedExecWorkdirPreparedState(preparedParams as ExecToolArgs);
|
||||
if (!envState && !workdirState) {
|
||||
if (!envState && !deferredEnvState && !workdirState) {
|
||||
return params;
|
||||
}
|
||||
if (!isExecToolArgsObject(params)) {
|
||||
return params;
|
||||
}
|
||||
const execParams = params;
|
||||
let host: ExecHost | undefined;
|
||||
const resolveFinalHost = () => {
|
||||
host ??= resolveHostForParams(execParams);
|
||||
@@ -1511,6 +1547,9 @@ export function createExecTool(
|
||||
if (envState) {
|
||||
markResolveExecEnvPrepared(execParams, envState);
|
||||
}
|
||||
if (deferredEnvState) {
|
||||
markDeferredResolveExecEnvPrepared(execParams, deferredEnvState);
|
||||
}
|
||||
if (workdirState) {
|
||||
markResolvedExecWorkdirPrepared(execParams, workdirState);
|
||||
}
|
||||
@@ -1522,6 +1561,7 @@ export function createExecTool(
|
||||
XML_ARG_VALUE_EXEC_PARAM_KEYS,
|
||||
);
|
||||
const resolveExecEnvPrepared = isResolveExecEnvPrepared(args as ExecToolArgs);
|
||||
const deferredResolveExecEnvState = getDeferredResolveExecEnvPreparedState(params);
|
||||
const preparedWorkdirState = getResolvedExecWorkdirPreparedState(params);
|
||||
|
||||
const maxOutput = DEFAULT_MAX_OUTPUT;
|
||||
@@ -1724,7 +1764,9 @@ export function createExecTool(
|
||||
logInfo(`exec: elevated command ${truncateMiddle(params.command, 120)}`);
|
||||
}
|
||||
if (!resolveExecEnvPrepared) {
|
||||
params = await prepareParamsWithResolvedExecEnv(params);
|
||||
params = await prepareParamsWithResolvedExecEnv(params, {
|
||||
hookContext: deferredResolveExecEnvState?.hookContext,
|
||||
});
|
||||
}
|
||||
|
||||
const inheritedBaseEnv = coerceEnv(process.env);
|
||||
|
||||
@@ -53,7 +53,6 @@ export {
|
||||
runSshSandboxCommand,
|
||||
shellEscape,
|
||||
uploadDirectoryToSshTarget,
|
||||
VALIDATE_REMOTE_WORKDIR_SCRIPT,
|
||||
} from "./sandbox/ssh.js";
|
||||
export { sanitizeEnvVars } from "./sandbox/sanitize-env-vars.js";
|
||||
export { createRemoteShellSandboxFsBridge } from "./sandbox/remote-fs-bridge.js";
|
||||
|
||||
Reference in New Issue
Block a user