diff --git a/src/agents/sandbox/fs-bridge-write-helper.test.ts b/src/agents/sandbox/fs-bridge-mutation-helper.test.ts similarity index 70% rename from src/agents/sandbox/fs-bridge-write-helper.test.ts rename to src/agents/sandbox/fs-bridge-mutation-helper.test.ts index 75da7046573..7f8e6ee7694 100644 --- a/src/agents/sandbox/fs-bridge-write-helper.test.ts +++ b/src/agents/sandbox/fs-bridge-mutation-helper.test.ts @@ -3,7 +3,7 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { describe, expect, it } from "vitest"; -import { SANDBOX_PINNED_WRITE_PYTHON } from "./fs-bridge-write-helper.js"; +import { SANDBOX_PINNED_FS_MUTATION_PYTHON } from "./fs-bridge-mutation-helper.js"; async function withTempRoot(prefix: string, run: (root: string) => Promise): Promise { const root = await fs.mkdtemp(path.join(os.tmpdir(), prefix)); @@ -14,23 +14,14 @@ async function withTempRoot(prefix: string, run: (root: string) => Promise } } -function runPinnedWrite(params: { - mountRoot: string; - relativeParentPath: string; - basename: string; - mkdir: boolean; - input: string; +function runPinnedMutation(params: { + op: "write" | "mkdirp" | "remove" | "rename"; + args: string[]; + input?: string; }) { return spawnSync( "python3", - [ - "-c", - SANDBOX_PINNED_WRITE_PYTHON, - params.mountRoot, - params.relativeParentPath, - params.basename, - params.mkdir ? "1" : "0", - ], + ["-c", SANDBOX_PINNED_FS_MUTATION_PYTHON, params.op, ...params.args], { input: params.input, encoding: "utf8", @@ -39,17 +30,15 @@ function runPinnedWrite(params: { ); } -describe("sandbox pinned write helper", () => { +describe("sandbox pinned mutation helper", () => { it("creates missing parents and writes through a pinned directory fd", async () => { await withTempRoot("openclaw-write-helper-", async (root) => { const workspace = path.join(root, "workspace"); await fs.mkdir(workspace, { recursive: true }); - const result = runPinnedWrite({ - mountRoot: workspace, - relativeParentPath: "nested/deeper", - basename: "note.txt", - mkdir: true, + const result = runPinnedMutation({ + op: "write", + args: [workspace, "nested/deeper", "note.txt", "1"], input: "hello", }); @@ -70,11 +59,9 @@ describe("sandbox pinned write helper", () => { await fs.mkdir(outside, { recursive: true }); await fs.symlink(outside, path.join(workspace, "alias")); - const result = runPinnedWrite({ - mountRoot: workspace, - relativeParentPath: "alias", - basename: "escape.txt", - mkdir: false, + const result = runPinnedMutation({ + op: "write", + args: [workspace, "alias", "escape.txt", "0"], input: "owned", }); diff --git a/src/agents/sandbox/fs-bridge-mutation-helper.ts b/src/agents/sandbox/fs-bridge-mutation-helper.ts new file mode 100644 index 00000000000..af03ec20338 --- /dev/null +++ b/src/agents/sandbox/fs-bridge-mutation-helper.ts @@ -0,0 +1,135 @@ +import { PATH_ALIAS_POLICIES } from "../../infra/path-alias-guards.js"; +import { SANDBOX_PINNED_FS_MUTATION_PYTHON } from "./fs-bridge-mutation-python-source.js"; +import type { PinnedSandboxEntry } from "./fs-bridge-path-safety.js"; +import type { SandboxFsCommandPlan } from "./fs-bridge-shell-command-plans.js"; +import type { SandboxResolvedFsPath } from "./fs-paths.js"; + +function buildPinnedMutationPlan(params: { + checks: SandboxFsCommandPlan["checks"]; + args: string[]; + stdin?: Buffer | string; +}): SandboxFsCommandPlan { + return { + checks: params.checks, + recheckBeforeCommand: true, + script: ["set -eu", "python3 - \"$@\" <<'PY'", SANDBOX_PINNED_FS_MUTATION_PYTHON, "PY"].join( + "\n", + ), + args: params.args, + stdin: params.stdin, + }; +} + +export function buildPinnedWritePlan(params: { + target: SandboxResolvedFsPath; + pinned: PinnedSandboxEntry; + mkdir: boolean; + stdin: Buffer | string; +}): SandboxFsCommandPlan { + return buildPinnedMutationPlan({ + checks: [ + { + target: params.target, + options: { action: "write files", requireWritable: true }, + }, + ], + args: [ + "write", + params.pinned.mountRootPath, + params.pinned.relativeParentPath, + params.pinned.basename, + params.mkdir ? "1" : "0", + ], + stdin: params.stdin, + }); +} + +export function buildPinnedMkdirpPlan(params: { + target: SandboxResolvedFsPath; + pinned: PinnedSandboxEntry; +}): SandboxFsCommandPlan { + return buildPinnedMutationPlan({ + checks: [ + { + target: params.target, + options: { + action: "create directories", + requireWritable: true, + allowedType: "directory", + }, + }, + ], + args: [ + "mkdirp", + params.pinned.mountRootPath, + params.pinned.relativeParentPath, + params.pinned.basename, + ], + }); +} + +export function buildPinnedRemovePlan(params: { + target: SandboxResolvedFsPath; + pinned: PinnedSandboxEntry; + recursive?: boolean; + force?: boolean; +}): SandboxFsCommandPlan { + return buildPinnedMutationPlan({ + checks: [ + { + target: params.target, + options: { + action: "remove files", + requireWritable: true, + aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, + }, + }, + ], + args: [ + "remove", + params.pinned.mountRootPath, + params.pinned.relativeParentPath, + params.pinned.basename, + params.recursive ? "1" : "0", + params.force === false ? "0" : "1", + ], + }); +} + +export function buildPinnedRenamePlan(params: { + from: SandboxResolvedFsPath; + to: SandboxResolvedFsPath; + pinnedFrom: PinnedSandboxEntry; + pinnedTo: PinnedSandboxEntry; +}): SandboxFsCommandPlan { + return buildPinnedMutationPlan({ + checks: [ + { + target: params.from, + options: { + action: "rename files", + requireWritable: true, + aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, + }, + }, + { + target: params.to, + options: { + action: "rename files", + requireWritable: true, + }, + }, + ], + args: [ + "rename", + params.pinnedFrom.mountRootPath, + params.pinnedFrom.relativeParentPath, + params.pinnedFrom.basename, + params.pinnedTo.mountRootPath, + params.pinnedTo.relativeParentPath, + params.pinnedTo.basename, + ], + }); +} + +export { SANDBOX_PINNED_FS_MUTATION_PYTHON }; diff --git a/src/agents/sandbox/fs-bridge-mutation-python-source.ts b/src/agents/sandbox/fs-bridge-mutation-python-source.ts new file mode 100644 index 00000000000..d0653e6ae41 --- /dev/null +++ b/src/agents/sandbox/fs-bridge-mutation-python-source.ts @@ -0,0 +1,190 @@ +// language=python +export const SANDBOX_PINNED_FS_MUTATION_PYTHON = String.raw`import os +import secrets +import subprocess +import sys + +operation = sys.argv[1] + +DIR_FLAGS = os.O_RDONLY +if hasattr(os, "O_DIRECTORY"): + DIR_FLAGS |= os.O_DIRECTORY +if hasattr(os, "O_NOFOLLOW"): + DIR_FLAGS |= os.O_NOFOLLOW + +WRITE_FLAGS = os.O_WRONLY | os.O_CREAT | os.O_EXCL +if hasattr(os, "O_NOFOLLOW"): + WRITE_FLAGS |= os.O_NOFOLLOW + + +def open_dir(path, dir_fd=None): + return os.open(path, DIR_FLAGS, dir_fd=dir_fd) + + +def walk_parent(root_fd, rel_parent, mkdir_enabled): + current_fd = os.dup(root_fd) + try: + segments = [segment for segment in rel_parent.split("/") if segment and segment != "."] + for segment in segments: + if segment == "..": + raise OSError("path traversal is not allowed") + try: + next_fd = open_dir(segment, dir_fd=current_fd) + except FileNotFoundError: + if not mkdir_enabled: + raise + os.mkdir(segment, 0o777, dir_fd=current_fd) + next_fd = open_dir(segment, dir_fd=current_fd) + os.close(current_fd) + current_fd = next_fd + return current_fd + except Exception: + os.close(current_fd) + raise + + +def create_temp_file(parent_fd, basename): + prefix = ".openclaw-write-" + basename + "." + for _ in range(128): + candidate = prefix + secrets.token_hex(6) + try: + fd = os.open(candidate, WRITE_FLAGS, 0o600, dir_fd=parent_fd) + return candidate, fd + except FileExistsError: + continue + raise RuntimeError("failed to allocate sandbox temp file") + + +def fd_path(fd, basename=None): + base = f"/proc/self/fd/{fd}" + if basename is None: + return base + return f"{base}/{basename}" + + +def run_command(argv, pass_fds): + subprocess.run(argv, check=True, pass_fds=tuple(pass_fds)) + + +def write_stdin_to_fd(fd): + while True: + chunk = sys.stdin.buffer.read(65536) + if not chunk: + break + os.write(fd, chunk) + + +def run_write(args): + mount_root, relative_parent, basename, mkdir_enabled_raw = args + mkdir_enabled = mkdir_enabled_raw == "1" + root_fd = open_dir(mount_root) + parent_fd = None + temp_fd = None + temp_name = None + try: + parent_fd = walk_parent(root_fd, relative_parent, mkdir_enabled) + temp_name, temp_fd = create_temp_file(parent_fd, basename) + write_stdin_to_fd(temp_fd) + os.fsync(temp_fd) + os.close(temp_fd) + temp_fd = None + os.replace(temp_name, basename, src_dir_fd=parent_fd, dst_dir_fd=parent_fd) + os.fsync(parent_fd) + except Exception: + if temp_fd is not None: + os.close(temp_fd) + temp_fd = None + if temp_name is not None and parent_fd is not None: + try: + os.unlink(temp_name, dir_fd=parent_fd) + except FileNotFoundError: + pass + raise + finally: + if parent_fd is not None: + os.close(parent_fd) + os.close(root_fd) + + +def run_mkdirp(args): + mount_root, relative_parent, basename = args + root_fd = open_dir(mount_root) + parent_fd = None + try: + parent_fd = walk_parent(root_fd, relative_parent, True) + run_command(["mkdir", "-p", "--", fd_path(parent_fd, basename)], [parent_fd]) + os.fsync(parent_fd) + finally: + if parent_fd is not None: + os.close(parent_fd) + os.close(root_fd) + + +def run_remove(args): + mount_root, relative_parent, basename, recursive_raw, force_raw = args + root_fd = open_dir(mount_root) + parent_fd = None + try: + parent_fd = walk_parent(root_fd, relative_parent, False) + argv = ["rm"] + if force_raw == "1": + argv.append("-f") + if recursive_raw == "1": + argv.append("-r") + argv.extend(["--", fd_path(parent_fd, basename)]) + run_command(argv, [parent_fd]) + os.fsync(parent_fd) + finally: + if parent_fd is not None: + os.close(parent_fd) + os.close(root_fd) + + +def run_rename(args): + ( + from_mount_root, + from_relative_parent, + from_basename, + to_mount_root, + to_relative_parent, + to_basename, + ) = args + from_root_fd = open_dir(from_mount_root) + to_root_fd = open_dir(to_mount_root) + from_parent_fd = None + to_parent_fd = None + try: + from_parent_fd = walk_parent(from_root_fd, from_relative_parent, False) + to_parent_fd = walk_parent(to_root_fd, to_relative_parent, True) + run_command( + [ + "mv", + "--", + fd_path(from_parent_fd, from_basename), + fd_path(to_parent_fd, to_basename), + ], + [from_parent_fd, to_parent_fd], + ) + os.fsync(from_parent_fd) + if to_parent_fd != from_parent_fd: + os.fsync(to_parent_fd) + finally: + if from_parent_fd is not None: + os.close(from_parent_fd) + if to_parent_fd is not None: + os.close(to_parent_fd) + os.close(from_root_fd) + os.close(to_root_fd) + + +OPERATIONS = { + "write": run_write, + "mkdirp": run_mkdirp, + "remove": run_remove, + "rename": run_rename, +} + +if operation not in OPERATIONS: + raise RuntimeError(f"unknown sandbox fs mutation: {operation}") + +OPERATIONS[operation](sys.argv[2:])`; diff --git a/src/agents/sandbox/fs-bridge-path-safety.ts b/src/agents/sandbox/fs-bridge-path-safety.ts index dabba7319b6..2c522cd04e6 100644 --- a/src/agents/sandbox/fs-bridge-path-safety.ts +++ b/src/agents/sandbox/fs-bridge-path-safety.ts @@ -23,7 +23,7 @@ export type AnchoredSandboxEntry = { basename: string; }; -export type PinnedSandboxWriteEntry = { +export type PinnedSandboxEntry = { mountRootPath: string; relativeParentPath: string; basename: string; @@ -135,29 +135,21 @@ export class SandboxFsPathGuard { } 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 splitTarget = this.splitSandboxEntryTarget(target); const canonicalParentPath = await this.resolveCanonicalContainerPath({ - containerPath: parentPath, + containerPath: splitTarget.parentPath, allowFinalSymlinkForUnlink: false, }); return { canonicalParentPath, - basename, + basename: splitTarget.basename, }; } - resolvePinnedWriteEntry(target: SandboxResolvedFsPath, action: string): PinnedSandboxWriteEntry { - 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 mount = this.resolveRequiredMount(parentPath, action); - const relativeParentPath = path.posix.relative(mount.containerRoot, parentPath); + resolvePinnedMutationEntry(target: SandboxResolvedFsPath, action: string): PinnedSandboxEntry { + const splitTarget = this.splitSandboxEntryTarget(target); + const mount = this.resolveRequiredMount(splitTarget.parentPath, action); + const relativeParentPath = path.posix.relative(mount.containerRoot, splitTarget.parentPath); if (relativeParentPath.startsWith("..") || path.posix.isAbsolute(relativeParentPath)) { throw new Error( `Sandbox path escapes allowed mounts; cannot ${action}: ${target.containerPath}`, @@ -166,6 +158,20 @@ export class SandboxFsPathGuard { return { mountRootPath: mount.containerRoot, relativeParentPath: relativeParentPath === "." ? "" : relativeParentPath, + basename: splitTarget.basename, + }; + } + + private splitSandboxEntryTarget(target: SandboxResolvedFsPath): { + basename: string; + parentPath: string; + } { + const basename = path.posix.basename(target.containerPath); + if (!basename || basename === "." || basename === "/") { + throw new Error(`Invalid sandbox entry target: ${target.containerPath}`); + } + return { + parentPath: normalizeContainerPath(path.posix.dirname(target.containerPath)), basename, }; } diff --git a/src/agents/sandbox/fs-bridge-shell-command-plans.ts b/src/agents/sandbox/fs-bridge-shell-command-plans.ts index e821f6cea7a..2987472762b 100644 --- a/src/agents/sandbox/fs-bridge-shell-command-plans.ts +++ b/src/agents/sandbox/fs-bridge-shell-command-plans.ts @@ -1,95 +1,15 @@ -import { PATH_ALIAS_POLICIES } from "../../infra/path-alias-guards.js"; -import type { AnchoredSandboxEntry, PathSafetyCheck } from "./fs-bridge-path-safety.js"; +import type { PathSafetyCheck } from "./fs-bridge-path-safety.js"; import type { SandboxResolvedFsPath } from "./fs-paths.js"; export type SandboxFsCommandPlan = { checks: PathSafetyCheck[]; script: string; args?: string[]; + stdin?: Buffer | string; recheckBeforeCommand?: boolean; allowFailure?: boolean; }; -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" } }], diff --git a/src/agents/sandbox/fs-bridge-write-helper.ts b/src/agents/sandbox/fs-bridge-write-helper.ts deleted file mode 100644 index a8a30388799..00000000000 --- a/src/agents/sandbox/fs-bridge-write-helper.ts +++ /dev/null @@ -1,109 +0,0 @@ -import type { PathSafetyCheck, PinnedSandboxWriteEntry } from "./fs-bridge-path-safety.js"; -import type { SandboxFsCommandPlan } from "./fs-bridge-shell-command-plans.js"; - -export const SANDBOX_PINNED_WRITE_PYTHON = [ - "import errno", - "import os", - "import secrets", - "import sys", - "", - "mount_root = sys.argv[1]", - "relative_parent = sys.argv[2]", - "basename = sys.argv[3]", - 'mkdir_enabled = sys.argv[4] == "1"', - "", - "DIR_FLAGS = os.O_RDONLY", - "if hasattr(os, 'O_DIRECTORY'):", - " DIR_FLAGS |= os.O_DIRECTORY", - "if hasattr(os, 'O_NOFOLLOW'):", - " DIR_FLAGS |= os.O_NOFOLLOW", - "", - "WRITE_FLAGS = os.O_WRONLY | os.O_CREAT | os.O_EXCL", - "if hasattr(os, 'O_NOFOLLOW'):", - " WRITE_FLAGS |= os.O_NOFOLLOW", - "", - "def open_dir(path, dir_fd=None):", - " return os.open(path, DIR_FLAGS, dir_fd=dir_fd)", - "", - "def walk_parent(root_fd, rel_parent, mkdir_enabled):", - " current_fd = os.dup(root_fd)", - " try:", - " segments = [segment for segment in rel_parent.split('/') if segment and segment != '.']", - " for segment in segments:", - " if segment == '..':", - " raise OSError(errno.EPERM, 'path traversal is not allowed', segment)", - " try:", - " next_fd = open_dir(segment, dir_fd=current_fd)", - " except FileNotFoundError:", - " if not mkdir_enabled:", - " raise", - " os.mkdir(segment, 0o777, dir_fd=current_fd)", - " next_fd = open_dir(segment, dir_fd=current_fd)", - " os.close(current_fd)", - " current_fd = next_fd", - " return current_fd", - " except Exception:", - " os.close(current_fd)", - " raise", - "", - "def create_temp_file(parent_fd, basename):", - " prefix = '.openclaw-write-' + basename + '.'", - " for _ in range(128):", - " candidate = prefix + secrets.token_hex(6)", - " try:", - " fd = os.open(candidate, WRITE_FLAGS, 0o600, dir_fd=parent_fd)", - " return candidate, fd", - " except FileExistsError:", - " continue", - " raise RuntimeError('failed to allocate sandbox temp file')", - "", - "root_fd = open_dir(mount_root)", - "parent_fd = None", - "temp_fd = None", - "temp_name = None", - "try:", - " parent_fd = walk_parent(root_fd, relative_parent, mkdir_enabled)", - " temp_name, temp_fd = create_temp_file(parent_fd, basename)", - " while True:", - " chunk = sys.stdin.buffer.read(65536)", - " if not chunk:", - " break", - " os.write(temp_fd, chunk)", - " os.fsync(temp_fd)", - " os.close(temp_fd)", - " temp_fd = None", - " os.replace(temp_name, basename, src_dir_fd=parent_fd, dst_dir_fd=parent_fd)", - " os.fsync(parent_fd)", - "except Exception:", - " if temp_fd is not None:", - " os.close(temp_fd)", - " temp_fd = None", - " if temp_name is not None and parent_fd is not None:", - " try:", - " os.unlink(temp_name, dir_fd=parent_fd)", - " except FileNotFoundError:", - " pass", - " raise", - "finally:", - " if parent_fd is not None:", - " os.close(parent_fd)", - " os.close(root_fd)", -].join("\n"); - -export function buildPinnedWritePlan(params: { - check: PathSafetyCheck; - pinned: PinnedSandboxWriteEntry; - mkdir: boolean; -}): SandboxFsCommandPlan & { stdin?: Buffer | string } { - return { - checks: [params.check], - recheckBeforeCommand: true, - script: ["set -eu", "python3 - \"$@\" <<'PY'", SANDBOX_PINNED_WRITE_PYTHON, "PY"].join("\n"), - args: [ - params.pinned.mountRootPath, - params.pinned.relativeParentPath, - params.pinned.basename, - params.mkdir ? "1" : "0", - ], - }; -} diff --git a/src/agents/sandbox/fs-bridge.anchored-ops.test.ts b/src/agents/sandbox/fs-bridge.anchored-ops.test.ts index 79bc5a55f3c..1082898fe6e 100644 --- a/src/agents/sandbox/fs-bridge.anchored-ops.test.ts +++ b/src/agents/sandbox/fs-bridge.anchored-ops.test.ts @@ -4,8 +4,7 @@ import { describe, expect, it } from "vitest"; import { createSandbox, createSandboxFsBridge, - findCallByScriptFragment, - findCallsByScriptFragment, + findCallByDockerArg, getDockerArg, installFsBridgeTestHarness, mockedExecDockerRaw, @@ -69,31 +68,28 @@ describe("sandbox fs bridge anchored ops", () => { const anchoredCases = [ { - name: "mkdirp anchors parent + basename", + name: "mkdirp pins mount root + relative 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", + op: "mkdirp", + expectedArgs: ["/workspace", "nested", "leaf"], + forbiddenArgs: ["/workspace/nested/leaf", "/workspace/nested"], }, { - name: "remove anchors parent + basename", + name: "remove pins mount root + relative 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", + op: "remove", + expectedArgs: ["/workspace", "nested", "file.txt", "0", "1"], + forbiddenArgs: ["/workspace/nested/file.txt", "/workspace/nested"], }, { - name: "rename anchors both parents + basenames", + name: "rename pins 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", + op: "rename", + expectedArgs: ["/workspace", "", "from.txt", "/workspace", "nested", "to.txt"], + forbiddenArgs: ["/workspace/from.txt", "/workspace/nested/to.txt", "/workspace/nested"], }, ] as const; @@ -102,19 +98,14 @@ describe("sandbox fs bridge anchored ops", () => { await testCase.invoke(bridge); - const opCall = findCallByScriptFragment(testCase.scriptFragment); + const opCall = findCallByDockerArg(1, testCase.op); expect(opCall).toBeDefined(); const args = opCall?.[0] ?? []; testCase.expectedArgs.forEach((value, index) => { - expect(getDockerArg(args, index + 1)).toBe(value); + expect(getDockerArg(args, index + 2)).toBe(value); }); testCase.forbiddenArgs.forEach((value) => { expect(args).not.toContain(value); }); - - const canonicalCalls = findCallsByScriptFragment('readlink -f -- "$cursor"'); - expect( - canonicalCalls.some(([callArgs]) => getDockerArg(callArgs, 1) === testCase.canonicalProbe), - ).toBe(true); }); }); diff --git a/src/agents/sandbox/fs-bridge.boundary.test.ts b/src/agents/sandbox/fs-bridge.boundary.test.ts index 3b86496fac6..574a698db4c 100644 --- a/src/agents/sandbox/fs-bridge.boundary.test.ts +++ b/src/agents/sandbox/fs-bridge.boundary.test.ts @@ -6,7 +6,7 @@ import { createSandbox, createSandboxFsBridge, expectMkdirpAllowsExistingDirectory, - getScriptsFromCalls, + findCallByDockerArg, installFsBridgeTestHarness, mockedExecDockerRaw, withTempDir, @@ -55,8 +55,7 @@ describe("sandbox fs bridge boundary validation", () => { 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); + expect(findCallByDockerArg(1, "mkdirp")).toBeUndefined(); }); }); @@ -111,7 +110,6 @@ describe("sandbox fs bridge boundary validation", () => { it("rejects missing files before any docker read command runs", async () => { const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); await expect(bridge.readFile({ filePath: "a.txt" })).rejects.toThrow(/ENOENT|no such file/i); - const scripts = getScriptsFromCalls(); - expect(scripts.some((script) => script.includes('cat -- "$1"'))).toBe(false); + expect(mockedExecDockerRaw).not.toHaveBeenCalled(); }); }); diff --git a/src/agents/sandbox/fs-bridge.e2e-docker.test.ts b/src/agents/sandbox/fs-bridge.e2e-docker.test.ts new file mode 100644 index 00000000000..62a064b49f5 --- /dev/null +++ b/src/agents/sandbox/fs-bridge.e2e-docker.test.ts @@ -0,0 +1,89 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import { DEFAULT_SANDBOX_IMAGE } from "./constants.js"; +import { buildSandboxCreateArgs, execDocker, execDockerRaw } from "./docker.js"; +import { createSandboxFsBridge } from "./fs-bridge.js"; +import { createSandboxTestContext } from "./test-fixtures.js"; +import { appendWorkspaceMountArgs } from "./workspace-mounts.js"; + +async function sandboxImageReady(): Promise { + try { + const dockerVersion = await execDockerRaw(["version"], { allowFailure: true }); + if (dockerVersion.code !== 0) { + return false; + } + const pythonCheck = await execDockerRaw( + ["run", "--rm", "--entrypoint", "python3", DEFAULT_SANDBOX_IMAGE, "--version"], + { allowFailure: true }, + ); + return pythonCheck.code === 0; + } catch { + return false; + } +} + +describe("sandbox fs bridge docker e2e", () => { + it.runIf(process.platform !== "win32")( + "writes through docker exec using the pinned mutation helper", + async () => { + if (!(await sandboxImageReady())) { + return; + } + + const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-fsbridge-e2e-")); + const workspaceDir = path.join(stateDir, "workspace"); + await fs.mkdir(workspaceDir, { recursive: true }); + + const suffix = `${process.pid}-${Date.now()}`; + const containerName = `openclaw-fsbridge-${suffix}`.slice(0, 63); + + try { + const sandbox = createSandboxTestContext({ + overrides: { + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerName, + containerWorkdir: "/workspace", + }, + dockerOverrides: { + image: DEFAULT_SANDBOX_IMAGE, + containerPrefix: "openclaw-fsbridge-", + user: "", + }, + }); + + const createArgs = buildSandboxCreateArgs({ + name: containerName, + cfg: sandbox.docker, + scopeKey: sandbox.sessionKey, + includeBinds: false, + bindSourceRoots: [workspaceDir], + }); + createArgs.push("--workdir", sandbox.containerWorkdir); + appendWorkspaceMountArgs({ + args: createArgs, + workspaceDir, + agentWorkspaceDir: workspaceDir, + workdir: sandbox.containerWorkdir, + workspaceAccess: sandbox.workspaceAccess, + }); + createArgs.push(sandbox.docker.image, "sleep", "infinity"); + + await execDocker(createArgs); + await execDocker(["start", containerName]); + + const bridge = createSandboxFsBridge({ sandbox }); + await bridge.writeFile({ filePath: "nested/hello.txt", data: "from-docker" }); + + await expect( + fs.readFile(path.join(workspaceDir, "nested", "hello.txt"), "utf8"), + ).resolves.toBe("from-docker"); + } finally { + await execDocker(["rm", "-f", containerName], { allowFailure: true }); + await fs.rm(stateDir, { recursive: true, force: true }); + } + }, + ); +}); diff --git a/src/agents/sandbox/fs-bridge.test-helpers.ts b/src/agents/sandbox/fs-bridge.test-helpers.ts index e81bb65a4e0..312c3fc295e 100644 --- a/src/agents/sandbox/fs-bridge.test-helpers.ts +++ b/src/agents/sandbox/fs-bridge.test-helpers.ts @@ -54,6 +54,10 @@ export function findCallsByScriptFragment(fragment: string) { ); } +export function findCallByDockerArg(position: number, value: string) { + return mockedExecDockerRaw.mock.calls.find(([args]) => getDockerArg(args, position) === value); +} + export function dockerExecResult(stdout: string) { return { stdout: Buffer.from(stdout), @@ -142,11 +146,13 @@ export async function expectMkdirpAllowsExistingDirectory(params?: { await expect(bridge.mkdirp({ filePath: "memory/kemik" })).resolves.toBeUndefined(); - const mkdirCall = findCallByScriptFragment('mkdir -p -- "$2"'); + const mkdirCall = findCallByDockerArg(1, "mkdirp"); expect(mkdirCall).toBeDefined(); - const mkdirParent = mkdirCall ? getDockerArg(mkdirCall[0], 1) : ""; - const mkdirBase = mkdirCall ? getDockerArg(mkdirCall[0], 2) : ""; - expect(mkdirParent).toBe("/workspace/memory"); + const mkdirRoot = mkdirCall ? getDockerArg(mkdirCall[0], 2) : ""; + const mkdirParent = mkdirCall ? getDockerArg(mkdirCall[0], 3) : ""; + const mkdirBase = mkdirCall ? getDockerArg(mkdirCall[0], 4) : ""; + expect(mkdirRoot).toBe("/workspace"); + expect(mkdirParent).toBe("memory"); expect(mkdirBase).toBe("kemik"); }); } diff --git a/src/agents/sandbox/fs-bridge.ts b/src/agents/sandbox/fs-bridge.ts index 67fedf3b515..f474cf710e7 100644 --- a/src/agents/sandbox/fs-bridge.ts +++ b/src/agents/sandbox/fs-bridge.ts @@ -1,14 +1,13 @@ import fs from "node:fs"; import { execDockerRaw, type ExecDockerRawResult } from "./docker.js"; -import { SandboxFsPathGuard } from "./fs-bridge-path-safety.js"; import { - buildMkdirpPlan, - buildRemovePlan, - buildRenamePlan, - buildStatPlan, - type SandboxFsCommandPlan, -} from "./fs-bridge-shell-command-plans.js"; -import { buildPinnedWritePlan } from "./fs-bridge-write-helper.js"; + buildPinnedMkdirpPlan, + buildPinnedRemovePlan, + buildPinnedRenamePlan, + buildPinnedWritePlan, +} from "./fs-bridge-mutation-helper.js"; +import { SandboxFsPathGuard } from "./fs-bridge-path-safety.js"; +import { buildStatPlan, type SandboxFsCommandPlan } from "./fs-bridge-shell-command-plans.js"; import { buildSandboxFsMounts, resolveSandboxFsPathWithMounts, @@ -111,31 +110,29 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { }): Promise { const target = this.resolveResolvedPath(params); this.ensureWriteAccess(target, "write files"); - const writeCheck = { - target, - options: { action: "write files", requireWritable: true } as const, - }; - await this.pathGuard.assertPathSafety(target, writeCheck.options); const buffer = Buffer.isBuffer(params.data) ? params.data : Buffer.from(params.data, params.encoding ?? "utf8"); - const pinnedWriteTarget = this.pathGuard.resolvePinnedWriteEntry(target, "write files"); - await this.runCheckedCommand({ - ...buildPinnedWritePlan({ - check: writeCheck, + const pinnedWriteTarget = this.pathGuard.resolvePinnedMutationEntry(target, "write files"); + await this.runCheckedCommand( + buildPinnedWritePlan({ + target, pinned: pinnedWriteTarget, mkdir: params.mkdir !== false, + stdin: buffer, }), - stdin: buffer, - signal: params.signal, - }); + params.signal, + ); } async mkdirp(params: { filePath: string; cwd?: string; signal?: AbortSignal }): Promise { const target = this.resolveResolvedPath(params); this.ensureWriteAccess(target, "create directories"); - const anchoredTarget = await this.pathGuard.resolveAnchoredSandboxEntry(target); - await this.runPlannedCommand(buildMkdirpPlan(target, anchoredTarget), params.signal); + const pinnedTarget = this.pathGuard.resolvePinnedMutationEntry(target, "create directories"); + await this.runCheckedCommand( + buildPinnedMkdirpPlan({ target, pinned: pinnedTarget }), + params.signal, + ); } async remove(params: { @@ -147,11 +144,11 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { }): Promise { const target = this.resolveResolvedPath(params); this.ensureWriteAccess(target, "remove files"); - const anchoredTarget = await this.pathGuard.resolveAnchoredSandboxEntry(target); - await this.runPlannedCommand( - buildRemovePlan({ + const pinnedTarget = this.pathGuard.resolvePinnedMutationEntry(target, "remove files"); + await this.runCheckedCommand( + buildPinnedRemovePlan({ target, - anchoredTarget, + pinned: pinnedTarget, recursive: params.recursive, force: params.force, }), @@ -169,14 +166,14 @@ 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.pathGuard.resolveAnchoredSandboxEntry(from); - const anchoredTo = await this.pathGuard.resolveAnchoredSandboxEntry(to); - await this.runPlannedCommand( - buildRenamePlan({ + const pinnedFrom = this.pathGuard.resolvePinnedMutationEntry(from, "rename files"); + const pinnedTo = this.pathGuard.resolvePinnedMutationEntry(to, "rename files"); + await this.runCheckedCommand( + buildPinnedRenamePlan({ from, to, - anchoredFrom, - anchoredTo, + pinnedFrom, + pinnedTo, }), params.signal, ); @@ -188,7 +185,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { signal?: AbortSignal; }): Promise { const target = this.resolveResolvedPath(params); - const result = await this.runPlannedCommand(buildStatPlan(target), params.signal); + const result = await this.runCheckedCommand(buildStatPlan(target), params.signal); if (result.code !== 0) { const stderr = result.stderr.toString("utf8"); if (stderr.includes("No such file or directory")) { @@ -241,7 +238,8 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { } private async runCheckedCommand( - plan: SandboxFsCommandPlan & { stdin?: Buffer | string; signal?: AbortSignal }, + plan: SandboxFsCommandPlan, + signal?: AbortSignal, ): Promise { await this.pathGuard.assertPathChecks(plan.checks); if (plan.recheckBeforeCommand) { @@ -251,17 +249,10 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { args: plan.args, stdin: plan.stdin, allowFailure: plan.allowFailure, - signal: plan.signal, + signal, }); } - private async runPlannedCommand( - plan: SandboxFsCommandPlan, - signal?: AbortSignal, - ): Promise { - return await this.runCheckedCommand({ ...plan, signal }); - } - 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}`);