diff --git a/src/agents/sandbox/fs-bridge-mutation-helper.test.ts b/src/agents/sandbox/fs-bridge-mutation-helper.test.ts index 7f8e6ee7694..f2d3974f0cc 100644 --- a/src/agents/sandbox/fs-bridge-mutation-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_FS_MUTATION_PYTHON } from "./fs-bridge-mutation-helper.js"; +import { SANDBOX_PINNED_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,33 +14,21 @@ async function withTempRoot(prefix: string, run: (root: string) => Promise } } -function runPinnedMutation(params: { - op: "write" | "mkdirp" | "remove" | "rename"; - args: string[]; - input?: string; -}) { - return spawnSync( - "python3", - ["-c", SANDBOX_PINNED_FS_MUTATION_PYTHON, params.op, ...params.args], - { - input: params.input, - encoding: "utf8", - stdio: ["pipe", "pipe", "pipe"], - }, - ); +function runMutation(args: string[], input?: string) { + return spawnSync("python3", ["-c", SANDBOX_PINNED_MUTATION_PYTHON, ...args], { + input, + encoding: "utf8", + stdio: ["pipe", "pipe", "pipe"], + }); } describe("sandbox pinned mutation helper", () => { - it("creates missing parents and writes through a pinned directory fd", async () => { - await withTempRoot("openclaw-write-helper-", async (root) => { + it("writes through a pinned directory fd", async () => { + await withTempRoot("openclaw-mutation-helper-", async (root) => { const workspace = path.join(root, "workspace"); await fs.mkdir(workspace, { recursive: true }); - const result = runPinnedMutation({ - op: "write", - args: [workspace, "nested/deeper", "note.txt", "1"], - input: "hello", - }); + const result = runMutation(["write", workspace, "nested/deeper", "note.txt", "1"], "hello"); expect(result.status).toBe(0); await expect( @@ -52,22 +40,104 @@ describe("sandbox pinned mutation helper", () => { it.runIf(process.platform !== "win32")( "rejects symlink-parent writes instead of materializing a temp file outside the mount", async () => { - await withTempRoot("openclaw-write-helper-", async (root) => { + await withTempRoot("openclaw-mutation-helper-", async (root) => { const workspace = path.join(root, "workspace"); const outside = path.join(root, "outside"); await fs.mkdir(workspace, { recursive: true }); await fs.mkdir(outside, { recursive: true }); await fs.symlink(outside, path.join(workspace, "alias")); - const result = runPinnedMutation({ - op: "write", - args: [workspace, "alias", "escape.txt", "0"], - input: "owned", - }); + const result = runMutation(["write", workspace, "alias", "escape.txt", "0"], "owned"); expect(result.status).not.toBe(0); await expect(fs.readFile(path.join(outside, "escape.txt"), "utf8")).rejects.toThrow(); }); }, ); + + it.runIf(process.platform !== "win32")("rejects symlink segments during mkdirp", async () => { + await withTempRoot("openclaw-mutation-helper-", async (root) => { + const workspace = path.join(root, "workspace"); + const outside = path.join(root, "outside"); + await fs.mkdir(workspace, { recursive: true }); + await fs.mkdir(outside, { recursive: true }); + await fs.symlink(outside, path.join(workspace, "alias")); + + const result = runMutation(["mkdirp", workspace, "alias/nested"]); + + expect(result.status).not.toBe(0); + await expect(fs.readFile(path.join(outside, "nested"), "utf8")).rejects.toThrow(); + }); + }); + + it.runIf(process.platform !== "win32")("remove unlinks the symlink itself", async () => { + await withTempRoot("openclaw-mutation-helper-", async (root) => { + const workspace = path.join(root, "workspace"); + const outside = path.join(root, "outside"); + await fs.mkdir(workspace, { recursive: true }); + await fs.mkdir(outside, { recursive: true }); + await fs.writeFile(path.join(outside, "secret.txt"), "classified", "utf8"); + await fs.symlink(path.join(outside, "secret.txt"), path.join(workspace, "link.txt")); + + const result = runMutation(["remove", workspace, "", "link.txt", "0", "0"]); + + expect(result.status).toBe(0); + await expect(fs.readlink(path.join(workspace, "link.txt"))).rejects.toThrow(); + await expect(fs.readFile(path.join(outside, "secret.txt"), "utf8")).resolves.toBe( + "classified", + ); + }); + }); + + it.runIf(process.platform !== "win32")( + "rejects symlink destination parents during rename", + async () => { + await withTempRoot("openclaw-mutation-helper-", async (root) => { + const workspace = path.join(root, "workspace"); + const outside = path.join(root, "outside"); + await fs.mkdir(workspace, { recursive: true }); + await fs.mkdir(outside, { recursive: true }); + await fs.writeFile(path.join(workspace, "from.txt"), "payload", "utf8"); + await fs.symlink(outside, path.join(workspace, "alias")); + + const result = runMutation([ + "rename", + workspace, + "", + "from.txt", + workspace, + "alias", + "escape.txt", + "1", + ]); + + expect(result.status).not.toBe(0); + await expect(fs.readFile(path.join(workspace, "from.txt"), "utf8")).resolves.toBe( + "payload", + ); + await expect(fs.readFile(path.join(outside, "escape.txt"), "utf8")).rejects.toThrow(); + }); + }, + ); + + it.runIf(process.platform !== "win32")( + "copies directories across different mount roots during rename fallback", + async () => { + await withTempRoot("openclaw-mutation-helper-", async (root) => { + const sourceRoot = path.join(root, "source"); + const destRoot = path.join(root, "dest"); + await fs.mkdir(path.join(sourceRoot, "dir", "nested"), { recursive: true }); + await fs.mkdir(destRoot, { recursive: true }); + await fs.writeFile(path.join(sourceRoot, "dir", "nested", "file.txt"), "payload", "utf8"); + + const result = runMutation(["rename", sourceRoot, "", "dir", destRoot, "", "moved", "1"]); + + expect(result.status).toBe(0); + await expect( + fs.readFile(path.join(destRoot, "moved", "nested", "file.txt"), "utf8"), + ).resolves.toBe("payload"); + await expect(fs.stat(path.join(sourceRoot, "dir"))).rejects.toThrow(); + }); + }, + ); }); diff --git a/src/agents/sandbox/fs-bridge-mutation-helper.ts b/src/agents/sandbox/fs-bridge-mutation-helper.ts index af03ec20338..fc50c5ab756 100644 --- a/src/agents/sandbox/fs-bridge-mutation-helper.ts +++ b/src/agents/sandbox/fs-bridge-mutation-helper.ts @@ -1,38 +1,274 @@ 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 { + PathSafetyCheck, + PinnedSandboxDirectoryEntry, + PinnedSandboxEntry, +} from "./fs-bridge-path-safety.js"; import type { SandboxFsCommandPlan } from "./fs-bridge-shell-command-plans.js"; -import type { SandboxResolvedFsPath } from "./fs-paths.js"; + +export const SANDBOX_PINNED_MUTATION_PYTHON = [ + "import errno", + "import os", + "import secrets", + "import stat", + "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", + "", + "READ_FLAGS = os.O_RDONLY", + "if hasattr(os, 'O_NOFOLLOW'):", + " READ_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 split_relative(path_value):", + " segments = []", + " for segment in path_value.split('/'):", + " if not segment or segment == '.':", + " continue", + " if segment == '..':", + " raise OSError(errno.EPERM, 'path traversal is not allowed', segment)", + " segments.append(segment)", + " return segments", + "", + "def open_dir(path_value, dir_fd=None):", + " return os.open(path_value, DIR_FLAGS, dir_fd=dir_fd)", + "", + "def walk_dir(root_fd, rel_path, mkdir_enabled):", + " current_fd = os.dup(root_fd)", + " try:", + " for segment in split_relative(rel_path):", + " 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 create_temp_dir(parent_fd, basename, mode):", + " prefix = '.openclaw-move-' + basename + '.'", + " for _ in range(128):", + " candidate = prefix + secrets.token_hex(6)", + " try:", + " os.mkdir(candidate, mode, dir_fd=parent_fd)", + " return candidate", + " except FileExistsError:", + " continue", + " raise RuntimeError('failed to allocate sandbox temp directory')", + "", + "def write_atomic(parent_fd, basename, stdin_buffer):", + " temp_fd = None", + " temp_name = None", + " try:", + " temp_name, temp_fd = create_temp_file(parent_fd, basename)", + " while True:", + " chunk = 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)", + " temp_name = None", + " os.fsync(parent_fd)", + " finally:", + " if temp_fd is not None:", + " os.close(temp_fd)", + " if temp_name is not None:", + " try:", + " os.unlink(temp_name, dir_fd=parent_fd)", + " except FileNotFoundError:", + " pass", + "", + "def remove_tree(parent_fd, basename):", + " entry_stat = os.lstat(basename, dir_fd=parent_fd)", + " if not stat.S_ISDIR(entry_stat.st_mode) or stat.S_ISLNK(entry_stat.st_mode):", + " os.unlink(basename, dir_fd=parent_fd)", + " return", + " dir_fd = open_dir(basename, dir_fd=parent_fd)", + " try:", + " for child in os.listdir(dir_fd):", + " remove_tree(dir_fd, child)", + " finally:", + " os.close(dir_fd)", + " os.rmdir(basename, dir_fd=parent_fd)", + "", + "def move_entry(src_parent_fd, src_basename, dst_parent_fd, dst_basename):", + " try:", + " os.rename(src_basename, dst_basename, src_dir_fd=src_parent_fd, dst_dir_fd=dst_parent_fd)", + " os.fsync(dst_parent_fd)", + " os.fsync(src_parent_fd)", + " return", + " except OSError as err:", + " if err.errno != errno.EXDEV:", + " raise", + " src_stat = os.lstat(src_basename, dir_fd=src_parent_fd)", + " if stat.S_ISDIR(src_stat.st_mode) and not stat.S_ISLNK(src_stat.st_mode):", + " temp_dir_name = create_temp_dir(dst_parent_fd, dst_basename, stat.S_IMODE(src_stat.st_mode) or 0o755)", + " temp_dir_fd = open_dir(temp_dir_name, dir_fd=dst_parent_fd)", + " src_dir_fd = open_dir(src_basename, dir_fd=src_parent_fd)", + " try:", + " for child in os.listdir(src_dir_fd):", + " move_entry(src_dir_fd, child, temp_dir_fd, child)", + " finally:", + " os.close(src_dir_fd)", + " os.close(temp_dir_fd)", + " os.rename(temp_dir_name, dst_basename, src_dir_fd=dst_parent_fd, dst_dir_fd=dst_parent_fd)", + " os.rmdir(src_basename, dir_fd=src_parent_fd)", + " os.fsync(dst_parent_fd)", + " os.fsync(src_parent_fd)", + " return", + " if stat.S_ISLNK(src_stat.st_mode):", + " link_target = os.readlink(src_basename, dir_fd=src_parent_fd)", + " try:", + " os.unlink(dst_basename, dir_fd=dst_parent_fd)", + " except FileNotFoundError:", + " pass", + " os.symlink(link_target, dst_basename, dir_fd=dst_parent_fd)", + " os.unlink(src_basename, dir_fd=src_parent_fd)", + " os.fsync(dst_parent_fd)", + " os.fsync(src_parent_fd)", + " return", + " src_fd = os.open(src_basename, READ_FLAGS, dir_fd=src_parent_fd)", + " temp_fd = None", + " temp_name = None", + " try:", + " temp_name, temp_fd = create_temp_file(dst_parent_fd, dst_basename)", + " while True:", + " chunk = os.read(src_fd, 65536)", + " if not chunk:", + " break", + " os.write(temp_fd, chunk)", + " try:", + " os.fchmod(temp_fd, stat.S_IMODE(src_stat.st_mode))", + " except AttributeError:", + " pass", + " os.fsync(temp_fd)", + " os.close(temp_fd)", + " temp_fd = None", + " os.replace(temp_name, dst_basename, src_dir_fd=dst_parent_fd, dst_dir_fd=dst_parent_fd)", + " temp_name = None", + " os.unlink(src_basename, dir_fd=src_parent_fd)", + " os.fsync(dst_parent_fd)", + " os.fsync(src_parent_fd)", + " finally:", + " if temp_fd is not None:", + " os.close(temp_fd)", + " if temp_name is not None:", + " try:", + " os.unlink(temp_name, dir_fd=dst_parent_fd)", + " except FileNotFoundError:", + " pass", + " os.close(src_fd)", + "", + "if operation == 'write':", + " root_fd = open_dir(sys.argv[2])", + " parent_fd = None", + " try:", + " parent_fd = walk_dir(root_fd, sys.argv[3], sys.argv[5] == '1')", + " write_atomic(parent_fd, sys.argv[4], sys.stdin.buffer)", + " finally:", + " if parent_fd is not None:", + " os.close(parent_fd)", + " os.close(root_fd)", + "elif operation == 'mkdirp':", + " root_fd = open_dir(sys.argv[2])", + " target_fd = None", + " try:", + " target_fd = walk_dir(root_fd, sys.argv[3], True)", + " os.fsync(target_fd)", + " finally:", + " if target_fd is not None:", + " os.close(target_fd)", + " os.close(root_fd)", + "elif operation == 'remove':", + " root_fd = open_dir(sys.argv[2])", + " parent_fd = None", + " try:", + " parent_fd = walk_dir(root_fd, sys.argv[3], False)", + " try:", + " if sys.argv[5] == '1':", + " remove_tree(parent_fd, sys.argv[4])", + " else:", + " entry_stat = os.lstat(sys.argv[4], dir_fd=parent_fd)", + " if stat.S_ISDIR(entry_stat.st_mode) and not stat.S_ISLNK(entry_stat.st_mode):", + " os.rmdir(sys.argv[4], dir_fd=parent_fd)", + " else:", + " os.unlink(sys.argv[4], dir_fd=parent_fd)", + " os.fsync(parent_fd)", + " except FileNotFoundError:", + " if sys.argv[6] != '1':", + " raise", + " finally:", + " if parent_fd is not None:", + " os.close(parent_fd)", + " os.close(root_fd)", + "elif operation == 'rename':", + " src_root_fd = open_dir(sys.argv[2])", + " dst_root_fd = open_dir(sys.argv[5])", + " src_parent_fd = None", + " dst_parent_fd = None", + " try:", + " src_parent_fd = walk_dir(src_root_fd, sys.argv[3], False)", + " dst_parent_fd = walk_dir(dst_root_fd, sys.argv[6], sys.argv[8] == '1')", + " move_entry(src_parent_fd, sys.argv[4], dst_parent_fd, sys.argv[7])", + " finally:", + " if src_parent_fd is not None:", + " os.close(src_parent_fd)", + " if dst_parent_fd is not None:", + " os.close(dst_parent_fd)", + " os.close(src_root_fd)", + " os.close(dst_root_fd)", + "else:", + " raise RuntimeError('unknown sandbox mutation operation: ' + operation)", +].join("\n"); function buildPinnedMutationPlan(params: { - checks: SandboxFsCommandPlan["checks"]; args: string[]; - stdin?: Buffer | string; + checks: PathSafetyCheck[]; }): SandboxFsCommandPlan { return { checks: params.checks, recheckBeforeCommand: true, - script: ["set -eu", "python3 - \"$@\" <<'PY'", SANDBOX_PINNED_FS_MUTATION_PYTHON, "PY"].join( - "\n", - ), + script: ["set -eu", "python3 - \"$@\" <<'PY'", SANDBOX_PINNED_MUTATION_PYTHON, "PY"].join("\n"), args: params.args, - stdin: params.stdin, }; } export function buildPinnedWritePlan(params: { - target: SandboxResolvedFsPath; + check: PathSafetyCheck; pinned: PinnedSandboxEntry; mkdir: boolean; - stdin: Buffer | string; }): SandboxFsCommandPlan { return buildPinnedMutationPlan({ - checks: [ - { - target: params.target, - options: { action: "write files", requireWritable: true }, - }, - ], + checks: [params.check], args: [ "write", params.pinned.mountRootPath, @@ -40,36 +276,21 @@ export function buildPinnedWritePlan(params: { params.pinned.basename, params.mkdir ? "1" : "0", ], - stdin: params.stdin, }); } export function buildPinnedMkdirpPlan(params: { - target: SandboxResolvedFsPath; - pinned: PinnedSandboxEntry; + check: PathSafetyCheck; + pinned: PinnedSandboxDirectoryEntry; }): 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, - ], + checks: [params.check], + args: ["mkdirp", params.pinned.mountRootPath, params.pinned.relativePath], }); } export function buildPinnedRemovePlan(params: { - target: SandboxResolvedFsPath; + check: PathSafetyCheck; pinned: PinnedSandboxEntry; recursive?: boolean; force?: boolean; @@ -77,10 +298,9 @@ export function buildPinnedRemovePlan(params: { return buildPinnedMutationPlan({ checks: [ { - target: params.target, + target: params.check.target, options: { - action: "remove files", - requireWritable: true, + ...params.check.options, aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, }, }, @@ -97,39 +317,31 @@ export function buildPinnedRemovePlan(params: { } export function buildPinnedRenamePlan(params: { - from: SandboxResolvedFsPath; - to: SandboxResolvedFsPath; - pinnedFrom: PinnedSandboxEntry; - pinnedTo: PinnedSandboxEntry; + fromCheck: PathSafetyCheck; + toCheck: PathSafetyCheck; + from: PinnedSandboxEntry; + to: PinnedSandboxEntry; }): SandboxFsCommandPlan { return buildPinnedMutationPlan({ checks: [ { - target: params.from, + target: params.fromCheck.target, options: { - action: "rename files", - requireWritable: true, + ...params.fromCheck.options, aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, }, }, - { - target: params.to, - options: { - action: "rename files", - requireWritable: true, - }, - }, + params.toCheck, ], args: [ "rename", - params.pinnedFrom.mountRootPath, - params.pinnedFrom.relativeParentPath, - params.pinnedFrom.basename, - params.pinnedTo.mountRootPath, - params.pinnedTo.relativeParentPath, - params.pinnedTo.basename, + params.from.mountRootPath, + params.from.relativeParentPath, + params.from.basename, + params.to.mountRootPath, + params.to.relativeParentPath, + params.to.basename, + "1", ], }); } - -export { SANDBOX_PINNED_FS_MUTATION_PYTHON }; diff --git a/src/agents/sandbox/fs-bridge-path-safety.ts b/src/agents/sandbox/fs-bridge-path-safety.ts index 2c522cd04e6..dfc6c6692a1 100644 --- a/src/agents/sandbox/fs-bridge-path-safety.ts +++ b/src/agents/sandbox/fs-bridge-path-safety.ts @@ -18,17 +18,17 @@ export type PathSafetyCheck = { options: PathSafetyOptions; }; -export type AnchoredSandboxEntry = { - canonicalParentPath: string; - basename: string; -}; - export type PinnedSandboxEntry = { mountRootPath: string; relativeParentPath: string; basename: string; }; +export type PinnedSandboxDirectoryEntry = { + mountRootPath: string; + relativePath: string; +}; + type RunCommand = ( script: string, options?: { @@ -134,22 +134,14 @@ export class SandboxFsPathGuard { return guarded; } - async resolveAnchoredSandboxEntry(target: SandboxResolvedFsPath): Promise { - const splitTarget = this.splitSandboxEntryTarget(target); - const canonicalParentPath = await this.resolveCanonicalContainerPath({ - containerPath: splitTarget.parentPath, - allowFinalSymlinkForUnlink: false, - }); - return { - canonicalParentPath, - basename: splitTarget.basename, - }; - } - - 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); + resolvePinnedEntry(target: SandboxResolvedFsPath, action: string): PinnedSandboxEntry { + 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); if (relativeParentPath.startsWith("..") || path.posix.isAbsolute(relativeParentPath)) { throw new Error( `Sandbox path escapes allowed mounts; cannot ${action}: ${target.containerPath}`, @@ -158,21 +150,24 @@ export class SandboxFsPathGuard { return { mountRootPath: mount.containerRoot, relativeParentPath: relativeParentPath === "." ? "" : relativeParentPath, - basename: splitTarget.basename, + 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}`); + resolvePinnedDirectoryEntry( + target: SandboxResolvedFsPath, + action: string, + ): PinnedSandboxDirectoryEntry { + const mount = this.resolveRequiredMount(target.containerPath, action); + const relativePath = path.posix.relative(mount.containerRoot, target.containerPath); + if (relativePath.startsWith("..") || path.posix.isAbsolute(relativePath)) { + throw new Error( + `Sandbox path escapes allowed mounts; cannot ${action}: ${target.containerPath}`, + ); } return { - parentPath: normalizeContainerPath(path.posix.dirname(target.containerPath)), - basename, + mountRootPath: mount.containerRoot, + relativePath: relativePath === "." ? "" : relativePath, }; } diff --git a/src/agents/sandbox/fs-bridge.anchored-ops.test.ts b/src/agents/sandbox/fs-bridge.anchored-ops.test.ts index 1082898fe6e..9b15f02adf5 100644 --- a/src/agents/sandbox/fs-bridge.anchored-ops.test.ts +++ b/src/agents/sandbox/fs-bridge.anchored-ops.test.ts @@ -4,7 +4,6 @@ import { describe, expect, it } from "vitest"; import { createSandbox, createSandboxFsBridge, - findCallByDockerArg, getDockerArg, installFsBridgeTestHarness, mockedExecDockerRaw, @@ -66,46 +65,60 @@ describe("sandbox fs bridge anchored ops", () => { }); }); - const anchoredCases = [ + const pinnedCases = [ { - name: "mkdirp pins mount root + relative parent + basename", + name: "mkdirp pins mount root + relative path", invoke: (bridge: ReturnType) => bridge.mkdirp({ filePath: "nested/leaf" }), - op: "mkdirp", - expectedArgs: ["/workspace", "nested", "leaf"], - forbiddenArgs: ["/workspace/nested/leaf", "/workspace/nested"], + expectedArgs: ["mkdirp", "/workspace", "nested/leaf"], + forbiddenArgs: ["/workspace/nested/leaf"], }, { - name: "remove pins mount root + relative parent + basename", + name: "remove pins mount root + parent/basename", invoke: (bridge: ReturnType) => bridge.remove({ filePath: "nested/file.txt" }), - op: "remove", - expectedArgs: ["/workspace", "nested", "file.txt", "0", "1"], - forbiddenArgs: ["/workspace/nested/file.txt", "/workspace/nested"], + expectedArgs: ["remove", "/workspace", "nested", "file.txt", "0", "1"], + forbiddenArgs: ["/workspace/nested/file.txt"], }, { name: "rename pins both parents + basenames", invoke: (bridge: ReturnType) => bridge.rename({ from: "from.txt", to: "nested/to.txt" }), - op: "rename", - expectedArgs: ["/workspace", "", "from.txt", "/workspace", "nested", "to.txt"], - forbiddenArgs: ["/workspace/from.txt", "/workspace/nested/to.txt", "/workspace/nested"], + expectedArgs: ["rename", "/workspace", "", "from.txt", "/workspace", "nested", "to.txt", "1"], + forbiddenArgs: ["/workspace/from.txt", "/workspace/nested/to.txt"], }, ] as const; - it.each(anchoredCases)("$name", async (testCase) => { - const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); + it.each(pinnedCases)("$name", async (testCase) => { + await withTempDir("openclaw-fs-bridge-contract-write-", async (stateDir) => { + const workspaceDir = path.join(stateDir, "workspace"); + await fs.mkdir(path.join(workspaceDir, "nested"), { recursive: true }); + await fs.writeFile(path.join(workspaceDir, "from.txt"), "hello", "utf8"); + await fs.writeFile(path.join(workspaceDir, "nested", "file.txt"), "bye", "utf8"); - await testCase.invoke(bridge); + const bridge = createSandboxFsBridge({ + sandbox: createSandbox({ + workspaceDir, + agentWorkspaceDir: workspaceDir, + }), + }); - const opCall = findCallByDockerArg(1, testCase.op); - expect(opCall).toBeDefined(); - const args = opCall?.[0] ?? []; - testCase.expectedArgs.forEach((value, index) => { - expect(getDockerArg(args, index + 2)).toBe(value); - }); - testCase.forbiddenArgs.forEach((value) => { - expect(args).not.toContain(value); + await testCase.invoke(bridge); + + const opCall = mockedExecDockerRaw.mock.calls.find( + ([args]) => + typeof args[5] === "string" && + args[5].includes("python3 - \"$@\" <<'PY'") && + getDockerArg(args, 1) === testCase.expectedArgs[0], + ); + expect(opCall).toBeDefined(); + const args = opCall?.[0] ?? []; + testCase.expectedArgs.forEach((value, index) => { + expect(getDockerArg(args, index + 1)).toBe(value); + }); + testCase.forbiddenArgs.forEach((value) => { + expect(args).not.toContain(value); + }); }); }); }); diff --git a/src/agents/sandbox/fs-bridge.shell.test.ts b/src/agents/sandbox/fs-bridge.shell.test.ts index 5b27f210333..24b7d9faba4 100644 --- a/src/agents/sandbox/fs-bridge.shell.test.ts +++ b/src/agents/sandbox/fs-bridge.shell.test.ts @@ -45,10 +45,10 @@ describe("sandbox fs bridge shell compatibility", () => { }); }); - it("resolveCanonicalContainerPath script is valid POSIX sh (no do; token)", async () => { + it("path canonicalization recheck script is valid POSIX sh", async () => { const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); - await bridge.mkdirp({ filePath: "nested" }); + await bridge.writeFile({ filePath: "b.txt", data: "hello" }); const scripts = getScriptsFromCalls(); const canonicalScript = scripts.find((script) => script.includes("allow_final")); @@ -134,6 +134,32 @@ describe("sandbox fs bridge shell compatibility", () => { expect(scripts.some((script) => script.includes("os.replace("))).toBe(true); }); + it("routes mkdirp, remove, and rename through the pinned mutation helper", async () => { + await withTempDir("openclaw-fs-bridge-shell-write-", async (stateDir) => { + const workspaceDir = path.join(stateDir, "workspace"); + await fs.mkdir(path.join(workspaceDir, "nested"), { recursive: true }); + await fs.writeFile(path.join(workspaceDir, "a.txt"), "hello", "utf8"); + await fs.writeFile(path.join(workspaceDir, "nested", "file.txt"), "bye", "utf8"); + + const bridge = createSandboxFsBridge({ + sandbox: createSandbox({ + workspaceDir, + agentWorkspaceDir: workspaceDir, + }), + }); + + await bridge.mkdirp({ filePath: "nested" }); + await bridge.remove({ filePath: "nested/file.txt" }); + await bridge.rename({ from: "a.txt", to: "nested/b.txt" }); + + const scripts = getScriptsFromCalls(); + expect(scripts.filter((script) => script.includes("operation = sys.argv[1]")).length).toBe(3); + expect(scripts.some((script) => script.includes('mkdir -p -- "$2"'))).toBe(false); + expect(scripts.some((script) => script.includes('rm -f -- "$2"'))).toBe(false); + expect(scripts.some((script) => script.includes('mv -- "$3" "$2/$4"'))).toBe(false); + }); + }); + it("re-validates target before the pinned write helper runs", async () => { const { mockedOpenBoundaryFile } = await import("./fs-bridge.test-helpers.js"); mockedOpenBoundaryFile diff --git a/src/agents/sandbox/fs-bridge.test-helpers.ts b/src/agents/sandbox/fs-bridge.test-helpers.ts index 312c3fc295e..5c6fc565532 100644 --- a/src/agents/sandbox/fs-bridge.test-helpers.ts +++ b/src/agents/sandbox/fs-bridge.test-helpers.ts @@ -54,10 +54,6 @@ 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), @@ -146,14 +142,16 @@ export async function expectMkdirpAllowsExistingDirectory(params?: { await expect(bridge.mkdirp({ filePath: "memory/kemik" })).resolves.toBeUndefined(); - const mkdirCall = findCallByDockerArg(1, "mkdirp"); + const mkdirCall = mockedExecDockerRaw.mock.calls.find( + ([args]) => + getDockerScript(args).includes("operation = sys.argv[1]") && + getDockerArg(args, 1) === "mkdirp", + ); expect(mkdirCall).toBeDefined(); - 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"); + const mountRoot = mkdirCall ? getDockerArg(mkdirCall[0], 2) : ""; + const relativePath = mkdirCall ? getDockerArg(mkdirCall[0], 3) : ""; + expect(mountRoot).toBe("/workspace"); + expect(relativePath).toBe("memory/kemik"); }); } diff --git a/src/agents/sandbox/fs-bridge.ts b/src/agents/sandbox/fs-bridge.ts index f474cf710e7..83504d9b908 100644 --- a/src/agents/sandbox/fs-bridge.ts +++ b/src/agents/sandbox/fs-bridge.ts @@ -110,29 +110,44 @@ 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.resolvePinnedMutationEntry(target, "write files"); - await this.runCheckedCommand( - buildPinnedWritePlan({ - target, + const pinnedWriteTarget = this.pathGuard.resolvePinnedEntry(target, "write files"); + await this.runCheckedCommand({ + ...buildPinnedWritePlan({ + check: writeCheck, pinned: pinnedWriteTarget, mkdir: params.mkdir !== false, - stdin: buffer, }), - params.signal, - ); + stdin: buffer, + signal: params.signal, + }); } async mkdirp(params: { filePath: string; cwd?: string; signal?: AbortSignal }): Promise { const target = this.resolveResolvedPath(params); this.ensureWriteAccess(target, "create directories"); - const pinnedTarget = this.pathGuard.resolvePinnedMutationEntry(target, "create directories"); - await this.runCheckedCommand( - buildPinnedMkdirpPlan({ target, pinned: pinnedTarget }), - params.signal, - ); + const mkdirCheck = { + target, + options: { + action: "create directories", + requireWritable: true, + allowedType: "directory", + } as const, + }; + await this.runCheckedCommand({ + ...buildPinnedMkdirpPlan({ + check: mkdirCheck, + pinned: this.pathGuard.resolvePinnedDirectoryEntry(target, "create directories"), + }), + signal: params.signal, + }); } async remove(params: { @@ -144,16 +159,22 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { }): Promise { const target = this.resolveResolvedPath(params); this.ensureWriteAccess(target, "remove files"); - const pinnedTarget = this.pathGuard.resolvePinnedMutationEntry(target, "remove files"); - await this.runCheckedCommand( - buildPinnedRemovePlan({ - target, - pinned: pinnedTarget, + const removeCheck = { + target, + options: { + action: "remove files", + requireWritable: true, + } as const, + }; + await this.runCheckedCommand({ + ...buildPinnedRemovePlan({ + check: removeCheck, + pinned: this.pathGuard.resolvePinnedEntry(target, "remove files"), recursive: params.recursive, force: params.force, }), - params.signal, - ); + signal: params.signal, + }); } async rename(params: { @@ -166,17 +187,29 @@ 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 pinnedFrom = this.pathGuard.resolvePinnedMutationEntry(from, "rename files"); - const pinnedTo = this.pathGuard.resolvePinnedMutationEntry(to, "rename files"); - await this.runCheckedCommand( - buildPinnedRenamePlan({ - from, - to, - pinnedFrom, - pinnedTo, + const fromCheck = { + target: from, + options: { + action: "rename files", + requireWritable: true, + } as const, + }; + const toCheck = { + target: to, + options: { + action: "rename files", + requireWritable: true, + } as const, + }; + await this.runCheckedCommand({ + ...buildPinnedRenamePlan({ + fromCheck, + toCheck, + from: this.pathGuard.resolvePinnedEntry(from, "rename files"), + to: this.pathGuard.resolvePinnedEntry(to, "rename files"), }), - params.signal, - ); + signal: params.signal, + }); } async stat(params: { @@ -185,7 +218,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { signal?: AbortSignal; }): Promise { const target = this.resolveResolvedPath(params); - const result = await this.runCheckedCommand(buildStatPlan(target), params.signal); + 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")) { @@ -238,8 +271,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { } private async runCheckedCommand( - plan: SandboxFsCommandPlan, - signal?: AbortSignal, + plan: SandboxFsCommandPlan & { stdin?: Buffer | string; signal?: AbortSignal }, ): Promise { await this.pathGuard.assertPathChecks(plan.checks); if (plan.recheckBeforeCommand) { @@ -249,10 +281,17 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { args: plan.args, stdin: plan.stdin, allowFailure: plan.allowFailure, - signal, + signal: plan.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}`); diff --git a/src/infra/fs-pinned-write-helper.test.ts b/src/infra/fs-pinned-write-helper.test.ts new file mode 100644 index 00000000000..c7f0fcd6069 --- /dev/null +++ b/src/infra/fs-pinned-write-helper.test.ts @@ -0,0 +1,86 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { createTrackedTempDirs } from "../test-utils/tracked-temp-dirs.js"; +import { runPinnedWriteHelper } from "./fs-pinned-write-helper.js"; + +const tempDirs = createTrackedTempDirs(); + +afterEach(async () => { + await tempDirs.cleanup(); +}); + +describe("fs pinned write helper", () => { + it.runIf(process.platform !== "win32")("writes through a pinned parent directory", async () => { + const root = await tempDirs.make("openclaw-fs-pinned-root-"); + + const identity = await runPinnedWriteHelper({ + rootPath: root, + relativeParentPath: "nested/deeper", + basename: "note.txt", + mkdir: true, + mode: 0o600, + input: { + kind: "buffer", + data: "hello", + }, + }); + + await expect( + fs.readFile(path.join(root, "nested", "deeper", "note.txt"), "utf8"), + ).resolves.toBe("hello"); + expect(identity.dev).toBeGreaterThanOrEqual(0); + expect(identity.ino).toBeGreaterThan(0); + }); + + it.runIf(process.platform !== "win32")( + "rejects symlink-parent writes instead of creating a temp file outside root", + async () => { + const root = await tempDirs.make("openclaw-fs-pinned-root-"); + const outside = await tempDirs.make("openclaw-fs-pinned-outside-"); + await fs.symlink(outside, path.join(root, "alias")); + + await expect( + runPinnedWriteHelper({ + rootPath: root, + relativeParentPath: "alias", + basename: "escape.txt", + mkdir: false, + mode: 0o600, + input: { + kind: "buffer", + data: "owned", + }, + }), + ).rejects.toThrow(); + + await expect(fs.stat(path.join(outside, "escape.txt"))).rejects.toThrow(); + const outsideFiles = await fs.readdir(outside); + expect(outsideFiles).toEqual([]); + }, + ); + + it.runIf(process.platform !== "win32")("accepts streamed input", async () => { + const root = await tempDirs.make("openclaw-fs-pinned-root-"); + const sourcePath = path.join(await tempDirs.make("openclaw-fs-pinned-src-"), "source.txt"); + await fs.writeFile(sourcePath, "streamed", "utf8"); + const sourceHandle = await fs.open(sourcePath, "r"); + try { + await runPinnedWriteHelper({ + rootPath: root, + relativeParentPath: "", + basename: "stream.txt", + mkdir: true, + mode: 0o600, + input: { + kind: "stream", + stream: sourceHandle.createReadStream(), + }, + }); + } finally { + await sourceHandle.close(); + } + + await expect(fs.readFile(path.join(root, "stream.txt"), "utf8")).resolves.toBe("streamed"); + }); +}); diff --git a/src/infra/fs-pinned-write-helper.ts b/src/infra/fs-pinned-write-helper.ts new file mode 100644 index 00000000000..b4bb5c56ed7 --- /dev/null +++ b/src/infra/fs-pinned-write-helper.ts @@ -0,0 +1,230 @@ +import { spawn } from "node:child_process"; +import { once } from "node:events"; +import fs from "node:fs/promises"; +import path from "node:path"; +import type { Readable } from "node:stream"; +import { pipeline } from "node:stream/promises"; +import type { FileIdentityStat } from "./file-identity.js"; + +export type PinnedWriteInput = + | { kind: "buffer"; data: string | Buffer; encoding?: BufferEncoding } + | { kind: "stream"; stream: Readable }; + +const LOCAL_PINNED_WRITE_PYTHON = [ + "import errno", + "import os", + "import secrets", + "import stat", + "import sys", + "", + "root_path = sys.argv[1]", + "relative_parent = sys.argv[2]", + "basename = sys.argv[3]", + 'mkdir_enabled = sys.argv[4] == "1"', + "file_mode = int(sys.argv[5], 8)", + "", + "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_value, dir_fd=None):", + " return os.open(path_value, DIR_FLAGS, dir_fd=dir_fd)", + "", + "def walk_parent(root_fd, rel_parent, mkdir_enabled):", + " current_fd = os.dup(root_fd)", + " try:", + " for segment in [part for part in rel_parent.split('/') if part and part != '.']:", + " 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, mode):", + " prefix = '.' + basename + '.'", + " for _ in range(128):", + " candidate = prefix + secrets.token_hex(6) + '.tmp'", + " try:", + " fd = os.open(candidate, WRITE_FLAGS, mode, dir_fd=parent_fd)", + " return candidate, fd", + " except FileExistsError:", + " continue", + " raise RuntimeError('failed to allocate pinned temp file')", + "", + "root_fd = open_dir(root_path)", + "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, file_mode)", + " 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)", + " temp_name = None", + " os.fsync(parent_fd)", + " result_stat = os.stat(basename, dir_fd=parent_fd, follow_symlinks=False)", + " print(f'{result_stat.st_dev}|{result_stat.st_ino}')", + "finally:", + " if temp_fd is not None:", + " os.close(temp_fd)", + " if temp_name is not None and parent_fd is not None:", + " try:", + " os.unlink(temp_name, dir_fd=parent_fd)", + " except FileNotFoundError:", + " pass", + " if parent_fd is not None:", + " os.close(parent_fd)", + " os.close(root_fd)", +].join("\n"); + +function parsePinnedIdentity(stdout: string): FileIdentityStat { + const line = stdout + .trim() + .split(/\r?\n/) + .map((value) => value.trim()) + .filter(Boolean) + .at(-1); + if (!line) { + throw new Error("Pinned write helper returned no identity"); + } + const [devRaw, inoRaw] = line.split("|"); + const dev = Number.parseInt(devRaw ?? "", 10); + const ino = Number.parseInt(inoRaw ?? "", 10); + if (!Number.isFinite(dev) || !Number.isFinite(ino)) { + throw new Error(`Pinned write helper returned invalid identity: ${line}`); + } + return { dev, ino }; +} + +export async function runPinnedWriteHelper(params: { + rootPath: string; + relativeParentPath: string; + basename: string; + mkdir: boolean; + mode: number; + input: PinnedWriteInput; +}): Promise { + const child = spawn( + "python3", + [ + "-c", + LOCAL_PINNED_WRITE_PYTHON, + params.rootPath, + params.relativeParentPath, + params.basename, + params.mkdir ? "1" : "0", + (params.mode || 0o600).toString(8), + ], + { + stdio: ["pipe", "pipe", "pipe"], + }, + ); + + let stdout = ""; + let stderr = ""; + child.stdout.setEncoding?.("utf8"); + child.stderr.setEncoding?.("utf8"); + child.stdout.on("data", (chunk: string) => { + stdout += chunk; + }); + child.stderr.on("data", (chunk: string) => { + stderr += chunk; + }); + + const exitPromise = once(child, "close") as Promise<[number | null, NodeJS.Signals | null]>; + try { + if (!child.stdin) { + const identity = await runPinnedWriteFallback(params); + await exitPromise.catch(() => {}); + return identity; + } + + if (params.input.kind === "buffer") { + const input = params.input; + await new Promise((resolve, reject) => { + child.stdin.once("error", reject); + if (typeof input.data === "string") { + child.stdin.end(input.data, input.encoding ?? "utf8", () => resolve()); + return; + } + child.stdin.end(input.data, () => resolve()); + }); + } else { + await pipeline(params.input.stream, child.stdin); + } + + const [code, signal] = await exitPromise; + if (code !== 0) { + throw new Error( + stderr.trim() || + `Pinned write helper failed with code ${code ?? "null"} (${signal ?? "?"})`, + ); + } + return parsePinnedIdentity(stdout); + } catch (error) { + child.kill("SIGKILL"); + await exitPromise.catch(() => {}); + throw error; + } +} + +async function runPinnedWriteFallback(params: { + rootPath: string; + relativeParentPath: string; + basename: string; + mkdir: boolean; + mode: number; + input: PinnedWriteInput; +}): Promise { + const parentPath = params.relativeParentPath + ? path.join(params.rootPath, ...params.relativeParentPath.split("/")) + : params.rootPath; + if (params.mkdir) { + await fs.mkdir(parentPath, { recursive: true }); + } + const targetPath = path.join(parentPath, params.basename); + const tempPath = path.join(parentPath, `.${params.basename}.fallback.tmp`); + if (params.input.kind === "buffer") { + if (typeof params.input.data === "string") { + await fs.writeFile(tempPath, params.input.data, { + encoding: params.input.encoding ?? "utf8", + mode: params.mode, + }); + } else { + await fs.writeFile(tempPath, params.input.data, { mode: params.mode }); + } + } else { + const handle = await fs.open(tempPath, "w", params.mode); + try { + await pipeline(params.input.stream, handle.createWriteStream()); + } finally { + await handle.close().catch(() => {}); + } + } + await fs.rename(tempPath, targetPath); + const stat = await fs.stat(targetPath); + return { dev: stat.dev, ino: stat.ino }; +} diff --git a/src/infra/fs-safe.test.ts b/src/infra/fs-safe.test.ts index ba4c13dfc7c..9227874cf8a 100644 --- a/src/infra/fs-safe.test.ts +++ b/src/infra/fs-safe.test.ts @@ -1,6 +1,6 @@ import fs from "node:fs/promises"; import path from "node:path"; -import { afterEach, describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it } from "vitest"; import { createRebindableDirectoryAlias, withRealpathSymlinkRebindRace, @@ -36,7 +36,9 @@ async function expectWriteOpenRaceIsBlocked(params: { symlinkTarget: params.outsideDir, timing: "before-realpath", run: async () => { - await expect(params.runWrite()).rejects.toMatchObject({ code: "outside-workspace" }); + await expect(params.runWrite()).rejects.toMatchObject({ + code: expect.stringMatching(/outside-workspace|invalid-path/), + }); }, }); } @@ -263,120 +265,6 @@ describe("fs-safe", () => { await expect(fs.readFile(targetPath, "utf8")).resolves.toBe("seed\nnext"); }); - it("does not truncate existing target when atomic rename fails", async () => { - const root = await tempDirs.make("openclaw-fs-safe-root-"); - const targetPath = path.join(root, "nested", "out.txt"); - await fs.mkdir(path.dirname(targetPath), { recursive: true }); - await fs.writeFile(targetPath, "existing-content"); - const renameSpy = vi - .spyOn(fs, "rename") - .mockRejectedValue(Object.assign(new Error("rename blocked"), { code: "EACCES" })); - try { - await expect( - writeFileWithinRoot({ - rootDir: root, - relativePath: "nested/out.txt", - data: "new-content", - }), - ).rejects.toMatchObject({ code: "EACCES" }); - } finally { - renameSpy.mockRestore(); - } - await expect(fs.readFile(targetPath, "utf8")).resolves.toBe("existing-content"); - }); - - it.runIf(process.platform !== "win32")( - "rejects when a hardlink appears after atomic write rename", - async () => { - const root = await tempDirs.make("openclaw-fs-safe-root-"); - const targetPath = path.join(root, "nested", "out.txt"); - const aliasPath = path.join(root, "nested", "alias.txt"); - await fs.mkdir(path.dirname(targetPath), { recursive: true }); - await fs.writeFile(targetPath, "existing-content"); - const realRename = fs.rename.bind(fs); - let linked = false; - const renameSpy = vi.spyOn(fs, "rename").mockImplementation(async (...args) => { - await realRename(...args); - if (!linked) { - linked = true; - await fs.link(String(args[1]), aliasPath); - } - }); - try { - await expect( - writeFileWithinRoot({ - rootDir: root, - relativePath: "nested/out.txt", - data: "new-content", - }), - ).rejects.toMatchObject({ code: "invalid-path" }); - } finally { - renameSpy.mockRestore(); - } - await expect(fs.readFile(aliasPath, "utf8")).resolves.toBe("new-content"); - }, - ); - - it("does not truncate existing target when atomic copy rename fails", async () => { - const root = await tempDirs.make("openclaw-fs-safe-root-"); - const sourceDir = await tempDirs.make("openclaw-fs-safe-source-"); - const sourcePath = path.join(sourceDir, "in.txt"); - const targetPath = path.join(root, "nested", "copied.txt"); - await fs.mkdir(path.dirname(targetPath), { recursive: true }); - await fs.writeFile(sourcePath, "copy-new"); - await fs.writeFile(targetPath, "copy-existing"); - const renameSpy = vi - .spyOn(fs, "rename") - .mockRejectedValue(Object.assign(new Error("rename blocked"), { code: "EACCES" })); - try { - await expect( - copyFileWithinRoot({ - sourcePath, - rootDir: root, - relativePath: "nested/copied.txt", - }), - ).rejects.toMatchObject({ code: "EACCES" }); - } finally { - renameSpy.mockRestore(); - } - await expect(fs.readFile(targetPath, "utf8")).resolves.toBe("copy-existing"); - }); - - it.runIf(process.platform !== "win32")( - "rejects when a hardlink appears after atomic copy rename", - async () => { - const root = await tempDirs.make("openclaw-fs-safe-root-"); - const sourceDir = await tempDirs.make("openclaw-fs-safe-source-"); - const sourcePath = path.join(sourceDir, "copy-source.txt"); - const targetPath = path.join(root, "nested", "copied.txt"); - const aliasPath = path.join(root, "nested", "alias.txt"); - await fs.mkdir(path.dirname(targetPath), { recursive: true }); - await fs.writeFile(sourcePath, "copy-new"); - await fs.writeFile(targetPath, "copy-existing"); - const realRename = fs.rename.bind(fs); - let linked = false; - const renameSpy = vi.spyOn(fs, "rename").mockImplementation(async (...args) => { - await realRename(...args); - if (!linked) { - linked = true; - await fs.link(String(args[1]), aliasPath); - } - }); - try { - await expect( - copyFileWithinRoot({ - sourcePath, - rootDir: root, - relativePath: "nested/copied.txt", - }), - ).rejects.toMatchObject({ code: "invalid-path" }); - } finally { - renameSpy.mockRestore(); - } - await expect(fs.readFile(aliasPath, "utf8")).resolves.toBe("copy-new"); - }, - ); - it("copies a file within root safely", async () => { const root = await tempDirs.make("openclaw-fs-safe-root-"); const sourceDir = await tempDirs.make("openclaw-fs-safe-source-"); @@ -537,47 +425,6 @@ describe("fs-safe", () => { await expect(fs.readFile(outsideTarget, "utf8")).resolves.toBe("X".repeat(4096)); }); - it("cleans up created out-of-root file when symlink retarget races create path", async () => { - const root = await tempDirs.make("openclaw-fs-safe-root-"); - const inside = path.join(root, "inside"); - const outside = await tempDirs.make("openclaw-fs-safe-outside-"); - await fs.mkdir(inside, { recursive: true }); - const outsideTarget = path.join(outside, "target.txt"); - const slot = path.join(root, "slot"); - await createRebindableDirectoryAlias({ - aliasPath: slot, - targetPath: inside, - }); - - const realOpen = fs.open.bind(fs); - let flipped = false; - const openSpy = vi.spyOn(fs, "open").mockImplementation(async (...args) => { - const [filePath] = args; - if (!flipped && String(filePath).endsWith(path.join("slot", "target.txt"))) { - flipped = true; - await createRebindableDirectoryAlias({ - aliasPath: slot, - targetPath: outside, - }); - } - return await realOpen(...args); - }); - try { - await expect( - writeFileWithinRoot({ - rootDir: root, - relativePath: path.join("slot", "target.txt"), - data: "new-content", - mkdir: false, - }), - ).rejects.toMatchObject({ code: "outside-workspace" }); - } finally { - openSpy.mockRestore(); - } - - await expect(fs.stat(outsideTarget)).rejects.toMatchObject({ code: "ENOENT" }); - }); - it("returns not-found for missing files", async () => { const dir = await tempDirs.make("openclaw-fs-safe-"); const missing = path.join(dir, "missing.txt"); diff --git a/src/infra/fs-safe.ts b/src/infra/fs-safe.ts index 77754437528..6bf0411a631 100644 --- a/src/infra/fs-safe.ts +++ b/src/infra/fs-safe.ts @@ -5,9 +5,9 @@ import type { FileHandle } from "node:fs/promises"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import { pipeline } from "node:stream/promises"; import { logWarn } from "../logger.js"; import { sameFileIdentity } from "./file-identity.js"; +import { runPinnedWriteHelper } from "./fs-pinned-write-helper.js"; import { expandHomePrefix } from "./home-dir.js"; import { assertNoPathAliasEscape } from "./path-alias-guards.js"; import { @@ -332,13 +332,13 @@ async function writeTempFileForAtomicReplace(params: { async function verifyAtomicWriteResult(params: { rootDir: string; targetPath: string; - expectedStat: Stats; + expectedIdentity: { dev: number | bigint; ino: number | bigint }; }): Promise { const rootReal = await fs.realpath(params.rootDir); const rootWithSep = ensureTrailingSep(rootReal); const opened = await openVerifiedLocalFile(params.targetPath, { rejectHardlinks: true }); try { - if (!sameFileIdentity(opened.stat, params.expectedStat)) { + if (!sameFileIdentity(opened.stat, params.expectedIdentity)) { throw new SafeOpenError("path-mismatch", "path changed during write"); } if (!isPathInside(rootWithSep, opened.realPath)) { @@ -550,6 +550,195 @@ export async function writeFileWithinRoot(params: { data: string | Buffer; encoding?: BufferEncoding; mkdir?: boolean; +}): Promise { + if (process.platform === "win32") { + await writeFileWithinRootLegacy(params); + return; + } + + const pinned = await resolvePinnedWriteTargetWithinRoot({ + rootDir: params.rootDir, + relativePath: params.relativePath, + }); + + const identity = await runPinnedWriteHelper({ + rootPath: pinned.rootReal, + relativeParentPath: pinned.relativeParentPath, + basename: pinned.basename, + mkdir: params.mkdir !== false, + mode: pinned.mode, + input: { + kind: "buffer", + data: params.data, + encoding: params.encoding, + }, + }).catch((error) => { + throw normalizePinnedWriteError(error); + }); + + try { + await verifyAtomicWriteResult({ + rootDir: params.rootDir, + targetPath: pinned.targetPath, + expectedIdentity: identity, + }); + } catch (err) { + emitWriteBoundaryWarning(`post-write verification failed: ${String(err)}`); + throw err; + } +} + +export async function copyFileWithinRoot(params: { + sourcePath: string; + rootDir: string; + relativePath: string; + maxBytes?: number; + mkdir?: boolean; + rejectSourceHardlinks?: boolean; +}): Promise { + const source = await openVerifiedLocalFile(params.sourcePath, { + rejectHardlinks: params.rejectSourceHardlinks, + }); + if (params.maxBytes !== undefined && source.stat.size > params.maxBytes) { + await source.handle.close().catch(() => {}); + throw new SafeOpenError( + "too-large", + `file exceeds limit of ${params.maxBytes} bytes (got ${source.stat.size})`, + ); + } + + try { + if (process.platform === "win32") { + await copyFileWithinRootLegacy(params, source); + return; + } + + const pinned = await resolvePinnedWriteTargetWithinRoot({ + rootDir: params.rootDir, + relativePath: params.relativePath, + }); + const sourceStream = source.handle.createReadStream(); + const identity = await runPinnedWriteHelper({ + rootPath: pinned.rootReal, + relativeParentPath: pinned.relativeParentPath, + basename: pinned.basename, + mkdir: params.mkdir !== false, + mode: pinned.mode, + input: { + kind: "stream", + stream: sourceStream, + }, + }).catch((error) => { + throw normalizePinnedWriteError(error); + }); + try { + await verifyAtomicWriteResult({ + rootDir: params.rootDir, + targetPath: pinned.targetPath, + expectedIdentity: identity, + }); + } catch (err) { + emitWriteBoundaryWarning(`post-copy verification failed: ${String(err)}`); + throw err; + } + } finally { + await source.handle.close().catch(() => {}); + } +} + +export async function writeFileFromPathWithinRoot(params: { + rootDir: string; + relativePath: string; + sourcePath: string; + mkdir?: boolean; +}): Promise { + await copyFileWithinRoot({ + sourcePath: params.sourcePath, + rootDir: params.rootDir, + relativePath: params.relativePath, + mkdir: params.mkdir, + rejectSourceHardlinks: true, + }); +} + +async function resolvePinnedWriteTargetWithinRoot(params: { + rootDir: string; + relativePath: string; +}): Promise<{ + rootReal: string; + targetPath: string; + relativeParentPath: string; + basename: string; + mode: number; +}> { + const { rootReal, rootWithSep, resolved } = await resolvePathWithinRoot(params); + try { + await assertNoPathAliasEscape({ + absolutePath: resolved, + rootPath: rootReal, + boundaryLabel: "root", + }); + } catch (err) { + throw new SafeOpenError("invalid-path", "path alias escape blocked", { cause: err }); + } + + const relativeResolved = path.relative(rootReal, resolved); + if (relativeResolved.startsWith("..") || path.isAbsolute(relativeResolved)) { + throw new SafeOpenError("outside-workspace", "file is outside workspace root"); + } + const relativePosix = relativeResolved + ? relativeResolved.split(path.sep).join(path.posix.sep) + : ""; + const basename = path.posix.basename(relativePosix); + if (!basename || basename === "." || basename === "/") { + throw new SafeOpenError("invalid-path", "invalid target path"); + } + let mode = 0o600; + try { + const opened = await openFileWithinRoot({ + rootDir: params.rootDir, + relativePath: params.relativePath, + rejectHardlinks: true, + }); + try { + mode = opened.stat.mode & 0o777; + if (!isPathInside(rootWithSep, opened.realPath)) { + throw new SafeOpenError("outside-workspace", "file is outside workspace root"); + } + } finally { + await opened.handle.close().catch(() => {}); + } + } catch (err) { + if (!(err instanceof SafeOpenError) || err.code !== "not-found") { + throw err; + } + } + + return { + rootReal, + targetPath: resolved, + relativeParentPath: + path.posix.dirname(relativePosix) === "." ? "" : path.posix.dirname(relativePosix), + basename, + mode: mode || 0o600, + }; +} + +function normalizePinnedWriteError(error: unknown): Error { + if (error instanceof SafeOpenError) { + return error; + } + return new SafeOpenError("invalid-path", "path is not a regular file under root", { + cause: error instanceof Error ? error : undefined, + }); +} + +async function writeFileWithinRootLegacy(params: { + rootDir: string; + relativePath: string; + data: string | Buffer; + encoding?: BufferEncoding; + mkdir?: boolean; }): Promise { const target = await openWritableFileWithinRoot({ rootDir: params.rootDir, @@ -575,7 +764,7 @@ export async function writeFileWithinRoot(params: { await verifyAtomicWriteResult({ rootDir: params.rootDir, targetPath: destinationPath, - expectedStat: writtenStat, + expectedIdentity: writtenStat, }); } catch (err) { emitWriteBoundaryWarning(`post-write verification failed: ${String(err)}`); @@ -588,25 +777,17 @@ export async function writeFileWithinRoot(params: { } } -export async function copyFileWithinRoot(params: { - sourcePath: string; - rootDir: string; - relativePath: string; - maxBytes?: number; - mkdir?: boolean; - rejectSourceHardlinks?: boolean; -}): Promise { - const source = await openVerifiedLocalFile(params.sourcePath, { - rejectHardlinks: params.rejectSourceHardlinks, - }); - if (params.maxBytes !== undefined && source.stat.size > params.maxBytes) { - await source.handle.close().catch(() => {}); - throw new SafeOpenError( - "too-large", - `file exceeds limit of ${params.maxBytes} bytes (got ${source.stat.size})`, - ); - } - +async function copyFileWithinRootLegacy( + params: { + sourcePath: string; + rootDir: string; + relativePath: string; + maxBytes?: number; + mkdir?: boolean; + rejectSourceHardlinks?: boolean; + }, + source: SafeOpenResult, +): Promise { let target: SafeWritableOpenResult | null = null; let sourceClosedByStream = false; let targetClosedByUs = false; @@ -635,7 +816,9 @@ export async function copyFileWithinRoot(params: { targetStream.once("close", () => { tempClosedByStream = true; }); - await pipeline(sourceStream, targetStream); + await import("node:stream/promises").then(({ pipeline }) => + pipeline(sourceStream, targetStream), + ); const writtenStat = await fs.stat(tempPath); if (!tempClosedByStream) { await tempHandle.close().catch(() => {}); @@ -648,7 +831,7 @@ export async function copyFileWithinRoot(params: { await verifyAtomicWriteResult({ rootDir: params.rootDir, targetPath: destinationPath, - expectedStat: writtenStat, + expectedIdentity: writtenStat, }); } catch (err) { emitWriteBoundaryWarning(`post-copy verification failed: ${String(err)}`); @@ -674,18 +857,3 @@ export async function copyFileWithinRoot(params: { } } } - -export async function writeFileFromPathWithinRoot(params: { - rootDir: string; - relativePath: string; - sourcePath: string; - mkdir?: boolean; -}): Promise { - await copyFileWithinRoot({ - sourcePath: params.sourcePath, - rootDir: params.rootDir, - relativePath: params.relativePath, - mkdir: params.mkdir, - rejectSourceHardlinks: true, - }); -}