🔒 Aisle Security Analysis

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

1. 🟡 Event-loop blocking / DoS via synchronous unbounded transcript read in sessions.reset before_reset hook

Description

In sessionsHandlers.reset, the new before_reset plugin hook implementation reads the entire session transcript synchronously and then parses it line-by-line.

• Input influence: sessionFile is taken from the persisted session store (entry.sessionFile). While it is not directly user-supplied via the API, it is still data-driven (persisted metadata) and can point to a very large transcript file in normal operation.
• DoS impact: fs.readFileSync(sessionFile, "utf-8") blocks the Node.js event loop for the duration of the read and allocates memory for the full file contents. A large transcript (or repeated resets) can significantly delay or stall the gateway process.
• Amplification: after building messages, the handler then awaits hookRunner.runBeforeReset(...). Plugin handlers are executed in parallel but can still be slow; this directly extends the latency of the reset endpoint.

Vulnerable code:

const fileContent = fs.readFileSync(sessionFile, "utf-8");
for (const line of fileContent.split("\n")) {
  ...
  const parsed = JSON.parse(line);
  ...
}
await hookRunner.runBeforeReset({ sessionFile, messages, reason: commandReason }, ...)

Even though sessions.reset is an admin-scoped RPC, this is still a concrete availability risk: any authenticated admin client (or an automated integration) can trigger it against sessions with large transcripts, blocking the entire gateway event loop during synchronous I/O and JSON parsing.

Recommendation

Avoid synchronous, unbounded reads in request handlers.

1. Limit size before reading (fail closed / degrade gracefully):
   • statSync(sessionFile).size and cap to a reasonable maximum (e.g. 1–5MB) for this hook.
2. Use streaming async reads and stop after N bytes/lines/messages.
3. Add an execution deadline around plugin hooks so reset cannot be stalled indefinitely.
4. Optionally harden path handling by re-resolving sessionFile via resolveSessionFilePath(...) or using a safe open helper with O_NOFOLLOW to avoid unexpected symlink/path behavior if persisted metadata is corrupt.

Example (stream + caps):

import { createReadStream } from "node:fs";
import readline from "node:readline";

const MAX_BYTES = 5 * 1024 * 1024;
const MAX_MESSAGES = 2000;

const stat = await fs.promises.stat(sessionFile);
if (stat.size > MAX_BYTES) {
  ​// either skip, or only read tail/head depending on needs
}

const rl = readline.createInterface({
  input: createReadStream(sessionFile, { encoding: "utf-8" }),
  crlfDelay: Infinity,
});

const messages: unknown[] = [];
for await (const line of rl) {
  if (messages.length >= MAX_MESSAGES) break;
  if (!line.trim()) continue;
  try {
    const parsed = JSON.parse(line);
    if (parsed?.type === "message" && parsed?.message) messages.push(parsed.message);
  } catch {}
}

await Promise.race([
  hookRunner.runBeforeReset({ sessionFile, messages, reason }, ctx),
  new Promise((_, reject) => setTimeout(() => reject(new Error("before_reset timeout")), 2000)),
]);

2. 🟡 Persistent plugin internal hooks survive in-process restarts due to globalThis hook registry (Symbol.for), bypassing config/plugin disable and causing handler accumulation

Description

The internal hook registry was moved to a globalThis singleton keyed by Symbol.for("openclaw:hookRegistry"). During gateway startup, hooks are cleared by source excluding the plugin source:

• Plugin hook handlers registered in a previous lifecycle remain in memory across SIGUSR1 in-process restarts (the gateway run-loop restarts the server in the same Node process).
• If plugins are disabled/removed or hooks.internal.enabled is turned off and the gateway restarts in-process, previously-registered plugin internal hooks can still fire because they were not cleared.
• If plugin configuration changes and plugins re-register, old plugin handlers are retained, leading to duplicate executions and unbounded growth of handler arrays.

