From c6f5725906dcaaaeeb0b5942addc844ffc579caf Mon Sep 17 00:00:00 2001 From: joshavant <830519+joshavant@users.noreply.github.com> Date: Wed, 24 Jun 2026 17:03:15 -0500 Subject: [PATCH] fix(openshell): pin local mirror fs mutations --- extensions/openshell/src/fs-bridge.ts | 190 ++++++++++++- .../openshell/src/openshell-core.test.ts | 265 ++++++++++++++++++ 2 files changed, 446 insertions(+), 9 deletions(-) diff --git a/extensions/openshell/src/fs-bridge.ts b/extensions/openshell/src/fs-bridge.ts index 5fbfa3d0889..bb00af6912c 100644 --- a/extensions/openshell/src/fs-bridge.ts +++ b/extensions/openshell/src/fs-bridge.ts @@ -8,9 +8,8 @@ import type { SandboxResolvedPath, } from "openclaw/plugin-sdk/sandbox"; import { createWritableRenameTargetResolver } from "openclaw/plugin-sdk/sandbox"; -import { isPathInside } from "openclaw/plugin-sdk/security-runtime"; +import { FsSafeError, isPathInside } from "openclaw/plugin-sdk/security-runtime"; import type { OpenShellFsBridgeContext, OpenShellSandboxBackend } from "./backend.types.js"; -import { movePathWithCopyFallback } from "./mirror.js"; type ResolvedMountPath = SandboxResolvedPath & { mountHostRoot: string; @@ -18,6 +17,9 @@ type ResolvedMountPath = SandboxResolvedPath & { source: "workspace" | "agent" | "protectedSkill"; }; +type FsSafeRoot = Awaited>; +type FsSafeStat = Awaited>; + const MATERIALIZED_SKILLS_CONTAINER_PARTS = [".openclaw", "sandbox-skills", "skills"] as const; export function createOpenShellFsBridge(params: { @@ -117,7 +119,7 @@ class OpenShellFsBridge implements SandboxFsBridge { allowFinalSymlinkForUnlink: false, }); await this.backend.mkdirpRemotePath(target.containerPath, params.signal); - await fsPromises.mkdir(hostPath, { recursive: true }); + await mkdirLocalRootPath({ hostPath, target }); } async remove(params: { @@ -141,9 +143,11 @@ class OpenShellFsBridge implements SandboxFsBridge { signal: params.signal, ignoreMissing: params.force !== false, }); - await fsPromises.rm(hostPath, { - recursive: params.recursive ?? false, - force: params.force !== false, + await removeLocalRootPath({ + force: params.force, + hostPath, + recursive: params.recursive, + target, }); } @@ -168,9 +172,17 @@ class OpenShellFsBridge implements SandboxFsBridge { allowMissingLeaf: true, allowFinalSymlinkForUnlink: false, }); + await assertRenameSourceSupported(fromHostPath); + if (from.mountHostRoot !== to.mountHostRoot) { + throw new Error("OpenShell cross-root mirror renames require pinned fs-safe support"); + } + await assertSameDeviceRenameSupported({ + fromHostPath, + root: from.mountHostRoot, + toHostPath, + }); await this.backend.renameRemotePath(from.containerPath, to.containerPath, params.signal); - await fsPromises.mkdir(path.dirname(toHostPath), { recursive: true }); - await movePathWithCopyFallback({ from: fromHostPath, to: toHostPath }); + await moveLocalRootPath({ from, fromHostPath, to, toHostPath }); } async stat(params: { @@ -343,6 +355,162 @@ class OpenShellFsBridge implements SandboxFsBridge { } } +async function mkdirLocalRootPath(params: { + target: ResolvedMountPath; + hostPath: string; +}): Promise { + const relativePath = relativeToRoot(params.target, params.hostPath); + if (!relativePath) { + return; + } + const root = await fsRoot(params.target.mountHostRoot); + await root.mkdir(relativePath); +} + +async function removeLocalRootPath(params: { + target: ResolvedMountPath; + hostPath: string; + recursive?: boolean; + force?: boolean; +}): Promise { + const root = await fsRoot(params.target.mountHostRoot); + const relativePath = relativeToRoot(params.target, params.hostPath); + try { + if (params.force === false) { + await fsPromises.lstat(params.hostPath); + } + if (params.recursive) { + const stats = await fsPromises.lstat(params.hostPath).catch((err: unknown) => { + if (isNotFoundError(err)) { + return null; + } + throw err; + }); + if (stats?.isSymbolicLink()) { + await root.remove(relativePath); + return; + } + await removeRootTree(root, relativePath); + return; + } + await root.remove(relativePath); + } catch (err) { + if (params.force !== false && isNotFoundError(err)) { + return; + } + throw err; + } +} + +async function removeRootTree( + root: FsSafeRoot, + relativePath: string, + knownStats?: FsSafeStat, +): Promise { + const stats = knownStats ?? (await root.stat(relativePath)); + if (stats.isDirectory && !stats.isSymbolicLink) { + const entries = await root.list(relativePath, { withFileTypes: true }); + for (const entry of entries) { + await removeRootTree(root, path.join(relativePath, entry.name), entry); + } + if (!relativePath) { + return; + } + } + await root.remove(relativePath); +} + +async function moveLocalRootPath(params: { + from: ResolvedMountPath; + fromHostPath: string; + to: ResolvedMountPath; + toHostPath: string; +}): Promise { + const root = await fsRoot(params.from.mountHostRoot); + const fromRelativePath = relativeToRoot(params.from, params.fromHostPath); + const toRelativePath = relativeToRoot(params.to, params.toHostPath); + await mkdirParentPath(root, toRelativePath); + await root.move(fromRelativePath, toRelativePath, { overwrite: true }); +} + +async function mkdirParentPath(root: FsSafeRoot, relativePath: string): Promise { + const parentPath = path.dirname(relativePath); + if (parentPath === "." || parentPath === "") { + return; + } + await root.mkdir(parentPath); +} + +function relativeToRoot(target: ResolvedMountPath, hostPath: string): string { + const relativePath = path.relative(target.mountHostRoot, hostPath); + return relativePath === "." ? "" : relativePath; +} + +async function assertRenameSourceSupported(fromHostPath: string): Promise { + const stats = await fsPromises.lstat(fromHostPath); + if (stats.isSymbolicLink()) { + throw new Error("Sandbox symlink rename sources are not supported by the local mirror bridge"); + } + if (stats.isFile() && stats.nlink > 1) { + throw new Error( + "Sandbox hardlinked rename sources are not supported by the local mirror bridge", + ); + } +} + +async function assertSameDeviceRenameSupported(params: { + fromHostPath: string; + root: string; + toHostPath: string; +}): Promise { + const sourceStats = await fsPromises.lstat(params.fromHostPath); + const destinationParentStats = await nearestExistingDirectoryStats({ + root: params.root, + targetPath: path.dirname(params.toHostPath), + }); + if (sourceStats.dev !== destinationParentStats.dev) { + throw new Error("OpenShell cross-device mirror renames require pinned fs-safe support"); + } +} + +async function nearestExistingDirectoryStats(params: { + root: string; + targetPath: string; +}): Promise>> { + const rootPath = path.resolve(params.root); + let cursor = path.resolve(params.targetPath); + while (isPathInside(rootPath, cursor)) { + const stats = await fsPromises.lstat(cursor).catch((err: unknown) => { + if (isNotFoundError(err)) { + return null; + } + throw err; + }); + if (stats) { + if (!stats.isDirectory()) { + throw new Error(`Sandbox rename destination parent is not a directory: ${cursor}`); + } + return stats; + } + const next = path.dirname(cursor); + if (next === cursor) { + break; + } + cursor = next; + } + return await fsPromises.lstat(rootPath); +} + +function isNotFoundError(err: unknown): boolean { + return ( + (err instanceof FsSafeError && err.code === "not-found") || + (typeof err === "object" && + err !== null && + "code" in err && + (err as { code?: unknown }).code === "ENOENT") + ); +} + function resolveProtectedSkillTarget(params: { input: string; skillsRoot: string; @@ -421,7 +589,11 @@ async function assertLocalPathSafety(params: { const canonicalRoot = await fsPromises .realpath(params.root) .catch(() => path.resolve(params.root)); - const candidate = await resolveCanonicalCandidate(params.target.hostPath); + const targetStats = await fsPromises.lstat(params.target.hostPath).catch(() => null); + const candidate = + params.allowFinalSymlinkForUnlink && targetStats?.isSymbolicLink() + ? path.resolve(canonicalRoot, path.relative(params.root, params.target.hostPath)) + : await resolveCanonicalCandidate(params.target.hostPath); if (!isPathInside(canonicalRoot, candidate)) { throw new Error( `Sandbox path escapes allowed mounts; cannot access: ${params.target.containerPath}`, diff --git a/extensions/openshell/src/openshell-core.test.ts b/extensions/openshell/src/openshell-core.test.ts index 3e791af75a0..2c6faacf761 100644 --- a/extensions/openshell/src/openshell-core.test.ts +++ b/extensions/openshell/src/openshell-core.test.ts @@ -733,6 +733,90 @@ describe("openshell fs bridges", () => { expect(backend["runRemoteShellScript"]).not.toHaveBeenCalled(); }); + it("rejects cross-root mirror renames before the remote backend commit", async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + const agentWorkspaceDir = await makeTempDir("openclaw-openshell-agent-fs-"); + const sourcePath = path.join(workspaceDir, "source.txt"); + await fs.writeFile(sourcePath, "payload", "utf8"); + const backend = createMirrorBackendMock(); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge } = await import("./fs-bridge.js"); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + + await expect(bridge.rename({ from: "source.txt", to: "/agent/source.txt" })).rejects.toThrow( + "OpenShell cross-root mirror renames require pinned fs-safe support", + ); + expect(backend["renameRemotePath"]).not.toHaveBeenCalled(); + await expect(fs.readFile(sourcePath, "utf8")).resolves.toBe("payload"); + await expectPathMissing(path.join(agentWorkspaceDir, "source.txt")); + await expect(fs.readdir(agentWorkspaceDir)).resolves.toStrictEqual([]); + }); + + it.runIf(process.platform !== "win32")( + "rejects local mirror symlink rename sources before the remote backend commit", + async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + await fs.writeFile(path.join(workspaceDir, "target.txt"), "payload", "utf8"); + await fs.symlink("target.txt", path.join(workspaceDir, "link.txt")); + const backend = createMirrorBackendMock(); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge } = await import("./fs-bridge.js"); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + + await expect(bridge.rename({ from: "link.txt", to: "moved-link.txt" })).rejects.toThrow( + "Sandbox symlink rename sources are not supported", + ); + expect(backend["renameRemotePath"]).not.toHaveBeenCalled(); + await expect(fs.readlink(path.join(workspaceDir, "link.txt"))).resolves.toBe("target.txt"); + await expectPathMissing(path.join(workspaceDir, "moved-link.txt")); + }, + ); + + it.runIf(process.platform !== "win32")( + "rejects local mirror hardlinked rename sources before the remote backend commit", + async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + const sourcePath = path.join(workspaceDir, "source.txt"); + await fs.writeFile(sourcePath, "payload", "utf8"); + await fs.link(sourcePath, path.join(workspaceDir, "other-link.txt")); + const backend = createMirrorBackendMock(); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge } = await import("./fs-bridge.js"); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + + await expect(bridge.rename({ from: "source.txt", to: "moved.txt" })).rejects.toThrow( + "Sandbox hardlinked rename sources are not supported", + ); + expect(backend["renameRemotePath"]).not.toHaveBeenCalled(); + await expect(fs.readFile(sourcePath, "utf8")).resolves.toBe("payload"); + await expectPathMissing(path.join(workspaceDir, "moved.txt")); + }, + ); + it("removes remote mirror paths through the pinned backend operation", async () => { const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); await fs.writeFile(path.join(workspaceDir, "target.txt"), "payload", "utf8"); @@ -759,6 +843,187 @@ describe("openshell fs bridges", () => { expect(backend["runRemoteShellScript"]).not.toHaveBeenCalled(); }); + it("removes recursive local mirror directories without raw path deletion", async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + await fs.mkdir(path.join(workspaceDir, "nested", "child"), { recursive: true }); + await fs.writeFile(path.join(workspaceDir, "nested", "child", "target.txt"), "payload", "utf8"); + const backend = createMirrorBackendMock(); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge } = await import("./fs-bridge.js"); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + await bridge.remove({ filePath: "nested", recursive: true, force: true }); + + await expectPathMissing(path.join(workspaceDir, "nested")); + expect(backend["removeRemotePath"]).toHaveBeenCalledWith("/sandbox/nested", { + recursive: true, + signal: undefined, + ignoreMissing: true, + }); + }); + + it.runIf(process.platform !== "win32")( + "removes recursive local mirror directories containing symlink leaves without following them", + async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + const outsideDir = await makeTempDir("openclaw-openshell-outside-"); + const outsideTarget = path.join(outsideDir, "target.txt"); + await fs.mkdir(path.join(workspaceDir, "nested"), { recursive: true }); + await fs.writeFile(outsideTarget, "outside", "utf8"); + await fs.symlink(outsideTarget, path.join(workspaceDir, "nested", "link.txt")); + const backend = createMirrorBackendMock(); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge } = await import("./fs-bridge.js"); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + await bridge.remove({ filePath: "nested", recursive: true, force: true }); + + await expectPathMissing(path.join(workspaceDir, "nested")); + await expect(fs.readFile(outsideTarget, "utf8")).resolves.toBe("outside"); + }, + ); + + it.runIf(process.platform !== "win32")( + "removes local mirror symlink leaves when force is false", + async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + const outsideDir = await makeTempDir("openclaw-openshell-outside-"); + const outsideTarget = path.join(outsideDir, "target.txt"); + await fs.writeFile(outsideTarget, "outside", "utf8"); + await fs.symlink(outsideTarget, path.join(workspaceDir, "link.txt")); + const backend = createMirrorBackendMock(); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge } = await import("./fs-bridge.js"); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + await bridge.remove({ filePath: "link.txt", force: false }); + + await expectPathMissing(path.join(workspaceDir, "link.txt")); + await expect(fs.readFile(outsideTarget, "utf8")).resolves.toBe("outside"); + expect(backend["removeRemotePath"]).toHaveBeenCalledWith("/sandbox/link.txt", { + recursive: false, + signal: undefined, + ignoreMissing: false, + }); + }, + ); + + it.runIf(process.platform !== "win32")( + "rejects local mirror mkdir when a validated parent is swapped to an outside symlink", + async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + const outsideDir = await makeTempDir("openclaw-openshell-outside-"); + const slotPath = path.join(workspaceDir, "slot"); + await fs.mkdir(slotPath, { recursive: true }); + const backend = createMirrorBackendMock(); + backend["mkdirpRemotePath"] = vi.fn().mockImplementation(async () => { + await fs.rm(slotPath, { recursive: true, force: true }); + await fs.symlink(outsideDir, slotPath); + }); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge } = await import("./fs-bridge.js"); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + + await expect(bridge.mkdirp({ filePath: "slot/escaped" })).rejects.toThrow(); + await expectPathMissing(path.join(outsideDir, "escaped")); + }, + ); + + it.runIf(process.platform !== "win32")( + "rejects local mirror remove when a validated parent is swapped to an outside symlink", + async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + const outsideDir = await makeTempDir("openclaw-openshell-outside-"); + const slotPath = path.join(workspaceDir, "slot"); + const outsideTarget = path.join(outsideDir, "target.txt"); + await fs.mkdir(slotPath, { recursive: true }); + await fs.writeFile(path.join(slotPath, "target.txt"), "inside", "utf8"); + await fs.writeFile(outsideTarget, "outside", "utf8"); + const backend = createMirrorBackendMock(); + backend["removeRemotePath"] = vi.fn().mockImplementation(async () => { + await fs.rm(slotPath, { recursive: true, force: true }); + await fs.symlink(outsideDir, slotPath); + }); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge } = await import("./fs-bridge.js"); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + + await expect(bridge.remove({ filePath: "slot/target.txt", force: true })).rejects.toThrow(); + await expect(fs.readFile(outsideTarget, "utf8")).resolves.toBe("outside"); + }, + ); + + it.runIf(process.platform !== "win32")( + "rejects local mirror rename when a validated destination parent is swapped to an outside symlink", + async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + const outsideDir = await makeTempDir("openclaw-openshell-outside-"); + const slotPath = path.join(workspaceDir, "slot"); + const sourcePath = path.join(workspaceDir, "source.txt"); + await fs.mkdir(slotPath, { recursive: true }); + await fs.writeFile(sourcePath, "payload", "utf8"); + const backend = createMirrorBackendMock(); + backend["renameRemotePath"] = vi.fn().mockImplementation(async () => { + await fs.rm(slotPath, { recursive: true, force: true }); + await fs.symlink(outsideDir, slotPath); + }); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge } = await import("./fs-bridge.js"); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + + await expect( + bridge.rename({ from: "source.txt", to: "slot/parent/moved.txt" }), + ).rejects.toThrow(); + await expect(fs.readFile(sourcePath, "utf8")).resolves.toBe("payload"); + await expectPathMissing(path.join(outsideDir, "parent", "moved.txt")); + }, + ); + it("keeps local mirror state unchanged when remote pinned mkdir is rejected", async () => { const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); const backend = createMirrorBackendMock();