From d22c52bc7e76001961c2661abf6ac308565fb092 Mon Sep 17 00:00:00 2001 From: andrew Date: Mon, 15 Jun 2026 21:39:01 -0700 Subject: [PATCH 1/2] Fix critical + high severity issues from code audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the Critical and 4 High-severity findings from a full code audit of the daemon. Each was independently verified against source. - B1 (Critical): RCE via issue title in the tmux pane script. Untrusted titles were interpolated into a double-quoted `echo -e` with only quote escaping, so `$(…)`/backticks executed as shell. Extract buildPaneBanner() which single-quotes the name via shellEscape (also de-dups the two script branches). - B2 (High): Execution success path discarded committed-but-unpushed work. Now salvages (push + draft PR) instead of returning null into cleanup. - B3 (High): Reflected XSS via the window.__CRITTERS__ bootstrap. Escape script-breaking chars (<, >, &, U+2028/9) in the serialized JSON. - B4 (High): `clean --panes` could kill live critters on a transient health blip. Distinguish connection-refused (down) from timeout/non-200 (maybe alive) and fail closed. - B5 (High): Transient tracker error on a terminal success transition flipped a real success into the failure path. Wrap success outcomes defensively, mirroring the already-guarded failure path. Adds regression tests for B1, B3, B4. typecheck + lint clean, 850 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/__tests__/audit-fixes.test.ts | 72 +++++++++++++++++++++++++++ src/cli-clean.ts | 82 +++++++++++++++++++++++++------ src/cli/spawn.ts | 18 ++++++- src/dashboard/main-page.ts | 12 ++++- src/runner/execution.ts | 10 +++- src/unified-spawner.ts | 29 +++++++++-- 6 files changed, 200 insertions(+), 23 deletions(-) create mode 100644 src/__tests__/audit-fixes.test.ts diff --git a/src/__tests__/audit-fixes.test.ts b/src/__tests__/audit-fixes.test.ts new file mode 100644 index 0000000..e313ae2 --- /dev/null +++ b/src/__tests__/audit-fixes.test.ts @@ -0,0 +1,72 @@ +import { describe, expect, test } from "bun:test"; +import { buildPaneBanner } from "../cli/spawn.js"; +import { isConnectionRefused } from "../cli-clean.js"; +import { renderDashboard } from "../dashboard/main-page.js"; +import type { HealthStatus } from "../health.js"; + +// B1 — Command injection via issue title in the generated tmux script. +describe("buildPaneBanner (B1: RCE via issue title)", () => { + test("single-quotes the window name so a title can't execute shell", () => { + const malicious = "ACK-1: $(touch /tmp/pwned) / plan"; + const line = buildPaneBanner("\x1b[1;36m", malicious, "\x1b[0m"); + // The untrusted name must sit inside a single-quoted segment spliced between the + // double-quoted color parts: `"━━━ "''" ━━━"`. Inside single + // quotes bash performs no command/parameter expansion, so `$(…)` stays literal. + expect(line).toContain(`"'${malicious}'"`); + // And the substitution must NOT appear unquoted inside a double-quoted run. + expect(line).not.toContain(`$(touch /tmp/pwned)" `); + }); + + test("escapes backticks the same way", () => { + const line = buildPaneBanner("L", "ACK-2: `id` now", "R"); + expect(line).toContain("'ACK-2: `id` now'"); + }); + + test("escapes embedded single quotes safely", () => { + const line = buildPaneBanner("L", "it's mine", "R"); + // shellEscape turns ' into '\'' — still no way to break out of the quoting. + expect(line).toContain("'it'\\''s mine'"); + }); +}); + +// B3 — Reflected XSS via the window.__CRITTERS__ bootstrap. +describe("renderDashboard (B3: reflected XSS in bootstrap)", () => { + const status = {} as HealthStatus; + + test("escapes script-breaking chars in the identifier", () => { + const html = renderDashboard("", status, 0, undefined, undefined, ""); + // The raw breakout must never reach the HTML verbatim... + expect(html).not.toContain(" { + const html = renderDashboard("", status, 0, "type", undefined, "ID&<>"); + const m = html.match(/window\.__CRITTERS__ = (\{.*?\});/); + expect(m).toBeTruthy(); + // JSON.parse natively decodes the \uXXXX escapes back to the real characters. + const parsed = JSON.parse(m?.[1] ?? "{}") as { typeFilter: string; identifier: string }; + expect(parsed.identifier).toBe("ID&<>"); + expect(parsed.typeFilter).toBe("type"); + }); +}); + +// B4 — `clean` must fail closed when a daemon may be alive but health didn't answer. +describe("isConnectionRefused (B4: clean fail-open guard)", () => { + test("treats genuine connection refusal as down (safe to clean)", () => { + expect(isConnectionRefused({ code: "ConnectionRefused" })).toBe(true); + expect(isConnectionRefused(new Error("connect ECONNREFUSED 127.0.0.1:7878"))).toBe(true); + expect(isConnectionRefused(new Error("Unable to connect. Is the computer able to access the url?"))).toBe(true); + }); + + test("treats timeouts/aborts as maybe-alive (unsafe — must NOT look like refusal)", () => { + expect(isConnectionRefused(new DOMException("The operation timed out.", "TimeoutError"))).toBe(false); + expect(isConnectionRefused(new Error("The operation was aborted"))).toBe(false); + }); + + test("unknown errors default to maybe-alive (fail closed)", () => { + expect(isConnectionRefused(new Error("something weird happened"))).toBe(false); + expect(isConnectionRefused(null)).toBe(false); + }); +}); diff --git a/src/cli-clean.ts b/src/cli-clean.ts index cf79d5d..d15a705 100644 --- a/src/cli-clean.ts +++ b/src/cli-clean.ts @@ -32,30 +32,77 @@ function formatSize(bytes: number): string { return `${bytes} B`; } -async function getActiveWorkDirs(healthPort: number): Promise> { - const active = new Set(); +type ActiveWorkDirsResult = { + /** true only when /healthz answered 200 and we have an authoritative active set. */ + reachable: boolean; + /** true when something is (or may be) listening — a daemon could be running. */ + daemonMaybeAlive: boolean; + workDirs: Set; +}; + +/** + * A refused connection means nothing is listening on the port → the daemon is down, + * so its panes/work dirs are genuinely orphaned and safe to clean. Any other failure + * (timeout, reset, non-2xx) means a daemon may be alive but unresponsive — unsafe. + */ +export function isConnectionRefused(err: unknown): boolean { + const e = err as { message?: string; code?: string }; + const code = String(e?.code ?? "").toLowerCase(); + if (code === "connectionrefused" || code === "econnrefused") return true; + const msg = String(e?.message ?? err).toLowerCase(); + // A timeout/abort means something may be there but is slow/wedged — NOT a refusal. + if (msg.includes("timeout") || msg.includes("timed out") || msg.includes("aborted")) return false; + return msg.includes("refused") || msg.includes("econnrefused") || + msg.includes("connectionrefused") || msg.includes("unable to connect") || + msg.includes("failed to connect"); +} + +async function getActiveWorkDirs(healthPort: number): Promise { + const workDirs = new Set(); try { const resp = await fetch(`http://localhost:${healthPort}/healthz`, { signal: AbortSignal.timeout(3000) }); if (resp.ok) { const data = await resp.json() as { activeCritterDetails?: Array<{ workDir?: string | null }> }; for (const d of data.activeCritterDetails ?? []) { - if (d.workDir) active.add(d.workDir); + if (d.workDir) workDirs.add(d.workDir); } + return { reachable: true, daemonMaybeAlive: true, workDirs }; } - } catch { - // Daemon not running or unreachable — no protection available + // Something answered but not 200 — likely the daemon with a momentarily unhealthy probe. + return { reachable: false, daemonMaybeAlive: true, workDirs }; + } catch (err) { + // Connection refused → daemon down (safe). Timeout/other → daemon may be alive (unsafe). + return { reachable: false, daemonMaybeAlive: !isConnectionRefused(err), workDirs }; } - return active; +} + +/** + * Resolve the active work dirs to protect during cleanup, or null when cleaning is + * UNSAFE: a daemon may be alive but its /healthz didn't answer cleanly, so we cannot tell + * which critters are live. Proceeding would fail open and could kill/delete live work. + */ +async function resolveActiveWorkDirs(healthPort: number): Promise | null> { + if (healthPort <= 0) return new Set(); + const health = await getActiveWorkDirs(healthPort); + if (!health.reachable && health.daemonMaybeAlive) return null; + return health.workDirs; } async function cleanStaleTmuxPanes(configPath: string | undefined, dryRun: boolean): Promise { const cleanConfig = loadCleanConfig(configPath); const tmuxSession = cleanConfig.tmuxSession; - // Get active work dirs from the daemon's health endpoint (if running) - const activeWorkDirs = cleanConfig.healthPort > 0 - ? await getActiveWorkDirs(cleanConfig.healthPort) - : new Set(); + // Determine which critters are live so we never kill an active pane. If a daemon may be + // alive but its health endpoint didn't answer cleanly, we can't tell — fail closed. + const activeWorkDirs = await resolveActiveWorkDirs(cleanConfig.healthPort); + if (activeWorkDirs === null) { + console.error( + `Refusing to clean tmux panes: the daemon health endpoint on port ${cleanConfig.healthPort} ` + + `did not respond cleanly, but something appears to be listening. A daemon may be running — ` + + `cleaning now could kill live critter panes. Stop the daemon (or fix healthPort) and retry.`, + ); + return; + } const stalePanes = await cleanupStalePanes(tmuxSession, activeWorkDirs); @@ -114,10 +161,17 @@ export async function runClean(args: string[]): Promise { let totalFreed = 0; let cleanedCount = 0; - // Query the daemon's health endpoint for active work directories - const activeWorkDirs = cleanConfig.healthPort > 0 - ? await getActiveWorkDirs(cleanConfig.healthPort) - : new Set(); + // Query the daemon's health endpoint for active work directories. If a daemon may be + // alive but health didn't answer cleanly, fail closed rather than risk deleting live work. + const activeWorkDirs = await resolveActiveWorkDirs(cleanConfig.healthPort); + if (activeWorkDirs === null) { + console.error( + `Refusing to clean work directories: the daemon health endpoint on port ${cleanConfig.healthPort} ` + + `did not respond cleanly, but something appears to be listening. A daemon may be running — ` + + `cleaning now could delete in-use work dirs. Stop the daemon (or fix healthPort) and retry.`, + ); + return; + } const dirs: { name: string; ageMs: number; size: number; stale: boolean; active: boolean }[] = []; diff --git a/src/cli/spawn.ts b/src/cli/spawn.ts index 79b2190..2623480 100644 --- a/src/cli/spawn.ts +++ b/src/cli/spawn.ts @@ -31,6 +31,20 @@ function buildPaneLabel(identifier: string, title: string, phase: string, repoSh return base; } +/** + * Build the colored banner `echo` line for a critter's tmux pane. + * + * `windowName` embeds the untrusted issue title, so it MUST be single-quoted via + * shellEscape: interpolating it into the double-quoted `echo -e "…"` directly (as + * the old `.replace(/"/g, …)` did) lets `$(…)` / backticks in a title execute as + * shell when the generated script runs — i.e. arbitrary RCE on the daemon host. + * The color codes are build-time constants and stay inside the double quotes so + * `echo -e` still interprets their escape sequences. + */ +export function buildPaneBanner(colorLabel: string, windowName: string, reset: string): string { + return `echo -e "${colorLabel}━━━ "${shellEscape(windowName)}" ━━━${reset}"`; +} + async function spawnInTmux( adapter: CliAdapter, prompt: string, @@ -99,7 +113,7 @@ async function spawnInTmux( set -o pipefail export PATH="$HOME/.bun/bin:$HOME/.local/bin:${currentPath}" ${cmd.env ? Object.entries(cmd.env).map(([k, v]) => v === undefined ? `unset ${k}` : `export ${k}=${shellEscape(v)}`).join("\n") : ""} -echo -e "${color.label}\u2501\u2501\u2501 ${windowName.replace(/"/g, '\\"')} \u2501\u2501\u2501${reset}" +${buildPaneBanner(color.label, windowName, reset)} echo "" cd ${shellEscape(workDir)} ${cmd.script} \\ @@ -121,7 +135,7 @@ sleep 5 set -o pipefail export PATH="$HOME/.bun/bin:$HOME/.local/bin:${currentPath}" ${cmd.env ? Object.entries(cmd.env).map(([k, v]) => v === undefined ? `unset ${k}` : `export ${k}=${shellEscape(v)}`).join("\n") : ""} -echo -e "${color.label}\u2501\u2501\u2501 ${windowName.replace(/"/g, '\\"')} \u2501\u2501\u2501${reset}" +${buildPaneBanner(color.label, windowName, reset)} echo "" cd ${shellEscape(workDir)} ${cmd.script} \\ diff --git a/src/dashboard/main-page.ts b/src/dashboard/main-page.ts index fa95a66..8e62f62 100644 --- a/src/dashboard/main-page.ts +++ b/src/dashboard/main-page.ts @@ -20,10 +20,20 @@ export function renderDashboard( _dashboardToken?: string, identifier?: string, ): string { + // Escape characters that could break out of the inline ", "<", ">", "&", or the JS line separators, so an + // attacker-controlled `identifier` (the raw URL path segment, reflected here) could + // otherwise terminate the script tag and inject live HTML — reflected XSS on the + // unauthenticated /dashboard/ route. Escaping to \uXXXX keeps the JSON valid. const bootstrap = JSON.stringify({ typeFilter: typeFilter ?? null, identifier: identifier ?? null, - }); + }) + .replace(//g, "\\u003e") + .replace(/&/g, "\\u0026") + .replace(/\u2028/g, "\\u2028") + .replace(/\u2029/g, "\\u2029"); const title = identifier ? `${escapeHtml(identifier)} - Critters` : typeFilter ? `Critters \u00b7 ${escapeHtml(typeFilter)}` diff --git a/src/runner/execution.ts b/src/runner/execution.ts index 27f152f..896156d 100644 --- a/src/runner/execution.ts +++ b/src/runner/execution.ts @@ -5,6 +5,7 @@ import { logTask, logTaskError, logTaskWarn } from "../logger.js"; import { buildExecutionPrompt, getExecutionAllowedTools } from "../prompt.js"; import { buildPromptVars, resolveSkills, resolveTools } from "../prompt-template.js"; import { withRetry } from "../retry.js"; +import { salvagePartialProgress } from "../task-salvage.js"; import { formatDuration, runCommand } from "../utils.js"; import { VERSION } from "../version.js"; import type { PhaseContext, PhaseResult, PhaseRunner } from "./types.js"; @@ -52,10 +53,15 @@ export class ExecutionPhaseRunner implements PhaseRunner { return { spawn, data: { prUrl: null } }; } - // Only look for a PR if the branch was pushed to the remote + // If the branch isn't on the remote, the agent committed locally but never pushed + // (ran out of turns, or `git push` failed while the CLI still exited 0). The work dir + // is the ONLY copy of those commits, and the spawner deletes it on the success path — + // so salvage the work (push + open a draft PR) instead of silently dropping it. const { stdout: remoteOut } = await runCommand("git", ["ls-remote", "--heads", "origin", branch], { cwd: workDir }); if (!remoteOut.trim()) { - return { spawn, data: { prUrl: null } }; + logTaskWarn(task.identifier, "Branch has local commits but was never pushed — salvaging to a draft PR to avoid data loss"); + const salvage = await salvagePartialProgress(workDir, branch, task.identifier, task.title, task.repoUrl, task.baseBranch); + return { spawn, data: { prUrl: salvage.prUrl ?? null } }; } // Detect PR diff --git a/src/unified-spawner.ts b/src/unified-spawner.ts index 1d6b8d4..4a9b704 100644 --- a/src/unified-spawner.ts +++ b/src/unified-spawner.ts @@ -568,7 +568,7 @@ export class UnifiedSpawner { const totalDuration = formatDuration(Date.now() - taskStart); logTask(task.identifier, `Completed in ${totalDuration}`); - await applyOutcome(critterType.outcomes.success, task, critterType, tracker); + await this.applySuccessOutcome(critterType.outcomes.success, task, critterType, tracker); // Upload report from the last phase (generic runner writes .critter-report.md) const lastPhaseData = phaseDataList.length > 0 ? phaseDataList[phaseDataList.length - 1] : null; @@ -844,6 +844,27 @@ export class UnifiedSpawner { return lastResult ?? { success: false, error: "Auto-retry did not produce a task result" }; } + /** + * Apply a terminal *success* outcome (status transition + optional label removal) + * defensively. A status-update failure on a genuinely successful task (API 5xx, + * rate limit, network blip) must never propagate: otherwise the outer catch would + * demote a real success into the FAILURE path — wrong status, a misleading "failed" + * comment, lost success comment/metrics, and (for transient errors) a wasteful full + * re-run of an already-created PR. The failure path already swallows these; mirror it. + */ + private async applySuccessOutcome( + outcome: Parameters[0], + task: TrackerTask, + critterType: CritterTypeConfig, + tracker: IssueTracker, + ): Promise { + try { + await applyOutcome(outcome, task, critterType, tracker); + } catch (err) { + logTaskError(task.identifier, `Failed to apply success status transition (continuing — task succeeded): ${formatError(err)}`); + } + } + private async handleCreateSuccess( task: TrackerTask, critterType: CritterTypeConfig, @@ -855,7 +876,7 @@ export class UnifiedSpawner { taskStart: number, tracker: IssueTracker, ): Promise { - await applyOutcome(critterType.outcomes.prCreated ?? critterType.outcomes.success, task, critterType, tracker); + await this.applySuccessOutcome(critterType.outcomes.prCreated ?? critterType.outcomes.success, task, critterType, tracker); try { await updatePrWithPlan(workDir, prUrl, task.identifier, allPhaseStats); @@ -926,7 +947,7 @@ export class UnifiedSpawner { const totalDuration = formatDuration(Date.now() - taskStart); if (decision === "merged" || data.alreadyMerged) { - await applyOutcome(critterType.outcomes.merged, task, critterType, tracker); + await this.applySuccessOutcome(critterType.outcomes.merged, task, critterType, tracker); if (data.alreadyMerged) { await tracker.comment(task.id, "PR was already merged"); logTask(task.identifier, "Review complete — PR was already merged"); @@ -974,7 +995,7 @@ export class UnifiedSpawner { } if (decision === "needs_changes") { - await applyOutcome(critterType.outcomes.needsChanges, task, critterType, tracker); + await this.applySuccessOutcome(critterType.outcomes.needsChanges, task, critterType, tracker); await tracker.comment(task.id, `Review critter (${critterType.phases[0].model}) requested changes: ${reason}`); await this.slackNotifier.notify( task.id, From 4a4ea660f60cc1b441f73df6e53bd339c474afd6 Mon Sep 17 00:00:00 2001 From: andrew Date: Tue, 16 Jun 2026 13:55:40 -0700 Subject: [PATCH 2/2] test: assert mcp config against homedir() to fix CI flakiness resolvePhaseMcpConfig expands ~ via node:os homedir(); the test compared it to process.env.HOME. Bun caches homedir() on first call and ignores later process.env.HOME mutations, so when a sibling test changes HOME they diverge on Linux CI and this test fails (works on macOS by execution-order luck). Assert against the same homedir() the implementation uses. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/__tests__/mcp.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/__tests__/mcp.test.ts b/src/__tests__/mcp.test.ts index 87ee767..02c3f7e 100644 --- a/src/__tests__/mcp.test.ts +++ b/src/__tests__/mcp.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test"; +import { homedir } from "node:os"; import { ClaudeCodeAdapter } from "../cli/claude.js"; import { CodexAdapter } from "../cli/codex.js"; import { resolvePhaseMcpConfig } from "../cli/mcp.js"; @@ -25,7 +26,7 @@ describe("resolvePhaseMcpConfig", () => { } as Config, ); - expect(result.mcpConfig).toEqual([`${process.env.HOME}/.critters/review-mcp.json`]); + expect(result.mcpConfig).toEqual([`${homedir()}/.critters/review-mcp.json`]); expect(result.strictMcpConfig).toBe(true); });