mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-08 05:41:24 +00:00
fix(security): add optional workspace-only path guards for fs tools
This commit is contained in:
@@ -81,7 +81,9 @@ describe("applyPatch", () => {
|
||||
+escaped
|
||||
*** End Patch`;
|
||||
|
||||
await expect(applyPatch(patch, { cwd: dir })).rejects.toThrow(/Path escapes sandbox root/);
|
||||
await expect(applyPatch(patch, { cwd: dir, workspaceOnly: true })).rejects.toThrow(
|
||||
/Path escapes sandbox root/,
|
||||
);
|
||||
await expect(fs.readFile(escapedPath, "utf8")).rejects.toBeDefined();
|
||||
});
|
||||
});
|
||||
@@ -96,7 +98,9 @@ describe("applyPatch", () => {
|
||||
*** End Patch`;
|
||||
|
||||
try {
|
||||
await expect(applyPatch(patch, { cwd: dir })).rejects.toThrow(/Path escapes sandbox root/);
|
||||
await expect(applyPatch(patch, { cwd: dir, workspaceOnly: true })).rejects.toThrow(
|
||||
/Path escapes sandbox root/,
|
||||
);
|
||||
await expect(fs.readFile(escapedPath, "utf8")).rejects.toBeDefined();
|
||||
} finally {
|
||||
await fs.rm(escapedPath, { force: true });
|
||||
@@ -112,7 +116,7 @@ describe("applyPatch", () => {
|
||||
+inside
|
||||
*** End Patch`;
|
||||
|
||||
await applyPatch(patch, { cwd: dir });
|
||||
await applyPatch(patch, { cwd: dir, workspaceOnly: true });
|
||||
const contents = await fs.readFile(target, "utf8");
|
||||
expect(contents).toBe("inside\n");
|
||||
});
|
||||
@@ -132,10 +136,32 @@ describe("applyPatch", () => {
|
||||
+pwned
|
||||
*** End Patch`;
|
||||
|
||||
await expect(applyPatch(patch, { cwd: dir })).rejects.toThrow(/Symlink not allowed/);
|
||||
await expect(applyPatch(patch, { cwd: dir, workspaceOnly: true })).rejects.toThrow(
|
||||
/Symlink escapes sandbox root/,
|
||||
);
|
||||
const outsideContents = await fs.readFile(outside, "utf8");
|
||||
expect(outsideContents).toBe("initial\n");
|
||||
await fs.rm(outside, { force: true });
|
||||
});
|
||||
});
|
||||
|
||||
it("allows symlinks that resolve within cwd", async () => {
|
||||
await withTempDir(async (dir) => {
|
||||
const target = path.join(dir, "target.txt");
|
||||
const linkPath = path.join(dir, "link.txt");
|
||||
await fs.writeFile(target, "initial\n", "utf8");
|
||||
await fs.symlink(target, linkPath);
|
||||
|
||||
const patch = `*** Begin Patch
|
||||
*** Update File: link.txt
|
||||
@@
|
||||
-initial
|
||||
+updated
|
||||
*** End Patch`;
|
||||
|
||||
await applyPatch(patch, { cwd: dir, workspaceOnly: true });
|
||||
const contents = await fs.readFile(target, "utf8");
|
||||
expect(contents).toBe("updated\n");
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,10 +1,11 @@
|
||||
import type { AgentTool } from "@mariozechner/pi-agent-core";
|
||||
import { Type } from "@sinclair/typebox";
|
||||
import fs from "node:fs/promises";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import type { SandboxFsBridge } from "./sandbox/fs-bridge.js";
|
||||
import { applyUpdateHunk } from "./apply-patch-update.js";
|
||||
import { assertSandboxPath } from "./sandbox-paths.js";
|
||||
import { assertSandboxPath, resolveSandboxPath } from "./sandbox-paths.js";
|
||||
|
||||
const BEGIN_PATCH_MARKER = "*** Begin Patch";
|
||||
const END_PATCH_MARKER = "*** End Patch";
|
||||
@@ -66,6 +67,8 @@ type SandboxApplyPatchConfig = {
|
||||
type ApplyPatchOptions = {
|
||||
cwd: string;
|
||||
sandbox?: SandboxApplyPatchConfig;
|
||||
/** When true, restrict patch paths to the workspace root (cwd). Default: false. */
|
||||
workspaceOnly?: boolean;
|
||||
signal?: AbortSignal;
|
||||
};
|
||||
|
||||
@@ -76,10 +79,11 @@ const applyPatchSchema = Type.Object({
|
||||
});
|
||||
|
||||
export function createApplyPatchTool(
|
||||
options: { cwd?: string; sandbox?: SandboxApplyPatchConfig } = {},
|
||||
options: { cwd?: string; sandbox?: SandboxApplyPatchConfig; workspaceOnly?: boolean } = {},
|
||||
): AgentTool<typeof applyPatchSchema, ApplyPatchToolDetails> {
|
||||
const cwd = options.cwd ?? process.cwd();
|
||||
const sandbox = options.sandbox;
|
||||
const workspaceOnly = options.workspaceOnly === true;
|
||||
|
||||
return {
|
||||
name: "apply_patch",
|
||||
@@ -102,6 +106,7 @@ export function createApplyPatchTool(
|
||||
const result = await applyPatch(input, {
|
||||
cwd,
|
||||
sandbox,
|
||||
workspaceOnly,
|
||||
signal,
|
||||
});
|
||||
|
||||
@@ -150,7 +155,7 @@ export async function applyPatch(
|
||||
}
|
||||
|
||||
if (hunk.kind === "delete") {
|
||||
const target = await resolvePatchPath(hunk.path, options);
|
||||
const target = await resolvePatchPath(hunk.path, options, "unlink");
|
||||
await fileOps.remove(target.resolved);
|
||||
recordSummary(summary, seen, "deleted", target.display);
|
||||
continue;
|
||||
@@ -249,6 +254,7 @@ async function ensureDir(filePath: string, ops: PatchFileOps) {
|
||||
async function resolvePatchPath(
|
||||
filePath: string,
|
||||
options: ApplyPatchOptions,
|
||||
purpose: "readWrite" | "unlink" = "readWrite",
|
||||
): Promise<{ resolved: string; display: string }> {
|
||||
if (options.sandbox) {
|
||||
const resolved = options.sandbox.bridge.resolvePath({
|
||||
@@ -261,17 +267,48 @@ async function resolvePatchPath(
|
||||
};
|
||||
}
|
||||
|
||||
const resolved = await assertSandboxPath({
|
||||
filePath,
|
||||
cwd: options.cwd,
|
||||
root: options.cwd,
|
||||
});
|
||||
const resolved = options.workspaceOnly
|
||||
? purpose === "unlink"
|
||||
? resolveSandboxPath({ filePath, cwd: options.cwd, root: options.cwd }).resolved
|
||||
: (
|
||||
await assertSandboxPath({
|
||||
filePath,
|
||||
cwd: options.cwd,
|
||||
root: options.cwd,
|
||||
})
|
||||
).resolved
|
||||
: resolvePathFromCwd(filePath, options.cwd);
|
||||
return {
|
||||
resolved: resolved.resolved,
|
||||
display: toDisplayPath(resolved.resolved, options.cwd),
|
||||
resolved,
|
||||
display: toDisplayPath(resolved, options.cwd),
|
||||
};
|
||||
}
|
||||
|
||||
const UNICODE_SPACES = /[\u00A0\u2000-\u200A\u202F\u205F\u3000]/g;
|
||||
|
||||
function normalizeUnicodeSpaces(value: string): string {
|
||||
return value.replace(UNICODE_SPACES, " ");
|
||||
}
|
||||
|
||||
function expandPath(filePath: string): string {
|
||||
const normalized = normalizeUnicodeSpaces(filePath);
|
||||
if (normalized === "~") {
|
||||
return os.homedir();
|
||||
}
|
||||
if (normalized.startsWith("~/")) {
|
||||
return os.homedir() + normalized.slice(1);
|
||||
}
|
||||
return normalized;
|
||||
}
|
||||
|
||||
function resolvePathFromCwd(filePath: string, cwd: string): string {
|
||||
const expanded = expandPath(filePath);
|
||||
if (path.isAbsolute(expanded)) {
|
||||
return path.normalize(expanded);
|
||||
}
|
||||
return path.resolve(cwd, expanded);
|
||||
}
|
||||
|
||||
function toDisplayPath(resolved: string, cwd: string): string {
|
||||
const relative = path.relative(cwd, resolved);
|
||||
if (!relative || relative === "") {
|
||||
|
||||
@@ -252,6 +252,23 @@ export function wrapToolParamNormalization(
|
||||
};
|
||||
}
|
||||
|
||||
export function wrapToolWorkspaceRootGuard(tool: AnyAgentTool, root: string): AnyAgentTool {
|
||||
return {
|
||||
...tool,
|
||||
execute: async (toolCallId, args, signal, onUpdate) => {
|
||||
const normalized = normalizeToolParams(args);
|
||||
const record =
|
||||
normalized ??
|
||||
(args && typeof args === "object" ? (args as Record<string, unknown>) : undefined);
|
||||
const filePath = record?.path;
|
||||
if (typeof filePath === "string" && filePath.trim()) {
|
||||
await assertSandboxPath({ filePath, cwd: root, root });
|
||||
}
|
||||
return tool.execute(toolCallId, normalized ?? args, signal, onUpdate);
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
function wrapSandboxPathGuard(tool: AnyAgentTool, root: string): AnyAgentTool {
|
||||
return {
|
||||
...tool,
|
||||
|
||||
@@ -40,6 +40,7 @@ import {
|
||||
createSandboxedWriteTool,
|
||||
normalizeToolParams,
|
||||
patchToolSchemaForClaudeCompatibility,
|
||||
wrapToolWorkspaceRootGuard,
|
||||
wrapToolParamNormalization,
|
||||
} from "./pi-tools.read.js";
|
||||
import { cleanToolSchemaForGemini, normalizeToolParameters } from "./pi-tools.schema.js";
|
||||
@@ -108,6 +109,16 @@ function resolveExecConfig(params: { cfg?: OpenClawConfig; agentId?: string }) {
|
||||
};
|
||||
}
|
||||
|
||||
function resolveFsConfig(params: { cfg?: OpenClawConfig; agentId?: string }) {
|
||||
const cfg = params.cfg;
|
||||
const globalFs = cfg?.tools?.fs;
|
||||
const agentFs =
|
||||
cfg && params.agentId ? resolveAgentConfig(cfg, params.agentId)?.tools?.fs : undefined;
|
||||
return {
|
||||
workspaceOnly: agentFs?.workspaceOnly ?? globalFs?.workspaceOnly,
|
||||
};
|
||||
}
|
||||
|
||||
export const __testing = {
|
||||
cleanToolSchemaForGemini,
|
||||
normalizeToolParams,
|
||||
@@ -236,11 +247,14 @@ export function createOpenClawCodingTools(options?: {
|
||||
subagentPolicy,
|
||||
]);
|
||||
const execConfig = resolveExecConfig({ cfg: options?.config, agentId });
|
||||
const fsConfig = resolveFsConfig({ cfg: options?.config, agentId });
|
||||
const sandboxRoot = sandbox?.workspaceDir;
|
||||
const sandboxFsBridge = sandbox?.fsBridge;
|
||||
const allowWorkspaceWrites = sandbox?.workspaceAccess !== "ro";
|
||||
const workspaceRoot = options?.workspaceDir ?? process.cwd();
|
||||
const applyPatchConfig = options?.config?.tools?.exec?.applyPatch;
|
||||
const workspaceOnly = fsConfig.workspaceOnly === true;
|
||||
const applyPatchConfig = execConfig.applyPatch;
|
||||
const applyPatchWorkspaceOnly = workspaceOnly || applyPatchConfig?.workspaceOnly === true;
|
||||
const applyPatchEnabled =
|
||||
!!applyPatchConfig?.enabled &&
|
||||
isOpenAIProvider(options?.modelProvider) &&
|
||||
@@ -265,7 +279,8 @@ export function createOpenClawCodingTools(options?: {
|
||||
];
|
||||
}
|
||||
const freshReadTool = createReadTool(workspaceRoot);
|
||||
return [createOpenClawReadTool(freshReadTool)];
|
||||
const wrapped = createOpenClawReadTool(freshReadTool);
|
||||
return [workspaceOnly ? wrapToolWorkspaceRootGuard(wrapped, workspaceRoot) : wrapped];
|
||||
}
|
||||
if (tool.name === "bash" || tool.name === execToolName) {
|
||||
return [];
|
||||
@@ -275,16 +290,22 @@ export function createOpenClawCodingTools(options?: {
|
||||
return [];
|
||||
}
|
||||
// Wrap with param normalization for Claude Code compatibility
|
||||
return [
|
||||
wrapToolParamNormalization(createWriteTool(workspaceRoot), CLAUDE_PARAM_GROUPS.write),
|
||||
];
|
||||
const wrapped = wrapToolParamNormalization(
|
||||
createWriteTool(workspaceRoot),
|
||||
CLAUDE_PARAM_GROUPS.write,
|
||||
);
|
||||
return [workspaceOnly ? wrapToolWorkspaceRootGuard(wrapped, workspaceRoot) : wrapped];
|
||||
}
|
||||
if (tool.name === "edit") {
|
||||
if (sandboxRoot) {
|
||||
return [];
|
||||
}
|
||||
// Wrap with param normalization for Claude Code compatibility
|
||||
return [wrapToolParamNormalization(createEditTool(workspaceRoot), CLAUDE_PARAM_GROUPS.edit)];
|
||||
const wrapped = wrapToolParamNormalization(
|
||||
createEditTool(workspaceRoot),
|
||||
CLAUDE_PARAM_GROUPS.edit,
|
||||
);
|
||||
return [workspaceOnly ? wrapToolWorkspaceRootGuard(wrapped, workspaceRoot) : wrapped];
|
||||
}
|
||||
return [tool];
|
||||
});
|
||||
@@ -330,6 +351,7 @@ export function createOpenClawCodingTools(options?: {
|
||||
sandboxRoot && allowWorkspaceWrites
|
||||
? { root: sandboxRoot, bridge: sandboxFsBridge! }
|
||||
: undefined,
|
||||
workspaceOnly: applyPatchWorkspaceOnly,
|
||||
});
|
||||
const tools: AnyAgentTool[] = [
|
||||
...base,
|
||||
|
||||
@@ -48,7 +48,7 @@ export function resolveSandboxPath(params: { filePath: string; cwd: string; root
|
||||
|
||||
export async function assertSandboxPath(params: { filePath: string; cwd: string; root: string }) {
|
||||
const resolved = resolveSandboxPath(params);
|
||||
await assertNoSymlink(resolved.relative, path.resolve(params.root));
|
||||
await assertNoSymlinkEscape(resolved.relative, path.resolve(params.root));
|
||||
return resolved;
|
||||
}
|
||||
|
||||
@@ -86,10 +86,11 @@ export async function resolveSandboxedMediaSource(params: {
|
||||
return resolved.resolved;
|
||||
}
|
||||
|
||||
async function assertNoSymlink(relative: string, root: string) {
|
||||
async function assertNoSymlinkEscape(relative: string, root: string) {
|
||||
if (!relative) {
|
||||
return;
|
||||
}
|
||||
const rootReal = await tryRealpath(root);
|
||||
const parts = relative.split(path.sep).filter(Boolean);
|
||||
let current = root;
|
||||
for (const part of parts) {
|
||||
@@ -97,7 +98,13 @@ async function assertNoSymlink(relative: string, root: string) {
|
||||
try {
|
||||
const stat = await fs.lstat(current);
|
||||
if (stat.isSymbolicLink()) {
|
||||
throw new Error(`Symlink not allowed in sandbox path: ${current}`);
|
||||
const target = await tryRealpath(current);
|
||||
if (!isPathInside(rootReal, target)) {
|
||||
throw new Error(
|
||||
`Symlink escapes sandbox root (${shortPath(rootReal)}): ${shortPath(current)}`,
|
||||
);
|
||||
}
|
||||
current = target;
|
||||
}
|
||||
} catch (err) {
|
||||
const anyErr = err as { code?: string };
|
||||
@@ -109,6 +116,22 @@ async function assertNoSymlink(relative: string, root: string) {
|
||||
}
|
||||
}
|
||||
|
||||
async function tryRealpath(value: string): Promise<string> {
|
||||
try {
|
||||
return await fs.realpath(value);
|
||||
} catch {
|
||||
return path.resolve(value);
|
||||
}
|
||||
}
|
||||
|
||||
function isPathInside(root: string, target: string): boolean {
|
||||
const relative = path.relative(root, target);
|
||||
if (!relative || relative === "") {
|
||||
return true;
|
||||
}
|
||||
return !(relative.startsWith("..") || path.isAbsolute(relative));
|
||||
}
|
||||
|
||||
function shortPath(value: string) {
|
||||
if (value.startsWith(os.homedir())) {
|
||||
return `~${value.slice(os.homedir().length)}`;
|
||||
|
||||
Reference in New Issue
Block a user