From 29d783958287b7e3216c1e8c9630f28261105c48 Mon Sep 17 00:00:00 2001 From: davidbors-snyk Date: Fri, 13 Feb 2026 17:29:10 +0200 Subject: [PATCH] fix: execute sandboxed file ops inside containers (#4026) Merged via /review-pr -> /prepare-pr -> /merge-pr. Prepared head SHA: 795ec6aa2f311fcda6660876dbadb4ef356bc0ac Co-authored-by: davidbors-snyk <240482518+davidbors-snyk@users.noreply.github.com> Co-authored-by: steipete <58493+steipete@users.noreply.github.com> Reviewed-by: @steipete --- src/agents/apply-patch-update.ts | 8 +- src/agents/apply-patch.ts | 77 ++++-- src/agents/openclaw-tools.ts | 7 +- src/agents/pi-embedded-runner/run/attempt.ts | 5 +- src/agents/pi-embedded-runner/run/images.ts | 72 ++--- ...ses-schemas-without-dropping-d.e2e.test.ts | 15 +- ...iases-schemas-without-dropping.e2e.test.ts | 6 +- src/agents/pi-tools.read.ts | 86 +++++- src/agents/pi-tools.ts | 23 +- .../pi-tools.workspace-paths.e2e.test.ts | 7 +- src/agents/sandbox/context.ts | 7 +- src/agents/sandbox/docker.ts | 141 ++++++++-- src/agents/sandbox/fs-bridge.test.ts | 88 ++++++ src/agents/sandbox/fs-bridge.ts | 257 ++++++++++++++++++ src/agents/sandbox/types.ts | 2 + .../test-helpers/host-sandbox-fs-bridge.ts | 74 +++++ src/agents/tools/image-tool.e2e.test.ts | 7 +- src/agents/tools/image-tool.ts | 56 ++-- src/web/media.test.ts | 35 +++ src/web/media.ts | 41 ++- 20 files changed, 862 insertions(+), 152 deletions(-) create mode 100644 src/agents/sandbox/fs-bridge.test.ts create mode 100644 src/agents/sandbox/fs-bridge.ts create mode 100644 src/agents/test-helpers/host-sandbox-fs-bridge.ts diff --git a/src/agents/apply-patch-update.ts b/src/agents/apply-patch-update.ts index 87d8b97f46a..eb664adcbac 100644 --- a/src/agents/apply-patch-update.ts +++ b/src/agents/apply-patch-update.ts @@ -7,11 +7,17 @@ type UpdateFileChunk = { isEndOfFile: boolean; }; +async function defaultReadFile(filePath: string): Promise { + return fs.readFile(filePath, "utf8"); +} + export async function applyUpdateHunk( filePath: string, chunks: UpdateFileChunk[], + options?: { readFile?: (filePath: string) => Promise }, ): Promise { - const originalContents = await fs.readFile(filePath, "utf8").catch((err) => { + const reader = options?.readFile ?? defaultReadFile; + const originalContents = await reader(filePath).catch((err) => { throw new Error(`Failed to read file to update ${filePath}: ${err}`); }); diff --git a/src/agents/apply-patch.ts b/src/agents/apply-patch.ts index 806333af2ee..f6b29d42423 100644 --- a/src/agents/apply-patch.ts +++ b/src/agents/apply-patch.ts @@ -3,8 +3,8 @@ 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"; @@ -59,9 +59,14 @@ export type ApplyPatchToolDetails = { summary: ApplyPatchSummary; }; +type SandboxApplyPatchConfig = { + root: string; + bridge: SandboxFsBridge; +}; + type ApplyPatchOptions = { cwd: string; - sandboxRoot?: string; + sandbox?: SandboxApplyPatchConfig; signal?: AbortSignal; }; @@ -72,11 +77,11 @@ const applyPatchSchema = Type.Object({ }); export function createApplyPatchTool( - options: { cwd?: string; sandboxRoot?: string } = {}, - // oxlint-disable-next-line typescript/no-explicit-any + options: { cwd?: string; sandbox?: SandboxApplyPatchConfig } = {}, + // biome-ignore lint/suspicious/noExplicitAny: TypeBox schema type from pi-agent-core uses a different module instance. ): AgentTool { const cwd = options.cwd ?? process.cwd(); - const sandboxRoot = options.sandboxRoot; + const sandbox = options.sandbox; return { name: "apply_patch", @@ -98,7 +103,7 @@ export function createApplyPatchTool( const result = await applyPatch(input, { cwd, - sandboxRoot, + sandbox, signal, }); @@ -129,6 +134,7 @@ export async function applyPatch( modified: new Set(), deleted: new Set(), }; + const fileOps = resolvePatchFileOps(options); for (const hunk of parsed.hunks) { if (options.signal?.aborted) { @@ -139,30 +145,32 @@ export async function applyPatch( if (hunk.kind === "add") { const target = await resolvePatchPath(hunk.path, options); - await ensureDir(target.resolved); - await fs.writeFile(target.resolved, hunk.contents, "utf8"); + await ensureDir(target.resolved, fileOps); + await fileOps.writeFile(target.resolved, hunk.contents); recordSummary(summary, seen, "added", target.display); continue; } if (hunk.kind === "delete") { const target = await resolvePatchPath(hunk.path, options); - await fs.rm(target.resolved); + await fileOps.remove(target.resolved); recordSummary(summary, seen, "deleted", target.display); continue; } const target = await resolvePatchPath(hunk.path, options); - const applied = await applyUpdateHunk(target.resolved, hunk.chunks); + const applied = await applyUpdateHunk(target.resolved, hunk.chunks, { + readFile: (path) => fileOps.readFile(path), + }); if (hunk.movePath) { const moveTarget = await resolvePatchPath(hunk.movePath, options); - await ensureDir(moveTarget.resolved); - await fs.writeFile(moveTarget.resolved, applied, "utf8"); - await fs.rm(target.resolved); + await ensureDir(moveTarget.resolved, fileOps); + await fileOps.writeFile(moveTarget.resolved, applied); + await fileOps.remove(target.resolved); recordSummary(summary, seen, "modified", moveTarget.display); } else { - await fs.writeFile(target.resolved, applied, "utf8"); + await fileOps.writeFile(target.resolved, applied); recordSummary(summary, seen, "modified", target.display); } } @@ -204,27 +212,54 @@ function formatSummary(summary: ApplyPatchSummary): string { return lines.join("\n"); } -async function ensureDir(filePath: string) { +type PatchFileOps = { + readFile: (filePath: string) => Promise; + writeFile: (filePath: string, content: string) => Promise; + remove: (filePath: string) => Promise; + mkdirp: (dir: string) => Promise; +}; + +function resolvePatchFileOps(options: ApplyPatchOptions): PatchFileOps { + if (options.sandbox) { + const { root, bridge } = options.sandbox; + return { + readFile: async (filePath) => { + const buf = await bridge.readFile({ filePath, cwd: root }); + return buf.toString("utf8"); + }, + writeFile: (filePath, content) => bridge.writeFile({ filePath, cwd: root, data: content }), + remove: (filePath) => bridge.remove({ filePath, cwd: root, force: false }), + mkdirp: (dir) => bridge.mkdirp({ filePath: dir, cwd: root }), + }; + } + return { + readFile: (filePath) => fs.readFile(filePath, "utf8"), + writeFile: (filePath, content) => fs.writeFile(filePath, content, "utf8"), + remove: (filePath) => fs.rm(filePath), + mkdirp: (dir) => fs.mkdir(dir, { recursive: true }).then(() => {}), + }; +} + +async function ensureDir(filePath: string, ops: PatchFileOps) { const parent = path.dirname(filePath); if (!parent || parent === ".") { return; } - await fs.mkdir(parent, { recursive: true }); + await ops.mkdirp(parent); } async function resolvePatchPath( filePath: string, options: ApplyPatchOptions, ): Promise<{ resolved: string; display: string }> { - if (options.sandboxRoot) { - const resolved = await assertSandboxPath({ + if (options.sandbox) { + const resolved = options.sandbox.bridge.resolvePath({ filePath, cwd: options.cwd, - root: options.sandboxRoot, }); return { - resolved: resolved.resolved, - display: resolved.relative || resolved.resolved, + resolved: resolved.hostPath, + display: resolved.relativePath || resolved.hostPath, }; } diff --git a/src/agents/openclaw-tools.ts b/src/agents/openclaw-tools.ts index b38645f1480..2be40ead3cc 100644 --- a/src/agents/openclaw-tools.ts +++ b/src/agents/openclaw-tools.ts @@ -1,5 +1,6 @@ import type { OpenClawConfig } from "../config/config.js"; import type { GatewayMessageChannel } from "../utils/message-channel.js"; +import type { SandboxFsBridge } from "./sandbox/fs-bridge.js"; import type { AnyAgentTool } from "./tools/common.js"; import { resolvePluginTools } from "../plugins/tools.js"; import { resolveSessionAgentId } from "./agent-scope.js"; @@ -37,6 +38,7 @@ export function createOpenClawTools(options?: { agentGroupSpace?: string | null; agentDir?: string; sandboxRoot?: string; + sandboxFsBridge?: SandboxFsBridge; workspaceDir?: string; sandboxed?: boolean; config?: OpenClawConfig; @@ -62,7 +64,10 @@ export function createOpenClawTools(options?: { ? createImageTool({ config: options?.config, agentDir: options.agentDir, - sandboxRoot: options?.sandboxRoot, + sandbox: + options?.sandboxRoot && options?.sandboxFsBridge + ? { root: options.sandboxRoot, bridge: options.sandboxFsBridge } + : undefined, modelHasVision: options?.modelHasVision, }) : null; diff --git a/src/agents/pi-embedded-runner/run/attempt.ts b/src/agents/pi-embedded-runner/run/attempt.ts index c1adef08b5e..41123de1474 100644 --- a/src/agents/pi-embedded-runner/run/attempt.ts +++ b/src/agents/pi-embedded-runner/run/attempt.ts @@ -798,7 +798,10 @@ export async function runEmbeddedAttempt( historyMessages: activeSession.messages, maxBytes: MAX_IMAGE_BYTES, // Enforce sandbox path restrictions when sandbox is enabled - sandboxRoot: sandbox?.enabled ? sandbox.workspaceDir : undefined, + sandbox: + sandbox?.enabled && sandbox?.fsBridge + ? { root: sandbox.workspaceDir, bridge: sandbox.fsBridge } + : undefined, }); // Inject history images into their original message positions. diff --git a/src/agents/pi-embedded-runner/run/images.ts b/src/agents/pi-embedded-runner/run/images.ts index 4bd6a35ba02..2511728ca79 100644 --- a/src/agents/pi-embedded-runner/run/images.ts +++ b/src/agents/pi-embedded-runner/run/images.ts @@ -1,11 +1,7 @@ import type { ImageContent } from "@mariozechner/pi-ai"; -import fs from "node:fs/promises"; import path from "node:path"; import { fileURLToPath } from "node:url"; -import { extractTextFromMessage } from "../../../tui/tui-formatters.js"; -import { resolveUserPath } from "../../../utils.js"; -import { loadWebMedia } from "../../../web/media.js"; -import { assertSandboxPath } from "../../sandbox-paths.js"; +import type { SandboxFsBridge } from "../../sandbox/fs-bridge.js"; import { sanitizeImageBlocks } from "../../tool-images.js"; import { log } from "../logger.js"; @@ -177,8 +173,7 @@ export async function loadImageFromRef( workspaceDir: string, options?: { maxBytes?: number; - /** If set, enforce that file paths are within this sandbox root */ - sandboxRoot?: string; + sandbox?: { root: string; bridge: SandboxFsBridge }; }, ): Promise { try { @@ -190,46 +185,34 @@ export async function loadImageFromRef( return null; } - // For file paths, resolve relative to the appropriate root: - // - When sandbox is enabled, resolve relative to sandboxRoot for security - // - Otherwise, resolve relative to workspaceDir - // Note: ref.resolved may already be absolute (e.g., after ~ expansion in detectImageReferences), - // in which case we skip relative resolution. - if (ref.type === "path" && !path.isAbsolute(targetPath)) { - const resolveRoot = options?.sandboxRoot ?? workspaceDir; - targetPath = path.resolve(resolveRoot, targetPath); - } - - // Enforce sandbox restrictions if sandboxRoot is set - if (ref.type === "path" && options?.sandboxRoot) { - try { - const validated = await assertSandboxPath({ - filePath: targetPath, - cwd: options.sandboxRoot, - root: options.sandboxRoot, - }); - targetPath = validated.resolved; - } catch (err) { - // Log the actual error for debugging (sandbox violation or other path error) - log.debug( - `Native image: sandbox validation failed for ${ref.resolved}: ${err instanceof Error ? err.message : String(err)}`, - ); - return null; - } - } - - // Check file exists for local paths + // Resolve paths relative to sandbox or workspace as needed if (ref.type === "path") { - try { - await fs.stat(targetPath); - } catch { - log.debug(`Native image: file not found: ${targetPath}`); - return null; + if (options?.sandbox) { + try { + const resolved = options.sandbox.bridge.resolvePath({ + filePath: targetPath, + cwd: options.sandbox.root, + }); + targetPath = resolved.hostPath; + } catch (err) { + log.debug( + `Native image: sandbox validation failed for ${ref.resolved}: ${err instanceof Error ? err.message : String(err)}`, + ); + return null; + } + } else if (!path.isAbsolute(targetPath)) { + targetPath = path.resolve(workspaceDir, targetPath); } } // loadWebMedia handles local file paths (including file:// URLs) - const media = await loadWebMedia(targetPath, options?.maxBytes); + const media = options?.sandbox + ? await loadWebMedia(targetPath, { + maxBytes: options.maxBytes, + readFile: (filePath) => + options.sandbox!.bridge.readFile({ filePath, cwd: options.sandbox!.root }), + }) + : await loadWebMedia(targetPath, options?.maxBytes); if (media.kind !== "image") { log.debug(`Native image: not an image file: ${targetPath} (got ${media.kind})`); @@ -344,8 +327,7 @@ export async function detectAndLoadPromptImages(params: { existingImages?: ImageContent[]; historyMessages?: unknown[]; maxBytes?: number; - /** If set, enforce that file paths are within this sandbox root */ - sandboxRoot?: string; + sandbox?: { root: string; bridge: SandboxFsBridge }; }): Promise<{ /** Images for the current prompt (existingImages + detected in current prompt) */ images: ImageContent[]; @@ -406,7 +388,7 @@ export async function detectAndLoadPromptImages(params: { for (const ref of allRefs) { const image = await loadImageFromRef(ref, params.workspaceDir, { maxBytes: params.maxBytes, - sandboxRoot: params.sandboxRoot, + sandbox: params.sandbox, }); if (image) { if (ref.messageIndex !== undefined) { diff --git a/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-d.e2e.test.ts b/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-d.e2e.test.ts index cf6fd4d7507..362d2835b86 100644 --- a/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-d.e2e.test.ts +++ b/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-d.e2e.test.ts @@ -4,7 +4,8 @@ import path from "node:path"; import sharp from "sharp"; import { describe, expect, it } from "vitest"; import "./test-helpers/fast-coding-tools.js"; -import { createOpenClawCodingTools } from "./pi-tools.js"; +import { createMoltbotCodingTools } from "./pi-tools.js"; +import { createHostSandboxFsBridge } from "./test-helpers/host-sandbox-fs-bridge.js"; const defaultTools = createOpenClawCodingTools(); @@ -72,14 +73,16 @@ describe("createOpenClawCodingTools", () => { } }); it("filters tools by sandbox policy", () => { + const sandboxDir = path.join(os.tmpdir(), "moltbot-sandbox"); const sandbox = { enabled: true, sessionKey: "sandbox:test", - workspaceDir: path.join(os.tmpdir(), "openclaw-sandbox"), - agentWorkspaceDir: path.join(os.tmpdir(), "openclaw-workspace"), + workspaceDir: sandboxDir, + agentWorkspaceDir: path.join(os.tmpdir(), "moltbot-workspace"), workspaceAccess: "none", containerName: "openclaw-sbx-test", containerWorkdir: "/workspace", + fsBridge: createHostSandboxFsBridge(sandboxDir), docker: { image: "openclaw-sandbox:bookworm-slim", containerPrefix: "openclaw-sbx-", @@ -103,14 +106,16 @@ describe("createOpenClawCodingTools", () => { expect(tools.some((tool) => tool.name === "browser")).toBe(false); }); it("hard-disables write/edit when sandbox workspaceAccess is ro", () => { + const sandboxDir = path.join(os.tmpdir(), "moltbot-sandbox"); const sandbox = { enabled: true, sessionKey: "sandbox:test", - workspaceDir: path.join(os.tmpdir(), "openclaw-sandbox"), - agentWorkspaceDir: path.join(os.tmpdir(), "openclaw-workspace"), + workspaceDir: sandboxDir, + agentWorkspaceDir: path.join(os.tmpdir(), "moltbot-workspace"), workspaceAccess: "ro", containerName: "openclaw-sbx-test", containerWorkdir: "/workspace", + fsBridge: createHostSandboxFsBridge(sandboxDir), docker: { image: "openclaw-sandbox:bookworm-slim", containerPrefix: "openclaw-sbx-", diff --git a/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping.e2e.test.ts b/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping.e2e.test.ts index 2ec219f6144..d4153740fd6 100644 --- a/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping.e2e.test.ts +++ b/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping.e2e.test.ts @@ -7,6 +7,7 @@ import "./test-helpers/fast-coding-tools.js"; import { createOpenClawTools } from "./openclaw-tools.js"; import { __testing, createOpenClawCodingTools } from "./pi-tools.js"; import { createSandboxedReadTool } from "./pi-tools.read.js"; +import { createHostSandboxFsBridge } from "./test-helpers/host-sandbox-fs-bridge.js"; import { createBrowserTool } from "./tools/browser-tool.js"; const defaultTools = createOpenClawCodingTools(); @@ -467,7 +468,10 @@ describe("createOpenClawCodingTools", () => { const outsidePath = path.join(os.tmpdir(), "openclaw-outside.txt"); await fs.writeFile(outsidePath, "outside", "utf8"); try { - const readTool = createSandboxedReadTool(tmpDir); + const readTool = createSandboxedReadTool({ + root: tmpDir, + bridge: createHostSandboxFsBridge(tmpDir), + }); await expect(readTool.execute("sandbox-1", { file_path: outsidePath })).rejects.toThrow( /sandbox root/i, ); diff --git a/src/agents/pi-tools.read.ts b/src/agents/pi-tools.read.ts index c30333c4f4c..30ca5fec3e5 100644 --- a/src/agents/pi-tools.read.ts +++ b/src/agents/pi-tools.read.ts @@ -1,6 +1,7 @@ import type { AgentToolResult } from "@mariozechner/pi-agent-core"; import { createEditTool, createReadTool, createWriteTool } from "@mariozechner/pi-coding-agent"; import type { AnyAgentTool } from "./pi-tools.types.js"; +import type { SandboxFsBridge } from "./sandbox/fs-bridge.js"; import { detectMime } from "../media/mime.js"; import { assertSandboxPath } from "./sandbox-paths.js"; import { sanitizeToolResultImages } from "./tool-images.js"; @@ -268,19 +269,36 @@ function wrapSandboxPathGuard(tool: AnyAgentTool, root: string): AnyAgentTool { }; } -export function createSandboxedReadTool(root: string) { - const base = createReadTool(root) as unknown as AnyAgentTool; - return wrapSandboxPathGuard(createOpenClawReadTool(base), root); +type SandboxToolParams = { + root: string; + bridge: SandboxFsBridge; +}; + +export function createSandboxedReadTool(params: SandboxToolParams) { + const base = createReadTool(params.root, { + operations: createSandboxReadOperations(params), + }) as unknown as AnyAgentTool; + return wrapSandboxPathGuard(createOpenClawReadTool(base), params.root); } -export function createSandboxedWriteTool(root: string) { - const base = createWriteTool(root) as unknown as AnyAgentTool; - return wrapSandboxPathGuard(wrapToolParamNormalization(base, CLAUDE_PARAM_GROUPS.write), root); +export function createSandboxedWriteTool(params: SandboxToolParams) { + const base = createWriteTool(params.root, { + operations: createSandboxWriteOperations(params), + }) as unknown as AnyAgentTool; + return wrapSandboxPathGuard( + wrapToolParamNormalization(base, CLAUDE_PARAM_GROUPS.write), + params.root, + ); } -export function createSandboxedEditTool(root: string) { - const base = createEditTool(root) as unknown as AnyAgentTool; - return wrapSandboxPathGuard(wrapToolParamNormalization(base, CLAUDE_PARAM_GROUPS.edit), root); +export function createSandboxedEditTool(params: SandboxToolParams) { + const base = createEditTool(params.root, { + operations: createSandboxEditOperations(params), + }) as unknown as AnyAgentTool; + return wrapSandboxPathGuard( + wrapToolParamNormalization(base, CLAUDE_PARAM_GROUPS.edit), + params.root, + ); } export function createOpenClawReadTool(base: AnyAgentTool): AnyAgentTool { @@ -300,3 +318,53 @@ export function createOpenClawReadTool(base: AnyAgentTool): AnyAgentTool { }, }; } + +function createSandboxReadOperations(params: SandboxToolParams) { + return { + readFile: (absolutePath: string) => + params.bridge.readFile({ filePath: absolutePath, cwd: params.root }), + access: async (absolutePath: string) => { + const stat = await params.bridge.stat({ filePath: absolutePath, cwd: params.root }); + if (!stat) { + throw createFsAccessError("ENOENT", absolutePath); + } + }, + detectImageMimeType: async (absolutePath: string) => { + const buffer = await params.bridge.readFile({ filePath: absolutePath, cwd: params.root }); + const mime = await detectMime({ buffer, filePath: absolutePath }); + return mime && mime.startsWith("image/") ? mime : undefined; + }, + } as const; +} + +function createSandboxWriteOperations(params: SandboxToolParams) { + return { + mkdir: async (dir: string) => { + await params.bridge.mkdirp({ filePath: dir, cwd: params.root }); + }, + writeFile: async (absolutePath: string, content: string) => { + await params.bridge.writeFile({ filePath: absolutePath, cwd: params.root, data: content }); + }, + } as const; +} + +function createSandboxEditOperations(params: SandboxToolParams) { + return { + readFile: (absolutePath: string) => + params.bridge.readFile({ filePath: absolutePath, cwd: params.root }), + writeFile: (absolutePath: string, content: string) => + params.bridge.writeFile({ filePath: absolutePath, cwd: params.root, data: content }), + access: async (absolutePath: string) => { + const stat = await params.bridge.stat({ filePath: absolutePath, cwd: params.root }); + if (!stat) { + throw createFsAccessError("ENOENT", absolutePath); + } + }, + } as const; +} + +function createFsAccessError(code: string, filePath: string): NodeJS.ErrnoException { + const error = new Error(`Sandbox FS error (${code}): ${filePath}`) as NodeJS.ErrnoException; + error.code = code; + return error; +} diff --git a/src/agents/pi-tools.ts b/src/agents/pi-tools.ts index 811d4708742..d3118fbbcc2 100644 --- a/src/agents/pi-tools.ts +++ b/src/agents/pi-tools.ts @@ -233,6 +233,7 @@ export function createOpenClawCodingTools(options?: { ]); const execConfig = resolveExecConfig(options?.config); 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; @@ -245,10 +246,19 @@ export function createOpenClawCodingTools(options?: { allowModels: applyPatchConfig?.allowModels, }); + if (sandboxRoot && !sandboxFsBridge) { + throw new Error("Sandbox filesystem bridge is unavailable."); + } + const base = (codingTools as unknown as AnyAgentTool[]).flatMap((tool) => { if (tool.name === readTool.name) { if (sandboxRoot) { - return [createSandboxedReadTool(sandboxRoot)]; + return [ + createSandboxedReadTool({ + root: sandboxRoot, + bridge: sandboxFsBridge!, + }), + ]; } const freshReadTool = createReadTool(workspaceRoot); return [createOpenClawReadTool(freshReadTool)]; @@ -312,13 +322,19 @@ export function createOpenClawCodingTools(options?: { ? null : createApplyPatchTool({ cwd: sandboxRoot ?? workspaceRoot, - sandboxRoot: sandboxRoot && allowWorkspaceWrites ? sandboxRoot : undefined, + sandbox: + sandboxRoot && allowWorkspaceWrites + ? { root: sandboxRoot, bridge: sandboxFsBridge! } + : undefined, }); const tools: AnyAgentTool[] = [ ...base, ...(sandboxRoot ? allowWorkspaceWrites - ? [createSandboxedEditTool(sandboxRoot), createSandboxedWriteTool(sandboxRoot)] + ? [ + createSandboxedEditTool({ root: sandboxRoot, bridge: sandboxFsBridge! }), + createSandboxedWriteTool({ root: sandboxRoot, bridge: sandboxFsBridge! }), + ] : [] : []), ...(applyPatchTool ? [applyPatchTool as unknown as AnyAgentTool] : []), @@ -339,6 +355,7 @@ export function createOpenClawCodingTools(options?: { agentGroupSpace: options?.groupSpace ?? null, agentDir: options?.agentDir, sandboxRoot, + sandboxFsBridge, workspaceDir: options?.workspaceDir, sandboxed: !!sandbox, config: options?.config, diff --git a/src/agents/pi-tools.workspace-paths.e2e.test.ts b/src/agents/pi-tools.workspace-paths.e2e.test.ts index 320bd7f9364..ea53e691ac1 100644 --- a/src/agents/pi-tools.workspace-paths.e2e.test.ts +++ b/src/agents/pi-tools.workspace-paths.e2e.test.ts @@ -3,11 +3,7 @@ import os from "node:os"; import path from "node:path"; import { describe, expect, it, vi } from "vitest"; import { createOpenClawCodingTools } from "./pi-tools.js"; - -vi.mock("../plugins/tools.js", () => ({ - getPluginToolMeta: () => undefined, - resolvePluginTools: () => [], -})); +import { createHostSandboxFsBridge } from "./test-helpers/host-sandbox-fs-bridge.js"; vi.mock("../infra/shell-env.js", async (importOriginal) => { const mod = await importOriginal(); @@ -163,6 +159,7 @@ describe("sandboxed workspace paths", () => { workspaceAccess: "rw", containerName: "openclaw-sbx-test", containerWorkdir: "/workspace", + fsBridge: createHostSandboxFsBridge(sandboxDir), docker: { image: "openclaw-sandbox:bookworm-slim", containerPrefix: "openclaw-sbx-", diff --git a/src/agents/sandbox/context.ts b/src/agents/sandbox/context.ts index 9f654dc2989..b82c3bcc838 100644 --- a/src/agents/sandbox/context.ts +++ b/src/agents/sandbox/context.ts @@ -9,6 +9,7 @@ import { DEFAULT_AGENT_WORKSPACE_DIR } from "../workspace.js"; import { ensureSandboxBrowser } from "./browser.js"; import { resolveSandboxConfigForAgent } from "./config.js"; import { ensureSandboxContainer } from "./docker.js"; +import { createSandboxFsBridge } from "./fs-bridge.js"; import { maybePruneSandboxes } from "./prune.js"; import { resolveSandboxRuntimeStatus } from "./runtime-status.js"; import { resolveSandboxScopeKey, resolveSandboxWorkspaceDir } from "./shared.js"; @@ -83,7 +84,7 @@ export async function resolveSandboxContext(params: { evaluateEnabled, }); - return { + const sandboxContext: SandboxContext = { enabled: true, sessionKey: rawSessionKey, workspaceDir, @@ -96,6 +97,10 @@ export async function resolveSandboxContext(params: { browserAllowHostControl: cfg.browser.allowHostControl, browser: browser ?? undefined, }; + + sandboxContext.fsBridge = createSandboxFsBridge({ sandbox: sandboxContext }); + + return sandboxContext; } export async function ensureSandboxWorkspaceForSession(params: { diff --git a/src/agents/sandbox/docker.ts b/src/agents/sandbox/docker.ts index 9ddec1978c5..5223e6edcf0 100644 --- a/src/agents/sandbox/docker.ts +++ b/src/agents/sandbox/docker.ts @@ -1,5 +1,109 @@ import { spawn } from "node:child_process"; -import type { SandboxConfig, SandboxDockerConfig, SandboxWorkspaceAccess } from "./types.js"; + +type ExecDockerRawOptions = { + allowFailure?: boolean; + input?: Buffer | string; + signal?: AbortSignal; +}; + +export type ExecDockerRawResult = { + stdout: Buffer; + stderr: Buffer; + code: number; +}; + +type ExecDockerRawError = Error & { + code: number; + stdout: Buffer; + stderr: Buffer; +}; + +function createAbortError(): Error { + const err = new Error("Aborted"); + err.name = "AbortError"; + return err; +} + +export function execDockerRaw( + args: string[], + opts?: ExecDockerRawOptions, +): Promise { + return new Promise((resolve, reject) => { + const child = spawn("docker", args, { + stdio: ["pipe", "pipe", "pipe"], + }); + const stdoutChunks: Buffer[] = []; + const stderrChunks: Buffer[] = []; + let aborted = false; + + const signal = opts?.signal; + const handleAbort = () => { + if (aborted) { + return; + } + aborted = true; + child.kill("SIGTERM"); + }; + if (signal) { + if (signal.aborted) { + handleAbort(); + } else { + signal.addEventListener("abort", handleAbort); + } + } + + child.stdout?.on("data", (chunk) => { + stdoutChunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)); + }); + child.stderr?.on("data", (chunk) => { + stderrChunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)); + }); + + child.on("error", (error) => { + if (signal) { + signal.removeEventListener("abort", handleAbort); + } + reject(error); + }); + + child.on("close", (code) => { + if (signal) { + signal.removeEventListener("abort", handleAbort); + } + const stdout = Buffer.concat(stdoutChunks); + const stderr = Buffer.concat(stderrChunks); + if (aborted || signal?.aborted) { + reject(createAbortError()); + return; + } + const exitCode = code ?? 0; + if (exitCode !== 0 && !opts?.allowFailure) { + const message = stderr.length > 0 ? stderr.toString("utf8").trim() : ""; + const error: ExecDockerRawError = Object.assign( + new Error(message || `docker ${args.join(" ")} failed`), + { + code: exitCode, + stdout, + stderr, + }, + ); + reject(error); + return; + } + resolve({ stdout, stderr, code: exitCode }); + }); + + const stdin = child.stdin; + if (stdin) { + if (opts?.input !== undefined) { + stdin.end(opts.input); + } else { + stdin.end(); + } + } + }); +} + import { formatCliCommand } from "../../cli/command-format.js"; import { defaultRuntime } from "../../runtime.js"; import { computeSandboxConfigHash } from "./config-hash.js"; @@ -9,28 +113,15 @@ import { resolveSandboxAgentId, resolveSandboxScopeKey, slugifySessionKey } from const HOT_CONTAINER_WINDOW_MS = 5 * 60 * 1000; -export function execDocker(args: string[], opts?: { allowFailure?: boolean }) { - return new Promise<{ stdout: string; stderr: string; code: number }>((resolve, reject) => { - const child = spawn("docker", args, { - stdio: ["ignore", "pipe", "pipe"], - }); - let stdout = ""; - let stderr = ""; - child.stdout?.on("data", (chunk) => { - stdout += chunk.toString(); - }); - child.stderr?.on("data", (chunk) => { - stderr += chunk.toString(); - }); - child.on("close", (code) => { - const exitCode = code ?? 0; - if (exitCode !== 0 && !opts?.allowFailure) { - reject(new Error(stderr.trim() || `docker ${args.join(" ")} failed`)); - return; - } - resolve({ stdout, stderr, code: exitCode }); - }); - }); +export type ExecDockerOptions = ExecDockerRawOptions; + +export async function execDocker(args: string[], opts?: ExecDockerOptions) { + const result = await execDockerRaw(args, opts); + return { + stdout: result.stdout.toString("utf8"), + stderr: result.stderr.toString("utf8"), + code: result.code, + }; } export async function readDockerPort(containerName: string, port: number) { @@ -195,9 +286,7 @@ export function buildSandboxCreateArgs(params: { if (typeof params.cfg.cpus === "number" && params.cfg.cpus > 0) { args.push("--cpus", String(params.cfg.cpus)); } - for (const [name, value] of Object.entries(params.cfg.ulimits ?? {}) as Array< - [string, string | number | { soft?: number; hard?: number }] - >) { + for (const [name, value] of Object.entries(params.cfg.ulimits ?? {})) { const formatted = formatUlimitValue(name, value); if (formatted) { args.push("--ulimit", formatted); diff --git a/src/agents/sandbox/fs-bridge.test.ts b/src/agents/sandbox/fs-bridge.test.ts new file mode 100644 index 00000000000..c956bfd6a40 --- /dev/null +++ b/src/agents/sandbox/fs-bridge.test.ts @@ -0,0 +1,88 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("./docker.js", () => ({ + execDockerRaw: vi.fn(), +})); + +import type { SandboxContext } from "./types.js"; +import { execDockerRaw } from "./docker.js"; +import { createSandboxFsBridge } from "./fs-bridge.js"; + +const mockedExecDockerRaw = vi.mocked(execDockerRaw); + +const sandbox: SandboxContext = { + enabled: true, + sessionKey: "sandbox:test", + workspaceDir: "/tmp/workspace", + agentWorkspaceDir: "/tmp/workspace", + workspaceAccess: "rw", + containerName: "moltbot-sbx-test", + containerWorkdir: "/workspace", + docker: { + image: "moltbot-sandbox:bookworm-slim", + containerPrefix: "moltbot-sbx-", + network: "none", + user: "1000:1000", + workdir: "/workspace", + readOnlyRoot: false, + tmpfs: [], + capDrop: [], + seccompProfile: "", + apparmorProfile: "", + setupCommand: "", + binds: [], + dns: [], + extraHosts: [], + pidsLimit: 0, + }, + tools: { allow: ["*"], deny: [] }, + browserAllowHostControl: false, +}; + +describe("sandbox fs bridge shell compatibility", () => { + beforeEach(() => { + mockedExecDockerRaw.mockReset(); + mockedExecDockerRaw.mockImplementation(async (args) => { + const script = args[5] ?? ""; + if (script.includes('stat -c "%F|%s|%Y"')) { + return { + stdout: Buffer.from("regular file|1|2"), + stderr: Buffer.alloc(0), + code: 0, + }; + } + if (script.includes('cat -- "$1"')) { + return { + stdout: Buffer.from("content"), + stderr: Buffer.alloc(0), + code: 0, + }; + } + return { + stdout: Buffer.alloc(0), + stderr: Buffer.alloc(0), + code: 0, + }; + }); + }); + + it("uses POSIX-safe shell prologue in all bridge commands", async () => { + const bridge = createSandboxFsBridge({ sandbox }); + + await bridge.readFile({ filePath: "a.txt" }); + await bridge.writeFile({ filePath: "b.txt", data: "hello" }); + await bridge.mkdirp({ filePath: "nested" }); + await bridge.remove({ filePath: "b.txt" }); + await bridge.rename({ from: "a.txt", to: "c.txt" }); + await bridge.stat({ filePath: "c.txt" }); + + expect(mockedExecDockerRaw).toHaveBeenCalled(); + + const scripts = mockedExecDockerRaw.mock.calls.map(([args]) => args[5] ?? ""); + const executables = mockedExecDockerRaw.mock.calls.map(([args]) => args[3] ?? ""); + + expect(executables.every((shell) => shell === "sh")).toBe(true); + expect(scripts.every((script) => script.includes("set -eu;"))).toBe(true); + expect(scripts.some((script) => script.includes("pipefail"))).toBe(false); + }); +}); diff --git a/src/agents/sandbox/fs-bridge.ts b/src/agents/sandbox/fs-bridge.ts new file mode 100644 index 00000000000..e7d0d12a16a --- /dev/null +++ b/src/agents/sandbox/fs-bridge.ts @@ -0,0 +1,257 @@ +import path from "node:path"; +import type { SandboxContext, SandboxWorkspaceAccess } from "./types.js"; +import { resolveSandboxPath } from "../sandbox-paths.js"; +import { execDockerRaw, type ExecDockerRawResult } from "./docker.js"; + +type RunCommandOptions = { + args?: string[]; + stdin?: Buffer | string; + allowFailure?: boolean; + signal?: AbortSignal; +}; + +export type SandboxResolvedPath = { + hostPath: string; + relativePath: string; + containerPath: string; +}; + +export type SandboxFsStat = { + type: "file" | "directory" | "other"; + size: number; + mtimeMs: number; +}; + +export type SandboxFsBridge = { + resolvePath(params: { filePath: string; cwd?: string }): SandboxResolvedPath; + readFile(params: { filePath: string; cwd?: string; signal?: AbortSignal }): Promise; + writeFile(params: { + filePath: string; + cwd?: string; + data: Buffer | string; + encoding?: BufferEncoding; + mkdir?: boolean; + signal?: AbortSignal; + }): Promise; + mkdirp(params: { filePath: string; cwd?: string; signal?: AbortSignal }): Promise; + remove(params: { + filePath: string; + cwd?: string; + recursive?: boolean; + force?: boolean; + signal?: AbortSignal; + }): Promise; + rename(params: { from: string; to: string; cwd?: string; signal?: AbortSignal }): Promise; + stat(params: { + filePath: string; + cwd?: string; + signal?: AbortSignal; + }): Promise; +}; + +export function createSandboxFsBridge(params: { sandbox: SandboxContext }): SandboxFsBridge { + return new SandboxFsBridgeImpl(params.sandbox); +} + +class SandboxFsBridgeImpl implements SandboxFsBridge { + private readonly sandbox: SandboxContext; + + constructor(sandbox: SandboxContext) { + this.sandbox = sandbox; + } + + resolvePath(params: { filePath: string; cwd?: string }): SandboxResolvedPath { + return resolveSandboxFsPath({ + sandbox: this.sandbox, + filePath: params.filePath, + cwd: params.cwd, + }); + } + + async readFile(params: { + filePath: string; + cwd?: string; + signal?: AbortSignal; + }): Promise { + const target = this.resolvePath(params); + const result = await this.runCommand('set -eu; cat -- "$1"', { + args: [target.containerPath], + signal: params.signal, + }); + return result.stdout; + } + + async writeFile(params: { + filePath: string; + cwd?: string; + data: Buffer | string; + encoding?: BufferEncoding; + mkdir?: boolean; + signal?: AbortSignal; + }): Promise { + this.ensureWriteAccess("write files"); + const target = this.resolvePath(params); + const buffer = Buffer.isBuffer(params.data) + ? params.data + : Buffer.from(params.data, params.encoding ?? "utf8"); + const script = + params.mkdir === false + ? 'set -eu; cat >"$1"' + : 'set -eu; dir=$(dirname -- "$1"); if [ "$dir" != "." ]; then mkdir -p -- "$dir"; fi; cat >"$1"'; + await this.runCommand(script, { + args: [target.containerPath], + stdin: buffer, + signal: params.signal, + }); + } + + async mkdirp(params: { filePath: string; cwd?: string; signal?: AbortSignal }): Promise { + this.ensureWriteAccess("create directories"); + const target = this.resolvePath(params); + await this.runCommand('set -eu; mkdir -p -- "$1"', { + args: [target.containerPath], + signal: params.signal, + }); + } + + async remove(params: { + filePath: string; + cwd?: string; + recursive?: boolean; + force?: boolean; + signal?: AbortSignal; + }): Promise { + this.ensureWriteAccess("remove files"); + const target = this.resolvePath(params); + const flags = [params.force === false ? "" : "-f", params.recursive ? "-r" : ""].filter( + Boolean, + ); + const rmCommand = flags.length > 0 ? `rm ${flags.join(" ")}` : "rm"; + await this.runCommand(`set -eu; ${rmCommand} -- "$1"`, { + args: [target.containerPath], + signal: params.signal, + }); + } + + async rename(params: { + from: string; + to: string; + cwd?: string; + signal?: AbortSignal; + }): Promise { + this.ensureWriteAccess("rename files"); + const from = this.resolvePath({ filePath: params.from, cwd: params.cwd }); + const to = this.resolvePath({ filePath: params.to, cwd: params.cwd }); + await this.runCommand( + 'set -eu; dir=$(dirname -- "$2"); if [ "$dir" != "." ]; then mkdir -p -- "$dir"; fi; mv -- "$1" "$2"', + { + args: [from.containerPath, to.containerPath], + signal: params.signal, + }, + ); + } + + async stat(params: { + filePath: string; + cwd?: string; + signal?: AbortSignal; + }): Promise { + const target = this.resolvePath(params); + const result = await this.runCommand('set -eu; stat -c "%F|%s|%Y" -- "$1"', { + args: [target.containerPath], + signal: params.signal, + allowFailure: true, + }); + if (result.code !== 0) { + const stderr = result.stderr.toString("utf8"); + if (stderr.includes("No such file or directory")) { + return null; + } + const message = stderr.trim() || `stat failed with code ${result.code}`; + throw new Error(`stat failed for ${target.containerPath}: ${message}`); + } + const text = result.stdout.toString("utf8").trim(); + const [typeRaw, sizeRaw, mtimeRaw] = text.split("|"); + const size = Number.parseInt(sizeRaw ?? "0", 10); + const mtime = Number.parseInt(mtimeRaw ?? "0", 10) * 1000; + return { + type: coerceStatType(typeRaw), + size: Number.isFinite(size) ? size : 0, + mtimeMs: Number.isFinite(mtime) ? mtime : 0, + }; + } + + private async runCommand( + script: string, + options: RunCommandOptions = {}, + ): Promise { + const dockerArgs = [ + "exec", + "-i", + this.sandbox.containerName, + "sh", + "-c", + script, + "moltbot-sandbox-fs", + ]; + if (options.args?.length) { + dockerArgs.push(...options.args); + } + return execDockerRaw(dockerArgs, { + input: options.stdin, + allowFailure: options.allowFailure, + signal: options.signal, + }); + } + + private ensureWriteAccess(action: string) { + if (!allowsWrites(this.sandbox.workspaceAccess)) { + throw new Error( + `Sandbox workspace (${this.sandbox.workspaceAccess}) does not allow ${action}.`, + ); + } + } +} + +function allowsWrites(access: SandboxWorkspaceAccess): boolean { + return access === "rw"; +} + +function resolveSandboxFsPath(params: { + sandbox: SandboxContext; + filePath: string; + cwd?: string; +}): SandboxResolvedPath { + const root = params.sandbox.workspaceDir; + const cwd = params.cwd ?? root; + const { resolved, relative } = resolveSandboxPath({ + filePath: params.filePath, + cwd, + root, + }); + const normalizedRelative = relative + ? relative.split(path.sep).filter(Boolean).join(path.posix.sep) + : ""; + const containerPath = normalizedRelative + ? path.posix.join(params.sandbox.containerWorkdir, normalizedRelative) + : params.sandbox.containerWorkdir; + return { + hostPath: resolved, + relativePath: normalizedRelative, + containerPath, + }; +} + +function coerceStatType(typeRaw?: string): "file" | "directory" | "other" { + if (!typeRaw) { + return "other"; + } + const normalized = typeRaw.trim().toLowerCase(); + if (normalized.includes("directory")) { + return "directory"; + } + if (normalized.includes("file")) { + return "file"; + } + return "other"; +} diff --git a/src/agents/sandbox/types.ts b/src/agents/sandbox/types.ts index f27dfd7157e..72d08fba316 100644 --- a/src/agents/sandbox/types.ts +++ b/src/agents/sandbox/types.ts @@ -1,3 +1,4 @@ +import type { SandboxFsBridge } from "./fs-bridge.js"; import type { SandboxDockerConfig } from "./types.docker.js"; export type { SandboxDockerConfig } from "./types.docker.js"; @@ -77,6 +78,7 @@ export type SandboxContext = { tools: SandboxToolPolicy; browserAllowHostControl: boolean; browser?: SandboxBrowserContext; + fsBridge?: SandboxFsBridge; }; export type SandboxWorkspaceInfo = { diff --git a/src/agents/test-helpers/host-sandbox-fs-bridge.ts b/src/agents/test-helpers/host-sandbox-fs-bridge.ts new file mode 100644 index 00000000000..4f3dc6bd8cd --- /dev/null +++ b/src/agents/test-helpers/host-sandbox-fs-bridge.ts @@ -0,0 +1,74 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import type { SandboxFsBridge, SandboxFsStat, SandboxResolvedPath } from "../sandbox/fs-bridge.js"; +import { resolveSandboxPath } from "../sandbox-paths.js"; + +export function createHostSandboxFsBridge(rootDir: string): SandboxFsBridge { + const root = path.resolve(rootDir); + + const resolvePath = (filePath: string, cwd?: string): SandboxResolvedPath => { + const resolved = resolveSandboxPath({ + filePath, + cwd: cwd ?? root, + root, + }); + const relativePath = resolved.relative + ? resolved.relative.split(path.sep).filter(Boolean).join(path.posix.sep) + : ""; + const containerPath = relativePath ? path.posix.join("/workspace", relativePath) : "/workspace"; + return { + hostPath: resolved.resolved, + relativePath, + containerPath, + }; + }; + + return { + resolvePath: ({ filePath, cwd }) => resolvePath(filePath, cwd), + readFile: async ({ filePath, cwd }) => { + const target = resolvePath(filePath, cwd); + return fs.readFile(target.hostPath); + }, + writeFile: async ({ filePath, cwd, data, mkdir = true }) => { + const target = resolvePath(filePath, cwd); + if (mkdir) { + await fs.mkdir(path.dirname(target.hostPath), { recursive: true }); + } + const buffer = Buffer.isBuffer(data) ? data : Buffer.from(data); + await fs.writeFile(target.hostPath, buffer); + }, + mkdirp: async ({ filePath, cwd }) => { + const target = resolvePath(filePath, cwd); + await fs.mkdir(target.hostPath, { recursive: true }); + }, + remove: async ({ filePath, cwd, recursive, force }) => { + const target = resolvePath(filePath, cwd); + await fs.rm(target.hostPath, { + recursive: recursive ?? false, + force: force ?? false, + }); + }, + rename: async ({ from, to, cwd }) => { + const source = resolvePath(from, cwd); + const target = resolvePath(to, cwd); + await fs.mkdir(path.dirname(target.hostPath), { recursive: true }); + await fs.rename(source.hostPath, target.hostPath); + }, + stat: async ({ filePath, cwd }) => { + try { + const target = resolvePath(filePath, cwd); + const stats = await fs.stat(target.hostPath); + return { + type: stats.isDirectory() ? "directory" : stats.isFile() ? "file" : "other", + size: stats.size, + mtimeMs: stats.mtimeMs, + } satisfies SandboxFsStat; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return null; + } + throw error; + } + }, + }; +} diff --git a/src/agents/tools/image-tool.e2e.test.ts b/src/agents/tools/image-tool.e2e.test.ts index 921246f94ce..2a9a1815337 100644 --- a/src/agents/tools/image-tool.e2e.test.ts +++ b/src/agents/tools/image-tool.e2e.test.ts @@ -3,6 +3,7 @@ import os from "node:os"; import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "../../config/config.js"; +import { createHostSandboxFsBridge } from "../test-helpers/host-sandbox-fs-bridge.js"; import { __testing, createImageTool, resolveImageModelConfigForTool } from "./image-tool.js"; async function writeAuthProfiles(agentDir: string, profiles: unknown) { @@ -156,12 +157,13 @@ describe("image tool implicit imageModel config", () => { await fs.mkdir(agentDir, { recursive: true }); await fs.mkdir(sandboxRoot, { recursive: true }); await fs.writeFile(path.join(sandboxRoot, "img.png"), "fake", "utf8"); + const sandbox = { root: sandboxRoot, bridge: createHostSandboxFsBridge(sandboxRoot) }; vi.stubEnv("OPENAI_API_KEY", "openai-test"); const cfg: OpenClawConfig = { agents: { defaults: { model: { primary: "minimax/MiniMax-M2.1" } } }, }; - const tool = createImageTool({ config: cfg, agentDir, sandboxRoot }); + const tool = createImageTool({ config: cfg, agentDir, sandbox }); expect(tool).not.toBeNull(); if (!tool) { throw new Error("expected image tool"); @@ -213,7 +215,8 @@ describe("image tool implicit imageModel config", () => { }, }, }; - const tool = createImageTool({ config: cfg, agentDir, sandboxRoot }); + const sandbox = { root: sandboxRoot, bridge: createHostSandboxFsBridge(sandboxRoot) }; + const tool = createImageTool({ config: cfg, agentDir, sandbox }); expect(tool).not.toBeNull(); if (!tool) { throw new Error("expected image tool"); diff --git a/src/agents/tools/image-tool.ts b/src/agents/tools/image-tool.ts index 6f713142625..9b08a0d19ec 100644 --- a/src/agents/tools/image-tool.ts +++ b/src/agents/tools/image-tool.ts @@ -1,8 +1,8 @@ import { type Api, type Context, complete, type Model } from "@mariozechner/pi-ai"; import { Type } from "@sinclair/typebox"; -import fs from "node:fs/promises"; import path from "node:path"; import type { OpenClawConfig } from "../../config/config.js"; +import type { SandboxFsBridge } from "../sandbox/fs-bridge.js"; import type { AnyAgentTool } from "./common.js"; import { resolveUserPath } from "../../utils.js"; import { loadWebMedia } from "../../web/media.js"; @@ -14,7 +14,6 @@ import { runWithImageModelFallback } from "../model-fallback.js"; import { resolveConfiguredModelRef } from "../model-selection.js"; import { ensureOpenClawModelsJson } from "../models-config.js"; import { discoverAuthStorage, discoverModels } from "../pi-model-discovery.js"; -import { assertSandboxPath } from "../sandbox-paths.js"; import { coerceImageAssistantText, coerceImageModelConfig, @@ -185,34 +184,42 @@ function buildImageContext(prompt: string, base64: string, mimeType: string): Co }; } +type ImageSandboxConfig = { + root: string; + bridge: SandboxFsBridge; +}; + async function resolveSandboxedImagePath(params: { - sandboxRoot: string; + sandbox: ImageSandboxConfig; imagePath: string; }): Promise<{ resolved: string; rewrittenFrom?: string }> { const normalize = (p: string) => (p.startsWith("file://") ? p.slice("file://".length) : p); const filePath = normalize(params.imagePath); try { - const out = await assertSandboxPath({ + const resolved = params.sandbox.bridge.resolvePath({ filePath, - cwd: params.sandboxRoot, - root: params.sandboxRoot, + cwd: params.sandbox.root, }); - return { resolved: out.resolved }; + return { resolved: resolved.hostPath }; } catch (err) { const name = path.basename(filePath); const candidateRel = path.join("media", "inbound", name); - const candidateAbs = path.join(params.sandboxRoot, candidateRel); try { - await fs.stat(candidateAbs); + const stat = await params.sandbox.bridge.stat({ + filePath: candidateRel, + cwd: params.sandbox.root, + }); + if (!stat) { + throw err; + } } catch { throw err; } - const out = await assertSandboxPath({ + const out = params.sandbox.bridge.resolvePath({ filePath: candidateRel, - cwd: params.sandboxRoot, - root: params.sandboxRoot, + cwd: params.sandbox.root, }); - return { resolved: out.resolved, rewrittenFrom: filePath }; + return { resolved: out.hostPath, rewrittenFrom: filePath }; } } @@ -306,7 +313,7 @@ async function runImagePrompt(params: { export function createImageTool(options?: { config?: OpenClawConfig; agentDir?: string; - sandboxRoot?: string; + sandbox?: ImageSandboxConfig; /** If true, the model has native vision capability and images in the prompt are auto-injected */ modelHasVision?: boolean; }): AnyAgentTool | null { @@ -385,14 +392,17 @@ export function createImageTool(options?: { const maxBytesMb = typeof record.maxBytesMb === "number" ? record.maxBytesMb : undefined; const maxBytes = pickMaxBytes(options?.config, maxBytesMb); - const sandboxRoot = options?.sandboxRoot?.trim(); + const sandboxConfig = + options?.sandbox && options?.sandbox.root.trim() + ? { root: options.sandbox.root.trim(), bridge: options.sandbox.bridge } + : null; const isUrl = isHttpUrl; - if (sandboxRoot && isUrl) { + if (sandboxConfig && isUrl) { throw new Error("Sandboxed image tool does not allow remote URLs."); } const resolvedImage = (() => { - if (sandboxRoot) { + if (sandboxConfig) { return imageRaw; } if (imageRaw.startsWith("~")) { @@ -402,9 +412,9 @@ export function createImageTool(options?: { })(); const resolvedPathInfo: { resolved: string; rewrittenFrom?: string } = isDataUrl ? { resolved: "" } - : sandboxRoot + : sandboxConfig ? await resolveSandboxedImagePath({ - sandboxRoot, + sandbox: sandboxConfig, imagePath: resolvedImage, }) : { @@ -416,7 +426,13 @@ export function createImageTool(options?: { const media = isDataUrl ? decodeDataUrl(resolvedImage) - : await loadWebMedia(resolvedPath ?? resolvedImage, maxBytes); + : sandboxConfig + ? await loadWebMedia(resolvedPath ?? resolvedImage, { + maxBytes, + readFile: (filePath) => + sandboxConfig.bridge.readFile({ filePath, cwd: sandboxConfig.root }), + }) + : await loadWebMedia(resolvedPath ?? resolvedImage, maxBytes); if (media.kind !== "image") { throw new Error(`Unsupported media type: ${media.kind}`); } diff --git a/src/web/media.test.ts b/src/web/media.test.ts index bc9c6392cac..d1f6d4e40c9 100644 --- a/src/web/media.test.ts +++ b/src/web/media.test.ts @@ -29,6 +29,22 @@ function buildDeterministicBytes(length: number): Buffer { return buffer; } +async function createLargeTestJpeg(): Promise<{ buffer: Buffer; file: string }> { + const buffer = await sharp({ + create: { + width: 1600, + height: 1600, + channels: 3, + background: "#ff0000", + }, + }) + .jpeg({ quality: 95 }) + .toBuffer(); + + const file = await writeTempFile(buffer, ".jpg"); + return { buffer, file }; +} + afterEach(async () => { await Promise.all(tmpFiles.map((file) => fs.rm(file, { force: true }))); tmpFiles.length = 0; @@ -70,6 +86,25 @@ describe("web media loading", () => { expect(result.buffer.length).toBeLessThan(buffer.length); }); + it("optimizes images when options object omits optimizeImages", async () => { + const { buffer, file } = await createLargeTestJpeg(); + const cap = Math.max(1, Math.floor(buffer.length * 0.8)); + + const result = await loadWebMedia(file, { maxBytes: cap }); + + expect(result.buffer.length).toBeLessThanOrEqual(cap); + expect(result.buffer.length).toBeLessThan(buffer.length); + }); + + it("allows callers to disable optimization via options object", async () => { + const { buffer, file } = await createLargeTestJpeg(); + const cap = Math.max(1, Math.floor(buffer.length * 0.8)); + + await expect(loadWebMedia(file, { maxBytes: cap, optimizeImages: false })).rejects.toThrow( + /Media exceeds/i, + ); + }); + it("sniffs mime before extension when loading local files", async () => { const pngBuffer = await sharp({ create: { width: 2, height: 2, channels: 3, background: "#00ff00" }, diff --git a/src/web/media.ts b/src/web/media.ts index bed9bafe18c..f7507223a34 100644 --- a/src/web/media.ts +++ b/src/web/media.ts @@ -28,6 +28,7 @@ type WebMediaOptions = { ssrfPolicy?: SsrFPolicy; /** Allowed root directories for local path reads. "any" skips the check (caller already validated). */ localRoots?: string[] | "any"; + readFile?: (filePath: string) => Promise; }; function getDefaultLocalRoots(): string[] { @@ -165,7 +166,13 @@ async function loadWebMediaInternal( mediaUrl: string, options: WebMediaOptions = {}, ): Promise { - const { maxBytes, optimizeImages = true, ssrfPolicy, localRoots } = options; + const { + maxBytes, + optimizeImages = true, + ssrfPolicy, + localRoots, + readFile: readFileOverride, + } = options; // Use fileURLToPath for proper handling of file:// URLs (handles file://localhost/path, etc.) if (mediaUrl.startsWith("file://")) { try { @@ -267,7 +274,7 @@ async function loadWebMediaInternal( await assertLocalMediaAllowed(mediaUrl, localRoots); // Local path - const data = await fs.readFile(mediaUrl); + const data = readFileOverride ? await readFileOverride(mediaUrl) : await fs.readFile(mediaUrl); const mime = await detectMime({ buffer: data, filePath: mediaUrl }); const kind = mediaKindFromMime(mime); let fileName = path.basename(mediaUrl) || undefined; @@ -287,27 +294,39 @@ async function loadWebMediaInternal( export async function loadWebMedia( mediaUrl: string, - maxBytes?: number, + maxBytesOrOptions?: number | WebMediaOptions, options?: { ssrfPolicy?: SsrFPolicy; localRoots?: string[] | "any" }, ): Promise { + if (typeof maxBytesOrOptions === "number" || maxBytesOrOptions === undefined) { + return await loadWebMediaInternal(mediaUrl, { + maxBytes: maxBytesOrOptions, + optimizeImages: true, + ssrfPolicy: options?.ssrfPolicy, + localRoots: options?.localRoots, + }); + } return await loadWebMediaInternal(mediaUrl, { - maxBytes, - optimizeImages: true, - ssrfPolicy: options?.ssrfPolicy, - localRoots: options?.localRoots, + ...maxBytesOrOptions, + optimizeImages: maxBytesOrOptions.optimizeImages ?? true, }); } export async function loadWebMediaRaw( mediaUrl: string, - maxBytes?: number, + maxBytesOrOptions?: number | WebMediaOptions, options?: { ssrfPolicy?: SsrFPolicy; localRoots?: string[] | "any" }, ): Promise { + if (typeof maxBytesOrOptions === "number" || maxBytesOrOptions === undefined) { + return await loadWebMediaInternal(mediaUrl, { + maxBytes: maxBytesOrOptions, + optimizeImages: false, + ssrfPolicy: options?.ssrfPolicy, + localRoots: options?.localRoots, + }); + } return await loadWebMediaInternal(mediaUrl, { - maxBytes, + ...maxBytesOrOptions, optimizeImages: false, - ssrfPolicy: options?.ssrfPolicy, - localRoots: options?.localRoots, }); }