From 4d105d055970a7033ca7cccf03b2d19921c26f89 Mon Sep 17 00:00:00 2001 From: JacobZyy Date: Tue, 19 May 2026 14:03:13 +0800 Subject: [PATCH 1/4] fix(hooks): merge marketplace plugin hooksConfigs into claude-code-hooks at config time Previously, loadPluginHooksConfigs() loaded plugin hooks from marketplace plugins (hookify, superpowers, zzcommon, zzfe, etc.) into pluginComponents.hooksConfigs, but config-handler.ts never consumed them. This meant plugin hooks were discovered but never merged into the runtime hooks dispatch system. Changes: - Extend ClaudeHookEvent and ClaudeHooksConfig to support all 12 event types (PostToolUseFailure, PermissionRequest, Notification, SubagentStart, SubagentStop, SessionStart, SessionEnd) in addition to the existing 5 - Add ALL_HOOK_EVENT_TYPES constant as single source of truth for event type iteration - Add mergePluginHooksConfigs() to unwrap plugin HooksConfig (with hooks wrapper) into flat ClaudeHooksConfig, filtering out unsupported prompt/agent hook types - Add setPluginHooksConfigs() to store pending plugin configs and invalidate the config cache - Create applyHookConfig() handler following existing applyXxxConfig pattern, wired into config-handler after loadPluginComponents() - Extend DisabledHooksConfig and mergeDisabledHooks for all 12 events Closes #4179 --- src/hooks/claude-code-hooks/config-loader.ts | 16 ++- src/hooks/claude-code-hooks/config.ts | 103 ++++++++++++++++--- src/hooks/claude-code-hooks/types.ts | 14 +++ src/plugin-handlers/config-handler.ts | 3 + src/plugin-handlers/hook-config-handler.ts | 18 ++++ src/plugin-handlers/index.ts | 1 + 6 files changed, 138 insertions(+), 17 deletions(-) create mode 100644 src/plugin-handlers/hook-config-handler.ts diff --git a/src/hooks/claude-code-hooks/config-loader.ts b/src/hooks/claude-code-hooks/config-loader.ts index ea494ffdb..2b079fbfc 100644 --- a/src/hooks/claude-code-hooks/config-loader.ts +++ b/src/hooks/claude-code-hooks/config-loader.ts @@ -11,7 +11,14 @@ export interface DisabledHooksConfig { Stop?: string[] PreToolUse?: string[] PostToolUse?: string[] + PostToolUseFailure?: string[] + PermissionRequest?: string[] UserPromptSubmit?: string[] + Notification?: string[] + SubagentStart?: string[] + SubagentStop?: string[] + SessionStart?: string[] + SessionEnd?: string[] PreCompact?: string[] } @@ -78,10 +85,17 @@ function mergeDisabledHooks( if (!base) return override return { - Stop: override.Stop ?? base.Stop, PreToolUse: override.PreToolUse ?? base.PreToolUse, PostToolUse: override.PostToolUse ?? base.PostToolUse, + PostToolUseFailure: override.PostToolUseFailure ?? base.PostToolUseFailure, + PermissionRequest: override.PermissionRequest ?? base.PermissionRequest, UserPromptSubmit: override.UserPromptSubmit ?? base.UserPromptSubmit, + Notification: override.Notification ?? base.Notification, + Stop: override.Stop ?? base.Stop, + SubagentStart: override.SubagentStart ?? base.SubagentStart, + SubagentStop: override.SubagentStop ?? base.SubagentStop, + SessionStart: override.SessionStart ?? base.SessionStart, + SessionEnd: override.SessionEnd ?? base.SessionEnd, PreCompact: override.PreCompact ?? base.PreCompact, } } diff --git a/src/hooks/claude-code-hooks/config.ts b/src/hooks/claude-code-hooks/config.ts index 7d6341cfe..5960703f1 100644 --- a/src/hooks/claude-code-hooks/config.ts +++ b/src/hooks/claude-code-hooks/config.ts @@ -22,11 +22,33 @@ interface RawHookMatcher { interface RawClaudeHooksConfig { PreToolUse?: RawHookMatcher[] PostToolUse?: RawHookMatcher[] + PostToolUseFailure?: RawHookMatcher[] + PermissionRequest?: RawHookMatcher[] UserPromptSubmit?: RawHookMatcher[] + Notification?: RawHookMatcher[] Stop?: RawHookMatcher[] + SubagentStart?: RawHookMatcher[] + SubagentStop?: RawHookMatcher[] + SessionStart?: RawHookMatcher[] + SessionEnd?: RawHookMatcher[] PreCompact?: RawHookMatcher[] } +const ALL_HOOK_EVENT_TYPES: (keyof ClaudeHooksConfig)[] = [ + "PreToolUse", + "PostToolUse", + "PostToolUseFailure", + "PermissionRequest", + "UserPromptSubmit", + "Notification", + "Stop", + "SubagentStart", + "SubagentStop", + "SessionStart", + "SessionEnd", + "PreCompact", +] + function normalizeHookMatcher(raw: RawHookMatcher): HookMatcher { return { matcher: raw.matcher ?? raw.pattern ?? "*", @@ -36,15 +58,8 @@ function normalizeHookMatcher(raw: RawHookMatcher): HookMatcher { function normalizeHooksConfig(raw: RawClaudeHooksConfig): ClaudeHooksConfig { const result: ClaudeHooksConfig = {} - const eventTypes: (keyof RawClaudeHooksConfig)[] = [ - "PreToolUse", - "PostToolUse", - "UserPromptSubmit", - "Stop", - "PreCompact", - ] - for (const eventType of eventTypes) { + for (const eventType of ALL_HOOK_EVENT_TYPES) { if (raw[eventType]) { result[eventType] = raw[eventType].map(normalizeHookMatcher) } @@ -97,14 +112,7 @@ function mergeHooksConfig( override: ClaudeHooksConfig ): ClaudeHooksConfig { const result: ClaudeHooksConfig = { ...base } - const eventTypes: (keyof ClaudeHooksConfig)[] = [ - "PreToolUse", - "PostToolUse", - "UserPromptSubmit", - "Stop", - "PreCompact", - ] - for (const eventType of eventTypes) { + for (const eventType of ALL_HOOK_EVENT_TYPES) { if (override[eventType]) { result[eventType] = [...(base[eventType] || []), ...override[eventType]] } @@ -112,6 +120,64 @@ function mergeHooksConfig( return result } +let pendingPluginHooksConfigs: Array<{ hooks?: Record }> = [] + +export function setPluginHooksConfigs(configs: Array<{ hooks?: Record }>): void { + pendingPluginHooksConfigs = configs + configCache.clear() +} + +function isHookAction(h: unknown): h is HookAction { + if (typeof h !== "object" || h === null) return false + const obj = h as Record + if (obj.type === "command" && typeof obj.command === "string") return true + if (obj.type === "http" && typeof obj.url === "string") return true + return false +} + +interface PluginHookMatcher { + matcher?: string + pattern?: string + hooks?: unknown[] +} + +function isPluginHookMatcher(m: unknown): m is PluginHookMatcher { + return typeof m === "object" && m !== null && Array.isArray((m as PluginHookMatcher).hooks) +} + +export function mergePluginHooksConfigs( + base: ClaudeHooksConfig, + pluginHooksConfigs: Array<{ hooks?: Record }> +): ClaudeHooksConfig { + let result = { ...base } + + for (const pluginConfig of pluginHooksConfigs) { + if (!pluginConfig.hooks) continue + + const pluginOverrides: ClaudeHooksConfig = {} + for (const eventType of ALL_HOOK_EVENT_TYPES) { + const pluginMatchers = pluginConfig.hooks[eventType] + if (!Array.isArray(pluginMatchers)) continue + + const converted: HookMatcher[] = pluginMatchers + .filter(isPluginHookMatcher) + .map((m) => ({ + matcher: m.matcher ?? m.pattern ?? "*", + hooks: (m.hooks ?? []).filter(isHookAction), + })) + .filter((m) => m.hooks.length > 0) + + if (converted.length > 0) { + pluginOverrides[eventType] = converted + } + } + + result = mergeHooksConfig(result, pluginOverrides) + } + + return result +} + export async function loadClaudeHooksConfig( customSettingsPath?: string ): Promise { @@ -139,6 +205,11 @@ export async function loadClaudeHooksConfig( } } + // Merge plugin hooks configs + if (pendingPluginHooksConfigs.length > 0) { + mergedConfig = mergePluginHooksConfigs(mergedConfig, pendingPluginHooksConfigs) + } + const resolvedConfig = Object.keys(mergedConfig).length > 0 ? mergedConfig : null configCache.set(cacheKey, { value: resolvedConfig, diff --git a/src/hooks/claude-code-hooks/types.ts b/src/hooks/claude-code-hooks/types.ts index 28924de10..84ccde210 100644 --- a/src/hooks/claude-code-hooks/types.ts +++ b/src/hooks/claude-code-hooks/types.ts @@ -6,8 +6,15 @@ export type ClaudeHookEvent = | "PreToolUse" | "PostToolUse" + | "PostToolUseFailure" + | "PermissionRequest" | "UserPromptSubmit" + | "Notification" | "Stop" + | "SubagentStart" + | "SubagentStop" + | "SessionStart" + | "SessionEnd" | "PreCompact" export interface HookMatcher { @@ -33,8 +40,15 @@ export type HookAction = HookCommand | HookHttp export interface ClaudeHooksConfig { PreToolUse?: HookMatcher[] PostToolUse?: HookMatcher[] + PostToolUseFailure?: HookMatcher[] + PermissionRequest?: HookMatcher[] UserPromptSubmit?: HookMatcher[] + Notification?: HookMatcher[] Stop?: HookMatcher[] + SubagentStart?: HookMatcher[] + SubagentStop?: HookMatcher[] + SessionStart?: HookMatcher[] + SessionEnd?: HookMatcher[] PreCompact?: HookMatcher[] } diff --git a/src/plugin-handlers/config-handler.ts b/src/plugin-handlers/config-handler.ts index d77920c44..ddd9b8fe0 100644 --- a/src/plugin-handlers/config-handler.ts +++ b/src/plugin-handlers/config-handler.ts @@ -4,6 +4,7 @@ import type { ModelCacheState } from "../plugin-state"; import { log } from "../shared"; import { applyAgentConfig } from "./agent-config-handler"; import { applyCommandConfig } from "./command-config-handler"; +import { applyHookConfig } from "./hook-config-handler"; import { applyMcpConfig } from "./mcp-config-handler"; import { applyProviderConfig } from "./provider-config-handler"; import { loadPluginComponents } from "./plugin-components-loader"; @@ -30,6 +31,8 @@ export function createConfigHandler(deps: ConfigHandlerDeps) { const pluginComponents = await loadPluginComponents({ pluginConfig }); + applyHookConfig({ pluginComponents }); + const agentResult = await applyAgentConfig({ config, pluginConfig, diff --git a/src/plugin-handlers/hook-config-handler.ts b/src/plugin-handlers/hook-config-handler.ts new file mode 100644 index 000000000..8c84e20c2 --- /dev/null +++ b/src/plugin-handlers/hook-config-handler.ts @@ -0,0 +1,18 @@ +import type { PluginComponents } from "./plugin-components-loader" +import { setPluginHooksConfigs } from "../hooks/claude-code-hooks/config" +import { log } from "../shared" + +export function applyHookConfig(params: { + pluginComponents: PluginComponents; +}): void { + const { pluginComponents } = params + + if (pluginComponents.hooksConfigs.length > 0) { + log("[hook-config-handler] Merging plugin hooks configs", { + count: pluginComponents.hooksConfigs.length, + plugins: pluginComponents.plugins.map(p => p.name), + }) + + setPluginHooksConfigs(pluginComponents.hooksConfigs) + } +} diff --git a/src/plugin-handlers/index.ts b/src/plugin-handlers/index.ts index fa9bde977..9fe4d58b2 100644 --- a/src/plugin-handlers/index.ts +++ b/src/plugin-handlers/index.ts @@ -3,6 +3,7 @@ export * from "./provider-config-handler"; export * from "./agent-config-handler"; export * from "./tool-config-handler"; export * from "./mcp-config-handler"; +export * from "./hook-config-handler"; export * from "./command-config-handler"; export * from "./plugin-components-loader"; export * from "./category-config-resolver"; From 5e20842262eb5c9309a063b5a05a5a39e93f49b1 Mon Sep 17 00:00:00 2001 From: JacobZyy Date: Tue, 19 May 2026 14:15:42 +0800 Subject: [PATCH 2/4] fix(hooks): always persist plugin hook config state, even when empty When all plugin hooks are removed (user disables/uninstalls plugins), hooksConfigs becomes an empty array. The previous guard (hooksConfigs.length > 0) skipped setPluginHooksConfigs(), leaving stale plugin hooks active in pendingPluginHooksConfigs. Now we always call setPluginHooksConfigs() so empty configs properly clear the pending state and invalidate the cache. --- src/plugin-handlers/hook-config-handler.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/plugin-handlers/hook-config-handler.ts b/src/plugin-handlers/hook-config-handler.ts index 8c84e20c2..3b6782b78 100644 --- a/src/plugin-handlers/hook-config-handler.ts +++ b/src/plugin-handlers/hook-config-handler.ts @@ -12,7 +12,7 @@ export function applyHookConfig(params: { count: pluginComponents.hooksConfigs.length, plugins: pluginComponents.plugins.map(p => p.name), }) - - setPluginHooksConfigs(pluginComponents.hooksConfigs) } + + setPluginHooksConfigs(pluginComponents.hooksConfigs) } From 0a20844bd4864f5e7829ac88e2eb6cd9cedf3237 Mon Sep 17 00:00:00 2001 From: JacobZyy Date: Wed, 20 May 2026 22:30:25 +0800 Subject: [PATCH 3/4] fix: address PR #4180 review - security, typing, and test coverage - Apply mcp_env_allowlist to plugin hooks: intersect HTTP allowedEnvVars with MCP allowlist, set command allowedEnvVars to full MCP allowlist - Scrub process.env in executeHookCommand when allowedEnvVars provided - Add PluginHooksState class with per-directory Map storage - Add PluginHooksConfig interface for typed boundary layer - Pass directory context through hook-config-handler - Add 16 tests across 4 files (40 assertions) covering allowlist filtering, env scrubbing, directory isolation, and edge cases - Remove unnecessary 'as' type assertions, use discriminated union narrowing instead --- src/hooks/claude-code-hooks/config.test.ts | 222 ++++++++++++++++++ src/hooks/claude-code-hooks/config.ts | 65 ++++- .../claude-code-hooks/dispatch-hook.test.ts | 67 ++++++ src/hooks/claude-code-hooks/dispatch-hook.ts | 15 +- src/hooks/claude-code-hooks/types.ts | 10 + src/plugin-handlers/config-handler.ts | 2 +- .../hook-config-handler.test.ts | 83 +++++++ src/plugin-handlers/hook-config-handler.ts | 5 +- .../plugin-components-loader.ts | 3 +- .../execute-hook-command.test.ts | 62 +++++ .../command-executor/execute-hook-command.ts | 18 +- 11 files changed, 535 insertions(+), 17 deletions(-) create mode 100644 src/hooks/claude-code-hooks/dispatch-hook.test.ts create mode 100644 src/plugin-handlers/hook-config-handler.test.ts create mode 100644 src/shared/command-executor/execute-hook-command.test.ts diff --git a/src/hooks/claude-code-hooks/config.test.ts b/src/hooks/claude-code-hooks/config.test.ts index 2fdaa9c70..ba6220a0c 100644 --- a/src/hooks/claude-code-hooks/config.test.ts +++ b/src/hooks/claude-code-hooks/config.test.ts @@ -93,4 +93,226 @@ function getStopCommands(config: Awaited { + const { mergePluginHooksConfigs, setPluginHooksConfigs, clearClaudeHooksConfigCache: _clearCache } = require("./config") + const { setAdditionalAllowedMcpEnvVars, resetAdditionalAllowedMcpEnvVars } = require("../../features/claude-code-mcp-loader/configure-allowed-env-vars") + + afterEach(() => { + resetAdditionalAllowedMcpEnvVars() + }) + + test("#given empty plugin hooks #when merged #then returns base unchanged", () => { + // given + const base = { + Stop: [{ matcher: "*", hooks: [{ type: "command" as const, command: "echo stop" }] }], + } + + // when + const result = mergePluginHooksConfigs(base, []) + + // then + expect(result).toEqual(base) + }) + + test("#given plugin command hook #when merged #then allowedEnvVars is set to MCP allowlist", () => { + // given + setAdditionalAllowedMcpEnvVars(["MY_VAR"]) + const base = {} + const pluginConfig = { + hooks: { + Stop: [ + { + matcher: "*", + hooks: [{ type: "command", command: "echo hello" }], + }, + ], + }, + } + + // when + const result = mergePluginHooksConfigs(base, [pluginConfig]) + + // then + const stopHooks = result.Stop ?? [] + expect(stopHooks.length).toBe(1) + const hook = stopHooks[0].hooks[0] + expect(hook.type).toBe("command") + if (hook.type === "command") { + expect(hook.allowedEnvVars).toContain("MY_VAR") + } + }) + + test("#given plugin HTTP hook with allowedEnvVars #when merged #then vars are intersected with MCP allowlist", () => { + // given + setAdditionalAllowedMcpEnvVars(["MY_TOKEN"]) + const base = {} + const pluginConfig = { + hooks: { + PreToolUse: [ + { + matcher: "*", + hooks: [{ type: "http", url: "https://example.com/hook", allowedEnvVars: ["MY_TOKEN", "SECRET_KEY"] }], + }, + ], + }, + } + + // when + const result = mergePluginHooksConfigs(base, [pluginConfig]) + + // then + const preToolHooks = result.PreToolUse ?? [] + expect(preToolHooks.length).toBe(1) + const hook = preToolHooks[0].hooks[0] + expect(hook.type).toBe("http") + if (hook.type === "http") { + expect(hook.allowedEnvVars).toContain("MY_TOKEN") + expect(hook.allowedEnvVars).not.toContain("SECRET_KEY") + } + }) + + test("#given plugin HTTP hook with no allowedEnvVars #when merged #then hook passes through without crash", () => { + // given + const base = {} + const pluginConfig = { + hooks: { + PostToolUse: [ + { + matcher: "*", + hooks: [{ type: "http", url: "https://example.com/post" }], + }, + ], + }, + } + + // when + const result = mergePluginHooksConfigs(base, [pluginConfig]) + + // then + const postToolHooks = result.PostToolUse ?? [] + expect(postToolHooks.length).toBe(1) + const hook = postToolHooks[0].hooks[0] + expect(hook.type).toBe("http") + if (hook.type === "http") { + expect(hook.allowedEnvVars).toBeUndefined() + } + }) + + test("#given plugin hook with invalid action type #when merged #then it is filtered out", () => { + // given + const base = {} + const pluginConfig = { + hooks: { + Stop: [ + { + matcher: "*", + hooks: [ + { type: "command", command: "echo valid" }, + { type: "invalid", something: "bad" }, + { notAHook: true }, + ], + }, + ], + }, + } + + // when + const result = mergePluginHooksConfigs(base, [pluginConfig]) + + // then + const stopHooks = result.Stop ?? [] + expect(stopHooks.length).toBe(1) + expect(stopHooks[0].hooks.length).toBe(1) + expect(stopHooks[0].hooks[0].type).toBe("command") + }) + + test("#given plugin hook with non-array hooks #when merged #then it is skipped", () => { + // given + const base = {} + const pluginConfig = { + hooks: { + Stop: "not-an-array", + PreToolUse: 42, + }, + } + + // when + const result = mergePluginHooksConfigs(base, [pluginConfig]) + + // then + expect(result.Stop).toBeUndefined() + expect(result.PreToolUse).toBeUndefined() + }) +}) + +describe("setPluginHooksConfigs", () => { + const { setPluginHooksConfigs, loadClaudeHooksConfig, clearClaudeHooksConfigCache: _clearCache } = require("./config") + const { resetAdditionalAllowedMcpEnvVars } = require("../../features/claude-code-mcp-loader/configure-allowed-env-vars") + let originalWorkingDirectory = "" + + beforeEach(() => { + originalWorkingDirectory = process.cwd() + _clearCache() + }) + + afterEach(() => { + _clearCache() + resetAdditionalAllowedMcpEnvVars() + process.chdir(originalWorkingDirectory) + }) + + test("#given configs set for directory A #when loading config from directory A #then plugin hooks are included", async () => { + // given + const dirA = process.cwd() + setPluginHooksConfigs(dirA, [ + { + hooks: { + Stop: [ + { + matcher: "*", + hooks: [{ type: "command", command: "echo plugin-stop" }], + }, + ], + }, + }, + ]) + + // when + const config = await loadClaudeHooksConfig() + + // then + const stopCommands = (config?.Stop ?? []).flatMap((m) => + m.hooks.flatMap((h) => (h.type === "command" && typeof h.command === "string" ? [h.command] : [])), + ) + expect(stopCommands).toContain("echo plugin-stop") + }) + + test("#given configs set for directory A #when loading config from directory B #then plugin hooks are NOT included", async () => { + // given + const dirA = "/tmp/omo-test-dir-a" + const dirB = process.cwd() + setPluginHooksConfigs(dirA, [ + { + hooks: { + Stop: [ + { + matcher: "*", + hooks: [{ type: "command", command: "echo plugin-stop-a" }], + }, + ], + }, + }, + ]) + + // when — cwd is dirB, not dirA + const config = await loadClaudeHooksConfig() + + // then — plugin hooks from dirA should NOT appear + const stopCommands = (config?.Stop ?? []).flatMap((m) => + m.hooks.flatMap((h) => (h.type === "command" && typeof h.command === "string" ? [h.command] : [])), + ) + expect(stopCommands).not.toContain("echo plugin-stop-a") + }) +}) + export {} diff --git a/src/hooks/claude-code-hooks/config.ts b/src/hooks/claude-code-hooks/config.ts index 5960703f1..2b9fc99fe 100644 --- a/src/hooks/claude-code-hooks/config.ts +++ b/src/hooks/claude-code-hooks/config.ts @@ -2,7 +2,8 @@ import { join } from "path" import { existsSync } from "fs" import { getClaudeConfigDir } from "../../shared" import { bunFile } from "../../shared/bun-file-shim" -import type { ClaudeHooksConfig, HookMatcher, HookAction } from "./types" +import { getAllowedMcpEnvVars } from "../../features/claude-code-mcp-loader/configure-allowed-env-vars" +import type { ClaudeHooksConfig, HookMatcher, HookAction, PluginHooksConfig } from "./types" const CONFIG_CACHE_TTL_MS = 30_000 @@ -120,10 +121,30 @@ function mergeHooksConfig( return result } -let pendingPluginHooksConfigs: Array<{ hooks?: Record }> = [] +/** + * Encapsulates mutable plugin hooks state with per-project keying. + * Replaces module-level `let pendingPluginHooksConfigs`. + */ +class PluginHooksState { + private configs = new Map() -export function setPluginHooksConfigs(configs: Array<{ hooks?: Record }>): void { - pendingPluginHooksConfigs = configs + setConfigs(directory: string, configs: PluginHooksConfig[]): void { + this.configs.set(directory, configs) + } + + getConfigs(directory: string): PluginHooksConfig[] { + return this.configs.get(directory) ?? [] + } + + clear(): void { + this.configs.clear() + } +} + +const pluginHooksState = new PluginHooksState() + +export function setPluginHooksConfigs(directory: string, configs: PluginHooksConfig[]): void { + pluginHooksState.setConfigs(directory, configs) configCache.clear() } @@ -145,9 +166,32 @@ function isPluginHookMatcher(m: unknown): m is PluginHookMatcher { return typeof m === "object" && m !== null && Array.isArray((m as PluginHookMatcher).hooks) } +/** + * Intersect plugin hook allowedEnvVars with the MCP env allowlist. + * For HTTP hooks: filter allowedEnvVars to only allowlisted vars. + * For command hooks: set allowedEnvVars to the full MCP allowlist. + */ +function applyMcpEnvAllowlist(action: HookAction): HookAction { + const allowedVars = getAllowedMcpEnvVars() + + if (action.type === "http") { + if (!action.allowedEnvVars || action.allowedEnvVars.length === 0) { + return action + } + const filtered = action.allowedEnvVars.filter((v) => allowedVars.has(v)) + return { ...action, allowedEnvVars: filtered } + } + + if (action.type === "command") { + return { ...action, allowedEnvVars: [...allowedVars] } + } + + return action +} + export function mergePluginHooksConfigs( base: ClaudeHooksConfig, - pluginHooksConfigs: Array<{ hooks?: Record }> + pluginHooksConfigs: PluginHooksConfig[] ): ClaudeHooksConfig { let result = { ...base } @@ -163,7 +207,9 @@ export function mergePluginHooksConfigs( .filter(isPluginHookMatcher) .map((m) => ({ matcher: m.matcher ?? m.pattern ?? "*", - hooks: (m.hooks ?? []).filter(isHookAction), + hooks: (m.hooks ?? []) + .filter(isHookAction) + .map(applyMcpEnvAllowlist), })) .filter((m) => m.hooks.length > 0) @@ -205,9 +251,10 @@ export async function loadClaudeHooksConfig( } } - // Merge plugin hooks configs - if (pendingPluginHooksConfigs.length > 0) { - mergedConfig = mergePluginHooksConfigs(mergedConfig, pendingPluginHooksConfigs) + // Merge plugin hooks configs for the current project directory + const projectConfigs = pluginHooksState.getConfigs(process.cwd()) + if (projectConfigs.length > 0) { + mergedConfig = mergePluginHooksConfigs(mergedConfig, projectConfigs) } const resolvedConfig = Object.keys(mergedConfig).length > 0 ? mergedConfig : null diff --git a/src/hooks/claude-code-hooks/dispatch-hook.test.ts b/src/hooks/claude-code-hooks/dispatch-hook.test.ts new file mode 100644 index 000000000..b8c0bda53 --- /dev/null +++ b/src/hooks/claude-code-hooks/dispatch-hook.test.ts @@ -0,0 +1,67 @@ +const { afterEach, beforeEach, describe, expect, mock, test } = require("bun:test") + +const capturedOptions: Array> = [] + +const mockExecuteHookCommand = mock( + (_command: string, _stdin: string, _cwd: string, options?: Record) => { + capturedOptions.push(options ?? {}) + return Promise.resolve({ exitCode: 0, stdout: "", stderr: "" }) + }, +) + +mock.module("../../shared/command-executor/execute-hook-command", () => ({ + executeHookCommand: mockExecuteHookCommand, +})) + +mock.module("./execute-http-hook", () => ({ + executeHttpHook: mock(() => Promise.resolve({ exitCode: 0, stdout: "", stderr: "" })), +})) + +const { dispatchHook } = await import("./dispatch-hook") + +describe("dispatchHook", () => { + beforeEach(() => { + mockExecuteHookCommand.mockClear() + capturedOptions.length = 0 + }) + + afterEach(() => { + mockExecuteHookCommand.mockClear() + capturedOptions.length = 0 + }) + + test("#given HookCommand with allowedEnvVars #when dispatchHook called #then options include allowedEnvVars", async () => { + // given + const hook = { + type: "command" as const, + command: "echo hello", + allowedEnvVars: ["MY_VAR"], + } + + // when + await dispatchHook(hook, "{}", "/tmp") + + // then + expect(mockExecuteHookCommand).toHaveBeenCalledTimes(1) + expect(capturedOptions.length).toBe(1) + expect(capturedOptions[0].allowedEnvVars).toEqual(["MY_VAR"]) + }) + + test("#given HookCommand without allowedEnvVars #when dispatchHook called #then options do NOT include allowedEnvVars", async () => { + // given + const hook = { + type: "command" as const, + command: "echo hello", + } + + // when + await dispatchHook(hook, "{}", "/tmp") + + // then + expect(mockExecuteHookCommand).toHaveBeenCalledTimes(1) + expect(capturedOptions.length).toBe(1) + expect(capturedOptions[0].allowedEnvVars).toBeUndefined() + }) +}) + +export {} diff --git a/src/hooks/claude-code-hooks/dispatch-hook.ts b/src/hooks/claude-code-hooks/dispatch-hook.ts index 5feeabb62..e53219ef3 100644 --- a/src/hooks/claude-code-hooks/dispatch-hook.ts +++ b/src/hooks/claude-code-hooks/dispatch-hook.ts @@ -1,5 +1,5 @@ -import type { HookAction } from "./types" -import type { CommandResult } from "../../shared/command-executor/execute-hook-command" +import type { HookAction, HookCommand } from "./types" +import type { CommandResult, ExecuteHookOptions } from "../../shared/command-executor/execute-hook-command" import { executeHookCommand } from "../../shared" import { executeHttpHook } from "./execute-http-hook" import { DEFAULT_CONFIG } from "./plugin-config" @@ -18,10 +18,19 @@ export async function dispatchHook( return executeHttpHook(hook, stdinJson) } + const cmdHook = hook as HookCommand + const options: ExecuteHookOptions = { + forceZsh: DEFAULT_CONFIG.forceZsh, + zshPath: DEFAULT_CONFIG.zshPath, + } + if (cmdHook.allowedEnvVars) { + options.allowedEnvVars = cmdHook.allowedEnvVars + } + return executeHookCommand( hook.command, stdinJson, cwd, - { forceZsh: DEFAULT_CONFIG.forceZsh, zshPath: DEFAULT_CONFIG.zshPath } + options ) } diff --git a/src/hooks/claude-code-hooks/types.ts b/src/hooks/claude-code-hooks/types.ts index 84ccde210..28e90c637 100644 --- a/src/hooks/claude-code-hooks/types.ts +++ b/src/hooks/claude-code-hooks/types.ts @@ -25,6 +25,8 @@ export interface HookMatcher { export interface HookCommand { type: "command" command: string + /** Env vars allowed to pass through to the spawned process (plugin-sourced hooks are intersected with mcp_env_allowlist) */ + allowedEnvVars?: string[] } export interface HookHttp { @@ -226,3 +228,11 @@ export interface PluginConfig { disabledHooks?: boolean | ClaudeHookEvent[] keywordDetectorDisabled?: boolean } + +/** + * Plugin hooks configuration shape. + * Replaces the loose `Array<{ hooks?: Record }>` with a proper typed interface. + */ +export interface PluginHooksConfig { + hooks?: Partial> +} diff --git a/src/plugin-handlers/config-handler.ts b/src/plugin-handlers/config-handler.ts index ddd9b8fe0..36961021f 100644 --- a/src/plugin-handlers/config-handler.ts +++ b/src/plugin-handlers/config-handler.ts @@ -31,7 +31,7 @@ export function createConfigHandler(deps: ConfigHandlerDeps) { const pluginComponents = await loadPluginComponents({ pluginConfig }); - applyHookConfig({ pluginComponents }); + applyHookConfig({ pluginComponents, ctx }); const agentResult = await applyAgentConfig({ config, diff --git a/src/plugin-handlers/hook-config-handler.test.ts b/src/plugin-handlers/hook-config-handler.test.ts new file mode 100644 index 000000000..1c9085e0a --- /dev/null +++ b/src/plugin-handlers/hook-config-handler.test.ts @@ -0,0 +1,83 @@ +const { afterEach, beforeEach, describe, expect, mock, test } = require("bun:test") + +const mockSetPluginHooksConfigs = mock(() => {}) + +mock.module("../hooks/claude-code-hooks/config", () => ({ + setPluginHooksConfigs: mockSetPluginHooksConfigs, +})) + +const { applyHookConfig } = await import("./hook-config-handler") + +describe("applyHookConfig", () => { + beforeEach(() => { + mockSetPluginHooksConfigs.mockClear() + }) + + afterEach(() => { + mockSetPluginHooksConfigs.mockClear() + }) + + test("#given ctx.directory #when applyHookConfig called #then setPluginHooksConfigs receives ctx.directory", () => { + // given + const testDirectory = "/test/dir" + const pluginComponents = { + commands: {}, + skills: {}, + agents: {}, + mcpServers: {}, + hooksConfigs: [ + { + hooks: { + Stop: [ + { + matcher: "*", + hooks: [{ type: "command", command: "echo test" }], + }, + ], + }, + }, + ], + plugins: [{ name: "test-plugin", version: "1.0.0" }], + errors: [], + } + + // when + applyHookConfig({ + pluginComponents, + ctx: { directory: testDirectory }, + }) + + // then + expect(mockSetPluginHooksConfigs).toHaveBeenCalledTimes(1) + expect(mockSetPluginHooksConfigs).toHaveBeenCalledWith( + testDirectory, + pluginComponents.hooksConfigs, + ) + }) + + test("#given empty hooksConfigs #when applyHookConfig called #then setPluginHooksConfigs still called with empty array", () => { + // given + const testDirectory = "/another/dir" + const pluginComponents = { + commands: {}, + skills: {}, + agents: {}, + mcpServers: {}, + hooksConfigs: [], + plugins: [], + errors: [], + } + + // when + applyHookConfig({ + pluginComponents, + ctx: { directory: testDirectory }, + }) + + // then + expect(mockSetPluginHooksConfigs).toHaveBeenCalledTimes(1) + expect(mockSetPluginHooksConfigs).toHaveBeenCalledWith(testDirectory, []) + }) +}) + +export {} diff --git a/src/plugin-handlers/hook-config-handler.ts b/src/plugin-handlers/hook-config-handler.ts index 3b6782b78..38381628d 100644 --- a/src/plugin-handlers/hook-config-handler.ts +++ b/src/plugin-handlers/hook-config-handler.ts @@ -4,8 +4,9 @@ import { log } from "../shared" export function applyHookConfig(params: { pluginComponents: PluginComponents; + ctx: { directory: string }; }): void { - const { pluginComponents } = params + const { pluginComponents, ctx } = params if (pluginComponents.hooksConfigs.length > 0) { log("[hook-config-handler] Merging plugin hooks configs", { @@ -14,5 +15,5 @@ export function applyHookConfig(params: { }) } - setPluginHooksConfigs(pluginComponents.hooksConfigs) + setPluginHooksConfigs(ctx.directory, pluginComponents.hooksConfigs) } diff --git a/src/plugin-handlers/plugin-components-loader.ts b/src/plugin-handlers/plugin-components-loader.ts index 7d122a39e..75e4f814b 100644 --- a/src/plugin-handlers/plugin-components-loader.ts +++ b/src/plugin-handlers/plugin-components-loader.ts @@ -1,5 +1,6 @@ import type { OhMyOpenCodeConfig } from "../config"; import { loadAllPluginComponents } from "../features/claude-code-plugin-loader"; +import type { PluginHooksConfig } from "../hooks/claude-code-hooks/types"; import { addConfigLoadError, log } from "../shared"; export type PluginComponents = { @@ -7,7 +8,7 @@ export type PluginComponents = { skills: Record; agents: Record; mcpServers: Record; - hooksConfigs: Array<{ hooks?: Record }>; + hooksConfigs: PluginHooksConfig[]; plugins: Array<{ name: string; version: string }>; errors: Array<{ pluginKey: string; installPath: string; error: string }>; }; diff --git a/src/shared/command-executor/execute-hook-command.test.ts b/src/shared/command-executor/execute-hook-command.test.ts new file mode 100644 index 000000000..c3688c91e --- /dev/null +++ b/src/shared/command-executor/execute-hook-command.test.ts @@ -0,0 +1,62 @@ +const { afterEach, beforeEach, describe, expect, test } = require("bun:test") +import { tmpdir } from "node:os" +import { join } from "node:path" +import { mkdtempSync, rmSync } from "node:fs" + +const { executeHookCommand } = await import("./execute-hook-command") + +describe("executeHookCommand", () => { + let tempDirectory = "" + + beforeEach(() => { + tempDirectory = mkdtempSync(join(tmpdir(), "omo-exec-hook-cmd-")) + }) + + afterEach(() => { + rmSync(tempDirectory, { recursive: true, force: true }) + }) + + test("#given allowedEnvVars provided #when executing command #then only allowed vars are in process.env", async () => { + // given + process.env.__OMO_TEST_ALLOWED_VAR = "visible" + process.env.__OMO_TEST_SECRET_VAR = "hidden" + + // when + const result = await executeHookCommand( + "echo $__OMO_TEST_ALLOWED_VAR $__OMO_TEST_SECRET_VAR", + "", + tempDirectory, + { allowedEnvVars: ["__OMO_TEST_ALLOWED_VAR"] }, + ) + + // then + expect(result.exitCode).toBe(0) + expect(result.stdout).toContain("visible") + expect(result.stdout).not.toContain("hidden") + + // cleanup + delete process.env.__OMO_TEST_ALLOWED_VAR + delete process.env.__OMO_TEST_SECRET_VAR + }) + + test("#given no allowedEnvVars #when executing command #then full env is available", async () => { + // given + process.env.__OMO_TEST_FULL_ENV_VAR = "present" + + // when + const result = await executeHookCommand( + "echo $__OMO_TEST_FULL_ENV_VAR", + "", + tempDirectory, + ) + + // then + expect(result.exitCode).toBe(0) + expect(result.stdout).toContain("present") + + // cleanup + delete process.env.__OMO_TEST_FULL_ENV_VAR + }) +}) + +export {} diff --git a/src/shared/command-executor/execute-hook-command.ts b/src/shared/command-executor/execute-hook-command.ts index d4ded4a55..d4bd41c39 100644 --- a/src/shared/command-executor/execute-hook-command.ts +++ b/src/shared/command-executor/execute-hook-command.ts @@ -16,6 +16,8 @@ export interface ExecuteHookOptions { zshPath?: string; /** Timeout in milliseconds. Process is killed after this. Default: 30000 */ timeoutMs?: number; + /** When provided, scrub process.env to only include these vars plus HOME/PATH/etc. Used for plugin-sourced hooks. */ + allowedEnvVars?: string[]; } export async function executeHookCommand( @@ -53,11 +55,25 @@ export async function executeHookCommand( let killTimer: ReturnType | null = null; const isWin32 = process.platform === "win32"; + + let env: Record; + if (options?.allowedEnvVars) { + const allowedSet = new Set(options.allowedEnvVars); + env = { HOME: home, CLAUDE_PROJECT_DIR: cwd }; + for (const key of Object.keys(process.env)) { + if (allowedSet.has(key)) { + env[key] = process.env[key]; + } + } + } else { + env = { ...process.env, HOME: home, CLAUDE_PROJECT_DIR: cwd }; + } + const proc = spawn(finalCommand, { cwd, shell: true, detached: !isWin32, - env: { ...process.env, HOME: home, CLAUDE_PROJECT_DIR: cwd }, + env, }); let stdout = ""; From 33f121b113419c3b342431b7479373c1eb078d35 Mon Sep 17 00:00:00 2001 From: JacobZyy Date: Wed, 20 May 2026 22:49:05 +0800 Subject: [PATCH 4/4] fix: add PATH to restricted hook env, protect HOME/CLAUDE_PROJECT_DIR from allowlist override, reset plugin hooks state in tests - P1: When allowedEnvVars is provided, PATH was missing from the base restricted env, causing non-builtin commands to fail at exec time - P2: Allowlisted HOME/CLAUDE_PROJECT_DIR could overwrite normalized values from getHomeDirectory()/cwd with ambient process.env values - P2: Test suite mutated shared pluginHooksState singleton without resetting it in afterEach, causing cross-test state leaks --- src/hooks/claude-code-hooks/config.test.ts | 6 ++++-- src/hooks/claude-code-hooks/config.ts | 4 ++++ src/shared/command-executor/execute-hook-command.ts | 12 ++++++++++-- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/hooks/claude-code-hooks/config.test.ts b/src/hooks/claude-code-hooks/config.test.ts index ba6220a0c..5de403701 100644 --- a/src/hooks/claude-code-hooks/config.test.ts +++ b/src/hooks/claude-code-hooks/config.test.ts @@ -94,11 +94,12 @@ function getStopCommands(config: Awaited { - const { mergePluginHooksConfigs, setPluginHooksConfigs, clearClaudeHooksConfigCache: _clearCache } = require("./config") + const { mergePluginHooksConfigs, setPluginHooksConfigs, clearClaudeHooksConfigCache: _clearCache, resetPluginHooksState } = require("./config") const { setAdditionalAllowedMcpEnvVars, resetAdditionalAllowedMcpEnvVars } = require("../../features/claude-code-mcp-loader/configure-allowed-env-vars") afterEach(() => { resetAdditionalAllowedMcpEnvVars() + resetPluginHooksState() }) test("#given empty plugin hooks #when merged #then returns base unchanged", () => { @@ -246,7 +247,7 @@ describe("mergePluginHooksConfigs", () => { }) describe("setPluginHooksConfigs", () => { - const { setPluginHooksConfigs, loadClaudeHooksConfig, clearClaudeHooksConfigCache: _clearCache } = require("./config") + const { setPluginHooksConfigs, loadClaudeHooksConfig, clearClaudeHooksConfigCache: _clearCache, resetPluginHooksState } = require("./config") const { resetAdditionalAllowedMcpEnvVars } = require("../../features/claude-code-mcp-loader/configure-allowed-env-vars") let originalWorkingDirectory = "" @@ -258,6 +259,7 @@ describe("setPluginHooksConfigs", () => { afterEach(() => { _clearCache() resetAdditionalAllowedMcpEnvVars() + resetPluginHooksState() process.chdir(originalWorkingDirectory) }) diff --git a/src/hooks/claude-code-hooks/config.ts b/src/hooks/claude-code-hooks/config.ts index 2b9fc99fe..362a2d021 100644 --- a/src/hooks/claude-code-hooks/config.ts +++ b/src/hooks/claude-code-hooks/config.ts @@ -108,6 +108,10 @@ export function clearClaudeHooksConfigCache(): void { configCache.clear() } +export function resetPluginHooksState(): void { + pluginHooksState.clear() +} + function mergeHooksConfig( base: ClaudeHooksConfig, override: ClaudeHooksConfig diff --git a/src/shared/command-executor/execute-hook-command.ts b/src/shared/command-executor/execute-hook-command.ts index d4bd41c39..43c628f2f 100644 --- a/src/shared/command-executor/execute-hook-command.ts +++ b/src/shared/command-executor/execute-hook-command.ts @@ -56,12 +56,20 @@ export async function executeHookCommand( const isWin32 = process.platform === "win32"; + // Keys that are always set from normalized sources and must not be + // overwritten by ambient process.env values during the allowlist merge. + const PROTECTED_ENV_KEYS = new Set(["HOME", "CLAUDE_PROJECT_DIR"]); + let env: Record; if (options?.allowedEnvVars) { const allowedSet = new Set(options.allowedEnvVars); - env = { HOME: home, CLAUDE_PROJECT_DIR: cwd }; + env = { + HOME: home, + CLAUDE_PROJECT_DIR: cwd, + PATH: process.env.PATH, + }; for (const key of Object.keys(process.env)) { - if (allowedSet.has(key)) { + if (allowedSet.has(key) && !PROTECTED_ENV_KEYS.has(key)) { env[key] = process.env[key]; } }