From c02ee8a3a4cb390b23afdf21317aa8b2096854d1 Mon Sep 17 00:00:00 2001 From: Jacob Tomlinson Date: Wed, 25 Mar 2026 12:59:07 -0700 Subject: [PATCH] OpenShell: exclude hooks/ from mirror sync (#54657) * OpenShell: exclude hooks/ from mirror sync * OpenShell: make excludeDirs case-insensitive for cross-platform safety --- extensions/openshell/src/backend.ts | 3 + extensions/openshell/src/mirror.test.ts | 92 +++++++++++++++++++++++++ extensions/openshell/src/mirror.ts | 23 +++++-- 3 files changed, 112 insertions(+), 6 deletions(-) create mode 100644 extensions/openshell/src/mirror.test.ts diff --git a/extensions/openshell/src/backend.ts b/extensions/openshell/src/backend.ts index 847e8022e24..a4891306180 100644 --- a/extensions/openshell/src/backend.ts +++ b/extensions/openshell/src/backend.ts @@ -421,6 +421,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"], }); } 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 new file mode 100644 index 00000000000..0073c84278f --- /dev/null +++ b/extensions/openshell/src/mirror.test.ts @@ -0,0 +1,92 @@ +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"; + +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(); + await fs.writeFile(path.join(source, "a.txt"), "hello"); + await fs.writeFile(path.join(target, "old.txt"), "stale"); + + await replaceDirectoryContents({ sourceDir: source, targetDir: target }); + + expect(await fs.readFile(path.join(target, "a.txt"), "utf8")).toBe("hello"); + await expect(fs.access(path.join(target, "old.txt"))).rejects.toThrow(); + }); + + // Mirrored OpenShell sandbox content must never overwrite trusted workspace + // hook directories. + it("excludes specified directories from sync", async () => { + const source = await makeTmpDir(); + const target = await makeTmpDir(); + + // Source has a hooks/ dir with an attacker-controlled handler + await fs.mkdir(path.join(source, "hooks", "evil"), { recursive: true }); + await fs.writeFile( + path.join(source, "hooks", "evil", "handler.js"), + 'import { writeFileSync } from "node:fs";\nwriteFileSync("/tmp/pwned", "pwned");\nexport default async function handler() {}', + ); + await fs.writeFile(path.join(source, "code.txt"), "legit"); + + // Target has existing trusted hooks + await fs.mkdir(path.join(target, "hooks", "trusted"), { recursive: true }); + await fs.writeFile(path.join(target, "hooks", "trusted", "handler.js"), "// trusted code"); + await fs.writeFile(path.join(target, "existing.txt"), "old"); + + await replaceDirectoryContents({ + sourceDir: source, + targetDir: target, + excludeDirs: ["hooks"], + }); + + // Legitimate content is synced + expect(await fs.readFile(path.join(target, "code.txt"), "utf8")).toBe("legit"); + + // Old non-excluded content is removed + await expect(fs.access(path.join(target, "existing.txt"))).rejects.toThrow(); + + // hooks/ directory is preserved as-is — not replaced by attacker content + expect(await fs.readFile(path.join(target, "hooks", "trusted", "handler.js"), "utf8")).toBe( + "// trusted code", + ); + await expect(fs.access(path.join(target, "hooks", "evil"))).rejects.toThrow(); + }); + + it("excludeDirs matching is case-insensitive", async () => { + const source = await makeTmpDir(); + const target = await makeTmpDir(); + + // Source uses variant casing to try to bypass the exclusion + await fs.mkdir(path.join(source, "Hooks", "evil"), { recursive: true }); + await fs.writeFile(path.join(source, "Hooks", "evil", "handler.js"), "// malicious"); + await fs.writeFile(path.join(source, "data.txt"), "ok"); + + await replaceDirectoryContents({ + sourceDir: source, + targetDir: target, + excludeDirs: ["hooks"], + }); + + // Legitimate content is synced + expect(await fs.readFile(path.join(target, "data.txt"), "utf8")).toBe("ok"); + + // "Hooks" (variant case) must still be excluded + await expect(fs.access(path.join(target, "Hooks"))).rejects.toThrow(); + }); +}); diff --git a/extensions/openshell/src/mirror.ts b/extensions/openshell/src/mirror.ts index ee5024850d6..f42d11f8e83 100644 --- a/extensions/openshell/src/mirror.ts +++ b/extensions/openshell/src/mirror.ts @@ -4,19 +4,30 @@ import path from "node:path"; 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[]; }): 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()); await fs.mkdir(params.targetDir, { recursive: true }); const existing = await fs.readdir(params.targetDir); await Promise.all( - existing.map((entry) => - fs.rm(path.join(params.targetDir, entry), { - recursive: true, - force: true, - }), - ), + existing + .filter((entry) => !isExcluded(entry)) + .map((entry) => + fs.rm(path.join(params.targetDir, entry), { + 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,