From 5e7c3250cb16df575cf22915b6b398923b340da7 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 14 Feb 2026 23:50:04 +0100 Subject: [PATCH] fix(security): add optional workspace-only path guards for fs tools --- SECURITY.md | 6 +++ docs/gateway/security/index.md | 5 +++ docs/tools/apply-patch.md | 3 +- docs/tools/exec.md | 3 +- docs/tools/index.md | 1 + src/agents/apply-patch.e2e.test.ts | 34 +++++++++++++-- src/agents/apply-patch.ts | 57 +++++++++++++++++++++----- src/agents/pi-tools.read.ts | 17 ++++++++ src/agents/pi-tools.ts | 34 ++++++++++++--- src/agents/sandbox-paths.ts | 29 +++++++++++-- src/config/schema.help.ts | 4 ++ src/config/schema.labels.ts | 2 + src/config/types.tools.ts | 17 ++++++++ src/config/zod-schema.agent-runtime.ts | 14 +++++++ 14 files changed, 201 insertions(+), 25 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index a0a43fac3ee..63440837047 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -55,6 +55,12 @@ For threat model + hardening guidance (including `openclaw security audit --deep - `https://docs.openclaw.ai/gateway/security` +### Tool filesystem hardening + +- `tools.exec.applyPatch.workspaceOnly: true` (recommended): keeps `apply_patch` writes/deletes within the configured workspace directory. +- `tools.fs.workspaceOnly: true` (optional): restricts `read`/`write`/`edit`/`apply_patch` paths to the workspace directory. +- Avoid setting `tools.exec.applyPatch.workspaceOnly: false` unless you fully trust who can trigger tool execution. + ### Web Interface Safety OpenClaw's web interface (Gateway Control UI + HTTP endpoints) is intended for **local use only**. diff --git a/docs/gateway/security/index.md b/docs/gateway/security/index.md index 9f8f831f148..b3f11d8d84f 100644 --- a/docs/gateway/security/index.md +++ b/docs/gateway/security/index.md @@ -577,6 +577,11 @@ You can already build a read-only profile by combining: We may add a single `readOnlyMode` flag later to simplify this configuration. +Additional hardening options: + +- `tools.exec.applyPatch.workspaceOnly: true` (recommended): ensures `apply_patch` cannot write/delete outside the workspace directory even when sandboxing is off. +- `tools.fs.workspaceOnly: true` (optional): restricts `read`/`write`/`edit`/`apply_patch` paths to the workspace directory (useful if you allow absolute paths today and want a single guardrail). + ### 5) Secure baseline (copy/paste) One “safe default” config that keeps the Gateway private, requires DM pairing, and avoids always-on group bots: diff --git a/docs/tools/apply-patch.md b/docs/tools/apply-patch.md index 5b2ab5d8e3c..fb10adc2cef 100644 --- a/docs/tools/apply-patch.md +++ b/docs/tools/apply-patch.md @@ -32,7 +32,8 @@ The tool accepts a single `input` string that wraps one or more file operations: ## Notes -- Paths are resolved relative to the workspace root. +- Patch paths support relative paths (from the workspace directory) and absolute paths. +- Optional: set `tools.exec.applyPatch.workspaceOnly: true` to restrict patch paths to the workspace directory (recommended when untrusted users can trigger tool execution). - Use `*** Move to:` within an `*** Update File:` hunk to rename files. - `*** End of File` marks an EOF-only insert when needed. - Experimental and disabled by default. Enable with `tools.exec.applyPatch.enabled`. diff --git a/docs/tools/exec.md b/docs/tools/exec.md index 27735f60d1b..47a79537353 100644 --- a/docs/tools/exec.md +++ b/docs/tools/exec.md @@ -167,7 +167,7 @@ Enable it explicitly: { tools: { exec: { - applyPatch: { enabled: true, allowModels: ["gpt-5.2"] }, + applyPatch: { enabled: true, workspaceOnly: true, allowModels: ["gpt-5.2"] }, }, }, } @@ -178,3 +178,4 @@ Notes: - Only available for OpenAI/OpenAI Codex models. - Tool policy still applies; `allow: ["exec"]` implicitly allows `apply_patch`. - Config lives under `tools.exec.applyPatch`. +- Optional: set `tools.exec.applyPatch.workspaceOnly: true` to restrict patch paths to the workspace directory (recommended when untrusted users can trigger tool execution). diff --git a/docs/tools/index.md b/docs/tools/index.md index 7e6fa8017c0..d0079c16975 100644 --- a/docs/tools/index.md +++ b/docs/tools/index.md @@ -181,6 +181,7 @@ Optional plugin tools: Apply structured patches across one or more files. Use for multi-hunk edits. Experimental: enable via `tools.exec.applyPatch.enabled` (OpenAI models only). +Optional: restrict patch paths to the workspace directory with `tools.exec.applyPatch.workspaceOnly: true`. ### `exec` diff --git a/src/agents/apply-patch.e2e.test.ts b/src/agents/apply-patch.e2e.test.ts index 55ce31f7af9..be89c1009ec 100644 --- a/src/agents/apply-patch.e2e.test.ts +++ b/src/agents/apply-patch.e2e.test.ts @@ -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"); + }); + }); }); diff --git a/src/agents/apply-patch.ts b/src/agents/apply-patch.ts index 47c5b8ca6c4..7f22e20c8b3 100644 --- a/src/agents/apply-patch.ts +++ b/src/agents/apply-patch.ts @@ -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 { 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 === "") { diff --git a/src/agents/pi-tools.read.ts b/src/agents/pi-tools.read.ts index 30ca5fec3e5..6a45de5d962 100644 --- a/src/agents/pi-tools.read.ts +++ b/src/agents/pi-tools.read.ts @@ -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) : 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, diff --git a/src/agents/pi-tools.ts b/src/agents/pi-tools.ts index 2fd6793b8ee..bab76895740 100644 --- a/src/agents/pi-tools.ts +++ b/src/agents/pi-tools.ts @@ -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, diff --git a/src/agents/sandbox-paths.ts b/src/agents/sandbox-paths.ts index 22c72947a51..2d39572e39d 100644 --- a/src/agents/sandbox-paths.ts +++ b/src/agents/sandbox-paths.ts @@ -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 { + 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)}`; diff --git a/src/config/schema.help.ts b/src/config/schema.help.ts index 14c735f9793..aef5edf83bf 100644 --- a/src/config/schema.help.ts +++ b/src/config/schema.help.ts @@ -58,6 +58,8 @@ export const FIELD_HELP: Record = { "diagnostics.cacheTrace.includeSystem": "Include system prompt in trace output (default: true).", "tools.exec.applyPatch.enabled": "Experimental. Enables apply_patch for OpenAI models when allowed by tool policy.", + "tools.exec.applyPatch.workspaceOnly": + "Restrict apply_patch paths to the workspace directory (default: false).", "tools.exec.applyPatch.allowModels": 'Optional allowlist of model ids (e.g. "gpt-5.2" or "openai/gpt-5.2").', "tools.exec.notifyOnExit": @@ -65,6 +67,8 @@ export const FIELD_HELP: Record = { "tools.exec.pathPrepend": "Directories to prepend to PATH for exec runs (gateway/sandbox).", "tools.exec.safeBins": "Allow stdin-only safe binaries to run without explicit allowlist entries.", + "tools.fs.workspaceOnly": + "Restrict filesystem tools (read/write/edit/apply_patch) to the workspace directory (default: false).", "tools.message.allowCrossContextSend": "Legacy override: allow cross-context sends across all providers.", "tools.message.crossContext.allowWithinProvider": diff --git a/src/config/schema.labels.ts b/src/config/schema.labels.ts index c93aa5ceb70..7afdf6c4eeb 100644 --- a/src/config/schema.labels.ts +++ b/src/config/schema.labels.ts @@ -71,7 +71,9 @@ export const FIELD_LABELS: Record = { "tools.byProvider": "Tool Policy by Provider", "agents.list[].tools.byProvider": "Agent Tool Policy by Provider", "tools.exec.applyPatch.enabled": "Enable apply_patch", + "tools.exec.applyPatch.workspaceOnly": "apply_patch Workspace-Only", "tools.exec.applyPatch.allowModels": "apply_patch Model Allowlist", + "tools.fs.workspaceOnly": "Workspace-only FS tools", "tools.exec.notifyOnExit": "Exec Notify On Exit", "tools.exec.approvalRunningNoticeMs": "Exec Approval Running Notice (ms)", "tools.exec.host": "Exec Host", diff --git a/src/config/types.tools.ts b/src/config/types.tools.ts index d5292e7c26f..fe9feba2615 100644 --- a/src/config/types.tools.ts +++ b/src/config/types.tools.ts @@ -187,6 +187,11 @@ export type ExecToolConfig = { applyPatch?: { /** Enable apply_patch for OpenAI models (default: false). */ enabled?: boolean; + /** + * Restrict apply_patch paths to the workspace directory. + * Default: true (safer; does not affect read/write/edit). + */ + workspaceOnly?: boolean; /** * Optional allowlist of model ids that can use apply_patch. * Accepts either raw ids (e.g. "gpt-5.2") or full ids (e.g. "openai/gpt-5.2"). @@ -195,6 +200,14 @@ export type ExecToolConfig = { }; }; +export type FsToolsConfig = { + /** + * Restrict filesystem tools (read/write/edit/apply_patch) to the agent workspace directory. + * Default: false (unrestricted, matches legacy behavior). + */ + workspaceOnly?: boolean; +}; + export type AgentToolsConfig = { /** Base tool profile applied before allow/deny lists. */ profile?: ToolProfileId; @@ -213,6 +226,8 @@ export type AgentToolsConfig = { }; /** Exec tool defaults for this agent. */ exec?: ExecToolConfig; + /** Filesystem tool path guards. */ + fs?: FsToolsConfig; sandbox?: { tools?: { allow?: string[]; @@ -442,6 +457,8 @@ export type ToolsConfig = { }; /** Exec tool defaults. */ exec?: ExecToolConfig; + /** Filesystem tool path guards. */ + fs?: FsToolsConfig; /** Sub-agent tool policy defaults (deny wins). */ subagents?: { /** Default model selection for spawned sub-agents (string or {primary,fallbacks}). */ diff --git a/src/config/zod-schema.agent-runtime.ts b/src/config/zod-schema.agent-runtime.ts index 4191d916562..04ac8f20da8 100644 --- a/src/config/zod-schema.agent-runtime.ts +++ b/src/config/zod-schema.agent-runtime.ts @@ -291,6 +291,7 @@ export const AgentToolsSchema = z applyPatch: z .object({ enabled: z.boolean().optional(), + workspaceOnly: z.boolean().optional(), allowModels: z.array(z.string()).optional(), }) .strict() @@ -298,6 +299,12 @@ export const AgentToolsSchema = z }) .strict() .optional(), + fs: z + .object({ + workspaceOnly: z.boolean().optional(), + }) + .strict() + .optional(), sandbox: z .object({ tools: ToolPolicySchema, @@ -542,6 +549,7 @@ export const ToolsSchema = z applyPatch: z .object({ enabled: z.boolean().optional(), + workspaceOnly: z.boolean().optional(), allowModels: z.array(z.string()).optional(), }) .strict() @@ -549,6 +557,12 @@ export const ToolsSchema = z }) .strict() .optional(), + fs: z + .object({ + workspaceOnly: z.boolean().optional(), + }) + .strict() + .optional(), subagents: z .object({ tools: ToolPolicySchema,