Skip to content

Commit 7e271d9

Browse files
steipetesteipete
committed
refactor(hooks): tighten bounded metadata coverage
1 parent b7b64d0 commit 7e271d9

2 files changed

Lines changed: 27 additions & 73 deletions

File tree

‎src/hooks/workspace.test.ts‎

Lines changed: 21 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -8,21 +8,9 @@ import { loadWorkspaceHookEntries } from "./workspace.js";
88

99
const { warnMock } = vi.hoisted(() => ({ warnMock: vi.fn() }));
1010

11-
vi.mock("../logging/subsystem.js", () => {
12-
const makeLogger = () => ({
13-
subsystem: "hooks/workspace",
14-
isEnabled: () => true,
15-
trace: vi.fn(),
16-
debug: vi.fn(),
17-
info: vi.fn(),
18-
warn: warnMock,
19-
error: vi.fn(),
20-
fatal: vi.fn(),
21-
raw: vi.fn(),
22-
child: () => makeLogger(),
23-
});
24-
return { createSubsystemLogger: () => makeLogger() };
25-
});
11+
vi.mock("../logging/subsystem.js", () => ({
12+
createSubsystemLogger: () => ({ warn: warnMock }),
13+
}));
2614

2715
function writeHookPackageManifest(pkgDir: string, hooks: string[]): void {
2816
fs.writeFileSync(
@@ -82,6 +70,14 @@ function loadWorkspaceEntriesFromHooksRoot(hooksRoot: string) {
8270

8371
const METADATA_MAX_BYTES = 1024 * 1024;
8472

73+
function writePlainHook(hooksRoot: string, name: string, content?: string): string {
74+
const hookDir = path.join(hooksRoot, name);
75+
fs.mkdirSync(hookDir, { recursive: true });
76+
fs.writeFileSync(path.join(hookDir, "HOOK.md"), content ?? `---\nname: ${name}\n---\n`);
77+
fs.writeFileSync(path.join(hookDir, "handler.js"), "export default async () => {};\n");
78+
return hookDir;
79+
}
80+
8581
function oversizedMetadataWarnings(filePath: string): string[] {
8682
return warnMock.mock.calls
8783
.map(([message]) => String(message))
@@ -141,55 +137,23 @@ describe("hooks workspace", () => {
141137
expect(hookNames(entries)).toContain("nested");
142138
});
143139

144-
it("warns and skips hook packages with an oversized package.json", () => {
145-
const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-oversized-manifest-"));
146-
const hooksRoot = path.join(root, "hooks");
147-
fs.mkdirSync(hooksRoot, { recursive: true });
148-
149-
const pkgDir = path.join(hooksRoot, "pkg");
150-
fs.mkdirSync(pkgDir, { recursive: true });
151-
const manifestPath = path.join(pkgDir, "package.json");
152-
fs.writeFileSync(manifestPath, "x".repeat(METADATA_MAX_BYTES + 1), "utf8");
153-
154-
const entries = loadWorkspaceEntriesFromHooksRoot(hooksRoot);
155-
expect(hookNames(entries)).toHaveLength(0);
156-
expect(oversizedMetadataWarnings(manifestPath)).toHaveLength(1);
157-
});
158-
159-
it("warns and skips hooks with an oversized HOOK.md", () => {
160-
const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-oversized-md-"));
161-
const hooksRoot = path.join(root, "hooks");
162-
fs.mkdirSync(hooksRoot, { recursive: true });
163-
164-
const hookDir = path.join(hooksRoot, "big-hook");
165-
fs.mkdirSync(hookDir, { recursive: true });
166-
const hookMdPath = path.join(hookDir, "HOOK.md");
167-
fs.writeFileSync(hookMdPath, "x".repeat(METADATA_MAX_BYTES + 1), "utf8");
168-
fs.writeFileSync(path.join(hookDir, "handler.js"), "export default async () => {};\n");
169-
170-
const entries = loadWorkspaceEntriesFromHooksRoot(hooksRoot);
171-
expect(hookNames(entries)).toHaveLength(0);
172-
expect(oversizedMetadataWarnings(hookMdPath)).toHaveLength(1);
173-
});
174-
175-
it("continues discovering other hooks after skipping oversized metadata", () => {
140+
it("warns, skips oversized metadata, and continues discovering other hooks", () => {
176141
const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-oversized-mixed-"));
177142
const hooksRoot = path.join(root, "hooks");
178143
fs.mkdirSync(hooksRoot, { recursive: true });
179144

180-
const bigHookDir = path.join(hooksRoot, "big-hook");
181-
fs.mkdirSync(bigHookDir, { recursive: true });
182-
const bigHookMdPath = path.join(bigHookDir, "HOOK.md");
183-
fs.writeFileSync(bigHookMdPath, "x".repeat(METADATA_MAX_BYTES + 1), "utf8");
184-
fs.writeFileSync(path.join(bigHookDir, "handler.js"), "export default async () => {};\n");
145+
const packageDir = path.join(hooksRoot, "big-package");
146+
fs.mkdirSync(packageDir);
147+
const manifestPath = path.join(packageDir, "package.json");
148+
fs.writeFileSync(manifestPath, "x".repeat(METADATA_MAX_BYTES + 1));
185149

186-
const smallHookDir = path.join(hooksRoot, "small-hook");
187-
fs.mkdirSync(smallHookDir, { recursive: true });
188-
fs.writeFileSync(path.join(smallHookDir, "HOOK.md"), "---\nname: small-hook\n---\n");
189-
fs.writeFileSync(path.join(smallHookDir, "handler.js"), "export default async () => {};\n");
150+
const bigHookDir = writePlainHook(hooksRoot, "big-hook", "x".repeat(METADATA_MAX_BYTES + 1));
151+
const bigHookMdPath = path.join(bigHookDir, "HOOK.md");
152+
writePlainHook(hooksRoot, "small-hook");
190153

191154
const entries = loadWorkspaceEntriesFromHooksRoot(hooksRoot);
192155
expect(hookNames(entries)).toEqual(["small-hook"]);
156+
expect(oversizedMetadataWarnings(manifestPath)).toHaveLength(1);
193157
expect(oversizedMetadataWarnings(bigHookMdPath)).toHaveLength(1);
194158
});
195159

@@ -221,12 +185,9 @@ describe("hooks workspace", () => {
221185
const hooksRoot = path.join(root, "hooks");
222186
fs.mkdirSync(hooksRoot, { recursive: true });
223187

224-
const hookDir = path.join(hooksRoot, "compat-hook");
225-
fs.mkdirSync(hookDir, { recursive: true });
188+
const hookDir = writePlainHook(hooksRoot, "compat-hook");
226189
const manifestPath = path.join(hookDir, "package.json");
227190
fs.writeFileSync(manifestPath, "x".repeat(METADATA_MAX_BYTES + 1), "utf8");
228-
fs.writeFileSync(path.join(hookDir, "HOOK.md"), "---\nname: compat-hook\n---\n");
229-
fs.writeFileSync(path.join(hookDir, "handler.js"), "export default async () => {};\n");
230191

231192
const entries = loadWorkspaceEntriesFromHooksRoot(hooksRoot);
232193
expect(hookNames(entries)).toContain("compat-hook");

‎src/hooks/workspace.ts‎

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,6 @@ import type { OpenClawConfig } from "../config/types.openclaw.js";
77
import { openRootFileSync } from "../infra/boundary-file-read.js";
88
import { readFileDescriptorBoundedSync } from "../infra/file-descriptor-read.js";
99
import { createSubsystemLogger } from "../logging/subsystem.js";
10-
11-
// Hook package manifests and HOOK.md files are small metadata; cap reads so a
12-
// malicious or runaway file cannot OOM workspace discovery.
13-
const HOOK_PACKAGE_JSON_MAX_BYTES = 1024 * 1024;
14-
const HOOK_MD_MAX_BYTES = 1024 * 1024;
1510
import { isPathInsideWithRealpath } from "../security/scan-paths.js";
1611
import { CONFIG_DIR, resolveUserPath } from "../utils.js";
1712
import { resolveBundledHooksDir } from "./bundled-dir.js";
@@ -24,6 +19,10 @@ import { resolvePluginHookDirs } from "./plugin-hooks.js";
2419
import { resolveHookEntries } from "./policy.js";
2520
import type { Hook, HookEntry, HookSource, ParsedHookFrontmatter } from "./types.js";
2621

22+
// Hook descriptors are small metadata. Bounding the pinned descriptor read also
23+
// covers files that grow after the boundary open validates their identity.
24+
const HOOK_METADATA_MAX_BYTES = 1024 * 1024;
25+
2726
type HookPackageManifest = {
2827
name?: string;
2928
} & Partial<Record<typeof MANIFEST_KEY, { hooks?: string[] }>>;
@@ -40,7 +39,7 @@ function readHookPackageManifest(dir: string): HookPackageManifest | null {
4039
absolutePath: manifestPath,
4140
rootPath: dir,
4241
boundaryLabel: "hook package directory",
43-
maxBytes: HOOK_PACKAGE_JSON_MAX_BYTES,
42+
maxBytes: HOOK_METADATA_MAX_BYTES,
4443
});
4544
if (raw === null) {
4645
return null;
@@ -80,7 +79,7 @@ function loadHookFromDir(params: {
8079
absolutePath: hookMdPath,
8180
rootPath: params.hookDir,
8281
boundaryLabel: "hook directory",
83-
maxBytes: HOOK_MD_MAX_BYTES,
82+
maxBytes: HOOK_METADATA_MAX_BYTES,
8483
});
8584
if (content === null) {
8685
return null;
@@ -305,14 +304,8 @@ function readRootFileUtf8(params: {
305304
}): string | null {
306305
return withOpenedRootFileSync(params, (opened) => {
307306
try {
308-
// Read through the already-validated descriptor so a parent-directory
309-
// symlink swap cannot redirect the read outside the hook root. The shared
310-
// bounded reader owns the byte cap, so a file that grows after the
311-
// open-time checks still cannot be buffered past the limit.
312307
return readFileDescriptorBoundedSync(opened.fd, params.maxBytes).toString("utf-8");
313308
} catch (err) {
314-
// Warn-and-skip contract: one actionable warning per oversized metadata
315-
// file while discovery continues for the remaining hooks.
316309
if (err instanceof RangeError) {
317310
log.warn(
318311
`Ignoring oversized hook metadata ${params.absolutePath}: file exceeds the ${params.maxBytes}-byte limit`,

0 commit comments

Comments
 (0)