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

@@ -93,4 +93,226 @@ function getStopCommands(config: Awaited<ReturnType<typeof loadClaudeHooksConfig
   )
 }
 
+describe("mergePluginHooksConfigs", () => {
+  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 {}

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<string, unknown> }> = []
+/**
+ * Encapsulates mutable plugin hooks state with per-project keying.
+ * Replaces module-level `let pendingPluginHooksConfigs`.
+ */
+class PluginHooksState {
+  private configs = new Map<string, PluginHooksConfig[]>()
 
-export function setPluginHooksConfigs(configs: Array<{ hooks?: Record<string, unknown> }>): 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<string, unknown> }>
+  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

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<Record<string, unknown>> = []
+
+const mockExecuteHookCommand = mock(
+  (_command: string, _stdin: string, _cwd: string, options?: Record<string, unknown>) => {
+    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 {}

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

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<string, unknown> }>` with a proper typed interface.
+ */
+export interface PluginHooksConfig {
+  hooks?: Partial<Record<ClaudeHookEvent, unknown[]>>
+}

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,

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

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

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<string, unknown>;
   agents: Record<string, unknown>;
   mcpServers: Record<string, unknown>;
-  hooksConfigs: Array<{ hooks?: Record<string, unknown> }>;
+  hooksConfigs: PluginHooksConfig[];
   plugins: Array<{ name: string; version: string }>;
   errors: Array<{ pluginKey: string; installPath: string; error: string }>;
 };

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

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<typeof setTimeout> | null = null;
 
     const isWin32 = process.platform === "win32";
+
+    let env: Record<string, string | undefined>;
+    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 = "";