From d4f535b20302d415706221d4ddaf747a5626617b Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Thu, 12 Mar 2026 20:47:30 -0400 Subject: [PATCH] fix(hooks): fail closed on unreadable loader paths (#44437) * Hooks: fail closed on unreadable loader paths * Changelog: note hooks loader hardening * Tests: cover sanitized hook loader logs * Hooks: use realpath containment for legacy loaders * Hooks: sanitize unreadable workspace log path --- CHANGELOG.md | 1 + src/hooks/loader.test.ts | 34 +++++++++++++++- src/hooks/loader.ts | 83 +++++++++++++++++++++++++++++++--------- 3 files changed, 99 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a595786e..376261c05 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,7 @@ Docs: https://docs.openclaw.ai - Windows/native update: make package installs use the npm update path instead of the git path, carry portable Git into native Windows updates, and mirror the installer's Windows npm env so `openclaw update` no longer dies early on missing `git` or `node-llama-cpp` download setup. - Sandbox/write: preserve pinned mutation-helper payload stdin so sandboxed `write` no longer reports success while creating empty files. (#43876) Thanks @glitch418x. - Security/exec approvals: escape invisible Unicode format characters in approval prompts so zero-width command text renders as visible `\u{...}` escapes instead of spoofing the reviewed command. (`GHSA-pcqg-f7rg-xfvv`)(#43687) Thanks @EkiXu and @vincentkoc. +- Hooks/loader: fail closed when workspace hook paths cannot be resolved with `realpath`, so unreadable or broken internal hook paths are skipped instead of falling back to unresolved imports. (#44437) Thanks @vincentkoc. - Hooks/agent deliveries: dedupe repeated hook requests by optional idempotency key so webhook retries can reuse the first run instead of launching duplicate agent executions. (#44438) Thanks @vincentkoc. - Security/exec detection: normalize compatibility Unicode and strip invisible formatting code points before obfuscation checks so zero-width and fullwidth command tricks no longer suppress heuristic detection. (`GHSA-9r3v-37xh-2cf6`)(#44091) Thanks @wooluo and @vincentkoc. - Security/exec allowlist: preserve POSIX case sensitivity and keep `?` within a single path segment so exact-looking allowlist patterns no longer overmatch executables across case or directory boundaries. (`GHSA-f8r2-vg7x-gh8m`)(#43798) Thanks @zpbrent and @vincentkoc. diff --git a/src/hooks/loader.test.ts b/src/hooks/loader.test.ts index a6618ab70..c1d71106d 100644 --- a/src/hooks/loader.test.ts +++ b/src/hooks/loader.test.ts @@ -1,8 +1,10 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from "vitest"; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "../config/config.js"; +import { setLoggerOverride } from "../logging/logger.js"; +import { loggingState } from "../logging/state.js"; import { captureEnv } from "../test-utils/env.js"; import { clearInternalHooks, @@ -31,6 +33,13 @@ describe("loader", () => { // Disable bundled hooks during tests by setting env var to non-existent directory envSnapshot = captureEnv(["OPENCLAW_BUNDLED_HOOKS_DIR"]); process.env.OPENCLAW_BUNDLED_HOOKS_DIR = "/nonexistent/bundled/hooks"; + setLoggerOverride({ level: "silent", consoleLevel: "error" }); + loggingState.rawConsole = { + log: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + }; }); async function writeHandlerModule( @@ -54,6 +63,8 @@ describe("loader", () => { afterEach(async () => { clearInternalHooks(); + loggingState.rawConsole = null; + setLoggerOverride(null); envSnapshot.restore(); }); @@ -336,5 +347,26 @@ describe("loader", () => { await expectNoCommandHookRegistration(createLegacyHandlerConfig()); }); + + it("sanitizes control characters in loader error logs", async () => { + const error = loggingState.rawConsole?.error; + expect(error).toBeTypeOf("function"); + + const cfg = createEnabledHooksConfig([ + { + event: "command:new", + module: `${tmpDir}\u001b[31m\nforged-log`, + }, + ]); + + await expectNoCommandHookRegistration(cfg); + + const messages = (error as ReturnType).mock.calls + .map((call) => String(call[0] ?? "")) + .join("\n"); + expect(messages).toContain("forged-log"); + expect(messages).not.toContain("\u001b[31m"); + expect(messages).not.toContain("\nforged-log"); + }); }); }); diff --git a/src/hooks/loader.ts b/src/hooks/loader.ts index 4a1fb9646..10dd8214a 100644 --- a/src/hooks/loader.ts +++ b/src/hooks/loader.ts @@ -10,6 +10,7 @@ import path from "node:path"; import type { OpenClawConfig } from "../config/config.js"; import { openBoundaryFile } from "../infra/boundary-file-read.js"; import { createSubsystemLogger } from "../logging/subsystem.js"; +import { sanitizeForLog } from "../terminal/ansi.js"; import { resolveHookConfig } from "./config.js"; import { shouldIncludeHook } from "./config.js"; import { buildImportUrl } from "./import-url.js"; @@ -20,6 +21,24 @@ import { loadWorkspaceHookEntries } from "./workspace.js"; const log = createSubsystemLogger("hooks:loader"); +function safeLogValue(value: string): string { + return sanitizeForLog(value); +} + +function maybeWarnTrustedHookSource(source: string): void { + if (source === "openclaw-workspace") { + log.warn( + "Loading workspace hook code into the gateway process. Workspace hooks are trusted local code.", + ); + return; + } + if (source === "openclaw-managed") { + log.warn( + "Loading managed hook code into the gateway process. Managed hooks are trusted local code.", + ); + } +} + /** * Load and register all hook handlers * @@ -74,7 +93,13 @@ export async function loadInternalHooks( } try { - const hookBaseDir = safeRealpathOrResolve(entry.hook.baseDir); + const hookBaseDir = resolveExistingRealpath(entry.hook.baseDir); + if (!hookBaseDir) { + log.error( + `Hook '${safeLogValue(entry.hook.name)}' base directory is no longer readable: ${safeLogValue(entry.hook.baseDir)}`, + ); + continue; + } const opened = await openBoundaryFile({ absolutePath: entry.hook.handlerPath, rootPath: hookBaseDir, @@ -82,12 +107,13 @@ export async function loadInternalHooks( }); if (!opened.ok) { log.error( - `Hook '${entry.hook.name}' handler path fails boundary checks: ${entry.hook.handlerPath}`, + `Hook '${safeLogValue(entry.hook.name)}' handler path fails boundary checks: ${safeLogValue(entry.hook.handlerPath)}`, ); continue; } const safeHandlerPath = opened.path; fs.closeSync(opened.fd); + maybeWarnTrustedHookSource(entry.hook.source); // Import handler module — only cache-bust mutable (workspace/managed) hooks const importUrl = buildImportUrl(safeHandlerPath, entry.hook.source); @@ -101,14 +127,16 @@ export async function loadInternalHooks( }); if (!handler) { - log.error(`Handler '${exportName}' from ${entry.hook.name} is not a function`); + log.error( + `Handler '${safeLogValue(exportName)}' from ${safeLogValue(entry.hook.name)} is not a function`, + ); continue; } // Register for all events listed in metadata const events = entry.metadata?.events ?? []; if (events.length === 0) { - log.warn(`Hook '${entry.hook.name}' has no events defined in metadata`); + log.warn(`Hook '${safeLogValue(entry.hook.name)}' has no events defined in metadata`); continue; } @@ -117,18 +145,18 @@ export async function loadInternalHooks( } log.info( - `Registered hook: ${entry.hook.name} -> ${events.join(", ")}${exportName !== "default" ? ` (export: ${exportName})` : ""}`, + `Registered hook: ${safeLogValue(entry.hook.name)} -> ${events.map((event) => safeLogValue(event)).join(", ")}${exportName !== "default" ? ` (export: ${safeLogValue(exportName)})` : ""}`, ); loadedCount++; } catch (err) { log.error( - `Failed to load hook ${entry.hook.name}: ${err instanceof Error ? err.message : String(err)}`, + `Failed to load hook ${safeLogValue(entry.hook.name)}: ${safeLogValue(err instanceof Error ? err.message : String(err))}`, ); } } } catch (err) { log.error( - `Failed to load directory-based hooks: ${err instanceof Error ? err.message : String(err)}`, + `Failed to load directory-based hooks: ${safeLogValue(err instanceof Error ? err.message : String(err))}`, ); } @@ -144,17 +172,29 @@ export async function loadInternalHooks( } if (path.isAbsolute(rawModule)) { log.error( - `Handler module path must be workspace-relative (got absolute path): ${rawModule}`, + `Handler module path must be workspace-relative (got absolute path): ${safeLogValue(rawModule)}`, ); continue; } const baseDir = path.resolve(workspaceDir); const modulePath = path.resolve(baseDir, rawModule); - const baseDirReal = safeRealpathOrResolve(baseDir); - const modulePathSafe = safeRealpathOrResolve(modulePath); - const rel = path.relative(baseDir, modulePath); + const baseDirReal = resolveExistingRealpath(baseDir); + if (!baseDirReal) { + log.error( + `Workspace directory is no longer readable while loading hooks: ${safeLogValue(baseDir)}`, + ); + continue; + } + const modulePathSafe = resolveExistingRealpath(modulePath); + if (!modulePathSafe) { + log.error( + `Handler module path could not be resolved with realpath: ${safeLogValue(rawModule)}`, + ); + continue; + } + const rel = path.relative(baseDirReal, modulePathSafe); if (!rel || rel.startsWith("..") || path.isAbsolute(rel)) { - log.error(`Handler module path must stay within workspaceDir: ${rawModule}`); + log.error(`Handler module path must stay within workspaceDir: ${safeLogValue(rawModule)}`); continue; } const opened = await openBoundaryFile({ @@ -163,11 +203,16 @@ export async function loadInternalHooks( boundaryLabel: "workspace directory", }); if (!opened.ok) { - log.error(`Handler module path fails boundary checks under workspaceDir: ${rawModule}`); + log.error( + `Handler module path fails boundary checks under workspaceDir: ${safeLogValue(rawModule)}`, + ); continue; } const safeModulePath = opened.path; fs.closeSync(opened.fd); + log.warn( + `Loading legacy internal hook module from workspace path ${safeLogValue(rawModule)}. Legacy hook modules are trusted local code.`, + ); // Legacy handlers are always workspace-relative, so use mtime-based cache busting const importUrl = buildImportUrl(safeModulePath, "openclaw-workspace"); @@ -181,18 +226,20 @@ export async function loadInternalHooks( }); if (!handler) { - log.error(`Handler '${exportName}' from ${modulePath} is not a function`); + log.error( + `Handler '${safeLogValue(exportName)}' from ${safeLogValue(modulePath)} is not a function`, + ); continue; } registerInternalHook(handlerConfig.event, handler); log.info( - `Registered hook (legacy): ${handlerConfig.event} -> ${modulePath}${exportName !== "default" ? `#${exportName}` : ""}`, + `Registered hook (legacy): ${safeLogValue(handlerConfig.event)} -> ${safeLogValue(modulePath)}${exportName !== "default" ? `#${safeLogValue(exportName)}` : ""}`, ); loadedCount++; } catch (err) { log.error( - `Failed to load hook handler from ${handlerConfig.module}: ${err instanceof Error ? err.message : String(err)}`, + `Failed to load hook handler from ${safeLogValue(handlerConfig.module)}: ${safeLogValue(err instanceof Error ? err.message : String(err))}`, ); } } @@ -200,10 +247,10 @@ export async function loadInternalHooks( return loadedCount; } -function safeRealpathOrResolve(value: string): string { +function resolveExistingRealpath(value: string): string | null { try { return fs.realpathSync(value); } catch { - return path.resolve(value); + return null; } }