This creates a security-relevant integrity issue (configuration meant to disable hooks/plugins may not actually stop their code from executing) and a potential DoS vector via handler accumulation.

Vulnerable code (startup clears everything except plugin hooks):

​// Clear file-discovered hooks but preserve plugin-registered hooks (#30784)
clearInternalHooksBySource(["bundled", "workspace", "managed", "config"]);
const loadedCount = await loadInternalHooks(params.cfg, params.defaultWorkspaceDir);

Because the registry itself persists on globalThis, the preserved plugin handlers can continue to run on future triggerInternalHook(...) calls even when no longer desired.

Recommendation

Ensure plugin internal hooks cannot outlive the lifecycle/configuration that enabled them.

Recommended options:

1. Clear plugin hooks on every gateway (re)startup iteration, then re-register from the currently loaded plugin registry:

​// On startup/restart, clear all sources including plugin
clearInternalHooksBySource(["bundled","workspace","managed","config","plugin"]);
await loadInternalHooks(cfg, workspaceDir);
​// plugins load should then register their hooks for this lifecycle

2. Track plugin registrations with identifiers and support targeted cleanup:

• Extend HookRegistryEntry with pluginId (or a lifecycleToken).
• On plugin reload/disable, call clearInternalHooksBySource(["plugin"]) or clearInternalHooksByPluginId(pluginId).

3. If the intent is only to dedupe module instances (not persist across restarts), store the registry in module scope and explicitly clear it during SIGUSR1 restart handling.

Add regression tests:

• start with plugin hooks enabled; restart with hooks disabled; assert plugin internal hook handler does not execute.
• change plugin set across restarts; assert no duplicate executions and no stale handlers.

Analyzed PR: #30853 at commit 9751562

_{Last updated on: 2026-03-01T19:42:24Z}

Greptile Summary

unified internal and plugin hook registries to resolve 5 systemic bugs related to hook lifecycle management

Key improvements:

• Bundler-safe singletons: both internal-hooks.ts and hook-runner-global.ts now use globalThis[Symbol.for()] pattern, ensuring hooks survive module bundling and chunk splitting
• Source-scoped hook clearing: hooks are tagged with source metadata (bundled, workspace, managed, config, plugin), enabling selective clearing during hot-reload while preserving plugin hooks
• Unified dispatch helpers: new dispatch-unified.ts consolidates dual-dispatch logic for message_received, message_sent, and gateway_startup events
• Race condition fixes: removed setTimeout(250) delay for gateway:startup, both internal and plugin hooks now fire synchronously after sidecars load; sessions.reset now reads session file before calling before_reset hook and awaits completion before archival
• API completeness: after_compaction hook now receives sessionKey in context, matching before_compaction

Technical implementation:

• Backward compatible: registerInternalHook accepts optional source parameter (defaults to config)
• Proper error handling: plugin hooks log failures via logVerbose, internal hooks log via triggerInternalHook
• Correct source mapping: filesystem-discovered plugin hooks map to workspace (cleared on reload), programmatic plugin hooks use plugin source (preserved on reload)

Confidence Score: 4/5

• safe to merge after the race condition and mapping issues were addressed in follow-up commits
• the initial PR introduced several issues (race conditions, incorrect source mapping, missing error logging) that were caught and fixed in subsequent commits 2e7972d, d7d638f, and 9751562. the current code correctly implements bundler-safe singletons, source-scoped clearing, and proper sequencing to prevent race conditions. the changes are backward compatible and well-tested based on the test plan
• all identified issues in dispatch-unified.ts, loader.ts, and sessions.ts have been resolved in follow-up commits

_{Last reviewed commit: 9751562}

@greptile-apps please review

@greptile-apps please review

@greptile review

Addressed all three findings from the previous review:

