mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-24 22:41:18 +00:00
fix(hooks): bound hook workspace manifest and HOOK.md reads (#101472)
* fix(hooks): bound hook workspace manifest and HOOK.md reads * fix(hooks): enforce maxBytes while reading hook metadata fd ClawSweeper review noted that openRootFileSync validates stat size before returning the fd, but the subsequent fs.readFileSync of the fd was unbounded. A file that grows after validation could still OOM workspace discovery. Replace the full fd read with a chunked bounded reader that throws once maxBytes is exceeded, and add regression coverage for the overflow case. * test(hooks): cover bounded hook reads through public workspace API * refactor(hooks): use canonical bounded fd reader for hook metadata * fix(hooks): warn and skip oversized hook metadata during discovery ClawSweeper review required the maintainer-selected oversized-metadata diagnostic contract: warn and skip. readRootFileUtf8 now catches the RangeError from the canonical readFileDescriptorBoundedSync helper and emits one warning per oversized package.json/HOOK.md identifying the path and the byte limit, while discovery continues for other hooks. The redundant open-time maxBytes stat check is removed so the shared bounded reader is the single owner of the byte cap; overflow always surfaces as RangeError, keeping the warning accurate for both static oversized files and post-open growth. Add focused tests: warning content for both oversized surfaces, continued discovery alongside an oversized hook, exact-limit acceptance, and the plain-hook fallback when a package manifest is oversized. * chore: retrigger CI after rebase * refactor(hooks): tighten bounded metadata coverage --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
@@ -2,10 +2,16 @@
|
||||
import fs from "node:fs";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { MANIFEST_KEY } from "../compat/legacy-names.js";
|
||||
import { loadWorkspaceHookEntries } from "./workspace.js";
|
||||
|
||||
const { warnMock } = vi.hoisted(() => ({ warnMock: vi.fn() }));
|
||||
|
||||
vi.mock("../logging/subsystem.js", () => ({
|
||||
createSubsystemLogger: () => ({ warn: warnMock }),
|
||||
}));
|
||||
|
||||
function writeHookPackageManifest(pkgDir: string, hooks: string[]): void {
|
||||
fs.writeFileSync(
|
||||
path.join(pkgDir, "package.json"),
|
||||
@@ -62,7 +68,38 @@ function loadWorkspaceEntriesFromHooksRoot(hooksRoot: string) {
|
||||
});
|
||||
}
|
||||
|
||||
const METADATA_MAX_BYTES = 1024 * 1024;
|
||||
|
||||
function writePlainHook(hooksRoot: string, name: string, content?: string): string {
|
||||
const hookDir = path.join(hooksRoot, name);
|
||||
fs.mkdirSync(hookDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(hookDir, "HOOK.md"), content ?? `---\nname: ${name}\n---\n`);
|
||||
fs.writeFileSync(path.join(hookDir, "handler.js"), "export default async () => {};\n");
|
||||
return hookDir;
|
||||
}
|
||||
|
||||
function oversizedMetadataWarnings(filePath: string): string[] {
|
||||
return warnMock.mock.calls
|
||||
.map(([message]) => String(message))
|
||||
.filter((message) => message.includes(filePath) && message.includes(`${METADATA_MAX_BYTES}`));
|
||||
}
|
||||
|
||||
function padToExactBytes(content: string, targetBytes: number): string {
|
||||
const padding = targetBytes - Buffer.byteLength(content, "utf8");
|
||||
return padding > 0 ? content + " ".repeat(padding) : content;
|
||||
}
|
||||
|
||||
function exactSizeHookPackageManifest(targetBytes: number): string {
|
||||
const base = { name: "pkg", [MANIFEST_KEY]: { hooks: ["./nested"] }, pad: "" };
|
||||
const baseBytes = Buffer.byteLength(JSON.stringify(base), "utf8");
|
||||
return JSON.stringify({ ...base, pad: "x".repeat(targetBytes - baseBytes) });
|
||||
}
|
||||
|
||||
describe("hooks workspace", () => {
|
||||
beforeEach(() => {
|
||||
warnMock.mockClear();
|
||||
});
|
||||
|
||||
it("ignores package.json hook paths that traverse outside package directory", () => {
|
||||
const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-workspace-"));
|
||||
const hooksRoot = path.join(root, "hooks");
|
||||
@@ -100,6 +137,63 @@ describe("hooks workspace", () => {
|
||||
expect(hookNames(entries)).toContain("nested");
|
||||
});
|
||||
|
||||
it("warns, skips oversized metadata, and continues discovering other hooks", () => {
|
||||
const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-oversized-mixed-"));
|
||||
const hooksRoot = path.join(root, "hooks");
|
||||
fs.mkdirSync(hooksRoot, { recursive: true });
|
||||
|
||||
const packageDir = path.join(hooksRoot, "big-package");
|
||||
fs.mkdirSync(packageDir);
|
||||
const manifestPath = path.join(packageDir, "package.json");
|
||||
fs.writeFileSync(manifestPath, "x".repeat(METADATA_MAX_BYTES + 1));
|
||||
|
||||
const bigHookDir = writePlainHook(hooksRoot, "big-hook", "x".repeat(METADATA_MAX_BYTES + 1));
|
||||
const bigHookMdPath = path.join(bigHookDir, "HOOK.md");
|
||||
writePlainHook(hooksRoot, "small-hook");
|
||||
|
||||
const entries = loadWorkspaceEntriesFromHooksRoot(hooksRoot);
|
||||
expect(hookNames(entries)).toEqual(["small-hook"]);
|
||||
expect(oversizedMetadataWarnings(manifestPath)).toHaveLength(1);
|
||||
expect(oversizedMetadataWarnings(bigHookMdPath)).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("loads hooks whose metadata sits exactly at the byte limit", () => {
|
||||
const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-exact-limit-"));
|
||||
const hooksRoot = path.join(root, "hooks");
|
||||
fs.mkdirSync(hooksRoot, { recursive: true });
|
||||
|
||||
const pkgDir = path.join(hooksRoot, "pkg");
|
||||
const nested = path.join(pkgDir, "nested");
|
||||
fs.mkdirSync(nested, { recursive: true });
|
||||
fs.writeFileSync(
|
||||
path.join(pkgDir, "package.json"),
|
||||
exactSizeHookPackageManifest(METADATA_MAX_BYTES),
|
||||
);
|
||||
fs.writeFileSync(
|
||||
path.join(nested, "HOOK.md"),
|
||||
padToExactBytes("---\nname: exact-limit\n---\n", METADATA_MAX_BYTES),
|
||||
);
|
||||
fs.writeFileSync(path.join(nested, "handler.js"), "export default async () => {};\n");
|
||||
|
||||
const entries = loadWorkspaceEntriesFromHooksRoot(hooksRoot);
|
||||
expect(hookNames(entries)).toContain("exact-limit");
|
||||
expect(warnMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("still loads a plain hook when its package.json is oversized", () => {
|
||||
const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-oversized-compat-"));
|
||||
const hooksRoot = path.join(root, "hooks");
|
||||
fs.mkdirSync(hooksRoot, { recursive: true });
|
||||
|
||||
const hookDir = writePlainHook(hooksRoot, "compat-hook");
|
||||
const manifestPath = path.join(hookDir, "package.json");
|
||||
fs.writeFileSync(manifestPath, "x".repeat(METADATA_MAX_BYTES + 1), "utf8");
|
||||
|
||||
const entries = loadWorkspaceEntriesFromHooksRoot(hooksRoot);
|
||||
expect(hookNames(entries)).toContain("compat-hook");
|
||||
expect(oversizedMetadataWarnings(manifestPath)).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("ignores package.json hook paths that escape via symlink", () => {
|
||||
const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-workspace-link-"));
|
||||
const hooksRoot = path.join(root, "hooks");
|
||||
|
||||
@@ -5,6 +5,7 @@ import { normalizeTrimmedStringList } from "@openclaw/normalization-core/string-
|
||||
import { MANIFEST_KEY } from "../compat/legacy-names.js";
|
||||
import type { OpenClawConfig } from "../config/types.openclaw.js";
|
||||
import { openRootFileSync } from "../infra/boundary-file-read.js";
|
||||
import { readFileDescriptorBoundedSync } from "../infra/file-descriptor-read.js";
|
||||
import { createSubsystemLogger } from "../logging/subsystem.js";
|
||||
import { isPathInsideWithRealpath } from "../security/scan-paths.js";
|
||||
import { CONFIG_DIR, resolveUserPath } from "../utils.js";
|
||||
@@ -18,6 +19,10 @@ import { resolvePluginHookDirs } from "./plugin-hooks.js";
|
||||
import { resolveHookEntries } from "./policy.js";
|
||||
import type { Hook, HookEntry, HookSource, ParsedHookFrontmatter } from "./types.js";
|
||||
|
||||
// Hook descriptors are small metadata. Bounding the pinned descriptor read also
|
||||
// covers files that grow after the boundary open validates their identity.
|
||||
const HOOK_METADATA_MAX_BYTES = 1024 * 1024;
|
||||
|
||||
type HookPackageManifest = {
|
||||
name?: string;
|
||||
} & Partial<Record<typeof MANIFEST_KEY, { hooks?: string[] }>>;
|
||||
@@ -34,6 +39,7 @@ function readHookPackageManifest(dir: string): HookPackageManifest | null {
|
||||
absolutePath: manifestPath,
|
||||
rootPath: dir,
|
||||
boundaryLabel: "hook package directory",
|
||||
maxBytes: HOOK_METADATA_MAX_BYTES,
|
||||
});
|
||||
if (raw === null) {
|
||||
return null;
|
||||
@@ -73,6 +79,7 @@ function loadHookFromDir(params: {
|
||||
absolutePath: hookMdPath,
|
||||
rootPath: params.hookDir,
|
||||
boundaryLabel: "hook directory",
|
||||
maxBytes: HOOK_METADATA_MAX_BYTES,
|
||||
});
|
||||
if (content === null) {
|
||||
return null;
|
||||
@@ -293,11 +300,17 @@ function readRootFileUtf8(params: {
|
||||
absolutePath: string;
|
||||
rootPath: string;
|
||||
boundaryLabel: string;
|
||||
maxBytes: number;
|
||||
}): string | null {
|
||||
return withOpenedRootFileSync(params, (opened) => {
|
||||
try {
|
||||
return fs.readFileSync(opened.fd, "utf-8");
|
||||
} catch {
|
||||
return readFileDescriptorBoundedSync(opened.fd, params.maxBytes).toString("utf-8");
|
||||
} catch (err) {
|
||||
if (err instanceof RangeError) {
|
||||
log.warn(
|
||||
`Ignoring oversized hook metadata ${params.absolutePath}: file exceeds the ${params.maxBytes}-byte limit`,
|
||||
);
|
||||
}
|
||||
return null;
|
||||
}
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user