feat: enhance hook templates management by adding protection category and improving command safety

2026-03-03 15:43:11 +08:00 · 2026-03-03 11:33:00 +08:00
parent 9cfd5c05fc
commit 08564d487a
6 changed files with 117 additions and 132 deletions
--- a/ccw/frontend/src/components/hook/HookQuickTemplates.tsx
+++ b/ccw/frontend/src/components/hook/HookQuickTemplates.tsx
@@ -274,7 +274,8 @@ function getCategoryName(category: TemplateCategory, formatMessage: ReturnType<t
    notification: formatMessage({ id: 'cliHooks.templates.categories.notification' }),
    indexing: formatMessage({ id: 'cliHooks.templates.categories.indexing' }),
    automation: formatMessage({ id: 'cliHooks.templates.categories.automation' }),
-    utility: formatMessage({ id: 'cliHooks.templates.categories.utility' })
+    utility: formatMessage({ id: 'cliHooks.templates.categories.utility' }),
+    protection: formatMessage({ id: 'cliHooks.templates.categories.protection' }),
  };
  return names[category];
 }
@@ -304,7 +305,7 @@ export function HookQuickTemplates({
  }, []);

  // Define category order
-  const categoryOrder: TemplateCategory[] = ['notification', 'indexing', 'automation'];
+  const categoryOrder: TemplateCategory[] = ['notification', 'indexing', 'automation', 'protection', 'utility'];

  const handleInstall = async (templateId: string) => {
    await onInstallTemplate(templateId);
--- a/ccw/frontend/src/components/hook/HookWizard.tsx
+++ b/ccw/frontend/src/components/hook/HookWizard.tsx
@@ -79,12 +79,6 @@ interface SkillContextConfig {
 // All templates use `ccw hook template exec <id> --stdin` format
 // This avoids Windows Git Bash quote handling issues

-interface HookTemplate {
-  event: string;
-  matcher: string;
-  timeout?: number;
-}
-
 // Template IDs that map to backend templates
 const TEMPLATE_IDS = {
  'memory-update-queue': 'memory-auto-compress',
@@ -106,72 +100,6 @@ const DANGER_OPTIONS = [
  { id: 'permission-change', templateId: 'danger-permission-change', labelKey: 'cliHooks.wizards.dangerProtection.options.permissionChange', descKey: 'cliHooks.wizards.dangerProtection.options.permissionChangeDesc' },
 ] as const;

-// ========== convertToClaudeCodeFormat (ported from old hook-manager.js) ==========
-
-function convertToClaudeCodeFormat(hookData: {
-  command: string;
-  args: string[];
-  matcher?: string;
-  timeout?: number;
-}): Record<string, unknown> {
-  let commandStr = hookData.command || '';
-
-  if (hookData.args && Array.isArray(hookData.args)) {
-    if (commandStr === 'bash' && hookData.args.length >= 2 && hookData.args[0] === '-c') {
-      const script = hookData.args[1];
-      const escapedScript = script.replace(/'/g, "'\\''");
-      commandStr = `bash -c '${escapedScript}'`;
-      if (hookData.args.length > 2) {
-        const additionalArgs = hookData.args.slice(2).map(arg =>
-          arg.includes(' ') && !arg.startsWith('"') && !arg.startsWith("'")
-            ? `"${arg.replace(/"/g, '\\"')}"`
-            : arg
-        );
-        commandStr += ' ' + additionalArgs.join(' ');
-      }
-    } else if (commandStr === 'node' && hookData.args.length >= 2 && hookData.args[0] === '-e') {
-      const script = hookData.args[1];
-      const isWindows = typeof navigator !== 'undefined' && navigator.userAgent.includes('Win');
-      if (isWindows) {
-        const escapedScript = script.replace(/"/g, '\\"');
-        commandStr = `node -e "${escapedScript}"`;
-      } else {
-        const escapedScript = script.replace(/'/g, "'\\''");
-        commandStr = `node -e '${escapedScript}'`;
-      }
-      if (hookData.args.length > 2) {
-        const additionalArgs = hookData.args.slice(2).map(arg =>
-          arg.includes(' ') && !arg.startsWith('"') && !arg.startsWith("'")
-            ? `"${arg.replace(/"/g, '\\"')}"`
-            : arg
-        );
-        commandStr += ' ' + additionalArgs.join(' ');
-      }
-    } else {
-      const quotedArgs = hookData.args.map(arg =>
-        arg.includes(' ') && !arg.startsWith('"') && !arg.startsWith("'")
-          ? `"${arg.replace(/"/g, '\\"')}"`
-          : arg
-      );
-      commandStr = `${commandStr} ${quotedArgs.join(' ')}`.trim();
-    }
-  }
-
-  const converted: Record<string, unknown> = {
-    hooks: [{
-      type: 'command',
-      command: commandStr,
-      ...(hookData.timeout ? { timeout: Math.ceil(hookData.timeout / 1000) } : {}),
-    }],
-  };
-
-  if (hookData.matcher) {
-    converted.matcher = hookData.matcher;
-  }
-
-  return converted;
-}
-
 // ========== Wizard Definitions ==========

 const WIZARD_METADATA = {
--- a/ccw/frontend/src/pages/HookManagerPage.tsx
+++ b/ccw/frontend/src/pages/HookManagerPage.tsx
@@ -28,7 +28,6 @@ import { Card } from '@/components/ui/Card';
 import { Badge } from '@/components/ui/Badge';
 import { HookCard, HookFormDialog, HookQuickTemplates, HookWizard, type HookCardData, type HookFormData, type HookTriggerType, HOOK_TEMPLATES, type WizardType } from '@/components/hook';
 import { useHooks, useToggleHook } from '@/hooks';
-import { installHookTemplate } from '@/lib/api';
 import { cn } from '@/lib/utils';

 // ========== Types ==========
--- a/ccw/src/commands/hook.ts
+++ b/ccw/src/commands/hook.ts
@@ -745,8 +745,8 @@ async function templateAction(subcommand: string, args: string[], options: HookO
    listTemplatesByCategory,
    executeTemplate,
    installTemplateToSettings,
-    type HookInputData,
  } = await import('../core/hooks/hook-templates.js');
+  type HookInputData = import('../core/hooks/hook-templates.js').HookInputData;

  switch (subcommand) {
    case 'list': {
--- a/ccw/src/core/hooks/hook-templates.ts
+++ b/ccw/src/core/hooks/hook-templates.ts
@@ -12,7 +12,7 @@

 import { spawnSync } from 'child_process';
 import { existsSync, readFileSync, writeFileSync } from 'fs';
-import { join } from 'path';
+import { join, resolve, basename } from 'path';
 import { homedir } from 'os';

 // ============================================================================
@@ -79,22 +79,24 @@ export interface HookOutput {
 // ============================================================================

 /**
- * Send notification to dashboard via HTTP
+ * Send notification to dashboard via HTTP (using native fetch)
 */
 function notifyDashboard(type: string, payload: Record<string, unknown>): void {
-  const data = JSON.stringify({
+  const data = {
    type,
    ...payload,
    project: process.env.CLAUDE_PROJECT_DIR || process.cwd(),
    timestamp: Date.now(),
-  });
+  };

-  spawnSync('curl', [
-    '-s', '-X', 'POST',
-    '-H', 'Content-Type: application/json',
-    '-d', data,
-    'http://localhost:3456/api/hook'
-  ], { stdio: 'inherit', shell: true });
+  // Use native fetch (Node.js 18+) to avoid shell command injection
+  fetch('http://localhost:3456/api/hook', {
+    method: 'POST',
+    headers: { 'Content-Type': 'application/json' },
+    body: JSON.stringify(data),
+  }).catch(() => {
+    // Silently ignore errors - dashboard may not be running
+  });
 }

 /**
@@ -144,6 +146,80 @@ function isDangerousGitCommand(cmd: string): boolean {
  return patterns.some(p => p.test(cmd));
 }

+/**
+ * Safely extract string value from unknown input
+ */
+function getStringInput(value: unknown): string {
+  if (typeof value === 'string') {
+    return value;
+  }
+  return '';
+}
+
+/**
+ * Validate file path to prevent command injection
+ * Returns null if path is invalid, otherwise returns the sanitized path
+ */
+function validateFilePath(filePath: string): string | null {
+  if (!filePath || typeof filePath !== 'string') {
+    return null;
+  }
+
+  // Check for dangerous characters that could be used for command injection
+  const dangerousPatterns = /[;&|`$(){}[\]<>!\\]/;
+  if (dangerousPatterns.test(filePath)) {
+    return null;
+  }
+
+  // Check for path traversal attempts
+  if (filePath.includes('..')) {
+    return null;
+  }
+
+  // Check for null bytes
+  if (filePath.includes('\0')) {
+    return null;
+  }
+
+  return filePath;
+}
+
+/**
+ * Safe spawnSync wrapper that avoids shell: true to prevent command injection
+ * On Windows, this uses .cmd extension for npm/npx commands
+ */
+function safeSpawnSync(command: string, args: string[]): { stdout: string; stderr: string; status: number | null } {
+  // Use spawnSync without shell to avoid command injection
+  // Note: On Windows, npx/npm/git may need .cmd extension
+  const isWindows = process.platform === 'win32';
+  const execCommand = isWindows && !command.endsWith('.cmd') && ['npx', 'npm', 'git', 'ccw'].includes(command)
+    ? `${command}.cmd`
+    : command;
+
+  return spawnSync(execCommand, args, {
+    stdio: ['inherit', 'pipe', 'pipe'],
+    shell: false,
+    encoding: 'utf8',
+    cwd: process.cwd(),
+  });
+}
+
+/**
+ * Safe spawnSync with inherited stdio (for tools that need interactive output)
+ */
+function safeSpawnSyncInherit(command: string, args: string[]): void {
+  const isWindows = process.platform === 'win32';
+  const execCommand = isWindows && !command.endsWith('.cmd') && ['npx', 'npm', 'git', 'ccw'].includes(command)
+    ? `${command}.cmd`
+    : command;
+
+  spawnSync(execCommand, args, {
+    stdio: 'inherit',
+    shell: false,
+    cwd: process.cwd(),
+  });
+}
+
 /**
 * Check if file is in protected system paths
 */
@@ -187,8 +263,8 @@ export const HOOK_TEMPLATES: HookTemplate[] = [
    trigger: 'PostToolUse',
    matcher: 'Write|Edit',
    execute: (data) => {
-      const file = (data.tool_input?.file_path as string) || '';
-      if (/workflow-session\.json$|session-metadata\.json$/.test(file)) {
+      const file = getStringInput(data.tool_input?.file_path);
+      if (file && /workflow-session\.json$|session-metadata\.json$/.test(file)) {
        try {
          if (existsSync(file)) {
            const content = readFileSync(file, 'utf8');
@@ -239,9 +315,10 @@ export const HOOK_TEMPLATES: HookTemplate[] = [
    trigger: 'PostToolUse',
    matcher: 'Write|Edit',
    execute: (data) => {
-      const file = (data.tool_input?.file_path as string) || '';
+      const rawFile = getStringInput(data.tool_input?.file_path);
+      const file = validateFilePath(rawFile);
      if (file) {
-        spawnSync('npx', ['prettier', '--write', file], { stdio: 'inherit', shell: true });
+        safeSpawnSyncInherit('npx', ['prettier', '--write', file]);
      }
      return { exitCode: 0 };
    }
@@ -254,9 +331,10 @@ export const HOOK_TEMPLATES: HookTemplate[] = [
    trigger: 'PostToolUse',
    matcher: 'Write|Edit',
    execute: (data) => {
-      const file = (data.tool_input?.file_path as string) || '';
+      const rawFile = getStringInput(data.tool_input?.file_path);
+      const file = validateFilePath(rawFile);
      if (file) {
-        spawnSync('npx', ['eslint', '--fix', file], { stdio: 'inherit', shell: true });
+        safeSpawnSyncInherit('npx', ['eslint', '--fix', file]);
      }
      return { exitCode: 0 };
    }
@@ -268,7 +346,7 @@ export const HOOK_TEMPLATES: HookTemplate[] = [
    category: 'automation',
    trigger: 'Stop',
    execute: () => {
-      spawnSync('git', ['add', '-u'], { stdio: 'inherit', shell: true });
+      safeSpawnSyncInherit('git', ['add', '-u']);
      return { exitCode: 0 };
    }
  },
@@ -282,8 +360,8 @@ export const HOOK_TEMPLATES: HookTemplate[] = [
    trigger: 'PreToolUse',
    matcher: 'Write|Edit',
    execute: (data) => {
-      const file = (data.tool_input?.file_path as string) || '';
-      if (isSensitiveFile(file)) {
+      const file = getStringInput(data.tool_input?.file_path);
+      if (file && isSensitiveFile(file)) {
        return {
          exitCode: 2,
          stderr: `Blocked: modifying sensitive file ${file}`,
@@ -324,9 +402,9 @@ export const HOOK_TEMPLATES: HookTemplate[] = [
    trigger: 'PreToolUse',
    matcher: 'Write|Edit',
    execute: (data) => {
-      const file = (data.tool_input?.file_path as string) || '';
+      const file = getStringInput(data.tool_input?.file_path);
      const protectedPatterns = /\.env|\.git\/|package-lock\.json|yarn\.lock|\.credentials|secrets|id_rsa|\.pem$|\.key$/i;
-      if (protectedPatterns.test(file)) {
+      if (file && protectedPatterns.test(file)) {
        return {
          exitCode: 2,
          jsonOutput: {
@@ -431,8 +509,8 @@ export const HOOK_TEMPLATES: HookTemplate[] = [
          };
        }
      } else {
-        const file = (data.tool_input?.file_path as string) || '';
-        if (isSystemPath(file)) {
+        const file = getStringInput(data.tool_input?.file_path);
+        if (file && isSystemPath(file)) {
          return {
            exitCode: 2,
            jsonOutput: {
@@ -483,7 +561,7 @@ export const HOOK_TEMPLATES: HookTemplate[] = [
    trigger: 'PostToolUse',
    matcher: 'Write|Edit',
    execute: (data) => {
-      const file = (data.tool_input?.file_path as string) || '';
+      const file = getStringInput(data.tool_input?.file_path);
      if (file) {
        notifyDashboard('FILE_MODIFIED', { file });
      }
@@ -512,7 +590,7 @@ export const HOOK_TEMPLATES: HookTemplate[] = [
    category: 'utility',
    trigger: 'Stop',
    execute: () => {
-      spawnSync('ccw', ['memory', 'consolidate', '--threshold', '50'], { stdio: 'inherit', shell: true });
+      safeSpawnSyncInherit('ccw', ['memory', 'consolidate', '--threshold', '50']);
      return { exitCode: 0 };
    }
  },
@@ -523,7 +601,7 @@ export const HOOK_TEMPLATES: HookTemplate[] = [
    category: 'utility',
    trigger: 'SessionStart',
    execute: () => {
-      spawnSync('ccw', ['memory', 'preview', '--include-native'], { stdio: 'inherit', shell: true });
+      safeSpawnSyncInherit('ccw', ['memory', 'preview', '--include-native']);
      return { exitCode: 0 };
    }
  },
@@ -534,7 +612,7 @@ export const HOOK_TEMPLATES: HookTemplate[] = [
    category: 'utility',
    trigger: 'SessionStart',
    execute: () => {
-      spawnSync('ccw', ['memory', 'status'], { stdio: 'inherit', shell: true });
+      safeSpawnSyncInherit('ccw', ['memory', 'status']);
      return { exitCode: 0 };
    }
  },
@@ -545,7 +623,7 @@ export const HOOK_TEMPLATES: HookTemplate[] = [
    category: 'utility',
    trigger: 'Stop',
    execute: () => {
-      spawnSync('ccw', ['core-memory', 'extract', '--max-sessions', '10'], { stdio: 'inherit', shell: true });
+      safeSpawnSyncInherit('ccw', ['core-memory', 'extract', '--max-sessions', '10']);
      return { exitCode: 0 };
    }
  },
@@ -556,14 +634,11 @@ export const HOOK_TEMPLATES: HookTemplate[] = [
    category: 'utility',
    trigger: 'Stop',
    execute: () => {
-      const result = spawnSync('ccw', ['core-memory', 'extract', '--json'], {
-        encoding: 'utf8',
-        shell: true
-      });
+      const result = safeSpawnSync('ccw', ['core-memory', 'extract', '--json']);
      try {
        const d = JSON.parse(result.stdout);
        if (d && d.total_stage1 >= 5) {
-          spawnSync('ccw', ['core-memory', 'consolidate'], { stdio: 'inherit', shell: true });
+          safeSpawnSyncInherit('ccw', ['core-memory', 'consolidate']);
        }
      } catch {
        // Ignore parse errors
@@ -681,8 +756,8 @@ export function installTemplateToSettings(
  }

  // Check if already installed
-  const triggerHooks = hooks[template.trigger];
-  const alreadyInstalled = triggerHooks.some((h: Record<string, unknown>) =>
+  const triggerHooks = hooks[template.trigger] as Array<Record<string, unknown>>;
+  const alreadyInstalled = triggerHooks.some((h) =>
    h._templateId === templateId
  );

--- a/ccw/src/scripts/migrate-hook-templates.ts
+++ b/ccw/src/scripts/migrate-hook-templates.ts
@@ -16,6 +16,7 @@ import { join } from 'path';
 interface OldHookEntry {
  matcher?: string;
  command?: string;
+  _templateId?: string;
  hooks?: Array<{
    type?: string;
    command?: string;
@@ -29,56 +30,45 @@ interface Settings {

 // Command patterns that indicate old-style inline scripts
 const OLD_PATTERNS = [
-  // Bash inline with jq
  /bash\s+-c.*jq/,
-  // Node inline with complex scripts
  /node\s+-e.*child_process/,
  /node\s+-e.*spawnSync/,
-  // Long inline commands
  /command.*node -e ".*\{.*\}.*"/,
 ];

 // Mapping from old patterns to new template IDs
 const MIGRATION_MAP: Record<string, string> = {
-  // Danger protection patterns
  'danger-bash-confirm': 'danger-bash-confirm',
  'danger-file-protection': 'danger-file-protection',
  'danger-git-destructive': 'danger-git-destructive',
  'danger-network-confirm': 'danger-network-confirm',
  'danger-system-paths': 'danger-system-paths',
  'danger-permission-change': 'danger-permission-change',
-  // Memory patterns
  'memory-update-queue': 'memory-auto-compress',
  'memory-v2-extract': 'memory-v2-extract',
-  // Notification patterns
  'session-start-notify': 'session-start-notify',
  'stop-notify': 'stop-notify',
  'session-state-watch': 'session-state-watch',
-  // Automation patterns
  'auto-format-on-write': 'auto-format-on-write',
  'auto-lint-on-write': 'auto-lint-on-write',
  'block-sensitive-files': 'block-sensitive-files',
  'git-auto-stage': 'git-auto-stage',
-  // Utility patterns
+  'post-edit-index': 'post-edit-index',
  'memory-preview-extract': 'memory-preview-extract',
  'memory-status-check': 'memory-status-check',
-  'post-edit-index': 'post-edit-index',
 };

 function detectTemplateFromCommand(command: string): string | null {
-  // Check for explicit template ID patterns
  for (const [pattern, templateId] of Object.entries(MIGRATION_MAP)) {
    if (command.includes(pattern)) {
      return templateId;
    }
  }

-  // Check for jq usage in bash (indicates old-style danger detection)
  if (command.includes('jq -r') && command.includes('DANGEROUS_PATTERNS')) {
    return 'danger-bash-confirm';
  }

-  // Check for curl to localhost:3456 (dashboard notification)
  if (command.includes('localhost:3456/api/hook')) {
    if (command.includes('SESSION_CREATED')) return 'session-start-notify';
    if (command.includes('TASK_COMPLETED')) return 'stop-notify';
@@ -86,22 +76,18 @@ function detectTemplateFromCommand(command: string): string | null {
    if (command.includes('SESSION_STATE_CHANGED')) return 'session-state-watch';
  }

-  // Check for prettier
  if (command.includes('prettier --write')) {
    return 'auto-format-on-write';
  }

-  // Check for eslint
  if (command.includes('eslint --fix')) {
    return 'auto-lint-on-write';
  }

-  // Check for git add
  if (command.includes('git add -u')) {
    return 'git-auto-stage';
  }

-  // Check for sensitive file patterns
  if (command.includes('.env') && command.includes('credential')) {
    return 'block-sensitive-files';
  }
@@ -156,9 +142,8 @@ function migrateSettings(settings: Settings, dryRun: boolean): Settings {
        const migratedEntry = migrateHookEntry(entry, trigger);
        newEntries.push(migratedEntry);
      } else {
-        // Check if already using template approach
        if (entry.hooks?.[0]?.command?.includes('ccw hook template')) {
-          console.log(`  ✓ Already using template: ${entry._templateId || 'unknown'}`);
+          console.log(`  ✓ Already using template: ${(entry as OldHookEntry & { _templateId?: string })._templateId || 'unknown'}`);
        }
        newEntries.push(entry);
      }
@@ -179,7 +164,6 @@ async function main(): Promise<void> {
  if (settingsIndex >= 0 && args[settingsIndex + 1]) {
    settingsPath = args[settingsIndex + 1];
  } else {
-    // Default to project settings
    settingsPath = join(process.cwd(), '.claude', 'settings.json');
  }

@@ -208,12 +192,10 @@ async function main(): Promise<void> {
    console.log('\n📄 Migrated settings (dry run):');
    console.log(JSON.stringify(migrated, null, 2));
  } else {
-    // Backup original
    const backupPath = `${settingsPath}.backup-${Date.now()}`;
    writeFileSync(backupPath, JSON.stringify(settings, null, 2));
    console.log(`\n💾 Backup saved to: ${backupPath}`);

-    // Write migrated
    writeFileSync(settingsPath, JSON.stringify(migrated, null, 2));
    console.log(`\n✅ Settings migrated successfully!`);
  }