mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-26 04:15:48 -06:00
fix(plugins): report missing plugin modules as missing, not boundary escapes (#115053)
* 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/<id> 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
This commit is contained in:
committed by
GitHub
parent
fe035de4d6
commit
0ecd449a43
@@ -329,7 +329,6 @@ function loadGeneratedBundledChannelModule(params: {
|
||||
return loadChannelPluginModule({
|
||||
modulePath,
|
||||
rootDir: boundaryRoot,
|
||||
boundaryRootDir: boundaryRoot,
|
||||
});
|
||||
} catch (error) {
|
||||
const canRetryWithCachedLoader =
|
||||
|
||||
@@ -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", () => ({
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<typeof import("../../logging/subsystem.js")>();
|
||||
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/<id>` 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);
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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<string> = 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 });
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user