From 40dfba85d87ff06e1f37032ecf252ba6d64c1213 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 8 Mar 2026 01:01:25 +0000 Subject: [PATCH] refactor(sandbox): split fs bridge path safety --- src/agents/sandbox/fs-bridge-command-plans.ts | 120 +++++ src/agents/sandbox/fs-bridge-path-safety.ts | 164 +++++++ .../sandbox/fs-bridge.anchored-ops.test.ts | 65 +++ src/agents/sandbox/fs-bridge.boundary.test.ts | 120 +++++ src/agents/sandbox/fs-bridge.shell.test.ts | 119 +++++ src/agents/sandbox/fs-bridge.test-helpers.ts | 160 +++++++ src/agents/sandbox/fs-bridge.test.ts | 414 ------------------ src/agents/sandbox/fs-bridge.ts | 309 +++---------- 8 files changed, 807 insertions(+), 664 deletions(-) create mode 100644 src/agents/sandbox/fs-bridge-command-plans.ts create mode 100644 src/agents/sandbox/fs-bridge-path-safety.ts create mode 100644 src/agents/sandbox/fs-bridge.anchored-ops.test.ts create mode 100644 src/agents/sandbox/fs-bridge.boundary.test.ts create mode 100644 src/agents/sandbox/fs-bridge.shell.test.ts create mode 100644 src/agents/sandbox/fs-bridge.test-helpers.ts delete mode 100644 src/agents/sandbox/fs-bridge.test.ts diff --git a/src/agents/sandbox/fs-bridge-command-plans.ts b/src/agents/sandbox/fs-bridge-command-plans.ts new file mode 100644 index 000000000..f411cb81b --- /dev/null +++ b/src/agents/sandbox/fs-bridge-command-plans.ts @@ -0,0 +1,120 @@ +import { PATH_ALIAS_POLICIES } from "../../infra/path-alias-guards.js"; +import type { AnchoredSandboxEntry, PathSafetyCheck } from "./fs-bridge-path-safety.js"; +import type { SandboxResolvedFsPath } from "./fs-paths.js"; + +export type SandboxFsCommandPlan = { + checks: PathSafetyCheck[]; + script: string; + args?: string[]; + recheckBeforeCommand?: boolean; + allowFailure?: boolean; +}; + +export function buildReadFilePlan(target: SandboxResolvedFsPath): SandboxFsCommandPlan { + return { + checks: [{ target, options: { action: "read files" } }], + script: 'set -eu; cat -- "$1"', + args: [target.containerPath], + }; +} + +export function buildWriteCommitPlan( + target: SandboxResolvedFsPath, + tempPath: string, +): SandboxFsCommandPlan { + return { + checks: [{ target, options: { action: "write files", requireWritable: true } }], + recheckBeforeCommand: true, + script: 'set -eu; mv -f -- "$1" "$2"', + args: [tempPath, target.containerPath], + }; +} + +export function buildMkdirpPlan( + target: SandboxResolvedFsPath, + anchoredTarget: AnchoredSandboxEntry, +): SandboxFsCommandPlan { + return { + checks: [ + { + target, + options: { + action: "create directories", + requireWritable: true, + allowedType: "directory", + }, + }, + ], + script: 'set -eu\ncd -- "$1"\nmkdir -p -- "$2"', + args: [anchoredTarget.canonicalParentPath, anchoredTarget.basename], + }; +} + +export function buildRemovePlan(params: { + target: SandboxResolvedFsPath; + anchoredTarget: AnchoredSandboxEntry; + recursive?: boolean; + force?: boolean; +}): SandboxFsCommandPlan { + const flags = [params.force === false ? "" : "-f", params.recursive ? "-r" : ""].filter(Boolean); + const rmCommand = flags.length > 0 ? `rm ${flags.join(" ")}` : "rm"; + return { + checks: [ + { + target: params.target, + options: { + action: "remove files", + requireWritable: true, + aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, + }, + }, + ], + recheckBeforeCommand: true, + script: `set -eu\ncd -- "$1"\n${rmCommand} -- "$2"`, + args: [params.anchoredTarget.canonicalParentPath, params.anchoredTarget.basename], + }; +} + +export function buildRenamePlan(params: { + from: SandboxResolvedFsPath; + to: SandboxResolvedFsPath; + anchoredFrom: AnchoredSandboxEntry; + anchoredTo: AnchoredSandboxEntry; +}): SandboxFsCommandPlan { + return { + checks: [ + { + target: params.from, + options: { + action: "rename files", + requireWritable: true, + aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, + }, + }, + { + target: params.to, + options: { + action: "rename files", + requireWritable: true, + }, + }, + ], + recheckBeforeCommand: true, + script: ["set -eu", 'mkdir -p -- "$2"', 'cd -- "$1"', 'mv -- "$3" "$2/$4"'].join("\n"), + args: [ + params.anchoredFrom.canonicalParentPath, + params.anchoredTo.canonicalParentPath, + params.anchoredFrom.basename, + params.anchoredTo.basename, + ], + }; +} + +export function buildStatPlan(target: SandboxResolvedFsPath): SandboxFsCommandPlan { + return { + checks: [{ target, options: { action: "stat files" } }], + script: 'set -eu; stat -c "%F|%s|%Y" -- "$1"', + args: [target.containerPath], + allowFailure: true, + }; +} diff --git a/src/agents/sandbox/fs-bridge-path-safety.ts b/src/agents/sandbox/fs-bridge-path-safety.ts new file mode 100644 index 000000000..2b8402260 --- /dev/null +++ b/src/agents/sandbox/fs-bridge-path-safety.ts @@ -0,0 +1,164 @@ +import fs from "node:fs"; +import path from "node:path"; +import { openBoundaryFile } from "../../infra/boundary-file-read.js"; +import type { PathAliasPolicy } from "../../infra/path-alias-guards.js"; +import type { SafeOpenSyncAllowedType } from "../../infra/safe-open-sync.js"; +import type { SandboxResolvedFsPath, SandboxFsMount } from "./fs-paths.js"; +import { isPathInsideContainerRoot, normalizeContainerPath } from "./path-utils.js"; + +export type PathSafetyOptions = { + action: string; + aliasPolicy?: PathAliasPolicy; + requireWritable?: boolean; + allowedType?: SafeOpenSyncAllowedType; +}; + +export type PathSafetyCheck = { + target: SandboxResolvedFsPath; + options: PathSafetyOptions; +}; + +export type AnchoredSandboxEntry = { + canonicalParentPath: string; + basename: string; +}; + +type RunCommand = ( + script: string, + options?: { + args?: string[]; + stdin?: Buffer | string; + allowFailure?: boolean; + signal?: AbortSignal; + }, +) => Promise<{ stdout: Buffer }>; + +export class SandboxFsPathGuard { + private readonly mountsByContainer: SandboxFsMount[]; + private readonly runCommand: RunCommand; + + constructor(params: { mountsByContainer: SandboxFsMount[]; runCommand: RunCommand }) { + this.mountsByContainer = params.mountsByContainer; + this.runCommand = params.runCommand; + } + + async assertPathChecks(checks: PathSafetyCheck[]): Promise { + for (const check of checks) { + await this.assertPathSafety(check.target, check.options); + } + } + + async assertPathSafety(target: SandboxResolvedFsPath, options: PathSafetyOptions) { + const lexicalMount = this.resolveMountByContainerPath(target.containerPath); + if (!lexicalMount) { + throw new Error( + `Sandbox path escapes allowed mounts; cannot ${options.action}: ${target.containerPath}`, + ); + } + + 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 = + options.allowedType === "directory" && this.pathIsExistingDirectory(target.hostPath); + if (!canFallbackToDirectoryStat) { + throw guarded.error instanceof Error + ? guarded.error + : new Error( + `Sandbox boundary checks failed; cannot ${options.action}: ${target.containerPath}`, + ); + } + } + } else { + fs.closeSync(guarded.fd); + } + + const canonicalContainerPath = await this.resolveCanonicalContainerPath({ + 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}`, + ); + } + if (options.requireWritable && !canonicalMount.writable) { + throw new Error( + `Sandbox path is read-only; cannot ${options.action}: ${target.containerPath}`, + ); + } + } + + async resolveAnchoredSandboxEntry(target: SandboxResolvedFsPath): Promise { + const basename = path.posix.basename(target.containerPath); + if (!basename || basename === "." || basename === "/") { + throw new Error(`Invalid sandbox entry target: ${target.containerPath}`); + } + const parentPath = normalizeContainerPath(path.posix.dirname(target.containerPath)); + const canonicalParentPath = await this.resolveCanonicalContainerPath({ + containerPath: parentPath, + allowFinalSymlinkForUnlink: false, + }); + return { + canonicalParentPath, + basename, + }; + } + + private pathIsExistingDirectory(hostPath: string): boolean { + try { + return fs.statSync(hostPath).isDirectory(); + } catch { + return false; + } + } + + private resolveMountByContainerPath(containerPath: string): SandboxFsMount | null { + const normalized = normalizeContainerPath(containerPath); + for (const mount of this.mountsByContainer) { + if (isPathInsideContainerRoot(normalizeContainerPath(mount.containerRoot), normalized)) { + return mount; + } + } + return null; + } + + private async resolveCanonicalContainerPath(params: { + containerPath: string; + allowFinalSymlinkForUnlink: boolean; + }): Promise { + const script = [ + "set -eu", + 'target="$1"', + 'allow_final="$2"', + 'suffix=""', + 'probe="$target"', + 'if [ "$allow_final" = "1" ] && [ -L "$target" ]; then probe=$(dirname -- "$target"); fi', + 'cursor="$probe"', + 'while [ ! -e "$cursor" ] && [ ! -L "$cursor" ]; do', + ' parent=$(dirname -- "$cursor")', + ' if [ "$parent" = "$cursor" ]; then break; fi', + ' base=$(basename -- "$cursor")', + ' suffix="/$base$suffix"', + ' cursor="$parent"', + "done", + 'canonical=$(readlink -f -- "$cursor")', + 'printf "%s%s\\n" "$canonical" "$suffix"', + ].join("\n"); + const result = await this.runCommand(script, { + args: [params.containerPath, params.allowFinalSymlinkForUnlink ? "1" : "0"], + }); + const canonical = result.stdout.toString("utf8").trim(); + if (!canonical.startsWith("/")) { + throw new Error(`Failed to resolve canonical sandbox path: ${params.containerPath}`); + } + return normalizeContainerPath(canonical); + } +} diff --git a/src/agents/sandbox/fs-bridge.anchored-ops.test.ts b/src/agents/sandbox/fs-bridge.anchored-ops.test.ts new file mode 100644 index 000000000..c5a7603c7 --- /dev/null +++ b/src/agents/sandbox/fs-bridge.anchored-ops.test.ts @@ -0,0 +1,65 @@ +import { describe, expect, it } from "vitest"; +import { + createSandbox, + createSandboxFsBridge, + findCallByScriptFragment, + findCallsByScriptFragment, + getDockerArg, + installFsBridgeTestHarness, +} from "./fs-bridge.test-helpers.js"; + +describe("sandbox fs bridge anchored ops", () => { + installFsBridgeTestHarness(); + + it("anchors mkdirp operations on canonical parent + basename", async () => { + const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); + + await bridge.mkdirp({ filePath: "nested/leaf" }); + + 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 canonicalCalls = findCallsByScriptFragment('readlink -f -- "$cursor"'); + expect( + canonicalCalls.some(([callArgs]) => getDockerArg(callArgs, 1) === "/workspace/nested"), + ).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.boundary.test.ts b/src/agents/sandbox/fs-bridge.boundary.test.ts new file mode 100644 index 000000000..dae841d37 --- /dev/null +++ b/src/agents/sandbox/fs-bridge.boundary.test.ts @@ -0,0 +1,120 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import { + createHostEscapeFixture, + createSandbox, + createSandboxFsBridge, + expectMkdirpAllowsExistingDirectory, + getScriptsFromCalls, + installDockerReadMock, + installFsBridgeTestHarness, + mockedExecDockerRaw, + withTempDir, +} from "./fs-bridge.test-helpers.js"; + +describe("sandbox fs bridge boundary validation", () => { + installFsBridgeTestHarness(); + + 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(); + }); + + it("allows mkdirp for existing in-boundary subdirectories", async () => { + await expectMkdirpAllowsExistingDirectory(); + }); + + it("allows mkdirp when boundary open reports io for an existing directory", async () => { + await expectMkdirpAllowsExistingDirectory({ forceBoundaryIoFallback: true }); + }); + + it("rejects mkdirp when target exists as a file", async () => { + 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 }); + await fs.writeFile(filePath, "not a directory"); + + const bridge = createSandboxFsBridge({ + sandbox: createSandbox({ + workspaceDir, + agentWorkspaceDir: workspaceDir, + }), + }); + + await expect(bridge.mkdirp({ filePath: "memory/kemik" })).rejects.toThrow( + /cannot create directories/i, + ); + const scripts = getScriptsFromCalls(); + expect(scripts.some((script) => script.includes('mkdir -p -- "$2"'))).toBe(false); + }); + }); + + it("rejects pre-existing host symlink escapes before docker exec", async () => { + await withTempDir("openclaw-fs-bridge-", async (stateDir) => { + const { workspaceDir, outsideFile } = await createHostEscapeFixture(stateDir); + if (process.platform === "win32") { + return; + } + await fs.symlink(outsideFile, path.join(workspaceDir, "link.txt")); + + const bridge = createSandboxFsBridge({ + sandbox: createSandbox({ + workspaceDir, + agentWorkspaceDir: workspaceDir, + }), + }); + + await expect(bridge.readFile({ filePath: "link.txt" })).rejects.toThrow(/Symlink escapes/); + expect(mockedExecDockerRaw).not.toHaveBeenCalled(); + }); + }); + + it("rejects pre-existing host hardlink escapes before docker exec", async () => { + if (process.platform === "win32") { + return; + } + await withTempDir("openclaw-fs-bridge-hardlink-", async (stateDir) => { + const { workspaceDir, outsideFile } = await createHostEscapeFixture(stateDir); + const hardlinkPath = path.join(workspaceDir, "link.txt"); + try { + await fs.link(outsideFile, hardlinkPath); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "EXDEV") { + return; + } + throw err; + } + + const bridge = createSandboxFsBridge({ + sandbox: createSandbox({ + workspaceDir, + agentWorkspaceDir: workspaceDir, + }), + }); + + await expect(bridge.readFile({ filePath: "link.txt" })).rejects.toThrow(/hardlink|sandbox/i); + expect(mockedExecDockerRaw).not.toHaveBeenCalled(); + }); + }); + + it("rejects container-canonicalized paths outside allowed mounts", async () => { + installDockerReadMock({ canonicalPath: "/etc/passwd" }); + + const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); + await expect(bridge.readFile({ filePath: "a.txt" })).rejects.toThrow(/escapes allowed mounts/i); + const scripts = getScriptsFromCalls(); + expect(scripts.some((script) => script.includes('cat -- "$1"'))).toBe(false); + }); +}); diff --git a/src/agents/sandbox/fs-bridge.shell.test.ts b/src/agents/sandbox/fs-bridge.shell.test.ts new file mode 100644 index 000000000..dd61ceadd --- /dev/null +++ b/src/agents/sandbox/fs-bridge.shell.test.ts @@ -0,0 +1,119 @@ +import { describe, expect, it } from "vitest"; +import { + createSandbox, + createSandboxFsBridge, + findCallByScriptFragment, + getDockerPathArg, + getScriptsFromCalls, + installFsBridgeTestHarness, + mockedExecDockerRaw, +} from "./fs-bridge.test-helpers.js"; + +describe("sandbox fs bridge shell compatibility", () => { + installFsBridgeTestHarness(); + + it("uses POSIX-safe shell prologue in all bridge commands", async () => { + const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); + + 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 = getScriptsFromCalls(); + const executables = mockedExecDockerRaw.mock.calls.map(([args]) => args[3] ?? ""); + + expect(executables.every((shell) => shell === "sh")).toBe(true); + expect(scripts.every((script) => /set -eu[;\n]/.test(script))).toBe(true); + expect(scripts.some((script) => script.includes("pipefail"))).toBe(false); + }); + + it("resolveCanonicalContainerPath script is valid POSIX sh (no do; token)", async () => { + const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); + + await bridge.readFile({ filePath: "a.txt" }); + + const scripts = getScriptsFromCalls(); + const canonicalScript = scripts.find((script) => script.includes("allow_final")); + expect(canonicalScript).toBeDefined(); + expect(canonicalScript).not.toMatch(/\bdo;/); + expect(canonicalScript).toMatch(/\bdo\n\s*parent=/); + }); + + it("reads inbound media-style filenames with triple-dash ids", async () => { + const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); + const inboundPath = "media/inbound/file_1095---f00a04a2-99a0-4d98-99b0-dfe61c5a4198.ogg"; + + await bridge.readFile({ filePath: inboundPath }); + + const readCall = findCallByScriptFragment('cat -- "$1"'); + expect(readCall).toBeDefined(); + const readPath = readCall ? getDockerPathArg(readCall[0]) : ""; + expect(readPath).toContain("file_1095---"); + }); + + it("resolves dash-leading basenames into absolute container paths", async () => { + const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); + + await bridge.readFile({ filePath: "--leading.txt" }); + + const readCall = findCallByScriptFragment('cat -- "$1"'); + expect(readCall).toBeDefined(); + const readPath = readCall ? getDockerPathArg(readCall[0]) : ""; + expect(readPath).toBe("/workspace/--leading.txt"); + }); + + 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(getDockerPathArg(args)).toBe("/workspace-two/README.md"); + }); + + it("writes via temp file + atomic rename (never direct truncation)", async () => { + const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); + + await bridge.writeFile({ filePath: "b.txt", data: "hello" }); + + const scripts = getScriptsFromCalls(); + expect(scripts.some((script) => script.includes('cat >"$1"'))).toBe(false); + expect(scripts.some((script) => script.includes('cat >"$tmp"'))).toBe(true); + expect(scripts.some((script) => script.includes('mv -f -- "$1" "$2"'))).toBe(true); + }); + + it("re-validates target before final rename and cleans temp file on failure", async () => { + const { mockedOpenBoundaryFile } = await import("./fs-bridge.test-helpers.js"); + mockedOpenBoundaryFile + .mockImplementationOnce(async () => ({ ok: false, reason: "path" })) + .mockImplementationOnce(async () => ({ + ok: false, + reason: "validation", + error: new Error("Hardlinked path is not allowed"), + })); + + const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); + await expect(bridge.writeFile({ filePath: "b.txt", data: "hello" })).rejects.toThrow( + /hardlinked path/i, + ); + + const scripts = getScriptsFromCalls(); + expect(scripts.some((script) => script.includes("mktemp"))).toBe(true); + expect(scripts.some((script) => script.includes('mv -f -- "$1" "$2"'))).toBe(false); + expect(scripts.some((script) => script.includes('rm -f -- "$1"'))).toBe(true); + }); +}); diff --git a/src/agents/sandbox/fs-bridge.test-helpers.ts b/src/agents/sandbox/fs-bridge.test-helpers.ts new file mode 100644 index 000000000..e81bb65a4 --- /dev/null +++ b/src/agents/sandbox/fs-bridge.test-helpers.ts @@ -0,0 +1,160 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { beforeEach, expect, vi } from "vitest"; + +vi.mock("./docker.js", () => ({ + execDockerRaw: vi.fn(), +})); + +vi.mock("../../infra/boundary-file-read.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + openBoundaryFile: vi.fn(actual.openBoundaryFile), + }; +}); + +import { openBoundaryFile } from "../../infra/boundary-file-read.js"; +import { execDockerRaw } from "./docker.js"; +import * as fsBridgeModule from "./fs-bridge.js"; +import { createSandboxTestContext } from "./test-fixtures.js"; +import type { SandboxContext } from "./types.js"; + +export const createSandboxFsBridge = fsBridgeModule.createSandboxFsBridge; + +export const mockedExecDockerRaw = vi.mocked(execDockerRaw); +export const mockedOpenBoundaryFile = vi.mocked(openBoundaryFile); +const DOCKER_SCRIPT_INDEX = 5; +const DOCKER_FIRST_SCRIPT_ARG_INDEX = 7; + +export function getDockerScript(args: string[]): string { + return String(args[DOCKER_SCRIPT_INDEX] ?? ""); +} + +export function getDockerArg(args: string[], position: number): string { + return String(args[DOCKER_FIRST_SCRIPT_ARG_INDEX + position - 1] ?? ""); +} + +export function getDockerPathArg(args: string[]): string { + return getDockerArg(args, 1); +} + +export function getScriptsFromCalls(): string[] { + return mockedExecDockerRaw.mock.calls.map(([args]) => getDockerScript(args)); +} + +export function findCallByScriptFragment(fragment: string) { + return mockedExecDockerRaw.mock.calls.find(([args]) => getDockerScript(args).includes(fragment)); +} + +export function findCallsByScriptFragment(fragment: string) { + return mockedExecDockerRaw.mock.calls.filter(([args]) => + getDockerScript(args).includes(fragment), + ); +} + +export function dockerExecResult(stdout: string) { + return { + stdout: Buffer.from(stdout), + stderr: Buffer.alloc(0), + code: 0, + }; +} + +export function createSandbox(overrides?: Partial): SandboxContext { + return createSandboxTestContext({ + overrides: { + containerName: "moltbot-sbx-test", + ...overrides, + }, + dockerOverrides: { + image: "moltbot-sandbox:bookworm-slim", + containerPrefix: "moltbot-sbx-", + }, + }); +} + +export 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 }); + } +} + +export function installDockerReadMock(params?: { canonicalPath?: string }) { + const canonicalPath = params?.canonicalPath; + mockedExecDockerRaw.mockImplementation(async (args) => { + const script = getDockerScript(args); + if (script.includes('readlink -f -- "$cursor"')) { + return dockerExecResult(`${canonicalPath ?? getDockerArg(args, 1)}\n`); + } + if (script.includes('stat -c "%F|%s|%Y"')) { + return dockerExecResult("regular file|1|2"); + } + if (script.includes('cat -- "$1"')) { + return dockerExecResult("content"); + } + if (script.includes("mktemp")) { + return dockerExecResult("/workspace/.openclaw-write-b.txt.ABC123\n"); + } + return dockerExecResult(""); + }); +} + +export async function createHostEscapeFixture(stateDir: string) { + 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"); + return { workspaceDir, outsideFile }; +} + +export async function expectMkdirpAllowsExistingDirectory(params?: { + forceBoundaryIoFallback?: boolean; +}) { + 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 }); + + if (params?.forceBoundaryIoFallback) { + mockedOpenBoundaryFile.mockImplementationOnce(async () => ({ + ok: false, + reason: "io", + error: Object.assign(new Error("EISDIR"), { code: "EISDIR" }), + })); + } + + const bridge = createSandboxFsBridge({ + sandbox: createSandbox({ + workspaceDir, + agentWorkspaceDir: workspaceDir, + }), + }); + + await expect(bridge.mkdirp({ filePath: "memory/kemik" })).resolves.toBeUndefined(); + + const mkdirCall = findCallByScriptFragment('mkdir -p -- "$2"'); + expect(mkdirCall).toBeDefined(); + const mkdirParent = mkdirCall ? getDockerArg(mkdirCall[0], 1) : ""; + const mkdirBase = mkdirCall ? getDockerArg(mkdirCall[0], 2) : ""; + expect(mkdirParent).toBe("/workspace/memory"); + expect(mkdirBase).toBe("kemik"); + }); +} + +export function installFsBridgeTestHarness() { + beforeEach(() => { + mockedExecDockerRaw.mockClear(); + mockedOpenBoundaryFile.mockClear(); + installDockerReadMock(); + }); +} diff --git a/src/agents/sandbox/fs-bridge.test.ts b/src/agents/sandbox/fs-bridge.test.ts deleted file mode 100644 index feec3f0af..000000000 --- a/src/agents/sandbox/fs-bridge.test.ts +++ /dev/null @@ -1,414 +0,0 @@ -import fs from "node:fs/promises"; -import os from "node:os"; -import path from "node:path"; -import { beforeEach, describe, expect, it, vi } from "vitest"; - -vi.mock("./docker.js", () => ({ - execDockerRaw: vi.fn(), -})); - -vi.mock("../../infra/boundary-file-read.js", async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - openBoundaryFile: vi.fn(actual.openBoundaryFile), - }; -}); - -import { openBoundaryFile } from "../../infra/boundary-file-read.js"; -import { execDockerRaw } from "./docker.js"; -import { createSandboxFsBridge } from "./fs-bridge.js"; -import { createSandboxTestContext } from "./test-fixtures.js"; -import type { SandboxContext } from "./types.js"; - -const mockedExecDockerRaw = vi.mocked(execDockerRaw); -const mockedOpenBoundaryFile = vi.mocked(openBoundaryFile); -const DOCKER_SCRIPT_INDEX = 5; -const DOCKER_FIRST_SCRIPT_ARG_INDEX = 7; - -function getDockerScript(args: string[]): string { - return String(args[DOCKER_SCRIPT_INDEX] ?? ""); -} - -function getDockerArg(args: string[], position: number): string { - return String(args[DOCKER_FIRST_SCRIPT_ARG_INDEX + position - 1] ?? ""); -} - -function getDockerPathArg(args: string[]): string { - return getDockerArg(args, 1); -} - -function getScriptsFromCalls(): string[] { - return mockedExecDockerRaw.mock.calls.map(([args]) => getDockerScript(args)); -} - -function findCallByScriptFragment(fragment: string) { - return mockedExecDockerRaw.mock.calls.find(([args]) => getDockerScript(args).includes(fragment)); -} - -function findCallsByScriptFragment(fragment: string) { - return mockedExecDockerRaw.mock.calls.filter(([args]) => - getDockerScript(args).includes(fragment), - ); -} - -function dockerExecResult(stdout: string) { - return { - stdout: Buffer.from(stdout), - stderr: Buffer.alloc(0), - code: 0, - }; -} - -function createSandbox(overrides?: Partial): SandboxContext { - return createSandboxTestContext({ - overrides: { - containerName: "moltbot-sbx-test", - ...overrides, - }, - dockerOverrides: { - image: "moltbot-sandbox:bookworm-slim", - containerPrefix: "moltbot-sbx-", - }, - }); -} - -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 }); - } -} - -function installDockerReadMock(params?: { canonicalPath?: string }) { - const canonicalPath = params?.canonicalPath; - mockedExecDockerRaw.mockImplementation(async (args) => { - const script = getDockerScript(args); - if (script.includes('readlink -f -- "$cursor"')) { - return dockerExecResult(`${canonicalPath ?? getDockerArg(args, 1)}\n`); - } - if (script.includes('stat -c "%F|%s|%Y"')) { - return dockerExecResult("regular file|1|2"); - } - if (script.includes('cat -- "$1"')) { - return dockerExecResult("content"); - } - if (script.includes("mktemp")) { - return dockerExecResult("/workspace/.openclaw-write-b.txt.ABC123\n"); - } - return dockerExecResult(""); - }); -} - -async function createHostEscapeFixture(stateDir: string) { - 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"); - return { workspaceDir, outsideFile }; -} - -async function expectMkdirpAllowsExistingDirectory(params?: { forceBoundaryIoFallback?: boolean }) { - 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 }); - - if (params?.forceBoundaryIoFallback) { - mockedOpenBoundaryFile.mockImplementationOnce(async () => ({ - ok: false, - reason: "io", - error: Object.assign(new Error("EISDIR"), { code: "EISDIR" }), - })); - } - - const bridge = createSandboxFsBridge({ - sandbox: createSandbox({ - workspaceDir, - agentWorkspaceDir: workspaceDir, - }), - }); - - await expect(bridge.mkdirp({ filePath: "memory/kemik" })).resolves.toBeUndefined(); - - const mkdirCall = findCallByScriptFragment('mkdir -p -- "$2"'); - expect(mkdirCall).toBeDefined(); - const mkdirParent = mkdirCall ? getDockerArg(mkdirCall[0], 1) : ""; - const mkdirBase = mkdirCall ? getDockerArg(mkdirCall[0], 2) : ""; - expect(mkdirParent).toBe("/workspace/memory"); - expect(mkdirBase).toBe("kemik"); - }); -} - -describe("sandbox fs bridge shell compatibility", () => { - beforeEach(() => { - mockedExecDockerRaw.mockClear(); - mockedOpenBoundaryFile.mockClear(); - installDockerReadMock(); - }); - - it("uses POSIX-safe shell prologue in all bridge commands", async () => { - const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); - - 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 = getScriptsFromCalls(); - const executables = mockedExecDockerRaw.mock.calls.map(([args]) => args[3] ?? ""); - - expect(executables.every((shell) => shell === "sh")).toBe(true); - expect(scripts.every((script) => /set -eu[;\n]/.test(script))).toBe(true); - expect(scripts.some((script) => script.includes("pipefail"))).toBe(false); - }); - - it("resolveCanonicalContainerPath script is valid POSIX sh (no do; token)", async () => { - const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); - - await bridge.readFile({ filePath: "a.txt" }); - - const scripts = getScriptsFromCalls(); - const canonicalScript = scripts.find((script) => script.includes("allow_final")); - expect(canonicalScript).toBeDefined(); - // "; " joining can create "do; cmd", which is invalid in POSIX sh. - expect(canonicalScript).not.toMatch(/\bdo;/); - // Keep command on the next line after "do" for POSIX-sh safety. - expect(canonicalScript).toMatch(/\bdo\n\s*parent=/); - }); - - it("reads inbound media-style filenames with triple-dash ids", async () => { - const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); - const inboundPath = "media/inbound/file_1095---f00a04a2-99a0-4d98-99b0-dfe61c5a4198.ogg"; - - await bridge.readFile({ filePath: inboundPath }); - - const readCall = findCallByScriptFragment('cat -- "$1"'); - expect(readCall).toBeDefined(); - const readPath = readCall ? getDockerPathArg(readCall[0]) : ""; - expect(readPath).toContain("file_1095---"); - }); - - it("resolves dash-leading basenames into absolute container paths", async () => { - const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); - - await bridge.readFile({ filePath: "--leading.txt" }); - - const readCall = findCallByScriptFragment('cat -- "$1"'); - expect(readCall).toBeDefined(); - const readPath = readCall ? getDockerPathArg(readCall[0]) : ""; - expect(readPath).toBe("/workspace/--leading.txt"); - }); - - 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(getDockerPathArg(args)).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(); - }); - - it("writes via temp file + atomic rename (never direct truncation)", async () => { - const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); - - await bridge.writeFile({ filePath: "b.txt", data: "hello" }); - - const scripts = getScriptsFromCalls(); - expect(scripts.some((script) => script.includes('cat >"$1"'))).toBe(false); - expect(scripts.some((script) => script.includes('cat >"$tmp"'))).toBe(true); - expect(scripts.some((script) => script.includes('mv -f -- "$1" "$2"'))).toBe(true); - }); - - it("anchors mkdirp operations on canonical parent + basename", async () => { - const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); - - await bridge.mkdirp({ filePath: "nested/leaf" }); - - 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 canonicalCalls = findCallsByScriptFragment('readlink -f -- "$cursor"'); - expect( - canonicalCalls.some(([callArgs]) => getDockerArg(callArgs, 1) === "/workspace/nested"), - ).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"); - }); - - it("re-validates target before final rename and cleans temp file on failure", async () => { - mockedOpenBoundaryFile - .mockImplementationOnce(async () => ({ ok: false, reason: "path" })) - .mockImplementationOnce(async () => ({ - ok: false, - reason: "validation", - error: new Error("Hardlinked path is not allowed"), - })); - - const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); - await expect(bridge.writeFile({ filePath: "b.txt", data: "hello" })).rejects.toThrow( - /hardlinked path/i, - ); - - const scripts = getScriptsFromCalls(); - expect(scripts.some((script) => script.includes("mktemp"))).toBe(true); - expect(scripts.some((script) => script.includes('mv -f -- "$1" "$2"'))).toBe(false); - expect(scripts.some((script) => script.includes('rm -f -- "$1"'))).toBe(true); - }); - - it("allows mkdirp for existing in-boundary subdirectories", async () => { - await expectMkdirpAllowsExistingDirectory(); - }); - - it("allows mkdirp when boundary open reports io for an existing directory", async () => { - await expectMkdirpAllowsExistingDirectory({ forceBoundaryIoFallback: true }); - }); - - it("rejects mkdirp when target exists as a file", async () => { - 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 }); - await fs.writeFile(filePath, "not a directory"); - - const bridge = createSandboxFsBridge({ - sandbox: createSandbox({ - workspaceDir, - agentWorkspaceDir: workspaceDir, - }), - }); - - await expect(bridge.mkdirp({ filePath: "memory/kemik" })).rejects.toThrow( - /cannot create directories/i, - ); - const scripts = getScriptsFromCalls(); - expect(scripts.some((script) => script.includes('mkdir -p -- "$2"'))).toBe(false); - }); - }); - - it("rejects pre-existing host symlink escapes before docker exec", async () => { - await withTempDir("openclaw-fs-bridge-", async (stateDir) => { - const { workspaceDir, outsideFile } = await createHostEscapeFixture(stateDir); - // File symlinks require SeCreateSymbolicLinkPrivilege on Windows. - if (process.platform === "win32") { - return; - } - await fs.symlink(outsideFile, path.join(workspaceDir, "link.txt")); - - const bridge = createSandboxFsBridge({ - sandbox: createSandbox({ - workspaceDir, - agentWorkspaceDir: workspaceDir, - }), - }); - - await expect(bridge.readFile({ filePath: "link.txt" })).rejects.toThrow(/Symlink escapes/); - expect(mockedExecDockerRaw).not.toHaveBeenCalled(); - }); - }); - - it("rejects pre-existing host hardlink escapes before docker exec", async () => { - if (process.platform === "win32") { - return; - } - await withTempDir("openclaw-fs-bridge-hardlink-", async (stateDir) => { - const { workspaceDir, outsideFile } = await createHostEscapeFixture(stateDir); - const hardlinkPath = path.join(workspaceDir, "link.txt"); - try { - await fs.link(outsideFile, hardlinkPath); - } catch (err) { - if ((err as NodeJS.ErrnoException).code === "EXDEV") { - return; - } - throw err; - } - - const bridge = createSandboxFsBridge({ - sandbox: createSandbox({ - workspaceDir, - agentWorkspaceDir: workspaceDir, - }), - }); - - await expect(bridge.readFile({ filePath: "link.txt" })).rejects.toThrow(/hardlink|sandbox/i); - expect(mockedExecDockerRaw).not.toHaveBeenCalled(); - }); - }); - - it("rejects container-canonicalized paths outside allowed mounts", async () => { - installDockerReadMock({ canonicalPath: "/etc/passwd" }); - - const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); - await expect(bridge.readFile({ filePath: "a.txt" })).rejects.toThrow(/escapes allowed mounts/i); - const scripts = getScriptsFromCalls(); - expect(scripts.some((script) => script.includes('cat -- "$1"'))).toBe(false); - }); -}); diff --git a/src/agents/sandbox/fs-bridge.ts b/src/agents/sandbox/fs-bridge.ts index 4a281efc0..a52a8acfd 100644 --- a/src/agents/sandbox/fs-bridge.ts +++ b/src/agents/sandbox/fs-bridge.ts @@ -1,16 +1,20 @@ -import fs from "node:fs"; -import path from "node:path"; -import { openBoundaryFile } from "../../infra/boundary-file-read.js"; -import { PATH_ALIAS_POLICIES, type PathAliasPolicy } from "../../infra/path-alias-guards.js"; -import type { SafeOpenSyncAllowedType } from "../../infra/safe-open-sync.js"; import { execDockerRaw, type ExecDockerRawResult } from "./docker.js"; +import { + buildMkdirpPlan, + buildReadFilePlan, + buildRemovePlan, + buildRenamePlan, + buildStatPlan, + buildWriteCommitPlan, + type SandboxFsCommandPlan, +} from "./fs-bridge-command-plans.js"; +import { SandboxFsPathGuard } from "./fs-bridge-path-safety.js"; import { buildSandboxFsMounts, resolveSandboxFsPathWithMounts, type SandboxResolvedFsPath, - type SandboxFsMount, } from "./fs-paths.js"; -import { isPathInsideContainerRoot, normalizeContainerPath } from "./path-utils.js"; +import { normalizeContainerPath } from "./path-utils.js"; import type { SandboxContext, SandboxWorkspaceAccess } from "./types.js"; type RunCommandOptions = { @@ -20,23 +24,6 @@ type RunCommandOptions = { signal?: AbortSignal; }; -type PathSafetyOptions = { - action: string; - aliasPolicy?: PathAliasPolicy; - requireWritable?: boolean; - allowedType?: SafeOpenSyncAllowedType; -}; - -type PathSafetyCheck = { - target: SandboxResolvedFsPath; - options: PathSafetyOptions; -}; - -type AnchoredSandboxEntry = { - canonicalParentPath: string; - basename: string; -}; - export type SandboxResolvedPath = { hostPath: string; relativePath: string; @@ -83,14 +70,18 @@ export function createSandboxFsBridge(params: { sandbox: SandboxContext }): Sand class SandboxFsBridgeImpl implements SandboxFsBridge { private readonly sandbox: SandboxContext; private readonly mounts: ReturnType; - private readonly mountsByContainer: ReturnType; + private readonly pathGuard: SandboxFsPathGuard; constructor(sandbox: SandboxContext) { this.sandbox = sandbox; this.mounts = buildSandboxFsMounts(sandbox); - this.mountsByContainer = [...this.mounts].toSorted( + const mountsByContainer = [...this.mounts].toSorted( (a, b) => b.containerRoot.length - a.containerRoot.length, ); + this.pathGuard = new SandboxFsPathGuard({ + mountsByContainer, + runCommand: (script, options) => this.runCommand(script, options), + }); } resolvePath(params: { filePath: string; cwd?: string }): SandboxResolvedPath { @@ -108,12 +99,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { signal?: AbortSignal; }): Promise { const target = this.resolveResolvedPath(params); - const result = await this.runCheckedCommand({ - checks: [{ target, options: { action: "read files" } }], - script: 'set -eu; cat -- "$1"', - args: [target.containerPath], - signal: params.signal, - }); + const result = await this.runPlannedCommand(buildReadFilePlan(target), params.signal); return result.stdout; } @@ -127,7 +113,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { }): Promise { const target = this.resolveResolvedPath(params); this.ensureWriteAccess(target, "write files"); - await this.assertPathSafety(target, { action: "write files", requireWritable: true }); + await this.pathGuard.assertPathSafety(target, { action: "write files", requireWritable: true }); const buffer = Buffer.isBuffer(params.data) ? params.data : Buffer.from(params.data, params.encoding ?? "utf8"); @@ -140,10 +126,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { try { await this.runCheckedCommand({ - checks: [{ target, options: { action: "write files", requireWritable: true } }], - recheckBeforeCommand: true, - script: 'set -eu; mv -f -- "$1" "$2"', - args: [tempPath, target.containerPath], + ...buildWriteCommitPlan(target, tempPath), signal: params.signal, }); } catch (error) { @@ -155,22 +138,8 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { async mkdirp(params: { filePath: string; cwd?: string; signal?: AbortSignal }): Promise { const target = this.resolveResolvedPath(params); this.ensureWriteAccess(target, "create directories"); - const anchoredTarget = await this.resolveAnchoredSandboxEntry(target); - await this.runCheckedCommand({ - checks: [ - { - target, - options: { - action: "create directories", - requireWritable: true, - allowedType: "directory", - }, - }, - ], - script: 'set -eu\ncd -- "$1"\nmkdir -p -- "$2"', - args: [anchoredTarget.canonicalParentPath, anchoredTarget.basename], - signal: params.signal, - }); + const anchoredTarget = await this.pathGuard.resolveAnchoredSandboxEntry(target); + await this.runPlannedCommand(buildMkdirpPlan(target, anchoredTarget), params.signal); } async remove(params: { @@ -182,27 +151,16 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { }): Promise { const target = this.resolveResolvedPath(params); this.ensureWriteAccess(target, "remove files"); - const anchoredTarget = await this.resolveAnchoredSandboxEntry(target); - const flags = [params.force === false ? "" : "-f", params.recursive ? "-r" : ""].filter( - Boolean, + const anchoredTarget = await this.pathGuard.resolveAnchoredSandboxEntry(target); + await this.runPlannedCommand( + buildRemovePlan({ + target, + anchoredTarget, + recursive: params.recursive, + force: params.force, + }), + params.signal, ); - const rmCommand = flags.length > 0 ? `rm ${flags.join(" ")}` : "rm"; - await this.runCheckedCommand({ - checks: [ - { - target, - options: { - action: "remove files", - requireWritable: true, - aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, - }, - }, - ], - recheckBeforeCommand: true, - script: `set -eu\ncd -- "$1"\n${rmCommand} -- "$2"`, - args: [anchoredTarget.canonicalParentPath, anchoredTarget.basename], - signal: params.signal, - }); } async rename(params: { @@ -215,36 +173,17 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { const to = this.resolveResolvedPath({ filePath: params.to, cwd: params.cwd }); this.ensureWriteAccess(from, "rename files"); this.ensureWriteAccess(to, "rename files"); - const anchoredFrom = await this.resolveAnchoredSandboxEntry(from); - const anchoredTo = await this.resolveAnchoredSandboxEntry(to); - await this.runCheckedCommand({ - checks: [ - { - target: from, - options: { - action: "rename files", - requireWritable: true, - aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, - }, - }, - { - target: to, - options: { - action: "rename files", - requireWritable: true, - }, - }, - ], - recheckBeforeCommand: true, - script: ["set -eu", 'mkdir -p -- "$2"', 'cd -- "$1"', 'mv -- "$3" "$2/$4"'].join("\n"), - args: [ - anchoredFrom.canonicalParentPath, - anchoredTo.canonicalParentPath, - anchoredFrom.basename, - anchoredTo.basename, - ], - signal: params.signal, - }); + const anchoredFrom = await this.pathGuard.resolveAnchoredSandboxEntry(from); + const anchoredTo = await this.pathGuard.resolveAnchoredSandboxEntry(to); + await this.runPlannedCommand( + buildRenamePlan({ + from, + to, + anchoredFrom, + anchoredTo, + }), + params.signal, + ); } async stat(params: { @@ -253,13 +192,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { signal?: AbortSignal; }): Promise { const target = this.resolveResolvedPath(params); - const result = await this.runCheckedCommand({ - checks: [{ target, options: { action: "stat files" } }], - script: 'set -eu; stat -c "%F|%s|%Y" -- "$1"', - args: [target.containerPath], - signal: params.signal, - allowFailure: true, - }); + const result = await this.runPlannedCommand(buildStatPlan(target), params.signal); if (result.code !== 0) { const stderr = result.stderr.toString("utf8"); if (stderr.includes("No such file or directory")) { @@ -302,150 +235,26 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { }); } - private async runCheckedCommand(params: { - checks: PathSafetyCheck[]; - script: string; - args?: string[]; - stdin?: Buffer | string; - allowFailure?: boolean; - signal?: AbortSignal; - recheckBeforeCommand?: boolean; - }): Promise { - await this.assertPathChecks(params.checks); - if (params.recheckBeforeCommand) { - await this.assertPathChecks(params.checks); + private async runCheckedCommand( + plan: SandboxFsCommandPlan & { stdin?: Buffer | string; signal?: AbortSignal }, + ): Promise { + await this.pathGuard.assertPathChecks(plan.checks); + if (plan.recheckBeforeCommand) { + await this.pathGuard.assertPathChecks(plan.checks); } - return await this.runCommand(params.script, { - args: params.args, - stdin: params.stdin, - allowFailure: params.allowFailure, - signal: params.signal, + return await this.runCommand(plan.script, { + args: plan.args, + stdin: plan.stdin, + allowFailure: plan.allowFailure, + signal: plan.signal, }); } - private async assertPathChecks(checks: PathSafetyCheck[]): Promise { - for (const check of checks) { - await this.assertPathSafety(check.target, check.options); - } - } - - private async assertPathSafety(target: SandboxResolvedFsPath, options: PathSafetyOptions) { - const lexicalMount = this.resolveMountByContainerPath(target.containerPath); - if (!lexicalMount) { - throw new Error( - `Sandbox path escapes allowed mounts; cannot ${options.action}: ${target.containerPath}`, - ); - } - - 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") { - // Some platforms cannot open directories via openSync(O_RDONLY), even when - // the path is a valid in-boundary directory. Allow mkdirp to proceed in that - // narrow case by verifying the host path is an existing directory. - const canFallbackToDirectoryStat = - options.allowedType === "directory" && this.pathIsExistingDirectory(target.hostPath); - if (!canFallbackToDirectoryStat) { - throw guarded.error instanceof Error - ? guarded.error - : new Error( - `Sandbox boundary checks failed; cannot ${options.action}: ${target.containerPath}`, - ); - } - } - } else { - fs.closeSync(guarded.fd); - } - - const canonicalContainerPath = await this.resolveCanonicalContainerPath({ - 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}`, - ); - } - if (options.requireWritable && !canonicalMount.writable) { - throw new Error( - `Sandbox path is read-only; cannot ${options.action}: ${target.containerPath}`, - ); - } - } - - private pathIsExistingDirectory(hostPath: string): boolean { - try { - return fs.statSync(hostPath).isDirectory(); - } catch { - return false; - } - } - - private resolveMountByContainerPath(containerPath: string): SandboxFsMount | null { - const normalized = normalizeContainerPath(containerPath); - for (const mount of this.mountsByContainer) { - if (isPathInsideContainerRoot(normalizeContainerPath(mount.containerRoot), normalized)) { - return mount; - } - } - return null; - } - - private async resolveCanonicalContainerPath(params: { - containerPath: string; - allowFinalSymlinkForUnlink: boolean; - }): Promise { - const script = [ - "set -eu", - 'target="$1"', - 'allow_final="$2"', - 'suffix=""', - 'probe="$target"', - 'if [ "$allow_final" = "1" ] && [ -L "$target" ]; then probe=$(dirname -- "$target"); fi', - 'cursor="$probe"', - 'while [ ! -e "$cursor" ] && [ ! -L "$cursor" ]; do', - ' parent=$(dirname -- "$cursor")', - ' if [ "$parent" = "$cursor" ]; then break; fi', - ' base=$(basename -- "$cursor")', - ' suffix="/$base$suffix"', - ' cursor="$parent"', - "done", - 'canonical=$(readlink -f -- "$cursor")', - 'printf "%s%s\\n" "$canonical" "$suffix"', - ].join("\n"); - const result = await this.runCommand(script, { - args: [params.containerPath, params.allowFinalSymlinkForUnlink ? "1" : "0"], - }); - const canonical = result.stdout.toString("utf8").trim(); - if (!canonical.startsWith("/")) { - throw new Error(`Failed to resolve canonical sandbox path: ${params.containerPath}`); - } - return normalizeContainerPath(canonical); - } - - private async resolveAnchoredSandboxEntry( - target: SandboxResolvedFsPath, - ): Promise { - const basename = path.posix.basename(target.containerPath); - if (!basename || basename === "." || basename === "/") { - throw new Error(`Invalid sandbox entry target: ${target.containerPath}`); - } - const parentPath = normalizeContainerPath(path.posix.dirname(target.containerPath)); - const canonicalParentPath = await this.resolveCanonicalContainerPath({ - containerPath: parentPath, - allowFinalSymlinkForUnlink: false, - }); - return { - canonicalParentPath, - basename, - }; + private async runPlannedCommand( + plan: SandboxFsCommandPlan, + signal?: AbortSignal, + ): Promise { + return await this.runCheckedCommand({ ...plan, signal }); } private async writeFileToTempPath(params: {