mirror of
https://github.com/openclaw/openclaw.git
synced 2026-04-13 02:01:16 +00:00
fix(agents): guard nodes tool outPath against workspace boundary [AI-assisted] (#63551)
* fix: address issue * fix: address review feedback * fix: finalize issue changes * fix: address PR review feedback * fix: address PR review feedback * docs: add changelog entry for PR merge
This commit is contained in:
committed by
GitHub
parent
1fede43b94
commit
635bb35b68
@@ -10,6 +10,7 @@ Docs: https://docs.openclaw.ai
|
||||
|
||||
### Fixes
|
||||
|
||||
- fix(agents): guard nodes tool outPath against workspace boundary [AI-assisted]. (#63551) Thanks @pgondhi987.
|
||||
- fix(qqbot): enforce media storage boundary for all outbound local file paths [AI]. (#63271) Thanks @pgondhi987.
|
||||
- iMessage/self-chat: distinguish normal DM outbound rows from true self-chat using `destination_caller_id` plus chat participants, while preserving multi-handle self-chat aliases so outbound DM replies stop looping back as inbound messages. (#61619) Thanks @neeravmakwana.
|
||||
- fix(browser): auto-generate browser control auth token for none/trusted-proxy modes [AI]. (#63280) Thanks @pgondhi987.
|
||||
|
||||
175
src/agents/openclaw-tools.nodes-workspace-guard.test.ts
Normal file
175
src/agents/openclaw-tools.nodes-workspace-guard.test.ts
Normal file
@@ -0,0 +1,175 @@
|
||||
import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import type { AnyAgentTool } from "./tools/common.js";
|
||||
|
||||
const mocks = vi.hoisted(() => ({
|
||||
assertSandboxPath: vi.fn(async (params: { filePath: string; cwd: string; root: string }) => {
|
||||
const root = `/${params.root.replace(/\\/g, "/").replace(/^\/+|\/+$/g, "")}`;
|
||||
const candidate = params.filePath.replace(/\\/g, "/");
|
||||
const input = candidate.startsWith("/") ? candidate : `${root}/${candidate}`;
|
||||
const segments = input.split("/");
|
||||
const stack: string[] = [];
|
||||
for (const segment of segments) {
|
||||
if (!segment || segment === ".") {
|
||||
continue;
|
||||
}
|
||||
if (segment === "..") {
|
||||
stack.pop();
|
||||
continue;
|
||||
}
|
||||
stack.push(segment);
|
||||
}
|
||||
const resolved = `/${stack.join("/")}`;
|
||||
const inside = resolved === root || resolved.startsWith(`${root}/`);
|
||||
if (!inside) {
|
||||
throw new Error(`Path escapes sandbox root (${root}): ${params.filePath}`);
|
||||
}
|
||||
const relative = resolved === root ? "" : resolved.slice(root.length + 1);
|
||||
return { resolved, relative };
|
||||
}),
|
||||
nodesExecute: vi.fn(async () => ({
|
||||
content: [{ type: "text", text: "ok" }],
|
||||
details: {},
|
||||
})),
|
||||
}));
|
||||
|
||||
vi.mock("./sandbox-paths.js", () => ({
|
||||
assertSandboxPath: mocks.assertSandboxPath,
|
||||
}));
|
||||
|
||||
vi.mock("./tools/nodes-tool.js", () => ({
|
||||
createNodesTool: () =>
|
||||
({
|
||||
name: "nodes",
|
||||
label: "Nodes",
|
||||
description: "nodes test tool",
|
||||
parameters: {
|
||||
type: "object",
|
||||
properties: {},
|
||||
},
|
||||
execute: mocks.nodesExecute,
|
||||
}) as unknown as AnyAgentTool,
|
||||
}));
|
||||
|
||||
let createOpenClawTools: typeof import("./openclaw-tools.js").createOpenClawTools;
|
||||
|
||||
const WORKSPACE_ROOT = "/tmp/openclaw-workspace-nodes-guard";
|
||||
|
||||
describe("createOpenClawTools nodes workspace guard", () => {
|
||||
beforeAll(async () => {
|
||||
vi.resetModules();
|
||||
({ createOpenClawTools } = await import("./openclaw-tools.js"));
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
mocks.assertSandboxPath.mockClear();
|
||||
mocks.nodesExecute.mockClear();
|
||||
});
|
||||
|
||||
function getNodesTool(
|
||||
workspaceOnly: boolean,
|
||||
options?: { sandboxRoot?: string; sandboxContainerWorkdir?: string },
|
||||
): AnyAgentTool {
|
||||
const tools = createOpenClawTools({
|
||||
workspaceDir: WORKSPACE_ROOT,
|
||||
fsPolicy: { workspaceOnly },
|
||||
sandboxRoot: options?.sandboxRoot,
|
||||
sandboxContainerWorkdir: options?.sandboxContainerWorkdir,
|
||||
disablePluginTools: true,
|
||||
disableMessageTool: true,
|
||||
});
|
||||
const nodesTool = tools.find((tool) => tool.name === "nodes");
|
||||
expect(nodesTool).toBeDefined();
|
||||
if (!nodesTool) {
|
||||
throw new Error("missing nodes tool");
|
||||
}
|
||||
return nodesTool;
|
||||
}
|
||||
|
||||
it("guards outPath when workspaceOnly is enabled", async () => {
|
||||
const nodesTool = getNodesTool(true);
|
||||
await nodesTool.execute("call-1", {
|
||||
action: "screen_record",
|
||||
outPath: `${WORKSPACE_ROOT}/videos/capture.mp4`,
|
||||
});
|
||||
|
||||
expect(mocks.assertSandboxPath).toHaveBeenCalledWith({
|
||||
filePath: `${WORKSPACE_ROOT}/videos/capture.mp4`,
|
||||
cwd: WORKSPACE_ROOT,
|
||||
root: WORKSPACE_ROOT,
|
||||
});
|
||||
expect(mocks.nodesExecute).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("normalizes relative outPath to an absolute workspace path before execute", async () => {
|
||||
const nodesTool = getNodesTool(true);
|
||||
await nodesTool.execute("call-rel", {
|
||||
action: "screen_record",
|
||||
outPath: "videos/capture.mp4",
|
||||
});
|
||||
|
||||
expect(mocks.assertSandboxPath).toHaveBeenCalledWith({
|
||||
filePath: "videos/capture.mp4",
|
||||
cwd: WORKSPACE_ROOT,
|
||||
root: WORKSPACE_ROOT,
|
||||
});
|
||||
expect(mocks.nodesExecute).toHaveBeenCalledWith(
|
||||
"call-rel",
|
||||
{
|
||||
action: "screen_record",
|
||||
outPath: `${WORKSPACE_ROOT}/videos/capture.mp4`,
|
||||
},
|
||||
undefined,
|
||||
undefined,
|
||||
);
|
||||
});
|
||||
|
||||
it("maps sandbox container outPath to host root when containerWorkdir is provided", async () => {
|
||||
const nodesTool = getNodesTool(true, {
|
||||
sandboxRoot: WORKSPACE_ROOT,
|
||||
sandboxContainerWorkdir: "/workspace",
|
||||
});
|
||||
await nodesTool.execute("call-sandbox", {
|
||||
action: "screen_record",
|
||||
outPath: "/workspace/videos/capture.mp4",
|
||||
});
|
||||
|
||||
expect(mocks.assertSandboxPath).toHaveBeenCalledWith({
|
||||
filePath: `${WORKSPACE_ROOT}/videos/capture.mp4`,
|
||||
cwd: WORKSPACE_ROOT,
|
||||
root: WORKSPACE_ROOT,
|
||||
});
|
||||
expect(mocks.nodesExecute).toHaveBeenCalledWith(
|
||||
"call-sandbox",
|
||||
{
|
||||
action: "screen_record",
|
||||
outPath: `${WORKSPACE_ROOT}/videos/capture.mp4`,
|
||||
},
|
||||
undefined,
|
||||
undefined,
|
||||
);
|
||||
});
|
||||
|
||||
it("rejects outPath outside workspace when workspaceOnly is enabled", async () => {
|
||||
const nodesTool = getNodesTool(true);
|
||||
await expect(
|
||||
nodesTool.execute("call-2", {
|
||||
action: "screen_record",
|
||||
outPath: "/etc/passwd",
|
||||
}),
|
||||
).rejects.toThrow(/Path escapes sandbox root/);
|
||||
|
||||
expect(mocks.assertSandboxPath).toHaveBeenCalledTimes(1);
|
||||
expect(mocks.nodesExecute).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("does not guard outPath when workspaceOnly is disabled", async () => {
|
||||
const nodesTool = getNodesTool(false);
|
||||
await nodesTool.execute("call-3", {
|
||||
action: "screen_record",
|
||||
outPath: "/etc/passwd",
|
||||
});
|
||||
|
||||
expect(mocks.assertSandboxPath).not.toHaveBeenCalled();
|
||||
expect(mocks.nodesExecute).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
@@ -9,6 +9,7 @@ import {
|
||||
collectPresentOpenClawTools,
|
||||
isUpdatePlanToolEnabledForOpenClawTools,
|
||||
} from "./openclaw-tools.registration.js";
|
||||
import { wrapToolWorkspaceRootGuardWithOptions } from "./pi-tools.read.js";
|
||||
import type { SandboxFsBridge } from "./sandbox/fs-bridge.js";
|
||||
import type { SpawnedToolContext } from "./spawned-context.js";
|
||||
import type { ToolFsPolicy } from "./tool-fs-policy.js";
|
||||
@@ -60,6 +61,7 @@ export function createOpenClawTools(
|
||||
agentThreadId?: string | number;
|
||||
agentDir?: string;
|
||||
sandboxRoot?: string;
|
||||
sandboxContainerWorkdir?: string;
|
||||
sandboxFsBridge?: SandboxFsBridge;
|
||||
fsPolicy?: ToolFsPolicy;
|
||||
sandboxed?: boolean;
|
||||
@@ -205,18 +207,27 @@ export function createOpenClawTools(
|
||||
requireExplicitTarget: options?.requireExplicitMessageTarget,
|
||||
requesterSenderId: options?.requesterSenderId ?? undefined,
|
||||
});
|
||||
const nodesToolBase = createNodesTool({
|
||||
agentSessionKey: options?.agentSessionKey,
|
||||
agentChannel: options?.agentChannel,
|
||||
agentAccountId: options?.agentAccountId,
|
||||
currentChannelId: options?.currentChannelId,
|
||||
currentThreadTs: options?.currentThreadTs,
|
||||
config: options?.config,
|
||||
modelHasVision: options?.modelHasVision,
|
||||
allowMediaInvokeCommands: options?.allowMediaInvokeCommands,
|
||||
});
|
||||
const nodesTool =
|
||||
options?.fsPolicy?.workspaceOnly === true
|
||||
? wrapToolWorkspaceRootGuardWithOptions(nodesToolBase, options?.sandboxRoot ?? workspaceDir, {
|
||||
containerWorkdir: options?.sandboxContainerWorkdir,
|
||||
pathParamKeys: ["outPath"],
|
||||
normalizeGuardedPathParams: true,
|
||||
})
|
||||
: nodesToolBase;
|
||||
const tools: AnyAgentTool[] = [
|
||||
createCanvasTool({ config: options?.config }),
|
||||
createNodesTool({
|
||||
agentSessionKey: options?.agentSessionKey,
|
||||
agentChannel: options?.agentChannel,
|
||||
agentAccountId: options?.agentAccountId,
|
||||
currentChannelId: options?.currentChannelId,
|
||||
currentThreadTs: options?.currentThreadTs,
|
||||
config: options?.config,
|
||||
modelHasVision: options?.modelHasVision,
|
||||
allowMediaInvokeCommands: options?.allowMediaInvokeCommands,
|
||||
}),
|
||||
nodesTool,
|
||||
createCronTool({
|
||||
agentSessionKey: options?.agentSessionKey,
|
||||
}),
|
||||
|
||||
@@ -551,22 +551,34 @@ export function wrapToolWorkspaceRootGuardWithOptions(
|
||||
root: string,
|
||||
options?: {
|
||||
containerWorkdir?: string;
|
||||
pathParamKeys?: readonly string[];
|
||||
normalizeGuardedPathParams?: boolean;
|
||||
},
|
||||
): AnyAgentTool {
|
||||
const pathParamKeys =
|
||||
options?.pathParamKeys && options.pathParamKeys.length > 0 ? options.pathParamKeys : ["path"];
|
||||
return {
|
||||
...tool,
|
||||
execute: async (toolCallId, args, signal, onUpdate) => {
|
||||
const record = getToolParamsRecord(args);
|
||||
const filePath = record?.path;
|
||||
if (typeof filePath === "string" && filePath.trim()) {
|
||||
let normalizedRecord: Record<string, unknown> | undefined;
|
||||
for (const key of pathParamKeys) {
|
||||
const filePath = record?.[key];
|
||||
if (typeof filePath !== "string" || !filePath.trim()) {
|
||||
continue;
|
||||
}
|
||||
const sandboxPath = mapContainerPathToWorkspaceRoot({
|
||||
filePath,
|
||||
root,
|
||||
containerWorkdir: options?.containerWorkdir,
|
||||
});
|
||||
await assertSandboxPath({ filePath: sandboxPath, cwd: root, root });
|
||||
const sandboxResult = await assertSandboxPath({ filePath: sandboxPath, cwd: root, root });
|
||||
if (options?.normalizeGuardedPathParams && record) {
|
||||
normalizedRecord ??= { ...record };
|
||||
normalizedRecord[key] = sandboxResult.resolved;
|
||||
}
|
||||
}
|
||||
return tool.execute(toolCallId, args, signal, onUpdate);
|
||||
return tool.execute(toolCallId, normalizedRecord ?? args, signal, onUpdate);
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
@@ -127,4 +127,38 @@ describe("wrapToolWorkspaceRootGuardWithOptions", () => {
|
||||
root,
|
||||
});
|
||||
});
|
||||
|
||||
it("does not guard outPath by default", async () => {
|
||||
const { tool } = createToolHarness();
|
||||
const wrapped = wrapToolWorkspaceRootGuardWithOptions(tool, root, {
|
||||
containerWorkdir: "/workspace",
|
||||
});
|
||||
|
||||
await wrapped.execute("tc-outpath-default", { outPath: "/workspace/videos/capture.mp4" });
|
||||
|
||||
expect(mocks.assertSandboxPath).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("guards custom outPath params when configured", async () => {
|
||||
const { execute, tool } = createToolHarness();
|
||||
const wrapped = wrapToolWorkspaceRootGuardWithOptions(tool, root, {
|
||||
containerWorkdir: "/workspace",
|
||||
pathParamKeys: ["outPath"],
|
||||
normalizeGuardedPathParams: true,
|
||||
});
|
||||
|
||||
await wrapped.execute("tc-outpath-custom", { outPath: "videos/capture.mp4" });
|
||||
|
||||
expect(mocks.assertSandboxPath).toHaveBeenCalledWith({
|
||||
filePath: "videos/capture.mp4",
|
||||
cwd: root,
|
||||
root,
|
||||
});
|
||||
expect(execute).toHaveBeenCalledWith(
|
||||
"tc-outpath-custom",
|
||||
{ outPath: path.resolve(root, "videos", "capture.mp4") },
|
||||
undefined,
|
||||
undefined,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -532,6 +532,7 @@ export function createOpenClawCodingTools(options?: {
|
||||
agentGroupSpace: options?.groupSpace ?? null,
|
||||
agentDir: options?.agentDir,
|
||||
sandboxRoot,
|
||||
sandboxContainerWorkdir: sandbox?.containerWorkdir,
|
||||
sandboxFsBridge,
|
||||
fsPolicy,
|
||||
workspaceDir: workspaceRoot,
|
||||
|
||||
Reference in New Issue
Block a user