From 36820f16765606fb9afb2e54ae0e9728f0688f60 Mon Sep 17 00:00:00 2001 From: ly85206559 Date: Tue, 14 Apr 2026 09:20:25 +0800 Subject: [PATCH] Agents: fix Windows drive path join for read/sandbox tools (#54039) (#66193) * Agents: fix Windows drive path join for read/sandbox tools (#54039) * fix(agents): harden Windows file URL path mapping * fix(agents): reject encoded file URL separators * Update CHANGELOG.md --------- Co-authored-by: Vincent Koc --- CHANGELOG.md | 1 + src/agents/pi-tools.read.ts | 51 ++++++++++++++++--- ...pi-tools.read.workspace-root-guard.test.ts | 32 ++++++++++++ src/agents/sandbox-paths.test.ts | 15 ++++++ src/agents/sandbox-paths.ts | 34 +++++++++++-- ...andbox-paths.windows-drive-resolve.test.ts | 33 ++++++++++++ src/infra/local-file-access.ts | 9 ++++ 7 files changed, 164 insertions(+), 11 deletions(-) create mode 100644 src/agents/sandbox-paths.windows-drive-resolve.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 559c09308ed..19ff7e1eb87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,7 @@ Docs: https://docs.openclaw.ai - Voice-call/media-stream: resolve the source IP from trusted forwarding headers for per-IP pending-connection limits when `webhookSecurity.trustForwardingHeaders` and `trustedProxyIPs` are configured, and reserve `maxConnections` capacity for in-flight WebSocket upgrades so concurrent handshakes can no longer momentarily exceed the operator-set cap. (#66027) Thanks @eleqtrizit. - Feishu/allowlist: canonicalize allowlist entries by explicit `user`/`chat` kind, strip repeated `feishu:`/`lark:` provider prefixes, and stop folding opaque Feishu IDs to lowercase, so allowlist matching no longer crosses user/chat namespaces or widens to case-insensitive ID matches the operator did not intend. (#66021) Thanks @eleqtrizit. - TTS/reply media: persist OpenClaw temp voice outputs into managed outbound media and allow them through reply-media normalization, so voice-note replies stop silently dropping. (#63511) Thanks @jetd1. +- Agents/tools: treat Windows drive-letter paths (`C:\\...`) as absolute when resolving sandbox and read-tool paths so workspace root is not prepended under POSIX path rules. (#54039) Thanks @ly85206559 and @vincentkoc. - Agents/OpenAI: recover embedded GPT-style runs when reasoning-only or empty turns need bounded continuation, with replay-safe retry gating and incomplete-turn fallback when no visible answer arrives. (#66167) thanks @jalehman - Outbound/relay-status: suppress internal relay-status placeholder payloads (`No channel reply.`, `Replied in-thread.`, `Replied in #...`, wiki-update status variants ending in `No channel reply.`) before channel delivery so internal housekeeping text does not leak to users. - Slack/doctor: add a dedicated doctor-contract sidecar so config warmup paths such as `openclaw cron` no longer fall back to Slack's broader contract surface, which could trigger Slack-related config-read crashes on affected setups. (#63192) Thanks @shhtheonlyperson. diff --git a/src/agents/pi-tools.read.ts b/src/agents/pi-tools.read.ts index 8041a1b760f..fefe6bc28ab 100644 --- a/src/agents/pi-tools.read.ts +++ b/src/agents/pi-tools.read.ts @@ -1,7 +1,9 @@ import fs from "node:fs/promises"; import path from "node:path"; +import { URL } from "node:url"; import type { AgentToolResult } from "@mariozechner/pi-agent-core"; import { createEditTool, createReadTool, createWriteTool } from "@mariozechner/pi-coding-agent"; +import { isWindowsDrivePath } from "../infra/archive-path.js"; import { appendFileWithinRoot, SafeOpenError, @@ -9,7 +11,7 @@ import { readFileWithinRoot, writeFileWithinRoot, } from "../infra/fs-safe.js"; -import { trySafeFileURLToPath } from "../infra/local-file-access.js"; +import { hasEncodedFileUrlSeparator, trySafeFileURLToPath } from "../infra/local-file-access.js"; import { detectMime } from "../media/mime.js"; import { sniffMimeFromBase64 } from "../media/sniff-mime-from-base64.js"; import type { ImageSanitizationLimits } from "./image-sanitization.js"; @@ -373,10 +375,41 @@ function mapContainerPathToWorkspaceRoot(params: { let candidate = params.filePath.startsWith("@") ? params.filePath.slice(1) : params.filePath; if (/^file:\/\//i.test(candidate)) { const localFilePath = trySafeFileURLToPath(candidate); - if (!localFilePath) { - return params.filePath; + if (localFilePath) { + candidate = localFilePath; + } else { + // Windows rejects posix-style file:///workspace/... in fileURLToPath; map via URL pathname + // when it clearly refers to the container workdir (same idea as sandbox-paths). + let parsed: URL; + try { + parsed = new URL(candidate); + } catch { + return params.filePath; + } + if (parsed.protocol !== "file:") { + return params.filePath; + } + const host = parsed.hostname.trim().toLowerCase(); + if (host && host !== "localhost") { + return params.filePath; + } + if (hasEncodedFileUrlSeparator(parsed.pathname)) { + return params.filePath; + } + let normalizedPathname: string; + try { + normalizedPathname = decodeURIComponent(parsed.pathname).replace(/\\/g, "/"); + } catch { + return params.filePath; + } + if ( + normalizedPathname !== normalizedWorkdir && + !normalizedPathname.startsWith(`${normalizedWorkdir}/`) + ) { + return params.filePath; + } + candidate = normalizedPathname; } - candidate = localFilePath; } const normalizedCandidate = candidate.replace(/\\/g, "/"); @@ -401,9 +434,13 @@ export function resolveToolPathAgainstWorkspaceRoot(params: { }): string { const mapped = mapContainerPathToWorkspaceRoot(params); const candidate = mapped.startsWith("@") ? mapped.slice(1) : mapped; - return path.isAbsolute(candidate) - ? path.resolve(candidate) - : path.resolve(params.root, candidate || "."); + if (isWindowsDrivePath(candidate)) { + return path.win32.normalize(candidate); + } + if (path.isAbsolute(candidate)) { + return path.resolve(candidate); + } + return path.resolve(params.root, candidate || "."); } type MemoryFlushAppendOnlyWriteOptions = { diff --git a/src/agents/pi-tools.read.workspace-root-guard.test.ts b/src/agents/pi-tools.read.workspace-root-guard.test.ts index c64facb81e9..c49f1215c98 100644 --- a/src/agents/pi-tools.read.workspace-root-guard.test.ts +++ b/src/agents/pi-tools.read.workspace-root-guard.test.ts @@ -96,6 +96,38 @@ describe("wrapToolWorkspaceRootGuardWithOptions", () => { }); }); + it("does not remap malformed file:// container workspace paths", async () => { + const { tool } = createToolHarness(); + const wrapped = wrapToolWorkspaceRootGuardWithOptions(tool, root, { + containerWorkdir: "/workspace", + }); + + await wrapped.execute("tc-malformed-file-url", { path: "file:///workspace/%E0%A4%A" }); + + expect(mocks.assertSandboxPath).toHaveBeenCalledWith({ + filePath: "file:///workspace/%E0%A4%A", + cwd: root, + root, + }); + }); + + it("does not remap file:// container workspace paths with encoded separators", async () => { + const { tool } = createToolHarness(); + const wrapped = wrapToolWorkspaceRootGuardWithOptions(tool, root, { + containerWorkdir: "/workspace", + }); + + await wrapped.execute("tc-encoded-separator-file-url", { + path: "file:///workspace/%2FREADME.md", + }); + + expect(mocks.assertSandboxPath).toHaveBeenCalledWith({ + filePath: "file:///workspace/%2FREADME.md", + cwd: root, + root, + }); + }); + it("maps @-prefixed container workspace paths to host workspace root", async () => { const { tool } = createToolHarness(); const wrapped = wrapToolWorkspaceRootGuardWithOptions(tool, root, { diff --git a/src/agents/sandbox-paths.test.ts b/src/agents/sandbox-paths.test.ts index b462ec60253..61d3c70ecb3 100644 --- a/src/agents/sandbox-paths.test.ts +++ b/src/agents/sandbox-paths.test.ts @@ -167,11 +167,26 @@ describe("resolveSandboxedMediaSource", () => { media: "file://attacker/share/photo.png", expected: /remote hosts are not allowed/i, }, + { + name: "file:// container URLs with remote hosts", + media: "file://attacker/workspace/photo.png", + expected: /remote hosts are not allowed/i, + }, { name: "invalid file:// URLs", media: "file://not a valid url\x00", expected: /Invalid file:\/\/ URL/, }, + { + name: "file:// URLs with malformed container pathname encoding", + media: "file:///workspace/%E0%A4%A", + expected: /Invalid file:\/\/ URL/, + }, + { + name: "file:// URLs with encoded separators in the pathname", + media: "file:///workspace/%2FREADME.md", + expected: /cannot encode path separators/i, + }, ])("rejects $name", async ({ media, expected }) => { await withSandboxRoot(async (sandboxDir) => { await expectSandboxRejection(media, sandboxDir, expected); diff --git a/src/agents/sandbox-paths.ts b/src/agents/sandbox-paths.ts index 7b8d0aa6a5e..11eaa930752 100644 --- a/src/agents/sandbox-paths.ts +++ b/src/agents/sandbox-paths.ts @@ -1,7 +1,12 @@ import os from "node:os"; import path from "node:path"; import { URL } from "node:url"; -import { assertNoWindowsNetworkPath, safeFileURLToPath } from "../infra/local-file-access.js"; +import { isWindowsDrivePath } from "../infra/archive-path.js"; +import { + assertNoWindowsNetworkPath, + hasEncodedFileUrlSeparator, + safeFileURLToPath, +} from "../infra/local-file-access.js"; import { assertNoPathAliasEscape, type PathAliasPolicy } from "../infra/path-alias-guards.js"; import { isPathInside } from "../infra/path-guards.js"; import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js"; @@ -30,8 +35,17 @@ function expandPath(filePath: string): string { return normalized; } +/** True when the path is absolute for the current platform or a Windows drive path (e.g. C:\\...), even if path.isAbsolute is false under POSIX rules. */ +function hostPathLooksAbsolute(expanded: string): boolean { + return path.isAbsolute(expanded) || isWindowsDrivePath(expanded); +} + function resolveToCwd(filePath: string, cwd: string): string { const expanded = expandPath(filePath); + // Drive-letter paths first: on Unix path.isAbsolute is false for C:/...; on Windows we still normalize. + if (isWindowsDrivePath(expanded)) { + return path.win32.normalize(expanded); + } if (path.isAbsolute(expanded)) { return expanded; } @@ -52,7 +66,7 @@ export function resolveSandboxPath(params: { filePath: string; cwd: string; root if (!relative || relative === "") { return { resolved, relative: "" }; } - if (relative.startsWith("..") || path.isAbsolute(relative)) { + if (relative.startsWith("..") || path.isAbsolute(relative) || isWindowsDrivePath(relative)) { throw new Error(`Path escapes sandbox root (${shortPath(rootResolved)}): ${params.filePath}`); } return { resolved, relative }; @@ -151,9 +165,21 @@ function mapContainerWorkspaceFileUrl(params: { if (parsed.protocol !== "file:") { return undefined; } + const host = parsed.hostname.trim().toLowerCase(); + if (host && host !== "localhost") { + return undefined; + } + if (hasEncodedFileUrlSeparator(parsed.pathname)) { + return undefined; + } // Sandbox paths are Linux-style (/workspace/*). Parse the URL path directly so // Windows hosts can still accept file:///workspace/... media references. - const normalizedPathname = decodeURIComponent(parsed.pathname).replace(/\\/g, "/"); + let normalizedPathname: string; + try { + normalizedPathname = decodeURIComponent(parsed.pathname).replace(/\\/g, "/"); + } catch { + return undefined; + } if ( normalizedPathname !== SANDBOX_CONTAINER_WORKDIR && !normalizedPathname.startsWith(`${SANDBOX_CONTAINER_WORKDIR}/`) @@ -189,7 +215,7 @@ async function resolveAllowedTmpMediaPath(params: { candidate: string; sandboxRoot: string; }): Promise { - const candidateIsAbsolute = path.isAbsolute(expandPath(params.candidate)); + const candidateIsAbsolute = hostPathLooksAbsolute(expandPath(params.candidate)); if (!candidateIsAbsolute) { return undefined; } diff --git a/src/agents/sandbox-paths.windows-drive-resolve.test.ts b/src/agents/sandbox-paths.windows-drive-resolve.test.ts new file mode 100644 index 00000000000..1e93e257aef --- /dev/null +++ b/src/agents/sandbox-paths.windows-drive-resolve.test.ts @@ -0,0 +1,33 @@ +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import { resolveSandboxInputPath } from "./sandbox-paths.js"; +import { resolveToolPathAgainstWorkspaceRoot } from "./pi-tools.read.js"; + +describe("resolveSandboxInputPath (Windows drive paths under POSIX rules)", () => { + it("does not join workspace cwd when path looks like a Windows drive path", () => { + const cwd = path.resolve("/workspace/project"); + const resolved = resolveSandboxInputPath("C:/Users/test/file.txt", cwd); + expect(resolved).toBe(path.win32.normalize("C:/Users/test/file.txt")); + expect(resolved).not.toContain("workspace"); + }); + + it("treats backslash Windows drive paths as absolute vs cwd", () => { + const cwd = path.resolve("/app/sandbox"); + const resolved = resolveSandboxInputPath("D:\\data\\out.log", cwd); + expect(resolved).toBe(path.win32.normalize("D:\\data\\out.log")); + expect(resolved).not.toContain("sandbox"); + }); +}); + +describe("resolveToolPathAgainstWorkspaceRoot (Windows drive paths)", () => { + const root = path.resolve("/host/workspace"); + + it("does not prefix workspace root for drive-letter paths", () => { + const resolved = resolveToolPathAgainstWorkspaceRoot({ + filePath: "C:/temp/agent-output.txt", + root, + }); + expect(resolved).toBe(path.win32.normalize("C:/temp/agent-output.txt")); + expect(resolved).not.toContain("host"); + }); +}); diff --git a/src/infra/local-file-access.ts b/src/infra/local-file-access.ts index 830238cfdbf..66354b00de2 100644 --- a/src/infra/local-file-access.ts +++ b/src/infra/local-file-access.ts @@ -2,11 +2,17 @@ import path from "node:path"; import { fileURLToPath, URL } from "node:url"; import { normalizeLowercaseStringOrEmpty } from "../shared/string-coerce.js"; +const ENCODED_FILE_URL_SEPARATOR_RE = /%(?:2f|5c)/i; + function isLocalFileUrlHost(hostname: string): boolean { const normalized = normalizeLowercaseStringOrEmpty(hostname); return normalized === "" || normalized === "localhost"; } +export function hasEncodedFileUrlSeparator(pathname: string): boolean { + return ENCODED_FILE_URL_SEPARATOR_RE.test(pathname); +} + export function isWindowsNetworkPath(filePath: string): boolean { if (process.platform !== "win32") { return false; @@ -34,6 +40,9 @@ export function safeFileURLToPath(fileUrl: string): string { if (!isLocalFileUrlHost(parsed.hostname)) { throw new Error(`file:// URLs with remote hosts are not allowed: ${fileUrl}`); } + if (hasEncodedFileUrlSeparator(parsed.pathname)) { + throw new Error(`file:// URLs cannot encode path separators: ${fileUrl}`); + } const filePath = fileURLToPath(parsed); assertNoWindowsNetworkPath(filePath, "Local file URL"); return filePath;