Merge pull request #4 from VarunGore36/fix/reviewer-followups

fix(security): security follow-ups for worker, installer, claw, hooks

scripts/claw.js

@@ -95,29 +95,54 @@ function askClaude(systemPrompt, history, userMessage, model) {
   }
   args.push('-p');
 
-  // SECURITY: never use shell:true — on Windows Node concatenates command+args
-  // unquoted (DEP0190), so a model value like `x & calc &` breaks out.
-  // Validate the model token and spawn without a shell; resolve .cmd shim explicitly.
+  // SECURITY: a model value like `x & calc &` breaks out when Node
+  // concatenates command+args unquoted under cmd.exe (DEP0190), so the model
+  // token is validated and only fixed flags reach the command line.
   if (model && !/^[A-Za-z0-9][A-Za-z0-9._:-]{0,63}$/.test(model)) {
     return `[Error: invalid model name]`;
   }
+  // On Windows the `claude` binary is usually a .cmd shim, which Node
+  // >=18.20/20.12 refuses to spawn directly (CVE-2024-27980 mitigation), and
+  // .ps1 shims are not directly executable at all. Resolve a natively
+  // executable target first; only .cmd/.bat go through cmd.exe, using the
+  // same quoted-command-line pattern as scripts/hooks/mcp-health-check.js so
+  // space-containing paths survive as single tokens. .ps1 is never executed
+  // directly — fall through to bare `claude` (pre-change behavior) instead.
+  // cmd.exe expands %NAME% even inside double-quoted strings, so reject
+  // percent-delimited executable paths rather than route them through the shell.
+  function quoteWinToken(token) {
+    if (/%/.test(token)) return null;
+    return /[\s"&|<>^();]/.test(token) ? '"' + token.replace(/"/g, '""') + '"' : token;
+  }
   let bin = 'claude';
+  let useShell = false;
   if (process.platform === 'win32') {
-    for (const ext of ['.cmd', '.exe', '.ps1']) {
+    const { spawnSync: spawnWhere } = require('child_process');
+    for (const ext of ['.exe', '.cmd', '.bat']) {
+      let found = null;
       try {
-        const found = require('child_process').spawnSync('where', [`claude${ext}`], { encoding: 'utf8' });
-        if (found.status === 0 && found.stdout.trim()) { bin = found.stdout.trim().split(/\r?\n/)[0]; break; }
+        found = spawnWhere('where', [`claude${ext}`], { encoding: 'utf8' });
       } catch { /* ignore */ }
+      if (found && found.status === 0 && found.stdout && found.stdout.trim()) {
+        bin = found.stdout.trim().split(/\r?\n/)[0];
+        useShell = /\.(cmd|bat)$/i.test(bin);
+        break;
+      }
+    }
+    if (useShell && quoteWinToken(bin) === null) {
+      useShell = false;
     }
   }
-  const result = spawnSync(bin, args, {
+  const spawnOpts = {
     input: fullPrompt,
     encoding: 'utf8',
     stdio: ['pipe', 'pipe', 'pipe'],
     env: { ...process.env, CLAUDECODE: '' },
     timeout: 300000,
-    shell: false
-  });
+  };
+  const result = useShell
+    ? spawnSync([bin, ...args].map(quoteWinToken).join(' '), { ...spawnOpts, shell: true })
+    : spawnSync(bin, args, { ...spawnOpts, shell: false });
 
   if (result.error) {
     return `[Error: ${result.error.message}]`;

scripts/codex/install-global-git-hooks.sh

@@ -41,17 +41,6 @@ log "Mode: $MODE"
 log "Source hooks: $SOURCE_DIR"
 log "Global hooks destination: $DEST_DIR"
 
-if [[ -d "$DEST_DIR" ]]; then
-  log "Backing up existing hooks directory to $BACKUP_DIR"
-  run_or_echo mkdir -p "$BACKUP_DIR"
-  run_or_echo cp -R "$DEST_DIR" "$BACKUP_DIR/hooks"
-fi
-
-run_or_echo mkdir -p "$DEST_DIR"
-run_or_echo cp "$SOURCE_DIR/pre-commit" "$DEST_DIR/pre-commit"
-run_or_echo cp "$SOURCE_DIR/pre-push" "$DEST_DIR/pre-push"
-run_or_echo chmod +x "$DEST_DIR/pre-commit" "$DEST_DIR/pre-push"
-
 if [[ "$MODE" == "apply" ]]; then
   prev_hooks_path="$(git config --global core.hooksPath || true)"
   if [[ -n "$prev_hooks_path" && "$prev_hooks_path" != "$DEST_DIR" ]]; then
@@ -70,6 +59,17 @@ if [[ "$MODE" == "apply" ]]; then
     log "Restore with: git config --global core.hooksPath \"$prev_hooks_path\""
   fi
 fi
+
+if [[ -d "$DEST_DIR" ]]; then
+  log "Backing up existing hooks directory to $BACKUP_DIR"
+  run_or_echo mkdir -p "$BACKUP_DIR"
+  run_or_echo cp -R "$DEST_DIR" "$BACKUP_DIR/hooks"
+fi
+
+run_or_echo mkdir -p "$DEST_DIR"
+run_or_echo cp "$SOURCE_DIR/pre-commit" "$DEST_DIR/pre-commit"
+run_or_echo cp "$SOURCE_DIR/pre-push" "$DEST_DIR/pre-push"
+run_or_echo chmod +x "$DEST_DIR/pre-commit" "$DEST_DIR/pre-push"
 run_or_echo git config --global core.hooksPath "$DEST_DIR"
 
 log "Installed ECC global git hooks."

scripts/orchestrate-codex-worker.sh

@@ -54,12 +54,15 @@ write_status "running" "- Task file: \`$task_file\`"
 # rm -rf / exfiltration commands without confirmation.
 # Default to the most restrictive approval mode; allow an explicit operator
 # override only via env (e.g. ECC_CODEX_APPROVAL_MODE=on-request for trusted runs).
-APPROVAL_MODE="${ECC_CODEX_APPROVAL_MODE:-never}"
-case "$APPROVAL_MODE" in
+# Codex profiles (-p) and approval policies (--ask-for-approval) are
+# independent concepts. SECURITY: default to never approving untrusted
+# tool execution; operators can override via env.
+APPROVAL_POLICY="${ECC_CODEX_APPROVAL_POLICY:-never}"
+case "$APPROVAL_POLICY" in
   never|on-request|on-failure) ;;
   *)
-    echo "[ECC worker] Refusing to run: unsupported ECC_CODEX_APPROVAL_MODE='$APPROVAL_MODE' (expected never|on-request|on-failure)" >&2
-    write_status "failed" "- Error: unsupported approval mode"
+    echo "[ECC worker] Refusing to run: unsupported ECC_CODEX_APPROVAL_POLICY='$APPROVAL_POLICY' (expected never|on-request|on-failure)" >&2
+    write_status "failed" "- Error: unsupported approval policy"
     exit 1
     ;;
 esac
@@ -106,7 +109,7 @@ Task file: $task_file
 $(cat "$task_file")
 EOF
 
-if codex exec -p "$APPROVAL_MODE" -m gpt-5.4 --color never -C "$(pwd)" -o "$output_file" - < "$prompt_file"; then
+if codex exec --ask-for-approval "$APPROVAL_POLICY" -m gpt-5.4 --color never -C "$(pwd)" -o "$output_file" - < "$prompt_file"; then
   {
     echo "# Handoff"
     echo

src/llm/tools/executor.py

@@ -78,6 +78,8 @@ class ToolExecutor:
                     "Async tool '%s' called via sync execute(); use execute_async()",
                     tool_call.name,
                 )
+                if inspect.iscoroutine(result):
+                    result.close()
                 return ToolResult(
                     tool_call_id=tool_call.id,
                     content=GENERIC_TOOL_FAILURE.format(name=tool_call.name),

tests/scripts/codex-hooks.test.js

@@ -169,6 +169,7 @@ function runHermeticPrePush({
   includeCorepack = true,
   includePnpm = false,
   audit = false,
+  runChecks = true,
 } = {}) {
   const tempDir = createTempDir('codex-pre-push-');
   const binDir = path.join(tempDir, 'bin');
@@ -204,6 +205,7 @@ ${includePnpm ? functionStub('pnpm', false) : ''}
       PATH: toBashPath(binDir),
       BASH_ENV: toBashPath(bashEnv),
       ECC_PREPUSH_AUDIT: audit ? '1' : '0',
+      ECC_PREPUSH_RUN_CHECKS: runChecks ? '1' : '0',
       ECC_SKIP_GIT_HOOKS: '0',
       ECC_SKIP_PREPUSH: '0',
       MSYS_NO_PATHCONV: '1',
@@ -221,7 +223,7 @@ ${includePnpm ? functionStub('pnpm', false) : ''}
 
 if (
   test('pre-push uses Corepack pinned pnpm and runs every required verification script', () => {
-    const { result, calls } = runHermeticPrePush();
+    const { result, calls } = runHermeticPrePush({ runChecks: true });
     assert.strictEqual(result.status, 0, JSON.stringify(result, null, 2));
     assert.deepStrictEqual(calls, [
       'pnpm run lint',
@@ -264,7 +266,7 @@ else failed++;
 
 if (
   test('pre-push stops immediately when a required verification script fails', () => {
-    const { result, calls } = runHermeticPrePush({ failScript: 'typecheck' });
+    const { result, calls } = runHermeticPrePush({ runChecks: true, failScript: 'typecheck' });
     assert.notStrictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`);
     assert.deepStrictEqual(calls, ['pnpm run lint', 'pnpm run typecheck']);
     assert.match(result.stderr, /typecheck failed/);
@@ -273,9 +275,20 @@ if (
   passed++;
 else failed++;
 
+if (
+  test('pre-push skips verification scripts by default when opt-in is not set', () => {
+    const { result, calls } = runHermeticPrePush({ runChecks: false });
+    assert.strictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`);
+    assert.deepStrictEqual(calls, []);
+    assert.match(result.stderr, /ECC_PREPUSH_RUN_CHECKS!=1/);
+  })
+)
+  passed++;
+else failed++;
+
 if (
   test('pre-push runs the production audit through Corepack pnpm', () => {
-    const { result, calls } = runHermeticPrePush({ audit: true });
+    const { result, calls } = runHermeticPrePush({ runChecks: true, audit: true });
     assert.strictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`);
     assert.deepStrictEqual(calls, [
       'pnpm run lint',