From 5544646a09c0121fca7d7093812dc2de8437c7f1 Mon Sep 17 00:00:00 2001 From: Mariano Date: Sat, 14 Feb 2026 19:11:12 +0000 Subject: [PATCH] security: block apply_patch path traversal outside workspace (#16405) Merged via /review-pr -> /prepare-pr -> /merge-pr. Prepared head SHA: 0fcd3f8c3a15993980eb89ecdae3e76de4f3f72d Co-authored-by: mbelinky <132747814+mbelinky@users.noreply.github.com> Co-authored-by: mbelinky <132747814+mbelinky@users.noreply.github.com> Reviewed-by: @mbelinky --- CHANGELOG.md | 1 + src/agents/apply-patch.e2e.test.ts | 68 ++++++++++++++++++++++++++++++ src/agents/apply-patch.ts | 36 ++++------------ 3 files changed, 77 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd879c8ff..7ea5607fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,7 @@ Docs: https://docs.openclaw.ai - Security/BlueBubbles: reject ambiguous shared-path webhook routing when multiple webhook targets match the same guid/password. - Security/BlueBubbles: harden BlueBubbles webhook auth behind reverse proxies by only accepting passwordless webhooks for direct localhost loopback requests (forwarded/proxied requests now require a password). Thanks @simecek. - Security/BlueBubbles: require explicit `mediaLocalRoots` allowlists for local outbound media path reads to prevent local file disclosure. (#16322) Thanks @mbelinky. +- Security/Agents: enforce workspace-root path bounds for `apply_patch` in non-sandbox mode to block traversal and symlink escape writes. Thanks @p80n-sec. - Cron/Slack: preserve agent identity (name and icon) when cron jobs deliver outbound messages. (#16242) Thanks @robbyczgw-cla. - Cron: deliver text-only output directly when `delivery.to` is set so cron recipients get full output instead of summaries. (#16360) Thanks @rubyrunsstuff. - Cron: repair missing/corrupt `nextRunAtMs` for the updated job without globally recomputing unrelated due jobs during `cron update`. (#15750) diff --git a/src/agents/apply-patch.e2e.test.ts b/src/agents/apply-patch.e2e.test.ts index 0e71fbc7c..55ce31f7a 100644 --- a/src/agents/apply-patch.e2e.test.ts +++ b/src/agents/apply-patch.e2e.test.ts @@ -70,4 +70,72 @@ describe("applyPatch", () => { expect(contents).toBe("line1\nline2\n"); }); }); + + it("rejects path traversal outside cwd", async () => { + await withTempDir(async (dir) => { + const escapedPath = path.join(path.dirname(dir), "escaped.txt"); + const relativeEscape = path.relative(dir, escapedPath); + + const patch = `*** Begin Patch +*** Add File: ${relativeEscape} ++escaped +*** End Patch`; + + await expect(applyPatch(patch, { cwd: dir })).rejects.toThrow(/Path escapes sandbox root/); + await expect(fs.readFile(escapedPath, "utf8")).rejects.toBeDefined(); + }); + }); + + it("rejects absolute paths outside cwd", async () => { + await withTempDir(async (dir) => { + const escapedPath = path.join(os.tmpdir(), `openclaw-apply-patch-${Date.now()}.txt`); + + const patch = `*** Begin Patch +*** Add File: ${escapedPath} ++escaped +*** End Patch`; + + 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("allows absolute paths within cwd", async () => { + await withTempDir(async (dir) => { + const target = path.join(dir, "nested", "inside.txt"); + const patch = `*** Begin Patch +*** Add File: ${target} ++inside +*** End Patch`; + + await applyPatch(patch, { cwd: dir }); + const contents = await fs.readFile(target, "utf8"); + expect(contents).toBe("inside\n"); + }); + }); + + it("rejects symlink escape attempts", async () => { + await withTempDir(async (dir) => { + const outside = path.join(path.dirname(dir), "outside-target.txt"); + const linkPath = path.join(dir, "link.txt"); + await fs.writeFile(outside, "initial\n", "utf8"); + await fs.symlink(outside, linkPath); + + const patch = `*** Begin Patch +*** Update File: link.txt +@@ +-initial ++pwned +*** End Patch`; + + await expect(applyPatch(patch, { cwd: dir })).rejects.toThrow(/Symlink not allowed/); + const outsideContents = await fs.readFile(outside, "utf8"); + expect(outsideContents).toBe("initial\n"); + await fs.rm(outside, { force: true }); + }); + }); }); diff --git a/src/agents/apply-patch.ts b/src/agents/apply-patch.ts index 731607602..47c5b8ca6 100644 --- a/src/agents/apply-patch.ts +++ b/src/agents/apply-patch.ts @@ -1,10 +1,10 @@ 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"; const BEGIN_PATCH_MARKER = "*** Begin Patch"; const END_PATCH_MARKER = "*** End Patch"; @@ -15,7 +15,6 @@ const MOVE_TO_MARKER = "*** Move to: "; const EOF_MARKER = "*** End of File"; const CHANGE_CONTEXT_MARKER = "@@ "; const EMPTY_CHANGE_CONTEXT_MARKER = "@@"; -const UNICODE_SPACES = /[\u00A0\u2000-\u200A\u202F\u205F\u3000]/g; type AddFileHunk = { kind: "add"; @@ -262,36 +261,17 @@ async function resolvePatchPath( }; } - const resolved = resolvePathFromCwd(filePath, options.cwd); + const resolved = await assertSandboxPath({ + filePath, + cwd: options.cwd, + root: options.cwd, + }); return { - resolved, - display: toDisplayPath(resolved, options.cwd), + resolved: resolved.resolved, + display: toDisplayPath(resolved.resolved, options.cwd), }; } -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 === "") {