1. resolveHookSource mapping — "openclaw-plugin" now maps to "workspace" (was "plugin"), fixing duplicate hooks on hot-reload
2. Silent error swallowing — Added logVerbose to all plugin hook .catch() handlers in dispatch-unified.ts
3. Race condition in sessions.ts — before_reset hook is now awaited before archiveSessionTranscriptsForSession renames the file

Hey, nice work tackling the root cause here — the 5 bugs all tracing back to the singleton duplication + clearing issue is a good catch. A few things I think would strengthen this before merge:

Tests

This PR introduces clearInternalHooksBySource, resolveHookSource, three new dispatch helpers, and the globalThis singleton pattern — but adds no tests for any of them. The source-scoped clearing logic is the core behavioral change and should have at least:

• A test proving plugin-source hooks survive clearInternalHooksBySource(["bundled", "workspace", "managed", "config"])
• A test proving the Symbol.for singleton survives simulated module duplication (the whole point of the refactor)
• Tests for the dispatch helpers (null hookRunner, missing sessionKey, error propagation)
• A test for resolveHookSource mapping correctness

Registry separation

Right now the registry state, source-scoped clearing, and hook lookup are all embedded inside internal-hooks.ts. Extracting the registry into its own module with a clean API (registerHook, unregisterHook, clearHooksBySource, getHookEntries, hasHooks, etc.) would make it independently testable and keep internal-hooks.ts focused on hook-specific dispatch logic. It also makes it reusable if a third hook source appears down the line.

Inline source array

The ["bundled", "workspace", "managed", "config"] list is used as a raw array literal at the call site. A named constant (e.g., FILE_HOOK_SOURCES) would be self-documenting and prevent drift if another file-based source is added later.

Plugin loader cache key

buildCacheKey() in src/plugins/loader.ts doesn't include hooks.internal.enabled. If someone toggles that config flag and does a SIGUSR1 restart, the cache hits and stale plugin-source internal hooks persist. Worth adding hooksEnabled to the cache key.

fs.readFileSync in the RPC handler

The before_reset path in sessions.ts uses fs.readFileSync to read session transcripts. These can be large and this blocks the event loop in the gateway RPC handler. Consider using an async read — the same JSONL parsing pattern already exists in commands-core.ts, so this is a good opportunity to extract a shared async helper (e.g., readSessionMessagesAsync in session-utils.fs.ts) and deduplicate both call sites.

before_reset ordering concern

The hook fires before ensureSessionRuntimeCleanup. If cleanup fails (e.g., waitForEmbeddedPiRunEnd times out), the RPC returns UNAVAILABLE and no reset actually happens — but the plugin hook already ran. Depending on what plugins do in before_reset (e.g., persisting memories), this could cause side-effects for a reset that never occurred.

Missing workspaceDir in before_reset context

The gateway RPC path omits workspaceDir from the before_reset hook context, but the matching commands-core.ts path includes it. This creates inconsistent behavior for plugin hooks across the two reset entry points.

Metadata mutation isolation

In the dispatch helpers, the plugin hook and internal hook paths should receive separate object allocations for metadata/context. Currently emitMessageReceived passes params.metadata ?? {} directly to the internal hook path without cloning — if a plugin hook mutates the metadata object, the internal hook sees the mutation (and vice versa).

Error logging consistency

The internal hook .catch() handlers went through a few iterations across commits (silent () => {} → logVerbose). Worth a pass to make sure all fire-and-forget paths consistently use logVerbose for observability. Silent swallowing makes hook failures invisible in debug logs.

AGENTS.md documentation

The hook system now has meaningful architectural invariants (globalThis storage, source-scoped clearing semantics, which sources get cleared on hot-reload, why plugin clearing lives in loadOpenClawPlugins on the non-cached path). Adding a "Hook System Architecture" section to AGENTS.md would help future contributors avoid reintroducing the same bugs.

You can use #30860 as reference.

Closing — significant merge conflicts after 12 days. The hooks system has evolved substantially. Will re-evaluate if still needed.