From dcd19da42504efa842965fff242511551c6ed71f Mon Sep 17 00:00:00 2001 From: Agent Date: Sun, 1 Mar 2026 21:49:42 +0000 Subject: [PATCH] refactor: simplify sandbox boundary open flow --- src/agents/sandbox/fs-bridge.test.ts | 78 ++++++++++++++-------------- src/agents/sandbox/fs-bridge.ts | 3 +- src/infra/boundary-file-read.ts | 48 ++++++++++++++--- 3 files changed, 81 insertions(+), 48 deletions(-) diff --git a/src/agents/sandbox/fs-bridge.test.ts b/src/agents/sandbox/fs-bridge.test.ts index a6c89100250..bb673898a24 100644 --- a/src/agents/sandbox/fs-bridge.test.ts +++ b/src/agents/sandbox/fs-bridge.test.ts @@ -49,6 +49,15 @@ function createSandbox(overrides?: Partial): SandboxContext { }); } +async function withTempDir(prefix: string, run: (stateDir: string) => Promise): Promise { + const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), prefix)); + try { + return await run(stateDir); + } finally { + await fs.rm(stateDir, { recursive: true, force: true }); + } +} + describe("sandbox fs bridge shell compatibility", () => { beforeEach(() => { mockedExecDockerRaw.mockClear(); @@ -174,8 +183,7 @@ describe("sandbox fs bridge shell compatibility", () => { }); it("allows mkdirp for existing in-boundary subdirectories", async () => { - const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-fs-bridge-mkdirp-")); - try { + await withTempDir("openclaw-fs-bridge-mkdirp-", async (stateDir) => { const workspaceDir = path.join(stateDir, "workspace"); const nestedDir = path.join(workspaceDir, "memory", "kemik"); await fs.mkdir(nestedDir, { recursive: true }); @@ -193,14 +201,11 @@ describe("sandbox fs bridge shell compatibility", () => { expect(mkdirCall).toBeDefined(); const mkdirPath = mkdirCall ? getDockerPathArg(mkdirCall[0]) : ""; expect(mkdirPath).toBe("/workspace/memory/kemik"); - } finally { - await fs.rm(stateDir, { recursive: true, force: true }); - } + }); }); it("rejects mkdirp when target exists as a file", async () => { - const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-fs-bridge-mkdirp-file-")); - try { + await withTempDir("openclaw-fs-bridge-mkdirp-file-", async (stateDir) => { const workspaceDir = path.join(stateDir, "workspace"); const filePath = path.join(workspaceDir, "memory", "kemik"); await fs.mkdir(path.dirname(filePath), { recursive: true }); @@ -217,46 +222,43 @@ describe("sandbox fs bridge shell compatibility", () => { /cannot create directories/i, ); expect(mockedExecDockerRaw).not.toHaveBeenCalled(); - } finally { - await fs.rm(stateDir, { recursive: true, force: true }); - } + }); }); it("rejects pre-existing host symlink escapes before docker exec", async () => { - const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-fs-bridge-")); - const workspaceDir = path.join(stateDir, "workspace"); - const outsideDir = path.join(stateDir, "outside"); - const outsideFile = path.join(outsideDir, "secret.txt"); - await fs.mkdir(workspaceDir, { recursive: true }); - await fs.mkdir(outsideDir, { recursive: true }); - await fs.writeFile(outsideFile, "classified"); - await fs.symlink(outsideFile, path.join(workspaceDir, "link.txt")); + await withTempDir("openclaw-fs-bridge-", async (stateDir) => { + const workspaceDir = path.join(stateDir, "workspace"); + const outsideDir = path.join(stateDir, "outside"); + const outsideFile = path.join(outsideDir, "secret.txt"); + await fs.mkdir(workspaceDir, { recursive: true }); + await fs.mkdir(outsideDir, { recursive: true }); + await fs.writeFile(outsideFile, "classified"); + await fs.symlink(outsideFile, path.join(workspaceDir, "link.txt")); - const bridge = createSandboxFsBridge({ - sandbox: createSandbox({ - workspaceDir, - agentWorkspaceDir: workspaceDir, - }), + const bridge = createSandboxFsBridge({ + sandbox: createSandbox({ + workspaceDir, + agentWorkspaceDir: workspaceDir, + }), + }); + + await expect(bridge.readFile({ filePath: "link.txt" })).rejects.toThrow(/Symlink escapes/); + expect(mockedExecDockerRaw).not.toHaveBeenCalled(); }); - - await expect(bridge.readFile({ filePath: "link.txt" })).rejects.toThrow(/Symlink escapes/); - expect(mockedExecDockerRaw).not.toHaveBeenCalled(); - await fs.rm(stateDir, { recursive: true, force: true }); }); it("rejects pre-existing host hardlink escapes before docker exec", async () => { if (process.platform === "win32") { return; } - const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-fs-bridge-hardlink-")); - const workspaceDir = path.join(stateDir, "workspace"); - const outsideDir = path.join(stateDir, "outside"); - const outsideFile = path.join(outsideDir, "secret.txt"); - await fs.mkdir(workspaceDir, { recursive: true }); - await fs.mkdir(outsideDir, { recursive: true }); - await fs.writeFile(outsideFile, "classified"); - const hardlinkPath = path.join(workspaceDir, "link.txt"); - try { + await withTempDir("openclaw-fs-bridge-hardlink-", async (stateDir) => { + const workspaceDir = path.join(stateDir, "workspace"); + const outsideDir = path.join(stateDir, "outside"); + const outsideFile = path.join(outsideDir, "secret.txt"); + await fs.mkdir(workspaceDir, { recursive: true }); + await fs.mkdir(outsideDir, { recursive: true }); + await fs.writeFile(outsideFile, "classified"); + const hardlinkPath = path.join(workspaceDir, "link.txt"); try { await fs.link(outsideFile, hardlinkPath); } catch (err) { @@ -275,9 +277,7 @@ describe("sandbox fs bridge shell compatibility", () => { await expect(bridge.readFile({ filePath: "link.txt" })).rejects.toThrow(/hardlink|sandbox/i); expect(mockedExecDockerRaw).not.toHaveBeenCalled(); - } finally { - await fs.rm(stateDir, { recursive: true, force: true }); - } + }); }); it("rejects container-canonicalized paths outside allowed mounts", async () => { diff --git a/src/agents/sandbox/fs-bridge.ts b/src/agents/sandbox/fs-bridge.ts index 4896a9ae9a8..7439978184b 100644 --- a/src/agents/sandbox/fs-bridge.ts +++ b/src/agents/sandbox/fs-bridge.ts @@ -23,7 +23,6 @@ type PathSafetyOptions = { action: string; aliasPolicy?: PathAliasPolicy; requireWritable?: boolean; - allowMissingTarget?: boolean; allowedType?: SafeOpenSyncAllowedType; }; @@ -267,7 +266,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { allowedType: options.allowedType, }); if (!guarded.ok) { - if (guarded.reason !== "path" || options.allowMissingTarget === false) { + if (guarded.reason !== "path") { throw guarded.error instanceof Error ? guarded.error : new Error( diff --git a/src/infra/boundary-file-read.ts b/src/infra/boundary-file-read.ts index 32cbea18092..fdd39fc8d9c 100644 --- a/src/infra/boundary-file-read.ts +++ b/src/infra/boundary-file-read.ts @@ -74,13 +74,33 @@ export function openBoundaryFileSync(params: OpenBoundaryFileSyncParams): Bounda return { ok: false, reason: "validation", error }; } - const opened = openVerifiedFileSync({ - filePath: absolutePath, + return openBoundaryFileResolved({ + absolutePath, resolvedPath, + rootRealPath, + maxBytes: params.maxBytes, + rejectHardlinks: params.rejectHardlinks, + allowedType: params.allowedType, + ioFs, + }); +} + +function openBoundaryFileResolved(params: { + absolutePath: string; + resolvedPath: string; + rootRealPath: string; + maxBytes?: number; + rejectHardlinks?: boolean; + allowedType?: SafeOpenSyncAllowedType; + ioFs: BoundaryReadFs; +}): BoundaryFileOpenResult { + const opened = openVerifiedFileSync({ + filePath: params.absolutePath, + resolvedPath: params.resolvedPath, rejectHardlinks: params.rejectHardlinks ?? true, maxBytes: params.maxBytes, allowedType: params.allowedType, - ioFs, + ioFs: params.ioFs, }); if (!opened.ok) { return opened; @@ -90,24 +110,38 @@ export function openBoundaryFileSync(params: OpenBoundaryFileSyncParams): Bounda path: opened.path, fd: opened.fd, stat: opened.stat, - rootRealPath, + rootRealPath: params.rootRealPath, }; } export async function openBoundaryFile( params: OpenBoundaryFileParams, ): Promise { + const ioFs = params.ioFs ?? fs; + const absolutePath = path.resolve(params.absolutePath); + let resolvedPath: string; + let rootRealPath: string; try { - await resolveBoundaryPath({ - absolutePath: params.absolutePath, + const resolved = await resolveBoundaryPath({ + absolutePath, rootPath: params.rootPath, rootCanonicalPath: params.rootRealPath, boundaryLabel: params.boundaryLabel, policy: params.aliasPolicy, skipLexicalRootCheck: params.skipLexicalRootCheck, }); + resolvedPath = resolved.canonicalPath; + rootRealPath = resolved.rootCanonicalPath; } catch (error) { return { ok: false, reason: "validation", error }; } - return openBoundaryFileSync(params); + return openBoundaryFileResolved({ + absolutePath, + resolvedPath, + rootRealPath, + maxBytes: params.maxBytes, + rejectHardlinks: params.rejectHardlinks, + allowedType: params.allowedType, + ioFs, + }); }