From 0ecd449a438d75b2a3a98040bb3d82f3d018e586 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 28 Jul 2026 07:06:24 -0400 Subject: [PATCH] fix(plugins): report missing plugin modules as missing, not boundary escapes (#115053) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(plugins): report missing plugin modules as missing, not boundary escapes The root-scoped open helper returns a classified failure, but five plugin loader sites collapsed every failure into "escapes plugin root or fails alias checks". A plugin artifact that is simply absent — e.g. while dist/extensions/ is being re-emitted by a build — was therefore logged as a containment violation. Classify the failure instead: missing (ENOENT/ENOTDIR), unreadable (coded), or an actual boundary/alias rejection. The containment check is unchanged; only the reported reason is. Also drops the never-supplied boundaryLabel/boundaryRootDir parameters on loadChannelPluginModule so one root carries one label. * test(infra): rename lint-flagged local helper in boundary failure test --- src/channels/plugins/bundled.ts | 1 - src/channels/plugins/module-loader.test.ts | 30 +++++++++++- src/channels/plugins/module-loader.ts | 25 ++++++---- .../plugins/package-state-probes.test.ts | 48 +++++++++++++++++++ src/infra/boundary-file-read.test.ts | 43 +++++++++++++++++ src/infra/boundary-file-read.ts | 39 +++++++++++++++ src/plugins/bundled-capability-runtime.ts | 12 +++-- src/plugins/loader-channel-runtime.ts | 11 ++++- src/plugins/loader-cli-registry.ts | 11 ++++- src/plugins/loader-runtime-candidate.ts | 11 ++++- 10 files changed, 210 insertions(+), 21 deletions(-) diff --git a/src/channels/plugins/bundled.ts b/src/channels/plugins/bundled.ts index c299f4b86039..5e2ccac8ba71 100644 --- a/src/channels/plugins/bundled.ts +++ b/src/channels/plugins/bundled.ts @@ -329,7 +329,6 @@ function loadGeneratedBundledChannelModule(params: { return loadChannelPluginModule({ modulePath, rootDir: boundaryRoot, - boundaryRootDir: boundaryRoot, }); } catch (error) { const canRetryWithCachedLoader = diff --git a/src/channels/plugins/module-loader.test.ts b/src/channels/plugins/module-loader.test.ts index d7b0a3ffaba0..5a36e8a449f9 100644 --- a/src/channels/plugins/module-loader.test.ts +++ b/src/channels/plugins/module-loader.test.ts @@ -7,7 +7,7 @@ import { fileURLToPath } from "node:url"; import { importFreshModule } from "openclaw/plugin-sdk/test-fixtures"; import { afterEach, describe, expect, it, vi } from "vitest"; import { isJavaScriptModulePath } from "../../plugins/native-module-require.js"; -import { resolveExistingPluginModulePath } from "./module-loader.js"; +import { loadChannelPluginModule, resolveExistingPluginModulePath } from "./module-loader.js"; const tempDirs: string[] = []; const testRequire = createRequire(import.meta.url); @@ -51,6 +51,34 @@ describe("channel plugin module loader helpers", () => { expect(isJavaScriptModulePath("/tmp/entry.ts")).toBe(false); }); + it("reports a missing plugin module as not found instead of a boundary escape", () => { + const rootDir = createTempDir(); + const modulePath = path.join(rootDir, "dist", "extensions", "demo", "auth-presence.js"); + + let thrown: unknown; + try { + loadChannelPluginModule({ modulePath, rootDir }); + } catch (error) { + thrown = error; + } + + expect(thrown).toBeInstanceOf(Error); + expect((thrown as Error).message).toBe(`plugin module path not found: ${modulePath}`); + expect((thrown as Error).message).not.toContain("escapes"); + expect(((thrown as Error).cause as NodeJS.ErrnoException | undefined)?.code).toBe("ENOENT"); + }); + + it("still reports a module outside the plugin root as a boundary escape", () => { + const rootDir = createTempDir(); + const outsideDir = createTempDir(); + const modulePath = path.join(outsideDir, "evil.cjs"); + fs.writeFileSync(modulePath, "module.exports = { ok: true };\n", "utf8"); + + expect(() => loadChannelPluginModule({ modulePath, rootDir })).toThrow( + `plugin module path escapes plugin root or fails alias checks: ${modulePath}`, + ); + }); + it("uses native require for eligible JavaScript modules without creating Jiti", async () => { const createJiti = vi.fn(() => vi.fn(() => ({ ok: false }))); vi.doMock("jiti", () => ({ diff --git a/src/channels/plugins/module-loader.ts b/src/channels/plugins/module-loader.ts index 9e0e814c1c39..cb4383845920 100644 --- a/src/channels/plugins/module-loader.ts +++ b/src/channels/plugins/module-loader.ts @@ -6,7 +6,7 @@ import fs from "node:fs"; import { createRequire } from "node:module"; import path from "node:path"; -import { openRootFileSync } from "../../infra/boundary-file-read.js"; +import { describeRootFileOpenFailure, openRootFileSync } from "../../infra/boundary-file-read.js"; import { isJavaScriptModulePath } from "../../plugins/native-module-require.js"; import { getCachedPluginModuleLoader, @@ -96,23 +96,28 @@ export function resolveExistingPluginModulePath(rootDir: string, specifier: stri /** * Loads a channel plugin module after enforcing plugin-root file boundaries. + * + * `rootDir` is always the plugin's own directory, so the containment failure is + * reported against that one root; no caller boundary override exists. */ -export function loadChannelPluginModule(params: { - modulePath: string; - rootDir: string; - boundaryRootDir?: string; - boundaryLabel?: string; -}): unknown { +export function loadChannelPluginModule(params: { modulePath: string; rootDir: string }): unknown { + const boundaryLabel = "plugin root"; const opened = openRootFileSync({ absolutePath: params.modulePath, - rootPath: params.boundaryRootDir ?? params.rootDir, - boundaryLabel: params.boundaryLabel ?? "plugin root", + rootPath: params.rootDir, + boundaryLabel, rejectHardlinks: false, skipLexicalRootCheck: true, }); if (!opened.ok) { throw new Error( - `${params.boundaryLabel ?? "plugin"} module path escapes plugin root or fails alias checks`, + describeRootFileOpenFailure({ + failure: opened, + subject: "plugin module path", + boundaryLabel, + filePath: params.modulePath, + }), + { cause: opened.error }, ); } const safePath = opened.path; diff --git a/src/channels/plugins/package-state-probes.test.ts b/src/channels/plugins/package-state-probes.test.ts index f50eb08e30d3..94dcbc65c5cb 100644 --- a/src/channels/plugins/package-state-probes.test.ts +++ b/src/channels/plugins/package-state-probes.test.ts @@ -13,6 +13,7 @@ const listChannelCatalogEntriesMock = vi.hoisted(() => vi.fn()); const isBundledSourceOverlayPathMock = vi.hoisted(() => vi.fn((_params: { sourcePath: string }) => false), ); +const probeLogWarnMock = vi.hoisted(() => vi.fn()); const tempDirs: string[] = []; vi.mock("../../plugins/channel-catalog-registry.js", () => ({ @@ -21,6 +22,16 @@ vi.mock("../../plugins/channel-catalog-registry.js", () => ({ vi.mock("../../plugins/bundled-source-overlays.js", () => ({ isBundledSourceOverlayPath: isBundledSourceOverlayPathMock, })); +vi.mock("../../logging/subsystem.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + createSubsystemLogger: (subsystem: string) => ({ + ...actual.createSubsystemLogger(subsystem), + warn: probeLogWarnMock, + }), + }; +}); function makeBundledChannelCatalogEntry(params: { pluginId: string; @@ -52,6 +63,7 @@ beforeEach(() => { listChannelCatalogEntriesMock.mockReset(); isBundledSourceOverlayPathMock.mockReset(); isBundledSourceOverlayPathMock.mockReturnValue(false); + probeLogWarnMock.mockReset(); }); afterEach(() => { @@ -297,6 +309,42 @@ describe("channel package-state probes", () => { ).toBe(true); }); + it("reports a missing built package-state artifact as not found, not a boundary escape", () => { + // Reproduces a rebuild window: the catalog root is the built plugin dir while + // `dist/extensions/` has not been re-emitted yet. + const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-package-state-missing-")); + tempDirs.push(root); + const builtRoot = path.join(root, "dist", "extensions", "matrix"); + fs.mkdirSync(builtRoot, { recursive: true }); + + listChannelCatalogEntriesMock.mockReturnValue([ + { + pluginId: "matrix", + origin: "bundled", + rootDir: builtRoot, + channel: { + id: "matrix", + persistedAuthState: { + specifier: "./auth-presence", + exportName: "hasAnyMatrixAuth", + }, + }, + } satisfies PluginChannelCatalogEntry, + ]); + + expect( + hasBundledChannelPackageState({ + metadataKey: "persistedAuthState", + channelId: "matrix", + cfg: {}, + }), + ).toBe(false); + const warning = String(probeLogWarnMock.mock.calls.at(0)?.[0] ?? ""); + expect(warning).toContain("failed to load persistedAuthState checker for matrix"); + expect(warning).toContain(`plugin module path not found: ${builtRoot}`); + expect(warning).not.toContain("escapes plugin root"); + }); + it("tries dist-runtime package-state probes before falling back to source", () => { const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-package-state-runtime-")); tempDirs.push(root); diff --git a/src/infra/boundary-file-read.test.ts b/src/infra/boundary-file-read.test.ts index 913dc44848fb..d4cd1a647980 100644 --- a/src/infra/boundary-file-read.test.ts +++ b/src/infra/boundary-file-read.test.ts @@ -16,6 +16,49 @@ describe("root file open shim", () => { expect(shim.openRootFileSync).toBe(upstream.openRootFileSync); }); + it("separates missing, unreadable, and boundary-violating open failures", () => { + const messageFor = (failure: upstream.RootFileOpenFailure) => + shim.describeRootFileOpenFailure({ + failure, + subject: "plugin entry path", + boundaryLabel: "plugin root", + filePath: "/plugins/demo/index.js", + }); + + expect( + messageFor({ + ok: false, + reason: "path", + error: Object.assign(new Error("nope"), { code: "ENOENT" }), + }), + ).toBe("plugin entry path not found: /plugins/demo/index.js"); + expect( + messageFor({ + ok: false, + reason: "path", + error: Object.assign(new Error("nope"), { code: "ENOTDIR" }), + }), + ).toBe("plugin entry path not found: /plugins/demo/index.js"); + // fs-safe also reports symlink loops as `path`; those are unreadable, not absent. + expect( + messageFor({ + ok: false, + reason: "path", + error: Object.assign(new Error("loop"), { code: "ELOOP" }), + }), + ).toBe("plugin entry path could not be read (ELOOP): /plugins/demo/index.js"); + expect(messageFor({ ok: false, reason: "validation" })).toBe( + "plugin entry path escapes plugin root or fails alias checks: /plugins/demo/index.js", + ); + expect( + messageFor({ + ok: false, + reason: "io", + error: Object.assign(new Error("io"), { code: "EACCES" }), + }), + ).toBe("plugin entry path could not be read (EACCES): /plugins/demo/index.js"); + }); + it("preserves the existing overflow error for fs-safe descriptor reads", async () => { const dir = tempDirs.make("openclaw-boundary-file-read-"); const filePath = path.join(dir, "oversized.txt"); diff --git a/src/infra/boundary-file-read.ts b/src/infra/boundary-file-read.ts index 39336f5e3cc7..f8e5c099509c 100644 --- a/src/infra/boundary-file-read.ts +++ b/src/infra/boundary-file-read.ts @@ -1,8 +1,10 @@ // Exposes root-scoped file open helpers with fs-safe defaults. import "./fs-safe-defaults.js"; import { + matchRootFileOpenFailure as matchRootFileOpenFailureFsSafe, readFileDescriptorBounded as readFileDescriptorBoundedFsSafe, readFileDescriptorBoundedSync as readFileDescriptorBoundedSyncFsSafe, + type RootFileOpenFailure, } from "@openclaw/fs-safe/advanced"; import { FsSafeError } from "@openclaw/fs-safe/errors"; @@ -17,6 +19,43 @@ export { type RootFileOpenResult, } from "@openclaw/fs-safe/advanced"; +// fs-safe folds ENOENT, ENOTDIR, and ELOOP into its `path` reason. Only the +// first two mean the artifact is absent; a symlink loop is an unreadable path. +const MISSING_PATH_ERROR_CODES: ReadonlySet = new Set(["ENOENT", "ENOTDIR"]); + +function readFailureErrorCode(error: unknown): string | undefined { + const code = error && typeof error === "object" ? (error as { code?: unknown }).code : undefined; + return typeof code === "string" && code ? code : undefined; +} + +/** + * Describes a root-scoped open failure without collapsing every cause into a + * containment violation. Only `validation` means the path failed the boundary or + * alias check; a missing artifact or an unreadable descriptor is an ordinary + * operational state, and reporting those as escapes sends operators hunting a + * security incident that never happened. + */ +export function describeRootFileOpenFailure(params: { + failure: RootFileOpenFailure; + subject: string; + boundaryLabel: string; + filePath: string; +}): string { + const unreadable = (code?: string) => + `${params.subject} could not be read${code ? ` (${code})` : ""}: ${params.filePath}`; + return matchRootFileOpenFailureFsSafe(params.failure, { + path: (failure) => { + const code = readFailureErrorCode(failure.error); + return code && !MISSING_PATH_ERROR_CODES.has(code) + ? unreadable(code) + : `${params.subject} not found: ${params.filePath}`; + }, + validation: () => + `${params.subject} escapes ${params.boundaryLabel} or fails alias checks: ${params.filePath}`, + fallback: (failure) => unreadable(readFailureErrorCode(failure.error)), + }); +} + function preserveOpenClawOverflowError(error: unknown, maxBytes: number): never { if (error instanceof FsSafeError && error.code === "too-large") { throw new RangeError(`File exceeds ${maxBytes} bytes`, { cause: error }); diff --git a/src/plugins/bundled-capability-runtime.ts b/src/plugins/bundled-capability-runtime.ts index e121f88d005b..dab5429e4bb8 100644 --- a/src/plugins/bundled-capability-runtime.ts +++ b/src/plugins/bundled-capability-runtime.ts @@ -1,7 +1,7 @@ /** Loads capability providers from bundled plugin public runtime artifacts. */ import fs from "node:fs"; import { fileURLToPath } from "node:url"; -import { openRootFileSync } from "../infra/boundary-file-read.js"; +import { describeRootFileOpenFailure, openRootFileSync } from "../infra/boundary-file-read.js"; import { createSubsystemLogger } from "../logging/subsystem.js"; import { withBundledPluginEnablementCompat, @@ -277,10 +277,11 @@ export function loadBundledCapabilityRuntimeRegistry(params: { workspaceDir: candidate.workspaceDir, }); + const boundaryLabel = record.source === candidate.source ? "plugin root" : "repo root"; const opened = openRootFileSync({ absolutePath: record.source, rootPath: record.source === candidate.source ? candidate.rootDir : repoRoot, - boundaryLabel: record.source === candidate.source ? "plugin root" : "repo root", + boundaryLabel, rejectHardlinks: false, skipLexicalRootCheck: true, }); @@ -288,7 +289,12 @@ export function loadBundledCapabilityRuntimeRegistry(params: { recordCapabilityLoadError( registry, record, - "plugin entry path escapes plugin root or fails alias checks", + describeRootFileOpenFailure({ + failure: opened, + subject: "plugin entry path", + boundaryLabel, + filePath: record.source, + }), ); continue; } diff --git a/src/plugins/loader-channel-runtime.ts b/src/plugins/loader-channel-runtime.ts index a8585f95a292..94d2708348cc 100644 --- a/src/plugins/loader-channel-runtime.ts +++ b/src/plugins/loader-channel-runtime.ts @@ -1,6 +1,6 @@ import fs from "node:fs"; import type { OpenClawConfig } from "../config/types.openclaw.js"; -import { openRootFileSync } from "../infra/boundary-file-read.js"; +import { describeRootFileOpenFailure, openRootFileSync } from "../infra/boundary-file-read.js"; import type { NormalizedPluginsConfig } from "./config-state.js"; import { channelPluginIdBelongsToManifest, @@ -114,7 +114,14 @@ export function loadSetupRuntimeChannelCandidate(params: { skipLexicalRootCheck: true, }); if (!runtimeOpened.ok) { - params.pushPluginLoadError("plugin entry path escapes plugin root or fails alias checks"); + params.pushPluginLoadError( + describeRootFileOpenFailure({ + failure: runtimeOpened, + subject: "plugin entry path", + boundaryLabel: "plugin root", + filePath: runtimeModuleSource, + }), + ); return true; } const safeRuntimeSource = runtimeOpened.path; diff --git a/src/plugins/loader-cli-registry.ts b/src/plugins/loader-cli-registry.ts index 7c4f86a9d83a..1f355d7c6f59 100644 --- a/src/plugins/loader-cli-registry.ts +++ b/src/plugins/loader-cli-registry.ts @@ -1,7 +1,7 @@ import fs from "node:fs"; import path from "node:path"; import type { GatewayRequestHandler } from "../gateway/server-methods/types.js"; -import { openRootFileSync } from "../infra/boundary-file-read.js"; +import { describeRootFileOpenFailure, openRootFileSync } from "../infra/boundary-file-read.js"; import { resolveUserPath } from "../utils.js"; import { buildPluginApi } from "./api-builder.js"; import { @@ -224,7 +224,14 @@ export async function loadOpenClawPluginCliRegistry( skipLexicalRootCheck: true, }); if (!opened.ok) { - pushPluginLoadError("plugin entry path escapes plugin root or fails alias checks"); + pushPluginLoadError( + describeRootFileOpenFailure({ + failure: opened, + subject: "plugin entry path", + boundaryLabel: "plugin root", + filePath: sourceForCliMetadata, + }), + ); continue; } const safeSource = opened.path; diff --git a/src/plugins/loader-runtime-candidate.ts b/src/plugins/loader-runtime-candidate.ts index 0bb159d247a9..3b4f660c47b1 100644 --- a/src/plugins/loader-runtime-candidate.ts +++ b/src/plugins/loader-runtime-candidate.ts @@ -1,5 +1,5 @@ import fs from "node:fs"; -import { openRootFileSync } from "../infra/boundary-file-read.js"; +import { describeRootFileOpenFailure, openRootFileSync } from "../infra/boundary-file-read.js"; import { inspectBundleMcpRuntimeSupport } from "./bundle-mcp.js"; import { resolveEffectiveEnableState, @@ -372,7 +372,14 @@ export function loadRuntimePluginCandidate(params: { skipLexicalRootCheck: true, }); if (!opened.ok) { - pushPluginLoadError("plugin entry path escapes plugin root or fails alias checks"); + pushPluginLoadError( + describeRootFileOpenFailure({ + failure: opened, + subject: "plugin entry path", + boundaryLabel: "plugin root", + filePath: moduleLoadSource, + }), + ); return; } const safeSource = opened.path;