fix(hooks): enforce default hook agent allowlist

eleqtrizit · web-flow · commit e72621e5668e · 2026-05-27T05:05:18.000+01:00
Enforce hook allowedAgentIds against the effective default agent when hook payloads omit or blank agentId, while preserving omitted-agent dispatch semantics for default/global routing.

Also updates the affected generated hook config docs from the contributor change and fixes the current-main memory-core test mock after rebasing the PR branch.

Verification:
- pnpm format:check extensions/memory-core/src/dreaming.test.ts src/gateway/hooks.ts src/gateway/hooks.test.ts src/gateway/server/hooks-request-handler.ts src/gateway/server.hooks.test.ts &amp;&amp; git diff --check
- node scripts/run-vitest.mjs run --config test/vitest/vitest.gateway-server.config.ts src/gateway/hooks.test.ts src/gateway/server.hooks.test.ts --reporter=dot --pool=forks --no-file-parallelism --testTimeout=120000
- node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.extensions.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/extensions-test-local-pr87124.tsbuildinfo
- pnpm check:test-types
- .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main
- GitHub PR merge state CLEAN; CodeQL Critical Quality rerun succeeded after first runner checkout wedged

Co-authored-by: Agustin Rivera &lt;agustin@rivera-web.com&gt;
diff --git a/docs/.generated/config-baseline.sha256 b/docs/.generated/config-baseline.sha256
@@ -1,4 +1,4 @@
-bd43ed4d839351337f16e151572b2eb22d62eabd007cab0d43feab355ef36dcd  config-baseline.json
-cd9a23799e2c04158253a60b79afd9c098db1ad615aab56eeebbed28e492ce14  config-baseline.core.json
+53b7621e99d75b98ecc8f4389d38900f84cf213f95dbcc877f36125d763c660d  config-baseline.json
+e92bbf45714e418383118098d4ff15d347fa8ffc7e7837b437b522d2b59ce9fe  config-baseline.core.json
 b901fb766edfd9df630690281476fc4032c64772f69d1d8f7b2e0e913a90f229  config-baseline.channel.json
 5c214ab364011fd95735755f9fa4298aa4de8ad81144ae8dd08d969bb7ba318b  config-baseline.plugin.json
diff --git a/docs/automation/cron-jobs.md b/docs/automation/cron-jobs.md
@@ -306,7 +306,7 @@ Keep hook endpoints behind loopback, tailnet, or trusted reverse proxy.
 
 - Use a dedicated hook token; do not reuse gateway auth tokens.
 - Keep `hooks.path` on a dedicated subpath; `/` is rejected.
-- Set `hooks.allowedAgentIds` to limit explicit `agentId` routing.
+- Set `hooks.allowedAgentIds` to limit which effective agent a hook can target, including the default agent when `agentId` is omitted.
 - Keep `hooks.allowRequestSessionKey=false` unless you require caller-selected sessions.
 - If you enable `hooks.allowRequestSessionKey`, also set `hooks.allowedSessionKeyPrefixes` to constrain allowed session key shapes.
 - Hook payloads are wrapped with safety boundaries by default.
diff --git a/docs/gateway/configuration-reference.md b/docs/gateway/configuration-reference.md
@@ -709,8 +709,8 @@ Validation and safety notes:
 - `transform` can point to a JS/TS module returning a hook action.
   - `transform.module` must be a relative path and stays within `hooks.transformsDir` (absolute paths and traversal are rejected).
   - Keep `hooks.transformsDir` under `~/.openclaw/hooks/transforms`; workspace skill directories are rejected. If `openclaw doctor` reports this path as invalid, move the transform module into the hooks transforms directory or remove `hooks.transformsDir`.
-- `agentId` routes to a specific agent; unknown IDs fall back to default.
-- `allowedAgentIds`: restricts explicit routing (`*` or omitted = allow all, `[]` = deny all).
+- `agentId` routes to a specific agent; unknown IDs fall back to the default agent.
+- `allowedAgentIds`: restricts effective agent routing, including the default-agent path when `agentId` is omitted (`*` or omitted = allow all, `[]` = deny all).
 - `defaultSessionKey`: optional fixed session key for hook agent runs without explicit `sessionKey`.
 - `allowRequestSessionKey`: allow `/hooks/agent` callers and template-driven mapping session keys to set `sessionKey` (default: `false`).
 - `allowedSessionKeyPrefixes`: optional prefix allowlist for explicit `sessionKey` values (request + mapping), e.g. `["hook:"]`. It becomes required when any mapping or preset uses a templated `sessionKey`.
