fix: bootstrap plugin-installed hook commands safely

2026-04-15 22:43:28 +08:00 · 2026-04-14 20:24:21 -07:00
parent 48a30b53c8
commit 1b7c5789fc
9 changed files with 564 additions and 95 deletions
--- a/tests/hooks/hooks.test.js
+++ b/tests/hooks/hooks.test.js
@@ -1912,13 +1912,14 @@ async function runTests() {
        for (const entry of hookArray) {
          for (const hook of entry.hooks) {
            if (hook.type === 'command') {
-              const isNode = hook.command.startsWith('node');
-              const isNpx = hook.command.startsWith('npx ');
-              const isSkillScript = hook.command.includes('/skills/') && (/^(bash|sh)\s/.test(hook.command) || hook.command.startsWith('${CLAUDE_PLUGIN_ROOT}/skills/'));
-              const isHookShellWrapper = /^(bash|sh)\s+["']?\$\{CLAUDE_PLUGIN_ROOT\}\/scripts\/hooks\/run-with-flags-shell\.sh/.test(hook.command);
+              const commandText = Array.isArray(hook.command) ? hook.command.join(' ') : hook.command;
+              const commandStart = Array.isArray(hook.command) ? hook.command[0] : hook.command;
+              const isNode = commandStart === 'node' || (typeof commandStart === 'string' && commandStart.startsWith('node'));
+              const isNpx = commandStart === 'npx' || (typeof commandStart === 'string' && commandStart.startsWith('npx '));
+              const isSkillScript = commandText.includes('/skills/') && (/^(bash|sh)\s/.test(commandText) || commandText.includes('/skills/'));
              assert.ok(
-                isNode || isNpx || isSkillScript || isHookShellWrapper,
-                `Hook command should use node or approved shell wrapper: ${hook.command.substring(0, 100)}...`
+                isNode || isNpx || isSkillScript,
+                `Hook command should use node or approved shell wrapper: ${commandText.substring(0, 100)}...`
              );
            }
          }
@@ -1940,16 +1941,18 @@ async function runTests() {
      const sessionStartHook = hooks.hooks.SessionStart?.[0]?.hooks?.[0];

      assert.ok(sessionStartHook, 'Should define a SessionStart hook');
-      // The bootstrap was extracted to a standalone file to avoid shell history
-      // expansion of `!` characters that caused startup hook errors when the
-      // logic was embedded as an inline `node -e "..."` string.
+      const commandText = Array.isArray(sessionStartHook.command)
+        ? sessionStartHook.command.join(' ')
+        : sessionStartHook.command;
+      assert.ok(Array.isArray(sessionStartHook.command), 'SessionStart should use argv form for cross-platform safety');
      assert.ok(
-        sessionStartHook.command.includes('session-start-bootstrap.js'),
+        commandText.includes('session-start-bootstrap.js'),
        'SessionStart should delegate to the extracted bootstrap script'
      );
-      assert.ok(sessionStartHook.command.includes('CLAUDE_PLUGIN_ROOT'), 'SessionStart should use CLAUDE_PLUGIN_ROOT');
-      assert.ok(!sessionStartHook.command.includes('find '), 'Should not scan arbitrary plugin paths with find');
-      assert.ok(!sessionStartHook.command.includes('head -n 1'), 'Should not pick the first matching plugin path');
+      assert.ok(commandText.includes('CLAUDE_PLUGIN_ROOT'), 'SessionStart should use CLAUDE_PLUGIN_ROOT');
+      assert.ok(!commandText.includes('${CLAUDE_PLUGIN_ROOT}'), 'SessionStart should not depend on raw shell placeholder expansion');
+      assert.ok(!commandText.includes('find '), 'Should not scan arbitrary plugin paths with find');
+      assert.ok(!commandText.includes('head -n 1'), 'Should not pick the first matching plugin path');

      // Verify the bootstrap script itself contains the expected logic
      const bootstrapPath = path.join(__dirname, '..', '..', 'scripts', 'hooks', 'session-start-bootstrap.js');
@@ -1971,29 +1974,41 @@ async function runTests() {
      const sessionEndHooks = (hooks.hooks.SessionEnd || []).flatMap(entry => entry.hooks || []);

      for (const hook of [...stopHooks, ...sessionEndHooks]) {
-        assert.ok(hook.command.startsWith('node -e "'), 'Lifecycle hook should use inline node resolver');
-        assert.ok(hook.command.includes('run-with-flags.js'), 'Lifecycle hook should resolve the runner script');
-        assert.ok(hook.command.includes('CLAUDE_PLUGIN_ROOT'), 'Lifecycle hook should consult CLAUDE_PLUGIN_ROOT');
-        assert.ok(hook.command.includes('plugins'), 'Lifecycle hook should probe known plugin roots');
-        assert.ok(!hook.command.includes('find '), 'Lifecycle hook should not scan arbitrary plugin paths with find');
-        assert.ok(!hook.command.includes('head -n 1'), 'Lifecycle hook should not pick the first matching plugin path');
+        const commandText = Array.isArray(hook.command) ? hook.command.join(' ') : hook.command;
+        assert.ok(
+          (Array.isArray(hook.command) && hook.command[0] === 'node' && hook.command[1] === '-e') ||
+          (typeof hook.command === 'string' && hook.command.startsWith('node -e "')),
+          'Lifecycle hook should use inline node resolver'
+        );
+        assert.ok(commandText.includes('run-with-flags.js'), 'Lifecycle hook should resolve the runner script');
+        assert.ok(commandText.includes('CLAUDE_PLUGIN_ROOT'), 'Lifecycle hook should consult CLAUDE_PLUGIN_ROOT');
+        assert.ok(!commandText.includes('${CLAUDE_PLUGIN_ROOT}'), 'Lifecycle hook should not depend on raw shell placeholder expansion');
+        assert.ok(commandText.includes('plugins'), 'Lifecycle hook should probe known plugin roots');
+        assert.ok(!commandText.includes('find '), 'Lifecycle hook should not scan arbitrary plugin paths with find');
+        assert.ok(!commandText.includes('head -n 1'), 'Lifecycle hook should not pick the first matching plugin path');
      }
    })
  )
    passed++;
  else failed++;
  if (
-    test('script references use CLAUDE_PLUGIN_ROOT variable or a safe inline resolver', () => {
+    test('script references use the safe inline resolver or plugin bootstrap', () => {
      const hooksPath = path.join(__dirname, '..', '..', 'hooks', 'hooks.json');
      const hooks = JSON.parse(fs.readFileSync(hooksPath, 'utf8'));

      const checkHooks = hookArray => {
        for (const entry of hookArray) {
          for (const hook of entry.hooks) {
-            if (hook.type === 'command' && hook.command.includes('scripts/hooks/')) {
-              const usesInlineResolver = hook.command.startsWith('node -e') && hook.command.includes('run-with-flags.js');
-              const hasPluginRoot = hook.command.includes('${CLAUDE_PLUGIN_ROOT}') || usesInlineResolver;
-              assert.ok(hasPluginRoot, `Script paths should use CLAUDE_PLUGIN_ROOT: ${hook.command.substring(0, 80)}...`);
+            const commandText = Array.isArray(hook.command) ? hook.command.join(' ') : hook.command;
+            const commandStart = Array.isArray(hook.command) ? `${hook.command[0]} ${hook.command[1] || ''}`.trim() : hook.command;
+            if (hook.type === 'command' && commandText.includes('scripts/hooks/')) {
+              const usesInlineResolver = commandStart.startsWith('node -e') && commandText.includes('run-with-flags.js');
+              const usesPluginBootstrap = commandStart.startsWith('node -e') && commandText.includes('plugin-hook-bootstrap.js');
+              assert.ok(!commandText.includes('${CLAUDE_PLUGIN_ROOT}'), `Script paths should not depend on raw shell placeholder expansion: ${commandText.substring(0, 80)}...`);
+              assert.ok(
+                usesInlineResolver || usesPluginBootstrap,
+                `Script paths should use the inline resolver or plugin bootstrap: ${commandText.substring(0, 80)}...`
+              );
            }
          }
        }
--- a/tests/integration/hooks.test.js
+++ b/tests/integration/hooks.test.js
@@ -110,24 +110,70 @@ function runHookCommand(command, input = {}, env = {}, timeoutMs = 10000) {
  return new Promise((resolve, reject) => {
    const isWindows = process.platform === 'win32';
    const mergedEnv = { ...process.env, CLAUDE_PLUGIN_ROOT: REPO_ROOT, ...env };
+    if (Array.isArray(command)) {
+      const [program, ...args] = command;
+      const proc = spawn(program, args, { env: mergedEnv, stdio: ['pipe', 'pipe', 'pipe'] });
+
+      let stdout = '';
+      let stderr = '';
+      let timer;
+
+      proc.stdout.on('data', data => stdout += data);
+      proc.stderr.on('data', data => stderr += data);
+
+      proc.stdin.on('error', (err) => {
+        if (err.code !== 'EPIPE' && err.code !== 'EOF') {
+          if (timer) clearTimeout(timer);
+          reject(err);
+        }
+      });
+
+      if (input && Object.keys(input).length > 0) {
+        proc.stdin.write(JSON.stringify(input));
+      }
+      proc.stdin.end();
+
+      timer = setTimeout(() => {
+        proc.kill(isWindows ? undefined : 'SIGKILL');
+        reject(new Error(`Hook command timed out after ${timeoutMs}ms`));
+      }, timeoutMs);
+
+      proc.on('close', code => {
+        clearTimeout(timer);
+        resolve({ code, stdout, stderr });
+      });
+
+      proc.on('error', err => {
+        clearTimeout(timer);
+        reject(err);
+      });
+      return;
+    }
+
    const resolvedCommand = command.replace(
      /\$\{([A-Z_][A-Z0-9_]*)\}/g,
      (_, name) => String(mergedEnv[name] || '')
    );

-    const nodeMatch = resolvedCommand.match(/^node\s+"([^"]+)"\s*(.*)$/);
-    const useDirectNodeSpawn = Boolean(nodeMatch);
+    const inlineNodeMatch = resolvedCommand.match(/^node -e "((?:[^"\\]|\\.)*)"(?:\s+(.*))?$/s);
+    const fileNodeMatch = resolvedCommand.match(/^node\s+"([^"]+)"\s*(.*)$/);
+    const useDirectNodeSpawn = Boolean(inlineNodeMatch || fileNodeMatch);
    const shell = isWindows ? 'cmd' : 'bash';
    const shellArgs = isWindows ? ['/d', '/s', '/c', resolvedCommand] : ['-lc', resolvedCommand];
-    const nodeArgs = nodeMatch
-      ? [
-          nodeMatch[1],
-          ...Array.from(
-            nodeMatch[2].matchAll(/"([^"]*)"|(\S+)/g),
-            m => m[1] !== undefined ? m[1] : m[2]
-          )
-        ]
-      : [];
+    const splitArgs = value => Array.from(
+      String(value || '').matchAll(/"([^"]*)"|(\S+)/g),
+      m => m[1] !== undefined ? m[1] : m[2]
+    );
+    const unescapeInlineJs = value => value
+      .replace(/\\\\/g, '\\')
+      .replace(/\\"/g, '"')
+      .replace(/\\n/g, '\n')
+      .replace(/\\t/g, '\t');
+    const nodeArgs = inlineNodeMatch
+      ? ['-e', unescapeInlineJs(inlineNodeMatch[1]), ...splitArgs(inlineNodeMatch[2])]
+      : fileNodeMatch
+        ? [fileNodeMatch[1], ...splitArgs(fileNodeMatch[2])]
+        : [];

    const proc = useDirectNodeSpawn
      ? spawn('node', nodeArgs, { env: mergedEnv, stdio: ['pipe', 'pipe', 'pipe'] })
@@ -899,16 +945,22 @@ async function runTests() {
    assert.ok(asyncHook.hooks[0].timeout > 0, 'Timeout should be positive');

    const command = asyncHook.hooks[0].command;
-    const isNodeInline = command.startsWith('node -e');
-    const isNodeScript = command.startsWith('node "');
+    const commandText = Array.isArray(command) ? command.join(' ') : command;
+    const isNodeInline =
+      (Array.isArray(command) && command[0] === 'node' && command[1] === '-e') ||
+      commandText.startsWith('node -e');
+    const isNodeScript =
+      (Array.isArray(command) && command[0] === 'node' && typeof command[1] === 'string' && command[1].endsWith('.js')) ||
+      commandText.startsWith('node "');
    const isShellWrapper =
-      command.startsWith('bash "') ||
-      command.startsWith('sh "') ||
-      command.startsWith('bash -lc ') ||
-      command.startsWith('sh -c ');
+      (Array.isArray(command) && (command[0] === 'bash' || command[0] === 'sh')) ||
+      commandText.startsWith('bash "') ||
+      commandText.startsWith('sh "') ||
+      commandText.startsWith('bash -lc ') ||
+      commandText.startsWith('sh -c ');
    assert.ok(
      isNodeInline || isNodeScript || isShellWrapper,
-      `Async hook command should be runnable (node -e, node script, or shell wrapper), got: ${command.substring(0, 80)}`
+      `Async hook command should be runnable (node -e, node script, or shell wrapper), got: ${commandText.substring(0, 80)}`
    );
  })) passed++; else failed++;

@@ -920,19 +972,28 @@ async function runTests() {
        for (const hook of hookDef.hooks) {
          assert.ok(hook.command, `Hook in ${hookType} should have command field`);

-          const isInline = hook.command.startsWith('node -e');
-          const isFilePath = hook.command.startsWith('node "');
-          const isNpx = hook.command.startsWith('npx ');
+          const command = hook.command;
+          const commandText = Array.isArray(command) ? command.join(' ') : command;
+          const isInline =
+            (Array.isArray(command) && command[0] === 'node' && command[1] === '-e') ||
+            commandText.startsWith('node -e');
+          const isFilePath =
+            (Array.isArray(command) && command[0] === 'node' && typeof command[1] === 'string' && command[1].endsWith('.js')) ||
+            commandText.startsWith('node "');
+          const isNpx = (Array.isArray(command) && command[0] === 'npx') || commandText.startsWith('npx ');
          const isShellWrapper =
-            hook.command.startsWith('bash "') ||
-            hook.command.startsWith('sh "') ||
-            hook.command.startsWith('bash -lc ') ||
-            hook.command.startsWith('sh -c ');
-          const isShellScriptPath = hook.command.endsWith('.sh');
+            (Array.isArray(command) && (command[0] === 'bash' || command[0] === 'sh')) ||
+            commandText.startsWith('bash "') ||
+            commandText.startsWith('sh "') ||
+            commandText.startsWith('bash -lc ') ||
+            commandText.startsWith('sh -c ');
+          const isShellScriptPath =
+            (Array.isArray(command) && typeof command[0] === 'string' && command[0].endsWith('.sh')) ||
+            commandText.endsWith('.sh');

          assert.ok(
            isInline || isFilePath || isNpx || isShellWrapper || isShellScriptPath,
-            `Hook command in ${hookType} should be node -e, node script, npx, or shell wrapper/script, got: ${hook.command.substring(0, 80)}`
+            `Hook command in ${hookType} should be node -e, node script, npx, or shell wrapper/script, got: ${commandText.substring(0, 80)}`
          );
        }
      }
--- a/tests/scripts/install-apply.test.js
+++ b/tests/scripts/install-apply.test.js
@@ -350,7 +350,7 @@ function runTests() {
    }
  })) passed++; else failed++;

-  if (test('resolves CLAUDE_PLUGIN_ROOT placeholders in installed claude hooks', () => {
+  if (test('installs claude hooks with the safe plugin bootstrap contract', () => {
    const homeDir = createTempDir('install-apply-home-');
    const projectDir = createTempDir('install-apply-project-');

@@ -361,18 +361,24 @@ function runTests() {
      const claudeRoot = path.join(homeDir, '.claude');
      const installedHooks = readJson(path.join(claudeRoot, 'hooks', 'hooks.json'));

-      const normSep = (s) => s.replace(/\\/g, '/');
-      const expectedFragment = normSep(path.join(claudeRoot, 'scripts', 'hooks', 'auto-tmux-dev.js'));
-
      const installedAutoTmuxEntry = installedHooks.hooks.PreToolUse.find(entry => entry.id === 'pre:bash:auto-tmux-dev');
      assert.ok(installedAutoTmuxEntry, 'hooks/hooks.json should include the auto tmux hook');
+      assert.ok(Array.isArray(installedAutoTmuxEntry.hooks[0].command), 'hooks/hooks.json should install argv-form commands for cross-platform safety');
      assert.ok(
-        normSep(installedAutoTmuxEntry.hooks[0].command).includes(expectedFragment),
-        'hooks/hooks.json should use the installed Claude root for hook commands'
+        installedAutoTmuxEntry.hooks[0].command[0] === 'node' && installedAutoTmuxEntry.hooks[0].command[1] === '-e',
+        'hooks/hooks.json should use the inline node bootstrap contract'
      );
      assert.ok(
-        !installedAutoTmuxEntry.hooks[0].command.includes('${CLAUDE_PLUGIN_ROOT}'),
-        'hooks/hooks.json should not retain CLAUDE_PLUGIN_ROOT placeholders after install'
+        installedAutoTmuxEntry.hooks[0].command.some(part => String(part).includes('plugin-hook-bootstrap.js')),
+        'hooks/hooks.json should route plugin-managed hooks through the shared bootstrap'
+      );
+      assert.ok(
+        installedAutoTmuxEntry.hooks[0].command.some(part => String(part).includes('CLAUDE_PLUGIN_ROOT')),
+        'hooks/hooks.json should still consult CLAUDE_PLUGIN_ROOT for runtime resolution'
+      );
+      assert.ok(
+        !installedAutoTmuxEntry.hooks[0].command.some(part => String(part).includes('${CLAUDE_PLUGIN_ROOT}')),
+        'hooks/hooks.json should not retain raw CLAUDE_PLUGIN_ROOT shell placeholders after install'
      );
    } finally {
      cleanup(homeDir);