diff --git a/src/agents/pi-tools.read.ts b/src/agents/pi-tools.read.ts index 6a45de5d96..a19f0e6cac 100644 --- a/src/agents/pi-tools.read.ts +++ b/src/agents/pi-tools.read.ts @@ -269,23 +269,6 @@ export function wrapToolWorkspaceRootGuard(tool: AnyAgentTool, root: string): An }; } -function wrapSandboxPathGuard(tool: AnyAgentTool, root: string): AnyAgentTool { - return { - ...tool, - execute: async (toolCallId, args, signal, onUpdate) => { - const normalized = normalizeToolParams(args); - const record = - normalized ?? - (args && typeof args === "object" ? (args as Record) : undefined); - const filePath = record?.path; - if (typeof filePath === "string" && filePath.trim()) { - await assertSandboxPath({ filePath, cwd: root, root }); - } - return tool.execute(toolCallId, normalized ?? args, signal, onUpdate); - }, - }; -} - type SandboxToolParams = { root: string; bridge: SandboxFsBridge; @@ -295,27 +278,21 @@ export function createSandboxedReadTool(params: SandboxToolParams) { const base = createReadTool(params.root, { operations: createSandboxReadOperations(params), }) as unknown as AnyAgentTool; - return wrapSandboxPathGuard(createOpenClawReadTool(base), params.root); + return createOpenClawReadTool(base); } 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, - ); + return wrapToolParamNormalization(base, CLAUDE_PARAM_GROUPS.write); } 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, - ); + return wrapToolParamNormalization(base, CLAUDE_PARAM_GROUPS.edit); } export function createOpenClawReadTool(base: AnyAgentTool): AnyAgentTool { diff --git a/src/agents/sandbox/fs-bridge.test.ts b/src/agents/sandbox/fs-bridge.test.ts index c956bfd6a4..0eeeb6ad98 100644 --- a/src/agents/sandbox/fs-bridge.test.ts +++ b/src/agents/sandbox/fs-bridge.test.ts @@ -10,34 +10,37 @@ 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, -}; +function createSandbox(overrides?: Partial): SandboxContext { + return { + 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, + ...overrides, + }; +} describe("sandbox fs bridge shell compatibility", () => { beforeEach(() => { @@ -67,7 +70,7 @@ describe("sandbox fs bridge shell compatibility", () => { }); it("uses POSIX-safe shell prologue in all bridge commands", async () => { - const bridge = createSandboxFsBridge({ sandbox }); + const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); await bridge.readFile({ filePath: "a.txt" }); await bridge.writeFile({ filePath: "b.txt", data: "hello" }); @@ -85,4 +88,37 @@ describe("sandbox fs bridge shell compatibility", () => { expect(scripts.every((script) => script.includes("set -eu;"))).toBe(true); expect(scripts.some((script) => script.includes("pipefail"))).toBe(false); }); + + it("resolves bind-mounted absolute container paths for reads", async () => { + const sandbox = createSandbox({ + docker: { + ...createSandbox().docker, + binds: ["/tmp/workspace-two:/workspace-two:ro"], + }, + }); + const bridge = createSandboxFsBridge({ sandbox }); + + await bridge.readFile({ filePath: "/workspace-two/README.md" }); + + const args = mockedExecDockerRaw.mock.calls.at(-1)?.[0] ?? []; + expect(args).toEqual( + expect.arrayContaining(["moltbot-sbx-test", "sh", "-c", 'set -eu; cat -- "$1"']), + ); + expect(args.at(-1)).toBe("/workspace-two/README.md"); + }); + + it("blocks writes into read-only bind mounts", async () => { + const sandbox = createSandbox({ + docker: { + ...createSandbox().docker, + binds: ["/tmp/workspace-two:/workspace-two:ro"], + }, + }); + const bridge = createSandboxFsBridge({ sandbox }); + + await expect( + bridge.writeFile({ filePath: "/workspace-two/new.txt", data: "hello" }), + ).rejects.toThrow(/read-only/); + expect(mockedExecDockerRaw).not.toHaveBeenCalled(); + }); }); diff --git a/src/agents/sandbox/fs-bridge.ts b/src/agents/sandbox/fs-bridge.ts index e7d0d12a16..dae5f6f22c 100644 --- a/src/agents/sandbox/fs-bridge.ts +++ b/src/agents/sandbox/fs-bridge.ts @@ -1,7 +1,10 @@ -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"; +import { + buildSandboxFsMounts, + resolveSandboxFsPathWithMounts, + type SandboxResolvedFsPath, +} from "./fs-paths.js"; type RunCommandOptions = { args?: string[]; @@ -55,17 +58,20 @@ export function createSandboxFsBridge(params: { sandbox: SandboxContext }): Sand class SandboxFsBridgeImpl implements SandboxFsBridge { private readonly sandbox: SandboxContext; + private readonly mounts: ReturnType; constructor(sandbox: SandboxContext) { this.sandbox = sandbox; + this.mounts = buildSandboxFsMounts(sandbox); } resolvePath(params: { filePath: string; cwd?: string }): SandboxResolvedPath { - return resolveSandboxFsPath({ - sandbox: this.sandbox, - filePath: params.filePath, - cwd: params.cwd, - }); + const target = this.resolveResolvedPath(params); + return { + hostPath: target.hostPath, + relativePath: target.relativePath, + containerPath: target.containerPath, + }; } async readFile(params: { @@ -73,7 +79,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { cwd?: string; signal?: AbortSignal; }): Promise { - const target = this.resolvePath(params); + const target = this.resolveResolvedPath(params); const result = await this.runCommand('set -eu; cat -- "$1"', { args: [target.containerPath], signal: params.signal, @@ -89,8 +95,8 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { mkdir?: boolean; signal?: AbortSignal; }): Promise { - this.ensureWriteAccess("write files"); - const target = this.resolvePath(params); + const target = this.resolveResolvedPath(params); + this.ensureWriteAccess(target, "write files"); const buffer = Buffer.isBuffer(params.data) ? params.data : Buffer.from(params.data, params.encoding ?? "utf8"); @@ -106,8 +112,8 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { } async mkdirp(params: { filePath: string; cwd?: string; signal?: AbortSignal }): Promise { - this.ensureWriteAccess("create directories"); - const target = this.resolvePath(params); + const target = this.resolveResolvedPath(params); + this.ensureWriteAccess(target, "create directories"); await this.runCommand('set -eu; mkdir -p -- "$1"', { args: [target.containerPath], signal: params.signal, @@ -121,8 +127,8 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { force?: boolean; signal?: AbortSignal; }): Promise { - this.ensureWriteAccess("remove files"); - const target = this.resolvePath(params); + const target = this.resolveResolvedPath(params); + this.ensureWriteAccess(target, "remove files"); const flags = [params.force === false ? "" : "-f", params.recursive ? "-r" : ""].filter( Boolean, ); @@ -139,9 +145,10 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { 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 }); + const from = this.resolveResolvedPath({ filePath: params.from, cwd: params.cwd }); + const to = this.resolveResolvedPath({ filePath: params.to, cwd: params.cwd }); + this.ensureWriteAccess(from, "rename files"); + this.ensureWriteAccess(to, "rename files"); await this.runCommand( 'set -eu; dir=$(dirname -- "$2"); if [ "$dir" != "." ]; then mkdir -p -- "$dir"; fi; mv -- "$1" "$2"', { @@ -156,7 +163,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { cwd?: string; signal?: AbortSignal; }): Promise { - const target = this.resolvePath(params); + const target = this.resolveResolvedPath(params); const result = await this.runCommand('set -eu; stat -c "%F|%s|%Y" -- "$1"', { args: [target.containerPath], signal: params.signal, @@ -204,44 +211,27 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { }); } - private ensureWriteAccess(action: string) { - if (!allowsWrites(this.sandbox.workspaceAccess)) { - throw new Error( - `Sandbox workspace (${this.sandbox.workspaceAccess}) does not allow ${action}.`, - ); + private ensureWriteAccess(target: SandboxResolvedFsPath, action: string) { + if (!allowsWrites(this.sandbox.workspaceAccess) || !target.writable) { + throw new Error(`Sandbox path is read-only; cannot ${action}: ${target.containerPath}`); } } + + private resolveResolvedPath(params: { filePath: string; cwd?: string }): SandboxResolvedFsPath { + return resolveSandboxFsPathWithMounts({ + filePath: params.filePath, + cwd: params.cwd ?? this.sandbox.workspaceDir, + defaultWorkspaceRoot: this.sandbox.workspaceDir, + defaultContainerRoot: this.sandbox.containerWorkdir, + mounts: this.mounts, + }); + } } 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";