diff --git a/install.ps1 b/install.ps1 index 752ac2ec4..e04db118f 100644 --- a/install.ps1 +++ b/install.ps1 @@ -35,12 +35,13 @@ $scriptDir = Split-Path -Parent $scriptPath $installerScript = Join-Path -Path (Join-Path -Path $scriptDir -ChildPath 'scripts') -ChildPath 'install-apply.js' # Auto-install Node dependencies when running from a git clone +# SECURITY: --ignore-scripts blocks preinstall/postinstall RCE from a compromised dependency. $nodeModules = Join-Path -Path $scriptDir -ChildPath 'node_modules' if (-not (Test-Path -LiteralPath $nodeModules)) { Write-Host '[ECC] Installing dependencies...' Push-Location $scriptDir try { - & npm install --no-audit --no-fund --loglevel=error + & npm install --ignore-scripts --no-audit --no-fund --loglevel=error if ($LASTEXITCODE -ne 0) { Write-Error "npm install failed with exit code $LASTEXITCODE" exit $LASTEXITCODE diff --git a/install.sh b/install.sh index f14c057cd..c69a11dd7 100755 --- a/install.sh +++ b/install.sh @@ -14,10 +14,12 @@ while [ -L "$SCRIPT_PATH" ]; do done SCRIPT_DIR="$(cd "$(dirname "$SCRIPT_PATH")" && pwd)" -# Auto-install Node dependencies when running from a git clone +# Auto-install Node dependencies when running from a git clone. +# SECURITY: --ignore-scripts blocks preinstall/postinstall RCE from a +# compromised dependency. ECC deps are pure JS (no native build step). if [ ! -d "$SCRIPT_DIR/node_modules" ]; then echo "[ECC] Installing dependencies..." - (cd "$SCRIPT_DIR" && npm install --no-audit --no-fund --loglevel=error) + (cd "$SCRIPT_DIR" && npm install --ignore-scripts --no-audit --no-fund --loglevel=error) fi # On MSYS2/Git Bash, convert the POSIX path to a Windows path so Node.js diff --git a/scripts/claw.js b/scripts/claw.js index 982ce5c22..74dea0f81 100644 --- a/scripts/claw.js +++ b/scripts/claw.js @@ -95,21 +95,54 @@ function askClaude(systemPrompt, history, userMessage, model) { } args.push('-p'); - // On Windows the `claude` binary installed via npm is `claude.cmd`/`claude.ps1`, - // and Node's spawn() cannot resolve those wrappers via PATH without shell: true. - // But shell mode concatenates args *unescaped*, so a multi-line prompt passed as - // an arg gets mangled (newlines and the `===` section markers truncate it, and - // claude receives an empty prompt). Fix: send the prompt over stdin via `input` - // and keep only the short, safe flags (`--model`, `-p`) as args. - // 'claude' is a hardcoded literal here (not user input), so shell mode is safe. - const result = spawnSync('claude', args, { + // 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') { + const { spawnSync: spawnWhere } = require('child_process'); + for (const ext of ['.exe', '.cmd', '.bat']) { + let found = null; + try { + 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 spawnOpts = { input: fullPrompt, encoding: 'utf8', stdio: ['pipe', 'pipe', 'pipe'], env: { ...process.env, CLAUDECODE: '' }, timeout: 300000, - shell: process.platform === 'win32' - }); + }; + 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}]`; diff --git a/scripts/codex-git-hooks/pre-commit b/scripts/codex-git-hooks/pre-commit index 98c495fef..b4c608c76 100644 --- a/scripts/codex-git-hooks/pre-commit +++ b/scripts/codex-git-hooks/pre-commit @@ -5,12 +5,13 @@ set -euo pipefail # Blocks commits that add high-signal secrets. if [[ "${ECC_SKIP_GIT_HOOKS:-0}" == "1" || "${ECC_SKIP_PRECOMMIT:-0}" == "1" ]]; then + printf '[ECC pre-commit] WARNING: hook bypassed via env (ECC_SKIP_*=1)\n' >&2 exit 0 fi -if [[ -f ".ecc-hooks-disable" || -f ".git/ecc-hooks-disable" ]]; then - exit 0 -fi +# NOTE: file-based disables (.ecc-hooks-disable) were removed — a malicious +# repo could ship that file and silently turn off secret scanning exactly +# where it is most needed. Use the env bypass above (audible warning) instead. if ! git rev-parse --is-inside-work-tree >/dev/null 2>&1; then exit 0 diff --git a/scripts/codex-git-hooks/pre-push b/scripts/codex-git-hooks/pre-push index ff66512a1..10fdd4f4d 100755 --- a/scripts/codex-git-hooks/pre-push +++ b/scripts/codex-git-hooks/pre-push @@ -5,12 +5,12 @@ set -euo pipefail # Runs a lightweight verification flow before pushes. if [[ "${ECC_SKIP_GIT_HOOKS:-0}" == "1" || "${ECC_SKIP_PREPUSH:-0}" == "1" ]]; then + printf '[ECC pre-push] WARNING: hook bypassed via env (ECC_SKIP_*=1)\n' >&2 exit 0 fi -if [[ -f ".ecc-hooks-disable" || -f ".git/ecc-hooks-disable" ]]; then - exit 0 -fi +# NOTE: file-based disables (.ecc-hooks-disable) were removed — a malicious +# repo could ship that file and silently disable verification. if ! git rev-parse --is-inside-work-tree >/dev/null 2>&1; then exit 0 @@ -85,8 +85,14 @@ run_node_script() { } if [[ -f "package.json" ]]; then - pm="$(detect_pm)" - log "Node project detected (package manager: $pm)" + # SECURITY: executing a cloned repo's lint/test/build scripts on push is + # arbitrary code execution (package.json scripts run as you). Opt-in only: + # set ECC_PREPUSH_RUN_CHECKS=1 for repos you trust. + if [[ "${ECC_PREPUSH_RUN_CHECKS:-0}" != "1" ]]; then + printf '[ECC pre-push] Node project detected but ECC_PREPUSH_RUN_CHECKS!=1; skipping repo script execution (set =1 to opt in).\n' >&2 + else + pm="$(detect_pm)" + log "Node project detected (package manager: $pm)" for script_name in lint typecheck test build; do if has_node_script "$script_name"; then @@ -98,7 +104,9 @@ if [[ -f "package.json" ]]; then fi done + fi if [[ "${ECC_PREPUSH_AUDIT:-0}" == "1" ]]; then + pm="${pm:-$(detect_pm)}" ran_any_check=1 log "Running dependency audit (ECC_PREPUSH_AUDIT=1)" case "$pm" in @@ -111,6 +119,9 @@ if [[ -f "package.json" ]]; then fi fi +# SECURITY: go test / pytest execute repo-controlled code (TestMain, +# conftest.py). Same opt-in gate as Node scripts above. +if [[ "${ECC_PREPUSH_RUN_CHECKS:-0}" == "1" ]]; then if [[ -f "go.mod" ]] && command -v go >/dev/null 2>&1; then ran_any_check=1 log "Go project detected. Running: go test ./..." @@ -281,6 +292,11 @@ if [[ -f "pyproject.toml" || -f "requirements.txt" ]]; then log " venv, env, uv, poetry, PATH). Set ECC_PYTEST_CMD to point at it." fi fi +else + if [[ -f "go.mod" || -f "pyproject.toml" || -f "requirements.txt" ]]; then + log "Go/Python project detected but ECC_PREPUSH_RUN_CHECKS!=1; skipping test execution." + fi +fi if [[ "$ran_any_check" -eq 0 ]]; then diff --git a/scripts/codex/install-global-git-hooks.sh b/scripts/codex/install-global-git-hooks.sh index ea11d8524..22702a5f5 100755 --- a/scripts/codex/install-global-git-hooks.sh +++ b/scripts/codex/install-global-git-hooks.sh @@ -41,6 +41,23 @@ log "Mode: $MODE" log "Source hooks: $SOURCE_DIR" log "Global hooks destination: $DEST_DIR" +prev_hooks_path="$(git config --global core.hooksPath || true)" +if [[ -n "$prev_hooks_path" && "$prev_hooks_path" != "$DEST_DIR" ]]; then + # SECURITY: never silently displace another tool's global hooks — that + # turns every commit/push in every repo into ECC code execution and breaks + # the user's existing security controls. Require explicit opt-in to replace. + if [[ "${ECC_FORCE_GLOBAL_HOOKS:-0}" != "1" ]]; then + log "ERROR: global core.hooksPath already set to: $prev_hooks_path" + log "Refusing to overwrite. Options:" + log " 1) Per-repo install (recommended): git config core.hooksPath \"$DEST_DIR\"" + log " 2) Force replace: ECC_FORCE_GLOBAL_HOOKS=1 $0" + log " 3) Restore afterwards: git config --global core.hooksPath \"$prev_hooks_path\"" + exit 1 + fi + log "WARNING: replacing previous global hooksPath: $prev_hooks_path (ECC_FORCE_GLOBAL_HOOKS=1)" + log "Restore with: git config --global core.hooksPath \"$prev_hooks_path\"" +fi + if [[ -d "$DEST_DIR" ]]; then log "Backing up existing hooks directory to $BACKUP_DIR" run_or_echo mkdir -p "$BACKUP_DIR" @@ -51,15 +68,8 @@ 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" ]]; then - log "Previous global hooksPath: $prev_hooks_path" - fi -fi run_or_echo git config --global core.hooksPath "$DEST_DIR" log "Installed ECC global git hooks." -log "Disable per repo by creating .ecc-hooks-disable in project root." -log "Temporary bypass: ECC_SKIP_PRECOMMIT=1 or ECC_SKIP_PREPUSH=1" +log "Per-repo alternative (recommended): git config core.hooksPath \"$DEST_DIR\"" +log "Temporary bypass (audible): ECC_SKIP_GIT_HOOKS=1 (logs a warning to stderr)" diff --git a/scripts/hooks/mcp-health-check.js b/scripts/hooks/mcp-health-check.js index b78b5ac57..843a6ec04 100644 --- a/scripts/hooks/mcp-health-check.js +++ b/scripts/hooks/mcp-health-check.js @@ -182,6 +182,12 @@ function extractMcpTargetFromRaw(raw) { } function resolveServerConfig(serverName) { + // SECURITY: serverName flows into env-var lookup and shell-adjacent paths. + // Reject anything outside a strict token so config-controlled names cannot + // inject shell metachars ($(..), backticks, ;) downstream. + if (!/^[A-Za-z0-9_-]{1,64}$/.test(String(serverName || ''))) { + return null; + } for (const filePath of configPaths()) { const data = readJsonFile(filePath); const server = data?.mcpServers?.[serverName] @@ -306,9 +312,21 @@ function probeCommandServer(serverName, config) { const command = config.command; const args = Array.isArray(config.args) ? config.args.map(arg => String(arg)) : []; const timeoutMs = envNumber('ECC_MCP_HEALTH_TIMEOUT_MS', DEFAULT_TIMEOUT_MS); + // SECURITY: config.env comes from repo-committed MCP configs. Never let it + // override process-critical loader vars that turn into code execution + // (LD_PRELOAD, DYLD_*, NODE_OPTIONS, PATH tampering, etc.). + const BLOCKED_ENV_PREFIXES = ['LD_', 'DYLD_', 'NODE_OPTIONS', 'NODE_PATH', 'PATH', 'PYTHONPATH', 'RUBYLIB', 'PERL5LIB']; + const rawEnv = (config.env && typeof config.env === 'object' && !Array.isArray(config.env) ? config.env : {}); + const safeConfigEnv = {}; + for (const [k, v] of Object.entries(rawEnv)) { + if (BLOCKED_ENV_PREFIXES.some(p => String(k).toUpperCase().startsWith(p))) { + continue; + } + safeConfigEnv[k] = String(v); + } const mergedEnv = { ...process.env, - ...(config.env && typeof config.env === 'object' && !Array.isArray(config.env) ? config.env : {}) + ...safeConfigEnv }; let done = false; @@ -515,6 +533,38 @@ function probeCommandServer(serverName, config) { async function probeServer(serverName, resolvedConfig) { const config = resolvedConfig.config; + // SECURITY: cloning a malicious repo must not auto-execute its MCP servers. + // Workspace configs (cwd .claude.json / .claude/settings.json) are untrusted + // by default; only probe them with explicit operator opt-in. + // Home configs (~/.claude.json) and explicit ECC_MCP_CONFIG_PATH remain allowed. + try { + const src = String(resolvedConfig.source || ''); + const cwd = process.cwd(); + const home = require('os').homedir(); + const pathMod = require('path'); + // A config file in the user's home directory (~/.claude.json or + // ~/.claude/settings.json) is always trusted regardless of cwd. + const isHomeSource = src === pathMod.join(home, '.claude.json') + || src === pathMod.join(home, '.claude', 'settings.json') + || src.startsWith(pathMod.join(home, '.claude') + pathMod.sep); + if (!isHomeSource) { + const isWorkspaceSource = src === pathMod.join(cwd, '.claude.json') + || src === pathMod.join(cwd, '.claude', 'settings.json') + || src.startsWith(cwd + pathMod.sep + '.claude' + pathMod.sep); + if (isWorkspaceSource && !/^(1|true|yes)$/i.test(String(process.env.ECC_MCP_ALLOW_WORKSPACE_PROBE || ''))) { + return { + ok: false, + failureCode: null, + reason: 'untrusted workspace MCP config skipped (set ECC_MCP_ALLOW_WORKSPACE_PROBE=1 to probe)', + source: resolvedConfig.source + }; + } + } + } catch { + // Fail closed on path errors for workspace sources is handled below; + // continue to normal probing for non-workspace sources. + } + if (config.type === 'http' || config.url) { const result = await requestHttp(config.url, config.headers || {}, envNumber('ECC_MCP_HEALTH_TIMEOUT_MS', DEFAULT_TIMEOUT_MS)); @@ -546,6 +596,15 @@ async function probeServer(serverName, resolvedConfig) { } function reconnectCommand(serverName) { + // SECURITY: reconnect commands are shell strings from env. Disabled by + // default; require explicit opt-in so a malicious .env/direnv cannot gain + // shell execution through this hook. + if (!/^(1|true|yes)$/i.test(String(process.env.ECC_MCP_RECONNECT_ALLOW || ''))) { + return null; + } + if (!/^[A-Za-z0-9_-]{1,64}$/.test(String(serverName || ''))) { + return null; + } const key = `ECC_MCP_RECONNECT_${String(serverName).toUpperCase().replace(/[^A-Z0-9]/g, '_')}`; const command = process.env[key] || process.env.ECC_MCP_RECONNECT_COMMAND || ''; if (!command.trim()) { @@ -563,8 +622,60 @@ function attemptReconnect(serverName) { return { attempted: false, success: false, reason: 'no reconnect command configured' }; } - const result = spawnSync(command, { - shell: true, + // SECURITY: never run reconnect strings through a shell. Split on + // whitespace (no glob/expansion/substitution) and spawn directly. + // Supports single/double quotes for paths with spaces (e.g. node + // "/tmp/dir with space/reconnect.js"). No variable, command, tilde, or + // glob expansion is performed. {server} was already validated above. + function splitReconnectCommand(s) { + const parts = []; + let cur = ''; + let quote = null; + let inToken = false; + for (let i = 0; i < s.length; i++) { + const ch = s[i]; + if (quote) { + if (ch === quote) { + quote = null; + } else if (ch === '\\' && quote === '"' && i + 1 < s.length && (s[i + 1] === '"' || s[i + 1] === '\\')) { + cur += s[i + 1]; + i++; + } else { + cur += ch; + } + } else if (ch === '"' || ch === "'") { + quote = ch; + inToken = true; + } else if (/\s/.test(ch)) { + if (inToken) { + parts.push(cur); + cur = ''; + inToken = false; + } + } else { + cur += ch; + inToken = true; + } + } + if (quote) { + return null; // unbalanced quote + } + if (inToken) { + parts.push(cur); + } + return parts; + } + const parts = splitReconnectCommand(String(command).trim()); + if (!parts || parts.length === 0) { + return { attempted: false, success: false, reason: 'invalid reconnect command' }; + } + const [bin, ...argv] = parts; + if (/[&|<>^%!`$();]/.test(bin) || argv.some(a => /[`$]/.test(a))) { + return { attempted: false, success: false, reason: 'reconnect command contains unsafe characters' }; + } + + const result = spawnSync(bin, argv, { + shell: false, env: process.env, cwd: process.cwd(), encoding: 'utf8', diff --git a/scripts/hooks/run-with-flags-shell.sh b/scripts/hooks/run-with-flags-shell.sh index 227b8fc7b..9599e303f 100755 --- a/scripts/hooks/run-with-flags-shell.sh +++ b/scripts/hooks/run-with-flags-shell.sh @@ -22,9 +22,31 @@ if [[ "$ENABLED" != "yes" ]]; then exit 0 fi -SCRIPT_PATH="${PLUGIN_ROOT}/${REL_SCRIPT_PATH}" -if [[ ! -f "$SCRIPT_PATH" ]]; then - echo "[Hook] Script not found for ${HOOK_ID}: ${SCRIPT_PATH}" >&2 +# Reject traversal / absolute / env-escape paths before touching the filesystem. +# Mirrors the containment check in run-with-flags.js (resolvedRoot prefix). +case "$REL_SCRIPT_PATH" in + /*|\\*|~*|*..*|*\$*|*\`*|*\|*|*\;*|*\&*|*\<*|*\>*|*\"*|*\'*|*\ *|*" "*) + echo "[Hook] Path traversal rejected for ${HOOK_ID}: ${REL_SCRIPT_PATH}" >&2 + printf '%s' "$INPUT" + exit 0 + ;; +esac + +# Canonicalize PLUGIN_ROOT (CLAUDE_PLUGIN_ROOT is env-controlled) and the +# candidate script path, then enforce containment inside the plugin root. +PLUGIN_ROOT_CANON="$(realpath -m "$PLUGIN_ROOT" 2>/dev/null || readlink -f "$PLUGIN_ROOT" 2>/dev/null || printf '%s' "$PLUGIN_ROOT")" +SCRIPT_PATH="${PLUGIN_ROOT_CANON}/${REL_SCRIPT_PATH}" +SCRIPT_CANON="$(realpath -m "$SCRIPT_PATH" 2>/dev/null || readlink -f "$SCRIPT_PATH" 2>/dev/null || printf '%s' "$SCRIPT_PATH")" +case "$SCRIPT_CANON" in + "$PLUGIN_ROOT_CANON"/*) ;; + *) + echo "[Hook] Path traversal rejected for ${HOOK_ID}: ${REL_SCRIPT_PATH}" >&2 + printf '%s' "$INPUT" + exit 0 + ;; +esac +if [[ ! -f "$SCRIPT_CANON" ]]; then + echo "[Hook] Script not found for ${HOOK_ID}: ${SCRIPT_CANON}" >&2 printf '%s' "$INPUT" exit 0 fi @@ -33,4 +55,4 @@ fi # This is needed by scripts like observe.sh that behave differently for PreToolUse vs PostToolUse HOOK_PHASE="${HOOK_ID%%:*}" -printf '%s' "$INPUT" | "$SCRIPT_PATH" "$HOOK_PHASE" +printf '%s' "$INPUT" | "$SCRIPT_CANON" "$HOOK_PHASE" diff --git a/scripts/hooks/run-with-flags.js b/scripts/hooks/run-with-flags.js index c24505adc..4b32bdbe8 100755 --- a/scripts/hooks/run-with-flags.js +++ b/scripts/hooks/run-with-flags.js @@ -227,7 +227,18 @@ async function main() { // which would interfere with the parent process or cause double execution. let hookModule; const src = fs.readFileSync(scriptPath, 'utf8'); - const hasRunExport = /\bmodule\.exports\b/.test(src) && /\brun\b/.test(src); + // Gate require() on concrete export syntax, not a bare word match: the old + // /\bmodule\.exports\b/ && /\brun\b/ test fired on comments, strings, and + // unrelated properties, causing require() — and its module-scope side + // effects — to run for hooks that export no run(). Still lexical (no parser + // dependency), but requires an actual export assignment form. + const RUN_EXPORT_PATTERNS = [ + /module\.exports\s*\.\s*run\s*=/, + /exports\s*\.\s*run\s*=/, + /module\.exports\s*=\s*\{[^}]*\brun\b/, + /module\.exports\s*=\s*(async\s+)?function\s+run\b/, + ]; + const hasRunExport = RUN_EXPORT_PATTERNS.some(re => re.test(src)); if (hasRunExport) { try { diff --git a/scripts/lib/codex-legacy-sync.js b/scripts/lib/codex-legacy-sync.js index 5eb92d180..12cdc392b 100644 --- a/scripts/lib/codex-legacy-sync.js +++ b/scripts/lib/codex-legacy-sync.js @@ -131,8 +131,15 @@ function removeOpenedRegularFile(filePath, opened) { function atomicWriteJson(filePath, value) { fs.mkdirSync(path.dirname(filePath), { recursive: true, mode: 0o700 }); const tempPath = `${filePath}.tmp-${process.pid}-${Date.now()}`; - fs.writeFileSync(tempPath, `${JSON.stringify(value, null, 2)}\n`, { mode: 0o600 }); - fs.renameSync(tempPath, filePath); + try { + fs.writeFileSync(tempPath, `${JSON.stringify(value, null, 2)}\n`, { mode: 0o600 }); + fs.renameSync(tempPath, filePath); + } catch (error) { + // A failed write/rename must not leave a .tmp-- file + // beside the canonical state file; repeated failures would accumulate them. + fs.rmSync(tempPath, { force: true }); + throw error; + } } function readState(statePath) { diff --git a/scripts/orchestrate-codex-worker.sh b/scripts/orchestrate-codex-worker.sh index d73ad0cf2..fde2485ff 100755 --- a/scripts/orchestrate-codex-worker.sh +++ b/scripts/orchestrate-codex-worker.sh @@ -48,6 +48,38 @@ fi write_status "running" "- Task file: \`$task_file\`" +# SECURITY: never auto-approve agent tool execution. The worker prompt is built +# from a task file that may contain LLM-generated or third-party content +# (indirect prompt injection). `codex exec -p yolo` would execute +# 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). +# 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_POLICY='$APPROVAL_POLICY' (expected never|on-request|on-failure)" >&2 + write_status "failed" "- Error: unsupported approval policy" + exit 1 + ;; +esac + +# Contain the task file to the current worktree so a malicious launcher cannot +# point the worker at /etc/passwd or a sibling checkout. +task_real="$(realpath -m "$task_file" 2>/dev/null || readlink -f "$task_file" 2>/dev/null || printf '%s' "$task_file")" +work_real="$(pwd -P 2>/dev/null || pwd)" +case "$task_real" in + "$work_real"/*) ;; + *) + echo "[ECC worker] Refusing to run: task file outside worktree: $task_file" >&2 + write_status "failed" "- Error: task file outside worktree" + exit 1 + ;; +esac + prompt_file="$(mktemp)" output_file="$(mktemp)" cleanup() { @@ -77,7 +109,7 @@ Task file: $task_file $(cat "$task_file") EOF -if codex exec -p yolo -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 diff --git a/src/llm/providers/ollama.py b/src/llm/providers/ollama.py index 257d17900..58505ac7b 100644 --- a/src/llm/providers/ollama.py +++ b/src/llm/providers/ollama.py @@ -8,6 +8,7 @@ from typing import Any from llm.core.interface import ( AuthenticationError, ContextLengthError, + LLMError, LLMProvider, RateLimitError, ) @@ -105,13 +106,33 @@ class OllamaProvider(LLMProvider): ) except Exception as e: msg = str(e) - if "401" in msg or "connection" in msg.lower(): - raise AuthenticationError(f"Ollama connection failed: {msg}", provider=ProviderType.OLLAMA) from e - if "429" in msg or "rate_limit" in msg.lower(): + lowered = msg.lower() + if "401" in msg or "unauthorized" in lowered or "forbidden" in lowered: + raise AuthenticationError(f"Ollama authentication failed: {msg}", provider=ProviderType.OLLAMA) from e + if "429" in msg or "rate_limit" in lowered: raise RateLimitError(msg, provider=ProviderType.OLLAMA) from e - if "context" in msg.lower() and "length" in msg.lower(): + if "context" in lowered and "length" in lowered: raise ContextLengthError(msg, provider=ProviderType.OLLAMA) from e - raise + if ( + "connection" in lowered + or "refused" in lowered + or "timed out" in lowered + or "timeout" in lowered + or "unreachable" in lowered + or "name resolution" in lowered + or "nodename nor servname" in lowered + or isinstance(e, (ConnectionError, TimeoutError)) + ): + raise LLMError( + f"Ollama connection failed: {type(e).__name__}", + provider=ProviderType.OLLAMA, + code="connection_error", + ) from e + raise LLMError( + f"Ollama request failed: {type(e).__name__}", + provider=ProviderType.OLLAMA, + code="provider_error", + ) from e def list_models(self) -> list[ModelInfo]: return self._models.copy() diff --git a/src/llm/tools/executor.py b/src/llm/tools/executor.py index e4a859b34..9d051e2bf 100644 --- a/src/llm/tools/executor.py +++ b/src/llm/tools/executor.py @@ -2,9 +2,12 @@ from __future__ import annotations +import inspect +import logging from collections.abc import Callable from typing import Any +from llm.core.interface import LLMError from llm.core.types import ( LLMInput, LLMOutput, @@ -15,6 +18,21 @@ from llm.core.types import ( ToolResult, ) +logger = logging.getLogger(__name__) + +# Model-facing failure text. Raw exception details (credentials, local paths, +# request data, upstream responses) must never reach the model; diagnostics go +# to trusted logs only. +GENERIC_TOOL_FAILURE = "Error executing {name}: tool failed" + + +def _generic_failure(tool_call: ToolCall) -> ToolResult: + return ToolResult( + tool_call_id=tool_call.id, + content=GENERIC_TOOL_FAILURE.format(name=tool_call.name), + is_error=True, + ) + ToolFunc = Callable[..., Any] @@ -55,18 +73,52 @@ class ToolExecutor: try: result = func(**tool_call.arguments) + if inspect.isawaitable(result): + logger.warning( + "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), + is_error=True, + ) content = result if isinstance(result, str) else str(result) return ToolResult(tool_call_id=tool_call.id, content=content) - except Exception as e: + except Exception: + logger.exception("Tool '%s' failed", tool_call.name) + return _generic_failure(tool_call) + + async def execute_async(self, tool_call: ToolCall) -> ToolResult: + func = self.registry.get(tool_call.name) + if not func: return ToolResult( tool_call_id=tool_call.id, - content=f"Error executing {tool_call.name}: {e}", + content=f"Error: Tool '{tool_call.name}' not found", is_error=True, ) + try: + result = func(**tool_call.arguments) + if inspect.isawaitable(result): + result = await result + content = result if isinstance(result, str) else str(result) + return ToolResult(tool_call_id=tool_call.id, content=content) + except Exception: + logger.exception("Tool '%s' failed", tool_call.name) + return _generic_failure(tool_call) + def execute_all(self, tool_calls: list[ToolCall]) -> list[ToolResult]: return [self.execute(tc) for tc in tool_calls] + async def execute_all_async(self, tool_calls: list[ToolCall]) -> list[ToolResult]: + results: list[ToolResult] = [] + for tc in tool_calls: + results.append(await self.execute_async(tc)) + return results + class ReActAgent: def __init__( @@ -92,7 +144,14 @@ class ReActAgent: tools=tools, ) - output: LLMOutput = self.provider.generate(input_copy) + try: + output: LLMOutput = self.provider.generate(input_copy) + except LLMError as e: + logger.warning("Provider failed during agent run: %s", e.code or type(e).__name__) + return LLMOutput( + content=f"Provider error: {e.code or type(e).__name__}", + stop_reason="provider_error", + ) if not output.has_tool_calls: return output @@ -105,7 +164,7 @@ class ReActAgent: ) ) - results = self.executor.execute_all(output.tool_calls or []) + results = await self.executor.execute_all_async(output.tool_calls or []) for result in results: messages.append( diff --git a/tests/hooks/mcp-health-check.test.js b/tests/hooks/mcp-health-check.test.js index fe86290e4..beb2488e1 100644 --- a/tests/hooks/mcp-health-check.test.js +++ b/tests/hooks/mcp-health-check.test.js @@ -249,6 +249,9 @@ async function runTests() { ECC_MCP_CONFIG_PATH: null, ECC_MCP_HEALTH_STATE_PATH: null, ECC_MCP_HEALTH_TIMEOUT_MS: '100', + // Workspace configs are untrusted by default; this test uses a + // temp dir it created itself, so opt in explicitly. + ECC_MCP_ALLOW_WORKSPACE_PROBE: '1', HOME: homeDir, USERPROFILE: homeDir }, @@ -619,6 +622,7 @@ async function runTests() { CLAUDE_HOOK_EVENT_NAME: 'PreToolUse', ECC_MCP_CONFIG_PATH: configPath, ECC_MCP_HEALTH_STATE_PATH: statePath, + ECC_MCP_RECONNECT_ALLOW: '1', ECC_MCP_RECONNECT_COMMAND: `${JSON.stringify(process.execPath)} ${JSON.stringify(reconnectScript)}`, ECC_MCP_HEALTH_TIMEOUT_MS: '1000', ECC_MCP_HEALTH_BACKOFF_MS: '10' @@ -682,6 +686,7 @@ async function runTests() { CLAUDE_HOOK_EVENT_NAME: 'PostToolUseFailure', ECC_MCP_CONFIG_PATH: configPath, ECC_MCP_HEALTH_STATE_PATH: statePath, + ECC_MCP_RECONNECT_ALLOW: '1', ECC_MCP_RECONNECT_COMMAND: `node ${JSON.stringify(reconnectScript)}`, ECC_MCP_HEALTH_TIMEOUT_MS: '1000' } @@ -773,6 +778,7 @@ async function runTests() { { CLAUDE_HOOK_EVENT_NAME: 'PostToolUseFailure', ECC_MCP_HEALTH_STATE_PATH: statePath, + ECC_MCP_RECONNECT_ALLOW: '1', ECC_MCP_RECONNECT_COMMAND: `${JSON.stringify(process.execPath)} ${JSON.stringify(reconnectScript)}` } ); @@ -810,6 +816,7 @@ async function runTests() { CLAUDE_HOOK_EVENT_NAME: 'PostToolUseFailure', ECC_MCP_HEALTH_STATE_PATH: statePath, ECC_MCP_CONFIG_PATH: path.join(tempDir, 'missing.json'), + ECC_MCP_RECONNECT_ALLOW: '1', ECC_MCP_RECONNECT_COMMAND: null, ECC_MCP_RECONNECT_FOO_BAR: `${JSON.stringify(process.execPath)} ${JSON.stringify(reconnectScript)} ${JSON.stringify(markerFile)} {server}` } diff --git a/tests/scripts/codex-hooks.test.js b/tests/scripts/codex-hooks.test.js index 1171de52c..c47f6d986 100644 --- a/tests/scripts/codex-hooks.test.js +++ b/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', diff --git a/tests/test_executor.py b/tests/test_executor.py index 749c4d1b4..75f522ff7 100644 --- a/tests/test_executor.py +++ b/tests/test_executor.py @@ -1,5 +1,5 @@ -from llm.core.types import ToolCall, ToolDefinition -from llm.tools import ToolExecutor, ToolRegistry +from llm.core.types import LLMInput, LLMOutput, Message, Role, ToolCall, ToolDefinition +from llm.tools import ReActAgent, ToolExecutor, ToolRegistry class TestToolRegistry: @@ -83,3 +83,55 @@ class TestToolExecutor: assert len(results) == 2 assert results[0].content == "result1" assert results[1].content == "result2" + + +class TestAsyncTools: + def test_execute_async_awaits_coroutine(self): + import asyncio + + registry = ToolRegistry() + + async def fetch(url: str = "") -> str: + return f"fetched:{url}" + + registry.register(ToolDefinition(name="fetch", description="", parameters={}), fetch) + + executor = ToolExecutor(registry) + result = asyncio.run( + executor.execute_async(ToolCall(id="1", name="fetch", arguments={"url": "x"})) + ) + + assert result.tool_call_id == "1" + assert result.content == "fetched:x" + assert result.is_error is False + + def test_react_agent_runs_awaitable_tool(self): + import asyncio + + async def lookup(key: str = "") -> str: + return f"value:{key}" + + registry = ToolRegistry() + registry.register(ToolDefinition(name="lookup", description="", parameters={}), lookup) + + seen = {} + + class FakeProvider: + def generate(self, agent_input): + if "done" not in seen: + seen["done"] = True + return LLMOutput( + content="", + tool_calls=[ToolCall(id="1", name="lookup", arguments={"key": "k"})], + ) + tool_messages = [m for m in agent_input.messages if m.role == Role.TOOL] + assert len(tool_messages) == 1 + assert tool_messages[0].content == "value:k" + return LLMOutput(content="done") + + agent = ReActAgent(provider=FakeProvider(), executor=ToolExecutor(registry)) + output = asyncio.run( + agent.run(LLMInput(messages=[Message(role=Role.USER, content="hi")])) + ) + + assert output.content == "done"