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

@@ -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.

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<typeof vi.fn>).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");
+    });
   });
 });

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;
   }
 }