🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

1. 🔵 Hook source spoofing allows file-loaded code to register persistent hooks that evade hot-reload clearing

Description

registerInternalHook() accepts an unvalidated string source and casts it to HookRegistrySource at runtime. This allows any code running in-process (including file-discovered internal hook handler modules) to register hooks tagged as "plugin" (or an arbitrary/unknown tag) so they survive clearInternalHooks() on hot reload.

Impact:

• clearInternalHooks() clears only FILE_HOOK_SOURCES (bundled,workspace,managed,config) and intentionally preserves plugin hooks.
• Because registerInternalHook() will accept caller-provided source strings without runtime validation, a file-based hook handler can call registerInternalHook(event, handler, "plugin") (or e.g. "evil").
• Such hooks will not be removed by hook reload (clearInternalHooks()), and unknown tags will also not be cleared by plugin reload (clearHooksBySource(["plugin"])).
• Result: hooks can persist in memory after the originating hook files/config are removed or hot-reloaded, creating an unexpected persistence mechanism.

Vulnerable code:

const registrySource: HookRegistrySource =
  (source ? (FILE_SOURCE_TO_REGISTRY[source] ?? (source as HookRegistrySource)) : undefined) ??
  "config";

Notes on data/control flow:

• src/hooks/loader.ts passes entry.hook.source for discovered hooks (safe, fixed set), but any other caller (including a loaded hook module) can pass arbitrary strings.
• src/hooks/hook-registry.ts enforces HookRegistrySource only at the TypeScript type level; runtime values are stored as-is and clearing is string-match based.

Recommendation

Enforce a strict runtime allowlist for source and prevent file-discovered hooks from ever selecting "plugin" (or arbitrary strings).

Suggested fix (runtime validation + narrower API):

import type { HookSource } from "./types";

const ALLOWED_REGISTRY_SOURCES = new Set<HookRegistrySource>([
  "bundled",
  "workspace",
  "managed",
  "config",
  "plugin",
]);

export function registerInternalHook(
  eventKey: string,
  handler: InternalHookHandler,
  source?: HookSource | HookRegistrySource,
): void {
  const mapped = source && source in FILE_SOURCE_TO_REGISTRY
    ? FILE_SOURCE_TO_REGISTRY[source as HookSource]
    : source;

  const registrySource: HookRegistrySource =
    mapped && ALLOWED_REGISTRY_SOURCES.has(mapped as HookRegistrySource)
      ? (mapped as HookRegistrySource)
      : "config";

  ​// Optional: if called from file loader, explicitly disallow "plugin".
  registerHook(eventKey, handler as (...args: unknown[]) => unknown, { source: registrySource });
}

Additionally:

• Consider splitting APIs: registerFileInternalHook(...) (no plugin source allowed) vs registerPluginInternalHook(...).
• Consider defaulting unknown values to "config" and logging a warning, or throwing to fail fast in dev/test.

Analyzed PR: #30860 at commit b77dc60

_{Last updated on: 2026-03-02T19:04:56Z}

Greptile Summary

This PR successfully unifies two independent hook systems (internal HOOK.md + plugin HookRunner) into a single globalThis[Symbol.for()]-backed registry, fixing 5 systemic bugs that have plagued the codebase for months.

Key Changes:

• Replaced module-level singleton Maps with globalThis[Symbol.for("openclaw:hookRegistry")] to survive bundler chunk splitting
• Added source tagging ("bundled", "workspace", "managed", "config", "plugin") to enable source-scoped clearing
• Implemented unified dispatch helpers (emitMessageReceived, emitMessageSent, emitGatewayStartup) to consolidate dual-dispatch logic
• Fixed sessions.reset to properly fire before_reset hook with full message history
• Fixed after_compaction to include missing sessionKey parameter
• Fixed gateway:startup race condition by calling synchronously after hook loading (removed setTimeout(250))

Architecture:

• The unified registry maintains backward compatibility - both hook facades (registerInternalHook and HookRunner.run*()) continue to work unchanged
• Plugin hooks now survive hot-reload by being tagged with "plugin" source while internal hooks are cleared
• Priority ordering ensures deterministic execution order
• Comprehensive documentation added to AGENTS.md explaining the architecture and rules

Testing:

• 28 new tests added (18 for registry, 10 for dispatch)
• Tests verify source-scoped clearing, priority ordering, metadata isolation, and dual-dispatch behavior
• All 229 existing tests reportedly pass

The refactor is well-architected with clear separation of concerns, defensive programming (metadata cloning), and thorough test coverage.

Confidence Score: 4/5

• Safe to merge with minor validation recommended for production gateway restart scenarios
• Excellent architectural design with comprehensive tests (28 new, 229 passing total), but lacks live gateway validation with real plugin hooks as noted in PR description. The core logic is sound, source-scoped clearing properly preserves plugin hooks, and all stated bugs are fixed. Deducted one point solely due to untested staging environment scenario.
• No files require special attention - the implementation is consistent and well-tested across all changed files

_{Last reviewed commit: 7cd3939}

@greptile please re-review this PR.

Since the initial review (70b9b1d), commit 7cd3939 addresses two issues:

1. clearAllHooks() replaced with clearHooksBySource(["bundled", "workspace", "managed", "config"]) in server-startup.ts — plugin hooks now properly survive startup clearing. clearHooksBySource(["plugin"]) added in server.impl.ts before loadGatewayPlugins() to prevent duplicates on SIGUSR1 restart.

2. sessions.reset before_reset hook is now fire-and-forget (matching commands-core.ts pattern) so slow plugin handlers don't block the RPC response.

Re: Aisle security finding — the unbounded readFile pattern in sessions.reset is identical to the existing code in commands-core.ts:100 which has the same readFile + split("\n") + JSON.parse pattern without size guards. This is pre-existing behavior, not introduced by this PR. Both paths are fire-and-forget and gated behind hookRunner.hasHooks("before_reset"), and sessions.reset is an authenticated admin RPC. Adding a size cap across both code paths would be a reasonable hardening follow-up but is out of scope for this hook unification PR.

@codex review

@codex review

Thanks for the PR! Multiple PRs address the same fix for #30784. Keeping #30853 as the earlier submission. Closing to reduce noise. This is an AI-assisted triage review. If we got this wrong, feel free to reopen — happy to revisit.