From 4b2b5bcf89559b4c4e6ba24ba93ebf9d2c03d950 Mon Sep 17 00:00:00 2001 From: cxbAsDev Date: Fri, 17 Jul 2026 18:23:21 +0800 Subject: [PATCH] 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 --- src/hooks/workspace.test.ts | 96 ++++++++++++++++++++++++++++++++++++- src/hooks/workspace.ts | 17 ++++++- 2 files changed, 110 insertions(+), 3 deletions(-) diff --git a/src/hooks/workspace.test.ts b/src/hooks/workspace.test.ts index aa4e4bee8b2a..7888f9f73a06 100644 --- a/src/hooks/workspace.test.ts +++ b/src/hooks/workspace.test.ts @@ -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"); diff --git a/src/hooks/workspace.ts b/src/hooks/workspace.ts index e8553a1a1c58..23c364a93156 100644 --- a/src/hooks/workspace.ts +++ b/src/hooks/workspace.ts @@ -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>; @@ -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; } });