diff --git a/src/agents/sandbox/fs-bridge-path-safety.ts b/src/agents/sandbox/fs-bridge-path-safety.ts index 1c7330ca526..a18ed500287 100644 --- a/src/agents/sandbox/fs-bridge-path-safety.ts +++ b/src/agents/sandbox/fs-bridge-path-safety.ts @@ -49,19 +49,17 @@ export class SandboxFsPathGuard { } async assertPathSafety(target: SandboxResolvedFsPath, options: PathSafetyOptions) { - const lexicalMount = this.resolveRequiredMount(target.containerPath, options.action); - await this.assertPathSafetyWithinMount(target, options, lexicalMount); + const guarded = await this.openBoundaryWithinRequiredMount(target, options.action, { + aliasPolicy: options.aliasPolicy, + allowedType: options.allowedType, + }); + await this.assertGuardedPathSafety(target, options, guarded); } async openReadableFile( target: SandboxResolvedFsPath, ): Promise { - const lexicalMount = this.resolveRequiredMount(target.containerPath, "read files"); - const opened = await openBoundaryFile({ - absolutePath: target.hostPath, - rootPath: lexicalMount.hostRoot, - boundaryLabel: "sandbox mount root", - }); + const opened = await this.openBoundaryWithinRequiredMount(target, "read files"); if (!opened.ok) { throw opened.error instanceof Error ? opened.error @@ -78,18 +76,11 @@ export class SandboxFsPathGuard { return lexicalMount; } - private async assertPathSafetyWithinMount( + private async assertGuardedPathSafety( target: SandboxResolvedFsPath, options: PathSafetyOptions, - lexicalMount: SandboxFsMount, + guarded: BoundaryFileOpenResult, ) { - const guarded = await openBoundaryFile({ - absolutePath: target.hostPath, - rootPath: lexicalMount.hostRoot, - boundaryLabel: "sandbox mount root", - aliasPolicy: options.aliasPolicy, - allowedType: options.allowedType, - }); if (!guarded.ok) { if (guarded.reason !== "path") { const canFallbackToDirectoryStat = @@ -110,12 +101,7 @@ export class SandboxFsPathGuard { containerPath: target.containerPath, allowFinalSymlinkForUnlink: options.aliasPolicy?.allowFinalSymlinkForUnlink === true, }); - const canonicalMount = this.resolveMountByContainerPath(canonicalContainerPath); - if (!canonicalMount) { - throw new Error( - `Sandbox path escapes allowed mounts; cannot ${options.action}: ${target.containerPath}`, - ); - } + const canonicalMount = this.resolveRequiredMount(canonicalContainerPath, options.action); if (options.requireWritable && !canonicalMount.writable) { throw new Error( `Sandbox path is read-only; cannot ${options.action}: ${target.containerPath}`, @@ -123,6 +109,25 @@ export class SandboxFsPathGuard { } } + private async openBoundaryWithinRequiredMount( + target: SandboxResolvedFsPath, + action: string, + options?: { + aliasPolicy?: PathAliasPolicy; + allowedType?: SafeOpenSyncAllowedType; + }, + ): Promise { + const lexicalMount = this.resolveRequiredMount(target.containerPath, action); + const guarded = await openBoundaryFile({ + absolutePath: target.hostPath, + rootPath: lexicalMount.hostRoot, + boundaryLabel: "sandbox mount root", + aliasPolicy: options?.aliasPolicy, + allowedType: options?.allowedType, + }); + return guarded; + } + async resolveAnchoredSandboxEntry(target: SandboxResolvedFsPath): Promise { const basename = path.posix.basename(target.containerPath); if (!basename || basename === "." || basename === "/") { diff --git a/src/agents/sandbox/fs-bridge-command-plans.ts b/src/agents/sandbox/fs-bridge-shell-command-plans.ts similarity index 100% rename from src/agents/sandbox/fs-bridge-command-plans.ts rename to src/agents/sandbox/fs-bridge-shell-command-plans.ts diff --git a/src/agents/sandbox/fs-bridge.anchored-ops.test.ts b/src/agents/sandbox/fs-bridge.anchored-ops.test.ts index c5a7603c717..79bc5a55f3c 100644 --- a/src/agents/sandbox/fs-bridge.anchored-ops.test.ts +++ b/src/agents/sandbox/fs-bridge.anchored-ops.test.ts @@ -1,3 +1,5 @@ +import fs from "node:fs/promises"; +import path from "node:path"; import { describe, expect, it } from "vitest"; import { createSandbox, @@ -6,60 +8,113 @@ import { findCallsByScriptFragment, getDockerArg, installFsBridgeTestHarness, + mockedExecDockerRaw, + withTempDir, } from "./fs-bridge.test-helpers.js"; describe("sandbox fs bridge anchored ops", () => { installFsBridgeTestHarness(); - it("anchors mkdirp operations on canonical parent + basename", async () => { + const pinnedReadCases = [ + { + name: "workspace reads use pinned file descriptors", + filePath: "notes/todo.txt", + contents: "todo", + setup: async (workspaceDir: string) => { + await fs.mkdir(path.join(workspaceDir, "notes"), { recursive: true }); + await fs.writeFile(path.join(workspaceDir, "notes", "todo.txt"), "todo"); + }, + sandbox: (workspaceDir: string) => + createSandbox({ + workspaceDir, + agentWorkspaceDir: workspaceDir, + }), + }, + { + name: "bind-mounted reads use pinned file descriptors", + filePath: "/workspace-two/README.md", + contents: "bind-read", + setup: async (workspaceDir: string, stateDir: string) => { + const bindRoot = path.join(stateDir, "workspace-two"); + await fs.mkdir(workspaceDir, { recursive: true }); + await fs.mkdir(bindRoot, { recursive: true }); + await fs.writeFile(path.join(bindRoot, "README.md"), "bind-read"); + }, + sandbox: (workspaceDir: string, stateDir: string) => + createSandbox({ + workspaceDir, + agentWorkspaceDir: workspaceDir, + docker: { + ...createSandbox().docker, + binds: [`${path.join(stateDir, "workspace-two")}:/workspace-two:ro`], + }, + }), + }, + ] as const; + + it.each(pinnedReadCases)("$name", async (testCase) => { + await withTempDir("openclaw-fs-bridge-contract-read-", async (stateDir) => { + const workspaceDir = path.join(stateDir, "workspace"); + await testCase.setup(workspaceDir, stateDir); + const bridge = createSandboxFsBridge({ + sandbox: testCase.sandbox(workspaceDir, stateDir), + }); + + await expect(bridge.readFile({ filePath: testCase.filePath })).resolves.toEqual( + Buffer.from(testCase.contents), + ); + expect(mockedExecDockerRaw).not.toHaveBeenCalled(); + }); + }); + + const anchoredCases = [ + { + name: "mkdirp anchors parent + basename", + invoke: (bridge: ReturnType) => + bridge.mkdirp({ filePath: "nested/leaf" }), + scriptFragment: 'mkdir -p -- "$2"', + expectedArgs: ["/workspace/nested", "leaf"], + forbiddenArgs: ["/workspace/nested/leaf"], + canonicalProbe: "/workspace/nested", + }, + { + name: "remove anchors parent + basename", + invoke: (bridge: ReturnType) => + bridge.remove({ filePath: "nested/file.txt" }), + scriptFragment: 'rm -f -- "$2"', + expectedArgs: ["/workspace/nested", "file.txt"], + forbiddenArgs: ["/workspace/nested/file.txt"], + canonicalProbe: "/workspace/nested", + }, + { + name: "rename anchors both parents + basenames", + invoke: (bridge: ReturnType) => + bridge.rename({ from: "from.txt", to: "nested/to.txt" }), + scriptFragment: 'mv -- "$3" "$2/$4"', + expectedArgs: ["/workspace", "/workspace/nested", "from.txt", "to.txt"], + forbiddenArgs: ["/workspace/from.txt", "/workspace/nested/to.txt"], + canonicalProbe: "/workspace/nested", + }, + ] as const; + + it.each(anchoredCases)("$name", async (testCase) => { const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); - await bridge.mkdirp({ filePath: "nested/leaf" }); + await testCase.invoke(bridge); - const mkdirCall = findCallByScriptFragment('mkdir -p -- "$2"'); - expect(mkdirCall).toBeDefined(); - const args = mkdirCall?.[0] ?? []; - expect(getDockerArg(args, 1)).toBe("/workspace/nested"); - expect(getDockerArg(args, 2)).toBe("leaf"); - expect(args).not.toContain("/workspace/nested/leaf"); + const opCall = findCallByScriptFragment(testCase.scriptFragment); + expect(opCall).toBeDefined(); + const args = opCall?.[0] ?? []; + testCase.expectedArgs.forEach((value, index) => { + expect(getDockerArg(args, index + 1)).toBe(value); + }); + testCase.forbiddenArgs.forEach((value) => { + expect(args).not.toContain(value); + }); const canonicalCalls = findCallsByScriptFragment('readlink -f -- "$cursor"'); expect( - canonicalCalls.some(([callArgs]) => getDockerArg(callArgs, 1) === "/workspace/nested"), + canonicalCalls.some(([callArgs]) => getDockerArg(callArgs, 1) === testCase.canonicalProbe), ).toBe(true); }); - - it("anchors remove operations on canonical parent + basename", async () => { - const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); - - await bridge.remove({ filePath: "nested/file.txt" }); - - const removeCall = findCallByScriptFragment('rm -f -- "$2"'); - expect(removeCall).toBeDefined(); - const args = removeCall?.[0] ?? []; - expect(getDockerArg(args, 1)).toBe("/workspace/nested"); - expect(getDockerArg(args, 2)).toBe("file.txt"); - expect(args).not.toContain("/workspace/nested/file.txt"); - - const canonicalCalls = findCallsByScriptFragment('readlink -f -- "$cursor"'); - expect( - canonicalCalls.some(([callArgs]) => getDockerArg(callArgs, 1) === "/workspace/nested"), - ).toBe(true); - }); - - it("anchors rename operations on canonical parents + basenames", async () => { - const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); - - await bridge.rename({ from: "from.txt", to: "nested/to.txt" }); - - const renameCall = findCallByScriptFragment('mv -- "$3" "$2/$4"'); - expect(renameCall).toBeDefined(); - const args = renameCall?.[0] ?? []; - expect(getDockerArg(args, 1)).toBe("/workspace"); - expect(getDockerArg(args, 2)).toBe("/workspace/nested"); - expect(getDockerArg(args, 3)).toBe("from.txt"); - expect(getDockerArg(args, 4)).toBe("to.txt"); - expect(args).not.toContain("/workspace/from.txt"); - expect(args).not.toContain("/workspace/nested/to.txt"); - }); }); diff --git a/src/agents/sandbox/fs-bridge.ts b/src/agents/sandbox/fs-bridge.ts index 37bf569e706..f937ad2c702 100644 --- a/src/agents/sandbox/fs-bridge.ts +++ b/src/agents/sandbox/fs-bridge.ts @@ -1,5 +1,6 @@ import fs from "node:fs"; import { execDockerRaw, type ExecDockerRawResult } from "./docker.js"; +import { SandboxFsPathGuard } from "./fs-bridge-path-safety.js"; import { buildMkdirpPlan, buildRemovePlan, @@ -7,8 +8,7 @@ import { buildStatPlan, buildWriteCommitPlan, type SandboxFsCommandPlan, -} from "./fs-bridge-command-plans.js"; -import { SandboxFsPathGuard } from "./fs-bridge-path-safety.js"; +} from "./fs-bridge-shell-command-plans.js"; import { buildSandboxFsMounts, resolveSandboxFsPathWithMounts, @@ -99,12 +99,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { signal?: AbortSignal; }): Promise { const target = this.resolveResolvedPath(params); - const opened = await this.pathGuard.openReadableFile(target); - try { - return fs.readFileSync(opened.fd); - } finally { - fs.closeSync(opened.fd); - } + return this.readPinnedFile(target); } async writeFile(params: { @@ -239,6 +234,15 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { }); } + private async readPinnedFile(target: SandboxResolvedFsPath): Promise { + const opened = await this.pathGuard.openReadableFile(target); + try { + return fs.readFileSync(opened.fd); + } finally { + fs.closeSync(opened.fd); + } + } + private async runCheckedCommand( plan: SandboxFsCommandPlan & { stdin?: Buffer | string; signal?: AbortSignal }, ): Promise {