diff --git a/docs/gateway/security/index.md b/docs/gateway/security/index.md index b3f11d8d84f..ca048fa6910 100644 --- a/docs/gateway/security/index.md +++ b/docs/gateway/security/index.md @@ -579,7 +579,7 @@ 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.exec.applyPatch.workspaceOnly: true` (default): ensures `apply_patch` cannot write/delete outside the workspace directory even when sandboxing is off. Set to `false` only if you intentionally want `apply_patch` to touch files outside the workspace. - `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) diff --git a/docs/tools/apply-patch.md b/docs/tools/apply-patch.md index fb10adc2cef..bf4e0d47035 100644 --- a/docs/tools/apply-patch.md +++ b/docs/tools/apply-patch.md @@ -33,7 +33,7 @@ The tool accepts a single `input` string that wraps one or more file operations: ## Notes - 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). +- `tools.exec.applyPatch.workspaceOnly` defaults to `true` (workspace-contained). Set it to `false` only if you intentionally want `apply_patch` to write/delete outside the workspace directory. - 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 47a79537353..70770af9f6f 100644 --- a/docs/tools/exec.md +++ b/docs/tools/exec.md @@ -178,4 +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). +- `tools.exec.applyPatch.workspaceOnly` defaults to `true` (workspace-contained). Set it to `false` only if you intentionally want `apply_patch` to write/delete outside the workspace directory. diff --git a/docs/tools/index.md b/docs/tools/index.md index d0079c16975..f1496a5982a 100644 --- a/docs/tools/index.md +++ b/docs/tools/index.md @@ -181,7 +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`. +`tools.exec.applyPatch.workspaceOnly` defaults to `true` (workspace-contained). Set it to `false` only if you intentionally want `apply_patch` to write/delete outside the workspace directory. ### `exec` diff --git a/src/agents/apply-patch.e2e.test.ts b/src/agents/apply-patch.e2e.test.ts index be89c1009ec..c3d3fa9ace2 100644 --- a/src/agents/apply-patch.e2e.test.ts +++ b/src/agents/apply-patch.e2e.test.ts @@ -71,9 +71,12 @@ describe("applyPatch", () => { }); }); - it("rejects path traversal outside cwd", async () => { + it("rejects path traversal outside cwd by default", async () => { await withTempDir(async (dir) => { - const escapedPath = path.join(path.dirname(dir), "escaped.txt"); + const escapedPath = path.join( + path.dirname(dir), + `escaped-${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}.txt`, + ); const relativeEscape = path.relative(dir, escapedPath); const patch = `*** Begin Patch @@ -81,14 +84,16 @@ describe("applyPatch", () => { +escaped *** End Patch`; - await expect(applyPatch(patch, { cwd: dir, workspaceOnly: true })).rejects.toThrow( - /Path escapes sandbox root/, - ); - await expect(fs.readFile(escapedPath, "utf8")).rejects.toBeDefined(); + try { + await expect(applyPatch(patch, { cwd: dir })).rejects.toThrow(/Path escapes sandbox root/); + await expect(fs.readFile(escapedPath, "utf8")).rejects.toBeDefined(); + } finally { + await fs.rm(escapedPath, { force: true }); + } }); }); - it("rejects absolute paths outside cwd", async () => { + it("rejects absolute paths outside cwd by default", async () => { await withTempDir(async (dir) => { const escapedPath = path.join(os.tmpdir(), `openclaw-apply-patch-${Date.now()}.txt`); @@ -98,9 +103,7 @@ describe("applyPatch", () => { *** End Patch`; try { - await expect(applyPatch(patch, { cwd: dir, workspaceOnly: true })).rejects.toThrow( - /Path escapes sandbox root/, - ); + await expect(applyPatch(patch, { cwd: dir })).rejects.toThrow(/Path escapes sandbox root/); await expect(fs.readFile(escapedPath, "utf8")).rejects.toBeDefined(); } finally { await fs.rm(escapedPath, { force: true }); @@ -108,7 +111,7 @@ describe("applyPatch", () => { }); }); - it("allows absolute paths within cwd", async () => { + it("allows absolute paths within cwd by default", async () => { await withTempDir(async (dir) => { const target = path.join(dir, "nested", "inside.txt"); const patch = `*** Begin Patch @@ -116,13 +119,13 @@ describe("applyPatch", () => { +inside *** End Patch`; - await applyPatch(patch, { cwd: dir, workspaceOnly: true }); + await applyPatch(patch, { cwd: dir }); const contents = await fs.readFile(target, "utf8"); expect(contents).toBe("inside\n"); }); }); - it("rejects symlink escape attempts", async () => { + it("rejects symlink escape attempts by default", async () => { await withTempDir(async (dir) => { const outside = path.join(path.dirname(dir), "outside-target.txt"); const linkPath = path.join(dir, "link.txt"); @@ -136,16 +139,14 @@ describe("applyPatch", () => { +pwned *** End Patch`; - await expect(applyPatch(patch, { cwd: dir, workspaceOnly: true })).rejects.toThrow( - /Symlink escapes sandbox root/, - ); + await expect(applyPatch(patch, { cwd: dir })).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 () => { + it("allows symlinks that resolve within cwd by default", async () => { await withTempDir(async (dir) => { const target = path.join(dir, "target.txt"); const linkPath = path.join(dir, "link.txt"); @@ -159,9 +160,60 @@ describe("applyPatch", () => { +updated *** End Patch`; - await applyPatch(patch, { cwd: dir, workspaceOnly: true }); + await applyPatch(patch, { cwd: dir }); const contents = await fs.readFile(target, "utf8"); expect(contents).toBe("updated\n"); }); }); + + it("rejects delete path traversal via symlink directories by default", async () => { + await withTempDir(async (dir) => { + const outsideDir = path.join(path.dirname(dir), `outside-dir-${process.pid}-${Date.now()}`); + const outsideFile = path.join(outsideDir, "victim.txt"); + await fs.mkdir(outsideDir, { recursive: true }); + await fs.writeFile(outsideFile, "victim\n", "utf8"); + + const linkDir = path.join(dir, "linkdir"); + await fs.symlink(outsideDir, linkDir); + + const patch = `*** Begin Patch +*** Delete File: linkdir/victim.txt +*** End Patch`; + + try { + await expect(applyPatch(patch, { cwd: dir })).rejects.toThrow( + /Symlink escapes sandbox root/, + ); + const stillThere = await fs.readFile(outsideFile, "utf8"); + expect(stillThere).toBe("victim\n"); + } finally { + await fs.rm(outsideFile, { force: true }); + await fs.rm(outsideDir, { recursive: true, force: true }); + } + }); + }); + + it("allows path traversal when workspaceOnly is explicitly disabled", async () => { + await withTempDir(async (dir) => { + const escapedPath = path.join( + path.dirname(dir), + `escaped-allow-${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}.txt`, + ); + const relativeEscape = path.relative(dir, escapedPath); + + const patch = `*** Begin Patch +*** Add File: ${relativeEscape} ++escaped +*** End Patch`; + + try { + const result = await applyPatch(patch, { cwd: dir, workspaceOnly: false }); + expect(result.summary.added.length).toBe(1); + const contents = await fs.readFile(escapedPath, "utf8"); + expect(contents).toBe("escaped\n"); + } finally { + await fs.rm(escapedPath, { force: true }); + } + }); + }); }); diff --git a/src/agents/apply-patch.ts b/src/agents/apply-patch.ts index 7f22e20c8b3..32c017e495a 100644 --- a/src/agents/apply-patch.ts +++ b/src/agents/apply-patch.ts @@ -5,7 +5,7 @@ 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, resolveSandboxPath } from "./sandbox-paths.js"; +import { assertSandboxPath } from "./sandbox-paths.js"; const BEGIN_PATCH_MARKER = "*** Begin Patch"; const END_PATCH_MARKER = "*** End Patch"; @@ -67,7 +67,7 @@ type SandboxApplyPatchConfig = { type ApplyPatchOptions = { cwd: string; sandbox?: SandboxApplyPatchConfig; - /** When true, restrict patch paths to the workspace root (cwd). Default: false. */ + /** Restrict patch paths to the workspace root (cwd). Default: true. Set false to opt out. */ workspaceOnly?: boolean; signal?: AbortSignal; }; @@ -83,7 +83,7 @@ export function createApplyPatchTool( ): AgentTool { const cwd = options.cwd ?? process.cwd(); const sandbox = options.sandbox; - const workspaceOnly = options.workspaceOnly === true; + const workspaceOnly = options.workspaceOnly !== false; return { name: "apply_patch", @@ -155,7 +155,7 @@ export async function applyPatch( } if (hunk.kind === "delete") { - const target = await resolvePatchPath(hunk.path, options, "unlink"); + const target = await resolvePatchPath(hunk.path, options); await fileOps.remove(target.resolved); recordSummary(summary, seen, "deleted", target.display); continue; @@ -254,7 +254,6 @@ 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({ @@ -267,16 +266,15 @@ async function resolvePatchPath( }; } - 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 + const workspaceOnly = options.workspaceOnly !== false; + const resolved = workspaceOnly + ? ( + await assertSandboxPath({ + filePath, + cwd: options.cwd, + root: options.cwd, + }) + ).resolved : resolvePathFromCwd(filePath, options.cwd); return { resolved, diff --git a/src/agents/pi-tools-agent-config.e2e.test.ts b/src/agents/pi-tools-agent-config.e2e.test.ts index cbd11203a06..220bb75b9cb 100644 --- a/src/agents/pi-tools-agent-config.e2e.test.ts +++ b/src/agents/pi-tools-agent-config.e2e.test.ts @@ -1,3 +1,6 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; import { describe, expect, it } from "vitest"; import "./test-helpers/fast-coding-tools.js"; import type { OpenClawConfig } from "../config/config.js"; @@ -5,6 +8,10 @@ import type { SandboxDockerConfig } from "./sandbox.js"; import type { SandboxFsBridge } from "./sandbox/fs-bridge.js"; import { createOpenClawCodingTools } from "./pi-tools.js"; +type ToolWithExecute = { + execute: (toolCallId: string, args: unknown, signal?: AbortSignal) => Promise; +}; + describe("Agent-specific tool filtering", () => { const sandboxFsBridgeStub: SandboxFsBridge = { resolvePath: () => ({ @@ -110,6 +117,99 @@ describe("Agent-specific tool filtering", () => { expect(toolNames).toContain("apply_patch"); }); + it("defaults apply_patch to workspace-only (blocks traversal)", async () => { + const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-pi-tools-")); + const escapedPath = path.join( + path.dirname(workspaceDir), + `escaped-${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}.txt`, + ); + const relativeEscape = path.relative(workspaceDir, escapedPath); + + try { + const cfg: OpenClawConfig = { + tools: { + allow: ["read", "exec"], + exec: { + applyPatch: { enabled: true }, + }, + }, + }; + + const tools = createOpenClawCodingTools({ + config: cfg, + sessionKey: "agent:main:main", + workspaceDir, + agentDir: "/tmp/agent", + modelProvider: "openai", + modelId: "gpt-5.2", + }); + + const applyPatchTool = tools.find((t) => t.name === "apply_patch"); + if (!applyPatchTool) { + throw new Error("apply_patch tool missing"); + } + + const patch = `*** Begin Patch +*** Add File: ${relativeEscape} ++escaped +*** End Patch`; + + await expect( + (applyPatchTool as unknown as ToolWithExecute).execute("tc1", { input: patch }), + ).rejects.toThrow(/Path escapes sandbox root/); + await expect(fs.readFile(escapedPath, "utf8")).rejects.toBeDefined(); + } finally { + await fs.rm(escapedPath, { force: true }); + await fs.rm(workspaceDir, { recursive: true, force: true }); + } + }); + + it("allows disabling apply_patch workspace-only via config (dangerous)", async () => { + const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-pi-tools-")); + const escapedPath = path.join( + path.dirname(workspaceDir), + `escaped-allow-${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}.txt`, + ); + const relativeEscape = path.relative(workspaceDir, escapedPath); + + try { + const cfg: OpenClawConfig = { + tools: { + allow: ["read", "exec"], + exec: { + applyPatch: { enabled: true, workspaceOnly: false }, + }, + }, + }; + + const tools = createOpenClawCodingTools({ + config: cfg, + sessionKey: "agent:main:main", + workspaceDir, + agentDir: "/tmp/agent", + modelProvider: "openai", + modelId: "gpt-5.2", + }); + + const applyPatchTool = tools.find((t) => t.name === "apply_patch"); + if (!applyPatchTool) { + throw new Error("apply_patch tool missing"); + } + + const patch = `*** Begin Patch +*** Add File: ${relativeEscape} ++escaped +*** End Patch`; + + await (applyPatchTool as unknown as ToolWithExecute).execute("tc2", { input: patch }); + const contents = await fs.readFile(escapedPath, "utf8"); + expect(contents).toBe("escaped\n"); + } finally { + await fs.rm(escapedPath, { force: true }); + await fs.rm(workspaceDir, { recursive: true, force: true }); + } + }); + it("should apply agent-specific tool policy", () => { const cfg: OpenClawConfig = { tools: { diff --git a/src/agents/pi-tools.ts b/src/agents/pi-tools.ts index acffdedf54b..15809c1c434 100644 --- a/src/agents/pi-tools.ts +++ b/src/agents/pi-tools.ts @@ -256,7 +256,9 @@ export function createOpenClawCodingTools(options?: { const workspaceRoot = options?.workspaceDir ?? process.cwd(); const workspaceOnly = fsConfig.workspaceOnly === true; const applyPatchConfig = execConfig.applyPatch; - const applyPatchWorkspaceOnly = workspaceOnly || applyPatchConfig?.workspaceOnly === true; + // Secure by default: apply_patch is workspace-contained unless explicitly disabled. + // (tools.fs.workspaceOnly is a separate umbrella flag for read/write/edit/apply_patch.) + const applyPatchWorkspaceOnly = workspaceOnly || applyPatchConfig?.workspaceOnly !== false; const applyPatchEnabled = !!applyPatchConfig?.enabled && isOpenAIProvider(options?.modelProvider) && diff --git a/src/config/schema.help.ts b/src/config/schema.help.ts index 97e43e5b187..7f399156785 100644 --- a/src/config/schema.help.ts +++ b/src/config/schema.help.ts @@ -59,7 +59,7 @@ export const FIELD_HELP: Record = { "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).", + "Restrict apply_patch paths to the workspace directory (default: true). Set false to allow writing outside the workspace (dangerous).", "tools.exec.applyPatch.allowModels": 'Optional allowlist of model ids (e.g. "gpt-5.2" or "openai/gpt-5.2").', "tools.exec.notifyOnExit":