From eac86c2081803fd69d1a20c25129c5fc464e92ce Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 26 Feb 2026 13:04:33 +0100 Subject: [PATCH] refactor: unify boundary hardening for file reads --- .../server-methods/agents-mutate.test.ts | 64 ++++++++++++++ src/gateway/server-methods/agents.ts | 12 +++ src/hooks/loader.test.ts | 66 ++++++++++++++ src/hooks/loader.ts | 51 +++++++---- src/hooks/workspace.test.ts | 63 ++++++++++++++ src/hooks/workspace.ts | 87 +++++++++++++++---- src/infra/boundary-file-read.ts | 31 +++++-- src/plugins/discovery.test.ts | 40 +++++++++ src/plugins/discovery.ts | 19 ++-- src/plugins/loader.test.ts | 50 +++++++++++ src/plugins/loader.ts | 28 ++++-- 11 files changed, 455 insertions(+), 56 deletions(-) diff --git a/src/gateway/server-methods/agents-mutate.test.ts b/src/gateway/server-methods/agents-mutate.test.ts index 26503db55..a12db195c 100644 --- a/src/gateway/server-methods/agents-mutate.test.ts +++ b/src/gateway/server-methods/agents-mutate.test.ts @@ -137,6 +137,7 @@ function makeFileStat(params?: { mtimeMs?: number; dev?: number; ino?: number; + nlink?: number; }): import("node:fs").Stats { return { isFile: () => true, @@ -145,6 +146,7 @@ function makeFileStat(params?: { mtimeMs: params?.mtimeMs ?? 1234, dev: params?.dev ?? 1, ino: params?.ino ?? 1, + nlink: params?.nlink ?? 1, } as unknown as import("node:fs").Stats; } @@ -649,4 +651,66 @@ describe("agents.files.get/set symlink safety", () => { undefined, ); }); + + it("rejects agents.files.get when allowlisted file is a hardlinked alias", async () => { + const workspace = "/workspace/test-agent"; + const candidate = path.resolve(workspace, "AGENTS.md"); + mocks.fsRealpath.mockImplementation(async (p: string) => { + if (p === workspace) { + return workspace; + } + return p; + }); + mocks.fsLstat.mockImplementation(async (...args: unknown[]) => { + const p = typeof args[0] === "string" ? args[0] : ""; + if (p === candidate) { + return makeFileStat({ nlink: 2 }); + } + throw createEnoentError(); + }); + + const { respond, promise } = makeCall("agents.files.get", { + agentId: "main", + name: "AGENTS.md", + }); + await promise; + + expect(respond).toHaveBeenCalledWith( + false, + undefined, + expect.objectContaining({ message: expect.stringContaining("unsafe workspace file") }), + ); + }); + + it("rejects agents.files.set when allowlisted file is a hardlinked alias", async () => { + const workspace = "/workspace/test-agent"; + const candidate = path.resolve(workspace, "AGENTS.md"); + mocks.fsRealpath.mockImplementation(async (p: string) => { + if (p === workspace) { + return workspace; + } + return p; + }); + mocks.fsLstat.mockImplementation(async (...args: unknown[]) => { + const p = typeof args[0] === "string" ? args[0] : ""; + if (p === candidate) { + return makeFileStat({ nlink: 2 }); + } + throw createEnoentError(); + }); + + const { respond, promise } = makeCall("agents.files.set", { + agentId: "main", + name: "AGENTS.md", + content: "x", + }); + await promise; + + expect(respond).toHaveBeenCalledWith( + false, + undefined, + expect.objectContaining({ message: expect.stringContaining("unsafe workspace file") }), + ); + expect(mocks.fsOpen).not.toHaveBeenCalled(); + }); }); diff --git a/src/gateway/server-methods/agents.ts b/src/gateway/server-methods/agents.ts index 413ffddc8..0e132e5b8 100644 --- a/src/gateway/server-methods/agents.ts +++ b/src/gateway/server-methods/agents.ts @@ -181,6 +181,9 @@ async function resolveAgentWorkspaceFilePath(params: { if (!targetStat.isFile()) { return { kind: "invalid", requestPath, reason: "symlink target is not a file" }; } + if (targetStat.nlink > 1) { + return { kind: "invalid", requestPath, reason: "hardlinked file target not allowed" }; + } } catch (err) { if (isNotFoundPathError(err) && params.allowMissing) { return { kind: "missing", requestPath, ioPath: targetReal, workspaceReal }; @@ -193,6 +196,9 @@ async function resolveAgentWorkspaceFilePath(params: { if (!candidateLstat.isFile()) { return { kind: "invalid", requestPath, reason: "path is not a regular file" }; } + if (candidateLstat.nlink > 1) { + return { kind: "invalid", requestPath, reason: "hardlinked file path not allowed" }; + } const candidateReal = await fs.realpath(candidatePath).catch(() => candidatePath); if (!isPathInside(workspaceReal, candidateReal)) { @@ -207,6 +213,9 @@ async function statFileSafely(filePath: string): Promise { if (lstat.isSymbolicLink() || !stat.isFile()) { return null; } + if (stat.nlink > 1) { + return null; + } if (!sameFileIdentity(stat, lstat)) { return null; } @@ -226,6 +235,9 @@ async function writeFileSafely(filePath: string, content: string): Promise if (lstat.isSymbolicLink() || !stat.isFile()) { throw new Error("unsafe file path"); } + if (stat.nlink > 1) { + throw new Error("hardlinked file path is not allowed"); + } if (!sameFileIdentity(stat, lstat)) { throw new Error("path changed during write"); } diff --git a/src/hooks/loader.test.ts b/src/hooks/loader.test.ts index 66ccf04b8..d9107d2e3 100644 --- a/src/hooks/loader.test.ts +++ b/src/hooks/loader.test.ts @@ -281,5 +281,71 @@ describe("loader", () => { expect(count).toBe(0); expect(getRegisteredEventKeys()).not.toContain("command:new"); }); + + it("rejects directory hook handlers that escape hook dir via hardlink", async () => { + if (process.platform === "win32") { + return; + } + const outsideHandlerPath = path.join(fixtureRoot, `outside-handler-hardlink-${caseId}.js`); + await fs.writeFile(outsideHandlerPath, "export default async function() {}", "utf-8"); + + const hookDir = path.join(tmpDir, "hooks", "hardlink-hook"); + await fs.mkdir(hookDir, { recursive: true }); + await fs.writeFile( + path.join(hookDir, "HOOK.md"), + [ + "---", + "name: hardlink-hook", + "description: hardlink test", + 'metadata: {"openclaw":{"events":["command:new"]}}', + "---", + "", + "# Hardlink Hook", + ].join("\n"), + "utf-8", + ); + try { + await fs.link(outsideHandlerPath, path.join(hookDir, "handler.js")); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "EXDEV") { + return; + } + throw err; + } + + const cfg = createEnabledHooksConfig(); + const count = await loadInternalHooks(cfg, tmpDir); + expect(count).toBe(0); + expect(getRegisteredEventKeys()).not.toContain("command:new"); + }); + + it("rejects legacy handler modules that escape workspace via hardlink", async () => { + if (process.platform === "win32") { + return; + } + const outsideHandlerPath = path.join(fixtureRoot, `outside-legacy-hardlink-${caseId}.js`); + await fs.writeFile(outsideHandlerPath, "export default async function() {}", "utf-8"); + + const linkedHandlerPath = path.join(tmpDir, "legacy-handler.js"); + try { + await fs.link(outsideHandlerPath, linkedHandlerPath); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "EXDEV") { + return; + } + throw err; + } + + const cfg = createEnabledHooksConfig([ + { + event: "command:new", + module: "legacy-handler.js", + }, + ]); + + const count = await loadInternalHooks(cfg, tmpDir); + expect(count).toBe(0); + expect(getRegisteredEventKeys()).not.toContain("command:new"); + }); }); }); diff --git a/src/hooks/loader.ts b/src/hooks/loader.ts index 30f37e4db..4a1fb9646 100644 --- a/src/hooks/loader.ts +++ b/src/hooks/loader.ts @@ -5,10 +5,11 @@ * and from directory-based discovery (bundled, managed, workspace) */ +import fs from "node:fs"; import path from "node:path"; import type { OpenClawConfig } from "../config/config.js"; +import { openBoundaryFile } from "../infra/boundary-file-read.js"; import { createSubsystemLogger } from "../logging/subsystem.js"; -import { isPathInsideWithRealpath } from "../security/scan-paths.js"; import { resolveHookConfig } from "./config.js"; import { shouldIncludeHook } from "./config.js"; import { buildImportUrl } from "./import-url.js"; @@ -73,18 +74,23 @@ export async function loadInternalHooks( } try { - if ( - !isPathInsideWithRealpath(entry.hook.baseDir, entry.hook.handlerPath, { - requireRealpath: true, - }) - ) { + const hookBaseDir = safeRealpathOrResolve(entry.hook.baseDir); + const opened = await openBoundaryFile({ + absolutePath: entry.hook.handlerPath, + rootPath: hookBaseDir, + boundaryLabel: "hook directory", + }); + if (!opened.ok) { log.error( - `Hook '${entry.hook.name}' handler path resolves outside hook directory: ${entry.hook.handlerPath}`, + `Hook '${entry.hook.name}' handler path fails boundary checks: ${entry.hook.handlerPath}`, ); continue; } + const safeHandlerPath = opened.path; + fs.closeSync(opened.fd); + // Import handler module — only cache-bust mutable (workspace/managed) hooks - const importUrl = buildImportUrl(entry.hook.handlerPath, entry.hook.source); + const importUrl = buildImportUrl(safeHandlerPath, entry.hook.source); const mod = (await import(importUrl)) as Record; // Get handler function (default or named export) @@ -144,24 +150,27 @@ export async function loadInternalHooks( } const baseDir = path.resolve(workspaceDir); const modulePath = path.resolve(baseDir, rawModule); + const baseDirReal = safeRealpathOrResolve(baseDir); + const modulePathSafe = safeRealpathOrResolve(modulePath); const rel = path.relative(baseDir, modulePath); if (!rel || rel.startsWith("..") || path.isAbsolute(rel)) { log.error(`Handler module path must stay within workspaceDir: ${rawModule}`); continue; } - if ( - !isPathInsideWithRealpath(baseDir, modulePath, { - requireRealpath: true, - }) - ) { - log.error( - `Handler module path resolves outside workspaceDir after symlink resolution: ${rawModule}`, - ); + const opened = await openBoundaryFile({ + absolutePath: modulePathSafe, + rootPath: baseDirReal, + boundaryLabel: "workspace directory", + }); + if (!opened.ok) { + log.error(`Handler module path fails boundary checks under workspaceDir: ${rawModule}`); continue; } + const safeModulePath = opened.path; + fs.closeSync(opened.fd); // Legacy handlers are always workspace-relative, so use mtime-based cache busting - const importUrl = buildImportUrl(modulePath, "openclaw-workspace"); + const importUrl = buildImportUrl(safeModulePath, "openclaw-workspace"); const mod = (await import(importUrl)) as Record; // Get the handler function @@ -190,3 +199,11 @@ export async function loadInternalHooks( return loadedCount; } + +function safeRealpathOrResolve(value: string): string { + try { + return fs.realpathSync(value); + } catch { + return path.resolve(value); + } +} diff --git a/src/hooks/workspace.test.ts b/src/hooks/workspace.test.ts index cc35ffdd6..dc3de2acd 100644 --- a/src/hooks/workspace.test.ts +++ b/src/hooks/workspace.test.ts @@ -102,4 +102,67 @@ describe("hooks workspace", () => { const entries = loadHookEntriesFromDir({ dir: hooksRoot, source: "openclaw-workspace" }); expect(entries.some((e) => e.hook.name === "outside")).toBe(false); }); + + it("ignores hooks with hardlinked HOOK.md aliases", () => { + if (process.platform === "win32") { + return; + } + + const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-workspace-hardlink-")); + const hooksRoot = path.join(root, "hooks"); + fs.mkdirSync(hooksRoot, { recursive: true }); + + const hookDir = path.join(hooksRoot, "hardlink-hook"); + const outsideDir = path.join(root, "outside"); + fs.mkdirSync(hookDir, { recursive: true }); + fs.mkdirSync(outsideDir, { recursive: true }); + fs.writeFileSync(path.join(hookDir, "handler.js"), "export default async () => {};\n"); + const outsideHookMd = path.join(outsideDir, "HOOK.md"); + const linkedHookMd = path.join(hookDir, "HOOK.md"); + fs.writeFileSync(linkedHookMd, "---\nname: hardlink-hook\n---\n"); + fs.rmSync(linkedHookMd); + fs.writeFileSync(outsideHookMd, "---\nname: outside\n---\n"); + try { + fs.linkSync(outsideHookMd, linkedHookMd); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "EXDEV") { + return; + } + throw err; + } + + const entries = loadHookEntriesFromDir({ dir: hooksRoot, source: "openclaw-workspace" }); + expect(entries.some((e) => e.hook.name === "hardlink-hook")).toBe(false); + expect(entries.some((e) => e.hook.name === "outside")).toBe(false); + }); + + it("ignores hooks with hardlinked handler aliases", () => { + if (process.platform === "win32") { + return; + } + + const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-workspace-hardlink-")); + const hooksRoot = path.join(root, "hooks"); + fs.mkdirSync(hooksRoot, { recursive: true }); + + const hookDir = path.join(hooksRoot, "hardlink-handler-hook"); + const outsideDir = path.join(root, "outside"); + fs.mkdirSync(hookDir, { recursive: true }); + fs.mkdirSync(outsideDir, { recursive: true }); + fs.writeFileSync(path.join(hookDir, "HOOK.md"), "---\nname: hardlink-handler-hook\n---\n"); + const outsideHandler = path.join(outsideDir, "handler.js"); + const linkedHandler = path.join(hookDir, "handler.js"); + fs.writeFileSync(outsideHandler, "export default async () => {};\n"); + try { + fs.linkSync(outsideHandler, linkedHandler); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "EXDEV") { + return; + } + throw err; + } + + const entries = loadHookEntriesFromDir({ dir: hooksRoot, source: "openclaw-workspace" }); + expect(entries.some((e) => e.hook.name === "hardlink-handler-hook")).toBe(false); + }); }); diff --git a/src/hooks/workspace.ts b/src/hooks/workspace.ts index 426569a21..ab6375cd8 100644 --- a/src/hooks/workspace.ts +++ b/src/hooks/workspace.ts @@ -2,6 +2,7 @@ import fs from "node:fs"; import path from "node:path"; import { MANIFEST_KEY } from "../compat/legacy-names.js"; import type { OpenClawConfig } from "../config/config.js"; +import { openBoundaryFileSync } from "../infra/boundary-file-read.js"; import { createSubsystemLogger } from "../logging/subsystem.js"; import { isPathInsideWithRealpath } from "../security/scan-paths.js"; import { CONFIG_DIR, resolveUserPath } from "../utils.js"; @@ -36,11 +37,15 @@ function filterHookEntries( function readHookPackageManifest(dir: string): HookPackageManifest | null { const manifestPath = path.join(dir, "package.json"); - if (!fs.existsSync(manifestPath)) { + const raw = readBoundaryFileUtf8({ + absolutePath: manifestPath, + rootPath: dir, + boundaryLabel: "hook package directory", + }); + if (raw === null) { return null; } try { - const raw = fs.readFileSync(manifestPath, "utf-8"); return JSON.parse(raw) as HookPackageManifest; } catch { return null; @@ -75,12 +80,15 @@ function loadHookFromDir(params: { nameHint?: string; }): Hook | null { const hookMdPath = path.join(params.hookDir, "HOOK.md"); - if (!fs.existsSync(hookMdPath)) { + const content = readBoundaryFileUtf8({ + absolutePath: hookMdPath, + rootPath: params.hookDir, + boundaryLabel: "hook directory", + }); + if (content === null) { return null; } - try { - const content = fs.readFileSync(hookMdPath, "utf-8"); const frontmatter = parseFrontmatter(content); const name = frontmatter.name || params.nameHint || path.basename(params.hookDir); @@ -90,8 +98,13 @@ function loadHookFromDir(params: { let handlerPath: string | undefined; for (const candidate of handlerCandidates) { const candidatePath = path.join(params.hookDir, candidate); - if (fs.existsSync(candidatePath)) { - handlerPath = candidatePath; + const safeCandidatePath = resolveBoundaryFilePath({ + absolutePath: candidatePath, + rootPath: params.hookDir, + boundaryLabel: "hook directory", + }); + if (safeCandidatePath) { + handlerPath = safeCandidatePath; break; } } @@ -192,11 +205,13 @@ export function loadHookEntriesFromDir(params: { }); return hooks.map((hook) => { let frontmatter: ParsedHookFrontmatter = {}; - try { - const raw = fs.readFileSync(hook.filePath, "utf-8"); + const raw = readBoundaryFileUtf8({ + absolutePath: hook.filePath, + rootPath: hook.baseDir, + boundaryLabel: "hook directory", + }); + if (raw !== null) { frontmatter = parseFrontmatter(raw); - } catch { - // ignore malformed hooks } const entry: HookEntry = { hook: { @@ -267,11 +282,13 @@ function loadHookEntries( return Array.from(merged.values()).map((hook) => { let frontmatter: ParsedHookFrontmatter = {}; - try { - const raw = fs.readFileSync(hook.filePath, "utf-8"); + const raw = readBoundaryFileUtf8({ + absolutePath: hook.filePath, + rootPath: hook.baseDir, + boundaryLabel: "hook directory", + }); + if (raw !== null) { frontmatter = parseFrontmatter(raw); - } catch { - // ignore malformed hooks } return { hook, @@ -316,3 +333,43 @@ export function loadWorkspaceHookEntries( ): HookEntry[] { return loadHookEntries(workspaceDir, opts); } + +function readBoundaryFileUtf8(params: { + absolutePath: string; + rootPath: string; + boundaryLabel: string; +}): string | null { + const opened = openBoundaryFileSync({ + absolutePath: params.absolutePath, + rootPath: params.rootPath, + boundaryLabel: params.boundaryLabel, + }); + if (!opened.ok) { + return null; + } + try { + return fs.readFileSync(opened.fd, "utf-8"); + } catch { + return null; + } finally { + fs.closeSync(opened.fd); + } +} + +function resolveBoundaryFilePath(params: { + absolutePath: string; + rootPath: string; + boundaryLabel: string; +}): string | null { + const opened = openBoundaryFileSync({ + absolutePath: params.absolutePath, + rootPath: params.rootPath, + boundaryLabel: params.boundaryLabel, + }); + if (!opened.ok) { + return null; + } + const safePath = opened.path; + fs.closeSync(opened.fd); + return safePath; +} diff --git a/src/infra/boundary-file-read.ts b/src/infra/boundary-file-read.ts index a8d416e03..fd3e0e649 100644 --- a/src/infra/boundary-file-read.ts +++ b/src/infra/boundary-file-read.ts @@ -65,17 +65,23 @@ export function openBoundaryFileSync(params: OpenBoundaryFileSyncParams): Bounda ? path.resolve(params.rootRealPath) : safeRealpathSync(ioFs, rootPath); - if (!params.skipLexicalRootCheck && !isPathInside(rootPath, absolutePath)) { - return { - ok: false, - reason: "validation", - error: new Error(`Path escapes ${params.boundaryLabel}: ${absolutePath} (root: ${rootPath})`), - }; - } - let resolvedPath = absolutePath; + const lexicalInsideRoot = isPathInside(rootPath, absolutePath); try { const candidateRealPath = path.resolve(ioFs.realpathSync(absolutePath)); + if ( + !params.skipLexicalRootCheck && + !lexicalInsideRoot && + !isPathInside(rootRealPath, candidateRealPath) + ) { + return { + ok: false, + reason: "validation", + error: new Error( + `Path escapes ${params.boundaryLabel}: ${absolutePath} (root: ${rootPath})`, + ), + }; + } if (!isPathInside(rootRealPath, candidateRealPath)) { return { ok: false, @@ -87,6 +93,15 @@ export function openBoundaryFileSync(params: OpenBoundaryFileSyncParams): Bounda } resolvedPath = candidateRealPath; } catch (error) { + if (!params.skipLexicalRootCheck && !lexicalInsideRoot) { + return { + ok: false, + reason: "validation", + error: new Error( + `Path escapes ${params.boundaryLabel}: ${absolutePath} (root: ${rootPath})`, + ), + }; + } if (!isNotFoundPathError(error)) { // Keep resolvedPath as lexical path; openVerifiedFileSync below will produce // a canonical error classification for missing/unreadable targets. diff --git a/src/plugins/discovery.test.ts b/src/plugins/discovery.test.ts index 180ab87cc..9a18e1f0c 100644 --- a/src/plugins/discovery.test.ts +++ b/src/plugins/discovery.test.ts @@ -231,6 +231,46 @@ describe("discoverOpenClawPlugins", () => { ); }); + it("rejects package extension entries that are hardlinked aliases", async () => { + if (process.platform === "win32") { + return; + } + const stateDir = makeTempDir(); + const globalExt = path.join(stateDir, "extensions", "pack"); + const outsideDir = path.join(stateDir, "outside"); + const outsideFile = path.join(outsideDir, "escape.ts"); + const linkedFile = path.join(globalExt, "escape.ts"); + fs.mkdirSync(globalExt, { recursive: true }); + fs.mkdirSync(outsideDir, { recursive: true }); + fs.writeFileSync(outsideFile, "export default {}", "utf-8"); + try { + fs.linkSync(outsideFile, linkedFile); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "EXDEV") { + return; + } + throw err; + } + + fs.writeFileSync( + path.join(globalExt, "package.json"), + JSON.stringify({ + name: "@openclaw/pack", + openclaw: { extensions: ["./escape.ts"] }, + }), + "utf-8", + ); + + const { candidates, diagnostics } = await withStateDir(stateDir, async () => { + return discoverOpenClawPlugins({}); + }); + + expect(candidates.some((candidate) => candidate.idHint === "pack")).toBe(false); + expect(diagnostics.some((entry) => entry.message.includes("escapes package directory"))).toBe( + true, + ); + }); + it.runIf(process.platform !== "win32")("blocks world-writable plugin paths", async () => { const stateDir = makeTempDir(); const globalExt = path.join(stateDir, "extensions"); diff --git a/src/plugins/discovery.ts b/src/plugins/discovery.ts index 1df727fab..789d93d82 100644 --- a/src/plugins/discovery.ts +++ b/src/plugins/discovery.ts @@ -1,6 +1,6 @@ import fs from "node:fs"; import path from "node:path"; -import { isPathInsideWithRealpath } from "../security/scan-paths.js"; +import { openBoundaryFileSync } from "../infra/boundary-file-read.js"; import { resolveConfigDir, resolveUserPath } from "../utils.js"; import { resolveBundledPluginsDir } from "./bundled-dir.js"; import { @@ -284,7 +284,7 @@ function addCandidate(params: { if (params.seen.has(resolved)) { return; } - const resolvedRoot = path.resolve(params.rootDir); + const resolvedRoot = safeRealpathSync(params.rootDir) ?? path.resolve(params.rootDir); if ( isUnsafePluginCandidate({ source: resolved, @@ -319,11 +319,12 @@ function resolvePackageEntrySource(params: { diagnostics: PluginDiagnostic[]; }): string | null { const source = path.resolve(params.packageDir, params.entryPath); - if ( - !isPathInsideWithRealpath(params.packageDir, source, { - requireRealpath: true, - }) - ) { + const opened = openBoundaryFileSync({ + absolutePath: source, + rootPath: params.packageDir, + boundaryLabel: "plugin package directory", + }); + if (!opened.ok) { params.diagnostics.push({ level: "error", message: `extension entry escapes package directory: ${params.entryPath}`, @@ -331,7 +332,9 @@ function resolvePackageEntrySource(params: { }); return null; } - return source; + const safeSource = opened.path; + fs.closeSync(opened.fd); + return safeSource; } function discoverInDirectory(params: { diff --git a/src/plugins/loader.test.ts b/src/plugins/loader.test.ts index c3430ebf0..80bccc6d8 100644 --- a/src/plugins/loader.test.ts +++ b/src/plugins/loader.test.ts @@ -651,6 +651,56 @@ describe("loadOpenClawPlugins", () => { expect(registry.diagnostics.some((entry) => entry.message.includes("escapes"))).toBe(true); }); + it("rejects plugin entry files that escape plugin root via hardlink", () => { + if (process.platform === "win32") { + return; + } + process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins"; + const pluginDir = makeTempDir(); + const outsideDir = makeTempDir(); + const outsideEntry = path.join(outsideDir, "outside.js"); + const linkedEntry = path.join(pluginDir, "entry.js"); + fs.writeFileSync( + outsideEntry, + 'export default { id: "hardlinked", register() { throw new Error("should not run"); } };', + "utf-8", + ); + fs.writeFileSync( + path.join(pluginDir, "openclaw.plugin.json"), + JSON.stringify( + { + id: "hardlinked", + configSchema: EMPTY_PLUGIN_SCHEMA, + }, + null, + 2, + ), + "utf-8", + ); + try { + fs.linkSync(outsideEntry, linkedEntry); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "EXDEV") { + return; + } + throw err; + } + + const registry = loadOpenClawPlugins({ + cache: false, + config: { + plugins: { + load: { paths: [linkedEntry] }, + allow: ["hardlinked"], + }, + }, + }); + + const record = registry.plugins.find((entry) => entry.id === "hardlinked"); + expect(record?.status).not.toBe("loaded"); + expect(registry.diagnostics.some((entry) => entry.message.includes("escapes"))).toBe(true); + }); + it("prefers dist plugin-sdk alias when loader runs from dist", () => { const root = makeTempDir(); const srcFile = path.join(root, "src", "plugin-sdk", "index.ts"); diff --git a/src/plugins/loader.ts b/src/plugins/loader.ts index e6e93d29f..e8ba559bc 100644 --- a/src/plugins/loader.ts +++ b/src/plugins/loader.ts @@ -4,8 +4,8 @@ import { fileURLToPath } from "node:url"; import { createJiti } from "jiti"; import type { OpenClawConfig } from "../config/config.js"; import type { GatewayRequestHandler } from "../gateway/server-methods/types.js"; +import { openBoundaryFileSync } from "../infra/boundary-file-read.js"; import { createSubsystemLogger } from "../logging/subsystem.js"; -import { isPathInsideWithRealpath } from "../security/scan-paths.js"; import { resolveUserPath } from "../utils.js"; import { clearPluginCommands } from "./commands.js"; import { @@ -525,13 +525,15 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi continue; } - if ( - !isPathInsideWithRealpath(candidate.rootDir, candidate.source, { - requireRealpath: true, - }) - ) { + const pluginRoot = safeRealpathOrResolve(candidate.rootDir); + const opened = openBoundaryFileSync({ + absolutePath: candidate.source, + rootPath: pluginRoot, + boundaryLabel: "plugin root", + }); + if (!opened.ok) { record.status = "error"; - record.error = "plugin entry path escapes plugin root"; + record.error = "plugin entry path escapes plugin root or fails alias checks"; registry.plugins.push(record); seenIds.set(pluginId, candidate.origin); registry.diagnostics.push({ @@ -542,10 +544,12 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi }); continue; } + const safeSource = opened.path; + fs.closeSync(opened.fd); let mod: OpenClawPluginModule | null = null; try { - mod = getJiti()(candidate.source) as OpenClawPluginModule; + mod = getJiti()(safeSource) as OpenClawPluginModule; } catch (err) { recordPluginError({ logger, @@ -707,3 +711,11 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi initializeGlobalHookRunner(registry); return registry; } + +function safeRealpathOrResolve(value: string): string { + try { + return fs.realpathSync(value); + } catch { + return path.resolve(value); + } +}