diff --git a/extensions/memory-core/src/dreaming.test.ts b/extensions/memory-core/src/dreaming.test.ts
@@ -2267,11 +2267,15 @@ describe("short-term dreaming trigger", () => {
       ],
     });
 
+    const narrativeMessages = [{ role: "assistant", content: "A diary entry." }];
     const subagent = {
       run: vi.fn(async (_params: { model?: string }) => ({ runId: "narrative-run-1" })),
-      waitForRun: vi.fn(async () => ({ status: "ok" })),
+      waitForRun: vi.fn(async () => ({ status: "ok" as const })),
       getSessionMessages: vi.fn(async () => ({
-        messages: [{ role: "assistant", content: "A diary entry." }],
+        messages: narrativeMessages,
+      })),
+      getSession: vi.fn(async () => ({
+        messages: narrativeMessages,
       })),
       deleteSession: vi.fn(async () => {}),
     };
diff --git a/src/config/schema.help.ts b/src/config/schema.help.ts
@@ -1743,7 +1743,7 @@ export const FIELD_HELP: Record<string, string> = {
   "hooks.allowedSessionKeyPrefixes":
     "Allowlist of accepted session-key prefixes for inbound hook requests when caller-provided keys are enabled. Use narrow prefixes to prevent arbitrary session-key injection.",
   "hooks.allowedAgentIds":
-    "Allowlist of agent IDs that hook mappings are allowed to target when selecting execution agents. Use this to constrain automation events to dedicated service agents and reduce blast radius if a hook token is exposed.",
+    "Allowlist of effective agent IDs that hook requests and mappings are allowed to target, including default-agent routing when agentId is omitted. Use this to constrain automation events to dedicated service agents and reduce blast radius if a hook token is exposed.",
   "hooks.maxBodyBytes":
     "Maximum accepted webhook payload size in bytes before the request is rejected. Keep this bounded to reduce abuse risk and protect memory usage under bursty integrations.",
   "hooks.presets":
diff --git a/src/config/types.hooks.ts b/src/config/types.hooks.ts
@@ -110,8 +110,9 @@ export type HooksConfig = {
    */
   allowedSessionKeyPrefixes?: string[];
   /**
-   * Restrict explicit hook `agentId` routing to these agent ids.
-   * Omit or include `*` to allow any agent. Set `[]` to deny all explicit `agentId` routing.
+   * Restrict hook execution to these effective agent ids, including
+   * default-agent routing when `agentId` is omitted. Omit or include `*` to
+   * allow any agent. Set `[]` to deny all agent routing.
    */
   allowedAgentIds?: string[];
   maxBodyBytes?: number;
diff --git a/src/gateway/hooks.test.ts b/src/gateway/hooks.test.ts
@@ -7,6 +7,7 @@ import {
   extractHookToken,
   isHookAgentAllowed,
   normalizeHookDispatchSessionKey,
+  resolveEffectiveHookTargetAgentId,
   resolveHookSessionKey,
   resolveHookTargetAgentId,
   normalizeAgentPayload,
@@ -181,7 +182,7 @@ describe("gateway hooks helpers", () => {
     }
   });
 
-  test("resolveHookTargetAgentId falls back to default for unknown agent ids", () => {
+  test("resolveHookTargetAgentId preserves omitted default target intent", () => {
     const cfg = {
       hooks: { enabled: true, token: "secret" },
       agents: {
@@ -192,25 +193,41 @@ describe("gateway hooks helpers", () => {
     expect(resolveHookTargetAgentId(resolved, "hooks")).toBe("hooks");
     expect(resolveHookTargetAgentId(resolved, "missing-agent")).toBe("main");
     expect(resolveHookTargetAgentId(resolved, undefined)).toBeUndefined();
+    expect(resolveHookTargetAgentId(resolved, " ")).toBeUndefined();
+    expect(resolveEffectiveHookTargetAgentId(resolved, undefined)).toBe("main");
+    expect(resolveEffectiveHookTargetAgentId(resolved, " ")).toBe("main");
   });
 
-  test("isHookAgentAllowed honors hooks.allowedAgentIds for explicit routing", () => {
+  test("isHookAgentAllowed honors hooks.allowedAgentIds for effective target routing", () => {
     const resolved = resolveHooksConfigOrThrow(buildHookAgentConfig(["hooks"]));
-    expect(isHookAgentAllowed(resolved, undefined)).toBe(true);
+    expect(isHookAgentAllowed(resolved, undefined)).toBe(false);
+    expect(isHookAgentAllowed(resolved, "")).toBe(false);
+    expect(isHookAgentAllowed(resolved, "   ")).toBe(false);
     expect(isHookAgentAllowed(resolved, "hooks")).toBe(true);
     expect(isHookAgentAllowed(resolved, "missing-agent")).toBe(false);
   });
 
-  test("isHookAgentAllowed treats empty allowlist as deny-all for explicit agentId", () => {
+  test("isHookAgentAllowed treats empty allowlist as deny-all routing", () => {
     const resolved = resolveHooksConfigOrThrow(buildHookAgentConfig([]));
-    expect(isHookAgentAllowed(resolved, undefined)).toBe(true);
+    expect(isHookAgentAllowed(resolved, undefined)).toBe(false);
+    expect(isHookAgentAllowed(resolved, "")).toBe(false);
     expect(isHookAgentAllowed(resolved, "hooks")).toBe(false);
     expect(isHookAgentAllowed(resolved, "main")).toBe(false);
   });
 
+  test("isHookAgentAllowed allows omitted agentId when default agent is allowlisted", () => {
+    const resolved = resolveHooksConfigOrThrow(buildHookAgentConfig(["main"]));
+    expect(isHookAgentAllowed(resolved, undefined)).toBe(true);
+    expect(isHookAgentAllowed(resolved, "")).toBe(true);
+    expect(isHookAgentAllowed(resolved, "hooks")).toBe(false);
+    expect(isHookAgentAllowed(resolved, "main")).toBe(true);
+    expect(isHookAgentAllowed(resolved, "missing-agent")).toBe(true);
+  });
+
   test("isHookAgentAllowed treats wildcard allowlist as allow-all", () => {
     const resolved = resolveHooksConfigOrThrow(buildHookAgentConfig(["*"]));
     expect(isHookAgentAllowed(resolved, undefined)).toBe(true);
+    expect(isHookAgentAllowed(resolved, "")).toBe(true);
     expect(isHookAgentAllowed(resolved, "hooks")).toBe(true);
     expect(isHookAgentAllowed(resolved, "missing-agent")).toBe(true);
   });
diff --git a/src/gateway/hooks.ts b/src/gateway/hooks.ts
@@ -294,21 +294,24 @@ export function resolveHookTargetAgentId(
   return hooksConfig.agentPolicy.defaultAgentId;
 }
 
+export function resolveEffectiveHookTargetAgentId(
+  hooksConfig: HooksConfigResolved,
+  agentId: string | undefined,
+): string {
+  return resolveHookTargetAgentId(hooksConfig, agentId) ?? hooksConfig.agentPolicy.defaultAgentId;
+}
+
 export function isHookAgentAllowed(
   hooksConfig: HooksConfigResolved,
   agentId: string | undefined,
 ): boolean {
-  // Keep backwards compatibility for callers that omit agentId.
-  const raw = normalizeOptionalString(agentId);
-  if (!raw) {
-    return true;
-  }
   const allowed = hooksConfig.agentPolicy.allowedAgentIds;
   if (allowed === undefined) {
     return true;
   }
-  const resolved = resolveHookTargetAgentId(hooksConfig, raw);
-  return resolved ? allowed.has(resolved) : false;
+  // Omitted agentId still dispatches to the default agent downstream, so the
+  // allowlist must authorize that effective target before dispatch.
+  return allowed.has(resolveEffectiveHookTargetAgentId(hooksConfig, agentId));
 }
 
 export const getHookAgentPolicyError = () => "agentId is not allowed by hooks.allowedAgentIds";
diff --git a/src/gateway/server.hooks.test.ts b/src/gateway/server.hooks.test.ts
@@ -790,12 +790,17 @@ describe("gateway server hooks", () => {
     });
   });
 
-  test("enforces hooks.allowedAgentIds for explicit agent routing", async () => {
+  test("enforces hooks.allowedAgentIds for effective agent routing", async () => {
     testState.hooksConfig = {
       enabled: true,
       token: HOOK_TOKEN,
       allowedAgentIds: ["hooks"],
       mappings: [
+        {
+          match: { path: "mapped-default" },
+          action: "agent",
+          messageTemplate: "Mapped default: {{payload.subject}}",
+        },
         {
           match: { path: "mapped" },
           action: "agent",
@@ -806,13 +811,21 @@ describe("gateway server hooks", () => {
     };
     setMainAndHooksAgents();
     await withGatewayServer(async ({ port }) => {
-      mockIsolatedRunOkOnce();
       const resNoAgent = await postHook(port, "/hooks/agent", { message: "No explicit agent" });
-      expect(resNoAgent.status).toBe(200);
-      await waitForSystemEvent();
-      const noAgentCall = cronRunCall();
-      expect(noAgentCall?.job?.agentId).toBeUndefined();
-      drainSystemEvents(resolveMainKey());
+      expect(resNoAgent.status).toBe(400);
+      const noAgentBody = (await resNoAgent.json()) as { error?: string };
+      expect(noAgentBody.error).toContain("hooks.allowedAgentIds");
+      expect(cronIsolatedRun).not.toHaveBeenCalled();
+      expect(peekSystemEvents(resolveMainKey()).length).toBe(0);
+
+      const resEmptyAgent = await postHook(port, "/hooks/agent", {
+        message: "Empty agent",
+        agentId: " ",
+      });
+      expect(resEmptyAgent.status).toBe(400);
+      const emptyAgentBody = (await resEmptyAgent.json()) as { error?: string };
+      expect(emptyAgentBody.error).toContain("hooks.allowedAgentIds");
+      expect(cronIsolatedRun).not.toHaveBeenCalled();
 
       mockIsolatedRunOkOnce();
       const resAllowed = await postHook(port, "/hooks/agent", {
@@ -833,6 +846,13 @@ describe("gateway server hooks", () => {
       const deniedBody = (await resDenied.json()) as { error?: string };
       expect(deniedBody.error).toContain("hooks.allowedAgentIds");
 
+      const resMappedDefaultDenied = await postHook(port, "/hooks/mapped-default", {
+        subject: "hello",
+      });
+      expect(resMappedDefaultDenied.status).toBe(400);
+      const mappedDefaultDeniedBody = (await resMappedDefaultDenied.json()) as { error?: string };
+      expect(mappedDefaultDeniedBody.error).toContain("hooks.allowedAgentIds");
+
       const resMappedDenied = await postHook(port, "/hooks/mapped", { subject: "hello" });
       expect(resMappedDenied.status).toBe(400);
       const mappedDeniedBody = (await resMappedDenied.json()) as { error?: string };
@@ -841,6 +861,43 @@ describe("gateway server hooks", () => {
     });
   });
 
+  test("allows omitted agentId when the default target is allowlisted", async () => {
+    testState.hooksConfig = {
+      enabled: true,
+      token: HOOK_TOKEN,
+      allowRequestSessionKey: true,
+      allowedSessionKeyPrefixes: ["hook:", "agent:"],
+      allowedAgentIds: ["main"],
+    };
+    testState.sessionConfig = { scope: "global" };
+    setMainAndHooksAgents();
+    await withGatewayServer(async ({ port }) => {
+      mockIsolatedRunOkOnce();
+      const resNoAgent = await postHook(port, "/hooks/agent", {
+        message: "Default target",
+        sessionKey: "agent:hooks:slack:channel:c123",
+      });
+      expect(resNoAgent.status).toBe(200);
+      await waitForSystemEventTexts(resolveMainKey());
+      const noAgentCall = cronRunCall();
+      expect(noAgentCall?.job?.agentId).toBeUndefined();
+      expect(noAgentCall?.sessionKey).toBe("agent:main:slack:channel:c123");
+      expect(peekSystemEventEntries("agent:main:main")).toStrictEqual([]);
+      drainSystemEvents(resolveMainKey());
+
+      mockIsolatedRunOkOnce();
+      const resBlankAgent = await postHook(port, "/hooks/agent", {
+        message: "Blank target",
+        agentId: " ",
+      });
+      expect(resBlankAgent.status).toBe(200);
+      await waitForSystemEventTexts(resolveMainKey());
+      const blankAgentCall = cronRunCall();
+      expect(blankAgentCall?.job?.agentId).toBeUndefined();
+      drainSystemEvents(resolveMainKey());
+    });
+  });
+
   test("denies explicit agentId when hooks.allowedAgentIds is empty", async () => {
     testState.hooksConfig = {
       enabled: true,
@@ -851,6 +908,13 @@ describe("gateway server hooks", () => {
       list: [{ id: "main", default: true }, { id: "hooks" }],
     };
     await withGatewayServer(async ({ port }) => {
+      const resNoAgent = await postHook(port, "/hooks/agent", {
+        message: "Denied by omission",
+      });
+      expect(resNoAgent.status).toBe(400);
+      const noAgentBody = (await resNoAgent.json()) as { error?: string };
+      expect(noAgentBody.error).toContain("hooks.allowedAgentIds");
+
       const resDenied = await postHook(port, "/hooks/agent", {
         message: "Denied",
         agentId: "hooks",
diff --git a/src/gateway/server/hooks-request-handler.ts b/src/gateway/server/hooks-request-handler.ts
@@ -23,6 +23,7 @@ import {
   normalizeHookHeaders,
   normalizeWakePayload,
   readJsonBody,
+  resolveEffectiveHookTargetAgentId,
   resolveHookChannel,
   resolveHookDeliver,
   resolveHookIdempotencyKey,
@@ -281,12 +282,16 @@ export function createHooksRequestHandler(
         return true;
       }
       const targetAgentId = resolveHookTargetAgentId(hooksConfig, normalized.value.agentId);
+      const effectiveTargetAgentId = resolveEffectiveHookTargetAgentId(
+        hooksConfig,
+        normalized.value.agentId,
+      );
       const replayKey = buildHookReplayCacheKey({
         pathKey: "agent",
         token,
         idempotencyKey,
         dispatchScope: {
-          agentId: targetAgentId ?? null,
+          agentId: effectiveTargetAgentId,
           sessionKey:
             normalized.value.sessionKey ?? hooksConfig.sessionPolicy.defaultSessionKey ?? null,
           message: normalized.value.message,
@@ -307,7 +312,7 @@ export function createHooksRequestHandler(
       }
       const normalizedDispatchSessionKey = normalizeHookDispatchSessionKey({
         sessionKey: sessionKey.value,
-        targetAgentId,
+        targetAgentId: effectiveTargetAgentId,
       });
       const allowedPrefixes = hooksConfig.sessionPolicy.allowedSessionKeyPrefixes;
       if (
@@ -376,9 +381,13 @@ export function createHooksRequestHandler(
             return true;
           }
           const targetAgentId = resolveHookTargetAgentId(hooksConfig, mapped.action.agentId);
+          const effectiveTargetAgentId = resolveEffectiveHookTargetAgentId(
+            hooksConfig,
+            mapped.action.agentId,
+          );
           const normalizedDispatchSessionKey = normalizeHookDispatchSessionKey({
             sessionKey: sessionKey.value,
-            targetAgentId,
+            targetAgentId: effectiveTargetAgentId,
           });
           const allowedPrefixes = hooksConfig.sessionPolicy.allowedSessionKeyPrefixes;
           if (
@@ -393,7 +402,7 @@ export function createHooksRequestHandler(
             token,
             idempotencyKey,
             dispatchScope: {
-              agentId: targetAgentId ?? null,
+              agentId: effectiveTargetAgentId,
               sessionKey:
                 mapped.action.sessionKey ?? hooksConfig.sessionPolicy.defaultSessionKey ?? null,
               message: mapped.action.message,
diff --git a/src/security/audit-extra.sync.ts b/src/security/audit-extra.sync.ts
@@ -664,9 +664,9 @@ export function collectHooksHardeningFindings(
       severity: remoteExposure ? "critical" : "warn",
       title: "Hook agent routing allows any configured agent",
       detail:
-        "hooks.allowedAgentIds is unset or includes '*', so authenticated hook callers may route to any configured agent id.",
+        "hooks.allowedAgentIds is unset or includes '*', so authenticated hook callers may route to any configured agent id, including the default agent when agentId is omitted.",
       remediation:
-        'Set hooks.allowedAgentIds to an explicit allowlist (for example, ["hooks", "main"]) or [] to deny explicit agent routing.',
+        'Set hooks.allowedAgentIds to an explicit allowlist (for example, ["hooks", "main"]) or [] to deny hook agent routing.',
     });
   }
 
