From 3b9dab0ece4643a9643e6a45459f5c709d3ce320 Mon Sep 17 00:00:00 2001 From: Jacob Tomlinson Date: Mon, 30 Mar 2026 06:51:44 -0700 Subject: [PATCH] OpenShell: harden mirror sync boundaries (#57693) * OpenShell: harden mirror sync boundaries * OpenShell: polish mirror hardening tests * OpenShell: preserve trusted mirror symlinks * OpenShell: bound mirror fs work globally --- extensions/openshell/src/backend.ts | 61 +++++++---- extensions/openshell/src/mirror.test.ts | 118 ++++++++++++++++++--- extensions/openshell/src/mirror.ts | 130 +++++++++++++++++++++--- 3 files changed, 262 insertions(+), 47 deletions(-) diff --git a/extensions/openshell/src/backend.ts b/extensions/openshell/src/backend.ts index a4891306180..2013fe0b10f 100644 --- a/extensions/openshell/src/backend.ts +++ b/extensions/openshell/src/backend.ts @@ -27,7 +27,11 @@ import { } from "./cli.js"; import { resolveOpenShellPluginConfig, type ResolvedOpenShellPluginConfig } from "./config.js"; import { createOpenShellFsBridge } from "./fs-bridge.js"; -import { replaceDirectoryContents } from "./mirror.js"; +import { + DEFAULT_OPEN_SHELL_MIRROR_EXCLUDE_DIRS, + replaceDirectoryContents, + stageDirectoryContents, +} from "./mirror.js"; type CreateOpenShellSandboxBackendFactoryParams = { pluginConfig: ResolvedOpenShellPluginConfig; @@ -293,6 +297,14 @@ class OpenShellSandboxBackendImpl { }); return; } + if (stats.isSymbolicLink()) { + await this.runRemoteShellScript({ + script: 'rm -rf -- "$1"', + args: [remotePath], + allowFailure: true, + }); + return; + } if (stats.isDirectory()) { await this.runRemoteShellScript({ script: 'mkdir -p -- "$1"', @@ -421,9 +433,9 @@ class OpenShellSandboxBackendImpl { await replaceDirectoryContents({ sourceDir: tmpDir, targetDir: this.params.createParams.workspaceDir, - // Never sync hooks/ from the remote sandbox — mirrored content must not - // become trusted workspace hook code on the host. - excludeDirs: ["hooks"], + // Never sync trusted host hook directories or repository metadata from + // the remote sandbox. + excludeDirs: DEFAULT_OPEN_SHELL_MIRROR_EXCLUDE_DIRS, }); } finally { await fs.rm(tmpDir, { recursive: true, force: true }); @@ -431,20 +443,33 @@ class OpenShellSandboxBackendImpl { } private async uploadPathToRemote(localPath: string, remotePath: string): Promise { - const result = await runOpenShellCli({ - context: this.params.execContext, - args: [ - "sandbox", - "upload", - "--no-git-ignore", - this.params.execContext.sandboxName, - localPath, - remotePath, - ], - cwd: this.params.createParams.workspaceDir, - }); - if (result.code !== 0) { - throw new Error(result.stderr.trim() || "openshell sandbox upload failed"); + const tmpDir = await fs.mkdtemp( + path.join(resolveOpenShellTmpRoot(), "openclaw-openshell-upload-"), + ); + try { + // Stage a symlink-free snapshot so upload never dereferences host paths + // outside the mirrored workspace tree. + await stageDirectoryContents({ + sourceDir: localPath, + targetDir: tmpDir, + }); + const result = await runOpenShellCli({ + context: this.params.execContext, + args: [ + "sandbox", + "upload", + "--no-git-ignore", + this.params.execContext.sandboxName, + tmpDir, + remotePath, + ], + cwd: this.params.createParams.workspaceDir, + }); + if (result.code !== 0) { + throw new Error(result.stderr.trim() || "openshell sandbox upload failed"); + } + } finally { + await fs.rm(tmpDir, { recursive: true, force: true }); } } diff --git a/extensions/openshell/src/mirror.test.ts b/extensions/openshell/src/mirror.test.ts index 0073c84278f..80556d0f41c 100644 --- a/extensions/openshell/src/mirror.test.ts +++ b/extensions/openshell/src/mirror.test.ts @@ -2,22 +2,26 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; -import { replaceDirectoryContents } from "./mirror.js"; +import { + DEFAULT_OPEN_SHELL_MIRROR_EXCLUDE_DIRS, + replaceDirectoryContents, + stageDirectoryContents, +} from "./mirror.js"; + +const dirs: string[] = []; + +async function makeTmpDir(): Promise { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-mirror-test-")); + dirs.push(dir); + return dir; +} + +afterEach(async () => { + await Promise.all(dirs.map((d) => fs.rm(d, { recursive: true, force: true }))); + dirs.length = 0; +}); describe("replaceDirectoryContents", () => { - const dirs: string[] = []; - - async function makeTmpDir(): Promise { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-mirror-test-")); - dirs.push(dir); - return dir; - } - - afterEach(async () => { - await Promise.all(dirs.map((d) => fs.rm(d, { recursive: true, force: true }))); - dirs.length = 0; - }); - it("copies source entries to target", async () => { const source = await makeTmpDir(); const target = await makeTmpDir(); @@ -89,4 +93,90 @@ describe("replaceDirectoryContents", () => { // "Hooks" (variant case) must still be excluded await expect(fs.access(path.join(target, "Hooks"))).rejects.toThrow(); }); + + it("preserves default excluded directories and repository metadata", async () => { + const source = await makeTmpDir(); + const target = await makeTmpDir(); + + await fs.mkdir(path.join(source, "hooks"), { recursive: true }); + await fs.writeFile(path.join(source, "hooks", "pre-commit"), "malicious"); + await fs.mkdir(path.join(source, "git-hooks"), { recursive: true }); + await fs.writeFile(path.join(source, "git-hooks", "pre-commit"), "malicious"); + await fs.mkdir(path.join(source, ".git", "hooks"), { recursive: true }); + await fs.writeFile(path.join(source, ".git", "hooks", "post-checkout"), "malicious"); + await fs.writeFile(path.join(source, "safe.txt"), "ok"); + + await fs.mkdir(path.join(target, "hooks"), { recursive: true }); + await fs.writeFile(path.join(target, "hooks", "trusted"), "trusted"); + await fs.mkdir(path.join(target, "git-hooks"), { recursive: true }); + await fs.writeFile(path.join(target, "git-hooks", "trusted"), "trusted"); + await fs.mkdir(path.join(target, ".git"), { recursive: true }); + await fs.writeFile(path.join(target, ".git", "HEAD"), "ref: refs/heads/main\n"); + + await replaceDirectoryContents({ + sourceDir: source, + targetDir: target, + excludeDirs: DEFAULT_OPEN_SHELL_MIRROR_EXCLUDE_DIRS, + }); + + expect(await fs.readFile(path.join(target, "safe.txt"), "utf8")).toBe("ok"); + expect(await fs.readFile(path.join(target, "hooks", "trusted"), "utf8")).toBe("trusted"); + expect(await fs.readFile(path.join(target, "git-hooks", "trusted"), "utf8")).toBe("trusted"); + expect(await fs.readFile(path.join(target, ".git", "HEAD"), "utf8")).toBe( + "ref: refs/heads/main\n", + ); + await expect(fs.access(path.join(target, ".git", "hooks", "post-checkout"))).rejects.toThrow(); + }); + + it("skips symbolic links when copying into the host workspace", async () => { + const source = await makeTmpDir(); + const target = await makeTmpDir(); + + await fs.writeFile(path.join(source, "safe.txt"), "ok"); + await fs.mkdir(path.join(source, "nested"), { recursive: true }); + await fs.writeFile(path.join(source, "nested", "file.txt"), "nested"); + await fs.symlink("/tmp/host-secret", path.join(source, "escaped-link")); + await fs.symlink("/tmp/host-secret-dir", path.join(source, "nested", "escaped-dir")); + + await replaceDirectoryContents({ sourceDir: source, targetDir: target }); + + expect(await fs.readFile(path.join(target, "safe.txt"), "utf8")).toBe("ok"); + expect(await fs.readFile(path.join(target, "nested", "file.txt"), "utf8")).toBe("nested"); + await expect(fs.lstat(path.join(target, "escaped-link"))).rejects.toThrow(); + await expect(fs.lstat(path.join(target, "nested", "escaped-dir"))).rejects.toThrow(); + }); + + it("preserves existing trusted host symlinks", async () => { + const source = await makeTmpDir(); + const target = await makeTmpDir(); + + await fs.writeFile(path.join(source, "safe.txt"), "ok"); + await fs.writeFile(path.join(source, "linked-entry"), "remote-plain-file"); + await fs.symlink("/tmp/trusted-host-target", path.join(target, "linked-entry")); + + await replaceDirectoryContents({ sourceDir: source, targetDir: target }); + + expect(await fs.readFile(path.join(target, "safe.txt"), "utf8")).toBe("ok"); + expect(await fs.readlink(path.join(target, "linked-entry"))).toBe("/tmp/trusted-host-target"); + }); +}); + +describe("stageDirectoryContents", () => { + it("stages upload content without symbolic links", async () => { + const source = await makeTmpDir(); + const staged = await makeTmpDir(); + + await fs.writeFile(path.join(source, "safe.txt"), "ok"); + await fs.mkdir(path.join(source, "nested"), { recursive: true }); + await fs.writeFile(path.join(source, "nested", "file.txt"), "nested"); + await fs.symlink("/tmp/host-secret", path.join(source, "escaped-link")); + await fs.symlink("/tmp/host-secret-dir", path.join(source, "nested", "escaped-dir")); + + await stageDirectoryContents({ sourceDir: source, targetDir: staged }); + + expect(await fs.readFile(path.join(staged, "safe.txt"), "utf8")).toBe("ok"); + expect(await fs.readFile(path.join(staged, "nested", "file.txt"), "utf8")).toBe("nested"); + await expect(fs.lstat(path.join(staged, "escaped-link"))).rejects.toThrow(); + await expect(fs.lstat(path.join(staged, "nested", "escaped-dir"))).rejects.toThrow(); + }); }); diff --git a/extensions/openshell/src/mirror.ts b/extensions/openshell/src/mirror.ts index f42d11f8e83..f56edca8725 100644 --- a/extensions/openshell/src/mirror.ts +++ b/extensions/openshell/src/mirror.ts @@ -1,37 +1,137 @@ import fs from "node:fs/promises"; import path from "node:path"; +export const DEFAULT_OPEN_SHELL_MIRROR_EXCLUDE_DIRS = ["hooks", "git-hooks", ".git"] as const; +const COPY_TREE_FS_CONCURRENCY = 16; + +function createExcludeMatcher(excludeDirs?: readonly string[]) { + const excluded = new Set((excludeDirs ?? []).map((d) => d.toLowerCase())); + return (name: string) => excluded.has(name.toLowerCase()); +} + +function createConcurrencyLimiter(limit: number) { + let active = 0; + const queue: Array<() => void> = []; + + const release = () => { + active -= 1; + queue.shift()?.(); + }; + + return async (task: () => Promise): Promise => { + if (active >= limit) { + await new Promise((resolve) => { + queue.push(resolve); + }); + } + active += 1; + try { + return await task(); + } finally { + release(); + } + }; +} + +const runLimitedFs = createConcurrencyLimiter(COPY_TREE_FS_CONCURRENCY); + +async function lstatIfExists(targetPath: string) { + return await runLimitedFs(async () => await fs.lstat(targetPath)).catch(() => null); +} + +async function copyTreeWithoutSymlinks(params: { + sourcePath: string; + targetPath: string; + preserveTargetSymlinks?: boolean; +}): Promise { + const stats = await runLimitedFs(async () => await fs.lstat(params.sourcePath)); + // Mirror sync only carries regular files and directories across the + // host/sandbox boundary. Symlinks and special files are dropped. + if (stats.isSymbolicLink()) { + return; + } + const targetStats = await lstatIfExists(params.targetPath); + if (params.preserveTargetSymlinks && targetStats?.isSymbolicLink()) { + return; + } + if (stats.isDirectory()) { + await runLimitedFs(async () => await fs.mkdir(params.targetPath, { recursive: true })); + const entries = await runLimitedFs(async () => await fs.readdir(params.sourcePath)); + await Promise.all( + entries.map(async (entry) => { + await copyTreeWithoutSymlinks({ + sourcePath: path.join(params.sourcePath, entry), + targetPath: path.join(params.targetPath, entry), + preserveTargetSymlinks: params.preserveTargetSymlinks, + }); + }), + ); + return; + } + if (stats.isFile()) { + await runLimitedFs( + async () => await fs.mkdir(path.dirname(params.targetPath), { recursive: true }), + ); + await runLimitedFs(async () => await fs.copyFile(params.sourcePath, params.targetPath)); + } +} + export async function replaceDirectoryContents(params: { sourceDir: string; targetDir: string; /** Top-level directory names to exclude from sync (preserved in target, skipped from source). */ - excludeDirs?: string[]; + excludeDirs?: readonly string[]; }): Promise { - // Case-insensitive matching: on macOS/Windows the filesystem is typically - // case-insensitive, so "Hooks" would resolve to the same directory as "hooks". - const excluded = new Set((params.excludeDirs ?? []).map((d) => d.toLowerCase())); - const isExcluded = (name: string) => excluded.has(name.toLowerCase()); + const isExcluded = createExcludeMatcher(params.excludeDirs); await fs.mkdir(params.targetDir, { recursive: true }); const existing = await fs.readdir(params.targetDir); await Promise.all( existing .filter((entry) => !isExcluded(entry)) - .map((entry) => - fs.rm(path.join(params.targetDir, entry), { - recursive: true, - force: true, - }), - ), + .map(async (entry) => { + const targetPath = path.join(params.targetDir, entry); + const stats = await lstatIfExists(targetPath); + if (stats?.isSymbolicLink()) { + return; + } + await runLimitedFs( + async () => + await fs.rm(targetPath, { + recursive: true, + force: true, + }), + ); + }), ); const sourceEntries = await fs.readdir(params.sourceDir); for (const entry of sourceEntries) { if (isExcluded(entry)) { continue; } - await fs.cp(path.join(params.sourceDir, entry), path.join(params.targetDir, entry), { - recursive: true, - force: true, - dereference: false, + await copyTreeWithoutSymlinks({ + sourcePath: path.join(params.sourceDir, entry), + targetPath: path.join(params.targetDir, entry), + preserveTargetSymlinks: true, + }); + } +} + +export async function stageDirectoryContents(params: { + sourceDir: string; + targetDir: string; + /** Top-level directory names to exclude from the staged upload. */ + excludeDirs?: readonly string[]; +}): Promise { + const isExcluded = createExcludeMatcher(params.excludeDirs); + await fs.mkdir(params.targetDir, { recursive: true }); + const sourceEntries = await fs.readdir(params.sourceDir); + for (const entry of sourceEntries) { + if (isExcluded(entry)) { + continue; + } + await copyTreeWithoutSymlinks({ + sourcePath: path.join(params.sourceDir, entry), + targetPath: path.join(params.targetDir, entry), }); } }